sync: log-only burst-completeness shortfall diagnostic (correct received-total signal) by abdulsaheel · Pull Request #232 · OpenStrap/edge

abdulsaheel · 2026-08-11T18:07:49Z

User description

What

Adds an honest, log-only burst frame-loss diagnostic at HISTORY_END. It computes the completeness signal against the all-types received total (totalTrafficPacketCount) instead of the banked R24 subset, and emits a would-flag line + a burst_shortfall ledger field — without changing the commit/ACK decision.

Why

The strap's HISTORY_END metadata carries num_packets (every frame it transmitted this burst — R24 data and interleaved console/event/unknown). The only correct completeness comparison is against the all-types received total. Comparing against the banked R24 subset fabricates a shortfall whenever console/event frames ride along un-banked — which is exactly why that gate was disabled and validateBurst went advisory-only.

Today, on a true frame drop the burst is committed + ACKed OK anyway and the loss is silent. Before wiring any FAIL gate we need to see true loss in telemetry. This PR is that groundwork.

The signal

New pure helper:

burstPacketShortfall = expectedPacketCount - (receivedTrafficCount + droppedThisBurst)

receivedTrafficCount = totalTrafficPacketCount (all types) — type-agnostic, interleaving-immune.
droppedThisBurst (RecordGate plausibility rejections) is added back so gate drops never read as radio loss.
Positive shortfall = frames the band sent that never reached us (true loss → would-flag). Zero = complete. Negative = retries/dupes, not loss.

At burst end we log a clear would-flag (LOG-ONLY, commit+ACK unchanged) line when shortfall > 0, and stamp burst_shortfall into the existing mismatch ledger entry.

Invariants (unchanged)

Commit-before-ACK, verbatim token echo, OK/FAIL decision: untouched.
Single _ingestHistoricalFrame; live streams never persisted.
No hard-FAIL / re-flood path — that needs its own design + field validation to avoid eternal re-flood, and is explicitly out of scope here.

Rejected alternative: per-revision counter-gap gate

The record counter is a global flash-log index sliced per revision, so gaps are the normal state — gating on the counter gap would false-positive constantly. num_packets vs received-total is the right signal.

Tests

Pure unit tests for burstPacketShortfall: benign console/event interleaving does not false-positive, true loss is quantified, gate-dropped add-back closes the gap, negative/retry case, and shortfall == 0 ⇔ burstPacketCountMatches. flutter test green (49 pass), flutter analyze clean on changed files.

PR Type

Enhancement, Tests

Description

Adds log-only burstPacketShortfall() helper comparing num_packets against all-types received total
Emits would-flag log line and stamps burst_shortfall into mismatch ledger at HISTORY_END
Commit-before-ACK, verbatim token echo, and OK/FAIL decision are all unchanged
Adds five unit tests covering interleaving, true loss, gate-drop add-back, retries, and equivalence with burstPacketCountMatches

Diagram Walkthrough

flowchart LR
  A["HISTORY_END received"] -- "compute" --> B["burstPacketShortfall()\nexpected - (receivedTrafficCount + droppedThisBurst)"]
  B -- "shortfall > 0" --> C["Log 'would-flag' (LOG-ONLY)"]
  B -- "mismatch ledger exists" --> D["Stamp burst_shortfall\ninto ledger entry"]
  A -- "unchanged" --> E["Commit + verbatim ACK\n(OK/FAIL decision untouched)"]

File Walkthrough

Relevant files

Enhancement

ble_engine.dart `Add log-only burst shortfall diagnostic at HISTORY_END` lib/ble/ble_engine.dart Adds pure `burstPacketShortfall()` helper (annotated `@visibleForTesting`) computing `expected - (receivedTrafficCount + droppedThisBurst)` using the all-types traffic total At HISTORY_END, computes `shortfall` using `d.currentBurstTrafficCount` (not the banked R24 subset) and `droppedThisBurst` Stamps `burst_shortfall` into the existing mismatch ledger `metaPatch` when a mismatch is recorded Emits a `would-flag (LOG-ONLY, commit+ACK unchanged)` log line when `shortfall > 0`; commit-before-ACK and ACK decision are untouched	+53/-0

Tests

ble_engine_test.dart `Unit tests for burstPacketShortfall helper` test/ble_engine_test.dart Adds `group('burst completeness shortfall ...')` with five unit tests Tests cover: zero shortfall when all types received, no false-positive from interleaved console/event frames, positive shortfall for true loss, gate-dropped add-back, negative/retry case, and equivalence with `burstPacketCountMatches`	+95/-0

… 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.

coderabbitai · 2026-08-11T18:08:11Z

Warning

Review limit reached

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

Next review available in: 49 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c9d8bc70-a8cb-4b84-92a0-3ccdeed65bc8

📥 Commits

Reviewing files that changed from the base of the PR and between d2ccec4 and a974918.

📒 Files selected for processing (2)

lib/ble/ble_engine.dart
test/ble_engine_test.dart

📝 Walkthrough

Walkthrough

The BLE engine adds burstPacketShortfall for all-traffic telemetry accounting. HISTORY_END processing records the calculated shortfall and logs positive advisory loss without blocking commit or ACK behavior. Tests cover complete, missing, dropped, excess, and matching traffic.

Changes

Telemetry packet shortfall

Layer / File(s)	Summary
Shortfall calculation and validation `lib/ble/ble_engine.dart`, `test/ble_engine_test.dart`	The engine adds `burstPacketShortfall`. Tests cover complete traffic, interleaved frames, true loss, gate drops, excess traffic, and equivalence with `burstPacketCountMatches`.
HISTORY_END diagnostic integration `lib/ble/ble_engine.dart`	`HISTORY_END` processing calculates all-traffic shortfall, persists it in sync diagnostics, and logs positive would-flag loss. Commit and ACK behavior remains unchanged.

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

Suggested reviewers: localhoop, brackyt, dannymcc

🚥 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 clearly summarizes the main change: adding a log-only burst-completeness shortfall diagnostic based on the correct received-total signal.
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.

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

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

github-actions · 2026-08-11T18:09:32Z

PR Reviewer Guide 🔍

(Review updated until commit `a974918`)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Shortfall default masks null When `expected == null`, `shortfall` is hardcoded to `0`, which means the `if (shortfall > 0)` log block is silently skipped. This is correct behavior for the log, but the `burst_shortfall` field stamped into the mismatch ledger (`metaPatch`) is only written inside the `!validated` branch. If `expected != null` and `validated` is true but `shortfall > 0` (i.e., the all-types count shows loss but the old R24-subset check passes), the shortfall is computed and logged but never recorded in the ledger — the telemetry entry that would let you act on it later is missing. The PR description says the ledger field is the groundwork for a future FAIL gate, so a positive shortfall that doesn't land in the ledger defeats that purpose. final shortfall = expected == null ? 0 : burstPacketShortfall( expectedPacketCount: expected, receivedTrafficCount: d.currentBurstTrafficCount, droppedThisBurst: droppedThisBurst, ); // ADVISORY ONLY, never a gate: `expectedPacketCount`'s exact semantics // (which transport packet types the band itself counts — command // responses interleaved with the burst? retried/duplicate frames?) are // not fully reverse-engineered, and field data shows the gap between // expected and actual varies run to run with no fixed offset. What IS // fully verified is frame-level CRC32 (framing.dart) and the RecordGate // plausibility check — both already ran on every buffered record before // we ever get here. So a count mismatch is NOT evidence of corrupt or // missing data; treating it as fatal was actively harmful: on mismatch // the OLD behavior discarded the entire buffered chunk (throwing away // perfectly good, already-CRC-verified, already-gate-passed records), // told the band FAIL, and re-requested the same block — forever, since // nothing about a retry changes the count relationship. Zero sync // progress, "last data" frozen indefinitely. Log the mismatch (still // useful signal — see the sync-diagnostics screen) and commit anyway. if (!validated) { _burstMismatchTotal++; _burstMismatchStreak++; _log( '[SYNC] Burst packet-count mismatch (advisory, NOT blocking commit) ' '(attempt ${d.consecutiveValidationFailures}, ' 'streak=$_burstMismatchStreak): expected=$expected, ' 'actual=${d.currentBurstPacketCount}, ' 'dropped_this_burst=$droppedThisBurst, ' 'historical=${d.currentBurstHistoricalPacketCount}, ' 'traffic=${d.currentBurstTrafficCount}, ' 'breakdown=${d.currentBurstBreakdown}', ); await _bestEffortLedgerWrite(() => LocalDb.upsertSyncLedgerEntry( status: 'validated_with_mismatch', lastError: 'burst_packet_mismatch', metaPatch: { 'expected_burst_packets': expected, 'actual_burst_packets': d.currentBurstPacketCount, 'dropped_this_burst': droppedThisBurst, 'historical_burst_packets': d.currentBurstHistoricalPacketCount, 'traffic_burst_packets': d.currentBurstTrafficCount, 'burst_validation_failures': d.consecutiveValidationFailures, 'burst_breakdown': d.currentBurstBreakdown, 'burst_shortfall': shortfall, }, )); Boolean latch risk The new `shortfall` computation path references `d.currentBurstTrafficCount` and `_recordGate.dropped - _burstDroppedAtStart`. If an exception is thrown between the shortfall computation and the commit+ACK below (e.g., in the log call or ledger write), the existing boolean latches (`_drainingOffloadFrames`, etc.) that AGENTS.md §4.3 flags as a recurring wedge pattern may not be cleared. This PR adds new code in that critical window without a `try/finally` guard — consistent with the known pattern. The risk is low for the log-only path itself, but worth confirming the surrounding finally block covers the new code. if (shortfall > 0) { _log( '[SYNC] burst completeness would-flag (LOG-ONLY, commit+ACK ' 'unchanged): expected=$expected ' 'received=${d.currentBurstTrafficCount} ' 'dropped_this_burst=$droppedThisBurst shortfall=$shortfall ' '(all-types received total — frames the band counted that we did ' 'not; missing or CRC-corrupted, potential loss; groundwork for a ' 'future FAIL gate, NOT gating today)', ); }

github-actions · 2026-08-11T18:10:09Z

PR Code Suggestions ✨

Latest suggestions up to a974918
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Absent input fabricates a zero-shortfall value When `expected` is null the shortfall is silently set to `0`, which is indistinguishable from a genuinely complete burst. The `if (shortfall > 0)` log block below will never fire for the null case, but the `burst_shortfall` key in `metaPatch` will record `0` even though no completeness check was possible, fabricating a "no loss" signal. Use `null` instead so telemetry can distinguish "unknown" from "complete". lib/ble/ble_engine.dart [2633-2639] final shortfall = expected == null - ? 0 + ? null : burstPacketShortfall( expectedPacketCount: expected, receivedTrafficCount: d.currentBurstTrafficCount, droppedThisBurst: droppedThisBurst, ); Suggestion importance[1-10]: 5 __ Why: When `expected` is null, recording `shortfall=0` in telemetry is misleading since it implies a complete burst rather than an unknown state. However, the `if (shortfall > 0)` log block correctly won't fire, and the `validated` variable already handles the null case separately, so the impact is limited to telemetry ambiguity rather than a functional bug.	Low
General	Guard log block against null shortfall If `shortfall` is changed to `null` when `expected` is null (per the previous suggestion), this guard must also handle the null case to avoid a null-comparison runtime error. Add a null check so the log only fires when shortfall is both non-null and positive. lib/ble/ble_engine.dart [2689-2699] -if (shortfall > 0) { +if (shortfall != null && shortfall > 0) { _log( '[SYNC] burst completeness would-flag (LOG-ONLY, commit+ACK ' 'unchanged): expected=$expected ' 'received=${d.currentBurstTrafficCount} ' 'dropped_this_burst=$droppedThisBurst shortfall=$shortfall ' '(all-types received total — frames the band counted that we did ' 'not; missing or CRC-corrupted, potential loss; groundwork for a ' 'future FAIL gate, NOT gating today)', ); } Suggestion importance[1-10]: 3 __ Why: This suggestion is contingent on suggestion 1 being applied first; without changing `shortfall` to `null`, this guard is unnecessary. As a standalone suggestion it adds no value since `shortfall` is currently typed as `int` and the `> 0` comparison is always valid.	Low

Previous suggestions

Suggestions up to commit d2ccec4

Category	Suggestion	Impact
Possible issue	Absent input fabricates zero instead of abstaining When `expected` is null the shortfall is silently set to `0`, which is indistinguishable from a genuine zero-loss burst. This fabricates a "complete" signal instead of abstaining, violating the never-fabricate-a-metric rule. Use `null` for the absent-expected case and guard the log block accordingly. lib/ble/ble_engine.dart [2630-2636] final shortfall = expected == null - ? 0 + ? null : burstPacketShortfall( expectedPacketCount: expected, receivedTrafficCount: d.currentBurstTrafficCount, droppedThisBurst: droppedThisBurst, ); Suggestion importance[1-10]: 4 __ Why: While the suggestion has a valid philosophical point about distinguishing "no expected value" from "zero shortfall", the current code already handles this correctly in context: when `expected == null`, the `validated` check also skips, and the `shortfall` is only used in the log block (`if (shortfall > 0)`) and in `metaPatch` (only when `expected != null` due to the surrounding `if` block). Setting it to `0` when `expected == null` is a safe sentinel that doesn't fabricate a misleading metric in practice, since the log block won't fire and the metaPatch is inside a conditional. The suggestion adds complexity without a clear practical benefit in this codebase context.	Low
General	Null-guard log block after absent-input fix After the `expected == null ? null` fix above, `shortfall` becomes `int?` and the `> 0` comparison will fail to compile. Guard the log block with a null check so it only fires when `expected` was present and a genuine shortfall was computed. lib/ble/ble_engine.dart [2686-2695] -if (shortfall > 0) { +if (shortfall != null && shortfall > 0) { _log( '[SYNC] burst completeness would-flag (LOG-ONLY, commit+ACK ' 'unchanged): expected=$expected ' 'received=${d.currentBurstTrafficCount} ' 'dropped_this_burst=$droppedThisBurst shortfall=$shortfall ' '(all-types received total — true frame loss; groundwork for a ' 'future FAIL gate, NOT gating today)', ); } Suggestion importance[1-10]: 2 __ Why: This suggestion is a follow-up to suggestion 1 and is only necessary if suggestion 1 is applied. Since suggestion 1 has limited merit, this suggestion is also of low value. Additionally, the `shortfall` variable is currently typed as `int` (not `int?`), so this null check would be unnecessary without the prior change.	Low

coderabbitai

Actionable comments posted: 2

🤖 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 2630-2636: Update the serialized frame-ingest flow in
lib/ble/ble_engine.dart#L2630-L2636 so all queued data-role packet types,
including console, event, unknown, and historical frames, are counted in the
burst traffic counters before burstPacketShortfall is calculated; use the
existing _drainOffloadFrames/_ingestHistoricalFrame path and preserve commit and
ACK behavior. Add a regression test in test/ble/ble_engine_test.dart#L125-L218
covering a HISTORY_END burst with data-role historical, console, and event
frames, asserting zero shortfall and unchanged commit and ACK behavior.
- Around line 169-174: Revise the documentation around droppedThisBurst in
lib/ble/ble_engine.dart:169-174 to describe a positive shortfall as potential
loss or missing valid traffic, not confirmed loss. Update the related log text
in lib/ble/ble_engine.dart:2682-2694 to remove the “true frame loss” claim.
Rename the affected test in test/ble/ble_engine_test.dart:165-178 to describe a
positive shortfall without asserting confirmed loss.

🪄 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: fc03a79c-baa4-44f4-b897-011c82edb140

📥 Commits

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

📒 Files selected for processing (2)

lib/ble/ble_engine.dart
test/ble_engine_test.dart

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

github-actions · 2026-08-11T18:15:11Z

Persistent review updated to latest commit a974918

abdulsaheel · 2026-08-11T19:53:00Z

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

github-actions Bot added the Review effort 2/5 label Aug 11, 2026

coderabbitai Bot reviewed Aug 11, 2026

View reviewed changes

Comment thread lib/ble/ble_engine.dart Outdated

Comment thread lib/ble/ble_engine.dart

abdulsaheel mentioned this pull request Aug 11, 2026

offload data loss fixes #235

Open

abdulsaheel closed this Aug 11, 2026

Navigation Menu

Uh oh!

Uh oh!

sync: log-only burst-completeness shortfall diagnostic (correct received-total signal) - #232

sync: log-only burst-completeness shortfall diagnostic (correct received-total signal)#232
abdulsaheel wants to merge 2 commits into
mainfrom
fix/num-packets-total-received-gate

abdulsaheel commented Aug 11, 2026 •

edited by github-actions Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 11, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Uh oh!

github-actions Bot commented Aug 11, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 11, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 11, 2026

Uh oh!

abdulsaheel commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

abdulsaheel commented Aug 11, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

What

Why

The signal

Invariants (unchanged)

Rejected alternative: per-revision counter-gap gate

Tests

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

coderabbitai Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Uh oh!

github-actions Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit a974918)

Uh oh!

github-actions Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 11, 2026

Uh oh!

abdulsaheel commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

abdulsaheel commented Aug 11, 2026 •

edited by github-actions Bot

Loading

coderabbitai Bot commented Aug 11, 2026 •

edited

Loading

github-actions Bot commented Aug 11, 2026 •

edited

Loading

(Review updated until commit `a974918`)

github-actions Bot commented Aug 11, 2026 •

edited

Loading