Skip to content

Commit 90cdee1

Browse files
thymikeethymikee
committed
fix(ios): stop the device runner build when its waiter's request is canceled
A request canceled while queued behind a detached cold start had no path to the build it was waiting on: the spawn carried only the starting request's signal, so a timed-out open left xcodebuild build-for-testing compiling under the daemon on the shared runner derived-data root, where a retried open raced it. The prep child set becomes a ledger keyed by device; raceRunnerStartAgainst Caller stops the waiting device's prep processes on a request cancel (a caller deadline still leaves the start for the retry to join, #2894), and a non-retained close stops the device's build alongside its session (#3177).
1 parent 477508e commit 90cdee1

7 files changed

Lines changed: 266 additions & 17 deletions

File tree

‎packages/platform-apple/src/runner/__tests__/runner-session-close.test.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import assert from 'node:assert/strict';
2+
import { EventEmitter } from 'node:events';
23
import { beforeEach, test, vi } from 'vitest';
4+
import type { ExecBackgroundResult } from '@agent-device/host-kit/command';
35
import { IOS_SIMULATOR } from './device-fixtures.ts';
46
import { appleRunnerTestHost } from '../test-host.ts';
57
import {
@@ -105,6 +107,7 @@ import {
105107
readRunnerSessionLiveness,
106108
releaseIosRunnerOnClose,
107109
} from '../runner-session.ts';
110+
import { registerRunnerPrepProcess, runnerPrepProcessChildren } from '../runner-xctestrun.ts';
108111

109112
// Test-only stand-in for the daemon's runtime lease-owner-state-dir setter (root-only; the package
110113
// cannot import it). Backs the host.leaseOwnerStateDir() getter the package reads instead.
@@ -317,3 +320,27 @@ test('releaseIosRunnerOnClose retains an idle runner, disposes a busy one, and t
317320
await releaseIosRunnerOnClose(device.id, { retain: false });
318321
assert.equal(readRunnerSessionLiveness(device.id), null);
319322
});
323+
324+
/**
325+
* A close that stops the device stops its in-flight runner build too (#3177). The build behind a
326+
* cold start is the resource a session teardown promises to leave behind, not an orphan for the
327+
* next `open` to race on the shared runner derived-data root.
328+
*/
329+
test('releaseIosRunnerOnClose stops the device build still in flight (#3177)', async () => {
330+
const device = { ...IOS_SIMULATOR, id: 'runner-session-close-build-sim' };
331+
await ensureRunnerSession(device, {});
332+
const build = Object.assign(new EventEmitter(), {
333+
pid: 4747,
334+
exitCode: null,
335+
}) as ExecBackgroundResult['child'];
336+
registerRunnerPrepProcess(device.id, build);
337+
338+
await releaseIosRunnerOnClose(device.id, { retain: false });
339+
340+
const signaledPids = mockSignalProcessGroupBestEffort.mock.calls.map(([pid]) => pid);
341+
assert.ok(
342+
signaledPids.includes(4747),
343+
'the non-retained close tree-killed the build still in flight',
344+
);
345+
assert.equal(runnerPrepProcessChildren(device.id).length, 0, 'the build left the prep ledger');
346+
});

‎packages/platform-apple/src/runner/__tests__/runner-start-budget.test.ts‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
import assert from 'node:assert/strict';
2+
import { EventEmitter } from 'node:events';
3+
import { beforeEach, test, vi } from 'vitest';
4+
import type { ExecBackgroundResult } from '@agent-device/host-kit/command';
5+
import {
6+
AppError,
7+
createRequestCanceledError,
8+
isRequestCanceledError,
9+
} from '@agent-device/kernel/errors';
10+
import { IOS_SIMULATOR } from './device-fixtures.ts';
11+
import { appleRunnerTestHost } from '../test-host.ts';
12+
import { raceRunnerStartAgainstCaller } from '../runner-start-budget.ts';
13+
import { registerRunnerPrepProcess, runnerPrepProcessChildren } from '../runner-xctestrun.ts';
14+
15+
const mockSignalPidsBestEffort = vi.fn();
16+
const mockSignalProcessGroupBestEffort = vi.fn();
17+
const mockRunAppleToolCommand = vi.fn();
18+
19+
beforeEach(() => {
20+
vi.resetAllMocks();
21+
mockRunAppleToolCommand.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' });
22+
appleRunnerTestHost.update({
23+
signalPidsBestEffort: mockSignalPidsBestEffort,
24+
signalProcessGroupBestEffort: mockSignalProcessGroupBestEffort,
25+
runAppleToolCommand: mockRunAppleToolCommand,
26+
});
27+
});
28+
29+
function callerDeadline(): DOMException {
30+
return new DOMException('Wait deadline exceeded', 'TimeoutError');
31+
}
32+
33+
/** A detached start nobody has finished — the shape of a cold build still under the lock. */
34+
function hangingStart(): Promise<never> {
35+
const promise = new Promise<never>(() => {});
36+
return promise;
37+
}
38+
39+
function makePrepChild(pid: number): ExecBackgroundResult['child'] {
40+
return Object.assign(new EventEmitter(), {
41+
pid,
42+
exitCode: null,
43+
}) as ExecBackgroundResult['child'];
44+
}
45+
46+
function signaledPids(): number[] {
47+
return mockSignalProcessGroupBestEffort.mock.calls.map(([pid]) => pid as number);
48+
}
49+
50+
function canceled(error: unknown): boolean {
51+
return isRequestCanceledError(error) && error instanceof AppError;
52+
}
53+
54+
async function settleAsyncWork(): Promise<void> {
55+
for (let i = 0; i < 4; i += 1) {
56+
await new Promise((resolve) => setTimeout(resolve, 5));
57+
}
58+
}
59+
60+
/**
61+
* The waiter's cancel owns the build it waited on (#3177). A request canceled while queued
62+
* behind a detached cold build must stop that build: its spawn carried only the STARTING
63+
* request's cancellation signal, so without the waiter's device-scoped prep kill the build
64+
* would keep compiling under the daemon on the shared runner derived-data root, and a retried
65+
* `open` would race it. The kill is the same tree-kill escalation a session stop uses.
66+
*/
67+
test('a request canceled while waiting on the detached start stops that device build', async () => {
68+
const device = { ...IOS_SIMULATOR, id: 'runner-waiter-cancel-sim' };
69+
const build = makePrepChild(4848);
70+
registerRunnerPrepProcess(device.id, build);
71+
72+
const controller = new AbortController();
73+
controller.abort(createRequestCanceledError());
74+
await assert.rejects(
75+
raceRunnerStartAgainstCaller(hangingStart(), controller.signal, device.id),
76+
canceled,
77+
);
78+
await settleAsyncWork();
79+
80+
assert.ok(
81+
signaledPids().includes(4848),
82+
'the waiter cancel reached the build through the prep tree-kill path',
83+
);
84+
assert.equal(
85+
runnerPrepProcessChildren(device.id).length,
86+
0,
87+
'the killed build left the prep ledger',
88+
);
89+
});
90+
91+
/**
92+
* #2894 still holds on this seam: the caller's own deadline (a bounded poll) is NOT a
93+
* cancellation. The start it interrupts is the one the retry joins, so a deadline must leave the
94+
* device's build running and un-signaled.
95+
*/
96+
test('a caller deadline on the same waiter leaves the build running', async () => {
97+
const device = { ...IOS_SIMULATOR, id: 'runner-waiter-deadline-sim' };
98+
const build = makePrepChild(4949);
99+
registerRunnerPrepProcess(device.id, build);
100+
101+
const controller = new AbortController();
102+
const waiting = raceRunnerStartAgainstCaller(hangingStart(), controller.signal, device.id);
103+
controller.abort(callerDeadline());
104+
await assert.rejects(waiting, canceled);
105+
await settleAsyncWork();
106+
107+
assert.equal(
108+
mockSignalProcessGroupBestEffort.mock.calls.length,
109+
0,
110+
'a deadline never signals the build a retry needs (#2894)',
111+
);
112+
assert.deepEqual(
113+
runnerPrepProcessChildren(device.id).map((child) => child.pid),
114+
[4949],
115+
'the deadline left the build registered',
116+
);
117+
});
118+
119+
/**
120+
* The kill is scoped to the waiting device. A canceled waiter for device A must not signal the
121+
* build device B is still paying for — the same request-scoping rule this PR applies to the
122+
* daemon-side timeout recovery.
123+
*/
124+
test('a canceled waiter signals only its own device build', async () => {
125+
const device = { ...IOS_SIMULATOR, id: 'runner-waiter-scope-sim' };
126+
const ownBuild = makePrepChild(5050);
127+
const siblingBuild = makePrepChild(5151);
128+
registerRunnerPrepProcess(device.id, ownBuild);
129+
registerRunnerPrepProcess('other-device', siblingBuild);
130+
131+
const controller = new AbortController();
132+
controller.abort(createRequestCanceledError());
133+
await assert.rejects(
134+
raceRunnerStartAgainstCaller(hangingStart(), controller.signal, device.id),
135+
canceled,
136+
);
137+
await settleAsyncWork();
138+
139+
assert.ok(signaledPids().includes(5050), 'the waiting device build was signaled');
140+
assert.ok(
141+
!signaledPids().includes(5151),
142+
'a build for another device was left running (#3177 sibling protection)',
143+
);
144+
});
145+
146+
/** A prep child that exits on its own leaves the ledger through its close event. */
147+
test('a closed build leaves the prep ledger on its own', () => {
148+
const device = { ...IOS_SIMULATOR, id: 'runner-prep-close-sim' };
149+
const build = new EventEmitter();
150+
registerRunnerPrepProcess(
151+
device.id,
152+
Object.assign(build, { pid: 5252 }) as ExecBackgroundResult['child'],
153+
);
154+
assert.equal(runnerPrepProcessChildren(device.id).length, 1);
155+
build.emit('close');
156+
assert.equal(runnerPrepProcessChildren(device.id).length, 0);
157+
});

‎packages/platform-apple/src/runner/runner-artifact.ts‎

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,45 @@ import { resolveAppleRunnerProjectPath } from './runner-source.ts';
6060
export { prepareXctestrunWithEnv } from './runner-artifact-env.ts';
6161

6262
const runnerXctestrunBuildLocks = new Map<string, Promise<unknown>>();
63-
export const runnerPrepProcesses = new Set<ExecBackgroundResult['child']>();
63+
64+
type RunnerPrepProcess = Readonly<{
65+
deviceId: string;
66+
child: ExecBackgroundResult['child'];
67+
}>;
68+
69+
const runnerPrepProcessLedger = new Set<RunnerPrepProcess>();
70+
71+
/**
72+
* Records a prep subprocess (`xcodebuild build-for-testing`) against the device it builds for, so
73+
* a request canceled while waiting on that build can stop it (#3177). The build child keeps its
74+
* owning start's signal as its first cancel path; this ledger is the device-scoped second one, for
75+
* the waiters whose cancellation the spawn never saw.
76+
*/
77+
export function registerRunnerPrepProcess(
78+
deviceId: string,
79+
child: ExecBackgroundResult['child'],
80+
): void {
81+
const entry: RunnerPrepProcess = { deviceId, child };
82+
runnerPrepProcessLedger.add(entry);
83+
child.on('close', () => {
84+
runnerPrepProcessLedger.delete(entry);
85+
});
86+
}
87+
88+
/** The prep subprocesses still running, for one device or for every device when none is named. */
89+
export function runnerPrepProcessChildren(
90+
deviceId?: string,
91+
): readonly ExecBackgroundResult['child'][] {
92+
return [...runnerPrepProcessLedger]
93+
.filter((entry) => deviceId === undefined || entry.deviceId === deviceId)
94+
.map((entry) => entry.child);
95+
}
96+
97+
export function forgetRunnerPrepProcess(child: ExecBackgroundResult['child']): void {
98+
for (const entry of runnerPrepProcessLedger) {
99+
if (entry.child === child) runnerPrepProcessLedger.delete(entry);
100+
}
101+
}
64102

65103
export type RunnerXctestrunArtifactState = 'valid' | 'rebuilt';
66104

@@ -484,10 +522,7 @@ async function buildRunnerXctestrun(
484522
timeoutMs: buildTimeoutMs,
485523
signal: options.budget?.signal,
486524
onSpawn: (child) => {
487-
runnerPrepProcesses.add(child);
488-
child.on('close', () => {
489-
runnerPrepProcesses.delete(child);
490-
});
525+
registerRunnerPrepProcess(device.id, child);
491526
},
492527
onStdoutChunk: (chunk) => {
493528
logChunk(chunk, options.logPath, options.traceLogPath, options.verbose);

‎packages/platform-apple/src/runner/runner-disposal.ts‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,11 @@ import {
2525
type RunnerLeaseCleanupAdapter,
2626
type RunnerXcodebuildCleanupTarget,
2727
} from './runner-lease.ts';
28-
import { IOS_RUNNER_CONTAINER_BUNDLE_IDS, runnerPrepProcesses } from './runner-xctestrun.ts';
28+
import {
29+
forgetRunnerPrepProcess,
30+
IOS_RUNNER_CONTAINER_BUNDLE_IDS,
31+
runnerPrepProcessChildren,
32+
} from './runner-xctestrun.ts';
2933
import { advanceRunnerSessionState, type RunnerSession } from './runner-session-types.ts';
3034

3135
export const RUNNER_INVALIDATE_WAIT_TIMEOUT_MS = 1_000;
@@ -88,7 +92,7 @@ export async function cleanupOwnedIosRunnerLease(deviceId: string): Promise<void
8892
export async function abortRunnerSessionsAndPrepProcesses(
8993
activeSessions: readonly RunnerSession[],
9094
): Promise<void> {
91-
const prepProcesses = Array.from(runnerPrepProcesses);
95+
const prepProcesses = runnerPrepProcessChildren();
9296
const macOsSessions = activeSessions.filter((session) => isMacOs(session.device));
9397
const otherSessions = activeSessions.filter((session) => !isMacOs(session.device));
9498
for (const session of activeSessions) {
@@ -108,15 +112,20 @@ export async function abortRunnerSessionsAndPrepProcesses(
108112
);
109113
}
110114

111-
export async function stopRunnerPrepProcesses(): Promise<void> {
112-
const prepProcesses = Array.from(runnerPrepProcesses);
115+
/**
116+
* Stops the prep subprocesses (the `xcodebuild build-for-testing` behind a cold runner start)
117+
* with the tree-kill escalation the sessions get. A device stops only its own builds: the caller
118+
* that stops device A's session must not sweep device B's in-flight build (#3177).
119+
*/
120+
export async function stopRunnerPrepProcesses(deviceId?: string): Promise<void> {
121+
const prepProcesses = runnerPrepProcessChildren(deviceId);
113122
await Promise.allSettled(
114123
prepProcesses.map(async (child) => {
115124
try {
116125
await killRunnerProcessTree(child.pid, 'SIGTERM');
117126
await killRunnerProcessTree(child.pid, 'SIGKILL');
118127
} finally {
119-
runnerPrepProcesses.delete(child);
128+
forgetRunnerPrepProcess(child);
120129
}
121130
}),
122131
);
@@ -311,7 +320,7 @@ async function signalRunnerPrepProcesses(
311320
prepProcesses.map(async (child) => {
312321
await killRunnerProcessTree(child.pid, signal);
313322
if (signal === 'SIGKILL') {
314-
runnerPrepProcesses.delete(child);
323+
forgetRunnerPrepProcess(child);
315324
}
316325
}),
317326
);

‎packages/platform-apple/src/runner/runner-session.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ export async function ensureRunnerSession(
120120
}
121121
});
122122
const { raceRunnerStartAgainstCaller } = await import('./runner-start-budget.ts');
123-
return await raceRunnerStartAgainstCaller(start, options.signal);
123+
return await raceRunnerStartAgainstCaller(start, options.signal, device.id);
124124
}
125125

126126
/** How long the device-readiness probe may take, bounded by the startup budget it runs inside. */
@@ -675,6 +675,8 @@ export async function stopIosRunnerSession(deviceId: string): Promise<void> {
675675
* or wedges, so pooling it back hands the same stalled process to the next `open` (#2552). An idle
676676
* retained runner keeps warm reuse via the idle-stop timer. The decision is owned here because the
677677
* occupancy fact lives on the session, and awaited so `close` returns only once the lease is gone.
678+
* A close that stops the device stops its in-flight runner build too (#3177): the build is the
679+
* resource a session teardown promises to leave behind, not an orphan for the next `open` to race.
678680
*/
679681
export async function releaseIosRunnerOnClose(
680682
deviceId: string,
@@ -693,6 +695,7 @@ export async function releaseIosRunnerOnClose(
693695
});
694696
}
695697
await stopIosRunnerSession(deviceId);
698+
await stopRunnerPrepProcesses(deviceId);
696699
}
697700

698701
export async function abortAllIosRunnerSessions(): Promise<void> {

‎packages/platform-apple/src/runner/runner-start-budget.ts‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ import {
44
isRequestCanceledError,
55
} from '@agent-device/kernel/errors';
66
import { emitDiagnostic } from './host.ts';
7-
import { resolveRunnerStartupSignal } from './runner-contract.ts';
7+
import { isCallerDeadlineAbortReason, resolveRunnerStartupSignal } from './runner-contract.ts';
8+
import { stopRunnerPrepProcesses } from './runner-disposal.ts';
89
import { createRunnerPhaseBudget, type RunnerPhaseBudget } from './runner-xctestrun.ts';
910
import { normalizeRunnerStartupTimeoutMs, type RunnerSession } from './runner-session-types.ts';
1011
import type { AppleRunnerLifecycleOptions } from './runner-provider.ts';
@@ -85,18 +86,33 @@ function runnerStartBudgetExhaustedError(timeoutMs: number, explicit: boolean):
8586
* signal allows. A caller whose deadline lands during a cold xctestrun build leaves on time, the
8687
* build keeps going under the lock, and the next request for the device queues behind it and joins
8788
* the session it registers (#2894). Whatever the abort reason, the caller sees the same cancelled
88-
* request it would have seen from any later step; a cancelled request's abort also reaches the
89-
* start through its own startup signal, so nothing here decides whether the start survives. A
90-
* start that fails after its caller left has nobody to report to, so its failure is logged here.
89+
* request it would have seen from any later step. A start that fails after its caller left has
90+
* nobody to report to, so its failure is logged here.
91+
*
92+
* The two abort reasons get opposite treatment of the detached start, and the difference is the
93+
* whole point (#2894 vs #3177). A caller's own deadline (a bounded poll) must leave the start
94+
* running: it is the start the retry joins. A cancelled request means the client is gone, and the
95+
* start it was waiting for belongs to nobody — its spawn carried the *waiting request's*
96+
* cancellation signal only when that request opened the start, so a request that merely joined a
97+
* start another request spawned has no path to it otherwise. On cancel the waiter stops the
98+
* device's prep subprocesses through the same tree-kill path a session stop uses, so a timed-out
99+
* `open` cannot orphan a `build-for-testing` on the shared runner derived-data root where a
100+
* retried `open` would race it. The start itself keeps running under the lock (bounded by its own
101+
* budget); only the build this waiter was waiting on is stopped, and the start's next step fails
102+
* and logs.
91103
*/
92104
export async function raceRunnerStartAgainstCaller(
93105
start: Promise<RunnerSession>,
94106
signal: AbortSignal | undefined,
107+
deviceId: string,
95108
): Promise<RunnerSession> {
96109
if (!signal) return await start;
97110
return await new Promise<RunnerSession>((resolve, reject) => {
98111
const abort = () => {
99112
reject(createRequestCanceledError(undefined, signal.reason));
113+
if (!isCallerDeadlineAbortReason(signal.reason)) {
114+
void stopRunnerPrepProcesses(deviceId);
115+
}
100116
start.catch(emitDetachedRunnerStartFailed);
101117
};
102118
if (signal.aborted) {

0 commit comments

Comments
 (0)