offload data loss fixes - #235
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).
… received-total signal) Emit an honest, observation-only frame-loss signal at HISTORY_END without touching the commit/ACK decision. The band's num_packets counts every frame it transmitted (all types); the correct completeness comparison is against totalTrafficPacketCount — the all-types received total — not the banked R24 subset (which fabricates a shortfall whenever console/event frames ride along un-banked). This is type-agnostic and interleaving-immune. New pure helper burstPacketShortfall() = expected - (received_all_types + dropped_this_burst): a POSITIVE result is frames the band sent that never reached us (true loss); zero is complete; negative is retries/dupes, not loss. Gate-dropped (RecordGate) records are added back so plausibility rejections never read as radio loss. At burst end we now log a "would-flag" line and stamp burst_shortfall into the existing mismatch ledger entry — LOG-ONLY. Commit-before-ACK, the verbatim token echo, and the OK/FAIL decision are all unchanged. This is groundwork so we can SEE true frame loss in telemetry before ever wiring a field-validated FAIL gate; a hard FAIL/re-flood path is deliberately NOT included here. Rejected alternative: gating on the per-revision counter gap — the counter is a GLOBAL flash-log index sliced per revision, so gaps are the normal state and would false-positive constantly. Adds pure unit tests covering benign interleaving (no false positive), true loss, the gate-dropped add-back, negative/retry case, and shortfall==0 == burstPacketCountMatches.
The DB runs WAL + synchronous=NORMAL, under which a commit is durable only at the next checkpoint, not at commit. commitSyncBatch persists the sync batch (raw_archive + samples + decoded + trim cursor) and returns; the caller then writes the BLE batch-ACK and the band trims its flash. A kernel panic / battery-yank AFTER the ACK but BEFORE the -wal is checkpointed lost those just-committed rows from the phone while they were already gone from the band. The commit-before-ACK ordering held; the durability did not. Raise durability to synchronous=FULL (fsync AT commit) for this one commit only, leaving every other path at NORMAL — they are all recomputable and FULL everywhere is brutally slow. synchronous is per-connection and cannot change mid-transaction, so it is set BEFORE db.transaction opens and reset to NORMAL in a finally (a leaked FULL would fsync every later write on the connection forever). Both the main and background-isolate drains funnel through commitSyncBatch, each on its own connection, so this single bracket covers both. PRAGMA synchronous returns no rows -> execute(), kept non-fatal like the open-time PRAGMAs. Adds a focused test (spies the FULL/NORMAL SQL bracket and reads resting PRAGMA synchronous) covering both a normal commit and a throwing one.
A positive burst shortfall means frames the band counted that we did not count as valid received traffic. CRC-failed frames also never enter currentBurstTrafficCount, so a positive shortfall can be missing OR corrupted traffic — it cannot by itself prove a frame never arrived. Soften the helper doc, the would-flag log text, and the test name accordingly. Wording-only; no behavior change (still log-only).
The strap resets its per-record `counter` to ~0 on every reboot, and `decoded_onehz` was `counter INTEGER PRIMARY KEY`. 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. Because `raw_records` is dropped (not a live ledger), the decoded store is the sole system of record, making the eviction UNRECOVERABLE. No orphan-guard patch can restore an evicted row — the key itself has to change. Re-key both decoded tables onto record time: - decoded_onehz PK -> rec_ts; `counter` demoted to a NOT NULL forensic column (+ index), still the keyset-cursor tiebreak (never fires now rec_ts is unique). - decoded_rr PK -> (rec_ts, beat_index); rr_ts_ms kept as the beat timestamp. - Write path per second: REPLACE decoded_onehz(rec_ts,...); DELETE decoded_rr by rec_ts; insert the beats. Parent and child now share the rec_ts key, so the counter-based orphan guard and the prune orphan-sweep are deleted — a shrinking beat count can no longer strand stale high-index beats. Caller audit (every counter-identity query rewritten to rec_ts): - decodedRrByCounterRange -> decodedRrByRecTsRange (a clean PK range read; drops the degraded counter-span fallback + truncation counter that only existed to paper over the reboot reset). - derive_prepare.addDecodedPage groups RR by rec_ts, not counter (a counter reuse within a page had mis-joined two seconds' beats). - deleteDays / pruneDecodedBeforeRecTs / export copyRawRange / importFromDb all select decoded_rr by rec_ts; import derives rec_ts from rr_ts_ms for legacy (counter-keyed, no rec_ts) backups. Migration v33 (`_rekeyDecodedStoreByRecTs`): rebuilds BOTH decoded tables FROM THE EXISTING decoded tables only (never from the dropped raw_records — that would zero the store), rename-aside, deterministic newest-wins by rec_ts, idempotent, pure INSERT..SELECT so the iOS 999-var limit never applies. The frozen v11/v17/v19 steps are made schema-adaptive so the ladder still completes. NOTE: base is origin/main at schemaVersion 31; PR #231 (pending) bumps to 32, so this uses 33 — a trivial schemaVersion rebase is expected when they merge.
The headless drain (background_sync.dart) is the iOS CoreBluetooth-restoration recovery path and runs in the MAIN isolate on the same shared _db connection — not a separate per-isolate connection as the prior comment claimed. The bracket is safe not because of isolation but because BandOwnership + the single-flight offload processor guarantee the two drains never overlap on one connection. Document that as the load-bearing invariant so a future concurrent caller does not silently defeat the FULL window.
…-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.
The v33 re-key touches frozen migration steps (v11/v17/v19), but no ladder test seeded a genuinely OLD counter-keyed decoded store. The riskiest path is a user installed at v19..31: raw_records is already dropped by then, so the rekey is the SOLE copy of their 1 Hz data with no raw-backfill safety net. Seed that exact origin/main schema at v31, run the real ladder, and assert every second/beat survives, counter is preserved as the forensic column, the PK moved to rec_ts, and no temp tables leak.
# Conflicts: # lib/data/db.dart # test/db_migration_ladder_test.dart
📝 WalkthroughWalkthroughThe PR adds phone-clock-aware BLE history deferral and burst shortfall telemetry. It also migrates decoded and raw archive storage from counter-based identity to timestamp or frame-content identity, updates compute and database flows, and adds migration, durability, and integrity tests. ChangesBLE reliability telemetry
Timestamp-keyed persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BleEngine
participant Strap
participant ClockPolicy
BleEngine->>Strap: Read strap clock
Strap-->>BleEngine: Return RTC timestamp
BleEngine->>ClockPolicy: Evaluate phoneClockSuspect
ClockPolicy-->>BleEngine: Return clock state
BleEngine->>BleEngine: Defer or resume history offload
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 Code Suggestions ✨Latest suggestions up to 997e149 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 8573d7e
Suggestions up to commit 90f9588
Suggestions up to commit 25a75f4
|
…clock-skew P1) The plausibility gate used the phone wall clock as ground truth. If the phone clock ran >1 day slow (dead-battery reboot, bad NTP, manual set-back), the strap's correctly-stamped records read as 'implausibly future', got dropped, and a mixed-burst ACK then TRIMMED them off the band — silent, permanent loss. Option A (trust the strap's GET_DATA_RANGE window instead) can't work: that window is itself discarded via isCorruptFutureRtc against the same wrong phone clock, so it's unavailable exactly when needed. Fix (option D): don't drain-and-trim under an untrustworthy clock. Before each history refresh, read the strap RTC and compare; if it reads a PLAUSIBLE time but >1 day ahead of the phone (ClockPolicy.phoneClockSuspect), the phone clock is likely slow, so DEFER the offload — the strap retains every record until the clocks agree (the phone almost always self-corrects via NTP within minutes). SET_CLOCK is deliberately NOT issued in this case: pushing the strap back to the slow phone would corrupt a correct RTC. The strap-behind and unset-RTC cases are unchanged (still corrected forward by shouldSetClock); only the future-skew case defers. Exposes historyPausedForClock for the UI so the pause is visible. Adds ClockPolicy.phoneClockSuspect unit coverage (agree / future-skew / behind / unset boundaries).
|
Persistent review updated to latest commit 90f9588 |
|
@CodeRabbit 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/db_p0_fixes_test.dart (1)
396-432: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a shrinking-beat-count case to this import fixture.
The foreign export supplies three beats for
collideTsand the local row also has three, so everybeat_indexis replaced and the assertion on line 414 passes.The import path merges
decoded_rrwithINSERT OR REPLACEper row and performs no delete for the second, unlike_queueDecodedOneHz. A foreign export with fewer beats for a colliding second would leave the local high-index beats in place.See the consolidated comment on
lib/data/db.dartfor the root cause.🤖 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 `@test/db_p0_fixes_test.dart` around lines 396 - 432, Extend the import fixture around the collideTs case to cover a foreign export with fewer beats than the local second, while retaining the existing collision assertions. Assert that the imported beat set exactly matches the foreign beats and that no higher-index local beats remain, then keep the orphan and timestamp consistency checks intact.
🤖 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/ble/ble_engine.dart`:
- Around line 1602-1622: Gate every history-start path on a session-bound,
completed GET_CLOCK response rather than the fixed delay and cached
_phoneClockSuspect flag: update _startHistoricalRefresh and the initial
connection flow around setClock/sendInit to await and apply the response before
any SET_CLOCK or historical-data trigger. Ensure delayed or missing responses
cannot proceed, preserve the defer behavior for a suspect phone clock, and add
regressions covering responses arriving after 120 ms and the first-connection
path.
In `@lib/compute/derivation_engine.dart`:
- Around line 1839-1849: Update the algorithm version constant kAlgoVersion from
62 to the next version, and add a changelog entry documenting the decoded RR
lookup change in the derivation engine so finalized days are recalculated with
RR data.
In `@lib/data/db.dart`:
- Around line 4041-4050: The import path in lib/data/db.dart lines 4041-4050
must replace each collided decoded_rr beat set rather than patching it: queue a
DELETE for every rec_ts represented by the page before its inserts, using the
same batch and transaction, and revise the comment to reflect that guard. Extend
test/db_p0_fixes_test.dart lines 396-432 so collideTs has fewer foreign beats
than local beats and assert only the foreign beat set remains.
- Around line 4041-4050: Update the decoded_rr legacy rec_ts derivation to
validate rr_ts_ms with the existing numeric-conversion approach used by
_PrepareAccumulator._num before converting it; only derive row['rec_ts'] for
values that are safely numeric, and avoid throwing for non-numeric strings
during the transaction.
- Around line 2537-2554: Update _queueDecodedOneHz to resolve recTs through the
existing _recTsFor fallback instead of using raw.recTs ?? decoded.tsEpoch, so an
explicit raw.recTs value of 0 falls back to decoded.tsEpoch before insertion
into decoded_onehz. Preserve nonzero stored timestamps unchanged.
In `@pubspec.yaml`:
- Around line 263-266: Update the sqflite_common dependency declaration used by
the ACK commit sync test to pin an exact version whose experimental
SqfliteDatabaseFactoryLogger constructor has been tested, or replace that
constructor usage with a stable logging mechanism. Keep the existing logger
symbols and test behavior otherwise unchanged.
In `@test/db_storage_hygiene_test.dart`:
- Around line 42-65: Update the test `rec_ts-range reads on decoded_rr are
served by the PK auto-index` to remove the assertion for the internal
`sqlite_autoindex_decoded_rr_1` name. Assert that the uppercased query-plan
detail contains `SEARCH`, does not contain `USE TEMP B-TREE`, and does not match
`SCAN TABLE DECODED_RR`, while preserving the existing planner-fallback
diagnostics.
---
Outside diff comments:
In `@test/db_p0_fixes_test.dart`:
- Around line 396-432: Extend the import fixture around the collideTs case to
cover a foreign export with fewer beats than the local second, while retaining
the existing collision assertions. Assert that the imported beat set exactly
matches the foreign beats and that no higher-index local beats remain, then keep
the orphan and timestamp consistency checks intact.
🪄 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: 8e60a82a-5d6e-44d5-99e5-580abc10e1b7
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
lib/ble/ble_engine.dartlib/compute/derivation_engine.dartlib/compute/derive_prepare.dartlib/data/db.dartlib/sync/sync_policy.dartpubspec.yamltest/ack_commit_sync_full_test.darttest/ble_engine_test.darttest/db_integrity_test.darttest/db_migration_ladder_test.darttest/db_p0_fixes_test.darttest/db_paged_import_export_test.darttest/db_storage_hygiene_test.darttest/local_persistence_test.darttest/raw_archive_test.darttest/sync_policy_test.dart
| // The page is ordered rec_ts ASC, so first = min second, last = max. | ||
| // decoded_rr shares the rec_ts key, so this pulls exactly the page's | ||
| // beats — no counter span (which broke across the strap's reboot reset). | ||
| final firstRecTs = (decodedRows.first['rec_ts'] as num?)?.toInt(); | ||
| final lastRecTs = (decodedRows.last['rec_ts'] as num?)?.toInt(); | ||
| final rrRows = firstRecTs == null || lastRecTs == null | ||
| ? const <Map<String, dynamic>>[] | ||
| : await LocalDb.decodedRrByCounterRange( | ||
| fromCounter: firstCounter, | ||
| toCounter: lastCounter, | ||
| : await LocalDb.decodedRrByRecTsRange( | ||
| fromRecTs: firstRecTs, | ||
| toRecTs: lastRecTs, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether kAlgoVersion changed in this PR and whether a changelog entry accompanies it.
set -euo pipefail
echo "== kAlgoVersion declaration =="
rg -nP --type=dart -C4 '\bkAlgoVersion\s*=' || echo "not found"
echo "== kAlgoVersion changes in this branch vs base =="
git diff origin/HEAD... -- '*.dart' 2>/dev/null | rg -n -C4 'kAlgoVersion' || echo "no diff hunks touching kAlgoVersion"
echo "== changelog files =="
fd -i -t f 'changelog' | head -20
echo "== changelog diff =="
git diff origin/HEAD... -- '*CHANGELOG*' 2>/dev/null | head -60 || echo "no changelog diff"Repository: OpenStrap/edge
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate analytics/version files =="
git ls-files | rg '(^|/)(derivation_engine\.dart|.*CHANGELOG.*|changelog.*)$|(^|/)pubspec\.yaml$'
echo "== Version declarations and changelog entries =="
rg -n -i -C3 'kAlgoVersion|algo.?version|changelog|change log' --glob '*.dart' --glob '*.md' --glob '*.yaml' . || true
echo "== Repository refs and diff summary =="
git branch --all --no-color
git diff --stat
git diff --stat origin/HEAD...HEAD 2>/dev/null || true
echo "== Relevant derivation-engine diff =="
git diff -- lib/compute/derivation_engine.dartRepository: OpenStrap/edge
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Revisions =="
git rev-parse --show-toplevel
git rev-parse HEAD
git rev-parse origin/HEAD 2>/dev/null || true
echo "== Version/changelog section =="
sed -n '250,710p' lib/compute/derivation_engine.dart
echo "== Current change around the reviewed code =="
sed -n '1800,1870p' lib/compute/derivation_engine.dart
echo "== Version-related diff only =="
git diff --unified=3 origin/HEAD...HEAD -- lib/compute/derivation_engine.dart 2>/dev/null \
| rg -n -C5 'kAlgoVersion|^[-+].*// v[0-9]+|^[-+].*version' || true
echo "== Reviewed-code diff only =="
git diff --unified=8 origin/HEAD...HEAD -- lib/compute/derivation_engine.dart 2>/dev/null \
| rg -n -C12 'decodedRrByRecTsRange|counter|decodedRows' || trueRepository: OpenStrap/edge
Length of output: 36432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
import subprocess
path = "lib/compute/derivation_engine.dart"
for label, rev in (("base", "origin/HEAD"), ("head", "HEAD")):
text = subprocess.check_output(["git", "show", f"{rev}:{path}"], text=True)
m = re.search(r"const\s+int\s+kAlgoVersion\s*=\s*(\d+)\s*;", text)
print(f"{label}: kAlgoVersion={m.group(1) if m else 'not found'}")
PY
echo "== Derivation gate around finalized-day selection =="
sed -n '1100,1160p' lib/compute/derivation_engine.dart
sed -n '1310,1360p' lib/compute/derivation_engine.dart
sed -n '1900,1950p' lib/compute/derivation_engine.dart
echo "== Finalized-day lookup implementation =="
rg -n -C8 'finalizedDayIds|dayResultIds' lib/data/db.dart lib/compute/derivation_engine.dart
echo "== RR range implementations =="
rg -n -C12 'decodedRrByRecTsRange|decodedRrByCounterRange' lib/data/db.dart lib/compute/derivation_engine.dartRepository: OpenStrap/edge
Length of output: 22826
Bump kAlgoVersion and add a changelog entry for the RR lookup change. Both base and head remain at version 62. Finalized days at version 62 will retain RR-less RMSSD, HRV, and readiness results.
🤖 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/compute/derivation_engine.dart` around lines 1839 - 1849, Update the
algorithm version constant kAlgoVersion from 62 to the next version, and add a
changelog entry documenting the decoded RR lookup change in the derivation
engine so finalized days are recalculated with RR data.
Source: Coding guidelines
| static int _queueDecodedOneHz(Batch batch, RawRecord raw, Sample? sample) { | ||
| final decoded = _decodeOneHzSample(raw, preferred: sample); | ||
| if (decoded == null) return 0; | ||
| final recTs = raw.recTs ?? decoded.tsEpoch; | ||
| // TIME-KEYED, NEWEST-WINS (noop/WHOOP-4 model: dedupe records by their | ||
| // embedded timestamp, not by a counter). decoded_onehz has a UNIQUE(rec_ts) | ||
| // index and decoded_rr a UNIQUE(rr_ts_ms, beat_index). We use REPLACE, not | ||
| // IGNORE: the strap's record `counter` RESETS to ~0 on every reboot, so a | ||
| // post-reboot record whose second already had a row would be SILENTLY DROPPED | ||
| // under IGNORE — quarantining everything after a reboot (observed: whole days | ||
| // present in raw_records but absent from the decoded substrate the engine | ||
| // reads → "not worn / metrics still computing / strain –"). REPLACE lets the | ||
| // freshly-offloaded record for a given second win, which is what we want. | ||
| // | ||
| // ORPHAN GUARD: decoded_rr rows are keyed by their record's own counter. When | ||
| // the REPLACE below evicts a DIFFERENT counter's row for this second, that | ||
| // loser's RR beats would stay behind under a counter with no decoded_onehz | ||
| // row — invisible to the counter-joined prune (permanent leak). The winner's | ||
| // REPLACE on UNIQUE(rr_ts_ms, beat_index) only overwrites overlapping beat | ||
| // indexes, so delete the evicted counter's beats explicitly, in the same | ||
| // batch/transaction (mirrors the v17 rebuild's decoded_onehz join). | ||
| // embedded timestamp, not by the volatile counter). decoded_onehz is keyed | ||
| // by rec_ts and decoded_rr by (rec_ts, beat_index). We use REPLACE, not | ||
| // IGNORE: a freshly-offloaded record for a given second should win over a | ||
| // stale one. Because rec_ts is the key, the strap's per-reboot counter reset | ||
| // can no longer make one second's record evict another's (the pre-fix | ||
| // counter-PK eviction that silently, unrecoverably deleted 1 Hz rows). | ||
| // | ||
| // …AND the COUNTER-PK eviction, which the guard used to miss entirely. | ||
| // `decoded_onehz` is `counter INTEGER PRIMARY KEY` as well as | ||
| // UNIQUE(rec_ts), and (per the comment above) the strap's counter RESETS to | ||
| // ~0 on every reboot — so this same REPLACE also silently DELETES the row | ||
| // of an OLDER SECOND that happened to reuse this counter. That older | ||
| // second's beats live under OUR counter carrying ITS rr_ts_ms, and only the | ||
| // overlapping beat_indexes get overwritten below: any beat at an index past | ||
| // the new record's beat count SURVIVES, still stamped days earlier. Neither | ||
| // prune path can ever see it (the counter-join finds a fresh rec_ts; the | ||
| // orphan sweep finds the counter present), so a later page's RR series was | ||
| // polluted with beats from another day — silently wrecking RMSSD/HRV. | ||
| // Drop every beat under this counter that is not stamped with THIS second. | ||
| var ops = _queueOrphanGuard(batch, counter: raw.counter, recTs: recTs); | ||
| // Clear this second's RR beats before reinserting so a SHRINKING beat count | ||
| // can't strand stale high-index beats — the parent+child share the rec_ts | ||
| // key, so this single DELETE replaces the old counter-based orphan guard. | ||
| batch.insert('decoded_onehz', { | ||
| 'counter': raw.counter, | ||
| 'rec_ts': recTs, | ||
| 'counter': raw.counter, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Guard rec_ts against an explicit 0 before it becomes the primary key.
Line 2540 uses raw.recTs ?? decoded.tsEpoch, which substitutes only on null. _backfillDecodedStore (line 2600) builds RawRecord.recTs from the stored raw_records.rec_ts column, which is NOT NULL DEFAULT 0 for legacy rows. Every such row now writes rec_ts = 0.
Under the previous counter primary key those rows coexisted. Under the rec_ts primary key they REPLACE each other, so the backfill keeps only the last one. firstAndLastRecordTs and rawStats already filter rec_ts > 0, which documents that 0 is a real stored value.
Reuse the existing _recTsFor fallback so a 0 resolves to the decoded timestamp.
🐛 Proposed fix
- final recTs = raw.recTs ?? decoded.tsEpoch;
+ // `?? ` substitutes on null only; an explicit 0 (legacy raw_records rows
+ // carry `rec_ts NOT NULL DEFAULT 0`) would become the rec_ts PRIMARY KEY
+ // and REPLACE-evict every other undated row.
+ final rawRecTs = raw.recTs;
+ final recTs =
+ (rawRecTs != null && rawRecTs > 0) ? rawRecTs : decoded.tsEpoch;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static int _queueDecodedOneHz(Batch batch, RawRecord raw, Sample? sample) { | |
| final decoded = _decodeOneHzSample(raw, preferred: sample); | |
| if (decoded == null) return 0; | |
| final recTs = raw.recTs ?? decoded.tsEpoch; | |
| // TIME-KEYED, NEWEST-WINS (noop/WHOOP-4 model: dedupe records by their | |
| // embedded timestamp, not by a counter). decoded_onehz has a UNIQUE(rec_ts) | |
| // index and decoded_rr a UNIQUE(rr_ts_ms, beat_index). We use REPLACE, not | |
| // IGNORE: the strap's record `counter` RESETS to ~0 on every reboot, so a | |
| // post-reboot record whose second already had a row would be SILENTLY DROPPED | |
| // under IGNORE — quarantining everything after a reboot (observed: whole days | |
| // present in raw_records but absent from the decoded substrate the engine | |
| // reads → "not worn / metrics still computing / strain –"). REPLACE lets the | |
| // freshly-offloaded record for a given second win, which is what we want. | |
| // | |
| // ORPHAN GUARD: decoded_rr rows are keyed by their record's own counter. When | |
| // the REPLACE below evicts a DIFFERENT counter's row for this second, that | |
| // loser's RR beats would stay behind under a counter with no decoded_onehz | |
| // row — invisible to the counter-joined prune (permanent leak). The winner's | |
| // REPLACE on UNIQUE(rr_ts_ms, beat_index) only overwrites overlapping beat | |
| // indexes, so delete the evicted counter's beats explicitly, in the same | |
| // batch/transaction (mirrors the v17 rebuild's decoded_onehz join). | |
| // embedded timestamp, not by the volatile counter). decoded_onehz is keyed | |
| // by rec_ts and decoded_rr by (rec_ts, beat_index). We use REPLACE, not | |
| // IGNORE: a freshly-offloaded record for a given second should win over a | |
| // stale one. Because rec_ts is the key, the strap's per-reboot counter reset | |
| // can no longer make one second's record evict another's (the pre-fix | |
| // counter-PK eviction that silently, unrecoverably deleted 1 Hz rows). | |
| // | |
| // …AND the COUNTER-PK eviction, which the guard used to miss entirely. | |
| // `decoded_onehz` is `counter INTEGER PRIMARY KEY` as well as | |
| // UNIQUE(rec_ts), and (per the comment above) the strap's counter RESETS to | |
| // ~0 on every reboot — so this same REPLACE also silently DELETES the row | |
| // of an OLDER SECOND that happened to reuse this counter. That older | |
| // second's beats live under OUR counter carrying ITS rr_ts_ms, and only the | |
| // overlapping beat_indexes get overwritten below: any beat at an index past | |
| // the new record's beat count SURVIVES, still stamped days earlier. Neither | |
| // prune path can ever see it (the counter-join finds a fresh rec_ts; the | |
| // orphan sweep finds the counter present), so a later page's RR series was | |
| // polluted with beats from another day — silently wrecking RMSSD/HRV. | |
| // Drop every beat under this counter that is not stamped with THIS second. | |
| var ops = _queueOrphanGuard(batch, counter: raw.counter, recTs: recTs); | |
| // Clear this second's RR beats before reinserting so a SHRINKING beat count | |
| // can't strand stale high-index beats — the parent+child share the rec_ts | |
| // key, so this single DELETE replaces the old counter-based orphan guard. | |
| batch.insert('decoded_onehz', { | |
| 'counter': raw.counter, | |
| 'rec_ts': recTs, | |
| 'counter': raw.counter, | |
| static int _queueDecodedOneHz(Batch batch, RawRecord raw, Sample? sample) { | |
| final decoded = _decodeOneHzSample(raw, preferred: sample); | |
| if (decoded == null) return 0; | |
| // `?? ` substitutes on null only; an explicit 0 (legacy raw_records rows | |
| // carry `rec_ts NOT NULL DEFAULT 0`) would become the rec_ts PRIMARY KEY | |
| // and REPLACE-evict every other undated row. | |
| final rawRecTs = raw.recTs; | |
| final recTs = | |
| (rawRecTs != null && rawRecTs > 0) ? rawRecTs : decoded.tsEpoch; | |
| // TIME-KEYED, NEWEST-WINS (noop/WHOOP-4 model: dedupe records by their | |
| // embedded timestamp, not by the volatile counter). decoded_onehz is keyed | |
| // by rec_ts and decoded_rr by (rec_ts, beat_index). We use REPLACE, not | |
| // IGNORE: a freshly-offloaded record for a given second should win over a | |
| // stale one. Because rec_ts is the key, the strap's per-reboot counter reset | |
| // can no longer make one second's record evict another's (the pre-fix | |
| // counter-PK eviction that silently, unrecoverably deleted 1 Hz rows). | |
| // | |
| // Clear this second's RR beats before reinserting so a SHRINKING beat count | |
| // can't strand stale high-index beats — the parent+child share the rec_ts | |
| // key, so this single DELETE replaces the old counter-based orphan guard. | |
| batch.insert('decoded_onehz', { | |
| 'rec_ts': recTs, | |
| 'counter': raw.counter, |
🤖 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 2537 - 2554, Update _queueDecodedOneHz to
resolve recTs through the existing _recTsFor fallback instead of using raw.recTs
?? decoded.tsEpoch, so an explicit raw.recTs value of 0 falls back to
decoded.tsEpoch before insertion into decoded_onehz. Preserve nonzero stored
timestamps unchanged.
| // Both decoded tables are now keyed by rec_ts, so a plain | ||
| // replace-insert merges cleanly (foreign-wins on a rec_ts | ||
| // collision) — no orphan guard needed. A LEGACY export's | ||
| // decoded_rr carries no rec_ts column; derive it from rr_ts_ms | ||
| // (= rec_ts*1000) so the NOT NULL PK column is always populated. | ||
| if (t == 'decoded_rr' && | ||
| row['rec_ts'] == null && | ||
| row['rr_ts_ms'] != null) { | ||
| row['rec_ts'] = ((row['rr_ts_ms'] as num).toInt()) ~/ 1000; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A shrinking foreign beat set leaves stale local beats on import. The comment on lib/data/db.dart line 4041 states that a plain replace-insert merges cleanly and that no orphan guard is needed. That holds only when the foreign export supplies at least as many beats for a colliding rec_ts as the local database already has. The import writes decoded_rr row by row with ConflictAlgorithm.replace keyed on (rec_ts, beat_index), so it never removes a local beat whose beat_index the foreign export does not reach. _queueDecodedOneHz guards the same hazard on the write path with DELETE FROM decoded_rr WHERE rec_ts = ? before reinserting. The import path has no equivalent, so a restore can produce one second holding a mix of foreign and stale local beats, which corrupts RMSSD for that second.
lib/data/db.dart#L4041-L4050: before inserting a page'sdecoded_rrrows, delete the existing beats for eachrec_tsthe page carries, queued into the same batch and the same transaction as the inserts, so the second's beat set is replaced rather than patched. Then correct the comment, which currently asserts that no guard is needed.test/db_p0_fixes_test.dart#L396-L432: extend the fixture so the foreign export supplies fewer beats forcollideTsthan the local row has (for example foreign[500]against local[700, 710, 720]), and assert that the collided second ends with exactly the foreign beat set.
📍 Affects 2 files
lib/data/db.dart#L4041-L4050(this comment)test/db_p0_fixes_test.dart#L396-L432
🤖 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 4041 - 4050, The import path in
lib/data/db.dart lines 4041-4050 must replace each collided decoded_rr beat set
rather than patching it: queue a DELETE for every rec_ts represented by the page
before its inserts, using the same batch and transaction, and revise the comment
to reflect that guard. Extend test/db_p0_fixes_test.dart lines 396-432 so
collideTs has fewer foreign beats than local beats and assert only the foreign
beat set remains.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Type-check rr_ts_ms before the as num cast.
SQLite storage class is per value, not per column, so a foreign or older export can return a String where rr_ts_ms is declared INTEGER. The guard on line 4048 tests for null only. A non-numeric value then throws inside db.transaction and aborts the whole restore.
This file already documents the same hazard for decoded-page reads (_PrepareAccumulator._num in lib/compute/derive_prepare.dart). Apply the same defence here.
🛡️ Proposed fix
- 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' && row['rec_ts'] == null) {
+ // Storage class is per-VALUE in SQLite: a foreign export can
+ // hand back a String where INTEGER is declared. Skip the row
+ // rather than throwing out of the whole import transaction.
+ final ms = row['rr_ts_ms'];
+ if (ms is! num) continue;
+ row['rec_ts'] = ms.toInt() ~/ 1000;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Both decoded tables are now keyed by rec_ts, so a plain | |
| // replace-insert merges cleanly (foreign-wins on a rec_ts | |
| // collision) — no orphan guard needed. A LEGACY export's | |
| // decoded_rr carries no rec_ts column; derive it from rr_ts_ms | |
| // (= rec_ts*1000) so the NOT NULL PK column is always populated. | |
| 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' && row['rec_ts'] == null) { | |
| // Storage class is per-VALUE in SQLite: a foreign export can | |
| // hand back a String where INTEGER is declared. Skip the row | |
| // rather than throwing out of the whole import transaction. | |
| final ms = row['rr_ts_ms']; | |
| if (ms is! num) continue; | |
| row['rec_ts'] = ms.toInt() ~/ 1000; | |
| } |
🤖 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 4041 - 4050, Update the decoded_rr legacy
rec_ts derivation to validate rr_ts_ms with the existing numeric-conversion
approach used by _PrepareAccumulator._num before converting it; only derive
row['rec_ts'] for values that are safely numeric, and avoid throwing for
non-numeric strings during the transaction.
| # ack_commit_sync_full_test wraps the ffi factory in SqfliteDatabaseFactoryLogger | ||
| # to spy the PRAGMA synchronous=FULL/NORMAL bracket around the ACK-gating commit | ||
| # (transitive via sqflite_common_ffi; declared directly since test/ now imports it). | ||
| sqflite_common: ^2.5.0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is SqfliteDatabaseFactoryLogger in sqflite_common sqflite_logger.dart still experimental?
💡 Result:
Yes, SqfliteDatabaseFactoryLogger in the sqflite_common package is still considered an experimental feature [1][2]. While the logger is fully functional and documented for use, the developer of the package has explicitly categorized it as experimental in the official documentation and package issues [1][2]. It remains a standard way to implement logging for database activities by wrapping your existing database factory, but users should be aware of this experimental status when integrating it into their projects [1][3].
Citations:
- 1: https://github.com/tekartik/sqflite/blob/master/sqflite/doc/dev_tips.md
- 2: Help with sqflite_logger tekartik/sqflite#1076
- 3: https://docsearch.algolia.com/mcp/docs/repo/tekartik/sqflite
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant files ---'
git ls-files | rg '(^|/)(pubspec\.yaml|pubspec\.lock|ack_commit_sync_full_test\.dart)$|sqflite'
printf '%s\n' '--- dependency declaration ---'
rg -n -C 6 'sqflite_common|kAlgoVersion' pubspec.yaml
printf '%s\n' '--- test API usage ---'
rg -n -C 5 'sqflite_common|SqfliteDatabaseFactoryLogger|SqfliteLoggerOptions|SqfliteLoggerSqlEvent|experimental_member_use' test
printf '%s\n' '--- lockfile package entry ---'
if [ -f pubspec.lock ]; then
rg -n -A 12 -B 2 '^ sqflite_common:' pubspec.lock || true
fi
printf '%s\n' '--- repository documentation or analyzer configuration ---'
rg -n -C 3 'experimental_member_use|SqfliteDatabaseFactoryLogger|SqfliteLoggerOptions|SqfliteLoggerSqlEvent' . --glob '!build/**' --glob '!dist/**' --glob '!node_modules/**' || trueRepository: OpenStrap/edge
Length of output: 45674
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import io
import json
import tarfile
import urllib.request
package = "sqflite_common"
versions = ["2.5.0", "2.5.8"]
def fetch(url):
request = urllib.request.Request(url, headers={"User-Agent": "review-evidence"})
with urllib.request.urlopen(request, timeout=20) as response:
return response.read()
metadata = json.loads(fetch(f"https://pub.dev/api/packages/{package}"))
listed = [v["version"] for v in metadata["versions"]]
print("latest:", metadata["latest"]["version"])
print("available requested versions:", {v: v in listed for v in versions})
for version in versions:
archive = fetch(f"https://pub.dev/api/archives/{package}-{version}.tar.gz")
with tarfile.open(fileobj=io.BytesIO(archive), mode="r:gz") as tf:
names = tf.getnames()
print(f"\n--- {package} {version} ---")
print("logger file present:", any(n.endswith("/lib/sqflite_logger.dart") for n in names))
for name in names:
if name.endswith("/pubspec.yaml") or name.endswith("/lib/sqflite_logger.dart"):
text = tf.extractfile(name).read().decode("utf-8", "replace")
if name.endswith("pubspec.yaml"):
print("[pubspec.yaml]")
print(text)
else:
print("[lib/sqflite_logger.dart: declarations/annotations]")
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if any(symbol in line for symbol in (
"SqfliteDatabaseFactoryLogger",
"SqfliteLoggerOptions",
"SqfliteLoggerSqlEvent",
"`@experimental`",
"experimental",
"library ",
"export ",
)):
start = max(1, i - 2)
end = min(len(lines), i + 3)
print("\n".join(f"{j}: {lines[j-1]}" for j in range(start, end + 1)))
print()
PYRepository: OpenStrap/edge
Length of output: 2475
🏁 Script executed:
#!/bin/bash
set -eu
for version in 2.5.0 2.5.8; do
archive="/tmp/sqflite_common-${version}.tar.gz"
curl -ksSfL "https://pub.dev/api/archives/sqflite_common-${version}.tar.gz" -o "$archive"
printf '\n--- sqflite_common %s files ---\n' "$version"
tar -tzf "$archive" | rg '(^|/)(pubspec\.yaml|lib/sqflite_logger\.dart)$'
printf '%s\n' '--- declarations and annotations ---'
tar -xOzf "$archive" "sqflite_common-${version}/lib/sqflite_logger.dart" |
nl -ba |
rg -n -C 3 'SqfliteDatabaseFactoryLogger|SqfliteLoggerOptions|SqfliteLoggerSqlEvent|`@experimental`|experimental|library |export '
doneRepository: OpenStrap/edge
Length of output: 427
🏁 Script executed:
#!/bin/bash
set -eu
for version in 2.5.0 2.5.8; do
archive="/tmp/sqflite_common-${version}.tar.gz"
[ -s "$archive" ] || curl -ksSfL "https://pub.dev/api/archives/sqflite_common-${version}.tar.gz" -o "$archive"
member="$(tar -tzf "$archive" | rg '/lib/sqflite_logger\.dart$|^lib/sqflite_logger\.dart$' | head -n 1)"
printf '\n--- sqflite_common %s member: %s ---\n' "$version" "$member"
printf '%s\n' '--- declarations and annotations ---'
tar -xOzf "$archive" "$member" |
awk '{ printf "%d:%s\n", NR, $0 }' |
rg -n -C 3 'SqfliteDatabaseFactoryLogger|SqfliteLoggerOptions|SqfliteLoggerSqlEvent|`@experimental`|experimental|library |export '
doneRepository: OpenStrap/edge
Length of output: 1358
🏁 Script executed:
#!/bin/bash
set -eu
for version in 2.5.0 2.5.8; do
archive="/tmp/sqflite_common-${version}.tar.gz"
member="$(tar -tzf "$archive" | rg '/src/logger/sqflite_logger\.dart$|^src/logger/sqflite_logger\.dart$' | head -n 1)"
printf '\n--- sqflite_common %s member: %s ---\n' "$version" "$member"
tar -xOzf "$archive" "$member" |
awk '{ printf "%d:%s\n", NR, $0 }' |
rg -n -C 5 'import |`@experimental`|experimental|class SqfliteDatabaseFactoryLogger|class SqfliteLoggerOptions|class SqfliteLoggerSqlEvent|typedef SqfliteDatabaseFactoryLogger|SqfliteDatabaseFactoryLogger\(|SqfliteLoggerOptions\('
doneRepository: OpenStrap/edge
Length of output: 7959
Do not treat the logger constructor as stable.
SqfliteDatabaseFactoryLogger, SqfliteLoggerOptions, and SqfliteLoggerSqlEvent are public exports in sqflite_common 2.5.0 and 2.5.8. The SqfliteDatabaseFactoryLogger constructor is marked @experimental in both versions. Pin an exact tested version or use a stable logging mechanism.
🤖 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 `@pubspec.yaml` around lines 263 - 266, Update the sqflite_common dependency
declaration used by the ACK commit sync test to pin an exact version whose
experimental SqfliteDatabaseFactoryLogger constructor has been tested, or
replace that constructor usage with a stable logging mechanism. Keep the
existing logger symbols and test behavior otherwise unchanged.
| test('rec_ts-range reads on decoded_rr are served by the PK auto-index', () async { | ||
| // decoded_rr shares the rec_ts key with decoded_onehz, so the derive read | ||
| // path (decodedRrByRecTsRange) is a PK range scan — never a full-table read. | ||
| final db = await LocalDb.instance; | ||
| for (final sql in const [ | ||
| 'EXPLAIN QUERY PLAN SELECT * FROM decoded_rr WHERE counter = 42 ' | ||
| 'ORDER BY beat_index', | ||
| 'EXPLAIN QUERY PLAN SELECT * FROM decoded_rr WHERE counter BETWEEN 1 AND 9', | ||
| ]) { | ||
| final detail = (await db.rawQuery( | ||
| sql, | ||
| )).map((r) => r['detail'].toString()).join(' | '); | ||
| expect( | ||
| detail.toUpperCase(), | ||
| contains('USING'), | ||
| reason: 'planner fell back to a full scan: $detail', | ||
| ); | ||
| expect( | ||
| detail, | ||
| contains('sqlite_autoindex_decoded_rr_1'), | ||
| reason: 'expected the primary key auto-index: $detail', | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| test('rr_ts_ms range scans are still served by an index', () async { | ||
| final db = await LocalDb.instance; | ||
| final plan = await db.rawQuery( | ||
| 'EXPLAIN QUERY PLAN SELECT * FROM decoded_rr WHERE rr_ts_ms < 1000 ' | ||
| 'ORDER BY rr_ts_ms ASC, beat_index ASC', | ||
| final detail = (await db.rawQuery( | ||
| 'EXPLAIN QUERY PLAN SELECT * FROM decoded_rr WHERE rec_ts BETWEEN 1 AND 9 ' | ||
| 'ORDER BY rec_ts ASC, beat_index ASC', | ||
| )).map((r) => r['detail'].toString()).join(' | '); | ||
| expect( | ||
| detail.toUpperCase(), | ||
| contains('USING'), | ||
| reason: 'planner fell back to a full scan: $detail', | ||
| ); | ||
| final detail = plan.map((r) => r['detail'].toString()).join(' | '); | ||
| expect( | ||
| detail, | ||
| contains('idx_decoded_rr_ts_beat_unique'), | ||
| reason: 'planner fell back to a scan: $detail', | ||
| contains('sqlite_autoindex_decoded_rr_1'), | ||
| reason: 'expected the primary key auto-index: $detail', | ||
| ); | ||
| expect( | ||
| detail.toUpperCase(), | ||
| isNot(contains('USE TEMP B-TREE')), | ||
| reason: 'ordering should come from the index: $detail', | ||
| reason: 'ordering should come from the PK: $detail', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Report the SQLite version used by the test backend and the EQP text for the asserted query.
set -euo pipefail
echo "== sqlite3 CLI version (sandbox) =="
sqlite3 --version 2>/dev/null || echo "sqlite3 CLI unavailable"
echo "== EQP for the same shape =="
sqlite3 ":memory:" <<'SQL' 2>/dev/null || echo "could not run"
CREATE TABLE decoded_rr (
rec_ts INTEGER NOT NULL,
beat_index INTEGER NOT NULL,
rr_ts_ms INTEGER NOT NULL,
rr_ms INTEGER NOT NULL,
PRIMARY KEY (rec_ts, beat_index)
);
EXPLAIN QUERY PLAN
SELECT * FROM decoded_rr WHERE rec_ts BETWEEN 1 AND 9
ORDER BY rec_ts ASC, beat_index ASC;
SQL
echo "== pinned ffi version =="
rg -nP 'sqflite_common_ffi|sqlite3_flutter_libs|sqlite3:' pubspec.yaml pubspec.lock 2>/dev/null || echo "not found"Repository: OpenStrap/edge
Length of output: 638
🏁 Script executed:
set -euo pipefail
echo "== test/db_storage_hygiene_test.dart =="
sed -n '1,110p' test/db_storage_hygiene_test.dart
echo "== dependency versions =="
sed -n '1328,1370p' pubspec.lock
sed -n '240,275p' pubspec.yaml
echo "== SQLite EQP across available Python SQLite builds =="
python3 - <<'PY'
import sqlite3
print("python sqlite version:", sqlite3.sqlite_version)
db = sqlite3.connect(":memory:")
db.execute("""
CREATE TABLE decoded_rr (
rec_ts INTEGER NOT NULL,
beat_index INTEGER NOT NULL,
rr_ts_ms INTEGER NOT NULL,
rr_ms INTEGER NOT NULL,
PRIMARY KEY (rec_ts, beat_index)
)
""")
query = """
SELECT * FROM decoded_rr WHERE rec_ts BETWEEN 1 AND 9
ORDER BY rec_ts ASC, beat_index ASC
"""
for row in db.execute("EXPLAIN QUERY PLAN " + query):
print(row)
PYRepository: OpenStrap/edge
Length of output: 7269
Reduce coupling to SQLite query-plan text
SQLite emits the expected SEARCH ... USING INDEX plan, but sqlite_autoindex_decoded_rr_1 is an internal name. Replace the index-name assertion with SEARCH and the absence of USE TEMP B-TREE. Avoid SCAN TABLE DECODED_RR; SQLite versions can emit different SCAN wording.
🤖 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 `@test/db_storage_hygiene_test.dart` around lines 42 - 65, Update the test
`rec_ts-range reads on decoded_rr are served by the PK auto-index` to remove the
assertion for the internal `sqlite_autoindex_decoded_rr_1` name. Assert that the
uppercased query-plan detail contains `SEARCH`, does not contain `USE TEMP
B-TREE`, and does not match `SCAN TABLE DECODED_RR`, while preserving the
existing planner-fallback diagnostics.
abdulsaheel
changed the title
init seq4 is send_historical so every fresh connect drained + trimmed under the bad clock anyway, and the unconditional set_clock before it clobbered the strap rtc and made the gate always see agreeing clocks. read first, skip both if suspect.
PR Reviewer Guide 🔍(Review updated until commit 997e149)Here are some key observations to aid the review process:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ble/ble_engine.dart (2)
1639-1647: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not consume the backfill floor for a deferred refresh.
_triggerBackfillsets_lastBackfillAtbefore this method runs. This return path sends no historical request, but it leaves that timestamp set and makes_triggerBackfillreturntrue. A corrected phone clock can then remain blocked by the backfill floor.Make
_startHistoricalRefreshreport whether it sentSEND_HISTORICAL_DATA. Update_lastBackfillAtonly after that result is true. Add a regression for a deferred refresh followed by an immediate successful retry.🤖 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/ble/ble_engine.dart` around lines 1639 - 1647, Update _startHistoricalRefresh to return whether SEND_HISTORICAL_DATA was actually sent, returning false for the _phoneClockSuspect deferred path and true after dispatch. In _triggerBackfill, assign _lastBackfillAt only when _startHistoricalRefresh returns true, and add a regression covering a deferred refresh followed immediately by a successful retry.
2293-2307: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winDo not correct the strap clock when the phone clock is suspect.
When Line 2300 sets
_phoneClockSuspectto true,ClockPolicy.shouldSetClock(dev, wall)is also true for the same drift. Line 2340 then callssetClock()and writes the bad phone time to a plausible strap RTC. Its readback can clear the flag before INIT starts history draining.Guard the automatic correction with
!_phoneClockSuspect. Add a connection regression that verifies a plausible strap clock more than one day ahead sends neitherSET_CLOCKnorSEND_HISTORICAL_DATA.Proposed fix
- if (ClockPolicy.shouldSetClock(dev, wall)) { + if (!_phoneClockSuspect && ClockPolicy.shouldSetClock(dev, wall)) {As per coding guidelines, “When adding or changing a capability, cover every call path.”
🤖 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/ble/ble_engine.dart` around lines 2293 - 2307, Guard the automatic strap-clock correction in the connection flow with !_phoneClockSuspect so a plausible strap RTC more than one day ahead of the phone is not overwritten; keep normal correction behavior when the phone clock is trusted. Add a connection regression covering this drift case and assert that neither SET_CLOCK nor SEND_HISTORICAL_DATA is sent, exercising the _phoneClockSuspect, ClockPolicy.shouldSetClock, and history-start paths.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@lib/ble/ble_engine.dart`:
- Around line 1639-1647: Update _startHistoricalRefresh to return whether
SEND_HISTORICAL_DATA was actually sent, returning false for the
_phoneClockSuspect deferred path and true after dispatch. In _triggerBackfill,
assign _lastBackfillAt only when _startHistoricalRefresh returns true, and add a
regression covering a deferred refresh followed immediately by a successful
retry.
- Around line 2293-2307: Guard the automatic strap-clock correction in the
connection flow with !_phoneClockSuspect so a plausible strap RTC more than one
day ahead of the phone is not overwritten; keep normal correction behavior when
the phone clock is trusted. Add a connection regression covering this drift case
and assert that neither SET_CLOCK nor SEND_HISTORICAL_DATA is sent, exercising
the _phoneClockSuspect, ClockPolicy.shouldSetClock, and history-start paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 64d2fd1a-2f6d-41dd-bd9d-37e4b144d3f1
📒 Files selected for processing (2)
lib/ble/ble_engine.darttest/ble_engine_test.dart
a slow phone fixes itself over ntp in minutes, so if we're still disagreeing 12h later its the strap rtc thats off. stop deferring at that point and let the normal set_clock fix it.
|
Persistent review updated to latest commit 997e149 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ble/ble_engine.dart (2)
850-862: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse monotonic time for the suspicion grace period.
_phoneClockSuspectSinceandsuspectGraceExpireduseDateTime.now(). If the phone clock moves forward but remains more than one day behind the strap, the 12-hour grace period can expire early. The next refresh can then drain and trim data under an untrusted phone clock. Store the start time from the existing_monotonicstopwatch, or pass an elapsedDurationintoClockPolicy.Add a regression test for a forward wall-clock jump.
As per coding guidelines, “Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests.”
🤖 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/ble/ble_engine.dart` around lines 850 - 862, Update the clock-suspicion grace-period flow centered on _phoneClockSuspectSince, _deferForClock, and ClockPolicy.suspectGraceExpired to use elapsed time from the existing _monotonic stopwatch rather than DateTime.now(), preserving deferral until the monotonic grace duration expires. Add a regression test that advances the wall clock forward while the monotonic elapsed duration remains within the grace period and verifies history remains deferred.Source: Coding guidelines
3080-3088: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the history gate inside the INIT API.
The production caller passes
drainOnInit, butsendInit()remains public and defaultsdraintotrue. Require a validated clock decision insidesendInit(), or make the method private. Add direct-call coverage for the deferred-clock case.🤖 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/ble/ble_engine.dart` around lines 3080 - 3088, Update sendInit so every invocation enforces the validated clock decision before including the historical-data packet, rather than relying on callers such as the drainOnInit path. Either require a validated clock-policy argument/decision in this public API or make sendInit private, preserving deferred history when the phone clock is suspect; add direct-call coverage for that deferred-clock case.Source: Coding guidelines
🤖 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/ble/ble_engine.dart`:
- Around line 2306-2311: Update the response-driven SET_CLOCK retry condition
near ClockPolicy.shouldSetClock so setClock() is invoked only when
!_deferForClock is true; preserve retries after the grace period expires or when
the reading is non-suspect, and add regression coverage for the plausible strap
RTC case during the 12-hour grace period.
- Around line 1366-1368: Make GET_CLOCK completion session-bound across
lib/ble/ble_engine.dart:1366-1368 by awaiting and validating the current
session’s clock response before deciding whether to call setClock(); at
lib/ble/ble_engine.dart:1643-1655, reuse that completed result and await any
required correction before SEND_HISTORICAL_DATA; at
lib/ble/ble_engine.dart:1452-1465, derive drainOnInit from the validated result
before sendInit, preventing stale _deferForClock state from advancing either
flow.
---
Outside diff comments:
In `@lib/ble/ble_engine.dart`:
- Around line 850-862: Update the clock-suspicion grace-period flow centered on
_phoneClockSuspectSince, _deferForClock, and ClockPolicy.suspectGraceExpired to
use elapsed time from the existing _monotonic stopwatch rather than
DateTime.now(), preserving deferral until the monotonic grace duration expires.
Add a regression test that advances the wall clock forward while the monotonic
elapsed duration remains within the grace period and verifies history remains
deferred.
- Around line 3080-3088: Update sendInit so every invocation enforces the
validated clock decision before including the historical-data packet, rather
than relying on callers such as the drainOnInit path. Either require a validated
clock-policy argument/decision in this public API or make sendInit private,
preserving deferred history when the phone clock is suspect; add direct-call
coverage for that deferred-clock case.
🪄 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: 99b71ad1-41c3-4c9e-ba85-a30b635765f4
📒 Files selected for processing (3)
lib/ble/ble_engine.dartlib/sync/sync_policy.darttest/sync_policy_test.dart
| await getClock(); | ||
| await Future.delayed(const Duration(milliseconds: 120)); | ||
| if (!_deferForClock) await setClock(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical
Make GET_CLOCK completion session-bound before clock correction or history draining. Both paths await only the command write and then sleep 120 ms. A late or missing clock_epoch leaves _deferForClock stale, allowing history or clock correction to proceed before the current session is classified.
lib/ble/ble_engine.dart#L1366-L1368: await the current session's clock response before deciding whether to callsetClock().lib/ble/ble_engine.dart#L1643-L1655: reuse the completed clock result and wait for any required correction before sendingSEND_HISTORICAL_DATA.lib/ble/ble_engine.dart#L1452-L1465: derivedrainOnInitfrom that validated result before callingsendInit.
📍 Affects 1 file
lib/ble/ble_engine.dart#L1366-L1368(this comment)lib/ble/ble_engine.dart#L1643-L1655lib/ble/ble_engine.dart#L1452-L1465
🤖 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/ble/ble_engine.dart` around lines 1366 - 1368, Make GET_CLOCK completion
session-bound across lib/ble/ble_engine.dart:1366-1368 by awaiting and
validating the current session’s clock response before deciding whether to call
setClock(); at lib/ble/ble_engine.dart:1643-1655, reuse that completed result
and await any required correction before SEND_HISTORICAL_DATA; at
lib/ble/ble_engine.dart:1452-1465, derive drainOnInit from the validated result
before sendInit, preventing stale _deferForClock state from advancing either
flow.
| final wasSuspect = _phoneClockSuspect; | ||
| _phoneClockSuspect = ClockPolicy.phoneClockSuspect(dev, wall); | ||
| if (_phoneClockSuspect && !wasSuspect) { | ||
| _phoneClockSuspectSince = DateTime.now(); | ||
| } else if (!_phoneClockSuspect) { | ||
| _phoneClockSuspectSince = null; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical
Gate response-driven SET_CLOCK retries on the grace policy.
After these lines set _phoneClockSuspect to true, the handler still reaches ClockPolicy.shouldSetClock and calls unawaited(setClock()) at Line 2352. For a plausible strap RTC more than one day ahead, this can write the phone's slow wall clock back to a correct strap RTC during the 12-hour grace period.
Require !_deferForClock before the retry. Allow correction after grace expiry or a non-suspect reading.
Suggested guard
- if (ClockPolicy.shouldSetClock(dev, wall)) {
+ if (!_deferForClock && ClockPolicy.shouldSetClock(dev, wall)) {As per coding guidelines, “Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests.”
🤖 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/ble/ble_engine.dart` around lines 2306 - 2311, Update the response-driven
SET_CLOCK retry condition near ClockPolicy.shouldSetClock so setClock() is
invoked only when !_deferForClock is true; preserve retries after the grace
period expires or when the reading is non-suspect, and add regression coverage
for the plausible strap RTC case during the 12-hour grace period.
Source: Coding guidelines
1 participant
bunch of data loss fixes on the offload path, all in one branch.
schema goes to 33.
Summary by CodeRabbit
Bug Fixes
Reliability