From 31eecfffcf6e01b7af85d41088d8a64d77ac7de5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 24 Sep 2026 10:49:38 +0200 Subject: [PATCH] fix(apple-runner): resend a read only on the runner's busy refusal The transport's same-request resend also matched a generic flagged_retriable row keyed on details.retriable, which tells a caller's poll to try again. Startup-phase failures (runner_phase_budget_exhausted, apple_toolchain_probe_unavailable) and external provider failures carry that flag, so a read-only command resent them up to three times, each with a fresh startup budget. Drop that row and the app_not_running row that undid it for APP_NOT_RUNNING: #2804's runner_busy_refusal row, keyed on the RUNNER_BUSY runner code, is now the only resend rule for a structured runner refusal. Closes #2862 --- CHANGELOG.md | 8 ++ .../runner-error-classification.test.ts | 15 ++- .../__tests__/runner-recovery-wiring.test.ts | 97 ++++++++++++++++++- .../src/runner/runner-error-classification.ts | 37 +++---- 4 files changed, 124 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4ee6cdf56..df12da1963 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ ## Unreleased +- Changed (apple): a read-only runner command is resent inside the same request only when the + runner refused it as `RUNNER_BUSY`. Before, any `COMMAND_FAILED` carrying `details.retriable: + true` was sent up to three times. That flag tells a caller's own poll, such as `wait`, to try + again. It does not mean a resend is safe inside one request. A runner startup that ran out of its + budget (`runner_phase_budget_exhausted`) or could not read the Xcode toolchain + (`apple_toolchain_probe_unavailable`) now fails once, not three times with a fresh startup budget + each time. A `retriable` failure from an external Apple runner provider is also no longer resent. + The error keeps `retriable: true`, so the caller's next request still tries again. (#2862) - Fixed (mobile): a read taken right after a `scroll`, `swipe`, or `gesture swipe` no longer reports a definite miss when the surface never settled. When post-gesture stabilization ran out of budget on a surface still moving, `is visible` answered a plain `selector_not_found` and `is absent` diff --git a/packages/platform-apple/src/runner/__tests__/runner-error-classification.test.ts b/packages/platform-apple/src/runner/__tests__/runner-error-classification.test.ts index 64b379f792..2919324ad9 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-error-classification.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-error-classification.test.ts @@ -59,11 +59,16 @@ test('boot-shaped failures are not retryable', () => { ); }); -test('an explicitly retriable flag wins over any message denial', () => { - const flagged = runnerConnectFailure('xcodebuild_exited_early', 'xcodebuild exited early', { - retriable: true, - }); - assert.equal(isRetryableRunnerError(flagged), true); +test('only the runner busy refusal earns a resend; a retriable flag alone does not', () => { + const busy = classifyRunnerReportedError('RUNNER_BUSY'); + assert.equal(isRetryableRunnerError(new AppError(busy.code, 'busy', busy.details)), true); + const notRunning = classifyRunnerReportedError('APP_NOT_RUNNING'); + assert.equal(notRunning.details.retriable, true); + assert.equal( + isRetryableRunnerError(new AppError(notRunning.code, 'not running', notRunning.details)), + false, + ); + assert.equal(isRetryableRunnerError(commandFailed('boom', { retriable: true })), false); }); test('retryable requires an AppError with COMMAND_FAILED', () => { diff --git a/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts b/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts index 5ca6489722..b71fe25732 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts @@ -1,12 +1,18 @@ import assert from 'node:assert/strict'; import { afterEach, beforeEach, expect, test, vi } from 'vitest'; import { AppError } from '@agent-device/kernel/errors'; +import { resetAllProcessMemosForTests } from '@agent-device/kernel/ttl-memo'; import { IOS_SIMULATOR } from './device-fixtures.ts'; import type { ExecResult } from '@agent-device/host-kit/command'; import type { RunnerSession } from '../runner-session.ts'; import { appleRunnerTestHost } from '../test-host.ts'; import { withAppleRunnerProvider } from '../runner-provider.ts'; -import type { RunnerCommand } from '../runner-contract.ts'; +import { classifyRunnerReportedError, type RunnerCommand } from '../runner-contract.ts'; +import { + createRunnerPhaseBudget, + requireRunnerPhaseRemainingMs, + resolveExpectedRunnerCacheMetadata, +} from '../runner-cache-metadata.ts'; import { startFakeRunnerServer, type FakeRunnerServer } from './fake-runner-server.ts'; /** @@ -298,7 +304,8 @@ test.each([undefined, 'get', 'accept', 'dismiss'] as const)( 'alert action %s selects provider retries by mutation semantics', async (action) => { const commands: RunnerCommand[] = []; - const failure = new AppError('COMMAND_FAILED', 'response unavailable', { retriable: true }); + const busy = classifyRunnerReportedError('RUNNER_BUSY'); + const failure = new AppError(busy.code, 'runner busy', busy.details); const result = withAppleRunnerProvider( async (_device, command) => { commands.push(command); @@ -346,3 +353,89 @@ test('a read refused over a not-running app is one definite answer, not a transp ); assert.equal(invalidateRunnerSessionMock.mock.calls.length, 0); }); + +test('a read the runner refused as busy is resent on the same session', async () => { + server = await startFakeRunnerServer({ + snapshot: [ + { kind: 'runnerError', code: 'RUNNER_BUSY', message: 'runner is draining' }, + { kind: 'ok', data: { captured: true } }, + ], + status: [ + { + kind: 'ok', + data: { + lifecycleState: 'failed', + lifecycleErrorCode: 'RUNNER_BUSY', + lifecycleErrorMessage: 'runner is draining', + }, + }, + ], + }); + seedSession(server.port); + + assert.deepEqual(await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'snapshot' }), { + captured: true, + }); + assert.equal(server.requests.filter((request) => request.command === 'snapshot').length, 2); + assert.equal(invalidateRunnerSessionMock.mock.calls.length, 0); +}); + +// `retriable` on these tells the caller's next request to try again. Resending inside this request +// would open a fresh startup budget per attempt, or second-guess a provider's own transport policy. +const readSnapshot = () => runAppleRunnerCommand(IOS_SIMULATOR, { command: 'snapshot' }); +const sessionStarts = () => ensureRunnerSessionMock.mock.calls.length; + +test.each([ + { + producer: 'a spent startup budget', + reason: 'runner_phase_budget_exhausted', + arrange: () => { + ensureRunnerSessionMock.mockImplementation(async () => + requireRunnerPhaseRemainingMs(createRunnerPhaseBudget(0, undefined), 'runner_startup'), + ); + return { run: readSnapshot, sends: sessionStarts }; + }, + }, + { + producer: 'an unreadable toolchain', + reason: 'apple_toolchain_probe_unavailable', + arrange: () => { + resetAllProcessMemosForTests(); + appleRunnerTestHost.update({ + runCmdSync: () => ({ exitCode: 1, stdout: '', stderr: 'xcode-select: error' }), + }); + ensureRunnerSessionMock.mockImplementation(async () => + resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR), + ); + return { run: readSnapshot, sends: sessionStarts }; + }, + }, + { + producer: 'an external runner provider', + reason: 'provider_transport_unavailable', + arrange: () => { + let calls = 0; + const provider = async () => { + calls += 1; + throw new AppError('COMMAND_FAILED', 'provider transport unavailable', { + reason: 'provider_transport_unavailable', + retriable: true, + }); + }; + return { + run: () => withAppleRunnerProvider(provider, { deviceId: IOS_SIMULATOR.id }, readSnapshot), + sends: () => calls, + }; + }, + }, +])('a retriable read failure from $producer is not resent', async ({ reason, arrange }) => { + const { run, sends } = arrange(); + + await assert.rejects(run(), (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.reason, reason); + assert.equal(error.details?.retriable, true); + return true; + }); + assert.equal(sends(), 1); +}); diff --git a/packages/platform-apple/src/runner/runner-error-classification.ts b/packages/platform-apple/src/runner/runner-error-classification.ts index aa8e1c072b..97dd1b393f 100644 --- a/packages/platform-apple/src/runner/runner-error-classification.ts +++ b/packages/platform-apple/src/runner/runner-error-classification.ts @@ -9,11 +9,7 @@ import { type IosDeveloperDiskImageState, type IosDeveloperModeState, } from './host.ts'; -import { - APP_NOT_RUNNING_RUNNER_CODE, - MAIN_THREAD_TIMEOUT_RUNNER_CODE, - RUNNER_BUSY_RUNNER_CODE, -} from './runner-contract.ts'; +import { MAIN_THREAD_TIMEOUT_RUNNER_CODE, RUNNER_BUSY_RUNNER_CODE } from './runner-contract.ts'; export const RUNNER_CACHE_RECOVERY_HINT = 'If runner build products look stale or corrupted, run `pnpm clean:xcuitest` in a local checkout, or remove ~/.agent-device/apple-runner/derived, then retry.'; @@ -21,7 +17,7 @@ export const RUNNER_CACHE_RECOVERY_HINT = /** * Details evidence a rule requires beyond code and message. A predicate rather than a * fixed vocabulary because the useful evidence is a shape: a recorded deadline, a - * preflight marker, a retriable flag. Every predicate below names one. + * preflight marker, a runner error code. Every predicate below names one. */ type RunnerErrorDetailsMatch = (details: AppErrorDetails) => boolean; @@ -69,9 +65,12 @@ type RunnerErrorMatch = { details?: RunnerErrorDetailsMatch; }; -const hasRetriableFlag: RunnerErrorDetailsMatch = (details) => details.retriable === true; -const hasAppNotRunningRunnerCode: RunnerErrorDetailsMatch = (details) => - details.runnerErrorCode === APP_NOT_RUNNING_RUNNER_CODE; +/** + * The runner refused the command before running it while abandoned main-thread work drains (#1105). + * A resend keys on this code, never on `details.retriable`: that flag tells a caller's poll to try + * again, and its other producers (a not-running app, a spent startup budget, an unavailable + * toolchain probe, an external provider) must not be resent inside one request. + */ const hasRunnerBusyCode: RunnerErrorDetailsMatch = (details) => details.runnerErrorCode === RUNNER_BUSY_RUNNER_CODE; /** @@ -210,11 +209,8 @@ const PROFILE_UNUSABLE: RunnerErrorRule['buildFailure'] = { * and since #2680 so does the one classification of startup failures — a row * carries recovery verdicts, a `buildFailure` reason and hint, or both. * Per axis, the FIRST matching rule that defines the axis wins — which is why - * `flagged_retriable` precedes the denials (an explicitly retriable error - * stays retriable whatever its message says), and `usbmux_device_unattached` - * sits first (retrying cannot attach a cable, and its typed verdict carries - * the recovery hint a generic connect failure would replace). `app_not_running` - * precedes it too: its retriable flag is for the caller's poll, not a resend. + * `usbmux_device_unattached` sits first (retrying cannot attach a cable, and its + * typed verdict carries the recovery hint a generic connect failure would replace). */ export const RUNNER_ERROR_RULES: readonly RunnerErrorRule[] = [ { @@ -223,22 +219,11 @@ export const RUNNER_ERROR_RULES: readonly RunnerErrorRule[] = [ verdicts: { connectRetry: false }, }, { - reason: 'app_not_running', - match: { code: 'COMMAND_FAILED', details: hasAppNotRunningRunnerCode }, - verdicts: { retryable: false, connectRetry: false }, - }, - { - // Named before the generic retriable flag so diagnostics say what refused, not that a flag - // was set. Nothing ran: the runner answered before dispatching the command (#1105). + // Nothing ran: the runner answered before dispatching the command (#1105). reason: 'runner_busy_refusal', match: { code: 'COMMAND_FAILED', details: hasRunnerBusyCode }, verdicts: { retryable: true, connectRetry: true, drainResend: true }, }, - { - reason: 'flagged_retriable', - match: { code: 'COMMAND_FAILED', details: hasRetriableFlag }, - verdicts: { retryable: true, connectRetry: true }, - }, { // Says `artifactSuspect: false` on purpose: a boot that cannot compile is not cured by wiping // derived data.