Clean up browser MCP subprocess tree#85832
Conversation
|
Behavior proof from this branch:
Process cleanup behavior covered by the new regression:
Also verified generated Chrome MCP args now include Note: |
|
Codex review: needs maintainer review before merge. Latest ClawSweeper review: 2026-05-24 05:49 UTC / May 24, 2026, 1:49 AM ET. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
PR Surface View PR surface stats
Summary Reproducibility: yes. at source level, though I did not run a live current-main leak loop in this read-only review. Current main only calls SDK PR rating Rank-up moves:
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. Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review detailsBest possible solution: Land the browser-plugin-owned cleanup after maintainer sign-off on process-tree termination semantics, then let the linked leak issue close from the merged fix. Do we have a high-confidence way to reproduce the issue? Yes at source level, though I did not run a live current-main leak loop in this read-only review. Current main only calls SDK Is this the best way to solve the issue? Yes, the PR is a narrow browser-plugin fix that keeps ownership local and adds focused regression coverage for the cleanup paths. The safer alternative would be a shared plugin-safe process cleanup seam, but that is a broader design choice rather than required for this bug fix. Label changes:
Label justifications:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 0ba6b235349a. |
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Cosmic Merge Sprite Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
|
Added live process-count proof to the PR body. Tested commit: @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
8f61f28 to
bc07969
Compare
|
Refreshed the live process-count proof on the current PR head. Tested commit:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
* fix: clean up browser MCP subprocess tree * fix: clean up windows browser mcp tree before close * fix(browser): repair chrome mcp cleanup rebase --------- Co-authored-by: Peter Steinberger <[email protected]>
* fix: clean up browser MCP subprocess tree * fix: clean up windows browser mcp tree before close * fix(browser): repair chrome mcp cleanup rebase --------- Co-authored-by: Peter Steinberger <[email protected]>
* fix: clean up browser MCP subprocess tree * fix: clean up windows browser mcp tree before close * fix(browser): repair chrome mcp cleanup rebase --------- Co-authored-by: Peter Steinberger <[email protected]>
* fix: clean up browser MCP subprocess tree * fix: clean up windows browser mcp tree before close * fix(browser): repair chrome mcp cleanup rebase --------- Co-authored-by: Peter Steinberger <[email protected]>
* fix: clean up browser MCP subprocess tree * fix: clean up windows browser mcp tree before close * fix(browser): repair chrome mcp cleanup rebase --------- Co-authored-by: Peter Steinberger <[email protected]>
* fix: clean up browser MCP subprocess tree * fix: clean up windows browser mcp tree before close * fix(browser): repair chrome mcp cleanup rebase --------- Co-authored-by: Peter Steinberger <[email protected]>
* fix: clean up browser MCP subprocess tree * fix: clean up windows browser mcp tree before close * fix(browser): repair chrome mcp cleanup rebase --------- Co-authored-by: Peter Steinberger <[email protected]>
Fixes #85721.
Summary
taskkill /Tstill sees the root treeReal behavior proof
npx -> chrome-devtools-mcp -> helperprocess tree and the Windowstaskkill /Tordering called out in review.fix/browser-mcp-process-cleanup-85721, current headbc07969c9312609ee2a3a72a006a4bc414e3ae86.node --import tsx .tmp/pr85832-live-chrome-mcp-proof.tsnode scripts/run-vitest.mjs extensions/browser/src/browser/chrome-mcp.test.ts extensions/browser/src/browser/server-context.existing-session.test.ts extensions/browser/src/browser/server-context.stop-running-browser.test.ts extensions/browser/src/browser/routes/basic.existing-session.test.tsnode scripts/check-changed.mjs --base origin/mainCurrent-head live process-count proof from the patched checkout (
bc07969c9312609ee2a3a72a006a4bc414e3ae86) against a dedicated headless Google Chrome remote-debugging endpoint:npm exec chrome-devtools-mcp@latest -> chrome-devtools-mcpowned subprocess tree, thencloseChromeMcpSessionremoved the owned tree and returned the global Chrome MCP/helper/watchdog/update-helper process count to zero. The after-fix regression also verifies owned Linux-style cleanup kills the simulated123 -> 124 -> 125Chrome MCP tree child-first and leaves unrelated pid126alone. The new Windows regression verifies cleanup order istaskkill:123beforeclient.close, so the root tree is still identifiable whentaskkill /Truns. Generated Chrome MCP args also include--no-usage-statisticsby default unless an explicit usage-statistics flag is supplied.taskkill /Tordering is covered by the regression test.Validation
node --import tsx .tmp/pr85832-live-chrome-mcp-proof.tsnode scripts/run-vitest.mjs extensions/browser/src/browser/chrome-mcp.test.ts extensions/browser/src/browser/server-context.existing-session.test.ts extensions/browser/src/browser/server-context.stop-running-browser.test.ts extensions/browser/src/browser/routes/basic.existing-session.test.tsnode scripts/check-changed.mjs --base origin/mainPlease preserve author attribution if this PR is squashed/reworked, or include:
Co-authored-by: Andy Ye [email protected]