diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index cdbd5fc500..b66a547635 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -40,6 +40,40 @@ export function isCustomChromium(): boolean { return p.includes('GBrowser') || p.includes('gbrowser'); } +/** + * Classify a Chromium `disconnected` event as a clean drain (the user just + * closed the last tab, Chromium had nothing left to display) vs a real crash. + * + * Before this distinction existed, every disconnect — including the totally + * benign "user closed last tab" path — printed `FATAL: Chromium process + * crashed` and `process.exit(1)`, which killed the sidebar-agent and any + * in-flight PTY work. The smoking gun in the postmortem log was the pair: + * + * [browse] Tab closed (id=N, remaining=0) + * [browse] FATAL: Chromium process crashed or was killed. + * + * Same event, two stories. After: `pages.size === 0` means the disconnect + * is the natural consequence of the close, exit 0, no FATAL. Pages still + * live means real crash, exit 1 — the original behavior, preserved. + * + * Pure function on purpose: unit-testable without spinning up Chromium. + */ +export type DisconnectKind = 'clean-drain' | 'crash'; +export interface DisconnectClassification { + kind: DisconnectKind; + reason: string; + exitCode: 0 | 1; +} +export function classifyDisconnect(opts: { + pagesSize: number; + mode: 'launched' | 'rehead'; +}): DisconnectClassification { + if (opts.pagesSize === 0) { + return { kind: 'clean-drain', reason: `browser-empty-${opts.mode}`, exitCode: 0 }; + } + return { kind: 'crash', reason: `chromium-crash-${opts.mode}`, exitCode: 1 }; +} + export type { RefEntry }; // Re-export TabSession for consumers @@ -119,9 +153,17 @@ export class BrowserManager { // Called when the headed browser disconnects without intentional teardown // (user closed the window). Wired up by server.ts to run full cleanup - // (sidebar-agent, state file, profile locks) before exiting with code 2. - // Returns void or a Promise; rejections are caught and fall back to exit(2). - public onDisconnect: (() => void | Promise) | null = null; + // (sidebar-agent, state file, profile locks) before exiting. + // Returns void or a Promise; rejections are caught and fall back to the + // passed exit code. + // Reason is a short tag describing why the disconnect fired; server.ts + // writes it into ~/.gstack/last-shutdown.json for post-mortem diagnostics. + // exitCode is the process exit code the caller should use after cleanup: + // 0 = clean drain (last tab closed, Chromium had nothing to display), + // 1 = real crash (Chromium died with pages still live), + // 2 = user-explicit close (existing convention from the page-close chain; + // also the default when no exit code is specified). + public onDisconnect: ((reason?: string, exitCode?: 0 | 1 | 2) => void | Promise) | null = null; getConnectionMode(): 'launched' | 'headed' { return this.connectionMode; } @@ -246,11 +288,24 @@ export class BrowserManager { ...(this.proxyConfig ? { proxy: this.proxyConfig } : {}), }); - // Chromium crash → exit with clear message + // Chromium disconnect → classifyDisconnect (see top of file) splits the + // event into clean-drain (last tab closed, exit 0) vs real crash (exit 1). this.browser.on('disconnected', () => { - console.error('[browse] FATAL: Chromium process crashed or was killed. Server exiting.'); - console.error('[browse] Console/network logs flushed to .gstack/browse-*.log'); - process.exit(1); + const c = classifyDisconnect({ pagesSize: this.pages.size, mode: 'launched' }); + if (c.kind === 'clean-drain') { + console.log('[browse] Chromium quit after last tab closed. Shutting down cleanly.'); + } else { + console.error('[browse] FATAL: Chromium process crashed or was killed. Server exiting.'); + console.error('[browse] Console/network logs flushed to .gstack/browse-*.log'); + } + try { + const result = this.onDisconnect?.(c.reason, c.exitCode); + if (result && typeof (result as Promise).catch === 'function') { + (result as Promise).catch(() => process.exit(c.exitCode)); + return; + } + } catch {} + process.exit(c.exitCode); }); const contextOptions: BrowserContextOptions = { @@ -557,7 +612,7 @@ export class BrowserManager { return; } try { - const result = this.onDisconnect(); + const result = this.onDisconnect('browser-disconnect'); if (result && typeof (result as Promise).catch === 'function') { (result as Promise).catch((err) => { console.error('[browse] onDisconnect rejected:', err); @@ -1336,8 +1391,20 @@ export class BrowserManager { if (this.browser) { this.browser.on('disconnected', () => { if (this.intentionalDisconnect) return; - console.error('[browse] FATAL: Chromium process crashed or was killed. Server exiting.'); - process.exit(1); + const c = classifyDisconnect({ pagesSize: this.pages.size, mode: 'rehead' }); + if (c.kind === 'clean-drain') { + console.log('[browse] Chromium quit after last tab closed. Shutting down cleanly.'); + } else { + console.error('[browse] FATAL: Chromium process crashed or was killed. Server exiting.'); + } + try { + const result = this.onDisconnect?.(c.reason, c.exitCode); + if (result && typeof (result as Promise).catch === 'function') { + (result as Promise).catch(() => process.exit(c.exitCode)); + return; + } + } catch {} + process.exit(c.exitCode); }); } diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 4f523bea7a..95513a55fb 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -241,10 +241,32 @@ async function startServer(extraEnv?: Record): Promise= 0 ? logFd : 'pipe', logFd >= 0 ? logFd : 'pipe'], env: { ...process.env, BROWSE_STATE_FILE: config.stateFile, BROWSE_PARENT_PID: parentPid, ...extraEnv }, }); + // Stash the log path for the startup-failure path (proc.stderr is not a + // pipe when stdio was redirected to a file fd, so getReader() would + // throw — read the tail of the log file instead). + if (logFd >= 0) (proc as any).__logPath = logPath; proc.unref(); } @@ -261,8 +283,31 @@ async function startServer(extraEnv?: Record): Promise = {}) { + try { + const stateDir = path.dirname(config.stateFile); + const outPath = path.join(stateDir, 'last-shutdown.json'); + const payload = { + ts: new Date().toISOString(), + pid: process.pid, + reason, + mode: browserManager.getConnectionMode(), + uptimeSeconds: Math.round(process.uptime()), + detail, + }; + // Atomic write: two browse instances writing to the same state dir + // can otherwise interleave JSON bytes and leave a corrupt file. + // tmp-then-rename is the standard POSIX pattern. + const tmpPath = `${outPath}.${process.pid}.tmp`; + fs.writeFileSync(tmpPath, JSON.stringify(payload, null, 2)); + fs.renameSync(tmpPath, outPath); + shutdownReasonRecorded = true; + console.log(`[browse] Shutdown reason recorded: ${reason}`); + } catch (err: any) { + console.warn('[browse] Failed to record shutdown reason:', err?.message); + } +} + // ─── Idle Timer ──────────────────────────────────────────────── let lastActivity = Date.now(); @@ -598,6 +631,7 @@ const idleCheckInterval = setInterval(() => { if (tunnelActive) return; if (Date.now() - lastActivity > IDLE_TIMEOUT_MS) { console.log(`[browse] Idle for ${IDLE_TIMEOUT_MS / 1000}s, shutting down`); + recordShutdownReason('idle-timeout', { idleSeconds: Math.round((Date.now() - lastActivity) / 1000) }); activeShutdown?.(); } }, 60_000); @@ -641,6 +675,7 @@ if (BROWSE_PARENT_PID > 0 && !IS_HEADED_WATCHDOG) { const headed = browserManager.getConnectionMode() === 'headed'; if (headed || tunnelActive) { console.log(`[browse] Parent process ${BROWSE_PARENT_PID} exited in ${headed ? 'headed' : 'tunnel'} mode, shutting down`); + recordShutdownReason('parent-exit', { parentPid: BROWSE_PARENT_PID, mode: headed ? 'headed' : 'tunnel' }); activeShutdown?.(); } else if (!parentGone) { parentGone = true; @@ -678,10 +713,21 @@ function emitInspectorEvent(event: any): void { // ─── Server ──────────────────────────────────────────────────── const browserManager = new BrowserManager(); -// When the user closes the headed browser window, run full cleanup -// (kill sidebar-agent, save session, remove profile locks, delete state file) -// before exiting with code 2. Exit code 2 distinguishes user-close from crashes (1). -browserManager.onDisconnect = () => activeShutdown?.(2); +// Exit-code semantics (passed by browser-manager.ts via classifyDisconnect +// for the Chromium 'disconnected' paths, or unset for the headed-mode +// page-close chain that uses the existing user-close convention): +// 0 — clean drain (Chromium quit after last tab closed) +// 1 — real crash (Chromium died with pages still live, preserves the +// legacy FATAL exit convention) +// 2 — user-explicit close from the page-close chain (default when no +// exit code is passed) +// activeShutdown runs the same full cleanup (kill sidebar-agent, save +// session, remove profile locks, delete state file) regardless. +browserManager.onDisconnect = (reason?: string, exitCode?: 0 | 1 | 2) => { + const code = exitCode ?? 2; + recordShutdownReason(reason || 'browser-disconnect', { exitCode: code }); + return activeShutdown?.(code); +}; let isShuttingDown = false; // Test if a port is available by binding and immediately releasing. @@ -1164,7 +1210,10 @@ async function handleCommand(body: any, tokenInfo?: TokenInfo | null): Promise activeShutdown?.()); + process.on('SIGINT', () => { + recordShutdownReason('signal', { signal: 'SIGINT' }); + 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 @@ -1182,6 +1231,7 @@ if (import.meta.main) { const headed = browserManager.getConnectionMode() === 'headed'; if (headed || tunnelActive) { console.log(`[browse] Received SIGTERM in ${headed ? 'headed' : 'tunnel'} mode, shutting down`); + recordShutdownReason('signal', { signal: 'SIGTERM', mode: headed ? 'headed' : 'tunnel' }); activeShutdown?.(); } else { console.log('[browse] Received SIGTERM (ignoring — use /stop or Ctrl+C for intentional shutdown)'); @@ -1313,6 +1363,16 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { if (isShuttingDown) return; isShuttingDown = true; + // Fallback diagnostics: if no caller recorded a reason yet (e.g. /stop, + // explicit $B disconnect, or a future code path we forgot to wire), + // log something rather than exiting silently. Specific callers + // (idle-timeout, parent-exit, signal handlers, browser-disconnect) + // record their own more-specific reason BEFORE calling activeShutdown, + // and recordShutdownReason overwrites by design — most recent wins. + if (!shutdownReasonRecorded) { + recordShutdownReason('shutdown-called', { exitCode }); + } + console.log('[browse] Shutting down...'); if (ownsTerminalAgent) { try { diff --git a/browse/test/browser-manager-classify-disconnect.test.ts b/browse/test/browser-manager-classify-disconnect.test.ts new file mode 100644 index 0000000000..cfe737b127 --- /dev/null +++ b/browse/test/browser-manager-classify-disconnect.test.ts @@ -0,0 +1,57 @@ +import { describe, test, expect } from 'bun:test'; +import { classifyDisconnect } from '../src/browser-manager'; + +// Pins the FATAL-on-clean-tab-close regression: pages.size === 0 at +// disconnect time means the user closed the last tab, not that Chromium +// crashed. Before this split, both paths exited 1 and the daemon log +// printed `FATAL: Chromium process crashed`, killing the sidebar-agent +// every time a user closed their last tab in headed mode. + +describe('classifyDisconnect — clean-drain vs crash', () => { + test('launched mode, zero pages → clean-drain, exit 0, browser-empty-launched', () => { + expect(classifyDisconnect({ pagesSize: 0, mode: 'launched' })).toEqual({ + kind: 'clean-drain', + reason: 'browser-empty-launched', + exitCode: 0, + }); + }); + + test('launched mode, pages still live → crash, exit 1, chromium-crash-launched', () => { + expect(classifyDisconnect({ pagesSize: 1, mode: 'launched' })).toEqual({ + kind: 'crash', + reason: 'chromium-crash-launched', + exitCode: 1, + }); + expect(classifyDisconnect({ pagesSize: 5, mode: 'launched' })).toEqual({ + kind: 'crash', + reason: 'chromium-crash-launched', + exitCode: 1, + }); + }); + + test('rehead mode, zero pages → clean-drain, exit 0, browser-empty-rehead', () => { + expect(classifyDisconnect({ pagesSize: 0, mode: 'rehead' })).toEqual({ + kind: 'clean-drain', + reason: 'browser-empty-rehead', + exitCode: 0, + }); + }); + + test('rehead mode, pages still live → crash, exit 1, chromium-crash-rehead', () => { + expect(classifyDisconnect({ pagesSize: 3, mode: 'rehead' })).toEqual({ + kind: 'crash', + reason: 'chromium-crash-rehead', + exitCode: 1, + }); + }); + + test('reason tag always carries the mode for postmortem disambiguation', () => { + // ~/.gstack/last-shutdown.json reads this string. Tag must include + // the mode so an embedder log can tell launched-disconnect from + // rehead-disconnect at a glance. + const launched = classifyDisconnect({ pagesSize: 0, mode: 'launched' }); + const rehead = classifyDisconnect({ pagesSize: 0, mode: 'rehead' }); + expect(launched.reason.endsWith('-launched')).toBe(true); + expect(rehead.reason.endsWith('-rehead')).toBe(true); + }); +});