From 5a6dd6bf88c86b32c526f30718be77780a18ec41 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 16 Aug 2026 09:21:29 -0700 Subject: [PATCH] =?UTF-8?q?fix(browse):=20`stop`=20on=20a=20dead=20daemon?= =?UTF-8?q?=20is=20success=20=E2=80=94=20never=20boots=20a=20daemon=20to?= =?UTF-8?q?=20stop=20it=20(#2254)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes, one contract: - Pre-server short-circuit: `browse stop` is handled BEFORE ensureServer(). No daemon state → "nothing to stop", exit 0. Stale state (dead pid AND dead port) → clean the state file, exit 0. The old flow routed stop through ensureServer(), which started a fresh daemon + Chromium (multi-second boot, resource churn) purely so it could be told to shut down — or crashed on the stale state. - Reconnect branch: a connection error while sending `stop` where the pid turns out dead (daemon died mid-flight, between the short-circuit check and the send) is treated as SUCCESS — the desired end state (no daemon) already holds — instead of the crash-restart path. Integration tests (stop-dead-daemon.test.ts, real spawned CLI + scratch BROWSE_STATE_FILE): stop with no state exits 0 and spawns nothing (a spawned daemon would have written the state file); stop with a stale state file (dead pid + verified-closed port) exits 0, cleans the state, and spawns nothing. Tests: stop-dead-daemon 2 pass; busy-daemon-iron-rule 8 pass; busy-daemon-recovery 1 pass (11/11 combined). Fixes #2254. Co-Authored-By: Claude Fable 5 --- browse/src/cli.ts | 28 ++++++++ browse/test/stop-dead-daemon.test.ts | 99 ++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 browse/test/stop-dead-daemon.test.ts diff --git a/browse/src/cli.ts b/browse/src/cli.ts index cfe8c5ee5..b4b1faea2 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -742,6 +742,14 @@ async function sendCommand(state: ServerState, command: string, args: string[], if (action === 'report-busy') { reportDaemonBusyAndExit(oldState!.pid); } + // #2254: `stop` against a daemon that died mid-flight is SUCCESS — the + // desired end state (no daemon) already holds. Restarting a daemon just + // to stop it again was the crash-restart loop the issue reports. + if (action === 'restart-dead' && command === 'stop') { + safeUnlinkQuiet(config.stateFile); + console.log('Daemon already stopped (cleaned stale state).'); + process.exit(0); + } // 'restart-dead' or explicit 'force-restart' → restart. if (retries >= 1) throw new Error('[browse] Server crashed twice in a row — aborting'); if (action === 'force-restart') { @@ -1498,6 +1506,26 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: process.exit(0); } + // ─── Stop (pre-server short-circuit, #2254) ────────────────── + // stop must be handled BEFORE ensureServer(): stopping a daemon that is + // not running must not START one just to stop it. The old flow booted a + // fresh daemon + Chromium (multi-second, resource churn) and then told it + // to shut down — or crashed trying. No state, or dead pid + dead port → + // report "nothing to stop" and exit 0. + if (command === 'stop') { + const stopState = readState(); + if (!stopState) { + console.log('No daemon running — nothing to stop.'); + process.exit(0); + } + if (!isProcessAlive(stopState.pid) && !(await isServerHealthy(stopState.port))) { + safeUnlinkQuiet(config.stateFile); + console.log('No daemon running (cleaned stale state) — nothing to stop.'); + process.exit(0); + } + // Live daemon → fall through to the normal sendCommand('stop') path. + } + // Special case: chain reads from stdin if (command === 'chain' && commandArgs.length === 0) { const stdin = await Bun.stdin.text(); diff --git a/browse/test/stop-dead-daemon.test.ts b/browse/test/stop-dead-daemon.test.ts new file mode 100644 index 000000000..7ec8eb8f4 --- /dev/null +++ b/browse/test/stop-dead-daemon.test.ts @@ -0,0 +1,99 @@ +/** + * #2254: `browse stop` against a daemon that isn't running must report + * success and exit 0 — WITHOUT starting a daemon just to stop it. The old + * flow routed stop through ensureServer(), which booted a fresh daemon + + * Chromium (multi-second, resource churn) and then shut it down — or + * crash-restarted on a stale state file. + * + * Integration pattern follows busy-daemon-recovery.test.ts: a scratch + * BROWSE_STATE_FILE + a real spawned CLI. + */ + +import { describe, test, expect } from 'bun:test'; +import { spawn } from 'child_process'; +import * as fs from 'fs'; +import * as net from 'net'; +import * as os from 'os'; +import * as path from 'path'; + +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; +} + +/** Grab a port that is definitely closed (bind, read, release). */ +async function closedPort(): Promise { + return new Promise((resolve, reject) => { + const srv = net.createServer(); + srv.once('error', reject); + srv.listen(0, '127.0.0.1', () => { + const addr = srv.address(); + if (!addr || typeof addr === 'string') { reject(new Error('bad address')); return; } + const port = addr.port; + srv.close(() => resolve(port)); + }); + }); +} + +describe('#2254 stop on a dead daemon', () => { + test('no daemon state at all → exit 0, nothing spawned', async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-stop-')); + const stateFile = path.join(tmpDir, 'browse.json'); + try { + const result = await runCli(['stop'], baseEnv(stateFile)); + + expect(result.code).toBe(0); + expect(result.stdout).toContain('nothing to stop'); + // The load-bearing half: NO daemon was started to serve the stop — + // a spawned daemon would have written the state file. + expect(fs.existsSync(stateFile)).toBe(false); + expect(result.stderr).not.toContain('Starting server'); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 30_000); + + test('stale state (dead pid + closed port) → exit 0, state cleaned, nothing spawned', async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-stop-')); + const stateFile = path.join(tmpDir, 'browse.json'); + try { + // A pid that is certainly not alive and a port nothing listens on. + const port = await closedPort(); + fs.writeFileSync(stateFile, JSON.stringify({ + pid: 2147483646, + port, + token: 'stale-token', + startedAt: new Date().toISOString(), + serverPath: '', + mode: 'launched' as const, + }, null, 2)); + + const result = await runCli(['stop'], baseEnv(stateFile)); + + expect(result.code).toBe(0); + expect(result.stdout).toContain('nothing to stop'); + // Stale state cleaned, and no daemon spawned to replace it. + expect(fs.existsSync(stateFile)).toBe(false); + expect(result.stderr).not.toContain('Starting server'); + expect(result.stderr).not.toContain('Restarting'); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 30_000); +});