From 342564d4d60a890a1bd1cd38c02066d5bb706309 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 21 May 2026 16:17:29 -0700 Subject: [PATCH] fix(browse): route 4 lifecycle handlers through activeBrowserManager indirection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Module-level idleCheckTick, parent watchdog, SIGTERM handler, and buildFetchHandler's onDisconnect wire all read the module-level BrowserManager directly. For embedders (gbrowser) that pass their own instance into buildFetchHandler, the module-level instance never has launchHeaded() called on it — connectionMode stays 'launched' forever, headed-mode early-returns never fire, and after 30 min of HTTP idle the server self-terminates out from under the overlay. Adds `let activeBrowserManager: BrowserManager` at module scope (symmetric with the existing `let activeShutdown` pattern). buildFetchHandler retargets it at cfg.browserManager and CHAINS cfg.browserManager.onDisconnect to activeShutdown, preserving any caller-installed handler instead of clobbering it. Six edit sites in browse/src/server.ts: - Edit 1 (~705): declare activeBrowserManager - Edit 2 (~596): extract idleCheckTick + __testInternals__ export - Edit 3 (~658): parent watchdog reads activeBrowserManager - Edit 4 (~1387): retarget + chain cfgBrowserManager.onDisconnect - Edit 5 (verify): line 714 default stays in place - Edit 6 (~1212): SIGTERM handler reads activeBrowserManager --- browse/src/server.ts | 68 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 6 deletions(-) diff --git a/browse/src/server.ts b/browse/src/server.ts index 05db6665b..afeb5a09e 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -590,17 +590,39 @@ function resetIdleTimer() { lastActivity = Date.now(); } -const idleCheckInterval = setInterval(() => { +// Named for behavioral testing via __testInternals__. The factory tests in +// server-factory.test.ts call this directly so the idle-shutdown path can be +// exercised without waiting 60s for the interval to fire. +function idleCheckTick() { // Headed mode: the user is looking at the browser. Never auto-die. // Only shut down when the user explicitly disconnects or closes the window. - if (browserManager.getConnectionMode() === 'headed') return; + // Reads via the activeBrowserManager indirection so embedders that pass + // their own BrowserManager into buildFetchHandler hit the right instance. + if (activeBrowserManager.getConnectionMode() === 'headed') return; // Tunnel mode: remote agents may send commands sporadically. Never auto-die. if (tunnelActive) return; if (Date.now() - lastActivity > IDLE_TIMEOUT_MS) { console.log(`[browse] Idle for ${IDLE_TIMEOUT_MS / 1000}s, shutting down`); activeShutdown?.(); } -}, 60_000); +} +const idleCheckInterval = setInterval(idleCheckTick, 60_000); + +// Test-only surface for server-factory.test.ts. Lets the dual-instance +// idle-timer behavior be exercised deterministically without mutating +// Date.now (which would interact with the leaked module-level setInterval). +// Production code must never import this — see `idle timer + onDisconnect +// dual-instance fix` describe block for usage. +export const __testInternals__ = { + idleCheckTick, + setTunnelActive: (v: boolean) => { tunnelActive = v; }, + setLastActivity: (t: number) => { lastActivity = t; }, + // Reset the module-level shutdown latch so tests that drive shutdown to + // completion (process.exit-stubbed) can be followed by tests that also + // need shutdown to fire. Without this, the second test's shutdown + // returns early at the `if (isShuttingDown) return;` guard. + resetShutdownState: () => { isShuttingDown = false; }, +}; // ─── Parent-Process Watchdog ──────────────────────────────────────── // When the spawning CLI process (e.g. a Claude Code session) exits, this @@ -638,7 +660,7 @@ if (BROWSE_PARENT_PID > 0 && !IS_HEADED_WATCHDOG) { // the parent shell between invocations. The idle timeout (30 min) // handles eventual cleanup. if (hasActivePicker()) return; - const headed = browserManager.getConnectionMode() === 'headed'; + const headed = activeBrowserManager.getConnectionMode() === 'headed'; if (headed || tunnelActive) { console.log(`[browse] Parent process ${BROWSE_PARENT_PID} exited in ${headed ? 'headed' : 'tunnel'} mode, shutting down`); activeShutdown?.(); @@ -678,13 +700,22 @@ function emitInspectorEvent(event: any): void { // ─── Server ──────────────────────────────────────────────────── const browserManager = new BrowserManager(); +// Indirection for embedders. Module-level handlers (idleCheckTick, parent +// watchdog, SIGTERM) read activeBrowserManager so that buildFetchHandler can +// retarget them at a caller-supplied BrowserManager. Symmetric with the +// existing `let activeShutdown` pattern at module scope (line ~113). +// Without this, embedders like gbrowser hit the dead module-level instance +// whose connectionMode never leaves 'launched' — and headed mode never +// short-circuits idle-shutdown. +let activeBrowserManager: BrowserManager = 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. Exit code 0 means user-initiated clean quit (Cmd+Q on // macOS) so process supervisors like gbrowser's gbd skip the restart loop; // 2 means a real crash that should respawn. The fallback `?? 2` preserves // legacy crash semantics for any caller that invokes onDisconnect without -// an explicit code. +// an explicit code. This is the safety-net default for the CLI flow before +// any buildFetchHandler call rebinds onDisconnect onto the cfg instance. browserManager.onDisconnect = (code) => activeShutdown?.(code ?? 2); let isShuttingDown = false; @@ -1183,7 +1214,7 @@ if (import.meta.main) { console.log('[browse] Received SIGTERM but cookie picker is active, ignoring to avoid stranding the picker UI'); return; } - const headed = browserManager.getConnectionMode() === 'headed'; + const headed = activeBrowserManager.getConnectionMode() === 'headed'; if (headed || tunnelActive) { console.log(`[browse] Received SIGTERM in ${headed ? 'headed' : 'tunnel'} mode, shutting down`); activeShutdown?.(); @@ -1360,6 +1391,31 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { // differs from the module-level instance. activeShutdown = shutdown; + // Retarget the BrowserManager indirection at the cfg-instance so the + // module-level idleCheckTick + parent watchdog + SIGTERM handler all read + // the right connectionMode. Without this, headed embedders auto-shutdown + // after 30 min of HTTP idle because the dead module-level instance still + // reports connectionMode === 'launched'. + activeBrowserManager = cfgBrowserManager; + + // Wire the cfg-instance's onDisconnect to run shutdown when the user + // closes the headed browser window. CHAIN any caller-provided handler + // instead of overwriting it: gbrowser may have set its own onDisconnect + // before calling buildFetchHandler (e.g. for snapshot/log work that needs + // to run before the process exits). Caller errors are logged but never + // block gstack shutdown — defensive symmetry with the safeUnlinkQuiet / + // safeKill philosophy in error-handling.ts. + const callerOnDisconnect = cfgBrowserManager.onDisconnect; + cfgBrowserManager.onDisconnect = async (code) => { + if (callerOnDisconnect) { + try { await callerOnDisconnect(code); } + catch (err: any) { + console.warn('[browse] caller onDisconnect threw:', err?.message ?? err); + } + } + await activeShutdown?.(code ?? 2); + }; + // Substitute cfgBrowserManager for module-level browserManager in the // dispatcher body so all browser-state reads/writes go through the cfg // instance. Other module-level references (handleCommand, getTokenInfo,