From 8bc51cd82cc41f411f0050b8cb403b7d511efb63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 6 Aug 2026 15:58:46 +0200 Subject: [PATCH 1/3] refactor(ios): runner error classification as data; recovery tested at the transport seam (#1631) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Error-retry classification lived in four places consulted from one catch dispatch: two overlapping substring chains in runner-contract.ts, a code-keyed fatality chain in runner-session.ts, and an inline composite in runner-lifecycle.ts. RUNNER_ERROR_RULES is now the one declaration (mirroring RUNNER_COMMAND_TRAIT_MANIFEST): each rule names an error shape and its verdicts across the four axes (retryable, connect-retry, session-fatal reason, restart-before-send); the predicates keep their signatures and derive from the table. One deliberate widening, noted at the declaration: restart-before-send matching is now case-insensitive like the other axes (the message is our own transport literal). runner-command-recovery.ts gets its first direct coverage — through the real stack: a scripted fake iOS runner (local HTTP server) stands in for the XCTest runner process, executeRunnerCommandWithSession and the transport fetch run for real, and invalidation is observed through the module's existing invalidateSession parameter. Seven scenarios (retained response, runner-reported failure, in-flight, unknown lifecycle, probe failure, missing command id, read-only completed-without-response) plus an 11-case classification suite. Zero vi.mocks of runner internals. --- .../core/__tests__/fake-runner-server.ts | 80 ++++++++ .../__tests__/runner-command-recovery.test.ts | 151 +++++++++++++++ .../runner-error-classification.test.ts | 112 ++++++++++++ .../apple/core/runner/runner-contract.ts | 173 ++++++++++++++++-- .../apple/core/runner/runner-lifecycle.ts | 9 +- .../apple/core/runner/runner-session.ts | 12 +- 6 files changed, 502 insertions(+), 35 deletions(-) create mode 100644 src/platforms/apple/core/__tests__/fake-runner-server.ts create mode 100644 src/platforms/apple/core/__tests__/runner-command-recovery.test.ts create mode 100644 src/platforms/apple/core/__tests__/runner-error-classification.test.ts diff --git a/src/platforms/apple/core/__tests__/fake-runner-server.ts b/src/platforms/apple/core/__tests__/fake-runner-server.ts new file mode 100644 index 0000000000..be32af219b --- /dev/null +++ b/src/platforms/apple/core/__tests__/fake-runner-server.ts @@ -0,0 +1,80 @@ +import http from 'node:http'; +import type { AddressInfo } from 'node:net'; + +/** + * A deterministic fake iOS runner: a local HTTP server standing in for the + * XCTest runner process, scripted per request. Recovery/retry suites drive the + * REAL send stack (executeRunnerCommandWithSession → transport fetch) against + * it instead of vi-mocking internal functions — the #1631 testing seam. Each + * incoming request consumes the next scripted response; running out of script + * fails loudly rather than improvising. + */ + +export type FakeRunnerResponse = + | { kind: 'ok'; data: Record } + | { kind: 'runnerError'; code: string; message: string } + | { kind: 'hangUp' }; + +export type FakeRunnerRequest = { + command: string; + body: Record; +}; + +export type FakeRunnerServer = { + port: number; + requests: FakeRunnerRequest[]; + close: () => Promise; +}; + +export async function startFakeRunnerServer( + script: FakeRunnerResponse[], +): Promise { + const remaining = [...script]; + const requests: FakeRunnerRequest[] = []; + const server = http.createServer((req, res) => { + let raw = ''; + req.on('data', (chunk) => { + raw += chunk; + }); + req.on('end', () => { + const body = parseBody(raw); + requests.push({ command: String(body.command ?? ''), body }); + const next = remaining.shift(); + if (!next) { + res.statusCode = 500; + res.end(JSON.stringify({ ok: false, error: { message: 'fake runner script exhausted' } })); + return; + } + if (next.kind === 'hangUp') { + res.destroy(); + return; + } + if (next.kind === 'runnerError') { + res.setHeader('content-type', 'application/json'); + res.end(JSON.stringify({ ok: false, error: { code: next.code, message: next.message } })); + return; + } + res.setHeader('content-type', 'application/json'); + res.end(JSON.stringify({ ok: true, data: next.data })); + }); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const port = (server.address() as AddressInfo).port; + return { + port, + requests, + close: () => + new Promise((resolve, reject) => + server.close((error) => (error ? reject(error) : resolve())), + ), + }; +} + +function parseBody(raw: string): Record { + try { + const parsed: unknown = JSON.parse(raw); + return parsed && typeof parsed === 'object' ? (parsed as Record) : {}; + } catch { + return {}; + } +} diff --git a/src/platforms/apple/core/__tests__/runner-command-recovery.test.ts b/src/platforms/apple/core/__tests__/runner-command-recovery.test.ts new file mode 100644 index 0000000000..820beecb5e --- /dev/null +++ b/src/platforms/apple/core/__tests__/runner-command-recovery.test.ts @@ -0,0 +1,151 @@ +import assert from 'node:assert/strict'; +import { afterEach, test, vi } from 'vitest'; +import { AppError } from '@agent-device/kernel/errors'; +import { IOS_SIMULATOR } from '../../../../__tests__/test-utils/device-fixtures.ts'; +import type { ExecResult } from '../../../../utils/exec.ts'; +import { handleRunnerTransportErrorAfterCommandSend } from '../runner/runner-command-recovery.ts'; +import type { RunnerCommand } from '../runner/runner-contract.ts'; +import type { RunnerSession } from '../runner/runner-session.ts'; +import { + startFakeRunnerServer, + type FakeRunnerResponse, + type FakeRunnerServer, +} from './fake-runner-server.ts'; + +// Recovery driven through the REAL stack: real executeRunnerCommandWithSession, +// real transport fetch, a scripted fake runner on localhost, and the +// invalidateSession parameter the module already exposes as its seam. The only +// faked thing is the runner process itself (#1631). + +let server: FakeRunnerServer | undefined; + +afterEach(async () => { + await server?.close(); + server = undefined; +}); + +function makeRunnerSession(port: number): RunnerSession { + return { + sessionId: `fake:${port}`, + device: IOS_SIMULATOR, + deviceId: IOS_SIMULATOR.id, + port, + xctestrunPath: '/tmp/fake.xctestrun', + jsonPath: '/tmp/fake.json', + testPromise: new Promise(() => {}), + child: { pid: process.pid, exitCode: null }, + ready: true, + }; +} + +function tapCommand(commandId = 'cmd-1'): RunnerCommand { + return { command: 'tap', x: 10, y: 10, commandId } as RunnerCommand; +} + +async function runRecovery(params: { + script: FakeRunnerResponse[]; + command?: RunnerCommand; + transportError?: AppError; +}): Promise<{ + result: Promise>; + session: RunnerSession; + invalidate: ReturnType; + transportError: AppError; +}> { + server = await startFakeRunnerServer(params.script); + const session = makeRunnerSession(server.port); + const invalidate = vi.fn(async () => {}); + const transportError = params.transportError ?? new AppError('COMMAND_FAILED', 'socket hang up'); + const result = handleRunnerTransportErrorAfterCommandSend({ + device: IOS_SIMULATOR, + session, + command: params.command ?? tapCommand(), + transportError, + options: {}, + signal: undefined, + invalidationReason: 'transport_error_after_command_send', + invalidateSession: invalidate, + }); + return { result, session, invalidate, transportError }; +} + +test('a completed command with a retained response recovers without invalidation', async () => { + const { result, invalidate } = await runRecovery({ + script: [ + { + kind: 'ok', + data: { + lifecycleState: 'completed', + lifecycleResponseJson: JSON.stringify({ ok: true, data: { tapped: true } }), + }, + }, + ], + }); + + assert.deepEqual(await result, { tapped: true }); + assert.equal(invalidate.mock.calls.length, 0); + assert.equal(server?.requests[0]?.command, 'status'); + assert.equal(server?.requests[0]?.body.statusCommandId, 'cmd-1'); +}); + +test('a runner-reported failure surfaces without invalidating the session', async () => { + const { result, invalidate } = await runRecovery({ + script: [ + { kind: 'ok', data: { lifecycleState: 'failed', lifecycleErrorMessage: 'tap failed' } }, + ], + }); + + await assert.rejects(result, (error: unknown) => error instanceof AppError); + assert.equal(invalidate.mock.calls.length, 0); +}); + +test('a command still in flight surfaces without invalidating the session', async () => { + const { result, invalidate } = await runRecovery({ + script: [{ kind: 'ok', data: { lifecycleState: 'started' } }], + }); + + await assert.rejects(result, (error: unknown) => error instanceof AppError); + assert.equal(invalidate.mock.calls.length, 0); +}); + +test('an unknown lifecycle state invalidates the session and says so', async () => { + const { result, invalidate } = await runRecovery({ + script: [{ kind: 'ok', data: { lifecycleState: 'zombie' } }], + }); + + await assert.rejects(result, (error: unknown) => { + return error instanceof AppError && error.message.includes('invalidated the runner session'); + }); + assert.equal(invalidate.mock.calls.length, 1); + assert.equal(invalidate.mock.calls[0]?.[1], 'transport_error_after_command_send'); +}); + +test('a failing status probe retains the invalidation and rethrows the transport error', async () => { + const { result, invalidate, transportError } = await runRecovery({ + script: [{ kind: 'runnerError', code: 'COMMAND_FAILED', message: 'status probe exploded' }], + }); + + await assert.rejects(result, (error: unknown) => error === transportError); + assert.equal(invalidate.mock.calls.length, 1); +}); + +test('a command without an id cannot be probed: invalidate and rethrow', async () => { + const { result, invalidate, transportError } = await runRecovery({ + script: [], + command: { command: 'tap', x: 10, y: 10 } as RunnerCommand, + }); + + await assert.rejects(result, (error: unknown) => error === transportError); + assert.equal(invalidate.mock.calls.length, 1); + assert.equal(server?.requests.length, 0); +}); + +test('a completed read-only command without a retained response rethrows without invalidation', async () => { + const { result, invalidate, transportError } = await runRecovery({ + script: [{ kind: 'ok', data: { lifecycleState: 'completed' } }], + command: { command: 'snapshot', commandId: 'cmd-ro' } as RunnerCommand, + }); + + await assert.rejects(result, (error: unknown) => error === transportError); + assert.equal(invalidate.mock.calls.length, 0); +}); diff --git a/src/platforms/apple/core/__tests__/runner-error-classification.test.ts b/src/platforms/apple/core/__tests__/runner-error-classification.test.ts new file mode 100644 index 0000000000..cc6133ed10 --- /dev/null +++ b/src/platforms/apple/core/__tests__/runner-error-classification.test.ts @@ -0,0 +1,112 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { AppError } from '@agent-device/kernel/errors'; +import { + RUNNER_ERROR_RULES, + isRetryableRunnerError, + resolveRunnerFatalErrorReason, + shouldRestartRunnerBeforeCommandSend, + shouldRetryRunnerConnectError, +} from '../runner/runner-contract.ts'; + +function commandFailed(message: string, details?: Record): AppError { + return new AppError('COMMAND_FAILED', message, details); +} + +test('every rule carries a unique reason', () => { + const reasons = RUNNER_ERROR_RULES.map((rule) => rule.reason); + assert.equal(new Set(reasons).size, reasons.length); +}); + +// --- retryable axis (isRetryableRunnerError) --- + +test('transport-shaped failures are retryable', () => { + for (const message of [ + 'Runner did not accept connection on port 8100', + 'fetch failed', + 'connect ECONNREFUSED 127.0.0.1:8100', + 'socket hang up', + ]) { + assert.equal(isRetryableRunnerError(commandFailed(message)), true, message); + } +}); + +test('boot-shaped failures are not retryable', () => { + assert.equal(isRetryableRunnerError(commandFailed('xcodebuild exited early (code 65)')), false); + assert.equal( + isRetryableRunnerError(commandFailed('Device is busy (Connecting to Simulator)')), + false, + ); +}); + +test('an explicitly retriable flag wins over any message denial', () => { + const flagged = commandFailed('xcodebuild exited early', { retriable: true }); + assert.equal(isRetryableRunnerError(flagged), true); +}); + +test('retryable requires an AppError with COMMAND_FAILED', () => { + assert.equal(isRetryableRunnerError(new Error('fetch failed')), false); + assert.equal(isRetryableRunnerError(new AppError('DEVICE_NOT_FOUND', 'fetch failed')), false); +}); + +// --- connect-retry axis (shouldRetryRunnerConnectError) --- + +test('connect loop keeps waiting by default, including for unknown errors', () => { + assert.equal( + shouldRetryRunnerConnectError(commandFailed('Runner did not accept connection')), + true, + ); + assert.equal(shouldRetryRunnerConnectError(new Error('anything')), true); + assert.equal(shouldRetryRunnerConnectError(new AppError('INVALID_ARGS', 'nope')), true); +}); + +test('connect loop stops for terminal verdicts', () => { + assert.equal(shouldRetryRunnerConnectError(commandFailed('xcodebuild exited early')), false); + const unattached = new AppError('DEVICE_NOT_FOUND', 'device not attached', { + usbmuxDeviceAttached: false, + }); + assert.equal(shouldRetryRunnerConnectError(unattached), false); + // The same code without the usbmux evidence keeps waiting. + assert.equal(shouldRetryRunnerConnectError(new AppError('DEVICE_NOT_FOUND', 'gone')), true); +}); + +// --- session-fatal axis (resolveRunnerFatalErrorReason) --- + +test('session-fatal codes map to their invalidation reasons', () => { + assert.equal( + resolveRunnerFatalErrorReason(new AppError('IOS_AX_SNAPSHOT_FAILED', 'ax root failed')), + 'ax_snapshot_failure', + ); + assert.equal( + resolveRunnerFatalErrorReason(new AppError('XCTEST_RECORDED_FAILURE', 'recorded failure')), + 'xctest_recorded_failure', + ); + assert.equal( + resolveRunnerFatalErrorReason(new AppError('RUNNER_WEDGED', 'main thread stuck')), + 'runner_main_thread_wedged', + ); +}); + +test('ordinary errors are never session-fatal', () => { + assert.equal(resolveRunnerFatalErrorReason(commandFailed('socket hang up')), undefined); + assert.equal(resolveRunnerFatalErrorReason(new Error('boom')), undefined); +}); + +// --- restart-before-send axis (shouldRestartRunnerBeforeCommandSend) --- + +test('a refused connection before send restarts the session, case-insensitively', () => { + assert.equal( + shouldRestartRunnerBeforeCommandSend(commandFailed('Runner did not accept connection')), + true, + ); + assert.equal( + shouldRestartRunnerBeforeCommandSend(commandFailed('runner did not accept connection')), + true, + ); +}); + +test('a terminal connect verdict refuses the restart even when the message matches', () => { + const both = commandFailed('xcodebuild exited early: runner did not accept connection'); + assert.equal(shouldRestartRunnerBeforeCommandSend(both), false); + assert.equal(shouldRestartRunnerBeforeCommandSend(commandFailed('socket hang up')), false); +}); diff --git a/src/platforms/apple/core/runner/runner-contract.ts b/src/platforms/apple/core/runner/runner-contract.ts index ee66a8386d..2f34916cfc 100644 --- a/src/platforms/apple/core/runner/runner-contract.ts +++ b/src/platforms/apple/core/runner/runner-contract.ts @@ -130,18 +130,138 @@ export function resolveRunnerRequestSignal(options: { return AbortSignal.any([registeredSignal, options.signal]); } +type RunnerErrorMatch = { + /** Required `AppError.code`; absent = any AppError. */ + code?: string; + /** Every entry must appear in the lowercased message. */ + messageIncludesAll?: readonly string[]; + /** Required details evidence beyond code/message. */ + details?: 'retriable' | 'usbmux-device-unattached'; +}; + +type RunnerErrorVerdicts = { + /** isRetryableRunnerError: transport error worth a same-session resend. */ + retryable?: boolean; + /** shouldRetryRunnerConnectError: connect loop may keep waiting for the runner. */ + connectRetry?: boolean; + /** Session-fatal classification: invalidate the cached runner session with this reason. */ + sessionFatalReason?: string; + /** Connect-shaped failure before the command was sent: restart the session and replay. */ + restartBeforeSend?: boolean; +}; + +type RunnerErrorRule = { + /** Stable rule name for tests and diagnostics. */ + reason: string; + match: RunnerErrorMatch; + verdicts: RunnerErrorVerdicts; +}; + +/** + * The one declaration of runner error classes (#1631), mirroring + * RUNNER_COMMAND_TRAIT_MANIFEST's role for commands: every recovery predicate + * below derives from this table instead of keeping its own substring chain. + * 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). + */ +export const RUNNER_ERROR_RULES: readonly RunnerErrorRule[] = [ + { + reason: 'usbmux_device_unattached', + match: { code: 'DEVICE_NOT_FOUND', details: 'usbmux-device-unattached' }, + verdicts: { connectRetry: false }, + }, + { + reason: 'flagged_retriable', + match: { code: 'COMMAND_FAILED', details: 'retriable' }, + verdicts: { retryable: true }, + }, + { + reason: 'xcodebuild_exited_early', + match: { code: 'COMMAND_FAILED', messageIncludesAll: ['xcodebuild exited early'] }, + verdicts: { retryable: false, connectRetry: false }, + }, + { + reason: 'device_busy_connecting', + match: { code: 'COMMAND_FAILED', messageIncludesAll: ['device is busy', 'connecting'] }, + verdicts: { retryable: false }, + }, + { + reason: 'runner_connect_refused', + match: { code: 'COMMAND_FAILED', messageIncludesAll: ['runner did not accept connection'] }, + verdicts: { retryable: true, restartBeforeSend: true }, + }, + { + reason: 'fetch_failed', + match: { code: 'COMMAND_FAILED', messageIncludesAll: ['fetch failed'] }, + verdicts: { retryable: true }, + }, + { + reason: 'econnrefused', + match: { code: 'COMMAND_FAILED', messageIncludesAll: ['econnrefused'] }, + verdicts: { retryable: true }, + }, + { + reason: 'socket_hang_up', + match: { code: 'COMMAND_FAILED', messageIncludesAll: ['socket hang up'] }, + verdicts: { retryable: true }, + }, + { + reason: 'ax_snapshot_failure', + match: { code: 'IOS_AX_SNAPSHOT_FAILED' }, + verdicts: { sessionFatalReason: 'ax_snapshot_failure' }, + }, + { + reason: 'xctest_recorded_failure', + match: { code: 'XCTEST_RECORDED_FAILURE' }, + verdicts: { sessionFatalReason: 'xctest_recorded_failure' }, + }, + { + // The runner reported its main thread stuck in abandoned work past the wedge + // threshold (#1105): only a restart cures it. The per-request recycle budget + // still bounds how many boots one request pays for. + reason: 'runner_main_thread_wedged', + match: { code: 'RUNNER_WEDGED' }, + verdicts: { sessionFatalReason: 'runner_main_thread_wedged' }, + }, +]; + +function matchesRunnerErrorRule(error: AppError, match: RunnerErrorMatch): boolean { + if (match.code !== undefined && error.code !== match.code) return false; + if (!matchesRunnerErrorDetails(error, match.details)) return false; + return matchesRunnerErrorMessage(error, match.messageIncludesAll); +} + +function matchesRunnerErrorDetails(error: AppError, details: RunnerErrorMatch['details']): boolean { + if (details === undefined) return true; + if (details === 'retriable') return error.details?.retriable === true; + return isUsbmuxDeviceUnattachedError(error); +} + +function matchesRunnerErrorMessage(error: AppError, parts: readonly string[] | undefined): boolean { + if (!parts) return true; + const message = `${error.message ?? ''}`.toLowerCase(); + return parts.every((part) => message.includes(part)); +} + +function runnerErrorVerdict( + error: unknown, + axis: Axis, +): RunnerErrorVerdicts[Axis] | undefined { + if (!(error instanceof AppError)) return undefined; + for (const rule of RUNNER_ERROR_RULES) { + if (rule.verdicts[axis] === undefined) continue; + if (matchesRunnerErrorRule(error, rule.match)) return rule.verdicts[axis]; + } + return undefined; +} + export function isRetryableRunnerError(err: unknown): boolean { if (!(err instanceof AppError)) return false; if (err.code !== 'COMMAND_FAILED') return false; - if (err.details?.retriable === true) return true; - const message = `${err.message ?? ''}`.toLowerCase(); - if (message.includes('xcodebuild exited early')) return false; - if (message.includes('device is busy') && message.includes('connecting')) return false; - if (message.includes('runner did not accept connection')) return true; - if (message.includes('fetch failed')) return true; - if (message.includes('econnrefused')) return true; - if (message.includes('socket hang up')) return true; - return false; + return runnerErrorVerdict(err, 'retryable') ?? false; } /** @@ -161,14 +281,33 @@ export function isUsbmuxDeviceUnattachedError(error: unknown): boolean { } export function shouldRetryRunnerConnectError(error: unknown): boolean { - // Retrying cannot attach a cable, and the typed verdict carries the recovery - // hint that a generic connect failure would replace. - if (isUsbmuxDeviceUnattachedError(error)) return false; - if (!(error instanceof AppError)) return true; - if (error.code !== 'COMMAND_FAILED') return true; - const message = String(error.message ?? '').toLowerCase(); - if (message.includes('xcodebuild exited early')) return false; - return true; + return runnerErrorVerdict(error, 'connectRetry') ?? true; +} + +/** + * Session-fatal classification for a runner response error: when defined, the + * cached runner session must be invalidated with this reason instead of being + * reused (see ADR 0005 and the Selector Capture Reliability Contract's + * runnerFatal rule). + */ +export function resolveRunnerFatalErrorReason(error: unknown): string | undefined { + return runnerErrorVerdict(error, 'sessionFatalReason'); +} + +/** + * A connect-shaped failure that surfaced before the command was sent: restart + * the runner session and replay the command, rather than probing a runner + * that never accepted the connection. Composed with the connect-retry axis so + * a terminal connect verdict (cable unattached, xcodebuild exited early) + * still refuses the restart. Matching is table-driven and therefore + * case-insensitive, unlike the raw-message check it replaced; the message is + * our own transport literal, so no real error changes class. + */ +export function shouldRestartRunnerBeforeCommandSend(error: unknown): boolean { + return ( + (runnerErrorVerdict(error, 'restartBeforeSend') ?? false) && + shouldRetryRunnerConnectError(error) + ); } export function resolveRunnerEarlyExitHint( diff --git a/src/platforms/apple/core/runner/runner-lifecycle.ts b/src/platforms/apple/core/runner/runner-lifecycle.ts index 97df1c0d80..bf4fbf3af8 100644 --- a/src/platforms/apple/core/runner/runner-lifecycle.ts +++ b/src/platforms/apple/core/runner/runner-lifecycle.ts @@ -18,6 +18,7 @@ import { shouldRetryRunnerConnectError, withRunnerCommandId, type RunnerCommand, + shouldRestartRunnerBeforeCommandSend, } from './runner-contract.ts'; import type { AppleRunnerCommandOptions, @@ -293,13 +294,7 @@ export async function executeRunnerCommand( await invalidateRunnerSessionBestEffort(session, 'runner_startup_request_canceled'); throw err; } - if ( - appErr.code === 'COMMAND_FAILED' && - typeof appErr.message === 'string' && - appErr.message.includes('Runner did not accept connection') && - shouldRetryRunnerConnectError(appErr) && - session - ) { + if (shouldRestartRunnerBeforeCommandSend(appErr) && session) { assertRunnerRequestActive(options.requestId); return await restartSessionAndRunCommand({ device, diff --git a/src/platforms/apple/core/runner/runner-session.ts b/src/platforms/apple/core/runner/runner-session.ts index b3dde74277..d35485a913 100644 --- a/src/platforms/apple/core/runner/runner-session.ts +++ b/src/platforms/apple/core/runner/runner-session.ts @@ -36,6 +36,7 @@ import { resolveRunnerRequestSignal, withRunnerCommandId, type RunnerCommand, + resolveRunnerFatalErrorReason, } from './runner-contract.ts'; import { canSkipRunnerReadinessPreflightAfterHealthyMutation, @@ -938,17 +939,6 @@ function resolveRunnerFatalReason(data: Record): string | undef : 'runner_reported_fatal_response'; } -function resolveRunnerFatalErrorReason(error: unknown): string | undefined { - if (!(error instanceof AppError)) return undefined; - if (error.code === 'IOS_AX_SNAPSHOT_FAILED') return 'ax_snapshot_failure'; - if (error.code === 'XCTEST_RECORDED_FAILURE') return 'xctest_recorded_failure'; - // The runner reported its main thread stuck in abandoned work past the wedge threshold - // (#1105): only a restart cures it. The per-request recycle budget still bounds how many - // boots one request pays for. - if (error.code === 'RUNNER_WEDGED') return 'runner_main_thread_wedged'; - return undefined; -} - function resolveRunnerReadinessPreflightDecision( session: RunnerSession, command: RunnerCommand, From 151be493db8103176f65db4d629fa8a4a50ab42e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 6 Aug 2026 17:37:34 +0200 Subject: [PATCH 2/3] test(ios): prove the recovery wiring and the recycled-pid guarantee (#1644 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 was right, and I verified it before fixing: disabling the shipped recovery callsite in runner-lifecycle.ts left all seven recovery tests green. They call handleRunnerTransportErrorAfterCommandSend directly, so they prove the module, not the wiring. runner-recovery-wiring.test.ts enters at runAppleRunnerCommand — the production facade and provider seam — and fakes only session creation (the xcodebuild spawn). Real: command-id assignment, provider resolution, executeRunnerCommand's catch/classification, the recovery callsite, the recovery module, executeRunnerCommandWithSession, the transport fetch, and response parsing. Disabling the callsite now turns all three red. Entering one layer lower silently defeats it: the id is assigned by the facade and recovery declines to probe a command without one. P2: runner-lease-recycled-pid.test.ts covers #1621 with a REAL spawned process and real readProcessStartTime — no mock choreography. It asserts through the injected cleanup adapter rather than by observing a kill, so a regression reports a pid instead of signalling a live process; removing the identity guard turns it red. Only the refusal direction, which is contention-proof: the recorded start time never matches, so a timed-out verification read yields the same verdict. The opposite direction needs a second live `ps` read inside production code and flakes under load, so it stays with the mocked cases in runner-session.test.ts where pinning is the point. The fake runner now scripts per command rather than as one queue, because production sends a readiness `uptime` probe before a mutating command and `status` during recovery; a rigid queue coupled every test to that order. --- .../core/__tests__/fake-runner-server.ts | 21 ++- .../runner-lease-recycled-pid.test.ts | 138 ++++++++++++++++++ .../__tests__/runner-recovery-wiring.test.ts | 126 ++++++++++++++++ 3 files changed, 282 insertions(+), 3 deletions(-) create mode 100644 src/platforms/apple/core/__tests__/runner-lease-recycled-pid.test.ts create mode 100644 src/platforms/apple/core/__tests__/runner-recovery-wiring.test.ts diff --git a/src/platforms/apple/core/__tests__/fake-runner-server.ts b/src/platforms/apple/core/__tests__/fake-runner-server.ts index be32af219b..b96acf5634 100644 --- a/src/platforms/apple/core/__tests__/fake-runner-server.ts +++ b/src/platforms/apple/core/__tests__/fake-runner-server.ts @@ -26,10 +26,23 @@ export type FakeRunnerServer = { close: () => Promise; }; +/** + * Per-command scripts. Production sends a readiness `uptime` probe before a + * mutating command, and recovery sends `status` afterwards, so a rigid + * one-queue script couples every test to that ordering; keying by command + * lets a test say only what it cares about. Each command's list is consumed + * in order, and a command with no script left answers `ok` with no data. + */ +export type FakeRunnerCommandScript = Record; + export async function startFakeRunnerServer( - script: FakeRunnerResponse[], + script: FakeRunnerResponse[] | FakeRunnerCommandScript, ): Promise { - const remaining = [...script]; + const sequential = Array.isArray(script) ? [...script] : undefined; + const byCommand = Array.isArray(script) + ? undefined + : Object.fromEntries(Object.entries(script).map(([key, list]) => [key, [...list]])); + const remaining = sequential ?? []; const requests: FakeRunnerRequest[] = []; const server = http.createServer((req, res) => { let raw = ''; @@ -39,7 +52,9 @@ export async function startFakeRunnerServer( req.on('end', () => { const body = parseBody(raw); requests.push({ command: String(body.command ?? ''), body }); - const next = remaining.shift(); + const next = byCommand + ? (byCommand[String(body.command ?? '')]?.shift() ?? { kind: 'ok' as const, data: {} }) + : remaining.shift(); if (!next) { res.statusCode = 500; res.end(JSON.stringify({ ok: false, error: { message: 'fake runner script exhausted' } })); diff --git a/src/platforms/apple/core/__tests__/runner-lease-recycled-pid.test.ts b/src/platforms/apple/core/__tests__/runner-lease-recycled-pid.test.ts new file mode 100644 index 0000000000..daf7f236e0 --- /dev/null +++ b/src/platforms/apple/core/__tests__/runner-lease-recycled-pid.test.ts @@ -0,0 +1,138 @@ +import assert from 'node:assert/strict'; +import { spawn, type ChildProcess } from 'node:child_process'; +import { afterEach, test } from 'vitest'; +import { mkdtempForTestSync } from '../../../../__tests__/test-utils/tmp-dir.ts'; +import { readProcessStartTime } from '../../../../utils/host-process.ts'; +import { + RUNNER_OWNER_TOKEN, + cleanupOwnedRunnerLease, + writeRunnerLease, + type RunnerLease, + type RunnerLeaseCleanupAdapter, +} from '../runner/runner-lease.ts'; + +/** + * #1621 (recycled runner pid) as a real-process scenario rather than + * `readProcessStartTime` mock choreography, per #1631's acceptance criteria. + * + * A lease file can outlive its runner by days, and the OS recycles pids — so + * a stale lease's `runnerPid` may now belong to somebody else's process. + * Here that "somebody else" is a REAL child process we spawn and own: its pid + * and start time come from the OS, and `readProcessStartTime` runs for real. + * The lease then claims that pid with a start time that does not match, which + * is exactly the recycled shape. Cleanup must refuse to signal it. + * + * Asserting through the injected cleanup adapter (a production seam, not a + * test-only one) rather than by observing a kill keeps the test safe: a + * regression reports a pid here instead of signalling a live process. + * + * The lease is written under this process's own owner token, so cleanup + * classifies it as `owned` without reading the OWNER's start time — the one + * `ps` read this test needs is the child's, which is its actual subject. + * Depending on owner-identity reads is what makes such tests flake under + * full-suite contention (see #1642). + * + * Deliberately only the refusal direction. It is contention-PROOF: the + * recorded start time never matches, so whether the verification read + * succeeds or times out under load, the verdict is the same refusal. The + * opposite direction ("identity matches, so signal") cannot be made robust + * with a real process — it needs a second live `ps` read inside production + * code, and a timed-out read there flips the result. That direction is + * covered by the mocked start-time cases in `runner-session.test.ts`, where + * pinning the read is the point rather than a workaround. + */ + +let child: ChildProcess | undefined; + +afterEach(() => { + child?.kill('SIGKILL'); + child = undefined; +}); + +function recordingCleanup(): { + adapter: RunnerLeaseCleanupAdapter; + signalledPids: (number | undefined)[]; +} { + const signalledPids: (number | undefined)[] = []; + return { + signalledPids, + adapter: { + cleanupRunnerProcessTree: async (pid) => { + signalledPids.push(pid); + }, + cleanupRunnerXcodebuildProcesses: async () => {}, + cleanupTempFile: () => {}, + }, + }; +} + +async function spawnRealChild(): Promise<{ pid: number; startTime: string | null }> { + child = spawn('sleep', ['30'], { stdio: 'ignore' }); + const pid = child.pid; + assert.ok(pid, 'the OS gave the child a pid'); + // `readProcessStartTime` shells out to `ps`, which under full-suite CPU + // contention can miss its own timeout and return null. The budget is + // generous because it is only ever paid when contended — a quiet machine + // answers on the first read. + const deadline = Date.now() + 10_000; + while (Date.now() < deadline) { + const startTime = readProcessStartTime(pid); + if (startTime) return { pid, startTime }; + await new Promise((resolve) => setTimeout(resolve, 50)); + } + throw new Error('could not read the real child start time within 10s'); +} + +function leaseFor(params: { + deviceId: string; + runnerPid: number; + runnerStartTime: string | null; + stateDir: string; +}): RunnerLease { + return { + schemaVersion: 1, + deviceId: params.deviceId, + ownerToken: RUNNER_OWNER_TOKEN, + ownerPid: process.pid, + ownerStartTime: null, + ownerStateDir: params.stateDir, + sessionId: `session-${params.deviceId}`, + runnerPid: params.runnerPid, + runnerStartTime: params.runnerStartTime, + port: 8100, + xctestrunPath: `${params.stateDir}/runner.xctestrun`, + jsonPath: `${params.stateDir}/runner.json`, + createdAtMs: Date.now(), + }; +} + +test('a lease whose runner pid was recycled is never signalled', async () => { + const stateDir = mkdtempForTestSync('agent-device-lease-recycled-'); + const real = await spawnRealChild(); + // Same pid, different start time: the process at this pid is NOT the runner + // the lease recorded. This is the recycled case, produced without touching + // a single process API. + const deviceId = `recycled-${real.pid}`; + writeRunnerLease( + leaseFor({ + deviceId, + runnerPid: real.pid, + runnerStartTime: '1970-01-01T00:00:00Z', + stateDir, + }), + ); + const cleanup = recordingCleanup(); + + await cleanupOwnedRunnerLease(deviceId, cleanup.adapter); + + assert.deepEqual( + cleanup.signalledPids, + [undefined, undefined], + 'both the SIGTERM and SIGKILL passes refuse the unverified pid', + ); + assert.equal(child?.killed, false, 'the real process was left alone'); + // Guards the assertion above against passing for the wrong reason: cleanup + // must actually have run (a lease that classified as anything but `owned` + // would also produce no signals). + assert.equal(cleanup.signalledPids.length, 2, 'cleanup ran both signal passes'); +}); diff --git a/src/platforms/apple/core/__tests__/runner-recovery-wiring.test.ts b/src/platforms/apple/core/__tests__/runner-recovery-wiring.test.ts new file mode 100644 index 0000000000..45f41fccaa --- /dev/null +++ b/src/platforms/apple/core/__tests__/runner-recovery-wiring.test.ts @@ -0,0 +1,126 @@ +import assert from 'node:assert/strict'; +import { afterEach, expect, test, vi } from 'vitest'; +import { AppError } from '@agent-device/kernel/errors'; +import { IOS_SIMULATOR } from '../../../../__tests__/test-utils/device-fixtures.ts'; +import type { ExecResult } from '../../../../utils/exec.ts'; +import type { RunnerSession } from '../runner/runner-session.ts'; +import { startFakeRunnerServer, type FakeRunnerServer } from './fake-runner-server.ts'; + +/** + * The wiring regression the recovery suite cannot provide (#1644 review P1): + * that suite calls `handleRunnerTransportErrorAfterCommandSend` directly, so + * deleting the shipped callsite in `runner-lifecycle.ts` leaves it green. + * + * This enters at `runAppleRunnerCommand` — the production facade and the + * AppleRunnerProvider seam — and fakes only session CREATION, the xcodebuild + * spawn no unit test can perform. Real: command-id assignment, provider + * resolution, `executeRunnerCommand`'s catch/classification, the recovery + * callsite, the whole recovery module, `executeRunnerCommandWithSession`, + * the transport fetch, and response parsing. Removing the + * `isRetryableRunnerError` branch that routes into recovery turns this red + * (verified by doing exactly that). + * + * Entering one layer lower (`executeRunnerCommand`) silently defeats the + * test: the command id is assigned by the facade, and recovery declines to + * probe a command without one. + */ + +let server: FakeRunnerServer | undefined; + +const ensureRunnerSessionMock = vi.hoisted(() => vi.fn()); + +vi.mock('../runner/runner-session.ts', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + // Session creation only. executeRunnerCommandWithSession stays REAL, so + // the send → classify → recover path under test is production code. + ensureRunnerSession: ensureRunnerSessionMock, + }; +}); + +const { runAppleRunnerCommand } = await import('../runner/runner-client.ts'); + +afterEach(async () => { + await server?.close(); + server = undefined; + ensureRunnerSessionMock.mockReset(); +}); + +function seedSession(port: number): RunnerSession { + const session: RunnerSession = { + sessionId: `wiring:${port}`, + device: IOS_SIMULATOR, + deviceId: IOS_SIMULATOR.id, + port, + xctestrunPath: '/tmp/wiring.xctestrun', + jsonPath: '/tmp/wiring.json', + testPromise: new Promise(() => {}), + child: { pid: process.pid, exitCode: null }, + ready: true, + }; + ensureRunnerSessionMock.mockResolvedValue(session); + return session; +} + +test('a lost transport response is recovered through the production command path', async () => { + // 1st request: the tap, answered by dropping the connection mid-response + // (the real "lost response" shape). 2nd: the status probe recovery issues. + server = await startFakeRunnerServer({ + // The tap's response is dropped mid-flight (the real "lost response" + // shape); the status probe recovery issues then returns the retained one. + tap: [{ kind: 'hangUp' }], + status: [ + { + kind: 'ok', + data: { + lifecycleState: 'completed', + lifecycleResponseJson: JSON.stringify({ ok: true, data: { recovered: true } }), + }, + }, + ], + }); + seedSession(server.port); + + const result = await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }); + + // The recovered payload proves the whole chain ran: classification said + // retryable, the callsite invoked recovery, recovery probed status, and the + // retained response replaced the lost one. + assert.deepEqual(result, { recovered: true }); + const tap = server.requests.find((request) => request.command === 'tap'); + const status = server.requests.find((request) => request.command === 'status'); + assert.ok(tap, 'the tap reached the runner'); + assert.ok(status, 'recovery probed status'); + // The probe must reference the exact command id the send assigned. + assert.equal(typeof tap.body.commandId, 'string'); + assert.equal(status.body.statusCommandId, tap.body.commandId); +}); + +test('a runner that reports the command failed surfaces that failure, not the transport error', async () => { + server = await startFakeRunnerServer({ + tap: [{ kind: 'hangUp' }], + status: [ + { kind: 'ok', data: { lifecycleState: 'failed', lifecycleErrorMessage: 'tap missed' } }, + ], + }); + seedSession(server.port); + + await expect( + runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }), + ).rejects.toThrow(AppError); + assert.ok(server.requests.some((request) => request.command === 'status')); +}); + +test('an unrecoverable lifecycle state still reaches recovery and reports the invalidation', async () => { + server = await startFakeRunnerServer({ + tap: [{ kind: 'hangUp' }], + status: [{ kind: 'ok', data: { lifecycleState: 'zombie' } }], + }); + seedSession(server.port); + + await expect( + runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }), + ).rejects.toThrow(/invalidated the runner session/); + assert.ok(server.requests.some((request) => request.command === 'status')); +}); From 47efaf1e12eebb01daad10b64345a53cde368eaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 6 Aug 2026 18:00:51 +0200 Subject: [PATCH 3/3] test(ios): route the recycled-pid test through exec.ts and off the real lease tree (#1644 review) Two fixes to the same test: - Process execution goes through utils/exec.ts's runCmdBackground, per AGENTS.md's hard rule, instead of node:child_process spawn. The kill-induced `wait` rejection is owned deliberately. - The lease root defaults to the developer's REAL ~/.agent-device tree, so the test was writing leases there. It now redirects AGENT_DEVICE_IOS_RUNNER_LEASE_DIR to a temp dir per test and restores it, writing nothing outside the sandbox. (Verified by inspecting and cleaning the leases the earlier revision left behind.) --- .../runner-lease-recycled-pid.test.ts | 32 +++++++++++++++---- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/src/platforms/apple/core/__tests__/runner-lease-recycled-pid.test.ts b/src/platforms/apple/core/__tests__/runner-lease-recycled-pid.test.ts index daf7f236e0..4301af57c9 100644 --- a/src/platforms/apple/core/__tests__/runner-lease-recycled-pid.test.ts +++ b/src/platforms/apple/core/__tests__/runner-lease-recycled-pid.test.ts @@ -1,7 +1,7 @@ import assert from 'node:assert/strict'; -import { spawn, type ChildProcess } from 'node:child_process'; -import { afterEach, test } from 'vitest'; +import { afterEach, beforeEach, test } from 'vitest'; import { mkdtempForTestSync } from '../../../../__tests__/test-utils/tmp-dir.ts'; +import { runCmdBackground, type ExecBackgroundResult } from '../../../../utils/exec.ts'; import { readProcessStartTime } from '../../../../utils/host-process.ts'; import { RUNNER_OWNER_TOKEN, @@ -42,11 +42,26 @@ import { * pinning the read is the point rather than a workaround. */ -let child: ChildProcess | undefined; +// Process execution goes through utils/exec.ts, never node:child_process +// directly (AGENTS.md hard rule) — including in tests. +let child: ExecBackgroundResult | undefined; +let previousLeaseDir: string | undefined; + +beforeEach(() => { + // Leases default to the REAL ~/.agent-device tree; redirect them so the + // suite never writes into the developer's own runner state. + previousLeaseDir = process.env.AGENT_DEVICE_IOS_RUNNER_LEASE_DIR; + process.env.AGENT_DEVICE_IOS_RUNNER_LEASE_DIR = mkdtempForTestSync('agent-device-lease-root-'); +}); afterEach(() => { - child?.kill('SIGKILL'); + child?.child.kill('SIGKILL'); child = undefined; + if (previousLeaseDir === undefined) { + delete process.env.AGENT_DEVICE_IOS_RUNNER_LEASE_DIR; + } else { + process.env.AGENT_DEVICE_IOS_RUNNER_LEASE_DIR = previousLeaseDir; + } }); function recordingCleanup(): { @@ -67,8 +82,11 @@ function recordingCleanup(): { } async function spawnRealChild(): Promise<{ pid: number; startTime: string | null }> { - child = spawn('sleep', ['30'], { stdio: 'ignore' }); - const pid = child.pid; + child = runCmdBackground('sleep', ['30'], { stdio: 'ignore', captureOutput: false }); + // The cleanup SIGKILL below makes `wait` reject; own that here so it is a + // deliberate no-op rather than an unhandled rejection. + child.wait.catch(() => {}); + const pid = child.child.pid; assert.ok(pid, 'the OS gave the child a pid'); // `readProcessStartTime` shells out to `ps`, which under full-suite CPU // contention can miss its own timeout and return null. The budget is @@ -130,7 +148,7 @@ test('a lease whose runner pid was recycled is never signalled', async () => { [undefined, undefined], 'both the SIGTERM and SIGKILL passes refuse the unverified pid', ); - assert.equal(child?.killed, false, 'the real process was left alone'); + assert.equal(child?.child.killed, false, 'the real process was left alone'); // Guards the assertion above against passing for the wrong reason: cleanup // must actually have run (a lease that classified as anything but `owned` // would also produce no signals).