From 572f5da1dcc73bb442b2f9a823fd595cd112d89c Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Mon, 20 Apr 2026 19:38:09 -0700 Subject: [PATCH] fix(cli): exit daemon when extension connection fails The cli daemon process leaked when the user rejected or closed the extension connect tab. The HTTP server, websocket relay, and stdio pipes all kept the event loop alive after createBrowserWithInfo threw. - Daemon process.exits after logging the startup error. - Extension factory closes the relay + http server on failure. - Connect UI tells the background to drop the pending relay socket on reject so the daemon sees the disconnect. - session.startDaemon includes the daemon pid in rejection errors so callers (and tests) can verify the daemon actually exited. --- packages/extension/src/background.ts | 13 +++- packages/extension/src/ui/connect.tsx | 3 + .../src/tools/cli-client/session.ts | 8 +-- .../src/tools/cli-daemon/program.ts | 4 ++ .../src/tools/mcp/extensionContextFactory.ts | 10 ++- tests/extension/cli.spec.ts | 72 +++++++++++++------ tests/extension/extension-fixtures.ts | 2 + 7 files changed, 84 insertions(+), 28 deletions(-) diff --git a/packages/extension/src/background.ts b/packages/extension/src/background.ts index 5a33eaa87e70e..881e69c8d80dd 100644 --- a/packages/extension/src/background.ts +++ b/packages/extension/src/background.ts @@ -31,6 +31,8 @@ type PageMessage = { type: 'getConnectionStatus'; } | { type: 'disconnect'; +} | { + type: 'rejectConnection'; }; const PLAYWRIGHT_GROUP_TITLE = 'Playwright'; @@ -110,8 +112,17 @@ class TabShareExtension { sendResponse({ success: false, error: error.message }); } return true; + case 'rejectConnection': { + const selectorTabId = sender.tab?.id; + const pending = selectorTabId !== undefined ? this._pendingTabSelection.get(selectorTabId) : undefined; + if (pending) { + this._pendingTabSelection.delete(selectorTabId!); + pending.close('Rejected by user'); + } + sendResponse({ success: true }); + return true; + } } - return false; } private async _connectToRelay(selectorTabId: number, mcpRelayUrl: string, protocolVersion: number): Promise { diff --git a/packages/extension/src/ui/connect.tsx b/packages/extension/src/ui/connect.tsx index 8c248328a8326..3094dab138fa1 100644 --- a/packages/extension/src/ui/connect.tsx +++ b/packages/extension/src/ui/connect.tsx @@ -115,6 +115,9 @@ const ConnectApp: React.FC = () => { setShowButtons(false); setShowTabList(false); setStatus({ type: 'error', message }); + // Ask the background to close the pending MCP relay connection so the + // client daemon sees a disconnect and exits. + chrome.runtime.sendMessage({ type: 'rejectConnection' }).catch(() => {}); }, []); const connectToMCPRelay = useCallback(async (mcpRelayUrl: string, protocolVersion: number) => { diff --git a/packages/playwright-core/src/tools/cli-client/session.ts b/packages/playwright-core/src/tools/cli-client/session.ts index 3aed7eee2e7e1..ce533cbe420cc 100644 --- a/packages/playwright-core/src/tools/cli-client/session.ts +++ b/packages/playwright-core/src/tools/cli-client/session.ts @@ -161,6 +161,8 @@ export class Session { process.on('SIGTERM', sigtermHandler); let outLog = ''; + const rejectWithPid = (reject: (e: Error) => void, message: string) => + reject(Object.assign(new Error(`Daemon pid=${child.pid}: ${message}`), { daemonPid: child.pid })); await new Promise((resolve, reject) => { child.stdout!.on('data', data => { outLog += data.toString(); @@ -170,8 +172,7 @@ export class Session { const error = errorMatch ? errorMatch[1].trim() : undefined; if (error) { const errLogContent = fs.readFileSync(errLog, 'utf-8'); - const message = error + (errLogContent ? '\n' + errLogContent : ''); - reject(new Error(message)); + rejectWithPid(reject, error + (errLogContent ? '\n' + errLogContent : '')); } const successMatch = outLog.match(/### Success\nDaemon listening on (.*)\n/); @@ -181,8 +182,7 @@ export class Session { child.on('close', code => { if (!signalled) { const errLogContent = fs.readFileSync(errLog, 'utf-8'); - const message = `Daemon process exited with code ${code}` + (errLogContent ? '\n' + errLogContent : ''); - reject(new Error(message)); + rejectWithPid(reject, `Daemon process exited with code ${code}` + (errLogContent ? '\n' + errLogContent : '')); } }); }); diff --git a/packages/playwright-core/src/tools/cli-daemon/program.ts b/packages/playwright-core/src/tools/cli-daemon/program.ts index 4d06a661622b2..39c3e54d70caf 100644 --- a/packages/playwright-core/src/tools/cli-daemon/program.ts +++ b/packages/playwright-core/src/tools/cli-daemon/program.ts @@ -72,6 +72,10 @@ export function decorateProgram(program: Command) { const message = process.env.PWDEBUGIMPL ? (error as Error).stack || (error as Error).message : (error as Error).message; console.log(`### Error\n${message}`); console.log(''); + // The cli-client never destroys our stdout pipe on the error path, + // so the libuv handle would keep the daemon alive forever. + // eslint-disable-next-line no-restricted-properties + process.exit(1); } }); } diff --git a/packages/playwright-core/src/tools/mcp/extensionContextFactory.ts b/packages/playwright-core/src/tools/mcp/extensionContextFactory.ts index a5d9e5696aac9..fb7d95b7a2f2a 100644 --- a/packages/playwright-core/src/tools/mcp/extensionContextFactory.ts +++ b/packages/playwright-core/src/tools/mcp/extensionContextFactory.ts @@ -34,6 +34,12 @@ export async function createExtensionBrowser(config: FullConfig, clientName: str config.browser.launchOptions.executablePath); debugLogger(`CDP relay server started, extension endpoint: ${relay.extensionEndpoint()}.`); - await relay.ensureExtensionConnectionForMCPContext(clientName); - return await playwright.chromium.connectOverCDP(relay.cdpEndpoint(), { isLocal: true, timeout: 0 }); + try { + await relay.ensureExtensionConnectionForMCPContext(clientName); + return await playwright.chromium.connectOverCDP(relay.cdpEndpoint(), { isLocal: true, timeout: 0 }); + } catch (error) { + relay.stop(); + httpServer.close(); + throw error; + } } diff --git a/tests/extension/cli.spec.ts b/tests/extension/cli.spec.ts index a90a54081274e..9bf4ba7b19056 100644 --- a/tests/extension/cli.spec.ts +++ b/tests/extension/cli.spec.ts @@ -15,36 +15,67 @@ */ import fs from 'fs/promises'; -import { test, expect, extensionId } from './extension-fixtures'; +import { test as base, expect, extensionId } from './extension-fixtures'; -test('attach --extension', async ({ browserWithExtension, cli, server }, testInfo) => { - const browserContext = await browserWithExtension.launch(); +import type { CliResult } from './extension-fixtures'; +import type { Page } from 'playwright'; - // Write config file with userDataDir - const configPath = testInfo.outputPath('cli-config.json'); - await fs.writeFile(configPath, JSON.stringify({ - browser: { - userDataDir: browserWithExtension.userDataDir, - } - }, null, 2)); +const test = base.extend<{ + startAttach: () => Promise<{ confirmationPage: Page, cliPromise: Promise }>, +}>({ + startAttach: async ({ browserWithExtension, cli }, use, testInfo) => { + await use(async () => { + await fs.writeFile(testInfo.outputPath('cli-config.json'), JSON.stringify({ + browser: { + userDataDir: browserWithExtension.userDataDir, + } + }, null, 2)); + const browserContext = await browserWithExtension.launch(); + const confirmationPagePromise = browserContext.waitForEvent('page', page => + page.url().startsWith(`chrome-extension://${extensionId}/connect.html`) + ); + const cliPromise = cli('attach', '--extension', `--config=cli-config.json`); + const confirmationPage = await confirmationPagePromise; + return { confirmationPage, cliPromise }; + }); + }, +}); - const confirmationPagePromise = browserContext.waitForEvent('page', page => { - return page.url().startsWith(`chrome-extension://${extensionId}/connect.html`); - }); +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} - // Start the CLI command in the background - const cliPromise = cli('attach', '--extension', `--config=cli-config.json`); +async function expectDaemonExited(cliPromise: Promise): Promise { + const { error } = await cliPromise; + const pidMatch = error.match(/Daemon pid=(\d+)/); + expect(pidMatch, `expected daemon pid in cli error:\n${error}`).toBeTruthy(); + const pid = parseInt(pidMatch![1], 10); + await expect.poll(() => isAlive(pid)).toBe(false); +} - // Wait for the confirmation page to appear - const confirmationPage = await confirmationPagePromise; +test('daemon exits when user rejects the extension connection', async ({ startAttach }) => { + const { confirmationPage, cliPromise } = await startAttach(); + await confirmationPage.getByRole('button', { name: 'Reject' }).click(); + await expectDaemonExited(cliPromise); +}); + +test('daemon exits when user closes the connect tab', async ({ startAttach }) => { + const { confirmationPage, cliPromise } = await startAttach(); + await confirmationPage.close(); + await expectDaemonExited(cliPromise); +}); - // Click the Connect button +test('attach --extension', async ({ startAttach, cli, server }) => { + const { confirmationPage, cliPromise } = await startAttach(); await confirmationPage.locator('.tab-item', { hasText: 'Welcome' }).getByRole('button', { name: 'Allow & select' }).click(); { - // Wait for the CLI command to complete const { output } = await cliPromise; - // Verify the output expect(output).toContain(`### Page`); expect(output).toContain(`- Page URL: chrome-extension://${extensionId}/connect.html?`); expect(output).toContain(`- Page Title: Welcome`); @@ -52,7 +83,6 @@ test('attach --extension', async ({ browserWithExtension, cli, server }, t { const { output } = await cli('goto', server.HELLO_WORLD); - // Verify the output expect(output).toContain(`### Page`); expect(output).toContain(`- Page URL: ${server.HELLO_WORLD}`); expect(output).toContain(`- Page Title: Title`); diff --git a/tests/extension/extension-fixtures.ts b/tests/extension/extension-fixtures.ts index 666c83cd1e40c..8231648895293 100644 --- a/tests/extension/extension-fixtures.ts +++ b/tests/extension/extension-fixtures.ts @@ -59,6 +59,8 @@ export const test = base.extend