fix(ios-runner): return work that finished at the timeout boundary and bound recorder frame capture - #2837
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
|
Reviewed at f1b04ec. #2801 asked for a live runner-recorder check on an iOS simulator. The PR body says the recorder was reached through HTTP Not blocking: the PR lists only the 1 s floor under "Deviation for sign-off", but the in-flight gate also skips every tick during any command hop, and the measurement shows 15-20% fewer frames under load, so it should be listed too. Also, in production Is there a smaller design here? Checks were still queued at review time. The next step is the live recorder run above and sign-off on the 1 s floor and the in-flight gate. |
f1b04ec to
2a165d3
Compare
|
Rebased onto 1. Live runner-recorder run on an iOS simulatorSetup. A simulator I created for this run: One honest correction to the ask, before the evidence. On an iOS Simulator the CLI does not reach the recorder through HTTP — it spawns so the only way to drive the changed route on a simulator is the runner's own HTTP transport, as the body says. That part is reachable exactly as described: the runner is an The run (~20 s,
{ "success": true,
"data": { "path": "/private/tmp/apex-2837-shots/during-recording.png",
"width": 402, "height": 874, "logicalWidth": 402, "logicalHeight": 874,
"pixelDensity": 1 } }Then commands that genuinely contend with the recorder's ticks on the runner's main thread — A second
{ "success": true,
"data": { "path": "/private/tmp/apex-2837-shots/after-record-stop.png",
"width": 402, "height": 874, "logicalWidth": 402, "logicalHeight": 874,
"pixelDensity": 1 } }Export, read back off the host from the runner's own sandbox ( What that says. 185 frames in a 20.28 s window at
The three PNGs are on the machine that ran this;
One limitation, stated rather than papered over. 2. (a) Second deviation added to the sign-off listAgreed and done. The body now lists two items: the 1 s floor, and the in-flight gate — it skips every tick dispatched during a hop, not only ticks after an abandoned one, because a healthy hop that finishes inside its slice marks nothing. Measured cost is in the body: 15-20% fewer frames than pre-fix under command load (n=1/leg), 91% of the requested rate in the 20 s run above. Say the word if you want the skipped-tick share disclosed per 3. (b) The abandoned term in the gate is dead — removedConfirmed dead in production, and the two counters make that provable rather than incidental:
So Removed in
4. (c) Smaller designs consideredTwo, both rejected, neither cheaper:
5. Smoke Tests: the marker grep is not what failedChecked before touching anything, because the diagnosis differs. In the last CI run at and neither log contains
The sibling PR's At the new head i.e. the two flaky-looking failures at Local gates at this head, for the record: |
|
Reviewed at 40a7bac. This is ready to merge. The timeout-boundary work-return fix and the bound on recorder frame capture both look correct now, and the earlier findings from f1b04ec are resolved. CI is green, 20 checks and 0 not passing. iOS Smoke exercises the changed runMainThreadWork hop on every runner command. The recorder route is not covered by CI, since simulator record goes through simctl; the live run is what covers that path. I did not rebuild the runner or re-run the live recorder session myself. I checked that the artifacts already on hand, the mp4 frame count and duration, the PNGs, and the transcript's lack of ABANDONED markers, match what the author described. The runner-side screenshots that went through the gate were driven over raw runner HTTP; on a simulator, the CLI's own screenshot command goes through simctl and does not reach that gate, so the changed recorder path is only exercised in production on runner-recorded targets, meaning physical devices, which were not tested here. I also did not run the Swift unit tests locally. There's no blocker left in this delta. What's still open is your own sign-off on the two deviations you called out: the 1 second per-frame floor, and the in-flight gate skipping every tick during a hop. Not blocking: could |
thymikee
added
the
ready-for-human
|
Commit 40a7bac now conflicts with main. The code verdict from the earlier review stays clean. I removed |
thymikee
removed
the
ready-for-human
40a7bac to
9fa57aa
Compare
… boundary runMainThreadWork threw MAIN_THREAD_TIMEOUT whenever its wait timed out, even when the main-queue block finished between the wait and the lock and had already stored its result. A tap that happened was then reported as a timeout, and a daemon retry could repeat the gesture. Work that is finished under the lock now returns or rethrows its stored result like work that finished in time, and is not counted as abandoned. Only work still unfinished under the lock is abandoned and throws the timeout error. No timeout value changes. Closes #2782
A recording tick captured its frame with an unbounded DispatchQueue.main.sync on the recorder queue. While the main thread was wedged the first tick blocked the recorder queue, and the busy and wedged accounting never saw it. Each tick after the bootstrap frame now hops to main through runMainThreadWork with a timeout of one frame interval, on iOS and macOS. A capture that times out drops the frame and counts as abandoned work; its late result is discarded and never appended. While any abandoned main-thread work is outstanding the tick is skipped, so the recorder keeps at most one capture pending on main. The bootstrap frame is still taken on the calling thread, and screenshots keep their on-main capture path. Closes #2801
…pture in time Refs #2801
… are using A recorder tick is optional main-thread work. It now hops to main only while no dispatched main-thread work is in flight or abandoned, checked and enqueued under the same lock every dispatch enqueues under, so it never queues behind a command's hop and never raises the occupancy that the busy gate, the response stamp and the snapshot tier skip read. The per-frame bound is a fixed one-second capture timeout instead of the frame interval: a capture slower than the interval lowers the frame rate, and only a capture that slow counts as abandoned main-thread work.
…d gate Production marks a dispatch abandoned only while its block is still running on main, so mainThreadWorkInFlightCount == 0 already implies abandonedMainThreadWorkCount == 0 and the second term in runMainThreadWorkIfIdle's guard could never decide anything. Drop it and pin the gate to the one counter that names main-thread occupancy.
9fa57aa to
3497826
Compare
|
Rebased onto
Validation at
|
|
Reviewed at 3497826. The code looks correct, and the rebase keeps the earlier clean patch: the production Swift matches 40a7bac, and the new upstream The earlier 20 s runner-recorder run on a simulator still covers this head, because the production code did not change. The physical-device recorder route has not been run. I did not run the Swift tests; the claim that the new abandoned-work test goes red without the in-flight guard comes from reading the code. Smoke Tests and Coverage were still running. Smoke Tests exercise Next step: the two deviations you called out (the 1 s per-frame floor and skipping ticks while a command hop is in flight) need a maintainer decision. |
thymikee
added
the
ready-for-human
1 participant
Summary
Two related Apple-runner main-thread-work bugs. #2782:
runMainThreadWorkcould discard workfinishing exactly at its timeout boundary (a semaphore-timeout/result-lock race), rethrowing a
spurious timeout instead of the result; fixed at the lock/return seam. #2801: the recorder's
per-tick capture ran on an unbounded
main.sync, letting a busy main thread queue a tickindefinitely or block a command hop; ticks now go through
runMainThreadWorkIfIdlewith boundedcapture, declining while any main-thread hop is in flight.
Closes #2782, Closes #2801.
Validation
Tested at
349782642c, rebased onorigin/mainfaa69ff784(clean; #2836 and #2852 touchedRunnerTests+CommandDispatch.swift/RunnerTests.swiftand add morerunMainThreadWorkcallers, withno semantic overlap with the gate).
testRunMainThreadWorkIfIdleDeclinesOnlyForInFlightWork(fabricatedabandoned mark over an idle main) is replaced by
testRunMainThreadWorkIfIdleDeclinesAbandonedWorkUntilItDrains,which drives a real timeout: while the abandoned block still holds main,
abandoned == 1andinFlight == 1, the optional tick declines and never dispatches; once drained it runs on main.pnpm check:affected --run: all runnable checks passed (swift-runner-*/replay-*areGitHub-authoritative).
pnpm check:xctest-selection: 0 methods reachable by no lane. iOS Simulator runnerXCTest (iOS 26.2, fresh sim): every test in
UnitTests/RunnerTests+MainThreadWorkTests.swift,UnitTests/RunnerTests+RecordingTests.swiftandUnitTests/RunnerTests+SnapshotCapturePlanOccupancyTests.swiftplus
testRecordStartThrowsTheCaptureRefusalItReceived—Executed 21 tests, with 0 failures.Live runner-recorder run on an iOS simulator — full transcript, commands and screenshots in the
review comment: a 20.28 s window at
fps: 10exportedduration=20.200000,nb_frames=185(9.16 fps, 91% of the requested rate) while
snapshot -i, twoswipes and three runner-sidescreenshots competed with the recorder ticks.agent-device screenshotanswered during therecording and again after
recordStop,runnerMainThreadBusystayedfalseon every answer, andno command failed.
Honestly: on an iOS simulator, CLI
recorduses simctl (recordingBackend: "simctl recordVideo"), not the runner recorder (production path for physical devices and macOSrecord).To reach the changed route, the runner's HTTP
recordStart/recordStopwere driven directlyalongside real CLI commands, so ticks competed with command hops on main. Idle yield was equal
pre/post-fix (~11.4 fps). Under command load, head captured ~15-20% fewer frames than pre-fix
(5.3-5.7 vs. 6.7 fps) — the intended skip-while-busy rule, not a regression (n=1/leg). Exports
decoded cleanly, no command failed; physical-device/macOS paths are unit-tested only.
Deviations for sign-off
The strict bound (~8ms at high fps) would near-zero frames on a slow target; the floor was
adopted instead. Open if the maintainer requires the interval bound.
ticks that follow an abandoned one. A healthy hop that finishes inside its slice marks nothing as
abandoned, so a tick landing during it is dropped rather than appended, and a busy command
surface costs frames: under command load head measured 15-20% fewer frames than pre-fix
(5.3-5.7 vs. 6.7 fps, n=1/leg); the live 20 s run above kept 91% of the requested rate. The
alternative — queueing the tick behind the hop — is what fix(ios-runner): bound recorder frame capture through runMainThreadWork #2801 reported, and it also converts the
tick into abandoned work that stamps
runnerMainThreadBusyand skips XCTest snapshot tiers. Openif the maintainer would rather see the skipped-tick share disclosed per recording.