mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-14 00:49:00 +02:00
fix(browse): honest probe budget, bounded daemon log, single refusal source, liveness + reinstall coverage
Five hardening items in the browse CLI and its tests: - probeHealthWithBackoff's advertised ~8s budget could really run ~10s: the final 2s probe could start 1ms before the deadline, and every call site had JUST run a failed probe yet the loop re-probed immediately. Iterations now start with the sleep and each probe's timeout clamps to the remaining budget (isServerHealthy takes an injectable timeout). - browse-daemon.log is append-mode across every respawn with no size cap, so a crash-respawn loop fills the disk. The path is now built in one place (daemonLogPath — the Unix fd path and the Windows launcher string had two spellings) and daemon start rotates a >10MB log to browse-daemon.log.1, single generation, matching the repo's 10MB rotation convention. Rotation is exported + injectable and behaviorally unit-tested. - The two "healthy daemon already running" refusal blocks in connect had already drifted (one lost the tabs/cookies/logins explainer) — extracted refuseHeadedOverLiveDaemon as the single source. - process-liveness: pinned the EPERM-means-alive contract (PID 1 on POSIX, PID 4 on Windows — signalable-or-EPERM, both alive). A probe that reads EPERM as dead is the false negative that leaked agents. - runBoundedChromiumReinstall had zero coverage: now exercised end-to-end against a stub bunx on a prepended PATH — exit 0, install-exit-N with stderr tail, the detached group-kill timeout path (child of the child dies too), and spawn-error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
412ad5c1f9
commit
aa6c73821f
@@ -40,10 +40,50 @@ describe('#2461 daemon log wiring', () => {
|
||||
|
||||
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')");
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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:');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user