Repository navigation
iOS: run a backgrounded re-score where it can finish, instead of restarting it - #1557
Conversation
…arting it #1538, the substantive half. A completed offload rescores immediately, and on iOS that routinely happens while the app is BACKGROUNDED -- it stays alive as a bluetooth-central to receive the offload at all. But analyzeRecent is all-or-nothing: pass 1 writes nothing, every store write lands after both loops, and the watermark advances only at the very end so an interrupted run can never mark unscored data as scored. On the reporter's install the pass measured 474,778 ms. iOS suspends the process long before that, so every scored night was lost. The lost work was not the worst of it. Because the watermark never advanced, the NEXT trigger still saw newData=yes and started another full pass, which was killed in turn. The report shows it plainly: every offload after 05:40:31 said "caught up" with nothing left to fetch, yet two later ticks still read newData=yes. Failing to finish is precisely what guaranteed it would be tried again. The score appeared 1 h 57 m after the data was complete, and only because the app happened to stay foregrounded for eight unbroken minutes. So decide before spending anything. A foreground pass always runs -- there is no suspension deadline and the existing behaviour is correct. A background pass runs if nothing is already owed and the last COMPLETED pass fits the budget a background wake is worth; otherwise the work is marked owed and handed to a BGProcessingTask, which is the long deferrable kind rather than the metered refresh kind the two existing schedulers use. Whichever comes first -- that task or the next foreground -- settles the debt. The measurement is taken, not guessed: a completed pass banks its own duration, because the cost varies by more than an order of magnitude with history size and a constant would either defer installs that finish comfortably or wave through ones that never could. Unknown is never read as "too slow": no measurement, a zero, a NaN, or a nonsensical budget all fall through to running, since refusing to score on a value we cannot interpret is a far worse failure than one wasted pass. HONEST about what this does NOT do. It does not make the pass fast -- a cold process still re-scores every night in the window, because the per-day reuse cache is in-memory and starts empty. iOS decides when a processing task runs and favours idle and charging, so this is "eventually, without needing the app held open", NOT "the score appears moments after a sync". On a heavy install the score still normally appears when the app is next opened. What changes is that the phone stops paying for a full doomed pass on every offload to get there, and that the work can no longer be silently dropped. Parity: iOS-only, and not a divergence. Android runs its offload under a FOREGROUND_SERVICE_CONNECTED_DEVICE, so its process is not suspended mid-pass and the defect does not exist there. Nothing in the Android tree needed a twin; the scoring path itself is untouched on both platforms. Verified: 20 new tests across the pure policy and the owed-mark bookkeeping, including the deferral branch, which is unreachable on macOS and is where the work-is-owed record lives -- an early version of this change deferred WITHOUT marking the debt, so the background task would have woken, found nothing owed, and silently dropped the pass. That is now pinned. Doc lint and diff-scoped i18n clean. app-build compiles both app targets and runs StrandTests. NOT validated on hardware. Background scheduling cannot be CI-tested or checked on Linux, and CLAUDE.md is explicit that this class of change needs a real device: what needs observing is a backgrounded offload logging a deferral rather than a killed pass, and the processing task later logging a completed one. Partial fix for the background half of #1538; the residual per-pass cache invalidation described there is untouched.
Re-review catch, and a regression I introduced rather than one I found. The foreground drain was awaited at the head of the scenePhase .active task -- the same sequential block that owns the Health auth refresh, the Health sync, the widget snapshot and the watch push. That pass is minutes long on exactly the installs that need it; being minutes long is why it was deferred in the first place. Awaiting it there would have left the Home-screen widget and the watch showing stale numbers for the whole re-score every time the app was opened, and held up the Health sync behind it. Trading an invisible background stall for a visible foreground one is a worse bug than the one being fixed. Its own task now. analyzeRecent serialises itself, so overlapping with the sync this same transition kicks off is safe. Also documents why isBackgrounded treats .inactive as background rather than foreground: .inactive is the state on the way to suspension, so reading it as foreground is the error that loses work, while reading it as background costs at most a deferral that the next .active transition drains. The transient cases -- app switcher, notification centre, an incoming call -- self-correct in seconds.
|
Re-reviewed. One real find, and it was a regression I introduced — fixed in The foreground stallI had awaited the deferred re-score at the head of the That pass is minutes long on exactly the installs that need it; being minutes long is why it was deferred. Awaiting it there would have left the Home-screen widget and the watch showing stale numbers for the entire re-score every time the app was opened, with the Health sync held up behind it. Trading an invisible background stall for a visible foreground one is a worse bug than the one this PR fixes. Its own Also documented, not changed
Traced this pass, no change needed
CI
Still not validated on hardware — that remains the gating item, and the three log lines to look for are in the description. |
… an escape
Third-pass find: a re-score could be left owed with NOTHING scheduled to run it.
The deferral path submits the processing request, and the assertion's expiry
handler submits one, but neither covers a pass that was simply killed -- a
foreground pass ended by a force-quit never touches either path -- and iOS can
discard a request that was submitted. In those cases the work waited for the next
offload to defer it or the next launch to drain it.
The scenePhase .background branch already re-submits the Health write-back
request on every transition for exactly this reason ("iOS may discard an old
best-effort request"). Same treatment, gated on something actually being owed so
it costs nothing on the installs that never defer.
Also renames the persisted key from noop.rescorePassPending to noop.rescoreOwed.
The surrounding code was renamed to the owed vocabulary earlier in this branch
and the key string was the last holdout; it has never shipped, so there is no
migration and nothing to lose.
|
Third pass. One real gap found and closed ( Owed work could have no escapeA re-score could be left owed with nothing scheduled to run it. The deferral path submits the processing request, and the assertion's expiry handler submits one — but neither covers a pass that was simply killed. A foreground pass ended by a force-quit touches neither path, and iOS can discard a request that was submitted. In those cases the work waited for the next offload to defer it, or the next launch to drain it. The Also renamed the persisted key Traced this pass, no change needed
CIBoth targets green on Three passes in, the finds have gone from a real regression (the foreground stall) to an edge case to a rename — the returns are flattening. Still not validated on hardware, which remains the gating item rather than anything CI can settle. |
Fourth-pass find, and a hole in this branch's own fix. Only the post-offload caller was wrapped. The steady-state backstop -- analyzeRecent(force: false) in AppModel's cadence loop -- was not, and it is the one path that can reproduce the whole bug on its own. That loop lives as long as the process, so it keeps ticking while the app is backgrounded as a bluetooth-central. Its own watermark gate cannot save it: a killed pass never advances the watermark, so the tick still reads the data as new and starts another full pass, which is killed in turn. The comment directly above it already notes the gate also cannot skip while the strap streams live HR. So every 30 minutes, in the background, on the exact installs this branch is meant to help, it could start a pass that could not finish -- with no assertion held and no deferral check. Wrapped now, with owesOnDefer: false. A skipped BACKSTOP owes nothing: every real update forces its own pass, so recording a debt here would send a processing task off to run a forced full pass when most likely nothing changed -- the churn #1146 exists to avoid. A debt a real pass already recorded survives untouched, and the log says "backstop tick skipped" rather than "deferred", because nothing is queued and promising a background task that is not coming would be a lie in the one place #1538 proved the log has to be trusted. Audited every other analyzeRecent call site while here. The screens (Workouts, Intelligence, Settings, Test Centre, Sleep) are all user-initiated from a visible screen, so they are foreground and always run -- wrapping them would change nothing. runDeferredRescoreIfOwed is deliberately NOT wrapped: it is the deferred run, and wrapping it would defer it again forever. The engine's own re-arm and adoptActiveDevice stay direct; both mark the debt via the engine, so a kill still escalates. Also spells out `live = self.live` in the new capture list. The cadence Task requires explicit self, so the bare-name shorthand this type uses elsewhere would not have resolved. Four new tests: a skipped backstop conjures no debt, leaves an existing one alone, and still runs in the foreground and in a background that can afford it. NOT COMPILED. This edits app-target Swift and no CI ran for this pass, so the earlier green no longer covers HEAD -- app-build must pass before this merges.
Curated notes for docs/releases/v10.6.0.md covering the 83 merged PRs since v10.5.0, then Tools/appchangelog-gen.py run over them to insert the entry into both AppChangelog.swift and AppChangelog.kt and localize the card title into all six Android locales. Generated rather than hand-written on purpose: the release workflow only runs appchangelog-gen when it performs the version bump itself, so running it here means the in-app card can be reviewed and lint-checked before anything is published instead of arriving as a CI warning nobody reads. Headline items are the selectable Banister TRIMP Effort scale (#1562, #1563), the background re-score that can now finish rather than restarting forever (#1557, #1559), the Oura ring being handed out of daytime-HR mode so its own sleep suite can run (#1526, #1550), ramp-aware sleep with the night's HR line (#1551, #1552), live-workout pause/discard plus SDNN on Android (#1533, #1535), and the WHOOP 4.0 device key no longer reaching a shared strap log (#1610). Third-party contributors credited by handle per CLAUDE.md, covering both merged PRs and the reports behind the fixes. The version itself is deliberately NOT bumped here: the release workflow skips appchangelog-gen when the source version already equals the target, so pre-bumping would ship a release whose in-app What's New still advertised the previous one. Verified: compileFullDebugKotlin, lintVitalFullRelease -PstagingRelease (the ExtraTranslation gate that compile and the i18n audit both miss), i18n_audit --ci, doc_comment_lint, and the 108 Tools tests.
The substantive half of #1538. Reported by @justinjor-bit with the measurements that made it findable.
The livelock
A completed offload rescores immediately, and on iOS that routinely happens while the app is backgrounded — it stays alive as a
bluetooth-centralto receive the offload at all. ButanalyzeRecentis all-or-nothing: pass 1 writes nothing, every store write lands after both loops, and the watermark advances only at the very end so an interrupted run can never mark unscored data as scored. On the reporter's install the pass measured 474,778 ms. iOS suspends the process long before that, so every scored night was lost.The lost work was not the worst of it:
This is visible in the reporter's own log. Every offload after 05:40:31 said
caught upwith nothing left to fetch, yet 06:01:55 and 06:29:47 both readnewData=yes. They should have readno. Failing to finish is precisely what guaranteed the work would be attempted again. The score appeared 1 h 57 m after the data was complete, and only because the app happened to stay foregrounded for eight unbroken minutes.The fix: decide before spending anything
Deferred work is marked owed and handed to a
BGProcessingTask— the long, deferrable kind, not the meteredBGAppRefreshTaskthe two existing schedulers use. Whichever comes first, that task or the next foreground, settles the debt.The measurement is taken, not guessed: a completed pass banks its own duration, because the cost varies by more than an order of magnitude with history size and a constant would either defer installs that finish comfortably or wave through ones that never could. Unknown is never read as "too slow" — no measurement, a zero, a NaN, or a nonsensical budget all fall through to running, since refusing to score on a value we cannot interpret is a far worse failure than one wasted pass.
Every path that can start a pass in the background
Review after the first draft found that wrapping the post-offload caller alone was not enough — the steady-state backstop could reproduce the whole bug by itself. Both are covered now:
refreshAfterCompletedBackfill)analyzeRecent(force: false))owesOnDefer: false— a skipped backstop owes nothingrunDeferredRescoreIfOwedadoptActiveDeviceThe backstop mattered because its loop lives as long as the process, so it keeps ticking while backgrounded as a
bluetooth-central— and its own watermark gate cannot save it, since a killed pass never advances the watermark. The comment above it already noted the gate also cannot skip while the strap streams live HR. So every 30 minutes it could start a pass that could not finish.owesOnDefer: falsefor it, because every real update forces its own pass: recording a debt there would send a processing task off to run a forced full pass when most likely nothing changed, which is the churn #1146 exists to avoid. Its log line saysbackstop tick skipped, notdeferred— nothing is queued, and promising a background task that is not coming would be a lie in the one place #1538 proved the log has to be trusted.Owed work also gets one more escape: the
scenePhase == .backgroundtransition re-submits the request when something is owed, matching what the Health write-back already does there. Without it, a pass killed by a force-quit — which touches neither the deferral path nor the assertion's expiry handler — had nothing scheduled to run it.What this does NOT do
What changes is that the phone stops paying for a full doomed pass on every offload to get there, and that the work can no longer be silently dropped.
Parity
iOS-only, and not a divergence. Android runs its offload under a
FOREGROUND_SERVICE_CONNECTED_DEVICE, so its process is not suspended mid-pass and the defect does not exist there. Nothing in the Android tree needed a twin. The scoring path itself is untouched on both platforms — this changes only where the work runs.Verification
runtakes an injectableisBackgroundpurely so that path can be driven under test. It is pinned now.app-buildcompiles both app targets and runsStrandTests.Not validated on hardware
Background scheduling cannot be CI-tested or checked on Linux, and CLAUDE.md is explicit that this class of change needs a real device. What needs observing on an iPhone:
re-score: deferred to a background task — …instead of a killed pass;re-score: resuming a pass an earlier attempt could not finishfollowed by are-score: done;trigger=line left without a matchingdoneonce things settle.@justinjor-bit — you offered to run an instrumented build. This is the one, if you are still willing. The three lines above are what would confirm or refute it, and a strap log covering one overnight sync would settle it either way.
Partial fix for the background half of #1538; the residual per-pass cache invalidation described there is untouched, so the issue stays open.