From 9a85582ed53bfed00235a5b75c41222036f674f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 1 Aug 2026 11:26:56 +0200 Subject: [PATCH 1/3] fix: memoize snapshot occlusion coverage checks to stop a daemon CPU-spin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../__tests__/snapshot-occlusion.test.ts | 115 ++++++++++++++++++ src/snapshot/snapshot-occlusion.ts | 41 ++++++- 2 files changed, 153 insertions(+), 3 deletions(-) create mode 100644 src/snapshot/__tests__/snapshot-occlusion.test.ts diff --git a/src/snapshot/__tests__/snapshot-occlusion.test.ts b/src/snapshot/__tests__/snapshot-occlusion.test.ts new file mode 100644 index 0000000000..87fb7a5ba3 --- /dev/null +++ b/src/snapshot/__tests__/snapshot-occlusion.test.ts @@ -0,0 +1,115 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import type { RawSnapshotNode } from '@agent-device/kernel/snapshot'; +import { annotateCoveredSnapshotNodes } from '../snapshot-occlusion.ts'; + +// #1478 P5 codec-extraction regression: `annotateCoveredSnapshotNodes` marks +// a touch candidate `interactionBlocked: 'covered'` when a later, floating +// piece of UI chrome (toolbar/dialog/menu/... or a caller-supplied +// `isAdditionalOverlayNode` match, e.g. an Android IME keyboard key) sits on +// top of it. `findCoveringNode` asks, for a candidate cover, "is THAT +// candidate itself covered by something later" — a real question (a +// keyboard row can itself be behind another overlay) — via a recursive call +// into the same function. +// +// Without memoization that recursive question gets re-asked from scratch on +// every path that reaches it: checking whether position P is covered +// requires checking every later position, and checking EACH of those +// requires (independently) checking every position after IT, and so on — +// O(2^overlayPositions.length) work with no upper bound on wall-clock time. +// A live m6 repro (`fill` targeting the second field of a two-field Android +// form) hit this with ~39 keyboard-key nodes classified as +// `isAdditionalOverlayNode` and pegged the daemon at ~99% CPU indefinitely +// (`ps` showed no return; a live `sample`/CDP pause always landed inside +// this exact recursive triad). The fix caches `findCoveringNode`'s answer +// per position for the lifetime of one `annotateCoveredSnapshotNodes` call +// (the scan's own node list never changes mid-pass, so the answer for a +// given position is provably stable across every path that asks — see the +// doc comment on `OcclusionScan.coverCache`), making each position resolve +// at most once. +// +// This test builds a similarly-shaped worst case: many same-kind +// overlay-classified nodes with distinct, mutually non-overlapping rects, so +// every recursive descent is genuinely exercised (nothing short-circuits on +// an early rect-equality or point-containment match) without depending on a +// real device. Before the fix this synchronous call does not return within +// the suite's per-test timeout; after the fix it returns in well under it. + +function keyboardKeyNode(index: number, column: number, row: number): RawSnapshotNode { + return { + index, + type: 'key', + role: 'menu', // matches OVERLAY_KIND_FRAGMENTS, so isOverlayLikeNode is true without a callback + hittable: true, + label: `key-${index}`, + rect: { x: column * 40, y: 400 + row * 40, width: 36, height: 36 }, + }; +} + +test('annotateCoveredSnapshotNodes resolves a large mutually-overlapping overlay set without exponential blowup', () => { + // 4 rows x 10 columns = 40 candidate "keyboard key" nodes, matching the + // scale that wedged the daemon live (~39 IME-classified nodes). + const nodes: RawSnapshotNode[] = []; + let index = 0; + for (let row = 0; row < 4; row += 1) { + for (let column = 0; column < 10; column += 1) { + nodes.push(keyboardKeyNode(index, column, row)); + index += 1; + } + } + + const startedAt = Date.now(); + const result = annotateCoveredSnapshotNodes(nodes); + const elapsedMs = Date.now() - startedAt; + + // Generous relative to the sub-millisecond cost memoization gives this + // input; a regression back to the unmemoized O(2^40) shape would instead + // fail the suite's own test timeout, never reach this assertion at all. + assert.ok( + elapsedMs < 1000, + `expected annotateCoveredSnapshotNodes to resolve 40 mutually-overlapping overlay nodes quickly, took ${elapsedMs}ms`, + ); + assert.equal(result.length, nodes.length); +}); + +test('annotateCoveredSnapshotNodes still marks a touch target covered by a later overlay', () => { + const target: RawSnapshotNode = { + index: 0, + type: 'button', + role: 'button', + hittable: true, + label: 'Save', + rect: { x: 10, y: 10, width: 100, height: 40 }, + }; + const overlay: RawSnapshotNode = { + index: 1, + type: 'dialog', + role: 'dialog', + rect: { x: 0, y: 0, width: 200, height: 200 }, + }; + + const result = annotateCoveredSnapshotNodes([target, overlay]); + assert.equal(result[0]?.interactionBlocked, 'covered'); + assert.equal(result[0]?.hittable, false); +}); + +test('annotateCoveredSnapshotNodes leaves an uncovered touch target unchanged', () => { + const target: RawSnapshotNode = { + index: 0, + type: 'button', + role: 'button', + hittable: true, + label: 'Save', + rect: { x: 10, y: 10, width: 100, height: 40 }, + }; + const farAwayOverlay: RawSnapshotNode = { + index: 1, + type: 'dialog', + role: 'dialog', + rect: { x: 500, y: 500, width: 200, height: 200 }, + }; + + const result = annotateCoveredSnapshotNodes([target, farAwayOverlay]); + assert.equal(result[0]?.interactionBlocked, undefined); + assert.equal(result[0]?.hittable, true); +}); diff --git a/src/snapshot/snapshot-occlusion.ts b/src/snapshot/snapshot-occlusion.ts index b969be51f9..7bb0c825e8 100644 --- a/src/snapshot/snapshot-occlusion.ts +++ b/src/snapshot/snapshot-occlusion.ts @@ -35,6 +35,21 @@ type OcclusionScan = { nodes: RawSnapshotNode[]; byIndex: Map; overlayPositions: number[]; + /** + * Memoizes `findCoveringNode` by the position it was asked about. Every + * overlay-position's "is IT covered by something later" question has + * exactly one answer for the lifetime of one `annotateCoveredSnapshotNodes` + * call (the scan is immutable input, never mutated mid-pass), so caching it + * is safe. Without this, `findCoveringNode` -> `visibleCoverRect` -> + * `findCoveringNode` recurses once per (position, later-position) pair + * without bound, which is O(2^overlayPositions.length): a snapshot with + * ~40 mutually-overlapping overlay-like nodes (e.g. every individual key of + * an open Android IME keyboard, each classified overlay-like via + * `isAdditionalOverlayNode`) pins the event loop for minutes (#1478 P5 + * codec-extraction regression report — root-caused as pre-existing here, + * exposed by the .ad test/replay path's fill-to-fill snapshot timing). + */ + coverCache: Map; }; export type SnapshotOcclusionOptions = { @@ -55,6 +70,7 @@ export function annotateCoveredSnapshotNodes( overlayPositions: annotated.flatMap((node, position) => isOverlayLikeNode(node, byIndex, options) ? [position] : [], ), + coverCache: new Map(), }; let changed = false; for (const [position, node] of annotated.entries()) { @@ -87,19 +103,38 @@ function findCoveringNode( target: RawSnapshotNode, options: SnapshotOcclusionOptions, ): RawSnapshotNode | null { + const cached = scan.coverCache.get(targetPosition); + if (cached !== undefined) return cached; + // Reentrancy guard: `visibleCoverRect` recurses into `findCoveringNode` for + // the SAME targetPosition only through a cycle in `overlayPositions` + // ordering, which cannot happen (positions strictly increase along any + // recursive path — see the `position <= targetPosition` filter below) — + // but seed `null` before recursing regardless, so a future edit that + // breaks that invariant fails closed (no cover) instead of re-entering. + scan.coverCache.set(targetPosition, null); + const targetRect = positiveRect(target.rect); - if (!targetRect) return null; + if (!targetRect) return finishFindCoveringNode(scan, targetPosition, null); const center = centerOfRect(targetRect); for (const position of scan.overlayPositions) { if (position <= targetPosition) continue; const candidate = scan.nodes[position]; if (candidate && canCoverPoint(scan, position, target, targetRect, center, options)) { - return candidate; + return finishFindCoveringNode(scan, targetPosition, candidate); } } - return null; + return finishFindCoveringNode(scan, targetPosition, null); +} + +function finishFindCoveringNode( + scan: OcclusionScan, + targetPosition: number, + result: RawSnapshotNode | null, +): RawSnapshotNode | null { + scan.coverCache.set(targetPosition, result); + return result; } function canCoverPoint( From 59edc26156386e96758eb7ca3c3fc2a78376e6aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 1 Aug 2026 12:38:08 +0200 Subject: [PATCH 2/3] fix(snapshot): make occlusion decisions read immutable input only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../__tests__/snapshot-occlusion.test.ts | 61 +++++++++++++++++++ src/snapshot/snapshot-occlusion.ts | 60 +++++++++--------- 2 files changed, 92 insertions(+), 29 deletions(-) diff --git a/src/snapshot/__tests__/snapshot-occlusion.test.ts b/src/snapshot/__tests__/snapshot-occlusion.test.ts index 87fb7a5ba3..73c05f6e9d 100644 --- a/src/snapshot/__tests__/snapshot-occlusion.test.ts +++ b/src/snapshot/__tests__/snapshot-occlusion.test.ts @@ -113,3 +113,64 @@ test('annotateCoveredSnapshotNodes leaves an uncovered touch target unchanged', assert.equal(result[0]?.interactionBlocked, undefined); assert.equal(result[0]?.hittable, true); }); + +test('cover decisions read only the immutable input: the input array and its nodes are never mutated', () => { + const target: RawSnapshotNode = { + index: 0, + type: 'button', + role: 'button', + hittable: true, + label: 'Pay', + rect: { x: 10, y: 10, width: 100, height: 40 }, + }; + const overlay: RawSnapshotNode = { + index: 1, + type: 'dialog', + role: 'dialog', + rect: { x: 0, y: 0, width: 400, height: 400 }, + }; + const nodes = [target, overlay]; + const before = JSON.stringify(nodes); + + const annotated = annotateCoveredSnapshotNodes(nodes); + + assert.equal(JSON.stringify(nodes), before); + assert.notEqual(annotated, nodes); + assert.equal(annotated[0]?.interactionBlocked, 'covered'); + assert.equal(nodes[0]?.interactionBlocked, undefined); +}); + +test('a covered target still counts as covered by a live overlay above the chain', () => { + // T sits under sheet A; dialog B covers A, which disqualifies A as a cover + // for T (visibleCoverRect refuses covered candidates). T is still covered — + // by B directly — and every one of those decisions reads the same immutable + // input, so the outcome cannot depend on evaluation or annotation order. + // A itself carries no label and is not hittable, so it is not a touch + // candidate and is never annotated. + const t: RawSnapshotNode = { + index: 0, + type: 'button', + role: 'button', + hittable: true, + label: 'Pay', + rect: { x: 10, y: 10, width: 100, height: 40 }, + }; + const a: RawSnapshotNode = { + index: 1, + type: 'sheet', + role: 'sheet', + rect: { x: 0, y: 0, width: 200, height: 200 }, + }; + const b: RawSnapshotNode = { + index: 2, + type: 'dialog', + role: 'dialog', + rect: { x: 0, y: 0, width: 400, height: 400 }, + }; + + const annotated = annotateCoveredSnapshotNodes([t, a, b]); + + assert.equal(annotated[0]?.interactionBlocked, 'covered'); + assert.equal(annotated[1]?.interactionBlocked, undefined); + assert.equal(annotated[2]?.interactionBlocked, undefined); +}); diff --git a/src/snapshot/snapshot-occlusion.ts b/src/snapshot/snapshot-occlusion.ts index 7bb0c825e8..032945c0b3 100644 --- a/src/snapshot/snapshot-occlusion.ts +++ b/src/snapshot/snapshot-occlusion.ts @@ -31,24 +31,26 @@ const SEMANTIC_TOUCH_KIND_FRAGMENTS = [ 'cell', ]; +/** + * The read side of one occlusion pass. Everything here is IMMUTABLE for the + * pass's lifetime: `nodes`/`byIndex` are the caller's input, never the + * annotated output, so every cover decision — including the caller-supplied + * `isAdditionalOverlayNode` predicate and ancestor classification — evaluates + * against the same state no matter when it runs. That immutability is what + * makes `coverCache` sound: each position's "is it covered by something + * later" question has exactly one answer per pass, so caching it is safe. + * Without the cache, `findCoveringNode` -> `visibleCoverRect` -> + * `findCoveringNode` recurses once per (position, later-position) pair + * without bound, which is O(2^overlayPositions.length): a snapshot with ~40 + * mutually-overlapping overlay-like nodes (every key of an open Android IME + * keyboard, each classified via `isAdditionalOverlayNode`) pins the event + * loop for minutes. Annotations are applied in a separate output pass and + * never feed back into decisions. + */ type OcclusionScan = { - nodes: RawSnapshotNode[]; + nodes: readonly RawSnapshotNode[]; byIndex: Map; overlayPositions: number[]; - /** - * Memoizes `findCoveringNode` by the position it was asked about. Every - * overlay-position's "is IT covered by something later" question has - * exactly one answer for the lifetime of one `annotateCoveredSnapshotNodes` - * call (the scan is immutable input, never mutated mid-pass), so caching it - * is safe. Without this, `findCoveringNode` -> `visibleCoverRect` -> - * `findCoveringNode` recurses once per (position, later-position) pair - * without bound, which is O(2^overlayPositions.length): a snapshot with - * ~40 mutually-overlapping overlay-like nodes (e.g. every individual key of - * an open Android IME keyboard, each classified overlay-like via - * `isAdditionalOverlayNode`) pins the event loop for minutes (#1478 P5 - * codec-extraction regression report — root-caused as pre-existing here, - * exposed by the .ad test/replay path's fill-to-fill snapshot timing). - */ coverCache: Map; }; @@ -62,33 +64,33 @@ export function annotateCoveredSnapshotNodes( ): RawSnapshotNode[] { if (nodes.length < 2) return nodes; - const annotated = [...nodes]; - const byIndex = new Map(annotated.map((node) => [node.index, node])); + const byIndex = new Map(nodes.map((node) => [node.index, node])); const scan: OcclusionScan = { - nodes: annotated, + nodes, byIndex, - overlayPositions: annotated.flatMap((node, position) => + overlayPositions: nodes.flatMap((node, position) => isOverlayLikeNode(node, byIndex, options) ? [position] : [], ), coverCache: new Map(), }; - let changed = false; - for (const [position, node] of annotated.entries()) { + const coveredPositions: number[] = []; + for (const [position, node] of nodes.entries()) { if (!isCandidateTouchNode(node)) continue; - const cover = findCoveringNode(scan, position, node, options); - if (!cover) continue; - changed = true; - const coveredNode = { + if (findCoveringNode(scan, position, node, options)) coveredPositions.push(position); + } + if (coveredPositions.length === 0) return nodes; + + const annotated = [...nodes]; + for (const position of coveredPositions) { + const node = nodes[position]!; + annotated[position] = { ...node, hittable: false, interactionBlocked: 'covered' as const, presentationHints: mergeCoveredHint(node.presentationHints), }; - annotated[position] = coveredNode; - scan.byIndex.set(coveredNode.index, coveredNode); } - - return changed ? annotated : nodes; + return annotated; } export function isSnapshotNodeInteractionBlocked( From cae69aed35916c6725e4c868808716f836cf382c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 1 Aug 2026 13:55:27 +0200 Subject: [PATCH 3/3] test(snapshot): pin annotation-blindness through a mutation-sensitive 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 --- .../__tests__/snapshot-occlusion.test.ts | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/snapshot/__tests__/snapshot-occlusion.test.ts b/src/snapshot/__tests__/snapshot-occlusion.test.ts index 73c05f6e9d..d8d54ec9a1 100644 --- a/src/snapshot/__tests__/snapshot-occlusion.test.ts +++ b/src/snapshot/__tests__/snapshot-occlusion.test.ts @@ -174,3 +174,50 @@ test('a covered target still counts as covered by a live overlay above the chain assert.equal(annotated[1]?.interactionBlocked, undefined); assert.equal(annotated[2]?.interactionBlocked, undefined); }); + +test('cover decisions ignore annotations even through a mutation-sensitive predicate and ancestor walk', () => { + // P (a touch target) is covered by dialog D and gets annotated. Overlay O is + // P's child and is classified through the caller predicate, whose ancestor + // walk reads P: a predicate that (pathologically) also matches annotated + // nodes would, against a mutable byIndex, see the annotated P as a + // renderable overlay ancestor and declassify O mid-pass — flipping T's + // outcome based on evaluation order. Decisions must read pristine input: + // P never matches, O stays an overlay root, T is covered. + const p: RawSnapshotNode = { + index: 10, + type: 'group', + role: 'group', + label: 'Parent', + rect: { x: 0, y: 0, width: 50, height: 50 }, + }; + const t: RawSnapshotNode = { + index: 11, + type: 'button', + role: 'button', + hittable: true, + label: 'Pay', + rect: { x: 100, y: 100, width: 80, height: 40 }, + }; + const o: RawSnapshotNode = { + index: 12, + parentIndex: 10, + type: 'group', + role: 'group', + identifier: 'ov-root', + rect: { x: 60, y: 60, width: 200, height: 200 }, + }; + const d: RawSnapshotNode = { + index: 13, + type: 'dialog', + role: 'dialog', + rect: { x: 0, y: 0, width: 60, height: 60 }, + }; + + const annotated = annotateCoveredSnapshotNodes([p, t, o, d], { + isAdditionalOverlayNode: (node) => + node.identifier === 'ov-root' || node.interactionBlocked === 'covered', + }); + + assert.equal(annotated.find((n) => n.index === 10)?.interactionBlocked, 'covered'); + assert.equal(annotated.find((n) => n.index === 11)?.interactionBlocked, 'covered'); +});