Skip to content

fix(ios): stop the XCTest-channel penalty from re-arming itself - #3896

Merged
thymikee merged 6 commits into
mainfrom
fix/ios-xctest-penalty-rearm
Oct 22, 2026
Merged

thymikee merged 6 commits into
mainfrom
fix/ios-xctest-penalty-rearm

Conversation

@thymikee

@thymikee thymikee commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Restore normal-plan query-sweep timeout penalties (base threshold >3 s unchanged).
  • Bounded recovery attempts cannot renew the penalty; an accepted tree probe within 1 s clears it early.
  • The penalty is one SnapshotXCTestChannelPenalty value (arm / clear / isProbeDue / claimProbe). Every 30 s a due probe runs in-band: effectiveSnapshotCapturePlan puts the 1 s tree ahead of private AX, so abandoned probe work is charged to that capture and its busy stamp. No XCTest work starts after a response. ADR 0004 updated. 8 files, no knobs.

Refs #3844

Validation

Head 7640bdc6e6 (Swift identical to 092d21aa4a); base 5f6dfb3e6f. Owned iPad Pro M5 / iOS 27.0, Release heavy feed. Both checkouts carry the same uncommitted harness line that keeps iPad off the host AX bridge (#3895), so the runner penalty is exercised.

  • 60 hostile snapshots / 5 min, two runs each: PENALIZED/CLEARED/DEFERRED 3/0/57 → 3/0/57 (7 probes per run); pooled median/p95 1.563/4.923 s → 1.558/4.900 s.
  • Heavy→Settings, 30 light captures: 1/0/24 → 1/1/7; first undeferred capture 122 s → cleared at 32 s; median/p95 0.551/0.631 → 0.431/0.616 s.
  • Snapshot-then-press, 10×: RUNNER_BUSY command=tap 3/3 on 2fd5cd8 → 0/10. The press itself fails on base too (deep-tree point.x != INFINITY, see fix(ios): synthesize long presses and the gesture viewport on deep trees #3893).
  • 83 focused runner XCTests pass, 0 skips; 4 planted mutations detected.
  • check:xctest-selection and check:affected --run --head 7640bdc6e6 passed (exactCommit=true). CI owns unit-ci and device lanes.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.53 MB 5.54 MB +2.9 kB
Package (unpacked) 5.53 MB 5.54 MB +2.9 kB
Package (download) 1.68 MB 1.68 MB +494 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 29.9 ms 33.8 ms +4.0 ms
CLI --help 56.8 ms 61.0 ms +4.2 ms

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed 2fd5cd8. The post-response recovery probe can leave abandoned XCTest work on the main thread, so the next tap can fail with RUNNER_BUSY.

After an accepted private-AX snapshot, scheduleSnapshotXCTestRecoveryProbeIfNeeded queues a probe on the command queue. The probe runs the whole recursiveTree capture inside one main-thread block, so app.snapshot() runs inline with no slice. On the hostile screen that armed the penalty, that call outlasts 1 s. The waiter gives up, and abandonedMainThreadWorkCount stays above 0 until the call drains. The issue's 72 tree_backend_timeout lines are this same 1 s abandonment. The next command, usually the tap after the snapshot, is refused by executeDispatched with RUNNER_BUSY. Mutating commands get no busy resend. The snapshot response already said runnerMainThreadBusy=false, so the daemon's occupancy view is also stale for retention and handoff. On hostile screens, a snapshot then tap loop would get a tap failure every 30 s that the caller did not cause. Before this PR, an accepted private-AX capture left the main thread idle. The rule the code must satisfy: the runner starts no XCTest main-thread work after it has answered a command, so every response's runnerMainThreadBusy stamp stays true until the next command. Please drop the post-response probe. If periodic recovery is still wanted, can the due tree probe run in-band in the deferred plan instead? Then any abandonment is charged to the snapshot response and its busy stamp.

The live runs only measure snapshot sequences. Snapshots skip XCTest tiers while abandoned work drains, so they cannot show the RUNNER_BUSY window. No run sends a mutating command after a scheduled probe. If the probe stays, please run this on the heavy feed. Arm the penalty, wait more than 30 s, and take a snapshot that logs RECOVERY_PROBE_SCHEDULED. Then run press on an @ref right away. The press must succeed, with no AGENT_DEVICE_RUNNER_BUSY command=tap and no MAIN_THREAD_WORK_ABANDONED operation=snapshot_recovery_probe before it. Please attach the runner.log excerpt, including any abandoned or drained lines from the probe. This is not needed if the probe is removed.

Could the PR be smaller? The channelState-gated penalty evidence and the early clear on an accepted fast in-plan tree probe look right and well owned. Could the post-response scheduler, nextRecoveryProbeAt and the stale-generation check go? If light screens still need recovery before expiry, effectiveSnapshotCapturePlan could put the bounded recursiveTree probe ahead of privateAX when the penalty state says a probe is due. Could the penalty fields also become one SnapshotXCTestChannelPenalty type with arm, clear and isProbeDue, so the plan derives the probe order from it and not from a parallel timestamp?

The one failing check is lint, on the 1003-line cap in packages/platform-apple/src/snapshot-route.test.ts. This PR touches only apple/runner Swift files and ADR 0004, and the cap breach comes from main (#3890), so it looks unrelated. I could not tell why the occupancy test lost its second-plan assertion that the armed penalty defers the next plan's XCTest tiers. No other test at this head asserts that a deferred plan starts no sweep query. #3895 shares no code with this PR, but both amend ADR 0004 and both reference #3844, so expect a possible text conflict there. I did not run the XCTest suite or a device. The RUNNER_BUSY finding is traced from the code path and the 1.01 s per-probe timing in the PR body. I did not see it in a runner.log. No conflicts are known. Before merge, the probe needs to go in-band or be removed, or the snapshot then press run above needs to pass on the hostile screen.

thymikee and others added 3 commits October 22, 2026 10:11
* origin/main:
  feat(test-app): add a Bluesky-depth heavy feed screen (#3884)
  fix(android): read a child that vanished mid-read as gone (#3901)
  feat(ios): route iPad Simulator snapshots through the host AX bridge (#3895)
Drop the post-response recovery probe: it started XCTest main-thread work
after the snapshot had answered, so its abandoned tree work could refuse the
next tap with RUNNER_BUSY while the snapshot's occupancy stamp said idle.

The penalty is now one SnapshotXCTestChannelPenalty value (arm, clear,
isProbeDue, claimProbe). When a probe is due, effectiveSnapshotCapturePlan
puts the bounded tree ahead of private AX in the deferred plan, so any
abandoned probe work is charged to that capture and its busy stamp.

Restore the occupancy test's second-plan assertions that an armed penalty
defers the next plan's XCTest tiers and starts no sweep query.

Refs #3844

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the post-response probe text with the in-band policy and note that,
after the iPad amendment, Simulator captures meet it only when the bridge
sends them to XCTest.

Refs #3844

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@thymikee

Copy link
Copy Markdown
Member Author

Thanks. Addressed at 7640bdc6e6 (fix in 092d21aa4a, ADR in 7640bdc6e6, origin/main merged at 5f6dfb3e6f).

1. Post-response probe → dropped. scheduleSnapshotXCTestRecoveryProbeIfNeeded, runScheduledSnapshotXCTestRecoveryProbe, nextRecoveryProbeAt and the stale-generation check are gone. The runner starts no XCTest work after it answers a command. Periodic recovery is now in-band: when a probe is due, effectiveSnapshotCapturePlan(…, xCTestRecoveryProbeDue:) orders the deferred plan [tree(1 s), private-ax] instead of [private-ax, tree]. The plan claims the probe when the tree tier starts. Abandoned probe work is therefore charged to that capture and its runnerMainThreadBusy stamp. The query sweep never joins a deferred plan.

2. One penalty type. SnapshotXCTestChannelPenalty holds the bundle, expiry and probe due time, with arm / clear / isProbeDue / claimProbe. The plan derives the order from isProbeDue, not a parallel timestamp. The channelState-gated evidence and the early clear are unchanged.

3. Tests. I restored main's sweep-timeout occupancy test with both second-plan assertions: an armed penalty defers the next plan's XCTest tiers, and a deferred plan starts no sweep query. New tests:

  • testDueRecoveryProbeRunsInsideTheCaptureAndChargesItsAbandonedWorkToIt: the probe runs before the answer, the answer is busy, there is no renewal, and the next plan in the interval does not enter the tree.
  • testDueRecoveryProbeLeadsOnlyTheDeferredRegularPlanAndNeverAddsTheSweep: pins, the raw plan and the physical-device plan keep their order.
  • A pure penalty-type test.

Four planted mutations are detected: ignoring the due flag, not claiming, sweep in the deferred plan, and a plan that ignores the penalty. The last one fails both restored assertions (tree != private-ax, sweep starts 15 != 14).

4. ADR 0004. The #3895 amendment merged cleanly. Our paragraph now describes the in-band rule and notes that, after the iPad amendment, Simulator captures meet it only when the bridge sends them to XCTest.

5. Your tap check. iPad Pro M5 / iOS 27.0, Release heavy feed, bridge kept off for iPad by one uncommitted harness line in each checkout. Each iteration: arm the penalty, wait 31 s, snapshot -i, then press @<heavy-post-0-like> immediately.

  • 2fd5cd8: 3/3 probes abandoned after the answer, so 3/3 AGENT_DEVICE_RUNNER_BUSY command=tap.
  • Head: 10/10 probes ran in-band and were abandoned at the 1 s slice. In 7/10 the work drained before the answer; in 3/10 the answer was stamped busy and the work drained before the tap arrived. Result: 0/10 RUNNER_BUSY command=tap, and no abandoned work was outstanding at any tap.

The press does not succeed on either revision. On base and head alike, every runner tap on this ≥27-wrapper feed fails with point.x != INFINITY after ~15 s, by @ref and by coordinates; scroll and back fail too. Below 27 wrappers the tree is not hostile and taps work. That is the deep-tree gesture gap (#3893), not this PR.

# 2fd5cd8
10:52:45.755 COMMAND_COMPLETED command=snapshot ok=1
10:52:45.756 SNAPSHOT_XCTEST_RECOVERY_PROBE_SCHEDULED
10:52:46.392 COMMAND_ACCEPTED command=tap
10:52:46.756 MAIN_THREAD_WORK_ABANDONED operation=snapshot_recovery_probe slice=1.0
10:52:46.756 AGENT_DEVICE_RUNNER_BUSY command=tap abandonedForSeconds=0.0
10:52:47.943 MAIN_THREAD_WORK_DRAINED operation=snapshot_recovery_probe
# head, the closest of the 10
11:11:42.082 SNAPSHOT_XCTEST_RECOVERY_PROBE
11:11:43.091 MAIN_THREAD_WORK_ABANDONED operation=tree_capture slice=1.0
11:11:44.269 COMMAND_COMPLETED command=snapshot ok=1        (stamped busy)
11:11:44.273 MAIN_THREAD_WORK_DRAINED operation=tree_capture
11:11:44.875 COMMAND_ACCEPTED command=tap
11:11:59.323 COMMAND_FAILED command=tap  point.x != INFINITY (same on base)

A client that sends the tap within ~4 ms of such an answer would still be refused. That is the same window that every in-plan XCTest tier has today: the arming query sweep abandons the same way. Closing it would need the plan to wait for its own abandoned work, so I left that out of this PR.

6. Cost. 60 hostile snapshots / 5 min, two runs per revision: PENALIZED/CLEARED/DEFERRED 3/0/57 on both, 7 probes per head run, each ~2.25 s runner-side. Pooled median/p95 1.563/4.923 s → 1.558/4.900 s; base p95 alone moves 0.6 s between runs. Heavy→Settings: 1/0/24 → 1/1/7. The first probe cleared the penalty 32 s after arming (168 ms, the tree answered), where base stayed deferred until 122 s.

7. Lint. The snapshot-route.test.ts line-cap failure came from main (#3890); I did not touch it.

@thymikee

Copy link
Copy Markdown
Member Author

This PR is ready for human review at 7640bdc. The earlier finding from the first review is fixed, and all 16 checks pass with none failing. The earlier lint failure on the line cap in snapshot-route.test.ts came from main (#3890), and this PR's logical diff does not touch that file. There are no conflicts, and nothing else must happen before merge besides the human review.

Not blocking, and fine to take or leave: on a hostile screen whose tree takes longer after the 1 s slice than the private-AX tier, a due probe can answer with runnerMainThreadBusy=true (RunnerTests+SnapshotCapturePlan.swift#L437), and the daemon reads that stamp only for retention and handoff, so a mutating command sent right after (for example a replay or batch snapshot then press) can be refused with RUNNER_BUSY and is not resent. On main a deferred capture never touched XCTest, so this window now opens once per 30 s. A follow-up could enforce the rule that a mutating command must not reach a runner whose last response reported busy until that work drains, either by waiting for the drain before dispatch or by a bounded resend of commands that never ran.

On evidence: I did not run the XCTest suite, the mutations, or a device, and the live numbers and the runner.log excerpt are yours. The live run used an iPad Simulator with an uncommitted harness line that kept it off the host AX bridge. On main, local Simulators normally use the bridge, so this runner route is reached in production only on bridge fallback. I judged the forced route equivalent to the runner path, but it was not an unmodified build. I traced the busy window above from code and your timings, and I did not observe it in a replay or batch run.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 22, 2026
@thymikee
thymikee merged commit 1e7092c into main Oct 22, 2026
16 checks passed
@thymikee
thymikee deleted the fix/ios-xctest-penalty-rearm branch October 22, 2026 12:07
@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-22 12:07 UTC

thymikee added a commit that referenced this pull request Oct 22, 2026
* origin/main:
  perf(ios): confirm fill focus from the tapped input before the keyboard (#3906)
  feat(perf): time every iOS command against the fixture app (#3885)
  fix(ios): stop the XCTest-channel penalty from re-arming itself (#3896)
  test(android): pin that a child vanishing mid-read reads as gone (#3905)
  ci: skip device smoke lanes when a PR changes no lane input (#3904)
  fix(ios): verify deep-tree back on the sole screen (#3894)
  fix(ios): synthesize long presses and the gesture viewport on deep trees (#3893)
  perf(ios): read each resolved text input through one snapshot (#3882)
  fix(gate): take the host validation lock for heavy gates (#3900)
  fix(logs): keep an early iOS simulator logs stop from hanging the session (#3892)
  fix(apple): clean the runner derived path the runner uses (#3897)
  test(provider-scenarios): open deadline scenarios without the test IME (#3903)
  test(host-kit): stub os.homedir for the exec-trace late-close test (#3898)
  feat(test-app): add a Bluesky-depth heavy feed screen (#3884)
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