Skip to content

sync: skip the #689 backlog decode on the GET_DATA_RANGE PENDING ack - #1746

Merged
ryanbr merged 1 commit into
ryanbr:mainfrom
Zebsi235:fix/689-pending-ack
Aug 31, 2026
Merged

ryanbr merged 1 commit into
ryanbr:mainfrom
Zebsi235:fix/689-pending-ack

Conversation

@Zebsi235

@Zebsi235 Zebsi235 commented Aug 30, 2026 •

Copy link
Copy Markdown

What this PR does

GET_DATA_RANGE answers twice: a short PENDING(2) acknowledgement, then the payload with SUCCESS(1). The #689 backlog decode runs on both. The ack carries no ring pointers, so it decodes to null and logs as a decode failure, once per sync, on healthy hardware:

→ GET_DATA_RANGE payload= (puffin)
Command response: GET_DATA_RANGE(34) → PENDING(2)
Get Data Range raw frame (#451 …): aa010c000100271124e8220402000000c391bc3d
Strap backlog pages behind: not decodable from this frame (#689 — offsets may have moved; …)
Backfill: GET_DATA_RANGE SUCCESS — requesting history
Get Data Range raw frame (#451 …): aa014c00010032d124e92204010100bf0000b4be…
Strap backlog pages behind: 13 (#689 — GET_DATA_RANGE ring backlog, diagnostic only)

The split is structural rather than marginal. One session on my MG on 2026-08-28, 34 responses:

frames length result byte decode
17 20 bytes 0x02 PENDING fails, every time
17 84 bytes 0x01 SUCCESS succeeds, every time

My 2026-08-27 capture has the same shape (19 / 26), so it is not a one-off.

The tree already knows this happens. Framing.kt's own result-code table says it:

2=PENDING precedes SUCCESS on GET_DATA_RANGE (hardware-confirmed, #78 fork)

The decode site just had no way to act on it, because pagesBehind only sees the frame and a too-short frame is indistinguishable from a misaligned one.

Why it is worth a change rather than ignoring

The message names a cause: "offsets may have moved", and points at the raw frame as the input to analyse. That was the right thing to write when the failure mode was unknown, and the comment above it explains why both branches log at all: until #818 the offsets really were wrong and this branch logged nothing, so a broken decode was invisible. That reasoning still holds for a SUCCESS frame that will not parse, and this PR leaves that case exactly as loud as it was.

What it stops is the ack. On current firmware that warning fires on a frame that is not a data-range frame by construction, so anyone following the hint goes looking for an alignment bug that is not there.

Shape of the change

A pure DataRange.isPendingResponse(frame, cmdOff) on both platforms, reading the result byte at cmdOff + 2, used to gate the decode at the two call sites. A frame too short to hold a result byte returns false, so the existing too-short handling is untouched.

The #451 raw-frame dump above it stays unconditional. It is cheap, and it is the input any future offset analysis would want.

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Documentation
  • CI / tooling

How it was tested

Not on the BLE path. isPendingResponse and pagesBehind are pure and the change only gates a log line; no bytes move on the wire.

Five new cases per platform in the files that already pin pagesBehind, using the real PENDING/SUCCESS pair from one request on my MG (WS50_r00, fw 50.39.1.0, 2026-08-28) rather than synthetic frames. One of them asserts the pair directly: the ack is exactly the frame pagesBehind returns null for, and the new predicate is what tells them apart.

./gradlew testFullDebugUnitTest --tests com.noop.ble.DataRangeScanTest on Windows 11, JDK 17:

  • with the fix: 17 tests, 0 failures
  • with the tests kept and isPendingResponse stubbed to return false: 2 failed, the PENDING ack is exactly the frame pagesBehind cannot decode and isPendingResponse true for the PENDING ack, both java.lang.AssertionError. The other 15, including all 12 pre-existing, still passed
  • fix restored: 17 tests, 0 failures

Full suite, since DataRange is shared surface: ./gradlew testFullDebugUnitTest --rerun-tasks --no-build-cache against main @ b2a4049 — 4,903 tests across 600 classes, 0 failures, 0 errors (6 skipped, pre-existing).

python Tools/doc_comment_lint.py: OK no NEW detached doc comments (1902 files, 24 baselined site(s) remaining).

The gap I would rather state than have you find: the Swift half is not run locally, no Apple machine. DataRangeTests lives in Packages/WhoopProtocol, which swift-packages.yml covers, and BLEManager.swift is compiled by app-build.yml. The Swift change is line for line the twin of the Kotlin one that was run, but that is reasoning plus CI rather than a local run.

Checklist

  • Swift package tests pass — not run locally (no Apple machine); left to swift-packages.yml
  • Android unit tests pass (./gradlew testFullDebugUnitTest, full suite)
  • No new build warnings introduced
  • UI changes use only StrandDesign tokens — n/a, no UI
  • No hardcoded hex frame bytes; protocol facts live in the schema / decoders — the new literals are test fixtures (captured frames), and the predicate reads a documented result byte
  • Follows the conventions in docs/CONTRIBUTING.md
  • I did not commit generated output or any secrets/keystores

Related issues

Refs #689 (the feature this guards), #818 (the offset fix whose comment explains why both branches log), #78 (where the PENDING-precedes-SUCCESS behaviour was confirmed).

Leaves #689 open on its own merits; that work landed in #694 and the decode itself is correct. This only stops it running on a frame that cannot carry its input. (Deliberately phrased with no closing verb adjacent to an issue number: GitHub matches those keywords literally and does not read negations, so a denial would have been actioned as a closing keyword on merge.)

…G ack

GET_DATA_RANGE answers twice, a short PENDING(2) ack then the SUCCESS(1)
payload, which Framing's result-code table already records. The ack
carries no ring pointers, so the pagesBehind decode failed on it every
time and logged a false 'offsets may have moved' once per sync on
healthy hardware. A pure isPendingResponse predicate now gates the
decode at both call sites; a SUCCESS frame that will not parse still
logs loudly, and the unconditional ryanbr#451 raw-frame dump is untouched.
Fixtures are the real PENDING/SUCCESS pair from one request.
@ryanbr
ryanbr merged commit c6dcef4 into ryanbr:main Aug 31, 2026
15 checks passed
ryanbr added a commit that referenced this pull request Aug 31, 2026
Build numbers only - 10.6.1 stays the unreleased line, and 395 clears the staging
high-water so the APK installs over 394.

Carries the sparse-bridge change: a night interrupted by one short trip out of bed
is no longer ended at the interruption. Anyone whose nights were being truncated
that way should see fuller totals; the honest caveat is that the absorbed minutes
land in sleep rather than wake, because both stagers call wake off HR and the bridge
only absorbs spans whose HR was already in the sleep band.

Also the #1746 PENDING-ack change, so one "offsets may have moved" line per sync
stops appearing on healthy 4.0 hardware, and the per-strap Last sync / Data write
attribution from #1745.
@Zebsi235
Zebsi235 deleted the fix/689-pending-ack branch September 6, 2026 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants