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 <noreply@anthropic.com>docs/962-reclamation-plan
diffwireauto
3 weeks ago
committed by
Strycher
parent
4b8f57312c
commit
e2b8970f1f
@ -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:<messageId>` whenever it is non-empty.
|
||||
String blobWith(List<String> ids) =>
|
||||
'[${ids.map((id) => '{"messageId":"$id","text":"t-$id","timestamp":1}').join(',')}]';
|
||||
|
||||
List<MessageRowInput> 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);
|
||||
},
|
||||
);
|
||||
}
|
||||
@ -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<String, String> 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<int> 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<Exception>()),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
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);
|
||||
},
|
||||
);
|
||||
});
|
||||
}
|
||||
Loading…
Reference in new issue