From 5099637f95d37c6c821dc9a6270686190597a0ac Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 14 Aug 2026 19:14:10 -0700 Subject: [PATCH] =?UTF-8?q?fix(server):=20delete=20ServerConfig.idleTimeou?= =?UTF-8?q?tMs=20+=20chromiumProfile=20=E2=80=94=20documented,=20never=20r?= =?UTF-8?q?ead?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both fields carried JSDoc asserting embedder behavior that did not exist: the idle check reads the module-level IDLE_TIMEOUT_MS env constant, and both resolveChromiumProfile() call sites pass no argument. Worse than absent — an embedder passing idleTimeoutMs: 5000 silently got 30 minutes. Wiring them honestly is impossible today: the idle timer, activity state, and shutdown target are module-global, so a per-factory value would lie for any process running more than one handler. Deleted instead, with a ServerConfig note pointing at the deferred singleton/route-table refactor where real support belongs. BROWSE_IDLE_TIMEOUT and CHROMIUM_PROFILE env remain the honest knobs. Co-Authored-By: Claude Fable 5 --- browse/src/config.ts | 3 ++- browse/src/server.ts | 11 ++++++----- browse/test/server-embedder-terminal-port.test.ts | 1 - browse/test/server-factory.test.ts | 13 ------------- 4 files changed, 8 insertions(+), 20 deletions(-) diff --git a/browse/src/config.ts b/browse/src/config.ts index fc4c97b95..f73fd71ee 100644 --- a/browse/src/config.ts +++ b/browse/src/config.ts @@ -169,7 +169,8 @@ export function resolveGstackHome(): string { * Resolve the Chromium profile directory. * * Resolution order: - * 1. `explicit` arg (passed via ServerConfig.chromiumProfile by embedders) + * 1. `explicit` arg (no production caller passes one today; kept for + * direct programmatic use) * 2. CHROMIUM_PROFILE env (used by gbrowser's gbd per-workspace) * 3. /chromium-profile (default) */ diff --git a/browse/src/server.ts b/browse/src/server.ts index 77c3d1b1d..f4194bf0c 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -190,14 +190,16 @@ export interface ServerConfig { authToken: string; /** Local listener port. Used in /welcome URL + state-file. */ browsePort: number; - /** Idle shutdown timeout. Default 30 min. */ - idleTimeoutMs: number; /** Result of resolveConfig() — stateDir, auditLog, stateFile. */ config: ReturnType; /** Pre-launched BrowserManager. Caller owns lifecycle. */ browserManager: BrowserManager; - /** Optional Chromium profile path override. Resolved by resolveChromiumProfile(). */ - chromiumProfile?: string; + // NOTE: per-factory idleTimeoutMs and chromiumProfile were deleted — they + // were documented but never read (the idle timer, activity state, and + // shutdown target are module-global, so per-factory wiring would lie for + // any embedder running >1 handler). Real support belongs to the deferred + // server.ts singleton/route-table refactor. Until then: BROWSE_IDLE_TIMEOUT + // and CHROMIUM_PROFILE env are the honest knobs. /** Caller-owned. shutdown() does NOT call xvfb.stop(); caller is responsible. */ xvfb?: XvfbHandle | null; /** Caller-owned. shutdown() does NOT call proxyBridge.close(); caller is responsible. */ @@ -283,7 +285,6 @@ export function resolveConfigFromEnv(): Omit = {}): ServerConfig return { authToken: token, browsePort: 34568, - idleTimeoutMs: 1_800_000, config: resolveConfig(), browserManager: new BrowserManager(), startTime: Date.now(), diff --git a/browse/test/server-factory.test.ts b/browse/test/server-factory.test.ts index 6b5feb264..474fa17e7 100644 --- a/browse/test/server-factory.test.ts +++ b/browse/test/server-factory.test.ts @@ -120,22 +120,11 @@ describe('server.ts factory API surface', () => { } }); - test('reads BROWSE_IDLE_TIMEOUT from env, defaults to 30 min (1800000ms)', () => { - const orig = process.env.BROWSE_IDLE_TIMEOUT; - delete process.env.BROWSE_IDLE_TIMEOUT; - try { - expect(resolveConfigFromEnv().idleTimeoutMs).toBe(1800000); - } finally { - if (orig !== undefined) process.env.BROWSE_IDLE_TIMEOUT = orig; - } - }); - test('returns a populated config object with the expected shape', () => { const cfg = resolveConfigFromEnv(); expect(cfg).toMatchObject({ authToken: expect.any(String), browsePort: expect.any(Number), - idleTimeoutMs: expect.any(Number), config: expect.objectContaining({ stateDir: expect.any(String), stateFile: expect.any(String), @@ -178,7 +167,6 @@ describe('server.ts factory API surface', () => { const minimalConfigShape = { authToken: 'tok', browsePort: 0, - idleTimeoutMs: 1800000, config: { stateDir: '', stateFile: '', consoleLog: '', networkLog: '', dialogLog: '', auditLog: '', projectDir: '' }, browserManager: {} as any, startTime: Date.now(), @@ -217,7 +205,6 @@ function makeMinimalConfig(overrides: Partial = {}): ServerConfig return { authToken: token, browsePort: 34567, - idleTimeoutMs: 1_800_000, config: resolveConfig(), browserManager: new BrowserManager(), startTime: Date.now(),