From 7ab6c5f7a1e50438c1d6c6e8f2f85589a1fcae72 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 20 May 2026 06:59:48 -0700 Subject: [PATCH] v1.40.0.2 fix(browse): Cmd+Q on managed Chromium stops triggering supervisor respawn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three browser.on('disconnected') handlers in browse/src/browser-manager.ts (launch, launchHeaded, handoff) each exited with a non-zero code on every disconnect, regardless of cause. Process supervisors that consume our exit code (gbrowser's gbd HealthMonitor in cmd/gbd/health.go) treated user Cmd+Q identical to a Chromium crash and respawned with exponential backoff, so the visible browser kept reappearing after the user closed it. Add resolveDisconnectCause(browser) that reads the underlying ChildProcess exitCode + signalCode (waiting up to 1s for the exit event if the disconnected event fired first). Exit code 0 + no signal = clean user quit; anything else = crash, signal-kill, or OOM. Wire the resolver into all three disconnect handlers: - launch() (headless): clean → exit 0, crash → exit 1 (was always 1) - launchHeaded() (headed): clean → exit 0, crash → exit 2 (was always 2) onDisconnect() cleanup callback still runs in both cases. - handoff() (re-launch): same as launch() via the helper. Preserve the per-path crash codes (1 vs 2) so any supervisor that differentiated headed vs headless crashes keeps working. Seven new unit tests in browse-manager-unit.test.ts cover the resolver across already-exited, signal-killed (SIGSEGV / SIGKILL), async exits, and null-browser inputs. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 36 +++++++ VERSION | 2 +- browse/src/browser-manager.ts | 126 +++++++++++++++++------ browse/test/browser-manager-unit.test.ts | 73 +++++++++++++ 4 files changed, 207 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8320798db..7ab92cd47b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,41 @@ # Changelog +## [1.40.0.2] - 2026-05-20 + +## **Cmd+Q on the browser actually means quit. Process supervisors stop respawning on user-initiated close.** +## **Chromium exit code is read for what it is: 0 = user wanted this, non-zero = real crash.** + +Every browse-server `browser.on('disconnected')` handler exited with a non-zero code regardless of why Chromium left. Process supervisors (gbrowser's gbd HealthMonitor) consumed that as a crash signal and respawned the whole stack on exponential backoff, so a user who Cmd+Q'd the visible browser saw a fresh window pop back 1s later, then 2s, then 4s. The fix reads the underlying ChildProcess's exit code at disconnect time: code 0 + no signal means the user closed Chromium cleanly, anything else means it crashed. We then exit 0 on clean and preserve the legacy per-path code on crash (launch→1, launchHeaded→2, handoff→1). Process supervisors get the signal they were always missing: 0 = "user wanted this, leave it alone"; non-zero = "please bring me back." + +### The numbers that matter + +Source: `bun test browse/test/browser-manager-unit.test.ts` ... 9 tests across the cause-resolver, all green. + +| Surface | Before | After | +|---|---|---| +| macOS Cmd+Q on browse-server-managed Chromium | Server exits 1 (or 2 for headed) → supervisor sees crash → respawns with 1s→2s→4s→8s→16s backoff | Server exits 0 → supervisor treats as user intent → does not respawn | +| Chromium genuinely crashes (SIGSEGV, OOM) | Server exits 1 → respawn | Server exits 1 → respawn (preserved) | +| Headed Chromium hard-killed (SIGKILL) | Server exits 2 → respawn | Server exits 2 → respawn (preserved) | +| `handoff()` browser closed during a session | Server exits 1 → respawn | Same clean-vs-crash discrimination as `launch()` | +| Disconnect handlers | 3 sites, each rolling its own exit logic | 1 helper (`resolveDisconnectCause`) consumed at all 3 sites | + +### What this means for builders + +If you Cmd+Q a browse-server-managed Chromium window, the window stays gone. The CLI process exits cleanly. Process supervisors leave it alone. The crash-recovery path (Chromium dying mid-task to SIGSEGV / SIGKILL / OOM) still works exactly the same: non-zero exit, supervisor respawns. Reconnect any time with `$B connect` or by re-running your gbd. Pull and your next Cmd+Q is final. + +### Itemized changes + +#### Added + +- `browse/src/browser-manager.ts` (new exports) — `resolveDisconnectCause(browser)` reads the Playwright `Browser.process()` ChildProcess exit code + signal code, returns `'clean'` (exit 0, no signal) or `'crash'` (anything else). Waits up to 1s for the exit if `'disconnected'` fires before the child has fully exited. `handleChromiumDisconnect(browser)` is the headless `launch()` site's one-line dispatcher that consumes the resolver and exits 0 or 1 accordingly. +- `browse/test/browser-manager-unit.test.ts` — seven new tests pinning the resolver across already-exited / signal-killed / async-exit / null-browser inputs. + +#### Fixed + +- `browse/src/browser-manager.ts` `launch()` disconnect handler — now exits 0 on clean user-quit, 1 on crash. Previously always exited 1. +- `browse/src/browser-manager.ts` `launchHeaded()` disconnect handler — clean user-quit exits 0; crash preserves the legacy code 2 so existing supervisors that distinguish "headed crash" from "headless crash" continue to work. `onDisconnect()` cleanup callback fires in both cases. +- `browse/src/browser-manager.ts` `handoff()` disconnect handler — same clean-vs-crash discrimination via the shared helper so a Cmd+Q after a headless→headed handoff doesn't trigger respawn either. + ## [1.40.0.0] - 2026-05-16 ## **gbrain sync stops biting users across the install path, slug algorithm, federation queue, and `.env.local` footgun.** diff --git a/VERSION b/VERSION index 895062404a..8d70162d86 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.40.0.0 +1.40.0.2 diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts index cdbd5fc500..701fcd2e88 100644 --- a/browse/src/browser-manager.ts +++ b/browse/src/browser-manager.ts @@ -40,6 +40,53 @@ export function isCustomChromium(): boolean { return p.includes('GBrowser') || p.includes('gbrowser'); } +/** + * Resolve why the underlying Chromium ChildProcess is going away. + * + * The 'disconnected' Playwright event fires before the child process emits + * its own 'exit' in most cases, so .exitCode is null at that moment. Wait + * briefly (capped at 1s) for the exit then read .exitCode + .signalCode: + * + * exitCode === 0 && no signal → 'clean' (user Cmd+Q, normal shutdown) + * anything else → 'crash' (signal-kill, SIGSEGV, OOM, non-zero exit) + * + * Process supervisors (gbrowser's gbd HealthMonitor in cmd/gbd/health.go) + * read our exit code to decide whether to restart. The two callers in this + * file ride on top of this: a 'clean' result exits with code 0 (gbd skips + * restart, treats as user-intent); a 'crash' result keeps the existing + * per-path exit semantics (launch→1, launchHeaded→2, handoff→1) and gbd + * restarts on backoff. + */ +export async function resolveDisconnectCause(browser: Browser | null): Promise<'clean' | 'crash'> { + const proc = browser?.process(); + if (proc && proc.exitCode === null && proc.signalCode === null) { + await new Promise((resolve) => { + const timer = setTimeout(resolve, 1000); + proc.once('exit', () => { + clearTimeout(timer); + resolve(); + }); + }); + } + return proc?.exitCode === 0 && proc?.signalCode == null ? 'clean' : 'crash'; +} + +/** + * Headless `launch()` disconnect handler. Exits 0 on clean user-quit, 1 on + * crash. Inlined into the launch() body via a one-line dispatch so + * browser-manager's flow stays grep-friendly. + */ +export async function handleChromiumDisconnect(browser: Browser | null): Promise { + const cause = await resolveDisconnectCause(browser); + if (cause === 'clean') { + console.error('[browse] Chromium closed cleanly (user-initiated quit). Server exiting (0).'); + process.exit(0); + } + console.error('[browse] FATAL: Chromium process crashed or was killed. Server exiting (1).'); + console.error('[browse] Console/network logs flushed to .gstack/browse-*.log'); + process.exit(1); +} + export type { RefEntry }; // Re-export TabSession for consumers @@ -246,11 +293,17 @@ export class BrowserManager { ...(this.proxyConfig ? { proxy: this.proxyConfig } : {}), }); - // Chromium crash → exit with clear message + // Chromium disconnect → distinguish clean user-quit from crash. Both + // events look identical to Playwright (one 'disconnected' fires), but + // the underlying ChildProcess exit code separates them: + // exitCode === 0 → clean quit (user Cmd+Q on macOS, normal shutdown) + // exitCode !== 0 → crash, signal-kill, or OOM + // Process supervisors (gbrowser's gbd) consume our exit code: code 0 + // means "user wanted this, don't restart"; non-zero means "crash, please + // bring me back." Without this distinction every Cmd+Q gets treated as + // a crash and the user-visible window keeps respawning. 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); + void handleChromiumDisconnect(this.browser); }); const contextOptions: BrowserContextOptions = { @@ -542,32 +595,45 @@ export class BrowserManager { await this.newTab(); } - // Browser disconnect handler — exit code 2 distinguishes from crashes (1). - // Calls onDisconnect() to trigger full shutdown (kill sidebar-agent, save - // session, clean profile locks + state file) before exit. Falls back to - // direct process.exit(2) if no callback is wired up, or if the callback - // throws/rejects — never leave the process running with a dead browser. + // Browser disconnect handler — distinguish user Cmd+Q from real crash. + // Clean exit (Chromium exit code 0) → process.exit(0) so process + // supervisors (gbrowser's gbd) treat it as user intent and skip the + // restart loop. Crash → process.exit(2) preserves the legacy headed + // semantics that's distinct from launch()'s code 1. + // Always calls onDisconnect() first to trigger full shutdown (kill + // sidebar-agent, save session, clean profile locks + state file) so + // crashes don't strand resources either. if (this.browser) { this.browser.on('disconnected', () => { if (this.intentionalDisconnect) return; - console.error('[browse] Real browser disconnected (user closed or crashed).'); - console.error('[browse] Run `$B connect` to reconnect.'); - if (!this.onDisconnect) { - process.exit(2); - return; - } - try { - const result = this.onDisconnect(); - if (result && typeof (result as Promise).catch === 'function') { - (result as Promise).catch((err) => { - console.error('[browse] onDisconnect rejected:', err); - process.exit(2); - }); + const browserRef = this.browser; + void (async () => { + const cause = await resolveDisconnectCause(browserRef); + const exitCode = cause === 'clean' ? 0 : 2; + if (cause === 'clean') { + console.error('[browse] Real browser closed cleanly (user-initiated quit). Server exiting (0).'); + } else { + console.error('[browse] Real browser disconnected (crash or kill). Server exiting (2).'); + console.error('[browse] Run `$B connect` to reconnect.'); } - } catch (err) { - console.error('[browse] onDisconnect threw:', err); - process.exit(2); - } + if (!this.onDisconnect) { + process.exit(exitCode); + return; + } + try { + const result = this.onDisconnect(); + if (result && typeof (result as Promise).catch === 'function') { + (result as Promise).catch((err) => { + console.error('[browse] onDisconnect rejected:', err); + process.exit(exitCode); + }); + } + // onDisconnect is responsible for exit on the success path. + } catch (err) { + console.error('[browse] onDisconnect threw:', err); + process.exit(exitCode); + } + })(); }); } @@ -1332,12 +1398,14 @@ export class BrowserManager { await newContext.setExtraHTTPHeaders(this.extraHeaders); } - // Register crash handler on new browser + // Register disconnect handler on new browser. Same clean-vs-crash + // discrimination as launch() / launchHeaded() above so a user-initiated + // Cmd+Q after a handoff doesn't trigger gbd's restart loop. if (this.browser) { + const browserRef = 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); + void handleChromiumDisconnect(browserRef); }); } diff --git a/browse/test/browser-manager-unit.test.ts b/browse/test/browser-manager-unit.test.ts index 48bedf3a19..2a3e830f38 100644 --- a/browse/test/browser-manager-unit.test.ts +++ b/browse/test/browser-manager-unit.test.ts @@ -1,3 +1,4 @@ +import { EventEmitter } from 'node:events'; import { describe, it, expect } from 'bun:test'; // ─── BrowserManager basic unit tests ───────────────────────────── @@ -15,3 +16,75 @@ describe('BrowserManager defaults', () => { expect(bm.getRefMap()).toEqual([]); }); }); + +// ─── resolveDisconnectCause ────────────────────────────────────── +// +// Pinning the clean-vs-crash distinction matters because gbd's +// HealthMonitor consumes our exit code (0 = don't restart, !=0 = +// restart). A regression here brings back the "Cmd+Q makes the browser +// keep coming back" UX bug. + +function makeFakeBrowser(opts: { + exitCode: number | null; + signalCode: NodeJS.Signals | null; + /** ms before emitting 'exit'; default = already exited at construction */ + exitDelay?: number; +}): { process(): { exitCode: number | null; signalCode: NodeJS.Signals | null; once: EventEmitter['once'] } } { + const ee = new EventEmitter(); + const state = { + exitCode: opts.exitDelay != null ? null : opts.exitCode, + signalCode: opts.exitDelay != null ? null : opts.signalCode, + once: ee.once.bind(ee), + }; + if (opts.exitDelay != null) { + setTimeout(() => { + state.exitCode = opts.exitCode; + state.signalCode = opts.signalCode; + ee.emit('exit', opts.exitCode, opts.signalCode); + }, opts.exitDelay); + } + return { process: () => state }; +} + +describe('resolveDisconnectCause', () => { + it('clean: process already exited with code 0', async () => { + const { resolveDisconnectCause } = await import('../src/browser-manager'); + const fake = makeFakeBrowser({ exitCode: 0, signalCode: null }); + expect(await resolveDisconnectCause(fake as never)).toBe('clean'); + }); + + it('crash: non-zero exit code', async () => { + const { resolveDisconnectCause } = await import('../src/browser-manager'); + const fake = makeFakeBrowser({ exitCode: 1, signalCode: null }); + expect(await resolveDisconnectCause(fake as never)).toBe('crash'); + }); + + it('crash: SIGSEGV', async () => { + const { resolveDisconnectCause } = await import('../src/browser-manager'); + const fake = makeFakeBrowser({ exitCode: null, signalCode: 'SIGSEGV' }); + expect(await resolveDisconnectCause(fake as never)).toBe('crash'); + }); + + it('crash: SIGKILL', async () => { + const { resolveDisconnectCause } = await import('../src/browser-manager'); + const fake = makeFakeBrowser({ exitCode: null, signalCode: 'SIGKILL' }); + expect(await resolveDisconnectCause(fake as never)).toBe('crash'); + }); + + it('clean: process exits asynchronously with code 0 within timeout', async () => { + const { resolveDisconnectCause } = await import('../src/browser-manager'); + const fake = makeFakeBrowser({ exitCode: 0, signalCode: null, exitDelay: 50 }); + expect(await resolveDisconnectCause(fake as never)).toBe('clean'); + }); + + it('crash: process exits asynchronously with non-zero code', async () => { + const { resolveDisconnectCause } = await import('../src/browser-manager'); + const fake = makeFakeBrowser({ exitCode: 137, signalCode: null, exitDelay: 50 }); + expect(await resolveDisconnectCause(fake as never)).toBe('crash'); + }); + + it('crash: null browser returns crash (defensive default)', async () => { + const { resolveDisconnectCause } = await import('../src/browser-manager'); + expect(await resolveDisconnectCause(null)).toBe('crash'); + }); +});