Skip to content

Commit c53aa0b

Browse files
committed
fix(apple-runner): fence prep spawns behind a start-owned admission (#3220)
A non-retained close killed the in-flight `build-for-testing` before waiting for the session lock the start holds, and the same caller's health retry answered the kill with a second build while close waited — close timed out on the build it just asked to stop. One start-owned admission now answers who may prepare a device: it is captured when a start requests the device, shared by every start that queues behind that work, closed by a teardown BEFORE it stops prep children or takes the session lock, and closed by the LAST interested waiter's cancellation while any other waiter still preserves the work. The preparation-spawn seam and the session-publish points both read it immediately before they act, so a retired start's retry is refused before the replacement child exists, and a retired start can never publish into a fresh start. A start that merely QUEUED behind a close re-routes to a fresh admission once that close has settled — the fence refuses mid-close retries, never innocent waiters; the settle clears the fence inside the session lock. The starting request counts as an interested waiter even on surfaces that pass no caller signal, so a joiner's cancellation cannot outvote the live owner and stop its build. Removed the #3193 request-owner prep filter (`runnerPrepProcessChildrenWithoutActiveOwner` / `stopRunnerPrepProcessesWithoutActiveOwner`): the admission's waiter count supersedes the owner-liveness sniff. The shutdown-detach mechanism moved to runner-adoption.ts beside the adoption it hands off to (pure move). Closes #3220
1 parent 14e1bd7 commit c53aa0b

11 files changed

Lines changed: 1498 additions & 230 deletions
Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
import assert from 'node:assert/strict';
2+
import fs from 'node:fs';
3+
import path from 'node:path';
4+
import { afterEach, beforeEach, test, vi } from 'vitest';
5+
import { isRequestCanceledError } from '@agent-device/kernel/errors';
6+
import type { ExecResult } from '@agent-device/host-kit/command';
7+
import { appleRunnerTestHost } from '../test-host.ts';
8+
import {
9+
addRunnerStartWaiter,
10+
cancelRunnerStartWaiter,
11+
retireRunnerStartAdmissionForDevice,
12+
runnerStartAdmissionForDevice,
13+
} from '../runner-start-admission.ts';
14+
import { ensureXctestrunArtifact } from '../runner-xctestrun.ts';
15+
import { appleToolchainProbeResult } from './apple-toolchain-fixtures.ts';
16+
import { IOS_SIMULATOR } from './device-fixtures.ts';
17+
import { seedRunnerProductBundle } from './runner-xctestrun.fixtures.ts';
18+
import { mkdtempForTestSync } from './tmp-dir.ts';
19+
20+
// The preparation-spawn seam of #3220: admission is read immediately before `xcodebuild
21+
// build-for-testing` is created, so a start whose device went down never answers the kill with
22+
// a replacement build. The tests drive `ensureXctestrunArtifact` with a stand-in `xcodebuild`
23+
// so the gate is proven at the spawn itself, not at a mock above it.
24+
25+
const runCmdStreaming = vi.fn();
26+
let projectRoot: string;
27+
let derived: string;
28+
29+
beforeEach(() => {
30+
projectRoot = mkdtempForTestSync('agent-device-start-admission-root-');
31+
fs.mkdirSync(
32+
path.join(projectRoot, 'apple', 'runner', 'AgentDeviceRunner', 'AgentDeviceRunner.xcodeproj'),
33+
{ recursive: true },
34+
);
35+
derived = mkdtempForTestSync('agent-device-start-admission-derived-');
36+
process.env.AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH = derived;
37+
runCmdStreaming.mockReset();
38+
appleRunnerTestHost.update({
39+
runCmdSync: vi.fn().mockImplementation(appleToolchainProbeResult),
40+
runCmdStreaming,
41+
findProjectRoot: () => projectRoot,
42+
readVersion: () => '0.0.0-test',
43+
});
44+
});
45+
46+
afterEach(() => {
47+
delete process.env.AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH;
48+
});
49+
50+
/**
51+
* The cold-build retry of the incident: a teardown kills the first `build-for-testing` before it
52+
* ever takes the session lock, and the retired start re-enters to build again. The second spawn
53+
* is what made close wait out its timeout, and it is exactly what the pre-spawn gate refuses —
54+
* the child that would be the replacement never exists.
55+
*/
56+
test('a start whose device was torn down spawns no replacement build after its first was killed', async () => {
57+
const device = { ...IOS_SIMULATOR, id: 'runner-admission-retry-sim' };
58+
const admission = runnerStartAdmissionForDevice(device.id);
59+
let killFirstBuild: () => void = () => {};
60+
const firstBuildKilled = new Promise<void>((resolve) => {
61+
killFirstBuild = resolve;
62+
});
63+
runCmdStreaming.mockImplementationOnce(() => {
64+
killFirstBuild();
65+
// What a killed build surfaces as at this seam: the exec layer rejects once the tree was
66+
// signaled, and the start's retry re-enters from here.
67+
return Promise.reject(new Error('Command was aborted'));
68+
});
69+
70+
const firstStart = ensureXctestrunArtifact(device, { startAdmission: admission }).catch(
71+
(error: unknown) => error,
72+
);
73+
await firstBuildKilled;
74+
retireRunnerStartAdmissionForDevice(device.id);
75+
76+
const failure = await firstStart;
77+
assert.equal(runCmdStreaming.mock.calls.length, 1, 'the killed build was the only spawn');
78+
assert.ok(failure instanceof Error, 'the killed build failed its start');
79+
80+
// The health retry the incident measured: same start, same options, after the kill.
81+
const retry = await ensureXctestrunArtifact(device, {
82+
startAdmission: admission,
83+
}).catch((error: unknown) => error);
84+
85+
assert.ok(isRequestCanceledError(retry), 'the fenced start fails as a canceled start');
86+
assert.equal(runCmdStreaming.mock.calls.length, 1, 'no replacement build was spawned');
87+
});
88+
89+
/**
90+
* The nearest negative: the same retry on a device no teardown touched must still build. A
91+
* guard that refused preparation on any hint of a prior failure would pass the case above and
92+
* break every cold start.
93+
*/
94+
test('a start nobody retired still builds after a killed build', async () => {
95+
const device = { ...IOS_SIMULATOR, id: 'runner-admission-survivor-sim' };
96+
const admission = runnerStartAdmissionForDevice(device.id);
97+
runCmdStreaming
98+
.mockResolvedValueOnce({ exitCode: 143, stdout: '', stderr: '' } satisfies ExecResult)
99+
.mockImplementationOnce(async () => {
100+
await seedBuiltRunner();
101+
return { exitCode: 0, stdout: '', stderr: '' } satisfies ExecResult;
102+
});
103+
104+
await assert.rejects(() => ensureXctestrunArtifact(device, { startAdmission: admission }));
105+
const rebuilt = await ensureXctestrunArtifact(device, { startAdmission: admission });
106+
107+
assert.equal(runCmdStreaming.mock.calls.length, 2, 'the retry built the artifact');
108+
assert.equal(rebuilt.artifact, 'rebuilt');
109+
});
110+
111+
/** A device under a teardown admits preparation no start carried: the prewarm's own build. */
112+
test('a fenced device admits a preparation that carries no start', async () => {
113+
const device = { ...IOS_SIMULATOR, id: 'runner-admission-prewarm-sim' };
114+
retireRunnerStartAdmissionForDevice(device.id);
115+
runCmdStreaming.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' } satisfies ExecResult);
116+
117+
const failure = await ensureXctestrunArtifact(device, {}).catch((error: unknown) => error);
118+
119+
assert.ok(isRequestCanceledError(failure));
120+
assert.equal(runCmdStreaming.mock.calls.length, 0, 'the prewarm build was never spawned');
121+
});
122+
123+
/**
124+
* The window #3193 left measured and open: a waiter cancels before the first prep child exists,
125+
* so a ledger sweep has nothing to stop and the build would simply start. The cancellation closes
126+
* admission, and the spawn that follows it is refused.
127+
*/
128+
test('a cancellation that arrives before the first prep spawn refuses the build that would follow', async () => {
129+
const device = { ...IOS_SIMULATOR, id: 'runner-admission-early-cancel-sim' };
130+
const admission = runnerStartAdmissionForDevice(device.id);
131+
const waiter = new AbortController();
132+
addRunnerStartWaiter(admission, waiter.signal);
133+
runCmdStreaming.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' } satisfies ExecResult);
134+
135+
assert.equal(cancelRunnerStartWaiter(admission, waiter.signal), true);
136+
const failure = await ensureXctestrunArtifact(device, { startAdmission: admission }).catch(
137+
(error: unknown) => error,
138+
);
139+
140+
assert.ok(isRequestCanceledError(failure));
141+
assert.equal(runCmdStreaming.mock.calls.length, 0, 'the never-canceled build was never spawned');
142+
});
143+
144+
/**
145+
* The nearest negative of the waiter rule, taken at the spawn seam: while another waiter is
146+
* still interested, a cancellation preserves the work — a start that asks next still builds.
147+
*/
148+
test('a spawn still admitted by a remaining waiter builds', async () => {
149+
const device = { ...IOS_SIMULATOR, id: 'runner-admission-peer-waiter-sim' };
150+
const admission = runnerStartAdmissionForDevice(device.id);
151+
addRunnerStartWaiter(admission, new AbortController().signal);
152+
const leaving = new AbortController();
153+
addRunnerStartWaiter(admission, leaving.signal);
154+
runCmdStreaming.mockImplementation(async () => {
155+
await seedBuiltRunner();
156+
return { exitCode: 0, stdout: '', stderr: '' } satisfies ExecResult;
157+
});
158+
159+
assert.equal(cancelRunnerStartWaiter(admission, leaving.signal), false);
160+
const built = await ensureXctestrunArtifact(device, { startAdmission: admission });
161+
162+
assert.equal(runCmdStreaming.mock.calls.length, 1, 'the surviving waiter kept the build alive');
163+
assert.equal(built.artifact, 'rebuilt');
164+
});
165+
166+
/** Stands in for a successful `xcodebuild build-for-testing`: the products land under SYMROOT. */
167+
async function seedBuiltRunner(): Promise<void> {
168+
const symroot = path.join(derived, 'Build', 'Products');
169+
await seedRunnerProductBundle(
170+
path.join(symroot, 'Debug-iphonesimulator', 'AgentDeviceRunner.app'),
171+
);
172+
fs.writeFileSync(
173+
path.join(
174+
symroot,
175+
'AgentDeviceRunner_AgentDeviceRunnerUITests_iphonesimulator27.0-arm64.xctestrun',
176+
),
177+
`<?xml version="1.0" encoding="UTF-8"?>
178+
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
179+
<plist version="1.0">
180+
<dict>
181+
<key>ProjectRootHint</key>
182+
<string>${projectRoot}</string>
183+
<key>ProductPaths</key>
184+
<array>
185+
<string>__TESTROOT__/Debug-iphonesimulator/AgentDeviceRunner.app</string>
186+
</array>
187+
</dict>
188+
</plist>`,
189+
);
190+
}

0 commit comments

Comments
 (0)