Repository navigation
fix(sleep): recover unmatched off-wrist tails from sustained HR - #2375
fadkeprasad wants to merge 4 commits into
Conversation
ryanbr
left a comment
There was a problem hiding this comment.
Thanks @fadkeprasad. The diagnosis is right and the recovery rule is the careful half of this: pure, conservative in the ways that matter (one invalid sample resets the run, duplicate timestamps resolve to invalid, the boundary is an observed sample rather than a fabricated event), a genuine line-for-line twin, and tested on both sides. firstSustainedHR is the kind of function this repo likes.
The retrospective repair is where I would ask for changes.
iOS does the full-history work twice, Android once
Android widens the existing repair's gate:
flagGet = { effortRescoreDone(ctx) && sleepWearRescoreDone(ctx) }
flagSet = { setEffortRescoreDone(ctx); setSleepWearRescoreDone(ctx) }One pass, doing both jobs. That is the better design of the two.
iOS instead adds a SECOND pass next to the first, in the same launch task:
await self.intelligence.runEffortRescoreIfNeeded() // analyzeRecent(maxDays: 4000)
await self.intelligence.runSleepWearRescoreIfNeeded() // analyzeRecent(maxDays: 4000)To be precise about who pays: an install that already completed the Effort rescore runs one pass on both platforms. An install that has not runs one on Android and two on iOS, back to back, awaited at launch. Given #1538 (a re-score that could not finish in the background at all) and that a 21-night pass measured 11.5 s on a current device, a 4000-day pass is not something to do twice by accident.
I would make iOS do what Android does.
The per-day persistence loop is quadratic
With preserveUnscoredHistory, windows becomes one (day, day) pair per scored day, and inside that loop each of persistedDailies, restPoints, provenanceByCell.values and markerPoints is filtered by that day. For N scored days that is N passes over each collection, and N separate persistComputedScores transactions where there used to be one.
The intent is right: a full-history repair must not erase cached scores whose raw inputs are gone. But grouping once (Dictionary(grouping:by: \.day)) gets the same guarantee in one pass, and keeps the store round-trips proportional to days rather than days-times-rows.
An unrelated feature changed quietly
Android passes preserveUnscoredHistory = true into runEffortRescoreIfNeeded, so #313's Effort repair now also persists per day and preserves cached days. That may well be an improvement, but it is a behaviour change to a different feature and it is not in the description.
Smaller
- Coupling the flags means neither repair can ever run alone again. Fine today; worth a comment saying so, since the next one-shot to use the trick inherits it.
- The base is
a56840bb9, now 20-odd commits behind, and several of those touch the same analytics files. CI is green but against that base, so it will want a rebase before it means anything. - The
RecoveryDriversTestrounding failures you hit are not yours: they are an arm64 JDKexpulp difference on a half-tie (-0.4999999999999929against an exact-0.5), green on CI's x86 runner. Several PRs have reported them; nothing to fix.
|
Thanks Ryan, agreed on the repair changes. I’ll make iOS use one shared full-history pass for the pending Effort/sleep repairs, group each collection once on both platforms, document the coupled flags and the changed Effort persistence behavior, and rebase before rerunning validation. Thanks for confirming the arm64 rounding failures too. I also checked the reported shifting sleep times and delayed loading before expanding this patch. A prefix replay with the recovery fix, using both stagers, ends an unfinished night at the available-data cutoff. Once the completed-night data is included, the wake boundary stabilizes in that capture; onset stays stable throughout. That establishes a partial-data case to test, but does not yet explain the separate erratic-onset report. I saw the new Apple Health open-night guard on main; I’ll check the in-app presentation separately rather than assume the export fix covers it. For loading, the scoring path still refreshes the repository near the end of the full pass, after other scoring and persistence work. I’ll profile sync completion → queue → detection → persistence → visible result, including background suspension, before choosing a change. Cache hits alone are not enough to establish that this path is fast. I’ll keep this PR focused on unmatched wear-event recovery and its safe historical repair. Boundary stability and time-to-display will get separate, reproducible follow-ups, with edited/dismissed sleep and cached history protected. No new accuracy claim from a single matching night. |
4d1a61f to
f4fcede
Compare
|
Ryan, the requested migration revisions are pushed in f4fcede, rebased onto upstream main at 9d85ea8. iOS now runs one shared full-history pass when either repair flag is pending and marks both flags only after successful persistence. The shared pass preserves cached-only days, so the Effort repair now writes only recomputed days. Both platforms group daily metrics, rest points, provenance, and marker points by day once before per-day persistence. I added coverage for all four flag combinations and the per-day preservation behavior. Local checks: StrandAnalytics 2,059 tests passed; doc-comment lint, i18n audit, and diff check passed; Android FullDebug app compilation passed. The Android unit-test task is blocked by the existing unrelated nullable-type compile error in StressRawReadScopeTest.kt:10 and a missing generated Room schema directory (local JDK is 21; repo requires 17). The macOS app test is blocked because xcodegen is unavailable and the checked-in generated project omits LiftRunningClock and StrapLogArchive sources. CI has started on the updated head; I’ll request review once the new checks finish. |
|
Correction to the original PR description's “How it was tested” section: StrandAnalytics passed 2,059 tests locally; Android targeted sleep-history and JaCoCo budget tests passed locally; Android FullDebug compilation, doc-comment lint, i18n audit, and diff check passed. The full Apple app test could not run locally because xcodegen is unavailable and the generated project omits LiftRunningClock and StrapLogArchive. The full Android unit suite is not locally verified (JDK 21 is installed while this repo requires 17; the existing StressRawReadScopeTest nullable-type diagnostic blocks the local task). The first CI run exposed a JaCoCo bytecode-budget regression in analyzeRecentOnCpu. I moved the persistence dispatch into the shared helper in c8909ee and reran the Android history and JaCoCo tests successfully. CI is rerunning on that commit; it currently has 7 of 16 checks successful, with 6 running and 3 queued. I will request review after all checks finish. |
c8909ee to
4739e25
Compare
|
I now have a reproducible iOS case from a WHOOP 5 capture. The old installed build (11.8.0 build 404) persisted Sep 25 23:24 to Sep 26 04:16, but the continuous raw HR/sleep-state stream remains asleep through the night and the detector resolves the wake boundary at 09:13 local. After installing testing build 419 and restoring the verified local history, the on-device row persists 23:24 to 09:13 and remains stable across relaunch. This suggests late/offloaded data is correct but the older persistence path can leave a completed night stale; the historical repair needs to rescore completed nights after the full stream is available. |
4739e25 to
cc1cb8f
Compare
|
@fadkeprasad Reviewed the current head. The core is good and the guards around the repair are the right ones. One blocking thing, which your CI could not have told you about. The recovery rule is conservative in the ways that matter. Five minutes of 30 to 220 bpm with no gap over five seconds, invalid samples reset the run, duplicate timestamps resolve "invalid wins", and it returns the first observed sample of the confirmed run rather than the OFF timestamp. That last choice is the one I would have argued for: it recovers a boundary from evidence instead of inventing one. The Kotlin twin is line for line equivalent, including the strict inequalities on the window and the reset semantics. The repair is guarded properly. One-shot, reentrancy-guarded, I also want to name something your summary undersells. Blocking: the parity authority driftsRebased onto current
Your "17 successful checks" is accurate and is not the issue. That run predates #2582, which moved the repository-acceptance suite onto the unfiltered leg a few hours ago, precisely because a change could move the declaration graph without the gate that measures it ever running. #2557 did exactly that earlier today and Suggested remedy, and I would try the first one. Neither If claim resolution does not take, the fallback is the one the tool names: an exact typed disposition for the platform-qualified identity, then refresh. That records debt which I do not think actually exists here, so it is worth trying the pairing first. One thing to measure before this ships
On a WHOOP 5 or MG that may be worse than it looks. R-R window reuse is disabled for that whole family, because the WHOOP 5 R-R read picks its channel with a For scale, from a field strap log today: a cold 21-day pass read 2.25M R-R rows with zero served from reuse, and spent 68 to 80 seconds in read and prep against about 3 seconds scoring. I am not asserting what 4000 days costs from that, which is exactly why it is worth a measurement on a 5 or MG install with deep history before this lands. If it is minutes, the pass is still correct and still retries, but a user will see it. Sort the authority and I will take it. The recovery logic itself I have no further questions about. |
cc1cb8f to
ba2c613
Compare
|
@fadkeprasad The authority is sorted, and I checked it the way that matters rather than from the green The twin claims did exactly what I hoped. One thing is still red, and it is not your logic
The test pins a literal: self.assertIn("dailyMetrics: persistedDailies", persistence)Your first commit replaced that argument with a ternary, so the literal is gone. But the invariant the
So repin it rather than deleting it. Something shaped like self.assertIn("let dailiesByDay = Dictionary(grouping: persistedDailies, by: \\.day)", persistence)
self.assertIn("? dailiesByDay[from, default: []] : persistedDailies", persistence)pins the same invariant through the new shape. Leave the Why this only showed up on day eight, which is on meThat test reads PRODUCT source ( That is the same shape as #2582, which moved the repository-acceptance suite onto the unfiltered leg Non-blocking, for whoever picks up #2567Four of the seven new dispositions ( Worth recording: when #2567 widens Swift scope to take in Still open from last timeYour updated body records the full-history WHOOP 5/MG foreground runtime as unmeasured, with the box Repin the contract test and I will take this. |
|
@fadkeprasad Rather than have you push again for four lines, I stacked your branch onto current To be exact about what moved: The repin pins both branches of the ternary instead of the single literal, and I mutation-checked it It had to ride along with your Swift, since Once #2585 goes green I will rebase-merge it so your commits land as yours. Thanks for four rounds on |
|
Landed on Marking this one done here because the rebase gave your commits new SHAs, so GitHub will not connect Thanks for the four rounds. Two things in here were better than what I asked for: the recovery rule The one open item is unchanged: elapsed time for a full foreground |
|
Following up on the workflow defect from this review, now that your work is on I said it was tracked separately when I wrote that. It was not. I had described the problem in the Nothing for you to do. Recording it because the eight silent days were the defect's doing, not yours, |
|
Thanks Ryan for carrying the contract repin, mutation-checking both persistence branches, and landing the stack with authorship preserved. I’ve confirmed #2585 is merged and both changes are on main. The distinction is clear: the persistence invariant held; the old literal assertion needed updating, and #2587 now tracks why that check was skipped. The cached-only preservation is now explicitly described as a data-loss fix in this PR. The full foreground WHOOP 5/MG historyDays: 4000 runtime remains unmeasured; I won’t substitute a short replay or a different history window for that device measurement. Thanks again for the detailed review and for getting this landed. |
What this PR does
An unmatched WRIST_OFF event can stay open through the scoring window and suppress a valid night after dense heart-rate data resumes. The pure Swift/Kotlin twins close only that unmatched tail after five minutes of valid HR (30–220 bpm), with no gap over five seconds. Invalid samples reset the run; explicit OFF/ON pairs remain authoritative.
Retrospective repair and review revisions
Verification
Local checks on the rebased changes:
Fresh GitHub CI is running for ba2c613. The previous head's successful CI is historical and is not evidence for this head.
Still unmeasured: elapsed time for a complete foreground historyDays: 4000 repair on WHOOP 5/MG with deep history. RR reuse remains disabled for that family; a short replay or a 21-day run does not establish this runtime. Device reproduction supports the recovery behavior, not general sleep-boundary accuracy or full-history performance.
Scope
This PR covers unmatched wear-event recovery and safe historical replay. Sleep-result notifications and other boundary/timing behavior are separate work.
Checklist