mirror of https://github.com/garrytan/gstack.git
feat: gate terminal-agent teardown on ServerConfig.ownsTerminalAgent
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.
This commit is contained in:
parent
026751ea20
commit
94e15c5778
|
|
@ -204,6 +204,35 @@ export interface ServerConfig {
|
||||||
* dispatch; returning null falls through.
|
* dispatch; returning null falls through.
|
||||||
*/
|
*/
|
||||||
beforeRoute?: (req: Request, surface: Surface, auth: TokenInfo | null) => Promise<Response | null>;
|
beforeRoute?: (req: Request, surface: Surface, auth: TokenInfo | null) => Promise<Response | null>;
|
||||||
|
/**
|
||||||
|
* Whether gstack owns the lifecycle of the terminal-agent process and its
|
||||||
|
* discovery files (`<stateDir>/terminal-port`, `<stateDir>/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(<stateDir>/terminal-port)`
|
||||||
|
* 3. `safeUnlinkQuiet(<stateDir>/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
|
* Build a request handler set for the browse daemon. Embedders (gbrowser
|
||||||
* phoenix overlay) call this directly with their own cfg to compose overlay
|
* phoenix overlay) call this directly with their own cfg to compose overlay
|
||||||
* routes via cfg.beforeRoute. The CLI path calls it through start() with
|
* routes via cfg.beforeRoute, pass a pre-launched cfg.browserManager, and
|
||||||
* env-derived defaults — externally-observable behavior is identical.
|
* 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
|
* Auth state lives ENTIRELY inside the factory closure: cfg.authToken is the
|
||||||
* single source of truth for the bearer secret, factory-scoped validateAuth
|
* single source of truth for the bearer secret, factory-scoped validateAuth
|
||||||
|
|
@ -1259,6 +1291,11 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle {
|
||||||
initRegistry(cfg.authToken);
|
initRegistry(cfg.authToken);
|
||||||
|
|
||||||
const { authToken, browserManager: cfgBrowserManager, startTime, beforeRoute, browsePort } = cfg;
|
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
|
// Factory-scoped validateAuth. Closes over cfg.authToken so every internal
|
||||||
// auth check sees the same token the routes receive. Module-level
|
// auth check sees the same token the routes receive. Module-level
|
||||||
|
|
@ -1276,14 +1313,16 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle {
|
||||||
isShuttingDown = true;
|
isShuttingDown = true;
|
||||||
|
|
||||||
console.log('[browse] Shutting down...');
|
console.log('[browse] Shutting down...');
|
||||||
try {
|
if (ownsTerminalAgent) {
|
||||||
const { spawnSync } = require('child_process');
|
try {
|
||||||
spawnSync('pkill', ['-f', 'terminal-agent\\.ts'], { stdio: 'ignore', timeout: 3000 });
|
const { spawnSync } = require('child_process');
|
||||||
} catch (err: any) {
|
spawnSync('pkill', ['-f', 'terminal-agent\\.ts'], { stdio: 'ignore', timeout: 3000 });
|
||||||
console.warn('[browse] Failed to kill terminal-agent:', err.message);
|
} 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) {
|
try { detachSession(); } catch (err: any) {
|
||||||
console.warn('[browse] Failed to detach CDP session:', err.message);
|
console.warn('[browse] Failed to detach CDP session:', err.message);
|
||||||
}
|
}
|
||||||
|
|
@ -2428,6 +2467,7 @@ export async function start() {
|
||||||
xvfb,
|
xvfb,
|
||||||
proxyBridge,
|
proxyBridge,
|
||||||
startTime,
|
startTime,
|
||||||
|
ownsTerminalAgent: true, // CLI spawns terminal-agent.ts itself (see cli.ts:1037-1063)
|
||||||
});
|
});
|
||||||
|
|
||||||
const server = Bun.serve({
|
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
|
// Auto-kickoff only when this module is the entry point. Embedders
|
||||||
// (gbrowser phoenix overlay) import { start, buildFetchHandler, ... }
|
// (gbrowser phoenix overlay) import { start, buildFetchHandler, ... }
|
||||||
// without triggering the listener-binding side effects.
|
// without triggering the listener-binding side effects.
|
||||||
|
|
|
||||||
|
|
@ -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> = {}): 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<void>
|
||||||
|
): Promise<any[][]> {
|
||||||
|
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<void> }): Promise<void> {
|
||||||
|
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/);
|
||||||
|
});
|
||||||
|
});
|
||||||
Loading…
Reference in New Issue