Skip to content

fix(db): stop raw_archive silently dropping distinct frames on a reused counter #231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
abdulsaheel wants to merge 2 commits into from
Closed

fix(db): stop raw_archive silently dropping distinct frames on a reused counter #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ccc263
fix(db): stop raw_archive silently dropping distinct frames on a reus…
abdulsaheel Aug 11, 2026
52ee804
test(db): cover the v31→v32 raw_archive re-key on a populated counter…
abdulsaheel Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 45 additions & 4 deletions lib/data/db.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class LocalDb {
/// pass it: sqflite throws `ArgumentError('onCreate must be null if no
/// version is specified')` BEFORE opening anything when `onCreate` is given
/// without `version` (sqflite_common database_mixin.dart).
static const int schemaVersion = 31;
static const int schemaVersion = 32;

/// SQLite caps host parameters per statement (`SQLITE_MAX_VARIABLE_NUMBER` —
/// only 999 on the builds shipped with older Android/iOS). Any `IN (?, ?, …)`
Expand Down Expand Up @@ -430,6 +430,40 @@ class LocalDb {
// output is untouched and the edits replay over it.
await _createSleepNap(db);
}
if (oldV < 32) {
// Re-key raw_archive off the volatile `counter` onto frame `hex`.
// `counter INTEGER PRIMARY KEY` + IGNORE silently DROPPED a distinct
// undecodable frame whenever a post-reboot counter (reset to ~0)
// collided with a still-present pre-reboot row — data loss in the
// "never lose" table. Rebuild keyed by content. Existing rows have
// unique counters, so the copy loses nothing; at most it collapses an
// exact-duplicate hex, which is the dedup we want.
//
// raw_archive is normally created lazily in onOpen (_repairOpenSchema),
// NOT in this ladder, so on an old DB it may not exist yet here — in
// which case there is nothing to migrate and a fresh (hex-keyed) create
// is all that's needed. DROP the old index name before the fresh CREATE
// so it can't collide on the name the rename carried onto the aside
// table (the leaked-`_new`-index footgun documented on the decoded
// rebuild).
final hasArchive = (await db.rawQuery(
"SELECT 1 FROM sqlite_master WHERE type='table' AND name='raw_archive'",
)).isNotEmpty;
if (hasArchive) {
await db.execute('ALTER TABLE raw_archive RENAME TO _raw_archive_old');
await db.execute('DROP INDEX IF EXISTS idx_raw_archive_captured');
await _createRawArchive(db);
await db.execute(
'INSERT OR IGNORE INTO raw_archive '
'(hex, counter, packet_type, rec_ts, captured_at, reason) '
'SELECT hex, counter, packet_type, rec_ts, captured_at, reason '
'FROM _raw_archive_old',
);
await db.execute('DROP TABLE _raw_archive_old');
} else {
await _createRawArchive(db);
}
}
Comment on lines +433 to +466

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Repair the legacy raw_archive shape during onOpen.

If a database has user_version = 32 but still has counter as the raw_archive primary key, this block does not run. _repairOpenSchema then calls _createRawArchive, but CREATE TABLE IF NOT EXISTS leaves the legacy table unchanged. A later counter reuse can still discard a distinct frame.

Extract this rebuild into an idempotent helper that checks the primary-key column with PRAGMA table_info(raw_archive). Call the helper from both onUpgrade and _repairOpenSchema. Add an upgrade regression test that creates the v31 table, inserts rows, opens the v32 database, and verifies that all rows remain available.

As per coding guidelines, “Keep migrations additive and idempotent using sequential onUpgrade if (oldV < N) steps; keep them cheap, repair schemas on open.”

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/data/db.dart` around lines 433 - 466, Extract the raw_archive rebuild
logic into an idempotent helper that uses PRAGMA table_info(raw_archive) to
detect whether counter, rather than hex, is the primary key; leave an already
hex-keyed table unchanged and create the table when absent. Invoke this helper
from both the onUpgrade oldV < 32 migration and _repairOpenSchema so databases
already at user_version 32 are repaired on open. Add an upgrade regression test
covering a v31 counter-keyed table with multiple rows and verify all rows remain
available after opening as v32.

Source: Coding guidelines

},
onOpen: (db) async {
await _repairOpenSchema(db);
Expand Down Expand Up @@ -2543,12 +2577,19 @@ class LocalDb {
/// Durable archive for historical records we received but could not decode
/// (unknown/unsupported version). NEVER pruned — the whole point is that a
/// future firmware's records survive until we understand the format. Keyed by
/// counter so a re-flood after a missed ACK dedups (IGNORE on conflict).
/// frame `hex` (content identity), like `events`/`band_events` — NOT by
/// `counter`. The strap resets its record counter to ~0 on every reboot, so
/// two DISTINCT undecodable frames from different boots can collide on a
/// reused counter; a `counter`-PK + IGNORE silently DROPPED the second, in
/// the one table whose whole purpose is to never lose a frame. Hashing on the
/// bytes means an identical re-flood (missed-ACK redelivery) still dedups,
/// while genuinely distinct frames both survive a counter collision. `counter`
/// is retained as a plain forensic column.
static Future<void> _createRawArchive(Database db) async {
await db.execute('''
CREATE TABLE IF NOT EXISTS raw_archive (
counter INTEGER PRIMARY KEY,
hex TEXT NOT NULL,
hex TEXT PRIMARY KEY,
counter INTEGER,
packet_type INTEGER NOT NULL,
rec_ts INTEGER,
captured_at INTEGER NOT NULL,
Expand Down
86 changes: 86 additions & 0 deletions test/db_migration_ladder_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import 'package:path/path.dart' as p;
import 'package:sqflite_common_ffi/sqflite_ffi.dart';
import 'package:openstrap_edge/data/db.dart';
import 'package:openstrap_edge/data/journal_fields.dart';
import 'package:openstrap_edge/data/models.dart';

/// The pre-v3 raw_records shape: keyed by frame hex, NO rec_ts column.
const _legacyRawDdl = '''
Expand Down Expand Up @@ -417,4 +418,89 @@ void main() {
expect(health['ok'], isTrue, reason: '$health');
},
);

test(
'v31→v32 re-keys raw_archive off the volatile counter onto frame hex '
'without losing a distinct frame, collapsing only exact-duplicate hex',
() async {
const name = 'migrate_from_v31_rawarchive_test.db';
created.add(name);
// The pre-v32 raw_archive shape: keyed by the strap counter, which resets
// to ~0 on reboot — so a post-reboot frame reusing a live counter was
// silently IGNORE-dropped in the one table meant to never lose a frame.
await _seedOldDb(
name,
31,
const [
'''
CREATE TABLE raw_archive (
counter INTEGER PRIMARY KEY,
hex TEXT NOT NULL,
packet_type INTEGER NOT NULL,
rec_ts INTEGER,
captured_at INTEGER NOT NULL,
reason TEXT NOT NULL
)
''',
'CREATE INDEX idx_raw_archive_captured ON raw_archive(captured_at DESC)',
],
seedRows: (db) async {
Future<void> row(int counter, String hex) => db.insert('raw_archive', {
'counter': counter,
'hex': hex,
'packet_type': 0x2F,
'captured_at': 1750000000000 + counter,
'reason': 'undecodable_rec_v99',
});
// Three distinct frames (distinct counter AND hex) — none may be lost.
await row(1, 'aa01');
await row(2, 'bb02');
await row(3, 'cc03');
// Two rows the OLD counter-PK allowed but that carry IDENTICAL bytes;
// the content re-key must collapse them to one (the dedup we want).
await row(10, 'ff06');
await row(11, 'ff06');
},
);

expect(await _openThroughLocalDb(name), LocalDb.schemaVersion);

// 5 old rows → 4: the three distinct frames survive, the duplicate-hex
// pair collapses to one. Nothing distinct was lost.
final stats = await LocalDb.rawArchiveStats();
expect(stats['count'], 4);

// The migrated table is now hex-PK, proven end-to-end through the REAL
// ladder (not just a fresh onCreate): two DISTINCT frames that reuse ONE
// counter both survive — the exact loss the old counter-PK caused.
await LocalDb.archiveRawRecord(ArchiveRecord(
counter: 1, // reuses a counter already present from the seed
hex: 'dd04',
packetType: 0x2F,
capturedAt: 1750000500000,
reason: 'undecodable_post_reboot',
));
await LocalDb.archiveRawRecord(ArchiveRecord(
counter: 1, // SAME counter, DIFFERENT bytes
hex: 'ee05',
packetType: 0x2F,
capturedAt: 1750000600000,
reason: 'undecodable_post_reboot',
));
expect((await LocalDb.rawArchiveStats())['count'], 6);

// …and an identical re-flood still dedups on content.
await LocalDb.archiveRawRecord(ArchiveRecord(
counter: 999, // different counter, but bytes already archived
hex: 'dd04',
packetType: 0x2F,
capturedAt: 1750000700000,
reason: 'undecodable_post_reboot',
));
expect((await LocalDb.rawArchiveStats())['count'], 6);

final health = await LocalDb.schemaHealth();
expect(health['ok'], isTrue, reason: '$health');
},
);
}
33 changes: 30 additions & 3 deletions test/raw_archive_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ void main() {
expect(await LocalDb.getCursorInt('counter_hw'), 1001);
});

test('re-flood of the same archived counter dedups (IGNORE on counter PK)', () async {
test('identical re-flood dedups on frame hex (missed-ACK redelivery)', () async {
final archive = ArchiveRecord(
counter: 2002, // same counter as above
counter: 2002, // same counter AND same bytes as above
hex: '2f63deadbeef',
packetType: 0x2F,
capturedAt: 1750000099999,
Expand All @@ -89,7 +89,7 @@ void main() {
trimToken: 'aabbccddeeff0022',
archives: [archive],
);
// Still one archived row — the re-delivery did not duplicate it.
// Still one archived row — same bytes, so the redelivery deduped.
final stats = await LocalDb.rawArchiveStats();
expect(stats['count'], 1);
// …but the trim cursor still advanced (this chunk was ACK-safe).
Expand All @@ -108,4 +108,31 @@ void main() {
expect(stats['count'], 2);
expect((stats['by_reason'] as Map)['undecodable_rec_v112'], 1);
});

test('two DISTINCT frames sharing a reused counter BOTH survive', () async {
// The strap resets its record counter to ~0 on reboot, so a post-reboot
// frame can reuse a pre-reboot counter while carrying different bytes.
// Under the old `counter INTEGER PRIMARY KEY` + IGNORE, the second frame
// was silently DROPPED — permanent loss in the table that exists precisely
// to never lose a frame. Content-keyed, both must survive.
final before = (await LocalDb.rawArchiveStats())['count'] as int;
const reusedCounter = 4004;
await LocalDb.archiveRawRecord(ArchiveRecord(
counter: reusedCounter,
hex: '2f63aaaaaaaa', // pre-reboot frame
packetType: 0x2F,
capturedAt: 1750000200000,
reason: 'undecodable_pre_reboot',
));
await LocalDb.archiveRawRecord(ArchiveRecord(
counter: reusedCounter, // SAME counter, DIFFERENT bytes
hex: '2f63bbbbbbbb', // post-reboot frame
packetType: 0x2F,
capturedAt: 1750000300000,
reason: 'undecodable_post_reboot',
));
final after = (await LocalDb.rawArchiveStats())['count'] as int;
// Pre-fix: +1 (second dropped by counter-PK IGNORE). Post-fix: +2.
expect(after - before, 2);
});
}
Loading