From b66d8b35febbc9948140feecc9a798ccfe69fe4b Mon Sep 17 00:00:00 2001
From: Garry Tan <garrytan@gmail.com>
Date: Fri, 22 May 2026 07:56:47 -0700
Subject: [PATCH] fix(browse): resolveDisconnectCause crashes on
 persistent-context disconnect
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`browser?.process()` in headed mode reaches a BrowserContext-owned Browser
stub whose `.process` is undefined (not a function). The optional-chain
`browser?.process()` does NOT short-circuit on undefined methods — only
on null/undefined receivers — so it evaluates to `undefined()` and throws
an unhandled rejection. The throw crashes the bun process, gbd respawns
it, the next tab close hits the same path, loop forever.

Reproducer (live in gbrowser amsterdam-v7 right now):

    [overlay] Local listener bound on 127.0.0.1:35300 (PID: 19445)
    [browse] Tab closed (id=1, remaining=0)
    [stderr] [overlay] FATAL unhandled rejection: browser?.process is
             not a function. (In 'browser?.process()', 'browser?.process'
             is undefined)
    [browse] Shutting down...
    ...respawn, same crash, repeat...

Fix: split the null case from the no-process case.
- null browser → 'crash' (preserves the existing contract pinned by the
  "null browser returns crash" test)
- truthy browser without callable .process → 'clean' (persistent contexts
  in headed mode; the user controls the lifecycle so the right default
  is exit 0 / gbd does not restart)
- truthy browser with callable .process → unchanged exit-code introspection

In headed mode we genuinely cannot distinguish "user pressed Cmd+Q or
closed all tabs" from "Chromium crashed" because Playwright doesn't
expose the underlying Chromium PID through a persistent context. The
tradeoff is: if Chromium genuinely crashes in headed mode we now exit 0
and don't auto-restart. That's preferable to the respawn loop, and the
user can re-launch manually if they want.

Test: added "clean: browser without .process method (persistent context)"
which would have caught this bug. All 21 browser-manager-unit tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 browse/src/browser-manager.ts            | 17 ++++++++++++++++-
 browse/test/browser-manager-unit.test.ts | 14 ++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/browse/src/browser-manager.ts b/browse/src/browser-manager.ts
index 7734f0a62..ae7bfa21b 100644
--- a/browse/src/browser-manager.ts
+++ b/browse/src/browser-manager.ts
@@ -88,7 +88,22 @@ export function shouldEnableChromiumSandbox(): boolean {
  * restarts on backoff.
  */
 export async function resolveDisconnectCause(browser: Browser | null): Promise<'clean' | 'crash'> {
-  const proc = browser?.process();
+  // Null browser → 'crash' (defensive default, matches existing contract
+  // pinned by the unit test "null browser returns crash").
+  if (browser === null) return 'crash';
+
+  // Persistent contexts (headed mode) expose a Browser object via
+  // BrowserContext.browser(), but Playwright doesn't surface the underlying
+  // Chromium process through it — `browser.process` is undefined on that
+  // object. Pre-fix, `browser?.process()` evaluated to `undefined()` and
+  // threw an unhandled rejection, crashing the bun process and putting gbd
+  // into a respawn loop. Without process introspection we can't distinguish
+  // "user pressed Cmd+Q or closed all tabs" from "Chromium crashed". In
+  // headed mode the user controls the lifecycle, so the right default is
+  // 'clean': exit 0, gbd does not restart, user re-launches if they want.
+  if (typeof browser.process !== 'function') return 'clean';
+
+  const proc = browser.process();
   if (proc && proc.exitCode === null && proc.signalCode === null) {
     await new Promise<void>((resolve) => {
       const timer = setTimeout(resolve, 1000);
diff --git a/browse/test/browser-manager-unit.test.ts b/browse/test/browser-manager-unit.test.ts
index 45bebc345..aea3f26ac 100644
--- a/browse/test/browser-manager-unit.test.ts
+++ b/browse/test/browser-manager-unit.test.ts
@@ -190,6 +190,20 @@ describe('resolveDisconnectCause', () => {
     const { resolveDisconnectCause } = await import('../src/browser-manager');
     expect(await resolveDisconnectCause(null)).toBe('crash');
   });
+
+  // Regression: persistent contexts in headed mode expose a Browser stub
+  // whose `.process` is undefined (not a function). Pre-fix, calling
+  // `browser?.process()` on such a value threw an unhandled rejection
+  // ("browser?.process is not a function"), crashed the bun process, and
+  // put gbd into a respawn loop on every tab close. The contract for that
+  // case is: default to 'clean' so gbd exits 0 and the user keeps lifecycle
+  // control. Live evidence in gbrowser amsterdam-v7's browse-server.log
+  // before this fix landed.
+  it('clean: browser without .process method (persistent context)', async () => {
+    const { resolveDisconnectCause } = await import('../src/browser-manager');
+    const fake = {} as never;  // truthy, no .process → would have thrown pre-fix
+    expect(await resolveDisconnectCause(fake)).toBe('clean');
+  });
 });
 
 // ─── onDisconnect exit-code propagation (regression test) ──────────