Re-enable NoteDoctor to sync notes on startup

[Usiel]

NoteDoctor scans for local/ghost discrepancies on idle and queues sync attempts. It was disabled during the 2020 rewrite and never turned back on. Looks to me like it was just forgotten in the already very complex rewrite, but please correct me if I'm wrong @dmsnell. The doctor would be quite useful in combination with #3387.

Fix

Uncommented code and added some automated tests.

Release

Other changes:

• Re-enable NoteDoctor to sync notes on startup

[dmsnell]

My memory is weak on this, but my guess is that we turned off the NoteDoctor and left it in just in case the need arose again. After some of the rewrites, the fundamental data-flow issues were resolved and so the problem the doctor solved no longer existed.

Since it adds new concurrency work I would be hesitant to re-enable it without a clear justification so that we can avoid any potential backfiring.

What led you to believe it was necessary again?

[Usiel]

Thanks for having a look at these PRs!

What led you to believe it was necessary again?

Re-enable NoteDoctor to sync notes on startup

Sorry, I forgot to mention this in the PR body 😅 In detail: When a user has a sync that failed to sync (e.g. 413) and starts up Simplenote, the note won't try to sync again without the NoteDoctor.

This became more obvious with #3387. If you're not convinced of NoteDoctor anymore, I'd suggest just removing the code completely, and we can handle this case in #3387 in a more defensive manner (i.e. no resync, just persist the error state properly across app instances).

[dmsnell]

When a user has a sync that failed to sync (e.g. 413) and starts up Simplenote, the note won't try to sync again without the NoteDoctor.

this seems problematic. why does it not try again? it should have a local copy of the note that doesn’t match what’s on the server. is there another bug this is hiding?

[Usiel]

this seems problematic. why does it not try again? it should have a local copy of the note that doesn’t match what’s on the server. is there another bug this is hiding?

Yep, it does have the local copy, and the NoteDoctor fills the gap by checking the ghost state vs the current local state. So not necessarily a bug, there just isn't any specific code to handle this case (besides the NoteDoctor) from what I see.

I had another look into NoteDoctor, reduced its scope further and further (only run once at startup instead of continuously, only resync notes that have no ghost) until it became safe enough for me... but also pretty useless :) I'm not comfortable to introduce this change anymore and will go with the defensive changes directly on #3387.

[...] we can handle this case in #3387 in a more defensive manner (i.e. no resync, just persist the error state properly across app instances).
(#3388 (comment))