mirror of
https://github.com/ChromeDevTools/chrome-devtools-mcp.git
synced 2026-11-04 15:00:11 +00:00
fix: exit on stdin EOF and SIGTERM/SIGINT/SIGHUP, closing the browser cleanly (#2117)
Fixes #2116. `chrome-devtools-mcp-main.ts` currently has no shutdown handler. After a session calls `navigate_page` (or anything else that launches Chrome), the Chrome subprocess keeps the Node event loop ref'd, so closing stdin (the stdio MCP convention for "I'm done") doesn't make the server exit. Callers that close stdin to terminate the server have to fall back to SIGTERM / SIGKILL on every page-loaded session — deterministically, not flakily. This change: 1. Adds `closeBrowser()` in `browser.ts` that calls `browser.close()` for launched instances (reaps the Chrome subprocess) and `browser.disconnect()` for attached instances (leaves the user's Chrome alive). No-op if no browser is active or the connection has already been dropped. 2. Registers shutdown handlers in `chrome-devtools-mcp-main.ts` for: - `stdin.on('end' | 'close')` — stdio MCP transport convention - `SIGTERM` / `SIGINT` / `SIGHUP` — clients that signal instead of closing stdin (`SIGHUP` for parity with `src/daemon/daemon.ts`) The handler is idempotent (guarded `shuttingDown` flag), and has an unref'd 10s timeout backstop in case Chrome teardown hangs (slow `beforeunload` handlers, many tabs, etc.). ### Note on scope This complements (does not replace) the client-side fixes filed against #1765, e.g. google-gemini/gemini-cli#13391 and anthropics/claude-code#42300. The MCP stdio convention is that closing stdin signals shutdown; a server that doesn't honor that forces every client to special-case it. The watchdog sub-process (`src/telemetry/watchdog/main.ts:145-146`) and the daemon (`src/daemon/daemon.ts:224-230`) both already implement this for the same reason — this PR extends the same pattern to the main entry point so all three execution paths behave consistently. ### Measurement Repro script in #2116, same env (chrome-devtools-mcp@1.0.1, Chrome 148.0.7778.178, Node v24.11.1, Linux), 10 iterations: | Scenario | Before | After | |---|---|---| | `tools/list only` (no navigation) | 10/10 clean, 30-37 ms | 10/10 clean, 29-40 ms | | `navigate example.com` | 10/10 SIGTERM at ~5080 ms | 10/10 clean at 145-180 ms | ### Notes - I didn't add a subprocess-based test for this; the existing `tests/utils.ts:runCli` infrastructure targets the `chrome-devtools` CLI, not the stdio MCP server, and a shutdown-timing test would introduce non-trivial Chrome-startup flakiness in CI. Happy to add one if maintainers want it — pointer to the right test directory appreciated.
This commit is contained in:
1 parent
8ea5f098ef
commit
43b934cd98
3 files changed
+256
-2
No files matched your search
@@ -8,6 +8,7 @@ import '../polyfill.js';
|
||||
|
||||
import process from 'node:process';
|
||||
|
||||
import {closeBrowser} from '../browser.js';
|
||||
import {createMcpServer, logDisclaimers} from '../index.js';
|
||||
import {logger, saveLogsToFile} from '../logger.js';
|
||||
import {ClearcutLogger} from '../telemetry/ClearcutLogger.js';
|
||||
@@ -32,6 +33,44 @@ if (process.env['CHROME_DEVTOOLS_MCP_CRASH_ON_UNCAUGHT'] !== 'true') {
|
||||
});
|
||||
}
|
||||
|
||||
// Shutdown on stdin EOF (stdio MCP convention — the client closes the
|
||||
// transport to signal exit) and on standard termination signals. Without
|
||||
// this, an active Chrome subprocess keeps the Node event loop ref'd after
|
||||
// stdin closes and the server hangs until something else kills it.
|
||||
let shuttingDown = false;
|
||||
async function shutdown(reason: string): Promise<void> {
|
||||
if (shuttingDown) {
|
||||
return;
|
||||
}
|
||||
shuttingDown = true;
|
||||
logger(`Shutting down (${reason})`);
|
||||
// Backstop in case browser teardown hangs (e.g. unresponsive Chrome,
|
||||
// slow beforeunload handlers, many tabs). Exits 0 because we still
|
||||
// honored the shutdown request; the log line preserves observability.
|
||||
// Unref'd so it doesn't keep the loop alive on the clean path.
|
||||
setTimeout(() => {
|
||||
logger('Shutdown timeout exceeded, forcing exit');
|
||||
process.exit(0);
|
||||
}, 10000).unref();
|
||||
await closeBrowser();
|
||||
process.exit(0);
|
||||
}
|
||||
process.stdin.on('end', () => {
|
||||
void shutdown('stdin end');
|
||||
});
|
||||
process.stdin.on('close', () => {
|
||||
void shutdown('stdin close');
|
||||
});
|
||||
process.on('SIGTERM', () => {
|
||||
void shutdown('SIGTERM');
|
||||
});
|
||||
process.on('SIGINT', () => {
|
||||
void shutdown('SIGINT');
|
||||
});
|
||||
process.on('SIGHUP', () => {
|
||||
void shutdown('SIGHUP');
|
||||
});
|
||||
|
||||
logger(`Starting Chrome DevTools MCP Server v${VERSION}`);
|
||||
const {server} = await createMcpServer(args, {
|
||||
logFile,
|
||||
|
||||
+37
-2
@@ -19,6 +19,7 @@ import type {
|
||||
import {puppeteer} from './third_party/index.js';
|
||||
|
||||
let browser: Browser | undefined;
|
||||
let browserMode: 'launched' | 'connected' | undefined;
|
||||
|
||||
function makeTargetFilter(enableExtensions = false) {
|
||||
const ignoredPrefixes = new Set(['chrome://', 'chrome-untrusted://']);
|
||||
@@ -120,7 +121,12 @@ export async function ensureBrowserConnected(options: {
|
||||
|
||||
logger('Connecting Puppeteer to ', JSON.stringify(connectOptions));
|
||||
try {
|
||||
browser = await puppeteer.connect(connectOptions);
|
||||
// Assign mode before browser so a concurrent closeBrowser() never sees
|
||||
// `browser` set with `browserMode` still undefined (would fall through
|
||||
// to the disconnect() path and orphan a launched Chrome).
|
||||
const connected = await puppeteer.connect(connectOptions);
|
||||
browserMode = 'connected';
|
||||
browser = connected;
|
||||
} catch (err) {
|
||||
throw new Error(
|
||||
`Could not connect to Chrome. ${autoConnect ? `Check if Chrome is running and remote debugging is enabled by going to chrome://inspect/#remote-debugging.` : `Check if Chrome is running.`}`,
|
||||
@@ -266,8 +272,37 @@ export async function ensureBrowserLaunched(
|
||||
if (browser?.connected) {
|
||||
return browser;
|
||||
}
|
||||
browser = await launch(options);
|
||||
// Assign mode before browser; see the connect path above for rationale.
|
||||
const launched = await launch(options);
|
||||
browserMode = 'launched';
|
||||
browser = launched;
|
||||
return browser;
|
||||
}
|
||||
|
||||
/**
|
||||
* Shutdown hook for the active browser. Closes a launched browser (so the
|
||||
* Chrome subprocess is reaped) or disconnects from an attached browser (so
|
||||
* the user's Chrome instance stays alive). No-op if no browser is active or
|
||||
* the connection has already been dropped. Called from the server entrypoint
|
||||
* on stdin EOF / SIGTERM / SIGINT.
|
||||
*/
|
||||
export async function closeBrowser(): Promise<void> {
|
||||
const b = browser;
|
||||
const mode = browserMode;
|
||||
browser = undefined;
|
||||
browserMode = undefined;
|
||||
if (!b || !b.connected) {
|
||||
return;
|
||||
}
|
||||
if (mode === 'launched') {
|
||||
await b.close().catch(err => {
|
||||
logger('Failed to close browser', err);
|
||||
});
|
||||
return;
|
||||
}
|
||||
await b.disconnect().catch(err => {
|
||||
logger('Failed to disconnect from browser', err);
|
||||
});
|
||||
}
|
||||
|
||||
export type Channel = 'stable' | 'canary' | 'beta' | 'dev';
|
||||
@@ -0,0 +1,180 @@
|
||||
/**
|
||||
* @license
|
||||
* Copyright 2026 Google LLC
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
import assert from 'node:assert';
|
||||
import type {ChildProcessByStdio} from 'node:child_process';
|
||||
import {spawn} from 'node:child_process';
|
||||
import type {Readable, Writable} from 'node:stream';
|
||||
import {describe, it} from 'node:test';
|
||||
|
||||
import {executablePath} from 'puppeteer';
|
||||
|
||||
type Server = ChildProcessByStdio<Writable, Readable, Readable>;
|
||||
|
||||
// Once shutdown is signalled, the server should be fully gone within this
|
||||
// budget. The actual fast path is well under 500ms; the budget is set to be
|
||||
// generous against CI noise without being so loose that it would hide a hang.
|
||||
const SHUTDOWN_BUDGET_MS = 3000;
|
||||
// Outer test timeout. If exit doesn't happen within this, treat as a hang
|
||||
// (the bug we're guarding against) and SIGKILL the subprocess.
|
||||
const EXIT_TIMEOUT_MS = 15000;
|
||||
|
||||
async function spawnServer(): Promise<Server> {
|
||||
const child = spawn(
|
||||
'node',
|
||||
[
|
||||
'build/src/bin/chrome-devtools-mcp.js',
|
||||
'--headless',
|
||||
'--isolated',
|
||||
'--executable-path',
|
||||
await executablePath(),
|
||||
],
|
||||
{
|
||||
env: {
|
||||
...process.env,
|
||||
CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS: 'true',
|
||||
},
|
||||
stdio: ['pipe', 'pipe', 'pipe'],
|
||||
},
|
||||
) as Server;
|
||||
// Drain stderr to avoid pipe-buffer backpressure stalling the server.
|
||||
child.stderr.on('data', () => {
|
||||
// discard
|
||||
});
|
||||
return child;
|
||||
}
|
||||
|
||||
async function waitForExit(
|
||||
child: Server,
|
||||
timeoutMs: number,
|
||||
): Promise<{
|
||||
code: number | null;
|
||||
signal: NodeJS.Signals | null;
|
||||
elapsedMs: number;
|
||||
}> {
|
||||
const start = Date.now();
|
||||
return await new Promise((resolve, reject) => {
|
||||
const timer = setTimeout(() => {
|
||||
child.kill('SIGKILL');
|
||||
reject(new Error(`server did not exit within ${timeoutMs}ms`));
|
||||
}, timeoutMs);
|
||||
child.once('exit', (code, signal) => {
|
||||
clearTimeout(timer);
|
||||
resolve({code, signal, elapsedMs: Date.now() - start});
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
async function rpc(
|
||||
child: Server,
|
||||
msg: {method: string; params?: unknown},
|
||||
): Promise<unknown> {
|
||||
const id = Math.floor(Math.random() * 1e9);
|
||||
const payload = JSON.stringify({jsonrpc: '2.0', id, ...msg}) + '\n';
|
||||
return await new Promise((resolve, reject) => {
|
||||
let buf = '';
|
||||
const onData = (chunk: Buffer) => {
|
||||
buf += chunk.toString();
|
||||
const lines = buf.split('\n');
|
||||
buf = lines.pop() ?? '';
|
||||
for (const line of lines) {
|
||||
if (!line.trim()) {
|
||||
continue;
|
||||
}
|
||||
try {
|
||||
const parsed = JSON.parse(line) as {id?: number};
|
||||
if (parsed.id === id) {
|
||||
child.stdout.off('data', onData);
|
||||
resolve(parsed);
|
||||
return;
|
||||
}
|
||||
} catch {
|
||||
// Not a JSON message; ignore.
|
||||
}
|
||||
}
|
||||
};
|
||||
child.stdout.on('data', onData);
|
||||
const onExit = () => {
|
||||
child.stdout.off('data', onData);
|
||||
reject(new Error('server exited before RPC response'));
|
||||
};
|
||||
child.once('exit', onExit);
|
||||
child.stdin.write(payload);
|
||||
});
|
||||
}
|
||||
|
||||
function notify(child: Server, msg: {method: string; params?: unknown}): void {
|
||||
child.stdin.write(JSON.stringify({jsonrpc: '2.0', ...msg}) + '\n');
|
||||
}
|
||||
|
||||
async function initializeAndLaunchBrowser(child: Server): Promise<void> {
|
||||
await rpc(child, {
|
||||
method: 'initialize',
|
||||
params: {
|
||||
protocolVersion: '2024-11-05',
|
||||
capabilities: {},
|
||||
clientInfo: {name: 'shutdown-test', version: '0.0.1'},
|
||||
},
|
||||
});
|
||||
notify(child, {method: 'notifications/initialized'});
|
||||
// navigate_page forces a real Chrome launch — this is what reproduces
|
||||
// the hang in #2116. Without an active Chrome subprocess, stdin EOF
|
||||
// would close the event loop on its own and shutdown would look fine
|
||||
// even with broken handlers.
|
||||
await rpc(child, {
|
||||
method: 'tools/call',
|
||||
params: {
|
||||
name: 'navigate_page',
|
||||
arguments: {url: 'about:blank'},
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
describe('shutdown', () => {
|
||||
it('exits within budget on stdin EOF after Chrome launch', async () => {
|
||||
const child = await spawnServer();
|
||||
await initializeAndLaunchBrowser(child);
|
||||
child.stdin.end();
|
||||
const {elapsedMs} = await waitForExit(child, EXIT_TIMEOUT_MS);
|
||||
assert.ok(
|
||||
elapsedMs < SHUTDOWN_BUDGET_MS,
|
||||
`stdin-EOF shutdown took ${elapsedMs}ms (budget ${SHUTDOWN_BUDGET_MS}ms)`,
|
||||
);
|
||||
});
|
||||
|
||||
it('exits within budget on SIGTERM after Chrome launch', async () => {
|
||||
const child = await spawnServer();
|
||||
await initializeAndLaunchBrowser(child);
|
||||
child.kill('SIGTERM');
|
||||
const {elapsedMs} = await waitForExit(child, EXIT_TIMEOUT_MS);
|
||||
assert.ok(
|
||||
elapsedMs < SHUTDOWN_BUDGET_MS,
|
||||
`SIGTERM shutdown took ${elapsedMs}ms (budget ${SHUTDOWN_BUDGET_MS}ms)`,
|
||||
);
|
||||
});
|
||||
|
||||
it('exits within budget on SIGINT after Chrome launch', async () => {
|
||||
const child = await spawnServer();
|
||||
await initializeAndLaunchBrowser(child);
|
||||
child.kill('SIGINT');
|
||||
const {elapsedMs} = await waitForExit(child, EXIT_TIMEOUT_MS);
|
||||
assert.ok(
|
||||
elapsedMs < SHUTDOWN_BUDGET_MS,
|
||||
`SIGINT shutdown took ${elapsedMs}ms (budget ${SHUTDOWN_BUDGET_MS}ms)`,
|
||||
);
|
||||
});
|
||||
|
||||
it('exits within budget on SIGHUP after Chrome launch', async () => {
|
||||
const child = await spawnServer();
|
||||
await initializeAndLaunchBrowser(child);
|
||||
child.kill('SIGHUP');
|
||||
const {elapsedMs} = await waitForExit(child, EXIT_TIMEOUT_MS);
|
||||
assert.ok(
|
||||
elapsedMs < SHUTDOWN_BUDGET_MS,
|
||||
`SIGHUP shutdown took ${elapsedMs}ms (budget ${SHUTDOWN_BUDGET_MS}ms)`,
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user