From 341b07cb0749265c38f1e1ad3abdd24bf5238228 Mon Sep 17 00:00:00 2001 From: pasichDev Date: Sat, 5 Sep 2026 18:19:21 +0300 Subject: [PATCH] test(sync): make the two-peer tests pin what they claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review of the previous release found no defect in the code but showed that two of its four tests proved nothing. The JVM two-peer test passed on the parent commit: its store mirror had already been retiring conflict rows on every path before the fix that introduced that behaviour, which is also why this suite never caught the fleet-wide deletion it was written to guard. Worse, its resolution always chose the live version, so the tap that actually loses data — keeping a winner that is a deletion — could not be expressed at all. The mirror now has the store's four arrival paths, its supersession rule and a resolution that can choose either side, and the three-peer test performs the losing tap and asserts the record survives on all peers. With the mirror reverted to the previous retire paths it fails on the phantom row; with the previous supersession rule as well it fails on the task being gone, which is the loss itself. The instrumented test claimed the record equalled the row's alternative and it did not — the loser JSON was hand-written and matched neither side, so the previous rule dropped the row too. It now uses the note's real serialised payload, and with the previous predicate restored the note is deleted, confirmed on the device rather than argued. Recovery after a crash between applying settings and recording their bookkeeping never recorded the base, leaving it naming a version that was never applied. It is recorded on the recovery path, skipping journals written by a conflict resolution and only while the record has not moved since. 343 unit tests, 93 instrumentation tests, 0 failures; lint 0 errors. --- CHANGELOG.md | 2 +- app/build.gradle | 2 +- .../pasich/mynotes/db/RoomSyncStoreTest.java | 57 ++++++++ .../mynotes/data/sync/RoomSyncStore.java | 34 +++++ .../data/sync/TwoPeerConvergenceTest.java | 136 +++++++++++++----- 5 files changed, 196 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cebf849..557ed4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # CHANGELOG -## [2.6.53] - 05.09.2026 +## [2.6.54] - 05.09.2026 **New** diff --git a/app/build.gradle b/app/build.gradle index f12a310..c69a447 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 = 53 +def appVersionCode = 54 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 fe8ee88..2132dcf 100644 --- a/app/src/androidTest/java/com/pasich/mynotes/db/RoomSyncStoreTest.java +++ b/app/src/androidTest/java/com/pasich/mynotes/db/RoomSyncStoreTest.java @@ -796,10 +796,21 @@ public void resolveConflict_dropsARowWhoseWinnerTheRecordHasAlreadyLeftBehind() // 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); + // The alternative is the note exactly as this store serialises it, so the record's + // content genuinely equals the loser: a rule that only drops rows equal to neither + // offer keeps this one and applies the deletion. + SyncRecord local = onlyNote(store.buildSnapshot().requireSnapshot()); 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.loserVersionId = local.getCanonicalPayloadHash(); + row.loserJson = + "{\"type\":\"note\",\"id\":\"11111111-1111-4111-8111-111111111111\"," + + "\"updatedAt\":\"1970-01-01T00:00:02Z\",\"deletedAt\":null," + + "\"payload\":" + + new com.google.gson.Gson().toJson(local.getPayload()) + + "}"; row.winnerJson = "{\"type\":\"note\",\"id\":\"11111111-1111-4111-8111-111111111111\"," + "\"updatedAt\":\"1970-01-01T00:00:03Z\",\"deletedAt\":\"1970-01-01T00:00:03Z\"," @@ -866,6 +877,52 @@ public void applySnapshot_doesNotRecordThePreferencesBaseUntilTheCommitSucceeded .isEqualTo(remote.getCanonicalPayloadHash()); } + @Test + public void recoveryRecordsTheBaseOfAnApplyThatDiedBeforeItsBookkeeping() throws Exception { + PreferencesAdapter adapter = new PreferencesAdapter(); + new RoomSyncStore(context, db, adapter.helper).readState(); + com.google.gson.Gson gson = new com.google.gson.Gson(); + SyncRecord remote = + SyncRecord.live( + SyncRecord.Type.PREFERENCES, + "00000000-0000-4000-8000-000000000000", + java.time.Instant.ofEpochMilli(2_000L), + gson.toJsonTree(preferencesWithTheme(3)).getAsJsonObject()); + // What the apply transaction leaves when the process dies right after it: the record + // timestamped at the remote version, its payload journaled against the live digest, the + // base not yet recorded. + String stagedJson = gson.toJson(preferencesWithTheme(3)); + String liveDigest = + sha256(gson.toJson(preferencesWithTheme(1)).getBytes(StandardCharsets.UTF_8)); + db.syncMetadataDao().setVersion(SyncMetadata.RECORD_TYPE_PREFERENCES, 0, 2_000L, null); + db.syncPendingPreferencesDao() + .upsert( + new com.pasich.mynotes.data.database.entities.SyncPendingPreferencesEntity( + 1, + stagedJson, + sha256(stagedJson.getBytes(StandardCharsets.UTF_8)), + liveDigest, + 2_000L, + false, + 0L, + "")); + + // A fresh store seeds, which is where recovery replays the journal. + new RoomSyncStore(context, db, adapter.helper).readState(); + + assertThat(adapter.committed.get().getThemeValue()).isEqualTo(3); + assertThat(db.syncPendingPreferencesDao().get()).isNull(); + // Replayed but left with the base naming the version before it, the next merge saw a + // local edit that nobody made. + 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/main/java/com/pasich/mynotes/data/sync/RoomSyncStore.java b/app/src/main/java/com/pasich/mynotes/data/sync/RoomSyncStore.java index 64f4458..9954bdc 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 @@ -643,6 +643,7 @@ private void recoverPendingPreferences() throws IOException { // stale baseline, calls this a local edit and touches the record to now, which // lets an unchanged copy outrank a genuine edit made on another device. preferences.edit().putString(PREFERENCES_HASH, target).commit(); + recordRecoveredPreferencesBase(pending, backup); finishJournal(pending); return; case DISCARD_STALE: @@ -655,10 +656,43 @@ private void recoverPendingPreferences() throws IOException { default: // Replay was decided because the live values still match the baseline. commitPendingPreferences(backup, target, pending.baselineHash); + recordRecoveredPreferencesBase(pending, backup); finishJournal(pending); } } + /** + * Records, on recovery, the base the interrupted apply would have recorded after its commit. + * + *

The apply transaction stamps the record with the remote version's time and journals its + * payload; the base is recorded only once the commit has succeeded. Dying in between left the + * live settings at the remote version with the base still naming the one before it, so the next + * merge saw a local edit nobody made. The version id is rebuilt from the journal: the same + * timestamp and the same payload, serialised by the same class, hash to the same id. A journal + * written by a conflict resolution is left alone; a resolution re-times the record without + * touching its base. + */ + private void recordRecoveredPreferencesBase( + @NonNull SyncPendingPreferencesEntity pending, @NonNull PreferencesBackup backup) { + if (pending.conflictId > 0 || pending.recordUpdatedAt <= 0L) return; + SyncMetadataEntity metadata = + database.syncMetadataDao() + .getByStableId(SyncMetadata.RECORD_TYPE_PREFERENCES, PREFERENCES_STABLE_ID); + if (metadata == null || metadata.updatedAt != pending.recordUpdatedAt) return; + String versionId = + SyncRecord.live( + SyncRecord.Type.PREFERENCES, + PREFERENCES_STABLE_ID, + Instant.ofEpochMilli(pending.recordUpdatedAt), + gson.toJsonTree(backup).getAsJsonObject()) + .getCanonicalPayloadHash(); + database.runInTransaction( + () -> + database.syncMetadataDao() + .setSyncedVersion( + metadata.recordType, metadata.localId, versionId)); + } + /** Clears the journal, completing the conflict bookkeeping when it names one. */ private void finishJournal(@NonNull SyncPendingPreferencesEntity pending) { if (pending.conflictId > 0) { 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 47ac7b3..ef998e0 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 @@ -97,6 +97,13 @@ public void aRecordRevivedElsewhereLeavesNoPhantomDeletionOnAPeerThatNeverHeldIt assertThat(a.store.pendingConflicts()).isEmpty(); assertThat(a.store.titleOf(type, RECORD_ID)).isEqualTo("TaskX-B"); + // The tap that did the damage: accepting whatever the dialog pre-selects. With the row + // gone there is nothing to accept; with it present, this deleted the task on A. + clock.advance(); + a.store.resolveAllKeepingWinner(clock.instant()); + assertThat(a.store.titleOf(type, RECORD_ID)).isEqualTo("TaskX-B"); + clock.advance(); + assertThat(a.sync().getConflictCount()).isEqualTo(0); clock.advance(); assertThat(c.sync().getConflictCount()).isEqualTo(0); assertThat(c.store.titleOf(type, RECORD_ID)).isEqualTo("TaskX-B"); @@ -220,6 +227,7 @@ static final class PeerStore implements SyncStore { private final Map records = new LinkedHashMap<>(); private final Map bases = new LinkedHashMap<>(); private final List conflicts = new ArrayList<>(); + private final Map built = new LinkedHashMap<>(); private SyncState state = SyncState.idle(); PeerStore(Clock clock) { @@ -276,74 +284,136 @@ List pendingConflicts() { /** The dialog loop: every open conflict, one after another, keeping the live version. */ void resolveAllKeepingLive(Instant resolvedAt) { for (ConflictRow row : new ArrayList<>(pendingConflicts())) { - resolveKeepingLive(row, resolvedAt); + resolve( + row, + row.conflict.getWinner().isTombstone() + ? SyncResolution.KEEP_ALTERNATIVE + : SyncResolution.KEEP_WINNER, + resolvedAt); + } + } + + /** The dialog loop with the pre-selected choice accepted every time, deletions included. */ + void resolveAllKeepingWinner(Instant resolvedAt) { + for (ConflictRow row : new ArrayList<>(pendingConflicts())) { + resolve(row, SyncResolution.KEEP_WINNER, resolvedAt); } } /** - * What resolveConflict does: drop a row whose record's content has moved on, otherwise - * re-time the chosen version and mark the row settled. + * What resolveConflict does: drop a row the record has moved past, otherwise apply the + * chosen version — a deletion included — re-timed and with the base left alone, and mark + * the row settled. */ - void resolveKeepingLive(ConflictRow row, Instant resolvedAt) { - SyncRecord chosen = - row.conflict.getWinner().isTombstone() - ? row.conflict.getLoser() - : row.conflict.getWinner(); - String key = key(chosen); - SyncRecord current = records.get(key); - if (current != null - && current.getUpdatedAt() - .isAfter( - row.conflict - .getWinner() - .getUpdatedAt() - .isAfter(row.conflict.getLoser().getUpdatedAt()) - ? row.conflict.getWinner().getUpdatedAt() - : row.conflict.getLoser().getUpdatedAt()) - && !contentDigest(current).equals(contentDigest(row.conflict.getWinner()))) { + void resolve(ConflictRow row, SyncResolution resolution, Instant resolvedAt) { + if (isSuperseded(row.conflict)) { conflicts.remove(row); return; } + SyncRecord chosen = + resolution == SyncResolution.KEEP_WINNER + ? row.conflict.getWinner() + : row.conflict.getLoser(); + String key = key(chosen); + SyncRecord current = records.get(key); Instant updatedAt = current != null && !resolvedAt.isAfter(current.getUpdatedAt()) ? current.getUpdatedAt().plusMillis(1) : resolvedAt; - records.put( - key, - SyncRecord.live( - chosen.getType(), chosen.getId(), updatedAt, chosen.getPayload())); + if (chosen.isTombstone()) { + // A record never held here has nothing to delete; the store leaves it alone. + if (current != null) { + records.put( + key, + SyncRecord.tombstone( + chosen.getType(), chosen.getId(), updatedAt, updatedAt)); + } + } else { + records.put( + key, + SyncRecord.live( + chosen.getType(), chosen.getId(), updatedAt, chosen.getPayload())); + } row.resolved = true; } + /** + * RoomSyncStore.isSuperseded: a record this peer does not hold is never superseded; one + * that is newer than both offers is, unless it still equals the row's winner. + */ + private boolean isSuperseded(SyncMergeResult.Conflict conflict) { + SyncRecord current = records.get(conflict.getType() + ":" + conflict.getId()); + if (current == null) return false; + Instant newest = + conflict.getWinner().getUpdatedAt().isAfter(conflict.getLoser().getUpdatedAt()) + ? conflict.getWinner().getUpdatedAt() + : conflict.getLoser().getUpdatedAt(); + if (!current.getUpdatedAt().isAfter(newest)) return false; + return !contentDigest(current).equals(contentDigest(conflict.getWinner())); + } + + /** + * RoomSyncStore.retireConflictsSupersededBy: open rows whose winner is not this version. + */ + private void retireConflictsSupersededBy(SyncRecord applied) { + String key = key(applied); + conflicts.removeIf( + row -> + !row.resolved + && key.equals( + row.conflict.getType() + ":" + row.conflict.getId()) + && !row.conflict + .getWinnerVersionId() + .equals(applied.getCanonicalPayloadHash())); + } + @Override public SyncSnapshot readSnapshot() { List withBases = new ArrayList<>(); + built.clear(); for (Map.Entry entry : records.entrySet()) { + built.put(entry.getKey(), entry.getValue().getCanonicalPayloadHash()); withBases.add(entry.getValue().withBaseVersion(bases.get(entry.getKey()))); } return new SyncSnapshot(withBases); } + /** + * The four paths of RoomSyncStore.applySnapshot, kept distinct on purpose: which of them + * retire open rows is exactly what the phantom-deletion scenario depends on. + */ @Override public void applySnapshot(SyncSnapshot snapshot, List incoming) { Set skipped = new LinkedHashSet<>(); for (SyncRecord record : snapshot.getRecords()) { String key = key(record); SyncRecord current = records.get(key); - if (current != null && current.getUpdatedAt().isAfter(record.getUpdatedAt())) { + if (current == null && !record.isTombstone()) { + // Insert: a record this peer never held. + records.put(key, record.withBaseVersion(null)); + bases.put(key, record.getCanonicalPayloadHash()); + retireConflictsSupersededBy(record); + continue; + } + if (current == null) { + // A deletion of a record never held: the store creates no metadata for it, + // so nothing is stored and nothing is published later; only open rows follow. + retireConflictsSupersededBy(record); + continue; + } + if (current.getUpdatedAt().isAfter(record.getUpdatedAt())) { + // Edited during the sync: left alone, except that this peer's own published + // version becomes the base the edit is measured against. skipped.add(key); + if (record.getCanonicalPayloadHash().equals(built.get(key))) { + bases.put(key, record.getCanonicalPayloadHash()); + } continue; } + // Tombstone or live update of a held record. records.put(key, record.withBaseVersion(null)); bases.put(key, record.getCanonicalPayloadHash()); - conflicts.removeIf( - row -> - !row.resolved - && key.equals( - row.conflict.getType() + ":" + row.conflict.getId()) - && !row.conflict - .getWinnerVersionId() - .equals(record.getCanonicalPayloadHash())); + retireConflictsSupersededBy(record); } for (SyncMergeResult.Conflict conflict : incoming) { String key = conflict.getType() + ":" + conflict.getId();