Background tabs do an extra upload after password change #1351
Labels
Comments
github-merge-queue bot
pushed a commit
that referenced
this issue
Dec 13, 2023
This is part of addressing #1342. This is the app-framework part of adding merge to E2EE sync. The merge algorithm itself is stubbed-out for separate implementation. There are two major cases covered here: background sync and password change. The behavior is summarized below. This conflict-resolution model is based on a pattern I used at Dropbox, so I hope it's clear to people who haven't seen it before. ### Background sync behavior - Pre-existing logic lets the server revision to be used to detect whether this client has seen the latest state on the server. - Pre-existing logic lets the storage hash be used to detect whether this client's state has changed from the most recent server state. - New upload behavior: Send the previous known revision to the server, which will abort rather than clobbering a new revision. - Client responds by triggering an extra download in order to merge, after which another upload will be attempted. - New download behavior: When a new revision is downloaded, recalculate the hash of data from AppState, and compare it to the hash of the **previous** server revision to see if this client has made any changes. - If so, trigger a merge and keep the merged state. - The download can trigger a merge on its own if it's the first point that a conflict is discovered. The case of upload aborting and triggering download is simply the common case in a quiescent system, because uploads are attempted eagerly after changes. ### Password change behavior - Password change (changeBlobKey) behaves like an upload, because it uploads the latest data using the new encryption key. - The known revision is added to the changeBlobKey request to detect conflicts and abort, same as an upload. - This requires some new react hooks to fetch this info from AppState. - When an abort happens, the user sees a message asking them to wait for sync to complete. - An extra download is triggered to help this happen. - Making that possible required adding `useSyncE2EEStorage()` to some more - Also added found #1351 in the process of testing, and added tweak to the behavior of background tab behavior to trigger an extra download and merge. ### Testing I tested the background sync and password change cases manually against my dev server. I don't think there's a way to write automated tests for them. I didn't test the non-standard password change cases (where a password is set for an existing account) because I don't know how to set up those cases. I'm hopeful that the relevant logic is all identical and will just work. Maybe our QA can set up those cases to be sure. ### Notes If this PR is pushed without a real merge algorithm, it ends up always preferring the remote copy, such that whichever change hits the server first wins. That's quite close to the current behavior (given the eager nature of uploads), though not identical because the current behavior doesn't ensure a consistent decision.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
During testing of password change I observed spurious increments of my storage revision in local storage not explained by the log of the tab I was testing. Turned out they were coming from an idle background tab (pond-control) with the scenario below. I don't have an immediate idea to fix this cleanly, but will instead focus on #1342 to reduce the risk of data loss. This issue is to capture my understanding of the problem for later.
Scenario:
selfandencryptionKeyfrom local storage into appState.self), finds it has changed, so it uploads.Note that being offscreen suppresses the periodic triggers for download + feeds, but doesn't suppress sync in general. When app state changes, and sync thinks an upload is needed, it'll do so in the background.
This extra upload uses the correct new key, so it's not blatantly broken. However it currently has a good chance of clobbering recent changes, since the background tabs in-memory state is likely to be older than the active tab's. Once merging is implemented (see #1342) it'll trigger and reduce this risk like others.
Fixing this in general is part of the broad class of issues of making sync coordinate through local storage between multiple tabs. Right now, multiple tabs interact primarily via the server (with special-case exceptions like password change). Fixing that general issue is a big work item for the future.
More targeted tweaks for this behavior are possible, but questionable:
The text was updated successfully, but these errors were encountered: