Skip to content

fix(ios-runner): resend read-only commands across the RUNNER_BUSY drain window - #2804

Open
okwasniewski wants to merge 2 commits into
callstack:mainfrom
okwasniewski:oskar/runner-busy-resend-window
Open

okwasniewski wants to merge 2 commits into
callstack:mainfrom
okwasniewski:oskar/runner-busy-resend-window

Conversation

@okwasniewski

@okwasniewski okwasniewski commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Running the Bluesky agentic e2e suite on one simulator (agent-device 0.21.6, iOS 26.2) produced 752 RUNNER_BUSY answers over two days, 723 of them to snapshot. The shape is always the same: a snapshot on a keyboard-up screen has its snapshot_viewport read abandoned after the 1s slice, recovers through private AX and returns ok=1, and the very next snapshot is refused while the abandoned block still drains. Verbatim runner.log:

15:03:47.211 MAIN_THREAD_WORK_ABANDONED operation=snapshot_viewport slice=1.0
15:03:47.528 SNAPSHOT_RECOVERED backend=private-ax ... COMPLETED ok=1
15:03:47.693 RUNNER_BUSY command=snapshot abandonedForSeconds=0.5
15:03:47.921 RUNNER_BUSY command=snapshot abandonedForSeconds=0.7   (daemon retry 1)
15:03:48.334 RUNNER_BUSY command=snapshot abandonedForSeconds=1.1   (daemon retry 2, client gets the error)
15:03:48.395 ABANDONED_WORK_DRAINED

The runner refuses fast on purpose (#1105) and expects the daemon to back off. The daemon's read-only resend in runAppleRunnerCommand used the default retryWithPolicy: three attempts at 200/400ms, about 0.6s total. Across 88 abandonment episodes the drain took 1.2s median, so the resend gave up right before the runner was answering again.

Change

  • runner-client.ts: read-only resends keep retrying a RUNNER_BUSY refusal across eight attempts (200, 400, 800, then 1000ms, no jitter: 5.4s of delay). Transport failures keep the previous three attempts. Both budgets are stated in one place (readOnlyResendBudget). The wait takes the request abort signal, and a caller deadline (a wait poll bounding the capture) that lands mid-window rethrows the last busy refusal so the wait keeps the runner's own code; only a cancelled request reports as a cancellation.
  • runner-error-classification.ts: RUNNER_BUSY is a row in RUNNER_ERROR_RULES (runner_busy_refusal, drainResend verdict) read by isRunnerBusyError. MAIN_THREAD_TIMEOUT is excluded on purpose, that command already spent its wait. isStructuredRunnerFailure moved here from runner-session.ts.
  • runner-lifecycle.ts: a structured runner reply no longer triggers a lifecycle status probe. The reply already says whether the command ran, so a busy refusal is rethrown to the caller's resend policy instead of paying a status round trip per attempt. This applies to mutating commands as well: they meet the refusal once, with no probe and no resend (pinned by a test).

Related: #2783 covers the runner side of the same symptom (query sweep outliving its slice).

Validation

  • runner-command-busy-resend.test.ts replays the logged sequence: three RUNNER_BUSY answers spaced 200/400/800ms, then success. Fails on main with The iOS runner is still finishing a previous command, passes here. It also pins that refusals outlasting the window still fail with RUNNER_BUSY, and that a request abort wakes the delay.
  • The transport resend budget stays at three attempts. isRunnerBusyError has direct negatives for MAIN_THREAD_TIMEOUT and message-only busy errors.
  • vitest --project apple-runner: 57 files, 603 tests pass. tsc -b packages/platform-apple, oxlint, oxfmt, check:layering clean. check:production-exports reports 69 preexisting unused exports, none in touched files.

Copilot AI lite review requested due to automatic review settings September 23, 2026 11:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The retry delay must support cancellation, and the requested test and file-organization follow-ups remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity · 2 Low severity

Open (3)
What changed in this PR

Extends iOS runner read-only retries across transient RUNNER_BUSY drain windows using typed error classification.

Changes:

  • Adds an eight-attempt busy retry policy while preserving transport retry limits.
  • Adds typed RUNNER_BUSY detection.
  • Adds regression tests for busy and transport retry behavior.
File Summary and findings
packages/​platform-apple/​src/​runner/​runner-contract.ts Adds typed busy-error classification. Nit (2 votes): Add direct negative assertions for MAIN_THREAD_TIMEOUT and message-only busy errors.
packages/​platform-apple/​src/​runner/​runner-client.ts Extends read-only busy retries. Moderate (2 votes): Pass the combined request signal so cancellation interrupts retry delays.
packages/​platform-apple/​src/​runner/​__tests__/​runner-command-retry.test.ts Covers busy and transport retry behavior. Nit (2 votes): Split retry-policy cases into a dedicated module because this file exceeds 1,000 lines.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/platform-apple/src/runner/runner-client.ts
Comment thread packages/platform-apple/src/runner/__tests__/runner-command-retry.test.ts Outdated
Comment thread packages/platform-apple/src/runner/runner-contract.ts Outdated
@okwasniewski

Copy link
Copy Markdown
Contributor Author

Addressed the review in 84f08e9:

  • Cancellable wait: retryWithPolicy now receives resolveRunnerRequestSignal(options), so an abort during a busy delay wakes it and the next loop iteration throws the canceled-request error rather than sending one more attempt. Covered by a fake-timer test that aborts 50ms into the first delay and asserts only snapshot, status were sent.
  • Oversized suite: the resend tests moved to runner-command-busy-resend.test.ts with their own fixtures; runner-command-retry.test.ts is back to its merge-base length, so the size ratchet passes.
  • Classifier negatives: runner-error-classification.test.ts asserts MAIN_THREAD_TIMEOUT, a message-only busy error with the retriable flag, a bare RUNNER_BUSY message and a non-AppError all read as not busy.
  • shouldResendReadOnlyRunnerCommand is module-private again (the Fallow audit failure was the unused export).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)
Resolved since last review (2)

Comment thread packages/platform-apple/src/runner/runner-client.ts
Copilot AI review requested due to automatic review settings September 23, 2026 12:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The readiness-preflight path can restart the runner before the new busy-retry behavior applies.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)

@okwasniewski

Copy link
Copy Markdown
Contributor Author

Restructured after an internal quality review (c0ef0fa):

  • executeRunnerCommand no longer runs a lifecycle status probe for a structured runner reply. A RUNNER_BUSY refusal already says the command did not run, so each resend was paying a status round trip and the final error was a two-layer wrap of the refusal. The raw refusal now reaches the caller. isStructuredRunnerFailure moved from runner-session.ts to the contract.
  • RUNNER_BUSY is a row in RUNNER_ERROR_RULES (runner_busy_refusal, drainResend verdict); isRunnerBusyError reads the table like its siblings and diagnostics name the refusal instead of flagged_retriable.
  • One place states both resend budgets (readOnlyResendBudget) instead of splitting the busy cap into maxAttempts and the transport cap into the predicate. The doc calls the window a heuristic and points at the abandonedForSeconds= marker for tuning.
  • Tests reuse makeRunnerSession from the shared fixtures and assert no status probes between resends.

@okwasniewski

Copy link
Copy Markdown
Contributor Author

The iOS smoke failure on the latest run (regular depth-1 snapshot must disclose the Simulator AX bridge evidence gap, run 35862236341) looks like the lane, not this change:

  • That run's snapshots were slow across the board (p95 3426ms, max 5626ms over 54 captures, all served by the XCTest tree), which is when the bridge tier does not get to answer.
  • The same assertion failed today on unrelated branches: fix/scroll-movement-disclosure (run 35862792165) and fix/native-stack-bridge-hittability (run 35845257256). Same shape as ci(ios): the iOS simulator smoke lane is failing on main across several rotating signatures #2491.
  • This PR only changes what the daemon does after a runner error; the failing step's snapshot succeeded on the first send.

The other smoke job and every other check on this head are green. I don't have rerun rights; a rerun of the failed job should clear it.

Copilot AI review requested due to automatic review settings September 23, 2026 13:33
@okwasniewski
okwasniewski force-pushed the oskar/runner-busy-resend-window branch from c0ef0fa to 890069c Compare September 23, 2026 13:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Retry-budget handling and lifecycle regression coverage need correction.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)

@thymikee

Copy link
Copy Markdown
Member

Two things should be fixed before this merges, reviewed at 890069c.

Does the read-only resend loop in runAppleRunnerCommand (https://github.com/callstack/agent-device/blob/890069c/packages/platform-apple/src/runner/runner-client.ts#L81) swallow the RUNNER_BUSY refusal when a wait/find deadline cancels mid-resend? wait-polling.ts (https://github.com/callstack/agent-device/blob/890069c/packages/platform-apple/src/runner/wait-polling.ts#L343) promises the last typed cause survives a mid-capture cancel, but inside the resend window retryWithPolicy throws createRequestCanceledError instead, so waitTimeoutError falls back to the generic waitCaptureStalledError and drops the runner's own code, message, and hint. Before this change, each poll returned RUNNER_BUSY directly within about 0.6s, so the tracker and rethrowNeverReadableCause saw it. Any wait/find whose timeout lands inside the resend window (up to about 6.5s) now reports a stalled-capture error instead of a busy-runner error, which is worse guidance for whoever has to act on the retry. Can the loop keep the last busy refusal and rethrow it when the abort comes after at least one refusal, treating only a real request cancel as a bare cancellation? A wait-level test through runAppleRunnerCommand with busy refusals and a short timeout, asserting details.runnerErrorCode === 'RUNNER_BUSY', would confirm the fix.

The only evidence for this change is mocked unit tests at the executeRunnerCommandWithSession seam (https://github.com/callstack/agent-device/blob/890069c/packages/platform-apple/src/runner/runner-client.ts#L68); the runner.log in the PR body predates the change, so nothing shows a resend with the same commandId actually landing after a drain on a real runner. A live iOS simulator run that reaches the drain window is needed, for example a snapshot on a keyboard-up screen where snapshot_viewport gets abandoned, with the runner.log showing AGENT_DEVICE_RUNNER_BUSY at least twice for one commandId followed by COMMAND_COMPLETED ok=1 for that same commandId, plus the client output showing the snapshot succeeding. The same run should also try wait --timeout 2000 and show which error code it reports, since that is exactly the case the first finding raises.

Not blocking: the PR body says mutating commands are untouched but the status-recovery bypass also reaches them with no test pinning that shape, the body names runner-contract.ts/shouldResendReadOnlyRunnerCommand and "about 5.4s" where the code has runner-error-classification.ts/runner-client.ts and a 4-6.5s comment, and the added COMMAND_FAILED check on isRunnerMainThreadOccupiedError looks fine since no producer emits RUNNER_BUSY under another code today — take or leave these.

The Smoke Tests failure about the AX bridge evidence gap comes from a snapshot that succeeded on its first send with no RUNNER_BUSY in the log, and this diff only touches the busy resend and structured-error recovery, not tier selection or the bridge-gap disclosure, so it looks unrelated; the same assertion also fails on other branches (#2491).

Before this merges, the resend loop needs to keep the last busy refusal through a mid-window deadline cancel, and a live simulator run through the drain window needs to be attached with the log evidence above.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Resolved since last review (2)

Comment on lines +66 to +70
function isRunnerRequestCancelled(options: AppleRunnerCommandOptions): boolean {
return (
isRequestCanceled(options.requestId) || getRequestSignal(options.requestId)?.aborted === true
);
}
@okwasniewski

Copy link
Copy Markdown
Contributor Author

[claude-fable-5-1] responding on behalf of Oskar

Thanks, reviewed at 890069c, both points taken.

Mid-window deadline (fixed, 0e12a46). The resend loop remembers the last RUNNER_BUSY refusal and, when the abort that ends the window is a caller deadline rather than a request cancel, rethrows that refusal. A real request cancel (the registered request signal or isRequestCanceled) still reports as a cancellation. Test: a TimeoutError abort 50ms into the first delay yields an error with details.runnerErrorCode === 'RUNNER_BUSY' and retriable: true, with one snapshot sent; a registered-request cancel in the same position yields the canceled error. The wait path itself is unchanged: unobserved.attempt records that refusal as the never-readable cause and rethrowNeverReadableCause surfaces it as before.

Non-blocking items. A test now pins that a mutating command meets RUNNER_BUSY once with no status probe and no resend. PR body corrected (runner-error-classification.ts, readOnlyResendBudget, the resend policy has jitter: 0 so 5.4s is the actual window). The COMMAND_FAILED check on isRunnerMainThreadOccupiedError is gone; it reads the busy verdict from the rules table and the MAIN_THREAD_TIMEOUT code directly.

Live run. Not delivered yet, honestly. On a spare simulator (iPhone 17, iOS 27.0) with the Bluesky App Store build on the sign-in form, keyboard up with typing predictions and 80 characters typed, regular snapshots forced onto the runner tree path (112 runner captures, 25 of them under 20-core host CPU load), the tree capture ran at a 1.74s median and never exceeded its slice: 0 MAIN_THREAD_WORK_ABANDONED, 0 RUNNER_BUSY. The drain in the report came from the composer on the e2e Release build (iOS 26.2, iPhone 16e), which is Oskar's working device. A script that runs the exact loop you describe (busy per commandId, the COMMAND_COMPLETED ok=1 for that id, client output, and wait --timeout 2000 fired the moment a refusal lands) is ready; Oskar will run it on that device and attach the log excerpt here.

@okwasniewski

Copy link
Copy Markdown
Contributor Author

[claude-fable-5-1] responding on behalf of Oskar

The Android smoke failure on 0e12a46 (run 35900071437) is wait text "Automation lab" 10000 timing out right after the fixture app launch on the emulator, before any assertion the suite is about. This diff changes six files under packages/platform-apple/src/runner and nothing Android or shared, so it cannot reach that path. Emulator readiness flake; a rerun of that job should clear it.

…it deadline

A wait bounds each poll with its own abort signal and keeps the last typed
refusal as the wait's cause. Inside the resend window that deadline surfaced
as a bare cancellation, so a wait timing out mid-window reported a stalled
capture and dropped the runner's own code, message and hint. The resend loop
now remembers the last busy refusal and rethrows it when the abort is a
caller deadline; only a cancelled request reports as a cancellation.

Also: jitter 0 on the resend policy so the documented 5.4s window is the
actual one, and a test pinning that a mutating command meets RUNNER_BUSY once
with no status probe and no resend.
Copilot AI review requested due to automatic review settings September 23, 2026 18:15
@okwasniewski
okwasniewski force-pushed the oskar/runner-busy-resend-window branch from 0e12a46 to 431ef62 Compare September 23, 2026 18:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 Medium severity

Open (2)

Comment on lines +66 to +69
function isRunnerRequestCancelled(options: AppleRunnerCommandOptions): boolean {
return (
isRequestCanceled(options.requestId) || getRequestSignal(options.requestId)?.aborted === true
);
@thymikee

Copy link
Copy Markdown
Member

Follow-up review at 431ef62 (after 890069c). The code change looks reasonable, but it still needs live proof that the resend works on a real iOS Simulator.

The resend and the wait-deadline handling only have mocked coverage at executeRunnerCommandWithSession (runner-client.ts). Please attach a run that drives the app into the drain window, for example a snapshot on a keyboard-up screen where snapshot_viewport is abandoned. The runner.log excerpt should show AGENT_DEVICE_RUNNER_BUSY at least twice for one commandId, then COMMAND_COMPLETED ok=1 for the same commandId, and the client output should show the snapshot succeeding. In the same session, a wait ... --timeout 2000 fired during a refusal should report details.runnerErrorCode: RUNNER_BUSY, not wait_capture_stalled.

Not blocking: isRunnerRequestCancelled infers a deadline from registry absence instead of the typed abort reason. Could a deadline that lands mid-fetch during a resend rethrow the raw TimeoutError instead of the last busy refusal? I could not find a production read-only caller that hits this today.

CI was still queued or running at review time. This diff touches the Apple runner command route, so a failure in the iOS Smoke Tests or the runner unit and coverage suites would need attribution.

This branch has not been deployed

No deployments
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.

3 participants