From a36e646891832a9a54dc98924fec8965315cbf87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 14 Sep 2026 18:59:58 +0200 Subject: [PATCH 1/8] fix(ios-snapshot): prepare the AX bridge off the capture deadline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cold host pays for the AX bridge inside the capture that happens to ask for it first, so the first capture of a session spent its whole deadline in a toolchain probe or a clang build and the lane lost the `wait` that was polling for a screen (#2491). Preparation is now a detached single-flight per runtime: the first capture that finds it running waits out a short budget and is served by the XCTest runner, the build keeps going for whoever asks next, and a failed attempt is answered as-is until a retry window measured from the failure expires. The grant is once per attempt rather than once per capture. A `wait` poll cycles every 200 ms or so, and a budget paid per poll would cost a cold build more in captured polls than the build itself costs, with every one of those captures ending up on the runner anyway. Both this and the pending target discovery in `snapshot-target.ts` are the same shape — one attempt per key, detached from whoever started it, a bounded wait, a typed answer while it runs — and they had already started to forget failures differently. One seam owns that shape; each owner declares its own wait budget, wait grant, retry window and pending error, and keeps its own cache of a finished value, because only the owner knows when that value stops being valid. The preparation carries the signal `close()` aborts. Once a capture has answered, no request owns this attempt any more: without that, `xcrun` could keep running for two minutes past shutdown and its cache write would land in a directory belonging to a source that is gone. --- .../src/detached-attempt.test.ts | 222 +++++++++++++ .../platform-apple/src/detached-attempt.ts | 151 +++++++++ .../platform-apple/src/snapshot-route.test.ts | 27 ++ packages/platform-apple/src/snapshot-route.ts | 30 +- .../src/snapshot-source/adapter.test.ts | 37 ++- .../src/snapshot-source/adapter.ts | 51 +-- .../src/snapshot-source/errors.ts | 1 + .../src/snapshot-source/preparation.test.ts | 294 ++++++++++++++++++ .../src/snapshot-source/preparation.ts | 131 ++++++++ .../src/snapshot-source/types.ts | 4 +- .../platform-apple/src/snapshot-target.ts | 65 ++-- 11 files changed, 932 insertions(+), 81 deletions(-) create mode 100644 packages/platform-apple/src/detached-attempt.test.ts create mode 100644 packages/platform-apple/src/detached-attempt.ts create mode 100644 packages/platform-apple/src/snapshot-source/preparation.test.ts create mode 100644 packages/platform-apple/src/snapshot-source/preparation.ts diff --git a/packages/platform-apple/src/detached-attempt.test.ts b/packages/platform-apple/src/detached-attempt.test.ts new file mode 100644 index 0000000000..edd1f2835b --- /dev/null +++ b/packages/platform-apple/src/detached-attempt.test.ts @@ -0,0 +1,222 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { createDetachedAttempts } from './detached-attempt.ts'; + +const PENDING = new Error('still-running'); + +/** A wait whose slice is instantly spent, which is how a caller reports "still running". */ +const spentWait = (calls: { count: number }) => async () => { + calls.count += 1; +}; + +test('one attempt answers every caller that arrives while it runs', async () => { + const attempts = createDetachedAttempts({ waitMs: 20 }); + let release = () => {}; + const running = new Promise((resolve) => { + release = resolve; + }); + let starts = 0; + const waitCalls = { count: 0 }; + const params = { + start: async () => { + starts += 1; + await running; + return 7; + }, + wait: spentWait(waitCalls), + pending: () => PENDING, + }; + + const first = await attempts.value('key', params).then(() => undefined, identity); + const second = await attempts.value('key', params).then(() => undefined, identity); + + assert.equal(first, PENDING); + assert.equal(second, PENDING); + assert.equal(starts, 1); + + release(); + await settle(); + assert.equal(await attempts.value('key', params), 7); +}); + +test('the first-caller grant spends the wait budget once per attempt', async () => { + const attempts = createDetachedAttempts({ waitMs: 20, waitGrant: 'first-caller' }); + let release = () => {}; + const running = new Promise((resolve) => { + release = resolve; + }); + const waitCalls = { count: 0 }; + const params = { + start: async () => { + await running; + return 7; + }, + wait: spentWait(waitCalls), + pending: () => PENDING, + }; + + await assert.rejects(attempts.value('key', params), (error) => error === PENDING); + await assert.rejects(attempts.value('key', params), (error) => error === PENDING); + assert.equal(waitCalls.count, 1); + + release(); + await settle(); + assert.equal(await attempts.value('key', params), 7); +}); + +test('the every-caller grant lets each capture wait for the same attempt', async () => { + const attempts = createDetachedAttempts({ waitMs: 20 }); + let release = () => {}; + const running = new Promise((resolve) => { + release = resolve; + }); + const waitCalls = { count: 0 }; + const params = { + start: async () => { + await running; + return 7; + }, + wait: spentWait(waitCalls), + pending: () => PENDING, + }; + + await assert.rejects(attempts.value('key', params), (error) => error === PENDING); + await assert.rejects(attempts.value('key', params), (error) => error === PENDING); + assert.equal(waitCalls.count, 2); + release(); +}); + +test('a caller that waits while the attempt settles is served the value', async () => { + const attempts = createDetachedAttempts({ waitMs: 20 }); + let release = () => {}; + const running = new Promise((resolve) => { + release = resolve; + }); + const started = attempts.value('key', { + start: async () => { + await running; + return 7; + }, + wait: async () => { + await running; + }, + pending: () => PENDING, + }); + + release(); + assert.equal(await started, 7); +}); + +test('the retry window is measured from the failure, not from the start of the attempt', async () => { + let clockMs = 0; + const attempts = createDetachedAttempts({ + waitMs: 20, + retryAfterMs: 60_000, + now: () => clockMs, + }); + let starts = 0; + const params = { + start: async () => { + starts += 1; + // A cold host can spend the whole window building before it fails at all. + clockMs += 90_000; + throw new Error('build-failed'); + }, + wait: async () => {}, + pending: () => PENDING, + }; + + await assert.rejects(attempts.value('key', params), /build-failed/); + assert.equal(starts, 1); + await assert.rejects(attempts.value('key', params), /build-failed/); + assert.equal(starts, 1); + + clockMs += 60_001; + await assert.rejects(attempts.value('key', params), /build-failed/); + assert.equal(starts, 2); +}); + +test('without a retry window the next caller starts a fresh attempt', async () => { + const attempts = createDetachedAttempts({ waitMs: 20 }); + let starts = 0; + const params = { + start: async () => { + starts += 1; + throw new Error('probe-failed'); + }, + wait: async () => {}, + pending: () => PENDING, + }; + + await assert.rejects(attempts.value('key', params), /probe-failed/); + await assert.rejects(attempts.value('key', params), /probe-failed/); + assert.equal(starts, 2); +}); + +test('close aborts a running attempt and the next caller starts its own', async () => { + const attempts = createDetachedAttempts({ waitMs: 20 }); + let release = () => {}; + const running = new Promise((resolve) => { + release = resolve; + }); + const seen: AbortSignal[] = []; + const params = { + start: async (signal: AbortSignal) => { + seen.push(signal); + await running; + return 7; + }, + wait: async () => {}, + pending: () => PENDING, + }; + + await assert.rejects(attempts.value('key', params), (error) => error === PENDING); + assert.equal(seen[0]?.aborted, false); + + attempts.close(); + assert.equal(seen[0]?.aborted, true); + + await assert.rejects(attempts.value('key', params), (error) => error === PENDING); + assert.equal(seen.length, 2); + release(); +}); + +test('a wait that rejects with the caller own cancellation stays that cancellation', async () => { + const attempts = createDetachedAttempts({ waitMs: 20 }); + const cancelled = new Error('cancelled-by-request'); + + await assert.rejects( + attempts.value('key', { + start: async () => 7, + wait: () => Promise.reject(cancelled), + pending: () => PENDING, + }), + (error) => error === cancelled, + ); +}); + +test('an attempt that throws before awaiting is reported as its own failure', async () => { + const attempts = createDetachedAttempts({ waitMs: 20 }); + + await assert.rejects( + attempts.value('key', { + start: () => { + throw new Error('source-missing'); + }, + wait: async () => {}, + pending: () => PENDING, + }), + /source-missing/, + ); +}); + +function identity(error: unknown): unknown { + return error; +} + +/** Lets a released attempt reach the settle handler before the next read. */ +function settle(): Promise { + return new Promise((resolve) => { + setTimeout(resolve, 0); + }); +} diff --git a/packages/platform-apple/src/detached-attempt.ts b/packages/platform-apple/src/detached-attempt.ts new file mode 100644 index 0000000000..4ef72d2e69 --- /dev/null +++ b/packages/platform-apple/src/detached-attempt.ts @@ -0,0 +1,151 @@ +/** + * Work that has to outlive the request that started it. + * + * A capture that gives up on a slow probe or build must not take that work down with it: the next + * capture wants the same result, and on a cold host the cost is host setup rather than request work + * (#2491). So one attempt runs per key, detached from whoever started it, and every caller that + * arrives while it runs is answered from that attempt instead of launching a second one. + * + * What each owner declares — rather than re-implements — is how long a caller may wait for an + * attempt (`waitMs`), whether every waiting caller may spend that budget or only the first one to + * arrive (`waitGrant`), how long a failed attempt keeps being answered as-is before another attempt + * starts (`retryAfterMs`, omitted for work worth retrying at once), and what an attempt that is + * still running costs the caller (`pending`). A settled success leaves the table, because only the + * owner knows when its value stops being valid; that owner caches it and invalidates it on its own + * terms. `close()` is for owners with a lifecycle: it aborts the running attempts so a build or + * probe cannot outlive the source that started it. + */ + +export type DetachedAttempts = Readonly<{ + /** + * The value for `key`, starting a detached attempt when none is running or the last failure is + * old enough to retry. Resolves when the attempt is ready, throws its failure, or throws + * `pending()` when it is still running once this caller's wait is spent. + */ + value( + key: string, + params: Readonly<{ + /** Called only when no attempt is running; must honour `signal` for `close()` to reach it. */ + start: (signal: AbortSignal) => Promise; + /** + * Sleeps inside the caller's own deadline, so a client abort rejects this call with the + * caller's typed cancellation instead of reporting a fresh `pending`. + */ + wait: (waitMs: number) => Promise; + pending: () => Error; + }>, + ): Promise; + /** Aborts every running attempt and forgets all of them. */ + close(): void; +}>; + +/** Whether a caller that arrives while an attempt runs may wait for it. */ +export type DetachedAttemptWaitGrant = + /** Each caller spends the wait budget, for work measured in seconds that every caller would + * rather wait out than give up on. */ + | 'every-caller' + /** Only the first caller to arrive spends it, for work that outlives any one capture: a poll + * loop that has already paid the budget once should not pay it again per poll (#2491). */ + | 'first-caller'; + +type Attempt = { + status: 'pending' | 'ready' | 'failed'; + value: Value; + error: unknown; + /** When the attempt stopped being pending. A retry window is measured from here, so an attempt + * that failed late is not already eligible for a retry the moment it fails. */ + settledAtMs: number; + waitSpent: boolean; + controller: AbortController; + settled: Promise; +}; + +export function createDetachedAttempts( + deps: Readonly<{ + waitMs: number; + waitGrant?: DetachedAttemptWaitGrant; + retryAfterMs?: number; + /** The clock the retry window is measured against; injected so a test can move time. */ + now?: () => number; + }>, +): DetachedAttempts { + const now = deps.now ?? Date.now; + const waitGrant = deps.waitGrant ?? 'every-caller'; + const attempts = new Map>(); + + const startAttempt = ( + key: string, + start: (signal: AbortSignal) => Promise, + ): Attempt => { + const attempt: Attempt = { + status: 'pending', + value: undefined as Value, + error: undefined, + settledAtMs: now(), + waitSpent: false, + controller: new AbortController(), + settled: undefined as unknown as Promise, + }; + let settle = () => {}; + attempt.settled = new Promise((resolve) => { + settle = resolve; + }); + attempts.set(key, attempt); + const finish = (status: 'ready' | 'failed', outcome: Value | unknown) => { + attempt.status = status; + if (status === 'ready') attempt.value = outcome as Value; + else attempt.error = outcome; + attempt.settledAtMs = now(); + // A success is the owner's to cache, because only the owner knows when the value stops being + // valid. A failure stays only as long as it should keep being answered as-is. + if (status === 'ready' || deps.retryAfterMs === undefined) { + if (attempts.get(key) === attempt) attempts.delete(key); + } + settle(); + }; + // The attempt outlives every caller that asked for it, so a rejection nobody is awaiting yet + // must never surface as an unhandled rejection; later callers read it from the record. + try { + void start(attempt.controller.signal).then( + (value) => finish('ready', value), + (error: unknown) => finish('failed', error), + ); + } catch (error) { + finish('failed', error); + } + return attempt; + }; + + const currentAttempt = ( + key: string, + start: (signal: AbortSignal) => Promise, + ): Attempt => { + const attempt = attempts.get(key); + if (!attempt) return startAttempt(key, start); + if (attempt.status !== 'failed') return attempt; + const failedAgoMs = now() - attempt.settledAtMs; + if (deps.retryAfterMs === undefined || failedAgoMs >= deps.retryAfterMs) { + return startAttempt(key, start); + } + return attempt; + }; + + return { + value: async (key, params) => { + const attempt = currentAttempt(key, params.start); + if (attempt.status === 'pending' && (waitGrant === 'every-caller' || !attempt.waitSpent)) { + // Marked before waiting: two captures arriving together must not each spend the budget. + attempt.waitSpent = true; + await Promise.race([attempt.settled, params.wait(deps.waitMs)]); + } + if (attempt.status === 'ready') return attempt.value; + if (attempt.status === 'failed') throw attempt.error; + throw params.pending(); + }, + close: () => { + const running = [...attempts.values()]; + attempts.clear(); + for (const attempt of running) attempt.controller.abort(); + }, + }; +} diff --git a/packages/platform-apple/src/snapshot-route.test.ts b/packages/platform-apple/src/snapshot-route.test.ts index fd06f4109e..cff163e2bd 100644 --- a/packages/platform-apple/src/snapshot-route.test.ts +++ b/packages/platform-apple/src/snapshot-route.test.ts @@ -302,6 +302,33 @@ test('typed bridge failure falls back once and disables retries for that app gen }); }); +test('a bridge still being prepared sends only that capture to the runner', async () => { + const source = sourceReturning({ + stage: 'failed', + failure: { kind: 'preparing', code: 'bridge-preparation-pending' }, + }); + const fallback = vi.fn(async () => runnerResult()); + const route = createAppleSnapshotRoute(platformRuntimeHostFixture(), { + source, + resolveTarget: vi.fn(async () => target), + }); + + const first = await route.capture(ios, input, signal(), fallback); + const second = await route.capture(ios, input, signal(), fallback); + + // An attempt in flight says nothing about this app generation, so unlike a failed bridge it must + // not close the circuit: a stable screen would never use the finished preparation. + expect(source.acquire).toHaveBeenCalledTimes(2); + expect(fallback).toHaveBeenCalledTimes(2); + const pending = [ + 'Simulator AX snapshot unavailable (bridge-preparation-pending); used XCTest for this capture while the bridge is still being prepared.', + ]; + expect(first.warnings).toEqual(pending); + // The circuit-disabled sentence would mean the route gave up on the bridge for this generation, + // and the app-generation sentence would mean the same; only this capture moved to the runner. + expect(second.warnings).toEqual(pending); +}); + test('a new app generation re-enables the bridge', async () => { const source = sourceReturning({ stage: 'failed', diff --git a/packages/platform-apple/src/snapshot-route.ts b/packages/platform-apple/src/snapshot-route.ts index 190aff51cc..0a7ea9b7c2 100644 --- a/packages/platform-apple/src/snapshot-route.ts +++ b/packages/platform-apple/src/snapshot-route.ts @@ -49,6 +49,13 @@ const SYSTEM_SURFACE_PRESENTED = 'system-surface-presented'; */ const SYSTEM_SURFACE_HOST_LINGERING = 'system-surface-host-lingering'; +/** + * Why this capture left the bridge: the bridge binary is still being built. The generation stays on + * the bridge path, so the warning has to say the runner served this capture rather than the + * generation (#2491). + */ +const BRIDGE_PREPARATION_PENDING = 'bridge-preparation-pending'; + export type AppleSnapshotRoute = LaunchObservationPort & Readonly<{ capture( @@ -258,7 +265,7 @@ async function fallbackAfterFailure( disabledGenerations: Set, cause?: unknown, ): Promise { - disabledGenerations.add(generationKey(failedTarget)); + disableGenerationFor(failure, failedTarget, disabledGenerations); emitRouteDiagnostic( failure.code, { id: failedTarget.udid }, @@ -277,6 +284,21 @@ async function fallbackAfterFailure( ); } +/** + * A failed bridge is evidence about this app generation, so its captures take the runner until the + * generation is rebaselined. A bridge that is merely still being prepared is evidence about the + * daemon's build queue instead: the same generation must be able to use it as soon as it exists, + * which is what let one cold host cost every later capture of a stable screen (#2491). + */ +function disableGenerationFor( + failure: SnapshotSourceFailure, + failedTarget: SimulatorSnapshotTarget, + disabledGenerations: Set, +): void { + if (failure.kind === 'preparing') return; + disabledGenerations.add(generationKey(failedTarget)); +} + async function runFallback( deviceId: string, input: CaptureSnapshotInput, @@ -420,6 +442,12 @@ function fallbackWarning(reason: string, lineage: IosSnapshotLineage, served: bo if (served) { return `Simulator AX snapshot unavailable (${reason}); used XCTest, which read the system surface presented over the app.`; } + // A bridge that is still being built says nothing about this generation, and the route keeps the + // generation on the bridge path because of it. The generation sentence would claim a retirement + // that did not happen (#2491). + if (reason === BRIDGE_PREPARATION_PENDING) { + return `Simulator AX snapshot unavailable (${reason}); used XCTest for this capture while the bridge is still being prepared.`; + } const generation = lineage.generation ? 'this app generation' : 'an unverified app generation'; return `Simulator AX snapshot unavailable (${reason}); used XCTest for ${generation}.`; } diff --git a/packages/platform-apple/src/snapshot-source/adapter.test.ts b/packages/platform-apple/src/snapshot-source/adapter.test.ts index bdb6e1970b..c4910022af 100644 --- a/packages/platform-apple/src/snapshot-source/adapter.test.ts +++ b/packages/platform-apple/src/snapshot-source/adapter.test.ts @@ -16,7 +16,12 @@ import { SNAPSHOT_SOURCE_PROTOCOL_VERSION, SNAPSHOT_SOURCE_VERSION, } from './protocol.ts'; -import type { SnapshotSourceHost, SnapshotSourceProcess, SnapshotSourceSocket } from './types.ts'; +import type { + SnapshotSourceHost, + SnapshotSourceOutcome, + SnapshotSourceProcess, + SnapshotSourceSocket, +} from './types.ts'; import { mkdtempForTest } from '../__tests__/tmp-dir.ts'; test('the Simulator AX source returns raw acquisition facts and discloses unsupported facets', async () => { @@ -144,8 +149,8 @@ test('the Simulator AX source refuses a tree that ends at content another proces } }); -test('preparation consumes the same acquisition deadline as bridge I/O', async () => { - const root = await mkdtempForTest('agent-device-snapshot-adapter-deadline-'); +test('a capture reports a cold preparation instead of spending its deadline on it', async () => { + const root = await mkdtempForTest('agent-device-snapshot-adapter-preparing-'); const sourceRoot = path.join(root, 'source'); const cacheRoot = path.join(root, 'cache'); await (await import('@agent-device/host-kit/host-file')).ensureHostDirectory(sourceRoot); @@ -154,19 +159,33 @@ test('preparation consumes the same acquisition deadline as bridge I/O', async ( await writeFile(path.join(sourceRoot, 'SnapshotBridgeRuntime.h'), 'native header'); await writeFile(path.join(sourceRoot, 'SnapshotBridgeCapture.h'), 'native header'); await writeFile(path.join(sourceRoot, 'SnapshotBridgeCapture.m'), 'native header'); - const fixture = createAdapterHost(150); + const fixture = createAdapterHost(300); const source = createSimulatorSnapshotSource({ host: fixture.host, sourceRoot, cacheRoot }); - const request = createIosSnapshotRequest(); - const hint = deriveIosCaptureHint(request); + const hint = deriveIosCaptureHint(createIosSnapshotRequest()); + const target = { ...targetForTest(), generation: 'generation-1' }; try { - const outcome = await source.acquire({ - target: { ...targetForTest(), generation: 'generation-1' }, + const startedAt = performance.now(); + let outcome: SnapshotSourceOutcome = await source.acquire({ + target, hint, limits: { maxDurationMs: 100 }, }); + const waitedMs = performance.now() - startedAt; assert.equal(outcome.stage, 'failed'); - if (outcome.stage === 'failed') assert.equal(outcome.failure.kind, 'timeout'); + if (outcome.stage === 'failed') { + assert.equal(outcome.failure.kind, 'preparing'); + assert.equal(outcome.failure.code, 'bridge-preparation-pending'); + } + // The capture ends on its own budget while the attempt it started keeps compiling detached; + // waiting for that build is what let a cold host cancel captures that had a working runner. + assert.ok(waitedMs < 300, `capture waited ${waitedMs}ms for a 300ms build`); + + for (let attempt = 0; attempt < 20 && outcome.stage !== 'acquired'; attempt += 1) { + await new Promise((resolve) => setTimeout(resolve, 50)); + outcome = await source.acquire({ target, hint }); + } + assert.equal(outcome.stage, 'acquired'); assert.equal(fixture.builds, 1); } finally { await source.close(); diff --git a/packages/platform-apple/src/snapshot-source/adapter.ts b/packages/platform-apple/src/snapshot-source/adapter.ts index a45cdab7ab..61e4692732 100644 --- a/packages/platform-apple/src/snapshot-source/adapter.ts +++ b/packages/platform-apple/src/snapshot-source/adapter.ts @@ -4,18 +4,17 @@ import type { IosSnapshotAcquisition, IosViewportEvidence, } from '@agent-device/contracts/ios-snapshot'; -import { ensureSnapshotBridgeBinary } from './cache.ts'; import { createSnapshotSourceDeadline, remainingSnapshotSourceMs } from './deadline.ts'; import { AcceptedDepthHints, type DepthHintDecision } from './depth-hints.ts'; import { asSnapshotSourceError, snapshotSourceError } from './errors.ts'; import { SnapshotBridgeManager } from './lifecycle.ts'; import { resolveSnapshotSourceLimits } from './limits.ts'; import { readSnapshotBridgeRecovery, type SnapshotBridgeEnvelope } from './protocol.ts'; +import { createSnapshotBridgePreparation } from './preparation.ts'; import { decodeSnapshotBridgeTree } from './tree.ts'; import { createSnapshotSourceHost } from './host.ts'; import type { SnapshotSourceHost, - SnapshotSourceBridgeBinary, SnapshotSourceLimits, SnapshotSourceOutcome, SnapshotSourceRequest, @@ -41,36 +40,17 @@ export function createSimulatorSnapshotSource( const host = options.host ?? createSnapshotSourceHost(); const manager = new SnapshotBridgeManager(host); const depthHints = new AcceptedDepthHints(); - const preparedBinaries = new Map(); + // Preparation is daemon-scoped work: it reads the toolchain identity, fingerprints the bridge + // source and may compile it. It runs detached so no capture's deadline pays for it (#2491). + const preparation = createSnapshotBridgePreparation({ + host, + limits: resolveSnapshotSourceLimits(options.limits), + producer: SNAPSHOT_SOURCE_PRODUCER, + sourceRoot: options.sourceRoot, + cacheRoot: options.cacheRoot, + }); let closed = false; - const prepare = async ( - input: Readonly<{ - runtime: string; - limits: SnapshotSourceLimits; - deadline: import('./deadline.ts').SnapshotSourceDeadline; - }>, - ) => { - if (closed) throw snapshotSourceError('unsupported', 'source-closed'); - const prepared = preparedBinaries.get(input.runtime); - if (prepared) return prepared; - const completed = await host.withDiagnosticTimer( - 'ios.snapshot-source.prepare', - async () => - await ensureSnapshotBridgeBinary({ - host, - runtime: input.runtime, - limits: input.limits, - deadline: input.deadline, - sourceRoot: options.sourceRoot, - cacheRoot: options.cacheRoot, - }), - { producer: SNAPSHOT_SOURCE_PRODUCER }, - ); - preparedBinaries.set(input.runtime, completed); - return completed; - }; - const acquire = async (request: SnapshotSourceRequest): Promise => { try { if (closed) throw snapshotSourceError('unsupported', 'source-closed'); @@ -83,11 +63,11 @@ export function createSimulatorSnapshotSource( return await host.withDiagnosticTimer( 'ios.snapshot-source.acquire', async () => { - const bridge = await prepare({ - runtime: request.target.runtime, - limits, + const bridge = await preparation.readyBinary( + request.target.runtime, deadline, - }); + 'bridge-preparation-deadline', + ); const decision = depthHints.consume(request.target, requestedLevels, explicitDepth); const envelope = await manager.request({ target: request.target, @@ -130,6 +110,9 @@ export function createSimulatorSnapshotSource( close: async () => { if (closed) return; closed = true; + // A bridge build that outlives this source answers to nobody: no request is waiting on it and + // its cache write would land after the source is gone (#2491). + preparation.close(); await manager.close(); }, }; diff --git a/packages/platform-apple/src/snapshot-source/errors.ts b/packages/platform-apple/src/snapshot-source/errors.ts index 07a2b64b7d..bd7671c007 100644 --- a/packages/platform-apple/src/snapshot-source/errors.ts +++ b/packages/platform-apple/src/snapshot-source/errors.ts @@ -9,6 +9,7 @@ const APP_ERROR_CODE_BY_KIND: Readonly cancelled: 'COMMAND_FAILED', 'process-crash': 'COMMAND_FAILED', 'transport-failure': 'COMMAND_FAILED', + preparing: 'COMMAND_FAILED', }; export class SnapshotSourceError extends AppError { diff --git a/packages/platform-apple/src/snapshot-source/preparation.test.ts b/packages/platform-apple/src/snapshot-source/preparation.test.ts new file mode 100644 index 0000000000..01efa1ab21 --- /dev/null +++ b/packages/platform-apple/src/snapshot-source/preparation.test.ts @@ -0,0 +1,294 @@ +import assert from 'node:assert/strict'; +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { test } from 'vitest'; +import { createSnapshotSourceDeadline, type SnapshotSourceDeadline } from './deadline.ts'; +import { createSnapshotSourceHost } from './host.ts'; +import { resolveSnapshotSourceLimits } from './limits.ts'; +import { + createSnapshotBridgePreparation, + type SnapshotBridgePreparation, + SNAPSHOT_BRIDGE_PREPARATION_RETRY_AFTER_MS, +} from './preparation.ts'; +import type { SnapshotSourceBridgeBinary, SnapshotSourceHost } from './types.ts'; + +const BRIDGE_SOURCES = [ + 'SnapshotBridge.m', + 'SnapshotBridgeRuntime.m', + 'SnapshotBridgeRuntime.h', + 'SnapshotBridgeCapture.h', + 'SnapshotBridgeCapture.m', +]; + +test('one detached preparation answers every capture that arrives while it runs', async () => { + const root = await writeBridgeSource('preparation-shared-'); + let releaseBuild = () => {}; + const gate = new Promise((resolve) => { + releaseBuild = resolve; + }); + const fixture = createPreparationHost(async () => { + await gate; + return 'built'; + }); + const preparation = createSnapshotBridgePreparation({ + host: fixture.host, + limits: resolveSnapshotSourceLimits({}), + producer: 'test', + sourceRoot: path.join(root, 'source'), + cacheRoot: path.join(root, 'cache'), + }); + + try { + const pending = await Promise.all([ + preparation.readyBinary('ios-simulator', deadline(300), 'test-deadline').then( + () => undefined, + (error: unknown) => failureOf(error), + ), + preparation.readyBinary('ios-simulator', deadline(300), 'test-deadline').then( + () => undefined, + (error: unknown) => failureOf(error), + ), + ]); + for (const failure of pending) { + assert.equal(failure?.kind, 'preparing'); + assert.equal(failure?.code, 'bridge-preparation-pending'); + } + assert.equal(fixture.builds, 1); + + releaseBuild(); + // A capture that arrives after the budget is spent is pointed at the runner rather than made to + // wait again, so this is a poll loop finding the finished bridge, not one waiting it out. + const binary = await readyWithin(preparation, 2_000); + assert.ok(binary.path.startsWith(path.join(root, 'cache'))); + assert.ok(binary.cacheKey.length > 0); + assert.equal(fixture.builds, 1); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test('a failed preparation is reported to each capture without rebuilding per capture', async () => { + const root = await writeBridgeSource('preparation-failed-'); + let clockMs = 0; + const fixture = createPreparationHost(async () => { + // A cold host spends the retry window building before it fails at all, which is what the + // window has to be measured against (#2491). + clockMs += SNAPSHOT_BRIDGE_PREPARATION_RETRY_AFTER_MS + 1_000; + return 'failed'; + }); + const preparation = createSnapshotBridgePreparation({ + host: fixture.host, + limits: resolveSnapshotSourceLimits({}), + producer: 'test', + sourceRoot: path.join(root, 'source'), + cacheRoot: path.join(root, 'cache'), + now: () => clockMs, + }); + + try { + const first = await prepare( + preparation, + deadline(5_000, () => clockMs), + ); + assert.equal(first?.kind, 'unsupported'); + assert.equal(first?.code, 'native-build-failed'); + assert.equal(fixture.builds, 1); + + // The attempt above is long past the window measured from its start; measured from the failure + // it is brand new, and a tight wait-poll loop must not launch a compile every 200 ms. + const again = await prepare( + preparation, + deadline(5_000, () => clockMs), + ); + assert.deepEqual(again, first); + assert.equal(fixture.builds, 1); + + clockMs += SNAPSHOT_BRIDGE_PREPARATION_RETRY_AFTER_MS + 1; + const retried = await prepare( + preparation, + deadline(5_000, () => clockMs), + ); + assert.equal(retried?.code, 'native-build-failed'); + assert.equal(fixture.builds, 2); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test('only the first capture waits out the budget for a cold preparation', async () => { + const root = await writeBridgeSource('preparation-wait-'); + let releaseBuild = () => {}; + const gate = new Promise((resolve) => { + releaseBuild = resolve; + }); + const fixture = createPreparationHost(async () => { + await gate; + return 'built'; + }); + const preparation = createSnapshotBridgePreparation({ + host: fixture.host, + limits: resolveSnapshotSourceLimits({}), + producer: 'test', + sourceRoot: path.join(root, 'source'), + cacheRoot: path.join(root, 'cache'), + }); + + try { + const paying = prepare(preparation, deadline(30_000)); + // The capture that arrives while the first is still waiting is the `wait` poll of a caller that + // has already spent the budget: it is told to use the runner at once, in seconds it can spare. + await delay(150); + const joined = Date.now(); + const second = await prepare(preparation, deadline(30_000)); + assert.equal(second?.code, 'bridge-preparation-pending'); + assert.ok( + Date.now() - joined < 500, + `a capture that arrived while the first was waiting took ${Date.now() - joined}ms`, + ); + assert.equal(fixture.builds, 1); + + releaseBuild(); + // The capture that paid the budget either caught the finished build or went to the runner with + // `preparing`; either answer is fine, what the test pinned is that only one capture paid. + await paying; + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test('closing the preparation stops a build that is still running', async () => { + const root = await writeBridgeSource('preparation-close-'); + const fixture = createPreparationHost(({ signal, attempt }) => { + if (attempt > 1) return Promise.resolve('built'); + // `run` rejects on abort the way the real host does, so the build's own cleanup runs. + return new Promise<'built'>((_resolve, reject) => { + signal?.addEventListener('abort', () => reject(new Error('aborted')), { once: true }); + }); + }); + const preparation = createSnapshotBridgePreparation({ + host: fixture.host, + limits: resolveSnapshotSourceLimits({}), + producer: 'test', + sourceRoot: path.join(root, 'source'), + cacheRoot: path.join(root, 'cache'), + }); + + try { + const pending = await prepare(preparation, deadline(300)); + assert.equal(pending?.code, 'bridge-preparation-pending'); + assert.equal(fixture.signals.at(-1)?.aborted, false); + + preparation.close(); + // Reaching the build is the point: no request owns this attempt any more, and a bridge write + // landing after the source closed would belong to nobody. + assert.equal(fixture.signals.at(-1)?.aborted, true); + + const next = await prepare(preparation, deadline(5_000)); + assert.equal(next, undefined); + assert.equal(fixture.builds, 2); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +async function writeBridgeSource(prefix: string): Promise { + const root = await mkdtemp(path.join(os.tmpdir(), `agent-device-bridge-${prefix}`)); + const sourceRoot = path.join(root, 'source'); + await (await import('@agent-device/host-kit/host-file')).ensureHostDirectory(sourceRoot); + for (const name of BRIDGE_SOURCES) { + await writeFile(path.join(sourceRoot, name), 'native source'); + } + return root; +} + +function deadline(timeoutMs: number, now: () => number = Date.now): SnapshotSourceDeadline { + return createSnapshotSourceDeadline(timeoutMs, undefined, now); +} + +function prepare( + preparation: SnapshotBridgePreparation, + captureDeadline: SnapshotSourceDeadline, +): Promise | undefined> { + return preparation.readyBinary('ios-simulator', captureDeadline, 'test-deadline').then( + () => undefined, + (error: unknown) => failureOf(error), + ); +} + +/** + * Reads the preparation the way a `wait` poll does: a capture that was pointed at the runner comes + * back later, when the bridge may exist. + */ +async function readyWithin( + preparation: SnapshotBridgePreparation, + timeoutMs: number, +): Promise { + const until = Date.now() + timeoutMs; + for (;;) { + try { + return await preparation.readyBinary('ios-simulator', deadline(300), 'test-deadline'); + } catch (error) { + if (Date.now() >= until || failureOf(error)?.code !== 'bridge-preparation-pending') { + throw error; + } + await delay(20); + } + } +} + +function delay(ms: number): Promise { + return new Promise((resolve) => { + setTimeout(resolve, ms); + }); +} + +function failureOf(error: unknown): Readonly<{ kind: string; code: string }> | undefined { + const failure = (error as Readonly<{ failureKind?: unknown; failureCode?: unknown }>) ?? {}; + return typeof failure.failureKind === 'string' && typeof failure.failureCode === 'string' + ? { kind: failure.failureKind, code: failure.failureCode } + : undefined; +} + +function createPreparationHost( + build: ( + context: Readonly<{ signal: AbortSignal | undefined; attempt: number }>, + ) => Promise<'built' | 'failed'>, +): Readonly<{ + host: SnapshotSourceHost; + builds: number; + signals: (AbortSignal | undefined)[]; +}> { + const fixture: { + host: SnapshotSourceHost; + builds: number; + signals: (AbortSignal | undefined)[]; + } = { host: undefined as never, builds: 0, signals: [] }; + const host: SnapshotSourceHost = { + ...createSnapshotSourceHost(), + run: async (command, args, options) => { + if (command === 'xcrun' && args.includes('clang')) { + fixture.builds += 1; + fixture.signals.push(options?.signal); + const outcome = await build({ signal: options?.signal, attempt: fixture.builds }); + if (outcome === 'failed') return { stdout: '', stderr: 'boom', exitCode: 1 }; + await writeFile(args.at(-1)!, 'bridge-binary'); + return { stdout: '', stderr: '', exitCode: 0 }; + } + return { + stdout: + command === 'xcodebuild' + ? 'Xcode 16.4\nBuild version 16F6' + : command === 'sw_vers' + ? '15.6' + : command === 'uname' + ? 'arm64' + : '26.2', + stderr: '', + exitCode: 0, + }; + }, + }; + fixture.host = host; + return fixture; +} diff --git a/packages/platform-apple/src/snapshot-source/preparation.ts b/packages/platform-apple/src/snapshot-source/preparation.ts new file mode 100644 index 0000000000..1be75b51f7 --- /dev/null +++ b/packages/platform-apple/src/snapshot-source/preparation.ts @@ -0,0 +1,131 @@ +import { BUILD_TIMEOUT_MS, ensureSnapshotBridgeBinary } from './cache.ts'; +import { COLD_TOOLCHAIN_PROBE_TIMEOUT_MS } from '../runner/apple-runner-platform.ts'; +import { createDetachedAttempts } from '../detached-attempt.ts'; +import { + createSnapshotSourceDeadline, + waitForSnapshotSourceDelay, + type SnapshotSourceDeadline, +} from './deadline.ts'; +import { asSnapshotSourceError, snapshotSourceError } from './errors.ts'; +import type { + SnapshotSourceBridgeBinary, + SnapshotSourceHost, + SnapshotSourceLimits, +} from './types.ts'; + +/** + * Ceiling on one detached preparation, sized from the two costs it can honestly pay: a cold Apple + * toolchain probe stall on a fresh host (`COLD_TOOLCHAIN_PROBE_TIMEOUT_MS`, #2422) and one clang + * build (`BUILD_TIMEOUT_MS`). No capture waits this long; it serves from the XCTest runner instead. + */ +const SNAPSHOT_BRIDGE_PREPARATION_DEADLINE_MS = COLD_TOOLCHAIN_PROBE_TIMEOUT_MS + BUILD_TIMEOUT_MS; + +/** + * How long one capture may wait for a preparation before the XCTest runner serves it. Measured on a + * warm cache, the first preparation in a fresh daemon process costs 1.5 s here — mostly the first + * in-process Apple toolchain probe — and 0.1 s after that, so this budget keeps a cached bridge on + * the bridge path. What it cannot cover is a cold host: the probe stall of #2422 or a clang build is + * host-setup cost, and that belongs to the daemon rather than to one capture (#2491). + * + * Granted to the first capture that finds an attempt running, not to each of them: a `wait` poll + * cycles about every 200 ms, and a budget paid per poll would cost a cold build ten times its own + * cost in captured polls that all end up on the runner anyway. + */ +const SNAPSHOT_BRIDGE_PREPARATION_WAIT_BUDGET_MS = 2_000; + +/** + * A failed attempt is reported as-is for this long instead of starting another build per capture. + * The retry is what recovers a one-off probe timeout; the window is what stops a tight wait-poll + * loop from launching a compile every 200 ms. Measured from the failure, because a cold host can + * spend the whole window building before it fails at all. + */ +export const SNAPSHOT_BRIDGE_PREPARATION_RETRY_AFTER_MS = 60_000; + +export type SnapshotBridgePreparation = Readonly<{ + /** + * The bridge binary for `runtime`, starting a detached preparation when none is running. An + * attempt still running when `deadline` has spent its wait budget is reported as `preparing` and + * keeps running: the caller serves that capture another way instead of paying for it. + */ + readyBinary( + runtime: string, + deadline: SnapshotSourceDeadline, + code: string, + ): Promise; + /** Stops a build that is still running. Called when the snapshot source closes. */ + close(): void; +}>; + +export function createSnapshotBridgePreparation( + deps: Readonly<{ + host: SnapshotSourceHost; + limits: SnapshotSourceLimits; + producer: string; + sourceRoot?: string; + cacheRoot?: string; + now?: () => number; + }>, +): SnapshotBridgePreparation { + const now = deps.now ?? Date.now; + // The binary a finished preparation produced. Content-addressed and verified at rest, so it is + // good for the life of this source; re-deriving it per capture would re-hash the bridge source + // every time. This is the cache the attempt table deliberately does not keep. + const binaries = new Map(); + const attempts = createDetachedAttempts({ + waitMs: SNAPSHOT_BRIDGE_PREPARATION_WAIT_BUDGET_MS, + waitGrant: 'first-caller', + retryAfterMs: SNAPSHOT_BRIDGE_PREPARATION_RETRY_AFTER_MS, + now, + }); + + const build = async ( + runtime: string, + signal: AbortSignal, + ): Promise => { + // The preparation carries the signal `close()` aborts: once a capture has answered there is no + // request deadline left to stop this build, and a bridge write landing after the source closed + // belongs to nobody (#2491). + const deadline = createSnapshotSourceDeadline( + SNAPSHOT_BRIDGE_PREPARATION_DEADLINE_MS, + signal, + now, + ); + try { + const binary = await deps.host.withDiagnosticTimer( + 'ios.snapshot-source.prepare', + async () => + await ensureSnapshotBridgeBinary({ + host: deps.host, + runtime, + limits: deps.limits, + deadline, + sourceRoot: deps.sourceRoot, + cacheRoot: deps.cacheRoot, + }), + { producer: deps.producer }, + ); + binaries.set(runtime, binary); + return binary; + } catch (error) { + throw asSnapshotSourceError(error); + } + }; + + return { + readyBinary: async (runtime, deadline, code) => { + const binary = binaries.get(runtime); + if (binary) return binary; + return await attempts.value(runtime, { + start: (signal) => build(runtime, signal), + // Waiting inside the request's own deadline keeps a genuine client abort typed + // `cancelled`: only a preparation that is simply still running is reported as `preparing`. + wait: (waitMs) => waitForSnapshotSourceDelay(deadline, waitMs, code), + pending: () => snapshotSourceError('preparing', 'bridge-preparation-pending'), + }); + }, + close: () => { + binaries.clear(); + attempts.close(); + }, + }; +} diff --git a/packages/platform-apple/src/snapshot-source/types.ts b/packages/platform-apple/src/snapshot-source/types.ts index ef62a3a024..9380fca885 100644 --- a/packages/platform-apple/src/snapshot-source/types.ts +++ b/packages/platform-apple/src/snapshot-source/types.ts @@ -43,7 +43,9 @@ export type SnapshotSourceFailureKind = | 'timeout' | 'cancelled' | 'process-crash' - | 'transport-failure'; + | 'transport-failure' + /** The bridge binary is still being prepared by a detached attempt; nothing failed. */ + | 'preparing'; export type SnapshotSourceFailure = Readonly<{ kind: SnapshotSourceFailureKind; diff --git a/packages/platform-apple/src/snapshot-target.ts b/packages/platform-apple/src/snapshot-target.ts index fdd145ffd8..e9b0391ba6 100644 --- a/packages/platform-apple/src/snapshot-target.ts +++ b/packages/platform-apple/src/snapshot-target.ts @@ -1,5 +1,6 @@ import type { DeviceInfo } from '@agent-device/kernel/device'; import { AppError } from '@agent-device/kernel/errors'; +import { createDetachedAttempts } from './detached-attempt.ts'; import { runSimctl } from './core/apps-simctl.ts'; import { readSnapshotTargetProcessStartTime } from './snapshot-process.ts'; @@ -33,8 +34,13 @@ export type SimulatorSnapshotTargetResolver = ( export function createSimulatorSnapshotTargetResolver(): SimulatorSnapshotTargetResolver { const targets = new Map(); - const discoveries = new Map>(); const runtimeByDevice = new Map>(); + // One discovery per target at a time, detached from the caller's signal: a capture that gives up + // on it, or is cancelled, must not take it down. A discovery that fails is forgotten, so the next + // capture starts a new one. + const discoveries = createDetachedAttempts({ + waitMs: TARGET_DISCOVERY_WAIT_MS, + }); return async (device, appBundleId, signal, refresh) => { signal.throwIfAborted(); const key = `${device.id}:${appBundleId}`; @@ -47,46 +53,33 @@ export function createSimulatorSnapshotTargetResolver(): SimulatorSnapshotTarget if (observed === cached.processStartTime) return cached; } targets.delete(key); - let discovery = discoveries.get(key); - if (!discovery) { - // One discovery per target at a time, detached from the caller's signal: a capture that - // gives up on it, or is cancelled, must not take it down. A discovery that fails is - // forgotten, so the next capture starts a new one. - discovery = resolveSimulatorSnapshotTarget(device, appBundleId, runtimeByDevice) - .then((target) => { - targets.set(key, target); - return target; - }) - .finally(() => discoveries.delete(key)); - discovery.catch(() => undefined); - discoveries.set(key, discovery); - } - return await awaitDiscovery(discovery, signal, device, appBundleId); + return await discoveries.value(key, { + start: async () => { + const target = await resolveSimulatorSnapshotTarget(device, appBundleId, runtimeByDevice); + targets.set(key, target); + return target; + }, + wait: (waitMs) => waitForDiscoveryAttempt(waitMs, signal), + pending: () => targetError('simulator-target-discovery-pending', device, appBundleId), + }); }; } -async function awaitDiscovery( - discovery: Promise, - signal: AbortSignal, - device: DeviceInfo, - appBundleId: string, -): Promise { - let timer: ReturnType | undefined; - let onAbort: (() => void) | undefined; - const bound = new Promise((_, reject) => { - timer = setTimeout( - () => reject(targetError('simulator-target-discovery-pending', device, appBundleId)), - TARGET_DISCOVERY_WAIT_MS, - ); - onAbort = () => reject(signal.reason); +/** + * One caller's wait for a discovery it did not start. Resolving is the wait being spent, not the + * discovery failing: a client abort rejects with its own reason so it stays typed `cancelled`. + */ +function waitForDiscoveryAttempt(waitMs: number, signal: AbortSignal): Promise { + return new Promise((resolve, reject) => { + const onAbort = () => finish(() => reject(signal.reason)); + const timer = setTimeout(() => finish(resolve), waitMs); + function finish(settle: () => void): void { + clearTimeout(timer); + signal.removeEventListener('abort', onAbort); + settle(); + } signal.addEventListener('abort', onAbort, { once: true }); }); - try { - return await Promise.race([discovery, bound]); - } finally { - clearTimeout(timer); - if (onAbort) signal.removeEventListener('abort', onAbort); - } } async function resolveSimulatorSnapshotTarget( From a704d2e8e3680c46c5ecb4359b8234a0d97af49a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 14:36:08 +0200 Subject: [PATCH 2/8] fix(ios-snapshot): end a detached wait the answer already settled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `value()` raced the attempt against the caller's wait and left the loser running. A capture answered by the attempt still held its timer and abort listener until the budget ran out — 1.5 s for discovery, 2 s for preparation — so a poll loop could stack one pending timer and one listener per capture. The `awaitDiscovery` it replaced cleared both in a `finally`. `wait` now takes a per-call stop signal that `value()` aborts in a `finally`, and both owners release their timer and listener on it: `waitForDiscoveryAttempt` resolves, and `waitForSnapshotSourceDelay` grows an optional `stop` that ends the sleep without spending the deadline it is measured against. Waiting inside the caller's own deadline still matters, so a client abort keeps rejecting with the typed cancellation; the stop only ever lands after the race is already decided, which is why the losing wait resolves rather than rejecting, and why the race keeps a handler of its own. --- .../src/detached-attempt.test.ts | 39 ++++++++++++++++ .../platform-apple/src/detached-attempt.ts | 16 ++++++- .../src/snapshot-source/deadline.test.ts | 46 +++++++++++++++++++ .../src/snapshot-source/deadline.ts | 10 ++++ .../src/snapshot-source/preparation.ts | 2 +- .../platform-apple/src/snapshot-target.ts | 15 ++++-- 6 files changed, 122 insertions(+), 6 deletions(-) create mode 100644 packages/platform-apple/src/snapshot-source/deadline.test.ts diff --git a/packages/platform-apple/src/detached-attempt.test.ts b/packages/platform-apple/src/detached-attempt.test.ts index edd1f2835b..a61f6388c4 100644 --- a/packages/platform-apple/src/detached-attempt.test.ts +++ b/packages/platform-apple/src/detached-attempt.test.ts @@ -210,6 +210,45 @@ test('an attempt that throws before awaiting is reported as its own failure', as ); }); +test('a caller answered by the attempt stops the wait it left running', async () => { + const attempts = createDetachedAttempts({ waitMs: 60_000 }); + let releaseBuild: (value: string) => void = () => {}; + const building = new Promise((resolve) => { + releaseBuild = resolve; + }); + let stopSignal: AbortSignal | undefined; + let waitState: 'pending' | 'resolved' = 'pending'; + + const caller = attempts.value('bridge', { + start: () => building, + // Deliberately has no timer of its own: only the stop can end it, so a wait that is never + // stopped stays observable as `pending` instead of quietly expiring. + wait: (_waitMs, stop) => { + stopSignal = stop; + return new Promise((resolve) => { + stop.addEventListener( + 'abort', + () => { + waitState = 'resolved'; + resolve(); + }, + { once: true }, + ); + }); + }, + pending: () => PENDING, + }); + + await settle(); + assert.equal(waitState, 'pending'); + releaseBuild('binary'); + assert.equal(await caller, 'binary'); + await settle(); + + assert.ok(stopSignal?.aborted, 'the losing wait is stopped once the attempt settles'); + assert.equal(waitState, 'resolved'); +}); + function identity(error: unknown): unknown { return error; } diff --git a/packages/platform-apple/src/detached-attempt.ts b/packages/platform-apple/src/detached-attempt.ts index 4ef72d2e69..45fed6a596 100644 --- a/packages/platform-apple/src/detached-attempt.ts +++ b/packages/platform-apple/src/detached-attempt.ts @@ -30,8 +30,11 @@ export type DetachedAttempts = Readonly<{ /** * Sleeps inside the caller's own deadline, so a client abort rejects this call with the * caller's typed cancellation instead of reporting a fresh `pending`. + * + * `stop` is aborted as soon as this caller has its answer from somewhere else; the wait has to + * release its timer and listeners then and resolve, since nobody is racing it any more. */ - wait: (waitMs: number) => Promise; + wait: (waitMs: number, stop: AbortSignal) => Promise; pending: () => Error; }>, ): Promise; @@ -136,7 +139,16 @@ export function createDetachedAttempts( if (attempt.status === 'pending' && (waitGrant === 'every-caller' || !attempt.waitSpent)) { // Marked before waiting: two captures arriving together must not each spend the budget. attempt.waitSpent = true; - await Promise.race([attempt.settled, params.wait(deps.waitMs)]); + const stopWaiting = new AbortController(); + const waiting = params.wait(deps.waitMs, stopWaiting.signal); + // A wait can still reject after it lost the race, e.g. a client aborting in the moment + // between the attempt settling and this call returning; nobody awaits it by then. + waiting.catch(() => {}); + try { + await Promise.race([attempt.settled, waiting]); + } finally { + stopWaiting.abort(); + } } if (attempt.status === 'ready') return attempt.value; if (attempt.status === 'failed') throw attempt.error; diff --git a/packages/platform-apple/src/snapshot-source/deadline.test.ts b/packages/platform-apple/src/snapshot-source/deadline.test.ts new file mode 100644 index 0000000000..8c3df4425f --- /dev/null +++ b/packages/platform-apple/src/snapshot-source/deadline.test.ts @@ -0,0 +1,46 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import { createSnapshotSourceDeadline, waitForSnapshotSourceDelay } from './deadline.ts'; +import { SnapshotSourceError } from './errors.ts'; + +const WAIT_CODE = 'bridge-preparation-pending'; + +test('a stopped delay returns without spending the rest of the deadline', async () => { + const stop = new AbortController(); + const deadline = createSnapshotSourceDeadline(60_000, undefined); + const waiting = waitForSnapshotSourceDelay(deadline, 60_000, WAIT_CODE, stop.signal); + + const startedAt = Date.now(); + stop.abort(); + await waiting; + assert.ok(Date.now() - startedAt < 5_000, 'a stopped wait does not sleep out its budget'); +}); + +test('a delay started with its stop already aborted does not sleep', async () => { + const stop = new AbortController(); + stop.abort(); + const deadline = createSnapshotSourceDeadline(60_000, undefined); + + const startedAt = Date.now(); + await waitForSnapshotSourceDelay(deadline, 60_000, WAIT_CODE, stop.signal); + assert.ok(Date.now() - startedAt < 5_000); +}); + +test('an aborted caller signal stays typed cancellation next to a stop', async () => { + const caller = new AbortController(); + const deadline = createSnapshotSourceDeadline(60_000, caller.signal); + const waiting = waitForSnapshotSourceDelay( + deadline, + 60_000, + WAIT_CODE, + new AbortController().signal, + ); + + caller.abort(); + await assert.rejects(waiting, (error: unknown) => { + assert.ok(error instanceof SnapshotSourceError); + assert.equal(error.failureKind, 'cancelled'); + assert.equal(error.failureCode, 'abort-signal'); + return true; + }); +}); diff --git a/packages/platform-apple/src/snapshot-source/deadline.ts b/packages/platform-apple/src/snapshot-source/deadline.ts index 0e95de0ef4..05d9465e82 100644 --- a/packages/platform-apple/src/snapshot-source/deadline.ts +++ b/packages/platform-apple/src/snapshot-source/deadline.ts @@ -24,10 +24,16 @@ export function remainingSnapshotSourceMs(deadline: SnapshotSourceDeadline, code return Math.max(1, Math.floor(remainingMs)); } +/** + * Sleeps inside the caller's own deadline. `stop` is for a caller that no longer needs the sleep + * because the work it was waiting on answered elsewhere: the delay resolves instead of burning its + * remaining budget, while an aborted `deadline` stays a typed `cancelled`. + */ export async function waitForSnapshotSourceDelay( deadline: SnapshotSourceDeadline, requestedMs: number, code: string, + stop?: AbortSignal, ): Promise { const delayMs = Math.min(requestedMs, remainingSnapshotSourceMs(deadline, code)); await new Promise((resolve, reject) => { @@ -36,14 +42,18 @@ export async function waitForSnapshotSourceDelay( const onAbort = () => { finish(() => reject(snapshotSourceError('cancelled', 'abort-signal'))); }; + const onStop = () => finish(resolve); const finish = (action: () => void) => { if (settled) return; settled = true; clearTimeout(timer); deadline.signal?.removeEventListener('abort', onAbort); + stop?.removeEventListener('abort', onStop); action(); }; + if (stop?.aborted) return finish(resolve); deadline.signal?.addEventListener('abort', onAbort, { once: true }); if (deadline.signal?.aborted) onAbort(); + stop?.addEventListener('abort', onStop, { once: true }); }); } diff --git a/packages/platform-apple/src/snapshot-source/preparation.ts b/packages/platform-apple/src/snapshot-source/preparation.ts index 1be75b51f7..1126c951f1 100644 --- a/packages/platform-apple/src/snapshot-source/preparation.ts +++ b/packages/platform-apple/src/snapshot-source/preparation.ts @@ -119,7 +119,7 @@ export function createSnapshotBridgePreparation( start: (signal) => build(runtime, signal), // Waiting inside the request's own deadline keeps a genuine client abort typed // `cancelled`: only a preparation that is simply still running is reported as `preparing`. - wait: (waitMs) => waitForSnapshotSourceDelay(deadline, waitMs, code), + wait: (waitMs, stop) => waitForSnapshotSourceDelay(deadline, waitMs, code, stop), pending: () => snapshotSourceError('preparing', 'bridge-preparation-pending'), }); }, diff --git a/packages/platform-apple/src/snapshot-target.ts b/packages/platform-apple/src/snapshot-target.ts index e9b0391ba6..3836892f85 100644 --- a/packages/platform-apple/src/snapshot-target.ts +++ b/packages/platform-apple/src/snapshot-target.ts @@ -59,7 +59,7 @@ export function createSimulatorSnapshotTargetResolver(): SimulatorSnapshotTarget targets.set(key, target); return target; }, - wait: (waitMs) => waitForDiscoveryAttempt(waitMs, signal), + wait: (waitMs, stop) => waitForDiscoveryAttempt(waitMs, signal, stop), pending: () => targetError('simulator-target-discovery-pending', device, appBundleId), }); }; @@ -67,18 +67,27 @@ export function createSimulatorSnapshotTargetResolver(): SimulatorSnapshotTarget /** * One caller's wait for a discovery it did not start. Resolving is the wait being spent, not the - * discovery failing: a client abort rejects with its own reason so it stays typed `cancelled`. + * discovery failing: a client abort rejects with its own reason so it stays typed `cancelled`, while + * `stop` means this caller already has its answer and is only letting go of the timer. */ -function waitForDiscoveryAttempt(waitMs: number, signal: AbortSignal): Promise { +function waitForDiscoveryAttempt( + waitMs: number, + signal: AbortSignal, + stop: AbortSignal, +): Promise { return new Promise((resolve, reject) => { const onAbort = () => finish(() => reject(signal.reason)); + const onStop = () => finish(resolve); const timer = setTimeout(() => finish(resolve), waitMs); function finish(settle: () => void): void { clearTimeout(timer); signal.removeEventListener('abort', onAbort); + stop.removeEventListener('abort', onStop); settle(); } + if (stop.aborted) return finish(resolve); signal.addEventListener('abort', onAbort, { once: true }); + stop.addEventListener('abort', onStop, { once: true }); }); } From 39c221478fc1e000420fea9e7b40f67016324cfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 15:46:59 +0200 Subject: [PATCH 3/8] test(ios-snapshot): prove the discovery wait stops when the discovery settles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stop path inside `waitForDiscoveryAttempt` had no test: the listener registration, the listener removal and the already-aborted check could each be deleted with every suite green. `snapshot-target.test.ts` now runs a deferred spawn through `createSimulatorSnapshotTargetResolver`, lets a second caller join the pending discovery, settles it, and asserts on the two things a leaked wait costs — a timer still pending and a listener still on the caller's `AbortSignal` — with fake timers, so neither is a timing race. `deadline.test.ts` measures the same two properties for the preparation owner and drops its already-aborted case. That case was one of the things the review asked about, and it is unreachable rather than untested: `value()` creates the stop moments before calling and aborts it in a `finally`, so the check can never be true and the stop's `{ once: true }` listener is always released by that abort. Both waits now drop the check and the redundant removal, and keep the cleanup that does matter, which is the caller's own signal: it outlives the wait, and forgetting it would leave a listener per capture. --- .../src/snapshot-source/deadline.test.ts | 69 ++++++++++--------- .../src/snapshot-source/deadline.ts | 6 +- .../src/snapshot-target.test.ts | 31 +++++++++ .../platform-apple/src/snapshot-target.ts | 6 +- 4 files changed, 75 insertions(+), 37 deletions(-) diff --git a/packages/platform-apple/src/snapshot-source/deadline.test.ts b/packages/platform-apple/src/snapshot-source/deadline.test.ts index 8c3df4425f..2d9341f593 100644 --- a/packages/platform-apple/src/snapshot-source/deadline.test.ts +++ b/packages/platform-apple/src/snapshot-source/deadline.test.ts @@ -1,46 +1,49 @@ -import assert from 'node:assert/strict'; -import { test } from 'vitest'; +import { expect, test, vi } from 'vitest'; import { createSnapshotSourceDeadline, waitForSnapshotSourceDelay } from './deadline.ts'; import { SnapshotSourceError } from './errors.ts'; const WAIT_CODE = 'bridge-preparation-pending'; test('a stopped delay returns without spending the rest of the deadline', async () => { - const stop = new AbortController(); - const deadline = createSnapshotSourceDeadline(60_000, undefined); - const waiting = waitForSnapshotSourceDelay(deadline, 60_000, WAIT_CODE, stop.signal); + vi.useFakeTimers(); + try { + const stop = new AbortController(); + const deadline = createSnapshotSourceDeadline(60_000, undefined); + let settled = false; + const waiting = waitForSnapshotSourceDelay(deadline, 60_000, WAIT_CODE, stop.signal).then( + () => { + settled = true; + }, + ); - const startedAt = Date.now(); - stop.abort(); - await waiting; - assert.ok(Date.now() - startedAt < 5_000, 'a stopped wait does not sleep out its budget'); -}); - -test('a delay started with its stop already aborted does not sleep', async () => { - const stop = new AbortController(); - stop.abort(); - const deadline = createSnapshotSourceDeadline(60_000, undefined); + await vi.advanceTimersByTimeAsync(0); + expect(settled).toBe(false); + stop.abort(); + await waiting; - const startedAt = Date.now(); - await waitForSnapshotSourceDelay(deadline, 60_000, WAIT_CODE, stop.signal); - assert.ok(Date.now() - startedAt < 5_000); + expect(settled).toBe(true); + expect(vi.getTimerCount()).toBe(0); + } finally { + vi.useRealTimers(); + } }); test('an aborted caller signal stays typed cancellation next to a stop', async () => { - const caller = new AbortController(); - const deadline = createSnapshotSourceDeadline(60_000, caller.signal); - const waiting = waitForSnapshotSourceDelay( - deadline, - 60_000, - WAIT_CODE, - new AbortController().signal, - ); + vi.useFakeTimers(); + try { + const caller = new AbortController(); + const stop = new AbortController(); + const deadline = createSnapshotSourceDeadline(60_000, caller.signal); + const waiting = waitForSnapshotSourceDelay(deadline, 60_000, WAIT_CODE, stop.signal); - caller.abort(); - await assert.rejects(waiting, (error: unknown) => { - assert.ok(error instanceof SnapshotSourceError); - assert.equal(error.failureKind, 'cancelled'); - assert.equal(error.failureCode, 'abort-signal'); - return true; - }); + caller.abort(); + await expect(waiting).rejects.toBeInstanceOf(SnapshotSourceError); + await expect(waiting).rejects.toMatchObject({ + failureKind: 'cancelled', + failureCode: 'abort-signal', + }); + expect(vi.getTimerCount()).toBe(0); + } finally { + vi.useRealTimers(); + } }); diff --git a/packages/platform-apple/src/snapshot-source/deadline.ts b/packages/platform-apple/src/snapshot-source/deadline.ts index 05d9465e82..c1d9eef5d2 100644 --- a/packages/platform-apple/src/snapshot-source/deadline.ts +++ b/packages/platform-apple/src/snapshot-source/deadline.ts @@ -28,6 +28,10 @@ export function remainingSnapshotSourceMs(deadline: SnapshotSourceDeadline, code * Sleeps inside the caller's own deadline. `stop` is for a caller that no longer needs the sleep * because the work it was waiting on answered elsewhere: the delay resolves instead of burning its * remaining budget, while an aborted `deadline` stays a typed `cancelled`. + * + * A stop is only ever created by the code that calls this and is always aborted by it afterwards, + * so it needs no already-aborted check and no listener removal; the deadline's signal is the + * caller's and does. */ export async function waitForSnapshotSourceDelay( deadline: SnapshotSourceDeadline, @@ -48,10 +52,8 @@ export async function waitForSnapshotSourceDelay( settled = true; clearTimeout(timer); deadline.signal?.removeEventListener('abort', onAbort); - stop?.removeEventListener('abort', onStop); action(); }; - if (stop?.aborted) return finish(resolve); deadline.signal?.addEventListener('abort', onAbort, { once: true }); if (deadline.signal?.aborted) onAbort(); stop?.addEventListener('abort', onStop, { once: true }); diff --git a/packages/platform-apple/src/snapshot-target.test.ts b/packages/platform-apple/src/snapshot-target.test.ts index 061684f879..553ae4f857 100644 --- a/packages/platform-apple/src/snapshot-target.test.ts +++ b/packages/platform-apple/src/snapshot-target.test.ts @@ -1,3 +1,4 @@ +import { listenerCount } from 'node:events'; import { expect, test, vi } from 'vitest'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { createLocalAppleToolProvider, withAppleToolProvider } from './core/tool-provider.ts'; @@ -140,6 +141,11 @@ test('an aborted request cannot reuse a cached target', async () => { }); }); +/** Node counts EventTarget listeners through the same entry point it uses for emitters. */ +function abortListeners(signal: AbortSignal): number { + return listenerCount(signal as unknown as Parameters[0], 'abort'); +} + function deferredSpawn(fixture: ReturnType) { let release!: () => void; const released = new Promise((resolve) => { @@ -263,3 +269,28 @@ test('a failed runtime probe does not release the slot while the launch-job prob vi.useRealTimers(); } }); + +test('a discovery that settles leaves no timer or abort listener behind for its waiters', async () => { + const fixture = targetFixture(); + const release = deferredSpawn(fixture); + vi.useFakeTimers(); + try { + await withAppleToolProvider(fixture.provider, async () => { + const starting = new AbortController(); + const joining = new AbortController(); + const first = fixture.resolve(ios, app, starting.signal); + const second = fixture.resolve(ios, app, joining.signal); + await vi.advanceTimersByTimeAsync(0); + + release(); + await vi.advanceTimersByTimeAsync(0); + await Promise.all([first, second]); + + expect(abortListeners(starting.signal)).toBe(0); + expect(abortListeners(joining.signal)).toBe(0); + expect(vi.getTimerCount()).toBe(0); + }); + } finally { + vi.useRealTimers(); + } +}); diff --git a/packages/platform-apple/src/snapshot-target.ts b/packages/platform-apple/src/snapshot-target.ts index 3836892f85..c9c116da18 100644 --- a/packages/platform-apple/src/snapshot-target.ts +++ b/packages/platform-apple/src/snapshot-target.ts @@ -69,6 +69,10 @@ export function createSimulatorSnapshotTargetResolver(): SimulatorSnapshotTarget * One caller's wait for a discovery it did not start. Resolving is the wait being spent, not the * discovery failing: a client abort rejects with its own reason so it stays typed `cancelled`, while * `stop` means this caller already has its answer and is only letting go of the timer. + * + * The stop needs no cleanup here and no already-aborted check: `value()` creates it moments before + * calling this and aborts it in a `finally`, so the listener is gone once that abort fires. The + * caller's signal outlives this wait and does need its listener removed. */ function waitForDiscoveryAttempt( waitMs: number, @@ -82,10 +86,8 @@ function waitForDiscoveryAttempt( function finish(settle: () => void): void { clearTimeout(timer); signal.removeEventListener('abort', onAbort); - stop.removeEventListener('abort', onStop); settle(); } - if (stop.aborted) return finish(resolve); signal.addEventListener('abort', onAbort, { once: true }); stop.addEventListener('abort', onStop, { once: true }); }); From 768e4ba0143657fb0f09794e44f99d7ab71d3b5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 17:26:15 +0200 Subject: [PATCH 4/8] test(ios-snapshot): count the discovery wait's listeners the way the type says --- packages/platform-apple/src/snapshot-target.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/platform-apple/src/snapshot-target.test.ts b/packages/platform-apple/src/snapshot-target.test.ts index 553ae4f857..5d50ac9ff2 100644 --- a/packages/platform-apple/src/snapshot-target.test.ts +++ b/packages/platform-apple/src/snapshot-target.test.ts @@ -1,4 +1,4 @@ -import { listenerCount } from 'node:events'; +import { getEventListeners } from 'node:events'; import { expect, test, vi } from 'vitest'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { createLocalAppleToolProvider, withAppleToolProvider } from './core/tool-provider.ts'; @@ -141,9 +141,9 @@ test('an aborted request cannot reuse a cached target', async () => { }); }); -/** Node counts EventTarget listeners through the same entry point it uses for emitters. */ +/** The abort listeners still attached to a caller's signal, once its call has returned. */ function abortListeners(signal: AbortSignal): number { - return listenerCount(signal as unknown as Parameters[0], 'abort'); + return getEventListeners(signal, 'abort').length; } function deferredSpawn(fixture: ReturnType) { From 0b6103724f1d8835637d06b4e79ac970cf26d6db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 17:28:31 +0200 Subject: [PATCH 5/8] refactor(ios-snapshot): build a detached wait once, and read the retirement rule where it applies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three reviewers in a row had to be satisfied about the same promise plumbing because it existed twice: the discovery wait and the bridge-preparation wait each hand-rolled the timer, the caller's abort listener, the stop listener and the cleanup that keeps a leak from costing a timer and a listener per capture. `waitForDetachedAttempt` now implements the wait that `value()`'s contract describes, and both owners hand it their own sleep length and their own cancellation error — which also means the invariant is deleted-and-caught in one place instead of two. `disableGenerationFor` said one thing about one call site, so the rule moves inline next to the set it edits: a failed bridge retires the app generation, a bridge that is merely still building does not. --- .../platform-apple/src/detached-attempt.ts | 35 ++++++++++++++++++ packages/platform-apple/src/snapshot-route.ts | 21 +++-------- .../src/snapshot-source/deadline.ts | 36 +++++++------------ .../platform-apple/src/snapshot-target.ts | 33 ++--------------- 4 files changed, 55 insertions(+), 70 deletions(-) diff --git a/packages/platform-apple/src/detached-attempt.ts b/packages/platform-apple/src/detached-attempt.ts index 45fed6a596..3c63488df3 100644 --- a/packages/platform-apple/src/detached-attempt.ts +++ b/packages/platform-apple/src/detached-attempt.ts @@ -63,6 +63,41 @@ type Attempt = { settled: Promise; }; +/** + * The wait an owner hands to `value()`, built once so the promise every owner needs is the promise + * this module describes: sleeps `waitMs`, resolves when that sleep is spent or when `stop` says the + * answer arrived elsewhere, and rejects only on the caller's own abort so that stays typed. + * + * `stop` needs neither cleanup nor an already-aborted check: `value()` creates it moments before + * calling and aborts it in a `finally`, which releases the `{ once: true }` listener. The caller's + * signal outlives the wait and does have its listener removed. + */ +export function waitForDetachedAttempt( + params: Readonly<{ + waitMs: number; + /** The caller's own deadline signal; a wait inside it keeps a client abort a client abort. */ + signal: AbortSignal | undefined; + stop: AbortSignal; + /** The rejection for the caller aborting, so each owner keeps its own error type. */ + cancelled: () => unknown; + }>, +): Promise { + const { waitMs, signal, stop, cancelled } = params; + return new Promise((resolve, reject) => { + const onAbort = () => finish(() => reject(cancelled())); + const onStop = () => finish(resolve); + const timer = setTimeout(() => finish(resolve), waitMs); + function finish(settle: () => void): void { + clearTimeout(timer); + signal?.removeEventListener('abort', onAbort); + settle(); + } + signal?.addEventListener('abort', onAbort, { once: true }); + if (signal?.aborted) onAbort(); + stop.addEventListener('abort', onStop, { once: true }); + }); +} + export function createDetachedAttempts( deps: Readonly<{ waitMs: number; diff --git a/packages/platform-apple/src/snapshot-route.ts b/packages/platform-apple/src/snapshot-route.ts index 0a7ea9b7c2..84250e883d 100644 --- a/packages/platform-apple/src/snapshot-route.ts +++ b/packages/platform-apple/src/snapshot-route.ts @@ -265,7 +265,11 @@ async function fallbackAfterFailure( disabledGenerations: Set, cause?: unknown, ): Promise { - disableGenerationFor(failure, failedTarget, disabledGenerations); + // A failed bridge is evidence about this app generation, so its captures take the runner until the + // generation is rebaselined. A bridge that is merely still being prepared is evidence about the + // daemon's build queue instead: the same generation has to be able to use it as soon as it exists, + // which is what let one cold host cost every later capture of a stable screen (#2491). + if (failure.kind !== 'preparing') disabledGenerations.add(generationKey(failedTarget)); emitRouteDiagnostic( failure.code, { id: failedTarget.udid }, @@ -284,21 +288,6 @@ async function fallbackAfterFailure( ); } -/** - * A failed bridge is evidence about this app generation, so its captures take the runner until the - * generation is rebaselined. A bridge that is merely still being prepared is evidence about the - * daemon's build queue instead: the same generation must be able to use it as soon as it exists, - * which is what let one cold host cost every later capture of a stable screen (#2491). - */ -function disableGenerationFor( - failure: SnapshotSourceFailure, - failedTarget: SimulatorSnapshotTarget, - disabledGenerations: Set, -): void { - if (failure.kind === 'preparing') return; - disabledGenerations.add(generationKey(failedTarget)); -} - async function runFallback( deviceId: string, input: CaptureSnapshotInput, diff --git a/packages/platform-apple/src/snapshot-source/deadline.ts b/packages/platform-apple/src/snapshot-source/deadline.ts index c1d9eef5d2..cf9932e036 100644 --- a/packages/platform-apple/src/snapshot-source/deadline.ts +++ b/packages/platform-apple/src/snapshot-source/deadline.ts @@ -1,4 +1,5 @@ import { Deadline } from '@agent-device/host-kit/retry'; +import { waitForDetachedAttempt } from '../detached-attempt.ts'; import { snapshotSourceError } from './errors.ts'; export type SnapshotSourceDeadline = Readonly<{ @@ -24,14 +25,13 @@ export function remainingSnapshotSourceMs(deadline: SnapshotSourceDeadline, code return Math.max(1, Math.floor(remainingMs)); } +/** A sleep nobody has asked to end early. */ +const NO_STOP = new AbortController().signal; + /** - * Sleeps inside the caller's own deadline. `stop` is for a caller that no longer needs the sleep - * because the work it was waiting on answered elsewhere: the delay resolves instead of burning its - * remaining budget, while an aborted `deadline` stays a typed `cancelled`. - * - * A stop is only ever created by the code that calls this and is always aborted by it afterwards, - * so it needs no already-aborted check and no listener removal; the deadline's signal is the - * caller's and does. + * Sleeps inside the caller's own deadline, so a client abort stays a typed `cancelled` instead of + * arriving as a fresh timeout. `stop` is for a caller that no longer needs the sleep because the work + * it was waiting on answered elsewhere: the delay ends without burning the rest of its budget. */ export async function waitForSnapshotSourceDelay( deadline: SnapshotSourceDeadline, @@ -40,22 +40,10 @@ export async function waitForSnapshotSourceDelay( stop?: AbortSignal, ): Promise { const delayMs = Math.min(requestedMs, remainingSnapshotSourceMs(deadline, code)); - await new Promise((resolve, reject) => { - let settled = false; - const timer = setTimeout(() => finish(resolve), delayMs); - const onAbort = () => { - finish(() => reject(snapshotSourceError('cancelled', 'abort-signal'))); - }; - const onStop = () => finish(resolve); - const finish = (action: () => void) => { - if (settled) return; - settled = true; - clearTimeout(timer); - deadline.signal?.removeEventListener('abort', onAbort); - action(); - }; - deadline.signal?.addEventListener('abort', onAbort, { once: true }); - if (deadline.signal?.aborted) onAbort(); - stop?.addEventListener('abort', onStop, { once: true }); + await waitForDetachedAttempt({ + waitMs: delayMs, + signal: deadline.signal, + stop: stop ?? NO_STOP, + cancelled: () => snapshotSourceError('cancelled', 'abort-signal'), }); } diff --git a/packages/platform-apple/src/snapshot-target.ts b/packages/platform-apple/src/snapshot-target.ts index c9c116da18..544dad5e21 100644 --- a/packages/platform-apple/src/snapshot-target.ts +++ b/packages/platform-apple/src/snapshot-target.ts @@ -1,6 +1,6 @@ import type { DeviceInfo } from '@agent-device/kernel/device'; import { AppError } from '@agent-device/kernel/errors'; -import { createDetachedAttempts } from './detached-attempt.ts'; +import { createDetachedAttempts, waitForDetachedAttempt } from './detached-attempt.ts'; import { runSimctl } from './core/apps-simctl.ts'; import { readSnapshotTargetProcessStartTime } from './snapshot-process.ts'; @@ -59,40 +59,13 @@ export function createSimulatorSnapshotTargetResolver(): SimulatorSnapshotTarget targets.set(key, target); return target; }, - wait: (waitMs, stop) => waitForDiscoveryAttempt(waitMs, signal, stop), + wait: (waitMs, stop) => + waitForDetachedAttempt({ waitMs, signal, stop, cancelled: () => signal.reason }), pending: () => targetError('simulator-target-discovery-pending', device, appBundleId), }); }; } -/** - * One caller's wait for a discovery it did not start. Resolving is the wait being spent, not the - * discovery failing: a client abort rejects with its own reason so it stays typed `cancelled`, while - * `stop` means this caller already has its answer and is only letting go of the timer. - * - * The stop needs no cleanup here and no already-aborted check: `value()` creates it moments before - * calling this and aborts it in a `finally`, so the listener is gone once that abort fires. The - * caller's signal outlives this wait and does need its listener removed. - */ -function waitForDiscoveryAttempt( - waitMs: number, - signal: AbortSignal, - stop: AbortSignal, -): Promise { - return new Promise((resolve, reject) => { - const onAbort = () => finish(() => reject(signal.reason)); - const onStop = () => finish(resolve); - const timer = setTimeout(() => finish(resolve), waitMs); - function finish(settle: () => void): void { - clearTimeout(timer); - signal.removeEventListener('abort', onAbort); - settle(); - } - signal.addEventListener('abort', onAbort, { once: true }); - stop.addEventListener('abort', onStop, { once: true }); - }); -} - async function resolveSimulatorSnapshotTarget( device: DeviceInfo, appBundleId: string, From bbc3e8cd090f5d7658acbcb79cc79eec3a9d7830 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 18:55:20 +0200 Subject: [PATCH 6/8] fix(ios-snapshot): remove the stop listener when a detached wait settles waitForDetachedAttempt added an abort listener to stop but only ever removed the one on signal, so every call with no explicit stop leaked one listener onto the module-level NO_STOP signal per production caller (lifecycle.ts's bridge-connect retry sleep and every bridge request's waitForSimulatorTurn). Make stop optional, remove its listener on every settle path, and delete NO_STOP now that the wait tolerates a missing stop directly. --- .../src/detached-attempt.test.ts | 48 ++++++++++++++++++- .../platform-apple/src/detached-attempt.ts | 11 +++-- .../src/snapshot-source/deadline.test.ts | 20 ++++++++ .../src/snapshot-source/deadline.ts | 5 +- 4 files changed, 73 insertions(+), 11 deletions(-) diff --git a/packages/platform-apple/src/detached-attempt.test.ts b/packages/platform-apple/src/detached-attempt.test.ts index a61f6388c4..ad1fb7d5c6 100644 --- a/packages/platform-apple/src/detached-attempt.test.ts +++ b/packages/platform-apple/src/detached-attempt.test.ts @@ -1,6 +1,7 @@ import assert from 'node:assert/strict'; -import { test } from 'vitest'; -import { createDetachedAttempts } from './detached-attempt.ts'; +import { getEventListeners } from 'node:events'; +import { test, vi } from 'vitest'; +import { createDetachedAttempts, waitForDetachedAttempt } from './detached-attempt.ts'; const PENDING = new Error('still-running'); @@ -249,6 +250,49 @@ test('a caller answered by the attempt stops the wait it left running', async () assert.equal(waitState, 'resolved'); }); +test('a wait that settles by its own timeout releases the stop listener it added', async () => { + vi.useFakeTimers(); + try { + const stop = new AbortController(); + const waiting = waitForDetachedAttempt({ + waitMs: 20, + signal: undefined, + stop: stop.signal, + cancelled: () => new Error('unreachable'), + }); + + await vi.advanceTimersByTimeAsync(20); + await waiting; + + assert.equal(getEventListeners(stop.signal, 'abort').length, 0); + } finally { + vi.useRealTimers(); + } +}); + +test('a caller signal already aborted rejects at once, without spending waitMs', async () => { + vi.useFakeTimers(); + try { + const controller = new AbortController(); + const stop = new AbortController(); + const reason = new Error('already-cancelled'); + controller.abort(reason); + + const waiting = waitForDetachedAttempt({ + waitMs: 60_000, + signal: controller.signal, + stop: stop.signal, + cancelled: () => reason, + }); + + await assert.rejects(waiting, (error) => error === reason); + assert.equal(vi.getTimerCount(), 0); + assert.equal(getEventListeners(stop.signal, 'abort').length, 0); + } finally { + vi.useRealTimers(); + } +}); + function identity(error: unknown): unknown { return error; } diff --git a/packages/platform-apple/src/detached-attempt.ts b/packages/platform-apple/src/detached-attempt.ts index 3c63488df3..43dde411f3 100644 --- a/packages/platform-apple/src/detached-attempt.ts +++ b/packages/platform-apple/src/detached-attempt.ts @@ -68,16 +68,16 @@ type Attempt = { * this module describes: sleeps `waitMs`, resolves when that sleep is spent or when `stop` says the * answer arrived elsewhere, and rejects only on the caller's own abort so that stays typed. * - * `stop` needs neither cleanup nor an already-aborted check: `value()` creates it moments before - * calling and aborts it in a `finally`, which releases the `{ once: true }` listener. The caller's - * signal outlives the wait and does have its listener removed. + * Every listener this adds to a signal it did not create is removed once the wait settles, on every + * path (timeout, `stop`, or the caller's own abort) — `stop` is optional for a caller with no signal + * to end the wait early. */ export function waitForDetachedAttempt( params: Readonly<{ waitMs: number; /** The caller's own deadline signal; a wait inside it keeps a client abort a client abort. */ signal: AbortSignal | undefined; - stop: AbortSignal; + stop: AbortSignal | undefined; /** The rejection for the caller aborting, so each owner keeps its own error type. */ cancelled: () => unknown; }>, @@ -90,11 +90,12 @@ export function waitForDetachedAttempt( function finish(settle: () => void): void { clearTimeout(timer); signal?.removeEventListener('abort', onAbort); + stop?.removeEventListener('abort', onStop); settle(); } signal?.addEventListener('abort', onAbort, { once: true }); + stop?.addEventListener('abort', onStop, { once: true }); if (signal?.aborted) onAbort(); - stop.addEventListener('abort', onStop, { once: true }); }); } diff --git a/packages/platform-apple/src/snapshot-source/deadline.test.ts b/packages/platform-apple/src/snapshot-source/deadline.test.ts index 2d9341f593..f9e54d8cf3 100644 --- a/packages/platform-apple/src/snapshot-source/deadline.test.ts +++ b/packages/platform-apple/src/snapshot-source/deadline.test.ts @@ -1,3 +1,4 @@ +import { getEventListeners } from 'node:events'; import { expect, test, vi } from 'vitest'; import { createSnapshotSourceDeadline, waitForSnapshotSourceDelay } from './deadline.ts'; import { SnapshotSourceError } from './errors.ts'; @@ -47,3 +48,22 @@ test('an aborted caller signal stays typed cancellation next to a stop', async ( vi.useRealTimers(); } }); + +test('a delay called with no stop, the way every lifecycle site sends it, leaves no listener behind', async () => { + vi.useFakeTimers(); + try { + const caller = new AbortController(); + const deadline = createSnapshotSourceDeadline(60_000, caller.signal); + + for (let attempt = 0; attempt < 3; attempt += 1) { + const waiting = waitForSnapshotSourceDelay(deadline, 1_000, WAIT_CODE); + await vi.advanceTimersByTimeAsync(1_000); + await waiting; + } + + expect(getEventListeners(caller.signal, 'abort').length).toBe(0); + expect(vi.getTimerCount()).toBe(0); + } finally { + vi.useRealTimers(); + } +}); diff --git a/packages/platform-apple/src/snapshot-source/deadline.ts b/packages/platform-apple/src/snapshot-source/deadline.ts index cf9932e036..219545d690 100644 --- a/packages/platform-apple/src/snapshot-source/deadline.ts +++ b/packages/platform-apple/src/snapshot-source/deadline.ts @@ -25,9 +25,6 @@ export function remainingSnapshotSourceMs(deadline: SnapshotSourceDeadline, code return Math.max(1, Math.floor(remainingMs)); } -/** A sleep nobody has asked to end early. */ -const NO_STOP = new AbortController().signal; - /** * Sleeps inside the caller's own deadline, so a client abort stays a typed `cancelled` instead of * arriving as a fresh timeout. `stop` is for a caller that no longer needs the sleep because the work @@ -43,7 +40,7 @@ export async function waitForSnapshotSourceDelay( await waitForDetachedAttempt({ waitMs: delayMs, signal: deadline.signal, - stop: stop ?? NO_STOP, + stop, cancelled: () => snapshotSourceError('cancelled', 'abort-signal'), }); } From 79f2d9f5e041d820c941eed2511d93d8e9ee2277 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 20:03:59 +0200 Subject: [PATCH 7/8] test(ios-snapshot): end a wait on an already-aborted stop and drop a case that cannot fail waitForDetachedAttempt only checked the caller signal for an already-aborted case; a pre-aborted `stop` would sit until waitMs expired instead of ending at once. Add the same already-aborted check for `stop`, guarded so the first settle wins if both signals happen to be aborted together. deadline.test.ts's "a delay with no stop leaves no listener behind" case only exercised the caller signal and the timer, both of which the pre-fix code already cleaned up correctly, so it could never fail. Delete it; the detached-attempt.test.ts cases pin the actual leak. --- .../src/detached-attempt.test.ts | 21 +++++++++++++++++++ .../platform-apple/src/detached-attempt.ts | 1 + .../src/snapshot-source/deadline.test.ts | 20 ------------------ 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/packages/platform-apple/src/detached-attempt.test.ts b/packages/platform-apple/src/detached-attempt.test.ts index ad1fb7d5c6..fdb4fe12fa 100644 --- a/packages/platform-apple/src/detached-attempt.test.ts +++ b/packages/platform-apple/src/detached-attempt.test.ts @@ -293,6 +293,27 @@ test('a caller signal already aborted rejects at once, without spending waitMs', } }); +test('a stop already aborted resolves at once, without spending waitMs', async () => { + vi.useFakeTimers(); + try { + const stop = new AbortController(); + stop.abort(); + + const waiting = waitForDetachedAttempt({ + waitMs: 60_000, + signal: undefined, + stop: stop.signal, + cancelled: () => new Error('unreachable'), + }); + + await waiting; + assert.equal(vi.getTimerCount(), 0); + assert.equal(getEventListeners(stop.signal, 'abort').length, 0); + } finally { + vi.useRealTimers(); + } +}); + function identity(error: unknown): unknown { return error; } diff --git a/packages/platform-apple/src/detached-attempt.ts b/packages/platform-apple/src/detached-attempt.ts index 43dde411f3..659c249c7d 100644 --- a/packages/platform-apple/src/detached-attempt.ts +++ b/packages/platform-apple/src/detached-attempt.ts @@ -96,6 +96,7 @@ export function waitForDetachedAttempt( signal?.addEventListener('abort', onAbort, { once: true }); stop?.addEventListener('abort', onStop, { once: true }); if (signal?.aborted) onAbort(); + else if (stop?.aborted) onStop(); }); } diff --git a/packages/platform-apple/src/snapshot-source/deadline.test.ts b/packages/platform-apple/src/snapshot-source/deadline.test.ts index f9e54d8cf3..2d9341f593 100644 --- a/packages/platform-apple/src/snapshot-source/deadline.test.ts +++ b/packages/platform-apple/src/snapshot-source/deadline.test.ts @@ -1,4 +1,3 @@ -import { getEventListeners } from 'node:events'; import { expect, test, vi } from 'vitest'; import { createSnapshotSourceDeadline, waitForSnapshotSourceDelay } from './deadline.ts'; import { SnapshotSourceError } from './errors.ts'; @@ -48,22 +47,3 @@ test('an aborted caller signal stays typed cancellation next to a stop', async ( vi.useRealTimers(); } }); - -test('a delay called with no stop, the way every lifecycle site sends it, leaves no listener behind', async () => { - vi.useFakeTimers(); - try { - const caller = new AbortController(); - const deadline = createSnapshotSourceDeadline(60_000, caller.signal); - - for (let attempt = 0; attempt < 3; attempt += 1) { - const waiting = waitForSnapshotSourceDelay(deadline, 1_000, WAIT_CODE); - await vi.advanceTimersByTimeAsync(1_000); - await waiting; - } - - expect(getEventListeners(caller.signal, 'abort').length).toBe(0); - expect(vi.getTimerCount()).toBe(0); - } finally { - vi.useRealTimers(); - } -}); From 0e1fd02b1325339e4a17d08f0ed8f93bbe6d1840 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 15 Sep 2026 20:09:43 +0200 Subject: [PATCH 8/8] fix(ios-snapshot): route preparation test scratch through mkdtempForTest Rebasing onto main picked up the ban on node:os in tests (#2629's mkdtempForTest routing), and this test's own scratch dir predates that change. Route it through the same helper so the branch's own new test stays lint-clean; behavior is unchanged, only where the temp directory comes from. --- .../platform-apple/src/snapshot-source/preparation.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/platform-apple/src/snapshot-source/preparation.test.ts b/packages/platform-apple/src/snapshot-source/preparation.test.ts index 01efa1ab21..9b1e9a0b40 100644 --- a/packages/platform-apple/src/snapshot-source/preparation.test.ts +++ b/packages/platform-apple/src/snapshot-source/preparation.test.ts @@ -1,8 +1,8 @@ import assert from 'node:assert/strict'; -import { mkdtemp, rm, writeFile } from 'node:fs/promises'; -import os from 'node:os'; +import { rm, writeFile } from 'node:fs/promises'; import path from 'node:path'; import { test } from 'vitest'; +import { mkdtempForTest } from '../__tests__/tmp-dir.ts'; import { createSnapshotSourceDeadline, type SnapshotSourceDeadline } from './deadline.ts'; import { createSnapshotSourceHost } from './host.ts'; import { resolveSnapshotSourceLimits } from './limits.ts'; @@ -193,7 +193,7 @@ test('closing the preparation stops a build that is still running', async () => }); async function writeBridgeSource(prefix: string): Promise { - const root = await mkdtemp(path.join(os.tmpdir(), `agent-device-bridge-${prefix}`)); + const root = await mkdtempForTest(`agent-device-bridge-${prefix}`); const sourceRoot = path.join(root, 'source'); await (await import('@agent-device/host-kit/host-file')).ensureHostDirectory(sourceRoot); for (const name of BRIDGE_SOURCES) {