Skip to content

fix(ios-runner): enter tree capture before reading snapshot viewport - #2938

Merged
thymikee merged 1 commit into
mainfrom
fix/ios-occupancy-determinism
Sep 25, 2026
Merged

thymikee merged 1 commit into
mainfrom
fix/ios-occupancy-determinism

Conversation

@thymikee

Copy link
Copy Markdown
Member

Summary

Start the required XCTest root capture before reading viewport geometry. A cold viewport hop previously consumed its 1 s cap before the blocking tree stub was entered, so the occupancy test failed for the wrong operation. The viewport timeout now names the viewport read. The test asserts the tree stub entered and no longer warms geometry uncapped.

Touches 2 Swift runner files.

Validation

Head 8310fda391: pnpm install --frozen-lockfile, pnpm build, pnpm format, pnpm check:xctest-selection, and pnpm check:affected --run passed. iOS 26.2 focused XCTest passed with tree_capture abandoned and private-AX recovery.

Planted red: a temporary 2 s delay in the original viewport hop reproduced the CI failure: the viewport timed out and the tree-stub entry assertion failed. With tree-first ordering, the same planted delay passed; it was removed before this commit.

GitHub iOS/macOS jobs are pending. Physical-device replay remains manual-only.

@github-actions

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.81 MB 4.81 MB -435 B
Package (unpacked) 4.81 MB 4.81 MB -435 B
Package (download) 1.44 MB 1.44 MB -260 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.0 ms 28.1 ms +0.1 ms
CLI --help 79.7 ms 79.9 ms +0.2 ms

@thymikee

Copy link
Copy Markdown
Member Author

Exact-head live validation is green on 8310fda391: iOS Smoke Tests passed all 64 selected XCTest cases, including testAbandonedTreeCaptureSkipsQuerySweepAndHonorsWarmupExemption (28.826 s). macOS and the remaining PR checks are green; GitHub reports the branch mergeable. This follows the local planted-red/green proof.

The iOS job queued about 7m31s and ran 28m00s. A separate Settings replay first attempt spent 165.128 s in click General, then failed its 5 s wait; the retry passed. That recurring smoke-lane cost is tracked in #2948 and does not change the occupancy test result. I have not merged this PR.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed at 8310fda. The code is ready for human review: moving tree capture before the viewport read closes the ordering gap, extending the same "read geometry after the tree" invariant already used for keyboardBand.

All 18 checks are green at head 8310fda, including the Smoke Tests job that ran the targeted iOS runner XCTest suite for this diff (64 tests, 0 failures, testAbandonedTreeCaptureSkipsQuerySweepAndHonorsWarmupExemption passed in 28.826s), confirmed against this exact SHA.

I could not reproduce the author's planted-red local repro (the temporary 2s viewport delay) since that code was removed before this commit, so I'm relying on the PR body's description of that methodology; I also didn't read the query-sweep tier and private-AX fallback call sites end to end, though grepping the safeSnapshotViewport/makeSnapshotTraversalContext/captureSnapshotRootBounded sites suggests neither races a competing tree XPC call on the same hop, so the fix shouldn't need to apply there — can you confirm that's the case?

Not blocking: box.blockingTreeEntered in RunnerTests+SnapshotCapturePlanOccupancyTests.swift:138 (https://github.com/callstack/agent-device/blob/8310fda/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+SnapshotCapturePlanOccupancyTests.swift#L138) would likely pass under the pre-fix ordering too outside the rare cold-cache/contention window the bug depends on, so it's a known limit of timing-based regression tests rather than something to fix now — take it or leave it.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 24, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Confirmed at 8310fda391. The changed makeSnapshotTraversalContext path is the only tier that reads a viewport and a blocking XCTest root snapshot as separate hops; it now captures the tree before reading geometry. The query sweep reads its viewport before element queries, but both are inside one bounded query_sweep main-thread hop, so it cannot queue a second hop behind its own abandoned tree XPC. After a tree hop is abandoned, the capture plan skips XCTest-backed tiers, including the query sweep. Private AX gets its root through RunnerAXSnapshotBridge first, then reads the XCTest viewport only when no main-thread work is abandoned and the channel is not penalized; that viewport has a 1 s bound and falls back to the bridge root frame. I do not see the same tree-before-viewport ordering gap in those tiers, so I am leaving their paths unchanged.

Agreed on the test limit: blockingTreeEntered makes the intended blocked tier observable, while the planted 2 s viewport delay was the deterministic red proof of the old order. No extra production guard is needed for that observation.

@thymikee
thymikee merged commit 5cd643c into main Sep 25, 2026
18 checks passed
@thymikee
thymikee deleted the fix/ios-occupancy-determinism branch September 25, 2026 05:45
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-25 05:47 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant