fix(sync): an ancestor for the merge, and conflicts that settle — 2.6.52 - #162
Merged
Conversation
…ts as a set Running the app on a phone found two defects no review had: every ordinary local edit raised a conflict on the next sync, and a task conflict never converged at all. The merge had no ancestor. It reported a conflict whenever the two hashes differed, so a note edited on this device alone was offered against the version it had just replaced — after every edit, with the wrong click discarding the user's work. sync_metadata now carries the canonical hash of the version this device last applied or published (schema 21 to 22), and the merge uses it: when the remote still equals that base only this side moved, so the local version is published without a dialog, and the same in reverse. Both sides moved, or no base recorded, keeps the old rule, so a fresh install and rows migrated from 2.6.51 behave exactly as before and fill the column in on their first sync. The task loop had a second cause. Without a base, a deletion merged by timestamp and published the version it replaced as an alternative, so the record ended up with two open conflicts; isSuperseded judged the second by timestamp alone and dropped it unsettled, and it returned on the next sync against the freshly re-timed version. One resolution per sync, forever. isSuperseded now compares content: a row is dropped only when the local record matches neither version it offers, which a resolution never causes. Rows whose winner is no longer the live version are retired when a record is applied, so the other device is not left holding a phantom. Also from the fourth review: accepting 2.6.50's repeated attachment id decoded to a shape this store could never rebuild, so those notes would have self-conflicted forever — the store and the decoder now agree on keeping the repeat; the backup pick could restore twice or be lost on rotation; the supersession mark carried the listing's time rather than its own; and a repeated attachment now travels the way 2.6.50 wrote it, so a mixed fleet needs no alias field. Verified on a Pixel 7a as two peers against one Drive: A deletes a task and publishes with no dialog, B edits it and gets exactly one honest conflict, B resolves once and two further syncs are clean, and A gets the task back with B's edit. Migration 21 to 22 ran on a live database with every row intact. 341 unit tests, 89 instrumentation tests, 0 failures; lint 0 errors.
Test coverage (unit + instrumentation)
Files
|
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.
Why
Running 2.6.51 on a phone found two defects that four rounds of review had all missed, because both need the app to actually be used rather than read.
Every ordinary local edit raised a conflict. Open a note on a fully synced device, add a word, save, sync — and the resolve-conflicts dialog offers your own new text against the text you just replaced. Not an edge case: it is what a person does every day, and the wrong click silently discards their work.
A task conflict never converged. Peer A deletes a task, peer B renames it, B gets a correct live-versus-tombstone conflict and keeps the live version — and from then on the same conflict returns on every sync, forever, while the resolution never reaches Drive.
What changed
The merge had no notion of an ancestor. It reported a conflict whenever the two hashes differed, which is indistinguishable from "we both edited".
sync_metadatanow carries the canonical hash of the version this device last applied or published (schema 21 → 22), and the merge reads it: while the remote still equals that base, only this side moved, so the local version is published with no dialog — and the same in reverse. Both sides moved, or no base recorded, keeps the previous rule, so a fresh install and rows migrated from 2.6.51 behave exactly as they do today and fill the column in on their first sync.The task loop had a second cause. Without a base, a deletion merged by timestamp and published the version it replaced as an alternative, leaving the record with two open conflicts.
isSupersededjudged the second row by timestamp alone and dropped it unsettled; it returned on the next sync against the freshly re-timed version. One resolution per sync, never a settled set.isSupersedednow compares content — a row is dropped only when the local record matches neither version it offers, which a resolution never causes — and rows whose winner is no longer the live version are retired when a record is applied, so the other device is not left holding a phantom.Also folded in, from the review of the previous commit: accepting 2.6.50's repeated attachment id decoded to a shape this store could never rebuild, so those notes would have self-conflicted forever; the backup pick could restore twice or be lost on rotation; the supersession mark carried the listing's time rather than its own; and a repeated attachment now travels exactly the way 2.6.50 wrote it, so a mixed fleet needs no alias field.
Verification
341 unit tests, 89 instrumentation tests, 0 failures; lint 0 errors; R8 clean.
New
TwoPeerConvergenceTest(protocol level) andTwoPeerStoreConvergenceTest(two real Room stores over one backend) run the tombstone-versus-edit scenario for task, note, tag and category, asserting a clean second sync, a clean sync on the other peer, the record revived there with the edit, and no unresolved rows left anywhere.On a Pixel 7a, driven as two peers against one real Drive: baseline of 4 notes, 2 tags and 1 task with all 8 bases populated and two clean syncs; A deletes the task and publishes with no dialog; B edits it and gets exactly one honest conflict; B resolves once and two further syncs are clean; A syncs and the task comes back carrying B's edit; both peers then stay clean. Migration 21 → 22 ran on a live database with every row intact. No exception in any run.
Known limitation
A record deleted or edited before its first sync after the migration still raises one conflict — the no-base fallback. It settles in one resolution and cannot recur once the base is recorded, but upgrading testers who edit immediately will see one dialog per such record on that first sync.