Skip to content

stress: take each live R-R packet once in the iOS check-in - #2418

Merged
ryanbr merged 1 commit into
ryanbr:mainfrom
UtkuDenizAltiok:stress-rr-once-per-packet
Sep 23, 2026
Merged

ryanbr merged 1 commit into
ryanbr:mainfrom
UtkuDenizAltiok:stress-rr-once-per-packet

Conversation

@UtkuDenizAltiok

Copy link
Copy Markdown

What this PR does

The iOS stress check-in (AppModel.evaluateStress, off by default) appended the live R-R intervals to its buffer every time ingestHR ran, and ingestHR runs from two @Published sinks — live.$heartRate and live.$rr (AppModel.swift ~258). A sink runs inside willSet, so the handler reads the packet before the one being written, and a packet whose heart rate also changed reached it twice: its intervals entered rrBuf twice, and the detector's slow EMA baseline advanced on every call rather than every packet.

Driven through a real LiveState in BLEManager's standard-HR write order (intervals first, then the heart rate when it changed), 21 packets came out as 800, 800, 801, 802, 802, 803, 804, 804, ….

RRPacketCursor states the rule RRPacketObserver.swift already gives views — take a packet once, keyed on rrSeq — for a consumer that is not a view, and evaluateStress asks it first. setRRIntervals moves rr and rrSeq together, so live.rr always belongs to live.rrSeq whichever sink is running.

Android runs the same detector once per history offload on rrRecent and is unchanged. The two still differ in how often they evaluate (per packet here, per offload there); that is a design question this PR leaves alone.

Type of change

  • Bug fix

How it was tested

  • RRPacketCursorTests (StrandTests), 3 tests: the cursor's rule, and a real LiveState fed 21 packets through both sinks — each interval taken once and in order with the cursor, more than once without it. With isNew made to accept a repeat, the first two fail (the doubled sequence above), and the file restored byte-identical (sha256).
  • Full local run on 48b772b6 (on 34211e18; the head 725c163b is the same diff rebased onto 5783c499, whose newer commits touch none of these files): WhoopStore 611 · StrandAnalytics 2048 · StrandImport 327 · doc lint · i18n · macOS StrandTests 2,110 (the only failures are the two locale-dependent TodayCarryOverTests, which fail on clean main too) · iOS build.
  • The parity ledger, ratchet and governance tests fail locally, and fail identically on a clean checkout of main (twin-map authority drift; the scheduled Parity Governance run failed on 971d0d9f). RRPacketCursor is app-target code, outside Packages/** and android/**.
  • Not tested on a strap.

Checklist

  • No UI change; no StrandDesign tokens involved
  • Follows the conventions in docs/CONTRIBUTING.md
  • I did not commit generated output (Strand.xcodeproj/) or any secrets/keystores

Related issues

Refs #2416 (the same two sinks double-recorded manual-workout samples)

🤖 Generated with Claude Code

AppModel.ingestHR runs from two @published sinks, heartRate and rr, and calls evaluateStress each time.
A sink runs inside willSet, so the handler reads the packet before the one being written, and a
packet whose heart rate also changed reached it twice: its intervals entered rrBuf twice and the
detector's slow EMA baseline advanced on every call instead of every packet. Driven through a real
LiveState in BLEManager's write order, 21 packets were taken as 800, 800, 801, 802, 802, ...

RRPacketCursor states RRPacketObserver.swift's rule (take a packet once, keyed on rrSeq) for a consumer
that is not a view; evaluateStress asks it first. setRRIntervals moves rr and rrSeq together, so live.rr
always belongs to live.rrSeq whichever sink is running. The check-in is off by default; Android runs the
same detector once per offload on rrRecent and is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
UtkuDenizAltiok added a commit to UtkuDenizAltiok/noop that referenced this pull request Sep 23, 2026

@ryanbr ryanbr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @UtkuDenizAltiok. This is right, and the thing I went looking for is not there.

RRPacketCursor starts at lastSeq = 0, so the obvious way for this to go wrong is a real packet also carrying sequence 0, which would be dropped silently and forever look like the detector simply never saw the first one. It cannot happen: rrSeq starts at 0 and setRRIntervals increments it before any sink can observe it, so the first real packet is always 1. Your XCTAssertFalse(cursor.isNew(0)) pins exactly that, and reads as deliberate rather than incidental.

The pairing the whole design rests on also holds. setRRIntervals assigns rr and then bumps rrSeq, so the $rr sink, firing in willSet, sees packet N-1 alongside sequence N-1, and the $heartRate sink sees packet N alongside N. Either way live.rr belongs to live.rrSeq, which is what makes keying on the sequence sound rather than lucky. Nothing is lost either: a packet the heart-rate sink never carries is taken on the next packet's R-R sink instead.

Driving a real LiveState through both sinks in BLEManager's own write order, and asserting exact equality with the packet list rather than a count, is what makes this reviewable. So is pinning the pre-fix behaviour in its own test instead of describing it.

Leaving Android alone is the right call, and saying plainly that the two platforms still differ in how often they evaluate, per packet here and per offload there, is better than quietly making them match in a PR about something else.

One thing to drop from the body next time, and it is my fault rather than yours:

The parity ledger, ratchet and governance tests fail locally, and fail identically on a clean checkout of main

True when you wrote it, not true now. main is repaired in 44ef71ebe: the governance suite is 49 of 49 again. The drift was the ledger's own "debt decreased; baseline cleanup is optional" line, which the ledger treats as optional and the governance suite asserts is empty, so eight findings that had gone away were still listed. You are the third contributor to carry that triage in a PR body this week, which is three too many.

Merging this as is.

@ryanbr
ryanbr merged commit 8f064df into ryanbr:main Sep 23, 2026
4 checks passed
@UtkuDenizAltiok
UtkuDenizAltiok deleted the stress-rr-once-per-packet branch September 23, 2026 12:12
UtkuDenizAltiok added a commit to UtkuDenizAltiok/noop that referenced this pull request Sep 23, 2026
…br#2420 MetricKit opened; off-wrist fix awaiting strap test; build 445fe65
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