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-24 10:39:57 +02:00 · 2026-05-20 06:59:48 -07:00
parent ec7d88e424
commit 0268c66b6f
4 changed files with 370 additions and 30 deletions
@@ -63,6 +63,53 @@ export function shouldEnableChromiumSandbox(): boolean {
  return !(process.env.CI || process.env.CONTAINER || isRoot);
 }

+/**
+ * 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
@@ -271,11 +318,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 = {
@@ -587,32 +640,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);
+          }
+        })();
      });
    }

@@ -1383,12 +1449,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 { afterEach, beforeEach, describe, it, expect } from 'bun:test';

 // ─── BrowserManager basic unit tests ─────────────────────────────
@@ -90,3 +91,75 @@ describe('shouldEnableChromiumSandbox', () => {
    expect(shouldEnableChromiumSandbox()).toBe(false);
  });
 });
+
+// ─── 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');
+  });
+});