fix(mcp): reconnect to the browser after disconnect#41966
Conversation
When the extension dropped the relay connection (MV3 service worker idle timeout, browser restart, extension reload), the next tool call failed against the dead browser. Recreate the backend when its browser has disconnected, so the call transparently re-establishes the extension connection instead. Also stop the single-shot relay servers once the browser connection drops. Fixes: microsoft#41874
The stale-backend recreation also covers a remote CDP browser that was restarted and an isolated browser that died: the next tool call reconnects or relaunches transparently. Log the observed disconnect under pw:mcp:backend so tests can synchronize on it.
- CDPRelayServer.stop() now closes its http server, so callers do not need to pair the two calls. - Share the dispose-and-reset logic between the stale and isClose paths in the tool call handler. - Extract readExtensionToken test helper.
The factory cached the shared browser until every client disconnected, so after a crash the stale backends were recreated against the dead browser and tool calls kept failing for all clients. Drop the cached browser when it is no longer connected, and balance the client count when backend creation fails so the browser close bookkeeping stays correct.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| }); | ||
| }); | ||
|
|
||
| test('transparently reconnects when the remote browser is restarted', async ({ cdpServer, startClient, server }) => { |
There was a problem hiding this comment.
Why is this a good thing? What if that's some completely unrelated browser? Do we have any evidence this is a useful scenario?
There was a problem hiding this comment.
This is what has been asked for the extension. We discussed it offline ad decided to make it transpantly reconnect if the remote connection is lost for whatever reason. It especially makes sense when you connect from and mcp client over stdio which doesn't restart the server if it fails and would require manual intervention. When the server is reconnected the agent will see that the state has changed (due to a user action, crash, update or what not) and adapt.
| }); | ||
|
|
||
| test('http transport isolated closes the browser despite an earlier failed backend creation', async ({ serverEndpoint, server }, testInfo) => { | ||
| // A failed backend creation must not leak the client count, otherwise the |
There was a problem hiding this comment.
What does this comment mean?
There was a problem hiding this comment.
There was a bug in program.ts, see new clientCount--; in the catch block. But I agree the comment does not make sense out of that context. Will remove.
| }); | ||
| }); | ||
|
|
||
| test('http transport isolated multiclient relaunches a crashed shared browser', async ({ serverEndpoint, server }, testInfo) => { |
There was a problem hiding this comment.
Is this a common scenario - with a crashed browser? I think this is more common when the user closes the browser window manually (for whatever reason) - do we want to relaunch it in this case?
There was a problem hiding this comment.
Yes, I believe this makes sense as it is the mcp server that launched the browser. The same logic as above.
Test results for "MCP"1 failed 7847 passed, 1278 skipped Merge workflow run. |
|
Hi, I'm the Playwright bot and I took a look at the CI failures on the latest run. 🟢 CI is clear — the one failure is a pre-existing flakeThe only failure is DetailsOverall: no failure is attributable to this PR. The diff is confined to the MCP backend reconnect path ( Pre-existing flake / infra
Note: earlier reports on this PR flagged Triaged by the Playwright bot - agent run |
2 participants
Summary
--extensionreopensconnect.html(silent with token bypass), cdp mode reconnects to the endpoint, persistent/isolated modes relaunch the browser.--isolated/--shared-browser-contextwith multiple HTTP clients) instead of caching the dead one until all clients disconnect.Fixes #41874