Repository navigation
sleep: let the sparse bridge cross one short active run - #1755
Merged
Merged
Conversation
A brief wake ended the night. @justinjor-bit's report in #1657: up once for the bathroom, straight back to sleep, and a 6h40m night scored 150 minutes with eff=0.96 - a confident, clean, wrong answer that then fed the 21-day baseline every later night is judged against. Two mechanisms, both confirmed against the code and both fixed here. THE BRIDGE COULD NEVER REACH THE CASE IT WAS BUILT FOR. bridgeSparseSleep merged only runs already adjacent in its own output: an intervening active run was appended first, so the next pair was unreachable. A bathroom trip is definitionally an active run, so the rescue for fragmentation was structurally unavailable in the scenario that needs it most. The reporter's trace put a number on it - the bridge merged NOTHING on 14 of 14 sparse nights, runsAfter == runsBefore every time - and the pieces then died at the 60-minute session floor. One active run up to sparseBridgeActiveMaxMin is now absorbed, the whole span still subject to sparseBridgeGapMin and to the HR band. Two or more consecutive active runs are not: that is a night with structure in it, not one interruption. The bound is DEFINED as maxGapMin rather than given a number of its own. That is already this file's threshold for "a discontinuity this long is decisive", and reusing it keeps one such judgement in the detector instead of two that drift. It is also the cheap half of the guard: the real one is the HR band, which a genuine wake fails because it keeps HR elevated for its duration. mergeMin (15) already absorbs shorter active runs upstream in mergePeriods, so this covers the band between that and a wake long enough to be a real break. THE HR GATE USED THE WRONG STATISTIC. hrSleepBandAcross judged on the MEAN, which a single arousal spike drags out of band. confirmSleepWithHR, twelve lines below, already documents exactly this - "brief arousal / wake HR spikes (observed to ~190 bpm) ... pull the MEAN above baseline x mult and reject the run" - and uses the median for that reason. This gate asks the same question over a SHORTER window, where one spike dominates the mean even harder. Median now, which still rejects a SUSTAINED elevation just as firmly - that is the discrimination the gate exists to make, and without it removing the active-run blocker would not have helped the target case anyway. Also removes the shadow tracer. Swift kept a second copy of the bridge loop purely to emit trace lines, which had to be edited in step with the real one; the two are one pass now, so a trace cannot silently disagree with the behaviour it describes. Android had none of that machinery at all - the line that made this issue diagnosable was iOS-only, which is why the reporter being on iPhone is the reason we know any of this. Ported, and both platforms now report the intervening active run's length, which is the number the old trace could not supply. Two comments corrected rather than left. The bridge's own doc said "Active runs ... are left untouched; the span between two bridged sleep runs (an "active"/gap run, if present) is absorbed", which reads as though the case was handled - it was not, and that sentence is a large part of why this went unnoticed. And the empty-attempts trace line no longer claims "fragments separated by active runs", because that cause has just been removed; an empty list now means one sleep run, or fragments split by CONSECUTIVE active runs. One existing Swift test asserted "an intervening active run is never a considered pair". True when written, and it was the bug. Repinned to the same shape with the opposite expectation - the third test today that was correct and had quietly become a guard on the wrong behaviour. NOT changed: minSleepMin stays at 60. The reporter also shows 45-57 minute runs dropped by it, which is real, but it is a separate judgement about what counts as a session and it deserves its own evidence rather than riding along here. With the bridge working, fewer runs reach that floor fragmented in the first place. Android 4,935 tests and StrandAnalytics 1,669 (one skip), zero failures on both. Doc lint and the i18n gate clean. Not validated on a real interrupted night - the reporter has not caught one with the trace on yet, so the unit cases stand in for it.
Re-review, and the end-to-end test earned its place on the first run. The unit cases all passed with the bound at maxGapMin (20). A test through the real detectSleep pipeline did not: a FIFTEEN-minute interruption produced a TWENTY-ONE minute active run, so the bridge rejected it as activeTooLong and the night still came back as two sessions. The fix did not reach the case the issue is about, and nothing at the unit level could have shown that. The cause is structural rather than incidental. classifyStill smears the still/moving boundary by roughly its rolling window, and buildRuns closes runs at sample edges, so the DETECTED active run is systematically longer than the interruption it represents — about six minutes longer here. A bound reasoned from how long someone is actually out of bed will always be too tight, because that is not the quantity being measured. Bound is 30 now, and the doc says plainly that it is a judgement rather than dressing it as derived: a realistic trip of up to about a quarter of an hour, plus the ~6 minutes of smear that measurement showed, with headroom. Well under minSleepMin, because an interruption long enough to be a session in its own right is a genuine awakening and should split the night. The HR band remains the substantive guard, and the awake-interruption test still refuses to bridge at 110 bpm. Two end-to-end cases added, both through detectSleep on a sparse fixture with a real 15-minute up-and-about: one asserts the night survives as a single session, the other that an elevated-HR interruption does not. The reasoning that produced 20 was tidier than the reality; only the pipeline knew. Android 4,937 tests and StrandAnalytics 1,669 (one skip), zero failures on both. Doc lint and i18n clean.
Third review pass. The two detectSleep cases existed only in Kotlin, which is
backwards: that pair is what caught the bound being too tight, and the reason it
was too tight is a property of THIS pipeline — classifyStill smears the
still/moving boundary and buildRuns closes at sample edges, so a 15-minute
interruption becomes a 21-minute run. Leave the twin out and a divergence in
Swift's windowing would make 30 silently wrong on Apple alone, with every unit case
still green.
They pass unchanged, so the smear is the same on both and the bound holds. Eleven
cases each side now.
Also worth recording what the disturbance path does with an absorbed interruption,
since the reporter asked for exactly that ("a disturbance, not the end of the
night"). A wake segment inside a session adds to WASO and increments `disturbances`
(HypnogramMetrics), so a wake that IS staged is preserved rather than deleted.
But both stagers call wake primarily off HR, not motion — the file says so at the
motion-corroborated-wake note, and the motion-aware refinement is opt-in and only
ever SHRINKS wake. So an interruption the bridge absorbed, which by construction
had HR in the sleep band, will generally stage as light sleep rather than wake. The
absorbed minutes do land in TST.
That is the honest description and it is not the one given earlier in this branch:
"staged awake, so no inflation" was wrong. The right framing is that the HR gate
decides. Movement with sleep-band HR is a stir scored as sleep, which is defensible;
movement with elevated HR never reaches the bridge at all. Trading four lost hours
for fifteen generously-scored minutes is a far smaller error, and pointed the right
way — but it is an error, not a clean win, and the next reader should not have to
discover that from the code.
StrandAnalytics 1,669 (one skip), zero failures. Doc lint clean.
Parity re-review. Everything lines up - constants both 30, reason tokens and their ORDER identical, gate names identical, hrSleepBandAcross down to one definition and two call sites each side (Swift correctly lost its third when the shadow tracer went), and the eleven test cases map one-to-one with nothing one-sided. The three emitted trace lines are byte-identical too, after normalising the interpolation syntax. But that was checked BY HAND, and nothing in the tree keeps it true: Tools/parity_cases holds one unrelated fixture, and neither suite asserted the rendered line at all - only the fields behind it. A format change on one platform would have diverged in silence and surfaced when somebody tried to read a capture from the other. Both suites now assert the same whole rendered string, byte for byte, which is the idiom this repo already uses elsewhere for exactly this reason. It is not a real cross-language oracle - nothing compares the two automatically - but it converts a silent divergence into a failing test on whichever side moved, and a reviewer looking at one literal can see the other. Android 4,938 and StrandAnalytics 1,672 (one skip), zero failures. Doc lint and i18n clean.
ryanbr
added a commit
that referenced
this pull request
Aug 31, 2026
Re-review of the contention fix, and it had a hole of its own. `disChainInFlight = true` was set BEFORE calling readDisIdentity, which has five early returns above the read it issues - already read this link, no gatt, a WHOOP4, no gattOps - plus a branch where the serial characteristic is unavailable. On any of those the chain never starts, yet the flag said it had. The probe would then stand aside for eight seconds waiting on something that was never running, and the capture would say "waiting for the DIS read chain" when there was no chain. The bounded cap kept it from hanging, which is exactly why it would have gone unnoticed. readDisIdentity now returns whether it ISSUED a read, and the flag is set from that answer instead of from the intention to ask. Every exit above the read returns false. Also moves the wait decision into a pure helper beside the rest of this file's judgements, with the cap next to it. It was inline in the client: argued for in a comment and asserted nowhere, which is precisely how #1755 shipped a bound every unit test agreed with and the real pipeline rejected. Four cases now cover it - waits while running, stops at the cap, and never waits when no chain started. 4,941 tests, zero failures. Doc lint and i18n clean.
ryanbr
added a commit
that referenced
this pull request
Aug 31, 2026
) Found in the 13:17 capture, and it was my own scheduling rather than the strap. DIS reads and CCCD writes share ONE serialized GATT queue. The probe was posted on a fixed 6s delay, chosen on the reasoning that the DIS chain - posted at 3s - would be done by then. On one link it was still running at 7s: 07:57:06 writeDescriptor busy for fd4b0003 ... retry 1/8 07:57:06 ... 8 busy retries across the four chars, shared budget exhausted 07:57:06 writeDescriptor rejected for fd4b0003/4/5/7 (gave up after 8 retries) 07:57:06 Unbonded offload probe: none of the 4 ... completed their subscribe, and none was refused either ... proves nothing either way 07:57:07 DIS: modelNumber=MG <- the chain, still going Not one descriptor write reached the air. Reasoning about a delay lost to measuring one. The probe now waits on the actual signal, checked once a second and bounded at eight checks, then goes anyway and says which happened. The same log also handed over a control case that #1749 asserted without one. Every link whose CCCD writes reached the air died at 10.8s - 4.8s after the probe, by localTerminate, the CLIENT_HELLO's own signature. The ONE link where all four writes were abandoned locally lived 31.3s and died of an ordinary connectionTimeout. The teardown tracks the write reaching the strap, not the probe running. Three review passes, and all three found the same shape of defect: a flag claiming something that had not happened. 1. the contention itself 2. disChainInFlight was set BEFORE readDisIdentity, which has five early returns above the read it issues plus an unavailable-characteristic branch - so on any of those the probe stood aside for a chain that never started. readDisIdentity now reports whether it ISSUED a read, and the flag is set from that answer rather than from the intention to ask. 3. readNextDisExtra is only reached from the SUCCESS path, so a REFUSED read ended the chain while the flag stayed set - and #490's whole subject is a strap that refuses DIS, so that was the case the probe most wanted a clear queue for. All three were survivable because the cap bounds them, which is exactly why they would have gone unnoticed: a wrong wait that self-corrects looks like a working wait. The wait decision also moved into a pure helper beside the rest of this file's judgements. It was inline in the client - argued for in a comment and asserted nowhere - which is precisely how #1755 shipped a bound every unit test agreed with and the real pipeline rejected. Parity checked rather than assumed: the DisUnbondedRead pure twin pair is untouched, four functions each side, and Swift performs its DIS read inline with no readDisIdentity counterpart, so the Bool return has no twin to keep in step. That structural difference predates this work. Everything else is the probe, Android-only by design. Staging 396 / iOS 277. 4,941 tests, zero failures. Doc lint and i18n clean. The next capture is what confirms the probe now starts on a clear queue.
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.
A brief wake ended the night. From @justinjor-bit's report: up once for the bathroom, straight back to sleep, and a 6h40m night scored 150 minutes at
eff=0.96— a confident, clean, wrong answer that then fed the 21-day baseline every later night is judged against.Two mechanisms, both confirmed against the code, both fixed.
The bridge could never reach the case it was built for
bridgeSparseSleepmerged only runs already adjacent in its own output:An intervening active run is appended first, so the next pair is unreachable. A bathroom trip is definitionally an active run — so the rescue built for fragmentation was structurally unavailable in the scenario that needs it most.
The reporter's trace put a number on it: the bridge merged nothing on 14 of 14 sparse nights,
runsAfter == runsBeforeevery time. The pieces then died at the 60-minute session floor.One active run up to
sparseBridgeActiveMaxMinis now absorbed, with the whole span still subject tosparseBridgeGapMinand to the HR band. Two or more consecutive active runs are not — that is a night with structure in it, not one interruption.The bound is defined as
maxGapMin, not given a number of its own. That is already this file's threshold for "a discontinuity this long is decisive", so reusing it keeps one such judgement in the detector instead of two that drift apart. It is also the cheap half of the guard — the real one is the HR band, which a genuine wake fails because it keeps HR elevated for its duration.mergeMin(15) already absorbs shorter active runs upstream inmergePeriods, so this covers the band between that and a wake long enough to be a real break.The HR gate used the wrong statistic
hrSleepBandAcrossjudged on the mean, which a single arousal spike drags out of band.confirmSleepWithHR, twelve lines below, already documents exactly this — "brief arousal / wake HR spikes (observed to ~190 bpm) … pull the MEAN above baseline × mult and reject the run" — and uses the median for that reason.This gate asks the same question over a shorter window, where one spike dominates the mean even harder. Median now. It still rejects a sustained elevation just as firmly, which is the discrimination the gate exists to make — and without this, removing the active-run blocker would not have helped the target case anyway.
The shadow tracer is gone
Swift kept a second copy of the bridge loop purely to emit trace lines, which had to be edited in step with the real one. They are one pass now, so a trace cannot silently disagree with the behaviour it describes.
Android had none of that machinery at all. The line that made this issue diagnosable was iOS-only — which is why the reporter being on iPhone is the reason we know any of this. Ported, and both platforms now report the intervening active run's length, the number the old trace could not supply.
Two comments corrected rather than left
A repinned test
One existing Swift test asserted "an intervening active run is never a considered pair". True when written — and it was the bug. Repinned to the same shape with the opposite expectation.
Not changed
minSleepMinstays at 60. The reporter also shows 45–57 minute runs dropped by it, which is real, but it is a separate judgement about what counts as a session and deserves its own evidence rather than riding along here. With the bridge working, fewer runs reach that floor fragmented in the first place.What the absorbed minutes become
Worth stating precisely, because the reporter asked for a disturbance rather than a truncation.
A wake segment inside a session adds to WASO and increments
disturbances(HypnogramMetrics), so a staged wake is preserved rather than deleted. But both stagers call wake primarily off HR, not motion — the motion-aware refinement is opt-in and only ever shrinks wake. An interruption the bridge absorbed had HR in the sleep band by construction, so it will generally stage as light sleep, and those minutes land in TST.So the HR gate is what decides: movement with sleep-band HR is a stir scored as sleep, which is defensible; movement with elevated HR never reaches the bridge. Trading four lost hours for ~15 generously-scored minutes is a far smaller error pointed the right way — but it is an error, not a clean win.
Verification
detectSleepon a sparse fixture with a real 15-minute up-and-about, on both platforms--cigate cleanAddresses the detection half of #1657.