fix(ios-runner): keep snapshot-plan target state on main and bound the query sweep by its slice - #2836
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at 8f69c76. A non-interactive snapshot where the tree tier fails fast and the query sweep runs long now returns a shallow partial result instead of arming the timeout penalty.
Not blocking: the 0.1 s admission margin in On size: the diff adds about 490 net lines, with four new seams. Could No run shows the query-sweep tier inside the full Checks were still queued at review time. This diff overlaps the iOS Smoke route and the Repo Guards XCTest list, so failures there need a look. The known The next step is the two fixes above, each with a plan-level test. |
8f69c76 to
9719773
Compare
|
Reviewed at 9719773. Both blocking findings from the earlier review (8f69c76, #2836 (comment)) are still open; this is a rebase, and commits 1, 2, and 4 are byte-identical in range-diff, so neither fix landed yet. In a non-interactive plan the tree tier fails fast and the query sweep runs long.
No simulator run yet shows the query-sweep tier inside a full Smoke Tests and Repo Guards were still running with no failure excerpt captured; Smoke covers the runner snapshot capture plan and the ios.yml workflow change, and Repo Guards covers the XCTest file and test list touched by the new UnitTests files, so a failure in either should be treated as possibly related to this diff. Before this is ready to merge, please push the slice-deadline sweep fix so it arms the penalty and falls through to privateAX, move the target-identity reads to main, add a plan-level test for each, and then run the live simulator validation described above. |
9719773 to
5385c2a
Compare
|
Both re-opened findings are addressed at Finding 1 — a deadline-exhausted query sweep was not treated as a tier timeout. The tier stopped starting queries at its slice deadline but the plan classified only what it collected, and Finding 2 — the coordinate tap's routing probe keyed a penalty with target identity read off main. One limit, stated plainly: the live triple you asked for (tree tier fails fast + slow sweep, then |
|
Reviewed at 5385c2a. Both findings from the earlier pass (#2836 (comment)) are fixed: snapshot-plan target state stays on main, and the query sweep is bounded by its slice. This is ready to merge. All 18 checks are green. The iOS runner-regressions job runs the two new tests, which exercise the changed capture-plan and tap-routing code, so its result covers this diff. I did not run the on-simulator tests or read the CI logs, so the "red before the fix" claims rest on tracing the pre-fix code rather than a reproduced failure. The plan-level test depends on Not blocking: on the |
thymikee
added
the
ready-for-human
…ites The capture plan runs on the command queue but read currentBundleId, currentAppProcessIdentifier and the penalty warm-up exemption, and wrote runnerAccessibilityHealth and invalidated the cached target directly. Snapshot preparation now takes a SnapshotCaptureTarget on main (app, bundle id, pid, consumed warm-up exemption) and the plan, the modal probe penalty, the XCTest penalty recorder and the private AX tier read only that copy. Health writes and the fail-closed invalidation go through applyMainOwnedSnapshotState, the abandoned-work-guarded main hop that invalidateCachedTargetAfterSnapshotFailure now also uses. Fixes #2781
A non-interactive query sweep used Date.distantFuture as its own deadline, so only the up-to-20 s plan deadline bounded it while the caller abandoned the tier after its 1 s slice. XCTest queries cannot be cancelled, so the sweep kept the main thread busy and later commands got RUNNER_BUSY. The caller now derives one slice deadline (querySweepSliceDeadline) for both its wait and the sweep, for interactive and non-interactive requests alike. Neither the element queries nor the per-element reads start with less than flatInteractiveQueryBudget left before that deadline (querySweepCanStartQuery). Fixes #2783
…that runs Taking the snapshot target on main consumed the exemption during command preparation, so a snapshot answered by the blocking system-modal probe, or an abandoned preparation that ran late, spent it without running a plan. The exemption now lives in a lock-owned type that lifecycle code arms on main and runSnapshotCapturePlan consumes at its start.
…ugh the plan Swizzles XCUIElementQuery.allElementsBoundByIndex with a slow query and runs a non-interactive [.querySweep] plan with a 20 s plan deadline. The sweep must not be abandoned, and no query may start after the plan answers, so passing the plan deadline to the sweep in either loop turns it red.
…ity to main Two regressions #2781 still leaves open, both red on this head: A query sweep that stops at its own slice deadline is a tier timeout, but the plan classified its payload by node count, so a partial sweep that beat the sparse threshold armed no XCTest-channel penalty and no later capture of the same screen was deferred. The coordinate tap's system-modal routing probe armed its penalty with currentBundleId read on the command queue, while main owned that state and could still be clearing or rebinding it.
…er timeout The sweep tier stopped starting queries at its slice deadline, but the plan classified only what it collected. sparsePayloadReason accepts anything above the sparse node threshold, so a partial flat sweep became a `recovered` capture: no XCTest-channel penalty was armed, private AX never ran, and every later capture of that screen paid for the full sweep again. `runFlatInteractiveQueries` now returns a typed `SnapshotTierOutcome`, the sweep tier carries it into the attempt, and the plan rejects a `.deadlineExhausted` tier through `snapshotTierRejectionReason` — keeping its payload only as the fallback, arming the penalty through the existing tier timeout reason, and letting the next backend answer.
The coordinate tap resolved its system-modal routing on the command queue and armed an abandoned probe's penalty with `currentBundleId` read there, while `applyMainOwnedSnapshotState` and the lifecycle code write that identity through `DispatchQueue.main.async`. A tap could penalize a target main was still clearing or rebinding. The probe now takes a `SnapshotProbePenaltyTarget`: a capture hands over the identity it already took on main, and the tap route hands over nothing and lets the probe's own main-side block capture the identity main holds once its work starts. No off-main path reads target identity any more.
main (#2854, #2855) moved the runner's inline unit tests into UnitTests/ and split RunnerTests+CommandExecution.swift along command families. Rebase conflicts dropped this PR's edits from the inline blocks the rebase kept on main; relocate them into the file main now owns for each family: - RunnerTests+SnapshotTests.swift: the query-sweep slice-deadline and fail-closed-invalidation tests, and the bounded-modal-probe helper now takes a SnapshotCaptureTarget. - RunnerTests+SnapshotCapturePlanTests.swift: the tier-timeout rejection pair and the private-AX depth helper now builds a SnapshotCaptureTarget. - RunnerTests+SnapshotTimingTests.swift: the deadline-exhausted tier penalty test.
5385c2a to
7414149
Compare
|
Reviewed at 7414149. The rebase keeps the reviewed behavior from 5385c2a. The snapshot-plan target state stays on main, and the query sweep stops at its slice deadline. The rebased patch matches the reviewed one, apart from one added type annotation. No code on main is reverted. The new commit moves this PR's tests into main's split UnitTests files, and no stale callers remain. I did not build or run the runner XCTest bundle. CI's iOS runner job for 7414149 confirms that the moved tests compile and pass. Smoke Tests is still running and has not failed. This diff touches the iOS snapshot and tap-routing paths, so please check that job before merge. |
|
…dState Target identity, accessibility health and the post-snapshot delay flag move into one @mainactor type, so Swift 5 mode rejects a read or write of them from the command queue instead of leaving the boundary to review (#2882). Every main hop's work closure is @mainactor and runs through one MainActor.assumeIsolated helper; timeoutError and onAbandoned are @sendable, so the #2836 routing-probe shape no longer compiles. Functions that only run on main carry @mainactor; the transport, command queue, capture plan, watchdog and the locked completion/abandonment owner stay nonisolated. Escaping closures carry @mainactor or @sendable: transport completions and in-flight waiters, DispatchSource handlers, the phase-timer clock and the unit-test hooks. The recorder's capture is @mainactor and is called only inside the frame tick's main hop; ScreenRecorder.start takes a non-escaping bootstrap and a @sendable frame. Unit tests that touch the state run on the main actor, or step onto it with MainActor.assumeIsolated where they also dispatch work off main.
…dState Target identity, accessibility health and the post-snapshot delay flag move into one @mainactor type, so Swift 5 mode rejects a read or write of them from the command queue instead of leaving the boundary to review (#2882). Every main hop's work closure is @mainactor and runs through one MainActor.assumeIsolated helper; timeoutError and onAbandoned are @sendable, so the #2836 routing-probe shape no longer compiles. Functions that only run on main carry @mainactor; the transport, command queue, capture plan, watchdog and the locked completion/abandonment owner stay nonisolated. Escaping closures carry @mainactor or @sendable: transport completions and in-flight waiters, DispatchSource handlers, the phase-timer clock and the unit-test hooks. The recorder's capture is @mainactor and is called only inside the frame tick's main hop; ScreenRecorder.start takes a non-escaping bootstrap and a @sendable frame. Unit tests that touch the state run on the main actor, or step onto it with MainActor.assumeIsolated where they also dispatch work off main.
…dState Target identity, accessibility health and the post-snapshot delay flag move into one @mainactor type, so Swift 5 mode rejects a read or write of them from the command queue instead of leaving the boundary to review (#2882). Every main hop's work closure is @mainactor and runs through one MainActor.assumeIsolated helper; timeoutError and onAbandoned are @sendable, so the #2836 routing-probe shape no longer compiles. Functions that only run on main carry @mainactor; the transport, command queue, capture plan, watchdog and the locked completion/abandonment owner stay nonisolated. Escaping closures carry @mainactor or @sendable: transport completions and in-flight waiters, DispatchSource handlers, the phase-timer clock and the unit-test hooks. The recorder's capture is @mainactor and is called only inside the frame tick's main hop; ScreenRecorder.start takes a non-escaping bootstrap and a @sendable frame. Unit tests that touch the state run on the main actor, or step onto it with MainActor.assumeIsolated where they also dispatch work off main.
…dState Target identity, accessibility health and the post-snapshot delay flag move into one @mainactor type, so Swift 5 mode rejects a read or write of them from the command queue instead of leaving the boundary to review (#2882). Every main hop's work closure is @mainactor and runs through one MainActor.assumeIsolated helper; timeoutError and onAbandoned are @sendable, so the #2836 routing-probe shape no longer compiles. Functions that only run on main carry @mainactor; the transport, command queue, capture plan, watchdog and the locked completion/abandonment owner stay nonisolated. Escaping closures carry @mainactor or @sendable: transport completions and in-flight waiters, DispatchSource handlers, the phase-timer clock and the unit-test hooks. The recorder's capture is @mainactor and is called only inside the frame tick's main hop; ScreenRecorder.start takes a non-escaping bootstrap and a @sendable frame. Unit tests that touch the state run on the main actor, or step onto it with MainActor.assumeIsolated where they also dispatch work off main.
…e fail the runner gates (#2911) * refactor(ios-runner): own main-thread capture state in RunnerMainOwnedState Target identity, accessibility health and the post-snapshot delay flag move into one @mainactor type, so Swift 5 mode rejects a read or write of them from the command queue instead of leaving the boundary to review (#2882). Every main hop's work closure is @mainactor and runs through one MainActor.assumeIsolated helper; timeoutError and onAbandoned are @sendable, so the #2836 routing-probe shape no longer compiles. Functions that only run on main carry @mainactor; the transport, command queue, capture plan, watchdog and the locked completion/abandonment owner stay nonisolated. Escaping closures carry @mainactor or @sendable: transport completions and in-flight waiters, DispatchSource handlers, the phase-timer clock and the unit-test hooks. The recorder's capture is @mainactor and is called only inside the frame tick's main hop; ScreenRecorder.start takes a non-escaping bootstrap and a @sendable frame. Unit tests that touch the state run on the main actor, or step onto it with MainActor.assumeIsolated where they also dispatch work off main. * build(ios-runner): fail runner builds on actor-isolation diagnostics Swift 5 mode reports several off-main uses of main-actor state only as warnings, for example a RunnerMainOwnedState read inside a DispatchQueue.async closure. build-xcuitest-apple.sh now tees the build-for-testing log, keeps xcodebuild's exit status, and after a successful build fails on any Swift diagnostic line, warning or error, that contains "actor-isolated" or "loses global actor". Other warnings stay ungated. The script says so when it reuses DerivedData, because an incremental build prints diagnostics only for the files it recompiles. * chore(gates): key the runner build cache on the isolation scan The build-for-testing step now runs scripts/runner-isolation-diagnostics.ts, so a change to the scan misses the cache and is exercised on a cold build, like the other scripts that build runs. * refactor(ios-runner): enter the main actor through one shared runOnMainActor The quiescence-skip block converted a @mainactor block to XCTest's nonisolated block parameter, which the compiler accepts without a diagnostic. It is now a plain block that enters isolation through runOnMainActor, as do both applyMainOwnedSnapshotState hops, so every main-actor entry checks the thread in one internal helper. * fix(ios-runner): capture the runner weakly after a response send * build(ios-runner): give the isolation scan a compiled positive control The runner gate builds compile RunnerIsolationCanary.swift with the runner's own flags, and the scan fails unless every canary line carries a concurrency diagnostic, so a reworded diagnostic fails the gate instead of passing it. The scan also matches the ActorIsolatedCall and SendableClosureCaptures groups and Sendable-capture prose. A failed scan drops the intermediates so a rerun recompiles and rescans every file. The npm package does not ship the canary. * refactor(ios-runner): hold main's new gesture-policy and alert-test code to the main-owned state performSynthesizedGesture reads accessibility health through mainOwned and runs on the main actor with its callers; the unit tests that set the health or resolve alerts on main carry @mainactor. * refactor(ios-runner): name the tuple send sendResult and start test recordings through one helper * test(gates): state where the isolation canary compiles and drop the duplicate line-count test
1 participant
Summary
Two related iOS runner snapshot-plan bugs:
snapshotAccessibilityUnavailablewrote bundleidentity and accessibility health directly instead of keeping snapshot-plan target state on
main, risking state for the wrong target. Fix: take target identity on main and hop plan-state
writes behind abandoned main-thread work. The coordinate tap's system-modal routing probe now
reads
currentBundleIdonly on main too.slice deadline, letting a slow query starve later tiers. A sweep that ends on its slice deadline
is now a typed tier timeout (
SnapshotTierOutcome.deadlineExhausted), so it arms the XCTest-channelpenalty and falls through to private AX — the plan branches on the tier outcome, not node count.
Closes #2781, Closes #2783.
Validation
Rebased onto
origin/main(fd8277e391); head7414149edb. main had moved the runner's inline unittests into
UnitTests/(#2854) and splitRunnerTests+CommandExecution.swiftalong command families(#2855), so the conflicts were file-location/file-split, not semantic. Adopted main's layout: the
snapshot-command production changes live in
RunnerTests+SnapshotExecution.swift, the routing probe inRunnerTests+CommandDispatch.swift, and this PR's tests in theUnitTests/…Tests.swiftfile main nowowns for each family (final commit
test(ios): place this PR's runner tests in main's split UnitTests files). NoRunnerTests+CommandExecution.swiftcontent was resurrected and no test main carries wasduplicated.
Both re-opened findings were re-proven on the rebased sources on an iPhone 17 / iOS 26.2 simulator
(created for the run, deleted after); the runner
TEST BUILD SUCCEEDEDwithAGENT_DEVICE_RUNNER_UNIT_TESTSand both pin tests passed, so main's refactor broke neither — notest-first red-fix was required:
testQuerySweepThatEndsOnItsSliceDeadlinePenalizesChannelAndReachesPrivateAX— passed (4.7s). Asserts the armed
queries_backend_timeoutpenalty, the next plan deferring itsXCTest tiers, and
private-axanswering.testCoordinateTapRoutingPenalizesTheIdentityMainSettledOnWhileTheWriteWasPending— passed (7.9s). Asserts the abandoned routing probe penalizes the identity main settled on, not the
one whose write was still pending.
The 9 relocated unit tests over the moved code also pass (query-sweep slice helpers, the tier-rejection
pair, the fail-closed invalidation-on-main hop, the
snapshotFast/snapshotRawmodal-probe route, theviewport bundle-id test):
Executed 9 tests, with 0 failures.Gates at
7414149edb:pnpm formatclean,pnpm lintandpnpm typecheckpass,pnpm check:xctest-selectionclean (295 declaredAgentDeviceRunnerUITestsmethods, 0 reachable by nolane),
pnpm check:affected --runall runnable checks passed.The requested live triple (tree tier fails fast while the sweep is slow, then
reason=queries_backend_timeout, thenprivate-ax) is not reachable through the CLI on a stocksurface: the tree tier answers first and the runner has no fault-injection seam for a real
kAXErrorIllegalArgumentrefusal or 50 000 presented nodes. The on-simulator XCTest above is theequivalent proof — it plants the fast tree failure and a gated slow sweep and asserts the full
penalize → defer →
private-axsequence.