From e2b8970f1f65cabc6cc492ff1a273e058eb484e2 Mon Sep 17 00:00:00 2001 From: diffwireauto <330573775+diffwireauto@users.noreply.github.com> Date: Thu, 22 Oct 2026 00:14:15 -0400 Subject: [PATCH] fix(#943): establish what governs a downgrade, and correct the claim in code #943 says drift "refuses to open a database whose user_version is newer than the binary's schemaVersion". That is wrong, and the real mechanism points somewhere else. drift has no newer-schema refusal. It compares the stored user_version against schemaVersion for INEQUALITY and routes both directions into onUpgrade ("Schema version upgrades and downgrades will both be run here"). The pre-#917 build declared schema 1 and no migration getter at all, so it got drift's default onUpgrade, which throws. That is why the owner's old install would not start: not a version check, but a missing strategy, so it never reached the blobs it would have found. The consequence matters for the decision. This build does NOT throw on a downgrade: the guard is `from < 2`, so a database from a future schema opens silently, and drift then stamps user_version back down to 2, erasing the only record that it has been through a newer build. The next schema bump inherits a silent failure rather than #943's loud one. - schema_downgrade_test.dart, six tests: a newer version reports as hadUpgrade rather than being refused; a bare strategy throws (the #943 failure); this build opens a schema-3 database without complaint; and it rewrites the version backwards. Two more pin that an equal version takes neither branch and that a fresh database still lands on 2, so the downgrade cases are genuinely testing the downgrade path. - Corrects the stale claim in offband_database.dart, which still told readers a downgrade finds its history. The merged plan was corrected in 4b8f5731; this is the same correction in the code it describes. Also lands migration_refusal_trap_test.dart for #952, found while gathering this evidence: four of the owner's store keys hold rows the blob does not, so the readback gate's equality check refuses them on every attempt, for good. Five tests, including the clean-key control and a check that no message is lost. Fix deliberately not included; #952 carries the decision. The three decisions in #943 are untouched and remain the owner's. Task: meshcore-open-3e1 Agent Mail: TealGlen Co-Authored-By: Claude Opus 5 --- lib/storage/drift/offband_database.dart | 17 ++- test/storage/migration_refusal_trap_test.dart | 140 +++++++++++++++++ test/storage/schema_downgrade_test.dart | 144 ++++++++++++++++++ 3 files changed, 298 insertions(+), 3 deletions(-) create mode 100644 test/storage/migration_refusal_trap_test.dart create mode 100644 test/storage/schema_downgrade_test.dart diff --git a/lib/storage/drift/offband_database.dart b/lib/storage/drift/offband_database.dart index d5a6f4d4..1ba501fe 100644 --- a/lib/storage/drift/offband_database.dart +++ b/lib/storage/drift/offband_database.dart @@ -318,9 +318,20 @@ class OffbandDatabase extends _$OffbandDatabase { int get schemaVersion => 2; /// Schema 2 adds [MessageRecords] (#920). Purely additive: [StoredBlobs] is - /// untouched and keeps every byte it had, so an install that opens this - /// schema and is then taken back to an older build still finds its history - /// where that build looks for it. + /// untouched and keeps every byte it had. + /// + /// That is true of the data and NOT of a downgrade, which is what #943 + /// corrects. drift has no "newer schema" refusal: it compares the stored + /// `user_version` against [schemaVersion] for inequality and routes BOTH + /// directions into this callback. The pre-#917 build declared schema 1 and + /// no strategy at all, so it got drift's default `onUpgrade`, which throws, + /// and so it could never reach the blobs it would have found. + /// + /// This build does not throw on a downgrade, because the guard below is + /// `from < 2`. A database from a future schema opens here silently and drift + /// then stamps `user_version` back down, erasing the only record that it has + /// been through a newer build. #943 holds the decision on what to do about + /// that; `test/storage/schema_downgrade_test.dart` pins the behaviour. @override MigrationStrategy get migration => MigrationStrategy( onUpgrade: (m, from, to) async { diff --git a/test/storage/migration_refusal_trap_test.dart b/test/storage/migration_refusal_trap_test.dart new file mode 100644 index 00000000..f08ec147 --- /dev/null +++ b/test/storage/migration_refusal_trap_test.dart @@ -0,0 +1,140 @@ +import 'package:drift/native.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:meshcore_open/storage/drift/blob_store.dart'; +import 'package:meshcore_open/storage/drift/offband_database.dart'; +import 'package:meshcore_open/storage/prefs_manager.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +/// Found while working #943, on the owner's live database. +/// +/// Four of his store keys hold rows but are NOT in the migrated marker, so +/// they still read and write through the blob. Each one has rows whose +/// `merge_key` does not appear in its blob (1, 3, 7 and 5 of them). The +/// readback gate in `ensureMessageRowsMigrated` compares the key's TOTAL row +/// count against the number of distinct records in the blob, so those extra +/// rows make the two numbers differ and the migration is refused. +/// +/// The gate's own log line calls the leftover rows "harmless because the key +/// is not marked migrated". These tests check that claim. It does not hold: +/// nothing ever removes the extra rows and the blob can never come to contain +/// them, so the comparison fails identically on every future attempt and the +/// key is stuck on the blob path for good. +void main() { + late OffbandDatabase db; + late BlobStore blobs; + + const key = 'channel_messages_aaaaaaaaaapsk_deadbeef'; + + /// Two records, as a channel history blob. Only `messageId` matters here: + /// both stores' merge key is `id:` whenever it is non-empty. + String blobWith(List ids) => + '[${ids.map((id) => '{"messageId":"$id","text":"t-$id","timestamp":1}').join(',')}]'; + + List toRows(String json) { + final ids = RegExp( + r'"messageId":"([^"]+)"', + ).allMatches(json).map((m) => m.group(1)!).toList(); + return ids + .map( + (id) => MessageRowInput( + mergeKey: 'id:$id', + timestampMs: 1, + payload: '{"messageId":"$id"}', + ), + ) + .toList(); + } + + setUp(() async { + SharedPreferences.setMockInitialValues({}); + PrefsManager.reset(); + await PrefsManager.initialize(); + db = OffbandDatabase(NativeDatabase.memory()); + blobs = BlobStore(db); + BlobStore.overrideForTest(blobs); + }); + + tearDown(() async { + await db.close(); + BlobStore.clearTestOverride(); + }); + + test('a clean key migrates, so the fixture itself is sound', () async { + await blobs.write(key, blobWith(['a', 'b', 'c'])); + + expect(await blobs.ensureMessageRowsMigrated(key, toRows), isTrue); + expect(await blobs.isOnMessageRows(key), isTrue); + expect(await blobs.messageRowCount(key), 3); + }); + + test('one row the blob does not contain is enough to refuse', () async { + await blobs.write(key, blobWith(['a', 'b', 'c'])); + // The state the owner's four keys are in: a row for a message that is not + // in the blob. On his biggest channel this is 5 rows against 6,308. + await blobs.upsertMessageRows(key, const [ + MessageRowInput(mergeKey: 'id:newer', timestampMs: 2, payload: '{}'), + ]); + + expect( + await blobs.ensureMessageRowsMigrated(key, toRows), + isFalse, + reason: 'the gate compares 4 stored rows against 3 blob records', + ); + expect(await blobs.isOnMessageRows(key), isFalse); + }); + + test('and it refuses again, every time, for good', () async { + await blobs.write(key, blobWith(['a', 'b', 'c'])); + await blobs.upsertMessageRows(key, const [ + MessageRowInput(mergeKey: 'id:newer', timestampMs: 2, payload: '{}'), + ]); + + for (var attempt = 1; attempt <= 3; attempt++) { + expect( + await blobs.ensureMessageRowsMigrated(key, toRows), + isFalse, + reason: 'attempt $attempt refused', + ); + } + + // Nothing cleaned up the extra row, which is why the next attempt fails + // the same way. This is the trap: the key cannot recover on its own. + expect(await blobs.messageRowCount(key), 4); + expect(await blobs.isOnMessageRows(key), isFalse); + }); + + test('a growing blob does not rescue it either', () async { + await blobs.write(key, blobWith(['a', 'b', 'c'])); + await blobs.upsertMessageRows(key, const [ + MessageRowInput(mergeKey: 'id:newer', timestampMs: 2, payload: '{}'), + ]); + expect(await blobs.ensureMessageRowsMigrated(key, toRows), isFalse); + + // The key stays on the blob path, so new traffic lands in the blob. That + // is the only thing about this key that changes over time, and it moves + // the two numbers further apart rather than together. + await blobs.write(key, blobWith(['a', 'b', 'c', 'd', 'e'])); + + expect( + await blobs.ensureMessageRowsMigrated(key, toRows), + isFalse, + reason: '6 rows against 5 blob records: still unequal', + ); + }); + + test( + 'no message is actually lost, which is why this is not data loss', + () async { + // Worth pinning: the blob remains complete and authoritative for a refused + // key, so the cost here is the save path, not history. + final history = blobWith(['a', 'b', 'c']); + await blobs.write(key, history); + await blobs.upsertMessageRows(key, const [ + MessageRowInput(mergeKey: 'id:newer', timestampMs: 2, payload: '{}'), + ]); + await blobs.ensureMessageRowsMigrated(key, toRows); + + expect(await blobs.read(key), history); + }, + ); +} diff --git a/test/storage/schema_downgrade_test.dart b/test/storage/schema_downgrade_test.dart new file mode 100644 index 00000000..3b37b486 --- /dev/null +++ b/test/storage/schema_downgrade_test.dart @@ -0,0 +1,144 @@ +import 'package:drift/drift.dart'; +import 'package:drift/native.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:meshcore_open/storage/drift/blob_store.dart'; +import 'package:meshcore_open/storage/drift/offband_database.dart'; +import 'package:meshcore_open/storage/prefs_manager.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +/// #943: what happens when a build opens a database left by a NEWER build. +/// +/// #917's plan claimed a downgrade still works because the blobs are retained. +/// The owner hit the opposite on 2026-10-20: his pre-#917 install would not +/// start against his migrated database. These tests pin down what actually +/// governs that, because the issue's own explanation ("drift refuses to open a +/// database whose user_version is newer") is wrong, and the real mechanism +/// points somewhere else entirely. +void main() { + setUp(() async { + SharedPreferences.setMockInitialValues({}); + PrefsManager.reset(); + await PrefsManager.initialize(); + }); + + tearDown(BlobStore.clearTestOverride); + + /// A database shaped like one a FUTURE build left behind: both of today's + /// tables, real data, and `user_version` ahead of this binary's + /// schemaVersion of 2. + QueryExecutor databaseAtVersion(int userVersion, Map blobs) { + return NativeDatabase.memory( + setup: (raw) { + raw.execute( + 'CREATE TABLE stored_blobs (' + 'key TEXT NOT NULL, ' + 'value TEXT NOT NULL, ' + "updated_at INTEGER NOT NULL DEFAULT (CAST(strftime('%s', " + "CURRENT_TIMESTAMP) AS INTEGER)), " + 'PRIMARY KEY (key))', + ); + raw.execute( + 'CREATE TABLE message_records (' + 'store_key TEXT NOT NULL, ' + 'merge_key TEXT NOT NULL, ' + 'timestamp_ms INTEGER NOT NULL, ' + 'payload TEXT NOT NULL, ' + 'PRIMARY KEY (store_key, merge_key))', + ); + for (final entry in blobs.entries) { + final escaped = entry.value.replaceAll("'", "''"); + raw.execute( + "INSERT INTO stored_blobs (key, value) VALUES ('${entry.key}', " + "'$escaped')", + ); + } + raw.execute('PRAGMA user_version = $userVersion'); + }, + ); + } + + Future userVersionOf(OffbandDatabase db) => db + .customSelect('PRAGMA user_version') + .getSingle() + .then((r) => r.data.values.first as int); + + group('drift dispatches a downgrade through onUpgrade', () { + test('a newer user_version is reported as hadUpgrade, not refused', () { + // The fact the whole question rests on. drift has no "newer schema" + // refusal: it compares the two numbers for INEQUALITY and routes both + // directions into onUpgrade ("Schema version upgrades and downgrades + // will both be run here"). So whether a downgrade works is decided by + // OUR onUpgrade, not by drift. + const downgrade = OpeningDetails(3, 2); + expect(downgrade.hadUpgrade, isTrue); + expect(downgrade.wasCreated, isFalse); + }); + + test('a strategy with no onUpgrade throws, which is what bit #943', () { + // The pre-#917 build declared schemaVersion 1 and NO migration getter, + // so it got exactly this default. That, and not a drift version check, + // is why the owner's old install would not open his schema-2 database. + final db = OffbandDatabase(NativeDatabase.memory()); + addTearDown(db.close); + + expect( + () => MigrationStrategy().onUpgrade(Migrator(db), 2, 1), + throwsA(isA()), + ); + }); + }); + + group('this build against a database from a future schema', () { + test('it opens without complaint, which is the real hazard', () async { + // Our onUpgrade guards on `from < 2`, so a 3 -> 2 downgrade falls + // straight through it. No throw: the database opens and reads fine, + // which means a future schema bump does NOT inherit #943's loud + // failure. It gets a silent one instead. + final db = OffbandDatabase( + databaseAtVersion(3, {'contacts': '[{"name":"someone"}]'}), + ); + addTearDown(db.close); + + expect(await BlobStore(db).read('contacts'), '[{"name":"someone"}]'); + }); + + test('and it stamps user_version BACKWARDS to 2', () async { + // drift writes its own schemaVersion once migrations "ran". A database + // carrying schema-3 data is now labelled schema 2, so the schema-3 + // build that wrote it will not run its own 2 -> 3 upgrade again, against + // tables that already exist in their newer shape. + final db = OffbandDatabase(databaseAtVersion(3, {'contacts': '[]'})); + addTearDown(db.close); + await BlobStore(db).read('contacts'); + + expect( + await userVersionOf(db), + 2, + reason: + 'a silent version rewrite is worse than a refusal: it destroys the ' + 'only record that this database has been through a newer build', + ); + }); + }); + + group('a normal upgrade still behaves', () { + test('an equal version is neither a create nor an upgrade', () { + // Guards the tests above: they only mean something if a matched version + // takes neither branch, so onUpgrade is genuinely downgrade-only here. + const same = OpeningDetails(2, 2); + expect(same.hadUpgrade, isFalse); + expect(same.wasCreated, isFalse); + }); + + test( + 'a fresh database lands on 2 and is not treated as a downgrade', + () async { + final db = OffbandDatabase(NativeDatabase.memory()); + addTearDown(db.close); + await BlobStore(db).read('anything'); + + expect(await userVersionOf(db), 2); + }, + ); + }); +}