From 897f41cfd9debbbd0f7c5120b48b90cd620045f0 Mon Sep 17 00:00:00 2001 From: TTmo123 Date: Sun, 28 Jun 2026 22:01:35 +0800 Subject: [PATCH 1/2] fix(browse): skip crash-retry when user invokes stop or restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sendCommand's ECONNRESET handler treats every dropped connection as a crash and restarts the daemon. But stop and restart are user-initiated shutdowns: the daemon intentionally closes the HTTP connection before the response body flushes (meta-commands.ts:423-433). The restart attempt sends stop again, the new daemon exits again, and retries >= 1 throws 'Server crashed twice in a row — aborting.' Fix: add a short-circuit check for command === 'stop' || command === 'restart' at the top of the ECONNRESET handler. When user-intended, call process.exit(0) to skip main()'s surrounding .catch(), which would print '[browse] fetch failed' and exit 1. 'command' is already in closure from sendCommand's signature (cli.ts:527). The recursive calls at cli.ts:596 and :613 pass 'command' explicitly. Daemon-side handlers (meta-commands.ts) are unchanged. No 'chore: rebuild' commit — CI's windows-setup-e2e.yml rebuilds browse binary on every push touching browse/src/cli.ts. Co-Authored-By: Claude Opus 4.8 (1M context) --- browse/src/cli.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 59327b792..7e41ca1d1 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -585,6 +585,15 @@ 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')) { + // User-initiated shutdown (stop / restart) intentionally closes the daemon. + // Both meta-commands.ts:423 ('stop') and :428 ('restart') call shutdown(), + // which closes the HTTP connection before the response body reaches the + // client. ECONNRESET here is the expected exit signal, not a crash. + // 'command' is already in closure from sendCommand's signature — no new + // param passing. + if (command === 'stop' || command === 'restart') { + process.exit(0); + } 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 From 9b36c050b09dfcd14ccef39df266a739003358e7 Mon Sep 17 00:00:00 2001 From: TTmo123 Date: Sun, 28 Jun 2026 22:02:38 +0800 Subject: [PATCH 2/2] test(browse): cover sendCommand skip-restart for stop and restart commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four test cases for the sendCommand ECONNRESET fix: 1. stop command on ECONNRESET does not print crash warning — runs browse stop against a real daemon; asserts exit 0 and absence of 'crashed' / 'aborting' / 'fetch failed' in stderr. 2. restart command on ECONNRESET does not print crash warning — same assertion for the restart command. 3. guard is scoped to stop + restart only (regression guard) — reads browse/src/cli.ts and asserts the new short-circuit checks both 'stop' and 'restart', and that the original ECONNREFUSED / ECONNRESET handler is preserved. 4. guard preserves original crash-retry path (regression guard) — asserts 'Server crashed twice in a row' and the 'retries >= 1' guard are still present. Cases 1-2 use describe.skipIf(!HAS_BINARY) so shards without a built binary gracefully skip. Cases 3-4 always run as source-shape checks, providing a safety net against future refactors that accidentally widen the short-circuit or strip the crash-retry path. Co-Authored-By: Claude Opus 4.8 (1M context) --- test/send-command-stop-skip-restart.test.ts | 101 ++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 test/send-command-stop-skip-restart.test.ts diff --git a/test/send-command-stop-skip-restart.test.ts b/test/send-command-stop-skip-restart.test.ts new file mode 100644 index 000000000..f7775a9b0 --- /dev/null +++ b/test/send-command-stop-skip-restart.test.ts @@ -0,0 +1,101 @@ +/** + * sendCommand ECONNRESET short-circuit for user-initiated shutdown. + * + * Validates that browse stop / browse restart do NOT trigger the + * "Server crashed twice in a row" warning when the daemon intentionally + * closes the HTTP connection. The regression guard confirms the + * short-circuit only applies to stop/restart — non-shutdown commands + * still hit the existing crash-retry logic. + * + * We exercise the actual binary because sendCommand is an internal + * module-level function and cannot be unit-mocked against a real + * daemon fetch endpoint without re-architecting cli.ts. The test + * gracefully degrades when the binary is not built + * (describe.skipIf(!HAS_BINARY)) so CI shards without a dist/ still + * pass. + */ +import { describe, test, expect } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const BROWSE_BIN = path.join( + ROOT, + 'browse', + 'dist', + process.platform === 'win32' ? 'browse.exe' : 'browse', +); + +const HAS_BINARY = (() => { + try { + return fs.existsSync(BROWSE_BIN); + } catch { + return false; + } +})(); + +describe.skipIf(!HAS_BINARY)('browse: user-initiated shutdown skips crash-retry', () => { + test('stop command on ECONNRESET does not print crash warning', () => { + // Ensure daemon is up; status is idempotent and will start one if needed. + spawnSync(BROWSE_BIN, ['status'], { encoding: 'utf-8', timeout: 10000 }); + + const result = spawnSync(BROWSE_BIN, ['stop'], { + encoding: 'utf-8', + timeout: 10000, + cwd: ROOT, + }); + + expect(result.status).toBe(0); + expect(result.stderr).not.toContain('crashed'); + expect(result.stderr).not.toContain('aborting'); + expect(result.stderr).not.toContain('fetch failed'); + }); + + test('restart command on ECONNRESET does not print crash warning', () => { + spawnSync(BROWSE_BIN, ['status'], { encoding: 'utf-8', timeout: 10000 }); + + const result = spawnSync(BROWSE_BIN, ['restart'], { + encoding: 'utf-8', + timeout: 15000, + cwd: ROOT, + }); + + expect(result.status).toBe(0); + expect(result.stderr).not.toContain('crashed'); + expect(result.stderr).not.toContain('aborting'); + }); +}); + +describe('browse: sendCommand short-circuit regression guard', () => { + // Code-shape test: catches a refactor that accidentally widens the + // short-circuit to all commands (which would silently skip crash-retry + // for genuine daemon failures during goto/status/etc.). + test('guard is scoped to stop + restart only', () => { + const cliPath = path.join(ROOT, 'browse', 'src', 'cli.ts'); + const cliSrc = fs.readFileSync(cliPath, 'utf-8'); + + // The guard must check both 'stop' AND 'restart' + expect(cliSrc).toMatch( + /if\s*\(\s*command\s*===\s*['"]stop['"]\s*\|\|\s*command\s*===\s*['"]restart['"]\s*\)/, + ); + // The guard must call process.exit(0) (not throw — throw would be + // caught by main().catch() and print "[browse] fetch failed") + expect(cliSrc).toMatch(/process\.exit\(0\)/); + // The guard must live inside the existing ECONNRESET handler, not + // replace it + expect(cliSrc).toContain('ECONNREFUSED'); + expect(cliSrc).toContain('ECONNRESET'); + }); + + test('guard preserves original crash-retry path for non-shutdown commands', () => { + const cliPath = path.join(ROOT, 'browse', 'src', 'cli.ts'); + const cliSrc = fs.readFileSync(cliPath, 'utf-8'); + + // The "Server crashed twice in a row" message must still exist — + // removing it would silently degrade the genuine-crash reporting. + expect(cliSrc).toContain('Server crashed twice in a row'); + // And it must be reached when retries >= 1 + expect(cliSrc).toMatch(/retries\s*>=\s*1.*aborting/); + }); +}); \ No newline at end of file