Skip to content

feat: cancel timed-out tasks; late ref writes cannot overwrite newer refs - #64

Open
cryptotavares wants to merge 5 commits into
cryptotavares/mmcli-p1-06-observe-level-no-statefrom
cryptotavares/mmcli-p1-08-abort-refs
Open

cryptotavares wants to merge 5 commits into
cryptotavares/mmcli-p1-06-observe-level-no-statefrom
cryptotavares/mmcli-p1-08-abort-refs

Conversation

@cryptotavares

@cryptotavares cryptotavares commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Summary: Cancel timed-out requests cooperatively and fence ref-map commits against aborted or older requests. Capture mobile trees and test IDs together, and settle mobile observations after mutations.

Scope change (2026-10-27): the ref-stability layer (renumbering refs so unchanged elements keep their numbers) was removed in 6944868. Refs are positional again, as on main. See Why no ref stability.

Stack context: mm CLI v2, Phase 1, P1-08 + P1-09, combined in one PR. Base: #62 (cryptotavares/mmcli-p1-06-observe-level-no-state); followed by #65.

Problem

A queue timeout rejects the caller, but the task can keep running and overwrite another request's refs. An observation timeout also leaves collection running.

Mobile observations read test IDs and the tree from separate snapshots. A click can return the old screen or Unlock, busy.

Solution

Keep cancellation and commit fencing at separate boundaries:

  1. Cooperative cancellation: RequestQueue passes an AbortSignal and aborts it before timeout rejection. Tools check supported checkpoints. Observations use a linked controller and their own deadline.
  2. Commit fencing: the server owns a small RefStore (94 lines). Each started task gets a generation. Every ref write (sessionManager.setRefMap / clearRefMap from tools, and the observation) checks the generation and request signal, even when a tool ignores cancellation.
flowchart TD
    A[Queued task starts] --> B[Create signal and ref generation]
    B --> C[Run tool through guarded context]
    C --> D[Optional observation checks linked abort signal]
    C -->|ref write| F{Request current and not aborted?}
    D -->|ref write| F
    F -->|yes| G[Commit map]
    F -->|no| X[Drop write. Keep current refs.]
    T[Queue timeout aborts request] -.-> C
    T -.-> D
    T -.-> F

    classDef new fill:#fff3cd,stroke:#d39e00
    classDef stop fill:#f8d7da,stroke:#c82333
    class B,C,D,F,G,T new
    class X stop
Loading

Yellow boxes are new. Red boxes stop the ref write.

Tools write refs through context.sessionManager, which the daemon proxies: setRefMap and clearRefMap go through the request handle. Other members retain the real receiver, including #private fields. ISessionManager stays unchanged.

Optional captureScreen() derives both mobile outputs from one snapshot; drivers without it retain separate reads. After eligible mutations, retry busy screens and allow one unchanged-screen retry, compared against the last tree the agent received (lastScreen: the delivered observation, else the describe_screen / full accessibility_snapshot tree). Launch, cleanup, and non-batch tools that set refs skip settling.

What changes

Condition or boundary Before After
Queued task times out Remaining work can continue and replace refs Abort checkpoints stop cooperative work; the commit fence drops later ref writes
Observation reaches its own timeout Collection continues after the response Abort collection; the final checkpoint prevents its late ref write
Observation not delivered (observe none) Still became the diff baseline Not recorded; the next diff is against what the agent last saw
list_testids Reads the tree and replaces refs despite returning no refs Collect only test IDs; leave refs unchanged
Mobile discovery and observations Separate snapshots can disagree or show an unsettled screen Share a snapshot when supported; bound post-mutation settle retries

Queue timeout errors use HTTP 500 with TOOL_EXECUTION_FAILED and Tool execution timed out after ${effectiveTimeout}ms. MM_REQUEST_ABORTED is the internal checkpoint response. Cancellation does not undo an action already dispatched to a driver. Refresh with describe-screen before retrying.

Decisions for reviewers

Decision Reason
Keep abort checkpoints and the commit fence A driver operation can finish after cancellation. Ref ownership must remain safe independently. Synchronous abort-handler settlement can win the queue's timeout race.
No ref stability; refs stay positional See Why no ref stability.
The observation's refMap always wins, also after a tool set refs The observation is the last tree in the response, so every ref in it resolves.
lastScreen updates only after a delivered response A timed-out request's tree is never the settle baseline.

Review guide

Read the files in this order:

# File What to check
1 src/server/request-queue.ts, src/types/http.ts Abort-before-reject ordering, bounded settle wait, timeout clamp, and optional context additions.
2 src/server/ref-store.ts Generation and abort checks; toolSetRefs.
3 src/server/create-server.ts Proxy receiver binding (setRefMap, clearRefMap), observation cancellation checkpoints, screenAfterResponse / lastScreen, batch behavior, separate diff baseline.
4 src/tools/utils.ts, src/tools/interaction.ts, src/tools/batch.ts, src/tools/discovery-tools.ts Checkpoints and list_testids tree-read removal.
5 src/platform/types.ts, src/platform/mobile-platform-driver.ts, src/server/screen-settle.ts Optional capture boundary, shared snapshot, busy detection, and bounded retries.
6 Tests beside these modules; README.md, SKILL.md Timeout isolation, matching exceptions, snapshot consistency, and caller recovery guidance.
Testing

At 6944868: yarn test 52 files / 1528 tests pass; coverage 97.03 / 93.95 / 95.21 / 97.19; yarn lint passes. Every branch above it in the stack (#65, #66, #67, #44, #68) was restacked and passes yarn test + yarn lint at its own tip.

Breaking changes

None.

  • ToolContext.signal, IPlatformDriver.captureScreen and ScreenCapture are optional additions.
  • RequestQueue.enqueue now passes the signal to its callback. Existing callbacks that take no argument keep working.

Consumer impact

Consumer Change needed Done on branch
Extension test/e2e/playwright/llm-workflow/ None. It does not use RequestQueue, and its setRefMap/getRefMap implementation is unchanged. n/a
Mobile tests/llm-workflow/ None (same). n/a

Why no ref stability

The first version of this PR also renumbered refs so an element kept its number across observations (stabilizeRefs, a session high-water mark, baseline checks, merging tool and observation refs: about 500 lines of code and 900 of tests). It was removed because the problem it solves is rare:

  • Refs are deterministic per tree: the same screen gives the same refs, and an unchanged screen diffs as empty (device proof below).
  • Since P1-05/P1-06, read commands collect no observation, so they no longer renumber anything.
  • The only case it fixed is an element inserted above the one the agent holds. The response then carries the new refs (the diff lists the shifted nodes, or the full tree when every node shifted), and SKILL.md tells agents to use the refs from the latest response.
  • No baseline defect (B1–B15) was a wrong-element click caused by renumbering.

If real runs show agents clicking stale refs, it can come back as its own ticket.

Proof

After the shrink (6944868, run on the restacked stack tip):

Check Extension iOS Android
Timed-out click, then describe-screen (queueMs 0), then click by ref pass pass pass
describe-screen twice: identical refs 17/17 67/67 242/242
Unchanged screen, 3 taps: diff 4,243 B, +0 −0 4,860 B, +0 −0 5,306 B, +0 −0
Navigate away and back, click by the latest ref pass pass pass

Android navigation observations can still show the previous screen (the new screen mounts about 2 s later). That is the existing Android settle limit noted in #44, not changed here.

Earlier revision (with ref stability), kept for history:

Tested with yalc builds in the Extension (e2e build, headless) and Mobile (dev client on iOS sim and Android emulator). The first full run used an earlier revision (e804b95). The final commit got a targeted mobile recheck plus a smoke run on all three platforms. Details are in the proof file in the mm-cli-v2 workspace.

Check Before (#62) After
click --testid does-not-exist --timeout 500, then describe-screen, then click by ref – works on ext, iOS and Android (queueMs 0, intended element clicked)
Android unlock-click observation login screen ("Unlock, busy") in 3/3 no login or busy in 4/4 (3 wallet screen, 1 tab bar only)
Unchanged-screen diff, iOS (wait-for wallet-screen) 7495 B 7518 B, diff +0 −0
Unchanged-screen diff, Android (click tab-bar-item-Wallet, p50 of 3) 7672 B, observationMs 1924 40–110 KB, observationMs 965–1001 (one snapshot instead of two)
Ext bench p50 (click / get-state / list-testids totalMs) 580 / 5 / 565 582 / 6 / 60 (list_testids no longer reads the tree)

The Android unchanged-screen diff gap in that revision is closed by #67 and #44 (see the table above).

@cryptotavares
cryptotavares added this pull request to stack #63 October 24, 2026 01:07
Comment thread src/server/request-queue.ts Fixed
@cryptotavares
cryptotavares force-pushed the cryptotavares/mmcli-p1-08-abort-refs branch from de9777c to 5131d82 Compare October 24, 2026 01:20
@cryptotavares

Copy link
Copy Markdown
Collaborator Author

Rebased onto #62 (1bbfc70). Fixed CodeQL js/resource-exhaustion in 5131d82: queued task timeouts (from request bodies) are clamped to MAX_QUEUE_TIMEOUT_MS = 600 s. Core: 49 files / 1561 tests, lint 0.

@cryptotavares
cryptotavares force-pushed the cryptotavares/mmcli-p1-08-abort-refs branch from 5131d82 to 69cf663 Compare October 24, 2026 01:32
@cryptotavares
cryptotavares force-pushed the cryptotavares/mmcli-p1-08-abort-refs branch from 69cf663 to 7d0c565 Compare October 24, 2026 01:35
@cryptotavares

Copy link
Copy Markdown
Collaborator Author

CodeQL js/resource-exhaustion on request-queue.ts is the same finding that is already dismissed on main (alert #32, "used in tests"); it re-appears because the lines moved. The timeout is now clamped to MAX_QUEUE_TIMEOUT_MS (600 s) and falls back to the default for non-positive values (7d0c565), and the schemas already cap every timeoutMs. Needs a maintainer to dismiss the re-opened alert.

@cryptotavares cryptotavares changed the title feat: cancel timed-out tasks; stable refs that late writes cannot overwrite feat: cancel timed-out tasks; late ref writes cannot overwrite newer refs Oct 27, 2026
@cryptotavares
cryptotavares marked this pull request as ready for review October 27, 2026 16:22
@cryptotavares
cryptotavares requested a review from a team as a code owner October 27, 2026 16:22
…rwrite

A request that timed out kept running: it could still click, snapshot and
then overwrite the refMap the next request relied on. Refs were renumbered
by every observation and discovery call, so refs an agent held could point
at other elements. On mobile, a click observation could show the old screen.

- RequestQueue passes an AbortSignal to each task and aborts it on timeout.
  ToolContext gains optional `signal` and `setRefMap`. Interaction tools,
  run_steps, discovery tools and the observation check the signal between
  steps (MM_REQUEST_ABORTED, internal).
- New RefStore (server-owned, wraps sessionManager.setRefMap): each request
  takes a generation; writes from an older or aborted request are dropped.
  Tools that call context.sessionManager.setRefMap go through it too (proxy).
  ISessionManager is unchanged.
- Stable refs: discovery tools and observations reuse the previous ref for
  the same element (browser key: role + name + testId; mobile: testId, else
  role + name; elements sharing a key are matched in screen order).
  ToolContext.setRefMap returns the stabilized { refMap, nodes }, which
  describe_screen / accessibility_snapshot return. New elements get refs
  above a session high-water mark, so a removed ref is never reused until
  launch/cleanup. When a tool set refs, the observation merges into them.
- list_testids no longer reads the a11y tree or sets refs (it returns none).
- Mobile observation and describe_screen: one device snapshot for both the
  tree and test IDs (optional IPlatformDriver.captureScreen; ScreenCapture
  type exported). After a mutating or batch call, re-snapshot every 300 ms
  while the screen is busy (", busy" label, progress/activity indicator), at
  most 3 times / 2 s; an unchanged screen gets one retry.
- The observation timeout now aborts the observation instead of leaving it
  running.
CodeQL (js/resource-exhaustion) flagged the queue timer because its
duration comes from the request body. Clamp it to MAX_QUEUE_TIMEOUT_MS.
Ref renumbering (stabilizeRefs, the session high-water mark, baseline
checks, merging tool and observation refs) solved a problem that rarely
happens: refs are already deterministic per tree, so an unchanged screen
keeps its refs and diffs as unchanged, and read commands no longer
collect observations (P1-05/P1-06). When elements are inserted above an
element, the diff (or the full tree) carries the new refs.

Kept:
- AbortSignal per queued task, and RefStore as a generation guard: a
  write from a timed-out or older request never reaches the session
  manager (`context.sessionManager.setRefMap` and `clearRefMap` go
  through it too).
- lastObservation only when delivered.
- Mobile settle after a mutating tool, now compared against the last
  tree the agent received (`lastScreen`: the delivered observation, else
  the describe_screen / full accessibility_snapshot tree). No settle when
  a non-batch tool set refs (it just read the screen).

Changed: the observation's refMap always wins, also after a tool set
refs (describe_screen --observe), so every ref in the last tree of a
response resolves.

Removed: ref-stability.ts, ToolContext.setRefMap, setToolRefMap, the
RefStore baseline / high-water mark / key modes.
@cryptotavares
cryptotavares force-pushed the cryptotavares/mmcli-p1-08-abort-refs branch from 6944868 to 674ef64 Compare October 28, 2026 11:29

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants