fix(db): re-key decoded ledger off volatile counter onto rec_ts (unrecoverable 1 Hz data loss)

[abdulsaheel]

User description

The bug (highest-severity: silent, unrecoverable data loss)

The strap resets its per-record counter to ~0 on every reboot, and
decoded_onehz was counter INTEGER PRIMARY KEY (+ UNIQUE(rec_ts)) written
with ConflictAlgorithm.replace. So a post-reboot record (counter=c,
rec_ts=T2) REPLACE-evicted a still-present pre-reboot row (counter=c,
rec_ts=T1) — silently deleting T1's only decoded 1 Hz row.

raw_records is dropped (db.dart has multiple DROP TABLE ... raw_records;
it is not a live ledger), so the decoded store is the sole system of record →
the eviction is unrecoverable. An in-code comment near the old orphan guard
already documented this exact hazard. No orphan-guard patch can restore an evicted
row — the only fix is to change the key.

The fix: re-key both decoded tables onto record time

• decoded_onehz PK → rec_ts. counter demoted to a NOT NULL forensic
  column with an index; it's still the keyset-cursor tiebreak (never fires now
  that rec_ts is unique).
• decoded_rr PK → (rec_ts, beat_index) (was (counter, beat_index)).
  rr_ts_ms (= rec_ts*1000) stays as the per-beat timestamp the compute worker
  reads.
• Write path per second: REPLACE decoded_onehz(rec_ts,…); DELETE FROM decoded_rr WHERE rec_ts=?; insert the beats. Parent and child now share the
  rec_ts key, so the counter-based _queueOrphanGuard and the prune
  orphan-sweep are deleted — they only ever papered over the counter-key
  mismatch, and a shrinking beat count can no longer strand stale high-index beats.

Caller audit (every counter-identity query rewritten — a counter join now

over-deletes / mis-reads since counter is no longer unique)

• decodedRrByCounterRange → decodedRrByRecTsRange: a clean, bounded PK
  range read. Drops the degraded counter-span fallback + decodedRrFallbackTruncations
  counter, which existed only because a reboot-straddling page read
  counter >= high AND counter <= low = zero rows (whole page's RR silently lost).
• derive_prepare.addDecodedPage groups RR by rec_ts, not counter (a
  counter reused within a page had mis-joined two seconds' beats).
• The counter-subquery deletes in deleteDays and pruneDecodedBeforeRecTs, the
  export copyRawRange, and the importFromDb decoded branch all select
  decoded_rr by rec_ts. Import derives rec_ts from rr_ts_ms for legacy
  (counter-keyed, no rec_ts) backups, so old exports still restore.
• Session HR joins (sessionHrStats/sessionHrSamplesBySession) already joined
  by rec_ts — unchanged.

Migration (v33, _rekeyDecodedStoreByRecTs)

• Rebuilds both decoded tables FROM THE EXISTING decoded tables only — never
  the tempting DROP + backfill-from-raw_records (that table is dropped, so it
  would zero the store: total loss). Rename-aside, deterministic newest-wins
  by rec_ts, idempotent (guards a re-run leaving _v33 temp tables).
• All copies are pure INSERT … SELECT (server-side, zero host-bound
  variables), so the iOS SQLITE_MAX_VARIABLE_NUMBER (999) never applies — no
  chunking needed.
• The fatal fallback was NOT used: raw-backfill is explicitly avoided.
• The frozen v11/v17/v19 decoded steps are made schema-adaptive (v17's counter
  rebuild is skipped when the store is already rec_ts-keyed; v19 converts before
  its rec_ts-keyed backfill) so the whole ladder still completes.

Schema-version note: base is origin/main at schemaVersion 31. PR #231
(pending, not merged) bumps to 32 with a raw_archive migration, so this uses
33 with an if (oldV < 33) block. A trivial schemaVersion rebase is
expected when they merge.

Invariants preserved

Durable ledger committed atomically before the trim ACK (commitSyncBatch's
single-transaction shape is unchanged beyond the key); decoded pruned only after
the covering day is derived; raw_archive never pruned; live streams never
persisted; day labels LOCAL; iOS value-returning PRAGMAs via rawQuery,
migrations stay in-openDatabase.

Tests

• Updated every test asserting the old counter-keyed behavior / orphan guard
  (db_p0_fixes_test, db_integrity_test, db_storage_hygiene_test,
  db_paged_import_export_test, local_persistence_test).
• Added the regression: pre-reboot (rec_ts=T1, counter=5) then post-reboot
  (rec_ts=T2, counter=5) → both decoded_onehz rows survive (pre-fix, T1
  was evicted), plus a shrink check that re-offloading a second with fewer beats
  strands none.
• Data-layer + derivation suites green; migration ladder v2→v33 completes.
  (Pre-existing UI/widget test failures are an unrelated Flutter-SDK/phosphor
  incompatibility in lib/theme/theme.dart, present on main.)

---

PR Type

Bug fix, Tests

---

Description

• Re-keys decoded_onehz PK from volatile counter to rec_ts, eliminating silent unrecoverable 1 Hz data loss on strap reboot

• Re-keys decoded_rr PK from (counter, beat_index) to (rec_ts, beat_index); removes counter-based orphan guard and orphan sweep, replacing with a single DELETE … WHERE rec_ts = ? before each beat insert

• Adds schema migration v33 (_rekeyDecodedStoreByRecTs) that rebuilds both decoded tables in-place; replaces decodedRrByCounterRange with decodedRrByRecTsRange and updates all callers (derivation engine, derive_prepare, deleteDays, prune, export, import)

• Updates all tests to use the new rec_ts-keyed API and adds regression coverage for the reboot-counter-reuse eviction scenario

---

Diagram Walkthrough

flowchart LR
  A["Strap reboot\n(counter resets to ~0)"]
  B["Old schema\ndecoded_onehz PK = counter\ndecoded_rr PK = (counter, beat_index)"]
  C["Post-reboot record\n(counter=c, rec_ts=T2)\nREPLACE evicts\npre-reboot row\n(counter=c, rec_ts=T1)"]
  D["T1's 1 Hz row\nPERMANENTLY LOST\n(raw_records dropped)"]
  E["New schema\ndecoded_onehz PK = rec_ts\ndecoded_rr PK = (rec_ts, beat_index)"]
  F["Migration v33\n_rekeyDecodedStoreByRecTs"]
  G["Write path\nREPLACE on rec_ts\nDELETE decoded_rr WHERE rec_ts=?\nbefore beat insert"]
  H["decodedRrByRecTsRange\n(replaces decodedRrByCounterRange)"]
  A -- "triggers" --> C
  B -- "caused" --> C
  C -- "results in" --> D
  E -- "applied via" --> F
  E -- "write path" --> G
  E -- "read path" --> H

Loading

File Walkthrough

	Relevant files
Bug fix	3 files db.dart Re-key decoded tables to rec_ts; add v33 migration; remove orphan guard +192/-214 derivation_engine.dart Switch RR page fetch from counter range to rec_ts range    +9/-6      derive_prepare.dart Group RR beats by rec_ts instead of counter in page accumulator +8/-7
Tests	5 files db_integrity_test.dart Update orphan and prune tests to rec_ts-keyed schema          +26/-29  db_p0_fixes_test.dart Rewrite reboot-counter and import regression tests for rec_ts key +81/-65  db_paged_import_export_test.dart Update orphan check and remove fallback truncation counter test +2/-7      db_storage_hygiene_test.dart Replace counter-index hygiene tests with rec_ts PK index tests +23/-69  local_persistence_test.dart Switch RR read assertion to decodedRrByRecTsRange API        +3/-3

---

[coderabbitai]

Warning

Review limit reached

@abdulsaheel, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 58320da7-c22e-44f4-8642-9bf5df03a6cf

📥 Commits

Reviewing files that changed from the base of the PR and between 6bddec4 and 161ded7.

📒 Files selected for processing (9)

• lib/compute/derivation_engine.dart
• lib/compute/derive_prepare.dart
• lib/data/db.dart
• test/db_integrity_test.dart
• test/db_migration_ladder_test.dart
• test/db_p0_fixes_test.dart
• test/db_paged_import_export_test.dart
• test/db_storage_hygiene_test.dart
• test/local_persistence_test.dart

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 161ded7)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Migration Gap (v32 skipped) The migration ladder jumps from oldV < 31 directly to oldV < 33, with a comment acknowledging that v32 is reserved for a pending PR. A user who upgrades through an intermediate build that ships v32 will have oldV == 32 and will never enter the oldV < 33 block, so _rekeyDecodedStoreByRecTs never runs for them. Their decoded store stays counter-keyed, silently. The comment says "a trivial schemaVersion rebase is expected when they merge," but if that rebase is forgotten the data-loss bug is re-introduced for anyone on the v32 intermediate. // NOTE: base is origin/main at schemaVersion 31. PR #231 (pending) bumps // to 32 with a raw_archive migration; this fix uses 33 so both land — a // trivial schemaVersion rebase is expected when they merge. if (oldV < 33) { // RE-KEY the decoded ledger off the volatile record `counter` onto // rec_ts. The counter resets to ~0 on every reboot, so counter-as-PK // let a post-reboot second REPLACE-evict a pre-reboot one — silently, // unrecoverably deleting a 1 Hz row (raw_records is dropped). await _rekeyDecodedStoreByRecTs(db); } _rekeyDecodedStoreByRecTs calls _createDecodedStore on existing tables _rekeyDecodedStoreByRecTs calls _createDecodedStore(db) at the top to ensure source tables exist. But _createDecodedStore now creates tables with the NEW rec_ts-keyed schema (rec_ts INTEGER PRIMARY KEY). If the existing tables are the OLD counter-keyed schema, CREATE TABLE IF NOT EXISTS is a no-op and the subsequent INSERT OR REPLACE INTO _decoded_onehz_v33 … SELECT rec_ts, counter, … FROM decoded_onehz works correctly. However, if the tables do NOT exist at all (pre-decoded-store upgrade path), _createDecodedStore creates them with the new schema, and then the INSERT SELECT from decoded_onehz (now new-schema, empty) into _decoded_onehz_v33 (also new-schema) succeeds as a no-op. The rename then replaces the newly-created tables with identical empty ones — harmless but wasteful. The real risk is that _rebuildCanonicalDecodedStore (called at v17 in the ladder) also calls _createDecodedStore now, but _rebuildCanonicalDecodedStore has its own guard that creates the OLD-schema tables inline before the rebuild. The interaction between these two code paths is fragile: if _createDecodedStore is called first (creating new-schema tables), then _rebuildCanonicalDecodedStore's CREATE TABLE IF NOT EXISTS for the old schema is a no-op, and the rebuild's SELECT on decoded_rr.counter will fail with "no such column: counter" on a fresh install going through the full ladder. The guard if (rrCols.isNotEmpty && !rrCols.any((c) => c['name'] == 'counter')) return; should catch this, but only if _createDecodedStore was already called before _rebuildCanonicalDecodedStore in the same upgrade session. await _createDecodedStore(db); Legacy import: decoded_onehz rows without rec_ts silently dropped In importFromDbFile, the legacy-export handling only patches decoded_rr rows that are missing rec_ts (deriving it from rr_ts_ms). But a legacy decoded_onehz row from the old counter-keyed schema has rec_ts as a regular column (not null), so it imports fine. However, if a legacy export's decoded_rr row has rr_ts_ms as null (corrupt or edge-case export), the expression ((row['rr_ts_ms'] as num).toInt()) ~/ 1000 will throw a null cast exception, causing the entire import batch to fail. The guard checks row['rr_ts_ms'] != null but the cast (row['rr_ts_ms'] as num) will still throw if the value is not a num. This is a narrow but realistic failure mode for corrupt exports. if (t == 'decoded_rr' && row['rec_ts'] == null && row['rr_ts_ms'] != null) { row['rec_ts'] = ((row['rr_ts_ms'] as num).toInt()) ~/ 1000;

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Strip legacy column before inserting into new schema When importing a legacy decoded_rr row (no rec_ts column), rec_ts is derived from rr_ts_ms. However, the batch.insert then inserts this row with conflictAlgorithm: replace into the new-schema decoded_rr table whose PRIMARY KEY is (rec_ts, beat_index). If the legacy row also carries a counter column (which the new schema does NOT have), SQLite will throw a "table has no column named counter" error, silently aborting the import or crashing. The legacy decoded_rr row must have its counter key stripped before insertion into the new-schema table. lib/data/db.dart [3969-3974] -if (t == 'decoded_rr' && - row['rec_ts'] == null && - row['rr_ts_ms'] != null) { - row['rec_ts'] = ((row['rr_ts_ms'] as num).toInt()) ~/ 1000; +if (t == 'decoded_rr') { + if (row['rec_ts'] == null && row['rr_ts_ms'] != null) { + row['rec_ts'] = ((row['rr_ts_ms'] as num).toInt()) ~/ 1000; + } + row.remove('counter'); // legacy exports carry counter; new schema does not } batch.insert(t, row, conflictAlgorithm: ConflictAlgorithm.replace); Suggestion importance[1-10]: 7 __ Why: This is a valid and important concern: a legacy decoded_rr export row carrying a counter column would cause a SQLite "table has no column named counter" error when inserted into the new rec_ts-keyed schema. The improved_code correctly strips the counter key before insertion, preventing a potential import crash for users with old exports.	Medium
General	Filter orphan beats during re-key migration _rekeyDecodedStoreByRecTs calls _createDecodedStore at the top, which creates the tables with the NEW rec_ts-keyed schema if they don't exist. However, on a normal upgrade path the OLD counter-keyed tables already exist, so CREATE TABLE IF NOT EXISTS is a no-op — that's fine. But on a path where the tables don't exist yet (pre-decoded-store origin), _createDecodedStore creates them with the NEW schema, and then the subsequent INSERT OR REPLACE INTO _decoded_onehz_v33 ... SELECT ... FROM decoded_onehz tries to select counter as a non-PK column from a table where rec_ts is already the PK and counter is NOT NULL — this works. The real problem is the _rebuildCanonicalDecodedStore guard: it checks rrCols.isNotEmpty && !rrCols.any((c) => c['name'] == 'counter') to skip the rebuild when already rec_ts-keyed. But _rekeyDecodedStoreByRecTs also calls _createDecodedStore first, which on a fresh path creates the new-schema tables — then the INSERT SELECT from decoded_onehz selects counter as a column, which exists in the new schema as NOT NULL. This is actually fine. However, the _rekeyDecodedStoreByRecTs is called from the oldV < 19 branch (which already called _rebuildCanonicalDecodedStore before this PR changed it to _rekeyDecodedStoreByRecTs), and also from oldV < 33. On the oldV < 19 path, _backfillDecodedStore runs after _rekeyDecodedStoreByRecTs — _backfillDecodedStore writes through _queueDecodedOneHz which now uses the new schema, so that's correct. The actual bug: _rekeyDecodedStoreByRecTs calls _createDecodedStore which may create decoded_onehz with the NEW schema (rec_ts PK), but then the INSERT SELECT copies from it — if it was just created empty, the copy is a no-op. That's safe. No critical bug here on this path. The real issue to flag: the decoded_rr migration in _rekeyDecodedStoreByRecTs derives rec_ts as rr_ts_ms / 1000 from the OLD decoded_rr, but the OLD decoded_rr may have orphan beats (counter with no decoded_onehz row) whose rr_ts_ms is from an evicted second. These orphan beats will be re-homed to their real second's rec_ts — but if that second's decoded_onehz row was already evicted, the beat will exist in the new decoded_rr with no parent in decoded_onehz, i.e. a new orphan. Add a WHERE EXISTS (SELECT 1 FROM decoded_onehz WHERE decoded_onehz.rec_ts = rr_ts_ms / 1000) filter to the decoded_rr INSERT to avoid importing orphan beats into the new schema. lib/data/db.dart [2301-2304] -await _createDecodedStore(db); -await db.execute('DROP TABLE IF EXISTS _decoded_onehz_v33'); -await db.execute('DROP TABLE IF EXISTS _decoded_rr_v33'); +await db.execute( + 'INSERT OR REPLACE INTO _decoded_rr_v33 (rec_ts, beat_index, rr_ts_ms, rr_ms) ' + 'SELECT rr_ts_ms / 1000, beat_index, rr_ts_ms, rr_ms ' + 'FROM decoded_rr ' + 'WHERE EXISTS (SELECT 1 FROM decoded_onehz WHERE decoded_onehz.rec_ts = decoded_rr.rr_ts_ms / 1000) ' + 'ORDER BY rr_ts_ms ASC, beat_index ASC', +); Suggestion importance[1-10]: 5 __ Why: The suggestion to filter orphan beats during the _rekeyDecodedStoreByRecTs migration has merit — orphan beats from the old schema could be re-homed to seconds with no parent row. However, the migration comment explicitly states "Pre-fix orphan beats (owning row evicted) re-home onto their real second here," suggesting this is intentional behavior. The improved code is accurate and would prevent creating new orphans, but the impact is limited since the subsequent _backfillDecodedStore and normal write path would handle consistency going forward.	Low

[abdulsaheel]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[abdulsaheel]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[abdulsaheel]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Head commit changed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[github-actions]

Persistent review updated to latest commit 161ded7

[github-actions]

PR Code Suggestions ✨

No code suggestions found for the PR.

[abdulsaheel]

Superseded by #235 — consolidated into the single integration/gen4-data-integrity branch per request. Same commits, same reviews; closing to keep one PR.