Merge branch 'main' into fix-vieport-timeout

wolfib · web-flow · commit 7a4689fb2e41 · 2026-05-26T15:46:45.000+02:00
diff --git a/src/bin/chrome-devtools-mcp-main.ts b/src/bin/chrome-devtools-mcp-main.ts
@@ -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,
diff --git a/src/browser.ts b/src/browser.ts
@@ -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';
diff --git a/tests/shutdown.test.ts b/tests/shutdown.test.ts
@@ -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)`,
+    );
+  });
+});
