fix(ios-runner): resend read-only commands across the RUNNER_BUSY drain window - #2804
okwasniewski wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
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
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_BUSYdetection. - 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.
|
Addressed the review in 84f08e9:
|
|
Restructured after an internal quality review (c0ef0fa):
|
|
The iOS smoke failure on the latest run (
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. |
c0ef0fa to
890069c
Compare
|
Two things should be fixed before this merges, reviewed at 890069c. Does the read-only resend loop in The only evidence for this change is mocked unit tests at the 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 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. |
| function isRunnerRequestCancelled(options: AppleRunnerCommandOptions): boolean { | ||
| return ( | ||
| isRequestCanceled(options.requestId) || getRequestSignal(options.requestId)?.aborted === true | ||
| ); | ||
| } |
|
[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 Non-blocking items. A test now pins that a mutating command meets 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 |
|
[claude-fable-5-1] responding on behalf of Oskar The Android smoke failure on 0e12a46 (run 35900071437) is |
…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.
0e12a46 to
431ef62
Compare
| function isRunnerRequestCancelled(options: AppleRunnerCommandOptions): boolean { | ||
| return ( | ||
| isRequestCanceled(options.requestId) || getRequestSignal(options.requestId)?.aborted === true | ||
| ); |
|
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 Not blocking: 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. |


Summary
Running the Bluesky agentic e2e suite on one simulator (agent-device 0.21.6, iOS 26.2) produced 752
RUNNER_BUSYanswers over two days, 723 of them tosnapshot. The shape is always the same: a snapshot on a keyboard-up screen has itssnapshot_viewportread abandoned after the 1s slice, recovers through private AX and returnsok=1, and the very next snapshot is refused while the abandoned block still drains. Verbatimrunner.log:The runner refuses fast on purpose (#1105) and expects the daemon to back off. The daemon's read-only resend in
runAppleRunnerCommandused the defaultretryWithPolicy: 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 aRUNNER_BUSYrefusal 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 (awaitpoll 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_BUSYis a row inRUNNER_ERROR_RULES(runner_busy_refusal,drainResendverdict) read byisRunnerBusyError.MAIN_THREAD_TIMEOUTis excluded on purpose, that command already spent its wait.isStructuredRunnerFailuremoved here fromrunner-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.tsreplays the logged sequence: threeRUNNER_BUSYanswers spaced 200/400/800ms, then success. Fails onmainwithThe iOS runner is still finishing a previous command, passes here. It also pins that refusals outlasting the window still fail withRUNNER_BUSY, and that a request abort wakes the delay.isRunnerBusyErrorhas direct negatives forMAIN_THREAD_TIMEOUTand message-only busy errors.vitest --project apple-runner: 57 files, 603 tests pass.tsc -b packages/platform-apple,oxlint,oxfmt,check:layeringclean.check:production-exportsreports 69 preexisting unused exports, none in touched files.