From a6c94c7feb365212558ad5aee7e886dba7f36b2e Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 14 Aug 2026 19:35:38 -0700 Subject: [PATCH] =?UTF-8?q?fix(browse):=20daemon=20resilience=20on=20loade?= =?UTF-8?q?d=20machines=20=E2=80=94=20Bun=20conn=20errors,=20stop/restart?= =?UTF-8?q?=20flush,=20startup=20+=20git-root=20budgets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four load-sensitivity fixes in the daemon lifecycle: - sendCommand only recognized Node's ECONNREFUSED/ECONNRESET; the compiled CLI runs on Bun, which reports 'ConnectionRefused'/'ConnectionClosed' ("Unable to connect..."), so daemon crashes leaked the raw error and exited 1 instead of entering the busy-check/restart path. Match both. - stop/restart called shutdown() inline, which exits before the HTTP response flushes — the CLI saw a dropped socket (and would now crash-retry a fresh daemon just to stop it). Defer shutdown ~100ms so the 200 lands first. - Non-CI POSIX startup budget raised 8s -> 15s (cold Chromium measured ~5.7s at load avg 10; load 12+ blew the old budget while the detached daemon was still booting). - getGitRoot's 2s git rev-parse timeout returned null under load (6.3s spikes measured), scattering state files across cwds into split-brain daemons. Raise to 8s, still bounded. Contributed by @mplatts (PR #1732). Co-Authored-By: Claude Fable 5 --- browse/src/cli.ts | 20 ++++++++++++++++++-- browse/src/config.ts | 7 ++++++- browse/src/meta-commands.ts | 18 ++++++++++++++---- browse/test/commands.test.ts | 5 ++++- 4 files changed, 42 insertions(+), 8 deletions(-) diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 4177eed3d..3c00f0fc2 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -36,7 +36,13 @@ const IS_WINDOWS = process.platform === 'win32'; * falls back to the platform default. Pure + exported for tests. */ export function resolveStartTimeout(env: NodeJS.ProcessEnv = process.env): number { - const platformDefault = IS_WINDOWS ? 15000 : (env.CI ? 30000 : 8000); // Node+Chromium takes longer on Windows + // Cold Chromium launch measured ~5.7s at load avg 10 on a dev machine running + // many servers; at load 12+ it exceeds the old 8s budget, so the CLI gave up + // while the (detached) daemon was still booting → "Server failed to start + // within 8s". 15s matches the Windows budget and gives real headroom; the poll + // loop returns the instant the daemon is healthy, so this only costs time in a + // genuine-failure case. + const platformDefault = IS_WINDOWS ? 15000 : (env.CI ? 30000 : 15000); // Node+Chromium takes longer on Windows const override = parseInt(env.BROWSE_START_TIMEOUT || '', 10); return Number.isFinite(override) && override > 0 ? override : platformDefault; } @@ -614,7 +620,17 @@ async function sendCommand(state: ServerState, command: string, args: string[], process.exit(1); } // Connection error — server may have crashed, OR may just be busy. - if (err.code === 'ECONNREFUSED' || err.code === 'ECONNRESET' || err.message?.includes('fetch failed')) { + // The compiled CLI runs on Bun, whose fetch reports a refused/dropped + // socket as err.code 'ConnectionRefused' / 'ConnectionClosed' (message + // "Unable to connect. Is the computer able to access the url?"), NOT Node's + // ECONNREFUSED/ECONNRESET. Match both, or daemon crashes leak the raw Bun + // error and exit 1 instead of triggering the busy-check/restart below. + const isConnError = + err.code === 'ECONNREFUSED' || err.code === 'ECONNRESET' || + err.code === 'ConnectionRefused' || err.code === 'ConnectionClosed' || + err.message?.includes('fetch failed') || + err.message?.includes('Unable to connect'); + if (isConnError) { const oldState = readState(); // #1781 busy-vs-dead: a single-threaded daemon under beacon/extension load // can briefly stop answering HTTP while still alive. Before declaring a diff --git a/browse/src/config.ts b/browse/src/config.ts index f348239de..da9ece4bf 100644 --- a/browse/src/config.ts +++ b/browse/src/config.ts @@ -34,7 +34,12 @@ export function getGitRoot(): string | null { const proc = Bun.spawnSync(['git', 'rev-parse', '--show-toplevel'], { stdout: 'pipe', stderr: 'pipe', - timeout: 2_000, // Don't hang if .git is broken + // Raised from 2s: under heavy machine load `git rev-parse` routinely + // takes >2s (measured 6.3s spikes). Timing out here returns null → + // resolveConfig falls back to process.cwd() → state files scatter across + // cwds (split-brain daemons; `goto` and `url` hit different servers). 8s + // still bounds a genuinely broken .git from hanging the CLI forever. + timeout: 8_000, }); if (proc.exitCode !== 0) return null; return proc.stdout.toString().trim() || null; diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index 4bd0faae7..2810b0cf6 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -421,15 +421,25 @@ export async function handleMetaCommand( } case 'stop': { - await shutdown(); + // Defer shutdown so the response flushes before process.exit() (same + // reason as 'restart' below). Otherwise the CLI sees a dropped socket; + // and now that connection-loss triggers the crash-retry path, that would + // resurrect a fresh daemon only to stop it again. Send the 200, then exit. + setTimeout(() => { void shutdown(); }, 100); return 'Server stopped'; } case 'restart': { - // Signal that we want a restart — the CLI will detect exit and restart + // Signal that we want a restart — the CLI will detect exit and restart. console.log('[browse] Restart requested. Exiting for CLI to restart.'); - await shutdown(); - return 'Restarting...'; + // Defer shutdown one tick so this HTTP response actually flushes before + // process.exit(). shutdown() exits inline (server.ts), so the old + // `await shutdown(); return 'Restarting...'` never sent a response — the + // CLI saw a dropped socket and `browse restart` errored out. The daemon + // now exits ~100ms after the CLI gets its 200; the next browse command + // lazily cold-starts a fresh one. + setTimeout(() => { void shutdown(); }, 100); + return 'Restarting... (daemon exiting; next browse command starts a fresh one)'; } // ─── Visual ──────────────────────────────────────── diff --git a/browse/test/commands.test.ts b/browse/test/commands.test.ts index 506885b56..b47914dc2 100644 --- a/browse/test/commands.test.ts +++ b/browse/test/commands.test.ts @@ -884,7 +884,10 @@ describe('CLI lifecycle', () => { cliEnv.BROWSE_STATE_FILE = stateFile; const result = await new Promise<{ code: number; stdout: string; stderr: string }>((resolve) => { const proc = spawn('bun', ['run', cliPath, 'status'], { - timeout: 15000, + // Must exceed the CLI's startup budget (resolveStartTimeout, 15s + // non-CI POSIX) or a slow cold boot under full-suite load gets the + // child killed at the exact moment the CLI would have succeeded. + timeout: 18000, env: cliEnv, }); let stdout = '';