fix(browser): isolate Chrome MCP pending attach aborts by rohitjavvadi · Pull Request #88305 · openclaw/openclaw

rohitjavvadi · 2026-05-30T11:08:35Z

Summary

keep Chrome MCP cached-session attach cancellation scoped to the caller that cancelled
track active waiters while a shared attach is still pending or waiting for session.ready
tear down the attach only when the last waiter cancels, evict cancelled pending entries before a fresh caller can join them, and evict cached sessions before awaited close cleanup

Motivation / root cause

getSession() shared one pending Chrome MCP attach per profile/cache key, but created that shared pending promise with the first caller's AbortSignal. If that first caller cancelled while another caller was also waiting for the same Chrome MCP session, the shared pending attach could reject with the first caller's abort reason and fail the unrelated caller.

The fix gives the shared attach its own abort controller and treats each caller as a waiter. Caller aborts release that waiter. The shared attach is only aborted or closed when no waiters remain, and last-waiter close cleanup removes the session from the cache before awaiting close so a fresh caller starts a new attach instead of reusing a handle being torn down.

Real behavior proof

Behavior addressed: one cancelled Chrome MCP caller no longer cancels another caller waiting on the same shared pending attach; all-cancelled attaches are still cleaned up; fresh callers do not reuse a session that is already closing after last-waiter cancellation.

Real environment tested: macOS local checkout with the Chrome MCP runtime module loaded through node --import tsx and the real listChromeMcpTabs path using a test session factory.

Exact steps or command run after this patch:

node scripts/run-vitest.mjs extensions/browser/src/browser/chrome-mcp.test.ts
node --import tsx --input-type=module  # mixed factory-pending repro/proof
node --import tsx --input-type=module  # mixed ready-pending repro/proof
node --import tsx --input-type=module  # all-aborted-before-ready cleanup proof
node --import tsx --input-type=module  # fresh caller during last-waiter close proof
git diff --check origin/main...HEAD

Evidence before fix:

{
  "first": "rejected",
  "firstReason": "first caller cancelled",
  "second": "rejected",
  "secondReason": "first caller cancelled",
  "closed": 1
}

Evidence after fix:

{
  "proof": "mixed-factory-pending",
  "first": "rejected",
  "firstReason": "first caller cancelled",
  "second": "fulfilled",
  "secondTabs": 1,
  "closed": 0
}

{
  "proof": "mixed-ready-pending",
  "factoryCalls": 1,
  "first": "rejected",
  "firstReason": "first caller cancelled",
  "second": "fulfilled",
  "thirdTabs": 1,
  "closed": 0
}

{
  "proof": "all-aborted-before-ready",
  "factoryCalls": 1,
  "first": "rejected",
  "firstReason": "caller cancelled",
  "closed": 1
}

{
  "proof": "fresh-caller-during-close",
  "factoryCalls": 2,
  "first": "rejected",
  "firstReason": "caller cancelled",
  "secondTabs": 1,
  "closeCounts": [
    1,
    0
  ]
}

Observed result after fix: a remaining waiter can complete and later calls reuse the same session; when every waiter cancels before readiness, the session handle is closed exactly once; when a fresh caller arrives while the old session close is still awaiting, it starts a new attach and does not reuse the closing session.

What was not tested: a live Chrome browser/CDP process was not launched for this proof. The exercised path is the Chrome MCP runtime session cache and cancellation behavior with a controlled session factory.

Regression tests

Added focused coverage in extensions/browser/src/browser/chrome-mcp.test.ts for:

one waiter aborting while another waiter remains during factory attach;
every waiter aborting before the factory resolves;
a fresh caller after an all-waiters-aborted pending attach;
every waiter aborting while session.ready is pending;
a fresh caller arriving while last-waiter abort cleanup is still awaiting close();
a mixed ready-pending wait where one caller aborts, another succeeds, and a later call reuses the same session.

Compatibility / risks

This only changes Chrome MCP cached-session cancellation and cleanup behavior. The main risk is around session lifecycle races, so the patch keeps stale-session, profile-key rotation, and transport-error cleanup behavior intact and adds tests for the cancellation cases that can otherwise leak, duplicate, or reuse closing sessions.

clawsweeper · 2026-05-30T11:15:01Z

Codex review: needs maintainer review before merge. Reviewed May 31, 2026, 9:43 PM ET / 01:43 UTC.

Summary
The PR replaces Chrome MCP cached pending-session promises with waiter-scoped pending-session leases/controllers and adds regression coverage, plus narrow lint-only Foundry and command test-mock adjustments.

PR surface: Source +191, Tests +529. Total +720 across 4 files.

Reproducibility: yes. Current main source shows the shared pending promise is created with the first caller's AbortSignal, and the linked issue/PR describe a gated factory path where aborting the first caller rejects an unrelated waiter; I did not run tests in this read-only review.

Review metrics: 1 noteworthy metric.

Chrome MCP Lifecycle Tests: 10 added focused race cases. The added cases are the main evidence that the new waiter accounting preserves cache cleanup, stale-session, timeout, and ephemeral-probe behavior.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

Browser/runtime owner should accept the session lifecycle risk; a live Chrome/CDP smoke would raise confidence if maintainers want proof beyond the deterministic factory path.

Risk before merge

[P1] Merging changes Chrome MCP cached-session waiter accounting, abort propagation, stale pending eviction, and close timing; a mistake here could close, leak, or reuse the wrong browser session under concurrent cancellation races.
[P1] The supplied proof is strong for deterministic cache and cancellation races, but it does not include a live Chrome/CDP process exercising the same interleaving.

Maintainer options:

Accept After Browser Owner Review (recommended)
If a browser/runtime owner is comfortable with the waiter lease semantics and deterministic proof, land with the existing Chrome MCP regression coverage.
Ask For Live Attach Smoke
If maintainers want stronger runtime confidence, request a redacted live Chrome/CDP attach smoke before merge while keeping the code path unchanged.

Next step before merge

[P2] No narrow automated repair is identified; the remaining action is maintainer/browser-owner acceptance of the Chrome MCP session lifecycle risk and proof depth.

Security
Cleared: The diff does not add dependencies, workflows, secrets handling, package scripts, or new external code execution surfaces; no concrete security or supply-chain concern was found.

Review details

Best possible solution:

Land the waiter-scoped Chrome MCP pending-session lifecycle change after browser/runtime owner acceptance and focused CI, keeping cancellation isolated without adding config or public API surface.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main source shows the shared pending promise is created with the first caller's AbortSignal, and the linked issue/PR describe a gated factory path where aborting the first caller rejects an unrelated waiter; I did not run tests in this read-only review.

Is this the best way to solve the issue?

Yes, this appears to be the right owner-boundary fix: it keeps the repair inside the Chrome MCP session cache, uses an internal shared attach controller, and releases per caller. The remaining question is lifecycle-risk acceptance, not a narrower mechanical fix.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 72bc9ae95231.

Label changes

Label changes:

add proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies terminal before/after proof through the real Chrome MCP module path with a controlled session factory; it is sufficient for this internal cache race, with live Chrome/CDP left as an optional confidence booster.

Label justifications:

P2: This is a normal-priority browser runtime bug fix with limited surface but real concurrent-session impact.
merge-risk: 🚨 session-state: The PR changes how shared Chrome MCP pending sessions are counted, cancelled, cached, evicted, and closed under concurrent waiters.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body supplies terminal before/after proof through the real Chrome MCP module path with a controlled session factory; it is sufficient for this internal cache race, with live Chrome/CDP left as an optional confidence booster.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies terminal before/after proof through the real Chrome MCP module path with a controlled session factory; it is sufficient for this internal cache race, with live Chrome/CDP left as an optional confidence booster.

Evidence reviewed

PR surface:

Source +191, Tests +529. Total +720 across 4 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	3	257	66	+191
Tests	1	529	0	+529
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	4	786	66	+720

What I checked:

Current main still has the reported coupling: On current main, getSession creates one shared pending promise and passes the first caller's signal into createChromeMcpSession, so concurrent callers can await a pending attach that is tied to the first caller's abort state. (extensions/browser/src/browser/chrome-mcp.ts:845, 72bc9ae95231)
PR introduces a shared pending-session object with waiter accounting: The PR head adds PendingChromeMcpSession state, an internal AbortController, and per-waiter release logic so one waiter can abort without cancelling remaining waiters unless it is the last active waiter. (extensions/browser/src/browser/chrome-mcp.ts:880, b5594391470c)
Runtime callers are the browser plugin Chrome MCP paths: Server context tab reads and availability checks call listChromeMcpTabs/ensureChromeMcpAvailable through the lazy Chrome MCP module, so the changed cache lifecycle sits on the browser plugin runtime boundary rather than a public SDK/config surface. (extensions/browser/src/browser/server-context.availability.ts:160, b5594391470c)
Focused regression coverage was added: The added tests cover one waiter aborting while another remains, all-waiters-aborted cleanup, fresh callers during close, ready-pending cancellation, stale transport retries, ephemeral probes, and readiness timeout behavior. (extensions/browser/src/browser/chrome-mcp.test.ts:669, b5594391470c)
PR proof is terminal/runtime-path proof, not live Chrome proof: The PR body includes before/after terminal JSON and reports node scripts/run-vitest.mjs extensions/browser/src/browser/chrome-mcp.test.ts plus node --import tsx repro/proof commands through the real listChromeMcpTabs path with a controlled session factory; it explicitly did not launch a live Chrome/CDP process. (b5594391470c)
History points to the current-main implementation and recent area work: git blame attributes the current-main getSession/pendingSessions implementation to bf777b9, while git log shows recent chrome-mcp maintenance in 27dde7a and the PR's browser lifecycle commits. (extensions/browser/src/browser/chrome-mcp.ts:830, bf777b9af29d)

Likely related people:

Gio Della-Libera: git blame on current main attributes the shared pending Chrome MCP getSession path and first-signal pending attach code to bf777b9. (role: introduced current-main behavior; confidence: medium; commits: bf777b9af29d; files: extensions/browser/src/browser/chrome-mcp.ts, extensions/browser/src/browser/chrome-mcp.test.ts)
steipete: Recent history shows chrome-mcp maintenance in 27dde7a, and this PR head includes multiple browser lifecycle repair commits authored by Peter Steinberger after the initial contributor patch. (role: recent area contributor; confidence: high; commits: 27dde7a4d69b, 09dc7347e2bf, 1a73c4e04770; files: extensions/browser/src/browser/chrome-mcp.ts, extensions/browser/src/browser/chrome-mcp.test.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

rohitjavvadi · 2026-05-30T11:36:53Z

Pushed a narrow repair for the pending cleanup race in 83f91cc50a.

What changed:

release(true) now removes the matching cached Chrome MCP session before awaiting last-waiter close.
Added a regression where the first close is held open, a fresh caller arrives during that window, and the fresh caller must create a second session instead of reusing the closing one.

Verification run after the repair:

node scripts/run-vitest.mjs extensions/browser/src/browser/chrome-mcp.test.ts
# 44 tests passed

git diff --check origin/main...HEAD -- extensions/browser/src/browser/chrome-mcp.ts extensions/browser/src/browser/chrome-mcp.test.ts
# passed

The proof remains scoped to the Chrome MCP session cache and controlled session factory; I did not launch a live Chrome/CDP process for this cleanup interleaving.

@clawsweeper re-review

clawsweeper · 2026-05-30T11:36:56Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

rohitjavvadi · 2026-05-30T11:37:09Z

@clawsweeper re-review

Addressed the last-waiter cleanup finding by evicting the matching cached Chrome MCP session before awaiting close(). Added a focused regression where a fresh caller arrives while the old last-waiter close is still blocked; the fresh caller now starts a new session and succeeds instead of reusing the closing handle.

clawsweeper · 2026-05-30T11:37:11Z

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

Re-review progress:

State: Failed
Detail: The targeted re-review did not finish cleanly. Check the workflow run for details.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26682834497
Updated: 2026-05-30T11:38:38.341Z

rohitjavvadi · 2026-05-30T14:27:02Z

@clawsweeper re-review

Rebased this PR onto latest origin/main so the workflow changed-target expectation fix from main is included. Focused verification after the rebase:

node scripts/run-vitest.mjs test/scripts/test-projects.test.ts
# 134 tests passed

node scripts/run-vitest.mjs extensions/browser/src/browser/chrome-mcp.test.ts
# 44 tests passed

clawsweeper · 2026-05-30T14:27:04Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26686278589
Updated: 2026-05-30T14:33:22.812Z

steipete · 2026-06-01T01:55:33Z

Land-ready verification for b5594391470c888bf8c86750de8e3fc42e39aa45:

Fixed Chrome MCP shared pending attach lifecycle so caller aborts/timeouts do not abort other waiters, last-waiter cleanup closes abandoned sessions, stale ready transports retry once then fail, and ephemeral probes do not inherit or persist pending real attaches.
Also included current-main CI cleanup for the agent-command model-selection mock and Microsoft Foundry provider lint.

Local proof:

node scripts/run-vitest.mjs extensions/browser/src/browser/chrome-mcp.test.ts (51 tests)
node scripts/run-vitest.mjs src/commands/agent.test.ts (24 tests)
pnpm lint:extensions:bundled -- extensions/browser/src/browser/chrome-mcp.ts
node scripts/run-oxlint.mjs extensions/microsoft-foundry/provider.ts
pnpm tsgo:prod
pnpm check:test-types
.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main clean, no accepted/actionable findings

CI proof:

PR check rollup is green: 135 success, 34 skipped, 1 neutral, 0 failing, 0 pending.

Known gap: no live Chrome/CDP manual session was run; the regression coverage exercises the Chrome MCP session factory lifecycle races directly.

* fix(browser): isolate Chrome MCP pending attach aborts * fix(browser): evict closing Chrome MCP sessions * fix(browser): clean chrome mcp pending session lifecycle * fix(browser): handle stale chrome mcp pending sessions * fix(browser): serialize stale chrome mcp replacement * fix(browser): skip cancelled chrome mcp attach * fix(browser): retire timed-out chrome mcp pending sessions * fix(browser): retire stale chrome mcp after readiness * fix(browser): keep shared chrome mcp timeouts isolated * fix(browser): bound stale chrome mcp ready retries * fix(browser): narrow pending session lease release * fix(browser): keep ephemeral probes out of pending attaches * fix(foundry): satisfy provider lint --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>

openclaw-barnacle Bot added size: M proof: supplied External PR includes structured after-fix real behavior proof. labels May 30, 2026

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 30, 2026

rohitjavvadi marked this pull request as ready for review May 30, 2026 11:36

rohitjavvadi marked this pull request as draft May 30, 2026 11:38

rohitjavvadi marked this pull request as ready for review May 30, 2026 11:39

rohitjavvadi marked this pull request as draft May 30, 2026 11:44

rohitjavvadi marked this pull request as ready for review May 30, 2026 11:53

rohitjavvadi force-pushed the fix/browser-chrome-mcp-abort-isolation branch from 83f91cc to 2e8bc90 Compare May 30, 2026 14:26

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 30, 2026

clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 30, 2026

rohitjavvadi force-pushed the fix/browser-chrome-mcp-abort-isolation branch from 2e8bc90 to 7954ffe Compare May 31, 2026 12:37

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 31, 2026

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 1, 2026

steipete force-pushed the fix/browser-chrome-mcp-abort-isolation branch from 3c182fd to 8c9dada Compare June 1, 2026 00:10

clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 1, 2026

steipete force-pushed the fix/browser-chrome-mcp-abort-isolation branch from 8c9dada to 398de8b Compare June 1, 2026 00:45

openclaw-barnacle Bot added the commands Command implementations label Jun 1, 2026

steipete force-pushed the fix/browser-chrome-mcp-abort-isolation branch from 398de8b to edcb5d0 Compare June 1, 2026 01:14

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 1, 2026

clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 1, 2026

rohitjavvadi and others added 13 commits June 1, 2026 02:24

fix(browser): isolate Chrome MCP pending attach aborts

7455cc6

fix(browser): evict closing Chrome MCP sessions

b3e567f

fix(browser): clean chrome mcp pending session lifecycle

09dc734

fix(browser): handle stale chrome mcp pending sessions

1a73c4e

fix(browser): serialize stale chrome mcp replacement

52c4346

fix(browser): skip cancelled chrome mcp attach

6ecb5e1

fix(browser): retire timed-out chrome mcp pending sessions

4b0f754

fix(browser): retire stale chrome mcp after readiness

18816df

fix(browser): keep shared chrome mcp timeouts isolated

9b1d147

fix(browser): bound stale chrome mcp ready retries

a9e2f39

fix(browser): narrow pending session lease release

19b742a

fix(browser): keep ephemeral probes out of pending attaches

69048f1

fix(foundry): satisfy provider lint

b559439

steipete force-pushed the fix/browser-chrome-mcp-abort-isolation branch from edcb5d0 to b559439 Compare June 1, 2026 01:35

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 1, 2026

clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 1, 2026

steipete merged commit 3fc485c into openclaw:main Jun 1, 2026
170 checks passed

github-actions Bot mentioned this pull request Jun 1, 2026

📡 Upstream Digest — 2026-06-01 02:51 UTC curtismercier/openclaw-mods#995

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(browser): isolate Chrome MCP pending attach aborts#88305

fix(browser): isolate Chrome MCP pending attach aborts#88305
steipete merged 13 commits into
openclaw:mainfrom
rohitjavvadi:fix/browser-chrome-mcp-abort-isolation

rohitjavvadi commented May 30, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented May 30, 2026 •

edited

Loading

Uh oh!

rohitjavvadi commented May 30, 2026

Uh oh!

clawsweeper Bot commented May 30, 2026 •

edited

Loading

Uh oh!

rohitjavvadi commented May 30, 2026

Uh oh!

clawsweeper Bot commented May 30, 2026 •

edited

Loading

Uh oh!

rohitjavvadi commented May 30, 2026

Uh oh!

clawsweeper Bot commented May 30, 2026 •

edited

Loading

Uh oh!

steipete commented Jun 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

rohitjavvadi commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation / root cause

Real behavior proof

Regression tests

Compatibility / risks

Uh oh!

clawsweeper Bot commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rohitjavvadi commented May 30, 2026

Uh oh!

clawsweeper Bot commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rohitjavvadi commented May 30, 2026

Uh oh!

clawsweeper Bot commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rohitjavvadi commented May 30, 2026

Uh oh!

clawsweeper Bot commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented Jun 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rohitjavvadi commented May 30, 2026 •

edited

Loading

clawsweeper Bot commented May 30, 2026 •

edited

Loading

clawsweeper Bot commented May 30, 2026 •

edited

Loading

clawsweeper Bot commented May 30, 2026 •

edited

Loading

clawsweeper Bot commented May 30, 2026 •

edited

Loading