diff --git a/CHANGELOG.md b/CHANGELOG.md index 961cc8406a..1430c33046 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,21 @@ BrowserStack runs for the session, as `bstack:options.appiumVersion`. Unset, BrowserStack falls back to Appium 1.x, which predates the `mobile:` commands the interactor issues (`deepLink`, `pressButton`, `activateApp`). +- Changed (iOS runner): what recovers a stuck runner is decided by the recorded error, not by its + wording. A readiness preflight marks the error it gives up with, and that marker is now the whole + test for restarting the session and replaying the command — except for a request that was canceled, + which that same catch also marks: a command nobody is going to send again has no restart to spend, + and the session it would tear down may be one that still works. Two message checks decided it before, + and a preflight reaches the caller in whatever shape its connect loop ended with — "Runner did not + accept connection", "Runner endpoint probe failed", a killed `simctl` fallback, a post that ran out + of its budget — so only some of those restarted and the rest failed the command. The other half is + what no longer happens: when a slow + boot spends the whole prepare budget, the health check reports "prepare ios-runner timed out", and + that no longer wipes a restored `xcodebuild` artifact on the way to a rebuild — the runner session + is invalidated and prepare retries with the artifact intact. Only a failure that indicts the + artifact rebuilds it, so a runner that refuses the connection or never answers on any route still + wipes it and rebuilds, which is what that rule is for. + - Changed (sessions): the implicit session is now keyed by workspace **and platform**, so one checkout can drive iOS and Android without inventing a `--session` name for every command (#2580). An implicit session was addressed by `cwd::default`, one slot per checkout, and it stayed diff --git a/packages/kernel/src/errors.ts b/packages/kernel/src/errors.ts index 95a1c70ece..b482a210dd 100644 --- a/packages/kernel/src/errors.ts +++ b/packages/kernel/src/errors.ts @@ -262,10 +262,19 @@ export function createRequestCanceledError(details?: AppErrorDetails, cause?: un ); } +/** + * The typed reason of a canceled request, for a caller holding the details rather than the error: + * a rule table that matches on details needs the same fact {@link isRequestCanceledError} reads, and + * must not restate the literal. + */ +export function isRequestCanceledDetails(details: AppErrorDetails | undefined): boolean { + return details?.reason === REQUEST_CANCELED_REASON; +} + export function isRequestCanceledError(error: unknown): boolean { if (!(error instanceof AppError)) return false; if (error.code !== 'COMMAND_FAILED') return false; - if (error.details?.reason === REQUEST_CANCELED_REASON) return true; + if (isRequestCanceledDetails(error.details)) return true; // Owned debt: canceled errors that crossed a wire without their details keep // the message; do not add new message sniffs beside it. return error.message === REQUEST_CANCELED_MESSAGE; diff --git a/packages/platform-apple/src/runner/__tests__/runner-command-retry.test.ts b/packages/platform-apple/src/runner/__tests__/runner-command-retry.test.ts index 0164168eaf..1713908ba2 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-command-retry.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-command-retry.test.ts @@ -342,56 +342,6 @@ test('mutating commands retry startup sessions with stale bundle cleanup', async assert.equal(mockExecuteRunnerCommandWithSession.mock.calls[1]?.[1], freshSession); }); -test('mutating commands restart stale sessions when readiness preflight fails before command send', async () => { - const staleSession = makeRunnerSession({ port: 8100, ready: true }); - const freshSession = makeRunnerSession({ port: 8101, ready: false }); - - mockEnsureRunnerSession.mockResolvedValueOnce(staleSession).mockResolvedValueOnce(freshSession); - mockExecuteRunnerCommandWithSession - .mockRejectedValueOnce( - new AppError('COMMAND_FAILED', 'fetch failed', { - runnerReadinessPreflightFailed: true, - }), - ) - .mockResolvedValueOnce({ message: 'tapped' }); - - const result = await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 120, y: 240 }); - - assert.deepEqual(result, { message: 'tapped' }); - assert.equal(mockEnsureRunnerSession.mock.calls.length, 2); - assert.deepEqual(mockInvalidateRunnerSession.mock.calls[0], [ - staleSession, - 'runner_readiness_preflight_failed_before_command_send', - ]); - assert.equal(mockExecuteRunnerCommandWithSession.mock.calls.length, 2); - assert.equal(mockExecuteRunnerCommandWithSession.mock.calls[1]?.[1], freshSession); -}); - -test('mutating commands restart stale sessions when readiness preflight times out before command send', async () => { - const staleSession = makeRunnerSession({ port: 8100, ready: true }); - const freshSession = makeRunnerSession({ port: 8101, ready: false }); - - mockEnsureRunnerSession.mockResolvedValueOnce(staleSession).mockResolvedValueOnce(freshSession); - mockExecuteRunnerCommandWithSession - .mockRejectedValueOnce( - new AppError('COMMAND_FAILED', 'Runner readiness timed out', { - runnerReadinessPreflightFailed: true, - }), - ) - .mockResolvedValueOnce({ message: 'tapped' }); - - const result = await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 120, y: 240 }); - - assert.deepEqual(result, { message: 'tapped' }); - assert.equal(mockEnsureRunnerSession.mock.calls.length, 2); - assert.deepEqual(mockInvalidateRunnerSession.mock.calls[0], [ - staleSession, - 'runner_readiness_preflight_failed_before_command_send', - ]); - assert.equal(mockExecuteRunnerCommandWithSession.mock.calls.length, 2); - assert.equal(mockExecuteRunnerCommandWithSession.mock.calls[1]?.[1], freshSession); -}); - test('mutating commands emit readiness recovery diagnostics after failed preflight restart succeeds', async () => { const staleSession = makeRunnerSession({ port: 8100, ready: true }); const freshSession = makeRunnerSession({ port: 8101, ready: false }); 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 20772cd044..eb8590480d 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 @@ -1,10 +1,12 @@ import assert from 'node:assert/strict'; import { test } from 'vitest'; -import { AppError } from '@agent-device/kernel/errors'; +import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; import { RUNNER_ERROR_RULES, isRetryableRunnerError, resolveRunnerFatalErrorReason, + shouldRebuildCachedRunnerArtifact, + shouldRestartRunnerAfterReadinessPreflight, shouldRestartRunnerBeforeCommandSend, shouldRetryRunnerConnectError, } from '../runner-contract.ts'; @@ -32,7 +34,12 @@ test('transport-shaped failures are retryable', () => { }); test('boot-shaped failures are not retryable', () => { - assert.equal(isRetryableRunnerError(commandFailed('xcodebuild exited early (code 65)')), false); + assert.equal( + isRetryableRunnerError( + commandFailed('Runner did not accept connection (xcodebuild exited early)'), + ), + false, + ); assert.equal( isRetryableRunnerError(commandFailed('Device is busy (Connecting to Simulator)')), false, @@ -70,6 +77,99 @@ test('connect loop stops for terminal verdicts', () => { assert.equal(shouldRetryRunnerConnectError(new AppError('DEVICE_NOT_FOUND', 'gone')), true); }); +// --- readiness preflight --- + +test('the preflight marker alone decides the restart', () => { + // The marker is applied by the preflight's own catch, whatever it was waiting on when it gave + // up: a killed fallback, an exhausted probe, a refusal. Which of those arrived is not evidence + // about whether the command reached the runner, and the marker is. + const killedSpawn = commandFailed('xcrun timed out after 45000ms', { + cmd: 'xcrun', + timeoutMs: 45_000, + runnerReadinessPreflightFailed: true, + }); + assert.equal(shouldRestartRunnerAfterReadinessPreflight(killedSpawn), true); + assert.equal( + shouldRestartRunnerAfterReadinessPreflight( + commandFailed('Runner readiness refused', { runnerReadinessPreflightFailed: true }), + ), + true, + ); + // The restart the marker authorises is a new session, not more waiting inside this one. + assert.equal(shouldRetryRunnerConnectError(killedSpawn), true); + // Without the marker the same two shapes say nothing about the command having been written. + assert.equal( + shouldRestartRunnerAfterReadinessPreflight( + commandFailed('xcrun timed out after 45000ms', { cmd: 'xcrun', timeoutMs: 45_000 }), + ), + false, + ); + assert.equal( + shouldRestartRunnerAfterReadinessPreflight(commandFailed('Runner readiness refused')), + false, + ); + // The same catch marks a caller that stopped waiting. That mark is not a runner that stopped + // answering: the command was canceled, so no restart has a request left to serve. + assert.equal( + shouldRestartRunnerAfterReadinessPreflight( + createRequestCanceledError({ runnerReadinessPreflightFailed: true }), + ), + false, + ); +}); + +test('a deadline on its own earns no recovery verdict', () => { + // The same recorded budget covers a wait inside the connect loop, where waiting is + // right, and a fetch that died after the command was written, where replaying is not. + const deadline = commandFailed('Runner command deadline exceeded', { + port: 8100, + timeoutMs: 45_000, + }); + assert.equal(isRetryableRunnerError(deadline), false); + assert.equal(shouldRestartRunnerBeforeCommandSend(deadline), false); + assert.equal(shouldRestartRunnerAfterReadinessPreflight(deadline), false); + assert.equal(shouldRebuildCachedRunnerArtifact(deadline), false); + assert.equal(shouldRetryRunnerConnectError(deadline), true); +}); + +// --- restored-artifact axis (shouldRebuildCachedRunnerArtifact) --- + +test('only a runner that never accepted a connection indicts the cached artifact', () => { + assert.equal( + shouldRebuildCachedRunnerArtifact(commandFailed('Runner endpoint probe failed')), + true, + ); + assert.equal( + shouldRebuildCachedRunnerArtifact(commandFailed('Runner did not accept connection')), + true, + ); + assert.equal( + shouldRebuildCachedRunnerArtifact( + commandFailed('Runner did not accept connection (simctl spawn)', { port: 8100 }), + ), + true, + ); + // Wiping derived data cannot fix a boot that refuses to compile, and its message + // otherwise reads as a refused connection. + assert.equal( + shouldRebuildCachedRunnerArtifact( + commandFailed('Runner did not accept connection (xcodebuild exited early)', { + port: 8100, + logPath: '/tmp/runner.log', + }), + ), + false, + ); + assert.equal(shouldRebuildCachedRunnerArtifact(commandFailed('fetch failed')), false); +}); + +test('a device that is busy connecting is a terminal connect verdict', () => { + assert.equal( + shouldRetryRunnerConnectError(commandFailed('Device is busy (Connecting to Simulator)')), + false, + ); +}); + // --- session-fatal axis (resolveRunnerFatalErrorReason) --- test('session-fatal codes map to their invalidation reasons', () => { diff --git a/packages/platform-apple/src/runner/__tests__/runner-lifecycle-prepare-artifact.test.ts b/packages/platform-apple/src/runner/__tests__/runner-lifecycle-prepare-artifact.test.ts new file mode 100644 index 0000000000..3309284b0f --- /dev/null +++ b/packages/platform-apple/src/runner/__tests__/runner-lifecycle-prepare-artifact.test.ts @@ -0,0 +1,123 @@ +import assert from 'node:assert/strict'; +import { beforeEach, test, vi } from 'vitest'; +import { AppError } from '@agent-device/kernel/errors'; +import { appleRunnerTestHost } from '../test-host.ts'; +import { Deadline } from '../host.ts'; +import type { RunnerXctestrunArtifact } from '../runner-xctestrun.ts'; +import { IOS_SIMULATOR } from './device-fixtures.ts'; +import { createTestRequestCancellation, makeRunnerSession } from './runner-session-fixtures.ts'; + +const { + mockEnsureRunnerSession, + mockExecuteRunnerCommandWithSession, + mockEmitDiagnostic, + mockGetRunnerSessionSnapshot, + mockInvalidateRunnerSession, + mockMarkRunnerXctestrunArtifactBadForRun, +} = vi.hoisted(() => ({ + mockEnsureRunnerSession: vi.fn(), + mockExecuteRunnerCommandWithSession: vi.fn(), + mockEmitDiagnostic: vi.fn(), + mockGetRunnerSessionSnapshot: vi.fn(), + mockInvalidateRunnerSession: vi.fn(), + mockMarkRunnerXctestrunArtifactBadForRun: vi.fn(), +})); + +vi.mock('../runner-session.ts', async () => { + const actual = + await vi.importActual('../runner-session.ts'); + return { + ...actual, + ensureRunnerSession: mockEnsureRunnerSession, + executeRunnerCommandWithSession: mockExecuteRunnerCommandWithSession, + getRunnerSessionSnapshot: mockGetRunnerSessionSnapshot, + invalidateRunnerSession: mockInvalidateRunnerSession, + }; +}); + +vi.mock('../runner-xctestrun.ts', async () => { + const actual = + await vi.importActual('../runner-xctestrun.ts'); + return { + ...actual, + markRunnerXctestrunArtifactBadForRun: mockMarkRunnerXctestrunArtifactBadForRun, + }; +}); + +import { prepareIosRunner } from '../runner-client.ts'; +import { resetRunnerRecycleLedgerForTests } from '../runner-recycle-ledger.ts'; + +function makeRunnerArtifact( + overrides: Partial = {}, +): RunnerXctestrunArtifact { + return { + xctestrunPath: '/tmp/runner.xctestrun', + derived: '/tmp/derived', + cache: 'exact', + artifact: 'valid', + buildMs: 0, + xctestrunPathSource: 'manifest', + ...overrides, + }; +} + +const requestCancellation = createTestRequestCancellation(); +const { isRequestCanceled } = requestCancellation; + +beforeEach(() => { + vi.resetAllMocks(); + resetRunnerRecycleLedgerForTests(); + mockGetRunnerSessionSnapshot.mockReturnValue(null); + mockMarkRunnerXctestrunArtifactBadForRun.mockResolvedValue(undefined); + requestCancellation.reset(); + appleRunnerTestHost.update({ + emitDiagnostic: mockEmitDiagnostic, + isRequestCanceled, + getRequestSignal: () => undefined, + }); +}); + +// What a spent prepare deadline does to a restored artifact. Prepare spends one `Deadline` across +// boot and health check, so the failure this decision actually sees is the one +// `readPreparePhaseTimeoutMs` raises when the boot ate the budget: "prepare ios-runner timed out" +// with reason `prepare_deadline_expired`. That indicts the boot, not the derived data it was +// launched from, so the artifact stays and prepare retries with a fresh session. The artifact is +// only wiped by the rules that indict it, such as a runner that refused the connection. + +test('a prepare deadline spent during boot keeps the restored artifact and retries', async () => { + vi.useFakeTimers(); + try { + vi.setSystemTime(1_000); + const restoredSession = makeRunnerSession({ + port: 8100, + xctestrunPath: '/tmp/restored.xctestrun', + xctestrunArtifact: makeRunnerArtifact({ xctestrunPath: '/tmp/restored.xctestrun' }), + }); + const prepareDeadline = Deadline.fromTimeoutMs(45_000); + + mockEnsureRunnerSession.mockImplementation(async () => { + // The boot consumed the whole prepare budget, so no health phase time remains. + vi.setSystemTime(46_000); + return restoredSession; + }); + + await assert.rejects( + () => prepareIosRunner(IOS_SIMULATOR, { healthTimeoutMs: 90_000, prepareDeadline }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.message, 'prepare ios-runner timed out'); + assert.equal(error.details?.reason, 'prepare_deadline_expired'); + assert.equal(error.details?.phase, 'runner_session'); + return true; + }, + ); + + assert.equal(mockMarkRunnerXctestrunArtifactBadForRun.mock.calls.length, 0); + assert.deepEqual(mockInvalidateRunnerSession.mock.calls.at(-1), [ + restoredSession, + 'prepare_runner_health_retry', + ]); + } finally { + vi.useRealTimers(); + } +}); diff --git a/packages/platform-apple/src/runner/__tests__/runner-lifecycle-readiness-preflight.test.ts b/packages/platform-apple/src/runner/__tests__/runner-lifecycle-readiness-preflight.test.ts new file mode 100644 index 0000000000..c105197a6a --- /dev/null +++ b/packages/platform-apple/src/runner/__tests__/runner-lifecycle-readiness-preflight.test.ts @@ -0,0 +1,232 @@ +import assert from 'node:assert/strict'; +import { beforeEach, test, vi } from 'vitest'; +import { + AppError, + createRequestCanceledError, + isRequestCanceledError, +} from '@agent-device/kernel/errors'; +import { appleRunnerTestHost } from '../test-host.ts'; +import type { RunnerXctestrunArtifact } from '../runner-xctestrun.ts'; +import { IOS_SIMULATOR } from './device-fixtures.ts'; +import { createTestRequestCancellation, makeRunnerSession } from './runner-session-fixtures.ts'; + +const { + mockEnsureRunnerSession, + mockExecuteRunnerCommandWithSession, + mockEmitDiagnostic, + mockGetRunnerSessionSnapshot, + mockInvalidateRunnerSession, + mockMarkRunnerXctestrunArtifactBadForRun, +} = vi.hoisted(() => ({ + mockEnsureRunnerSession: vi.fn(), + mockExecuteRunnerCommandWithSession: vi.fn(), + mockEmitDiagnostic: vi.fn(), + mockGetRunnerSessionSnapshot: vi.fn(), + mockInvalidateRunnerSession: vi.fn(), + mockMarkRunnerXctestrunArtifactBadForRun: vi.fn(), +})); + +vi.mock('../runner-session.ts', async () => { + const actual = + await vi.importActual('../runner-session.ts'); + return { + ...actual, + ensureRunnerSession: mockEnsureRunnerSession, + executeRunnerCommandWithSession: mockExecuteRunnerCommandWithSession, + getRunnerSessionSnapshot: mockGetRunnerSessionSnapshot, + invalidateRunnerSession: mockInvalidateRunnerSession, + }; +}); + +vi.mock('../runner-xctestrun.ts', async () => { + const actual = + await vi.importActual('../runner-xctestrun.ts'); + return { + ...actual, + markRunnerXctestrunArtifactBadForRun: mockMarkRunnerXctestrunArtifactBadForRun, + }; +}); + +import { prepareIosRunner, runAppleRunnerCommand } from '../runner-client.ts'; +import { resetRunnerRecycleLedgerForTests } from '../runner-recycle-ledger.ts'; + +// What `executeRunnerCommand` decides when a readiness preflight refuses the runner +// before the command was ever written: the table's own verdict axes say whether the +// session restarts, whether the restored artifact is suspect, and whether the caller +// simply hears the refusal. + +const requestCancellation = createTestRequestCancellation(); +const { isRequestCanceled } = requestCancellation; + +beforeEach(() => { + vi.resetAllMocks(); + resetRunnerRecycleLedgerForTests(); + mockGetRunnerSessionSnapshot.mockReturnValue(null); + mockMarkRunnerXctestrunArtifactBadForRun.mockResolvedValue(undefined); + requestCancellation.reset(); + appleRunnerTestHost.update({ + emitDiagnostic: mockEmitDiagnostic, + isRequestCanceled, + getRequestSignal: () => undefined, + }); +}); + +function makeRunnerArtifact( + overrides: Partial = {}, +): RunnerXctestrunArtifact { + return { + xctestrunPath: '/tmp/runner.xctestrun', + derived: '/tmp/derived', + cache: 'exact', + artifact: 'valid', + buildMs: 0, + xctestrunPathSource: 'manifest', + ...overrides, + }; +} + +test('mutating commands restart stale sessions when readiness preflight fails before command send', async () => { + const staleSession = makeRunnerSession({ port: 8100, ready: true }); + const freshSession = makeRunnerSession({ port: 8101, ready: false }); + + mockEnsureRunnerSession.mockResolvedValueOnce(staleSession).mockResolvedValueOnce(freshSession); + mockExecuteRunnerCommandWithSession + .mockRejectedValueOnce( + new AppError('COMMAND_FAILED', 'fetch failed', { + runnerReadinessPreflightFailed: true, + }), + ) + .mockResolvedValueOnce({ message: 'tapped' }); + + const result = await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 120, y: 240 }); + + assert.deepEqual(result, { message: 'tapped' }); + assert.equal(mockEnsureRunnerSession.mock.calls.length, 2); + assert.deepEqual(mockInvalidateRunnerSession.mock.calls[0], [ + staleSession, + 'runner_readiness_preflight_failed_before_command_send', + ]); + assert.equal(mockExecuteRunnerCommandWithSession.mock.calls.length, 2); + assert.equal(mockExecuteRunnerCommandWithSession.mock.calls[1]?.[1], freshSession); +}); + +test('a readiness preflight that runs out a post deadline restarts the session and replays', async () => { + const staleSession = makeRunnerSession({ port: 8100, ready: true }); + const freshSession = makeRunnerSession({ port: 8101, ready: false }); + + // The simulator and usbmux routes post to the runner themselves, and `fetchWithTimeout` reports + // an expiry as "Runner command deadline exceeded" with the budget it ran out. Neither of the two + // message checks this rule replaced matched that wording, so the command was never replayed. + // What routes it is the marker the preflight catch puts on its way out, not the wording. + mockEnsureRunnerSession.mockResolvedValueOnce(staleSession).mockResolvedValueOnce(freshSession); + mockExecuteRunnerCommandWithSession + .mockRejectedValueOnce( + new AppError('COMMAND_FAILED', 'Runner command deadline exceeded', { + port: 8100, + timeoutMs: 45_000, + runnerReadinessPreflightFailed: true, + }), + ) + .mockResolvedValueOnce({ message: 'tapped' }); + + const result = await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 120, y: 240 }); + + assert.deepEqual(result, { message: 'tapped' }); + assert.equal(mockEnsureRunnerSession.mock.calls.length, 2); + assert.equal(mockExecuteRunnerCommandWithSession.mock.calls[1]?.[1], freshSession); +}); + +test('a readiness preflight refusal restarts the session like any other preflight failure', async () => { + const staleSession = makeRunnerSession({ port: 8100, ready: true }); + const freshSession = makeRunnerSession({ port: 8101, ready: false }); + + // This is the shape no message check could have been written for: the runner answered the probe + // and the answer was no, which says nothing about whether the command was written. The marker + // says that, and it says it for every shape at once. + mockEnsureRunnerSession.mockResolvedValueOnce(staleSession).mockResolvedValueOnce(freshSession); + mockExecuteRunnerCommandWithSession + .mockRejectedValueOnce( + new AppError('COMMAND_FAILED', 'Runner readiness refused', { + runnerReadinessPreflightFailed: true, + }), + ) + .mockResolvedValueOnce({ message: 'tapped' }); + + const result = await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 120, y: 240 }); + + assert.deepEqual(result, { message: 'tapped' }); + assert.deepEqual(mockInvalidateRunnerSession.mock.calls[0], [ + staleSession, + 'runner_readiness_preflight_failed_before_command_send', + ]); +}); + +test('a failed readiness probe without the marker does not restart the session', async () => { + const session = makeRunnerSession({ port: 8100, ready: true }); + + // Without the marker the failure is just a transport shape, and the one that says the command + // was never written is the reason this restart is safe at all. + mockEnsureRunnerSession.mockResolvedValueOnce(session); + mockExecuteRunnerCommandWithSession.mockRejectedValueOnce( + new AppError('COMMAND_FAILED', 'Runner readiness refused'), + ); + + await assert.rejects( + () => runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 120, y: 240 }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.message, 'Runner readiness refused'); + return true; + }, + ); + assert.equal(mockEnsureRunnerSession.mock.calls.length, 1); +}); + +test('a cancellation during the readiness preflight does not restart the session it canceled', async () => { + const session = makeRunnerSession({ port: 8100, ready: true }); + + // The preflight's catch marks whatever it was waiting on when it gave up, and one of the things + // it waits on is a caller that stopped waiting. That mark describes a walkaway, not a wedged + // runner, and a session that is ready is the one the caller just left: restarting it would boot a + // runner for a command nobody is going to send again, and take down a session that still works. + mockEnsureRunnerSession.mockResolvedValueOnce(session); + mockExecuteRunnerCommandWithSession.mockRejectedValueOnce( + createRequestCanceledError({ runnerReadinessPreflightFailed: true, command: 'tap' }), + ); + + await assert.rejects( + () => runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 120, y: 240 }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.ok(isRequestCanceledError(error)); + return true; + }, + ); + assert.equal(mockInvalidateRunnerSession.mock.calls.length, 0); + assert.equal(mockEnsureRunnerSession.mock.calls.length, 1); + assert.equal(mockExecuteRunnerCommandWithSession.mock.calls.length, 1); +}); + +test('a boot that exited early does not wipe a restored runner artifact', async () => { + const restoredSession = makeRunnerSession({ + port: 8100, + xctestrunPath: '/tmp/restored.xctestrun', + xctestrunArtifact: makeRunnerArtifact({ xctestrunPath: '/tmp/restored.xctestrun' }), + }); + + mockEnsureRunnerSession.mockResolvedValueOnce(restoredSession); + mockExecuteRunnerCommandWithSession.mockRejectedValueOnce( + new AppError('COMMAND_FAILED', 'Runner did not accept connection (xcodebuild exited early)'), + ); + + await assert.rejects( + () => prepareIosRunner(IOS_SIMULATOR, { healthTimeoutMs: 90_000 }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.message, 'Runner did not accept connection (xcodebuild exited early)'); + return true; + }, + ); + assert.equal(mockMarkRunnerXctestrunArtifactBadForRun.mock.calls.length, 0); + assert.equal(mockEnsureRunnerSession.mock.calls.length, 1); +}); diff --git a/packages/platform-apple/src/runner/__tests__/runner-transport.test.ts b/packages/platform-apple/src/runner/__tests__/runner-transport.test.ts index 71f0fc6712..edd7f7bc16 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-transport.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-transport.test.ts @@ -13,6 +13,7 @@ import { xctestIosDevice, } from './runner-transport.fixtures.ts'; import { appleRunnerTestHost } from '../test-host.ts'; +import { isCommandTimeoutError } from '../host.ts'; import type { IosPhysicalDeviceRunnerControl } from '../host.ts'; const { mockRunCmd, mockUsbmuxPostCommand } = vi.hoisted(() => ({ @@ -31,7 +32,7 @@ vi.mock('../runner-usbmux.ts', async (importOriginal) => { }); import { clearDeviceTunnelIpCache } from '../runner-command-route.ts'; -import { sendRunnerCommandOnce } from '../runner-transport.ts'; +import { fetchWithTimeout, sendRunnerCommandOnce } from '../runner-transport.ts'; // The real `resolveIosPhysicalDeviceControl` resolves the CoreDevice tunnel IP // through root-level tooling this package cannot reach; a fake control backed @@ -115,6 +116,28 @@ test('sendRunnerCommandOnce does not retry or simulator fallback after request f assert.equal(mockRunCmd.mock.calls.length, 0); }); +test('fetchWithTimeout reports its own deadline as a typed command timeout', async () => { + vi.stubGlobal( + 'fetch', + vi.fn((_url: string, init: RequestInit) => { + return new Promise((_resolve, reject) => { + init.signal?.addEventListener('abort', () => reject(init.signal?.reason)); + }); + }), + ); + + await assert.rejects( + () => fetchWithTimeout('http://127.0.0.1:8100/command', { method: 'POST' }, 5), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.code, 'COMMAND_FAILED'); + assert.equal(error.details?.timeoutMs, 5); + assert.equal(isCommandTimeoutError(error), true); + return true; + }, + ); +}); + test('sendRunnerCommandOnce routes xctest physical devices through usbmux', async () => { const fetchMock = vi.fn(); vi.stubGlobal('fetch', fetchMock); diff --git a/packages/platform-apple/src/runner/runner-contract.ts b/packages/platform-apple/src/runner/runner-contract.ts index 649ffce8f1..7b7501a140 100644 --- a/packages/platform-apple/src/runner/runner-contract.ts +++ b/packages/platform-apple/src/runner/runner-contract.ts @@ -1,4 +1,11 @@ -import { AppError, createRequestCanceledError, toAppErrorCode } from '@agent-device/kernel/errors'; +import { + AppError, + createRequestCanceledError, + isRequestCanceledDetails, + toAppErrorCode, + type AppErrorCode, + type AppErrorDetails, +} from '@agent-device/kernel/errors'; import crypto from 'node:crypto'; import { ALERT_NOT_FOUND_RUNNER_CODE } from '@agent-device/contracts/alert-contract'; import type { DeviceRotation } from '@agent-device/contracts/device'; @@ -143,15 +150,34 @@ export function resolveRunnerRequestSignal(options: { return AbortSignal.any([registeredSignal, options.signal]); } +/** + * 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. + */ +type RunnerErrorDetailsMatch = (details: AppErrorDetails) => boolean; + type RunnerErrorMatch = { /** Required `AppError.code`; absent = any AppError. */ - code?: string; + code?: AppErrorCode; /** Every entry must appear in the lowercased message. */ messageIncludesAll?: readonly string[]; /** Required details evidence beyond code/message. */ - details?: 'retriable' | 'usbmux-device-unattached'; + details?: RunnerErrorDetailsMatch; }; +const hasRetriableFlag: RunnerErrorDetailsMatch = (details) => details.retriable === true; +const hasUsbmuxDeviceUnattached: RunnerErrorDetailsMatch = (details) => + details.usbmuxDeviceAttached === false; +/** + * The preflight marks whatever it was waiting on when it stopped, and one of the things it waits on + * is a caller that stopped waiting. A canceled request is not a wedged runner: the restart this + * marker authorises would boot a runner for a command nobody is going to send again. Every abort in + * the connect loop normalizes to the typed canceled reason before it reaches here. + */ +const hasReadinessPreflightFailure: RunnerErrorDetailsMatch = (details) => + details.runnerReadinessPreflightFailed === true && !isRequestCanceledDetails(details); + type RunnerErrorVerdicts = { /** isRetryableRunnerError: transport error worth a same-session resend. */ retryable?: boolean; @@ -161,6 +187,10 @@ type RunnerErrorVerdicts = { sessionFatalReason?: string; /** Connect-shaped failure before the command was sent: restart the session and replay. */ restartBeforeSend?: boolean; + /** Readiness preflight gave up before the command was written: restart the session and replay. */ + restartAfterReadinessPreflight?: boolean; + /** The runner never accepted a connection, so the restored artifact itself is suspect. */ + artifactSuspect?: boolean; }; type RunnerErrorRule = { @@ -183,43 +213,68 @@ type RunnerErrorRule = { export const RUNNER_ERROR_RULES: readonly RunnerErrorRule[] = [ { reason: 'usbmux_device_unattached', - match: { code: 'DEVICE_NOT_FOUND', details: 'usbmux-device-unattached' }, + match: { code: 'DEVICE_NOT_FOUND', details: hasUsbmuxDeviceUnattached }, verdicts: { connectRetry: false }, }, { reason: 'flagged_retriable', - match: { code: 'COMMAND_FAILED', details: 'retriable' }, - verdicts: { retryable: true }, + match: { code: 'COMMAND_FAILED', details: hasRetriableFlag }, + verdicts: { retryable: true, connectRetry: true }, }, { + // Says `artifactSuspect: false` on purpose: its message also reads as a refused + // connection, and a boot that cannot compile is not cured by wiping derived data. reason: 'xcodebuild_exited_early', match: { code: 'COMMAND_FAILED', messageIncludesAll: ['xcodebuild exited early'] }, - verdicts: { retryable: false, connectRetry: false }, + verdicts: { retryable: false, connectRetry: false, artifactSuspect: false }, }, { + // A device still mid-attachment is not a runner we can talk to yet, and waiting on + // it inside this request is what the caller's own retry is for. reason: 'device_busy_connecting', match: { code: 'COMMAND_FAILED', messageIncludesAll: ['device is busy', 'connecting'] }, - verdicts: { retryable: false }, + verdicts: { retryable: false, connectRetry: false }, + }, + { + // The marker is the whole fact: the preflight gave up before the command was written, so + // replaying it cannot duplicate anything. It is asked for alone because the preflight fails in + // whatever shape the connect loop ended with — a refusal, an exhausted probe, a killed + // fallback — and a rule that also required a recorded budget would fire on only some of them. + reason: 'runner_readiness_preflight_failed', + match: { code: 'COMMAND_FAILED', details: hasReadinessPreflightFailure }, + verdicts: { restartAfterReadinessPreflight: true }, }, { reason: 'runner_connect_refused', match: { code: 'COMMAND_FAILED', messageIncludesAll: ['runner did not accept connection'] }, - verdicts: { retryable: true, restartBeforeSend: true }, + verdicts: { + retryable: true, + connectRetry: true, + restartBeforeSend: true, + artifactSuspect: true, + }, + }, + { + // Every endpoint answered and none of them had a runner: with a restored artifact + // in hand, that artifact is the common cause. + reason: 'runner_endpoint_probe_exhausted', + match: { code: 'COMMAND_FAILED', messageIncludesAll: ['runner endpoint probe failed'] }, + verdicts: { artifactSuspect: true }, }, { reason: 'fetch_failed', match: { code: 'COMMAND_FAILED', messageIncludesAll: ['fetch failed'] }, - verdicts: { retryable: true }, + verdicts: { retryable: true, connectRetry: true }, }, { reason: 'econnrefused', match: { code: 'COMMAND_FAILED', messageIncludesAll: ['econnrefused'] }, - verdicts: { retryable: true }, + verdicts: { retryable: true, connectRetry: true }, }, { reason: 'socket_hang_up', match: { code: 'COMMAND_FAILED', messageIncludesAll: ['socket hang up'] }, - verdicts: { retryable: true }, + verdicts: { retryable: true, connectRetry: true }, }, { reason: 'ax_snapshot_failure', @@ -249,8 +304,7 @@ function matchesRunnerErrorRule(error: AppError, match: RunnerErrorMatch): boole function matchesRunnerErrorDetails(error: AppError, details: RunnerErrorMatch['details']): boolean { if (details === undefined) return true; - if (details === 'retriable') return error.details?.retriable === true; - return isUsbmuxDeviceUnattachedError(error); + return details((error.details ?? {}) as AppErrorDetails); } function matchesRunnerErrorMessage(error: AppError, parts: readonly string[] | undefined): boolean { @@ -287,16 +341,36 @@ export function isRetryableRunnerError(err: unknown): boolean { */ export function isUsbmuxDeviceUnattachedError(error: unknown): boolean { if (!(error instanceof AppError) || error.code !== 'DEVICE_NOT_FOUND') return false; - return ( - (error.details as { usbmuxDeviceAttached?: unknown } | undefined)?.usbmuxDeviceAttached === - false - ); + return hasUsbmuxDeviceUnattached((error.details ?? {}) as AppErrorDetails); } +/** + * Default is true and not false: the common failure while the runner boots is an error + * no rule describes, and giving up on it would fail a command that the connect loop was + * about to succeed. A rule says false only when waiting cannot help. + */ export function shouldRetryRunnerConnectError(error: unknown): boolean { return runnerErrorVerdict(error, 'connectRetry') ?? true; } +/** + * The readiness preflight gave up, so the runner never saw the command: restarting the session and + * replaying is both safe and the only way out. The marker carries the rule; the message does not. + */ +export function shouldRestartRunnerAfterReadinessPreflight(error: unknown): boolean { + return runnerErrorVerdict(error, 'restartAfterReadinessPreflight') ?? false; +} + +/** + * The runner refused or never answered on every route, which is what a restored artifact + * that cannot boot looks like. Deliberately narrow: the recovery it authorises is a clean + * `xcodebuild` rebuild, so a slow boot, a busy device or a transport failure partway + * through a command must not pay that price. + */ +export function shouldRebuildCachedRunnerArtifact(error: unknown): boolean { + return runnerErrorVerdict(error, 'artifactSuspect') ?? false; +} + /** * Session-fatal classification for a runner response error: when defined, the * cached runner session must be invalidated with this reason instead of being diff --git a/packages/platform-apple/src/runner/runner-lifecycle.ts b/packages/platform-apple/src/runner/runner-lifecycle.ts index 1644092c78..3c0c309f75 100644 --- a/packages/platform-apple/src/runner/runner-lifecycle.ts +++ b/packages/platform-apple/src/runner/runner-lifecycle.ts @@ -17,10 +17,12 @@ import { assertRunnerRequestActive, isRetryableRunnerError, resolveRunnerRequestSignal, + shouldRebuildCachedRunnerArtifact, + shouldRestartRunnerAfterReadinessPreflight, + shouldRestartRunnerBeforeCommandSend, shouldRetryRunnerConnectError, withRunnerCommandId, type RunnerCommand, - shouldRestartRunnerBeforeCommandSend, } from './runner-contract.ts'; import type { AppleRunnerCommandOptions, @@ -246,11 +248,7 @@ async function invalidateRunnerSessionBestEffort( function shouldRetryPrepareRunnerHealthFailure(error: AppError): boolean { if (isRequestCanceledError(error)) return false; - return ( - isRetryableRunnerError(error) || - shouldRetryRunnerConnectError(error) || - isPrepareHealthTimeout(error) - ); + return isRetryableRunnerError(error) || shouldRetryRunnerConnectError(error); } // fallow-ignore-next-line complexity @@ -310,7 +308,7 @@ export async function executeRunnerCommand( restartReason: 'runner_connect_failed_before_command_send', }); } - if (session && shouldRestartAfterReadinessPreflightError(appErr)) { + if (session && shouldRestartRunnerAfterReadinessPreflight(appErr)) { assertRunnerRequestActive(options.requestId); return await restartSessionAndRunCommand({ device, @@ -498,6 +496,11 @@ function readPreparePhaseTimeoutMs( return remainingMs; } +/** + * A rebuild here is a clean `xcodebuild`, so the verdict comes from the rules that indict + * the artifact itself rather than from "anything the connect loop would tolerate". + * Whether the session even carries a cached artifact is this module's fact, not the table's. + */ function shouldRecoverBadCachedRunnerArtifact( error: AppError, session: RunnerSession, @@ -506,14 +509,7 @@ function shouldRecoverBadCachedRunnerArtifact( } { const artifact = session.xctestrunArtifact; if (!artifact || artifact.cache === 'miss') return false; - return shouldRetryPrepareRunnerHealthFailure(error); -} - -function isPrepareHealthTimeout(error: AppError): boolean { - const message = error.message.toLowerCase(); - return ( - message.includes('timeout') || message.includes('timed out') || message.includes('deadline') - ); + return shouldRebuildCachedRunnerArtifact(error); } function wrapPrepareHealthFailure( @@ -603,19 +599,3 @@ function emitPrepareDiagnostic( }, }); } - -function isRunnerReadinessPreflightError(error: AppError): boolean { - return error.details?.runnerReadinessPreflightFailed === true; -} - -function shouldRestartAfterReadinessPreflightError(error: AppError): boolean { - return ( - isRunnerReadinessPreflightError(error) && - (isRetryableRunnerError(error) || isRunnerReadinessPreflightTimeout(error)) - ); -} - -function isRunnerReadinessPreflightTimeout(error: AppError): boolean { - const message = error.message.toLowerCase(); - return message.includes('timeout') || message.includes('timed out'); -} diff --git a/packages/platform-apple/src/runner/runner-transport.ts b/packages/platform-apple/src/runner/runner-transport.ts index 6f8cdd4957..efb89a6c26 100644 --- a/packages/platform-apple/src/runner/runner-transport.ts +++ b/packages/platform-apple/src/runner/runner-transport.ts @@ -87,5 +87,20 @@ export async function fetchWithTimeout( ): Promise { const timeoutSignal = AbortSignal.timeout(timeoutMs); const signal = requestSignal ? AbortSignal.any([requestSignal, timeoutSignal]) : timeoutSignal; - return await fetch(url, { ...init, signal }); + try { + return await fetch(url, { ...init, signal }); + } catch (error) { + // `AbortSignal.timeout` rejects with a bare DOMException that no recovery rule can + // read. Only a rejection carrying that signal's own reason is our deadline: a + // refused connection or a canceled request keeps the error it actually failed with. + if (error === timeoutSignal.reason) { + throw new AppError( + 'COMMAND_FAILED', + 'Runner command deadline exceeded', + { timeoutMs }, + error, + ); + } + throw error; + } }