Desktop, Mobile: Automatically retrigger the sync if there are more unsynced outgoing changes when sync completes

[mrjo118]

On mobile, sync is triggered continuously as the user types. This helps to ensure that the latest changes get uploaded to the server, particularly because the Joplin mobile app does not support syncing in the background. Due to the synchronization process having multiple stages, it is possible that the continuous sync can stop prior to the latest change being uploaded, leaving unsynced the last characters the user enters when finishing editing a note. This is because the upload step of the sync suppresses multiple changes to the same file within the loop (to avoid a potential infinite loop) and also, if changes are saved during the delta step of the sync process (which runs last), the loop for the changes to upload would have ended at this point. This can result in conflicts, if the user closes or minimises the app after this happens (which is likely, because the sidebar would show that the sync is completed), and then starts using Joplin on another device.

In order to remedy this, I have addressed this in 2 ways:

• For changes made during the upload step: There is currently handling for a 'processingPathTwice' error which is thrown for 3 reasons, when an item is selected more than once for processing in the upload loop. I have now split the 3 scenarios up, so that only two of them will throw an error, while the other will just write an entry to the log
  1. The updated_time on the server object is in the future (eg. due to manual modification on the server): In this scenario it will genuinely cause an infinite loop if the time is far in the future, so leave the logic as per existing
  2. The item has force_sync set to true: In this scenario we want to leave the logic as per existing
  3. Remaining cases when sync_time was not updated: This happens when an item that was already synced in the current sync in progress is changed again by the user while syncing. In this scenario we do want to continue syncing, so I have changed it to not throw an error. It wont cause an infinite loop, but will continue syncing until the user stops typing, which is what we want to achieve by this PR
• For changes made during the delta step (only applicable when the sync was triggered manually or by a timer based on the sync interval, rather than triggered by making changes to a note on mobile): I have added a check at the very end of the synchronization process, which checks again if there are any pending outgoing changes. If this is the case, the sync will be triggered again, similar to if a change was made just after the sync has completed. This will happen recursively until there are no outgoing changes pending, at the last part of the execution of the synchronizer logic. The re-trigger of the sync will not apply if any errors in the sync occur, if the sync is cancelled by the user

Testing

Please note, the behaviour changes would be very hard to test with automated tests, due to the sync context object being reset before being returned by the start() function, and also because some of the error handling is disabled when running in an automated test (see usage of shim.isTestingEnv()). I have therefore done extensive manual testing.

• I have tested this on Android emulator using OneDrive sync, where the issue is easy to reproduce (file system sync is too quick). I also used the Extended Markdown editor settings plugin, which has a feature to show a visual sync indicator in the note editor screen. When the above scenario is produced, the editing icon is displayed indefinitely on the plugin's sync indicator after stopping typing, and this only goes away if exiting edit mode or starting typing again, which triggers the sync again. When exiting the editor when the indicator is 'stuck' on the editing icon, manually triggering the sync would show 1 update being uploaded for the change that was outstanding.
  After applying this code change, when the plugin's sync indicator is stuck on the editing icon, exiting the note editor and manually triggering the sync shows fetches only, and no updates being made. Also, in the log it contains 'Processing a path that has already been done: %s. The user is making changes while the sync is in progress' entries during the period the changes were made
• To test the re-trigger sync behaviour, on Joplin desktop I setup a profile using file system sync, and duplicated a blank note to have about 50 notes. Then I duplicated all the notes and manually triggered the sync. While the delta step is processing (visible by the create remote count incrementing), I typed some changes in a note. I observed that the sync completes after a few seconds, then automatically re-triggers and uploads the latest change. In the log I can find a 'There are more outgoing changes to sync, trigger the sync again' entry during the sync period. I then verified when manually triggering the sync again, the sync shows fetches or creates (for revisions) only, and no updates being made
• To test the force sync scenario, similar to the previous scenario I setup about 90 notes and filesystem sync, then I toggled encryption on, triggered the sync, and toggled it back of while it was syncing. I observed a 'Processing a path that has already been done: %s. Item was marked for sync using force_sync' entry in the log during the sync period
• On desktop and mobile, I have verified that manually syncing (and when the automatic sync triggers) on the note list screen will come to an end after only one sync completes (so it does not get stuck in an infinite loop), when nothing is being changed
• I've verified that in the event of an error in the synchronizer (eg. failsafe) or a manual cancellation, that this will cause the sync to stop
• I've verified that if changing the updated_time on a server object to a time in the future, that the sync will does not continue, when the 'Processing a path that has already been done: %s. Remote item has an updated_time in the future' entry is written to the console

Video of the behaviour in the current app:

Original.app.mp4

Video of the behaviour with the code changes (this was recorded before the logging was added, so the logging results shown are not up to date):

New.change.mp4

[laurent22]

We don't set the return value normally, it should be implied

[mrjo118]

That is needed to get around a compile error, due to the function now invoking itself and returning the value

[laurent22]

Oh that's right, it's needed in this case. Thanks for clarifying