fix: repin siblings, per-day prune scoping, monotonic auto-continue clock, CI pin guard in the release path

[abdulsaheel]

User description

Summary

• Repin protocol → 5bb8606, analytics → 5d42191 (both were one merge behind main). Concretely: protocol's CRC8-on-length-field check (7269ef9) merged a full day before this repin — until now, edge's pinned FrameReassembler had no check on the declared frame length before trusting it, so one corrupted length byte during a historical offload could consume up to ~4092 bytes of good stream (data the strap was about to trim from flash and would never resend). This shipped unpinned in every CI run and any release cut from the old pin.
• pruneSupersededIntermediates() was table-wide, not per-day. It kept the 2 highest algo_versions present anywhere in sleep_session_candidates/wake_day_features. A day whose raw substrate already aged out (3-day retention) never re-enters the derive pipeline to write a newer-version row of its own — so the moment any two other days reached the next two kAlgoVersion generations, that day's only cached row got deleted, orphaning a day that can never be re-derived. Scoped retention per day_id instead.
• AutoContinueRun's time ceiling used DateTime.now() — real wall-clock, not monotonic. A DST fall-back or NTP correction mid-chain could make elapsed() go small/negative and silently disable the one time-based cap standing between a stuck-but-churning offload chain and pausing heartbeat/keepalive/RTC-reverify for far longer than intended. BleEngine now feeds it a dedicated Stopwatch-backed monotonic clock; AutoContinueRun's own double-seconds interface is untouched so its existing deterministic tests are unaffected.
• CI: the pin-drift guard only ran in test.yml, which a tag-triggered release never executes — so it protected nothing about an actual release. Extracted to .github/scripts/check_sibling_pins.sh, wired into build.yml's preflight (which both release jobs needs:) too. Also added a non-fatal staleness check against each sibling's live GitHub main via git ls-remote — the internal lock-vs-yaml check alone passes clean on a pin that's simply gone stale, which is exactly what happened here.

Reviewed, deliberately NOT changed

The auto-continue "productive" streak resets on any HISTORY_END token that merely differs from the immediately-previous one, with no documented monotonicity guarantee on that token. Whether this is actually exploitable rests on an assumption about firmware continuation-token behavior I can't verify without real hardware — a speculative change to that gate risks a worse, concrete regression (treating a legitimate later batch as already-seen and under-draining real backlog) in exchange for fixing a hypothetical one. The monotonic-clock fix above already makes the 10-minute ceiling — the real backstop by design — actually enforce itself reliably.

Known follow-up

This pin predates protocol fix/issues PR #21 (realtimeRr RR-bound, historical-family activity/steps_inc null-vs-0) — repin again once that merges.

Test plan

• flutter test --concurrency=1 — 1055 passed (added a per-day-prune regression test that reproduces the exact orphaning scenario)
• flutter analyze — clean
• .github/scripts/check_sibling_pins.sh manually verified against both its fatal (path-source) and warning (staleness) paths with synthetic fixtures, including replaying this incident's actual before/after SHAs through it
• pubspec.lock regenerated with pubspec_overrides.yaml moved aside first (restored after), so it reflects the real git pins, not local paths

---

PR Type

Bug fix, Tests, Enhancement

---

Description

• Repin protocol (5bb8606) and analytics (5d42191) to pick up CRC8 length-field check preventing up to 4092 bytes of stream data loss on corrupted frames

• Fix pruneSupersededIntermediates() to scope retention per day_id instead of table-wide, preventing orphaned rows for days that can never be re-derived

• Replace wall-clock DateTime.now() with monotonic Stopwatch for AutoContinueRun elapsed ceiling, preventing DST/NTP jumps from silently disabling the cap

• Extract sibling-pin guard to shared script and wire it into build.yml preflight so release tag builds are also protected

---

Diagram Walkthrough

flowchart LR
  A["pubspec.yaml\nprotocol → 5bb8606\nanalytics → 5d42191"]
  B["check_sibling_pins.sh\n(shared guard script)"]
  C["build.yml preflight\n(release path)"]
  D["test.yml\n(PR/push path)"]
  E["BleEngine\n_monotonicSecs()"]
  F["AutoContinueRun\nelapsed ceiling"]
  G["pruneSupersededIntermediates()\nper day_id scoping"]
  H["db_storage_hygiene_test\nnew regression test"]

  A -- "pins protocol CRC8 fix" --> C
  B -- "wired into" --> C
  B -- "wired into" --> D
  E -- "feeds monotonic clock" --> F
  G -- "scoped per day_id" --> H

Loading

File Walkthrough

	Relevant files
Bug fix	2 files ble_engine.dart Add monotonic Stopwatch clock for auto-continue elapsed ceiling +14/-2    db.dart Scope intermediate prune retention per day_id not table-wide +32/-15
Tests	1 files db_storage_hygiene_test.dart Add regression test for per-day prune orphan bug                  +43/-0
Configuration changes	3 files check_sibling_pins.sh Extract shared sibling-pin guard script with staleness warning +86/-0    build.yml Wire pin-drift guard into release preflight job                    +9/-0      test.yml Replace inline pin guard with shared script invocation      +5/-41
Dependencies	1 files pubspec.yaml Repin protocol and analytics to current main SHAs                +17/-14

---

Summary by CodeRabbit

• Bug Fixes
  • Improved Bluetooth auto-continuation timing to stay stable even if the device clock changes.
  • Refined intermediate data pruning to preserve superseded records correctly on a per-day basis.
• Maintenance
  • Updated pinned protocol and analytics component versions.
  • Strengthened CI preflight guards to detect stale or inconsistent sibling dependency pinning and prevent local override drift.
• Tests
  • Added a widget test to ensure cleanup doesn’t delete older intermediates for a specific day_id when other days have newer algorithm versions.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 483cee6f-6db0-462b-922a-113482e9f4df

📥 Commits

Reviewing files that changed from the base of the PR and between 4b055f4 and 7e83b15.

📒 Files selected for processing (2)

• .github/scripts/check_sibling_pins.sh
• lib/data/db.dart

---

📝 Walkthrough

Walkthrough

The PR centralizes sibling dependency pin checks in CI, updates sibling commit pins, uses monotonic elapsed time for BLE auto-continuation limits, and changes intermediate database pruning to retain versions independently per day.

Changes

Sibling dependency pin validation

Layer / File(s)	Summary
Pin updates and CI guard wiring .github/scripts/check_sibling_pins.sh, .github/workflows/*, pubspec.yaml	Sibling git refs are updated, and shared CI checks detect overrides, local resolutions, lockfile drift, and stale upstream pins.

Monotonic continuation timing

Layer / File(s)	Summary
Monotonic auto-continuation timing lib/ble/ble_engine.dart	Auto-continuation ceiling and continuation timestamps now use a process-lifetime monotonic stopwatch.

Scoped intermediate pruning

Layer / File(s)	Summary
Per-day pruning and regression coverage lib/data/db.dart, test/db_storage_hygiene_test.dart	Pruning retains the newest algorithm-version generations separately for each day_id, with coverage for uneven version histories across days.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• OpenStrap/edge#154: Also changes CI workflow gating around dependency retrieval and tests.
• OpenStrap/edge#158: Updates the same sibling dependency commit pins.
• OpenStrap/edge#168: Overlaps in sibling-pin guards, BLE continuation timing, and database pruning.

Suggested reviewers: dannymcc, localhoop

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: sibling repins, per-day pruning, monotonic timing, and the CI guard in the release path.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

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 7e83b15)

Here are some key observations to aid the review process:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Off-by-one cutoff In the new per-day pruning logic, cutoff = versions[keepVersions - 1] and then deletes rows where algo_version < cutoff. With keepVersions = 2 and versions sorted descending as [50, 49, 48], cutoff = versions[1] = 49, so the delete removes rows where algo_version < 49, keeping both 49 and 50. This is correct. However, if a day has exactly keepVersions versions (e.g., [50, 49]), versions.length <= keepVersions short-circuits and nothing is deleted — also correct. The edge case to verify: with keepVersions = 1 and versions [50, 49], cutoff = versions[0] = 50, deleting algo_version < 50, which keeps only 50. That is correct. The logic appears sound, but the cutoff index arithmetic (versions[keepVersions - 1]) is subtle enough that a dedicated unit test for keepVersions = 1 would be valuable to pin this invariant, since the existing tests only exercise the default keepVersions = 2. final versions = entry.value..sort((a, b) => b.compareTo(a)); if (versions.length <= keepVersions) continue; final cutoff = versions[keepVersions - 1]; deleted += await txn.delete( table, where: 'day_id = ? AND algo_version < ?', whereArgs: [entry.key, cutoff], ); Unbounded SELECT before transaction pruneSupersededIntermediates issues a SELECT DISTINCT day_id, algo_version FROM $table outside any transaction, then opens a transaction to delete. Between the SELECT and the transaction, a concurrent derive run could insert new rows for a day. The in-memory versionsByDay map would then have a stale view: the cutoff computed from the pre-transaction snapshot could delete a row that was the newest at SELECT time but is now superseded by a freshly written row — or conversely, could leave a row that should now be pruned. For a maintenance-only prune function this is low severity, but given the invariant that intermediate tables must never orphan a day's only cached row, a TOCTOU window here could delete the sole row for a day if a new version was inserted between the SELECT and the DELETE. Wrapping the SELECT inside the same transaction would close this window. final rows = await db.rawQuery( 'SELECT DISTINCT day_id, algo_version FROM $table', ); final versionsByDay = <String, List<int>>{}; for (final r in rows) { final day = r['day_id'] as String; final v = r['algo_version'] as int; (versionsByDay[day] ??= <int>[]).add(v); } // One transaction per table instead of one round-trip per day_id — // right after a kAlgoVersion bump forces a bulk re-derive (or a user // runs "Re-analyze data"), many days can cross the keepVersions // threshold in the same pass, and un-batched deletes on iOS run under // the same CPU-watchdog constraint the rest of derivation is careful // about. await db.transaction((txn) async { for (final entry in versionsByDay.entries) { final versions = entry.value..sort((a, b) => b.compareTo(a)); if (versions.length <= keepVersions) continue; final cutoff = versions[keepVersions - 1]; deleted += await txn.delete( table, where: 'day_id = ? AND algo_version < ?', whereArgs: [entry.key, cutoff], ); } }); awk block parser fragile The awk pattern used to extract a package's block from pubspec.lock and pubspec.yaml — '$0==p{f=1;next} /^ [a-z_]+:/{f=0} f' — stops at the next top-level key that matches ^ [a-z_]+:. Package names containing hyphens (e.g., flutter_blue_plus) would not terminate the block, but a package whose name starts with an uppercase letter or contains a digit would also not terminate it. More critically, if pubspec.lock ever gains a package whose block contains a line that exactly matches openstrap_protocol: or openstrap_analytics: as a nested key, the parser would start a second extraction mid-block. This is unlikely in practice but the script is now load-bearing for releases; a comment noting the assumption (no nested keys matching the pattern) would make the fragility explicit. Not a blocking issue given the current lockfile shape, but worth noting given the history of lockfile surprises in this repo. block=$(awk -v p=" $pkg:" '$0==p{f=1;next} /^ [a-z_]+:/{f=0} f' pubspec.lock) if printf '%s' "$block" | grep -qE '^\s+source: path'; then echo "::error::pubspec.lock resolves $pkg from a local path." echo "::error::Re-run: mv pubspec_overrides.yaml /tmp/ && flutter pub get && mv /tmp/pubspec_overrides.yaml ." fail=1 continue fi locked=$(printf '%s' "$block" | grep -E '^\s+resolved-ref:' | head -1 | awk '{print $2}' | tr -d '"') pinned=$(awk -v p=" $pkg:" '$0==p{f=1;next} /^ [a-z_]+:/{f=0} f' pubspec.yaml \ | grep -E '^\s+ref:' | head -1 | awk '{print $2}' | tr -d '"')

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 7e83b15
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fix deleted count on transaction rollback The cutoff is versions[keepVersions - 1], which is the last kept version (the oldest one to retain). The delete condition algo_version < cutoff correctly removes everything strictly older than that version. However, with keepVersions = 2 and versions [50, 49, 48], cutoff = versions[1] = 49, so the delete removes algo_version < 49, keeping both 49 and 50 — this is correct. But if there are exactly keepVersions entries (e.g., [50, 49]), versions.length <= keepVersions skips the delete, which is also correct. The real issue is that deleted is mutated inside the transaction closure but declared outside — if the transaction is rolled back due to an exception, deleted will still reflect the (now-rolled-back) count, returning a misleading total. Move deleted accumulation to after the transaction, or capture the per-table count inside and add it after the transaction completes. lib/data/db.dart [4583-4594] +int tableDeleted = 0; await db.transaction((txn) async { for (final entry in versionsByDay.entries) { final versions = entry.value..sort((a, b) => b.compareTo(a)); if (versions.length <= keepVersions) continue; final cutoff = versions[keepVersions - 1]; - deleted += await txn.delete( + tableDeleted += await txn.delete( table, where: 'day_id = ? AND algo_version < ?', whereArgs: [entry.key, cutoff], ); } }); +deleted += tableDeleted; Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that deleted is mutated inside a transaction closure and could be misleading if the transaction rolls back. However, in practice SQLite transactions in Flutter's sqflite package throw exceptions on rollback (they don't silently roll back), so the caller would see an exception rather than a wrong count. The fix is valid but addresses a relatively edge-case scenario.	Low
Possible issue	Verify monotonic origin resets across BLE sessions _monotonic is started at field-initialization time (object construction), but BleEngine instances may be long-lived across multiple connect/disconnect cycles. The AutoContinueRun elapsed clock is used to enforce a per-chain ceiling, but if the engine object is reused across sessions, _monotonicSecs() will keep accumulating from the very first construction, meaning the elapsed value passed to _autoContinue.elapsed() will be enormous on any session after the first and the auto-continue ceiling will never arm. The _autoContinue state is reset on connect (per the existing code), but the monotonic origin is not — AutoContinueRun.elapsed() should be diffing two readings, not using an absolute value from construction. Verify that AutoContinueRun.continued(now:) stores the now passed to it and that elapsed() returns currentNow - storedNow, not currentNow alone; if it stores an absolute and _autoContinue is reset on connect without re-seeding the origin, the ceiling will fire immediately on the first elapsed() call of a new session. lib/ble/ble_engine.dart [4719-4720] +// No code change needed here if AutoContinueRun.elapsed() correctly diffs +// two _monotonicSecs() readings (continued(now:) stores the start, elapsed(now) +// returns now - start). Confirm that _autoContinue.continued() is called with +// _monotonicSecs() at chain-start and that reset on connect re-arms the origin +// via a fresh continued() call or explicit reset, so elapsed() never sees a +// stale absolute from a prior session. final Stopwatch _monotonic = Stopwatch()..start(); double _monotonicSecs() => _monotonic.elapsedMicroseconds / 1e6; Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify that AutoContinueRun.elapsed() diffs two _monotonicSecs() readings rather than using an absolute value. The improved_code is essentially the same as existing_code with added comments, making this more of a verification request than an actionable fix. The concern is valid but speculative without seeing AutoContinueRun's implementation.	Low

---

Previous suggestions

Suggestions up to commit 4b055f4

Category	Suggestion	Impact
Possible issue	Wrap per-day deletes in a single transaction The per-day delete loop issues one DELETE statement per day, which can be thousands of round-trips for a table with many days. More critically, the deletes are not batched in a single transaction, so a crash mid-loop leaves the table in a partially pruned state — some days pruned, others not — which is non-idempotent and can corrupt the rollback guarantee the keepVersions window is meant to provide. Wrap the entire loop body for each table in a single db.transaction(...) call so all deletes for that table are atomic. lib/data/db.dart [4568-4586] final rows = await db.rawQuery( 'SELECT DISTINCT day_id, algo_version FROM $table', ); final versionsByDay = <String, List<int>>{}; for (final r in rows) { final day = r['day_id'] as String; final v = r['algo_version'] as int; (versionsByDay[day] ??= <int>[]).add(v); } - for (final entry in versionsByDay.entries) { - final versions = entry.value..sort((a, b) => b.compareTo(a)); - if (versions.length <= keepVersions) continue; - final cutoff = versions[keepVersions - 1]; - deleted += await db.delete( - table, - where: 'day_id = ? AND algo_version < ?', - whereArgs: [entry.key, cutoff], - ); - } + await db.transaction((txn) async { + for (final entry in versionsByDay.entries) { + final versions = entry.value..sort((a, b) => b.compareTo(a)); + if (versions.length <= keepVersions) continue; + final cutoff = versions[keepVersions - 1]; + deleted += await txn.delete( + table, + where: 'day_id = ? AND algo_version < ?', + whereArgs: [entry.key, cutoff], + ); + } + }); Suggestion importance[1-10]: 5 __ Why: The suggestion to wrap deletes in a transaction is a valid improvement for atomicity and performance, but the claim that partial pruning is "non-idempotent" or "corrupts the rollback guarantee" is overstated — the pruning function is a best-effort cleanup, not a critical ledger operation. The improved code is accurate and the change is reasonable but not critical.	Low
	Prevent mixed wall/monotonic clock seeds in elapsed computation _autoContinue.continued(now: _monotonicSecs()) stores the monotonic timestamp, but _autoContinue.elapsed(_monotonicSecs()) computes now - lastContinuedAt. If _autoContinue is ever initialized with a wall-clock seed (e.g. from _wallSecs()) at construction or on connect, the first elapsed() call will compute a huge negative or positive delta against the monotonic origin. Verify that AutoContinueRun's initial lastContinuedAt (or equivalent field) is seeded with _monotonicSecs() on connect, not _wallSecs(), so the two clocks are never mixed in the same subtraction. lib/ble/ble_engine.dart [719-720] final Stopwatch _monotonic = Stopwatch()..start(); double _monotonicSecs() => _monotonic.elapsedMicroseconds / 1e6; +// On connect, seed AutoContinueRun with the monotonic clock: +// _autoContinue.reset(now: _monotonicSecs()); // not _wallSecs() + Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about mixing wall-clock and monotonic seeds in _autoContinue, but the improved_code is essentially the same as existing_code with only a comment added, making it a verification/ensure suggestion rather than a concrete fix. The actual initialization of AutoContinueRun is not shown in the diff, so this is speculative.	Low

[coderabbitai]

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 (1)

lib/data/db.dart (1)

4560-4589: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Per-day scoping fix looks correct. The versionsByDay map correctly isolates each day's own version history, so a day whose raw substrate aged out and can never write a newer generation is no longer collateral damage from unrelated days advancing — matches the new regression test's exact scenario.

One trade-off worth noting: this now issues one db.delete per day requiring pruning instead of the old single table-wide delete, an N+1 pattern. Given sleep_session_candidates/wake_day_features are small and most days settle at ≤keepVersions after steady state, this is likely fine, but a large backlog (e.g., first run after many historical algo bumps) could issue many round trips.

🤖 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 4560 - 4589, The per-day pruning logic in
pruneSupersededIntermediates is correct; retain the versionsByDay scoping and
current behavior. No change is required for the noted per-day delete round trips
unless performance optimization is specifically needed.

🤖 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 @.github/scripts/check_sibling_pins.sh:
- Around line 75-83: Bound the git ls-remote probe in the upstream check around
the existing repo and refs/heads/main arguments by running it through the
repository’s available timeout utility, with a finite duration. Preserve the
current successful pin comparison and fallback warning behavior when the command
times out or otherwise fails.

In `@lib/ble/ble_engine.dart`:
- Around line 2572-2578: Add an engine-level regression test covering the
auto-continue policy path around the 599/600-second boundary, using an
injectable elapsed-time provider or equivalent clock control to supply monotonic
readings while wall time moves backward. Verify the ceiling remains enforced and
the behavior is driven by the monotonic value passed through the BleEngine
auto-continue flow, reusing existing test setup and symbols in
test/sync_policy_test.dart where applicable.

---

Outside diff comments:
In `@lib/data/db.dart`:
- Around line 4560-4589: The per-day pruning logic in
pruneSupersededIntermediates is correct; retain the versionsByDay scoping and
current behavior. No change is required for the noted per-day delete round trips
unless performance optimization is specifically needed.

🪄 Autofix (Beta)

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: 19dde18e-14be-437e-a3a4-21b92aa77396

📥 Commits

Reviewing files that changed from the base of the PR and between e534074 and 4b055f4.

⛔ Files ignored due to path filters (1)

• pubspec.lock is excluded by !**/*.lock

📒 Files selected for processing (7)

• .github/scripts/check_sibling_pins.sh
• .github/workflows/build.yml
• .github/workflows/test.yml
• lib/ble/ble_engine.dart
• lib/data/db.dart
• pubspec.yaml
• test/db_storage_hygiene_test.dart

[abdulsaheel]

Went through the PR-Agent findings too:

• "Prune cutoff off-by-one" — self-resolved in the same comment ("No bug here — noting it was carefully checked"). Verified independently, agrees: correct.
• "N+1 queries per table" / "Wrap per-day deletes in a single transaction" — real, fixed in 7e83b15. Checked it's safe first: pruneDecodedBeforeRecTs (the sibling prune call) already wraps its own deletes in db.transaction(...), and the two prune calls in _pruneOldDecoded run sequentially rather than nested, so wrapping this one too doesn't risk a nested-transaction conflict. I'd push back a little on the PR-Agent's framing that a crash mid-loop "corrupts the rollback guarantee" — pruning here is a best-effort cleanup of recomputable rows, not a durable-ledger operation, so a partial prune isn't actually unsafe, just untidy — but the transaction wrap is a legitimate improvement regardless (fewer round-trips, real atomicity) so I took it.

Also replied inline to both CodeRabbit findings — one confirmed and fixed (the git ls-remote timeout), one traced and found to be a false alarm (no wall-clock/monotonic mixing exists; grepped every call site to confirm).

[github-actions]

Persistent review updated to latest commit 7e83b15