Skip to content

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

Open
fadkeprasad wants to merge 1 commit into
ryanbr:mainfrom
fadkeprasad:fix/stale-off-wrist-sleep
Open

fadkeprasad wants to merge 1 commit into
ryanbr:mainfrom
fadkeprasad:fix/stale-off-wrist-sleep

Conversation

@fadkeprasad

Copy link
Copy Markdown

What this PR does

An unmatched WRIST_OFF event can stay open through the whole scoring window. The sleep wear filter then drops a valid night even when dense heart-rate data resumes.

This change closes only an unmatched off-wrist tail after five minutes of valid HR (30–220 bpm) with no gap over five seconds. Explicit OFF/ON pairs remain authoritative, and the Swift and Kotlin implementations stay in sync.

Retrospective repair

A guarded one-time upgrade replay scans up to 4000 days. It writes only days that produce fresh scores, preserving cached days whose raw streams are gone, and it preserves edited or dismissed sleep sessions. The repair flag is set only after successful persistence; cancellation, busy state, or persistence failure leaves it pending for a retry.

How it was tested

  • Swift package suite: 2037 tests passed.
  • Apple history-repair tests: 2 passed.
  • iOS build passed.
  • Android recovery/history and JaCoCo budget tests passed.
  • Locale and detached-document comment audits passed.

The full Android baseline still has two pre-existing RecoveryDriversTest rounding failures and an existing StressRawReadScopeTest nullable-type warning.

Checklist

  • Swift package tests pass for the package touched
  • Android targeted unit tests pass
  • No generated output 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.

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