From 313368530aa9bab41997de18e536e6cb57e7d623 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Mon, 20 Jul 2026 14:24:13 +0200 Subject: [PATCH] Restore notes at their own edit time, not "now" Backup recovery and file import both wrote restored notes through `upsert`, which stamps a fresh clock. Under crdt_sync's per-field last-writer-wins that makes a *restore* outrank every other device: the restoring device claims it edited every field just now, so it silently overwrites newer edits made elsewhere on the next sync. This is not hypothetical. The new desktop app seeded itself from BACKLOG.md and rewrote 245 of 330 field clocks to today under a fresh node id, then synced -- taking authority over the phone for ~74% of all fields. Content happened to survive (the copy came from the same data; only a trailing newline on 5 notes differed via the Markdown round-trip), but nothing about the mechanism guaranteed that. The legacy sqlite migration already did the right thing for exactly this reason, so the fix is to use that same helper everywhere a note is *restored* rather than newly written. Renamed legacyRecordFor -> recordAtNoteTime, since it is no longer migration-specific. The freshness check in importNotes is unchanged: it decides whether the import wins *locally*. The clock decides whether it wins on *other* devices, and an import must not win there merely by being recent. Co-Authored-By: Claude Opus 4.8 (1M context) --- lib/data/legacy_sqlite_migration.dart | 2 +- lib/data/note_repository.dart | 28 ++++++++++++++++++--------- test/note_repository_test.dart | 23 ++++++++++++++++++++++ 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/lib/data/legacy_sqlite_migration.dart b/lib/data/legacy_sqlite_migration.dart index 843db27..be6b44c 100644 --- a/lib/data/legacy_sqlite_migration.dart +++ b/lib/data/legacy_sqlite_migration.dart @@ -42,7 +42,7 @@ class LegacySqliteMigration { final log = {}; for (final row in rows) { final note = Note.fromRow(row); - log[note.id] = NoteRepository.legacyRecordFor( + log[note.id] = NoteRepository.recordAtNoteTime( note, nodeId, deleted: (row['is_deleted'] as int? ?? 0) != 0, diff --git a/lib/data/note_repository.dart b/lib/data/note_repository.dart index 7da1805..567f12a 100644 --- a/lib/data/note_repository.dart +++ b/lib/data/note_repository.dart @@ -115,14 +115,20 @@ class NoteRepository { return NoteRepository._(store, nodeId); } - /// Builds the record for one migrated legacy note, tombstones included. + /// Builds a record whose field clocks come from the note's own `updatedAt` + /// rather than from "now". /// - /// Each note's field clocks are seeded from its own `updated_at`, not "now", - /// so that when two devices migrate independently, a pre-cutover edit still - /// wins by real edit time under per-field last-writer-wins instead of being - /// resolved arbitrarily. Public because the migration that calls it lives in - /// the io-only library, which cannot reach private members here. - static Record legacyRecordFor( + /// Used by every path that *restores* notes the user already had — the + /// legacy sqlite migration, backup recovery, and file import. Stamping "now" + /// there is actively harmful: under per-field last-writer-wins, a restored + /// copy would outrank the same note on every other device, so a device that + /// recovered from a backup would silently overwrite newer edits made + /// elsewhere. Seeding from real edit time makes a restore lose to genuinely + /// newer data, which is what a restore should do. + /// + /// Public because the legacy migration lives in an io-only library that + /// cannot reach private members here. + static Record recordAtNoteTime( Note note, String nodeId, { bool deleted = false, @@ -169,11 +175,15 @@ class NoteRepository { var skipped = 0; for (final note in incoming) { final local = existing[note.id]; + // Restore at the note's own edit time, never "now" — see + // [recordAtNoteTime]. The freshness check above is what decides whether + // the import wins locally; the clock decides whether it wins on *other* + // devices, and an import must not win there simply by being recent. if (local == null) { - await upsert(note); + await _store.upsert(recordAtNoteTime(note, _nodeId)); added++; } else if (note.updatedAt.isAfter(local.updatedAt)) { - await upsert(note); + await _store.upsert(recordAtNoteTime(note, _nodeId)); updated++; } else { skipped++; diff --git a/test/note_repository_test.dart b/test/note_repository_test.dart index fc39cee..fbd877c 100644 --- a/test/note_repository_test.dart +++ b/test/note_repository_test.dart @@ -243,6 +243,29 @@ void main() { }); group('importNotes (safe merge)', () { + test('restores at the note\'s own edit time, not "now"', () async { + // Regression guard. Recovery used to stamp fresh clocks on every + // restored note, so a device that recovered from a backup outranked + // every other device under per-field last-writer-wins and silently + // overwrote newer edits made elsewhere. Observed for real: a restored + // desktop rewrote 245 of 330 field clocks to "now". + final repo = await NoteRepository.openInMemory(); + addTearDown(repo.close); + final edited = DateTime(2026, 3, 4, 5, 6, 7); + + await repo.importNotes([ + note('restored', 'from a backup', createdAt: edited, updatedAt: edited), + ]); + + final field = repo.exportLog()['restored']!.fields['text']!; + expect( + field.$2.wallTimeMs, + edited.millisecondsSinceEpoch, + reason: 'clock must come from updatedAt so a restore cannot outrank ' + 'genuinely newer data on another device', + ); + }); + test('adds notes whose id is not present locally', () async { final repo = await NoteRepository.openInMemory(); addTearDown(repo.close);