fix(ios-snapshot): prepare the AX bridge off the capture deadline - #2607
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at caae0b0. Moving preparation off the capture deadline is the right fix, but the retry window and shutdown do not hold yet, and the reported timing does not match this commit. The retry window counts from the start of the attempt, not from its failure. Every capture waits the full 2 s slice while the attempt is pending ( Shutdown cannot stop an in-flight build. A design question: this is a second hand-written detached single-flight with a bounded wait and a typed pending error in this package, next to The PR body reports the stalled-build CI is green and there are no conflicts. |
|
Reviewed at caae0b0. Moving preparation off the capture deadline fits #2491, but a few points need a look. The retry window is measured from the attempt's start (
Every capture that finds the attempt pending waits the full 2 s budget before it gets The Could this be smaller? An owned Could you link the output of the live A/B run from the description? Its 0.3 s figure does not match the 2 s wait budget in this diff. All checks pass, and there are no conflicts. |
caae0b0 to
cc8f11b
Compare
|
All five points addressed at Retry window from the failure. Wait slice once per attempt. The grant is now declared: Shutdown cannot stop an in-flight build. The preparation owns an Warning wording. The The design question — you were right, and nothing had to change first. Preparation and target discovery are the same shape and had already diverged on failure retention. The 0.3 s figure was not a cold-preparation measurement, so it could not match the wait budget; the re-run is on the final head, with the stall in place and per-capture timings in the Validation section. |
|
Reviewed at cc8f11b, as a follow-up to the review at caae0b0. The five earlier points are addressed, and the author-reported cold-build run with an injected clang stall reaches the changed route. The detached-attempt helper is the smaller design the last review asked for. One small regression is left. Attaching the request-log lines from the cold-build run (the Typecheck & Package fails with TS2353 on Next: the wait cleanup. |
cc8f11b to
5391485
Compare
|
Rebased onto
Also unblocked: the red Typecheck & Package job was main's own |
|
The first Coverage run at 1 failed / 10,819 passed. It is that file's temp-directory teardown racing a concurrent write, not a coverage threshold, and nothing in this PR touches device claims; Coverage is green on |
5391485 to
111e06f
Compare
|
Fixed at
Request-log lines from the cold-build run at this head (fresh runner build, warm XCTest runner, stubbed Only the first capture pays; the six following
|
|
Reviewed at 111e06f, as a follow-up to the review at 5391485. One gap is left: the discovery stop path has no test. In snapshot-target.ts#L90, could the All checks pass, including Smoke Tests, which exercises this path, and there are no conflicts. Next: the discovery stop-path test. |
111e06f to
6adf53c
Compare
|
Fixed at The test is there, driven through await vi.advanceTimersByTimeAsync(0);
release();
await vi.advanceTimersByTimeAsync(0);
await Promise.all([first, second]);
expect(abortListeners(starting.signal)).toBe(0);
expect(abortListeners(joining.signal)).toBe(0);
expect(vi.getTimerCount()).toBe(0);
Your reachability question is the interesting part, and I checked it line by line rather than writing a test to keep each one:
The last two are dead rather than under-tested, so both waits now drop them and say why in the doc comment: the stop is owned and always aborted by the caller of the wait, while the caller's own signal outlives the wait and does need its listener removed — that removal is covered, and deleting it fails. At this head: One thing worth flagging about the local gate: |
|
Reviewed at 6adf53c, as a follow-up to the review at 111e06f. The discovery stop path now has a test through One small thing is left in the test helper. Smoke Tests are still running. They exercise the iOS snapshot route this PR changes, so a failure there needs its log read. There are no conflicts. Next: swap the cast for |
6adf53c to
58e61bf
Compare
|
Simplified at The stop/cleanup questions all came back to the same shape existing twice: the discovery wait and the bridge-preparation wait each hand-rolled a timer, the caller's abort listener, a stop listener and the cleanup that keeps a capture from leaking a timer and a listener. Live re-run at this head, cold bridge, one stalled |
|
Reviewed at 58e61bf, as a follow-up to the review at 6adf53c. One wait helper for discovery and the deadline is smaller, but it adds a listener leak.
The new comment at detached-attempt.ts#L72 says snapshot-target.ts#L63 now rejects at once when the signal is already aborted, where the old discovery wait waited out All 18 checks pass and there are no conflicts. Next: remove the stop listener when the wait settles, with the no- |
|
Pushed 14a2dea to fix the listener leak. |
|
Reviewed at 14a2dea. The listener leak from the 58e61bf review is fixed: In detached-attempt.ts#L96, only the caller signal gets an already-aborted check. If a caller passes a The new case at deadline.test.ts#L52 says a delay with no On the earlier question: the immediate rejection for a pre-aborted signal is not new, because main had the same check. Nothing to change there. Smoke Tests is still running on 14a2dea. This PR changes the iOS snapshot route that the lane exercises, so a green run there is the evidence #2491 needs. There are no conflicts. Next: a green Smoke Tests run. The two points above are small. |
|
Smoke Tests has now failed on 14a2dea: |
A cold host pays for the AX bridge inside the capture that happens to ask for it first, so the first capture of a session spent its whole deadline in a toolchain probe or a clang build and the lane lost the `wait` that was polling for a screen (#2491). Preparation is now a detached single-flight per runtime: the first capture that finds it running waits out a short budget and is served by the XCTest runner, the build keeps going for whoever asks next, and a failed attempt is answered as-is until a retry window measured from the failure expires. The grant is once per attempt rather than once per capture. A `wait` poll cycles every 200 ms or so, and a budget paid per poll would cost a cold build more in captured polls than the build itself costs, with every one of those captures ending up on the runner anyway. Both this and the pending target discovery in `snapshot-target.ts` are the same shape — one attempt per key, detached from whoever started it, a bounded wait, a typed answer while it runs — and they had already started to forget failures differently. One seam owns that shape; each owner declares its own wait budget, wait grant, retry window and pending error, and keeps its own cache of a finished value, because only the owner knows when that value stops being valid. The preparation carries the signal `close()` aborts. Once a capture has answered, no request owns this attempt any more: without that, `xcrun` could keep running for two minutes past shutdown and its cache write would land in a directory belonging to a source that is gone.
`value()` raced the attempt against the caller's wait and left the loser running. A capture answered by the attempt still held its timer and abort listener until the budget ran out — 1.5 s for discovery, 2 s for preparation — so a poll loop could stack one pending timer and one listener per capture. The `awaitDiscovery` it replaced cleared both in a `finally`. `wait` now takes a per-call stop signal that `value()` aborts in a `finally`, and both owners release their timer and listener on it: `waitForDiscoveryAttempt` resolves, and `waitForSnapshotSourceDelay` grows an optional `stop` that ends the sleep without spending the deadline it is measured against. Waiting inside the caller's own deadline still matters, so a client abort keeps rejecting with the typed cancellation; the stop only ever lands after the race is already decided, which is why the losing wait resolves rather than rejecting, and why the race keeps a handler of its own.
… settles
The stop path inside `waitForDiscoveryAttempt` had no test: the listener registration, the listener
removal and the already-aborted check could each be deleted with every suite green.
`snapshot-target.test.ts` now runs a deferred spawn through `createSimulatorSnapshotTargetResolver`,
lets a second caller join the pending discovery, settles it, and asserts on the two things a leaked
wait costs — a timer still pending and a listener still on the caller's `AbortSignal` — with fake
timers, so neither is a timing race. `deadline.test.ts` measures the same two properties for the
preparation owner and drops its already-aborted case.
That case was one of the things the review asked about, and it is unreachable rather than untested:
`value()` creates the stop moments before calling and aborts it in a `finally`, so the check can never
be true and the stop's `{ once: true }` listener is always released by that abort. Both waits now
drop the check and the redundant removal, and keep the cleanup that does matter, which is the
caller's own signal: it outlives the wait, and forgetting it would leave a listener per capture.
…rement rule where it applies Three reviewers in a row had to be satisfied about the same promise plumbing because it existed twice: the discovery wait and the bridge-preparation wait each hand-rolled the timer, the caller's abort listener, the stop listener and the cleanup that keeps a leak from costing a timer and a listener per capture. `waitForDetachedAttempt` now implements the wait that `value()`'s contract describes, and both owners hand it their own sleep length and their own cancellation error — which also means the invariant is deleted-and-caught in one place instead of two. `disableGenerationFor` said one thing about one call site, so the rule moves inline next to the set it edits: a failed bridge retires the app generation, a bridge that is merely still building does not.
waitForDetachedAttempt added an abort listener to stop but only ever removed the one on signal, so every call with no explicit stop leaked one listener onto the module-level NO_STOP signal per production caller (lifecycle.ts's bridge-connect retry sleep and every bridge request's waitForSimulatorTurn). Make stop optional, remove its listener on every settle path, and delete NO_STOP now that the wait tolerates a missing stop directly.
…case that cannot fail waitForDetachedAttempt only checked the caller signal for an already-aborted case; a pre-aborted `stop` would sit until waitMs expired instead of ending at once. Add the same already-aborted check for `stop`, guarded so the first settle wins if both signals happen to be aborted together. deadline.test.ts's "a delay with no stop leaves no listener behind" case only exercised the caller signal and the timer, both of which the pre-fix code already cleaned up correctly, so it could never fail. Delete it; the detached-attempt.test.ts cases pin the actual leak.
Rebasing onto main picked up the ban on node:os in tests (#2629's mkdtempForTest routing), and this test's own scratch dir predates that change. Route it through the same helper so the branch's own new test stays lint-clean; behavior is unchanged, only where the temp directory comes from.
14a2dea to
0e1fd02
Compare
|
Rebased onto main as 0e1fd02, with both points from the 14a2dea review fixed. |
|
Reviewed at 0e1fd02, as a follow-up to the review at 14a2dea. Both points are fixed. There are no conflicts. One Smoke Tests job is still running on 0e1fd02. This PR changes the iOS snapshot route that the lane exercises, so a failure there would not be unrelated. Next: a green Smoke Tests run on 0e1fd02, which is the evidence #2491 needs. |
|
The Smoke Tests failure on The lane's
Re-run requested on the failed job. One weakness worth its own issue rather than a change here: |
|
Rerun of the failed job is green: all four Smoke Tests jobs pass on |
|
Follow-up at 0e1fd02: the evidence gap is closed. All four Smoke Tests jobs pass on this head after the rerun. The code verdict from the review above stands, and there are no conflicts. Next: human review. |
thymikee
added
the
ready-for-human
|
1 participant
Summary
A cold host paid for the AX bridge inside whichever capture happened to ask first, so that capture spent its whole deadline in an Apple toolchain probe or a clang build and the lane lost the
waitthat was polling for a screen (#2491). Preparation is now a detached single-flight per runtime: the first capture that finds it running waits out a 2 s budget and is served by the XCTest runner, the build keeps running for whoever asks next, and a failure is answered as-is until a retry window measured from the failure expires. The budget is granted once per attempt, not per capture — awaitpoll cycles every ~200 ms and a budget paid per poll would cost more than the build it is waiting for.packages/platform-apple/src/detached-attempt.tsis the one seam for this and for the pending target discovery, which had already started to forget failures differently. Each owner declares its wait budget, wait grant, retry window and pending error, and keeps its own cache of a finished value, because only the owner knows when that value stops being valid.waitForDetachedAttemptis the matching implementation of that wait contract: one timer, the caller's abort listener and thestoplistener, all released on every settling path, so a capture that stops waiting leaves neither a timer nor a listener behind. Preparation owns theAbortControllerwhose signal goes into the deadline, andclose()aborts it.14 files, +1154/-100, of which 755 lines are tests.
Validation
Run at
0e1fd02, rebased onorigin/main.pnpm check:affected --rungreen:format:check,lint,typecheck,check:layering,check:fallow --base origin/main,build, andvitest relatedacross the 263 files it selects. Thepackages/platform-appleunit suite is 696 passed.xcrun clangstall of 60 s, dedicatedApex CI Smokesimulator, XCTest runner warm, runner built from this tree):wait textanswers in 1.4 s and the six captures after it in 0.6-1.4 s each, every one served by the runner while the bridge is preparing.ios.snapshot-source.acquirespends 2006 ms once and 0-1 ms per capture after it. Under the same injectionmainfailswait textafter 60.7 s withwait_capture_stalledandreadableCaptures: 0.xcrunchild count goes from 1 to 0 within 2 s ofdaemon stop.stopWaiting.abort(), thestoplistener, theclearTimeout, the caller-listener removal, or the already-abortedstopcheck each fails at least one test; the deadline case that could not fail was deleted instead of kept. The cold-bridge numbers above are from58e61bf8dc; the two commits since change only listener cleanup on that wait path, which is what those deletion checks cover.