From 062cdd07045ff4e77d84fae2e0f3d4c834da3e56 Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Fri, 2 May 2025 15:32:37 +0200 Subject: [PATCH] fix: sticky launch errors (#324) This fixes an issue that there were sticky launch errors. When the [following code path](https://github.com/microsoft/playwright-mcp/blob/a15f0f301b405c00e9987fd84bf0f1f1fe856125/src/context.ts#L307-L339) was throwing, the Error was stored in the Promise and not cleared afterwards, this meant: - If a browser was not there and the user tried to install it via `browser_install` it was never working since the error was sticky. - If other errors like CDP is not available yet etc. error appear a re-connect would not work - the MCP server would require a restart. Test plan: Since we don't have any `browser_install` tests I added a CDP test for now to cover this bug. --- src/context.ts | 6 +++++- tests/cdp.spec.ts | 22 +++++++++++++++++++-- tests/fixtures.ts | 48 ++++++++++++++++++++++++++-------------------- tests/tabs.spec.ts | 4 ++-- 4 files changed, 54 insertions(+), 26 deletions(-) diff --git a/src/context.ts b/src/context.ts index 3847d99..313cb95 100644 --- a/src/context.ts +++ b/src/context.ts @@ -299,8 +299,12 @@ ${code.join('\n')} } private async _createBrowserContext(): Promise<{ browser?: playwright.Browser, browserContext: playwright.BrowserContext }> { - if (!this._createBrowserContextPromise) + if (!this._createBrowserContextPromise) { this._createBrowserContextPromise = this._innerCreateBrowserContext(); + void this._createBrowserContextPromise.catch(() => { + this._createBrowserContextPromise = undefined; + }); + } return this._createBrowserContextPromise; } diff --git a/tests/cdp.spec.ts b/tests/cdp.spec.ts index 7e6d29f..98e4944 100644 --- a/tests/cdp.spec.ts +++ b/tests/cdp.spec.ts @@ -17,7 +17,7 @@ import { test, expect } from './fixtures.js'; test('cdp server', async ({ cdpEndpoint, startClient }) => { - const client = await startClient({ args: [`--cdp-endpoint=${cdpEndpoint}`] }); + const client = await startClient({ args: [`--cdp-endpoint=${await cdpEndpoint()}`] }); expect(await client.callTool({ name: 'browser_navigate', arguments: { @@ -27,7 +27,7 @@ test('cdp server', async ({ cdpEndpoint, startClient }) => { }); test('cdp server reuse tab', async ({ cdpEndpoint, startClient }) => { - const client = await startClient({ args: [`--cdp-endpoint=${cdpEndpoint}`] }); + const client = await startClient({ args: [`--cdp-endpoint=${await cdpEndpoint()}`] }); expect(await client.callTool({ name: 'browser_click', @@ -54,3 +54,21 @@ test('cdp server reuse tab', async ({ cdpEndpoint, startClient }) => { \`\`\` `); }); + +test('should throw connection error and allow re-connecting', async ({ cdpEndpoint, startClient }) => { + const port = 3200 + test.info().parallelIndex; + const client = await startClient({ args: [`--cdp-endpoint=http://localhost:${port}`] }); + expect(await client.callTool({ + name: 'browser_navigate', + arguments: { + url: 'data:text/html,TitleHello, world!', + }, + })).toContainTextContent(`Error: browserType.connectOverCDP: connect ECONNREFUSED`); + await cdpEndpoint(port); + expect(await client.callTool({ + name: 'browser_navigate', + arguments: { + url: 'data:text/html,TitleHello, world!', + }, + })).toContainTextContent(`- text: Hello, world!`); +}); diff --git a/tests/fixtures.ts b/tests/fixtures.ts index c38b4a8..7b7dfee 100644 --- a/tests/fixtures.ts +++ b/tests/fixtures.ts @@ -22,7 +22,7 @@ import { chromium } from 'playwright'; import { test as baseTest, expect as baseExpect } from '@playwright/test'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; -import { spawn } from 'child_process'; +import { ChildProcessWithoutNullStreams, spawn } from 'child_process'; import { TestServer } from './testserver/index.ts'; import type { Config } from '../config'; @@ -36,7 +36,7 @@ type TestFixtures = { visionClient: Client; startClient: (options?: { args?: string[], config?: Config }) => Promise; wsEndpoint: string; - cdpEndpoint: string; + cdpEndpoint: (port?: number) => Promise; server: TestServer; httpsServer: TestServer; mcpHeadless: boolean; @@ -95,27 +95,33 @@ export const test = baseTest.extend( }, cdpEndpoint: async ({ }, use, testInfo) => { - const port = 3200 + (+process.env.TEST_PARALLEL_INDEX!); - const executablePath = chromium.executablePath(); - const browserProcess = spawn(executablePath, [ - `--user-data-dir=${testInfo.outputPath('user-data-dir')}`, - `--remote-debugging-port=${port}`, - `--no-first-run`, - `--no-sandbox`, - `--headless`, - '--use-mock-keychain', - `data:text/html,hello world`, - ], { - stdio: 'pipe', - }); - await new Promise(resolve => { - browserProcess.stderr.on('data', data => { - if (data.toString().includes('DevTools listening on ')) - resolve(); + let browserProcess: ChildProcessWithoutNullStreams | undefined; + + await use(async port => { + if (!port) + port = 3200 + test.info().parallelIndex; + if (browserProcess) + return `http://localhost:${port}`; + browserProcess = spawn(chromium.executablePath(), [ + `--user-data-dir=${testInfo.outputPath('user-data-dir')}`, + `--remote-debugging-port=${port}`, + `--no-first-run`, + `--no-sandbox`, + `--headless`, + '--use-mock-keychain', + `data:text/html,hello world`, + ], { + stdio: 'pipe', }); + await new Promise(resolve => { + browserProcess!.stderr.on('data', data => { + if (data.toString().includes('DevTools listening on ')) + resolve(); + }); + }); + return `http://localhost:${port}`; }); - await use(`http://localhost:${port}`); - browserProcess.kill(); + browserProcess?.kill(); }, mcpHeadless: async ({ headless }, use) => { diff --git a/tests/tabs.spec.ts b/tests/tabs.spec.ts index 12beed3..9582ab3 100644 --- a/tests/tabs.spec.ts +++ b/tests/tabs.spec.ts @@ -142,11 +142,11 @@ test('close tab', async ({ client }) => { }); test('reuse first tab when navigating', async ({ startClient, cdpEndpoint }) => { - const browser = await chromium.connectOverCDP(cdpEndpoint); + const browser = await chromium.connectOverCDP(await cdpEndpoint()); const [context] = browser.contexts(); const pages = context.pages(); - const client = await startClient({ args: [`--cdp-endpoint=${cdpEndpoint}`] }); + const client = await startClient({ args: [`--cdp-endpoint=${await cdpEndpoint()}`] }); await client.callTool({ name: 'browser_navigate', arguments: {