From 4353e76de3476b5cb6ff3a20db0f4ba47eede40b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 10 Sep 2026 19:24:26 +0200 Subject: [PATCH] Revert "refactor(apple): carry one phase Deadline through the runner interfaces and test cancellation as a matrix (#2473)" This reverts commit f4c8f3dddaf79bcbdd0e59a5b55d30af6a09c25a. --- .../runner/__tests__/runner-adoption.test.ts | 13 +- .../runner-artifact-phase-budget.test.ts | 8 +- .../__tests__/runner-cache-metadata.test.ts | 264 ++++++------------ .../runner/__tests__/runner-client.test.ts | 7 +- .../src/runner/runner-adoption.ts | 28 +- .../src/runner/runner-artifact.ts | 55 ++-- .../src/runner/runner-cache-metadata.ts | 61 ++-- .../platform-apple/src/runner/runner-cache.ts | 2 +- .../src/runner/runner-client.ts | 9 +- .../src/runner/runner-session.ts | 54 ++-- .../src/runner/runner-xctestrun.ts | 4 +- .../snapshot-source/cache-identity.test.ts | 180 ++++-------- 12 files changed, 264 insertions(+), 421 deletions(-) diff --git a/packages/platform-apple/src/runner/__tests__/runner-adoption.test.ts b/packages/platform-apple/src/runner/__tests__/runner-adoption.test.ts index b9bb459d9f..a965252528 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-adoption.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-adoption.test.ts @@ -13,10 +13,7 @@ import { } from '../runner-lease.ts'; import { isIosRunnerDetachEnabled, tryAdoptRunnerSessionFromLease } from '../runner-adoption.ts'; import { sendRunnerCommandOnce } from '../runner-transport.ts'; -import { - createRunnerPhaseBudget, - resolveExpectedRunnerCacheMetadata, -} from '../runner-xctestrun.ts'; +import { resolveExpectedRunnerCacheMetadata } from '../runner-xctestrun.ts'; import { appleRunnerTestHost } from '../test-host.ts'; import { appleToolchainProbeResult } from './apple-toolchain-fixtures.ts'; import { mkdtempForTestSync } from './tmp-dir.ts'; @@ -164,9 +161,7 @@ test('a request canceled during the fingerprint probe fails adoption instead of }); await expect( - tryAdoptRunnerSessionFromLease(simulator, { - budget: createRunnerPhaseBudget(undefined, request.signal), - }), + tryAdoptRunnerSessionFromLease(simulator, { signal: request.signal }), ).rejects.toSatisfy(isRequestCanceledError); expect(mockSendRunnerCommandOnce).not.toHaveBeenCalled(); @@ -191,9 +186,7 @@ test('a request canceled during the fingerprint probe fails adoption instead of ); await expect( - tryAdoptRunnerSessionFromLease(simulator, { - budget: createRunnerPhaseBudget(undefined, warmRequest.signal), - }), + tryAdoptRunnerSessionFromLease(simulator, { signal: warmRequest.signal }), ).rejects.toSatisfy(isRequestCanceledError); expect(mockSendRunnerCommandOnce).not.toHaveBeenCalled(); // Ownership was never transferred: the stale lease is untouched. diff --git a/packages/platform-apple/src/runner/__tests__/runner-artifact-phase-budget.test.ts b/packages/platform-apple/src/runner/__tests__/runner-artifact-phase-budget.test.ts index 463ac1c456..f641aa4814 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-artifact-phase-budget.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-artifact-phase-budget.test.ts @@ -6,7 +6,7 @@ import { AppError } from '@agent-device/kernel/errors'; import { resetAllProcessMemosForTests } from '@agent-device/kernel/ttl-memo'; import { appleRunnerTestHost } from '../test-host.ts'; import type { ExecOptions, ExecResult, ExecStreamOptions } from '../host.ts'; -import { createRunnerPhaseBudget, ensureXctestrunArtifact } from '../runner-xctestrun.ts'; +import { ensureXctestrunArtifact } from '../runner-xctestrun.ts'; import { appleToolchainProbeResult } from './apple-toolchain-fixtures.ts'; import { MACOS_DEVICE } from './device-fixtures.ts'; import { mkdtempForTestSync } from './tmp-dir.ts'; @@ -67,7 +67,7 @@ afterEach(() => { test('a warm toolchain leaves the build the whole phase budget', async () => { await assert.rejects( - ensureXctestrunArtifact(MACOS_DEVICE, { budget: createRunnerPhaseBudget(120_000, undefined) }), + ensureXctestrunArtifact(MACOS_DEVICE, { buildTimeoutMs: 120_000 }), missingXctestrun, ); @@ -88,7 +88,7 @@ test('a cold-start probe stall comes out of the build budget instead of being ad }); await assert.rejects( - ensureXctestrunArtifact(MACOS_DEVICE, { budget: createRunnerPhaseBudget(120_000, undefined) }), + ensureXctestrunArtifact(MACOS_DEVICE, { buildTimeoutMs: 120_000 }), missingXctestrun, ); @@ -107,7 +107,7 @@ test('a probe that spends the whole phase fails before xcodebuild is spawned', a }); await assert.rejects( - ensureXctestrunArtifact(MACOS_DEVICE, { budget: createRunnerPhaseBudget(30_000, undefined) }), + ensureXctestrunArtifact(MACOS_DEVICE, { buildTimeoutMs: 30_000 }), (error: unknown) => error instanceof AppError && error.details?.reason === 'runner_phase_budget_exhausted' && diff --git a/packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts b/packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts index 7825723273..91415f15a7 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts @@ -6,7 +6,7 @@ import { IOS_DEVICE, IOS_SIMULATOR, MACOS_DEVICE } from './device-fixtures.ts'; import { appleRunnerTestHost } from '../test-host.ts'; import type { ExecOptions } from '../host.ts'; import { - createRunnerPhaseBudget, + createRunnerPhaseDeadline, diffComparableRunnerCacheMetadata, resolveRunnerBundleBuildSettings, resolveRunnerMaxConcurrentDestinationsFlag, @@ -278,16 +278,37 @@ describe('toolchain probe budget', () => { assert.deepEqual(xcodebuildTimeouts, [COLD_TOOLCHAIN_PROBE_TIMEOUT_MS, 15_000]); }); + test('a toolchain host that never returns stops at the shared budget instead of once per probe', () => { + const clock = installFakeToolchainClock(); + runCmdSync.mockImplementation((command: string, args: string[], options: ExecOptions) => { + throw blockForWholeTimeout(clock, command, args, options); + }); + runCmdSync.mockClear(); + + assert.throws( + () => resolveExpectedRunnerCacheMetadata(MACOS_DEVICE), + // A budget spent before the remaining probes could start is not an + // unreadable toolchain: nothing probed it, so the error says the budget + // ran out rather than pointing at `xcode-select`. + (error: unknown) => expectRunnerPhaseBudgetExhausted(error), + ); + // 30 s + a 15 s retry spends the whole budget on the first probe; the two + // xcrun probes then fail on the budget instead of blocking for 30 s each. + assert.equal(runCmdSync.mock.calls.length, 2); + assert.equal(clock.nowMs, 45_000); + }); + test('an owning phase with 4 s left gets one 4 s attempt and no retry', () => { const clock = installFakeToolchainClock(); - const phaseBudget = createRunnerPhaseBudget(4_000, undefined); + const phaseDeadline = createRunnerPhaseDeadline(4_000); runCmdSync.mockImplementation((command: string, args: string[], options: ExecOptions) => { throw blockForWholeTimeout(clock, command, args, options); }); runCmdSync.mockClear(); assert.throws( - () => resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR, undefined, phaseBudget), + () => + resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR, undefined, { deadline: phaseDeadline }), (error: unknown) => expectRunnerPhaseBudgetExhausted(error), ); assert.equal(runCmdSync.mock.calls.length, 1); @@ -296,6 +317,75 @@ describe('toolchain probe budget', () => { assert.equal(clock.nowMs, 4_000); }); + test('a request canceled while a probe blocked surfaces the cancellation instead of retrying', () => { + const clock = installFakeToolchainClock(); + const request = new AbortController(); + runCmdSync.mockImplementation((command: string, args: string[], options: ExecOptions) => { + const timeout = blockForWholeTimeout(clock, command, args, options); + request.abort(); + throw timeout; + }); + runCmdSync.mockClear(); + + assert.throws( + () => + resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR, undefined, { signal: request.signal }), + (error: unknown) => isRequestCanceledError(error), + ); + assert.equal(runCmdSync.mock.calls.length, 1); + }); + + test('a non-timeout error that also cancels the request on the last probe surfaces cancellation, not an unavailable toolchain', () => { + const request = new AbortController(); + runCmdSync.mockImplementation((command: string, args: string[]) => { + // The final probe: abort the request and fail with a plain command + // error, not the exec layer's structured timeout -- there is no next + // attempt left to catch the cancellation, so the catch here must. + if (command === 'xcrun' && args.includes('--show-sdk-build-version')) { + request.abort(); + throw new AppError('COMMAND_FAILED', 'xcrun: unexpected error', {}); + } + return appleToolchainProbeResult(command, args); + }); + runCmdSync.mockClear(); + + assert.throws( + () => + resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR, undefined, { signal: request.signal }), + (error: unknown) => isRequestCanceledError(error), + ); + assert.equal(runCmdSync.mock.calls.length, 3); + }); + + test('an already-canceled request runs no toolchain probe at all, cold or with the fingerprint cache warm', () => { + installFakeToolchainClock(); + runCmdSync.mockClear(); + + assert.throws( + () => + resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR, undefined, { + signal: AbortSignal.abort(), + }), + (error: unknown) => isRequestCanceledError(error), + ); + assert.equal(runCmdSync.mock.calls.length, 0); + + // Warm the real fingerprint memo with an ordinary request, then repeat + // with an already-aborted signal: the cache-hit path must check + // cancellation before it returns the memoized value, not skip it (#2422). + resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR); + runCmdSync.mockClear(); + + assert.throws( + () => + resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR, undefined, { + signal: AbortSignal.abort(), + }), + (error: unknown) => isRequestCanceledError(error), + ); + assert.equal(runCmdSync.mock.calls.length, 0); + }); + test('a probe that failed on its own and merely says "timed out" in its message is not retried', () => { installFakeToolchainClock(); runCmdSync.mockImplementation((command: string, args: string[]) => { @@ -319,174 +409,6 @@ describe('toolchain probe budget', () => { ); expect(runCmdSync.mock.calls.filter(([command]) => command === 'xcodebuild')).toHaveLength(1); }); - - /** - * One row per way a fingerprint read can be interrupted: when the owning request aborts, - * what the probe that abort lands on does, and what the phase must then surface. - * `spawnSync` cannot be interrupted once it has started, so a cancellation is only ever - * observed between attempts -- the exec count is what pins which attempt each row stopped. - */ - type ToolchainProbeCancellationCase = { - label: string; - /** Which execs refuse to answer, and how they end; every other exec answers. */ - failing?: { at: 'first' | 'final' | 'every'; as: 'exec-timeout' | 'command-failure' }; - /** - * When the owning request aborts: never, before the phase runs a probe at all, while - * the failing exec is still blocked, or as that exec's timeout unwinds. - */ - aborts: 'never' | 'before-any-probe' | 'while-it-blocks' | 'as-it-unwinds'; - /** Whether the process-wide fingerprint memo already holds an answer. */ - fingerprintCache?: 'warm'; - expected: 'request-canceled' | 'budget-exhausted' | 'fingerprint'; - /** Execs the phase is allowed to have run by the time it settles. */ - execs: number; - /** Wall clock the phase spent: only an exec that blocks for its whole timeout moves it. */ - clockMs: number; - }; - - const CANCELLATION_CASES: ToolchainProbeCancellationCase[] = [ - { - label: 'aborted before the first probe, cold cache', - aborts: 'before-any-probe', - expected: 'request-canceled', - execs: 0, - clockMs: 0, - }, - { - // A cache hit must not answer a request that is already gone (#2422 round 4). - label: 'aborted before the first probe, fingerprint cache warm', - aborts: 'before-any-probe', - fingerprintCache: 'warm', - expected: 'request-canceled', - execs: 0, - clockMs: 0, - }, - { - label: 'aborted while the first probe blocks, and it then times out', - failing: { at: 'first', as: 'exec-timeout' }, - aborts: 'while-it-blocks', - expected: 'request-canceled', - execs: 1, - clockMs: COLD_TOOLCHAIN_PROBE_TIMEOUT_MS, - }, - { - label: 'aborted while the first probe fails with a non-timeout error', - failing: { at: 'first', as: 'command-failure' }, - aborts: 'while-it-blocks', - expected: 'request-canceled', - execs: 1, - clockMs: 0, - }, - { - // The budget still had 15 s: only the cancellation stops the retry. - label: "aborted as the first attempt's timeout unwinds, before its retry", - failing: { at: 'first', as: 'exec-timeout' }, - aborts: 'as-it-unwinds', - expected: 'request-canceled', - execs: 1, - clockMs: COLD_TOOLCHAIN_PROBE_TIMEOUT_MS, - }, - { - // Nothing is left to catch the cancellation on a later attempt, so the failing - // probe's own catch must: an unavailable toolchain would be the wrong verdict. - label: 'aborted while the final probe fails with a non-timeout error', - failing: { at: 'final', as: 'command-failure' }, - aborts: 'while-it-blocks', - expected: 'request-canceled', - execs: 3, - clockMs: 0, - }, - { - label: 'aborted while the final probe blocks, and it then times out', - failing: { at: 'final', as: 'exec-timeout' }, - aborts: 'while-it-blocks', - expected: 'request-canceled', - execs: 3, - clockMs: COLD_TOOLCHAIN_PROBE_TIMEOUT_MS, - }, - { - // 30 s plus a 15 s retry spends the whole 45 s ceiling on the first probe. The two - // xcrun probes never ran, so the verdict is the budget, not an unreadable toolchain: - // an `xcode-select` hint here would point at the wrong thing. - label: 'never aborted, the first probe and its retry spend the whole budget', - failing: { at: 'every', as: 'exec-timeout' }, - aborts: 'never', - expected: 'budget-exhausted', - execs: 2, - clockMs: 45_000, - }, - { - label: 'never aborted, the first probe times out and its retry recovers', - failing: { at: 'first', as: 'exec-timeout' }, - aborts: 'never', - expected: 'fingerprint', - execs: 4, - clockMs: COLD_TOOLCHAIN_PROBE_TIMEOUT_MS, - }, - ]; - - test.each(CANCELLATION_CASES)('cancellation matrix: $label', (testCase) => { - const clock = installFakeToolchainClock(); - const request = new AbortController(); - if (testCase.fingerprintCache === 'warm') { - runCmdSync.mockImplementation(appleToolchainProbeResult); - resolveExpectedRunnerCacheMetadata(IOS_SIMULATOR); - } - if (testCase.aborts === 'before-any-probe') request.abort(); - - let execs = 0; - runCmdSync.mockImplementation((command: string, args: string[], options: ExecOptions) => { - execs += 1; - if (!failsOnExec(testCase, execs)) return appleToolchainProbeResult(command, args); - if (testCase.aborts === 'while-it-blocks') request.abort(); - const failure = - testCase.failing?.as === 'exec-timeout' - ? blockForWholeTimeout(clock, command, args, options) - : // No `timeoutMs` detail: the tool failed on its own, so nothing retries it. - new AppError('COMMAND_FAILED', `${command}: unexpected error`, { cmd: command, args }); - if (testCase.aborts === 'as-it-unwinds') request.abort(); - throw failure; - }); - - const readFingerprint = () => - resolveExpectedRunnerCacheMetadata( - IOS_SIMULATOR, - undefined, - createRunnerPhaseBudget(undefined, request.signal), - ); - - if (testCase.expected === 'fingerprint') { - const metadata = readFingerprint(); - assert.equal(metadata.xcodeVersion, '26.2', testCase.label); - assert.equal(metadata.xcodeBuildVersion, '17C52', testCase.label); - } else { - assert.throws(readFingerprint, (error: unknown) => { - assert.ok( - testCase.expected === 'request-canceled' - ? isRequestCanceledError(error) - : expectRunnerPhaseBudgetExhausted(error), - `${testCase.label}: expected ${testCase.expected}, got ${String(error)}`, - ); - return true; - }); - } - assert.equal(execs, testCase.execs, `${testCase.label}: exec count`); - assert.equal(clock.nowMs, testCase.clockMs, `${testCase.label}: wall clock spent`); - }); - - /** Which exec a row's failing probe is: the first, the last of the three, or all of them. */ - function failsOnExec(testCase: ToolchainProbeCancellationCase, exec: number): boolean { - switch (testCase.failing?.at) { - case 'first': - return exec === 1; - case 'final': - return exec === 3; - case 'every': - return true; - default: - return false; - } - } }); /** The error a runner phase raises when a step is reached with nothing left to spend. */ diff --git a/packages/platform-apple/src/runner/__tests__/runner-client.test.ts b/packages/platform-apple/src/runner/__tests__/runner-client.test.ts index 0afa695a0f..ba9be5f861 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-client.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-client.test.ts @@ -52,7 +52,6 @@ import { } from '../runner-cache.ts'; import { ensureXctestrunArtifact, xctestrunReferencesProjectRoot } from '../runner-artifact.ts'; import { - createRunnerPhaseBudget, markRunnerXctestrunArtifactBadForRun, resolveExpectedRunnerCacheMetadata, resolveRunnerDerivedPath, @@ -1100,10 +1099,10 @@ test('ensureXctestrunArtifact aborts only the disconnected request build and pre }); const canceledPromise = ensureXctestrunArtifact(canceledDevice, { - budget: createRunnerPhaseBudget(undefined, canceledController.signal), + signal: canceledController.signal, }); const survivorPromise = ensureXctestrunArtifact(survivorDevice, { - budget: createRunnerPhaseBudget(undefined, survivorController.signal), + signal: survivorController.signal, }); await Promise.all([canceledBuildStarted.promise, survivorBuildStarted.promise]); @@ -1409,7 +1408,7 @@ test('ensureXctestrunArtifact stress-recovers after a bad restored artifact', as }); const rebuilt = await ensureXctestrunArtifact(macOsDevice, { - budget: createRunnerPhaseBudget(300_000, undefined), + buildTimeoutMs: 300_000, }); assert.equal(rebuilt.xctestrunPath, rebuiltXctestrunPath); diff --git a/packages/platform-apple/src/runner/runner-adoption.ts b/packages/platform-apple/src/runner/runner-adoption.ts index 3c2819def1..ad97f5e7d9 100644 --- a/packages/platform-apple/src/runner/runner-adoption.ts +++ b/packages/platform-apple/src/runner/runner-adoption.ts @@ -1,6 +1,7 @@ import path from 'node:path'; import { resolveIosSimulatorDeviceSetPath, + type Deadline, emitDiagnostic, isProcessAlive, parseBooleanLiteral, @@ -21,7 +22,7 @@ import { requireRunnerPhaseRemainingMs, resolveExpectedRunnerCacheMetadata, resolveRunnerDerivedPath, - type RunnerPhaseBudget, + type RunnerCacheProbeBudget, type RunnerXctestrunArtifact, } from './runner-xctestrun.ts'; import { @@ -53,11 +54,11 @@ export function isIosRunnerDetachEnabled(env: NodeJS.ProcessEnv = process.env): export async function tryAdoptRunnerSessionFromLease( device: DeviceInfo, options: { - /** - * The startup phase's one budget: the fingerprint check below spends from it, its - * cancellation reaches those probes, and the adopted session inherits the rest (#2422). - */ - budget?: RunnerPhaseBudget; + startupTimeoutMs?: number; + /** The startup phase's clock: the fingerprint check below spends from it (#2422). */ + phaseDeadline?: Deadline; + /** The owning request's cancellation signal, forwarded to those probes. */ + signal?: AbortSignal; expectedRunnerSessionId?: string; }, ): Promise { @@ -96,7 +97,10 @@ export async function tryAdoptRunnerSessionFromLease( if (!verifyLeaseRunnerPidIdentity(lease, runnerPid)) { return skip('runner_pid_recycled'); } - const expectedDerived = resolveExpectedDerivedPath(device, options.budget); + const expectedDerived = resolveExpectedDerivedPath(device, { + deadline: options.phaseDeadline, + signal: options.signal, + }); if (!expectedDerived) return skip('expected_derived_unresolved'); if (!lease.xctestrunPath.startsWith(`${expectedDerived}${path.sep}`)) { return skip('artifact_fingerprint_mismatch'); @@ -146,7 +150,7 @@ async function probeRunnerAnswersUptime(device: DeviceInfo, port: number): Promi function resolveExpectedDerivedPath( device: DeviceInfo, - budget: RunnerPhaseBudget | undefined, + budget: RunnerCacheProbeBudget, ): string | null { try { return resolveRunnerDerivedPath( @@ -165,7 +169,7 @@ function buildAdoptedRunnerSession( lease: RunnerLease, runnerPid: number, expectedDerived: string, - options: { budget?: RunnerPhaseBudget }, + options: { startupTimeoutMs?: number; phaseDeadline?: Deadline }, ): RunnerSession & { lease: RunnerLease } { const sessionId = lease.sessionId; const artifact: RunnerXctestrunArtifact = { @@ -191,7 +195,11 @@ function buildAdoptedRunnerSession( // The probe already proved the runner answers commands. ready: true, startupTimeoutMs: normalizeRunnerStartupTimeoutMs( - requireRunnerPhaseRemainingMs(options.budget, 'runner_session_adoption'), + requireRunnerPhaseRemainingMs( + options.phaseDeadline, + options.startupTimeoutMs, + 'runner_session_adoption', + ), ), lease: buildRunnerLease({ deviceId: device.id, diff --git a/packages/platform-apple/src/runner/runner-artifact.ts b/packages/platform-apple/src/runner/runner-artifact.ts index 5ed1d81680..af7156d46e 100644 --- a/packages/platform-apple/src/runner/runner-artifact.ts +++ b/packages/platform-apple/src/runner/runner-artifact.ts @@ -5,6 +5,7 @@ import os from 'node:os'; import path from 'node:path'; import { runCmdStreaming, + type Deadline, type ExecBackgroundResult, withKeyedLock, emitRequestProgress, @@ -19,6 +20,7 @@ import { assertSafeDerivedCleanup, cleanRunnerDerivedArtifacts, cleanRunnerDerivedBeforeEvaluation, + createRunnerPhaseDeadline, emitRunnerXctestrunDecision, emitRunnerXctestrunRebuildDecision, evaluateExistingXctestrun, @@ -32,7 +34,6 @@ import { resolveRunnerSigningBuildSettings, writeRunnerCacheMetadataForArtifacts, type ExistingXctestrunState, - type RunnerPhaseBudget, type RunnerXctestrunCacheKind, type RunnerXctestrunCacheMetadata, } from './runner-cache.ts'; @@ -70,17 +71,13 @@ export type ExternalXctestRunnerOptions = { iosXctestEnvDir?: string; }; -/** What the build phase reads: its budget, and where it logs. */ +/** What the build phase reads: its budget, where it logs, and how it is canceled. */ type RunnerXctestrunBuildOptions = { verbose?: boolean; logPath?: string; traceLogPath?: string; - /** - * The build phase's one budget, opened by whoever owns the build: the cache decision's - * blocking toolchain probes and `xcodebuild` spend the same clock, and the owning - * request cancels both (#2422). - */ - budget?: RunnerPhaseBudget; + buildTimeoutMs?: number; + signal?: AbortSignal; }; export async function ensureXctestrunArtifact( @@ -93,11 +90,12 @@ export async function ensureXctestrunArtifact( if (external) return external; const projectRoot = findProjectRoot(); - const expectedCacheMetadata = resolveExpectedRunnerCacheMetadata( - device, - projectRoot, - options.budget, - ); + // One clock for the whole build phase: the toolchain probes and the xcodebuild share it. + const phaseDeadline = createRunnerPhaseDeadline(options.buildTimeoutMs); + const expectedCacheMetadata = resolveExpectedRunnerCacheMetadata(device, projectRoot, { + deadline: phaseDeadline, + signal: options.signal, + }); const derived = resolveRunnerDerivedPath(device, expectedCacheMetadata); return await withKeyedLock(runnerXctestrunBuildLocks, derived, async () => { const releaseCacheLock = await acquireRunnerXctestrunCacheLock(derived); @@ -105,6 +103,7 @@ export async function ensureXctestrunArtifact( return await ensureXctestrunUnderCacheLock({ device, options, + phaseDeadline, projectRoot, expectedCacheMetadata, derived, @@ -162,12 +161,13 @@ function resolveExternalXctestDerivedDataPath(xctestrunPath: string): string { async function ensureXctestrunUnderCacheLock(params: { device: DeviceInfo; options: RunnerXctestrunBuildOptions; + phaseDeadline: Deadline | undefined; projectRoot: string; expectedCacheMetadata: RunnerXctestrunCacheMetadata; derived: string; forceRebuild: boolean; }): Promise { - const { device, options, projectRoot, expectedCacheMetadata, derived } = params; + const { device, options, phaseDeadline, projectRoot, expectedCacheMetadata, derived } = params; cleanRunnerDerivedBeforeEvaluation(derived, params.forceRebuild); const existing = await evaluateExistingXctestrunForDevice({ device, @@ -195,6 +195,7 @@ async function ensureXctestrunUnderCacheLock(params: { return await buildXctestrunArtifact({ device, options, + phaseDeadline, projectRoot, expectedCacheMetadata, derived, @@ -232,27 +233,41 @@ async function resolveReusableXctestrunArtifact(params: { async function buildXctestrunArtifact(params: { device: DeviceInfo; options: RunnerXctestrunBuildOptions; + phaseDeadline: Deadline | undefined; projectRoot: string; expectedCacheMetadata: RunnerXctestrunCacheMetadata; derived: string; cache: RunnerXctestrunArtifact['cache']; reason: ExistingXctestrunState['reason']; }): Promise { - const { device, options, projectRoot, expectedCacheMetadata, derived, cache, reason } = params; + const { + device, + options, + phaseDeadline, + projectRoot, + expectedCacheMetadata, + derived, + cache, + reason, + } = params; const projectPath = resolveAppleRunnerProjectPath(projectRoot); if (!fs.existsSync(projectPath)) { throw new AppError('COMMAND_FAILED', 'iOS runner project not found', { projectPath }); } - const buildTimeoutMs = requireRunnerPhaseRemainingMs(options.budget, 'runner_xctestrun_build'); + const buildTimeoutMs = requireRunnerPhaseRemainingMs( + phaseDeadline, + options.buildTimeoutMs, + 'runner_xctestrun_build', + ); const buildStartedAt = Date.now(); emitRequestProgress({ type: 'command', status: 'progress', message: 'Building Apple runner...', }); - await buildRunnerXctestrun(device, projectPath, derived, options, buildTimeoutMs); + await buildRunnerXctestrun(device, projectPath, derived, { ...options, buildTimeoutMs }); const buildMs = Math.max(0, Date.now() - buildStartedAt); const built = findXctestrun(derived, device); @@ -459,8 +474,6 @@ async function buildRunnerXctestrun( projectPath: string, derived: string, options: RunnerXctestrunBuildOptions, - /** What {@link requireRunnerPhaseRemainingMs} left of the build phase, for the exec layer. */ - buildTimeoutMs: number | undefined, ): Promise { const runnerBundleBuildSettings = resolveRunnerBundleBuildSettings(process.env); const signingBuildSettings = resolveRunnerSigningBuildSettings( @@ -497,8 +510,8 @@ async function buildRunnerXctestrun( ], { detached: true, - timeoutMs: buildTimeoutMs, - signal: options.budget?.signal, + timeoutMs: options.buildTimeoutMs, + signal: options.signal, onSpawn: (child) => { runnerPrepProcesses.add(child); child.on('close', () => { diff --git a/packages/platform-apple/src/runner/runner-cache-metadata.ts b/packages/platform-apple/src/runner/runner-cache-metadata.ts index 702aec6bd6..15a402d844 100644 --- a/packages/platform-apple/src/runner/runner-cache-metadata.ts +++ b/packages/platform-apple/src/runner/runner-cache-metadata.ts @@ -67,43 +67,24 @@ type ToolchainProbeFailure = { }; /** - * Everything one runner phase may spend: the single clock every step of the phase reads, - * and the owning request's cancellation. Created once, where the phase begins, and handed - * on as this object — no step below receives a timeout number it could open a second phase - * with, which is how a cold probe stall and the build each spent the same budget (#2422). + * The one clock a runner phase spends, created once per phase and read with + * {@link requireRunnerPhaseRemainingMs}; `undefined` for a caller carrying no budget. */ -export type RunnerPhaseBudget = Readonly<{ - /** The phase's clock; absent when its owner carries no budget at all. */ - deadline?: Deadline; - /** The owning request's cancellation signal, if it carries one. */ - signal?: AbortSignal; -}>; - -/** - * Opens a phase from the numeric timeout its public option carries: the one place a number - * becomes a budget, so every boundary below it takes the {@link RunnerPhaseBudget} instead. - */ -export function createRunnerPhaseBudget( - timeoutMs: number | undefined, - signal: AbortSignal | undefined, -): RunnerPhaseBudget { - const bounded = timeoutMs !== undefined && Number.isFinite(timeoutMs); - return { - deadline: bounded ? Deadline.fromTimeoutMs(Math.max(0, timeoutMs)) : undefined, - signal, - }; +export function createRunnerPhaseDeadline(timeoutMs: number | undefined): Deadline | undefined { + if (timeoutMs === undefined || !Number.isFinite(timeoutMs)) return undefined; + return Deadline.fromTimeoutMs(Math.max(0, timeoutMs)); } /** - * What the phase has left for its next step, or `undefined` when it carries no deadline. - * Throws rather than returning zero, so a spent phase fails before it spawns. + * What the phase has left for its next step, or `fallbackTimeoutMs` when it carries no + * deadline. Throws rather than returning zero, so a spent phase fails before it spawns. */ export function requireRunnerPhaseRemainingMs( - budget: RunnerPhaseBudget | undefined, + deadline: Deadline | undefined, + fallbackTimeoutMs: number | undefined, phase: string, ): number | undefined { - const deadline = budget?.deadline; - if (!deadline) return undefined; + if (!deadline) return fallbackTimeoutMs; const remainingMs = Math.floor(deadline.remainingMs()); if (remainingMs <= 0) throw runnerPhaseBudgetExhaustedError(phase); return remainingMs; @@ -118,10 +99,20 @@ function runnerPhaseBudgetExhaustedError(phase: string): AppError { }); } +/** + * What the phase that wants a runner cache decision has left to spend on it. A caller + * with neither field still gets {@link TOOLCHAIN_FINGERPRINT_BUDGET_MS} as the ceiling. + */ +export type RunnerCacheProbeBudget = { + /** The owning phase's clock, shared with whatever the phase does next. */ + deadline?: Deadline; + /** The owning request's cancellation signal, if it carries one. */ + signal?: AbortSignal; +}; + /** * The remaining-time and cancellation view the probes consult: one per fingerprint read, - * so the three probes and their retries share a single budget. A phase with no deadline - * still gets {@link TOOLCHAIN_FINGERPRINT_BUDGET_MS} as the ceiling. + * so the three probes and their retries share a single budget. * * `spawnSync` cannot be interrupted once it has started, so cancellation is observed * between attempts; the per-attempt cap is what bounds how long that takes. @@ -133,7 +124,9 @@ type ToolchainProbeClock = { throwIfCanceled(): void; }; -function createToolchainProbeClock(budget: RunnerPhaseBudget | undefined): ToolchainProbeClock { +function createToolchainProbeClock( + budget: RunnerCacheProbeBudget | undefined, +): ToolchainProbeClock { const phaseDeadline = budget?.deadline; const deadline = Deadline.fromTimeoutMs( Math.min( @@ -224,7 +217,7 @@ export const IOS_RUNNER_CONTAINER_BUNDLE_IDS: string[] = resolveRunnerContainerB export function resolveExpectedRunnerCacheMetadata( device: DeviceInfo, projectRoot: string = findProjectRoot(), - budget?: RunnerPhaseBudget, + budget?: RunnerCacheProbeBudget, ): RunnerXctestrunCacheMetadata { const platformName = resolveRunnerPlatformName(device); return { @@ -263,7 +256,7 @@ function toolchainFingerprintCache(): TtlMemo { - // A cache prewarm owns the build phase it starts: one budget for the cache - // decision's toolchain probes and the `xcodebuild` that may follow them. - await ensureXctestrunArtifact(device, { - ...runnerOptions, - budget: createRunnerPhaseBudget(runnerOptions.buildTimeoutMs, runnerOptions.signal), - }); + await ensureXctestrunArtifact(device, runnerOptions); }, }); } diff --git a/packages/platform-apple/src/runner/runner-session.ts b/packages/platform-apple/src/runner/runner-session.ts index 5e070f3c42..40198d4b61 100644 --- a/packages/platform-apple/src/runner/runner-session.ts +++ b/packages/platform-apple/src/runner/runner-session.ts @@ -19,14 +19,14 @@ import { waitForRunner, RUNNER_STARTUP_TIMEOUT_MS } from './runner-startup-trans import { sendRunnerCommandOnce } from './runner-transport.ts'; import { acquireXcodebuildSimulatorSetRedirect, - createRunnerPhaseBudget, + createRunnerPhaseDeadline, ensureXctestrunArtifact, IOS_RUNNER_CONTAINER_BUNDLE_IDS, prepareXctestrunWithEnv, requireRunnerPhaseRemainingMs, resolveExpectedRunnerCacheMetadata, resolveRunnerDerivedPath, - type RunnerPhaseBudget, + type RunnerCacheProbeBudget, } from './runner-xctestrun.ts'; import { resolveRunnerRequestSignal, @@ -112,27 +112,21 @@ export async function ensureRunnerSession( // from a retained-after-close runner no longer applies. cancelIosRunnerIdleStop(device.id); return await withRunnerSessionLock(device.id, async () => { - // One budget for the whole startup phase, opened here from the request-level - // `startupTimeoutMs`: the reuse check's toolchain probes, adoption and the startup - // itself all spend this one clock. The request's abort signal rides with it, so a - // client disconnect kills the blocking xctestrun build and runner launch - // (killProcessTree via exec) instead of orphaning them. Request-scoped: only this - // request's device startup reacts, and a signal-less internal caller (shutdown) - // simply gets undefined. - const startupBudget = createRunnerPhaseBudget( - options.startupTimeoutMs, - resolveRunnerRequestSignal(options), - ); + // One clock for the whole startup phase: the toolchain probes and the startup share it. + const phaseDeadline = createRunnerPhaseDeadline(options.startupTimeoutMs); const existing = runnerSessions.get(device.id); if (existing) { assertExpectedRunnerSession(existing, options.expectedRunnerSessionId); - const reusable = await resolveReusableRunnerSession(device, existing, startupBudget); + const reusable = await resolveReusableRunnerSession(device, existing, { + deadline: phaseDeadline, + signal: resolveRunnerRequestSignal(options), + }); if (reusable) return reusable; } return await withRunnerLeaseLock( device.id, - async () => await startRunnerSessionWithLease(device, options, startupBudget), + async () => await startRunnerSessionWithLease(device, options, phaseDeadline), ); }); } @@ -140,10 +134,14 @@ export async function ensureRunnerSession( async function startRunnerSessionWithLease( device: DeviceInfo, options: RunnerSessionOptions, - startupBudget: RunnerPhaseBudget, + phaseDeadline: Deadline | undefined, ): Promise { const startupTimings: Record = {}; - const signal = startupBudget.signal; + // The owning request's abort signal so a client disconnect kills the blocking + // xctestrun build and runner launch (killProcessTree via exec) instead of + // orphaning them. Request-scoped: only this request's device startup reacts, + // and a signal-less internal caller (shutdown) simply gets undefined. + const signal = resolveRunnerRequestSignal(options); const logicalLeaseContext = normalizeRunnerLogicalLeaseContext( options.runnerLeaseContext, device.id, @@ -161,7 +159,9 @@ async function startRunnerSessionWithLease( 'adopt_detached_runner', async () => await tryAdoptRunnerSessionFromLease(device, { - budget: startupBudget, + startupTimeoutMs: options.startupTimeoutMs, + phaseDeadline, + signal, expectedRunnerSessionId: options.expectedRunnerSessionId, }), ); @@ -192,16 +192,16 @@ async function startRunnerSessionWithLease( phase: 'ios_runner_startup_cleanup_stale_bundles_skipped', }); } - // Read before the build, which is a phase of its own with its own budget (#2422). - const startupTimeoutMs = requireRunnerPhaseRemainingMs(startupBudget, 'runner_session_startup'); + // Read before the build, which answers to its own `buildTimeoutMs` deadline (#2422). + const startupTimeoutMs = requireRunnerPhaseRemainingMs( + phaseDeadline, + options.startupTimeoutMs, + 'runner_session_startup', + ); const xctestrunArtifact = await measureRunnerStartupStep( startupTimings, 'ensure_xctestrun', - async () => - await ensureXctestrunArtifact(device, { - ...options, - budget: createRunnerPhaseBudget(options.buildTimeoutMs, signal), - }), + async () => await ensureXctestrunArtifact(device, { ...options, signal }), ); startupTimings.build_xctestrun = xctestrunArtifact.buildMs; const port = await measureRunnerStartupStep( @@ -324,7 +324,7 @@ function runnerSessionOwnershipChanged(): AppError { async function resolveReusableRunnerSession( device: DeviceInfo, existing: RunnerSession, - startupBudget: RunnerPhaseBudget, + cacheProbeBudget: RunnerCacheProbeBudget, ): Promise { if (!isRunnerProcessAlive(existing.child.pid)) { await measureRunnerStartupStep({}, 'stop_stale_session', async () => { @@ -354,7 +354,7 @@ async function resolveReusableRunnerSession( const expectedDerived = resolveRunnerDerivedPath( device, - resolveExpectedRunnerCacheMetadata(device, undefined, startupBudget), + resolveExpectedRunnerCacheMetadata(device, undefined, cacheProbeBudget), ); if (existingArtifact?.derived !== expectedDerived) { emitDiagnostic({ diff --git a/packages/platform-apple/src/runner/runner-xctestrun.ts b/packages/platform-apple/src/runner/runner-xctestrun.ts index 41e4694db4..ca024295fc 100644 --- a/packages/platform-apple/src/runner/runner-xctestrun.ts +++ b/packages/platform-apple/src/runner/runner-xctestrun.ts @@ -12,12 +12,12 @@ export { type RunnerXctestrunCacheKind, } from './runner-cache.ts'; export { - createRunnerPhaseBudget, + createRunnerPhaseDeadline, IOS_RUNNER_CONTAINER_BUNDLE_IDS, requireRunnerPhaseRemainingMs, resolveExpectedRunnerCacheMetadata, resolveRunnerAppBundleId, resolveRunnerDerivedPath, - type RunnerPhaseBudget, + type RunnerCacheProbeBudget, } from './runner-cache-metadata.ts'; export { acquireXcodebuildSimulatorSetRedirect } from './runner-device-set.ts'; diff --git a/packages/platform-apple/src/snapshot-source/cache-identity.test.ts b/packages/platform-apple/src/snapshot-source/cache-identity.test.ts index fc71799fd0..0a81965027 100644 --- a/packages/platform-apple/src/snapshot-source/cache-identity.test.ts +++ b/packages/platform-apple/src/snapshot-source/cache-identity.test.ts @@ -46,6 +46,24 @@ test('a cold-start toolchain probe recovers on retry, and the retry gets only wh assert.equal(calls, 6); }); +test('a probe that spends the whole deadline is not retried', async () => { + const clock = { nowMs: 0 }; + const timeouts: number[] = []; + const host = fakeToolchainHost((command, _args, options) => { + timeouts.push(options.timeoutMs ?? 0); + throw blockForWholeTimeout(clock, command, options); + }); + + await assert.rejects( + readSnapshotSourceToolchain(host, 'iOS 26.2', fakeClockDeadline(30_000, clock)), + (error: unknown) => + error instanceof AppError && error.message === 'xcodebuild timed out after 30000ms', + ); + // Nothing left to retry on, so the original timeout propagates unchanged. + assert.deepEqual(timeouts, [30_000]); + assert.equal(clock.nowMs, 30_000); +}); + test('a toolchain host that never returns still fails at the deadline with the same timeout error', async () => { const clock = { nowMs: 0 }; const timeouts: number[] = []; @@ -65,6 +83,38 @@ test('a toolchain host that never returns still fails at the deadline with the s assert.equal(clock.nowMs, 60_000); }); +test('a request canceled while a probe blocked surfaces the cancellation, not the timeout', async () => { + const clock = { nowMs: 0 }; + const request = new AbortController(); + let calls = 0; + const host = fakeToolchainHost((command, _args, options) => { + calls += 1; + const timeout = blockForWholeTimeout(clock, command, options); + // The abort lands while the attempt is still blocked, which is the case the + // deadline alone cannot tell from a plain timeout: it still has room. + request.abort(); + throw timeout; + }); + + await assert.rejects( + readSnapshotSourceToolchain( + host, + 'iOS 26.2', + createSnapshotSourceDeadline(120_000, request.signal, () => clock.nowMs), + ), + (error: unknown) => { + assert.ok(error instanceof SnapshotSourceError); + assert.equal(error.failureKind, 'cancelled'); + assert.equal(error.failureCode, 'abort-signal'); + assert.equal(error.details?.reason, 'request_canceled'); + return true; + }, + ); + // The deadline still had 90 s, so only the cancellation stops the retry. + assert.equal(calls, 1); + assert.equal(clock.nowMs, 30_000); +}); + test('a probe that failed on its own and merely says "timed out" in its message is not retried', async () => { const clock = { nowMs: 0 }; let calls = 0; @@ -84,136 +134,6 @@ test('a probe that failed on its own and merely says "timed out" in its message assert.equal(calls, 1); }); -/** - * One row per way a toolchain read can be interrupted: how the probe the row exercises - * ends, when the owning request aborts relative to it, and what the caller must then see. - * The retry is the only place a cancellation can be observed -- an exec already running - * cannot be taken back -- so the exec count is what pins where each row stopped. - */ -type ToolchainProbeCancellationCase = { - label: string; - /** How the first probe ends; later probes answer. Absent when no probe runs at all. */ - firstProbe?: 'exec-timeout' | 'command-failure'; - /** - * When the owning request aborts: never, before the phase even opens its deadline, while - * the first probe is still blocked, or as that probe's timeout unwinds. - */ - aborts: 'never' | 'before-the-deadline' | 'while-it-blocks' | 'as-it-unwinds'; - /** The phase deadline. 30 s is spent in full by one stalled probe, leaving no retry. */ - deadlineMs: number; - expected: 'cancelled' | 'exec-timeout' | 'command-failure'; - execs: number; - clockMs: number; -}; - -const CANCELLATION_CASES: ToolchainProbeCancellationCase[] = [ - { - label: 'aborted before the phase opened its deadline', - aborts: 'before-the-deadline', - deadlineMs: 120_000, - expected: 'cancelled', - execs: 0, - clockMs: 0, - }, - { - // The deadline still had 90 s, so only the cancellation stops the retry. - label: 'aborted while the first probe blocks, and it then times out', - firstProbe: 'exec-timeout', - aborts: 'while-it-blocks', - deadlineMs: 120_000, - expected: 'cancelled', - execs: 1, - clockMs: 30_000, - }, - { - // A probe that failed on its own is never retried, so there is no retry to cancel: - // the tool's own failure is what the caller sees, and the request fails either way. - label: 'aborted while the first probe fails with a non-timeout error', - firstProbe: 'command-failure', - aborts: 'while-it-blocks', - deadlineMs: 120_000, - expected: 'command-failure', - execs: 1, - clockMs: 0, - }, - { - label: "aborted as the first probe's timeout unwinds, before its retry", - firstProbe: 'exec-timeout', - aborts: 'as-it-unwinds', - deadlineMs: 120_000, - expected: 'cancelled', - execs: 1, - clockMs: 30_000, - }, - { - // Nothing left to retry on, so the original timeout propagates unchanged. - label: 'never aborted, the first probe spends the whole deadline', - firstProbe: 'exec-timeout', - aborts: 'never', - deadlineMs: 30_000, - expected: 'exec-timeout', - execs: 1, - clockMs: 30_000, - }, -]; - -test.each(CANCELLATION_CASES)('cancellation matrix: $label', async (testCase) => { - const clock = { nowMs: 0 }; - const request = new AbortController(); - if (testCase.aborts === 'before-the-deadline') request.abort(); - let execs = 0; - const host = fakeToolchainHost((command, args, options) => { - execs += 1; - if (execs > 1 || !testCase.firstProbe) return toolchainAnswer(command, args); - if (testCase.aborts === 'while-it-blocks') request.abort(); - const failure = - testCase.firstProbe === 'exec-timeout' - ? blockForWholeTimeout(clock, command, options) - : // No `timeoutMs` detail: the tool failed on its own, so nothing retries it. - new AppError('COMMAND_FAILED', `${command}: unexpected error`, { cmd: command }); - if (testCase.aborts === 'as-it-unwinds') request.abort(); - throw failure; - }); - - await assert.rejects( - // The deadline is opened inside the rejected call: an already-aborted request must - // fail as it is opened, before any probe runs. - async () => - await readSnapshotSourceToolchain( - host, - 'iOS 26.2', - createSnapshotSourceDeadline(testCase.deadlineMs, request.signal, () => clock.nowMs), - ), - (error: unknown) => { - assertExpectedToolchainFailure(error, testCase); - return true; - }, - ); - assert.equal(execs, testCase.execs, `${testCase.label}: exec count`); - assert.equal(clock.nowMs, testCase.clockMs, `${testCase.label}: wall clock spent`); -}); - -function assertExpectedToolchainFailure( - error: unknown, - testCase: ToolchainProbeCancellationCase, -): void { - if (testCase.expected === 'cancelled') { - assert.ok(error instanceof SnapshotSourceError, `${testCase.label}: expected a cancellation`); - assert.equal(error.failureKind, 'cancelled', testCase.label); - assert.equal(error.failureCode, 'abort-signal', testCase.label); - assert.equal(error.details?.reason, 'request_canceled', testCase.label); - return; - } - assert.ok(error instanceof AppError, `${testCase.label}: expected the probe's own failure`); - assert.equal( - error.message, - testCase.expected === 'exec-timeout' - ? 'xcodebuild timed out after 30000ms' - : 'xcodebuild: unexpected error', - testCase.label, - ); -} - /** A deadline read against a clock only {@link blockForWholeTimeout} advances. */ function fakeClockDeadline(timeoutMs: number, clock: { nowMs: number }): SnapshotSourceDeadline { return createSnapshotSourceDeadline(timeoutMs, undefined, () => clock.nowMs);