Merge remote-tracking branch 'origin/main' into garrytan/sidebar-claude-timeouts

browse/src/browser-manager.ts

@@ -59,6 +59,13 @@ export function isCustomChromium(): boolean {
  */
 export function shouldEnableChromiumSandbox(): boolean {
   if (process.platform === 'win32') return false;
+  // Explicit user override for Ubuntu/AppArmor and similar environments where
+  // unprivileged Chromium sandboxing is blocked even for normal users (the
+  // sandbox needs unprivileged user namespaces that the host policy denies,
+  // so /qa hangs without --no-sandbox). Setting GSTACK_CHROMIUM_NO_SANDBOX=1
+  // forces the sandbox off without changing the default for everyone else.
+  // See #1562.
+  if (process.env.GSTACK_CHROMIUM_NO_SANDBOX === '1') return false;
   const isRoot = typeof process.getuid === 'function' && process.getuid() === 0;
   return !(process.env.CI || process.env.CONTAINER || isRoot);
 }
@@ -300,12 +307,16 @@ export class BrowserManager {
     }
 
     if (extensionsDir) {
-      launchArgs.push(
-        `--disable-extensions-except=${extensionsDir}`,
-        `--load-extension=${extensionsDir}`,
-        '--window-position=-9999,-9999',
-        '--window-size=1,1',
-      );
+      // Skip --load-extension when running against a custom Chromium build that
+      // already bakes the extension in (e.g., GBrowser / GStack Browser.app).
+      // Loading it twice causes a ServiceWorkerState::SetWorkerId DCHECK crash.
+      if (!isCustomChromium()) {
+        launchArgs.push(
+          `--disable-extensions-except=${extensionsDir}`,
+          `--load-extension=${extensionsDir}`,
+        );
+      }
+      launchArgs.push('--window-position=-9999,-9999', '--window-size=1,1');
       useHeadless = false; // extensions require headed mode; off-screen window simulates headless
       console.log(`[browse] Extensions loaded from: ${extensionsDir}`);
     }

browse/src/cli.ts

@@ -11,6 +11,7 @@
 
 import * as fs from 'fs';
 import * as path from 'path';
+import { spawn as nodeSpawn } from 'child_process';
 import { safeUnlink, safeUnlinkQuiet, safeKill, isProcessAlive } from './error-handling';
 import { writeSecureFile, mkdirSecure } from './file-permissions';
 import { resolveConfig, ensureStateDir, readVersionHash } from './config';
@@ -218,8 +219,6 @@ async function startServer(extraEnv?: Record<string, string>): Promise<ServerSta
   safeUnlink(config.stateFile);
   safeUnlink(path.join(config.stateDir, 'browse-startup-error.log'));
 
-  let proc: any = null;
-
   // Allow the caller to opt out of the parent-process watchdog by setting
   // BROWSE_PARENT_PID=0 in the environment. Useful for CI, non-interactive
   // shells, and short-lived Bash invocations that need the server to outlive
@@ -241,12 +240,22 @@ async function startServer(extraEnv?: Record<string, string>): Promise<ServerSta
       `${extraEnvStr})}).unref()`;
     Bun.spawnSync(['node', '-e', launcherCode], { stdio: ['ignore', 'ignore', 'ignore'] });
   } else {
-    // macOS/Linux: Bun.spawn + unref works correctly
-    proc = Bun.spawn(['bun', 'run', SERVER_SCRIPT], {
-      stdio: ['ignore', 'pipe', 'pipe'],
+    // macOS/Linux: Bun.spawn().unref() only removes the child from Bun's event
+    // loop — it does NOT call setsid(), so the spawned server stays in the
+    // parent's process session. When the CLI runs inside a session-managed
+    // shell (e.g. Claude Code's per-command Bash sandbox, Conductor, CI
+    // step runners), the session leader's exit sends SIGHUP to every PID in
+    // the session, killing the bun server (and its Chromium grandchildren).
+    // Even with BROWSE_PARENT_PID=0 disabling the watchdog, SIGHUP still
+    // reaps the server. Use Node's child_process.spawn with detached:true,
+    // which calls setsid() so the server becomes its own session leader
+    // (PPID=1, STAT=Ss) and survives the spawning shell's exit. Mirrors
+    // the Windows path's rationale — same root cause, different OS API.
+    nodeSpawn('bun', ['run', SERVER_SCRIPT], {
+      detached: true,
+      stdio: ['ignore', 'ignore', 'ignore'],
       env: { ...process.env, BROWSE_STATE_FILE: config.stateFile, BROWSE_PARENT_PID: parentPid, ...extraEnv },
-    });
-    proc.unref();
+    }).unref();
   }
 
   // Wait for server to become healthy.
@@ -261,27 +270,17 @@ async function startServer(extraEnv?: Record<string, string>): Promise<ServerSta
     await Bun.sleep(100);
   }
 
-  // Server didn't start in time — try to get error details
-  if (proc?.stderr) {
-    // macOS/Linux: read stderr from the spawned process
-    const reader = proc.stderr.getReader();
-    const { value } = await reader.read();
-    if (value) {
-      const errText = new TextDecoder().decode(value);
-      throw new Error(`Server failed to start:\n${errText}`);
-    }
-  } else {
-    // Windows: check startup error log (server writes errors to disk since
-    // stderr is unavailable due to stdio: 'ignore' for detachment)
-    const errorLogPath = path.join(config.stateDir, 'browse-startup-error.log');
-    try {
-      const errorLog = fs.readFileSync(errorLogPath, 'utf-8').trim();
-      if (errorLog) {
-        throw new Error(`Server failed to start:\n${errorLog}`);
-      }
-    } catch (e: any) {
-      if (e.code !== 'ENOENT') throw e;
+  // Server didn't start in time — check the on-disk startup error log.
+  // Both platforms now spawn with stdio: 'ignore', so the server writes
+  // errors to disk for the CLI to read (see server.ts start().catch).
+  const errorLogPath = path.join(config.stateDir, 'browse-startup-error.log');
+  try {
+    const errorLog = fs.readFileSync(errorLogPath, 'utf-8').trim();
+    if (errorLog) {
+      throw new Error(`Server failed to start:\n${errorLog}`);
     }
+  } catch (e: any) {
+    if (e.code !== 'ENOENT') throw e;
   }
   throw new Error(`Server failed to start within ${MAX_START_WAIT / 1000}s`);
 }

browse/src/server.ts

@@ -623,17 +623,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
@@ -671,7 +693,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?.();
@@ -711,13 +733,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;
 
@@ -1216,7 +1247,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?.();
@@ -1478,6 +1509,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,

browse/test/browser-manager-unit.test.ts

@@ -29,17 +29,20 @@ describe('shouldEnableChromiumSandbox', () => {
   const origPlatform = process.platform;
   const origCI = process.env.CI;
   const origContainer = process.env.CONTAINER;
+  const origNoSandbox = process.env.GSTACK_CHROMIUM_NO_SANDBOX;
   const origGetuid = process.getuid;
 
   beforeEach(() => {
     delete process.env.CI;
     delete process.env.CONTAINER;
+    delete process.env.GSTACK_CHROMIUM_NO_SANDBOX;
   });
 
   afterEach(() => {
     Object.defineProperty(process, 'platform', { value: origPlatform });
     if (origCI === undefined) delete process.env.CI; else process.env.CI = origCI;
     if (origContainer === undefined) delete process.env.CONTAINER; else process.env.CONTAINER = origContainer;
+    if (origNoSandbox === undefined) delete process.env.GSTACK_CHROMIUM_NO_SANDBOX; else process.env.GSTACK_CHROMIUM_NO_SANDBOX = origNoSandbox;
     process.getuid = origGetuid;
   });
 
@@ -90,6 +93,31 @@ describe('shouldEnableChromiumSandbox', () => {
     const { shouldEnableChromiumSandbox } = await import('../src/browser-manager');
     expect(shouldEnableChromiumSandbox()).toBe(false);
   });
+
+  // #1562 — Ubuntu/AppArmor opt-in override
+  it('linux + GSTACK_CHROMIUM_NO_SANDBOX=1 → false (Ubuntu/AppArmor opt-out)', async () => {
+    setPlatform('linux');
+    process.env.GSTACK_CHROMIUM_NO_SANDBOX = '1';
+    process.getuid = (() => 1000) as typeof process.getuid;
+    const { shouldEnableChromiumSandbox } = await import('../src/browser-manager');
+    expect(shouldEnableChromiumSandbox()).toBe(false);
+  });
+
+  it('darwin + GSTACK_CHROMIUM_NO_SANDBOX=1 → false (env override wins on any platform)', async () => {
+    setPlatform('darwin');
+    process.env.GSTACK_CHROMIUM_NO_SANDBOX = '1';
+    process.getuid = (() => 501) as typeof process.getuid;
+    const { shouldEnableChromiumSandbox } = await import('../src/browser-manager');
+    expect(shouldEnableChromiumSandbox()).toBe(false);
+  });
+
+  it('GSTACK_CHROMIUM_NO_SANDBOX=0 → does NOT trigger override (must be exactly "1")', async () => {
+    setPlatform('linux');
+    process.env.GSTACK_CHROMIUM_NO_SANDBOX = '0';
+    process.getuid = (() => 1000) as typeof process.getuid;
+    const { shouldEnableChromiumSandbox } = await import('../src/browser-manager');
+    expect(shouldEnableChromiumSandbox()).toBe(true);
+  });
 });
 
 // ─── resolveDisconnectCause ──────────────────────────────────────

browse/test/cli-setsid-daemonize.test.ts

@@ -0,0 +1,75 @@
+/**
+ * Coverage for #1612 — macOS/Linux server must survive sandboxed-shell
+ * harnesses by becoming its own session leader (setsid).
+ *
+ * Pre-#1612, Bun.spawn().unref() removed the child from Bun's event loop
+ * but did NOT call setsid(). When the CLI ran inside Claude Code's
+ * per-command sandbox, Conductor, or CI step runners, the session leader's
+ * exit sent SIGHUP to every PID in the session, killing the bun server.
+ *
+ * The fix routes macOS/Linux spawn through Node's child_process.spawn with
+ * detached:true, which calls setsid() so the server becomes its own session
+ * leader (PPID=1 on Linux, similar reparenting on Darwin).
+ *
+ * The actual setsid syscall is hard to assert in a unit test without a
+ * real spawn — testing here is static: the cli.ts source must use the
+ * Node spawn path on macOS/Linux, with detached:true and .unref(). If a
+ * future refactor reverts to Bun.spawn().unref() on the macOS/Linux branch
+ * the regression returns and these tests fail.
+ */
+import { describe, expect, test } from "bun:test";
+import * as fs from "node:fs";
+import * as path from "node:path";
+
+const ROOT = path.resolve(import.meta.dir, "..", "..");
+const CLI = path.join(ROOT, "browse", "src", "cli.ts");
+
+function read(): string {
+  return fs.readFileSync(CLI, "utf-8");
+}
+
+describe("#1612 macOS/Linux daemonize via Node setsid path", () => {
+  test("cli.ts imports nodeSpawn from child_process (Node spawn alias)", () => {
+    const body = read();
+    // The fix relies on Node's child_process.spawn (which calls setsid on
+    // detached:true), aliased to avoid name collision with Bun.spawn. Match
+    // either `nodeSpawn` or `spawn as nodeSpawn` to be flexible to the
+    // exact import style.
+    expect(body).toMatch(/(spawn as nodeSpawn|nodeSpawn\s*[,}])/);
+    expect(body).toMatch(/from\s+['"]child_process['"]/);
+  });
+
+  test("non-Windows branch uses nodeSpawn(...).unref() with detached:true", () => {
+    const body = read();
+    // Find the non-Windows branch and assert it uses the Node spawn alias
+    // with detached:true. Match the pattern `nodeSpawn(...) ... detached:true`.
+    expect(body).toMatch(/nodeSpawn\([\s\S]{0,500}detached:\s*true/);
+    expect(body).toMatch(/nodeSpawn\([\s\S]{0,500}\.unref\(\)/);
+  });
+
+  test("non-Windows branch comment documents setsid/SIGHUP root cause", () => {
+    const body = read();
+    // The comment block must mention setsid() so a future refactor sees the
+    // why before changing the spawn call.
+    expect(body).toMatch(/setsid/);
+    expect(body).toMatch(/SIGHUP/);
+  });
+
+  test("the spawn call on macOS/Linux is nodeSpawn, not Bun.spawn", () => {
+    const body = read();
+    // Strip line comments before regex matching, so the "Bun.spawn().unref()"
+    // mentions inside the explanatory comment don't trigger false positives.
+    const codeOnly = body
+      .split("\n")
+      .filter((line) => !line.trim().startsWith("//"))
+      .join("\n");
+    // Find the non-Windows branch. The `} else {` block following the
+    // Windows branch. We then require its first ~400 chars contain a
+    // nodeSpawn() call and NOT a Bun.spawn() call (excluding the comment).
+    const nonWindowsStart = codeOnly.indexOf("nodeSpawn('bun'");
+    expect(nonWindowsStart).toBeGreaterThan(-1);
+    const slice = codeOnly.slice(nonWindowsStart, nonWindowsStart + 400);
+    expect(slice).toMatch(/nodeSpawn\(/);
+    expect(slice).not.toMatch(/Bun\.spawn\(/);
+  });
+});

browse/test/server-factory.test.ts

@@ -1,7 +1,8 @@
-import { describe, test, expect, beforeEach } from 'bun:test';
+import { describe, test, expect, beforeEach, mock } from 'bun:test';
 import {
   resolveConfigFromEnv,
   buildFetchHandler,
+  __testInternals__,
   type ServerConfig,
   type ServerHandle,
   type Surface,
@@ -11,6 +12,8 @@ import { __resetRegistry, initRegistry } from '../src/token-registry';
 import { BrowserManager } from '../src/browser-manager';
 import { resolveConfig } from '../src/config';
 import * as crypto from 'crypto';
+import * as fs from 'node:fs';
+import * as path from 'node:path';
 
 /**
  * Tests for the factory-export API surface added so gbrowser (phoenix) can
@@ -381,3 +384,141 @@ describe('buildFetchHandler factory contract', () => {
     expect(() => initRegistry('second-token-pad-to-16-chars')).toThrow(/already initialized/i);
   });
 });
+
+// ─── Idle timer + onDisconnect dual-instance fix (v1.42.3.0) ──────────
+//
+// Before this fix, module-level handlers (idleCheckTick, parent watchdog,
+// SIGTERM, onDisconnect default wire) all read the module-level
+// BrowserManager directly. For embedders (gbrowser) that pass their own
+// BrowserManager into buildFetchHandler, the module-level instance never
+// has launchHeaded() called on it — so connectionMode stays 'launched'
+// forever and headed mode never short-circuits idle-shutdown. Result:
+// 30-min auto-shutdown of overlay sessions.
+//
+// Fix: introduce `let activeBrowserManager` indirection (symmetric with
+// the existing `let activeShutdown` pattern). buildFetchHandler retargets
+// it at cfg.browserManager AND chains cfg.browserManager.onDisconnect to
+// activeShutdown (without clobbering any caller-provided handler).
+
+function makeMockBrowserManager(mode: 'launched' | 'headed') {
+  return {
+    getConnectionMode: () => mode,
+    isWatching: () => false,
+    stopWatch: () => {},
+    close: async () => {},
+    onDisconnect: null as ((code?: number) => void | Promise<void>) | null,
+  };
+}
+
+describe('idle timer + onDisconnect dual-instance fix', () => {
+  beforeEach(() => {
+    __resetRegistry();
+    // Reset module state every test. Bun memoizes the server.ts module
+    // import for the whole test process, so `lastActivity`, `tunnelActive`,
+    // `activeShutdown`, `activeBrowserManager`, and `isShuttingDown` leak
+    // between tests. We reset what we touch here; the rest is fresh
+    // because each test calls buildFetchHandler with a new mock instance.
+    __testInternals__.setTunnelActive(false);
+    __testInternals__.setLastActivity(Date.now());
+    __testInternals__.resetShutdownState();
+  });
+
+  test('CRITICAL — REGRESSION: headed embedder does not auto-shutdown at idle', () => {
+    const exitMock = mock((_code?: number) => { throw new Error('process.exit called'); });
+    const originalExit = process.exit;
+    (process as any).exit = exitMock;
+    try {
+      const mockBM = makeMockBrowserManager('headed');
+      buildFetchHandler(makeMinimalConfig({ browserManager: mockBM as any }));
+      // Drive lastActivity past the idle threshold via the test seam instead
+      // of mutating Date.now — the leaked module-level setInterval would
+      // see fake-time and could fire shutdown if the timing aligned.
+      __testInternals__.setLastActivity(Date.now() - (31 * 60 * 1000));
+      __testInternals__.idleCheckTick();
+      expect(exitMock).not.toHaveBeenCalled();
+    } finally {
+      (process as any).exit = originalExit;
+    }
+  });
+
+  test('headless still auto-shuts down at idle (paired defensive)', async () => {
+    // Non-throwing mock: idleCheckTick fires shutdown as a fire-and-forget
+    // async call. Throwing from process.exit becomes an unhandled rejection
+    // that the test runner catches. Recording the call is enough.
+    const exitMock = mock((_code?: number) => {});
+    const originalExit = process.exit;
+    (process as any).exit = exitMock;
+    try {
+      const mockBM = makeMockBrowserManager('launched');
+      buildFetchHandler(makeMinimalConfig({ browserManager: mockBM as any }));
+      __testInternals__.setLastActivity(Date.now() - (31 * 60 * 1000));
+      __testInternals__.idleCheckTick();
+      // Drain microtasks: shutdown awaits flushBuffers + cfgBrowserManager.close
+      // before reaching process.exit.
+      await Promise.resolve();
+      await Promise.resolve();
+      await new Promise<void>(r => setImmediate(r));
+      await new Promise<void>(r => setImmediate(r));
+      expect(exitMock).toHaveBeenCalled();
+    } finally {
+      (process as any).exit = originalExit;
+    }
+  });
+
+  test('buildFetchHandler chains cfgBrowserManager.onDisconnect, preserving caller-set handler', async () => {
+    const mockBM = makeMockBrowserManager('headed');
+    const callerCb = mock(async (_code?: number) => {});
+    mockBM.onDisconnect = callerCb;
+    buildFetchHandler(makeMinimalConfig({ browserManager: mockBM as any }));
+    // gstack should have wrapped the caller-installed handler instead of
+    // clobbering it (Codex finding: BrowserManager.onDisconnect is a public
+    // field; gbrowser may set it before calling buildFetchHandler).
+    expect(typeof mockBM.onDisconnect).toBe('function');
+    expect(mockBM.onDisconnect).not.toBe(callerCb);
+    // Verify the chain: invoking the wrapped handler runs the caller
+    // callback AND reaches activeShutdown (which calls process.exit at the
+    // very end of its async path). Stubbing process.exit to throw aborts
+    // the chain before isShuttingDown can leak into later tests.
+    const exitMock = mock((_code?: number) => { throw new Error('process.exit called'); });
+    const originalExit = process.exit;
+    (process as any).exit = exitMock;
+    try {
+      await expect((mockBM.onDisconnect as any)(0)).rejects.toThrow('process.exit called');
+      expect(callerCb).toHaveBeenCalledWith(0);
+      expect(exitMock).toHaveBeenCalledWith(0);
+    } finally {
+      (process as any).exit = originalExit;
+    }
+  });
+
+  test('tunnelActive blocks idle-shutdown even in headless mode', () => {
+    const exitMock = mock((_code?: number) => { throw new Error('process.exit called'); });
+    const originalExit = process.exit;
+    (process as any).exit = exitMock;
+    try {
+      const mockBM = makeMockBrowserManager('launched');
+      buildFetchHandler(makeMinimalConfig({ browserManager: mockBM as any }));
+      __testInternals__.setTunnelActive(true);
+      __testInternals__.setLastActivity(Date.now() - (31 * 60 * 1000));
+      __testInternals__.idleCheckTick();
+      expect(exitMock).not.toHaveBeenCalled();
+    } finally {
+      (process as any).exit = originalExit;
+    }
+  });
+
+  test('lifecycle handlers (idleCheckTick + parent watchdog + SIGTERM) read activeBrowserManager, not module-level browserManager', () => {
+    // Static guard against a future refactor reintroducing a stale read.
+    // The 3 lifecycle sites this plan fixed all call getConnectionMode via
+    // the indirection. Other module-level browserManager reads inside
+    // handleCommandInternalImpl (informational mode reporting in response
+    // payloads) are out of scope and intentionally untouched.
+    const src = fs.readFileSync(path.join(__dirname, '..', 'src', 'server.ts'), 'utf-8');
+    const factoryStart = src.indexOf('export function buildFetchHandler');
+    expect(factoryStart).toBeGreaterThan(0);
+    const moduleLevel = src.slice(0, factoryStart);
+    const activeCount = (moduleLevel.match(/activeBrowserManager\.getConnectionMode\(\)/g) || []).length;
+    // Edit 2 (idleCheckTick), Edit 3 (parent watchdog), Edit 6 (SIGTERM).
+    expect(activeCount).toBe(3);
+  });
+});

browse/test/sidebar-ux.test.ts

@@ -1589,19 +1589,17 @@ describe('tool calls collapse into reasoning disclosure', () => {
 });
 
 // ─── Idle timeout disabled in headed mode (server.ts) ───────────
+//
+// The original 'idle check skips in headed mode' string-grep test was deleted
+// in v1.42.3.0 — it would have passed even with the dual-instance bug present
+// because it only grepped for "=== 'headed'" + 'return' in the same window.
+// Behavioral coverage lives in browse/test/server-factory.test.ts under the
+// 'idle timer + onDisconnect dual-instance fix' describe block, which
+// exercises the headed/headless/tunnel branches of idleCheckTick directly.
 
 describe('idle timeout behavior (server.ts)', () => {
   const serverSrc = fs.readFileSync(path.join(ROOT, 'src', 'server.ts'), 'utf-8');
 
-  test('idle check skips in headed mode', () => {
-    const idleCheck = serverSrc.slice(
-      serverSrc.indexOf('idleCheckInterval'),
-      serverSrc.indexOf('idleCheckInterval') + 300,
-    );
-    expect(idleCheck).toContain("=== 'headed'");
-    expect(idleCheck).toContain('return');
-  });
-
   test('sidebar-command resets idle timer', () => {
     const sidebarCmd = serverSrc.slice(
       serverSrc.indexOf("url.pathname === '/sidebar-command'"),