fix(mcp): do not close browser while tool call is running - #42696
Anurag Singh (Anurag-M1) wants to merge 2 commits into
Conversation
|
@microsoft-github-policy-service agree |
|
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 On this branch: which matches what 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. |
| if (this._running > 0) | ||
| --this._running; | ||
| if (this._running === 0) | ||
| this._timer = setTimeout(this._onIdle, this._timeout).unref(); |
There was a problem hiding this comment.
this can restart the timer after browser_close, potentially keeping the closed browser alive longer than needed
There was a problem hiding this comment.
Good catch! Pushed a fix:
- Added a
_disposedflag toIdleTimerand separated clearing the active timer during calls (_clearTimer) from permanent disposal (dispose). - Guarded
callStarted(),callFinished(), andpoke()so that once disposed, the timer is never re-armed. - Explicitly dispose
_idleTimeronbrowser_close/ disconnect so the closed browser instance is not retained. - Added regression test
does not restart the idle timer after browser_closeintests/mcp/idle-timeout.spec.ts.
This comment has been minimized.
This comment has been minimized.
| } | ||
| if (this._disconnected || responseObject.isClose) { | ||
| delete responseObject.isClose; | ||
| this._idleTimer?.dispose(); |
There was a problem hiding this comment.
with --isolated clients share this timer so browser_close from one client permanently disables idle cleanup for the remaining clients
| // Outlast the idle timeout to ensure the timer is not re-armed after browser_close. | ||
| await new Promise(f => setTimeout(f, 1000)); | ||
|
|
||
| expect(formatLog(stderr())).toEqual({ |
There was a problem hiding this comment.
a restarted timer does not emit another close browser log so this wont detect a regression
Test results for "MCP"3 failed 8587 passed, 1446 skipped Merge workflow run. |
3 participants
Summary
callStarted()andcallFinished()onIdleTimerwith reference counting (_running), ensuring the timer is cleared during tool execution and only re-armed once all active calls complete.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.does not close the browser while a tool call is runningintests/mcp/idle-timeout.spec.ts.Fixes #42693