mirror of https://github.com/garrytan/gstack.git
fix(server): delete ServerConfig.idleTimeoutMs + chromiumProfile — documented, never read
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 <noreply@anthropic.com>
This commit is contained in:
parent
fc4cb6aaa9
commit
5099637f95
|
|
@ -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. <resolveGstackHome()>/chromium-profile (default)
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -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<typeof resolveConfig>;
|
||||
/** 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, 'browserManager' | 's
|
|||
// embedder can't ship a BOM/zero-width as the bearer secret.
|
||||
authToken: sanitizeAuthToken(process.env.AUTH_TOKEN) || crypto.randomUUID(),
|
||||
browsePort: parseInt(process.env.BROWSE_PORT || '0', 10),
|
||||
idleTimeoutMs: parseInt(process.env.BROWSE_IDLE_TIMEOUT || '1800000', 10),
|
||||
config: resolveConfig(),
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -49,7 +49,6 @@ function makeMinimalConfig(overrides: Partial<ServerConfig> = {}): ServerConfig
|
|||
return {
|
||||
authToken: token,
|
||||
browsePort: 34568,
|
||||
idleTimeoutMs: 1_800_000,
|
||||
config: resolveConfig(),
|
||||
browserManager: new BrowserManager(),
|
||||
startTime: Date.now(),
|
||||
|
|
|
|||
|
|
@ -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> = {}): ServerConfig
|
|||
return {
|
||||
authToken: token,
|
||||
browsePort: 34567,
|
||||
idleTimeoutMs: 1_800_000,
|
||||
config: resolveConfig(),
|
||||
browserManager: new BrowserManager(),
|
||||
startTime: Date.now(),
|
||||
|
|
|
|||
Loading…
Reference in New Issue