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; }