From 6d6ffd5df45d36dca63a871ed02a7ace9372ea74 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 21 Aug 2026 07:47:08 -0600 Subject: [PATCH 01/14] fix(lifecycle): reap Harper after runner death Co-Authored-By: GPT-5 Codex --- CONTRIBUTING.md | 2 +- README.md | 5 +- src/harperLifecycle.ts | 113 +++++++++++++++----- src/harperSupervisor.ts | 114 ++++++++++++++++++++ test/harperLifecycle.test.ts | 199 +++++++++++++++++++++++++++++++++-- 5 files changed, 399 insertions(+), 34 deletions(-) create mode 100644 src/harperSupervisor.ts diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9d69b22..6eb85f6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -6,7 +6,7 @@ Contributors are encouraged to communicate with maintainers in issues or other c Source files are located in `src/`. These are built to the `dist/` directory. The published package includes `dist/`, `scripts/`, and the regular npm metadata and documentation files. -The `src/index.ts` is the source for the main export. This is the public re-export of all the various utilities from `src/harperLifecycle.ts`, `targz.ts`, and more. The `src/run.ts` is the source for the `harper-integration-test-run` bin script. And the `scripts/setup-loopback.sh` is the source for the `harper-integration-test-setup-loopback` bin script. +The `src/index.ts` is the source for the main export. This is the public re-export of all the various utilities from `src/harperLifecycle.ts`, `targz.ts`, and more. The `src/run.ts` is the source for the `harper-integration-test-run` bin script. The internal `src/harperSupervisor.ts` process owns the detached Harper group and watches a runner-liveness pipe so hard runner death cannot orphan Harper. And the `scripts/setup-loopback.sh` is the source for the `harper-integration-test-setup-loopback` bin script. The package is `"type": "module"` — all source files are ESM by default. diff --git a/README.md b/README.md index 9ee90f1..5f770fd 100644 --- a/README.md +++ b/README.md @@ -88,6 +88,8 @@ The lifecycle and utility APIs below are framework-agnostic. They manage Harper Allocates a loopback address from the pool, creates a temporary install directory, starts a Harper process, and waits for it to be ready. Populates `ctx.harper` with the instance details. Call in a setup/`before()` hook. +The harness starts Harper behind a small lifecycle supervisor. A private pipe lets the supervisor detect runner death even when JavaScript cleanup cannot run (for example `SIGKILL` or a hard crash), then terminate Harper's detached process tree so it cannot remain orphaned holding ports. `ctx.harper.process` is the supervisor-backed lifecycle handle used by `killHarper`; use `ctx.harper.harperPid` when the Harper runtime's own PID is specifically needed. + The Harper binary is resolved in the following order: 1. `harperBinPath` option passed directly to `startHarper()` @@ -165,7 +167,8 @@ interface HarperContext { httpURL: string; // e.g. 'http://127.0.0.2:9926' operationsAPIURL: string; // e.g. 'http://127.0.0.2:9925' hostname: string; // e.g. '127.0.0.2' - process: ChildProcess; + process: ChildProcess; // supervisor-backed lifecycle handle; pass to lifecycle APIs + harperPid?: number; // PID of the managed Harper runtime logDir?: string; // set when HARPER_INTEGRATION_TEST_LOG_DIR is configured startupOutput?: { stdout: string; stderr: string }; // captured startup output } diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index 11f733b..d0ccdeb 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -8,6 +8,7 @@ import { getNextAvailableLoopbackAddress, releaseLoopbackAddress } from './loopb import { waitForPortsFree } from './portUtils.ts'; import { ok, equal } from 'node:assert'; import { createRequire } from 'node:module'; +import { fileURLToPath } from 'node:url'; /** * Minimal context interface required by startHarper/teardownHarper. @@ -185,8 +186,10 @@ export interface HarperContext { operationsAPIURL: string; /** Assigned loopback IP address (e.g., '127.0.0.2') */ hostname: string; - /** Child process for the Harper instance */ + /** Lifecycle process handle for the Harper instance (the harness supervisor when started by this package). */ process: ChildProcess; + /** PID of the Harper runtime managed by the lifecycle process. */ + harperPid?: number; /** Absolute path to the log directory for this suite (only set when HARPER_INTEGRATION_TEST_LOG_DIR is configured) */ logDir?: string; /** Captured stdout/stderr from Harper startup, up to the point it reported ready. */ @@ -362,12 +365,30 @@ interface RunHarperCommandOptions { interface RunHarperCommandResult { process: ChildProcess; + harperPid: number; /** Captured stdout up to the point the process was considered ready or exited. */ stdout: string; /** Captured stderr up to the point the process was considered ready or exited. */ stderr: string; } +interface TrackedHarperProcess { + harperPid?: number; + harperExited: boolean; + harperPidReady: Promise; + resolveHarperPid: (pid: number) => void; + livenessPipe: { unref(): void; destroy(): void }; +} + +type SupervisorMessage = + | { type: 'harper-spawn'; pid: number } + | { type: 'harper-exit' }; + +function getHarperSupervisorScript(): string { + const extension = import.meta.url.endsWith('.ts') ? 'ts' : 'js'; + return fileURLToPath(new URL(`./harperSupervisor.${extension}`, import.meta.url)); +} + /** * Runs a Harper CLI command and captures output. * @@ -393,18 +414,17 @@ export function runHarperCommand({ runtime === 'bun' ? [harperScript, ...args] : ['--trace-warnings', '--force-node-api-uncaught-exceptions-policy=true', harperScript, ...args]; - const proc = spawn(runtime, runtimeArgs, { + const proc = spawn(process.execPath, [getHarperSupervisorScript(), runtime, ...runtimeArgs], { env: { ...process.env, ...env }, - // On POSIX, run Harper as its own process-group leader so teardown can signal the whole - // group (parent + any worker children), not just the direct child. Windows has no process - // groups; killHarper uses `taskkill /T` there instead. stdio stays piped (not detached), - // and we never unref, so output capture and lifetime management are unchanged. + // The supervisor is the POSIX process-group leader, preserving whole-tree teardown while + // its inherited stdio keeps Harper output capture unchanged. Windows uses `taskkill /T`. detached: process.platform !== 'win32', + stdio: ['pipe', 'pipe', 'pipe', 'pipe', 'ipc'], }); // Reap this process's tree if the runner exits/is interrupted before teardown (it's detached, // so it would otherwise survive signals sent to the runner's group). - trackHarperProcess(proc); + const trackedProcess = trackHarperProcess(proc); let stdoutStream: WriteStream | undefined; let stderrStream: WriteStream | undefined; @@ -420,6 +440,7 @@ export function runHarperCommand({ let stdout = ''; let stderr = ''; let settled = false; + let readinessDetected = false; let idleTimer: NodeJS.Timeout; let maxTimer: NodeJS.Timeout; @@ -433,16 +454,19 @@ export function runHarperCommand({ settled = true; clearTimers(); reject(new HarperStartupError(message, stdout, stderr)); - // Harper is spawned detached (its own process group), so `proc.kill()` would only hit the - // direct child and orphan any worker children still holding ports. Reap the whole tree. + // The supervisor is the detached group leader, so reap the whole group rather than only it. signalHarperTree(proc, 'SIGKILL'); }; const succeed = () => { - if (settled) return; - settled = true; + if (settled || readinessDetected) return; + readinessDetected = true; clearTimers(); - resolve({ process: proc, stdout, stderr }); + void trackedProcess.harperPidReady.then((harperPid) => { + if (settled) return; + settled = true; + resolve({ process: proc, harperPid, stdout, stderr }); + }); }; // Reset on every chunk of output so the limit is time-since-last-progress, not total boot @@ -501,7 +525,7 @@ export function runHarperCommand({ if (!settled) { settled = true; if (statusCode === 0) { - resolve({ process: proc, stdout, stderr }); + void trackedProcess.harperPidReady.then((harperPid) => resolve({ process: proc, harperPid, stdout, stderr })); } else { const errorMessage = `Harper process failed with exit code/signal ${statusCode ?? signal}`; stderrStream?.write(errorMessage); @@ -641,6 +665,7 @@ export async function startHarper(ctx: HarperTestContext, options?: StartHarperO operationsAPIURL: `http://${loopbackAddress}:${OPERATIONS_API_PORT}`, hostname: loopbackAddress, process: result.process, + harperPid: result.harperPid, logDir, startupOutput: { stdout: result.stdout, stderr: result.stderr }, }); @@ -661,14 +686,18 @@ export async function startHarper(ctx: HarperTestContext, options?: StartHarperO * Best-effort: errors (e.g. the process already exited) are swallowed; teardown's port assertion * and the wait-for-exit are the safety nets. */ +function signalWindowsProcessTree(pid: number, signal: 'SIGTERM' | 'SIGKILL'): void { + const args = ['/pid', String(pid), '/T']; + if (signal === 'SIGKILL') args.push('/F'); + spawn('taskkill', args, { stdio: 'ignore' }).on('error', () => {}); +} + function signalHarperTree(proc: ChildProcess, signal: 'SIGTERM' | 'SIGKILL'): void { const pid = proc.pid; if (pid === undefined) return; if (process.platform === 'win32') { - const args = ['/pid', String(pid), '/T']; - if (signal === 'SIGKILL') args.push('/F'); try { - spawn('taskkill', args, { stdio: 'ignore' }).on('error', () => {}); + signalWindowsProcessTree(pid, signal); } catch { try { proc.kill(signal); @@ -697,30 +726,62 @@ function signalHarperTree(proc: ChildProcess, signal: 'SIGTERM' | 'SIGKILL'): vo * orphaned holding the fixed ports. This matters specifically because Harper is spawned `detached` * (its own process group), so it would otherwise survive signals delivered to the runner's group. * - * Best-effort: on POSIX the group SIGKILL is delivered synchronously (works even from the 'exit' - * handler); on Windows the `taskkill` shell-out may not complete before the runner exits. + * The supervisor's liveness pipe handles uncatchable runner death. These parent-side hooks provide + * immediate cleanup for cooperative exits and retain a fallback for unexpected supervisor death. */ -const liveHarperProcesses = new Set(); +const liveHarperProcesses = new Map(); let runnerCleanupRegistered = false; -function trackHarperProcess(proc: ChildProcess): void { - liveHarperProcesses.add(proc); - proc.once('exit', () => liveHarperProcesses.delete(proc)); +function trackHarperProcess(proc: ChildProcess): TrackedHarperProcess { + const livenessPipe = proc.stdio[3] as unknown as TrackedHarperProcess['livenessPipe']; + livenessPipe.unref(); + let resolveHarperPid!: (pid: number) => void; + const trackedProcess: TrackedHarperProcess = { + harperExited: false, + harperPidReady: new Promise((resolve) => { + resolveHarperPid = resolve; + }), + resolveHarperPid: (pid) => resolveHarperPid(pid), + livenessPipe, + }; + liveHarperProcesses.set(proc, trackedProcess); + proc.on('message', (message: SupervisorMessage) => { + if (message.type === 'harper-spawn') { + trackedProcess.harperPid = message.pid; + trackedProcess.resolveHarperPid(message.pid); + } else if (message.type === 'harper-exit') { + trackedProcess.harperExited = true; + } + }); + proc.channel?.unref(); + proc.once('exit', () => { + if (!trackedProcess.harperExited) { + signalHarperTree(proc, 'SIGKILL'); + if (process.platform === 'win32' && trackedProcess.harperPid !== undefined) { + signalWindowsProcessTree(trackedProcess.harperPid, 'SIGKILL'); + } + } + trackedProcess.livenessPipe.destroy(); + liveHarperProcesses.delete(proc); + }); - if (runnerCleanupRegistered) return; + if (runnerCleanupRegistered) return trackedProcess; runnerCleanupRegistered = true; const reapAll = () => { - for (const child of liveHarperProcesses) signalHarperTree(child, 'SIGKILL'); + for (const child of liveHarperProcesses.keys()) signalHarperTree(child, 'SIGKILL'); }; process.once('exit', reapAll); - // SIGINT/SIGTERM don't fire 'exit'; reap, then re-raise so the runner still terminates normally. - for (const signal of ['SIGINT', 'SIGTERM'] as const) { + // These signals don't fire 'exit'; reap, then re-raise so the runner still terminates normally. + const cleanupSignals: NodeJS.Signals[] = ['SIGINT', 'SIGTERM']; + if (process.platform !== 'win32') cleanupSignals.push('SIGHUP'); + for (const signal of cleanupSignals) { process.once(signal, () => { reapAll(); process.kill(process.pid, signal); }); } + return trackedProcess; } /** Identifies the objects published as `ctx.harper`; a shallow copy of a node does not carry it. */ diff --git a/src/harperSupervisor.ts b/src/harperSupervisor.ts new file mode 100644 index 0000000..c766b8e --- /dev/null +++ b/src/harperSupervisor.ts @@ -0,0 +1,114 @@ +import { spawn, spawnSync, type ChildProcess } from 'node:child_process'; +import { Socket } from 'node:net'; + +const SUPERVISOR_FAILURE_EXIT_CODE = 70; +const LIVENESS_FD = 3; + +type SupervisorMessage = + | { type: 'harper-spawn'; pid: number } + | { type: 'harper-exit' }; + +const [runtime, ...runtimeArgs] = process.argv.slice(2); +if (!runtime) { + process.stderr.write('[harper-supervisor] Missing Harper runtime\n'); + process.exit(SUPERVISOR_FAILURE_EXIT_CODE); +} + +let harperProcess: ChildProcess | undefined; +let harperExited = false; +let terminatingTree = false; + +function sendToRunner(message: SupervisorMessage, callback?: () => void): void { + if (!process.send || !process.connected) { + callback?.(); + return; + } + process.send(message, (error: Error | null) => { + if (error) process.stderr.write(`[harper-supervisor] Failed to notify runner: ${error.message}\n`); + callback?.(); + }); +} + +function terminateHarperTree(): never { + if (terminatingTree || harperExited) process.exit(SUPERVISOR_FAILURE_EXIT_CODE); + terminatingTree = true; + const harperPid = harperProcess?.pid; + if (process.platform === 'win32') { + if (harperPid !== undefined) { + spawnSync('taskkill', ['/pid', String(harperPid), '/T', '/F'], { stdio: 'ignore' }); + } + process.exit(SUPERVISOR_FAILURE_EXIT_CODE); + } + try { + process.kill(-process.pid, 'SIGKILL'); + } catch { + if (harperPid !== undefined) { + try { + process.kill(harperPid, 'SIGKILL'); + } catch { + // The Harper process already exited. + } + } + } + process.exit(SUPERVISOR_FAILURE_EXIT_CODE); +} + +function reportUnexpectedFailure(reason: unknown): never { + const message = reason instanceof Error ? reason.stack || reason.message : String(reason); + process.stderr.write(`[harper-supervisor] ${message}\n`); + terminateHarperTree(); +} + +process.on('uncaughtExceptionMonitor', reportUnexpectedFailure); +process.on('unhandledRejection', reportUnexpectedFailure); +process.once('exit', () => { + if (!harperExited) terminateHarperTree(); +}); + +const forwardedSignals: NodeJS.Signals[] = ['SIGINT', 'SIGTERM']; +if (process.platform !== 'win32') forwardedSignals.push('SIGHUP'); +for (const signal of forwardedSignals) { + process.on(signal, () => { + if (!harperExited) harperProcess?.kill(signal); + }); +} + +// The runner is the only holder of the peer endpoint, so EOF is delivered even when it cannot run cleanup code. +const runnerLiveness = new Socket({ fd: LIVENESS_FD, readable: true, writable: false }); +runnerLiveness.unref(); +runnerLiveness.once('end', terminateHarperTree); +runnerLiveness.once('close', terminateHarperTree); +runnerLiveness.on('error', (error) => { + process.stderr.write(`[harper-supervisor] Runner-liveness pipe error: ${error.message}\n`); +}); +process.channel?.unref(); + +let spawnedHarperProcess: ChildProcess; +try { + spawnedHarperProcess = spawn(runtime, runtimeArgs, { stdio: ['inherit', 'inherit', 'inherit'] }); +} catch (error) { + reportUnexpectedFailure(error); +} +harperProcess = spawnedHarperProcess; + +spawnedHarperProcess.once('spawn', () => { + const harperPid = harperProcess?.pid; + if (harperPid !== undefined) sendToRunner({ type: 'harper-spawn', pid: harperPid }); +}); +spawnedHarperProcess.once('error', (error) => { + process.stderr.write(`[harper-supervisor] Failed to spawn Harper: ${error.message}\n`); + harperExited = true; + process.exit(SUPERVISOR_FAILURE_EXIT_CODE); +}); +spawnedHarperProcess.once('exit', (statusCode, signal) => { + harperExited = true; + const finish = () => { + if (signal) { + process.removeAllListeners(signal); + process.kill(process.pid, signal); + } else { + process.exit(statusCode ?? SUPERVISOR_FAILURE_EXIT_CODE); + } + }; + sendToRunner({ type: 'harper-exit' }, finish); +}); diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 0f1ba75..12e2f72 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -5,6 +5,8 @@ import { once } from 'node:events'; import { mkdtempSync, writeFileSync, rmSync, existsSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { createServer, type AddressInfo } from 'node:net'; import { setTimeout as sleep } from 'node:timers/promises'; import { killHarper, @@ -18,6 +20,7 @@ import { buildHarperChildEnv, type StartedHarperTestContext, } from '../src/harperLifecycle.ts'; +import { isPortFree, waitForPortsFree } from '../src/portUtils.ts'; // Standalone scripts used as a fake "Harper binary" (passed via harperBinPath) to drive // runHarperCommand's startup watchdog through specific timing scenarios without a real Harper. @@ -33,8 +36,62 @@ const FIXTURE_SOURCES: Record = { 'idle-reset.cjs': "let n = 0;\nconst t = setInterval(() => {\n n++;\n process.stdout.write('progress ' + n + '\\n');\n if (n >= 8) { clearInterval(t); process.stdout.write('successfully started\\n'); }\n}, 100);\nsetInterval(() => {}, 1000);\n", // Exits non-zero. 'exit-nonzero.cjs': "process.stderr.write('boom\\n');\nprocess.exit(1);\n", + 'process-tree.cjs': ` +const { spawn } = require('node:child_process'); +const { createServer } = require('node:net'); +const host = '127.0.0.1'; +if (process.env.HARPER_FAKE_DESCENDANT === '1') { + const server = createServer(); + server.listen(Number(process.env.HARPER_DESCENDANT_PORT), host, () => { + process.stdout.write('descendant-ready:' + process.pid + '\\n'); + }); +} else { + const descendant = spawn(process.execPath, [__filename], { + env: { ...process.env, HARPER_FAKE_DESCENDANT: '1' }, + stdio: ['ignore', 'pipe', 'inherit'], + }); + let descendantOutput = ''; + descendant.stdout.on('data', (chunk) => { + descendantOutput += chunk; + if (!descendantOutput.includes('descendant-ready:')) return; + const server = createServer(); + server.listen(Number(process.env.HARPER_PORT), host, () => { + process.stdout.write('tree-ready:' + process.pid + ':' + descendant.pid + '\\n'); + process.stdout.write('successfully started\\n'); + }); + const delay = Number(process.env.HARPER_TERM_DELAY_MS || 0); + if (delay > 0) process.on('SIGTERM', () => setTimeout(() => server.close(() => process.exit(0)), delay)); + }); +} +`, + 'orphan-runner.mjs': ` +const { runHarperCommand, killHarper } = await import(process.env.HARPER_LIFECYCLE_URL); +const result = await runHarperCommand({ + args: [], + env: { + HARPER_PORT: process.env.HARPER_PORT, + HARPER_DESCENDANT_PORT: process.env.HARPER_DESCENDANT_PORT, + HARPER_TERM_DELAY_MS: process.env.HARPER_TERM_DELAY_MS, + }, + completionMessage: 'successfully started', + harperBinPath: process.env.HARPER_FAKE_SCRIPT, + timeoutMs: 5000, + maxMs: 10000, +}); +const match = result.stdout.match(/tree-ready:(\\d+):(\\d+)/); +if (!match) throw new Error('Missing fake Harper process markers: ' + result.stdout); +process.stdout.write('runner-ready:' + result.process.pid + ':' + match[1] + ':' + match[2] + '\\n'); +if (process.env.HARPER_RUNNER_MODE === 'teardown') { + await killHarper({ harper: { process: result.process } }, { graceMs: 2000 }); + process.stdout.write('teardown-complete\\n'); +} else { + setInterval(() => {}, 1000); +} +`, }; +const isPosix = process.platform !== 'win32'; + let fixtureDir: string; const fixtures: Record = {}; @@ -67,13 +124,17 @@ function waitForOutput(child: ChildProcess, needle: string): Promise { } /** Resolves with the regex match once it appears on the child's stdout. */ -function waitForMatch(child: ChildProcess, regex: RegExp): Promise { - return new Promise((resolve) => { +function waitForMatch(child: ChildProcess, regex: RegExp, timeoutMs = 5000): Promise { + return new Promise((resolve, reject) => { let buffer = ''; + const timeout = setTimeout(() => reject(new Error(`Timed out waiting for ${regex}; output: ${buffer}`)), timeoutMs); child.stdout?.on('data', (chunk: Buffer) => { buffer += chunk.toString(); const matched = buffer.match(regex); - if (matched) resolve(matched); + if (matched) { + clearTimeout(timeout); + resolve(matched); + } }); }); } @@ -92,6 +153,76 @@ async function waitProcessGone(pid: number, timeoutMs: number): Promise } } +async function getFreePorts(count: number): Promise { + const servers = await Promise.all( + Array.from({ length: count }, () => + new Promise>((resolve, reject) => { + const server = createServer(); + server.once('error', reject); + server.listen(0, '127.0.0.1', () => resolve(server)); + }) + ) + ); + const ports = servers.map((server) => (server.address() as AddressInfo).port); + await Promise.all(servers.map((server) => new Promise((resolve) => server.close(() => resolve())))); + return ports; +} + +interface RunningFakeHarperTree { + runner: ChildProcess; + supervisorPid: number; + harperPid: number; + descendantPid: number; + ports: number[]; +} + +async function startFakeHarperTree(mode?: 'teardown'): Promise { + const ports = await getFreePorts(2); + const runner = spawn(process.execPath, [fixtures['orphan-runner.mjs']], { + env: { + ...process.env, + HARPER_LIFECYCLE_URL: pathToFileURL(join(process.cwd(), 'src/harperLifecycle.ts')).href, + HARPER_FAKE_SCRIPT: fixtures['process-tree.cjs'], + HARPER_PORT: String(ports[0]), + HARPER_DESCENDANT_PORT: String(ports[1]), + HARPER_TERM_DELAY_MS: mode === 'teardown' ? '300' : '0', + HARPER_RUNNER_MODE: mode, + }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + let match: RegExpMatchArray; + try { + match = await waitForMatch(runner, /runner-ready:(\d+):(\d+):(\d+)/); + } catch (error) { + forceKill(runner.pid); + throw error; + } + return { + runner, + supervisorPid: Number(match[1]), + harperPid: Number(match[2]), + descendantPid: Number(match[3]), + ports, + }; +} + +function forceKill(pid: number | undefined): void { + if (pid === undefined) return; + try { + process.kill(pid, 'SIGKILL'); + } catch { + // The process already exited. + } +} + +async function cleanupFakeHarperTree(tree: Partial): Promise { + forceKill(tree.runner?.pid); + forceKill(tree.supervisorPid); + forceKill(tree.harperPid); + forceKill(tree.descendantPid); + if (tree.ports) await waitForPortsFree('127.0.0.1', tree.ports, 2000, 50); +} + // --- Startup watchdog (Race 1) --- test('runHarperCommand resolves when the completion message appears', async () => { @@ -152,7 +283,7 @@ test('runHarperCommand keeps a slow-but-progressing boot alive past the idle win env: {}, completionMessage: 'successfully started', harperBinPath: fixtures['idle-reset.cjs'], - timeoutMs: 400, + timeoutMs: 700, maxMs: 10000, }); try { @@ -162,6 +293,64 @@ test('runHarperCommand keeps a slow-but-progressing boot alive past the idle win } }); +test('runner SIGKILL reaps the supervised Harper tree and releases its ports', async () => { + const tree = await startFakeHarperTree(); + try { + strictEqual(await isPortFree('127.0.0.1', tree.ports[0]), false); + strictEqual(await isPortFree('127.0.0.1', tree.ports[1]), false); + tree.runner.kill('SIGKILL'); + await once(tree.runner, 'exit'); + ok(await waitProcessGone(tree.harperPid, 5000), `Harper ${tree.harperPid} should die with its runner`); + ok(await waitProcessGone(tree.descendantPid, 5000), `descendant ${tree.descendantPid} should die with its runner`); + ok(await waitForPortsFree('127.0.0.1', tree.ports, 5000, 50), 'Harper tree ports should be reusable'); + } finally { + await cleanupFakeHarperTree(tree); + } +}); + +test('runner SIGHUP reaps the supervised Harper tree and releases its ports', { skip: !isPosix }, async () => { + const tree = await startFakeHarperTree(); + try { + tree.runner.kill('SIGHUP'); + await once(tree.runner, 'exit'); + ok(await waitProcessGone(tree.harperPid, 5000), `Harper ${tree.harperPid} should die on runner SIGHUP`); + ok(await waitProcessGone(tree.descendantPid, 5000), `descendant ${tree.descendantPid} should die on runner SIGHUP`); + ok(await waitForPortsFree('127.0.0.1', tree.ports, 5000, 50), 'Harper tree ports should be reusable'); + } finally { + await cleanupFakeHarperTree(tree); + } +}); + +test('unexpected supervisor death falls back to reaping the Harper tree', async () => { + const tree = await startFakeHarperTree(); + try { + forceKill(tree.supervisorPid); + ok(await waitProcessGone(tree.harperPid, 5000), `Harper ${tree.harperPid} should die with its supervisor`); + ok(await waitProcessGone(tree.descendantPid, 5000), `descendant ${tree.descendantPid} should die with its supervisor`); + ok(await waitForPortsFree('127.0.0.1', tree.ports, 5000, 50), 'Harper tree ports should be reusable'); + } finally { + await cleanupFakeHarperTree(tree); + } +}); + +test('killHarper waits for supervised Harper shutdown and does not keep the runner alive', { skip: !isPosix }, async () => { + const startedAt = Date.now(); + const tree = await startFakeHarperTree('teardown'); + try { + await waitForMatch(tree.runner, /teardown-complete/); + await Promise.race([ + once(tree.runner, 'exit'), + sleep(3000).then(() => { + throw new Error('Runner stayed alive after supervised teardown'); + }), + ]); + ok(Date.now() - startedAt >= 250, 'killHarper should wait for Harper to finish its delayed SIGTERM handler'); + ok(await waitForPortsFree('127.0.0.1', tree.ports, 2000, 50), 'ports should be free when killHarper resolves'); + } finally { + await cleanupFakeHarperTree(tree); + } +}); + test('runHarperCommand rejects when the process exits non-zero', async () => { await rejects( runHarperCommand({ @@ -186,8 +375,6 @@ test('runHarperCommand rejects when the process exits non-zero', async () => { // leader), so killHarper's group signal (negative PID) targets the whole tree. Windows has no // process groups — killHarper uses `taskkill /T` there — and no real POSIX signals, so the // signal-specific assertions below are guarded to POSIX. -const isPosix = process.platform !== 'win32'; - test('killHarper terminates a process that exits on SIGTERM, before the grace deadline', async () => { const child = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { detached: isPosix }); await once(child, 'spawn'); From 3ac968cd56675f72b7cb10905b383122e4fcc3e3 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 21 Aug 2026 07:57:42 -0600 Subject: [PATCH 02/14] fix(lifecycle): harden supervisor handoff Co-Authored-By: GPT-5 Codex --- README.md | 2 +- src/harperLifecycle.ts | 47 ++++++++++++++++++------------------ src/harperSupervisor.ts | 8 +++--- test/harperLifecycle.test.ts | 21 ++++++++-------- 4 files changed, 39 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index 5f770fd..f46fd58 100644 --- a/README.md +++ b/README.md @@ -100,7 +100,7 @@ The Harper binary is resolved in the following order: ```ts interface StartHarperOptions { - startupTimeoutMs?: number; // Idle timeout: max gap between startup output chunks. Default: 60000 (150000 under CI) or HARPER_INTEGRATION_TEST_STARTUP_TIMEOUT_MS + startupTimeoutMs?: number; // Idle timeout: max gap between startup output chunks, including supervisor launch before the first chunk. Default: 60000 (150000 under CI) or HARPER_INTEGRATION_TEST_STARTUP_TIMEOUT_MS startupMaxMs?: number; // Absolute startup ceiling regardless of output. Default: 120000 (300000 under CI) or HARPER_INTEGRATION_TEST_STARTUP_MAX_MS config?: object; // Harper config overrides (passed via HARPER_SET_CONFIG) env?: object; // Additional environment variables for the Harper process diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index d0ccdeb..67b070d 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -121,7 +121,8 @@ export const LOG_DIR_MARKER_PREFIX = '[Harper] Logs for this instance will be st export interface StartHarperOptions { /** * Maximum time (ms) to wait between chunks of startup output before treating Harper as hung. - * Resets on every chunk of output, so it bounds silence, not total boot time. + * Resets on every chunk of output, so it bounds silence, including the supervisor launch before + * Harper emits its first chunk, rather than total boot time. * Falls back to {@link DEFAULT_STARTUP_TIMEOUT_MS} (60s locally, 150s under CI). */ startupTimeoutMs?: number; @@ -461,10 +462,11 @@ export function runHarperCommand({ const succeed = () => { if (settled || readinessDetected) return; readinessDetected = true; - clearTimers(); + clearTimeout(idleTimer); void trackedProcess.harperPidReady.then((harperPid) => { if (settled) return; settled = true; + clearTimers(); resolve({ process: proc, harperPid, stdout, stderr }); }); }; @@ -521,11 +523,16 @@ export function runHarperCommand({ reject(error); }); proc.on('exit', (statusCode, signal) => { - clearTimers(); if (!settled) { settled = true; + clearTimers(); if (statusCode === 0) { - void trackedProcess.harperPidReady.then((harperPid) => resolve({ process: proc, harperPid, stdout, stderr })); + if (trackedProcess.harperPid === undefined) { + const errorMessage = 'Harper supervisor exited without reporting the Harper PID'; + reject(new HarperStartupError(errorMessage, stdout, stderr)); + } else { + resolve({ process: proc, harperPid: trackedProcess.harperPid, stdout, stderr }); + } } else { const errorMessage = `Harper process failed with exit code/signal ${statusCode ?? signal}`; stderrStream?.write(errorMessage); @@ -673,25 +680,19 @@ export async function startHarper(ctx: HarperTestContext, options?: StartHarperO return ctx as StartedHarperTestContext; } -/** - * Signals Harper's entire process tree, not just the direct child. - * - * Harper runs its listeners in worker threads (in-process), but components can also spawn - * child processes, so we target the whole tree to be safe: - * - POSIX: Harper is spawned as its own process-group leader (`detached`), so a negative PID - * signals the group (parent + descendants). - * - Windows: there are no process groups, so we shell out to `taskkill /T` (tree). `/F` (force) - * is required to actually terminate console processes, so it is used for the SIGKILL step. - * - * Best-effort: errors (e.g. the process already exited) are swallowed; teardown's port assertion - * and the wait-for-exit are the safety nets. - */ function signalWindowsProcessTree(pid: number, signal: 'SIGTERM' | 'SIGKILL'): void { const args = ['/pid', String(pid), '/T']; if (signal === 'SIGKILL') args.push('/F'); spawn('taskkill', args, { stdio: 'ignore' }).on('error', () => {}); } +/** + * Signals Harper's entire process tree, not just the lifecycle process. + * + * The supervisor is the detached POSIX process-group leader, with Harper and its descendants in + * that group. Windows has no process groups, so `taskkill /T` terminates the supervisor's tree. + * Errors are best-effort because the target may already have exited. + */ function signalHarperTree(proc: ChildProcess, signal: 'SIGTERM' | 'SIGKILL'): void { const pid = proc.pid; if (pid === undefined) return; @@ -723,8 +724,8 @@ function signalHarperTree(proc: ChildProcess, signal: 'SIGTERM' | 'SIGKILL'): vo /** * Tracks live Harper processes so that if the test runner exits or is interrupted before teardown * runs (Ctrl+C, or a CI job killing the runner), their process trees are reaped instead of left - * orphaned holding the fixed ports. This matters specifically because Harper is spawned `detached` - * (its own process group), so it would otherwise survive signals delivered to the runner's group. + * orphaned holding the fixed ports. The detached supervisor group would otherwise survive signals + * delivered to the runner's group. * * The supervisor's liveness pipe handles uncatchable runner death. These parent-side hooks provide * immediate cleanup for cooperative exits and retain a fallback for unexpected supervisor death. @@ -755,11 +756,9 @@ function trackHarperProcess(proc: ChildProcess): TrackedHarperProcess { }); proc.channel?.unref(); proc.once('exit', () => { - if (!trackedProcess.harperExited) { - signalHarperTree(proc, 'SIGKILL'); - if (process.platform === 'win32' && trackedProcess.harperPid !== undefined) { - signalWindowsProcessTree(trackedProcess.harperPid, 'SIGKILL'); - } + signalHarperTree(proc, 'SIGKILL'); + if (process.platform === 'win32' && !trackedProcess.harperExited && trackedProcess.harperPid !== undefined) { + signalWindowsProcessTree(trackedProcess.harperPid, 'SIGKILL'); } trackedProcess.livenessPipe.destroy(); liveHarperProcesses.delete(proc); diff --git a/src/harperSupervisor.ts b/src/harperSupervisor.ts index c766b8e..0729495 100644 --- a/src/harperSupervisor.ts +++ b/src/harperSupervisor.ts @@ -45,9 +45,7 @@ function terminateHarperTree(): never { if (harperPid !== undefined) { try { process.kill(harperPid, 'SIGKILL'); - } catch { - // The Harper process already exited. - } + } catch {} } } process.exit(SUPERVISOR_FAILURE_EXIT_CODE); @@ -85,7 +83,9 @@ process.channel?.unref(); let spawnedHarperProcess: ChildProcess; try { - spawnedHarperProcess = spawn(runtime, runtimeArgs, { stdio: ['inherit', 'inherit', 'inherit'] }); + spawnedHarperProcess = spawn(runtime, runtimeArgs, { + stdio: ['inherit', 'inherit', 'inherit', 'ignore', 'ignore'], + }); } catch (error) { reportUnexpectedFailure(error); } diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 12e2f72..dd102f9 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -5,7 +5,6 @@ import { once } from 'node:events'; import { mkdtempSync, writeFileSync, rmSync, existsSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { pathToFileURL } from 'node:url'; import { createServer, type AddressInfo } from 'node:net'; import { setTimeout as sleep } from 'node:timers/promises'; import { @@ -80,10 +79,12 @@ const result = await runHarperCommand({ }); const match = result.stdout.match(/tree-ready:(\\d+):(\\d+)/); if (!match) throw new Error('Missing fake Harper process markers: ' + result.stdout); -process.stdout.write('runner-ready:' + result.process.pid + ':' + match[1] + ':' + match[2] + '\\n'); +process.stdout.write('runner-ready:' + result.process.pid + ':' + result.harperPid + ':' + match[1] + ':' + match[2] + '\\n'); if (process.env.HARPER_RUNNER_MODE === 'teardown') { await killHarper({ harper: { process: result.process } }, { graceMs: 2000 }); - process.stdout.write('teardown-complete\\n'); + let harperGone = false; + try { process.kill(result.harperPid, 0); } catch { harperGone = true; } + process.stdout.write('teardown-complete:' + harperGone + '\\n'); } else { setInterval(() => {}, 1000); } @@ -181,7 +182,7 @@ async function startFakeHarperTree(mode?: 'teardown'): Promise { - const startedAt = Date.now(); const tree = await startFakeHarperTree('teardown'); try { - await waitForMatch(tree.runner, /teardown-complete/); + await waitForMatch(tree.runner, /teardown-complete:true/); await Promise.race([ once(tree.runner, 'exit'), sleep(3000).then(() => { throw new Error('Runner stayed alive after supervised teardown'); }), ]); - ok(Date.now() - startedAt >= 250, 'killHarper should wait for Harper to finish its delayed SIGTERM handler'); ok(await waitForPortsFree('127.0.0.1', tree.ports, 2000, 50), 'ports should be free when killHarper resolves'); } finally { await cleanupFakeHarperTree(tree); From 0385ac047cb4d865356652d52c555de02141def2 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 21 Aug 2026 08:00:40 -0600 Subject: [PATCH 03/14] docs(lifecycle): describe supervisor process group Co-Authored-By: GPT-5 Codex --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index f46fd58..203deed 100644 --- a/README.md +++ b/README.md @@ -125,7 +125,7 @@ Like `startHarper()`, but copies a component directory into the Harper install b ### `killHarper(ctx, options?)` -Terminates Harper's whole process tree and waits for it to exit. It sends SIGTERM first, giving Harper a grace period to shut down cleanly (flush RocksDB, release ports, reap workers) before escalating to SIGKILL, then waits briefly for the actual exit. Because Harper is spawned as its own process group (`detached` on POSIX), the signal targets the group — parent and any child processes — rather than only the direct child; on Windows it uses `taskkill /T`. A dead process releases its listening sockets, so once `killHarper` returns the fixed ports are free. Does not release the loopback address or clean up the install directory. Useful for restart scenarios where the test will call `startHarper` again. +Terminates Harper's whole process tree and waits for it to exit. It sends SIGTERM first, giving Harper a grace period to shut down cleanly (flush RocksDB, release ports, reap workers) before escalating to SIGKILL, then waits briefly for the actual exit. The lifecycle supervisor is the detached POSIX process-group leader, with Harper and its children in that group; on Windows the harness uses `taskkill /T`. A dead process releases its listening sockets, so once `killHarper` returns the fixed ports are free. Does not release the loopback address or clean up the install directory. Useful for restart scenarios where the test will call `startHarper` again. `options.graceMs` overrides the SIGTERM→SIGKILL grace period (default `5000`, or `HARPER_INTEGRATION_TEST_TEARDOWN_GRACE_MS`). From 75d3cbec5e6a6f0aff21c9b8eaec7db10d2a5e1f Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Fri, 21 Aug 2026 08:04:59 -0600 Subject: [PATCH 04/14] test(lifecycle): remove port allocation race Co-Authored-By: GPT-5 Codex --- src/harperSupervisor.ts | 15 ++++++++++---- test/harperLifecycle.test.ts | 40 +++++++++--------------------------- 2 files changed, 21 insertions(+), 34 deletions(-) diff --git a/src/harperSupervisor.ts b/src/harperSupervisor.ts index 0729495..506a4da 100644 --- a/src/harperSupervisor.ts +++ b/src/harperSupervisor.ts @@ -23,10 +23,15 @@ function sendToRunner(message: SupervisorMessage, callback?: () => void): void { callback?.(); return; } - process.send(message, (error: Error | null) => { - if (error) process.stderr.write(`[harper-supervisor] Failed to notify runner: ${error.message}\n`); + try { + process.send(message, (error: Error | null) => { + if (error) process.stderr.write(`[harper-supervisor] Failed to notify runner: ${error.message}\n`); + callback?.(); + }); + } catch (error) { + process.stderr.write(`[harper-supervisor] Failed to notify runner: ${(error as Error).message}\n`); callback?.(); - }); + } } function terminateHarperTree(): never { @@ -45,7 +50,9 @@ function terminateHarperTree(): never { if (harperPid !== undefined) { try { process.kill(harperPid, 'SIGKILL'); - } catch {} + } catch { + // The Harper process already exited. + } } } process.exit(SUPERVISOR_FAILURE_EXIT_CODE); diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index dd102f9..821d922 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -5,7 +5,6 @@ import { once } from 'node:events'; import { mkdtempSync, writeFileSync, rmSync, existsSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { createServer, type AddressInfo } from 'node:net'; import { setTimeout as sleep } from 'node:timers/promises'; import { killHarper, @@ -41,8 +40,8 @@ const { createServer } = require('node:net'); const host = '127.0.0.1'; if (process.env.HARPER_FAKE_DESCENDANT === '1') { const server = createServer(); - server.listen(Number(process.env.HARPER_DESCENDANT_PORT), host, () => { - process.stdout.write('descendant-ready:' + process.pid + '\\n'); + server.listen(0, host, () => { + process.stdout.write('descendant-ready:' + process.pid + ':' + server.address().port + '\\n'); }); } else { const descendant = spawn(process.execPath, [__filename], { @@ -52,10 +51,11 @@ if (process.env.HARPER_FAKE_DESCENDANT === '1') { let descendantOutput = ''; descendant.stdout.on('data', (chunk) => { descendantOutput += chunk; - if (!descendantOutput.includes('descendant-ready:')) return; + const descendantMatch = descendantOutput.match(/descendant-ready:(\\d+):(\\d+)/); + if (!descendantMatch) return; const server = createServer(); - server.listen(Number(process.env.HARPER_PORT), host, () => { - process.stdout.write('tree-ready:' + process.pid + ':' + descendant.pid + '\\n'); + server.listen(0, host, () => { + process.stdout.write('tree-ready:' + process.pid + ':' + descendant.pid + ':' + server.address().port + ':' + descendantMatch[2] + '\\n'); process.stdout.write('successfully started\\n'); }); const delay = Number(process.env.HARPER_TERM_DELAY_MS || 0); @@ -68,8 +68,6 @@ const { runHarperCommand, killHarper } = await import(process.env.HARPER_LIFECYC const result = await runHarperCommand({ args: [], env: { - HARPER_PORT: process.env.HARPER_PORT, - HARPER_DESCENDANT_PORT: process.env.HARPER_DESCENDANT_PORT, HARPER_TERM_DELAY_MS: process.env.HARPER_TERM_DELAY_MS, }, completionMessage: 'successfully started', @@ -77,9 +75,9 @@ const result = await runHarperCommand({ timeoutMs: 5000, maxMs: 10000, }); -const match = result.stdout.match(/tree-ready:(\\d+):(\\d+)/); +const match = result.stdout.match(/tree-ready:(\\d+):(\\d+):(\\d+):(\\d+)/); if (!match) throw new Error('Missing fake Harper process markers: ' + result.stdout); -process.stdout.write('runner-ready:' + result.process.pid + ':' + result.harperPid + ':' + match[1] + ':' + match[2] + '\\n'); +process.stdout.write('runner-ready:' + result.process.pid + ':' + result.harperPid + ':' + match[1] + ':' + match[2] + ':' + match[3] + ':' + match[4] + '\\n'); if (process.env.HARPER_RUNNER_MODE === 'teardown') { await killHarper({ harper: { process: result.process } }, { graceMs: 2000 }); let harperGone = false; @@ -154,21 +152,6 @@ async function waitProcessGone(pid: number, timeoutMs: number): Promise } } -async function getFreePorts(count: number): Promise { - const servers = await Promise.all( - Array.from({ length: count }, () => - new Promise>((resolve, reject) => { - const server = createServer(); - server.once('error', reject); - server.listen(0, '127.0.0.1', () => resolve(server)); - }) - ) - ); - const ports = servers.map((server) => (server.address() as AddressInfo).port); - await Promise.all(servers.map((server) => new Promise((resolve) => server.close(() => resolve())))); - return ports; -} - interface RunningFakeHarperTree { runner: ChildProcess; supervisorPid: number; @@ -178,14 +161,11 @@ interface RunningFakeHarperTree { } async function startFakeHarperTree(mode?: 'teardown'): Promise { - const ports = await getFreePorts(2); const runner = spawn(process.execPath, [fixtures['orphan-runner.mjs']], { env: { ...process.env, HARPER_LIFECYCLE_URL: new URL('../src/harperLifecycle.ts', import.meta.url).href, HARPER_FAKE_SCRIPT: fixtures['process-tree.cjs'], - HARPER_PORT: String(ports[0]), - HARPER_DESCENDANT_PORT: String(ports[1]), HARPER_TERM_DELAY_MS: mode === 'teardown' ? '300' : '0', HARPER_RUNNER_MODE: mode, }, @@ -193,7 +173,7 @@ async function startFakeHarperTree(mode?: 'teardown'): Promise Date: Mon, 24 Aug 2026 09:21:46 -0600 Subject: [PATCH 05/14] refactor(lifecycle): reap orphans with a shared monitor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the per-instance supervisor sidecar with a single monitor process shared by every concurrent test runner on the machine. Harper is spawned directly again (its own detached process group), so `ctx.harper.process` is the Harper process itself and the public API is unchanged from main. Each instance publishes explicit metadata — PID, process start time, owning runner, loopback address, lifetime deadline — to an on-disk registry, and `startHarper` ensures exactly one monitor is running for that registry. The monitor scans on an interval and terminates the process group of any instance whose owning runner is gone or which has outlived its budget, then shuts down once the registry has been empty for a while. PID plus start time identifies each process, so a recycled PID is never mistaken for a live one. The trade against the supervisor: reaping takes one scan interval rather than a pipe EOF, in exchange for one process per machine instead of one per instance, plus coverage of stale instances left by earlier runs. POSIX only — reaping relies on process groups. On Windows registration is skipped and the runner-side cleanup handlers are the only protection. Refs #29 Co-Authored-By: Claude Opus 5 --- CONTRIBUTING.md | 2 +- README.md | 38 +++- src/harperInstanceRegistry.ts | 347 ++++++++++++++++++++++++++++++++++ src/harperLifecycle.ts | 138 ++++++-------- src/harperMonitor.ts | 192 +++++++++++++++++++ src/harperSupervisor.ts | 121 ------------ test/harperLifecycle.test.ts | 180 +++++++++++++++--- 7 files changed, 783 insertions(+), 235 deletions(-) create mode 100644 src/harperInstanceRegistry.ts create mode 100644 src/harperMonitor.ts delete mode 100644 src/harperSupervisor.ts diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6eb85f6..81541e3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -6,7 +6,7 @@ Contributors are encouraged to communicate with maintainers in issues or other c Source files are located in `src/`. These are built to the `dist/` directory. The published package includes `dist/`, `scripts/`, and the regular npm metadata and documentation files. -The `src/index.ts` is the source for the main export. This is the public re-export of all the various utilities from `src/harperLifecycle.ts`, `targz.ts`, and more. The `src/run.ts` is the source for the `harper-integration-test-run` bin script. The internal `src/harperSupervisor.ts` process owns the detached Harper group and watches a runner-liveness pipe so hard runner death cannot orphan Harper. And the `scripts/setup-loopback.sh` is the source for the `harper-integration-test-setup-loopback` bin script. +The `src/index.ts` is the source for the main export. This is the public re-export of all the various utilities from `src/harperLifecycle.ts`, `targz.ts`, and more. The `src/run.ts` is the source for the `harper-integration-test-run` bin script. The internal `src/harperInstanceRegistry.ts` publishes each running Harper instance to a shared on-disk registry, and `src/harperMonitor.ts` is the singleton monitor process that reads it and reaps instances whose test runner died without cleaning up (see README's *Orphaned Instance Monitor* section). And the `scripts/setup-loopback.sh` is the source for the `harper-integration-test-setup-loopback` bin script. The package is `"type": "module"` — all source files are ESM by default. diff --git a/README.md b/README.md index 203deed..d5b0f3b 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ The lifecycle and utility APIs below are framework-agnostic. They manage Harper Allocates a loopback address from the pool, creates a temporary install directory, starts a Harper process, and waits for it to be ready. Populates `ctx.harper` with the instance details. Call in a setup/`before()` hook. -The harness starts Harper behind a small lifecycle supervisor. A private pipe lets the supervisor detect runner death even when JavaScript cleanup cannot run (for example `SIGKILL` or a hard crash), then terminate Harper's detached process tree so it cannot remain orphaned holding ports. `ctx.harper.process` is the supervisor-backed lifecycle handle used by `killHarper`; use `ctx.harper.harperPid` when the Harper runtime's own PID is specifically needed. +Harper runs as its own detached process group, so a runner that dies without running cleanup would otherwise leave it alive holding the fixed ports. On POSIX the instance is registered with a shared [instance monitor](#orphaned-instance-monitor-posix) that reaps it in that case. The Harper binary is resolved in the following order: @@ -100,7 +100,7 @@ The Harper binary is resolved in the following order: ```ts interface StartHarperOptions { - startupTimeoutMs?: number; // Idle timeout: max gap between startup output chunks, including supervisor launch before the first chunk. Default: 60000 (150000 under CI) or HARPER_INTEGRATION_TEST_STARTUP_TIMEOUT_MS + startupTimeoutMs?: number; // Idle timeout: max gap between startup output chunks. Default: 60000 (150000 under CI) or HARPER_INTEGRATION_TEST_STARTUP_TIMEOUT_MS startupMaxMs?: number; // Absolute startup ceiling regardless of output. Default: 120000 (300000 under CI) or HARPER_INTEGRATION_TEST_STARTUP_MAX_MS config?: object; // Harper config overrides (passed via HARPER_SET_CONFIG) env?: object; // Additional environment variables for the Harper process @@ -125,7 +125,7 @@ Like `startHarper()`, but copies a component directory into the Harper install b ### `killHarper(ctx, options?)` -Terminates Harper's whole process tree and waits for it to exit. It sends SIGTERM first, giving Harper a grace period to shut down cleanly (flush RocksDB, release ports, reap workers) before escalating to SIGKILL, then waits briefly for the actual exit. The lifecycle supervisor is the detached POSIX process-group leader, with Harper and its children in that group; on Windows the harness uses `taskkill /T`. A dead process releases its listening sockets, so once `killHarper` returns the fixed ports are free. Does not release the loopback address or clean up the install directory. Useful for restart scenarios where the test will call `startHarper` again. +Terminates Harper's whole process tree and waits for it to exit. It sends SIGTERM first, giving Harper a grace period to shut down cleanly (flush RocksDB, release ports, reap workers) before escalating to SIGKILL, then waits briefly for the actual exit. Because Harper is spawned as its own process group (`detached` on POSIX), the signal targets the group — parent and any child processes — rather than only the direct child; on Windows it uses `taskkill /T`. A dead process releases its listening sockets, so once `killHarper` returns the fixed ports are free. Does not release the loopback address or clean up the install directory. Useful for restart scenarios where the test will call `startHarper` again. `options.graceMs` overrides the SIGTERM→SIGKILL grace period (default `5000`, or `HARPER_INTEGRATION_TEST_TEARDOWN_GRACE_MS`). @@ -167,8 +167,7 @@ interface HarperContext { httpURL: string; // e.g. 'http://127.0.0.2:9926' operationsAPIURL: string; // e.g. 'http://127.0.0.2:9925' hostname: string; // e.g. '127.0.0.2' - process: ChildProcess; // supervisor-backed lifecycle handle; pass to lifecycle APIs - harperPid?: number; // PID of the managed Harper runtime + process: ChildProcess; logDir?: string; // set when HARPER_INTEGRATION_TEST_LOG_DIR is configured startupOutput?: { stdout: string; stderr: string }; // captured startup output } @@ -207,6 +206,35 @@ suite('my suite', (ctx: ContextWithHarper) => { If you are not using `node:test`, use `createHarperContext()` to create a plain `HarperTestContext` instead. +### Orphaned Instance Monitor (POSIX) + +A test runner that dies without running its teardown — `SIGKILL`, a hard crash, a cancelled CI job — cannot reap the Harper instances it started, because those are deliberately detached into their own process groups so that whole-tree teardown works. Left alone, they hold their loopback address's fixed ports until the machine is rebooted. + +To close that gap, `startHarper()` registers each instance in a small on-disk registry and makes sure a single shared **monitor** process is running. The monitor is not a per-instance sidecar: one is started on demand per registry directory (per machine, by default), every concurrent runner reuses it, and it exits once the registry has been empty for a while. It scans the registry on an interval and terminates — `SIGTERM`, then `SIGKILL` after a grace period — the process group of any instance whose owning runner is gone, or which has outlived its lifetime budget. Instances are matched by PID *and* process start time, so a recycled PID is never mistaken for a live one. + +The runner's own `exit`/`SIGINT`/`SIGTERM`/`SIGHUP` handlers still reap instances immediately on any exit it can observe; the monitor only handles the deaths it cannot. + +Registry directory layout (`${TMPDIR}/harper-integration-test-monitor` by default): + +| File | Contents | +| --- | --- | +| `registry.json` | The current monitor and every registered instance (PID, start time, owning runner, loopback address, deadline) | +| `registry.lock` | Cross-process mutex guarding `registry.json` | +| `monitor.log` | Append-only record of monitor start/exit and every reap, with the reason | + +Each managed Harper process also carries `HARPER_IT_KIND=harper-instance`, `HARPER_IT_INSTANCE_ID`, and `HARPER_IT_OWNER_PID` in its environment, and the monitor's command line contains `--harper-integration-test-monitor`, so both are identifiable from `ps` / `/proc` without consulting the registry. + +This is POSIX-only: reaping relies on process groups, which Windows does not have. On Windows, registration is skipped and the runner-side cleanup handlers are the only protection. + +**Environment Variables:** + +- `HARPER_INTEGRATION_TEST_MONITOR` - Set to `off` to disable registration and the monitor entirely. Default on (POSIX only). +- `HARPER_INTEGRATION_TEST_MONITOR_DIR` - Registry directory. Default `${TMPDIR}/harper-integration-test-monitor`. Point separate runs at separate directories to give them separate monitors. +- `HARPER_INTEGRATION_TEST_MONITOR_INTERVAL_MS` - How often the monitor rescans the registry. Default `2000`. +- `HARPER_INTEGRATION_TEST_MONITOR_REAP_GRACE_MS` - Grace period between the monitor's SIGTERM and SIGKILL. Default `5000`. +- `HARPER_INTEGRATION_TEST_MONITOR_IDLE_MS` - How long the registry must stay empty before the monitor shuts down. Default `60000`. +- `HARPER_INTEGRATION_TEST_INSTANCE_MAX_LIFETIME_MS` - Backstop lifetime after which an instance is reaped even if its runner still looks alive. Default `14400000` (4h). + ### Server Log Capture When `HARPER_INTEGRATION_TEST_LOG_DIR` is set, each Harper instance writes its logs to a per-suite subdirectory. Logs are preserved for the lifetime of the log directory. In CI, combine with artifact upload steps that run on failure to capture logs from failing runs. diff --git a/src/harperInstanceRegistry.ts b/src/harperInstanceRegistry.ts new file mode 100644 index 0000000..9968bf2 --- /dev/null +++ b/src/harperInstanceRegistry.ts @@ -0,0 +1,347 @@ +import { spawn, spawnSync } from 'node:child_process'; +import { mkdir, open, readFile, stat, unlink, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { setTimeout as sleep } from 'node:timers/promises'; +import { fileURLToPath } from 'node:url'; + +/** + * Cross-process registry of running Harper test instances, and the singleton monitor process that + * reaps them. + * + * Every Harper instance the harness starts is detached (its own process-group leader) so that a + * test runner dying without running cleanup — `SIGKILL`, a hard crash, a cancelled CI job — leaves + * Harper alive holding its fixed ports. Instead of pairing every instance with its own sidecar, + * each instance publishes explicit metadata here and a single shared monitor (one per registry + * directory, i.e. per machine by default) reaps whatever is orphaned or overdue. + * + * The registry is the contract between the two halves: `harperLifecycle.ts` registers and + * deregisters instances, `harperMonitor.ts` scans and reaps them. Both agree on tunables through + * the environment, which the monitor inherits from whichever runner first spawned it. + * + * POSIX only. Reaping is `kill(-pgid)`, which Windows has no equivalent for; on Windows + * registration is skipped and the runner-side cleanup handlers in `harperLifecycle.ts` remain the + * only protection. + */ + +/** Marker passed on the monitor's command line so `ps -ef | grep` finds it. */ +export const MONITOR_ARGV_MARKER = '--harper-integration-test-monitor'; + +/** Marks the environment of a Harper process the harness owns (visible in `/proc//environ`). */ +export const INSTANCE_ENV_KIND = 'HARPER_IT_KIND'; +export const INSTANCE_ENV_KIND_VALUE = 'harper-instance'; +export const INSTANCE_ENV_ID = 'HARPER_IT_INSTANCE_ID'; +export const INSTANCE_ENV_OWNER_PID = 'HARPER_IT_OWNER_PID'; + +const LOCK_STALE_TIMEOUT_MS = 10000; +const LOCK_RETRY_DELAY_MS = 50; + +function envInt(name: string, fallback: number): number { + const parsed = parseInt(process.env[name] || '', 10); + return Number.isNaN(parsed) || parsed <= 0 ? fallback : parsed; +} + +/** + * Directory holding the registry, its lock, and the monitor log. Resolved on every call rather + * than at module load so a test (or a caller isolating a run) can point it somewhere private + * after import, and so the value the monitor inherits always matches its parent's. + * + * Override with `HARPER_INTEGRATION_TEST_MONITOR_DIR`. + */ +export function getRegistryDir(): string { + return process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR || join(tmpdir(), 'harper-integration-test-monitor'); +} + +export function getRegistryPath(): string { + return join(getRegistryDir(), 'registry.json'); +} + +function getLockPath(): string { + return join(getRegistryDir(), 'registry.lock'); +} + +export function getMonitorLogPath(): string { + return join(getRegistryDir(), 'monitor.log'); +} + +/** How often the monitor rescans the registry. Default 2s. */ +export function getMonitorScanIntervalMs(): number { + return envInt('HARPER_INTEGRATION_TEST_MONITOR_INTERVAL_MS', 2000); +} + +/** How long the registry must stay empty before the monitor shuts itself down. Default 60s. */ +export function getMonitorIdleExitMs(): number { + return envInt('HARPER_INTEGRATION_TEST_MONITOR_IDLE_MS', 60000); +} + +/** Grace period between the monitor's `SIGTERM` and its `SIGKILL` escalation. Default 5s. */ +export function getReapGraceMs(): number { + return envInt('HARPER_INTEGRATION_TEST_MONITOR_REAP_GRACE_MS', 5000); +} + +/** + * Backstop lifetime for a registered instance. The sharp signal is owner death; this only catches + * the pathological case where the runner's PID was recycled by a long-lived process, so it is + * deliberately far longer than any plausible suite. Default 4h. + */ +export function getInstanceMaxLifetimeMs(): number { + return envInt('HARPER_INTEGRATION_TEST_INSTANCE_MAX_LIFETIME_MS', 4 * 60 * 60 * 1000); +} + +/** Whether instances should be registered with a monitor at all. */ +export function isInstanceMonitorEnabled(): boolean { + return process.platform !== 'win32' && process.env.HARPER_INTEGRATION_TEST_MONITOR !== 'off'; +} + +/** Identifies a process well enough to survive PID reuse: the PID plus its wall-clock start time. */ +export interface ProcessIdentity { + pid: number; + /** + * `ps` start time (e.g. `Mon Aug 24 09:09:38 2026`). Absolute wall clock, so it stays valid + * across reboots, unlike the boot-relative tick counts in `/proc`. Undefined when `ps` was + * unavailable, in which case liveness degrades to a plain PID check. + */ + startTime?: string; +} + +export interface HarperInstanceRecord extends ProcessIdentity { + /** Stable id, also exported to the process as `HARPER_IT_INSTANCE_ID`. */ + id: string; + /** The test runner that started this instance; its death is what makes the instance an orphan. */ + owner: ProcessIdentity; + /** Loopback address the instance was assigned, for diagnostics in the monitor log. */ + hostname?: string; + registeredAt: number; + /** Wall-clock deadline after which the monitor reaps the instance regardless of owner liveness. */ + expiresAt: number; +} + +export interface InstanceRegistry { + /** The monitor currently responsible for this registry, if any. */ + monitor?: ProcessIdentity; + instances: HarperInstanceRecord[]; +} + +/** + * Reads the wall-clock start time of each PID in a single `ps` call. + * + * Absence from the result means the PID does not exist. A present-but-different value means the + * PID was recycled — the case that makes a bare `kill(-pgid)` dangerous, since the group we + * recorded may now belong to something unrelated. + */ +export function readProcessStartTimes(pids: number[]): Map { + const startTimes = new Map(); + const uniquePids = [...new Set(pids)].filter((pid) => Number.isInteger(pid) && pid > 0); + if (uniquePids.length === 0) return startTimes; + const result = spawnSync('ps', ['-o', 'pid=,lstart=', '-p', uniquePids.join(',')], { encoding: 'utf8' }); + // A non-zero status just means none of the PIDs exist; only a missing `ps` is worth noticing, + // and there the empty map degrades callers to a plain PID check rather than reporting deaths. + if (result.error || typeof result.stdout !== 'string') return startTimes; + for (const line of result.stdout.split('\n')) { + const parsed = line.trim().match(/^(\d+)\s+(.*\S)$/); + if (parsed) startTimes.set(Number(parsed[1]), parsed[2]); + } + return startTimes; +} + +export function readProcessIdentity(pid: number): ProcessIdentity { + return { pid, startTime: readProcessStartTimes([pid]).get(pid) }; +} + +/** + * Whether `identity` still refers to the same live process, given start times already collected + * for this scan. When `ps` produced nothing at all (no start times for any PID) we cannot + * distinguish "gone" from "unmeasurable", so fall back to a signal-0 existence check. + */ +export function isSameProcessAlive(identity: ProcessIdentity, startTimes: Map): boolean { + const currentStartTime = startTimes.get(identity.pid); + if (currentStartTime === undefined) return startTimes.size === 0 && pidExists(identity.pid); + return identity.startTime === undefined || identity.startTime === currentStartTime; +} + +function pidExists(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + // EPERM means the process exists but belongs to someone else — still alive. + return (error as NodeJS.ErrnoException).code === 'EPERM'; + } +} + +/** + * Acquires the registry lock, mirroring the `wx` create-or-fail mutex the loopback pool uses. + * Critical sections here are a read/modify/write of one small file, so the retry delay is much + * shorter than the pool's. + */ +async function acquireLock(): Promise { + const lockPath = getLockPath(); + await mkdir(getRegistryDir(), { recursive: true }); + while (true) { + try { + const lockFileHandle = await open(lockPath, 'wx'); + await lockFileHandle.close(); + return; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; + try { + const lockFileStat = await stat(lockPath); + // A holder that died mid-section would otherwise wedge every runner on the machine. + if (Date.now() - lockFileStat.mtimeMs > LOCK_STALE_TIMEOUT_MS) await unlink(lockPath); + } catch { + // Another process removed it first; just retry. + } + await sleep(LOCK_RETRY_DELAY_MS); + } + } +} + +export async function withRegistryLock(callback: () => Promise): Promise { + await acquireLock(); + try { + return await callback(); + } finally { + try { + await unlink(getLockPath()); + } catch { + // Already released (e.g. reclaimed as stale). + } + } +} + +/** Reads the registry. Only call while holding the lock; a torn or absent file reads as empty. */ +export async function readRegistryFile(): Promise { + try { + const parsed = JSON.parse(await readFile(getRegistryPath(), 'utf-8')) as InstanceRegistry; + return { monitor: parsed.monitor, instances: Array.isArray(parsed.instances) ? parsed.instances : [] }; + } catch { + return { instances: [] }; + } +} + +/** Writes the registry. Only call while holding the lock. */ +export async function writeRegistryFile(registry: InstanceRegistry): Promise { + await mkdir(getRegistryDir(), { recursive: true }); + await writeFile(getRegistryPath(), JSON.stringify(registry)); +} + +function getMonitorScript(): string { + const extension = import.meta.url.endsWith('.ts') ? 'ts' : 'js'; + return fileURLToPath(new URL(`./harperMonitor.${extension}`, import.meta.url)); +} + +/** + * Starts a monitor. Safe to call speculatively: a monitor that finds a live one already recorded + * in the registry exits immediately, so a race between two runners costs one short-lived process + * rather than a second reaper. + * + * Detached and unref'd so it outlives the runner that happened to start it — the whole point is to + * still be there when that runner dies. + */ +function spawnMonitor(): void { + try { + const monitor = spawn(process.execPath, [getMonitorScript(), MONITOR_ARGV_MARKER], { + detached: true, + stdio: 'ignore', + env: process.env, + }); + monitor.on('error', (error) => { + console.warn(`[harper-monitor] Failed to start the Harper instance monitor: ${error.message}`); + }); + monitor.unref(); + } catch (error) { + console.warn(`[harper-monitor] Failed to start the Harper instance monitor: ${(error as Error).message}`); + } +} + +let instanceCounter = 0; + +/** + * Environment markers identifying a Harper process as harness-owned. The registry is the + * authoritative list; these make an individual process self-describing for anyone inspecting it + * directly (`tr '\0' '\n' < /proc//environ | grep HARPER_IT_`). + */ +export function buildInstanceEnv(instanceId: string): Record { + return { + [INSTANCE_ENV_KIND]: INSTANCE_ENV_KIND_VALUE, + [INSTANCE_ENV_ID]: instanceId, + [INSTANCE_ENV_OWNER_PID]: String(process.pid), + }; +} + +/** Allocates the id used for both the instance's environment markers and its registry record. */ +export function nextInstanceId(): string { + return `${process.pid}-${++instanceCounter}`; +} + +/** + * Registers a running Harper instance with the shared monitor, starting one if none is live. + * + * The liveness check and the insert happen in a single critical section, which is what keeps the + * monitor's idle shutdown from racing a new registration: the monitor only exits while holding the + * lock with an empty registry, so either we see it alive and it sees our instance, or we see it + * gone and start a replacement. + */ +export async function registerHarperInstance(instance: { + id: string; + pid: number; + hostname?: string; +}): Promise { + if (!isInstanceMonitorEnabled()) return; + const startTimes = readProcessStartTimes([instance.pid, process.pid]); + const now = Date.now(); + const record: HarperInstanceRecord = { + id: instance.id, + pid: instance.pid, + startTime: startTimes.get(instance.pid), + owner: { pid: process.pid, startTime: startTimes.get(process.pid) }, + hostname: instance.hostname, + registeredAt: now, + expiresAt: now + getInstanceMaxLifetimeMs(), + }; + + const monitorNeeded = await withRegistryLock(async () => { + const registry = await readRegistryFile(); + registry.instances = registry.instances.filter((existing) => existing.id !== record.id); + registry.instances.push(record); + await writeRegistryFile(registry); + return !(registry.monitor && isSameProcessAlive(registry.monitor, readProcessStartTimes([registry.monitor.pid]))); + }); + + // Outside the lock: the monitor claims its slot under the same lock and would otherwise wait + // out our critical section before it could start. + if (monitorNeeded) spawnMonitor(); +} + +/** + * Removes an instance from the registry after normal teardown. + * + * Best-effort — a record left behind by an abrupt exit is pruned by the monitor as soon as the + * Harper PID is gone, so a missed deregistration costs a log line, not a stale reap target. + */ +export async function deregisterHarperInstance(id: string): Promise { + if (!isInstanceMonitorEnabled()) return; + try { + await withRegistryLock(async () => { + const registry = await readRegistryFile(); + const remaining = registry.instances.filter((instance) => instance.id !== id); + if (remaining.length === registry.instances.length) return; + registry.instances = remaining; + await writeRegistryFile(registry); + }); + } catch (error) { + console.warn(`[harper-monitor] Failed to deregister Harper instance ${id}: ${(error as Error).message}`); + } +} + +/** + * Signals a whole process group. Instances are spawned detached, so the instance PID is also its + * process-group id and this reaches Harper plus anything it spawned. + */ +export function signalProcessGroup(pgid: number, signal: 'SIGTERM' | 'SIGKILL'): void { + try { + process.kill(-pgid, signal); + } catch { + // The group is already gone. + } +} diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index 67b070d..e506eff 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -8,7 +8,12 @@ import { getNextAvailableLoopbackAddress, releaseLoopbackAddress } from './loopb import { waitForPortsFree } from './portUtils.ts'; import { ok, equal } from 'node:assert'; import { createRequire } from 'node:module'; -import { fileURLToPath } from 'node:url'; +import { + buildInstanceEnv, + deregisterHarperInstance, + nextInstanceId, + registerHarperInstance, +} from './harperInstanceRegistry.ts'; /** * Minimal context interface required by startHarper/teardownHarper. @@ -121,8 +126,7 @@ export const LOG_DIR_MARKER_PREFIX = '[Harper] Logs for this instance will be st export interface StartHarperOptions { /** * Maximum time (ms) to wait between chunks of startup output before treating Harper as hung. - * Resets on every chunk of output, so it bounds silence, including the supervisor launch before - * Harper emits its first chunk, rather than total boot time. + * Resets on every chunk of output, so it bounds silence rather than total boot time. * Falls back to {@link DEFAULT_STARTUP_TIMEOUT_MS} (60s locally, 150s under CI). */ startupTimeoutMs?: number; @@ -187,10 +191,8 @@ export interface HarperContext { operationsAPIURL: string; /** Assigned loopback IP address (e.g., '127.0.0.2') */ hostname: string; - /** Lifecycle process handle for the Harper instance (the harness supervisor when started by this package). */ + /** Child process handle for the Harper instance */ process: ChildProcess; - /** PID of the Harper runtime managed by the lifecycle process. */ - harperPid?: number; /** Absolute path to the log directory for this suite (only set when HARPER_INTEGRATION_TEST_LOG_DIR is configured) */ logDir?: string; /** Captured stdout/stderr from Harper startup, up to the point it reported ready. */ @@ -362,34 +364,18 @@ interface RunHarperCommandOptions { timeoutMs?: number; /** Absolute timeout (ms): ceiling on total time regardless of output. Falls back to DEFAULT_STARTUP_MAX_MS. */ maxMs?: number; + /** Loopback address this instance is bound to; recorded with the instance monitor for diagnostics. */ + hostname?: string; } interface RunHarperCommandResult { process: ChildProcess; - harperPid: number; /** Captured stdout up to the point the process was considered ready or exited. */ stdout: string; /** Captured stderr up to the point the process was considered ready or exited. */ stderr: string; } -interface TrackedHarperProcess { - harperPid?: number; - harperExited: boolean; - harperPidReady: Promise; - resolveHarperPid: (pid: number) => void; - livenessPipe: { unref(): void; destroy(): void }; -} - -type SupervisorMessage = - | { type: 'harper-spawn'; pid: number } - | { type: 'harper-exit' }; - -function getHarperSupervisorScript(): string { - const extension = import.meta.url.endsWith('.ts') ? 'ts' : 'js'; - return fileURLToPath(new URL(`./harperSupervisor.${extension}`, import.meta.url)); -} - /** * Runs a Harper CLI command and captures output. * @@ -408,6 +394,7 @@ export function runHarperCommand({ harperBinPath, timeoutMs, maxMs, + hostname, }: RunHarperCommandOptions): Promise { const harperScript = getHarperScript(harperBinPath); const runtime = HARPER_RUNTIME; @@ -415,17 +402,20 @@ export function runHarperCommand({ runtime === 'bun' ? [harperScript, ...args] : ['--trace-warnings', '--force-node-api-uncaught-exceptions-policy=true', harperScript, ...args]; - const proc = spawn(process.execPath, [getHarperSupervisorScript(), runtime, ...runtimeArgs], { - env: { ...process.env, ...env }, - // The supervisor is the POSIX process-group leader, preserving whole-tree teardown while - // its inherited stdio keeps Harper output capture unchanged. Windows uses `taskkill /T`. + const instanceId = nextInstanceId(); + const proc = spawn(runtime, runtimeArgs, { + env: { ...process.env, ...env, ...buildInstanceEnv(instanceId) }, + // On POSIX, run Harper as its own process-group leader so both teardown and the shared + // instance monitor can signal the whole group (parent + any worker children), not just the + // direct child. Windows has no process groups; killHarper uses `taskkill /T` there instead. + // stdio stays piped (not detached), and we never unref, so output capture and lifetime + // management are unchanged. detached: process.platform !== 'win32', - stdio: ['pipe', 'pipe', 'pipe', 'pipe', 'ipc'], }); - // Reap this process's tree if the runner exits/is interrupted before teardown (it's detached, - // so it would otherwise survive signals sent to the runner's group). - const trackedProcess = trackHarperProcess(proc); + // Publishes the instance to the shared monitor, which reaps it if this runner dies without + // running cleanup, and installs the runner-side handlers covering cooperative exits. + const trackedProcess = trackHarperProcess(proc, instanceId, hostname); let stdoutStream: WriteStream | undefined; let stderrStream: WriteStream | undefined; @@ -455,7 +445,7 @@ export function runHarperCommand({ settled = true; clearTimers(); reject(new HarperStartupError(message, stdout, stderr)); - // The supervisor is the detached group leader, so reap the whole group rather than only it. + // Harper is the detached group leader, so reap the whole group rather than only it. signalHarperTree(proc, 'SIGKILL'); }; @@ -463,11 +453,13 @@ export function runHarperCommand({ if (settled || readinessDetected) return; readinessDetected = true; clearTimeout(idleTimer); - void trackedProcess.harperPidReady.then((harperPid) => { + // Resolve only once the instance is durably registered, so a runner killed the moment + // startHarper returns still leaves the monitor a record to act on. + void trackedProcess.registered.then(() => { if (settled) return; settled = true; clearTimers(); - resolve({ process: proc, harperPid, stdout, stderr }); + resolve({ process: proc, stdout, stderr }); }); }; @@ -527,12 +519,7 @@ export function runHarperCommand({ settled = true; clearTimers(); if (statusCode === 0) { - if (trackedProcess.harperPid === undefined) { - const errorMessage = 'Harper supervisor exited without reporting the Harper PID'; - reject(new HarperStartupError(errorMessage, stdout, stderr)); - } else { - resolve({ process: proc, harperPid: trackedProcess.harperPid, stdout, stderr }); - } + resolve({ process: proc, stdout, stderr }); } else { const errorMessage = `Harper process failed with exit code/signal ${statusCode ?? signal}`; stderrStream?.write(errorMessage); @@ -660,6 +647,7 @@ export async function startHarper(ctx: HarperTestContext, options?: StartHarperO harperBinPath: options?.harperBinPath, timeoutMs: options?.startupTimeoutMs, maxMs: options?.startupMaxMs, + hostname: loopbackAddress, }); publishHarperNode(ctx, { @@ -672,7 +660,6 @@ export async function startHarper(ctx: HarperTestContext, options?: StartHarperO operationsAPIURL: `http://${loopbackAddress}:${OPERATIONS_API_PORT}`, hostname: loopbackAddress, process: result.process, - harperPid: result.harperPid, logDir, startupOutput: { stdout: result.stdout, stderr: result.stderr }, }); @@ -687,11 +674,11 @@ function signalWindowsProcessTree(pid: number, signal: 'SIGTERM' | 'SIGKILL'): v } /** - * Signals Harper's entire process tree, not just the lifecycle process. + * Signals Harper's entire process tree, not just the direct child. * - * The supervisor is the detached POSIX process-group leader, with Harper and its descendants in - * that group. Windows has no process groups, so `taskkill /T` terminates the supervisor's tree. - * Errors are best-effort because the target may already have exited. + * Harper is the detached POSIX process-group leader, with anything it spawns in that group, so a + * negative PID reaches the whole tree. Windows has no process groups, so `taskkill /T` is used + * there. Errors are best-effort because the target may already have exited. */ function signalHarperTree(proc: ChildProcess, signal: 'SIGTERM' | 'SIGKILL'): void { const pid = proc.pid; @@ -721,54 +708,47 @@ function signalHarperTree(proc: ChildProcess, signal: 'SIGTERM' | 'SIGKILL'): vo } } +interface TrackedHarperProcess { + /** Settles once the instance is recorded with the shared monitor (or registration was skipped). */ + registered: Promise; +} + /** * Tracks live Harper processes so that if the test runner exits or is interrupted before teardown * runs (Ctrl+C, or a CI job killing the runner), their process trees are reaped instead of left - * orphaned holding the fixed ports. The detached supervisor group would otherwise survive signals - * delivered to the runner's group. + * orphaned holding the fixed ports. Harper is spawned detached (its own process group), so it + * would otherwise survive signals delivered to the runner's group. * - * The supervisor's liveness pipe handles uncatchable runner death. These parent-side hooks provide - * immediate cleanup for cooperative exits and retain a fallback for unexpected supervisor death. + * These parent-side hooks cover every exit the runner can still run JavaScript for. Death it + * cannot observe — `SIGKILL`, a hard crash, a cancelled CI job — is covered by the shared instance + * monitor, which reaps whatever the registry says is orphaned. See `harperInstanceRegistry.ts`. */ -const liveHarperProcesses = new Map(); +const liveHarperProcesses = new Set(); let runnerCleanupRegistered = false; -function trackHarperProcess(proc: ChildProcess): TrackedHarperProcess { - const livenessPipe = proc.stdio[3] as unknown as TrackedHarperProcess['livenessPipe']; - livenessPipe.unref(); - let resolveHarperPid!: (pid: number) => void; - const trackedProcess: TrackedHarperProcess = { - harperExited: false, - harperPidReady: new Promise((resolve) => { - resolveHarperPid = resolve; - }), - resolveHarperPid: (pid) => resolveHarperPid(pid), - livenessPipe, - }; - liveHarperProcesses.set(proc, trackedProcess); - proc.on('message', (message: SupervisorMessage) => { - if (message.type === 'harper-spawn') { - trackedProcess.harperPid = message.pid; - trackedProcess.resolveHarperPid(message.pid); - } else if (message.type === 'harper-exit') { - trackedProcess.harperExited = true; - } - }); - proc.channel?.unref(); +function trackHarperProcess(proc: ChildProcess, instanceId: string, hostname?: string): TrackedHarperProcess { + liveHarperProcesses.add(proc); + // A failed spawn leaves no PID and nothing to reap; the caller rejects on the 'error' event. + const registered = + proc.pid === undefined + ? Promise.resolve() + : registerHarperInstance({ id: instanceId, pid: proc.pid, hostname }).catch((error: Error) => { + console.warn(`[harper-monitor] Failed to register Harper instance ${instanceId}: ${error.message}`); + }); + const trackedProcess: TrackedHarperProcess = { registered }; + proc.once('exit', () => { - signalHarperTree(proc, 'SIGKILL'); - if (process.platform === 'win32' && !trackedProcess.harperExited && trackedProcess.harperPid !== undefined) { - signalWindowsProcessTree(trackedProcess.harperPid, 'SIGKILL'); - } - trackedProcess.livenessPipe.destroy(); liveHarperProcesses.delete(proc); + // Chained on registration so a fast exit cannot deregister before the record exists. Best + // effort either way: the monitor prunes records whose process is gone. + void registered.then(() => deregisterHarperInstance(instanceId)); }); if (runnerCleanupRegistered) return trackedProcess; runnerCleanupRegistered = true; const reapAll = () => { - for (const child of liveHarperProcesses.keys()) signalHarperTree(child, 'SIGKILL'); + for (const child of liveHarperProcesses) signalHarperTree(child, 'SIGKILL'); }; process.once('exit', reapAll); // These signals don't fire 'exit'; reap, then re-raise so the runner still terminates normally. diff --git a/src/harperMonitor.ts b/src/harperMonitor.ts new file mode 100644 index 0000000..3375ff3 --- /dev/null +++ b/src/harperMonitor.ts @@ -0,0 +1,192 @@ +import { existsSync } from 'node:fs'; +import { appendFile } from 'node:fs/promises'; +import { setTimeout as sleep } from 'node:timers/promises'; +import { + getMonitorIdleExitMs, + getMonitorLogPath, + getMonitorScanIntervalMs, + getReapGraceMs, + getRegistryPath, + isSameProcessAlive, + readProcessIdentity, + readProcessStartTimes, + readRegistryFile, + signalProcessGroup, + withRegistryLock, + writeRegistryFile, + type HarperInstanceRecord, +} from './harperInstanceRegistry.ts'; + +/** + * The singleton Harper instance monitor. + * + * One of these runs per registry directory, shared by every concurrent test runner on the machine. + * It periodically scans the registry written by `harperLifecycle.ts` and reaps any instance whose + * owning runner has died or which has outlived its budget, then shuts itself down once the + * registry has been empty long enough. See `harperInstanceRegistry.ts` for the shared contract. + * + * Runs detached with no stdio, so diagnostics go to `monitor.log` in the registry directory. + */ + +const scanIntervalMs = getMonitorScanIntervalMs(); +const idleExitMs = getMonitorIdleExitMs(); +const reapGraceMs = getReapGraceMs(); + +/** Instances already sent `SIGTERM`, mapped to the time we escalate to `SIGKILL`. */ +const escalationDeadlines = new Map(); +let idleSince: number | undefined; +let running = true; + +async function log(message: string): Promise { + try { + await appendFile(getMonitorLogPath(), `${new Date().toISOString()} [${process.pid}] ${message}\n`); + } catch { + // The registry directory is gone; the next loop iteration notices and shuts us down. + } +} + +/** + * Takes ownership of the registry, or reports that someone else already has it. + * + * Two runners starting at once can each decide a monitor is needed; the loser exits here rather + * than double-reaping. Recording our own identity (not just our PID) means a later monitor can + * tell "still running" from "PID recycled". + */ +async function claimMonitorSlot(): Promise { + // A registrant always writes the registry before starting us, so a missing file means the run + // we were spawned for was already torn down. Stand down rather than recreate its directory. + if (!existsSync(getRegistryPath())) return false; + return withRegistryLock(async () => { + const registry = await readRegistryFile(); + const existing = registry.monitor; + if (existing && isSameProcessAlive(existing, readProcessStartTimes([existing.pid]))) return false; + registry.monitor = readProcessIdentity(process.pid); + await writeRegistryFile(registry); + return true; + }); +} + +/** Gives up ownership so the next registration starts a fresh monitor rather than trusting a dead one. */ +async function releaseMonitorSlot(): Promise { + try { + await withRegistryLock(async () => { + const registry = await readRegistryFile(); + if (registry.monitor?.pid !== process.pid) return; + delete registry.monitor; + await writeRegistryFile(registry); + }); + } catch { + // Best-effort: a stale slot is detected by the liveness check on the next registration. + } +} + +interface ReapTarget { + instance: HarperInstanceRecord; + reason: string; +} + +/** + * Prunes records whose process is gone and returns the ones that should be reaped. + * + * Reaped instances stay in the registry until their process actually disappears, so a monitor + * killed mid-grace leaves a target its successor picks straight back up. + */ +async function scanRegistry(): Promise<{ live: HarperInstanceRecord[]; targets: ReapTarget[] }> { + return withRegistryLock(async () => { + const registry = await readRegistryFile(); + const startTimes = readProcessStartTimes( + registry.instances.flatMap((instance) => [instance.pid, instance.owner.pid]) + ); + const live: HarperInstanceRecord[] = []; + const targets: ReapTarget[] = []; + const now = Date.now(); + for (const instance of registry.instances) { + if (!isSameProcessAlive(instance, startTimes)) continue; + live.push(instance); + if (!isSameProcessAlive(instance.owner, startTimes)) { + targets.push({ instance, reason: `owning runner ${instance.owner.pid} is gone` }); + } else if (now > instance.expiresAt) { + targets.push({ + instance, + reason: `exceeded its ${Math.round((instance.expiresAt - instance.registeredAt) / 1000)}s lifetime budget`, + }); + } + } + if (live.length !== registry.instances.length) { + registry.instances = live; + await writeRegistryFile(registry); + } + return { live, targets }; + }); +} + +/** + * Terminates an instance's process group: `SIGTERM` first so Harper can flush and release its + * ports cleanly, escalating to `SIGKILL` on a later scan if it is still alive after the grace + * period. Killing by group id is safe here because the record's start time already proved the + * group leader is the process we registered, not a recycled PID. + */ +async function reap({ instance, reason }: ReapTarget): Promise { + const escalateAt = escalationDeadlines.get(instance.id); + if (escalateAt === undefined) { + await log(`Reaping Harper ${instance.pid}${instance.hostname ? ` (${instance.hostname})` : ''}: ${reason}. SIGTERM.`); + escalationDeadlines.set(instance.id, Date.now() + reapGraceMs); + signalProcessGroup(instance.pid, 'SIGTERM'); + return; + } + if (Date.now() < escalateAt) return; + await log(`Harper ${instance.pid} survived SIGTERM for ${reapGraceMs}ms; escalating to SIGKILL.`); + // Push the next escalation past this scan's horizon so a process stuck in D-state does not + // produce a SIGKILL (and a log line) on every tick. + escalationDeadlines.set(instance.id, Date.now() + reapGraceMs); + signalProcessGroup(instance.pid, 'SIGKILL'); +} + +/** Shuts down once the registry has been empty for the idle window, under the lock so a concurrent registration wins. */ +async function exitIfIdle(): Promise { + if (idleSince === undefined || Date.now() - idleSince < idleExitMs) return false; + const released = await withRegistryLock(async () => { + const registry = await readRegistryFile(); + if (registry.instances.length > 0) return false; + if (registry.monitor?.pid === process.pid) delete registry.monitor; + await writeRegistryFile(registry); + return true; + }); + if (!released) idleSince = undefined; + return released; +} + +for (const signal of ['SIGINT', 'SIGTERM', 'SIGHUP'] as const) { + process.once(signal, () => { + running = false; + void releaseMonitorSlot().then(() => process.exit(0)); + }); +} + +if (!(await claimMonitorSlot())) process.exit(0); +await log(`Monitor started (scan ${scanIntervalMs}ms, idle exit ${idleExitMs}ms, reap grace ${reapGraceMs}ms).`); + +while (running) { + // Someone removed the registry out from under us (a cleaned-up run, or a developer clearing + // tmp): there is nothing left to supervise, and recreating it would strand an empty directory. + if (!existsSync(getRegistryPath())) process.exit(0); + try { + const { live, targets } = await scanRegistry(); + for (const id of escalationDeadlines.keys()) { + if (!live.some((instance) => instance.id === id)) escalationDeadlines.delete(id); + } + for (const target of targets) await reap(target); + + if (live.length > 0) idleSince = undefined; + else idleSince ??= Date.now(); + if (await exitIfIdle()) { + await log('Monitor exiting: no registered Harper instances.'); + process.exit(0); + } + } catch (error) { + // Never let one bad scan take the monitor down — it is the last line of defence against + // orphaned instances, and the next scan may well succeed. + await log(`Scan failed: ${(error as Error).stack || (error as Error).message}`); + } + await sleep(scanIntervalMs); +} diff --git a/src/harperSupervisor.ts b/src/harperSupervisor.ts deleted file mode 100644 index 506a4da..0000000 --- a/src/harperSupervisor.ts +++ /dev/null @@ -1,121 +0,0 @@ -import { spawn, spawnSync, type ChildProcess } from 'node:child_process'; -import { Socket } from 'node:net'; - -const SUPERVISOR_FAILURE_EXIT_CODE = 70; -const LIVENESS_FD = 3; - -type SupervisorMessage = - | { type: 'harper-spawn'; pid: number } - | { type: 'harper-exit' }; - -const [runtime, ...runtimeArgs] = process.argv.slice(2); -if (!runtime) { - process.stderr.write('[harper-supervisor] Missing Harper runtime\n'); - process.exit(SUPERVISOR_FAILURE_EXIT_CODE); -} - -let harperProcess: ChildProcess | undefined; -let harperExited = false; -let terminatingTree = false; - -function sendToRunner(message: SupervisorMessage, callback?: () => void): void { - if (!process.send || !process.connected) { - callback?.(); - return; - } - try { - process.send(message, (error: Error | null) => { - if (error) process.stderr.write(`[harper-supervisor] Failed to notify runner: ${error.message}\n`); - callback?.(); - }); - } catch (error) { - process.stderr.write(`[harper-supervisor] Failed to notify runner: ${(error as Error).message}\n`); - callback?.(); - } -} - -function terminateHarperTree(): never { - if (terminatingTree || harperExited) process.exit(SUPERVISOR_FAILURE_EXIT_CODE); - terminatingTree = true; - const harperPid = harperProcess?.pid; - if (process.platform === 'win32') { - if (harperPid !== undefined) { - spawnSync('taskkill', ['/pid', String(harperPid), '/T', '/F'], { stdio: 'ignore' }); - } - process.exit(SUPERVISOR_FAILURE_EXIT_CODE); - } - try { - process.kill(-process.pid, 'SIGKILL'); - } catch { - if (harperPid !== undefined) { - try { - process.kill(harperPid, 'SIGKILL'); - } catch { - // The Harper process already exited. - } - } - } - process.exit(SUPERVISOR_FAILURE_EXIT_CODE); -} - -function reportUnexpectedFailure(reason: unknown): never { - const message = reason instanceof Error ? reason.stack || reason.message : String(reason); - process.stderr.write(`[harper-supervisor] ${message}\n`); - terminateHarperTree(); -} - -process.on('uncaughtExceptionMonitor', reportUnexpectedFailure); -process.on('unhandledRejection', reportUnexpectedFailure); -process.once('exit', () => { - if (!harperExited) terminateHarperTree(); -}); - -const forwardedSignals: NodeJS.Signals[] = ['SIGINT', 'SIGTERM']; -if (process.platform !== 'win32') forwardedSignals.push('SIGHUP'); -for (const signal of forwardedSignals) { - process.on(signal, () => { - if (!harperExited) harperProcess?.kill(signal); - }); -} - -// The runner is the only holder of the peer endpoint, so EOF is delivered even when it cannot run cleanup code. -const runnerLiveness = new Socket({ fd: LIVENESS_FD, readable: true, writable: false }); -runnerLiveness.unref(); -runnerLiveness.once('end', terminateHarperTree); -runnerLiveness.once('close', terminateHarperTree); -runnerLiveness.on('error', (error) => { - process.stderr.write(`[harper-supervisor] Runner-liveness pipe error: ${error.message}\n`); -}); -process.channel?.unref(); - -let spawnedHarperProcess: ChildProcess; -try { - spawnedHarperProcess = spawn(runtime, runtimeArgs, { - stdio: ['inherit', 'inherit', 'inherit', 'ignore', 'ignore'], - }); -} catch (error) { - reportUnexpectedFailure(error); -} -harperProcess = spawnedHarperProcess; - -spawnedHarperProcess.once('spawn', () => { - const harperPid = harperProcess?.pid; - if (harperPid !== undefined) sendToRunner({ type: 'harper-spawn', pid: harperPid }); -}); -spawnedHarperProcess.once('error', (error) => { - process.stderr.write(`[harper-supervisor] Failed to spawn Harper: ${error.message}\n`); - harperExited = true; - process.exit(SUPERVISOR_FAILURE_EXIT_CODE); -}); -spawnedHarperProcess.once('exit', (statusCode, signal) => { - harperExited = true; - const finish = () => { - if (signal) { - process.removeAllListeners(signal); - process.kill(process.pid, signal); - } else { - process.exit(statusCode ?? SUPERVISOR_FAILURE_EXIT_CODE); - } - }; - sendToRunner({ type: 'harper-exit' }, finish); -}); diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 821d922..92ac376 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -3,9 +3,11 @@ import { ok, strictEqual, match, rejects } from 'node:assert'; import { spawn, type ChildProcess } from 'node:child_process'; import { once } from 'node:events'; import { mkdtempSync, writeFileSync, rmSync, existsSync } from 'node:fs'; +import { readFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { setTimeout as sleep } from 'node:timers/promises'; +import { fileURLToPath } from 'node:url'; import { killHarper, teardownHarper, @@ -18,6 +20,7 @@ import { buildHarperChildEnv, type StartedHarperTestContext, } from '../src/harperLifecycle.ts'; +import { MONITOR_ARGV_MARKER, type InstanceRegistry } from '../src/harperInstanceRegistry.ts'; import { isPortFree, waitForPortsFree } from '../src/portUtils.ts'; // Standalone scripts used as a fake "Harper binary" (passed via harperBinPath) to drive @@ -74,14 +77,15 @@ const result = await runHarperCommand({ harperBinPath: process.env.HARPER_FAKE_SCRIPT, timeoutMs: 5000, maxMs: 10000, + hostname: '127.0.0.1', }); const match = result.stdout.match(/tree-ready:(\\d+):(\\d+):(\\d+):(\\d+)/); if (!match) throw new Error('Missing fake Harper process markers: ' + result.stdout); -process.stdout.write('runner-ready:' + result.process.pid + ':' + result.harperPid + ':' + match[1] + ':' + match[2] + ':' + match[3] + ':' + match[4] + '\\n'); +process.stdout.write('runner-ready:' + result.process.pid + ':' + match[1] + ':' + match[2] + ':' + match[3] + ':' + match[4] + '\\n'); if (process.env.HARPER_RUNNER_MODE === 'teardown') { await killHarper({ harper: { process: result.process } }, { graceMs: 2000 }); let harperGone = false; - try { process.kill(result.harperPid, 0); } catch { harperGone = true; } + try { process.kill(result.process.pid, 0); } catch { harperGone = true; } process.stdout.write('teardown-complete:' + harperGone + '\\n'); } else { setInterval(() => {}, 1000); @@ -101,6 +105,10 @@ before(() => { writeFileSync(path, src); fixtures[name] = path; } + // This process starts fake Harper instances directly in several tests; keep them out of the + // shared registry so they can never outlive the run or perturb the monitor tests below, each of + // which opts back in against its own private registry directory. + process.env.HARPER_INTEGRATION_TEST_MONITOR = 'off'; }); after(() => { @@ -152,40 +160,94 @@ async function waitProcessGone(pid: number, timeoutMs: number): Promise } } +const MONITOR_SCRIPT = fileURLToPath(new URL('../src/harperMonitor.ts', import.meta.url)); + interface RunningFakeHarperTree { runner: ChildProcess; - supervisorPid: number; harperPid: number; descendantPid: number; ports: number[]; + /** Private registry directory this runner's monitor owns. */ + monitorDir: string; + /** True when this tree created `monitorDir` and is therefore responsible for removing it. */ + ownsMonitorDir: boolean; +} + +interface FakeHarperTreeOptions { + mode?: 'teardown'; + /** Share an existing registry directory (and therefore an existing monitor) with another tree. */ + monitorDir?: string; + /** `HARPER_INTEGRATION_TEST_MONITOR_IDLE_MS` for the monitor this runner may start. */ + idleMs?: string; + /** `HARPER_INTEGRATION_TEST_INSTANCE_MAX_LIFETIME_MS` for this runner's instance. */ + maxLifetimeMs?: string; +} + +/** Reads a registry directly. Poll-and-retry callers tolerate the rare read of a partial write. */ +async function readMonitorRegistry(monitorDir: string): Promise { + try { + return JSON.parse(await readFile(join(monitorDir, 'registry.json'), 'utf-8')) as InstanceRegistry; + } catch { + return { instances: [] }; + } } -async function startFakeHarperTree(mode?: 'teardown'): Promise { +/** Waits for a monitor to claim `monitorDir` and returns its PID. */ +async function waitForMonitor(monitorDir: string, timeoutMs = 10000): Promise { + const deadline = Date.now() + timeoutMs; + for (;;) { + const monitorPid = (await readMonitorRegistry(monitorDir)).monitor?.pid; + if (monitorPid !== undefined) return monitorPid; + if (Date.now() >= deadline) throw new Error(`No monitor claimed ${monitorDir} within ${timeoutMs}ms`); + await sleep(50); + } +} + +/** + * Starts a separate runner process that launches a fake Harper tree (a process-group leader plus a + * descendant, each holding a TCP port) through `runHarperCommand`, so tests can kill the runner + * the way CI does and observe what happens to the tree. + */ +async function startFakeHarperTree(options: FakeHarperTreeOptions = {}): Promise { + const monitorDir = options.monitorDir ?? mkdtempSync(join(tmpdir(), 'harper-it-monitor-')); const runner = spawn(process.execPath, [fixtures['orphan-runner.mjs']], { env: { ...process.env, HARPER_LIFECYCLE_URL: new URL('../src/harperLifecycle.ts', import.meta.url).href, HARPER_FAKE_SCRIPT: fixtures['process-tree.cjs'], - HARPER_TERM_DELAY_MS: mode === 'teardown' ? '300' : '0', - HARPER_RUNNER_MODE: mode, + HARPER_TERM_DELAY_MS: options.mode === 'teardown' ? '300' : '0', + HARPER_RUNNER_MODE: options.mode, + // Re-enable monitoring (this test process disables it) against a private registry, with + // timings compressed so an orphan is reaped in well under a test timeout. + HARPER_INTEGRATION_TEST_MONITOR: 'on', + HARPER_INTEGRATION_TEST_MONITOR_DIR: monitorDir, + HARPER_INTEGRATION_TEST_MONITOR_INTERVAL_MS: '200', + HARPER_INTEGRATION_TEST_MONITOR_REAP_GRACE_MS: '500', + HARPER_INTEGRATION_TEST_MONITOR_IDLE_MS: options.idleMs ?? '60000', + HARPER_INTEGRATION_TEST_INSTANCE_MAX_LIFETIME_MS: options.maxLifetimeMs ?? '', }, stdio: ['ignore', 'pipe', 'pipe'], }); + const tree: RunningFakeHarperTree = { + runner, + harperPid: 0, + descendantPid: 0, + ports: [], + monitorDir, + ownsMonitorDir: options.monitorDir === undefined, + }; let match: RegExpMatchArray; try { - match = await waitForMatch(runner, /runner-ready:(\d+):(\d+):(\d+):(\d+):(\d+):(\d+)/); + match = await waitForMatch(runner, /runner-ready:(\d+):(\d+):(\d+):(\d+):(\d+)/); } catch (error) { - forceKill(runner.pid); + await cleanupFakeHarperTree(tree); throw error; } - strictEqual(Number(match[2]), Number(match[3]), 'reported harperPid should match the Harper runtime PID'); - return { - runner, - supervisorPid: Number(match[1]), - harperPid: Number(match[3]), - descendantPid: Number(match[4]), - ports: [Number(match[5]), Number(match[6])], - }; + strictEqual(Number(match[1]), Number(match[2]), 'the lifecycle handle should be the Harper process itself'); + tree.harperPid = Number(match[2]); + tree.descendantPid = Number(match[3]); + tree.ports = [Number(match[4]), Number(match[5])]; + return tree; } function forceKill(pid: number | undefined): void { @@ -199,10 +261,16 @@ function forceKill(pid: number | undefined): void { async function cleanupFakeHarperTree(tree: Partial): Promise { forceKill(tree.runner?.pid); - forceKill(tree.supervisorPid); forceKill(tree.harperPid); forceKill(tree.descendantPid); - if (tree.ports) await waitForPortsFree('127.0.0.1', tree.ports, 2000, 50); + if (tree.monitorDir) { + // Monitors outlive their runner by design, so tests must reap their own. Wait for the slot + // to be claimed first, otherwise a monitor still starting up survives the cleanup. + const monitorPid = await waitForMonitor(tree.monitorDir, 5000).catch(() => undefined); + forceKill(monitorPid); + if (tree.ownsMonitorDir) rmSync(tree.monitorDir, { recursive: true, force: true }); + } + if (tree.ports?.length) await waitForPortsFree('127.0.0.1', tree.ports, 2000, 50); } // --- Startup watchdog (Race 1) --- @@ -276,22 +344,27 @@ test('runHarperCommand keeps a slow-but-progressing boot alive past the idle win } }); -test('runner SIGKILL reaps the supervised Harper tree and releases its ports', async () => { +// --- Orphan reaping via the shared instance monitor (POSIX only) --- + +test('runner SIGKILL leaves the monitor to reap the orphaned Harper tree and release its ports', { skip: !isPosix }, async () => { const tree = await startFakeHarperTree(); try { + await waitForMonitor(tree.monitorDir); strictEqual(await isPortFree('127.0.0.1', tree.ports[0]), false); strictEqual(await isPortFree('127.0.0.1', tree.ports[1]), false); + // SIGKILL is precisely the death the runner's own cleanup handlers cannot observe, so + // everything below is the monitor's doing. tree.runner.kill('SIGKILL'); await once(tree.runner, 'exit'); - ok(await waitProcessGone(tree.harperPid, 5000), `Harper ${tree.harperPid} should die with its runner`); - ok(await waitProcessGone(tree.descendantPid, 5000), `descendant ${tree.descendantPid} should die with its runner`); + ok(await waitProcessGone(tree.harperPid, 10000), `Harper ${tree.harperPid} should be reaped with its runner`); + ok(await waitProcessGone(tree.descendantPid, 10000), `descendant ${tree.descendantPid} should be reaped with its runner`); ok(await waitForPortsFree('127.0.0.1', tree.ports, 5000, 50), 'Harper tree ports should be reusable'); } finally { await cleanupFakeHarperTree(tree); } }); -test('runner SIGHUP reaps the supervised Harper tree and releases its ports', { skip: !isPosix }, async () => { +test('runner SIGHUP reaps the Harper tree and releases its ports', { skip: !isPosix }, async () => { const tree = await startFakeHarperTree(); try { tree.runner.kill('SIGHUP'); @@ -304,20 +377,69 @@ test('runner SIGHUP reaps the supervised Harper tree and releases its ports', { } }); -test('unexpected supervisor death falls back to reaping the Harper tree', async () => { - const tree = await startFakeHarperTree(); +test('concurrent runners share one monitor', { skip: !isPosix }, async () => { + const monitorDir = mkdtempSync(join(tmpdir(), 'harper-it-monitor-')); + let first: RunningFakeHarperTree | undefined; + let second: RunningFakeHarperTree | undefined; + try { + first = await startFakeHarperTree({ monitorDir }); + const monitorPid = await waitForMonitor(monitorDir); + second = await startFakeHarperTree({ monitorDir }); + + const registry = await readMonitorRegistry(monitorDir); + strictEqual(registry.monitor?.pid, monitorPid, 'the second runner should reuse the running monitor'); + strictEqual(registry.instances.length, 2, 'both instances should be registered with that one monitor'); + + // Two runners registering at the same instant can both decide a monitor is needed; the + // loser must stand down rather than become a second reaper. + const redundant = spawn(process.execPath, [MONITOR_SCRIPT, MONITOR_ARGV_MARKER], { + env: { ...process.env, HARPER_INTEGRATION_TEST_MONITOR_DIR: monitorDir }, + stdio: 'ignore', + }); + const exited = await Promise.race([ + once(redundant, 'exit').then(([code]) => code as number | null), + sleep(5000).then(() => 'timeout' as const), + ]); + forceKill(redundant.pid); + strictEqual(exited, 0, 'a redundant monitor should exit immediately'); + strictEqual((await readMonitorRegistry(monitorDir)).monitor?.pid, monitorPid, 'it must not take over the registry'); + } finally { + if (first) await cleanupFakeHarperTree(first); + if (second) await cleanupFakeHarperTree(second); + forceKill((await readMonitorRegistry(monitorDir)).monitor?.pid); + rmSync(monitorDir, { recursive: true, force: true }); + } +}); + +test('an instance that outlives its lifetime budget is reaped while its runner is still alive', { skip: !isPosix }, async () => { + // Owner death is the sharp signal; this budget is the backstop for a runner PID recycled by + // some other long-lived process, which would otherwise look alive forever. + const tree = await startFakeHarperTree({ maxLifetimeMs: '250' }); try { - forceKill(tree.supervisorPid); - ok(await waitProcessGone(tree.harperPid, 5000), `Harper ${tree.harperPid} should die with its supervisor`); - ok(await waitProcessGone(tree.descendantPid, 5000), `descendant ${tree.descendantPid} should die with its supervisor`); + ok(await waitProcessGone(tree.harperPid, 10000), `overdue Harper ${tree.harperPid} should be reaped`); + strictEqual(tree.runner.exitCode, null, 'the owning runner should be untouched'); ok(await waitForPortsFree('127.0.0.1', tree.ports, 5000, 50), 'Harper tree ports should be reusable'); } finally { await cleanupFakeHarperTree(tree); } }); -test('killHarper waits for supervised Harper shutdown and does not keep the runner alive', { skip: !isPosix }, async () => { - const tree = await startFakeHarperTree('teardown'); +test('the monitor shuts down once no instances remain', { skip: !isPosix }, async () => { + const tree = await startFakeHarperTree({ idleMs: '500' }); + try { + const monitorPid = await waitForMonitor(tree.monitorDir); + tree.runner.kill('SIGKILL'); + await once(tree.runner, 'exit'); + ok(await waitProcessGone(tree.harperPid, 10000), `Harper ${tree.harperPid} should be reaped`); + ok(await waitProcessGone(monitorPid, 10000), `monitor ${monitorPid} should exit once the registry empties`); + strictEqual((await readMonitorRegistry(tree.monitorDir)).monitor, undefined, 'it should release its slot on the way out'); + } finally { + await cleanupFakeHarperTree(tree); + } +}); + +test('killHarper waits for Harper shutdown and does not keep the runner alive', { skip: !isPosix }, async () => { + const tree = await startFakeHarperTree({ mode: 'teardown' }); try { await waitForMatch(tree.runner, /teardown-complete:true/); await Promise.race([ From efecedbf09f23d38f903a0a01b185e2cb6f44614 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 09:38:13 -0600 Subject: [PATCH 06/14] fix(lifecycle): publish registry updates by atomic rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writing registry.json in place truncated the live file first, so a runner killed in that window left torn JSON. readRegistryFile maps torn JSON to an empty registry, and the next writer persists that emptiness — dropping every orphan-reaping target on the machine, including other runners', under exactly the SIGKILL the monitor exists to survive. Each update is now written to a unique pending file beside the registry and renamed into place, so a reader only ever sees the previous registry or the complete new one. The name is unique per write because a fixed one could be truncated by a second writer that reclaimed the lock as stale. Co-Authored-By: Claude Opus --- README.md | 3 +- src/harperInstanceRegistry.ts | 35 +++++++++++++++-- test/harperLifecycle.test.ts | 74 ++++++++++++++++++++++++++++++++++- 3 files changed, 106 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index d5b0f3b..6b96e96 100644 --- a/README.md +++ b/README.md @@ -210,7 +210,7 @@ If you are not using `node:test`, use `createHarperContext()` to create a plain A test runner that dies without running its teardown — `SIGKILL`, a hard crash, a cancelled CI job — cannot reap the Harper instances it started, because those are deliberately detached into their own process groups so that whole-tree teardown works. Left alone, they hold their loopback address's fixed ports until the machine is rebooted. -To close that gap, `startHarper()` registers each instance in a small on-disk registry and makes sure a single shared **monitor** process is running. The monitor is not a per-instance sidecar: one is started on demand per registry directory (per machine, by default), every concurrent runner reuses it, and it exits once the registry has been empty for a while. It scans the registry on an interval and terminates — `SIGTERM`, then `SIGKILL` after a grace period — the process group of any instance whose owning runner is gone, or which has outlived its lifetime budget. Instances are matched by PID *and* process start time, so a recycled PID is never mistaken for a live one. +To close that gap, `startHarper()` registers each instance in a small on-disk registry and makes sure a single shared **monitor** process is running. The monitor is not a per-instance sidecar: one is started on demand per registry directory (per machine, by default), every concurrent runner reuses it, and it exits once the registry has been empty for a while. Registry updates are published by renaming a complete file into place, so a runner killed mid-write leaves the previous registry — and therefore every other runner's reap targets — intact. It scans the registry on an interval and terminates — `SIGTERM`, then `SIGKILL` after a grace period — the process group of any instance whose owning runner is gone, or which has outlived its lifetime budget. Instances are matched by PID *and* process start time, so a recycled PID is never mistaken for a live one. The runner's own `exit`/`SIGINT`/`SIGTERM`/`SIGHUP` handlers still reap instances immediately on any exit it can observe; the monitor only handles the deaths it cannot. @@ -221,6 +221,7 @@ Registry directory layout (`${TMPDIR}/harper-integration-test-monitor` by defaul | `registry.json` | The current monitor and every registered instance (PID, start time, owning runner, loopback address, deadline) | | `registry.lock` | Cross-process mutex guarding `registry.json` | | `monitor.log` | Append-only record of monitor start/exit and every reap, with the reason | +| `registry.json.*.pending` | A registry update being written, renamed over `registry.json` once complete so no reader ever sees a partial one. Only present transiently, or left behind by a writer that was killed mid-update | Each managed Harper process also carries `HARPER_IT_KIND=harper-instance`, `HARPER_IT_INSTANCE_ID`, and `HARPER_IT_OWNER_PID` in its environment, and the monitor's command line contains `--harper-integration-test-monitor`, so both are identifiable from `ps` / `/proc` without consulting the registry. diff --git a/src/harperInstanceRegistry.ts b/src/harperInstanceRegistry.ts index 9968bf2..c41ba08 100644 --- a/src/harperInstanceRegistry.ts +++ b/src/harperInstanceRegistry.ts @@ -1,5 +1,5 @@ import { spawn, spawnSync } from 'node:child_process'; -import { mkdir, open, readFile, stat, unlink, writeFile } from 'node:fs/promises'; +import { mkdir, open, readFile, rename, stat, unlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { setTimeout as sleep } from 'node:timers/promises'; @@ -209,7 +209,11 @@ export async function withRegistryLock(callback: () => Promise): Promise { try { const parsed = JSON.parse(await readFile(getRegistryPath(), 'utf-8')) as InstanceRegistry; @@ -219,10 +223,33 @@ export async function readRegistryFile(): Promise { } } -/** Writes the registry. Only call while holding the lock. */ +let pendingWriteCounter = 0; + +/** + * Writes the registry. Only call while holding the lock. + * + * Publishes by writing a complete file alongside the registry and renaming it into place, so + * readers only ever see the old registry or the new one. Writing the live file in place would + * truncate it first, and a writer that died in that window — the `SIGKILL` this whole mechanism + * exists to survive — would leave torn JSON that `readRegistryFile` reads as an empty registry, + * discarding every reap target on the machine including other runners'. + * + * The pending file's name is unique per write, because a fixed one could be truncated underneath + * us by a second writer that reclaimed the lock as stale — reintroducing exactly the tearing the + * rename removes. A writer killed between the two steps leaves its pending file behind; it is + * inert, and lives in a directory that is already per-machine scratch. + */ export async function writeRegistryFile(registry: InstanceRegistry): Promise { await mkdir(getRegistryDir(), { recursive: true }); - await writeFile(getRegistryPath(), JSON.stringify(registry)); + const registryPath = getRegistryPath(); + const pendingPath = `${registryPath}.${process.pid}.${++pendingWriteCounter}.pending`; + try { + await writeFile(pendingPath, JSON.stringify(registry)); + await rename(pendingPath, registryPath); + } catch (error) { + await unlink(pendingPath).catch(() => {}); + throw error; + } } function getMonitorScript(): string { diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 92ac376..7924d1a 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -65,6 +65,30 @@ if (process.env.HARPER_FAKE_DESCENDANT === '1') { if (delay > 0) process.on('SIGTERM', () => setTimeout(() => server.close(() => process.exit(0)), delay)); }); } +`, + // Rewrites the registry back to back so a reader — or the SIGKILL the test sends — is almost + // always looking at a write in flight. The padded shape makes each write big enough to span + // several of the reader's polls. + 'registry-writer.mjs': ` +const { writeRegistryFile } = await import(process.env.HARPER_REGISTRY_URL); +// Never a real process group: a bogus start time means any monitor that somehow read this private +// registry would treat the record as gone rather than signalling the PID's group. +const seeded = { + id: 'seeded', + pid: process.pid, + startTime: 'writer-fixture', + owner: { pid: process.pid, startTime: 'writer-fixture' }, + registeredAt: 0, + expiresAt: 0, +}; +const small = { instances: [seeded] }; +const padded = { instances: [seeded, { ...seeded, id: 'padding', hostname: 'x'.repeat(2_000_000) }] }; +await writeRegistryFile(small); +process.stdout.write('writer-ready\\n'); +for (;;) { + await writeRegistryFile(padded); + await writeRegistryFile(small); +} `, 'orphan-runner.mjs': ` const { runHarperCommand, killHarper } = await import(process.env.HARPER_LIFECYCLE_URL); @@ -183,7 +207,7 @@ interface FakeHarperTreeOptions { maxLifetimeMs?: string; } -/** Reads a registry directly. Poll-and-retry callers tolerate the rare read of a partial write. */ +/** Reads a registry directly. Registry writes are atomic, so this only reads empty before the first one. */ async function readMonitorRegistry(monitorDir: string): Promise { try { return JSON.parse(await readFile(join(monitorDir, 'registry.json'), 'utf-8')) as InstanceRegistry; @@ -438,6 +462,54 @@ test('the monitor shuts down once no instances remain', { skip: !isPosix }, asyn } }); +test('a registry write survives the abrupt death of its writer', { skip: !isPosix }, async () => { + const monitorDir = mkdtempSync(join(tmpdir(), 'harper-it-monitor-')); + const registryPath = join(monitorDir, 'registry.json'); + const writer = spawn(process.execPath, [fixtures['registry-writer.mjs']], { + env: { + ...process.env, + HARPER_REGISTRY_URL: new URL('../src/harperInstanceRegistry.ts', import.meta.url).href, + HARPER_INTEGRATION_TEST_MONITOR_DIR: monitorDir, + }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + /** Parses the registry the way a monitor would, failing loudly instead of reading torn JSON as empty. */ + async function readCompleteRegistry(context: string): Promise { + const raw = await readFile(registryPath, 'utf-8'); + try { + return JSON.parse(raw) as InstanceRegistry; + } catch { + throw new Error(`Torn registry ${context}: ${raw.length} bytes, ending ${JSON.stringify(raw.slice(-40))}`); + } + } + try { + await waitForMatch(writer, /writer-ready/); + // Read while the writer rewrites the file back to back. A truncate-in-place write is visible + // here as an empty or half-written file, which `readRegistryFile` maps to an empty registry — + // after which the next writer persists that emptiness, dropping every runner's reap targets. + let reads = 0; + for (const deadline = Date.now() + 500; Date.now() < deadline; reads++) { + const observed = await readCompleteRegistry(`while the writer was running (read ${reads})`); + strictEqual(observed.instances[0]?.id, 'seeded', 'a visible registry must never lose its instances'); + } + ok(reads > 20, `expected to catch many writes in flight, only managed ${reads} reads`); + + // SIGKILL mid-write: whatever is on disk afterwards must still be one of the two complete + // shapes the writer alternates between, never a partial one. + writer.kill('SIGKILL'); + await once(writer, 'exit'); + const survivor = await readCompleteRegistry('after the writer was killed'); + strictEqual(survivor.instances[0]?.id, 'seeded', 'a killed writer must leave the reap target behind'); + ok( + survivor.instances.length === 1 || survivor.instances.length === 2, + `expected one of the writer's two complete shapes, got ${survivor.instances.length} instances` + ); + } finally { + forceKill(writer.pid); + rmSync(monitorDir, { recursive: true, force: true }); + } +}); + test('killHarper waits for Harper shutdown and does not keep the runner alive', { skip: !isPosix }, async () => { const tree = await startFakeHarperTree({ mode: 'teardown' }); try { From c9bb8d273919c5fc952c7f820b820a608c4c069a Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 09:54:02 -0600 Subject: [PATCH 07/14] fix(lifecycle): keep registry failures from erasing live instances Three ways the registry could still lose the records the monitor reaps from, found by the pre-push review of the atomic-write fix: - withRegistryLock unlinked the lock file unconditionally, so a critical section that overran the stale timeout deleted the lock of the process that had superseded it, admitting a third process into the section alongside it. It now writes a token into the lock and only releases its own. - readRegistryFile reported every read failure as an empty registry, so a permission error or corrupt file made the caller write that emptiness back. Only ENOENT is an empty registry now; anything else propagates. - The absolute startup deadline stayed armed while startHarper waited for registration, so contention on the shared lock could time out and kill a Harper that had already reported ready. Co-Authored-By: Claude Opus --- src/harperInstanceRegistry.ts | 36 ++++++++++++---- src/harperLifecycle.ts | 9 ++-- test/harperLifecycle.test.ts | 80 ++++++++++++++++++++++++++++++++++- 3 files changed, 112 insertions(+), 13 deletions(-) diff --git a/src/harperInstanceRegistry.ts b/src/harperInstanceRegistry.ts index c41ba08..64ccf8c 100644 --- a/src/harperInstanceRegistry.ts +++ b/src/harperInstanceRegistry.ts @@ -36,6 +36,8 @@ export const INSTANCE_ENV_OWNER_PID = 'HARPER_IT_OWNER_PID'; const LOCK_STALE_TIMEOUT_MS = 10000; const LOCK_RETRY_DELAY_MS = 50; +let lockTokenCounter = 0; + function envInt(name: string, fallback: number): number { const parsed = parseInt(process.env[name] || '', 10); return Number.isNaN(parsed) || parsed <= 0 ? fallback : parsed; @@ -174,14 +176,16 @@ function pidExists(pid: number): boolean { * Critical sections here are a read/modify/write of one small file, so the retry delay is much * shorter than the pool's. */ -async function acquireLock(): Promise { +async function acquireLock(): Promise { const lockPath = getLockPath(); + const token = `${process.pid}-${++lockTokenCounter}-${Math.random().toString(36).slice(2)}`; await mkdir(getRegistryDir(), { recursive: true }); while (true) { try { const lockFileHandle = await open(lockPath, 'wx'); + await lockFileHandle.writeFile(token); await lockFileHandle.close(); - return; + return token; } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; try { @@ -197,12 +201,15 @@ async function acquireLock(): Promise { } export async function withRegistryLock(callback: () => Promise): Promise { - await acquireLock(); + const token = await acquireLock(); try { return await callback(); } finally { try { - await unlink(getLockPath()); + // Release only a lock we still hold. A critical section that overran the stale timeout has + // already been superseded, and unlinking then would remove the *new* holder's lock and let + // a third process into the section alongside it. + if ((await readFile(getLockPath(), 'utf-8')) === token) await unlink(getLockPath()); } catch { // Already released (e.g. reclaimed as stale). } @@ -215,12 +222,25 @@ export async function withRegistryLock(callback: () => Promise): Promise { + let contents: string; try { - const parsed = JSON.parse(await readFile(getRegistryPath(), 'utf-8')) as InstanceRegistry; - return { monitor: parsed.monitor, instances: Array.isArray(parsed.instances) ? parsed.instances : [] }; - } catch { - return { instances: [] }; + contents = await readFile(getRegistryPath(), 'utf-8'); + } catch (error) { + // Only a missing file means an empty registry. Every other read failure has to propagate: + // reporting one as empty is what turns a transient problem into the caller writing that + // emptiness back, erasing live instances other runners are relying on us to reap. + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return { instances: [] }; + throw error; + } + let parsed: InstanceRegistry; + try { + parsed = JSON.parse(contents) as InstanceRegistry; + } catch (error) { + throw new Error( + `Harper instance registry at ${getRegistryPath()} is not valid JSON (delete it to reset monitoring): ${(error as Error).message}` + ); } + return { monitor: parsed.monitor, instances: Array.isArray(parsed.instances) ? parsed.instances : [] }; } let pendingWriteCounter = 0; diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index e506eff..d801dae 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -452,13 +452,16 @@ export function runHarperCommand({ const succeed = () => { if (settled || readinessDetected) return; readinessDetected = true; - clearTimeout(idleTimer); + // Harper has reported ready, so the startup watchdog has nothing left to guard. Leaving the + // absolute deadline armed across registration would let contention on the shared registry + // lock time out — and kill — an instance that already booted successfully. + clearTimers(); // Resolve only once the instance is durably registered, so a runner killed the moment - // startHarper returns still leaves the monitor a record to act on. + // startHarper returns still leaves the monitor a record to act on. Registration always + // settles: it self-heals a stale lock, and `trackHarperProcess` absorbs its failures. void trackedProcess.registered.then(() => { if (settled) return; settled = true; - clearTimers(); resolve({ process: proc, stdout, stderr }); }); }; diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 7924d1a..a9c20bc 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -3,7 +3,7 @@ import { ok, strictEqual, match, rejects } from 'node:assert'; import { spawn, type ChildProcess } from 'node:child_process'; import { once } from 'node:events'; import { mkdtempSync, writeFileSync, rmSync, existsSync } from 'node:fs'; -import { readFile } from 'node:fs/promises'; +import { readFile, writeFile as writeFileAsync } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { setTimeout as sleep } from 'node:timers/promises'; @@ -20,7 +20,12 @@ import { buildHarperChildEnv, type StartedHarperTestContext, } from '../src/harperLifecycle.ts'; -import { MONITOR_ARGV_MARKER, type InstanceRegistry } from '../src/harperInstanceRegistry.ts'; +import { + MONITOR_ARGV_MARKER, + readRegistryFile, + withRegistryLock, + type InstanceRegistry, +} from '../src/harperInstanceRegistry.ts'; import { isPortFree, waitForPortsFree } from '../src/portUtils.ts'; // Standalone scripts used as a fake "Harper binary" (passed via harperBinPath) to drive @@ -510,6 +515,77 @@ test('a registry write survives the abrupt death of its writer', { skip: !isPosi } }); +test('a superseded lock holder does not release the lock that replaced it', { skip: !isPosix }, async () => { + const monitorDir = mkdtempSync(join(tmpdir(), 'harper-it-monitor-')); + const previousDir = process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR; + process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR = monitorDir; + try { + await withRegistryLock(async () => { + // What a critical section that overran the stale timeout comes back to: another process + // reclaimed the lock and is now inside the section itself. + await writeFileAsync(join(monitorDir, 'registry.lock'), 'another-holder'); + }); + strictEqual( + await readFile(join(monitorDir, 'registry.lock'), 'utf-8'), + 'another-holder', + 'releasing must not evict the holder that superseded us — that admits a third process alongside it' + ); + } finally { + process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR = previousDir; + rmSync(monitorDir, { recursive: true, force: true }); + } +}); + +test('an unreadable registry is reported rather than silently read as empty', { skip: !isPosix }, async () => { + const monitorDir = mkdtempSync(join(tmpdir(), 'harper-it-monitor-')); + const previousDir = process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR; + process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR = monitorDir; + try { + // An empty registry is the one answer that must never be invented: the caller writes it back, + // and every live instance on the machine loses the record that would have got it reaped. + await writeFileAsync(join(monitorDir, 'registry.json'), '{"instances":[{"id":"trunc'); + await rejects(readRegistryFile(), /is not valid JSON/); + } finally { + process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR = previousDir; + rmSync(monitorDir, { recursive: true, force: true }); + } +}); + +test('a contended registry lock cannot time out a Harper that already started', { skip: !isPosix }, async () => { + const monitorDir = mkdtempSync(join(tmpdir(), 'harper-it-monitor-')); + const previousDir = process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR; + const previousEnabled = process.env.HARPER_INTEGRATION_TEST_MONITOR; + process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR = monitorDir; + process.env.HARPER_INTEGRATION_TEST_MONITOR = 'on'; + // Held for longer than maxMs but well inside the stale timeout, so registration blocks on a lock + // that is legitimately someone else's — what a machine full of concurrent runners looks like. + const lockPath = join(monitorDir, 'registry.lock'); + await writeFileAsync(lockPath, 'other-runner'); + let started: Awaited> | undefined; + try { + const starting = runHarperCommand({ + args: [], + env: {}, + completionMessage: 'successfully started', + harperBinPath: fixtures['ready.cjs'], + timeoutMs: 2000, + maxMs: 2000, + hostname: '127.0.0.1', + }); + // Past the point where the startup deadline used to fire on a Harper that was already up. + await sleep(3000); + rmSync(lockPath, { force: true }); + started = await starting; + match(started.stdout, /successfully started/); + } finally { + if (started) started.process.kill('SIGKILL'); + process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR = previousDir; + process.env.HARPER_INTEGRATION_TEST_MONITOR = previousEnabled; + forceKill(await waitForMonitor(monitorDir, 5000).catch(() => undefined)); + rmSync(monitorDir, { recursive: true, force: true }); + } +}); + test('killHarper waits for Harper shutdown and does not keep the runner alive', { skip: !isPosix }, async () => { const tree = await startFakeHarperTree({ mode: 'teardown' }); try { From ec5cdf889636a18076a434ba05790880545462f9 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 10:28:07 -0600 Subject: [PATCH 08/14] fix(lifecycle): end the startup watchdog at readiness, not at resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-push review found that waiting for registration before resolving left `settled` false past readiness, so every guard keyed on it stayed open: a post-readiness log line — which a real Harper emits constantly — re-armed the idle watchdog that had just been cleared, letting a registry-lock stall SIGKILL a healthy instance, and `startupOutput` kept accumulating past the snapshot its own comment promises. Startup now ends at readiness, which is what those guards always meant. Also from that round: - Bound lock acquisition (30s) and the `ps` lookup (5s), so a pathological lock holder surfaces as a failed registration instead of a startHarper that never settles. - An `instances` that parses but is not an array now throws like the torn-JSON case rather than being read as empty and written back. - Default the registry directory per-user and create it 0700. Cross-user reaping could never work — signalling another user's group returns EPERM, which reads as "still alive", so a foreign record pinned the monitor forever. - Log once when `ps` cannot report start times (busybox), where PID-reuse detection silently degrades to a bare PID check. Co-Authored-By: Claude Opus --- README.md | 6 ++--- src/harperInstanceRegistry.ts | 51 ++++++++++++++++++++++++----------- src/harperLifecycle.ts | 17 +++++++----- src/harperMonitor.ts | 6 +++++ test/harperLifecycle.test.ts | 31 ++++++++++++++------- 5 files changed, 77 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 6b96e96..ba2eff4 100644 --- a/README.md +++ b/README.md @@ -210,11 +210,11 @@ If you are not using `node:test`, use `createHarperContext()` to create a plain A test runner that dies without running its teardown — `SIGKILL`, a hard crash, a cancelled CI job — cannot reap the Harper instances it started, because those are deliberately detached into their own process groups so that whole-tree teardown works. Left alone, they hold their loopback address's fixed ports until the machine is rebooted. -To close that gap, `startHarper()` registers each instance in a small on-disk registry and makes sure a single shared **monitor** process is running. The monitor is not a per-instance sidecar: one is started on demand per registry directory (per machine, by default), every concurrent runner reuses it, and it exits once the registry has been empty for a while. Registry updates are published by renaming a complete file into place, so a runner killed mid-write leaves the previous registry — and therefore every other runner's reap targets — intact. It scans the registry on an interval and terminates — `SIGTERM`, then `SIGKILL` after a grace period — the process group of any instance whose owning runner is gone, or which has outlived its lifetime budget. Instances are matched by PID *and* process start time, so a recycled PID is never mistaken for a live one. +To close that gap, `startHarper()` registers each instance in a small on-disk registry and makes sure a single shared **monitor** process is running. The monitor is not a per-instance sidecar: one is started on demand per registry directory (per machine, by default), every concurrent runner reuses it, and it exits once the registry has been empty for a while. Registry updates are published by renaming a complete file into place, so a runner killed mid-write leaves the previous registry — and therefore every other runner's reap targets — intact. It scans the registry on an interval and terminates — `SIGTERM`, then `SIGKILL` after a grace period — the process group of any instance whose owning runner is gone, or which has outlived its lifetime budget. Instances are matched by PID *and* process start time, so a recycled PID is never mistaken for a live one — on a host whose `ps` cannot report a start time (busybox/Alpine) this degrades to a PID-only check, which the monitor logs on startup. The runner's own `exit`/`SIGINT`/`SIGTERM`/`SIGHUP` handlers still reap instances immediately on any exit it can observe; the monitor only handles the deaths it cannot. -Registry directory layout (`${TMPDIR}/harper-integration-test-monitor` by default): +Registry directory layout (`${TMPDIR}/harper-integration-test-monitor-${uid}` by default — per-user, because signalling another user's process group fails with `EPERM` and could never have reaped it): | File | Contents | | --- | --- | @@ -230,7 +230,7 @@ This is POSIX-only: reaping relies on process groups, which Windows does not hav **Environment Variables:** - `HARPER_INTEGRATION_TEST_MONITOR` - Set to `off` to disable registration and the monitor entirely. Default on (POSIX only). -- `HARPER_INTEGRATION_TEST_MONITOR_DIR` - Registry directory. Default `${TMPDIR}/harper-integration-test-monitor`. Point separate runs at separate directories to give them separate monitors. +- `HARPER_INTEGRATION_TEST_MONITOR_DIR` - Registry directory. Default `${TMPDIR}/harper-integration-test-monitor-${uid}`. Point separate runs at separate directories to give them separate monitors. - `HARPER_INTEGRATION_TEST_MONITOR_INTERVAL_MS` - How often the monitor rescans the registry. Default `2000`. - `HARPER_INTEGRATION_TEST_MONITOR_REAP_GRACE_MS` - Grace period between the monitor's SIGTERM and SIGKILL. Default `5000`. - `HARPER_INTEGRATION_TEST_MONITOR_IDLE_MS` - How long the registry must stay empty before the monitor shuts down. Default `60000`. diff --git a/src/harperInstanceRegistry.ts b/src/harperInstanceRegistry.ts index 64ccf8c..866bc42 100644 --- a/src/harperInstanceRegistry.ts +++ b/src/harperInstanceRegistry.ts @@ -35,6 +35,10 @@ export const INSTANCE_ENV_OWNER_PID = 'HARPER_IT_OWNER_PID'; const LOCK_STALE_TIMEOUT_MS = 10000; const LOCK_RETRY_DELAY_MS = 50; +/** Ceiling on waiting for the lock, well past a stale reclaim so only a pathological holder hits it. */ +const LOCK_ACQUIRE_TIMEOUT_MS = 30000; +/** Ceiling on the `ps` lookup, so a call wedged in D-state cannot stall a caller indefinitely. */ +const PS_TIMEOUT_MS = 5000; let lockTokenCounter = 0; @@ -48,10 +52,17 @@ function envInt(name: string, fallback: number): number { * than at module load so a test (or a caller isolating a run) can point it somewhere private * after import, and so the value the monitor inherits always matches its parent's. * + * Per-user, because reaping across users cannot work anyway: signalling another user's process + * group fails with `EPERM`, which reads as "still alive", so a foreign record would never be + * pruned and would keep the monitor running forever. It also keeps the shared-`/tmp` path from + * being one another account can plant records in. + * * Override with `HARPER_INTEGRATION_TEST_MONITOR_DIR`. */ export function getRegistryDir(): string { - return process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR || join(tmpdir(), 'harper-integration-test-monitor'); + if (process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR) return process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR; + const uid = process.getuid?.(); + return join(tmpdir(), uid === undefined ? 'harper-integration-test-monitor' : `harper-integration-test-monitor-${uid}`); } export function getRegistryPath(): string { @@ -135,7 +146,10 @@ export function readProcessStartTimes(pids: number[]): Map { const startTimes = new Map(); const uniquePids = [...new Set(pids)].filter((pid) => Number.isInteger(pid) && pid > 0); if (uniquePids.length === 0) return startTimes; - const result = spawnSync('ps', ['-o', 'pid=,lstart=', '-p', uniquePids.join(',')], { encoding: 'utf8' }); + const result = spawnSync('ps', ['-o', 'pid=,lstart=', '-p', uniquePids.join(',')], { + encoding: 'utf8', + timeout: PS_TIMEOUT_MS, + }); // A non-zero status just means none of the PIDs exist; only a missing `ps` is worth noticing, // and there the empty map degrades callers to a plain PID check rather than reporting deaths. if (result.error || typeof result.stdout !== 'string') return startTimes; @@ -179,7 +193,10 @@ function pidExists(pid: number): boolean { async function acquireLock(): Promise { const lockPath = getLockPath(); const token = `${process.pid}-${++lockTokenCounter}-${Math.random().toString(36).slice(2)}`; - await mkdir(getRegistryDir(), { recursive: true }); + await mkdir(getRegistryDir(), { recursive: true, mode: 0o700 }); + // Bounded so a pathological holder — one refreshing the lock faster than it goes stale — surfaces + // as a failed registration the caller can warn about, rather than a caller that never settles. + const deadline = Date.now() + LOCK_ACQUIRE_TIMEOUT_MS; while (true) { try { const lockFileHandle = await open(lockPath, 'wx'); @@ -195,6 +212,9 @@ async function acquireLock(): Promise { } catch { // Another process removed it first; just retry. } + if (Date.now() >= deadline) { + throw new Error(`Timed out after ${LOCK_ACQUIRE_TIMEOUT_MS}ms waiting for the Harper instance registry lock at ${lockPath}`); + } await sleep(LOCK_RETRY_DELAY_MS); } } @@ -240,7 +260,12 @@ export async function readRegistryFile(): Promise { `Harper instance registry at ${getRegistryPath()} is not valid JSON (delete it to reset monitoring): ${(error as Error).message}` ); } - return { monitor: parsed.monitor, instances: Array.isArray(parsed.instances) ? parsed.instances : [] }; + if (!Array.isArray(parsed.instances)) { + // Same erasure the parse failure above guards against: reporting this as empty would have the + // caller write that emptiness back over whatever the file really described. + throw new Error(`Harper instance registry at ${getRegistryPath()} has no instance list (delete it to reset monitoring)`); + } + return { monitor: parsed.monitor, instances: parsed.instances }; } let pendingWriteCounter = 0; @@ -248,19 +273,15 @@ let pendingWriteCounter = 0; /** * Writes the registry. Only call while holding the lock. * - * Publishes by writing a complete file alongside the registry and renaming it into place, so - * readers only ever see the old registry or the new one. Writing the live file in place would - * truncate it first, and a writer that died in that window — the `SIGKILL` this whole mechanism - * exists to survive — would leave torn JSON that `readRegistryFile` reads as an empty registry, - * discarding every reap target on the machine including other runners'. - * - * The pending file's name is unique per write, because a fixed one could be truncated underneath - * us by a second writer that reclaimed the lock as stale — reintroducing exactly the tearing the - * rename removes. A writer killed between the two steps leaves its pending file behind; it is - * inert, and lives in a directory that is already per-machine scratch. + * Publishes by rename so readers see either the old registry or the new one. An in-place write + * truncates first, and a writer killed in that window — the `SIGKILL` this whole mechanism exists + * to survive — leaves torn JSON, which used to read as an empty registry and take every reap + * target on the machine with it. The pending name is unique per write because a fixed one could be + * truncated by a writer that reclaimed the lock as stale, reintroducing that same tearing; one + * left behind by a killed writer is inert. */ export async function writeRegistryFile(registry: InstanceRegistry): Promise { - await mkdir(getRegistryDir(), { recursive: true }); + await mkdir(getRegistryDir(), { recursive: true, mode: 0o700 }); const registryPath = getRegistryPath(); const pendingPath = `${registryPath}.${process.pid}.${++pendingWriteCounter}.pending`; try { diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index d801dae..5229223 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -452,13 +452,12 @@ export function runHarperCommand({ const succeed = () => { if (settled || readinessDetected) return; readinessDetected = true; - // Harper has reported ready, so the startup watchdog has nothing left to guard. Leaving the - // absolute deadline armed across registration would let contention on the shared registry - // lock time out — and kill — an instance that already booted successfully. + // Left armed across registration, these would let registry-lock contention time out — and + // kill — an instance that already booted successfully. clearTimers(); // Resolve only once the instance is durably registered, so a runner killed the moment // startHarper returns still leaves the monitor a record to act on. Registration always - // settles: it self-heals a stale lock, and `trackHarperProcess` absorbs its failures. + // settles: `acquireLock` is bounded and `trackHarperProcess` absorbs its failures. void trackedProcess.registered.then(() => { if (settled) return; settled = true; @@ -466,10 +465,14 @@ export function runHarperCommand({ }); }; + // Startup ends at readiness, not at resolution: the watchdog and the `startupOutput` snapshot + // both stop here, while resolution waits on registration for a little longer. + const startupFinished = () => settled || readinessDetected; + // Reset on every chunk of output so the limit is time-since-last-progress, not total boot // time: a slow-but-healthy boot that keeps logging never trips it, only true silence does. const resetIdleTimer = () => { - if (settled) return; + if (startupFinished()) return; clearTimeout(idleTimer); idleTimer = setTimeout( () => failStartup(`Harper produced no startup output for ${idleTimeoutMs}ms before reporting ready (likely hung)`), @@ -490,7 +493,7 @@ export function runHarperCommand({ // Once ready, keep streaming logs to disk but stop the watchdog and capture: the // returned startupOutput is a snapshot taken at readiness, and the server may run // (and log) for the rest of the suite. - if (settled) return; + if (startupFinished()) return; resetIdleTimer(); stdout += dataString; // Match against the accumulated output, not just this chunk, so a marker split across @@ -503,7 +506,7 @@ export function runHarperCommand({ proc.stderr?.on('data', (data: Buffer) => { const dataString = stripAnsi(data.toString()); stderrStream?.write(dataString); - if (settled) return; + if (startupFinished()) return; resetIdleTimer(); stderr += dataString; }); diff --git a/src/harperMonitor.ts b/src/harperMonitor.ts index 3375ff3..18eb858 100644 --- a/src/harperMonitor.ts +++ b/src/harperMonitor.ts @@ -165,6 +165,12 @@ for (const signal of ['SIGINT', 'SIGTERM', 'SIGHUP'] as const) { if (!(await claimMonitorSlot())) process.exit(0); await log(`Monitor started (scan ${scanIntervalMs}ms, idle exit ${idleExitMs}ms, reap grace ${reapGraceMs}ms).`); +// `ps -o lstart=` is absent on busybox, so identity checks silently degrade to a bare PID test and +// a recycled PID can be mistaken for its predecessor. Say so once rather than reaping on a +// guarantee this host cannot provide. +if (readProcessStartTimes([process.pid]).size === 0) { + await log('WARNING: `ps -o pid=,lstart=` returned nothing; falling back to PID-only liveness checks (PID reuse is undetectable).'); +} while (running) { // Someone removed the registry out from under us (a cleaned-up run, or a developer clearing diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index a9c20bc..dad9426 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -1,5 +1,5 @@ import { test, before, after } from 'node:test'; -import { ok, strictEqual, match, rejects } from 'node:assert'; +import { ok, strictEqual, match, doesNotMatch, rejects } from 'node:assert'; import { spawn, type ChildProcess } from 'node:child_process'; import { once } from 'node:events'; import { mkdtempSync, writeFileSync, rmSync, existsSync } from 'node:fs'; @@ -33,6 +33,9 @@ import { isPortFree, waitForPortsFree } from '../src/portUtils.ts'; const FIXTURE_SOURCES: Record = { // Reports ready, then stays alive like a server. 'ready.cjs': "process.stdout.write('booting\\n');\nsetTimeout(() => process.stdout.write('successfully started\\n'), 50);\nsetInterval(() => {}, 1000);\n", + // Reports ready, then logs once more and goes quiet — what a real Harper does after startup, + // and what re-arms a startup watchdog that keys on resolution instead of readiness. + 'ready-then-log.cjs': "process.stdout.write('booting\\n');\nsetTimeout(() => process.stdout.write('successfully started\\n'), 50);\nsetTimeout(() => process.stdout.write('post-readiness log line\\n'), 150);\nsetInterval(() => {}, 1000);\n", // Emits one line, then goes silent forever (hung during startup). 'idle-hang.cjs': "process.stdout.write('booting\\n');\nsetInterval(() => {}, 1000);\n", // Emits output continuously but never reports ready. @@ -212,6 +215,12 @@ interface FakeHarperTreeOptions { maxLifetimeMs?: string; } +/** Restores an environment variable, removing it when it was previously unset — assigning `undefined` would set the string `"undefined"`. */ +function restoreEnv(name: string, value: string | undefined): void { + if (value === undefined) delete process.env[name]; + else process.env[name] = value; +} + /** Reads a registry directly. Registry writes are atomic, so this only reads empty before the first one. */ async function readMonitorRegistry(monitorDir: string): Promise { try { @@ -362,7 +371,7 @@ test('runHarperCommand keeps a slow-but-progressing boot alive past the idle win env: {}, completionMessage: 'successfully started', harperBinPath: fixtures['idle-reset.cjs'], - // The idle window includes launching both the supervisor and fake Harper before first output. + // The idle window has to cover process launch before the fixture's first output. timeoutMs: 700, maxMs: 10000, }); @@ -531,7 +540,7 @@ test('a superseded lock holder does not release the lock that replaced it', { sk 'releasing must not evict the holder that superseded us — that admits a third process alongside it' ); } finally { - process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR = previousDir; + restoreEnv('HARPER_INTEGRATION_TEST_MONITOR_DIR', previousDir); rmSync(monitorDir, { recursive: true, force: true }); } }); @@ -545,13 +554,15 @@ test('an unreadable registry is reported rather than silently read as empty', { // and every live instance on the machine loses the record that would have got it reaped. await writeFileAsync(join(monitorDir, 'registry.json'), '{"instances":[{"id":"trunc'); await rejects(readRegistryFile(), /is not valid JSON/); + await writeFileAsync(join(monitorDir, 'registry.json'), '{"instances":null}'); + await rejects(readRegistryFile(), /has no instance list/); } finally { - process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR = previousDir; + restoreEnv('HARPER_INTEGRATION_TEST_MONITOR_DIR', previousDir); rmSync(monitorDir, { recursive: true, force: true }); } }); -test('a contended registry lock cannot time out a Harper that already started', { skip: !isPosix }, async () => { +test('neither startup watchdog can time out a Harper that already reported ready', { skip: !isPosix }, async () => { const monitorDir = mkdtempSync(join(tmpdir(), 'harper-it-monitor-')); const previousDir = process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR; const previousEnabled = process.env.HARPER_INTEGRATION_TEST_MONITOR; @@ -567,20 +578,22 @@ test('a contended registry lock cannot time out a Harper that already started', args: [], env: {}, completionMessage: 'successfully started', - harperBinPath: fixtures['ready.cjs'], + // Both windows expire while registration is still blocked on the lock: the absolute one + // counting from launch, the idle one if the post-readiness log line re-arms it. + harperBinPath: fixtures['ready-then-log.cjs'], timeoutMs: 2000, maxMs: 2000, hostname: '127.0.0.1', }); - // Past the point where the startup deadline used to fire on a Harper that was already up. await sleep(3000); rmSync(lockPath, { force: true }); started = await starting; match(started.stdout, /successfully started/); + doesNotMatch(started.stdout, /post-readiness log line/, 'startupOutput is a snapshot taken at readiness'); } finally { if (started) started.process.kill('SIGKILL'); - process.env.HARPER_INTEGRATION_TEST_MONITOR_DIR = previousDir; - process.env.HARPER_INTEGRATION_TEST_MONITOR = previousEnabled; + restoreEnv('HARPER_INTEGRATION_TEST_MONITOR_DIR', previousDir); + restoreEnv('HARPER_INTEGRATION_TEST_MONITOR', previousEnabled); forceKill(await waitForMonitor(monitorDir, 5000).catch(() => undefined)); rmSync(monitorDir, { recursive: true, force: true }); } From 74f2dcd93d848f4bbfacd661fc3570fd5325a45c Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 10:38:00 -0600 Subject: [PATCH 09/14] fix(lifecycle): create the pending registry file with O_EXCL Round-3 review findings: - The pending file was written with a plain truncating open, so a symlink planted at its name in a shared registry directory would have been followed. `wx` (O_CREAT|O_EXCL) refuses to follow one, which the unique-per-write name already made free. - A failed token write leaked the lock file descriptor; close it in a finally. - `spawnSync`'s timeout escalates to SIGKILL, so a `ps` ignoring SIGTERM cannot outlive it. - Restore the caveat this PR dropped: Windows' `taskkill` shell-out may not complete from the 'exit' handler, and there is no monitor to fall back on. - Say "per user on the machine" where the docs still said "per machine". Co-Authored-By: Claude Opus --- README.md | 2 +- src/harperInstanceRegistry.ts | 21 ++++++++++++++++----- src/harperLifecycle.ts | 4 ++++ 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index ba2eff4..eef88c1 100644 --- a/README.md +++ b/README.md @@ -210,7 +210,7 @@ If you are not using `node:test`, use `createHarperContext()` to create a plain A test runner that dies without running its teardown — `SIGKILL`, a hard crash, a cancelled CI job — cannot reap the Harper instances it started, because those are deliberately detached into their own process groups so that whole-tree teardown works. Left alone, they hold their loopback address's fixed ports until the machine is rebooted. -To close that gap, `startHarper()` registers each instance in a small on-disk registry and makes sure a single shared **monitor** process is running. The monitor is not a per-instance sidecar: one is started on demand per registry directory (per machine, by default), every concurrent runner reuses it, and it exits once the registry has been empty for a while. Registry updates are published by renaming a complete file into place, so a runner killed mid-write leaves the previous registry — and therefore every other runner's reap targets — intact. It scans the registry on an interval and terminates — `SIGTERM`, then `SIGKILL` after a grace period — the process group of any instance whose owning runner is gone, or which has outlived its lifetime budget. Instances are matched by PID *and* process start time, so a recycled PID is never mistaken for a live one — on a host whose `ps` cannot report a start time (busybox/Alpine) this degrades to a PID-only check, which the monitor logs on startup. +To close that gap, `startHarper()` registers each instance in a small on-disk registry and makes sure a single shared **monitor** process is running. The monitor is not a per-instance sidecar: one is started on demand per registry directory (per user on the machine, by default), every concurrent runner reuses it, and it exits once the registry has been empty for a while. Registry updates are published by renaming a complete file into place, so a runner killed mid-write leaves the previous registry — and therefore every other runner's reap targets — intact. It scans the registry on an interval and terminates — `SIGTERM`, then `SIGKILL` after a grace period — the process group of any instance whose owning runner is gone, or which has outlived its lifetime budget. Instances are matched by PID *and* process start time, so a recycled PID is never mistaken for a live one — on a host whose `ps` cannot report a start time (busybox/Alpine) this degrades to a PID-only check, which the monitor logs on startup. The runner's own `exit`/`SIGINT`/`SIGTERM`/`SIGHUP` handlers still reap instances immediately on any exit it can observe; the monitor only handles the deaths it cannot. diff --git a/src/harperInstanceRegistry.ts b/src/harperInstanceRegistry.ts index 866bc42..36be67c 100644 --- a/src/harperInstanceRegistry.ts +++ b/src/harperInstanceRegistry.ts @@ -1,5 +1,5 @@ import { spawn, spawnSync } from 'node:child_process'; -import { mkdir, open, readFile, rename, stat, unlink, writeFile } from 'node:fs/promises'; +import { mkdir, open, readFile, rename, stat, unlink } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { setTimeout as sleep } from 'node:timers/promises'; @@ -13,7 +13,7 @@ import { fileURLToPath } from 'node:url'; * test runner dying without running cleanup — `SIGKILL`, a hard crash, a cancelled CI job — leaves * Harper alive holding its fixed ports. Instead of pairing every instance with its own sidecar, * each instance publishes explicit metadata here and a single shared monitor (one per registry - * directory, i.e. per machine by default) reaps whatever is orphaned or overdue. + * directory, i.e. per user on the machine by default) reaps whatever is orphaned or overdue. * * The registry is the contract between the two halves: `harperLifecycle.ts` registers and * deregisters instances, `harperMonitor.ts` scans and reaps them. Both agree on tunables through @@ -149,6 +149,7 @@ export function readProcessStartTimes(pids: number[]): Map { const result = spawnSync('ps', ['-o', 'pid=,lstart=', '-p', uniquePids.join(',')], { encoding: 'utf8', timeout: PS_TIMEOUT_MS, + killSignal: 'SIGKILL', }); // A non-zero status just means none of the PIDs exist; only a missing `ps` is worth noticing, // and there the empty map degrades callers to a plain PID check rather than reporting deaths. @@ -200,8 +201,11 @@ async function acquireLock(): Promise { while (true) { try { const lockFileHandle = await open(lockPath, 'wx'); - await lockFileHandle.writeFile(token); - await lockFileHandle.close(); + try { + await lockFileHandle.writeFile(token); + } finally { + await lockFileHandle.close(); + } return token; } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; @@ -285,7 +289,14 @@ export async function writeRegistryFile(registry: InstanceRegistry): Promise {}); diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index 5229223..97fe2b2 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -728,6 +728,10 @@ interface TrackedHarperProcess { * These parent-side hooks cover every exit the runner can still run JavaScript for. Death it * cannot observe — `SIGKILL`, a hard crash, a cancelled CI job — is covered by the shared instance * monitor, which reaps whatever the registry says is orphaned. See `harperInstanceRegistry.ts`. + * + * On POSIX the group `SIGKILL` is delivered synchronously, so it works even from the 'exit' + * handler. On Windows the `taskkill` shell-out may not complete before the runner exits, and there + * is no monitor to fall back on. */ const liveHarperProcesses = new Set(); let runnerCleanupRegistered = false; From a97688b94f4de0d1f684b725b1e5e9785a701486 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 10:46:40 -0600 Subject: [PATCH 10/14] fix(lifecycle): make pending registry names unguessable, keep the log unfollowable Round-4 review findings: - `O_EXCL` on the pending file turned a leftover from a killed writer plus PID reuse into a failed, silently unmonitored registration. The name now carries a random component, so it collides with nothing and cannot be pre-planted. - The monitor's log was appended with a plain open, the one remaining path in a shared registry directory that would follow a symlink. It now opens with O_NOFOLLOW at mode 0600. - Drop the two `off('data')`-less stdout listeners in the test helpers, and the last "per machine" wording the per-user default contradicts. Co-Authored-By: Claude Opus --- src/harperInstanceRegistry.ts | 12 ++++++------ src/harperMonitor.ts | 17 +++++++++++++---- test/harperLifecycle.test.ts | 27 +++++++++++++++++---------- 3 files changed, 36 insertions(+), 20 deletions(-) diff --git a/src/harperInstanceRegistry.ts b/src/harperInstanceRegistry.ts index 36be67c..f1d7f09 100644 --- a/src/harperInstanceRegistry.ts +++ b/src/harperInstanceRegistry.ts @@ -211,7 +211,7 @@ async function acquireLock(): Promise { if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; try { const lockFileStat = await stat(lockPath); - // A holder that died mid-section would otherwise wedge every runner on the machine. + // A holder that died mid-section would otherwise wedge every runner sharing this registry. if (Date.now() - lockFileStat.mtimeMs > LOCK_STALE_TIMEOUT_MS) await unlink(lockPath); } catch { // Another process removed it first; just retry. @@ -242,7 +242,7 @@ export async function withRegistryLock(callback: () => Promise): Promise { @@ -280,14 +280,14 @@ let pendingWriteCounter = 0; * Publishes by rename so readers see either the old registry or the new one. An in-place write * truncates first, and a writer killed in that window — the `SIGKILL` this whole mechanism exists * to survive — leaves torn JSON, which used to read as an empty registry and take every reap - * target on the machine with it. The pending name is unique per write because a fixed one could be - * truncated by a writer that reclaimed the lock as stale, reintroducing that same tearing; one - * left behind by a killed writer is inert. + * target on the machine with it. The pending name is unpredictable and unique per write: a fixed + * one could be truncated by a writer that reclaimed the lock as stale, and one reused after PID + * reuse would collide with a leftover file and fail the registration outright. */ export async function writeRegistryFile(registry: InstanceRegistry): Promise { await mkdir(getRegistryDir(), { recursive: true, mode: 0o700 }); const registryPath = getRegistryPath(); - const pendingPath = `${registryPath}.${process.pid}.${++pendingWriteCounter}.pending`; + const pendingPath = `${registryPath}.${process.pid}.${++pendingWriteCounter}.${Math.random().toString(36).slice(2)}.pending`; try { // `wx` (O_CREAT|O_EXCL) refuses to follow a symlink planted at the pending name, so a shared // registry directory cannot be turned into an arbitrary-write primitive. diff --git a/src/harperMonitor.ts b/src/harperMonitor.ts index 18eb858..e852cbd 100644 --- a/src/harperMonitor.ts +++ b/src/harperMonitor.ts @@ -1,5 +1,5 @@ -import { existsSync } from 'node:fs'; -import { appendFile } from 'node:fs/promises'; +import { constants, existsSync } from 'node:fs'; +import { open } from 'node:fs/promises'; import { setTimeout as sleep } from 'node:timers/promises'; import { getMonitorIdleExitMs, @@ -20,7 +20,7 @@ import { /** * The singleton Harper instance monitor. * - * One of these runs per registry directory, shared by every concurrent test runner on the machine. + * One of these runs per registry directory, shared by every concurrent test runner using it. * It periodically scans the registry written by `harperLifecycle.ts` and reaps any instance whose * owning runner has died or which has outlived its budget, then shuts itself down once the * registry has been empty long enough. See `harperInstanceRegistry.ts` for the shared contract. @@ -37,9 +37,18 @@ const escalationDeadlines = new Map(); let idleSince: number | undefined; let running = true; +// O_NOFOLLOW so a symlink planted at the log path in a shared registry directory cannot redirect +// these appends into a file the runner can write. +const LOG_FLAGS = constants.O_WRONLY | constants.O_CREAT | constants.O_APPEND | (constants.O_NOFOLLOW ?? 0); + async function log(message: string): Promise { try { - await appendFile(getMonitorLogPath(), `${new Date().toISOString()} [${process.pid}] ${message}\n`); + const logFileHandle = await open(getMonitorLogPath(), LOG_FLAGS, 0o600); + try { + await logFileHandle.writeFile(`${new Date().toISOString()} [${process.pid}] ${message}\n`); + } finally { + await logFileHandle.close(); + } } catch { // The registry directory is gone; the next loop iteration notices and shuts us down. } diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index dad9426..101b620 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -155,10 +155,13 @@ function fakeCtx(process?: ChildProcess): StartedHarperTestContext { function waitForOutput(child: ChildProcess, needle: string): Promise { return new Promise((resolve) => { let buffer = ''; - child.stdout?.on('data', (chunk: Buffer) => { + const onData = (chunk: Buffer) => { buffer += chunk.toString(); - if (buffer.includes(needle)) resolve(); - }); + if (!buffer.includes(needle)) return; + child.stdout?.off('data', onData); + resolve(); + }; + child.stdout?.on('data', onData); }); } @@ -166,15 +169,19 @@ function waitForOutput(child: ChildProcess, needle: string): Promise { function waitForMatch(child: ChildProcess, regex: RegExp, timeoutMs = 5000): Promise { return new Promise((resolve, reject) => { let buffer = ''; - const timeout = setTimeout(() => reject(new Error(`Timed out waiting for ${regex}; output: ${buffer}`)), timeoutMs); - child.stdout?.on('data', (chunk: Buffer) => { + const onData = (chunk: Buffer) => { buffer += chunk.toString(); const matched = buffer.match(regex); - if (matched) { - clearTimeout(timeout); - resolve(matched); - } - }); + if (!matched) return; + clearTimeout(timeout); + child.stdout?.off('data', onData); + resolve(matched); + }; + const timeout = setTimeout(() => { + child.stdout?.off('data', onData); + reject(new Error(`Timed out waiting for ${regex}; output: ${buffer}`)); + }, timeoutMs); + child.stdout?.on('data', onData); }); } From 76aecd0ab3d7c15b97a1e24fc327f76b8732e7be Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 10:53:18 -0600 Subject: [PATCH 11/14] fix(lifecycle): derive registry lock and pending names from a CSPRNG The pending name's random component is what the comment leans on when it calls the name unguessable, so take it from `randomBytes` rather than `Math.random`. Same for the lock token, in a file that now reasons explicitly about a shared registry directory another account can reach. Co-Authored-By: Claude Opus --- src/harperInstanceRegistry.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/harperInstanceRegistry.ts b/src/harperInstanceRegistry.ts index f1d7f09..5d8c870 100644 --- a/src/harperInstanceRegistry.ts +++ b/src/harperInstanceRegistry.ts @@ -1,4 +1,5 @@ import { spawn, spawnSync } from 'node:child_process'; +import { randomBytes } from 'node:crypto'; import { mkdir, open, readFile, rename, stat, unlink } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -193,7 +194,7 @@ function pidExists(pid: number): boolean { */ async function acquireLock(): Promise { const lockPath = getLockPath(); - const token = `${process.pid}-${++lockTokenCounter}-${Math.random().toString(36).slice(2)}`; + const token = `${process.pid}-${++lockTokenCounter}-${randomBytes(8).toString('hex')}`; await mkdir(getRegistryDir(), { recursive: true, mode: 0o700 }); // Bounded so a pathological holder — one refreshing the lock faster than it goes stale — surfaces // as a failed registration the caller can warn about, rather than a caller that never settles. @@ -287,7 +288,7 @@ let pendingWriteCounter = 0; export async function writeRegistryFile(registry: InstanceRegistry): Promise { await mkdir(getRegistryDir(), { recursive: true, mode: 0o700 }); const registryPath = getRegistryPath(); - const pendingPath = `${registryPath}.${process.pid}.${++pendingWriteCounter}.${Math.random().toString(36).slice(2)}.pending`; + const pendingPath = `${registryPath}.${process.pid}.${++pendingWriteCounter}.${randomBytes(8).toString('hex')}.pending`; try { // `wx` (O_CREAT|O_EXCL) refuses to follow a symlink planted at the pending name, so a shared // registry directory cannot be turned into an arbitrary-write primitive. From 08b0cd134041c2c7afa4485f84e3ace6d3d49436 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 8 Sep 2026 16:29:25 -0600 Subject: [PATCH 12/14] fix(lifecycle): finish reaping a group after its leader exits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The monitor's SIGTERM reaches the whole group, so Harper (no handler) exits first while a child that ignored the signal stays in that group holding the ports. Pruning the record the moment its leader was gone dropped the escalation deadline with it, so the SIGKILL never landed and the survivor outlived the registry that described it. A record now survives its leader while its process group still has members and the instance is still orphaned; POSIX keeps the group id reserved for exactly that long, so it remains ours to signal. Instance ids also carry a random suffix. Worker threads share process.pid and each holds its own copy of the registry module, so two workers' first starts both claimed `-1` and registration — which replaces same-id records — discarded one of two live instances, leaving it with nothing to reap it. Co-Authored-By: Claude Opus 5 --- src/harperInstanceRegistry.ts | 32 ++++++++- src/harperMonitor.ts | 38 +++++++---- test/harperLifecycle.test.ts | 125 ++++++++++++++++++++++++++++++++++ 3 files changed, 181 insertions(+), 14 deletions(-) diff --git a/src/harperInstanceRegistry.ts b/src/harperInstanceRegistry.ts index 5d8c870..f38d4fa 100644 --- a/src/harperInstanceRegistry.ts +++ b/src/harperInstanceRegistry.ts @@ -349,9 +349,17 @@ export function buildInstanceEnv(instanceId: string): Record { }; } -/** Allocates the id used for both the instance's environment markers and its registry record. */ +/** + * Allocates the id used for both the instance's environment markers and its registry record. + * + * The random suffix is what makes it unique, not the counter: worker threads share `process.pid` + * but each gets its own copy of this module, so two workers' first starts would otherwise both + * claim `-1` and `registerHarperInstance` would drop the earlier record — leaving a live + * instance with nothing in the registry to reap it. The PID and counter stay for legibility in + * `ps`, the monitor log, and `/proc//environ`. + */ export function nextInstanceId(): string { - return `${process.pid}-${++instanceCounter}`; + return `${process.pid}-${++instanceCounter}-${randomBytes(6).toString('hex')}`; } /** @@ -414,6 +422,26 @@ export async function deregisterHarperInstance(id: string): Promise { } } +/** + * Whether a process group still has members, used to decide when an instance's cleanup is + * finished. A group outlives its leader: Harper exiting on `SIGTERM` leaves any child that + * ignored the signal running in the same group, still holding the ports. + * + * Safe to act on for a group we recorded, because POSIX reserves a process-group id for as long + * as the group has members — so a group that answers here is still ours, not a recycled id. + */ +export function processGroupExists(pgid: number): boolean { + // Group 0 is "the caller's own group" and 1 is init's; neither can be an instance we recorded. + if (!Number.isInteger(pgid) || pgid <= 1) return false; + try { + process.kill(-pgid, 0); + return true; + } catch (error) { + // EPERM means members exist that we may not signal — still a live group. + return (error as NodeJS.ErrnoException).code === 'EPERM'; + } +} + /** * Signals a whole process group. Instances are spawned detached, so the instance PID is also its * process-group id and this reaches Harper plus anything it spawned. diff --git a/src/harperMonitor.ts b/src/harperMonitor.ts index e852cbd..c473c96 100644 --- a/src/harperMonitor.ts +++ b/src/harperMonitor.ts @@ -8,6 +8,7 @@ import { getReapGraceMs, getRegistryPath, isSameProcessAlive, + processGroupExists, readProcessIdentity, readProcessStartTimes, readRegistryFile, @@ -94,10 +95,19 @@ interface ReapTarget { reason: string; } +/** Why an instance should be reaped, or undefined while it is still legitimately running. */ +function reapReason(instance: HarperInstanceRecord, startTimes: Map, now: number): string | undefined { + if (!isSameProcessAlive(instance.owner, startTimes)) return `owning runner ${instance.owner.pid} is gone`; + if (now > instance.expiresAt) { + return `exceeded its ${Math.round((instance.expiresAt - instance.registeredAt) / 1000)}s lifetime budget`; + } + return undefined; +} + /** - * Prunes records whose process is gone and returns the ones that should be reaped. + * Prunes records whose process group is gone and returns the ones that should be reaped. * - * Reaped instances stay in the registry until their process actually disappears, so a monitor + * Reaped instances stay in the registry until their processes actually disappear, so a monitor * killed mid-grace leaves a target its successor picks straight back up. */ async function scanRegistry(): Promise<{ live: HarperInstanceRecord[]; targets: ReapTarget[] }> { @@ -110,16 +120,19 @@ async function scanRegistry(): Promise<{ live: HarperInstanceRecord[]; targets: const targets: ReapTarget[] = []; const now = Date.now(); for (const instance of registry.instances) { - if (!isSameProcessAlive(instance, startTimes)) continue; - live.push(instance); - if (!isSameProcessAlive(instance.owner, startTimes)) { - targets.push({ instance, reason: `owning runner ${instance.owner.pid} is gone` }); - } else if (now > instance.expiresAt) { - targets.push({ - instance, - reason: `exceeded its ${Math.round((instance.expiresAt - instance.registeredAt) / 1000)}s lifetime budget`, - }); + const reason = reapReason(instance, startTimes, now); + if (!isSameProcessAlive(instance, startTimes)) { + // The record has to outlive its leader while the group is still running and still + // orphaned: our own `SIGTERM` exits Harper, and a child that ignored it stays in the + // group holding the ports, so dropping the record here would cancel the `SIGKILL` + // escalation and strand exactly what this monitor exists to reap. POSIX reserves the + // group id for as long as the group has members, so it is still ours to signal. With + // the owner alive there is nothing to escalate, and nothing to hold either: the runner + // deregisters the record by id as soon as it observes the exit. + if (reason === undefined || !processGroupExists(instance.pid)) continue; } + live.push(instance); + if (reason !== undefined) targets.push({ instance, reason }); } if (live.length !== registry.instances.length) { registry.instances = live; @@ -133,7 +146,8 @@ async function scanRegistry(): Promise<{ live: HarperInstanceRecord[]; targets: * Terminates an instance's process group: `SIGTERM` first so Harper can flush and release its * ports cleanly, escalating to `SIGKILL` on a later scan if it is still alive after the grace * period. Killing by group id is safe here because the record's start time already proved the - * group leader is the process we registered, not a recycled PID. + * group leader is the process we registered, not a recycled PID — and once that leader is gone, + * because the id stays reserved while the group it led still has members. */ async function reap({ instance, reason }: ReapTarget): Promise { const escalateAt = escalationDeadlines.get(instance.id); diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 101b620..147fc93 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -50,6 +50,9 @@ const { spawn } = require('node:child_process'); const { createServer } = require('node:net'); const host = '127.0.0.1'; if (process.env.HARPER_FAKE_DESCENDANT === '1') { + // A child that ignores SIGTERM outlives the group leader the monitor's SIGTERM does kill, and + // keeps its listener — the case where reaping has to continue after the leader is gone. + if (process.env.HARPER_DESCENDANT_IGNORE_TERM === '1') process.on('SIGTERM', () => {}); const server = createServer(); server.listen(0, host, () => { process.stdout.write('descendant-ready:' + process.pid + ':' + server.address().port + '\\n'); @@ -104,6 +107,7 @@ const result = await runHarperCommand({ args: [], env: { HARPER_TERM_DELAY_MS: process.env.HARPER_TERM_DELAY_MS, + HARPER_DESCENDANT_IGNORE_TERM: process.env.HARPER_DESCENDANT_IGNORE_TERM, }, completionMessage: 'successfully started', harperBinPath: process.env.HARPER_FAKE_SCRIPT, @@ -122,6 +126,37 @@ if (process.env.HARPER_RUNNER_MODE === 'teardown') { } else { setInterval(() => {}, 1000); } +`, + // One runner process starting a Harper instance from each of two worker threads — the shape a + // worker-pooled test runner (vitest threads, and anything else sharing one process) produces. + // Both workers share process.pid, so only an id unique per module copy keeps both records. + 'worker-runner.mjs': ` +import { Worker } from 'node:worker_threads'; +for (const index of [0, 1]) { + const worker = new Worker(process.env.HARPER_WORKER_SCRIPT, { workerData: { index } }); + worker.on('message', (message) => process.stdout.write(message + '\\n')); + worker.on('error', (error) => { + process.stderr.write('worker ' + index + ' failed: ' + (error.stack || error) + '\\n'); + process.exit(1); + }); +} +setInterval(() => {}, 1000); +`, + 'worker-harper.mjs': ` +import { parentPort, workerData } from 'node:worker_threads'; +const { runHarperCommand } = await import(process.env.HARPER_LIFECYCLE_URL); +const result = await runHarperCommand({ + args: [], + env: {}, + completionMessage: 'successfully started', + harperBinPath: process.env.HARPER_FAKE_SCRIPT, + timeoutMs: 5000, + maxMs: 10000, + hostname: '127.0.0.1', +}); +const match = result.stdout.match(/tree-ready:(\\d+):(\\d+):(\\d+):(\\d+)/); +if (!match) throw new Error('Missing fake Harper process markers: ' + result.stdout); +parentPort.postMessage('worker-ready:' + workerData.index + ':' + match[1] + ':' + match[2] + ':' + match[3] + ':' + match[4]); `, }; @@ -185,6 +220,26 @@ function waitForMatch(child: ChildProcess, regex: RegExp, timeoutMs = 5000): Pro }); } +/** Resolves with all `count` matches of a global regex once they have appeared on the child's stdout. */ +function waitForMatches(child: ChildProcess, regex: RegExp, count: number, timeoutMs = 15000): Promise { + return new Promise((resolve, reject) => { + let buffer = ''; + const onData = (chunk: Buffer) => { + buffer += chunk.toString(); + const matched = [...buffer.matchAll(regex)]; + if (matched.length < count) return; + clearTimeout(timeout); + child.stdout?.off('data', onData); + resolve(matched); + }; + const timeout = setTimeout(() => { + child.stdout?.off('data', onData); + reject(new Error(`Timed out waiting for ${count} matches of ${regex}; output: ${buffer}`)); + }, timeoutMs); + child.stdout?.on('data', onData); + }); +} + /** Polls (signal 0) until `pid` no longer exists, or the timeout elapses. */ async function waitProcessGone(pid: number, timeoutMs: number): Promise { const deadline = Date.now() + timeoutMs; @@ -214,6 +269,8 @@ interface RunningFakeHarperTree { interface FakeHarperTreeOptions { mode?: 'teardown'; + /** Make the descendant ignore `SIGTERM`, so it outlives the leader the monitor's `SIGTERM` kills. */ + descendantIgnoresTerm?: boolean; /** Share an existing registry directory (and therefore an existing monitor) with another tree. */ monitorDir?: string; /** `HARPER_INTEGRATION_TEST_MONITOR_IDLE_MS` for the monitor this runner may start. */ @@ -261,6 +318,7 @@ async function startFakeHarperTree(options: FakeHarperTreeOptions = {}): Promise HARPER_LIFECYCLE_URL: new URL('../src/harperLifecycle.ts', import.meta.url).href, HARPER_FAKE_SCRIPT: fixtures['process-tree.cjs'], HARPER_TERM_DELAY_MS: options.mode === 'teardown' ? '300' : '0', + HARPER_DESCENDANT_IGNORE_TERM: options.descendantIgnoresTerm ? '1' : '0', HARPER_RUNNER_MODE: options.mode, // Re-enable monitoring (this test process disables it) against a private registry, with // timings compressed so an orphan is reaped in well under a test timeout. @@ -409,6 +467,27 @@ test('runner SIGKILL leaves the monitor to reap the orphaned Harper tree and rel } }); +test('the monitor finishes reaping a group whose leader dies before its children', { skip: !isPosix }, async () => { + // The monitor's SIGTERM lands on the whole group, so the leader (no handler) exits first while a + // child that ignores SIGTERM keeps running — and keeps the port. Dropping the record the moment + // its leader is gone would cancel the SIGKILL escalation and strand exactly that child. + const tree = await startFakeHarperTree({ descendantIgnoresTerm: true }); + try { + await waitForMonitor(tree.monitorDir); + strictEqual(await isPortFree('127.0.0.1', tree.ports[1]), false); + tree.runner.kill('SIGKILL'); + await once(tree.runner, 'exit'); + ok(await waitProcessGone(tree.harperPid, 10000), `leader ${tree.harperPid} should exit on the monitor's SIGTERM`); + ok( + await waitProcessGone(tree.descendantPid, 15000), + `TERM-ignoring descendant ${tree.descendantPid} should still be escalated to SIGKILL` + ); + ok(await waitForPortsFree('127.0.0.1', tree.ports, 5000, 50), 'the surviving child must not keep its port'); + } finally { + await cleanupFakeHarperTree(tree); + } +}); + test('runner SIGHUP reaps the Harper tree and releases its ports', { skip: !isPosix }, async () => { const tree = await startFakeHarperTree(); try { @@ -456,6 +535,52 @@ test('concurrent runners share one monitor', { skip: !isPosix }, async () => { } }); +test('instances started from two worker threads of one runner are registered and reaped separately', { skip: !isPosix }, async () => { + // Worker threads share process.pid and each gets its own copy of the registry module, so an id + // built from pid + a module-local counter collides on their first starts — and registration + // replaces same-id records, leaving one of two live instances with nothing to reap it. + const monitorDir = mkdtempSync(join(tmpdir(), 'harper-it-monitor-')); + const runner = spawn(process.execPath, [fixtures['worker-runner.mjs']], { + env: { + ...process.env, + HARPER_WORKER_SCRIPT: fixtures['worker-harper.mjs'], + HARPER_LIFECYCLE_URL: new URL('../src/harperLifecycle.ts', import.meta.url).href, + HARPER_FAKE_SCRIPT: fixtures['process-tree.cjs'], + HARPER_INTEGRATION_TEST_MONITOR: 'on', + HARPER_INTEGRATION_TEST_MONITOR_DIR: monitorDir, + HARPER_INTEGRATION_TEST_MONITOR_INTERVAL_MS: '200', + HARPER_INTEGRATION_TEST_MONITOR_REAP_GRACE_MS: '500', + HARPER_INTEGRATION_TEST_MONITOR_IDLE_MS: '60000', + HARPER_INTEGRATION_TEST_INSTANCE_MAX_LIFETIME_MS: '', + }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + const pids: number[] = []; + const ports: number[] = []; + try { + const ready = await waitForMatches(runner, /worker-ready:\d+:(\d+):(\d+):(\d+):(\d+)/g, 2, 20000); + for (const worker of ready) { + pids.push(Number(worker[1]), Number(worker[2])); + ports.push(Number(worker[3]), Number(worker[4])); + } + await waitForMonitor(monitorDir); + const registry = await readMonitorRegistry(monitorDir); + strictEqual(registry.instances.length, 2, 'each worker should get its own registry record'); + strictEqual(new Set(registry.instances.map((instance) => instance.id)).size, 2, 'their ids must not collide'); + + runner.kill('SIGKILL'); + await once(runner, 'exit'); + for (const pid of pids) ok(await waitProcessGone(pid, 15000), `${pid} should be reaped with its runner`); + ok(await waitForPortsFree('127.0.0.1', ports, 5000, 50), 'both trees should release their ports'); + } finally { + forceKill(runner.pid); + for (const pid of pids) forceKill(pid); + forceKill(await waitForMonitor(monitorDir, 5000).catch(() => undefined)); + rmSync(monitorDir, { recursive: true, force: true }); + if (ports.length) await waitForPortsFree('127.0.0.1', ports, 2000, 50); + } +}); + test('an instance that outlives its lifetime budget is reaped while its runner is still alive', { skip: !isPosix }, async () => { // Owner death is the sharp signal; this budget is the backstop for a runner PID recycled by // some other long-lived process, which would otherwise look alive forever. From a8f7be44b68b0fcade6b3f162c42cfbd37b086dc Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 8 Sep 2026 17:01:57 -0600 Subject: [PATCH 13/14] fix(lifecycle): let the monitor alone decide a group is finished MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A registry record describes a process group, so its lifetime is the group's, not its leader's. Two writers ended it at the leader's exit instead: the monitor pruned any record whose leader was gone, and the runner deregistered on the exit event. Either erases the only durable description of a group whose child ignored SIGTERM and kept the ports — and with it the SIGKILL escalation, the lifetime backstop, and every later chance to reap. Removal now belongs to the monitor alone, which is the half that can see the group, and it happens when the group has no members left rather than when a reap is due. A leader that exits on its own with a live runner is the case that makes those two different: nothing is due yet, but the survivors still have to be remembered. Holding a record past its leader also has to survive PID reuse. A leader PID that ps still describes with a different start time is a reused id, and its group is not ours to signal. An id reused after our group ended unobserved stays indistinguishable — the same best-effort bar as the rest of the identity checks here, now stated in the code and the README rather than claimed away. Co-Authored-By: Claude Opus 5 --- CONTRIBUTING.md | 2 +- README.md | 4 +- src/harperInstanceRegistry.ts | 47 +++++++----------- src/harperLifecycle.ts | 17 ++----- src/harperMonitor.ts | 40 ++++++++++----- test/harperLifecycle.test.ts | 93 +++++++++++++++++++++++++++++++++++ 6 files changed, 147 insertions(+), 56 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 81541e3..8f7b180 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -6,7 +6,7 @@ Contributors are encouraged to communicate with maintainers in issues or other c Source files are located in `src/`. These are built to the `dist/` directory. The published package includes `dist/`, `scripts/`, and the regular npm metadata and documentation files. -The `src/index.ts` is the source for the main export. This is the public re-export of all the various utilities from `src/harperLifecycle.ts`, `targz.ts`, and more. The `src/run.ts` is the source for the `harper-integration-test-run` bin script. The internal `src/harperInstanceRegistry.ts` publishes each running Harper instance to a shared on-disk registry, and `src/harperMonitor.ts` is the singleton monitor process that reads it and reaps instances whose test runner died without cleaning up (see README's *Orphaned Instance Monitor* section). And the `scripts/setup-loopback.sh` is the source for the `harper-integration-test-setup-loopback` bin script. +The `src/index.ts` is the source for the main export. This is the public re-export of all the various utilities from `src/harperLifecycle.ts`, `targz.ts`, and more. The `src/run.ts` is the source for the `harper-integration-test-run` bin script. The internal `src/harperInstanceRegistry.ts` publishes each running Harper instance to a shared on-disk registry, and `src/harperMonitor.ts` is the singleton monitor process that reads it and reaps instances whose test runner died without cleaning up (see README's *Orphaned Instance Monitor* section). A registry record covers a whole process group, so the monitor — the only half that can see when that group is finished — is also the only writer that removes one; the lifecycle side registers and never deregisters. And the `scripts/setup-loopback.sh` is the source for the `harper-integration-test-setup-loopback` bin script. The package is `"type": "module"` — all source files are ESM by default. diff --git a/README.md b/README.md index eef88c1..aaecbcb 100644 --- a/README.md +++ b/README.md @@ -210,7 +210,9 @@ If you are not using `node:test`, use `createHarperContext()` to create a plain A test runner that dies without running its teardown — `SIGKILL`, a hard crash, a cancelled CI job — cannot reap the Harper instances it started, because those are deliberately detached into their own process groups so that whole-tree teardown works. Left alone, they hold their loopback address's fixed ports until the machine is rebooted. -To close that gap, `startHarper()` registers each instance in a small on-disk registry and makes sure a single shared **monitor** process is running. The monitor is not a per-instance sidecar: one is started on demand per registry directory (per user on the machine, by default), every concurrent runner reuses it, and it exits once the registry has been empty for a while. Registry updates are published by renaming a complete file into place, so a runner killed mid-write leaves the previous registry — and therefore every other runner's reap targets — intact. It scans the registry on an interval and terminates — `SIGTERM`, then `SIGKILL` after a grace period — the process group of any instance whose owning runner is gone, or which has outlived its lifetime budget. Instances are matched by PID *and* process start time, so a recycled PID is never mistaken for a live one — on a host whose `ps` cannot report a start time (busybox/Alpine) this degrades to a PID-only check, which the monitor logs on startup. +To close that gap, `startHarper()` registers each instance in a small on-disk registry and makes sure a single shared **monitor** process is running. The monitor is not a per-instance sidecar: one is started on demand per registry directory (per user on the machine, by default), every concurrent runner reuses it, and it exits once the registry has been empty for a while. Registry updates are published by renaming a complete file into place, so a runner killed mid-write leaves the previous registry — and therefore every other runner's reap targets — intact. It scans the registry on an interval and terminates — `SIGTERM`, then `SIGKILL` after a grace period — the process group of any instance whose owning runner is gone, or which has outlived its lifetime budget. Instances are matched by PID *and* process start time, which narrows PID reuse rather than eliminating it — a recycled PID that `ps` can still describe is rejected, and on a host whose `ps` cannot report a start time (busybox/Alpine) this degrades to a PID-only check, which the monitor logs on startup. + +A record describes a process *group*, not one process, and the monitor is the only thing that removes one — when that group has no members left. Harper exiting is not the end of the group: a child that ignored the `SIGTERM` stays in it, still holding the ports, and the record is what remembers the group long enough to escalate to `SIGKILL` or to reap it later when its runner dies. The runner's own `exit`/`SIGINT`/`SIGTERM`/`SIGHUP` handlers still reap instances immediately on any exit it can observe; the monitor only handles the deaths it cannot. diff --git a/src/harperInstanceRegistry.ts b/src/harperInstanceRegistry.ts index f38d4fa..7b2d92a 100644 --- a/src/harperInstanceRegistry.ts +++ b/src/harperInstanceRegistry.ts @@ -16,9 +16,11 @@ import { fileURLToPath } from 'node:url'; * each instance publishes explicit metadata here and a single shared monitor (one per registry * directory, i.e. per user on the machine by default) reaps whatever is orphaned or overdue. * - * The registry is the contract between the two halves: `harperLifecycle.ts` registers and - * deregisters instances, `harperMonitor.ts` scans and reaps them. Both agree on tunables through - * the environment, which the monitor inherits from whichever runner first spawned it. + * The registry is the contract between the two halves: `harperLifecycle.ts` registers instances, + * `harperMonitor.ts` scans, reaps, and removes them. Removal belongs to the monitor alone: a record + * describes a process *group*, and a runner watching only its direct child cannot tell when that + * group is finished. Both agree on tunables through the environment, which the monitor inherits + * from whichever runner first spawned it. * * POSIX only. Reaping is `kill(-pgid)`, which Windows has no equivalent for; on Windows * registration is skipped and the runner-side cleanup handlers in `harperLifecycle.ts` remain the @@ -177,6 +179,15 @@ export function isSameProcessAlive(identity: ProcessIdentity, startTimes: Map): boolean { + const currentStartTime = startTimes.get(identity.pid); + return currentStartTime !== undefined && identity.startTime !== undefined && currentStartTime !== identity.startTime; +} + function pidExists(pid: number): boolean { try { process.kill(pid, 0); @@ -352,11 +363,10 @@ export function buildInstanceEnv(instanceId: string): Record { /** * Allocates the id used for both the instance's environment markers and its registry record. * - * The random suffix is what makes it unique, not the counter: worker threads share `process.pid` - * but each gets its own copy of this module, so two workers' first starts would otherwise both - * claim `-1` and `registerHarperInstance` would drop the earlier record — leaving a live - * instance with nothing in the registry to reap it. The PID and counter stay for legibility in - * `ps`, the monitor log, and `/proc//environ`. + * Uniqueness comes from the random suffix, not the counter: worker threads share `process.pid` and + * each holds its own copy of this module, so both first starts would claim `-1` and + * registration — which replaces same-id records — would leave one live instance unreapable. The + * PID and counter stay for legibility in `ps` and the monitor log. */ export function nextInstanceId(): string { return `${process.pid}-${++instanceCounter}-${randomBytes(6).toString('hex')}`; @@ -401,27 +411,6 @@ export async function registerHarperInstance(instance: { if (monitorNeeded) spawnMonitor(); } -/** - * Removes an instance from the registry after normal teardown. - * - * Best-effort — a record left behind by an abrupt exit is pruned by the monitor as soon as the - * Harper PID is gone, so a missed deregistration costs a log line, not a stale reap target. - */ -export async function deregisterHarperInstance(id: string): Promise { - if (!isInstanceMonitorEnabled()) return; - try { - await withRegistryLock(async () => { - const registry = await readRegistryFile(); - const remaining = registry.instances.filter((instance) => instance.id !== id); - if (remaining.length === registry.instances.length) return; - registry.instances = remaining; - await writeRegistryFile(registry); - }); - } catch (error) { - console.warn(`[harper-monitor] Failed to deregister Harper instance ${id}: ${(error as Error).message}`); - } -} - /** * Whether a process group still has members, used to decide when an instance's cleanup is * finished. A group outlives its leader: Harper exiting on `SIGTERM` leaves any child that diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index 97fe2b2..97a3ed6 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -8,12 +8,7 @@ import { getNextAvailableLoopbackAddress, releaseLoopbackAddress } from './loopb import { waitForPortsFree } from './portUtils.ts'; import { ok, equal } from 'node:assert'; import { createRequire } from 'node:module'; -import { - buildInstanceEnv, - deregisterHarperInstance, - nextInstanceId, - registerHarperInstance, -} from './harperInstanceRegistry.ts'; +import { buildInstanceEnv, nextInstanceId, registerHarperInstance } from './harperInstanceRegistry.ts'; /** * Minimal context interface required by startHarper/teardownHarper. @@ -747,12 +742,10 @@ function trackHarperProcess(proc: ChildProcess, instanceId: string, hostname?: s }); const trackedProcess: TrackedHarperProcess = { registered }; - proc.once('exit', () => { - liveHarperProcesses.delete(proc); - // Chained on registration so a fast exit cannot deregister before the record exists. Best - // effort either way: the monitor prunes records whose process is gone. - void registered.then(() => deregisterHarperInstance(instanceId)); - }); + // Only the direct child is untracked here. Its registry record describes the whole process + // group, which can outlive it, so removing that record is the monitor's call — it is the half + // that can see when the group is actually finished. + proc.once('exit', () => liveHarperProcesses.delete(proc)); if (runnerCleanupRegistered) return trackedProcess; runnerCleanupRegistered = true; diff --git a/src/harperMonitor.ts b/src/harperMonitor.ts index c473c96..6c1f978 100644 --- a/src/harperMonitor.ts +++ b/src/harperMonitor.ts @@ -7,6 +7,7 @@ import { getMonitorScanIntervalMs, getReapGraceMs, getRegistryPath, + isProcessIdentityReused, isSameProcessAlive, processGroupExists, readProcessIdentity, @@ -95,6 +96,21 @@ interface ReapTarget { reason: string; } +/** + * Whether the instance's group still has members now that its leader is gone — our own `SIGTERM` + * exits Harper, and a child that ignored it stays in that group holding the ports. + * + * A PID reporting a *different* start time is not that case but a reused id, and its group belongs + * to something unrelated. Absence is the best evidence available, not proof: a group id is reserved + * only for the lifetime of the group that held it, so a group that ended between two scans, had its + * id reused, and then lost its own leader is indistinguishable from ours here. That is the same + * best-effort bar as every other identity check in this registry (`ps` start times narrow PID reuse + * rather than eliminating it), and the scan interval is what bounds it. + */ +function groupOutlivedLeader(instance: HarperInstanceRecord, startTimes: Map): boolean { + return !isProcessIdentityReused(instance, startTimes) && processGroupExists(instance.pid); +} + /** Why an instance should be reaped, or undefined while it is still legitimately running. */ function reapReason(instance: HarperInstanceRecord, startTimes: Map, now: number): string | undefined { if (!isSameProcessAlive(instance.owner, startTimes)) return `owning runner ${instance.owner.pid} is gone`; @@ -120,18 +136,16 @@ async function scanRegistry(): Promise<{ live: HarperInstanceRecord[]; targets: const targets: ReapTarget[] = []; const now = Date.now(); for (const instance of registry.instances) { - const reason = reapReason(instance, startTimes, now); - if (!isSameProcessAlive(instance, startTimes)) { - // The record has to outlive its leader while the group is still running and still - // orphaned: our own `SIGTERM` exits Harper, and a child that ignored it stays in the - // group holding the ports, so dropping the record here would cancel the `SIGKILL` - // escalation and strand exactly what this monitor exists to reap. POSIX reserves the - // group id for as long as the group has members, so it is still ours to signal. With - // the owner alive there is nothing to escalate, and nothing to hold either: the runner - // deregisters the record by id as soon as it observes the exit. - if (reason === undefined || !processGroupExists(instance.pid)) continue; - } + // A record is the cleanup state of a process group, so it lives as long as that group and + // not as long as its leader — the two differ precisely when it matters. Our own SIGTERM + // exits Harper first, and a child that ignored it stays in the group holding the ports; + // dropping the record there would cancel the SIGKILL escalation. A leader that exits on + // its own is the same picture without the signal: whether the survivors are reaped now, + // later when their runner dies, or at the lifetime budget, this record is what remembers + // them, so retention cannot depend on the reap being due yet. + if (!isSameProcessAlive(instance, startTimes) && !groupOutlivedLeader(instance, startTimes)) continue; live.push(instance); + const reason = reapReason(instance, startTimes, now); if (reason !== undefined) targets.push({ instance, reason }); } if (live.length !== registry.instances.length) { @@ -146,8 +160,8 @@ async function scanRegistry(): Promise<{ live: HarperInstanceRecord[]; targets: * Terminates an instance's process group: `SIGTERM` first so Harper can flush and release its * ports cleanly, escalating to `SIGKILL` on a later scan if it is still alive after the grace * period. Killing by group id is safe here because the record's start time already proved the - * group leader is the process we registered, not a recycled PID — and once that leader is gone, - * because the id stays reserved while the group it led still has members. + * group leader is the process we registered, not a recycled PID — or, once that leader is gone, on + * the narrower evidence `groupOutlivedLeader` describes. */ async function reap({ instance, reason }: ReapTarget): Promise { const escalateAt = escalationDeadlines.get(instance.id); diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 147fc93..6644076 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -594,6 +594,99 @@ test('an instance that outlives its lifetime budget is reaped while its runner i } }); +test('a reap in flight is finished even though its runner is alive to see the leader exit', { skip: !isPosix }, async () => { + // The leader's exit ends this reap as far as the still-live runner can see, and the runner used + // to remove the record on it — dropping the escalation just as surely as the monitor's own + // pruning did, while the TERM-ignoring descendant kept the port. + const tree = await startFakeHarperTree({ maxLifetimeMs: '250', descendantIgnoresTerm: true }); + try { + ok(await waitProcessGone(tree.harperPid, 10000), `overdue Harper ${tree.harperPid} should be reaped`); + ok( + await waitProcessGone(tree.descendantPid, 15000), + `its TERM-ignoring descendant ${tree.descendantPid} should still be escalated to SIGKILL` + ); + strictEqual(tree.runner.exitCode, null, 'the owning runner should be untouched'); + ok(await waitForPortsFree('127.0.0.1', tree.ports, 5000, 50), 'Harper tree ports should be reusable'); + } finally { + await cleanupFakeHarperTree(tree); + } +}); + +test('a group that outlives its leader stays reapable until its runner dies', { skip: !isPosix }, async () => { + // Nothing is due to be reaped here — the runner is alive and the budget is unexpired — so this + // is the case that says retention cannot be conditional on a reap being due. The record is the + // only description of the surviving group; the runner's own exit handler cannot cover it, + // having dropped the leader from its live set the moment it exited. + const tree = await startFakeHarperTree(); + try { + await waitForMonitor(tree.monitorDir); + process.kill(tree.harperPid, 'SIGKILL'); + ok(await waitProcessGone(tree.harperPid, 5000), `leader ${tree.harperPid} should be gone`); + await sleep(1000); // several scan intervals + strictEqual( + (await readMonitorRegistry(tree.monitorDir)).instances.length, + 1, + 'the record should survive the leader while the group still holds a port' + ); + strictEqual(await isPortFree('127.0.0.1', tree.ports[1]), false); + + tree.runner.kill('SIGKILL'); + await once(tree.runner, 'exit'); + ok(await waitProcessGone(tree.descendantPid, 15000), `descendant ${tree.descendantPid} should be reaped with its runner`); + ok(await waitForPortsFree('127.0.0.1', tree.ports, 5000, 50), 'its port should come back'); + } finally { + await cleanupFakeHarperTree(tree); + } +}); + +test('a record whose leader PID has been reused is dropped, not signalled', { skip: !isPosix }, async () => { + // Holding a record past its leader's death is only safe while that PID is *absent*: a group id + // stays reserved for the lifetime of its group, so a PID running something else means the group + // ended and its id was handed out again. + const monitorDir = mkdtempSync(join(tmpdir(), 'harper-it-monitor-')); + const bystander = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { detached: true, stdio: 'ignore' }); + const exitedOwner = spawn(process.execPath, ['-e', '']); + await once(exitedOwner, 'exit'); + let monitor: ChildProcess | undefined; + try { + await writeFileAsync( + join(monitorDir, 'registry.json'), + JSON.stringify({ + instances: [ + { + id: 'stale', + pid: bystander.pid, + // Not the bystander's start time: this record describes whatever held the PID before it. + startTime: 'Mon Jan 1 00:00:00 2001', + owner: { pid: exitedOwner.pid, startTime: 'Mon Jan 1 00:00:00 2001' }, + registeredAt: Date.now(), + expiresAt: Date.now() + 3600000, + }, + ], + }) + ); + monitor = spawn(process.execPath, [MONITOR_SCRIPT, MONITOR_ARGV_MARKER], { + env: { + ...process.env, + HARPER_INTEGRATION_TEST_MONITOR_DIR: monitorDir, + HARPER_INTEGRATION_TEST_MONITOR_INTERVAL_MS: '200', + HARPER_INTEGRATION_TEST_MONITOR_REAP_GRACE_MS: '200', + HARPER_INTEGRATION_TEST_MONITOR_IDLE_MS: '60000', + }, + stdio: 'ignore', + }); + await waitForMonitor(monitorDir); + await sleep(1500); + strictEqual(bystander.signalCode, null, 'an unrelated process group must not be signalled for a recycled PID'); + strictEqual(bystander.exitCode, null, 'the bystander should still be running'); + strictEqual((await readMonitorRegistry(monitorDir)).instances.length, 0, 'the stale record should be pruned instead'); + } finally { + forceKill(monitor?.pid); + forceKill(bystander.pid); + rmSync(monitorDir, { recursive: true, force: true }); + } +}); + test('the monitor shuts down once no instances remain', { skip: !isPosix }, async () => { const tree = await startFakeHarperTree({ idleMs: '500' }); try { From e7adea253c7fc5a3abf0901e0b1ac9eb6a4075b5 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 8 Sep 2026 17:23:38 -0600 Subject: [PATCH 14/14] fix(lifecycle): make process identity and reap targets environment-proof MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ps -o lstart=` renders in the caller's timezone and locale, so two runners configured differently recorded different strings for the same process and read each other's live records as PID reuse — a monitor started beside the live one, and scans discarding instances that were still running. The lookup now pins TZ=UTC and LC_ALL=C, making the string a property of the process alone. `signalProcessGroup` also refuses a group id of 1 or below. The registry is on-disk state a corrupt or planted record can reach, and `kill(-1)` broadcasts to every process the monitor may signal; no instance we register is ever that. The test helper's `forceKill` refuses the same ids, where an unparsed fixture PID of 0 would have signalled the test runner's own group. Co-Authored-By: Claude Opus 5 --- src/harperInstanceRegistry.ts | 19 ++++++++---- src/harperLifecycle.ts | 5 ++-- src/harperMonitor.ts | 34 ++++++++++----------- test/harperLifecycle.test.ts | 56 ++++++++++++++++++++++++++++------- 4 files changed, 75 insertions(+), 39 deletions(-) diff --git a/src/harperInstanceRegistry.ts b/src/harperInstanceRegistry.ts index 7b2d92a..e0b553c 100644 --- a/src/harperInstanceRegistry.ts +++ b/src/harperInstanceRegistry.ts @@ -153,6 +153,10 @@ export function readProcessStartTimes(pids: number[]): Map { encoding: 'utf8', timeout: PS_TIMEOUT_MS, killSignal: 'SIGKILL', + // `lstart` is rendered in the caller's timezone and locale, so runners configured differently + // would record different strings for one process and read each other's records as PID reuse — + // discarding live instances. Pin both so the string is a property of the process alone. + env: { ...process.env, TZ: 'UTC', LC_ALL: 'C' }, }); // A non-zero status just means none of the PIDs exist; only a missing `ps` is worth noticing, // and there the empty map degrades callers to a plain PID check rather than reporting deaths. @@ -412,12 +416,11 @@ export async function registerHarperInstance(instance: { } /** - * Whether a process group still has members, used to decide when an instance's cleanup is - * finished. A group outlives its leader: Harper exiting on `SIGTERM` leaves any child that - * ignored the signal running in the same group, still holding the ports. - * - * Safe to act on for a group we recorded, because POSIX reserves a process-group id for as long - * as the group has members — so a group that answers here is still ours, not a recycled id. + * Whether a process group still has members. A group outlives its leader: Harper exiting on + * `SIGTERM` leaves any child that ignored the signal running in the same group, still holding the + * ports. POSIX reserves a group id for the lifetime of the group that holds it, so this answers for + * one continuous group — see `groupOutlivedLeader` for what that does and does not establish about + * whose group it is. */ export function processGroupExists(pgid: number): boolean { // Group 0 is "the caller's own group" and 1 is init's; neither can be an instance we recorded. @@ -436,6 +439,10 @@ export function processGroupExists(pgid: number): boolean { * process-group id and this reaches Harper plus anything it spawned. */ export function signalProcessGroup(pgid: number, signal: 'SIGTERM' | 'SIGKILL'): void { + // The registry is on-disk state a corrupt or hostile writer can reach, and `kill(-1)` broadcasts + // to every process this user may signal while `kill(-0)` hits our own group. No instance we + // registered is ever either, so refuse rather than translate a bad record into a wide signal. + if (!Number.isInteger(pgid) || pgid <= 1) return; try { process.kill(-pgid, signal); } catch { diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index 97a3ed6..65a656c 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -742,9 +742,8 @@ function trackHarperProcess(proc: ChildProcess, instanceId: string, hostname?: s }); const trackedProcess: TrackedHarperProcess = { registered }; - // Only the direct child is untracked here. Its registry record describes the whole process - // group, which can outlive it, so removing that record is the monitor's call — it is the half - // that can see when the group is actually finished. + // Only the direct child is untracked here; its registry record covers the group, which can + // outlive it, and the monitor owns removing that. proc.once('exit', () => liveHarperProcesses.delete(proc)); if (runnerCleanupRegistered) return trackedProcess; diff --git a/src/harperMonitor.ts b/src/harperMonitor.ts index 6c1f978..7ea4678 100644 --- a/src/harperMonitor.ts +++ b/src/harperMonitor.ts @@ -97,15 +97,15 @@ interface ReapTarget { } /** - * Whether the instance's group still has members now that its leader is gone — our own `SIGTERM` - * exits Harper, and a child that ignored it stays in that group holding the ports. + * Whether the instance's group is still running, now that the process we recorded as its leader is + * not. A leader PID `ps` still describes, with a different start time, is a reused id whose group + * is somebody else's. * - * A PID reporting a *different* start time is not that case but a reused id, and its group belongs - * to something unrelated. Absence is the best evidence available, not proof: a group id is reserved - * only for the lifetime of the group that held it, so a group that ended between two scans, had its - * id reused, and then lost its own leader is indistinguishable from ours here. That is the same - * best-effort bar as every other identity check in this registry (`ps` start times narrow PID reuse - * rather than eliminating it), and the scan interval is what bounds it. + * PID *absence* is the best evidence available here, not proof of ownership: a group id is reserved + * only for the lifetime of one group, so a group of ours that ended while nothing was watching, had + * its id reused, and then lost its own leader is indistinguishable from ours. Reaping is best-effort + * against PID reuse throughout this registry, and this is that same bar, widened by however long the + * monitor was not looking. */ function groupOutlivedLeader(instance: HarperInstanceRecord, startTimes: Map): boolean { return !isProcessIdentityReused(instance, startTimes) && processGroupExists(instance.pid); @@ -121,10 +121,9 @@ function reapReason(instance: HarperInstanceRecord, startTimes: Map { return withRegistryLock(async () => { @@ -136,13 +135,10 @@ async function scanRegistry(): Promise<{ live: HarperInstanceRecord[]; targets: const targets: ReapTarget[] = []; const now = Date.now(); for (const instance of registry.instances) { - // A record is the cleanup state of a process group, so it lives as long as that group and - // not as long as its leader — the two differ precisely when it matters. Our own SIGTERM - // exits Harper first, and a child that ignored it stays in the group holding the ports; - // dropping the record there would cancel the SIGKILL escalation. A leader that exits on - // its own is the same picture without the signal: whether the survivors are reaped now, - // later when their runner dies, or at the lifetime budget, this record is what remembers - // them, so retention cannot depend on the reap being due yet. + // Retention is the group's lifetime, deliberately not the leader's and not whether a reap + // is due: the survivors of a leader that exited — on our SIGTERM or on its own — still + // hold the ports, and this record is the only thing that remembers them, whether they are + // reaped now, when their runner dies, or at the lifetime budget. if (!isSameProcessAlive(instance, startTimes) && !groupOutlivedLeader(instance, startTimes)) continue; live.push(instance); const reason = reapReason(instance, startTimes, now); diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 6644076..4e7a70a 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -22,7 +22,9 @@ import { } from '../src/harperLifecycle.ts'; import { MONITOR_ARGV_MARKER, + readProcessStartTimes, readRegistryFile, + signalProcessGroup, withRegistryLock, type InstanceRegistry, } from '../src/harperInstanceRegistry.ts'; @@ -354,7 +356,9 @@ async function startFakeHarperTree(options: FakeHarperTreeOptions = {}): Promise } function forceKill(pid: number | undefined): void { - if (pid === undefined) return; + // A tree whose readiness timed out still carries its initial 0s, and `process.kill(0, ...)` + // signals the test runner's own process group. + if (pid === undefined || pid <= 0) return; try { process.kill(pid, 'SIGKILL'); } catch { @@ -595,9 +599,8 @@ test('an instance that outlives its lifetime budget is reaped while its runner i }); test('a reap in flight is finished even though its runner is alive to see the leader exit', { skip: !isPosix }, async () => { - // The leader's exit ends this reap as far as the still-live runner can see, and the runner used - // to remove the record on it — dropping the escalation just as surely as the monitor's own - // pruning did, while the TERM-ignoring descendant kept the port. + // The leader's exit is all the live runner can see of this reap, and removing the record on it + // dropped the escalation while the TERM-ignoring descendant kept the port. const tree = await startFakeHarperTree({ maxLifetimeMs: '250', descendantIgnoresTerm: true }); try { ok(await waitProcessGone(tree.harperPid, 10000), `overdue Harper ${tree.harperPid} should be reaped`); @@ -613,10 +616,8 @@ test('a reap in flight is finished even though its runner is alive to see the le }); test('a group that outlives its leader stays reapable until its runner dies', { skip: !isPosix }, async () => { - // Nothing is due to be reaped here — the runner is alive and the budget is unexpired — so this - // is the case that says retention cannot be conditional on a reap being due. The record is the - // only description of the surviving group; the runner's own exit handler cannot cover it, - // having dropped the leader from its live set the moment it exited. + // Nothing is due to be reaped yet, and the runner's own handlers dropped the leader from their + // live set the moment it exited, so the record is all that is left to remember the survivors. const tree = await startFakeHarperTree(); try { await waitForMonitor(tree.monitorDir); @@ -640,9 +641,7 @@ test('a group that outlives its leader stays reapable until its runner dies', { }); test('a record whose leader PID has been reused is dropped, not signalled', { skip: !isPosix }, async () => { - // Holding a record past its leader's death is only safe while that PID is *absent*: a group id - // stays reserved for the lifetime of its group, so a PID running something else means the group - // ended and its id was handed out again. + // A PID running something else means our group ended and its id was handed out again. const monitorDir = mkdtempSync(join(tmpdir(), 'harper-it-monitor-')); const bystander = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { detached: true, stdio: 'ignore' }); const exitedOwner = spawn(process.execPath, ['-e', '']); @@ -687,6 +686,41 @@ test('a record whose leader PID has been reused is dropped, not signalled', { sk } }); +test('a process identity does not depend on the environment that read it', { skip: !isPosix }, () => { + // `ps -o lstart=` renders in the caller's timezone and locale. Two runners configured differently + // would record different strings for one process and read each other's live records as PID reuse. + const previousTz = process.env.TZ; + try { + process.env.TZ = 'UTC'; + const asUtc = readProcessStartTimes([process.pid]).get(process.pid); + process.env.TZ = 'America/Denver'; + const asDenver = readProcessStartTimes([process.pid]).get(process.pid); + ok(asUtc, 'this host should report a start time'); + strictEqual(asUtc, asDenver); + } finally { + restoreEnv('TZ', previousTz); + } +}); + +test('a reap cannot be widened into a broadcast by a bad record', { skip: !isPosix }, () => { + // The registry is on-disk state outside this process, so a corrupt or planted record can name + // PID 1 — and `kill(-1)` reaches every process the monitor may signal. + const realKill = process.kill; + const attempted: number[] = []; + process.kill = ((pid: number) => { + attempted.push(pid); + return true; + }) as typeof process.kill; + try { + signalProcessGroup(1, 'SIGKILL'); + signalProcessGroup(0, 'SIGTERM'); + signalProcessGroup(-1, 'SIGKILL'); + } finally { + process.kill = realKill; + } + strictEqual(attempted.length, 0, `no signal should be sent for these group ids, got ${attempted.join()}`); +}); + test('the monitor shuts down once no instances remain', { skip: !isPosix }, async () => { const tree = await startFakeHarperTree({ idleMs: '500' }); try {