fix(browser): cancel Chrome MCP requests on crash#101454
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 3:49 AM ET / 07:49 UTC. Summary PR surface: Source -66, Tests +155, Docs +1. Total +90 across 13 files. Reproducibility: yes. for a source-level reproduction path: current main has Chrome MCP actions and wait paths without SDK-owned timeout/signal propagation. No live paused-breakpoint or target-Chrome crash run was provided or reproduced in this review. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Keep this PR as the candidate fix, remove the hand-edited changelog entry, add redacted live crash/reconnect proof, and then merge after exact-head checks are green. Do we have a high-confidence way to reproduce the issue? Yes for a source-level reproduction path: current main has Chrome MCP actions and wait paths without SDK-owned timeout/signal propagation. No live paused-breakpoint or target-Chrome crash run was provided or reproduced in this review. Is this the best way to solve the issue? Yes for the code direction: SDK-owned RequestOptions plus one route budget is the narrow maintainable fix. Merge readiness still depends on live crash proof and removing the release-owned changelog edit. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e72dadbb3bb1. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source -66, Tests +155, Docs +1. Total +90 across 13 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66e5519925
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const readTabs = async (): Promise<BrowserTab[]> => { | ||
| try { | ||
| const tabs = await listTabs(); | ||
| const tabs = await listTabs(options); |
There was a problem hiding this comment.
The new request budget is only forwarded to listTabs here; when both discovery reads succeed with no tabs, openWhenConfirmedEmpty still calls openTab("about:blank") without the same signal/timeout. For Chrome MCP profiles that path goes through openChromeMcpTab/new_page with no SDK RequestOptions, so a crashed or stuck target during first-tab creation ignores the route abort/action timeout and falls back to the SDK default instead of cancelling promptly. Pass the operation options through the openTab dependency as well.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
66e5519 to
7fd183e
Compare
|
Land-ready proof for exact head
Dependency contract checked directly in
This removes duplicate timeout ownership without relaxing navigation, SSRF, browser-access, or sandbox policy. |
|
Merged via squash.
|
* fix(browser): cancel Chrome MCP requests on crash * test(browser): cover request budget options
* fix(browser): cancel Chrome MCP requests on crash * test(browser): cover request budget options
1 participant
What Problem This Solves
Closes #101070.
Chrome MCP existing-session actions could outlive the browser request that started them.
evaluate, coordinate clicks, wait-loop sleeps, navigation probes, tab discovery, snapshots, uploads, and dialog hooks did not all share the route abort signal and action timeout. A target Chrome crash could therefore leave an agent turn waiting until a deeper default timeout, while OpenClaw's outerPromise.racereturned without cancelling the MCP SDK's correlated request.The report's “indefinite” wording is not universal:
@modelcontextprotocol/sdk1.29.0 has a 60-second default request timeout and rejects in-flight requests when its transport closes. The real OpenClaw gap was that target-browser failure can leave the MCP subprocess alive, and several route paths neither supplied the request signal nor an explicit action budget.Why This Change Was Made
RequestOptions.timeoutandRequestOptions.signal. The SDK then removes its correlation handler instead of leaving the underlying request pending behind an outer race.No config, protocol, provider, permission, or browser feature is removed or added.
User Impact
When attached Chrome dies or a browser request is cancelled, the agent now reaches a bounded terminal error and can reconnect on a later request instead of retaining a dead Chrome MCP request. Existing-session clicks, keyboard input, evaluation, waits, snapshots, screenshots, uploads, and dialog hooks remain available.
Evidence
Remote provider: Blacksmith Testbox, lease
tbx_01kwxpxmb3x1g1prnaa9cs4230(silver-prawn).pnpm test extensions/browser/src/browser/chrome-mcp.test.ts extensions/browser/src/browser/routes/agent.act.existing-session-navigation-guard.test.ts extensions/browser/src/browser/server-context.selection.test.ts— 74 passed.pnpm test extensions/browser/src/browser/routes/agent.existing-session.test.ts extensions/browser/src/browser/routes/dispatcher.abort.test.ts extensions/browser/src/browser/routes/agent.snapshot.test.ts extensions/browser/src/browser/routes/agent.snapshot.timeout.test.ts extensions/browser/src/browser/server-context.existing-session.test.ts— 33 passed.pnpm check:changed -- <12 touched browser files>— production/test typechecks, extension lint, repository guards, and import-cycle checks passed.gpt-5.5, high) found one cleanup issue; fixed with an independent overlay-cleanup signal and a regression assertion. The rerun reported no actionable findings.Focused regressions prove that the SDK receives the timeout/signal, a stuck evaluate cancels and reconnects, wait sleeps abort, coordinate/probe calls share the route budget, initial tab discovery receives it, and label cleanup does not reuse the route signal.
Live Dev Gateway + real OpenAI agent + real target-Chrome crash proof is still pending. This PR does not claim that proof yet and should not land until it is added in a follow-up comment.