fix: memoize snapshot occlusion coverage checks to stop a daemon CPU-spin - #1541
Conversation
…spin Root-caused a deterministic daemon hang reported against this branch's `.ad` test/replay path (a two-fill Android form wedges the daemon at ~99% CPU indefinitely, blocking the checkout-form-android.ad live evidence). Mechanism: `annotateCoveredSnapshotNodes` (src/snapshot/snapshot-occlusion.ts) asks, for every overlay-classified candidate cover, whether THAT candidate is itself covered by something later — via a recursive call back into `findCoveringNode` (through `visibleCoverRect`). That recursive question was never memoized: resolving position P's answer required resolving every later position Q > P from scratch, and resolving Q required resolving every position after IT from scratch again, giving O(2^overlayPositions.length) work with no bound. A live CDP pause on the wedged daemon (`kill -USR1`, `Debugger.pause` over the inspector) landed repeatedly inside exactly this recursive triad (`findCoveringNode` -> `canCoverPoint` -> `visibleCoverRect` -> `findCoveringNode`), matching the reporter's own `sample` profile (role-normalization / `normalizeType` hot, called from inside this loop). The pathological input is real, not synthetic: the second `fill` in a two-field Android form runs while the on-screen IME keyboard is open, and each individual key is classified `isAdditionalOverlayNode` — roughly 40 mutually-adjacent "overlay-like" nodes, confirmed by instrumenting the function directly against the live repro (`nodes=62 overlayPositions=39` right where the daemon stops responding). A/B against the p4a branch head with matching instrumentation shows the equivalent snapshot there carries zero overlay-classified nodes at the same point in the script and completes in ~7s; extending the gap between fills with a genuine (non-instant) real wait on p4a does not reproduce the 39-overlay state either, ruling out a pure timing race as the sole explanation. `snapshot-occlusion.ts` itself is untouched by the codec extraction, so the exponential blowup is a pre-existing latent algorithmic defect — this PR's consumer rewiring is the first thing to reliably land the fill-resolution snapshot in the 39-overlay-node regime; the exact mechanism connecting the codec/target- identity import changes to that timing shift was not pinned to a single line, and is called out as a residual question in the PR body. Fix: cache `findCoveringNode`'s answer per position on the scan object, scoped to one `annotateCoveredSnapshotNodes` call. The scan's own node list is immutable input for the duration of one pass (byIndex is only ever extended forward, never revised for a position already resolved), so a given position's covered-by-something-later answer is provably stable across every path that asks it — caching turns the unbounded double recursion into O(K) resolutions of O(K) work each, i.e. O(K^2) instead of O(2^K). Regression test (src/snapshot/__tests__/snapshot-occlusion.test.ts): constructs a synthetic 40-node "keyboard" (mutually non-overlapping, same-kind overlay-classified nodes, matching the live scale) and asserts `annotateCoveredSnapshotNodes` returns well under a second. Verified the test actually catches the regression: with the memoization reverted, the same test times out (never returns) instead of failing an assertion — it hangs exactly like the daemon did. Two existing-behavior sanity cases (a covered touch target, an uncovered one) guard against a memoization bug silently changing output. Live verification: `node bin/agent-device.mjs test /tmp/m6.ad --platform android` (the reported minimal repro) now passes in ~7.5s with no orphaned daemon, down from a 180s timeout at ~99% CPU. The full two-script run (`checkout-form-android.ad` + `gesture-lab-android.ad --platform android`) passes in ~75s with no orphan. Refs #1478 Co-Authored-By: Claude <noreply@anthropic.com>
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
|
Blocking at |
The memoized pass cached findCoveringNode by position while the annotation loop mutated scan.nodes and scan.byIndex, so a cached answer could predate annotations the caller predicate or ancestor classification would observe — first-evaluation-wins order dependence (present, unmemoized, in the original too). Decisions now evaluate exclusively against the caller's input; covered positions are collected read-only and annotations applied in a separate output pass that never feeds back. Two invariants pinned: the input array and its nodes are never mutated, and a chain (target under a covered sheet under a dialog) resolves identically regardless of evaluation order. Co-Authored-By: Claude <noreply@anthropic.com>
|
Fixed in Note this also removes the order dependence that existed before the memoization (the original mutated Two new invariant tests: (1) the input array and its nodes are byte-identical after the pass and the output is a fresh array; (2) a cover chain (target under a covered sheet under a dialog) resolves identically regardless of order — the covered sheet is disqualified as a cover, the target is covered by the dialog directly. 15/15 snapshot tests green including the 40-node keyboard performance case; typecheck/lint/format clean. Generated by Claude Code |
|
At Also rerun iOS smoke: the exact-head failure is a runner-side |
… predicate The prior invariant tests pass against the mutable implementation too (it copied the array up front, and the chain case never exercised the caller predicate). This one fails against it: a predicate that also matches annotated nodes would, through the ancestor walk over a mutable byIndex, declassify a child overlay mid-pass and flip a later target's outcome. Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed in The fresh push retriggers the full check set including iOS smoke. Generated by Claude Code |
|
Re-review at |
|
Android smoke rerun at Generated by Claude Code |
|
Final readiness check at unchanged head |
thymikee
added
the
ready-for-human
|
1 participant
Cherry-picked from the #1536 branch so main gets it without waiting on the stack:
annotateCoveredSnapshotNodes(src/snapshot/snapshot-occlusion.ts) had an unmemoized mutual recursion —findCoveringNode→canCoverPoint→visibleCoverRect→findCoveringNode— that is O(2^K) in the number of overlay-classified nodes. An Android snapshot taken while the IME keyboard is open classifies each key as an overlay (~39 mutually-adjacent overlay nodes observed live), which wedges the daemon at ~99% CPU indefinitely. This code is reachable on main today; the #1478 stack merely shifted when the trigger state occurs, which is how the livetest-app:replayevidence caught it.Fix: memoize
findCoveringNodeper position for the lifetime of one pass (a position's answer is stable within a pass), turning O(2^K) into O(K^2).Test: synthetic 40-node keyboard case asserting sub-second completion plus two correctness cases — verified to hang (not merely fail) with the memoization reverted.
Live verification on the discovery branch: the deterministic two-fill repro went from a 180s timeout with an orphaned spinning daemon to 6.9s green, and the full checkout+gesture Android replay pair passes in 75s.
Refs #1478