diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index 4863c29a7..6a0050a7d 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -466,6 +466,16 @@ export class BrowserManager { // Playwright cache (no executablePath), so the heal is never scoped out. this.browser = await launchWithXProtectHeal(() => chromium.launch({ headless: useHeadless, + // #2220: the daemon owns signal policy, not Playwright. Playwright's + // default handlers close Chromium the moment THIS process receives + // SIGINT/SIGTERM/SIGHUP — which fights the deliberate headless + // SIGTERM-ignore in server.ts (the daemon survives the signal but + // loses its browser out from under it). All three are false; server.ts + // routes the signals it actually honors through activeShutdown, which + // closes Chromium itself. + handleSIGINT: false, + handleSIGTERM: false, + handleSIGHUP: false, // On Windows, Chromium's sandbox fails when the server is spawned through // the Bun→Node process chain (GitHub #276). Disable it — local daemon // browsing user-specified URLs has marginal sandbox benefit. Also disabled @@ -663,6 +673,10 @@ export class BrowserManager { // reinstalled over (probePoisonedChromiumBundle's scope contract). this.context = await launchWithXProtectHeal(() => chromium.launchPersistentContext(userDataDir, { headless: false, + // #2220: daemon owns signal policy — see launch() for the rationale. + handleSIGINT: false, + handleSIGTERM: false, + handleSIGHUP: false, // Match the sandbox policy used by launch() above. Without this, // Playwright auto-adds --no-sandbox on every headed launch and the user // sees Chromium's "unsupported command-line flag" yellow infobar. @@ -1717,6 +1731,10 @@ export class BrowserManager { // exactly as in launch()/launchHeaded(). newContext = await launchWithXProtectHeal(() => chromium.launchPersistentContext(userDataDir, { headless: false, + // #2220: daemon owns signal policy — see launch() for the rationale. + handleSIGINT: false, + handleSIGTERM: false, + handleSIGHUP: false, // Match the sandbox policy used by launchHeaded() / launch(). The // handoff path is the headless→headed re-launch and shares the same // anti-detection posture, including no spurious --no-sandbox infobar. diff --git a/browse/src/server.ts b/browse/src/server.ts index be6f5438b..7b234fbf9 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -1493,6 +1493,13 @@ async function handleCommand(body: any, tokenInfo?: TokenInfo | null): Promise activeShutdown?.()); + // SIGHUP (terminal hangup): with handleSIGHUP:false at the three launch + // sites (#2220), Playwright no longer closes Chromium when this process + // gets hung up on — this handler is now the ONLY Chromium cleanup on + // SIGHUP (ENG-OV4). Route to the same shutdown path as SIGINT: + // activeShutdown closes the browser, releases ports, and removes the + // state file. Without it, a hangup would leak a live Chromium. + process.on('SIGHUP', () => activeShutdown?.()); // SIGTERM behavior depends on mode: // - Normal (headless) mode: Claude Code's Bash sandbox fires SIGTERM when the // parent shell exits between tool invocations. Ignoring it keeps the server diff --git a/browse/test/launch-signal-flags.test.ts b/browse/test/launch-signal-flags.test.ts new file mode 100644 index 000000000..2a26fa4bc --- /dev/null +++ b/browse/test/launch-signal-flags.test.ts @@ -0,0 +1,74 @@ +/** + * Static tripwire for #2220: every Playwright launch site must disable + * Playwright's process-level signal handlers (handleSIGINT / handleSIGTERM / + * handleSIGHUP), and server.ts must own the SIGHUP cleanup those flags + * remove. + * + * WHY handleSIGTERM:false is correct here: server.ts DELIBERATELY ignores + * SIGTERM in normal headless mode (the process.on('SIGTERM') handler — + * Claude Code's Bash sandbox fires SIGTERM when the parent shell exits + * between tool invocations, and the daemon must survive it). Playwright's + * default handleSIGTERM:true registers its OWN handler that closes Chromium + * on the same signal — so the daemon survived but its browser died out from + * under it. With the flag false, the daemon's signal policy is the only + * signal policy: the signals server.ts honors route through activeShutdown, + * which closes Chromium itself. + * + * WHY server.ts needs a SIGHUP handler (ENG-OV4): before #2220 the daemon + * had NO process-level SIGHUP handler — Playwright's default handleSIGHUP + * was the only thing closing Chromium on hangup. Flipping the flag without + * adding a handler would leak a live Chromium on every hangup. + * + * Source-level, same style as windows-spawn-hide.test.ts: cheap, + * deterministic, runs on every platform. + */ + +import { describe, expect, test } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; + +const SRC = (f: string) => fs.readFileSync(path.join(import.meta.dir, '../src', f), 'utf-8'); + +/** Every occurrence of `needle` must carry all three handleSIG* flags within + * the next `window` chars (the launch options object). */ +function expectSignalFlagsNearEvery(src: string, needle: string, window = 1200): number { + let idx = src.indexOf(needle); + expect(idx).toBeGreaterThanOrEqual(0); + let count = 0; + while (idx !== -1) { + const slice = src.slice(idx, idx + window); + expect(slice).toMatch(/handleSIGINT:\s*false/); + expect(slice).toMatch(/handleSIGTERM:\s*false/); + expect(slice).toMatch(/handleSIGHUP:\s*false/); + count++; + idx = src.indexOf(needle, idx + needle.length); + } + return count; +} + +describe('Playwright launch sites disable signal handlers (#2220)', () => { + test('all chromium.launch / launchPersistentContext sites carry the three flags', () => { + const src = SRC('browser-manager.ts'); + const launchCount = expectSignalFlagsNearEvery(src, 'chromium.launch({'); + const persistentCount = expectSignalFlagsNearEvery(src, 'chromium.launchPersistentContext('); + // Three launch sites today: headless launch(), headed launchHeaded(), + // and the handoff relaunch. A NEW launch site must carry the flags too — + // bump this only after adding them. + expect(launchCount + persistentCount).toBe(3); + }); + + test('server.ts owns SIGHUP cleanup now that Playwright does not (ENG-OV4)', () => { + const src = SRC('server.ts'); + // The SIGHUP handler must route to the same shutdown path Chromium + // cleanup uses (activeShutdown), like SIGINT does. + expect(src).toMatch(/process\.on\('SIGHUP',\s*\(\)\s*=>\s*activeShutdown\?\.\(\)\)/); + }); + + test('the deliberate headless SIGTERM-ignore still exists (the reason handleSIGTERM:false is safe)', () => { + const src = SRC('server.ts'); + // If this handler ever disappears, revisit handleSIGTERM:false — the + // flag is correct BECAUSE server.ts owns SIGTERM policy. + expect(src).toContain("process.on('SIGTERM'"); + expect(src).toContain('Received SIGTERM (ignoring'); + }); +});