fix(mcp): drop the backend after browser_close with a shared browser - #42365
fix(mcp): drop the backend after browser_close with a shared browser#42365om singhal (Om-singhaI) wants to merge 5 commits into
Conversation
With the HTTP transport and the shared browser context option, browser_close disposes the backend of the calling client while the browser stays alive for the other clients. The server only drops its cached backend on the disconnected event, which never fires in that case, so the next tool call of that client runs against a disposed context and fails with a TypeError. Emit the disconnected event from BrowserBackend.dispose so that any disposal invalidates the cached backend and the next call creates a fresh one. Fixes microsoft#42363
| this._disposed = true; | ||
| await this._context?.dispose().catch(e => debug('pw:tools:error')(e)); | ||
| await this._disposeCallback?.().catch(e => debug('pw:tools:error')(e)); | ||
| // The browser may outlive this backend, e.g. when other clients share it, |
There was a problem hiding this comment.
disposing a backend while the shared browser survives leaves the constructor's close and disconnected listeners attached
repeating browser_close therefore retains every disposed backend until the shared browser exits, so please detach those listeners during disposal
| // so tell the server to stop handing out the disposed backend. | ||
| if (!this._disconnected) { | ||
| this._disconnected = true; | ||
| this.emit('disconnected'); |
There was a problem hiding this comment.
createConnection() without a contextGetter owns the browser it launches but supplies no dispose callback, so the next tool call launches another browser while the previous process remains open
please close factory owned browsers before invalidating and recreating this backend
Review feedback on the previous commit. BrowserBackend attaches markDisconnected to the browser context close event and to the browser disconnected event, and dispose removed neither. When the browser outlives the backend, as it does with a shared browser or with a caller supplied context, repeating browser_close leaves one listener per disposed backend on objects that stay alive, so every disposed backend stays reachable. Keep the handler on the instance and detach both listeners in dispose. createConnection without a contextGetter launches its own browser and passed no dispose callback, so browser_close invalidated the backend and left the browser running. The next tool call then launched another one, and against a persistent profile it failed outright with "Browser is already in use". Give that path a dispose callback that closes the context and the browser, in the shape program.ts already uses. With a contextGetter the caller owns the context, so that path still gets no callback.
|
Both fixed in the commit on top. The handler is kept on the instance now and removed in
The second test asserts the profile lock clears after Both tests are in |
| // whoever we connected to, and the next call re attaches to it. | ||
| return new BrowserBackend(config, context, tools, async () => { | ||
| await context.close().catch(() => {}); | ||
| if (ownership === 'own') |
There was a problem hiding this comment.
ownership === 'attached' means the browser is externally owned, but this factory still owns its Playwright connection
with an isolated CDP or remote endpoint, context.close() leaves that connection alive, so please call browser.close() for both ownership modes
This comment has been minimized.
This comment has been minimized.
Test results for "MCP"2 failed 7555 passed, 1359 skipped Merge workflow run. |
…t-close-invalidates-backend
2 participants
Fixes #42363
What happens
With the HTTP transport and
--shared-browser-context,browser_closedisposes the calling client'sBrowserBackend: theContextis cleared and the dispose callback only decrements the client count, because another client keeps the shared browser alive.server.tscachesbackendPromiseper session and clears it only on the backend'sdisconnectedevent, whichBrowserBackendemits only when the browser context closes or the browser disconnects. Neither happens here, so the next tool call from that client reuses the disposed backend:ensureBrowserContextreturns the already resolved promise without re registering the page listener,newTabfinds no tab, andbrowser_tabs(new) orbrowser_navigatefail withuntil the server restarts. In the CLI server, non shared and isolated modes are unaffected because they close the context or the browser and the event fires.
The change
BrowserBackend.dispose()now emitsdisconnected(guarded so it fires once) after the context and the dispose callback have run.server.tsalready drops the cached backend in its existingonce('disconnected')listener, so the next call creates a fresh backend through the factory. The listener also callsbackend.dispose()again, which is a no op through the existing_disposedguard.This was chosen over special casing the close result in
server.ts:callToolalready stripsisClosefrom the result before returning it, so the server cannot see a close without widening theServerBackendcontract, and the event approach makes every disposal path invalidate the cache, not onlybrowser_close. No debug line is logged fromdispose, so the tests that countbrowser disconnectedlines are unaffected.The library entry point (
createConnectioninmcp/index.ts) goes through the samecreateServer, so it picks up the same behaviour: afterbrowser_closethe next call now drops the disposed backend and asks the factory for a new one (a freshBrowserBackendon the context the getter returns when acontextGetteris supplied), instead of failing with the TypeError. Without a getter the factory launches a second browser while the first stays open, becausecreateConnectionpasses no dispose callback andbrowser_closenever closed the browser in library mode before or after this change; that is pre existing. The otherBrowserBackendconsumers (cli-daemon,traceSnapshot, the test backend underplaywright/src/mcp) register nodisconnectedlistener, and an emit with no listener is harmless.Test
New
http transport shared context survives browser_closeintests/mcp/http.spec.ts, modelled on the neighbouring shared context test: two HTTP clients with--shared-browser-context, both navigate, client 1 callsbrowser_closethenbrowser_tabs(new) and must get a snapshot, client 2'sbrowser_snapshotmust still work, and the log counts must match (create context 3,close browser 1).The one failure in
http.spec.tsisshould close session when heartbeat ping is not answered, and it fails identically with the source reverted, so it is unrelated to this change: on the machine used here the defaultchromechannel takes 7 to 8 seconds to launch a fresh persistent profile, and the heartbeat only starts once the backend exists, so the session delete lands afterexpect.poll's default 5 second timeout. With--browser=chromiumthe same scenario reaps the session in under 3 seconds.The issue's three step scenario was also run end to end against a built
mcp.jswith two streamable HTTP clients before and after the change; before, step 3 returns the TypeError above and the second client keeps working; after, all steps succeed.eslint,tsc,lint-testsandcheck-depspass. Only the chromium project was run locally; chrome, firefox and webkit were not.