fix(mcp): do not close browser while tool call is running

[Anurag Singh (Anurag-M1)]

Summary

• Prevents the MCP idle timeout from firing and closing the browser while a tool call is actively running.
• Restores callStarted() and callFinished() on IdleTimer with reference counting (_running), ensuring the timer is cleared during tool execution and only re-armed once all active calls complete.
• Keeps poke() guarded so that daemon or client initialization does not arm the timer while calls are in progress, and restores .unref() so pending idle timeouts do not hold the Node event loop open.
• Re-adds regression test does not close the browser while a tool call is running in tests/mcp/idle-timeout.spec.ts.

Fixes #42693

[Anurag Singh (Anurag-M1)]

@microsoft-github-policy-service agree

[Ayaan Gazali (ayaangazali)]

Reporter of #42693 here. Checked this out and ran the exact reproducer from the issue against it, and it fixes the case I filed.

Before, on d1ead3ecc:

wait isError: true | "### Error\nError: No open pages available."
log: {"create browser (persistent)":1,"create context":1,"close browser":1}

On this branch:

wait isError: false | "### Result\nWaited for 3 ..."
log: {"create browser (persistent)":1,"create context":1}

which matches what dc0f85227 did before the regression. tests/mcp/idle-timeout.spec.ts is 3/3 green here too, including the restored does not close the browser while a tool call is running.

Glad to see that test come back specifically. It existed for this, was removed in #42666 while consolidating to one test per scenario, and #42676 then landed without it, which is how the guard went missing quietly. Nothing further from me, just wanted to confirm the fix from the reporting side.

[Devin Rousso (dcrousso)]

this can restart the timer after browser_close, potentially keeping the closed browser alive longer than needed

[Anurag Singh (Anurag-M1)]

Good catch! Pushed a fix:

• Added a _disposed flag to IdleTimer and separated clearing the active timer during calls (_clearTimer) from permanent disposal (dispose).
• Guarded callStarted(), callFinished(), and poke() so that once disposed, the timer is never re-armed.
• Explicitly dispose _idleTimer on browser_close / disconnect so the closed browser instance is not retained.
• Added regression test does not restart the idle timer after browser_close in tests/mcp/idle-timeout.spec.ts.

[Devin Rousso (dcrousso)]

with --isolated clients share this timer so browser_close from one client permanently disables idle cleanup for the remaining clients

[Anurag Singh (Anurag-M1)]

Good point. Removed this._idleTimer?.dispose() from BrowserBackend. In --isolated mode, clients share the same info.idleTimer, so closing one client's context/connection shouldn't disable idle cleanup for remaining clients. The timer is already disposed when the root browser disconnects via info.browser.once('disconnected', () => info.idleTimer?.dispose()) in browserFactory.ts, which sets _disposed = true and prevents callFinished() from restarting the timer.

[Devin Rousso (dcrousso)]

a restarted timer does not emit another close browser log so this wont detect a regression

[Anurag Singh (Anurag-M1)]

Removed this test since formatLog(stderr()) cannot observe whether an already closed browser attempted to close again.

[github-actions]

Test results for "MCP"

3 failed
❌ [chrome] › mcp/cli-core.spec.ts:228 › resize @mcp-macos-latest-chrome
❌ [firefox] › mcp/cli-parsing.spec.ts:76 › negative number arguments @mcp-windows-latest-firefox
❌ [firefox] › mcp/cli-session.spec.ts:54 › idle timeout shuts the session down @mcp-windows-latest-firefox

8587 passed, 1446 skipped

---

Merge workflow run.

[Ayaan Gazali (ayaangazali)]

Reporter of #42693 here. Since the last MCP run came back with three failures, I ran them against your branch locally to see which ones are actually in reach of this diff. Short version: none of the three reproduce for me, and two of them cannot be caused by this change.

macOS arm64, chrome, at 1c947d07f:

tests/mcp/cli-core.spec.ts          38 passed   (includes :228 resize, which CI failed on macos-latest-chrome)
tests/mcp/cli-session.spec.ts       33 passed   (with cli-parsing)
tests/mcp/cli-parsing.spec.ts       included above, green
:54 idle timeout shuts the session down   5/5 passed with --repeat-each=5

The diff touches only browserBackend.ts, idleTimer.ts and idle-timeout.spec.ts. cli-core :228 resize and cli-parsing :76 negative number arguments are viewport handling and argv parsing respectively, with no path into the idle timer, so I would treat those two as unrelated noise rather than something to chase.

cli-session :54 idle timeout shuts the session down is the one that is genuinely in your blast radius, since it drives --idle-timeout=3000 and polls for (no browsers). If the refcount ever failed to return to zero the timer would never arm and that test would hang exactly this way. It passes here 5 out of 5, but it is a three second wall clock assertion on a windows runner, which is the classic shape for a slow-CI flake, so a re-run would tell you a lot before you go changing anything.

One thing that cost me twenty minutes and might save you some: this branch predates the chromium roll in a9da2dbe2 and still pins r1244, so if you have r1245 installed from main you get Executable doesn't exist at chromium_headless_shell-1244 and five bogus browser server failures that look like your bug. node ./packages/playwright-core/cli.js install chromium chromium-headless-shell on the branch clears it.

Separately, thanks for doing this one. The repro in my issue was the easy half; restoring the deleted test is the part that stops it coming back.