Uh oh!
There was an error while loading. Please reload this page.
fix(mcp): drop the backend after browser_close with a shared browser - #42365
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. Fixesmicrosoft#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.
om singhal (Om-singhaI)
commented
Aug 25, 2026
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
There was a problem hiding this comment.
Done, closing unconditionally now.
You are right that the gate guarded the wrong thing. browser.close() on a connected browser only clears the contexts this connection created and drops the connection itself; the external browser keeps running. That is what the Browser.close docs say and what _shouldCloseConnectionOnClose does in client/connect.ts. So the attached case was leaking one Playwright connection per disposed backend, and the next call stacked a fresh one on top. The dispose callback now closes the browser in both ownership modes, same shape as program.ts.
New test in tests/mcp/library.spec.ts attaches to a launchServer browser through a small counting TCP proxy, so the connection the factory holds is visible from outside. After browser_close the open count has to drop to zero while the server stays up and still accepts a fresh connect. With the old gate it stays at one and the test times out waiting for zero.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…t-close-invalidates-backend
…t-close-invalidates-backend
Review feedback on the previous commit. The dispose callback only closed the browser when this factory had launched it, on the theory that an attached browser belongs to whoever we connected to. That guarded the wrong thing. For a browser reached over a CDP or remote endpoint, close() clears the contexts this connection created and drops the connection itself, and the external browser keeps running. Skipping the close therefore leaked one connection per disposed backend. Close in both ownership modes, matching what program.ts already does. The new test attaches to a browser server through a counting TCP proxy, so the connection the factory holds is observable from the outside. Disposing the backend has to drop the count to zero while the server stays up and accepts a fresh connection.
This comment has been minimized.
This comment has been minimized.
om singhal (Om-singhaI)
commented
Aug 28, 2026
The two red checks are not the fix. The ubuntu one is my test: it builds a config with Worth a separate look: on linux that line reads |
Devin Rousso (dcrousso)
commented
Aug 28, 2026
yes please open another issue for that :) |
om singhal (Om-singhaI)
commented
Aug 28, 2026
Opened #42452 for it. |
Test results for "MCP"8305 passed, 1367 skipped Merge workflow run. |
e65229f
into
microsoft:mainUh oh!
There was an error while loading. Please reload this page.
om singhal (Om-singhaI)
commented
Aug 28, 2026
Thanks for the reviews on this one 😊 Was the sandbox issue useful? Happy to put up the change if #42452 looks right to you. |
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.