v1.40.0.2 fix(browse): Cmd+Q on managed Chromium stops triggering supervisor respawn

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) <noreply@anthropic.com>
2026-06-17 07:10:12 +02:00 · 2026-05-20 06:59:48 -07:00
parent 026751ea20
commit 7ab6c5f7a1
4 changed files with 207 additions and 30 deletions
@@ -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.**
@@ -1 +1 @@
-1.40.0.0
+1.40.0.2
@@ -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<void>((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<void> {
+  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<void>).catch === 'function') {
-            (result as Promise<void>).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<void>).catch === 'function') {
+              (result as Promise<void>).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);
        });
      }

@@ -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');
+  });
+});
@@ -1 +1 @@
 .40.0.0
 .40.0.2