Desktop, Mobile: Harden failsafe logic to check for the presence of info.json, rather than just the item count #11750
Conversation
…than just the item count
|
@personalizedrefrigerator all comments now addressed |
|
This looks good to me! Thank you for working on this! |
| // This scenario is possible with OneDrive sync, see https://github.com/laurent22/joplin/issues/11489 | ||
| // This check while the sync is running is only necessary for the delta step of the sync, as this is where local deletions are calculated by comparing the local database and the sync target. These deletions are driven by the listResult field to determine which remote items exist | ||
| // As long as we check that info.json still exists after each time the listResult field is repopulated, there should not be a risk of unwanted deletions when failsafe is enabled, unless the target directory is directly manipulated by the user | ||
| await checkSyncTargetIsValid(this.api()); |
There was a problem hiding this comment.
@laurent22 raised the possibility of performance impacts.
Since this called once per delta, do you think that this could have a significant performance impact on slow sync targets? (Would it make sense to run this check only for certain sync targets)?
There was a problem hiding this comment.
I don't think the performance impact should be significant because it only runs once per 50 items (I have also tested and confirmed that is the case). We already make a call to get the full file contents for each of the 50 items in each batch using a download queue, and then wait for the download to complete in order to perform delta logic, if the target does not support accurate timestamps. So I think 1 additional call per 50 is worth a small compromise for people with large collections and a slow target, in favour of increased safety
There was a problem hiding this comment.
Ok that makes sense, thank you for the detailed explanation. Thanks a lot for adding the automated tests too, that's very useful to validate this kind of special feature.
In the past 3-4 months there have been a number of issues raised on the support forum where Joplin users who use OneDrive sync have claimed that their notes 'suddenly disappeared'. While it is not possible to determine if all cases were caused by the same issue, there is evidence through user feedback from @BeefStorm on this thread Restore Deleted Backups - #18 by BeefStorm that this issue can occur without any user interaction, via a re-authentication of OneDrive happening in the background, which may sometimes cause the sync target (special) folder on OneDrive to change and somehow bypass the failsafe mechanism when that occurs.
I proposed and have now implemented a method for how the failsafe could be hardened to be more protective from data loss on all sync targets. In summary:
During the delta step of the synchronization code, make it check whether the sync target (remote) contains an info.json file, following each invocation of the delta api which fetches the list of remote items, and also when synchronization is started (just before info.json is created). If it does not, then trigger a failsafe error specifying that the sync target is empty or damaged. To avoid hitting this error when doing the initial sync on a device, bypass this error if the local database contains no items in the sync_items table matching the current sync target (then when it starts syncing it will create the info.json so that subsequent syncs should work without issue).
I have tested the following scenarios on Joplin desktop, using a profile containing at least a couple of notes:
-Test setting up a new sync target with file system sync, when sync has never been set up before: The sync completes without issues, regardless of whether failsafe is enabled
-Test setting up a new sync target with onedrive sync, when sync has never been set up before: The sync completes without issues, regardless of whether failsafe is enabled
-Test setting up a new sync target with onedrive sync, when sync has never been set up before for onedrive, but has for file system sync: The sync completes without issues, regardless of whether failsafe is enabled
-Test setting up file system sync and syncing fully, then setting up onedrive sync and syncing fully, then switching back to file system sync on the same directly but with the .sync folder and info.json deleted: This should trigger the error if failsafe is enabled, or no error if it is not
-Test .sync folder and info.json being deleted for file system sync for an existing target, then trigger the sync: This should trigger the error if failsafe is enabled, or no error if it is not
-Test failsafe check during the delta step: Make 60 dummy notes, duplicate them and click sync. As the sync status starts notifying of items created, delete the .sync folder and info.json before it completes. This should trigger a failsafe error if failsafe is enabled, or complete the sync if it is not
-Test upgrading the sync target by deleting info.json but not .sync/version.txt: This should work correctly, prompting the user to upgrade via a banner, and upong restarting via this banner the sync should be restored and the info.json file gets recreated. Note that on the desktop, the upgrade banner is dismissed if switching note after it shows, and requires restart of Joplin for it to come back, after which the message is persistent. Note that this is existing behaviour
-Test 'Delete local data and re-download from sync target' option: It should work correctly without errors and sync continues to work afterwards (tested with OneDrive)
-Test 'Re-upload local data to sync target' option: It should work correctly without errors and sync continues to work afterwards (tested with OneDrive)
-Test 'Delete local data and re-download from sync target' option with the .sync folder and info.json deleted: It should work correctly without errors and sync continues to work afterwards (tested with file system sync). Also when deleting just info.json, it triggers the sync upgrade banner, and upon restarting via the banner the sync completes successfully
-Test 'Re-upload local data to sync target' option with the .sync folder and info.json deleted: It should work correctly without errors and sync continues to work afterwards (tested with file system sync). Also when deleting just info.json, it triggers the sync upgrade banner, and upon restarting via the banner the sync completes successfully