Skip to content

Commit 5c4a3eb

Browse files
thymikeethymikee
committed
fix(apple-runner): the preflight marker alone decides the restart
The rule asked for two facts: the marker the readiness preflight puts on its way out, and a recorded budget in the same details. A device run says those two rarely arrive together. Wedging a warm runner (`kill -STOP`, then a `tap`) produced "Runner did not accept connection" and invalidated with `runner_connect_failed_before_command_send`; the budget-carrying shape appeared only post-send, where replaying is not safe and no restart is wanted. A preflight reaches the caller in whatever shape its connect loop ended with — a refusal, an exhausted probe, a killed `simctl` fallback, a post that ran out of its budget — and only the last of those carries `timeoutMs` at the top level. The marker is the fact the decision needs, so it is now the only one asked for. `hasCommandDeadline` and `hasReadinessPreflightDeadline` go with it, `shouldRestartAfterReadinessPreflightError` reduces to the table lookup and is gone, and `isRunnerReadinessPreflightFailure` loses its only consumer. `connectRetry` left the verdicts too: the marker is applied after the connect loop has returned, so no loop can ever consult it. The mocked decision tests stop asserting a message, which is what makes them enough: the shape is no longer part of the rule, and the shapes a real preflight produces are what the device run recorded rather than what a fixture invents. Restoring the budget conjunction turns the refusal and the `fetch failed` cases red again, so the tests still discriminate the claim.
1 parent 553610f commit 5c4a3eb

5 files changed

Lines changed: 57 additions & 70 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,12 @@
1515
back to Appium 1.x, which predates the `mobile:` commands the interactor issues (`deepLink`,
1616
`pressButton`, `activateApp`).
1717
- Changed (iOS runner): what recovers a stuck runner is decided by the recorded error, not by its
18-
wording. A readiness preflight that runs out of time posting to the runner records the budget it
19-
ran out and says "Runner command deadline exceeded" — neither of the two message checks on the
20-
recovery paths looked for that phrasing, so the session was never restarted and the command was
21-
never replayed. The rule reads the preflight marker and the recorded deadline now, so the restart
22-
happens whatever the message happens to say. The other half is what no longer happens: when a slow
18+
wording. A readiness preflight marks the error it gives up with, and that marker is now the whole
19+
test for restarting the session and replaying the command. Two message checks decided it before,
20+
and a preflight reaches the caller in whatever shape its connect loop ended with — "Runner did not
21+
accept connection", "Runner endpoint probe failed", a killed `simctl` fallback, a post that ran out
22+
of its budget — so only some of those restarted and the rest failed the command. The other half is
23+
what no longer happens: when a slow
2324
boot spends the whole prepare budget, the health check reports "prepare ios-runner timed out", and
2425
that no longer wipes a restored `xcodebuild` artifact on the way to a rebuild — the runner session
2526
is invalidated and prepare retries with the artifact intact. Only a failure that indicts the

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

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -77,23 +77,35 @@ test('connect loop stops for terminal verdicts', () => {
7777
assert.equal(shouldRetryRunnerConnectError(new AppError('DEVICE_NOT_FOUND', 'gone')), true);
7878
});
7979

80-
// --- connect deadline ---
80+
// --- readiness preflight ---
8181

82-
test('a readiness preflight that outlived its deadline restarts the session', () => {
83-
// The marked shape this recovery actually sees: the simctl-spawn fallback killed at
84-
// its budget, then marked by the readiness preflight that was waiting on it.
82+
test('the preflight marker alone decides the restart', () => {
83+
// The marker is applied by the preflight's own catch, whatever it was waiting on when it gave
84+
// up: a killed fallback, an exhausted probe, a refusal. Which of those arrived is not evidence
85+
// about whether the command reached the runner, and the marker is.
8586
const killedSpawn = commandFailed('xcrun timed out after 45000ms', {
8687
cmd: 'xcrun',
8788
timeoutMs: 45_000,
8889
runnerReadinessPreflightFailed: true,
8990
});
9091
assert.equal(shouldRestartRunnerAfterReadinessPreflight(killedSpawn), true);
91-
assert.equal(shouldRetryRunnerConnectError(killedSpawn), true);
92-
// A preflight refusal with no deadline restarts nothing: the runner answered and said no.
9392
assert.equal(
9493
shouldRestartRunnerAfterReadinessPreflight(
9594
commandFailed('Runner readiness refused', { runnerReadinessPreflightFailed: true }),
9695
),
96+
true,
97+
);
98+
// The restart the marker authorises is a new session, not more waiting inside this one.
99+
assert.equal(shouldRetryRunnerConnectError(killedSpawn), true);
100+
// Without the marker the same two shapes say nothing about the command having been written.
101+
assert.equal(
102+
shouldRestartRunnerAfterReadinessPreflight(
103+
commandFailed('xcrun timed out after 45000ms', { cmd: 'xcrun', timeoutMs: 45_000 }),
104+
),
105+
false,
106+
);
107+
assert.equal(
108+
shouldRestartRunnerAfterReadinessPreflight(commandFailed('Runner readiness refused')),
97109
false,
98110
);
99111
});

‎packages/platform-apple/src/runner/__tests__/runner-lifecycle-readiness-preflight.test.ts‎

Lines changed: 22 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -106,15 +106,19 @@ test('mutating commands restart stale sessions when readiness preflight fails be
106106
assert.equal(mockExecuteRunnerCommandWithSession.mock.calls[1]?.[1], freshSession);
107107
});
108108

109-
test('mutating commands restart stale sessions when readiness preflight outlives its deadline', async () => {
109+
test('a readiness preflight that runs out a post deadline restarts the session and replays', async () => {
110110
const staleSession = makeRunnerSession({ port: 8100, ready: true });
111111
const freshSession = makeRunnerSession({ port: 8101, ready: false });
112112

113+
// The simulator and usbmux routes post to the runner themselves, and `fetchWithTimeout` reports
114+
// an expiry as "Runner command deadline exceeded" with the budget it ran out. Neither of the two
115+
// message checks this rule replaced matched that wording, so the command was never replayed.
116+
// What routes it is the marker the preflight catch puts on its way out, not the wording.
113117
mockEnsureRunnerSession.mockResolvedValueOnce(staleSession).mockResolvedValueOnce(freshSession);
114118
mockExecuteRunnerCommandWithSession
115119
.mockRejectedValueOnce(
116-
new AppError('COMMAND_FAILED', 'xcrun timed out after 45000ms', {
117-
cmd: 'xcrun',
120+
new AppError('COMMAND_FAILED', 'Runner command deadline exceeded', {
121+
port: 8100,
118122
timeoutMs: 45_000,
119123
runnerReadinessPreflightFailed: true,
120124
}),
@@ -125,29 +129,20 @@ test('mutating commands restart stale sessions when readiness preflight outlives
125129

126130
assert.deepEqual(result, { message: 'tapped' });
127131
assert.equal(mockEnsureRunnerSession.mock.calls.length, 2);
128-
assert.deepEqual(mockInvalidateRunnerSession.mock.calls[0], [
129-
staleSession,
130-
'runner_readiness_preflight_failed_before_command_send',
131-
]);
132-
assert.equal(mockExecuteRunnerCommandWithSession.mock.calls.length, 2);
133132
assert.equal(mockExecuteRunnerCommandWithSession.mock.calls[1]?.[1], freshSession);
134133
});
135134

136-
test('a readiness preflight that runs out a post deadline restarts the session and replays', async () => {
135+
test('a readiness preflight refusal restarts the session like any other preflight failure', async () => {
137136
const staleSession = makeRunnerSession({ port: 8100, ready: true });
138137
const freshSession = makeRunnerSession({ port: 8101, ready: false });
139138

140-
// The real shape of a preflight that ran out of time on a direct post: the simulator and
141-
// usbmux paths post to the runner themselves, and `fetchWithTimeout` reports its expiry as
142-
// "Runner command deadline exceeded" with the budget it ran out. The preflight marker is
143-
// added on the way past the preflight catch. Neither of the deleted message checks matched
144-
// that wording, so the session was never restarted and the command was never replayed.
139+
// This is the shape no message check could have been written for: the runner answered the probe
140+
// and the answer was no, which says nothing about whether the command was written. The marker
141+
// says that, and it says it for every shape at once.
145142
mockEnsureRunnerSession.mockResolvedValueOnce(staleSession).mockResolvedValueOnce(freshSession);
146143
mockExecuteRunnerCommandWithSession
147144
.mockRejectedValueOnce(
148-
new AppError('COMMAND_FAILED', 'Runner command deadline exceeded', {
149-
port: 8100,
150-
timeoutMs: 45_000,
145+
new AppError('COMMAND_FAILED', 'Runner readiness refused', {
151146
runnerReadinessPreflightFailed: true,
152147
}),
153148
)
@@ -156,18 +151,20 @@ test('a readiness preflight that runs out a post deadline restarts the session a
156151
const result = await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 120, y: 240 });
157152

158153
assert.deepEqual(result, { message: 'tapped' });
159-
assert.equal(mockEnsureRunnerSession.mock.calls.length, 2);
160-
assert.equal(mockExecuteRunnerCommandWithSession.mock.calls[1]?.[1], freshSession);
154+
assert.deepEqual(mockInvalidateRunnerSession.mock.calls[0], [
155+
staleSession,
156+
'runner_readiness_preflight_failed_before_command_send',
157+
]);
161158
});
162159

163-
test('a readiness preflight refusal that is neither transport-shaped nor deadline-shaped surfaces', async () => {
164-
const staleSession = makeRunnerSession({ port: 8100, ready: true });
160+
test('a failed readiness probe without the marker does not restart the session', async () => {
161+
const session = makeRunnerSession({ port: 8100, ready: true });
165162

166-
mockEnsureRunnerSession.mockResolvedValueOnce(staleSession);
163+
// Without the marker the failure is just a transport shape, and the one that says the command
164+
// was never written is the reason this restart is safe at all.
165+
mockEnsureRunnerSession.mockResolvedValueOnce(session);
167166
mockExecuteRunnerCommandWithSession.mockRejectedValueOnce(
168-
new AppError('COMMAND_FAILED', 'Runner readiness refused', {
169-
runnerReadinessPreflightFailed: true,
170-
}),
167+
new AppError('COMMAND_FAILED', 'Runner readiness refused'),
171168
);
172169

173170
await assert.rejects(

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

Lines changed: 10 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -168,13 +168,8 @@ type RunnerErrorMatch = {
168168
const hasRetriableFlag: RunnerErrorDetailsMatch = (details) => details.retriable === true;
169169
const hasUsbmuxDeviceUnattached: RunnerErrorDetailsMatch = (details) =>
170170
details.usbmuxDeviceAttached === false;
171-
/** The numeric deadline a command timeout records, including an `AbortSignal.timeout` wrap. */
172-
const hasCommandDeadline: RunnerErrorDetailsMatch = (details) =>
173-
typeof details.timeoutMs === 'number';
174171
const hasReadinessPreflightFailure: RunnerErrorDetailsMatch = (details) =>
175172
details.runnerReadinessPreflightFailed === true;
176-
const hasReadinessPreflightDeadline: RunnerErrorDetailsMatch = (details) =>
177-
hasReadinessPreflightFailure(details) && hasCommandDeadline(details);
178173

179174
type RunnerErrorVerdicts = {
180175
/** isRetryableRunnerError: transport error worth a same-session resend. */
@@ -185,7 +180,7 @@ type RunnerErrorVerdicts = {
185180
sessionFatalReason?: string;
186181
/** Connect-shaped failure before the command was sent: restart the session and replay. */
187182
restartBeforeSend?: boolean;
188-
/** Readiness preflight gave up on its deadline: restart the session and replay. */
183+
/** Readiness preflight gave up before the command was written: restart the session and replay. */
189184
restartAfterReadinessPreflight?: boolean;
190185
/** The runner never accepted a connection, so the restored artifact itself is suspect. */
191186
artifactSuspect?: boolean;
@@ -234,13 +229,13 @@ export const RUNNER_ERROR_RULES: readonly RunnerErrorRule[] = [
234229
verdicts: { retryable: false, connectRetry: false },
235230
},
236231
{
237-
// A deadline alone earns no verdict: the same recorded budget covers a wait inside
238-
// the connect loop, where waiting is right, and a fetch that died after the command
239-
// was written, where replaying it is not. Only the preflight marker says the runner
240-
// never saw the command, and it sits beside the deadline here for that reason.
241-
reason: 'runner_readiness_preflight_deadline',
242-
match: { code: 'COMMAND_FAILED', details: hasReadinessPreflightDeadline },
243-
verdicts: { restartAfterReadinessPreflight: true, connectRetry: true },
232+
// The marker is the whole fact: the preflight gave up before the command was written, so
233+
// replaying it cannot duplicate anything. It is asked for alone because the preflight fails in
234+
// whatever shape the connect loop ended with — a refusal, an exhausted probe, a killed
235+
// fallback — and a rule that also required a recorded budget would fire on only some of them.
236+
reason: 'runner_readiness_preflight_failed',
237+
match: { code: 'COMMAND_FAILED', details: hasReadinessPreflightFailure },
238+
verdicts: { restartAfterReadinessPreflight: true },
244239
},
245240
{
246241
reason: 'runner_connect_refused',
@@ -352,18 +347,8 @@ export function shouldRetryRunnerConnectError(error: unknown): boolean {
352347
}
353348

354349
/**
355-
* The readiness preflight gave up before the command was written. The marker is the only
356-
* evidence for that: an error carrying it arrives in whatever shape the preflight failed
357-
* in, so no message check can be the test.
358-
*/
359-
export function isRunnerReadinessPreflightFailure(error: unknown): boolean {
360-
if (!(error instanceof AppError)) return false;
361-
return hasReadinessPreflightFailure((error.details ?? {}) as AppErrorDetails);
362-
}
363-
364-
/**
365-
* The readiness preflight ran out of its own deadline, so the runner never saw the
366-
* command: restarting the session and replaying is both safe and the only way out.
350+
* The readiness preflight gave up, so the runner never saw the command: restarting the session and
351+
* replaying is both safe and the only way out. The marker carries the rule; the message does not.
367352
*/
368353
export function shouldRestartRunnerAfterReadinessPreflight(error: unknown): boolean {
369354
return runnerErrorVerdict(error, 'restartAfterReadinessPreflight') ?? false;

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

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import {
1616
import {
1717
assertRunnerRequestActive,
1818
isRetryableRunnerError,
19-
isRunnerReadinessPreflightFailure,
2019
resolveRunnerRequestSignal,
2120
shouldRebuildCachedRunnerArtifact,
2221
shouldRestartRunnerAfterReadinessPreflight,
@@ -309,7 +308,7 @@ export async function executeRunnerCommand(
309308
restartReason: 'runner_connect_failed_before_command_send',
310309
});
311310
}
312-
if (session && shouldRestartAfterReadinessPreflightError(appErr)) {
311+
if (session && shouldRestartRunnerAfterReadinessPreflight(appErr)) {
313312
assertRunnerRequestActive(options.requestId);
314313
return await restartSessionAndRunCommand({
315314
device,
@@ -600,10 +599,3 @@ function emitPrepareDiagnostic(
600599
},
601600
});
602601
}
603-
604-
function shouldRestartAfterReadinessPreflightError(error: AppError): boolean {
605-
return (
606-
isRunnerReadinessPreflightFailure(error) &&
607-
(isRetryableRunnerError(error) || shouldRestartRunnerAfterReadinessPreflight(error))
608-
);
609-
}

0 commit comments

Comments
 (0)