From fccc52b6ebc1f96fe262ecf245de11e9380bcc47 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Mon, 1 Jun 2026 17:56:53 -0700 Subject: [PATCH] fix(mcp): close browser when host dies without a signal The MCP exit watchdog only reacted to stdin 'close', SIGINT and SIGTERM. When the host process dies without delivering a signal (SIGKILL, OOM kill, IDE hard-reload), the server is re-parented and none of those fire, leaking the browser process tree. Poll the parent pid and shut down once orphaned. Fixes: https://github.com/microsoft/playwright/issues/41013 --- .../playwright-core/src/tools/mcp/watchdog.ts | 20 +++- tests/config/utils.ts | 9 ++ tests/extension/cli.spec.ts | 10 +- tests/mcp/cli-killall.spec.ts | 10 +- tests/mcp/dashboard.spec.ts | 10 +- tests/mcp/watchdog.spec.ts | 107 ++++++++++++++++++ 6 files changed, 135 insertions(+), 31 deletions(-) create mode 100644 tests/mcp/watchdog.spec.ts diff --git a/packages/playwright-core/src/tools/mcp/watchdog.ts b/packages/playwright-core/src/tools/mcp/watchdog.ts index 0f947ee7ca8bf..49cca2dfd86b4 100644 --- a/packages/playwright-core/src/tools/mcp/watchdog.ts +++ b/packages/playwright-core/src/tools/mcp/watchdog.ts @@ -19,7 +19,7 @@ import { testDebug } from './log'; export function setupExitWatchdog() { let isExiting = false; - const handleExit = async (signal: string) => { + const handleExit = async () => { if (isExiting) return; isExiting = true; @@ -31,7 +31,19 @@ export function setupExitWatchdog() { process.exit(0); }; - process.stdin.on('close', () => handleExit('close')); - process.on('SIGINT', () => handleExit('SIGINT')); - process.on('SIGTERM', () => handleExit('SIGTERM')); + process.stdin.on('close', handleExit); + process.on('SIGINT', handleExit); + process.on('SIGTERM', handleExit); + + // The host can die without delivering a signal or closing stdin (SIGKILL, OOM, + // IDE reload), in which case we get re-parented. Poll for that to avoid leaking + // the browser. No re-parenting on Windows. + if (process.platform !== 'win32') { + const parentPid = process.ppid; + const interval = setInterval(() => { + if (process.ppid !== parentPid) + void handleExit(); + }, 1000); + interval.unref(); + } } diff --git a/tests/config/utils.ts b/tests/config/utils.ts index f39363a61f344..d734b49e18d10 100644 --- a/tests/config/utils.ts +++ b/tests/config/utils.ts @@ -267,3 +267,12 @@ export function inheritAndCleanEnv(env: NodeJS.ProcessEnv | undefined): NodeJS.P ...env, }; } + +export function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} diff --git a/tests/extension/cli.spec.ts b/tests/extension/cli.spec.ts index 11a6a7324287b..2b1fd3a8e9119 100644 --- a/tests/extension/cli.spec.ts +++ b/tests/extension/cli.spec.ts @@ -15,6 +15,7 @@ */ import { test as base, expect, extensionId, clickAllowAndSelect } from './extension-fixtures'; +import { isAlive } from '../config/utils'; import type { CliResult } from './extension-fixtures'; import type { Page } from 'playwright'; @@ -37,15 +38,6 @@ const test = base.extend<{ }, }); -function isAlive(pid: number): boolean { - try { - process.kill(pid, 0); - return true; - } catch { - return false; - } -} - async function expectDaemonExited(cliPromise: Promise): Promise { const { error } = await cliPromise; const pidMatch = error.match(/Daemon pid=(\d+)/); diff --git a/tests/mcp/cli-killall.spec.ts b/tests/mcp/cli-killall.spec.ts index 2fb812ab0fe3a..338b4e8335469 100644 --- a/tests/mcp/cli-killall.spec.ts +++ b/tests/mcp/cli-killall.spec.ts @@ -15,15 +15,7 @@ */ import { test, expect } from './cli-fixtures'; - -function isAlive(pid: number): boolean { - try { - process.kill(pid, 0); - return true; - } catch { - return false; - } -} +import { isAlive } from '../config/utils'; test('kill-all kills only filtered pid', async ({ cli, server }) => { const { daemonPid } = await cli('open', server.HELLO_WORLD); diff --git a/tests/mcp/dashboard.spec.ts b/tests/mcp/dashboard.spec.ts index eda77af07cce1..00975a1bb2ea5 100644 --- a/tests/mcp/dashboard.spec.ts +++ b/tests/mcp/dashboard.spec.ts @@ -19,6 +19,7 @@ import os from 'os'; import path from 'path'; import { test, expect, installSaveFilePickerMock } from './cli-fixtures'; +import { isAlive } from '../config/utils'; function displayPath(p: string): string { const home = os.homedir(); @@ -111,15 +112,6 @@ test('should activate session when show is called with -s', async ({ cli, server await expect(activeSession(dashboard)).toHaveAccessibleName('Session sessB'); }); -function isAlive(pid: number): boolean { - try { - process.kill(pid, 0); - return true; - } catch { - return false; - } -} - test('daemon show: closing page exits the process', async ({ cli, connectToDashboard }) => { const bindTitle = `--playwright-internal--${crypto.randomUUID()}`; const { exitCode, dashboardPid } = await cli('show', { bindTitle }); diff --git a/tests/mcp/watchdog.spec.ts b/tests/mcp/watchdog.spec.ts new file mode 100644 index 0000000000000..9902ccbafc73c --- /dev/null +++ b/tests/mcp/watchdog.spec.ts @@ -0,0 +1,107 @@ +/** + * Copyright (c) Microsoft Corporation. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import fs from 'fs'; +import { execSync, spawn } from 'child_process'; + +import { test, expect, mcpServerPath } from './fixtures'; +import { inheritAndCleanEnv, isAlive } from '../config/utils'; + +test.slow(); + +// Regression test for https://github.com/microsoft/playwright/issues/41013: +// when the host process dies without delivering SIGINT/SIGTERM (e.g. SIGKILL, +// OOM kill, IDE hard-reload) the MCP server is re-parented and must still shut +// the browser down. We reproduce this by putting the MCP server's stdin behind +// a FIFO held open by the test process, spawning it from an intermediate parent +// and then killing that parent: neither a stdin 'close' nor a signal reaches +// the MCP server, so only parent-death (PPID) detection can clean it up. +test('should close the browser when the MCP host is killed without a signal', async ({ mcpBrowser, mcpHeadless }, testInfo) => { + test.skip(process.platform === 'win32', 'There is no process re-parenting on Windows'); + + const fifo = testInfo.outputPath('mcp-stdin.fifo'); + execSync(`mkfifo ${fifo}`); + // Open read-write so the open never blocks and the write end stays open even + // after the intermediate parent dies. + const stdinFd = fs.openSync(fifo, 'r+'); + + const pidFile = testInfo.outputPath('mcp.pid'); + const stdoutLog = testInfo.outputPath('mcp-stdout.log'); + + const mcpArgs = [...mcpServerPath, '--isolated']; + if (mcpHeadless) + mcpArgs.push('--headless'); + if (mcpBrowser) + mcpArgs.push(`--browser=${mcpBrowser}`); + + // The intermediate parent launches the MCP server reading from the FIFO and + // then stays alive, so the MCP server only dies once we kill this parent. + const parentScript = ` + const fs = require('fs'); + const { spawn } = require('child_process'); + const readFd = fs.openSync(${JSON.stringify(fifo)}, 'r'); + const out = fs.openSync(${JSON.stringify(stdoutLog)}, 'a'); + const child = spawn(process.execPath, ${JSON.stringify(mcpArgs)}, { + stdio: [readFd, out, 'inherit'], + }); + fs.writeFileSync(${JSON.stringify(pidFile)}, String(child.pid)); + setInterval(() => {}, 1000); + `; + + const parent = spawn(process.execPath, ['-e', parentScript], { + stdio: ['ignore', 'inherit', 'inherit'], + env: inheritAndCleanEnv({ + PWMCP_PROFILES_DIR_FOR_TEST: testInfo.outputPath('ms-playwright'), + PW_TMPDIR_FOR_TEST: testInfo.outputPath('tmp'), + }), + }); + + // Wait for the MCP server pid to be reported. + await expect.poll(() => fs.existsSync(pidFile), { timeout: 15000 }).toBe(true); + const mcpPid = Number(fs.readFileSync(pidFile, 'utf8')); + expect(mcpPid).toBeGreaterThan(0); + + // Drive the MCP server to launch the browser. + const send = (msg: any) => fs.writeSync(stdinFd, JSON.stringify(msg) + '\n'); + send({ jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: '2024-11-05', capabilities: {}, clientInfo: { name: 'repro', version: '1' } } }); + send({ jsonrpc: '2.0', method: 'notifications/initialized' }); + send({ jsonrpc: '2.0', id: 2, method: 'tools/call', params: { name: 'browser_navigate', arguments: { url: 'about:blank' } } }); + + // Wait for the navigate response, which means the browser is up. + await expect.poll(() => { + try { + return fs.readFileSync(stdoutLog, 'utf8').includes('"id":2'); + } catch { + return false; + } + }, { timeout: 30000 }).toBe(true); + + // The browser is a direct child of the MCP server process. + const browserPids = execSync(`pgrep -P ${mcpPid} || true`).toString().split('\n').map(Number).filter(Boolean); + expect(browserPids.length).toBeGreaterThan(0); + expect(isAlive(mcpPid)).toBe(true); + + // Simulate the host dying without a signal: the MCP server is re-parented but + // keeps its (FIFO) stdin and never receives SIGINT/SIGTERM. + process.kill(parent.pid!, 'SIGKILL'); + + // The watchdog should notice the orphaning and shut everything down. + await expect.poll(() => isAlive(mcpPid), { timeout: 20000, message: 'MCP server should exit after being orphaned' }).toBe(false); + for (const pid of browserPids) + await expect.poll(() => isAlive(pid), { timeout: 20000, message: `Browser process ${pid} should exit` }).toBe(false); + + fs.closeSync(stdinFd); +});