mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-20 11:52:20 +02:00
fix(browse): stop on a dead daemon is success — never boots a daemon to stop it (#2254)
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
e2f70f1704
commit
5a6dd6bf88
@@ -742,6 +742,14 @@ async function sendCommand(state: ServerState, command: string, args: string[],
|
|||||||
if (action === 'report-busy') {
|
if (action === 'report-busy') {
|
||||||
reportDaemonBusyAndExit(oldState!.pid);
|
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.
|
// 'restart-dead' or explicit 'force-restart' → restart.
|
||||||
if (retries >= 1) throw new Error('[browse] Server crashed twice in a row — aborting');
|
if (retries >= 1) throw new Error('[browse] Server crashed twice in a row — aborting');
|
||||||
if (action === 'force-restart') {
|
if (action === 'force-restart') {
|
||||||
@@ -1498,6 +1506,26 @@ Refs: After 'snapshot', use @e1, @e2... as selectors:
|
|||||||
process.exit(0);
|
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
|
// Special case: chain reads from stdin
|
||||||
if (command === 'chain' && commandArgs.length === 0) {
|
if (command === 'chain' && commandArgs.length === 0) {
|
||||||
const stdin = await Bun.stdin.text();
|
const stdin = await Bun.stdin.text();
|
||||||
|
|||||||
@@ -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<string, string>, 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<string, string> {
|
||||||
|
const env: Record<string, string> = {};
|
||||||
|
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<number> {
|
||||||
|
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);
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user