Skip to content

fix(wait): report readiness work that consumed the wait budget - #2893

Merged
thymikee merged 6 commits into
mainfrom
fix/wait-readiness-exhausted
Sep 24, 2026
Merged

thymikee merged 6 commits into
mainfrom
fix/wait-readiness-exhausted

Conversation

@thymikee

@thymikee thymikee commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Summary

A wait whose deadline cancels a poll while the iOS runner is still starting, or while the capture waits on a running Simulator app discovery, now fails with details.reason: "wait_readiness_exhausted" and readinessPhase: "runner-start" | "target-discovery", and that poll is recorded as outcome: "readiness". Before, it read wait_capture_stalled.

  • Only the code doing the work tags the cancellation: runner-lifecycle.ts for a runner that never answered, and the resolver's discovery wait in snapshot-target.ts. The cached-target ps re-check does not tag. Imports stay type-only.
  • A retriable refusal from an earlier poll (such as APP_NOT_RUNNING) still wins; readinessPhase is added to its details. wait_runner_restart_exhausted is unchanged. The timeout still covers readiness.
  • Other requests cancelled during a runner start or a discovery wait also carry readinessPhase.

11 files. Refs #2343. It stays open: readiness time in completed polls is not accounted.

Validation

Tested 24401a694f (on dc9ab863ad): pnpm check:affected --run exit 0 (2865 tests); eager-closure-budgets 688/688.

  • Mutations killed: the phase in runner-lifecycle, the route and resolver tags, the ps re-check abort, the refusal order, and the readiness branches in wait-polling and wait-absent.
  • Live (iOS 26.2 sim): wait text … 9000 with the runner killed and the app terminated gave 4/4 readinessPhase: "runner-start". On main before this PR, the same scenario gave 5/5 wait_capture_stalled. wait absent on the found app gave 4/4 wait_target_present, with no readinessPhase.

@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.81 MB 4.81 MB -633 B
Package (unpacked) 4.81 MB 4.81 MB -633 B
Package (download) 1.44 MB 1.44 MB -432 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 30.3 ms 29.8 ms -0.5 ms
CLI --help 84.5 ms 83.1 ms -1.4 ms

@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://callstack.github.io/agent-device/pr-preview/pr-2893/

Built to branch gh-pages at 2026-09-24 17:08 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@thymikee

Copy link
Copy Markdown
Member Author

Adversarial review: clean. Applied its doc notes in cd646b8cc6:

  • commands.md now says runner-start covers the time before the runner answers its first command, which includes first-command warm-up.
  • The poll outcome list now includes readiness.
  • The CHANGELOG notes that other requests cancelled while the runner is starting now carry readinessPhase too.

The review's smaller shape, one readiness reason with runner-restart folded into it, is blocked: wait_runner_restart_exhausted has shipped since v0.21.0. Still open under #2343: the headline case (discovery failed, then the fallback capture was cancelled on a runner that was already ready) and reporting readiness time separately from observation time. Also related: #2894 (a runner-start timeout stops the runner).

@thymikee thymikee left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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') {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thymikee

Copy link
Copy Markdown
Member Author

At cd646b8, throwIfTargetResolutionCancelled tags any abort during resolveTargetForObservation as target-discovery, but that path also covers the steady-state re-check. With a cached target, every poll calls readSnapshotTargetProcessStartTime(cached.pid, { signal }) (https://github.com/callstack/agent-device/blob/cd646b8/packages/platform-apple/src/snapshot-target.ts#L51-L57), and if the deadline lands there, the route rethrows with readinessPhase: 'target-discovery' (https://github.com/callstack/agent-device/blob/cd646b8/packages/platform-apple/src/snapshot-route.ts#L144) even though no discovery ran. cancelledPoll then reports wait_readiness_exhausted, which outranks earlier present evidence in wait-absent.ts, so a wait absent whose earlier polls already saw the element can report readiness-exhausted instead of wait_target_present, and a positive wait can report readiness instead of a plain deadline. This breaks the PR's claim that normal waits are unchanged. readinessPhase should only be attached by the code doing discovery at the moment of cancellation; move the tagging into createSimulatorSnapshotTargetResolver, tag only the discovery wait's cancelled callback, call signal.throwIfAborted() right after the cached ps re-check so an aborted re-check doesn't drop the cache or start a discovery, and restore a plain throwIfAborted() in the route's catch. Please add a regression test that aborts during the cached ps re-check and confirms wait absent still reports wait_target_present and a positive wait still reports wait_deadline_exceeded.

Does the change need a separate readinessPhaseDetails wrapper, or could producers just write the phase literal checked against a type-only ReadinessPhase? That would also remove the value import at runner-lifecycle.ts (https://github.com/callstack/agent-device/blob/cd646b8/packages/platform-apple/src/runner/runner-lifecycle.ts#L8), which is the edge the failing eager-closure-budgets checks point at (app-lifecycle-facade, doctor-facade, runner-operations-facade all grow by one), and CI looks related to this PR for that reason. Not blocking: the waitTimeoutError if-ladder could become a switch with assertNever so a missed arm fails to compile, runnerStarting could be renamed to say it reflects entry-time liveness rather than current state, the runner-restart-exhausted/readiness-exhausted OR in wait-absent.ts and cancelledPoll could become one named predicate, and the CHANGELOG could note that bridge-route captures cancelled during target resolution now throw with a readinessPhase instead of the raw signal reason — take or leave these.

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 ps call throws or returns a non-zero exit in the Apple tool provider; either way it leads to the same mislabel, and how often it happens depends on how long ps takes relative to the rest of the poll. I read the code rather than running the added tests, so whether they'd fail on the pre-change code is inferred, not observed. Please tag target-discovery only in the resolver that owns real discovery, add the wait_target_present regression test, and switch the contracts/wait import to type-only so the eager-closure check passes before this merges.

@thymikee
thymikee force-pushed the fix/wait-readiness-exhausted branch from cd646b8 to aed9cd8 Compare September 24, 2026 13:08
@thymikee

Copy link
Copy Markdown
Member Author

Rebased onto main and squashed into one commit, aed9cd8562. The only conflict was the CHANGELOG, where the #2860 entry landed above; both entries are kept. pnpm check:affected --run passed on this head.

@thymikee

Copy link
Copy Markdown
Member Author

Correction to my previous comment: pnpm check:affected --run on aed9cd8 did not pass locally; it exited 1 with three 5 s timeouts.

  • runner-request-cancellation.test.ts and runner-session-lifecycle.test.ts pass when run alone (22/22). Host load was around 200 during the gate.
  • provider-scenarios/ios-lifecycle.test.ts ("Settings flow uses scripted simctl and runner providers") times out locally on this head and on origin/main (2/2 each, load about 40). It passes in main's CI Integration Tests, so it's a local-environment failure, not this PR.

This PR's CI Integration Tests job is the authoritative check.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed at aed9cd8. The mislabel from the last review is still there, and a new import breaks the Coverage job.

throwIfTargetResolutionCancelled still tags every abort in resolveTargetForObservation with readinessPhase: 'target-discovery', also during the cached-target check where no discovery runs (snapshot-target.ts#L51-L57). wait-absent.ts then puts readiness-exhausted ahead of earlier present evidence (wait-absent.ts#L95). So when the deadline lands on that check, wait absent reports wait_readiness_exhausted instead of wait_target_present, and a positive wait reports readiness instead of wait_deadline_exceeded. Callers then retry for the wrong reason. Please tag only the discovery wait in createSimulatorSnapshotTargetResolver, and use a plain signal.throwIfAborted() after the cached check and in the route catch (snapshot-route.ts#L144).

The regression test is still missing. Please add one that caches a target, aborts inside readSnapshotTargetProcessStartTime, and expects wait_target_present for wait absent and wait_deadline_exceeded for a positive wait.

The Coverage failure is likely from this PR. The value import of readinessPhaseDetails from @agent-device/contracts/wait in runner-lifecycle.ts#L8 adds contracts/src/wait.ts to the eager closure of three facades, and eager-closure-budgets fails on them. Could platform-apple write { readinessPhase: 'runner-start' satisfies ReadinessPhase } with a type-only import, here and in snapshot-route.ts#L20?

After these fixes, please attach a live iOS Simulator run: one wait whose timeout ends during runner start, showing wait_readiness_exhausted with readinessPhase: 'runner-start', and one wait absent on an already found app that still reports wait_target_present.

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
@thymikee
thymikee force-pushed the fix/wait-readiness-exhausted branch from aed9cd8 to 24401a6 Compare September 24, 2026 17:02
@thymikee

thymikee commented Sep 24, 2026 •

Copy link
Copy Markdown
Member Author

Sweep pass. Rebased onto dc9ab863ad (#2922 removed CHANGELOG.md, so the entry is dropped; the user-facing wording is in the PR body). Head: 24401a6.

Due items from the 12:41Z and 13:39Z reviews:

  • target-discovery mislabel: fixed. Only the resolver's discovery wait tags the cancellation now (snapshot-target.ts, discoveryCancelled). The cached ps re-check is followed by signal.throwIfAborted(), so an aborted re-check keeps the cache and starts no discovery. The route no longer tags. Its catch rethrows the resolver's own request-canceled error, or else signal.reason. A plain throwIfAborted() there would drop the resolver's tag, because the tagged error only arrives through that catch.
  • Regression tests: added. Resolver test: an aborted re-check keeps the target, starts no second discovery and has no phase. Route test: ps rejects or exits 1 on abort, and there is no phase and no second spawn in either case. Daemon wait tests with that untagged cancellation: wait absent still returns wait_target_present, and a positive wait returns wait_deadline_exceeded. A src test cannot import platform-apple internals, so the chain is split at the error the route throws.
  • Coverage / eager-closure: fixed. readinessPhaseDetails is removed. platform-apple writes readinessPhase: 'runner-start' satisfies ReadinessPhase with a type-only import. eager-closure-budgets passes 688/688 locally.
  • Non-blocking notes, all taken: waitTimeoutError is a switch with assertNever; runnerStarting is now runnerNeverAnswered over livenessAtEntry; isSelfReportedWaitDeadline names the restart/readiness pair.
  • Found live: a poll refused with APP_NOT_RUNNING followed by a discovery cut by the deadline reported readiness and lost the refusal. The earlier retriable refusal now wins, and readinessPhase is added to its details. There is a test for this.

Live on a fresh iOS 26.2 sim (deleted afterwards):

  • wait text … 9000 with the runner killed and the app terminated: 4/4 wait_readiness_exhausted, readinessPhase: "runner-start".
  • wait absent 'label="Agent Device Tester"' 6000 on the already-found app: 4/4 wait_target_present. 3 of those ended on a deadline-cancelled steady-state poll, and none carried readinessPhase.

pnpm check:affected --run on 24401a6: exit 0 (2865 tests), after a first run under host load 200+ timed out 13 unrelated or contention-bound tests (all 13 passed in the full rerun; the four runner-file failures also passed when rerun alone).

@thymikee

Copy link
Copy Markdown
Member Author

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 signal.throwIfAborted() sites sit outside this delta: resolveFailureFallbackIdentity (https://github.com/callstack/agent-device/blob/24401a6/packages/platform-apple/src/snapshot-route.ts#L493) and the launch probe (https://github.com/callstack/agent-device/blob/24401a6/packages/platform-apple/src/snapshot-observability.ts#L118). A deadline during a stale-target refresh there still drops the resolver's target-discovery tag and falls back to the pre-PR capture-stalled/deadline reason, so it under-reports readiness rather than mislabeling it. Is that worth a follow-up, or is under-reporting acceptable here?

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 wait and snapshot on iOS through runner-lifecycle.ts executeRunnerCommand, the snapshot route and target resolver, and wait-polling.ts, which is exactly the code this PR touches, so it needs to finish green on 24401a6 before merge, and a failure there needs attribution rather than a pass-on-main dismissal.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 24, 2026
@thymikee
thymikee merged commit 3e5af3a into main Sep 24, 2026
20 checks passed
@thymikee
thymikee deleted the fix/wait-readiness-exhausted branch September 24, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant