From 35ce6a411940aa50d1fc1ba3b558f39033db3a3b Mon Sep 17 00:00:00 2001 From: pasichDev Date: Sat, 5 Sep 2026 16:33:51 +0300 Subject: [PATCH] fix(sync): stop a phantom conflict deleting a record everywhere MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first review of the previous release found that its two changes, each right on its own, combined into silent loss. Conflict rows replicate: a device that has never held a record can receive one. Retiring a stale row ran only when a tombstone or a live update was applied, never on the insert path a fresh or third device takes, and the new content rule kept a row alive while the local record matched either of its versions. So after another device revived a record by keeping the alternative, the newcomer still held a row whose pre-selected winner was the deletion. One tap and the record was gone from every device, with no conflict raised anywhere. Retiring now runs on every path a record arrives by, and a row survives only while the record still equals its winner: a record that matches the alternative was switched to it by a resolution somewhere, so the winner is a version it has left behind. An unsettled alternative comes back once, against the current version, rather than as a deletion. The preferences base was written inside the apply transaction, before the commit that stores the settings had succeeded. A refused or thrown commit left the base naming a version that was never applied, and the next build then published the local settings with no conflict, silently discarding the other device's change. It is written after the commit returns true. Taking the remote whenever the local version equals the base ignored direction, so a missing head bundle made a device holding the newer version revert, and every unedited device followed. An older remote now falls through to last-writer-wins, which keeps the newer copy and republishes it. Verified on a Pixel 7a as three peers against one real Drive: B edits a task, C deletes it, B stores the conflict, a fresh peer A receives that row while holding no task at all, B keeps the alternative and republishes — and A's next sync retires the phantom and ends up with the task alive. Both peers stay clean afterwards. 343 unit tests, 92 instrumentation tests, 0 failures; lint 0 errors. --- CHANGELOG.md | 2 +- app/build.gradle | 2 +- .../pasich/mynotes/db/RoomSyncStoreTest.java | 77 +++++++++++++++++++ .../db/TwoPeerStoreConvergenceTest.java | 60 +++++++++++++++ .../mynotes/data/sync/RoomSyncStore.java | 57 ++++++++++---- .../pasich/mynotes/data/sync/SyncMerger.java | 9 ++- .../mynotes/data/sync/SyncMergerTest.java | 15 ++++ .../data/sync/TwoPeerConvergenceTest.java | 52 ++++++++++++- 8 files changed, 254 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe4b6ce..cebf849 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # CHANGELOG -## [2.6.52] - 05.09.2026 +## [2.6.53] - 05.09.2026 **New** diff --git a/app/build.gradle b/app/build.gradle index feea544..f12a310 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -21,7 +21,7 @@ apply from: "$projectDir/gradle/libs-task.gradle" apply from: "$projectDir/gradle/changelog-task.gradle" -def appVersionCode = 52 +def appVersionCode = 53 def appVersionName = "2.6.${appVersionCode}" def gitCommitHashProvider = providers.exec { diff --git a/app/src/androidTest/java/com/pasich/mynotes/db/RoomSyncStoreTest.java b/app/src/androidTest/java/com/pasich/mynotes/db/RoomSyncStoreTest.java index 89cac52..fe8ee88 100644 --- a/app/src/androidTest/java/com/pasich/mynotes/db/RoomSyncStoreTest.java +++ b/app/src/androidTest/java/com/pasich/mynotes/db/RoomSyncStoreTest.java @@ -789,6 +789,83 @@ public void resolveConflict_dropsAConflictTheRecordHasMovedPastInsteadOfApplying .isEqualTo(5_000L); } + @Test + public void resolveConflict_dropsARowWhoseWinnerTheRecordHasAlreadyLeftBehind() + throws Exception { + // The record now equals the row's alternative — another device kept it — so the + // pre-selected winner is a version the record has moved past. Offered anyway, it was + // applied on a tap; here the winner is a deletion. + int noteId = seedNote("loser", "body", null); + db.syncMetadataDao().touch(SyncMetadata.RECORD_TYPE_NOTE, noteId, 5_000L); + com.pasich.mynotes.data.database.entities.SyncConflictEntity row = + noteConflictRow( + "11111111-1111-4111-8111-111111111111", "deleted-winner", 3_000L, 2_000L); + row.winnerJson = + "{\"type\":\"note\",\"id\":\"11111111-1111-4111-8111-111111111111\"," + + "\"updatedAt\":\"1970-01-01T00:00:03Z\",\"deletedAt\":\"1970-01-01T00:00:03Z\"," + + "\"payload\":{}}"; + row.winnerTombstone = true; + db.syncConflictDao().insertIgnoringDuplicates(Collections.singletonList(row)); + long conflictId = db.syncConflictDao().getAll().get(0).id; + + store.resolveConflict(conflictId, SyncResolution.KEEP_WINNER); + + assertThat(db.noteDao().getNoteSync(noteId)).isNotNull(); + assertThat(db.noteDao().getNoteSync(noteId).getTitle()).isEqualTo("loser"); + assertThat(db.syncConflictDao().getById(conflictId)).isNull(); + } + + @Test + public void applySnapshot_doesNotRecordThePreferencesBaseUntilTheCommitSucceeded() + throws Exception { + // Recorded inside the transaction, a base for a version that was then never committed + // made the next build publish the local settings over the other device's change with + // no conflict. + PreferencesAdapter adapter = new PreferencesAdapter(); + RoomSyncStore preferencesStore = new RoomSyncStore(context, db, adapter.helper); + preferencesStore.readState(); + // A build first, as every sync does: it records the live digest as the baseline, so the + // apply below sees unchanged settings rather than a leftover baseline from another test. + preferencesStore.buildSnapshot(); + adapter.succeeds.set(false); + db.syncMetadataDao().setVersion(SyncMetadata.RECORD_TYPE_PREFERENCES, 0, 1_000L, null); + SyncRecord remote = + SyncRecord.live( + SyncRecord.Type.PREFERENCES, + "00000000-0000-4000-8000-000000000000", + java.time.Instant.ofEpochMilli(2_000L), + new com.google.gson.Gson() + .toJsonTree(preferencesWithTheme(3)) + .getAsJsonObject()); + + try { + preferencesStore.applySnapshot( + new SyncSnapshot(Collections.singletonList(remote)), Collections.emptyList()); + throw new AssertionError("Expected the failed commit to propagate"); + } catch (IOException expected) { + // The journal stays for recovery; the base must not claim the version landed. + } + + assertThat( + db.syncMetadataDao() + .getByStableId( + SyncMetadata.RECORD_TYPE_PREFERENCES, + "00000000-0000-4000-8000-000000000000") + .syncedVersionId) + .isNull(); + + adapter.succeeds.set(true); + preferencesStore.applySnapshot( + new SyncSnapshot(Collections.singletonList(remote)), Collections.emptyList()); + assertThat( + db.syncMetadataDao() + .getByStableId( + SyncMetadata.RECORD_TYPE_PREFERENCES, + "00000000-0000-4000-8000-000000000000") + .syncedVersionId) + .isEqualTo(remote.getCanonicalPayloadHash()); + } + /** A stored note conflict whose winner is titled after its version id. */ private com.pasich.mynotes.data.database.entities.SyncConflictEntity noteConflictRow( String stableId, String winnerVersionId, long winnerUpdatedAt, long loserUpdatedAt) { diff --git a/app/src/androidTest/java/com/pasich/mynotes/db/TwoPeerStoreConvergenceTest.java b/app/src/androidTest/java/com/pasich/mynotes/db/TwoPeerStoreConvergenceTest.java index 8000910..0e4e571 100644 --- a/app/src/androidTest/java/com/pasich/mynotes/db/TwoPeerStoreConvergenceTest.java +++ b/app/src/androidTest/java/com/pasich/mynotes/db/TwoPeerStoreConvergenceTest.java @@ -61,6 +61,66 @@ public void setUp() { public void tearDown() { a.db.close(); b.db.close(); + if (c != null) c.db.close(); + } + + private Peer c; + + @Test + public void aRecordRevivedElsewhereLeavesNoPhantomDeletionOnAPeerThatNeverHeldIt() + throws Exception { + // B edits, C deletes, B keeps the edit. A — a fresh install that never held the record — + // received the deletion-versus-edit conflict along the way. Once the edit arrives on A the + // row is meaningless: offered anyway, pre-selected on the deletion, one tap deleted the + // record on every device with no conflict raised anywhere. + RecordKind kind = new TaskRecord(); + c = new Peer(); + kind.create(b, "TaskX"); + clock.advance(); + assertClean(b.sync()); + clock.advance(); + assertClean(c.sync()); + clock.advance(); + assertClean(b.sync()); + + kind.edit(b, "TaskX-B"); + clock.advance(); + kind.delete(c); + clock.advance(); + assertClean(c.sync()); + clock.advance(); + assertThat(b.sync().getStatus()).isEqualTo(SyncState.Status.SUCCESS); + assertThat(b.store.getUnresolvedConflicts()).hasSize(1); + clock.advance(); + assertThat(a.sync().getStatus()).isEqualTo(SyncState.Status.SUCCESS); + + // B keeps its edit: the winner was the deletion, so it is the alternative that stays. + clock.advance(); + for (SyncConflictEntity conflict : b.store.getUnresolvedConflicts()) { + b.store.resolveConflict( + conflict.id, + conflict.winnerTombstone + ? SyncResolution.KEEP_ALTERNATIVE + : SyncResolution.KEEP_WINNER); + } + clock.advance(); + assertClean(b.sync()); + clock.advance(); + assertClean(a.sync()); + + // The record arrived on A; nothing is left there to tap, and it lives on everywhere. + assertThat(a.store.getUnresolvedConflicts()).isEmpty(); + assertThat(kind.title(a)).isEqualTo("TaskX-B"); + clock.advance(); + assertClean(c.sync()); + assertThat(kind.title(c)).isEqualTo("TaskX-B"); + clock.advance(); + assertClean(a.sync()); + clock.advance(); + assertClean(b.sync()); + assertThat(kind.title(b)).isEqualTo("TaskX-B"); + assertThat(b.store.getUnresolvedConflicts()).isEmpty(); + assertThat(c.store.getUnresolvedConflicts()).isEmpty(); } @Test diff --git a/app/src/main/java/com/pasich/mynotes/data/sync/RoomSyncStore.java b/app/src/main/java/com/pasich/mynotes/data/sync/RoomSyncStore.java index a460420..64f4458 100644 --- a/app/src/main/java/com/pasich/mynotes/data/sync/RoomSyncStore.java +++ b/app/src/main/java/com/pasich/mynotes/data/sync/RoomSyncStore.java @@ -308,10 +308,21 @@ private void applySnapshotInternal( localId, record.getCanonicalPayloadHash()); } + // A row stored here before this device held the record — a + // conflict replicated from elsewhere — may name a winner + // that is no longer the live version. Left open, it was + // offered pre-selected on that winner, and when the winner + // was a deletion one tap deleted the record everywhere. + retireConflictsSupersededBy(record); transactionFailureInjector.afterRecordApplied(record); continue; } - if (metadata == null) continue; + if (metadata == null) { + // A deletion of a record this device never held: nothing to + // apply, but an open row for it must still follow the version. + retireConflictsSupersededBy(record); + continue; + } if (record.getType() == SyncRecord.Type.PREFERENCES && !record.isTombstone()) { // Decided last, once every other record has been applied: @@ -414,14 +425,11 @@ private void applySnapshotInternal( preferencesMetadata[0].localId, stagedPreferencesUpdatedAt, null); - if (preferencesRecord != null) { - database.syncMetadataDao() - .setSyncedVersion( - preferencesMetadata[0].recordType, - preferencesMetadata[0].localId, - preferencesRecord - .getCanonicalPayloadHash()); - } + // The synced version is recorded only once the commit below + // has succeeded: recorded here, a refused or failed commit + // left the base naming a version that was never applied, and + // the next build then published the local settings over the + // other device's change with no conflict. database.syncPendingPreferencesDao() .upsert( new SyncPendingPreferencesEntity( @@ -447,18 +455,35 @@ private void applySnapshotInternal( throw error.ioException; } if (deferFinalState || preferencesBaseline[0] != null) { + boolean committed = false; if (preferencesBaseline[0] != null) { // The journal is only dropped once the adapter reports a durable commit; a // failure here leaves it in place for recoverPendingPreferences and keeps the // sync state retryable rather than claiming success. A commit refused because // the settings moved in the meantime is not a failure: the journal is dropped // and the edit is published by the next build. - commitPendingPreferences( - stagedPreferences, stagedPreferencesTarget, preferencesBaseline[0]); + committed = + commitPendingPreferences( + stagedPreferences, stagedPreferencesTarget, preferencesBaseline[0]); } + boolean recordBase = committed && preferencesRecord != null; database.runInTransaction( () -> { database.syncPendingPreferencesDao().clear(); + if (recordBase) { + SyncMetadataEntity metadata = + database.syncMetadataDao() + .getByStableId( + SyncMetadata.RECORD_TYPE_PREFERENCES, + PREFERENCES_STABLE_ID); + if (metadata != null) { + database.syncMetadataDao() + .setSyncedVersion( + metadata.recordType, + metadata.localId, + preferencesRecord.getCanonicalPayloadHash()); + } + } if (finalState != null) database.syncStateDao().upsert(toEntity(finalState)); }); @@ -1095,6 +1120,11 @@ public void resolveConflict(long conflictId, @NonNull SyncResolution resolution) * was dropped unsettled instead of offered. Its alternative came back at the next sync as a * fresh conflict, was dropped again after the next resolution, and the account never settled. * Only content the user actually changed since the conflict was recorded counts. + * + *

The one content that keeps a row alive is the row's own winner. A record that now equals + * the row's alternative was switched to it — by a resolution here or on another device — so the + * pre-selected winner is a version the record has left behind; offered anyway, and when that + * winner was a deletion, one tap deleted the record everywhere. */ private boolean isSuperseded(@NonNull SyncConflictEntity conflict) throws IOException { SyncMetadataEntity metadata = @@ -1104,9 +1134,8 @@ private boolean isSuperseded(@NonNull SyncConflictEntity conflict) throws IOExce <= Math.max(conflict.winnerUpdatedAt, conflict.loserUpdatedAt)) { return false; } - String current = contentDigest(metadata); - return !current.equals(contentDigest(conflict.recordType, conflict.winnerJson)) - && !current.equals(contentDigest(conflict.recordType, conflict.loserJson)); + return !contentDigest(metadata) + .equals(contentDigest(conflict.recordType, conflict.winnerJson)); } /** A digest of what the local record says, independent of when it last changed. */ diff --git a/app/src/main/java/com/pasich/mynotes/data/sync/SyncMerger.java b/app/src/main/java/com/pasich/mynotes/data/sync/SyncMerger.java index 9bd139c..9bb8796 100644 --- a/app/src/main/java/com/pasich/mynotes/data/sync/SyncMerger.java +++ b/app/src/main/java/com/pasich/mynotes/data/sync/SyncMerger.java @@ -91,8 +91,13 @@ private void mergeVersions( merged.put(key, local); return; } - if (base.equals(local.getCanonicalPayloadHash())) { - // This device has not moved since it last synchronized; the remote has. + if (base.equals(local.getCanonicalPayloadHash()) + && !remote.getUpdatedAt().isBefore(local.getUpdatedAt())) { + // This device has not moved since it last synchronized; the remote has. Only + // forwards, though: a remote older than what this device synchronized is a head + // that has gone missing, and following it would revert the newer version on every + // unedited device until it existed nowhere. That case falls through to + // last-writer-wins below, which keeps the local copy and republishes it. merged.put(key, remote); return; } diff --git a/app/src/test/java/com/pasich/mynotes/data/sync/SyncMergerTest.java b/app/src/test/java/com/pasich/mynotes/data/sync/SyncMergerTest.java index 313a461..fa99bf6 100644 --- a/app/src/test/java/com/pasich/mynotes/data/sync/SyncMergerTest.java +++ b/app/src/test/java/com/pasich/mynotes/data/sync/SyncMergerTest.java @@ -55,6 +55,21 @@ public void merge_takesTheRemoteWithoutAConflictWhenOnlyTheOtherSideMoved() { assertThat(result.getConflicts()).isEmpty(); } + @Test + public void merge_doesNotFollowARemoteOlderThanWhatItLastSynchronized() { + // A head bundle gone missing from Drive makes the remote fall back to an older version. + // Taking it because "only the remote moved" reverted the newer copy on every unedited + // device until it existed nowhere; last-writer-wins keeps it and republishes it. + SyncRecord synced = note(NOTE_ID, TWENTY, "Milk and bread"); + SyncRecord unchanged = synced.withBaseVersion(synced.getCanonicalPayloadHash()); + SyncRecord olderRemote = note(NOTE_ID, TEN, "Milk"); + + SyncMergeResult result = merger.merge(snapshot(unchanged), snapshot(olderRemote)); + + assertThat(result.getMergedSnapshot().getRecords()).containsExactly(unchanged); + assertThat(result.getConflicts()).hasSize(1); + } + @Test public void merge_stillReportsAConflictWhenBothSidesMovedFromTheSameBase() { SyncRecord synced = note(NOTE_ID, TEN, "Milk"); diff --git a/app/src/test/java/com/pasich/mynotes/data/sync/TwoPeerConvergenceTest.java b/app/src/test/java/com/pasich/mynotes/data/sync/TwoPeerConvergenceTest.java index e30b79f..47ac7b3 100644 --- a/app/src/test/java/com/pasich/mynotes/data/sync/TwoPeerConvergenceTest.java +++ b/app/src/test/java/com/pasich/mynotes/data/sync/TwoPeerConvergenceTest.java @@ -58,6 +58,55 @@ public void aCategoryDeletedOnOnePeerAndEditedOnTheOtherSettlesAfterOneResolutio deletedHereEditedThereSettlesAfterOneResolution(SyncRecord.Type.CATEGORY); } + @Test + public void aRecordRevivedElsewhereLeavesNoPhantomDeletionOnAPeerThatNeverHeldIt() { + // B edits, C deletes, B keeps the edit. A — a fresh install that never held the record — + // received the deletion-versus-edit conflict along the way. Once the edit arrives on A the + // row is meaningless: offered anyway, pre-selected on the deletion, one tap deleted the + // record on every device with no conflict raised anywhere. + SyncRecord.Type type = SyncRecord.Type.TASK; + Peer b = new Peer("B", clock, drive); + Peer c = new Peer("C", clock, drive); + Peer a = new Peer("A", clock, drive); + b.create(type, RECORD_ID, "TaskX"); + clock.advance(); + assertThat(b.sync().getConflictCount()).isEqualTo(0); + clock.advance(); + assertThat(c.sync().getConflictCount()).isEqualTo(0); + clock.advance(); + assertThat(b.sync().getConflictCount()).isEqualTo(0); + + b.edit(type, RECORD_ID, "TaskX-B"); + clock.advance(); + c.delete(type, RECORD_ID); + clock.advance(); + assertThat(c.sync().getConflictCount()).isEqualTo(0); + clock.advance(); + assertThat(b.sync().getStatus()).isEqualTo(SyncState.Status.SUCCESS); + assertThat(b.store.pendingConflicts()).hasSize(1); + clock.advance(); + a.sync(); + + // B keeps its edit and publishes it. + clock.advance(); + b.store.resolveAllKeepingLive(clock.instant()); + clock.advance(); + assertThat(b.sync().getConflictCount()).isEqualTo(0); + clock.advance(); + assertThat(a.sync().getConflictCount()).isEqualTo(0); + + assertThat(a.store.pendingConflicts()).isEmpty(); + assertThat(a.store.titleOf(type, RECORD_ID)).isEqualTo("TaskX-B"); + clock.advance(); + assertThat(c.sync().getConflictCount()).isEqualTo(0); + assertThat(c.store.titleOf(type, RECORD_ID)).isEqualTo("TaskX-B"); + clock.advance(); + assertThat(a.sync().getConflictCount()).isEqualTo(0); + clock.advance(); + assertThat(b.sync().getConflictCount()).isEqualTo(0); + assertThat(b.store.titleOf(type, RECORD_ID)).isEqualTo("TaskX-B"); + } + @Test public void aTaskSyncedBeforeBasesWereRecordedStillSettlesAfterOneResolution() { deletedHereEditedThereSettlesAfterOneResolution(SyncRecord.Type.TASK, true); @@ -251,8 +300,7 @@ void resolveKeepingLive(ConflictRow row, Instant resolvedAt) { .isAfter(row.conflict.getLoser().getUpdatedAt()) ? row.conflict.getWinner().getUpdatedAt() : row.conflict.getLoser().getUpdatedAt()) - && !contentDigest(current).equals(contentDigest(row.conflict.getWinner())) - && !contentDigest(current).equals(contentDigest(row.conflict.getLoser()))) { + && !contentDigest(current).equals(contentDigest(row.conflict.getWinner()))) { conflicts.remove(row); return; }