fix(wait): report readiness work that consumed the wait budget - #2893
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
|
Adversarial review: clean. Applied its doc notes in
The review's smaller shape, one readiness reason with |
thymikee
left a comment
There was a problem hiding this comment.
Thermo-nuclear structural pass (implementation quality only). Net: the core design is sound — readiness rides a typed readinessPhase on the cancellation error instead of text-sniffing, and collapsing cancelledPoll into {outcome, deadline, evidence} is a real simplification over threading a runnerRestarted boolean into timedOutDeadline. No file crossed 1k. Not blocking — three refinements that each harden the taxonomy against the next variant.
(Head moved to cd646b8c after I pulled; the new commit is docs-only, so these anchors are on the current head.)
Nit (not inline): packages/contracts/src/wait.ts:39 readinessPhaseDetails is a one-prop identity wrapper whose two non-test callers already name the phase literal at the throw site; it only earns its keep as the typed twin of readinessPhaseOf. Fine to keep, flagging as borderline.
| if (deadline === 'runner-restart-exhausted') { | ||
| return waitRunnerRestartExhaustedError(message, evidence); | ||
| } | ||
| if (deadline === 'readiness-exhausted') return waitReadinessExhaustedError(message, evidence); |
There was a problem hiding this comment.
waitTimeoutError is now a 4-deep if ladder whose tail (capture-stalled / target-absent) is the catch-all, so a new WaitPollDeadline that forgets an arm silently degrades to the wrong reason instead of failing the build — this very PR hinged on remembering to insert line 322, and miss it and wait_readiness_exhausted quietly becomes wait_target_absent with zero signal. Can we make it a switch (deadline) with default: return assertNever(deadline)? assertNever is already the idiom here (selector-wait.ts:237, capture/wait.ts:199), so a dropped arm turns into a compile error.
| if (session && session.state === 'starting' && isRequestCanceledError(appErr)) { | ||
| await invalidateRunnerSessionBestEffort(session, 'runner_startup_request_canceled'); | ||
| throw error; | ||
| const runnerStarting = session ? session.state === 'starting' : liveness !== 'ready'; |
There was a problem hiding this comment.
Predicate is right — resolveRunnerSessionLiveness is closed over starting | ready | draining | stopped | gone, so liveness !== 'ready' never mislabels — but the name and the reader disagree. runnerStarting actually means "no observable runner at entry", and it mixes an authoritative session.state with a pre-flight liveness snapshot read at line 274 that is likely stale by the time we hit the catch. AGENTS.md wants invariants in names/types. Can we name the liveness arm honestly (runnerUnavailableAtEntry) or fold both arms into one resolveRunnerReadinessPhase(session, liveness): ReadinessPhase | undefined so "was the runner observable when we started?" lives in exactly one place?
| // even when an earlier readable poll saw the target. Returning stale | ||
| // target-present evidence would hide the retriable cause and make callers stop | ||
| // retrying for the wrong reason. | ||
| if (deadline === 'runner-restart-exhausted' || deadline === 'readiness-exhausted') { |
There was a problem hiding this comment.
runner-restart-exhausted and readiness-exhausted now move together here AND in cancelledPoll, for the same reason ("a self-reported cause outranks stale present-evidence"). Two free-floating variants that always travel as a pair is a missing named model — every new self-reported cause means another || deadline === ... in two files. Can we extract it once (overridesPresentEvidence(deadline) / a SELF_REPORTED_DEADLINES set)? More ambitiously, model runner-restart as a third ReadinessPhase so the two collapse into one deadline + reason + builder and the pairwise OR disappears.
|
At cd646b8, Does the change need a separate The live iOS 26.2 numbers (9/9 on head, 5/5 on main) are quoted from the PR body, with no logs attached, and I haven't checked whether the aborted |
cd646b8 to
aed9cd8
Compare
|
Rebased onto main and squashed into one commit, |
|
Correction to my previous comment:
This PR's CI Integration Tests job is the authoritative check. |
|
Reviewed at aed9cd8. The mislabel from the last review is still there, and a new import breaks the Coverage job.
The regression test is still missing. Please add one that caches a target, aborts inside The Coverage failure is likely from this PR. The value import of After these fixes, please attach a live iOS Simulator run: one |
A wait whose deadline cancels a poll while the iOS XCTest runner is still starting, or while the Simulator app is still being discovered, now fails with wait_readiness_exhausted and details.readinessPhase instead of wait_capture_stalled. The platform names the phase on the cancellation it throws; the wait classifies the cancelled poll as readiness. Refs #2343
…imports type-only
aed9cd8 to
24401a6
Compare
|
Sweep pass. Rebased onto Due items from the 12:41Z and 13:39Z reviews:
Live on a fresh iOS 26.2 sim (deleted afterwards):
|
|
This follows up on the review at aed9cd8. The fix now covers both routes that review asked about, and the code looks clean at 24401a6. The live iOS 26.2 results (runner-start 4/4, wait absent 4/4, 3 of 4 ending on a deadline-cancelled steady-state poll) come from the author with no logs attached; I'm counting them because they name the two routes the earlier review asked for. I did not run the tests or the eager-closure-budgets gate myself, and I judged whether each test would fail without the fix by reading the pre-delta code. Two related The daemon wait tests split the chain at the route's thrown error, so no single test drives a cached-target ps abort end to end through the daemon wait. Does the split coverage already prove that path, or should one test carry it through? No conflicts. Smoke Tests is still queued. It runs |
Summary
A
waitwhose deadline cancels a poll while the iOS runner is still starting, or while the capture waits on a running Simulator app discovery, now fails withdetails.reason: "wait_readiness_exhausted"andreadinessPhase: "runner-start" | "target-discovery", and that poll is recorded asoutcome: "readiness". Before, it readwait_capture_stalled.runner-lifecycle.tsfor a runner that never answered, and the resolver's discovery wait insnapshot-target.ts. The cached-targetpsre-check does not tag. Imports stay type-only.APP_NOT_RUNNING) still wins;readinessPhaseis added to its details.wait_runner_restart_exhaustedis unchanged. The timeout still covers readiness.readinessPhase.11 files. Refs #2343. It stays open: readiness time in completed polls is not accounted.
Validation
Tested
24401a694f(ondc9ab863ad):pnpm check:affected --runexit 0 (2865 tests);eager-closure-budgets688/688.runner-lifecycle, the route and resolver tags, thepsre-check abort, the refusal order, and the readiness branches inwait-pollingandwait-absent.wait text … 9000with the runner killed and the app terminated gave 4/4readinessPhase: "runner-start". On main before this PR, the same scenario gave 5/5wait_capture_stalled.wait absenton the found app gave 4/4wait_target_present, with noreadinessPhase.