fix(sync): refuse HISTORY_END trim when gate drops leave an empty buffer - #198
fix(sync): refuse HISTORY_END trim when gate drops leave an empty buffer#198Brackyt wants to merge 3 commits into
Conversation
Empty drop-only ACKs were advancing the strap cursor and auto-continue while decoded_onehz stayed frozen, permanently deleting unread 1 Hz flash after reconnect. Also require durable rows before lastTrimAdvanced.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughHistorical BLE trim handling now requires durable progress. Drop-only bursts refuse ChangesBLE trim safety
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BLEEngine
participant DrainController
participant TrimAckPolicy
participant NoDurableProgressEscalation
BLEEngine->>DrainController: Commit historical burst
DrainController-->>BLEEngine: Report durable rows and dropped records
BLEEngine->>TrimAckPolicy: Evaluate trim observations
TrimAckPolicy-->>BLEEngine: Allow or block HISTORY_END
BLEEngine->>NoDurableProgressEscalation: Track refusal or successful ACK
NoDurableProgressEscalation-->>BLEEngine: Request SET_CLOCK and reconnect
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
|
This removed gaps in my HR graph correctly |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 2244-2288: Add engine-level regression coverage around the
HISTORY_END handling for TrimAckVerdict.blockedNoDurableProgress: verify the
first two refusals neither ACK nor reconnect, the third invokes setClock and
tears down the session, and a successful ACK resets _noDurableTrimRefuseStreak
so subsequent refusals start from zero. Reuse the existing BLE engine test
harness and mocks for session lifecycle and command transmission.
- Around line 2403-2426: Track completion of records written through the
unbuffered path in DrainController.onHistoricalRecord, and await those writes
before the TrimAckPolicy.evaluate decision in the HistoryEnd flow so
hadDurableRows reflects persisted direct writes. Alternatively, route historical
drains through an awaited batch commit while preserving the no-progress gate for
rejected records. Add a regression test for a mixed burst with neither
onCommitBatch nor onRecordsBatch, covering the applicable raw decode and
session/export trigger paths.
- Around line 3426-3434: Update the fallback commit path used when onCommit is
null and onRecordsBatch is available so every archive in archives is persisted
through onArchive before reporting success; if no archive sink exists, fail the
commit and preserve the buffer so the trim token is not acknowledged. Keep raw
persistence unchanged, and add a regression covering an archive-only chunk
through this fallback path.
🪄 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: ca9cd751-0aa7-4714-820c-a8bf1957de88
📒 Files selected for processing (3)
lib/ble/ble_engine.dartlib/ble/ble_state.darttest/ble_safe_trim_test.dart
| case TrimAckVerdict.blockedNoDurableProgress: | ||
| // Gate rejected every historical sample this burst and nothing was | ||
| // banked (no raws, no archives). Echoing the token would trim flash | ||
| // we never stored — the HR-gap / frozen-cursor failure mode after a | ||
| // reconnect with a bad plausibility window. Keep the chunk on the | ||
| // band; after a short streak, re-correlate the clock and bounce. | ||
| _noDurableTrimRefuseStreak++; | ||
| _log( | ||
| '[SYNC] HISTORY_END token=$tokenHex has no durable rows but the ' | ||
| 'plausibility gate dropped samples this burst — NOT ACKing ' | ||
| '(streak=$_noDurableTrimRefuseStreak). The band keeps the chunk; ' | ||
| 'a SET_CLOCK/reconnect may clear a poisoned gate window.', | ||
| ); | ||
| await _bestEffortLedgerWrite(() => LocalDb.upsertSyncLedgerEntry( | ||
| chunkId: 'batch:$tokenHex', | ||
| kind: 'historical_batch', | ||
| status: 'trim_refused', | ||
| lastError: 'no_durable_progress', | ||
| metaPatch: { | ||
| 'batch_id': batchId, | ||
| 'records': d.records, | ||
| 'no_durable_refuse_streak': _noDurableTrimRefuseStreak, | ||
| }, | ||
| )); | ||
| if (_noDurableTrimRefuseStreak >= 3 && !_sessionIsStale(session)) { | ||
| _log( | ||
| '[SYNC] $_noDurableTrimRefuseStreak consecutive no-durable trim ' | ||
| 'refuses — defensive SET_CLOCK + bounce so the next session can ' | ||
| 're-admit the re-delivered chunk.', | ||
| ); | ||
| _noDurableTrimRefuseStreak = 0; | ||
| try { | ||
| await setClock(); | ||
| } catch (e) { | ||
| _log('[SYNC] defensive SET_CLOCK after no-durable refuse failed: $e'); | ||
| } | ||
| if (!_sessionIsStale(session)) { | ||
| unawaited( | ||
| _teardownSession(intentional: false).then((_) { | ||
| _setPhase(BleConnState.idle); | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
| return; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add regression coverage for refusal recovery.
The changed tests cover TrimAckPolicy and DrainController, but not the new engine behavior after three no-durable-progress refusals. Add an engine-level regression that verifies the first two refusals do not ACK or reconnect, the third sends SET_CLOCK and tears down the session, and a successful ACK resets the streak.
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 2244 - 2288, Add engine-level
regression coverage around the HISTORY_END handling for
TrimAckVerdict.blockedNoDurableProgress: verify the first two refusals neither
ACK nor reconnect, the third invokes setClock and tears down the session, and a
successful ACK resets _noDurableTrimRefuseStreak so subsequent refusals start
from zero. Reuse the existing BLE engine test harness and mocks for session
lifecycle and command transmission.
Source: Coding guidelines
…ing silent Refusing a HISTORY_END whose burst banked nothing is right, and this PR gets that part right -- echoing the token lets the band delete flash we never stored, the one irreversible act in this protocol. The problem is everything AFTER the refusal. PARTIALLY REFUTING the review note first: this branch DOES already have a refusal counter and a SET_CLOCK+bounce remedy at >= 3. The claim of "no counter, no eventual path" is out of date. What is real is that neither could actually fire. 1. THE COUNTER COULD NEVER CLIMB. `_noDurableTrimRefuseStreak` was zeroed in the per-connection setup block -- but the streak's own remedy is a SET_CLOCK + BOUNCE, i.e. a reconnect, and the idle watchdog also tears the session down after a burst that banked nothing. Every cycle reset the count, so the ">= 3 then run the remedy" branch was unreachable in the field and the loop ran forever at streak 1. The engine already documents this exact distinction one block up: "marginal-radio + post-bond-loop are NOT reset here -- they count consecutive bad cycles across reconnects". This is the same class of counter and belongs on the same side of that line. 2. A REMEDY THAT KEEPS FAILING WAS INVISIBLE. `EmptySyncTracker` (which sets `syncClockLost`) and `StuckStrapDetector` are BOTH only evaluated inside `_onOffloadFinished`, and that needs a HISTORY_COMPLETE this loop never reaches, because the band keeps re-delivering the same un-trimmed chunk. So a band with a lost RTC -- every record below the plausibility floor -- sat in refuse -> bounce -> refuse indefinitely behind a debug log, with no user signal at all. Both now live in `NoDurableProgressEscalation` (lib/sync/sync_policy.dart), pure and unit-testable, alongside BondRefusalGiveUp which it deliberately mirrors -- the engine wires, the policy decides. Two thresholds, because there are two questions: N consecutive refusals => try the remedy; M remedies that did NOT work => surface `syncClockLost`. Counting spans reconnects and clears only on a successful trim ACK, which is the only thing that proves the condition is actually over. It still NEVER ACKs. "Keep the data" stays the answer -- what changes is that a persistently failing remedy becomes visible instead of silent. 7 tests, mutation-verified. Suite 1208 passing; the 6 failures in notification_dedupe_test are pre-existing and reproduce on origin/main unmodified.
|
Reviewed and pushed First, partly refuting the review noteThe finding I was handed said "no counter, no age bound, no eventual path". That's out of date — this branch already has a refusal counter and a SET_CLOCK+bounce remedy at What's real is that neither could actually fire. 1. The counter could never climb
So every cycle reset the count. The The engine already documents this exact distinction one block up:
This is the same class of counter and belongs on the same side of that line. 2. A remedy that keeps failing was invisible
So a band with a lost RTC — every record below the plausibility floor — sat in What I didBoth concerns now live in Two thresholds, because there are two different questions:
Counting spans reconnects and clears only on a successful trim ACK — the one event that actually proves the condition is over. It still never ACKs. "Keep the data" stays the answer. What changes is that a persistently failing remedy becomes visible instead of silent. Verification7 tests, mutation-verified — breaking the remedy counter fails exactly the four tests that depend on it. Suite 1208 passing / 6 failing; the 6 are |
|
Nice find too |
|
Checked @coderabbitai's three findings against the code. One is a real safe-trim hole, one is real but latent, one my refactor partly answers. Reachability analysis for each, since that's what decides whether they block. 1. "Persist archive-only fallback commits before reporting success" — REAL, and the serious oneConfirmed in if (onCommit != null) {
await onCommit!(raws, samples, tokenHex, archives: archives);
} else if (onRecordsBatch != null && raws.isNotEmpty) {
await onRecordsBatch!(raws, samples);
}
return true;With That's a direct violation of the invariant this PR exists to defend. Reachability: latent, not live. Both production constructions set all three sinks ( I have not fixed it, deliberately. The obvious fix — refuse to report success when archives have no sink — converts silent data loss into a permanent refusal loop for that configuration, because archives can never be persisted there. That's a design question about whether the configuration should exist at all, and it's your drain controller. Options as I see them: drop the 2. "Track durable progress for the unbuffered record path" — real, same latencyIn the both-sinks-null configuration, Also only reachable via 3. "Add regression coverage for refusal recovery" — partly answeredI moved the escalation out of the engine into Worth noting these three are all independent of the bug I fixed, which was that the counter was zeroed per-connection while its own remedy is a reconnect. That one's live, not latent. @Brackyt — you mentioned this "removed gaps in my HR graph correctly", which is good confirmation the core gate is doing its job. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Candidate files =='
fd -t f -a 'ble_engine.dart|app_state.dart|background_sync.dart|no_durable_progress_escalation_test.dart|ble_safe_trim_test.dart' . | sort
echo
echo '== BLE engine outline =='
ast-grep outline lib/ble/ble_engine.dart --items all --type class,function --match 'Drain|Historical|commit|onHistoricalRecord' || true
echo
echo '== Drain controller and historical-record contexts =='
rg -n -A45 -B20 \
'class _?DrainController\b|Future<bool>\s+commit\s*\(|Future<void>\s+commit\s*\(|onHistoricalRecord\s*\(' \
lib/ble/ble_engine.dart
echo
echo '== Sink construction contexts =='
rg -n -A20 -B12 \
'onCommit|onRecordsBatch|onCommitBatch|onHistoricalRecord' \
lib/app_state.dart lib/background_sync.dart 2>/dev/null || true
echo
echo '== Relevant test names and assertions =='
rg -n -A8 -B3 \
'NoDurableProgressEscalation|archive|unbuffered|trimAcked|trimRefused|SET_CLOCK|disconnect|bounce' \
test/no_durable_progress_escalation_test.dart test/ble_safe_trim_test.dart 2>/dev/null || trueLength of output: 36047
I recommend that The unbuffered path needs more than The I could not independently verify the production construction sites in this pass because the shell search used paths that do not match the repository layout. That does not change the verified controller defects. ✏️ Learnings added
You are interacting with an AI system. |
|
Correction to my earlier note on this PR, and it's good news. I described the 6 They are a time bomb, not a standing breakage. The suite builds date-prefixed dedupe keys from a hardcoded
Proved it by substituting today's date into the unmodified file on Practical impact here: this PR's CI cannot go green until #207 merges, regardless of its own content. Sorry for the noise — "pre-existing and unrelated" was accurate but undersold that it was actively blocking you. |
…nbuffered drains Reject onRecordsBatch without onCommit at construction, buffer only when onCommit is wired, and refuse HISTORY_END when supportsSafeTrim is false so archive-only or fire-and-forget paths cannot ACK flash that was never banked.
DrainController safe-trim hardening landed (
|
Summary
SET_CLOCK+ bounce so the next session can re-admit the chunk.lastTrimAdvancedonly flips when durable rows were banked, so empty token ACKs no longer feed auto-continue while the frontier stays frozen.Observed on WHOOP 5 hardware as frozen
rec_ts_hw/ HR graph gaps while batches kept ACKing with unchanged record counts; the guard is band-agnostic sync safety (not gen5-specific).Test plan
flutter test test/ble_safe_trim_test.dart test/sync_policy_test.dart(86 passed)NOT ACKing/no_durable_progressinstead of empty drop-ACKs walking the cursorSummary by CodeRabbit