Skip to content

Commit 6902f10

Browse files
committed
fix(apple-runner): stop an inline status probe clearing an outstanding command charge
1 parent 3660c7f commit 6902f10

17 files changed

Lines changed: 888 additions & 147 deletions

‎packages/platform-apple/src/runner/__tests__/runner-command-recovery.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import assert from 'node:assert/strict';
2+
import { RunnerCommandAccounting } from '../runner-session-types.ts';
23
import { afterEach, test, vi } from 'vitest';
34
import { AppError } from '@agent-device/kernel/errors';
45
import { IOS_SIMULATOR } from './device-fixtures.ts';
@@ -35,8 +36,7 @@ function makeRunnerSession(port: number): RunnerSession {
3536
testPromise: new Promise<ExecResult>(() => {}),
3637
child: { pid: process.pid, exitCode: null },
3738
state: 'ready',
38-
inFlightCommands: 0,
39-
hasAbandonedCommands: false,
39+
commandCharges: new RunnerCommandAccounting(),
4040
};
4141
}
4242

‎packages/platform-apple/src/runner/__tests__/runner-command-retry.test.ts‎

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
import { beforeEach, test, vi } from 'vitest';
22
import assert from 'node:assert/strict';
33
import { IOS_SIMULATOR } from './device-fixtures.ts';
4-
import { createTestRequestCancellation, runnerConnectFailure } from './runner-session-fixtures.ts';
4+
import {
5+
createTestRequestCancellation,
6+
makeRunnerSession,
7+
runnerConnectFailure,
8+
} from './runner-session-fixtures.ts';
59
import { AppError } from '@agent-device/kernel/errors';
610
import { Deadline } from '../host.ts';
711
import { appleRunnerTestHost } from '../test-host.ts';
8-
import type { RunnerSession } from '../runner-session-types.ts';
912

1013
const {
1114
mockEnsureRunnerSession,
@@ -1090,21 +1093,6 @@ function assertDiagnosticDecision(expected: {
10901093
);
10911094
}
10921095

1093-
function makeRunnerSession(overrides: Partial<RunnerSession> = {}): RunnerSession {
1094-
return {
1095-
sessionId: `session-${overrides.port ?? 8100}`,
1096-
device: IOS_SIMULATOR,
1097-
deviceId: IOS_SIMULATOR.id,
1098-
port: 8100,
1099-
xctestrunPath: '/tmp/runner.xctestrun',
1100-
jsonPath: '/tmp/runner.json',
1101-
testPromise: Promise.resolve({ exitCode: 0, stdout: '', stderr: '' }),
1102-
child: { pid: 1234, exitCode: null },
1103-
state: 'ready',
1104-
...overrides,
1105-
} as RunnerSession;
1106-
}
1107-
11081096
function makeRunnerArtifact(
11091097
overrides: Partial<RunnerXctestrunArtifact> = {},
11101098
): RunnerXctestrunArtifact {

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { afterEach, beforeEach, expect, test, vi } from 'vitest';
22
import { IOS_SIMULATOR, MACOS_DEVICE, TVOS_SIMULATOR } from './device-fixtures.ts';
33
import type { ExecResult } from '@agent-device/host-kit/command';
4-
import type { RunnerSession } from '../runner-session-types.ts';
4+
import { RunnerCommandAccounting, type RunnerSession } from '../runner-session-types.ts';
55
import { appleRunnerTestHost } from '../test-host.ts';
66
import { makeRunnerLease } from './runner-session-fixtures.ts';
77
import { mkdtempForTestSync } from './tmp-dir.ts';
@@ -257,8 +257,7 @@ function makeRunnerSession(
257257
testPromise,
258258
child: { pid: 42, exitCode: null },
259259
state: 'ready',
260-
inFlightCommands: 0,
261-
hasAbandonedCommands: false,
260+
commandCharges: new RunnerCommandAccounting(),
262261
...overrides,
263262
};
264263
}

‎packages/platform-apple/src/runner/__tests__/runner-early-exit-diagnosis.test.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { resetAllProcessMemosForTests } from '@agent-device/kernel/ttl-memo';
77
import type { ExecBackgroundResult } from '@agent-device/host-kit/command';
88
import { buildRunnerEarlyExitError } from '../runner-startup-transport.ts';
99
import { readRunnerLogTail } from '../runner-io.ts';
10-
import type { RunnerSession } from '../runner-session-types.ts';
10+
import { RunnerCommandAccounting, type RunnerSession } from '../runner-session-types.ts';
1111
import { mkdtempForTestSync } from './tmp-dir.ts';
1212
import { STUBBED_APPLE_TOOLCHAIN, stubAppleToolchainProbes } from './apple-toolchain-fixtures.ts';
1313
import {
@@ -52,8 +52,7 @@ function sessionFailingWith(
5252
child: { pid: 4242, exitCode: 1 } as ExecBackgroundResult['child'],
5353
state: 'starting',
5454
startupDeviceStates,
55-
inFlightCommands: 0,
56-
hasAbandonedCommands: false,
55+
commandCharges: new RunnerCommandAccounting(),
5756
};
5857
}
5958

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import { test } from 'vitest';
2+
import type { RunnerCommand } from '../runner-contract.ts';
3+
import { isRunnerReadinessProbeCommand, RUNNER_COMMAND_TRAITS } from '../runner-command-traits.ts';
4+
import { readSwiftInlineCommands } from './runner-swift-settlement-fixtures.ts';
5+
6+
/**
7+
* The runner serves `status` and `uptime` inline — outside its journal and off the serial command
8+
* queue — which is why the daemon charges no exchange for them: an inline reply proves the runner is
9+
* reachable and nothing about queued work, so a charge for one could only ever be paid by the wrong
10+
* exchange (#2965). The `readinessProbe` trait is how the daemon names that set, and this tie is what
11+
* keeps the two sides one claim: add an inline arm in Swift that the trait missed and that reply would
12+
* discharge an unrelated command's charge; drop one and a genuinely queued command would go uncharged,
13+
* so a shutdown could hand off a runner with work on its queue.
14+
*
15+
* The Swift half is read from the runner's own switch rather than restated here, so the check cannot
16+
* agree with a stale copy of the answer.
17+
*/
18+
19+
function readinessProbeCommands(): string[] {
20+
return (Object.keys(RUNNER_COMMAND_TRAITS) as RunnerCommand['command'][])
21+
.filter((command) => isRunnerReadinessProbeCommand({ command }))
22+
.sort();
23+
}
24+
25+
test('the readinessProbe trait names exactly the commands the runner serves inline', () => {
26+
const inline = readSwiftInlineCommands();
27+
28+
if (inline.length === 0) {
29+
throw new Error(
30+
'the runner serves no command inline, so the daemon must stop routing any reply as an inline answer',
31+
);
32+
}
33+
for (const command of inline) {
34+
if (!readinessProbeCommands().includes(command)) {
35+
throw new Error(`the runner answers "${command}" inline but the trait does not name it`);
36+
}
37+
}
38+
for (const command of readinessProbeCommands()) {
39+
if (!inline.includes(command)) {
40+
throw new Error(`the trait calls "${command}" a probe but the runner queues it`);
41+
}
42+
}
43+
});

‎packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts‎

Lines changed: 184 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@ import {
1515
requireRunnerPhaseRemainingMs,
1616
resolveExpectedRunnerCacheMetadata,
1717
} from '../runner-cache-metadata.ts';
18+
import { captureDiagnostics } from './runner-session-fixtures.ts';
1819
import { startFakeRunnerServer, type FakeRunnerServer } from './fake-runner-server.ts';
20+
import { resolveRunnerDetachDecision, RunnerCommandAccounting } from '../runner-session-types.ts';
21+
import { requireLifecycleSettlementRows } from './runner-swift-settlement-fixtures.ts';
1922

2023
/**
2124
* The wiring regression the recovery suite cannot provide (#1644 review P1):
@@ -104,8 +107,7 @@ function makeRunnerSession(port: number, sessionId = `wiring:${port}`): RunnerSe
104107
testPromise: new Promise<ExecResult>(() => {}),
105108
child: { pid: process.pid, exitCode: null },
106109
state: 'ready',
107-
inFlightCommands: 0,
108-
hasAbandonedCommands: false,
110+
commandCharges: new RunnerCommandAccounting(),
109111
};
110112
return session;
111113
}
@@ -158,6 +160,186 @@ test.each(Object.values(LOST_RESPONSE_MUTATION_ROWS))(
158160
},
159161
);
160162

163+
// #2965: an inline `status` probe answers while the command it probes may still be executing, so its
164+
// own reply must not clear the mutation's outstanding charge. The handoff verdict is asserted through
165+
// `resolveRunnerDetachDecision` — the exact gate `detachRunnerSessionForShutdown` consults. Rows come
166+
// from the runner journal's own state list, so a state the runner gains is a missing row rather than an
167+
// unruled verdict.
168+
const LOST_RESPONSE_HANDOFF_ROWS = requireLifecycleSettlementRows({
169+
completed: true,
170+
failed: true,
171+
accepted: false,
172+
started: false,
173+
notAccepted: false,
174+
});
175+
176+
test.each(LOST_RESPONSE_HANDOFF_ROWS)(
177+
'a lost mutation and status $lifecycleState leaves handoff $settlesCharge',
178+
async ({ lifecycleState, settlesCharge }) => {
179+
server = await startFakeRunnerServer({
180+
tap: [{ kind: 'hangUp' }],
181+
status: [{ kind: 'ok', data: { lifecycleState } }],
182+
});
183+
const session = seedSession(server.port);
184+
185+
// The mutation is refused, never replayed, whatever the terminal verdict — recovery keeps the
186+
// session and reports the command's state to the caller.
187+
await expect(
188+
runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }),
189+
).rejects.toThrow(AppError);
190+
assert.equal(
191+
server.requests.filter((request) => request.command === 'tap').length,
192+
1,
193+
'the mutation is sent once',
194+
);
195+
// `notAccepted` is the journal's "never seen this id", which this daemon cannot read as a safe
196+
// terminal state: it keeps the invalidation path and its charge, while every state the journal
197+
// actually reports for the command keeps the session.
198+
assert.equal(
199+
invalidateRunnerSessionMock.mock.calls.length,
200+
lifecycleState === 'notAccepted' ? 1 : 0,
201+
`${lifecycleState} must ${lifecycleState === 'notAccepted' ? '' : 'not '}invalidate the session`,
202+
);
203+
assert.equal(resolveRunnerDetachDecision(session).detach, settlesCharge);
204+
},
205+
);
206+
207+
test('a lost mutation answered by an inline status probe keeps handoff refused', async () => {
208+
// `started` with the runner reporting itself not busy is the legitimate state this issue is about:
209+
// the probe is served off the XCTest channel while the abandoned mutation keeps running.
210+
server = await startFakeRunnerServer({
211+
tap: [{ kind: 'hangUp' }],
212+
status: [{ kind: 'ok', data: { lifecycleState: 'started', runnerMainThreadBusy: false } }],
213+
});
214+
const session = seedSession(server.port);
215+
216+
await expect(
217+
runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }),
218+
).rejects.toThrow(AppError);
219+
assert.equal(session.runnerMainThreadBusy, false);
220+
assert.deepEqual(resolveRunnerDetachDecision(session), {
221+
detach: false,
222+
reason: 'command_in_flight',
223+
});
224+
});
225+
226+
test('an answered uptime health probe keeps handoff refused over an abandoned mutation', async () => {
227+
// The other inline probe, and the realistic #2965 producer: `prepareIosRunner` and prewarm send
228+
// `uptime` as background health traffic while a mutation is still owed. `status` only ever comes
229+
// from recovery, so pinning it alone would leave `uptime` free to start carrying a charge again —
230+
// whose answer would then forgive the mutation's residue on the serial-queue premise and hand a
231+
// runner off mid-mutation.
232+
server = await startFakeRunnerServer({
233+
tap: [{ kind: 'hangUp' }],
234+
status: [{ kind: 'ok', data: { lifecycleState: 'started' } }],
235+
uptime: [
236+
{ kind: 'ok', data: { uptime: 12 } },
237+
{ kind: 'ok', data: { uptime: 13 } },
238+
],
239+
});
240+
const session = seedSession(server.port);
241+
242+
await expect(
243+
runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }),
244+
).rejects.toThrow(AppError);
245+
// One charge: the mutation. The readiness preflight's probe is inline and carries no charge, so it
246+
// cannot arrive here as a second debt that some later answer could pay off (#2965).
247+
assert.equal(session.commandCharges.outstandingChargeCount, 1);
248+
249+
await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'uptime' });
250+
assert.equal(
251+
server.requests.filter((request) => request.command === 'uptime').length > 1,
252+
true,
253+
'this command is a second probe, answered by the same session',
254+
);
255+
// The probe answered and owed nothing, so the mutation is still the only thing owed and the runner
256+
// stays on the kill path rather than being handed off mid-mutation.
257+
assert.equal(session.commandCharges.outstandingChargeCount, 1);
258+
assert.equal(session.commandCharges.hasAbandonedCharges, true);
259+
assert.deepEqual(resolveRunnerDetachDecision(session), {
260+
detach: false,
261+
reason: 'command_in_flight',
262+
});
263+
});
264+
265+
test('terminal status for a lost mutation charges the next lost mutation afresh', async () => {
266+
server = await startFakeRunnerServer({
267+
tap: [{ kind: 'hangUp' }, { kind: 'hangUp' }],
268+
status: [
269+
{ kind: 'ok', data: { lifecycleState: 'completed' } },
270+
{ kind: 'ok', data: { lifecycleState: 'started' } },
271+
],
272+
});
273+
const session = seedSession(server.port);
274+
275+
const first = await captureDiagnostics(async () => {
276+
await expect(
277+
runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }),
278+
).rejects.toThrow(AppError);
279+
});
280+
// The daemon log has to say whether terminal evidence actually paid the debt, because a refused
281+
// settlement and a settled one both leave the runner answering (#2965).
282+
assert.match(first, /"abandonedChargeSettled":true/);
283+
assert.equal(resolveRunnerDetachDecision(session).detach, true);
284+
285+
// The settlement is this command's, not a latch: a second mutation that loses its response is
286+
// charged again and only its own terminal verdict frees the runner.
287+
await expect(
288+
runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }),
289+
).rejects.toThrow(AppError);
290+
assert.equal(resolveRunnerDetachDecision(session).detach, false);
291+
});
292+
293+
test('terminal status with the runner reporting busy still refuses handoff', async () => {
294+
server = await startFakeRunnerServer({
295+
tap: [{ kind: 'hangUp' }],
296+
status: [{ kind: 'ok', data: { lifecycleState: 'completed', runnerMainThreadBusy: true } }],
297+
});
298+
const session = seedSession(server.port);
299+
300+
await expect(
301+
runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }),
302+
).rejects.toThrow(AppError);
303+
304+
// The journal verdict discharged the charge, yet the runner's own occupancy report keeps it on the
305+
// kill path: a terminal state never bypasses the busy gate (#2552).
306+
assert.equal(session.runnerMainThreadBusy, true);
307+
assert.deepEqual(resolveRunnerDetachDecision(session), {
308+
detach: false,
309+
reason: 'main_thread_occupied',
310+
});
311+
});
312+
313+
test('a lost mutation whose status probe fails keeps the runner on the kill path', async () => {
314+
server = await startFakeRunnerServer({
315+
tap: [{ kind: 'hangUp' }],
316+
status: [{ kind: 'hangUp' }],
317+
});
318+
const session = seedSession(server.port);
319+
320+
await expect(
321+
runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }),
322+
).rejects.toThrow(AppError);
323+
assert.equal(resolveRunnerDetachDecision(session).detach, false);
324+
});
325+
326+
test('a healthy command after a settled abandoned charge returns the runner to handoff-eligible', async () => {
327+
server = await startFakeRunnerServer({
328+
tap: [{ kind: 'hangUp' }],
329+
status: [{ kind: 'ok', data: { lifecycleState: 'completed' } }],
330+
snapshot: [{ kind: 'ok', data: { nodes: [] } }],
331+
});
332+
const session = seedSession(server.port);
333+
334+
await expect(
335+
runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }),
336+
).rejects.toThrow(AppError);
337+
assert.equal(resolveRunnerDetachDecision(session).detach, true);
338+
339+
await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'snapshot' });
340+
assert.equal(resolveRunnerDetachDecision(session).detach, true);
341+
});
342+
161343
test('a runner that reports the command failed surfaces that failure, not the transport error', async () => {
162344
server = await startFakeRunnerServer({
163345
tap: [{ kind: 'hangUp' }],

0 commit comments

Comments
 (0)