Skip to content

fix(rescore): retry an unfinished background pass after a cooldown instead of deferring until the app is opened - #2334

Merged
ryanbr merged 2 commits into
ryanbr:mainfrom
Iskrata:fix/rescore-run-owed-in-background
Sep 19, 2026
Merged

ryanbr merged 2 commits into
ryanbr:mainfrom
Iskrata:fix/rescore-run-owed-in-background

Conversation

@Iskrata

@Iskrata Iskrata commented Sep 19, 2026

Copy link
Copy Markdown

What this PR does

A backgrounded offload that finds a re-score already owed, with nothing running, now defers only while the last pass started less than 30 minutes ago (RescoreBackgroundPolicy.interruptedRetryCooldownSeconds). After that it runs, paced as usual.

  • The start time of the last pass is recorded only by a pass that is about to work (markRescoreOwed(passStarting: true), from IntelligenceEngine).
  • The deferral path does not record it. It still re-marks the debt, and if it refreshed the time too, an old unfinished pass would look recent forever.

Why

The owed rule dates from #1538, when a backgrounded pass was killed by the CPU limit every time and retrying on each offload was the livelock. Pacing (#2296, #2318) now keeps a background pass under that limit. But the rule still deferred on any unfinished debt, with no end, until a BGProcessingTask or the next foreground.

On one phone (this fork's Release build with #2293 and #2318):

  • A pass left unfinished at 11:25 on 2026-09-18 deferred every offload after it: re-score: deferred to a background task — a re-score is already outstanding from an earlier trigger, roughly every 10 minutes.
  • That lasted until 09:26 the next morning, when the app was opened: 19 hours with no re-score, and that night was never scored (no daily row, no session). The processing task was never granted in that time.
  • The debt was not from a CPU kill. There are no cpu_resource_fatal reports for NOOP. The app was terminated while suspended (the strap log rolled a fresh session at 15:55, 16:35 and 16:55 UTC), which iOS does routinely for memory.

The cooldown keeps what #1538 needed, no retry on every offload, without letting one interrupted pass stop scoring for good.

Type of change

  • Bug fix

How it was tested

  • RescoreBackgroundPolicyTests:
    • an interrupted attempt defers within the cooldown and runs once it has passed;
    • no recorded attempt counts as not recent;
    • the existing cases are unchanged.
  • RescoreBackgroundSchedulerTests: a deferral does not make an old attempt look recent. The fixtures that model a just-killed pass now record their attempt time.
  • 41 tests across those suites and IntelligenceRRSourceTests pass. doc_comment_lint.py is clean.

Android: nothing to port. Android has no equivalent deferral or background suspension.

Checklist

  • No new build warnings introduced
  • Follows the conventions in docs/CONTRIBUTING.md
  • I did not commit generated output (Strand.xcodeproj/) or any secrets/keystores

…stead of deferring every offload until the app is opened
@ryanbr

ryanbr commented Sep 19, 2026

Copy link
Copy Markdown
Owner

Thanks @Iskrata. The reasoning holds up and I checked the parts that could have bitten: decide has exactly one production caller and it passes the new argument, so the defaulted nil cannot quietly change behaviour anywhere else; passStarting: true is only on the engine path, with the deferral site left bare, so repeated deferrals cannot refresh a stale debt; the boundary is pinned exactly (cooldown - 1 defers, cooldown runs); and setUp/tearDown save and restore the new key so it cannot leak between tests. The helper default of 60 seconds keeping every pre-existing case inside the cooldown is a nice touch, it makes "the existing cases are unchanged" true by construction. Android really does have no equivalent, so "nothing to port" is right.

One thing to fix before this goes in.

The two new declarations landed inside markRescoreOwed's doc comment:

/// Mark a re-score as owed. Called by `IntelligenceEngine` once a pass is past every gate and is
/// definitely about to work ...
/// Returns the token stamped on this debt. A pass keeps it and hands it back at completion; every
/// other caller (the deferral path) can ignore it, since it is not the one that will settle up.
/// When the last pass started (unix seconds), written only by a pass that is about to work ...
static let lastAttemptStartedAtKey = "noop.rescoreLastAttemptStartedAt"

Swift merges consecutive /// runs, so that whole block now documents lastAttemptStartedAtKey, a String that returns no token and is called by nobody. markRescoreOwed is left with just its - Parameter passStarting: line, so the function carrying the debt contract no longer states it.

Moving lastAttemptStartedAtKey and secondsSinceLastAttempt above the /// Mark a re-score as owed. block sorts it.

Worth saying that your doc_comment_lint.py run was not wrong. It detects a doc block left DETACHED, stacked or split off by a blank line. Here nothing is detached, the block was reassigned to a different declaration, which neither pattern sees. The script's own header names this exact cause though ("dropping a new declaration into the gap between a comment and the thing it documents"), so it is the case it was written for even if it cannot spot this shape.

Everything else is green and it merges clean. Push that and I will take it.

@Iskrata

Iskrata commented Sep 19, 2026

Copy link
Copy Markdown
Author

Good catch, thanks. Moved lastAttemptStartedAtKey and secondsSinceLastAttempt above the /// Mark a re-score as owed. block, so the debt contract is back on markRescoreOwed.

@ryanbr

ryanbr commented Sep 19, 2026

Copy link
Copy Markdown
Owner

That is it, thanks @Iskrata. lastAttemptStartedAtKey has its own doc now and markRescoreOwed has its contract back, so the block that explains the debt token is attached to the thing that issues it again.

Everything else stood on the first pass: one production caller of decide and it passes the new argument, passStarting: true only on the engine path with the deferral site left bare, the cooldown boundary pinned exactly, setUp/tearDown restoring the new key, and Android genuinely having no equivalent to port.

Merging.

@ryanbr
ryanbr merged commit 2ae4257 into ryanbr:main Sep 19, 2026
4 checks passed
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