fix(apple-runner): recovery decisions read typed rules, and a deadline gets a verdict - #2600
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
At 8b250ca the message chains are gone from the recovery decisions, a user abort is still rethrown unchanged, and a deadline after send is not replayed. I traced those through The tests do not show the old routing was wrong. The edited retry test now uses The cache wipe is narrower than before. On main, Coverage fails because of this change: |
|
Two follow-ups while CI was running. Coverage was red for a reason of mine. Smoke Tests is a flake in the alert-observation fixture, not this change. The job failed on Local verification: |
|
ea981ab moves the readiness-preflight tests into |
|
Pushed 1. Route-level test that the old wording check fails. Verified it actually pins the routing, not just the outcome — restored the deleted One test red, the four existing cases green, which is the routing difference nothing else on the branch could show. Restored afterwards ( 2. Restored artifact that hangs past the prepare deadline on every attempt. Nothing rebuilds it, and that is now written down twice rather than left to the reader:
3. Duplication. Gate: |
|
Reviewed the ea981ab..1b0a1a6 delta. The prepare-artifact test does not guard the wording check this PR removes. It builds the deadline as Only mocked unit tests cover the two changed routes. Before merge, please add a live iOS run that shows a readiness-preflight deadline restarting the runner and replaying the command, and a restored artifact that survives a prepare deadline. CI was still running at review time, with no failures to attribute. |
|
Reviewed at 1b0a1a6. Both points from the last review are covered: the new readiness-preflight test fails on the old message-based routing, and the CHANGELOG now says a restored artifact that hangs past the deadline is kept and the deadline is reported. One question on the CHANGELOG wording at All checks pass, and there are no conflicts. |
|
Both asks are addressed, and the second one changed the shape of the first, so they read best together. The fixture now carries the error a real deadline producesYou were right, and my first pass was still wrong. Tracing each decision to its producer:
Proof the fixture bites: putting the deleted wording match back ( Live runOwn simulator (
What I could not show live, stated plainlyRun 2 is the connect route (
So that route rests on: the typed producer chain ( Session closed, daemon stopped with |
|
CI note so the missing smoke row on the current head is not read as a gap: |
b98894b to
553610f
Compare
|
Rebased onto
Local |
|
Reviewed at 553610f. The CHANGELOG entry and the test comment now describe the route the test closes, the new tests fail on the old message-based routing, and the three runner files are unchanged from the tree tested at 1b0a1a6. One evidence gap is left.
Smoke Tests failed in the fixture E2E: the long-press target did not become visible after scrolling, after all 94 runner XCTests passed. This PR does not touch that scroll and selector path, so the failure is likely unrelated. There are no conflicts. |
|
Answered at What the wedge produced on a device. Own simulator, own state dir, warm Safari session,
So The rule now keys on the marker alone ( Why the mocked test is enough now when it was not before. It no longer asserts a message, because the message is no longer part of the rule. What a device can contribute to this route is which shapes occur, and that is what the run above recorded; the verdict itself is a table walk over
Row two is the old rule, and it is red on the refused and Tests. CHANGELOG corrected: it claimed a preflight that runs out of time says "Runner command deadline exceeded" and that the rule reads the marker and the recorded deadline. Both files changed, so the iOS lane runs this head; |
|
Reviewed at 5c4a3eb, as a follow-up to the review at 553610f. The evidence gap from that review is closed. The verdict now depends only on the preflight marker, not on the error's shape, so the runner-session tests that prove the preflight catch applies the marker and the lifecycle tests that prove A canceled command on a ready session can restart a healthy runner. A command on a READY session still runs the readiness preflight (runner-contract.ts#L237). If the caller's AbortSignal fires during that preflight, the startup transport throws A smaller question: a marked preflight failure that is not transport-shaped now costs one session restart before it surfaces. The per-request recycle budget bounds it and the CHANGELOG describes it. Is that the intended trade-off? All 18 checks pass at 5c4a3eb, and there are no conflicts. The next step is the cancellation guard and its test. |
…e gets a verdict Two private message-substring chains decided runner recovery outside RUNNER_ERROR_RULES, and both got the common case wrong. A command that ran out its connection deadline arrives in a shape whose message matches neither 'timeout' nor 'timed out', so executeRunnerCommand rethrew without restarting a session whose runner never received the command. And shouldRetryPrepareRunnerHealthFailure ORed in shouldRetryRunnerConnectError, whose ?? true default let the connect loop retry past a rule that had just denied it. fetchWithTimeout now reports its own expiry as a COMMAND_FAILED carrying timeoutMs, the shape isCommandTimeoutError already understands. Until then the bare AbortSignal.timeout rejection reached asAppError with no details at all, so neither a rule nor a timeout predicate could see it and the message chains were the only thing catching it. Only a rejection carrying that signal's own reason is relabelled: a refused connection or a canceled request keeps the error it actually failed with. RunnerErrorMatch.details is a predicate rather than a closed union, which is what makes a recorded deadline and the readiness-preflight marker expressible as evidence, and the table gains two axes: restartAfterReadinessPreflight, and artifactSuspect for the rules that say the restored xctestrun itself is at fault. The cache wipe reads that axis, so derived data is destroyed only when the runner refused or never answered on every route, and a boot that exited early says false even though its message also reads as a refused connection. Both chains are deleted; what depends on runtime state (a cached artifact being present, the request being canceled) stays in runner-lifecycle.ts. A bare deadline earns no replay verdict on purpose: the same recorded budget covers a wait inside the connect loop, where waiting is right, and a fetch that died after the command was written, where replaying it is not.
`runner-command-retry.test.ts` sits above the test-size tripwire and may not grow, and the four preflight verdicts did not belong in a retry aggregation anyway: they answer one question about what `executeRunnerCommand` does with a runner that refused before the command was written. `makeRunnerSession` comes from the fixtures module that already owns it. Co-authored-by: Apex by Callstack <noreply@callstack.com>
… not see The route-level case the review asked for: a readiness preflight failure that records the budget it ran out, carries the preflight marker, and says nothing about time in its message. The table restarts the session and replays; the deleted substring chain rethrew. Restoring that message check turns this one test red and leaves the rest of the file green, which is the difference the earlier cases could not show. The other half is the question the narrower wipe raises: a restored artifact whose runner never answers past its deadline on every attempt. It is not rebuilt. `runner-lifecycle-prepare-artifact.test.ts` pins what does happen — the artifact survives, the session is invalidated, the deadline is reported — and it goes red if the wipe goes back to firing on any prepare timeout. The same answer is now in the changelog, because "nothing rebuilds it" is a behavior someone will hit. `isRunnerReadinessPreflightError` moves into the contract module it was duplicating. Co-authored-by: Apex by Callstack <noreply@callstack.com>
Both fixtures invented wording, which is the mistake the review caught: the case they are supposed to pin is a specific producer's error, and a message nobody emits cannot show that a deleted message check used to fire on it. `ensureRunnerAttemptCanStart` reports an exhausted startup attempt as "Runner connection deadline exceeded" with the budget in details — the word the deleted `isPrepareHealthTimeout` matched. With that message the test fails when a wording check goes back into the wipe decision, and passes with the invented "xcrun simctl spawn did not answer" against the same mutant, which is the hole the fixture used to leave. `fetchWithTimeout` reports its own expiry as "Runner command deadline exceeded", and the preflight marker rides along on the way out of the preflight catch; neither deleted message chain looked for "deadline", so the route-level case is now built from that shape. The changelog said the `fetchWithTimeout` wrap reaches the lifecycle on the preflight route. It does not: `waitForRunner` turns each endpoint failure into "Runner did not accept connection". The reachable case is the direct post on the simulator and usbmux paths, which is what the entry says now. Co-authored-by: Apex by Callstack <noreply@callstack.com>
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.
…swering The marker says the preflight gave up, and one of the things a preflight gives up on is a caller that stopped waiting. `createRequestCanceledError` answers `COMMAND_FAILED` too, so the rule written last round matched a cancellation exactly as well as a wedged runner: on a session that was ready and still working, the cancel invalidated the session and replayed the canceled command against a fresh boot nobody was going to wait for. The main branch rethrew cancellations before any restart branch could see them; the marker rule made the marker the only question, and the marker does not distinguish the two shapes it is applied to. The exception belongs on the rule, where the verdict is, rather than on the call site that happens to notice: a marked error restarts the session unless the typed reason on it says the request was canceled. `isRequestCanceledDetails` is exported from the owning type so the rule reads that reason instead of restating the literal, and `isRequestCanceledError` now asks it. Deleting the conjunct turns both the classification test and the lifecycle test red — the lifecycle one pins the exact shape from the report: ready session, marked cancellation, error rethrown, no invalidate called, no second boot.
5c4a3eb to
f83bdfe
Compare
|
Reviewed at f83bdfe, as a follow-up to the review at 5c4a3eb. The code looks good: recovery decisions read the typed rules, the deadline case gets a real verdict, and the earlier gaps are closed. The earlier trade-off question (a marked failure that is not transport-shaped costs one bounded restart) has no reply, but the CHANGELOG describes that behaviour, so I read it as intended. Typecheck & Package fails with TS2353 on |
thymikee
added
the
ready-for-human
|
|
Answered at The cancellation is a real match, not a hypothetical. The exception went on the rule rather than on the call site that happened to notice, because that is where the verdict is: a marked error restarts the session unless the typed reason on it says the request was canceled. I did not take the Tests: the shape joins the classification table, and On the trade-off question: yes, that is the intended cost, and it is bounded at one. |
1 participant
Summary
Two private message-substring chains decided runner recovery outside
RUNNER_ERROR_RULES. A command that ran out its connection deadline arrives in a shape whose message matches neithertimeoutnortimed out, soexecuteRunnerCommandrethrew without restarting a session whose runner never received the command, andshouldRetryPrepareRunnerHealthFailureORed in a connect-retry default that retried past a rule which had just denied it. Both chains are deleted.fetchWithTimeoutreports its own expiry asCOMMAND_FAILEDcarryingtimeoutMs— the shapeisCommandTimeoutErroralready reads. Until then the bareAbortSignal.timeoutrejection reachedasAppErrorwith no details, so no rule could see it. Only a rejection carrying that signal's own reason is relabelled; a refused connection or a canceled request keeps its own error.RunnerErrorMatch.detailsis a predicate, which is what makes a recorded deadline and the readiness-preflight marker expressible as evidence. The table gainsrestartAfterReadinessPreflightandartifactSuspect; the cache wipe reads the latter, so derived data is destroyed only when the runner refused or never answered on every route — a boot that exited early says false although its message also reads as a refused connection. A bare deadline earns no replay verdict: the same recorded budget covers a wait in the connect loop and a fetch that died after the command was written.Closes #2524. 6 files, one module group. Two notes: a prepare deadline no longer wipes derived data (deliberate — the cost is a clean build);
device_busy_connecting: falseis a table verdict only, since no production message carries that text today.Validation
Tested at
8b250ca757.pnpm check:affected --runpassed: 367 test files. The 6 new/changed classification, restart and wipe tests were run red first; removingartifactSuspect: falsefrom the boot-denial rule turns two of them red.Live:
open "Agent Device Tester" --relaunchon iPhone 16 Pro (4.7s, runner adopted), then a forcedprepare ios-runner --timeout 4000deadline — 9 connect retries, one session invalidation, typedprepare_deadline_expired, and noDOMException/TimeoutErrorin the request diagnostics orrunner.log. Sessions closed, daemons stopped--clean, app uninstalled, no stale claims.runner-contract.tsis 553 lines: the table must stay there (eager-closure-budgetsis no-growth), so it grows rather than splitting.