fix(mcp): reconnect to the browser after disconnect#41966
Conversation
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Dispose the backend when its browser disconnects between tool calls, so that the next call transparently creates a fresh one: --extension reopens connect.html, cdp mode reconnects to the endpoint, persistent/isolated modes relaunch the browser. Drop the cached shared browser when it disconnects and balance the client count when backend creation fails. Stop the single-shot extension relay and its http server once the browser connection drops. Fixes: microsoft#41874
Drop the 'disposed' event and the disposeCallback parameter: the server resets its backend on 'disconnected', and program.ts tracks the context/browser teardown itself to tell a dead browser from a deliberate close.
Test results for "MCP"3 failed 7845 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 — all three failures are pre-existing flakesThe three failures live in DetailsOverall: no failure is attributable to this PR. The diff is confined to the MCP backend reconnect path ( Pre-existing flake / infra
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