Skip to content

fix(sleep): recover unmatched off-wrist tails from sustained HR - #2375

Closed
fadkeprasad wants to merge 4 commits into
ryanbr:mainfrom
fadkeprasad:fix/stale-off-wrist-sleep
Closed

fadkeprasad wants to merge 4 commits into
ryanbr:mainfrom
fadkeprasad:fix/stale-off-wrist-sleep

Conversation

@fadkeprasad

@fadkeprasad fadkeprasad commented Sep 21, 2026 •

Copy link
Copy Markdown

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

  • Apple and Android run one shared, guarded history pass (up to 4000 days) when either the Effort or sleep-wear repair flag is pending. Both flags are marked only after successful persistence. Busy, cancelled, or failed attempts leave the repair pending. The coupled flags are documented in code.
  • Persistence groups daily metrics, metric points, provenance, and marker points by day once, then writes the scored days without repeatedly filtering the full collections.
  • Data-loss fix for the existing Effort repair: the shared pass preserves cached-only days whose raw inputs are no longer available. Replacing the entire historical window could erase those days. Both platforms now preserve them, and edited/dismissed sleep sessions remain protected.
  • Rebased onto upstream main at 1d66594; current head is ba2c613.
  • Reciprocal twin declarations now register offWristIntervals and WristWearRecovery.firstSustainedHR. The derived parity inventory was refreshed through the guarded tool. Seven exact out_of_scope_twin dispositions document Android orchestration roles implemented in the excluded Swift app layer; they do not assert function-level identity. The parity gate and existing debt baseline are unchanged.

Verification

Local checks on the rebased changes:

  • python3 Tools/parity_ledger.py --refresh-derived: passed its ratchet validation and wrote the reviewed inventory.
  • Full tests.test_parity_governance_acceptance suite: 50 tests passed, including repository acceptance.
  • Swift WristWearRecoveryTests and WristOffSleepFilterTests: 10 tests passed.
  • Android WristWearRecoveryTest, SleepStagerOffWristTest, SleepWearHistoryWindowTest, and IntelligenceEngineJacocoBudgetTest: 24 tests passed with JDK 17.
  • Documentation-comment lint and git diff --check passed.

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

  • Swift and Kotlin recovery/persistence behavior covered
  • Current parity authority and repository acceptance checked
  • Current head GitHub CI completed successfully
  • Full-history WHOOP 5/MG foreground runtime measured
  • No private health captures or secrets committed

@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 @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 RecoveryDriversTest rounding failures you hit are not yours: they are an arm64 JDK exp ulp difference on a half-tie (-0.4999999999999929 against an exact -0.5), green on CI's x86 runner. Several PRs have reported them; nothing to fix.

@fadkeprasad

Copy link
Copy Markdown
Author

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.

@fadkeprasad
fadkeprasad force-pushed the fix/stale-off-wrist-sleep branch from 4d1a61f to f4fcede Compare September 23, 2026 13:18
@fadkeprasad

Copy link
Copy Markdown
Author

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.

@fadkeprasad

Copy link
Copy Markdown
Author

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.

@fadkeprasad
fadkeprasad requested a review from ryanbr September 23, 2026 13:51
@fadkeprasad
fadkeprasad force-pushed the fix/stale-off-wrist-sleep branch from c8909ee to 4739e25 Compare September 24, 2026 13:54
@fadkeprasad

Copy link
Copy Markdown
Author

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.

@ryanbr

ryanbr commented Sep 29, 2026

Copy link
Copy Markdown
Owner

@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, !computing, !Task.isCancelled, and foreground-only, with both flags set inside the post-persistence callback so a busy, cancelled or failed attempt leaves the work pending. Marking only after every required write succeeds is the part that makes a retry safe.

I also want to name something your summary undersells. preserveUnscoredHistory does not just avoid rewriting cached-only days; it stops the existing Effort repair REPLACING an entire historical window, which is how a day whose raw inputs are gone could previously be lost. That is a data-loss fix riding along inside a sleep PR. It is the right change, and it deserves to be called that rather than described as persistence tidying.

Blocking: the parity authority drifts

Rebased onto current main, the repository-acceptance suite fails:

add-unpaired-function|swift|Packages/StrandAnalytics/Sources/StrandAnalytics/AnalyticsEngine.swift::offWristIntervals/3#1
  is source drift against the checked-in twin map
authority files: 505 -> 507

offWristIntervals is not new, but its signature moved from arity 2 to arity 3 on both platforms, which changes its identity in the inventory. Together with the two new WristWearRecovery files that is a declaration-graph change, and no Tools/parity_* file is touched, so merged as-is this would turn main red.

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 main carried it red until #2581. Two other PRs have hit this since; you are the third, and in each case the PR's own green checks described a base that no longer existed.

Suggested remedy, and I would try the first one. Neither offWristIntervals carries a twin claim, on either platform, which is why the pair does not re-form at the new arity. They are genuine twins: same name, same role, same new signature. Reciprocal Kotlin twin: and Swift twin: claims on the two doc blocks should pair them, so the identity change costs no debt at all. Then python3 Tools/parity_ledger.py --refresh-derived.

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

historyDays: 4000 is a full-history pass and it is foreground-only, so its cost lands visibly on the first launch after an update.

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 MIN(srcChannel) subquery over the same window, so a slice of a wider read is not equivalent to a narrow one. The restriction is correct. The consequence is that every day in the pass pays a full R-R read with nothing shared.

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.

@fadkeprasad
fadkeprasad force-pushed the fix/stale-off-wrist-sleep branch from cc1cb8f to ba2c613 Compare September 29, 2026 03:22
@ryanbr

ryanbr commented Sep 29, 2026

Copy link
Copy Markdown
Owner

@fadkeprasad The authority is sorted, and I checked it the way that matters rather than from the green
tick. I rebased your head onto current main (which has moved twice since your run) and re-derived:
the repository-acceptance suite is 50/50 OK and the full ratchet reports errors=0, so the
checked-in authority reproduces from current product source. The rebase was clean.

The twin claims did exactly what I hoped. file_pairs 76 to 77 with unpaired_files flat means both
WristWearRecovery files paired; function_pairs 207 to 209 is offWristIntervals re-forming at
the new arity, plus firstSustainedHR. The identity change cost no debt, which was the point.
Replacing the old vague "Mirrors Swift." with an explicit reciprocal claim is better than what I
asked for.

One thing is still red, and it is not your logic

parity-governance fails on test_rr_legacy_preservation_contract. I checked whether the contract it
guards is genuinely broken. It is not.

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
test exists to protect is that only the snapshot-protected copy reaches persistence, and that it never
leaks into the recomputed derivations. Both still hold:

  • let dailiesByDay = Dictionary(grouping: persistedDailies, by: \.day), so the preserve branch and
    the wide branch both trace back to persistedDailies
  • there is exactly one persistComputedScores call site in the engine
  • assertNotIn("persistedDailies", derivations) still passes, and that is the assertion actually
    standing between a legacy snapshot and something recalculated against today's baselines

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 assertNotIn exactly as it is; that one is
load-bearing.

Why this only showed up on day eight, which is on me

That test reads PRODUCT source (Strand/Data/IntelligenceEngine.swift and
IntelligencePersistence.kt) but lives behind parity-governance.yml's Tools/parity_* path filter,
and the core tools-python step collects with find . -maxdepth 1 from Tools, so Tools/tests/**
is not in it either. Your ternary landed on 2026-09-21 and the test did not run once until
ba2c613f touched Tools/parity_dispositions.json seven days later. Eight days red, invisible,
across three pushes.

That is the same shape as #2582, which moved the repository-acceptance suite onto the unfiltered leg
for exactly this reason, and as the #1691 trap before it. I wrote #2582 today and did not notice the
companion test sitting in the same workflow with the same problem. I will move it; you should not
carry a workflow defect in a sleep PR.

Non-blocking, for whoever picks up #2567

Four of the seven new dispositions (readDaySkinAndWristOff, byScoredDay, persistComputedWindow,
persistInto) are typed out_of_scope_twin, and I confirmed none of those four names is declared in
the twin_path file. Your rationales say so plainly, one of them in as many words ("a role twin, not
a function parity claim"), so this is not a wrong claim and the validator only checks the file. The
other three (analyzeRecent, runEffortRescoreIfNeeded, historyRepairIsPending) do name real
declarations in that file.

Worth recording: when #2567 widens Swift scope to take in Strand/, those three re-pair on their own
and the four role-only ones resurface as genuine unpaired findings needing a fresh decision. They read
more like Android-structural extractions (bytecode budget, test seam) than out-of-scope twins. Not
worth another push on its own.

Still open from last time

Your updated body records the full-history WHOOP 5/MG foreground runtime as unmeasured, with the box
unchecked. That is the honest answer and I would rather have it than a guess. It stays the one thing I
want before this ships, and it needs hardware, not another review pass.

Repin the contract test and I will take this.

@ryanbr

ryanbr commented Sep 29, 2026

Copy link
Copy Markdown
Owner

@fadkeprasad Rather than have you push again for four lines, I stacked your branch onto current main
and added the contract repin myself in #2585. Your four commits are replayed unchanged and keep your
authorship; the only commit that is mine is the test repin.

To be exact about what moved: Tools/tests/test_rr_legacy_preservation_contract.py only. None of your
Swift or Kotlin is touched, and neither is the parity authority you registered.

The repin pins both branches of the ternary instead of the single literal, and I mutation-checked it
rather than trusting it green. Grouping from raw dailies, and writing raw dailies in the wide
branch, each turn it red. The literal it replaces only ever pinned the wide branch, so the contract
ends up better guarded than before your change.

It had to ride along with your Swift, since main still carries the old literal and the new assertions
would not match there. That is why it could not land ahead of you, and why asking you for it would just
have cost another few days on a workflow defect that was never yours.

Once #2585 goes green I will rebase-merge it so your commits land as yours. Thanks for four rounds on
this, and for recording the full-history WHOOP 5 runtime as unmeasured rather than guessing at it.

@ryanbr

ryanbr commented Sep 29, 2026

Copy link
Copy Markdown
Owner

Landed on main as 7f1e42f, b9e1a3c, 333c44e and e768326, all four still authored by you, plus
5ac2cc2 carrying the contract repin. All 18 checks were green on the stack, parity-governance
included.

Marking this one done here because the rebase gave your commits new SHAs, so GitHub will not connect
them on its own. Nothing is outstanding on your side.

Thanks for the four rounds. Two things in here were better than what I asked for: the recovery rule
returning the first observed sample of the confirmed run instead of the OFF timestamp, which recovers a
boundary from evidence rather than inventing one, and preserveUnscoredHistory, which stops the Effort
repair replacing a whole historical window. That second one is a data-loss fix that arrived under a
sleep title and deserved louder billing than your summary gave it.

The one open item is unchanged: elapsed time for a full foreground historyDays: 4000 repair on a
WHOOP 5 or MG with deep history. That needs a device, not another review pass.

@ryanbr

ryanbr commented Sep 29, 2026

Copy link
Copy Markdown
Owner

Following up on the workflow defect from this review, now that your work is on main.

I said it was tracked separately when I wrote that. It was not. I had described the problem in the
#2585 body and in the review above without ever filing it, which made "tracked separately" a claim
about something that did not exist. It is #2587 now, with the dates from this PR as the evidence:
the ternary landed 2026-09-21 and the test first ran on 2026-09-28, only because ba2c613f touched
Tools/parity_dispositions.json and matched the filter by accident.

Nothing for you to do. Recording it because the eight silent days were the defect's doing, not yours,
and that should be written somewhere durable rather than in a review comment.

@fadkeprasad

Copy link
Copy Markdown
Author

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.

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