fix(sync): four review rounds of data-loss repairs, shipped as 2.6.51 - #161
Merged
Conversation
A verified review of the 2.6.50 sync code found ten correctness defects and thirty-one more beyond the ranking cap. The serious ones were races that a single device cannot show: - A note carrying attachments hashed differently on every device because its editor blocks were rewritten to device-local paths before hashing, so it conflicted with itself on every sync. Blocks now travel in a device-independent wire form and are mapped back on arrival. - A conflict was stored even for a record whose apply had just been skipped as stale, so resolving it later overwrote an edit the user made during the sync with a version they were never shown. - Older unresolved conflicts for the same record were never retired, so a stale pre-selected winner could be applied over the current edit. - Settings changed during a long sync were committed over and the hash rewritten to hide it. - A task, category or tag deleted here and edited elsewhere never came back: the update DAOs are no-ops on a missing row. - Restoring a ZIP extracted straight into live note folders before the JSON was validated; it now stages and adopts per note. - Applying a restored theme recreated BackupActivity mid-restore and disposed the in-flight inserts. - Reading history retained every bundle's bytes and validated ancestry recursively; superseded bundles are now pruned after a grace period. The rest: five digest implementations became one, three Drive failure classifiers became one, folder listings and attachment hashes are cached per sync, restore batches that repeat an id keep both rows, and same-named tags created on two devices reconcile deterministically. 308 unit tests (was 248) and 74 instrumentation tests (was 60), 0 failures; lint 0 errors; R8 clean.
A second verified review of the round-one fixes found that several of them had traded one defect for another: - Changing the wire form of block URLs changed the canonical hash of every note with attachments already on Drive from 2.6.50, so an unchanged note conflicted with itself after upgrade, in about half the cases on every sync. Decoding now recognises a payload in the 2.6.50 shape by proving its attachment ids are the old derivation, and rewrites it to the current shape, so it hashes identically to what the upgraded store builds. A hand-built 2.6.50 payload merges with no conflict; a receiver-shaped one conflicts exactly once and then syncs clean. - Staged ZIP attachments were adopted only for notes that survived the already-present filter, so a note whose row existed but whose files were gone never got them back. Adoption now runs for every note. - The relocator asked the mover twice for the same URL and got two different copies, so the attachments column and the editor blocks named different files and the cleaner deleted one. One answer per URL. - The settings guard ran before the apply transaction; an edit made inside it was still committed over. The compare now happens at commit time and the commit is refused when the live values moved. - The sync path still applied the theme from inside applySnapshot, recreating BackupActivity mid-sync. The theme is applied after the sync finishes. - A single corrupt copy of a blob in one root was returned unread when it was the only candidate there, shadowing a good copy in another root. - Reviving a tombstoned tag bypassed same-name reconciliation. - Pruning deleted a bundle without a publish timestamp immediately and measured grace by another device's clock; it now uses Drive's own createdTime and never deletes a bundle whose age Drive does not report. - A raw NUL character sat inside a string literal as a memo separator. 321 unit tests, 80 instrumentation tests, 0 failures; lint 0 errors.
…r own backups Third review of the fix rounds, three findings plus one from the device. Aliasing a colliding attachment id had replaced an encode-time refusal with silent loss: when one note carried two manifest entries under the same logical id with different content, both were re-keyed to the second entry's alias and every receiver wrote the second blob's bytes for both references. Wire ids are now recorded per manifest position, so both blobs travel; a note that references one manifest entry twice is refused by the validator, which is the safe failure the old code had; and the store derives a fresh id rather than emitting a repeated one. Bundle pruning measured the grace from when a bundle was created, not from when it was superseded, so one created days ago and superseded seconds ago was deleted while another device was mid-read. Bundles outside the frontier are now marked on Drive when a read finds them, and only a bundle already marked when this sync read it, whose mark is older than the grace, is deleted. A blob whose Drive checksum matched but whose listing size was missing was treated as corrupt instead of being read and verified, turning a good file into a permanent sync failure and a duplicate upload every sync. Restoring a backup was refused when the name did not end in .mnbkn, and Android's SAF appends " (2)" to a second backup saved in the same folder, so the app rejected its own file. The name is now a fast accept and the decision falls back to opening the document and looking for the backup JSON inside the archive. 325 unit tests, 80 instrumentation tests, 0 failures; lint 0 errors.
A narrow review of the previous commit found one release blocker. The within-note rule added there refused any note that referenced one manifest entry twice, but 2.6.50 — which is in closed testing now — legitimately publishes that shape when a note's column repeats a canonical id with the same content; its encoder only refused differing content. Because a read decodes every bundle in every root, one such bundle would have failed the whole read forever, and no read means no publish, so it could never be superseded or pruned either. The two shapes are now told apart. A repeated plain manifest id is one entry and one blob, so it is accepted and both references are kept. A repeated re-keyed id is the shape in which two attachments were collapsed into one reference; no released client produced it and it is still refused. Encode now gives each repeat its own wire id, so neither shape is emitted, including when a conflict alternative is republished. The prune grace compared Drive's timestamp against the phone's clock, so a device running fast collapsed it to zero. Both ends are Drive's clock now, taken from the Date header of the listing, which also drops the assumption that Drive bumps modifiedTime for an appProperties-only update. The backup content check ran on the main thread and inflated a wrongly picked archive to its end; it now runs on the background executor and gives up after 8 MiB. Ships as 2.6.51. The changelog keeps one entry for everything since 2.6.46, the last release users received: 2.6.50 reached closed testing only, so the difference between it and this build is repairs to code no one outside testing ever ran. 330 unit tests, 80 instrumentation tests, 0 failures; lint 0 errors.
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
1 participant
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.
What this is
2.6.50 went to closed testing, and a verified review of it found ten correctness defects — several of them silent data loss in races a single device cannot show. This is four rounds of repairs to that, reviewed after each round, and it ships as 2.6.51.
The changelog keeps one entry covering everything since 2.6.46, the last release users actually received. 2.6.50 reached closed testing only, so the difference between it and this build is repairs to code no one outside testing ever ran.
The rounds
Round one closed the ten ranked defects and thirty-one more beyond the cap: a note with attachments hashed differently on every device and so conflicted with itself on every sync; a conflict was stored even for a record whose apply had been skipped as stale, so resolving it later overwrote an edit the user made during the sync; older unresolved conflicts were never retired; settings changed during a long sync were committed over and the hash rewritten to hide it; a task, category or tag deleted here and edited elsewhere never came back; restoring a ZIP extracted into live note folders before the JSON was validated; applying a restored theme recreated BackupActivity mid-restore and disposed the in-flight inserts; reading history retained every bundle's bytes and validated ancestry recursively.
Round two repaired what round one broke. The new wire form changed the canonical hash of every note already on Drive, so an unchanged note conflicted with itself after upgrade — in about half the cases on every sync. Decoding now recognises a 2.6.50-shaped payload by proving its attachment ids are the old derivation and rewrites it, so it hashes identically to what the upgraded store builds. Staged ZIP attachments were adopted only for notes that survived the already-present filter; the relocator made two copies for one URL so the column and the blocks named different files; the settings guard ran before the transaction rather than at commit; the sync path still recreated the screen mid-sync.
Round three kept both blobs when one note repeats an attachment id with different content (aliasing had silently dropped the first), timed bundle pruning from supersession rather than creation, stopped treating a blob whose listing size is missing as corrupt, and made restore accept its own backup when Android's SAF appends " (2)" to the name.
Round four fixed a release blocker: the within-note rule added in round three refused a shape 2.6.50 legitimately publishes, and because a read decodes every bundle in every root, one such bundle would have failed the whole read forever — and no read means no publish, so it could never be superseded or pruned either. A repeated plain manifest id is now accepted; a repeated re-keyed id is still refused. The prune grace also moved fully onto Drive's clock, and the backup content check moved off the main thread.
Verification
330 unit tests and 80 instrumentation tests, 0 failures; lint 0 errors; R8 clean.
On a Pixel 7a, across several clean installs: the 2.6.50 upgrade path gives exactly one conflict and then syncs clean over four syncs; a fresh install pulling mixed 2.6.50 and 2.6.51 bundles from Drive raises no note conflict at all and stays clean over three syncs; deleting an attachment file and restoring a ZIP brings it back for a note the already-present filter drops; the attachments column and the editor blocks agree on one file; a backup named
My_Notes_Backup.mnbkn (2)restores. No exception in any run.Known limitations
Cross-device behaviour — a task deleted here and edited elsewhere, a theme arriving from another device, tag revival, prune grace across devices with skewed clocks — is covered by tests but was never exercised on two physical devices.