Do not sync invalid user-config

[Bad3r]

Issue

Errors in config.edn break graph sync. The only possible workaround is unlinking the graph and waiting ~24 hours for it to be pruned before re-syncing. Another workaround is renaming the graph.

Steps to reproduce

1. break config.edn by adding this incorrect setting (related to #8419):

   :journal/page-title-format "EEE,FFF do MMM yyyy"

2. Logseq UI crashes && Logseq syncs the incorrect config.edn at the same time
3. update config.edn outside Logseq to fix the error by removing the line.
4. you open the log set
5. Logseq attempts to download config.edn from the server which is the broken version
6. Logseq crashes again
7. repeat steps 3-7

This is after the new config validation change. The old method of handling broken config.edn used to be to rename config.edn to bad_config.edn and create a new fresh config.edn

https://user-images.githubusercontent.com/25513724/218212212-8c8d5ed2-1d9f-43f7-83e0-92dd2e3132ce.mp4

Proposed Solution

@RCmerci: I think we need a special rule for config.edn when syncing. a. validate config.edn content and b. only sync if it's valid

CC: @logseq-cldwalker

[logseq-cldwalker]

@Bad3r Could you explain how the user experience is different than #8419 ? Your initial repro steps are the same. I'm not able to repro a "UI crashes". I'm also not seeing how this "breaks graph sync". If you synced an invalid config.edn, you should be able to sync back a valid config.edn, at worst after reloading your app. If you are indeed experiencing crashes in both, please share the details that our bug reports ask for

As an aside, please don't use github assignees as engineers use these to communicate when they are actively working on or to whom to assign an issue

[Bad3r]

Hey @logseq-cldwalker, this was a follow-up to a conversation I was having with @RCmerci in DMs, and I wanted to have a place to track it and document it before I forgot.

Please see the video to understand what I mean; The config continues to be overwritten by Logseq Sync:

https://user-images.githubusercontent.com/25513724/218212212-8c8d5ed2-1d9f-43f7-83e0-92dd2e3132ce.mp4

Also note this affects mobile when the user opens the logseq app; it will crash with the same error.

The only workaround I found so far is renaming the folder/graph and re-adding it.

[Bad3r]

@Bad3r Could you explain how the user experience is different than #8419? Your initial repro steps are the same.

This issue focuses on how Logseq Sync should handle broken user-config, not specific to a particular user-config option. #8419 is for one specific issue in user-config that could cause Logseq to misbehave (crash).

The question here is: should Logseq Sync have a special way to handle config.edn?, i.e., if a config doesn't pass validation, should it be synced?

[Bad3r]

As an aside, please don't use GitHub assignees, as engineers use these to communicate when they are actively working on or to whom to assign an issue

It was assigned to @RCmerci due to the understanding we reached in DMs. He mentioned that he would take a look at it tomorrow/Monday. And I told him that I'd create a tracking ticket.

I pinged you because I thought you might be curious about this issue since it relates to user-config. I have updated the original post with more precise steps.

[logseq-cldwalker]

@Bad3r Thanks for updating the issue with more detailed steps. I am able to repro this issue but this doesn't have anything to do with sync or recent config changes. This invalid config option was already causing a crash weeks ago as you reported in https://github.com/logseq/logseq/issues/8418. For that specific config change, let's keep the convo in that issue. For sync, maybe this is an interesting problem to think about it but it's not specific to config. Will defer to @RCmerci if it's interesting to solve this generically for any crash-inducing file

It was assigned to @RCmerci due to the understanding we reached in DMs.

Thanks for getting his permission when you assigned it to him. Would be good to keep with this process as Logseq is a distributed, async team and we rely on these indicators to know who's looking at what

[RCmerci]

Hi @Bad3r @logseq-cldwalker I'm currently refactoring copy/export related codes. Will definitely look into this issue in detail.

[Bad3r]

this doesn't have anything to do with sync or recent config changes. This invalid config option was already causing a crash weeks ago

I thought it was related, since previously an invalid config would have moved the config file to bad_config.edn and created a new config.edn file to replace it. In that case, the consistent overwriting issue wouldn't occur and Logseq wouldn't get stuck in a crash-loop.

but yes, you are correct, this is a generic issue. I just can't think of other test cases to cause the same behaviour. Others might come up in the future, though.

Edit: I got more context from the PR on how it works.