fix(db): stop raw_archive silently dropping distinct frames on a reused counter - #231
fix(db): stop raw_archive silently dropping distinct frames on a reused counter#231abdulsaheel wants to merge 2 commits into
Conversation
…ed counter raw_archive is the durable dead-letter box for undecodable frames — its whole purpose is to never lose a frame until we can decode it. But it was counter INTEGER PRIMARY KEY with IGNORE-on-conflict, and the strap resets its record counter to ~0 on every reboot. So a post-reboot frame that reused a still-present pre-reboot counter was silently DROPPED, even though its bytes were completely different data. Re-key the table off the volatile counter onto frame hex (content identity), exactly like events/band_events already do: an identical re-flood (missed-ACK redelivery) still dedups, but two genuinely distinct frames survive a counter collision. counter is retained as a plain forensic column. v32 migration rebuilds the table preserving every existing row (their counters are unique, so the content-keyed copy loses nothing). Guarded because raw_archive is created lazily in onOpen, not the ladder, so an old DB may not have it yet at migration time — then a fresh hex-keyed create is all that's needed. Adds a regression test: two distinct frames sharing a reused counter both survive (previously the second was lost).
|
Warning Review limit reached
Next review available in: 16 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe database schema version increases to 32. The ChangesRaw archive identity
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Reviewer Guide 🔍(Review updated until commit 52ee804)Here are some key observations to aid the review process:
|
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@lib/data/db.dart`:
- Around line 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: de74c0bd-7b03-4889-aae6-634e9ca79045
📒 Files selected for processing (2)
lib/data/db.darttest/raw_archive_test.dart
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ 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
…-PK DB The v32 migration (RENAME → drop-index → hex-PK create → INSERT OR IGNORE SELECT → drop-old) had no coverage — the archive test only exercises the fresh onCreate schema, and the ladder test never touched raw_archive. Seed a populated v31 counter-PK table, open it through the REAL ladder, and assert: distinct frames survive, an exact-duplicate hex collapses (5 rows → 4), a reused counter no longer drops a distinct frame (hex-PK proven end-to-end), and an identical re-flood still dedups on content.
|
Persistent review updated to latest commit 52ee804 |
PR Code Suggestions ✨Explore these optional code suggestions:
|
|
Superseded by #235 — consolidated into the single |
1 participant
User description
The bug
`raw_archive` is the durable dead-letter box for undecodable historical frames — its entire purpose is to never lose a frame until a future firmware/decoder can interpret it (`db.dart` comment: "future firmware's records survive until we understand the format").
But it was keyed `counter INTEGER PRIMARY KEY` with `ConflictAlgorithm.ignore`, and the strap resets its record counter to ~0 on every reboot. So when a post-reboot frame reused a counter value still held by a pre-reboot row, the insert hit the PK conflict and was silently dropped — even though its bytes were entirely different data. Silent permanent loss, in the one table that must never lose anything.
An in-code comment already documents the same "counter resets on reboot → collision" hazard for `decoded_onehz`; this is the same root cause in `raw_archive`.
The fix
Re-key the table off the volatile counter onto frame `hex` (content identity) — exactly the pattern `events`/`band_events` already use:
`counter` is kept as a plain forensic column. Nothing reads `raw_archive` by counter (only inserts + `COUNT(*)`/`GROUP BY reason` diagnostics), so the re-key touches no read path.
v32 migration rebuilds the table preserving every existing row (existing counters are unique, so the content-keyed copy loses nothing; at most it collapses an exact-duplicate hex, which is the dedup we want). It's guarded: `raw_archive` is created lazily in `onOpen`/`_repairOpenSchema`, not the upgrade ladder, so an old DB may not have the table yet at migration time — then a fresh hex-keyed create is all that's needed. The old index name is dropped before the fresh `CREATE INDEX` to avoid the leaked-index-name collision documented on the decoded rebuild.
Tests
Context
First of a short series of P0 data-integrity fixes surfaced by a deep review of the gen4 storage/BLE path. This is the smallest, most self-contained one. Larger follow-ups (the `decoded_onehz`/`decoded_rr` `rec_ts`-PK re-key that fixes the unrecoverable 1Hz eviction, the ACK-gating `synchronous=FULL` durability bracket, and the clock-skew drop-then-trim fix) will come as separate focused PRs.
PR Type
Bug fix, Tests
Description
raw_archivere-keyed fromcounterPK tohexPK, preventing silent frame loss on counter reuse after strap rebootSchema bumped to v32 with a safe migration that preserves all existing rows
Regression test added: two distinct frames sharing a reused counter both survive
Diagram Walkthrough
File Walkthrough
db.dart
Re-key raw_archive on hex PK, add v32 migrationlib/data/db.dart
schemaVersionbumped from 31 to 32_createRawArchivechanged:hex TEXT PRIMARY KEY,counter INTEGER(non-PK forensic column)
hex-keyed table, copies rows with
INSERT OR IGNORE, drops old table;guarded for DBs where
raw_archivedoesn't exist yet_createRawArchiveexplaining the counter-reusehazard and content-keyed fix
raw_archive_test.dart
Add regression test for counter-reuse frame losstest/raw_archive_test.dart
(missed-ACK redelivery), not counter-PK dedup
counter both survive (previously the second was silently dropped)
Summary by CodeRabbit
Bug Fixes
Tests