Desktop, Mobile, Cli: Implement automatic recovery where the basic delta deletes local items due to a temporary server blip - #16177
Open
mrjo118 wants to merge 3 commits into
Conversation
…s due to a temporary server blip
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As per https://discourse.joplinapp.org/t/sync-randomly-deleted-all-notes-on-just-one-device/50593, it was found that it is possible for sync target to have temporarily blips, where the API to list files in a directory / bucket does not list all files which are present (visible from the count of items reported by the basic delta in the logs). In this case it happened using a Cloudflare R2 S3 bucket compatible storage.
This is problematic because the absence of a file in the directory listing is what is used to determine when a file should be deleted, for sync targets which use the basic delta (e.g. WebDAV and S3 buckets). However, when a local item is deleted in this way, there are no deleted_items entries created (as ItemClass.delete uses trackDeleted false) and the sync_items entry is removed (via BaseItem.deleteOrphanSyncItems), so Joplin no longer holds a reference to these items internally. The reason why the data is not re-downloaded when the stat endpoint recovers is simply because Joplin stores a context timestamp to indicate the earliest timestamp which should be considered for downloading files on the server.
This PR addresses this issue by allowing Joplin to recover from this situation, by automatically re-downloading the locally deleted data when it becomes available again in the stat listing. Note that the scenario reported by the OP also created conflicts for a considerable amount of notes, but if automatic conflict resolution is enabled (if the feature is merged), this should hopefully prevent any conflicts being created either (or if the conflicts are equivalent to the original notes, maybe they can just be ignored).
Note that it is also possible for items which were deleted within Joplin to be restored (usually immediately, but depending on whether the delete_items entry was processed yet), if the server item was manually backed up and then restored after the remote deletion was synced. Because deleted_items records are deleted when processed for the current target, but not for the other targets, if the sync target was changed in future, those items would then get deleted at that point. This however is very edge cases (would require the user to manually manipulate the sync target) and matches the existing behaviour of the enhanced basic delta algorithm used by file system sync.
Testing
See video demonstrating manually removing a file from the sync target, syncing, then adding it back again and syncing, to demonstrate a file locally deleted by the sync being recovered, and also demonstrating the sync being stable, with no changes being made locally when the server does not change. Note that in order to test this, I hard-coded the enableEnhancedBasicDeltaAlgorithm function to return false, as file system sync used for testing would always use the enhanced basic delta algorithm, which already supports automatic recovery in this scenario. I also verified the change works as expected with E2EE enabled.
electron_BGrSs2KI4Z.mp4
Additionally I tested that when and item is removed and restored from the sync target multiple times, the item is restored in the profile repeatedly. And if the local version has a change when the remote item is remote, a conflict is created, but the original item is still restored when it is restored on the server. Additionally I tried removing a large proportion of items, syncing, then restoring them, and as a result the items were restored. The same applied when restoring the files mid sync, while it was applying the local deletions.