Repository navigation
diag(hrv): census the two LIVE R-R transports, not just the historical one - #1693
Merged
Merged
Conversation
…l one #1118 instrumentation, prompted by #1689 (thanks @Meriquium, whose log carries the result below). THREE WRITERS PUT R-R ROWS IN ONE TABLE, AND ONLY ONE OF THEM IS CENSUSED. standard 0x2A37 notify stamped at the ARRIVAL wall-clock second no census puffin REALTIME_DATA stamped from the strap's record clock no census historical offload stamped from the record's own second rr emit path=historical The standard path is deliberately unconditional — its own comment says "Record it continuously — independent of the realtime stream or which screen is open" — so on a strap that also banks the same beats, one beat can be stored at the second it arrived AND at the second it was recorded. Different ts, so the (deviceId, ts) primary key cannot collapse them: that is the `crossSecondOverCount` every affected night prints. WHY THIS IS INSTRUMENTATION RATHER THAN A REPAIR. The #1689 log already carries the historical transport's own number: ratioRep=0.99 on the offload, against a stored night reading coverage=2.77. RrEmissionStats' own doc names that case — "near 1.0 while the stored night still reads 1.7 means the duplication is in ingest". One transport measured clean is suggestive; three transports each measured clean while the union reads 2.77 is proof, and only one of the three currently reports. Their cov40=1.97 is the other half of the shape: collapsing same-second near-duplicates leaves almost exactly two full copies. So the same pure census now runs on the two live paths before the rows are stored, emitting `rr emit path=live-standard` and `path=live-realtime` beside the existing historical line. Nothing reads any of it — no scoring, no storage, no schema change. A SMALLER CHANGE THAN THE ONE PROPOSED. The plan was a nullable transport column plus a Room + GRDB migration, because nothing in the stored data can attribute a row to its writer (srcChannel is Oura-only; the WHOOP rows log src=none). That is still true, but it is not needed to ANSWER the question: comparing each transport's pre-storage ratioRep against the stored night's coverage settles which side the duplication is on, and needs no persisted tag. A migration is the right tool for a fix that has to drop one transport's rows; it is the wrong tool for finding out whether that is the fix. Rate-limited to one line per path per 15 minutes: the live paths flush roughly every 30 readings, so a per-flush line would put an entry on the strap log every ~30 seconds all night and bury the evidence it exists to provide. A single flush already spans enough seconds for ratioRep to mean something — the historical line settles it on 39 s. A BUG THE ORACLE FOUND IN MY OWN HELPER. The rate-limit documented "0 = never emitted, so the first flush always reports". It did not: with lastEmit=0 the gap arithmetic returns false for any nowSec under 900. It behaved correctly in production only because a real unix timestamp is ~1.8e9 and clears the gap by accident of magnitude — and that accident is also why a test with small timestamps would have failed. The sentinel is now checked explicitly, and the test table pins exactly those rows. Verification: twin Kotlin/Swift tests over one table, oracle-checked — the Swift helper compiled standalone reproduces the Kotlin expectations row for row, including the clock-moved-backwards case (emit rather than wedge the instrument shut). Android 4604 tests 0 failures on a --rerun-tasks pass. doc_comment_lint OK, i18n green. app-build dispatched: Collector.swift and BLEManager.swift are app-target Swift with no default CI. Not validated on a strap. The lines are emitted from the BLE path, so what they PRINT on real hardware is unproven until someone posts a log — which is the point of shipping them.
…measured Re-review of my own change. The two live census lines were passing `offered` as `inserted` as well, so they printed `offered=N inserted=N` — which reads as "the primary key absorbed nothing". Neither live path is in a position to say that: the census runs BEFORE the insert, and only the store's conflict key knows what it kept. The historical line can say it because `sessionRrInserted` is a real count from the store. Worse, the code comment beside it said "reporting a guessed `inserted` would be the diagnostic asserting more than it observed" — while the call on the next line did exactly that. A comment describing the opposite of its code is worse than no comment, because it defends the defect from the next reader. CLAUDE.md states the rule this broke: a diagnostic may only assert what it can attribute. And the tree already had the precedent. GpsSession's fix line passes `rawFixes = null` for the same reason, with the reasoning written out: pass null "so the line reads rawFixes=n/a instead of rawFixes == accepted, which would falsely imply a 100% accept rate the platform never actually measured". Identical shape, identical answer — I had walked past it in the same file earlier in this investigation. `inserted` is now nullable on both platforms and renders `n/a`. The historical caller passes its real count and is unchanged; the two live callers pass null. Both platforms' logLine shape tests pin the n/a rendering beside the numeric one, so echoing `offered` back cannot quietly return. Android 4604 tests 0 failures on a --rerun-tasks pass. doc_comment_lint OK, i18n green. app-build re-dispatched for the Collector change; the previous commit's run returned Build Strand: success and Build NOOPiOS: success.
Second re-review pass. No defect this time; two undocumented properties that a reader
would otherwise have to derive, and one of them is a real cross-platform asymmetry.
The two Kotlin rate-limit stamps are UNSYNCHRONIZED while sitting beside buffers that
are not. That is deliberate: both flushes can run on the io scope, but a 32-bit write is
atomic on the JVM, so the worst outcome of a stale read is one duplicate diagnostic line
— cheaper than taking collectorLock on a path whose only job is to describe itself. The
Swift twin is @MainActor-isolated and gets the guarantee for free, so the platforms
genuinely differ here and the difference is now stated rather than left to be
rediscovered as a suspected bug.
The stamps also live on the CLIENT, so they reset with it: a strap that reconnects often
emits a line per connection rather than one per 15 minutes. Left alone. Persisting a
diagnostic's rate-limit across reconnects costs more than it saves, and on the 4.0 a
reconnect storm is itself something worth seeing in the log.
Also verified this pass rather than assumed, since the whole instrument depends on it:
- The 4.0 really does drive BOTH live transports. onInbound routes HEART_RATE_CHAR
(0x2A37) to parseStandardHr for every strap, and the reporter's own log shows
TOGGLE_REALTIME_HR payload=01, so puffin REALTIME_DATA is streaming at the same time.
Had the standard path been generic-strap-only, the live-standard line would never
have printed for the strap this was built for.
- Both standard-HR parsers convert 1/1024 s to ms with round(raw / 1024.0 * 1000.0).
An unconverted value would bias every live ratioRep to ~1.024 — a systematic error in
the exact number the comparison turns on.
- StreamPersistence.toBatch maps rr 1:1 with no filtering, so censusing batch.rr on
Kotlin and streams.rr on Swift measures the same set.
- Nothing else constructs a Collector, so the new optional log sink cannot break a
caller.
Android 4604 tests 0 failures. doc_comment_lint OK, i18n green. Comments only — no
behaviour change, so the app-build already run against this Swift still stands.
…it diverges Parity pass, and it caught a false statement in the comment my PREVIOUS pass added. I wrote that the Kotlin rate-limit stamps "live on the CLIENT, so they reset with it: a strap that reconnects often emits a line per connection". That is not what happens. WhoopBleClient is the process-wide lazy singleton on NoopApplication, and a device switch mutates deviceId through setActiveDeviceId rather than rebuilding the client — so on Android the stamps NEVER reset for the life of the process and the 15-minute cadence holds across every reconnect. I asserted the opposite of the behaviour, in a comment added specifically to document that behaviour. The check that found it was a parity one, and the divergence is real but the other way round. Swift keeps the stamps on Collector, which BLEManager.bootstrapStore() REBUILDS — it is called from several places, including a store rebuild after unlock — so an iOS log can carry an extra line after one of those, while an Android log cannot. Both sides now state their own lifetime and name the other's, so a reader comparing two logs does not have to work out why one has more lines. Harmless in effect: it is a rate-limit on a diagnostic, not a measurement. The reason to correct it is that a wrong comment is worse than no comment — the previous pass added it so nobody would have to derive the behaviour, and it would have sent them the wrong way. Rest of the parity audit, built from the code rather than from memory — helper, 900 s constant, nullable `inserted`, both live census sites, the rate-limit test and the n/a rendering pin all present exactly once on each platform. The two deliberate asymmetries are now both documented at both ends: the stamp lifetime above, and the concurrency guarantee (@mainactor on Swift, unsynchronized-by-choice on Kotlin). Comments only. Android 4604 tests 0 failures, doc_comment_lint OK, i18n green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instrumentation for #1118, prompted by the report in #1689 — whose log already carries half the answer.
Three writers put R-R rows in one table; one of them is censused
0x2A37notifyREALTIME_DATArr emit path=historicalThe standard path is deliberately unconditional — its own comment reads "Record it continuously —
independent of the realtime stream or which screen is open" — so on a strap that also banks those
beats, one beat can be stored at the second it arrived and at the second it was recorded.
Different
ts, so the(deviceId, ts)primary key cannot collapse them. That is precisely thecrossSecondOverCountverdict every affected night prints.Why this is instrumentation and not a repair
The #1689 log already reports the historical transport's own number:
ratioRep=0.99, against astored night reading
coverage=2.77.RrEmissionStats' own doc names that exact case —"near 1.0 while the stored night still reads 1.7 means the duplication is in ingest".
One transport measured clean is suggestive. Three transports each measured clean while the union
reads 2.77 is proof — and only one of the three currently reports. The same log's
cov40=1.97isthe other half of the shape: collapsing same-second near-duplicates leaves almost exactly two full
copies of the beat train.
So the same pure census now runs on the two live paths before the rows are stored:
Nothing reads any of it. No scoring change, no storage change, no schema change.
This is smaller than the change I proposed
The plan was a nullable transport column plus a Room + GRDB migration, because nothing in the stored
data can attribute a row to its writer (
srcChannelis Oura-only; WHOOP rows logsrc=none). That isstill true — but it isn't needed to answer the question. Comparing each transport's pre-storage
ratioRepagainst the stored night's coverage settles which side of the store the duplication is on,with no persisted tag at all.
A migration is the right tool for a repair that has to drop one transport's rows. It's the wrong tool
for finding out whether that is the repair.
Rate limiting
One line per path per 15 minutes. The live paths flush roughly every 30 readings, so a per-flush line
would put an entry on the strap log every ~30 seconds all night and bury the evidence it exists to
provide. A single flush already spans enough seconds for
ratioRepto mean something — the historicalline settles it on 39 s.
A bug the oracle found in my own helper
The rate-limit documented "0 = never emitted, so the first flush always reports". It didn't:
With
lastEmit=0the gap arithmetic returns false for anynowSecunder 900. It behaved correctly inproduction only because a real unix timestamp is ~1.8e9 and clears the gap by accident of magnitude
— and that accident is also why a test written with small timestamps would have failed. The sentinel is
now checked explicitly, and the test table pins exactly those rows.
Verification
reproduces the Kotlin expectations row for row, including the clock-moved-backwards case (emit rather
than wedge the instrument shut).
--rerun-taskspass;doc_comment_lintOK; i18n green.Collector.swift/BLEManager.swiftare app-target Swift with no default CI, so app-build wasdispatched on this branch.
Not validated on a strap
These lines are emitted from the BLE path, so what they actually print on real hardware is unproven
until someone posts a log — which is the point of shipping them. The next 4.0 log with all three lines
on it settles #1118's mechanism.
Type of change
Checklist
StrandAnalyticsneeds macOS (GRDB via WhoopStore), left toswift-packages.yml; the new helper was oracle-verified standalone