From 015f6885d72d4a3277e2715c596cdf5b5dff74e2 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 21 May 2026 16:17:50 -0700 Subject: [PATCH] test(browse): pin idle timer + onDisconnect dual-instance fix behaviorally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds 5 behavioral tests to browse/test/server-factory.test.ts under a new 'idle timer + onDisconnect dual-instance fix' describe block: - T1 (CRITICAL — REGRESSION): headed embedder does not auto-shutdown at idle. Pins the bug this PR fixes. - T2 (paired defensive): headless still auto-shuts down at idle. Catches a future refactor that breaks the inverse case. - T3 (chain semantics): buildFetchHandler chains cfgBrowserManager.onDisconnect, preserving any caller-set handler. Uses .rejects.toThrow for the async shutdown path. - T4 (tunnelActive): tunnel-active blocks idle-shutdown even in headless mode. - T5 (static guard): exactly 3 module-level lifecycle sites use activeBrowserManager.getConnectionMode() — idleCheckTick, parent watchdog, SIGTERM. Catches refactor-introduced regressions before CI. Reuses existing makeMinimalConfig() + __resetRegistry() patterns from the factory contract tests. New makeMockBrowserManager() helper. beforeEach also resets module state via setTunnelActive, setLastActivity, and resetShutdownState from __testInternals__. Also deletes the old 'idle check skips in headed mode' string-grep test from browse/test/sidebar-ux.test.ts at line 1596. That test would have passed even with the dual-instance bug present (grepped for "=== 'headed'" + 'return' in the same window). Behavioral coverage moved to server-factory.test.ts. Verified: 33/33 tests pass in browse/test/server-factory.test.ts. --- browse/test/server-factory.test.ts | 143 ++++++++++++++++++++++++++++- browse/test/sidebar-ux.test.ts | 16 ++-- 2 files changed, 149 insertions(+), 10 deletions(-) diff --git a/browse/test/server-factory.test.ts b/browse/test/server-factory.test.ts index bf9d3f7fc..6b5feb264 100644 --- a/browse/test/server-factory.test.ts +++ b/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) | 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(r => setImmediate(r)); + await new Promise(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); + }); +}); diff --git a/browse/test/sidebar-ux.test.ts b/browse/test/sidebar-ux.test.ts index 1ae3feabe..74ced5efd 100644 --- a/browse/test/sidebar-ux.test.ts +++ b/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'"),