Skip to content

Add per-chunk historical clock + R-R packing diagnostic (#1008) - #1476

Merged
ryanbr merged 3 commits into
mainfrom
diag/per-chunk-clock-correlation
Aug 20, 2026
Merged

ryanbr merged 3 commits into
mainfrom
diag/per-chunk-clock-correlation

Conversation

@ryanbr

@ryanbr ryanbr commented Aug 20, 2026 •

Copy link
Copy Markdown
Owner

Adds one log-only line per decoded historical chunk, on both platforms.

Why per-chunk

The session summary already logs the one (device, wall) correlation captured on the first chunk (#67). That is enough to say whether the stale-RTC correction could engage, but it cannot answer either question #1008 turns on:

  1. Does the offset move within one offload? A strap RTC that drifts (measured at ~8 s/hour on my 4.0) has a different wall - device at the start and the end of a long session. One number per session hides the trajectory.
  2. Is the R-R overcount duplication, or packing? extractHistoricalStreams stamps every R-R interval inside a type-47 record with that record's single unix second, so a record carrying 8 intervals emits 8 rows on one timestamp (ord 0..7 — exactly what the WHOOP 4.0 R-R over-count contaminates HRV (not reproducible since 2026-08-25, cause unidentified) #1118 dumps show). That inflates beats-per-second without any record being delivered twice.

The line

Backfill: hist clock chunk=4 offset=+48s staleGate=closed rr=24 secs=3 pack=8.00 max=8 span=21s dens=1.14
field meaning
offset wall - device for this chunk, always signed so a drift trajectory reads at a glance
staleGate is the offset past the 1-day stale-RTC threshold?
pack intervals per stamped second
dens intervals per span second

pack high while dens sits near the true beat rate ⇒ packing. Both moving together ⇒ genuine duplication. That is the discrimination the existing logs could not make.

What staleGate does and does not claim

staleGate reads the same threshold the decoder uses. It was a local let inside extractHistoricalStreams; this hoists it to a shared constant on both platforms (histStaleClockThresholdSec / HIST_STALE_CLOCK_THRESHOLD_SEC, same 86 400 value) so the log and the decoder cannot disagree about where that threshold sits. Worth stating plainly: at the offsets actually observed (15–224 s) the correction is three orders of magnitude below the threshold, so the offset is discarded and each record keeps its own raw stamp — the drift is baked into the strap's own timestamps rather than applied by us. That is a different problem with a different fix, and staleGate=closed makes it visible instead of assumed.

The field is named for the gate, not the outcome, and that distinction is load-bearing — see the self-review note below.

Scope

Log-only. Nothing here feeds a stored value, a gate, or a score. No schema change.

Verification

  • 7 unit tests per platform, asserting the rendered line verbatim on both, so the logs stay byte-identical and one strap-log parser reads either. They cover the honest case, the packing case, the duplication case, the threshold boundary, a negative offset, and unsorted input.
  • WhoopProtocol: 614 tests pass on Linux (the new ones run in default CI).
  • Android: compileFullDebugKotlin clean; the JVM twin reports tests=7 (checked the XML, not the exit code).
  • The Swift Backfiller call site is app-target, so default CI does not compile it — covered by an on-demand app-build run.
  • Not yet exercised on a strap. It is log-only, so the risk is a useless line rather than a bad value, but the field shapes are unproven until a real offload prints them.

Self-review found a parity defect (fixed in 347c8e1)

The first revision formatted pack/dens with String(format: "%.2f") on Swift and String.format("%.2f") on Kotlin. Those do not agree. C/Swift printf rounds half-to-even; Java rounds half-up. They diverge on any exact binary tie, and ties are ordinary values here:

value Swift Kotlin
pack = 9/8 = 1.125 1.12 1.13
pack = 21/8 = 2.625 2.62 2.63

So the very property this PR asserts — one strap-log parser reads either platform — was false, and the seven verbatim-string tests all passed anyway because none of them happened to land on a tie.

Both platforms now compute the two decimals by integer arithmetic with an explicit half-up bias, so the rounding mode is defined in the code rather than inherited from a platform's printf. That also removes the locale hazard outright: there is no float formatting left to hand a comma decimal separator. The 9/8 tie is now a regression test on both sides.

Self-review found a second defect (fixed in c162fcd)

The field was originally corr=on|off, documented as "did the stale-RTC correction engage". It cannot know that. It mirrored only the first gate in extractHistoricalStreams — whether |wall - device| exceeds the 1-day threshold. Past that gate, the #471 overshoot guard re-checks every record and keeps the raw ts whenever the corrected value would post-date wall time.

So corr=on asserted an outcome that may never have happened, and it was most wrong in the case the guard exists for: a drained strap whose RTC has reset to ~epoch has an offset of decades, so every record overshoots and none are corrected — while the log claimed a correction throughout. That is the year-2081 class of bug (#471), where a misleading diagnostic costs the most.

That decision is per-record and depends on each raw stamp, so no single per-chunk flag can state it. The field is now staleGate=open|closed, named for what it actually observes, and the doc says plainly that an open gate does not prove the correction was applied. Reporting the gate is the honest half.

ryanbr added 3 commits August 20, 2026 12:40
The session summary logs one (device, wall) correlation, captured on the first
chunk. That cannot show the offset moving across a long offload, and it cannot
tell the two candidate explanations for the R-R overcount apart.

extractHistoricalStreams stamps every R-R interval inside a type-47 record with
that record's single unix second, so a record carrying 8 intervals emits 8 rows
on one timestamp. That inflates beats-per-second without any record arriving
twice. The new line separates the readings:

  pack (intervals per STAMPED second) high, dens (per SPAN second) near the true
  beat rate  -> packing
  pack and dens moving together                -> genuine duplication

corr=on|off reports whether the stale-RTC correction actually engaged. It reads
the same threshold the decoder uses, now hoisted to a shared constant on both
platforms so the log cannot claim a correction the stored stamps never received.

Log-only: nothing here feeds a stored value or a gate.

Verification: 7 unit tests per platform asserting the rendered line verbatim, so
the two logs stay byte-identical and one parser reads either. WhoopProtocol
614 tests pass on Linux; Android compileFullDebugKotlin clean and the JVM twin
reports tests=7. The Swift Backfiller call site is app-target, so it is covered
by an on-demand app-build run, not default CI. Not yet exercised on a strap.
Self-review parity defect. The line's pack/dens were formatted with
String(format:"%.2f") on Swift and String.format("%.2f") on Kotlin, which do
not agree: C/Swift rounds half-to-EVEN, Java rounds half-UP. They diverge on
any exact binary tie, and ties are ordinary values here — a pack of 9/8 is
1.125, rendering 1.12 on Swift and 1.13 on Kotlin.

That silently breaks the property the tests assert and the whole point of the
line: that one strap-log parser reads either platform.

Both now compute the two decimals by integer arithmetic with an explicit
half-up bias, so the rounding mode is defined in the code rather than inherited
from a platform's printf. It also removes the locale hazard outright — there is
no longer any float formatting to hand a comma decimal separator.

Adds the 9/8 tie as a regression test on both platforms. WhoopProtocol 615 pass
on Linux; the Android twin reports tests=8 under --rerun-tasks (the first run
reported UP-TO-DATE against a stale result file).
…e applied

Self-review. The field was named `corr=on|off` and documented as "did the
stale-RTC correction engage". It cannot know that. It mirrored only the FIRST
gate in extractHistoricalStreams — whether |wall - device| exceeds the 1-day
threshold. Past that gate the #471 overshoot guard re-checks EVERY record and
keeps the raw ts whenever the corrected value would post-date wall time.

So `corr=on` claimed an outcome that may not have happened, and it was most
wrong in the case the guard exists for: a drained strap whose RTC reset to
~epoch has an offset of decades, so every record overshoots and NONE are
corrected — while the log asserted a correction throughout. That is the year-2081
class of bug, where a misleading diagnostic costs the most.

The decision is per-record and depends on each raw stamp, so no single
per-chunk flag can state it. The field is now `staleGate=open|closed`, named for
what it actually observes, and the doc says plainly that an open gate does not
prove the correction was applied.

Renaming only; the threshold comparison is unchanged. WhoopProtocol 615 pass on
Linux, Android twin tests=8, compileFullDebugKotlin clean.
@ryanbr
ryanbr merged commit 8124da3 into main Aug 20, 2026
16 checks passed
@ryanbr
ryanbr deleted the diag/per-chunk-clock-correlation branch August 20, 2026 02:00
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.

1 participant