Repository navigation
sleep: give the HR-only spine a funnel line, because it shipped silent - #1812
Merged
Merged
Conversation
#1801 landed a whole detection path that emits nothing. A field log on 11.1.0 build 412 then showed sleep-detect day=2026-09-02 NO-NIGHT hr=161126 rr=104007 resp=0 grav=0 steps=0 provided=0 window=50h reason=no-motion with no way to tell whether the spine ran and found nothing or never ran at all - `provided=0` says only that it returned nothing. Zero mentions of it in 3,702 lines. That is the wrong shape for this file: sleep-detect, the [sleep] gate lines, #1719 and #547 all exist because this area is only debuggable as a funnel, and I reworded the no-motion comment to claim HR had been tried while leaving nothing to show for it. The line names what the spine derived and where the candidates went: [sleep] hr-only spine anchorBpm=61.0 bandBpm=64.1 epochs=3021 runs=48 merged=12 sleepRuns=7 longestMin=41 staged=0 kept=0 minSleepMin=60 anchorBpm and bandBpm are printed because they are DERIVED, not configured - the anchor is a percentile of the wearer's own window, so the same code gives every wearer a different threshold and a complaint is unreadable without knowing which one they got. sleepRuns against longestMin separates the two failures that actually happen: a band too tight, versus the duration gate eating real runs. Emitted on every call, including the no-anchor early return, where the anchor prints `nil` rather than a fabricated 0.0. Two things caught while writing it. `epochs` first reported the HR SAMPLE count, which would have made the diagnostic lie on the axis every other number is measured on. And the test asserting a flat 120 bpm window is rejected was wrong: the anchor is the tenth percentile of the SAME window, so a constant rate puts everything in band and the whole window becomes sleep. That is inherent to a relative anchor, and is now pinned as a property rather than corrected away. 24 HR-only tests, 0 failures. Refs #1801.
… on each platform Re-review of the PR, three findings. KOTLIN-ONLY WAS WRONG. The spine exists on both platforms and is equally silent on Apple, and this file's formatters are twinned precisely so an Android and an Apple log of the same night read side by side - #1796 said so explicitly about the standard-HR lines. Adds the Swift twin and routes it through the Apple day scan. THE ROUNDING DIVERGED, which a harness caught before it shipped. round1 used String.format / String(format:), and they are not twins: Java rounds HALF_UP on the decimal expansion, Swift goes through C printf and rounds half-to-even on the binary value. On 64.05 that is Kotlin 64.1 against Swift 64.0 - and the band is anchor * 1.05, so it produces trailing decimals constantly and would have disagreed on most real values. Both now multiply by ten and round, which is IEEE-deterministic and cannot disagree whichever side of .5 the product lands on. Ten values checked across both toolchains, including the one that broke it. Same reasoning as round2 directly below, which was already arithmetic - I wrote a printf twin next to an arithmetic one and did not notice. THE APPLE SINK WAS UNGATED. Apple's sleep trace is nil unless Sleep test mode is active; I passed a closure of my own, which would have appended on every scored day regardless. Now uses the existing traceSink. Android's dayDiag stays always-on, as it already was - that asymmetry is pre-existing and not this change's to alter. 24 HR-only tests, 0 failures. Swift twins compile; the package test binary cannot link GRDB's sqlite3 on Linux, so CI runs them.
Re-review pass two. Checked first whether replacing round1 had changed any EXISTING line's output - it had not, because round1 did not exist on main at all: I introduced it in the first commit of this branch and rewrote it in the second, so there are no other callers. Worth confirming rather than assuming, since silently altering a shared formatter would have moved lines nothing in this PR is about. round1 is non-negative only: -0.5 prints "0.5", because the integer division truncates toward zero and the sign is then lost to abs. Every caller is a bpm so it is unreachable today, and it is a trap for the next caller, so both twins now say so. Also twinned the epochs test. Kotlin had it and Swift did not, and it guards a bug I actually made - the first version of the line reported the HR SAMPLE count on the axis every other number is measured against. The implementations are structurally identical, which is exactly why the guard should exist on both rather than trusting that they stay that way. Verified this pass and unchanged: the two hrOnlySessions bodies are line-for-line equivalent, with `staged` incremented at the same point (after staging, before the empty check) and longestSleepS taken before the duration gate, so the funnel's numbers mean the same thing on both platforms. 24 HR-only tests, 0 failures; Swift tests compile; doc_comment_lint clean.
… spread Answering "what if this still does not explain it". Two gaps the funnel line alone leaves. THE LINE ONLY EXISTS WHEN THE SPINE IS CALLED, so its absence meant either the gate never opened or the sink was never wired - which is exactly the ambiguity that made the first field report unreadable, reproduced one layer up. Now every day with NO motion states the decision: [sleep] hr-only gate attempted=true reason=no-motion-no-hypnogram grav=0 stored=0 [sleep] hr-only gate attempted=false reason=stored-hypnogram grav=0 stored=3 [sleep] hr-only gate attempted=false reason=imported-owner grav=0 stored=0 A day WITH motion prints nothing: the motion spine is the answer there and nothing was passed over, so a line would be noise rather than evidence. AN ANCHOR ALONE CANNOT BE JUDGED. p10=60 means one thing when the median is 63 and quite another when it is 74, and only the second leaves a night the band can separate. The line now carries hrP50 and hrP90 from the same nearest-rank helper the anchor uses - measuring the spread a different way would invite precisely the wrong conclusion. The guard test caught the restructure, correctly: it measured PROXIMITY, asserting the gate appeared within 1200 characters before the call, and inserting two diagnostic lines between them broke it while the gate still held. Now scoped to the providedSleep expression and asserting CONTAINMENT, which is the actual invariant - distance never was. Both platforms. 24 HR-only tests, 0 failures; swift builds; doc_comment_lint and i18n_audit clean.
Re-review pass three. Adding hrP50 and hrP90 to the funnel line meant three calls to hrPercentile, and each one sorted the whole window: on a 5/MG day that is ~160k samples sorted THREE times, on every scored day, across the 21-day rescore window. This file has been burned by exactly that shape before (#836, #841), and the diagnostic I added to explain a problem should not become one. The sort is now done once in hrOnlySessions and read three times through percentileOfSorted, which the anchor also uses - so the spread the trace reports and the threshold it is meant to be judged against come off the same sorted axis by construction, not by two implementations agreeing. Net effect on a normal pass is one sort, the same as before this PR: the earlier code already sorted once for the anchor. Worth noting what did NOT need fixing: both platforms build the line inside a safe/optional call, so when the sink is nil the arguments are never evaluated and none of this runs at all. That is why the cost only lands on Android, whose dayDiag is always-on. 25 HR-only tests, 0 failures; swift builds; doc_comment_lint clean.
Perf audit of this PR, second finding. The epoch count built a set over EVERY sample -
mapTo(HashSet()) on Kotlin, Set(hrS.map{}) on Swift, the latter allocating a full intermediate array
as well - to arrive at a few thousand distinct keys. That is ~160k boxed values per scored day across
the 21-day rescore, and it is precisely the allocation shape #707 names as the heap-churn source
under the OOM.
hrS is already sorted by timestamp, so the bucket key is non-decreasing and a running comparison
counts them with no allocation at all. Pinned by a test, because the zero-allocation version trades
the set for that ordering assumption: unsorted input would silently over-count rather than fail.
The rest of the audit came back clean, and the parts that look expensive are not this PR's. The bpm
boxing in the percentile sort and the epoch HashMap inside hrOnlySleepRuns both predate it - they
arrived with the spine in #1805/#1806 - and this change does not add to either. Worth a separate look
if the spine ever runs on every day rather than only motionless ones; not worth widening a
diagnostic PR to chase.
26 HR-only tests, 0 failures; swift builds; doc_comment_lint clean.
ryanbr
added a commit
that referenced
this pull request
Sep 2, 2026
Build numbers only: versionCode 412 -> 413 and CURRENT_PROJECT_VERSION 293 -> 294. MARKETING_VERSION and versionName stay 11.1.0, because 11.1.0 has not been released - only cut as a testing build - so the artefacts stay v11.1.0 and 413 simply beats the staging high-water an installer would otherwise refuse as a downgrade. Carries the HR-only spine's funnel and gate lines (#1812), which is the point of this build: the first log from it will say whether the p10 band is too tight for the wearer or the minimum-duration gate is eating real runs, instead of leaving reason=no-motion to mean either.
ryanbr
added a commit
that referenced
this pull request
Sep 2, 2026
…eep WHOOP out (#1813) The spine had never once run. The gate line from #1812 said so on the first build that carried it: [sleep] hr-only gate attempted=false reason=imported-owner grav=0 stored=0 on a day holding 165,980 HR rows - skipped before any heart rate was looked at. Neither of the two failures the funnel was built to separate; it never got that far. #1806 took its condition verbatim from #804 Fix A, whose comment says it exists "so WHOOP straps and the my-whoop import namespace are untouched". #804 is about Oura RINGS and deliberately excludes WHOOP. The HR-only fallback is FOR a WHOOP strap, so it inherited a condition whose entire purpose was to exclude the case it was built for. resolveDayOwner returns importedDeviceId whenever the owner source is absent or the candidates collapse to it, which on a live 5/MG install is every day - so the p10 anchor tuned in #1806 was never reached either. Two owner concepts disagree, which is why older logs could not show this: the [universal] dayOwner line reports the strap id while resolveDayOwner returns the imported one. Split by question rather than by gate. #804's stored-hypnogram branch keeps the owner exclusion. The heart-rate fallback drops it. Between them a new branch leaves any day alone that already has stored sessions, whoever owns it, because inferring a night from heart rate when the device recorded a real one replaces better evidence with worse. The stored lookup now runs for every no-motion day, so "nothing else knows about this night" is checked rather than assumed - one indexed read, and only on days that were already scoring blank, since a normal WHOOP day short-circuits at grav.size >= 2. Re-review recorded what dropping the exclusion widens: the branch is now reachable for ANY owner, including a WHOOP 4.0 day that banked nothing at all. A normal 4.0 day never reaches the gate, and an empty one has too little to clear minSleepMin - but "too little" is not "none", and I had described a 4.0 as untouched more broadly than was true. Both platforms. 26 HR-only tests, 0 failures. The Apple half is app-target Swift that no default CI job builds; the testing build's iOS and macOS legs are what compile it. Refs #1801.
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.
Refs #1801. Instrumentation only — no behaviour change.
Why
#1801 landed a whole detection path that emits nothing. A field log on 11.1.0 build 412 (which
does contain the wiring — verified) shows:
provided=0says the spine returned nothing. It does not say why, or even whether it ran.Zero mentions in 3,702 lines.
That is the wrong shape for this file.
sleep-detect, the[sleep] gatelines, #1719 and #547 allexist because this area is only debuggable as a funnel — and I reworded the
no-motioncomment toclaim heart rate had been tried while leaving nothing to show for it.
The line
anchorBpm/bandBpmare printed because they are derived, not configured. The anchor is apercentile of the wearer's own window, so the same code hands every wearer a different threshold; a
report is unreadable without knowing which one they got.
sleepRunsagainstlongestMinseparates the two failures that actually occur: a band tootight (
sleepRunsnear zero) from the duration gate eating real runs (sleepRunshigh,longestMinunderminSleepMin).Emitted on every call, including the no-anchor early return — where the anchor prints
nilrather than a fabricated
0.0.Two things caught while writing it
epochsfirst reported the HR sample count. That would have made the diagnostic lie about theaxis every other number is measured on.
percentile of the same window, so a constant rate puts everything in band and the whole window
becomes sleep. Inherent to a relative anchor — now pinned as a property rather than corrected away.
What this is for
The strongest hypothesis for the empty result is that the p10 anchor is too tight for this
wearer: #1805 used the window median (band ≈ 77 bpm on their data), #1806 replaced it with p10 to stop
it calling half the day sleep, and that likely put the band near 63–65 against a night running 59–70.
This line will confirm or kill that in one build, instead of tuning on a guess.
Verification
compileFullDebugKotlin,i18n_audit --ci,doc_comment_lintclean.
Re-review found three things
these formatters are twinned precisely so the two logs read side by side — android: twin standardHRHostReceivedLine and be honest about take(200) #1796 said so explicitly
about the standard-HR lines. The Swift twin and the Apple day-scan wiring are now here.
round1usedString.format/String(format:), which are not twins: Java rounds HALF_UP on the decimalexpansion, Swift goes through C
printfand rounds half-to-even on the binary value. On64.05that is Kotlin 64.1 against Swift 64.0 — and the band is
anchor * 1.05, so it producestrailing decimals constantly and would have disagreed on most real values. Both now multiply by ten
and round, which is IEEE-deterministic. Ten values verified across both toolchains. The galling part
is that
round2, one line below, was already arithmetic for exactly this reason.nilunless Sleep test mode is active; Ipassed a closure of my own, which would have appended on every scored day. Now uses the existing
traceSink. Android'sdayDiagstays always-on as it already was — that asymmetry is pre-existing.