diff --git a/browse/src/cli.ts b/browse/src/cli.ts index ab90dd918..cfe8c5ee5 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -270,15 +270,62 @@ async function killOrphanChromium(profileDir: string = chromiumProfileDir()): Pr } } -/** Bounded /health probe. Returns true if the server answers within `attempts` - * tries spaced `backoffMs` apart — distinguishes a busy-but-alive daemon from a - * dead one (#1781) so a slow server isn't killed and restarted into a crash-loop. */ -async function probeHealthWithBackoff(port: number, attempts = 3, backoffMs = 250): Promise { - for (let i = 0; i < attempts; i++) { +/** Total wall-clock budget for the busy-vs-dead health probe (#2219, + * decision F10). The old ~1s window (3 × 250ms) was shorter than how long a + * daemon stays unresponsive while Chromium chews a heavy dev-mode page with a + * timed-out navigation still in flight — so live daemons got killed and every + * kill lost the session's cookies/tabs/logins. ~8s covers the observed busy + * windows; past it we REPORT busy instead of killing (never auto-kill). */ +export const HEALTH_PROBE_TOTAL_BUDGET_MS = 8_000; + +/** Bounded /health probe. Returns true if the server answers within the + * total budget — distinguishes a busy-but-alive daemon from a dead one + * (#1781, #2219) so a slow server isn't killed and restarted into a + * crash-loop. Each individual probe self-bounds at 2s (isServerHealthy). */ +async function probeHealthWithBackoff( + port: number, + totalBudgetMs = HEALTH_PROBE_TOTAL_BUDGET_MS, + intervalMs = 500, +): Promise { + const deadline = Date.now() + totalBudgetMs; + for (;;) { if (await isServerHealthy(port)) return true; - if (i < attempts - 1) await Bun.sleep(backoffMs); + if (Date.now() + intervalMs >= deadline) return false; + await Bun.sleep(intervalMs); } - return false; +} + +export type DaemonRestartAction = + | 'retry-command' // healthy again after the bounded probe — retry against the SAME daemon + | 'report-busy' // alive but unresponsive — report + nonzero exit, daemon untouched + | 'force-restart' // alive but the user explicitly passed --force-restart + | 'restart-dead'; // process is gone — safe to clean up and restart + +/** + * Decide what to do about a daemon that failed to answer (#2219, decision 9). + * + * IRON RULE: an alive pid is NEVER auto-killed. A kill loses the session's + * tabs, cookies, and logins — strictly worse than a slow command. The ONLY + * path that kills a live daemon is the user explicitly passing + * --force-restart. Pure and exported for unit coverage. + */ +export function decideDaemonRestart(opts: { + pidAlive: boolean; + healthyAfterProbe: boolean; + forceRestart: boolean; +}): DaemonRestartAction { + if (opts.pidAlive && opts.healthyAfterProbe) return 'retry-command'; + if (opts.pidAlive && opts.forceRestart) return 'force-restart'; + if (opts.pidAlive) return 'report-busy'; + return 'restart-dead'; +} + +/** The busy report (F10): what happened, what to do, what a force costs. */ +function reportDaemonBusyAndExit(pid: number): never { + console.error(`[browse] Daemon busy — process ${pid} is alive but did not answer /health within ~${HEALTH_PROBE_TOTAL_BUDGET_MS / 1000}s.`); + console.error('[browse] Retry shortly (heavy page loads pass), or force a restart — which LOSES tabs, cookies, and logins:'); + console.error('[browse] browse --force-restart '); + process.exit(1); } /** @@ -483,7 +530,12 @@ async function ensureServer(flags?: GlobalFlags): Promise { // Health-check-first: HTTP is definitive proof the server is alive and responsive. // This replaces the PID-gated approach which breaks on Windows (Bun's process.kill // always throws ESRCH for Windows PIDs in compiled binaries). - if (state && await isServerHealthy(state.port)) { + // + // #2219: when the single 2s probe fails but the PID is alive, extend to the + // bounded ~8s probe before concluding anything — a daemon chewing a heavy + // page is busy, not dead, and killing it loses the session. + const daemonPidAlive = Boolean(state?.pid && isProcessAlive(state.pid)); + if (state && (await isServerHealthy(state.port) || (daemonPidAlive && await probeHealthWithBackoff(state.port)))) { // D2 daemon-mismatch check: existing daemon's configHash must match the // CLI's resolved hash. If --proxy or --headed are passed and the existing // daemon was started with different config, refuse with a `disconnect` @@ -531,6 +583,18 @@ async function ensureServer(flags?: GlobalFlags): Promise { process.exit(1); } + // #2219 IRON RULE: never auto-kill an alive pid. The daemon didn't answer + // /health within the bounded ~8s budget but its process is alive — that's + // busy, not dead. Report + nonzero exit; only an explicit --force-restart + // proceeds to the kill-and-restart below. + if (state && daemonPidAlive) { + if (flags?.forceRestart) { + console.error('[browse] --force-restart: replacing live-but-unresponsive daemon (tabs/cookies/logins will be lost)...'); + } else { + reportDaemonBusyAndExit(state.pid); + } + } + // Ensure state directory exists before lock acquisition (lock file lives there) ensureStateDir(config); @@ -657,18 +721,34 @@ async function sendCommand(state: ServerState, command: string, args: string[], // Connection error — server may have crashed, OR may just be busy. if (err.code === 'ECONNREFUSED' || err.code === 'ECONNRESET' || err.message?.includes('fetch failed')) { 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 - // crash, if the process is alive give /health a bounded chance to recover - // and just retry the command — never kill+restart a live-but-busy server. - if (oldState?.pid && isProcessAlive(oldState.pid) && await probeHealthWithBackoff(oldState.port)) { + // #1781/#2219 busy-vs-dead: a single-threaded daemon under beacon/ + // extension load (or with a timed-out navigation still churning) can + // stop answering HTTP for seconds while fully alive. Give /health a + // bounded ~8s to recover, then decide via the pure rule: retry against + // the same daemon, report busy (NEVER kill an alive pid), or restart a + // genuinely dead one. Only --force-restart may kill a live daemon. + const pidAlive = Boolean(oldState?.pid && isProcessAlive(oldState.pid)); + const healthyAfterProbe = pidAlive ? await probeHealthWithBackoff(oldState!.port) : false; + const action = decideDaemonRestart({ + pidAlive, + healthyAfterProbe, + forceRestart: Boolean(_globalFlags?.forceRestart), + }); + if (action === 'retry-command') { if (retries >= 1) throw new Error('[browse] Server unresponsive after retry — aborting'); console.error('[browse] Server was briefly unresponsive (busy); retrying command...'); - return sendCommand(oldState, command, args, retries + 1); + return sendCommand(oldState!, command, args, retries + 1); } - // Truly dead (or health never recovered) → restart. + if (action === 'report-busy') { + reportDaemonBusyAndExit(oldState!.pid); + } + // 'restart-dead' or explicit 'force-restart' → restart. if (retries >= 1) throw new Error('[browse] Server crashed twice in a row — aborting'); - console.error('[browse] Server connection lost. Restarting...'); + if (action === 'force-restart') { + console.error('[browse] --force-restart: killing live daemon and restarting (tabs/cookies/logins will be lost)...'); + } else { + console.error('[browse] Server connection lost. Restarting...'); + } if (oldState && oldState.pid) { await killServer(oldState.pid); } @@ -845,6 +925,9 @@ export interface GlobalFlags { configHash: string; /** Redacted form of proxyUrl, safe for logs. */ redactedProxyUrl: string; + /** Whether --force-restart was passed (#2219): the ONLY thing that may + * kill a live-but-unresponsive daemon. */ + forceRestart: boolean; } /** @@ -858,9 +941,11 @@ export function extractGlobalFlags(rawArgs: string[], env: NodeJS.ProcessEnv): G const out: string[] = []; let proxyUrl: string | null = null; let headed = false; + let forceRestart = false; for (let i = 0; i < rawArgs.length; i++) { const arg = rawArgs[i]; + if (arg === '--force-restart') { forceRestart = true; continue; } if (arg === '--proxy') { const value = rawArgs[i + 1]; if (!value) { @@ -903,6 +988,7 @@ export function extractGlobalFlags(rawArgs: string[], env: NodeJS.ProcessEnv): G headed, configHash: computeConfigHash({ proxyUrl: canonicalProxyUrl, headed }), redactedProxyUrl: redactProxyUrl(canonicalProxyUrl), + forceRestart, }; } @@ -1108,6 +1194,8 @@ Multi-step: chain (reads JSON from stdin) Tabs: tabs | tab | newtab [url] | closetab [id] Server: status | cookie = | header : useragent | stop | restart + --force-restart: replace a live-but-busy daemon (any command; + LOSES tabs/cookies/logins — never done automatically) Dialogs: dialog-accept [text] | dialog-dismiss Refs: After 'snapshot', use @e1, @e2... as selectors: @@ -1138,12 +1226,35 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: process.exit(0); } } catch { - // Headed server alive but not responding — kill and restart + // Headed server alive but not responding — handled below (#2219: + // busy semantics; only --force-restart may kill it). } } - // Kill ANY existing server (SIGTERM → wait 2s → SIGKILL) + // #2219 IRON RULE: a HEALTHY daemon survives connect. The old behavior + // ("kill ANY existing server") silently destroyed a working headless + // session — tabs, cookies, logins — whenever someone opened the headed + // browser. A live daemon is only replaced with explicit consent. + if (existingState && isProcessAlive(existingState.pid) && !globalFlags.forceRestart) { + if (await isServerHealthy(existingState.port)) { + console.error(`[browse] A healthy daemon is already running (PID ${existingState.pid}, ${existingState.mode} mode).`); + console.error('[browse] Connecting headed would kill it and lose its tabs/cookies/logins.'); + console.error("[browse] Run 'browse disconnect' first, or pass --force-restart to replace it."); + process.exit(1); + } + // Alive but unhealthy after the bounded probe → busy, not dead. + if (await probeHealthWithBackoff(existingState.port)) { + console.error(`[browse] A healthy daemon is already running (PID ${existingState.pid}, ${existingState.mode} mode).`); + console.error("[browse] Run 'browse disconnect' first, or pass --force-restart to replace it."); + process.exit(1); + } + reportDaemonBusyAndExit(existingState.pid); + } + + // Explicit --force-restart (or a dead pid): kill any remnant + // (SIGTERM → wait 2s → SIGKILL). if (existingState && isProcessAlive(existingState.pid)) { + console.error('[browse] --force-restart: replacing live daemon (tabs/cookies/logins will be lost)...'); safeKill(existingState.pid, 'SIGTERM'); await new Promise(resolve => setTimeout(resolve, 2000)); if (isProcessAlive(existingState.pid)) { @@ -1404,7 +1515,10 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: // In compiled binaries, process.argv[1] is /$bunfs/... (virtual). // Use process.execPath which is the real binary on disk. const browseBin = process.execPath; - const connectProc = Bun.spawn([browseBin, 'connect'], { + // --force-restart: the headed switch is this command's explicit purpose + // (the user asked to SEE the shared browser), and connect's #2219 guard + // would otherwise refuse to replace the healthy headless daemon. + const connectProc = Bun.spawn([browseBin, 'connect', '--force-restart'], { cwd: process.cwd(), stdio: ['ignore', 'inherit', 'inherit'], // Disable parent-PID monitoring: pair-agent needs the server to outlive diff --git a/browse/test/busy-daemon-iron-rule.test.ts b/browse/test/busy-daemon-iron-rule.test.ts new file mode 100644 index 000000000..e34acbac8 --- /dev/null +++ b/browse/test/busy-daemon-iron-rule.test.ts @@ -0,0 +1,226 @@ +/** + * #2219 IRON RULE regression tests (E5): an alive daemon pid is NEVER + * auto-killed. Killing a live daemon loses the session's tabs, cookies, and + * logins — strictly worse than a slow command. Only an explicit + * --force-restart may replace a live daemon. + * + * Integration legs follow the busy-daemon-recovery.test.ts pattern: a fake + * HTTP daemon + a live `sleep` child standing in for the daemon PID, wired + * through BROWSE_STATE_FILE. Unit legs pin the pure decision function. + */ + +import { describe, test, expect, afterEach } from 'bun:test'; +import { spawn, type ChildProcess } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import * as http from 'http'; +import { isProcessAlive } from '../src/error-handling'; +import { decideDaemonRestart, HEALTH_PROBE_TOTAL_BUDGET_MS } from '../src/cli'; + +// ─── Unit: the pure restart decision (decision 9 / F10) ────────────────── + +describe('decideDaemonRestart (pure)', () => { + test('healthy after probe → retry against the SAME daemon', () => { + expect(decideDaemonRestart({ pidAlive: true, healthyAfterProbe: true, forceRestart: false })) + .toBe('retry-command'); + // Even with --force-restart in hand, a healthy daemon is retried, not killed. + expect(decideDaemonRestart({ pidAlive: true, healthyAfterProbe: true, forceRestart: true })) + .toBe('retry-command'); + }); + + test('IRON RULE: alive + unhealthy + no flag → report busy, never kill', () => { + expect(decideDaemonRestart({ pidAlive: true, healthyAfterProbe: false, forceRestart: false })) + .toBe('report-busy'); + }); + + test('alive + unhealthy + explicit --force-restart → force-restart', () => { + expect(decideDaemonRestart({ pidAlive: true, healthyAfterProbe: false, forceRestart: true })) + .toBe('force-restart'); + }); + + test('dead pid → restart, with or without the flag', () => { + expect(decideDaemonRestart({ pidAlive: false, healthyAfterProbe: false, forceRestart: false })) + .toBe('restart-dead'); + expect(decideDaemonRestart({ pidAlive: false, healthyAfterProbe: false, forceRestart: true })) + .toBe('restart-dead'); + }); + + test('probe budget is ~8s (F10) — long enough for heavy-page busy windows', () => { + expect(HEALTH_PROBE_TOTAL_BUDGET_MS).toBeGreaterThanOrEqual(7_000); + expect(HEALTH_PROBE_TOTAL_BUDGET_MS).toBeLessThanOrEqual(10_000); + }); +}); + +// ─── Integration: real spawned CLI vs fake daemons ─────────────────────── + +/** A daemon whose /health always answers healthy but never serves /command. */ +async function startHealthyDaemon(): Promise<{ port: number; close: () => Promise }> { + const server = http.createServer((req, res) => { + if (req.url === '/health') { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ status: 'healthy' })); + return; + } + res.writeHead(200, { 'Content-Type': 'text/plain' }); + res.end('ok'); + }); + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(0, '127.0.0.1', () => resolve()); + }); + const addr = server.address(); + if (!addr || typeof addr === 'string') throw new Error('fake daemon: bad address'); + return { port: addr.port, close: () => new Promise((r) => server.close(() => r())) }; +} + +/** A WEDGED daemon: alive socket, but /health always answers unhealthy. */ +async function startWedgedDaemon(): Promise<{ port: number; close: () => Promise }> { + const server = http.createServer((req, res) => { + res.writeHead(503, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ status: 'wedged' })); + }); + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(0, '127.0.0.1', () => resolve()); + }); + const addr = server.address(); + if (!addr || typeof addr === 'string') throw new Error('fake daemon: bad address'); + return { port: addr.port, close: () => new Promise((r) => server.close(() => r())) }; +} + +function runCli(args: string[], env: Record, timeoutMs = 30_000): + Promise<{ code: number; stdout: string; stderr: string }> { + const cliPath = path.resolve(import.meta.dir, '../src/cli.ts'); + return new Promise((resolve) => { + const proc = spawn('bun', ['run', cliPath, ...args], { timeout: timeoutMs, env }); + let stdout = ''; let stderr = ''; + proc.stdout.on('data', (d) => stdout += d.toString()); + proc.stderr.on('data', (d) => stderr += d.toString()); + proc.on('close', (code) => resolve({ code: code ?? 1, stdout, stderr })); + }); +} + +function baseEnv(stateFile: string): Record { + const env: Record = {}; + for (const [k, v] of Object.entries(process.env)) { + if (v !== undefined) env[k] = v; + } + env.BROWSE_STATE_FILE = stateFile; + return env; +} + +let pidChild: ChildProcess | null = null; +afterEach(() => { pidChild?.kill('SIGKILL'); pidChild = null; }); + +describe('#2219 iron rule (CLI integration)', () => { + test('healthy daemon SURVIVES `browse connect` — refused with guidance, no kill', async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-iron-')); + const stateFile = path.join(tmpDir, 'browse.json'); + const daemon = await startHealthyDaemon(); + try { + pidChild = spawn('sleep', ['60'], { stdio: 'ignore' }); + const daemonPid = pidChild.pid!; + const stateContent = { + pid: daemonPid, + port: daemon.port, + token: 'iron-rule-token', + startedAt: new Date().toISOString(), + serverPath: '', + mode: 'launched' as const, + }; + fs.writeFileSync(stateFile, JSON.stringify(stateContent, null, 2)); + + const result = await runCli(['connect'], baseEnv(stateFile)); + + expect(result.code).not.toBe(0); + expect(result.stderr).toContain('healthy daemon is already running'); + expect(result.stderr).toContain('--force-restart'); + // THE IRON RULE: the daemon process was not killed. + expect(isProcessAlive(daemonPid)).toBe(true); + // And the state file was not clobbered. + expect(JSON.parse(fs.readFileSync(stateFile, 'utf-8'))).toEqual(stateContent); + } finally { + await daemon.close(); + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 30_000); + + test('wedged-alive daemon + plain command → busy report + nonzero exit, NO kill', async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-iron-')); + const stateFile = path.join(tmpDir, 'browse.json'); + const daemon = await startWedgedDaemon(); + try { + pidChild = spawn('sleep', ['120'], { stdio: 'ignore' }); + const daemonPid = pidChild.pid!; + fs.writeFileSync(stateFile, JSON.stringify({ + pid: daemonPid, + port: daemon.port, + token: 'iron-rule-token', + startedAt: new Date().toISOString(), + serverPath: '', + mode: 'launched' as const, + }, null, 2)); + + const result = await runCli(['status'], baseEnv(stateFile)); + + expect(result.code).not.toBe(0); + expect(result.stderr).toContain('Daemon busy'); + expect(result.stderr).toContain('--force-restart'); + // Never killed, never restarted. + expect(isProcessAlive(daemonPid)).toBe(true); + expect(result.stderr).not.toContain('Restarting'); + } finally { + await daemon.close(); + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 45_000); + + test('wedged-alive daemon + --force-restart IS killed (explicit consent path)', async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-iron-')); + const stateFile = path.join(tmpDir, 'browse.json'); + const daemon = await startWedgedDaemon(); + try { + pidChild = spawn('sleep', ['120'], { stdio: 'ignore' }); + const daemonPid = pidChild.pid!; + fs.writeFileSync(stateFile, JSON.stringify({ + pid: daemonPid, + port: daemon.port, + token: 'iron-rule-token', + startedAt: new Date().toISOString(), + serverPath: '', + mode: 'launched' as const, + }, null, 2)); + + // `status` with --force-restart: the wedged "daemon" must be killed and + // a REAL daemon started in its place. + const result = await runCli(['--force-restart', 'status'], baseEnv(stateFile), 60_000); + + // The wedged pid was killed — the explicit consent path. + expect(isProcessAlive(daemonPid)).toBe(false); + expect(result.stderr).toContain('--force-restart'); + // The replacement daemon answered the command. + expect(result.code).toBe(0); + } finally { + // Kill the REAL daemon's whole PROCESS GROUP, not just its pid. + // startServer spawns the daemon detached (setsid — its own group + // leader), so a bare SIGKILL on the pid orphans its Chromium child, + // which then squats memory for the REST of the suite (~100 files) — + // enough pressure on a loaded box for the OS to kill a LATER test's + // in-process Chromium, whose disconnect handler process.exit(1)s the + // whole bun run mid-suite with no summary. + try { + const newState = JSON.parse(fs.readFileSync(stateFile, 'utf-8')); + if (newState?.pid && isProcessAlive(newState.pid)) { + try { + process.kill(-newState.pid, 'SIGKILL'); // group: daemon + Chromium + } catch { + process.kill(newState.pid, 'SIGKILL'); // fallback: pid only + } + } + } catch { /* state file gone — nothing started */ } + await daemon.close(); + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 90_000); +});