From 94e15c5778d48db135a125fb8f45fc1ed292ee18 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 19 May 2026 19:26:29 -0700 Subject: [PATCH] feat: gate terminal-agent teardown on ServerConfig.ownsTerminalAgent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds ownsTerminalAgent?: boolean to ServerConfig (default true). Wraps the three shutdown side effects (pkill -f terminal-agent\.ts + 2 safeUnlinkQuiet calls for terminal-port and terminal-internal-token) inside a single if (ownsTerminalAgent) block. Embedders (gbrowser phoenix overlay) pass false to keep their own PTY lifecycle intact across gstack's teardown. CLI start() call site passes ownsTerminalAgent: true explicitly; static-grep test in the new test file catches a refactor that drops it. Strict opt-out: only explicit false flips the gate (cfg.ownsTerminalAgent === false ? false : true). Defends against JS callers passing truthy non-bool values. Adds __resetShuttingDown test-only export mirroring __resetRegistry. The module-scoped isShuttingDown latch otherwise silently no-ops a second shutdown() in the same process. Drops dead try/catch wrappers around safeUnlinkQuiet inside the new gate — safeUnlinkQuiet already swallows all errors internally. New test file (4 cases) stubs both process.exit AND child_process.spawnSync so a real pkill -f terminal-agent\.ts never fires on the developer machine. beforeAll/afterAll save and restore real-daemon file contents in the state dir so the test cannot clobber a running gstack session. --- browse/src/server.ts | 75 ++++++- .../server-embedder-terminal-port.test.ts | 189 ++++++++++++++++++ 2 files changed, 255 insertions(+), 9 deletions(-) create mode 100644 browse/test/server-embedder-terminal-port.test.ts diff --git a/browse/src/server.ts b/browse/src/server.ts index 1b1d23bc9..f43f4e91a 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -204,6 +204,35 @@ export interface ServerConfig { * dispatch; returning null falls through. */ beforeRoute?: (req: Request, surface: Surface, auth: TokenInfo | null) => Promise; + /** + * Whether gstack owns the lifecycle of the terminal-agent process and its + * discovery files (`/terminal-port`, `/terminal-internal-token`). + * + * When true (default), shutdown() runs three side effects: + * 1. `pkill -f terminal-agent\.ts` — regex-broad, matches ANY process whose + * command line contains `terminal-agent.ts` on this host (including + * sibling gstack sessions). Pre-existing CLI behavior, not introduced by + * this flag. Identity-based PID kill is a separate followup (see TODOS). + * 2. `safeUnlinkQuiet(/terminal-port)` + * 3. `safeUnlinkQuiet(/terminal-internal-token)` + * + * This is correct for gstack's CLI path, which spawns `terminal-agent.ts` as + * the producer of those files (see cli.ts:1037-1063). + * + * Embedders (gbrowser phoenix overlay, future hosts) that run their own PTY + * server and write those files themselves should pass `false`. When `false`, + * the embedder owns BOTH the agent process AND both discovery files — + * terminal-agent.ts's own SIGTERM cleanup only removes `terminal-port` + * (see terminal-agent.ts:558), so the internal-token file is the embedder's + * full responsibility. + * + * Polarity note: this differs from `xvfb?` and `proxyBridge?`, which gate by + * the *presence* of a caller-owned handle (presence ⇒ don't close). This + * field gates by an explicit boolean because there is no handle object — + * the terminal-agent is started elsewhere (cli.ts), and shutdown's only + * reference is the regex-based pkill + the file paths. + */ + ownsTerminalAgent?: boolean; } /** @@ -1228,8 +1257,11 @@ if (import.meta.main) { /** * Build a request handler set for the browse daemon. Embedders (gbrowser * phoenix overlay) call this directly with their own cfg to compose overlay - * routes via cfg.beforeRoute. The CLI path calls it through start() with - * env-derived defaults — externally-observable behavior is identical. + * routes via cfg.beforeRoute, pass a pre-launched cfg.browserManager, and + * opt out of terminal-agent teardown via cfg.ownsTerminalAgent (default + * true, set to false when the embedder runs its own PTY server). The CLI + * path calls this through start() with env-derived defaults and explicit + * cfg.ownsTerminalAgent: true — externally-observable behavior is identical. * * Auth state lives ENTIRELY inside the factory closure: cfg.authToken is the * single source of truth for the bearer secret, factory-scoped validateAuth @@ -1259,6 +1291,11 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { initRegistry(cfg.authToken); const { authToken, browserManager: cfgBrowserManager, startTime, beforeRoute, browsePort } = cfg; + // Strict opt-out: only explicit `false` flips the gate. Any other value + // (undefined, truthy non-bool from a JS caller bypassing TS, etc.) defaults + // to gstack-owns. Matches the "default-true preserves CLI bit-for-bit" + // premise even under malformed cfg. + const ownsTerminalAgent = cfg.ownsTerminalAgent === false ? false : true; // Factory-scoped validateAuth. Closes over cfg.authToken so every internal // auth check sees the same token the routes receive. Module-level @@ -1276,14 +1313,16 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { isShuttingDown = true; console.log('[browse] Shutting down...'); - try { - const { spawnSync } = require('child_process'); - spawnSync('pkill', ['-f', 'terminal-agent\\.ts'], { stdio: 'ignore', timeout: 3000 }); - } catch (err: any) { - console.warn('[browse] Failed to kill terminal-agent:', err.message); + if (ownsTerminalAgent) { + try { + const { spawnSync } = require('child_process'); + spawnSync('pkill', ['-f', 'terminal-agent\\.ts'], { stdio: 'ignore', timeout: 3000 }); + } catch (err: any) { + console.warn('[browse] Failed to kill terminal-agent:', err.message); + } + safeUnlinkQuiet(path.join(path.dirname(config.stateFile), 'terminal-port')); + safeUnlinkQuiet(path.join(path.dirname(config.stateFile), 'terminal-internal-token')); } - try { safeUnlinkQuiet(path.join(path.dirname(config.stateFile), 'terminal-port')); } catch {} - try { safeUnlinkQuiet(path.join(path.dirname(config.stateFile), 'terminal-internal-token')); } catch {} try { detachSession(); } catch (err: any) { console.warn('[browse] Failed to detach CDP session:', err.message); } @@ -2428,6 +2467,7 @@ export async function start() { xvfb, proxyBridge, startTime, + ownsTerminalAgent: true, // CLI spawns terminal-agent.ts itself (see cli.ts:1037-1063) }); const server = Bun.serve({ @@ -2573,6 +2613,23 @@ export async function start() { } } +/** + * Test-only. Resets the module-level shutdown latch so a second test case + * can exercise shutdown() in the same process. Mirrors __resetRegistry in + * token-registry.ts. shutdown() short-circuits when isShuttingDown is true + * (see line near the start of shutdown), so without this, tests that call + * shutdown() more than once silently no-op after the first call. + * + * DO NOT call from production code. Defeats the shutdown re-entry guard, + * which can race process.exit with cfgBrowserManager.close() and the pkill / + * safeUnlinkQuiet side effects. The `__` prefix is the convention; nothing + * enforces it. If you find yourself reaching for this outside a test file, + * the right fix is to make isShuttingDown factory-scoped instead. + */ +export function __resetShuttingDown(): void { + isShuttingDown = false; +} + // Auto-kickoff only when this module is the entry point. Embedders // (gbrowser phoenix overlay) import { start, buildFetchHandler, ... } // without triggering the listener-binding side effects. diff --git a/browse/test/server-embedder-terminal-port.test.ts b/browse/test/server-embedder-terminal-port.test.ts new file mode 100644 index 000000000..722a331d8 --- /dev/null +++ b/browse/test/server-embedder-terminal-port.test.ts @@ -0,0 +1,189 @@ +import { describe, test, expect, beforeEach, beforeAll, afterAll } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as crypto from 'crypto'; +import { + buildFetchHandler, + __resetShuttingDown, + type ServerConfig, +} from '../src/server'; +import { __resetRegistry } from '../src/token-registry'; +import { BrowserManager } from '../src/browser-manager'; +import { resolveConfig } from '../src/config'; + +// Tests for the v1.41+ ownsTerminalAgent flag. +// +// Embedders (gbrowser phoenix overlay) that run their own PTY server and write +// terminal-port / terminal-internal-token themselves were getting those files +// clobbered by gstack's shutdown(). The flag (default true) gates three side +// effects: pkill -f terminal-agent\.ts, unlink terminal-port, unlink +// terminal-internal-token. False = embedder owns them, gstack stays hands-off. +// +// CRITICAL: each test stubs BOTH process.exit (so shutdown's exit doesn't kill +// the test runner) AND child_process.spawnSync (so pkill doesn't run real +// `pkill -f terminal-agent\.ts` on the developer's machine — would kill any +// sibling gstack sessions). + +const stateDir = resolveConfig().stateDir; +const PORT_FILE = path.join(stateDir, 'terminal-port'); +const TOKEN_FILE = path.join(stateDir, 'terminal-internal-token'); +const SENTINEL_PORT = 'sentinel-port-65432'; +const SENTINEL_TOKEN = 'sentinel-token-abcdef1234567890'; + +function makeMinimalConfig(overrides: Partial = {}): ServerConfig { + const token = 'embedder-test-' + crypto.randomBytes(16).toString('hex'); + return { + authToken: token, + browsePort: 34568, + idleTimeoutMs: 1_800_000, + config: resolveConfig(), + browserManager: new BrowserManager(), + startTime: Date.now(), + ...overrides, + }; +} + +function writeSentinels(): void { + fs.mkdirSync(stateDir, { recursive: true }); + fs.writeFileSync(PORT_FILE, SENTINEL_PORT); + fs.writeFileSync(TOKEN_FILE, SENTINEL_TOKEN); +} + +function readIfExists(p: string): string | null { + try { return fs.readFileSync(p, 'utf-8'); } catch { return null; } +} + +/** + * Stubs process.exit + child_process.spawnSync, runs the callback, and + * restores both regardless of throw. Returns the captured spawnSync argv + * list so callers can assert pkill was or wasn't invoked. The callback + * is expected to swallow the __exit:N throw from shutdown(). + */ +async function withStubs( + cb: (spawnSyncCalls: any[][]) => Promise +): Promise { + const origExit = process.exit; + const childProcess = require('child_process'); + const origSpawnSync = childProcess.spawnSync; + const spawnSyncCalls: any[][] = []; + (process as any).exit = ((code: number) => { + throw new Error(`__exit:${code}`); + }) as any; + childProcess.spawnSync = ((...args: any[]) => { + spawnSyncCalls.push(args); + return { status: 0, stdout: '', stderr: '', signal: null, pid: 0, output: [] }; + }) as any; + try { + await cb(spawnSyncCalls); + } finally { + (process as any).exit = origExit; + childProcess.spawnSync = origSpawnSync; + } + return spawnSyncCalls; +} + +async function runShutdown(handle: { shutdown: (code?: number) => Promise }): Promise { + try { + await handle.shutdown(0); + } catch (err: any) { + if (typeof err?.message !== 'string' || !err.message.startsWith('__exit:')) throw err; + } +} + +function pkillCalls(calls: any[][]): any[][] { + return calls.filter((call) => call[0] === 'pkill'); +} + +describe('buildFetchHandler ownsTerminalAgent gate', () => { + // shutdown() reads `path.dirname(config.stateFile)` from module-level config + // (composition gap — see TODOS T9). So unlinks target the real state dir, + // not a per-test temp dir. If a real gstack daemon is running on this host, + // its terminal-port + terminal-internal-token live where this test writes. + // Save + restore real-daemon file contents around the whole suite so the + // test never clobbers a developer's running session. + let realPortBackup: string | null = null; + let realTokenBackup: string | null = null; + + beforeAll(() => { + realPortBackup = readIfExists(PORT_FILE); + realTokenBackup = readIfExists(TOKEN_FILE); + }); + + afterAll(() => { + if (realPortBackup !== null) { + fs.mkdirSync(stateDir, { recursive: true }); + fs.writeFileSync(PORT_FILE, realPortBackup); + } else { + try { fs.unlinkSync(PORT_FILE); } catch {} + } + if (realTokenBackup !== null) { + fs.mkdirSync(stateDir, { recursive: true }); + fs.writeFileSync(TOKEN_FILE, realTokenBackup); + } else { + try { fs.unlinkSync(TOKEN_FILE); } catch {} + } + }); + + beforeEach(() => { + __resetRegistry(); + __resetShuttingDown(); + // Clean any leftover sentinels from a prior failed run so the "preserved" + // assertion can't pass spuriously off a stale file. + try { fs.unlinkSync(PORT_FILE); } catch {} + try { fs.unlinkSync(TOKEN_FILE); } catch {} + }); + + test('1. ownsTerminalAgent:false preserves both files and skips pkill', async () => { + writeSentinels(); + const handle = buildFetchHandler(makeMinimalConfig({ ownsTerminalAgent: false })); + const calls = await withStubs(async () => { + await runShutdown(handle); + }); + expect(readIfExists(PORT_FILE)).toBe(SENTINEL_PORT); + expect(readIfExists(TOKEN_FILE)).toBe(SENTINEL_TOKEN); + expect(pkillCalls(calls).length).toBe(0); + }); + + test('2. ownsTerminalAgent:true (explicit) deletes both files and invokes pkill exactly once', async () => { + writeSentinels(); + const handle = buildFetchHandler(makeMinimalConfig({ ownsTerminalAgent: true })); + const calls = await withStubs(async () => { + await runShutdown(handle); + }); + expect(readIfExists(PORT_FILE)).toBeNull(); + expect(readIfExists(TOKEN_FILE)).toBeNull(); + const pkills = pkillCalls(calls); + expect(pkills.length).toBe(1); + // argv[1] is the args array passed to spawnSync. + expect(pkills[0][1]).toEqual(['-f', 'terminal-agent\\.ts']); + }); + + test('3. ownsTerminalAgent unset defaults to true (deletes + pkill)', async () => { + writeSentinels(); + // Note: no ownsTerminalAgent in the overrides — uses the `?? true` default. + const handle = buildFetchHandler(makeMinimalConfig()); + const calls = await withStubs(async () => { + await runShutdown(handle); + }); + expect(readIfExists(PORT_FILE)).toBeNull(); + expect(readIfExists(TOKEN_FILE)).toBeNull(); + expect(pkillCalls(calls).length).toBe(1); + }); + + test('4. CLI start() call site passes ownsTerminalAgent: true literally (static grep)', () => { + // Resolves browse/src/server.ts relative to this test file so the test + // works regardless of cwd. import.meta.url is the test file's URL. + const serverTsPath = path.resolve( + new URL(import.meta.url).pathname, + '..', + '..', + 'src', + 'server.ts', + ); + const source = fs.readFileSync(serverTsPath, 'utf-8'); + // Match the call site inside start()'s buildFetchHandler({...}) literal. + // The pattern looks for the trailing comma and trailing context so the + // match cannot be satisfied by the JSDoc reference earlier in the file. + expect(source).toMatch(/ownsTerminalAgent:\s*true,\s*\/\/\s*CLI spawns terminal-agent\.ts/); + }); +});