diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 1651fed4b..c29833f52 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -146,10 +146,10 @@ function readState(): ServerState | null { * HTTP health check — definitive proof the server is alive and responsive. * Used in all polling loops instead of isProcessAlive() (which is slow on Windows). */ -export async function isServerHealthy(port: number): Promise { +export async function isServerHealthy(port: number, timeoutMs = 2000): Promise { try { const resp = await fetch(`http://127.0.0.1:${port}/health`, { - signal: AbortSignal.timeout(2000), + signal: AbortSignal.timeout(timeoutMs), }); if (!resp.ok) return false; const health = await resp.json() as any; @@ -281,7 +281,13 @@ 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). */ + * crash-loop. + * + * P4 wall-time honesty: every call site reaches here right after a probe or + * command already failed, so iterations START with the sleep (an immediate + * re-probe would just re-fail), and each probe's timeout is clamped to the + * remaining budget — otherwise the last 2s probe could start 1ms before the + * deadline and the reported "~8s" budget would really be ~10s. */ async function probeHealthWithBackoff( port: number, totalBudgetMs = HEALTH_PROBE_TOTAL_BUDGET_MS, @@ -289,9 +295,11 @@ async function probeHealthWithBackoff( ): Promise { const deadline = Date.now() + totalBudgetMs; for (;;) { - if (await isServerHealthy(port)) return true; if (Date.now() + intervalMs >= deadline) return false; await Bun.sleep(intervalMs); + const remainingMs = deadline - Date.now(); + if (remainingMs <= 0) return false; + if (await isServerHealthy(port, Math.min(2000, remainingMs))) return true; } } @@ -320,6 +328,18 @@ export function decideDaemonRestart(opts: { return 'restart-dead'; } +/** #2219 IRON RULE refusal for `connect`: a live daemon is never replaced + * without explicit consent. Single source for the refusal text (M7) — the + * two call sites (healthy fast-path, busy-but-alive after the bounded probe) + * previously duplicated it, and the tabs/cookies/logins explainer had + * already drifted out of one of them. */ +function refuseHeadedOverLiveDaemon(state: { pid: number; mode?: string }): never { + console.error(`[browse] A healthy daemon is already running (PID ${state.pid}, ${state.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); +} + /** 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.`); @@ -378,9 +398,38 @@ function raiseHeadedWindowMacOS(): void { // F6 log hygiene: nothing that reaches the daemon's stdout/stderr may carry // an auth token or unsanitized page-derived strings — // browse/test/daemon-log-hygiene.test.ts pins this with needle tests. +// +// Single source for the log path (M4): the Unix fd-open path and the Windows +// launcher string both build it, and a drifted spelling would silently split +// the daemon's history across two files. +function daemonLogPath(): string { + return path.join(config.stateDir, 'browse-daemon.log'); +} + +/** Append-mode growth bound: the log accumulates across every respawn (a + * crash-respawn loop would otherwise fill the disk), so on daemon start a + * log past 10MB (the repo's rotation convention — tunnel-denial-log.ts uses + * the same cap) is renamed to browse-daemon.log.1, single generation. + * Best-effort: a failed stat/rename must never block the launch. + * Path + cap injectable for unit coverage; exported for the same reason. */ +export const DAEMON_LOG_MAX_BYTES = 10 * 1024 * 1024; +export function rotateDaemonLogIfOversized( + p: string = daemonLogPath(), + maxBytes: number = DAEMON_LOG_MAX_BYTES, +): void { + try { + if (fs.statSync(p).size > maxBytes) { + fs.renameSync(p, `${p}.1`); + } + } catch { + // Missing log (first launch) or unwritable state dir — rotation is + // best-effort, the launch matters more. + } +} + function openDaemonLogSink(): number | 'ignore' { try { - return fs.openSync(path.join(config.stateDir, 'browse-daemon.log'), 'a'); + return fs.openSync(daemonLogPath(), 'a'); } catch { // stateDir not writable (permissions, disk full) — fall back to the // previous behavior rather than fail the whole launch over logging. @@ -391,6 +440,9 @@ function openDaemonLogSink(): number | 'ignore' { async function startServer(extraEnv?: Record): Promise { ensureStateDir(config); + // Bound the append-mode daemon log before the new daemon starts writing. + rotateDaemonLogIfOversized(); + // Clean up stale state file and error log safeUnlink(config.stateFile); safeUnlink(path.join(config.stateDir, 'browse-startup-error.log')); @@ -421,7 +473,7 @@ async function startServer(extraEnv?: Record): Promise { test('log sink is append-mode (accumulates across respawns)', () => { const cli = SRC('cli.ts'); - expect(cli).toMatch(/openSync\(path\.join\(config\.stateDir, 'browse-daemon\.log'\), 'a'\)/); + // Both spawn paths open through the single daemonLogPath() source (M4), + // which itself must build from the state dir. + expect(cli).toMatch(/openSync\(daemonLogPath\(\), 'a'\)/); + expect(cli).toMatch(/path\.join\(config\.stateDir, 'browse-daemon\.log'\)/); expect(cli).toMatch(/openSync\(\$\{daemonLogPathStr\},'a'\)/); }); + test('append-mode log is growth-bounded: rotated at 10MB before daemon start', () => { + const cli = SRC('cli.ts'); + expect(cli).toMatch(/DAEMON_LOG_MAX_BYTES = 10 \* 1024 \* 1024/); + expect(cli).toMatch(/rotateDaemonLogIfOversized\(\);/); + // Single generation: rename to .1, matching the repo's 10MB conventions. + expect(cli).toMatch(/renameSync\(p, `\$\{p\}\.1`\)/); + }); + + test('rotation behavior: oversized rotates to a single .1 generation, small/missing are no-ops', () => { + const os = require('os'); + const { rotateDaemonLogIfOversized } = require('../src/cli'); + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-daemon-log-')); + try { + const p = path.join(tmp, 'browse-daemon.log'); + // Missing log: no throw (first launch). + rotateDaemonLogIfOversized(p, 1024); + // Under the cap: untouched, no generation created. + fs.writeFileSync(p, 'x'.repeat(10)); + rotateDaemonLogIfOversized(p, 1024); + expect(fs.existsSync(p)).toBe(true); + expect(fs.existsSync(`${p}.1`)).toBe(false); + // Over the cap: rotated out of the way so the daemon starts fresh. + fs.writeFileSync(p, 'y'.repeat(2048)); + rotateDaemonLogIfOversized(p, 1024); + expect(fs.existsSync(p)).toBe(false); + expect(fs.readFileSync(`${p}.1`, 'utf-8')).toContain('y'); + // Single generation: the next rotation REPLACES .1 (bounded at ~2x cap + // total, never a .2). + fs.writeFileSync(p, 'z'.repeat(2048)); + rotateDaemonLogIfOversized(p, 1024); + expect(fs.readFileSync(`${p}.1`, 'utf-8')).toContain('z'); + expect(fs.existsSync(`${p}.2`)).toBe(false); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + test('bun-polyfill routes Windows spawns through cross-spawn (ENOENT + cmd.exe injection fix)', () => { const polyfill = SRC('bun-polyfill.cjs'); expect(polyfill).toContain("require('cross-spawn')"); diff --git a/browse/test/process-liveness-windows.test.ts b/browse/test/process-liveness-windows.test.ts index 104f23d71..de907af40 100644 --- a/browse/test/process-liveness-windows.test.ts +++ b/browse/test/process-liveness-windows.test.ts @@ -54,6 +54,15 @@ describe('process liveness probe (Windows terminal-agent leak)', () => { expect(isProcessAlive(2147483646)).toBe(false); }); + test('2b. EPERM means ALIVE: an unsignalable-but-existing PID is not dead (T2)', () => { + // signal-0 to a process we lack permission over throws EPERM — the + // process EXISTS, we just can't signal it. Treating EPERM as "dead" is + // the false negative that leaked agents. PID 1 (launchd/init) on POSIX + // and PID 4 (System) on Windows always exist and are either signalable + // or EPERM — both must read as alive. + expect(isProcessAlive(process.platform === 'win32' ? 4 : 1)).toBe(true); + }); + test('3. isProcessAlive spawns NO subprocess on ANY platform (signal-0, #1952)', () => { // The heart of the bug: a liveness probe that forks is slow enough to // time out, and a timed-out probe silently answers "dead". Signal 0 diff --git a/browse/test/xprotect-heal.test.ts b/browse/test/xprotect-heal.test.ts index 014c6125d..5a41898fe 100644 --- a/browse/test/xprotect-heal.test.ts +++ b/browse/test/xprotect-heal.test.ts @@ -28,6 +28,7 @@ import { launchWithXProtectHeal, resetXProtectHealForTests, buildXProtectGuidance, + runBoundedChromiumReinstall, } from '../src/xprotect-heal'; const REPO_ROOT = path.resolve(import.meta.dir, '..', '..'); @@ -391,3 +392,66 @@ describe('buildXProtectGuidance', () => { expect(out).toContain('#2554'); }); }); + +// ─── runBoundedChromiumReinstall (T3) — previously zero coverage ────────── +// +// Exercised end-to-end against a STUB `bunx` on a prepended PATH: real spawn, +// real process group, real timer — only the binary is fake. Shell stubs +// don't exist on Windows, and the group-kill path is POSIX (`kill(-pid)`), +// so the suite is Unix-only like the shape it tests. +describe.skipIf(process.platform === 'win32')('runBoundedChromiumReinstall', () => { + let stubDir: string; + let savedPath: string | undefined; + + function installStubBunx(script: string): void { + const stub = path.join(stubDir, 'bunx'); + fs.writeFileSync(stub, `#!/bin/sh\n${script}\n`, { mode: 0o755 }); + } + + beforeEach(() => { + stubDir = fs.mkdtempSync(path.join(os.tmpdir(), 'xprotect-stub-')); + savedPath = process.env.PATH; + process.env.PATH = `${stubDir}${path.delimiter}${process.env.PATH ?? ''}`; + }); + + afterEach(() => { + process.env.PATH = savedPath; + fs.rmSync(stubDir, { recursive: true, force: true }); + }); + + it('resolves ok on exit 0', async () => { + installStubBunx('exit 0'); + const result = await runBoundedChromiumReinstall(stubDir, 10_000); + expect(result).toEqual({ ok: true, exitCode: 0 }); + }); + + it('reports install-exit-N with the stderr tail on a nonzero exit', async () => { + installStubBunx('echo "download failed: mirror unreachable" >&2\nexit 7'); + const result = await runBoundedChromiumReinstall(stubDir, 10_000); + expect(result.ok).toBe(false); + expect(result.exitCode).toBe(7); + expect(result.reason).toStartWith('install-exit-7'); + expect(result.reason).toContain('download failed: mirror unreachable'); + }); + + it('group-kills a hung install at timeoutMs and reports timeout', async () => { + // The stub spawns its own child (like bunx → playwright CLI → download + // workers) and sleeps well past the bound; the detached process group + // must take BOTH down, and the result must arrive at ~timeoutMs, not + // after the sleep. + installStubBunx('sleep 30 &\nsleep 30'); + const started = Date.now(); + const result = await runBoundedChromiumReinstall(stubDir, 500); + const elapsed = Date.now() - started; + expect(result).toEqual({ ok: false, reason: 'timeout' }); + expect(elapsed).toBeLessThan(5_000); // resolved at the bound, not the sleep + }); + + it('reports spawn-error when the binary cannot be executed', async () => { + // No stub installed and PATH reduced to the empty stub dir only. + process.env.PATH = stubDir; + const result = await runBoundedChromiumReinstall(stubDir, 10_000); + expect(result.ok).toBe(false); + expect(result.reason).toStartWith('spawn-error:'); + }); +});