From 08609caf70da4a3ab75706816e78b641aaa34e89 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 23 Sep 2026 09:15:14 -0600 Subject: [PATCH 01/11] fix: stop a Harper node that restarted itself in killHarper Harper's `restart` operation relaunches the node as a new, detached process and lets the process startHarper spawned exit 0. killHarper returned as soon as that handle had exited, so teardown left the relaunched node running with its ports while deleting its data root underneath it. When the spawned process has exited, killHarper now terminates the process named in /hdb.pid (the identity `harper stop` uses) with the same SIGTERM, grace, SIGKILL sequence, waiting on its whole process group on POSIX, and removes the pid file a SIGKILL leaves behind. Because the replacement records its pid only after its predecessor has exited, a clean exit killHarper did not cause opens a bounded 5s window in which it waits for the pid file. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016U46s3CDVAfhBsVXHFLaPJ Dispatch-Task: integration-testing-killharper-after-restart --- README.md | 4 +- src/harperLifecycle.ts | 126 +++++++++++++++++++- test/harperLifecycle.test.ts | 223 +++++++++++++++++++++++++++++++++++ 3 files changed, 346 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 8ef3398..4316291 100644 --- a/README.md +++ b/README.md @@ -136,13 +136,15 @@ Like `startHarper()`, but copies a component directory into the Harper install b 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. +It also stops a node that restarted itself. Harper's `restart` operation relaunches the node as a new, detached process and lets the one `startHarper` spawned exit, so once that process has exited `killHarper` terminates whichever process `/hdb.pid` names — the pid file `harper stop` uses — in the same SIGTERM, grace, SIGKILL sequence, and removes the pid file if the SIGKILL left it behind. The replacement records its pid only after its predecessor has exited (about half a second on an idle machine), so if the spawned process exited cleanly within the last 5 seconds, `killHarper` waits up to the rest of that window for the pid file to appear. A replacement slower than that is missed, as is one launched in the few milliseconds between a restart forking it and `killHarper` stopping its predecessor; wait for a restart to finish before tearing down. + `options.graceMs` overrides the SIGTERM→SIGKILL grace period (default `5000`, or `HARPER_INTEGRATION_TEST_TEARDOWN_GRACE_MS`). ### `teardownHarper(ctx)` Kills Harper's process tree, releases the loopback address back to the pool, and removes the install directory. Call in a teardown/`after()` hook. -Since `killHarper` waits for the process tree to exit, its fixed ports (Operations API, HTTP/S, MQTT/S) are already released by the time the address is recycled. As a safety assertion, teardown still verifies those ports are free before recycling — the pool only guarantees the *address* is bindable, not that these specific ports are free — and logs a warning if any are somehow still held (a sign a Harper child process escaped the kill). The address is recycled regardless. +Since `killHarper` waits for the process tree to exit, its fixed ports (Operations API, HTTP/S, MQTT/S) are already released by the time the address is recycled. As a safety assertion, teardown still verifies those ports are free before recycling — the pool only guarantees the *address* is bindable, not that these specific ports are free — and if any are still held (a sign a Harper process escaped the kill) it logs a warning and does not recycle the address: the slot stays parked until this process exits, so a later suite cannot co-bind the ports. **Environment Variables:** diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index 292473d..0a058d7 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -2,13 +2,20 @@ import { spawn, ChildProcess } from 'node:child_process'; import { createWriteStream, existsSync, type WriteStream } from 'node:fs'; import { join, basename, dirname } from 'node:path'; import { tmpdir } from 'node:os'; -import { mkdtemp, mkdir, rm, cp } from 'node:fs/promises'; +import { mkdtemp, mkdir, rm, cp, readFile } from 'node:fs/promises'; +import { setTimeout as sleep } from 'node:timers/promises'; import { type SuiteContext, type TestContext } from 'node:test'; import { getNextAvailableLoopbackAddress, releaseLoopbackAddress } from './loopbackAddressPool.ts'; import { waitForPortsFree } from './portUtils.ts'; import { ok, equal } from 'node:assert'; import { createRequire } from 'node:module'; -import { buildInstanceEnv, nextInstanceId, registerHarperInstance } from './harperInstanceRegistry.ts'; +import { + buildInstanceEnv, + nextInstanceId, + processGroupExists, + registerHarperInstance, + signalProcessGroup, +} from './harperInstanceRegistry.ts'; /** * Minimal context interface required by startHarper/teardownHarper. @@ -101,6 +108,21 @@ export const DEFAULT_PORT_RELEASE_TIMEOUT_MS = parseInt(process.env.HARPER_INTEG /** Short backstop wait for the process 'exit' event after sending SIGKILL during teardown. */ const SIGKILL_EXIT_WAIT_MS = 1000; +/** Where Harper's main process records its pid, relative to its root path — the identity `harper stop` uses. */ +const HARPER_PID_FILE = 'hdb.pid'; + +/** + * How long after Harper exits cleanly on its own teardown still watches for the process a `restart` + * relaunched it as. The replacement records its pid only after its predecessor has exited and it has + * finished loading (~0.5s measured on an idle machine), so a teardown inside that window would + * otherwise find no pid file and leave it running. Not scaled for CI: a node that exited without + * relaunching, or whose replacement was already stopped, pays this on teardown. + */ +const RELAUNCH_PID_WAIT_MS = 5000; + +/** Poll interval while waiting on a process that is not our child, which emits no 'exit' event. */ +const PROCESS_POLL_MS = 50; + /** * The runtime to use for running Harper during tests. * Set via the HARPER_RUNTIME environment variable ('node' or 'bun'). @@ -760,6 +782,12 @@ interface TrackedHarperProcess { const liveHarperProcesses = new Set(); let runnerCleanupRegistered = false; +/** + * Deadline until which `killHarper` looks for a relaunched replacement, per spawned process that exited + * cleanly on its own. A clean exit is what Harper's `restart` looks like from here. + */ +const relaunchDeadlines = new WeakMap(); + 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. @@ -773,7 +801,10 @@ function trackHarperProcess(proc: ChildProcess, instanceId: string, hostname?: s // 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)); + proc.once('exit', (code) => { + liveHarperProcesses.delete(proc); + if (code === 0) relaunchDeadlines.set(proc, Date.now() + RELAUNCH_PID_WAIT_MS); + }); if (runnerCleanupRegistered) return trackedProcess; runnerCleanupRegistered = true; @@ -846,6 +877,9 @@ const START_FROM_THE_CONTEXT = * After SIGKILL it waits briefly for the process to actually exit, so callers can rely on it * being gone — and, since a dead process releases its listening sockets, on its ports being free. * + * If the process we spawned has already exited because Harper's `restart` operation relaunched the + * node as a new process, that process — the one `hdb.pid` names — is terminated instead. + * * @param ctx * @param options.graceMs Time to wait after SIGTERM before sending SIGKILL. Defaults to * {@link DEFAULT_TEARDOWN_GRACE_MS}. @@ -854,10 +888,11 @@ export async function killHarper(ctx: StartedHarperTestContext, options?: { grac assertHarperTestContext(ctx, 'killHarper', WRAP_THE_NODE('killHarper')); const proc = ctx.harper?.process; if (!proc) return; - // Already exited — nothing to do. - if (proc.exitCode !== null || proc.signalCode !== null) return; - const graceMs = options?.graceMs ?? DEFAULT_TEARDOWN_GRACE_MS; + if (proc.exitCode !== null || proc.signalCode !== null) { + if (ctx.harper.dataRootDir) await killRelaunchedHarper(proc, ctx.harper.dataRootDir, graceMs); + return; + } await new Promise((resolve) => { let done = false; @@ -867,6 +902,8 @@ export async function killHarper(ctx: StartedHarperTestContext, options?: { grac const finish = () => { if (done) return; done = true; + // Our SIGTERM caused this exit, not a `restart`, so there is no relaunch to look for. + relaunchDeadlines.delete(proc); proc.off('exit', finish); clearTimeout(sigkillTimer); clearTimeout(backstopTimer); @@ -887,6 +924,83 @@ export async function killHarper(ctx: StartedHarperTestContext, options?: { grac }); } +/** + * Terminates the process Harper relaunched itself as, once the one we spawned has exited. + * + * Harper's `restart` forks a detached replacement and exits 0; the replacement writes its pid to + * `hdb.pid` only once its predecessor is gone. So the node now runs as whatever that file names, and + * shortly after a clean exit the file may not exist yet. + */ +async function killRelaunchedHarper(proc: ChildProcess, dataRootDir: string, graceMs: number): Promise { + const relaunchDeadline = relaunchDeadlines.get(proc) ?? 0; + // Only the first call after the exit waits for the relaunch; a later teardown must not wait again. + relaunchDeadlines.delete(proc); + let pid = await readHarperPid(dataRootDir); + while (pid === undefined && Date.now() < relaunchDeadline) { + await sleep(PROCESS_POLL_MS); + pid = await readHarperPid(dataRootDir); + } + // A stale file can still name the spawned process, whose pid may since have been reused. + if (pid === undefined || pid === proc.pid || pid === process.pid || !isProcessTreeAlive(pid)) return; + + signalProcessTree(pid, 'SIGTERM'); + if (!(await waitForProcessTreeExit(pid, graceMs))) { + signalProcessTree(pid, 'SIGKILL'); + if (!(await waitForProcessTreeExit(pid, SIGKILL_EXIT_WAIT_MS))) return; + } + // Harper removes its pid file on SIGTERM but not on SIGKILL, and a pid left behind can be reused + // before a later call reads it. + if ((await readHarperPid(dataRootDir)) === pid) await rm(join(dataRootDir, HARPER_PID_FILE), { force: true }); +} + +/** + * Reads the pid Harper recorded for the node rooted at `dataRootDir`, or undefined if there is none. + * Exported for tests, not from `index.ts`. + */ +export async function readHarperPid(dataRootDir: string): Promise { + let contents: string; + try { + contents = await readFile(join(dataRootDir, HARPER_PID_FILE), 'utf8'); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; + throw error; + } + const trimmed = contents.trim(); + if (!/^\d+$/.test(trimmed)) return undefined; + const pid = Number(trimmed); + // As a process group, 0 is our own and 1 is every process we may signal. + return Number.isSafeInteger(pid) && pid > 1 ? pid : undefined; +} + +/** + * On POSIX, whether `pid` still identifies a live process group. Harper's relaunch always leads its own + * group (it forks `detached`), and requiring that keeps an ordinary process that reused a stale pid + * from being signalled. + */ +function isProcessTreeAlive(pid: number): boolean { + if (process.platform !== 'win32') return processGroupExists(pid); + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +function signalProcessTree(pid: number, signal: 'SIGTERM' | 'SIGKILL'): void { + if (process.platform === 'win32') signalWindowsProcessTree(pid, signal); + else signalProcessGroup(pid, signal); +} + +async function waitForProcessTreeExit(pid: number, timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs; + while (isProcessTreeAlive(pid)) { + if (Date.now() >= deadline) return false; + await sleep(PROCESS_POLL_MS); + } + return true; +} + /** * Tears down a Harper instance and cleans up all resources. * diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 4e7a70a..6239c3b 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -13,6 +13,7 @@ import { teardownHarper, markHarperNode, publishHarperNode, + readHarperPid, setupHarperWithFixture, startHarper, runHarperCommand, @@ -102,6 +103,53 @@ for (;;) { await writeRegistryFile(padded); await writeRegistryFile(small); } +`, + // Mirrors the handoff of Harper's `restart`: on a 'restart' line it removes hdb.pid, launches a detached + // replacement and exits 0; the replacement waits for its predecessor to exit, then records its pid. + 'restartable.cjs': ` +const { spawn } = require('node:child_process'); +const { writeFileSync, rmSync } = require('node:fs'); +const { join } = require('node:path'); +const root = process.env.HARPER_FAKE_ROOT; +const pidFile = join(root, 'hdb.pid'); +const replacedPid = Number(process.env.HARPER_PARENT_PROCESS_PID || 0); +function serve() { + if (replacedPid && process.env.HARPER_FAKE_IGNORE_TERM === '1') process.on('SIGTERM', () => {}); + else process.on('SIGTERM', () => { rmSync(pidFile, { force: true }); process.exit(0); }); + if (replacedPid && process.env.HARPER_FAKE_DESCENDANT) { + const script = process.env.HARPER_FAKE_DESCENDANT === 'ignore-term' + ? "process.on('SIGTERM', () => {}); setInterval(() => {}, 1000)" + : 'setInterval(() => {}, 1000)'; + const descendant = spawn(process.execPath, ['-e', script], { stdio: 'ignore' }); + writeFileSync(join(root, 'descendant.pid'), String(descendant.pid)); + } + writeFileSync(pidFile, String(process.pid)); + setInterval(() => {}, 1000); +} +if (replacedPid) { + writeFileSync(join(root, 'replacement.pid'), String(process.pid)); + const waitForReplaced = setInterval(() => { + try { + process.kill(replacedPid, 0); + } catch { + clearInterval(waitForReplaced); + setTimeout(serve, Number(process.env.HARPER_FAKE_RELAUNCH_DELAY_MS || 0)); + } + }, 20); +} else { + serve(); + process.stdin.on('data', (chunk) => { + if (!String(chunk).includes('restart')) return; + rmSync(pidFile, { force: true }); + spawn(process.execPath, [__filename], { + detached: true, + stdio: 'ignore', + env: { ...process.env, HARPER_PARENT_PROCESS_PID: String(process.pid) }, + }).unref(); + process.exit(0); + }); + process.stdout.write('successfully started\\n'); +} `, 'orphan-runner.mjs': ` const { runHarperCommand, killHarper } = await import(process.env.HARPER_LIFECYCLE_URL); @@ -1065,6 +1113,181 @@ test('killHarper returns immediately for an already-exited process', async () => ok(Date.now() - start < 1000, 'should not wait the grace period for an already-dead process'); }); +// --- Teardown after Harper's own `restart` --- + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +async function readPidFile(path: string, timeoutMs = 5000): Promise { + const deadline = Date.now() + timeoutMs; + for (;;) { + try { + const pid = Number((await readFile(path, 'utf-8')).trim()); + if (pid > 0) return pid; + } catch { + // Not written yet. + } + if (Date.now() >= deadline) throw new Error(`${path} was not written within ${timeoutMs}ms`); + await sleep(20); + } +} + +interface FakeHarperNode { + ctx: StartedHarperTestContext; + dataRootDir: string; + spawnedPid: number; +} + +/** Starts the restartable fake the way startHarper does (through runHarperCommand), with a data root of its own. */ +async function startRestartableFakeHarper(env: Record = {}): Promise { + const dataRootDir = mkdtempSync(join(tmpdir(), 'harper-it-relaunch-')); + const result = await runHarperCommand({ + args: [], + env: { HARPER_FAKE_ROOT: dataRootDir, ...env }, + completionMessage: 'successfully started', + harperBinPath: fixtures['restartable.cjs'], + timeoutMs: 5000, + maxMs: 10000, + }); + const ctx = { harper: markHarperNode({ process: result.process, dataRootDir }) } as unknown as StartedHarperTestContext; + return { ctx, dataRootDir, spawnedPid: result.process.pid! }; +} + +/** Sends the fake its `restart` and resolves once the spawned process has exited, as Harper's does. */ +async function restartFakeHarper(node: FakeHarperNode): Promise { + const exited = once(node.ctx.harper.process, 'exit'); + node.ctx.harper.process.stdin!.write('restart\n'); + await exited; +} + +async function cleanupFakeHarperNode(node: FakeHarperNode | undefined): Promise { + if (!node) return; + for (const name of ['replacement.pid', 'descendant.pid']) { + const pid = Number(await readFile(join(node.dataRootDir, name), 'utf-8').catch(() => '')); + if (pid > 0) forceKill(pid); + } + rmSync(node.dataRootDir, { recursive: true, force: true }); +} + +test('killHarper terminates the process Harper relaunched itself as, with its tree', async () => { + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper({ HARPER_FAKE_DESCENDANT: '1' }); + await restartFakeHarper(node); + const replacementPid = await readPidFile(join(node.dataRootDir, 'replacement.pid')); + strictEqual(await readPidFile(join(node.dataRootDir, 'hdb.pid')), replacementPid, 'the replacement should own hdb.pid'); + const descendantPid = await readPidFile(join(node.dataRootDir, 'descendant.pid')); + ok(isAlive(replacementPid), 'the replacement should be running before killHarper'); + + // Windows gets a short grace: its SIGTERM-equivalent does not stop a background node process. + await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); + + ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone when killHarper resolves`); + ok(await waitProcessGone(descendantPid, 2000), `its descendant ${descendantPid} should have been killed with it`); + strictEqual(existsSync(join(node.dataRootDir, 'hdb.pid')), false, 'no pid file should name the dead replacement'); + } finally { + await cleanupFakeHarperNode(node); + } +}); + +test('killHarper waits for a replacement that has not yet recorded its pid', async () => { + let node: FakeHarperNode | undefined; + try { + // Harper's replacement writes hdb.pid ~0.5s after its predecessor exits; tear down inside that gap. + node = await startRestartableFakeHarper({ HARPER_FAKE_RELAUNCH_DELAY_MS: '500' }); + await restartFakeHarper(node); + strictEqual(existsSync(join(node.dataRootDir, 'hdb.pid')), false, 'the test must run inside the handoff gap'); + + await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); + + const replacementPid = await readPidFile(join(node.dataRootDir, 'replacement.pid')); + ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone when killHarper resolves`); + } finally { + await cleanupFakeHarperNode(node); + } +}); + +test('killHarper escalates to SIGKILL on a relaunched Harper and clears the pid file it leaves', { skip: !isPosix }, async () => { + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper({ HARPER_FAKE_IGNORE_TERM: '1' }); + await restartFakeHarper(node); + const replacementPid = await readPidFile(join(node.dataRootDir, 'hdb.pid')); + + const start = Date.now(); + await killHarper(node.ctx, { graceMs: 200 }); + ok(Date.now() - start >= 150, 'should wait the grace period before escalating to SIGKILL'); + ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone after SIGKILL`); + strictEqual(existsSync(join(node.dataRootDir, 'hdb.pid')), false, 'SIGKILL leaves hdb.pid behind; killHarper must remove it'); + + // teardownHarper after killHarper: must neither signal anything nor wait out the handoff window again. + const secondStart = Date.now(); + await killHarper(node.ctx, { graceMs: 200 }); + ok(Date.now() - secondStart < 1000, 'a second call should return immediately'); + } finally { + await cleanupFakeHarperNode(node); + } +}); + +test('killHarper waits for the relaunched process group, not just its leader', { skip: !isPosix }, async () => { + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper({ HARPER_FAKE_DESCENDANT: 'ignore-term' }); + await restartFakeHarper(node); + await readPidFile(join(node.dataRootDir, 'hdb.pid')); + const descendantPid = await readPidFile(join(node.dataRootDir, 'descendant.pid')); + + // The leader exits on SIGTERM; its descendant ignores it and keeps the group alive. + await killHarper(node.ctx, { graceMs: 200 }); + ok(!isAlive(descendantPid), `descendant ${descendantPid} should be SIGKILLed with the group before killHarper resolves`); + } finally { + await cleanupFakeHarperNode(node); + } +}); + +test('killHarper does not wait for a relaunch after a shutdown it caused itself', async () => { + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper(); + await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); + // On POSIX the fake exits 0 from its SIGTERM handler, which is what a restart looks like too. + const start = Date.now(); + await killHarper(node.ctx, { graceMs: 200 }); + ok(Date.now() - start < 1000, 'the second call must not wait out the relaunch window'); + } finally { + await cleanupFakeHarperNode(node); + } +}); + +test('readHarperPid accepts only a plain pid a group signal cannot widen', async () => { + const dataRootDir = mkdtempSync(join(tmpdir(), 'harper-it-pidfile-')); + try { + strictEqual(await readHarperPid(dataRootDir), undefined, 'a missing pid file means no process'); + for (const [contents, expected] of [ + ['4242', 4242], + [' 4242\n', 4242], + ['', undefined], + ['0', undefined], + ['1', undefined], + ['-4242', undefined], + ['4242abc', undefined], + ['42.5', undefined], + ['99999999999999999999', undefined], + ] as const) { + await writeFileAsync(join(dataRootDir, 'hdb.pid'), contents); + strictEqual(await readHarperPid(dataRootDir), expected, `hdb.pid containing ${JSON.stringify(contents)}`); + } + } finally { + rmSync(dataRootDir, { recursive: true, force: true }); + } +}); + // Regression guard: the dataRootDir HOME isolation must take precedence over any caller-supplied env (or a // spread of process.env, which contains HOME). If a caller could clobber HOME/USERPROFILE, Harper's global // boot pointer would land in the developer's real ~/.harperdb and outlive the throwaway instance. From 7efc6d26cfb5783a6e2f5c1ef8b89d3a15b9b4fa Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 23 Sep 2026 09:36:40 -0600 Subject: [PATCH 02/11] fix: wait for a relaunch only after Harper announces one Review follow-up. Every clean exit used to open the 5s relaunch window, so a test that stopped Harper itself paid it on teardown. Harper's restart reports ready on stdout a second time just before it forks its replacement and exits, and a plain stop reports nothing; runHarperCommand now records that second report and killHarper waits only after one (5s, 15s under CI). killHarper also checks for a relaunch after stopping a live spawned process, and concurrent calls on one node share a single relaunch kill so none returns before the replacement is gone. Stale comments claiming teardown recycles an address whose ports are still held are corrected, and CONTRIBUTING records which cleanup paths follow a restart. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016U46s3CDVAfhBsVXHFLaPJ Dispatch-Task: integration-testing-killharper-after-restart --- CONTRIBUTING.md | 2 +- README.md | 2 +- src/harperLifecycle.ts | 71 ++++++++++++++++++++---------------- test/harperLifecycle.test.ts | 41 ++++++++++++++++----- 4 files changed, 74 insertions(+), 42 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5d22b90..5358b58 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). 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. The pool behind the loopback utilities, `src/loopbackAddressPool.ts`, never writes an unusable pool file back as an empty pool before its quarantine ends (see README's *Loopback Pool Utilities*): every path that rewrites the file returns early when `readPoolFile` reports it quarantined (`null`), so a new write path must do the same. 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. The pool behind the loopback utilities, `src/loopbackAddressPool.ts`, never writes an unusable pool file back as an empty pool before its quarantine ends (see README's *Loopback Pool Utilities*): every path that rewrites the file returns early when `readPoolFile` reports it quarantined (`null`), so a new write path must do the same. Harper's `restart` operation replaces the process the harness spawned with a detached one the harness never tracked, so `killHarper` identifies the node by `/hdb.pid` once the spawned process has exited (covered by the relaunch tests in `test/harperLifecycle.test.ts`); the runner-exit handlers and the monitor still key on the spawned process and do not follow a restart. 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 4316291..6b526e3 100644 --- a/README.md +++ b/README.md @@ -136,7 +136,7 @@ Like `startHarper()`, but copies a component directory into the Harper install b 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. -It also stops a node that restarted itself. Harper's `restart` operation relaunches the node as a new, detached process and lets the one `startHarper` spawned exit, so once that process has exited `killHarper` terminates whichever process `/hdb.pid` names — the pid file `harper stop` uses — in the same SIGTERM, grace, SIGKILL sequence, and removes the pid file if the SIGKILL left it behind. The replacement records its pid only after its predecessor has exited (about half a second on an idle machine), so if the spawned process exited cleanly within the last 5 seconds, `killHarper` waits up to the rest of that window for the pid file to appear. A replacement slower than that is missed, as is one launched in the few milliseconds between a restart forking it and `killHarper` stopping its predecessor; wait for a restart to finish before tearing down. +It also stops a node that restarted itself. Harper's `restart` operation relaunches the node as a new, detached process and lets the one `startHarper` spawned exit, so once that process has exited `killHarper` terminates whichever process `/hdb.pid` names — the pid file `harper stop` uses — in the same SIGTERM, grace, SIGKILL sequence (on POSIX, waiting for its whole process group), and removes the pid file if the SIGKILL left it behind. The replacement records its pid only after its predecessor has exited, about half a second later on an idle machine. The spawned process reports ready on stdout a second time just before it relaunches, so after that report `killHarper` waits up to 5 seconds (15 under CI) from it for the pid file to appear. A node that stopped without relaunching reports nothing and pays no wait. Only the first relaunch is followed this way: when a replacement restarts again, wait for that restart to finish before tearing down. `options.graceMs` overrides the SIGTERM→SIGKILL grace period (default `5000`, or `HARPER_INTEGRATION_TEST_TEARDOWN_GRACE_MS`). diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index 0a058d7..df98215 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -98,8 +98,8 @@ export const DEFAULT_TEARDOWN_GRACE_MS = parseInt(process.env.HARPER_INTEGRATION * Time teardown's safety assertion waits for Harper's fixed ports to be free before recycling the * loopback address. Killing Harper's process tree (and waiting for exit) should free them * immediately, so this normally returns on the first check; it only matters if a child process - * escaped the kill. If the ports are still in use at the deadline, the address is recycled anyway - * (no worse than not waiting) and a warning is logged. + * escaped the kill. If the ports are still in use at the deadline, a warning is logged and the address + * is not recycled. * * Override with `HARPER_INTEGRATION_TEST_PORT_RELEASE_TIMEOUT_MS`. Default 5s. */ @@ -108,19 +108,16 @@ export const DEFAULT_PORT_RELEASE_TIMEOUT_MS = parseInt(process.env.HARPER_INTEG /** Short backstop wait for the process 'exit' event after sending SIGKILL during teardown. */ const SIGKILL_EXIT_WAIT_MS = 1000; -/** Where Harper's main process records its pid, relative to its root path — the identity `harper stop` uses. */ +/** Harper's own record of which process runs the node, relative to its root path; `harper stop` reads it too. */ const HARPER_PID_FILE = 'hdb.pid'; /** - * How long after Harper exits cleanly on its own teardown still watches for the process a `restart` - * relaunched it as. The replacement records its pid only after its predecessor has exited and it has - * finished loading (~0.5s measured on an idle machine), so a teardown inside that window would - * otherwise find no pid file and leave it running. Not scaled for CI: a node that exited without - * relaunching, or whose replacement was already stopped, pays this on teardown. + * How long after Harper announces a relaunch teardown waits for the replacement to record its pid. The + * replacement does that only once its predecessor has exited and it has finished loading (~0.5s on an + * idle machine), so a teardown inside that window would otherwise find no pid file. */ -const RELAUNCH_PID_WAIT_MS = 5000; +const RELAUNCH_PID_WAIT_MS = IS_CI ? 15000 : 5000; -/** Poll interval while waiting on a process that is not our child, which emits no 'exit' event. */ const PROCESS_POLL_MS = 50; /** @@ -447,6 +444,7 @@ export function runHarperCommand({ return new Promise((resolve, reject) => { let stdout = ''; let stderr = ''; + let postReadinessTail = ''; let settled = false; let readinessDetected = false; let idleTimer: NodeJS.Timeout; @@ -510,7 +508,16 @@ 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 (startupFinished()) return; + if (startupFinished()) { + // Harper's `restart` goes back through its startup path, reporting ready again just + // before it hands the node to a detached replacement and exits. + if (completionMessage && readinessDetected && !relaunchAnnouncements.has(proc)) { + const recentOutput = postReadinessTail + dataString; + if (recentOutput.includes(completionMessage)) relaunchAnnouncements.set(proc, Date.now()); + else postReadinessTail = recentOutput.slice(-(completionMessage.length - 1)); + } + return; + } resetIdleTimer(); stdout += dataString; // Match against the accumulated output, not just this chunk, so a marker split across @@ -782,11 +789,11 @@ interface TrackedHarperProcess { const liveHarperProcesses = new Set(); let runnerCleanupRegistered = false; -/** - * Deadline until which `killHarper` looks for a relaunched replacement, per spawned process that exited - * cleanly on its own. A clean exit is what Harper's `restart` looks like from here. - */ -const relaunchDeadlines = new WeakMap(); +/** When each spawned Harper announced it was relaunching itself as a new process. */ +const relaunchAnnouncements = new WeakMap(); + +/** Shared so that a concurrent `killHarper` on the same node cannot return before the first finishes. */ +const relaunchKills = new WeakMap>(); function trackHarperProcess(proc: ChildProcess, instanceId: string, hostname?: string): TrackedHarperProcess { liveHarperProcesses.add(proc); @@ -801,10 +808,7 @@ function trackHarperProcess(proc: ChildProcess, instanceId: string, hostname?: s // 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', (code) => { - liveHarperProcesses.delete(proc); - if (code === 0) relaunchDeadlines.set(proc, Date.now() + RELAUNCH_PID_WAIT_MS); - }); + proc.once('exit', () => liveHarperProcesses.delete(proc)); if (runnerCleanupRegistered) return trackedProcess; runnerCleanupRegistered = true; @@ -889,11 +893,17 @@ export async function killHarper(ctx: StartedHarperTestContext, options?: { grac const proc = ctx.harper?.process; if (!proc) return; const graceMs = options?.graceMs ?? DEFAULT_TEARDOWN_GRACE_MS; - if (proc.exitCode !== null || proc.signalCode !== null) { - if (ctx.harper.dataRootDir) await killRelaunchedHarper(proc, ctx.harper.dataRootDir, graceMs); - return; + if (proc.exitCode === null && proc.signalCode === null) await killSpawnedHarper(proc, graceMs); + if (!ctx.harper.dataRootDir) return; + let relaunchKill = relaunchKills.get(proc); + if (!relaunchKill) { + relaunchKill = killRelaunchedHarper(proc, ctx.harper.dataRootDir, graceMs).finally(() => relaunchKills.delete(proc)); + relaunchKills.set(proc, relaunchKill); } + await relaunchKill; +} +async function killSpawnedHarper(proc: ChildProcess, graceMs: number): Promise { await new Promise((resolve) => { let done = false; let sigkillTimer: NodeJS.Timeout; @@ -902,8 +912,6 @@ export async function killHarper(ctx: StartedHarperTestContext, options?: { grac const finish = () => { if (done) return; done = true; - // Our SIGTERM caused this exit, not a `restart`, so there is no relaunch to look for. - relaunchDeadlines.delete(proc); proc.off('exit', finish); clearTimeout(sigkillTimer); clearTimeout(backstopTimer); @@ -929,17 +937,19 @@ export async function killHarper(ctx: StartedHarperTestContext, options?: { grac * * Harper's `restart` forks a detached replacement and exits 0; the replacement writes its pid to * `hdb.pid` only once its predecessor is gone. So the node now runs as whatever that file names, and - * shortly after a clean exit the file may not exist yet. + * shortly after the relaunch the file may not exist yet. */ async function killRelaunchedHarper(proc: ChildProcess, dataRootDir: string, graceMs: number): Promise { - const relaunchDeadline = relaunchDeadlines.get(proc) ?? 0; - // Only the first call after the exit waits for the relaunch; a later teardown must not wait again. - relaunchDeadlines.delete(proc); let pid = await readHarperPid(dataRootDir); + // Read after the await above, which lets stdout that arrived alongside the exit be processed first. + const announcedAt = relaunchAnnouncements.get(proc); + const relaunchDeadline = announcedAt === undefined ? 0 : announcedAt + RELAUNCH_PID_WAIT_MS; while (pid === undefined && Date.now() < relaunchDeadline) { await sleep(PROCESS_POLL_MS); pid = await readHarperPid(dataRootDir); } + // Once one call has waited, a later teardown must not wait again. + relaunchAnnouncements.delete(proc); // A stale file can still name the spawned process, whose pid may since have been reused. if (pid === undefined || pid === proc.pid || pid === process.pid || !isProcessTreeAlive(pid)) return; @@ -1030,8 +1040,7 @@ export async function teardownHarper(ctx: StartedHarperTestContext): Promise { if (!String(chunk).includes('restart')) return; rmSync(pidFile, { force: true }); + process.stdout.write('successfully started\\n'); spawn(process.execPath, [__filename], { detached: true, stdio: 'ignore', env: { ...process.env, HARPER_PARENT_PROCESS_PID: String(process.pid) }, }).unref(); - process.exit(0); + // Real Harper exits synchronously here; lingering lets a test stop it after it announced the relaunch. + setTimeout(() => process.exit(0), Number(process.env.HARPER_FAKE_LINGER_MS || 0)); }); process.stdout.write('successfully started\\n'); } @@ -1144,7 +1147,6 @@ interface FakeHarperNode { spawnedPid: number; } -/** Starts the restartable fake the way startHarper does (through runHarperCommand), with a data root of its own. */ async function startRestartableFakeHarper(env: Record = {}): Promise { const dataRootDir = mkdtempSync(join(tmpdir(), 'harper-it-relaunch-')); const result = await runHarperCommand({ @@ -1159,7 +1161,6 @@ async function startRestartableFakeHarper(env: Record = {}): Pro return { ctx, dataRootDir, spawnedPid: result.process.pid! }; } -/** Sends the fake its `restart` and resolves once the spawned process has exited, as Harper's does. */ async function restartFakeHarper(node: FakeHarperNode): Promise { const exited = once(node.ctx.harper.process, 'exit'); node.ctx.harper.process.stdin!.write('restart\n'); @@ -1204,10 +1205,13 @@ test('killHarper waits for a replacement that has not yet recorded its pid', asy await restartFakeHarper(node); strictEqual(existsSync(join(node.dataRootDir, 'hdb.pid')), false, 'the test must run inside the handoff gap'); + // An un-awaited killHarper followed by teardown's: neither may return before the replacement is gone. + const firstCall = killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); const replacementPid = await readPidFile(join(node.dataRootDir, 'replacement.pid')); ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone when killHarper resolves`); + await firstCall; } finally { await cleanupFakeHarperNode(node); } @@ -1251,15 +1255,34 @@ test('killHarper waits for the relaunched process group, not just its leader', { } }); -test('killHarper does not wait for a relaunch after a shutdown it caused itself', async () => { +test('killHarper follows a relaunch the spawned process announced before it was stopped', async () => { let node: FakeHarperNode | undefined; try { - node = await startRestartableFakeHarper(); + node = await startRestartableFakeHarper({ HARPER_FAKE_LINGER_MS: '3000' }); + node.ctx.harper.process.stdin!.write('restart\n'); + const replacementPid = await readPidFile(join(node.dataRootDir, 'replacement.pid')); + strictEqual(node.ctx.harper.process.exitCode, null, 'the spawned process must still be running when killHarper starts'); + await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); - // On POSIX the fake exits 0 from its SIGTERM handler, which is what a restart looks like too. + + ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone when killHarper resolves`); + } finally { + await cleanupFakeHarperNode(node); + } +}); + +test('killHarper does not wait for a relaunch after a clean stop that announced none', { skip: !isPosix }, async () => { + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper(); + // A test stopping Harper itself: exit code 0, like a restart, but no second readiness report. + const exited = once(node.ctx.harper.process, 'exit'); + process.kill(node.spawnedPid, 'SIGTERM'); + strictEqual((await exited)[0], 0); + const start = Date.now(); await killHarper(node.ctx, { graceMs: 200 }); - ok(Date.now() - start < 1000, 'the second call must not wait out the relaunch window'); + ok(Date.now() - start < 1000, 'killHarper must not wait out the relaunch window'); } finally { await cleanupFakeHarperNode(node); } From 12d6e234e0544ddbfaf343d3def92bafcac94b8e Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 23 Sep 2026 09:54:59 -0600 Subject: [PATCH 03/11] fix: keep a Harper install when teardown cannot confirm it stopped Review follow-up. When Harper announced a relaunch but the replacement never recorded its pid, or a process outlived SIGKILL, teardownHarper still recycled the address and deleted the install directory, under a process that may be running or about to bind. It now warns and leaves both in place; killHarper's signature is unchanged. A readiness line Harper logs to stdout at boot (logging.stdStreams) no longer pins the relaunch announcement: every report refreshes it, so a later restart is still followed. The relaunch wait can be tuned with HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016U46s3CDVAfhBsVXHFLaPJ Dispatch-Task: integration-testing-killharper-after-restart --- README.md | 5 ++- src/harperLifecycle.ts | 77 +++++++++++++++++++++++------------- test/harperLifecycle.test.ts | 39 ++++++++++++++++++ 3 files changed, 92 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 6b526e3..ceb5dad 100644 --- a/README.md +++ b/README.md @@ -136,7 +136,7 @@ Like `startHarper()`, but copies a component directory into the Harper install b 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. -It also stops a node that restarted itself. Harper's `restart` operation relaunches the node as a new, detached process and lets the one `startHarper` spawned exit, so once that process has exited `killHarper` terminates whichever process `/hdb.pid` names — the pid file `harper stop` uses — in the same SIGTERM, grace, SIGKILL sequence (on POSIX, waiting for its whole process group), and removes the pid file if the SIGKILL left it behind. The replacement records its pid only after its predecessor has exited, about half a second later on an idle machine. The spawned process reports ready on stdout a second time just before it relaunches, so after that report `killHarper` waits up to 5 seconds (15 under CI) from it for the pid file to appear. A node that stopped without relaunching reports nothing and pays no wait. Only the first relaunch is followed this way: when a replacement restarts again, wait for that restart to finish before tearing down. +It also stops a node that restarted itself. Harper's `restart` operation relaunches the node as a new, detached process and lets the one `startHarper` spawned exit, so once that process has exited `killHarper` terminates whichever process `/hdb.pid` names — the pid file `harper stop` uses — in the same SIGTERM, grace, SIGKILL sequence (on POSIX, waiting for its whole process group), and removes the pid file if the SIGKILL left it behind. The replacement records its pid only after its predecessor has exited, about half a second later on an idle machine. The spawned process reports ready on stdout a second time just before it relaunches, so after that report `killHarper` waits up to 5 seconds (15 under CI, or `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS`) from it for the pid file to appear. A node that stopped without relaunching reports nothing and pays no wait. Only the first relaunch is followed this way: when a replacement restarts again, wait for that restart to finish before tearing down. `options.graceMs` overrides the SIGTERM→SIGKILL grace period (default `5000`, or `HARPER_INTEGRATION_TEST_TEARDOWN_GRACE_MS`). @@ -146,10 +146,13 @@ Kills Harper's process tree, releases the loopback address back to the pool, and Since `killHarper` waits for the process tree to exit, its fixed ports (Operations API, HTTP/S, MQTT/S) are already released by the time the address is recycled. As a safety assertion, teardown still verifies those ports are free before recycling — the pool only guarantees the *address* is bindable, not that these specific ports are free — and if any are still held (a sign a Harper process escaped the kill) it logs a warning and does not recycle the address: the slot stays parked until this process exits, so a later suite cannot co-bind the ports. +If the kill itself could not be confirmed — Harper announced a relaunch but the replacement never recorded its pid, or a process outlived SIGKILL — teardown logs a warning and leaves both the install directory and the address in place rather than deleting the directory under a process that may still be running. + **Environment Variables:** - `HARPER_INTEGRATION_TEST_TEARDOWN_GRACE_MS` - Grace period after SIGTERM before escalating to SIGKILL. Default `5000`. - `HARPER_INTEGRATION_TEST_PORT_RELEASE_TIMEOUT_MS` - Max time teardown's safety assertion waits for Harper's ports to be free before recycling the loopback address (normally instant, since the process tree is already dead). Default `5000`. +- `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS` - After Harper announces a `restart` relaunch, how long `killHarper` waits for the replacement to record its pid. Default `5000` (`15000` under CI). ### `sendOperation(context, operation)` diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index df98215..fdaea95 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -114,9 +114,14 @@ const HARPER_PID_FILE = 'hdb.pid'; /** * How long after Harper announces a relaunch teardown waits for the replacement to record its pid. The * replacement does that only once its predecessor has exited and it has finished loading (~0.5s on an - * idle machine), so a teardown inside that window would otherwise find no pid file. + * idle machine), so a teardown inside that window would otherwise find no pid file. Read per call, like + * the monitor's tunables, so a test can shorten it after import. + * + * Override with `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS`. Default 5s (15s under CI). */ -const RELAUNCH_PID_WAIT_MS = IS_CI ? 15000 : 5000; +function getRelaunchPidWaitMs(): number { + return parseInt(process.env.HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS || '', 10) || (IS_CI ? 15000 : 5000); +} const PROCESS_POLL_MS = 50; @@ -511,10 +516,12 @@ export function runHarperCommand({ if (startupFinished()) { // Harper's `restart` goes back through its startup path, reporting ready again just // before it hands the node to a detached replacement and exits. - if (completionMessage && readinessDetected && !relaunchAnnouncements.has(proc)) { + // Every report refreshes the time, so one Harper logs to stdout at boot (`logging.stdStreams`) + // cannot stand in for a later restart's. + if (completionMessage && readinessDetected) { const recentOutput = postReadinessTail + dataString; if (recentOutput.includes(completionMessage)) relaunchAnnouncements.set(proc, Date.now()); - else postReadinessTail = recentOutput.slice(-(completionMessage.length - 1)); + postReadinessTail = recentOutput.slice(-(completionMessage.length - 1)); } return; } @@ -789,11 +796,10 @@ interface TrackedHarperProcess { const liveHarperProcesses = new Set(); let runnerCleanupRegistered = false; -/** When each spawned Harper announced it was relaunching itself as a new process. */ const relaunchAnnouncements = new WeakMap(); /** Shared so that a concurrent `killHarper` on the same node cannot return before the first finishes. */ -const relaunchKills = new WeakMap>(); +const relaunchKills = new WeakMap>(); function trackHarperProcess(proc: ChildProcess, instanceId: string, hostname?: string): TrackedHarperProcess { liveHarperProcesses.add(proc); @@ -890,35 +896,41 @@ const START_FROM_THE_CONTEXT = */ export async function killHarper(ctx: StartedHarperTestContext, options?: { graceMs?: number }): Promise { assertHarperTestContext(ctx, 'killHarper', WRAP_THE_NODE('killHarper')); + await stopHarperNode(ctx, options?.graceMs ?? DEFAULT_TEARDOWN_GRACE_MS); +} + +/** Resolves false when the node's processes could not be confirmed gone. */ +async function stopHarperNode(ctx: StartedHarperTestContext, graceMs: number): Promise { const proc = ctx.harper?.process; - if (!proc) return; - const graceMs = options?.graceMs ?? DEFAULT_TEARDOWN_GRACE_MS; - if (proc.exitCode === null && proc.signalCode === null) await killSpawnedHarper(proc, graceMs); - if (!ctx.harper.dataRootDir) return; + if (!proc) return true; + const spawnedStopped = proc.exitCode !== null || proc.signalCode !== null || (await killSpawnedHarper(proc, graceMs)); + if (!ctx.harper.dataRootDir) return spawnedStopped; let relaunchKill = relaunchKills.get(proc); if (!relaunchKill) { relaunchKill = killRelaunchedHarper(proc, ctx.harper.dataRootDir, graceMs).finally(() => relaunchKills.delete(proc)); relaunchKills.set(proc, relaunchKill); } - await relaunchKill; + return (await relaunchKill) && spawnedStopped; } -async function killSpawnedHarper(proc: ChildProcess, graceMs: number): Promise { - await new Promise((resolve) => { +/** Resolves false if the process never reported its exit. */ +function killSpawnedHarper(proc: ChildProcess, graceMs: number): Promise { + return new Promise((resolve) => { let done = false; let sigkillTimer: NodeJS.Timeout; let backstopTimer: NodeJS.Timeout; - const finish = () => { + const finish = (exited: boolean) => { if (done) return; done = true; - proc.off('exit', finish); + proc.off('exit', onExit); clearTimeout(sigkillTimer); clearTimeout(backstopTimer); - resolve(); + resolve(exited); }; + const onExit = () => finish(true); - proc.once('exit', finish); + proc.once('exit', onExit); // Ask the whole Harper tree to shut down cleanly first. signalHarperTree(proc, 'SIGTERM'); @@ -927,7 +939,7 @@ async function killSpawnedHarper(proc: ChildProcess, graceMs: number): Promise { signalHarperTree(proc, 'SIGKILL'); - backstopTimer = setTimeout(finish, SIGKILL_EXIT_WAIT_MS); + backstopTimer = setTimeout(() => finish(false), SIGKILL_EXIT_WAIT_MS); }, graceMs); }); } @@ -937,30 +949,32 @@ async function killSpawnedHarper(proc: ChildProcess, graceMs: number): Promise { +async function killRelaunchedHarper(proc: ChildProcess, dataRootDir: string, graceMs: number): Promise { let pid = await readHarperPid(dataRootDir); // Read after the await above, which lets stdout that arrived alongside the exit be processed first. const announcedAt = relaunchAnnouncements.get(proc); - const relaunchDeadline = announcedAt === undefined ? 0 : announcedAt + RELAUNCH_PID_WAIT_MS; + relaunchAnnouncements.delete(proc); + const relaunchDeadline = announcedAt === undefined ? 0 : announcedAt + getRelaunchPidWaitMs(); while (pid === undefined && Date.now() < relaunchDeadline) { await sleep(PROCESS_POLL_MS); pid = await readHarperPid(dataRootDir); } - // Once one call has waited, a later teardown must not wait again. - relaunchAnnouncements.delete(proc); + if (pid === undefined) return announcedAt === undefined; // A stale file can still name the spawned process, whose pid may since have been reused. - if (pid === undefined || pid === proc.pid || pid === process.pid || !isProcessTreeAlive(pid)) return; + if (pid === proc.pid || pid === process.pid || !isProcessTreeAlive(pid)) return true; signalProcessTree(pid, 'SIGTERM'); if (!(await waitForProcessTreeExit(pid, graceMs))) { signalProcessTree(pid, 'SIGKILL'); - if (!(await waitForProcessTreeExit(pid, SIGKILL_EXIT_WAIT_MS))) return; + if (!(await waitForProcessTreeExit(pid, SIGKILL_EXIT_WAIT_MS))) return false; } // Harper removes its pid file on SIGTERM but not on SIGKILL, and a pid left behind can be reused // before a later call reads it. if ((await readHarperPid(dataRootDir)) === pid) await rm(join(dataRootDir, HARPER_PID_FILE), { force: true }); + return true; } /** @@ -984,8 +998,8 @@ export async function readHarperPid(dataRootDir: string): Promise { assertHarperTestContext(ctx, 'teardownHarper', WRAP_THE_NODE('teardownHarper')); if (!ctx.harper) return; - await killHarper(ctx); + if (!(await stopHarperNode(ctx, DEFAULT_TEARDOWN_GRACE_MS))) { + // Deleting the root under a process that may still be running (or about to bind the address) + // is worse than leaking both until this process exits. + console.warn( + `Could not confirm Harper at ${ctx.harper.dataRootDir} stopped; leaving that directory in place and NOT recycling ${ctx.harper.hostname ?? 'its address'}.` + ); + return; + } // Safety assertion: killHarper waits for Harper's process tree to exit, which releases its // listening sockets, so the fixed ports should already be free here. We still verify before diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 7e2eab1..cad0279 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -152,6 +152,8 @@ if (replacedPid) { setTimeout(() => process.exit(0), Number(process.env.HARPER_FAKE_LINGER_MS || 0)); }); process.stdout.write('successfully started\\n'); + // What Harper's logger adds right after readiness when \`logging.stdStreams\` is on. + if (process.env.HARPER_FAKE_BOOT_NOTIFY === '1') setTimeout(() => process.stdout.write('Harper successfully started.\\n'), 50); } `, 'orphan-runner.mjs': ` @@ -1288,6 +1290,43 @@ test('killHarper does not wait for a relaunch after a clean stop that announced } }); +test('a readiness line logged at boot does not hide a later restart', async () => { + const previousWait = process.env.HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS; + process.env.HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS = '1000'; + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper({ HARPER_FAKE_BOOT_NOTIFY: '1', HARPER_FAKE_RELAUNCH_DELAY_MS: '300' }); + // Let the boot-time report's own relaunch window lapse before the real restart. + await sleep(1200); + await restartFakeHarper(node); + + await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); + + const replacementPid = await readPidFile(join(node.dataRootDir, 'replacement.pid')); + ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone when killHarper resolves`); + } finally { + restoreEnv('HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS', previousWait); + await cleanupFakeHarperNode(node); + } +}); + +test('teardownHarper keeps the data root when an announced replacement never records its pid', async () => { + const previousWait = process.env.HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS; + process.env.HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS = '300'; + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper({ HARPER_FAKE_RELAUNCH_DELAY_MS: '3000' }); + await restartFakeHarper(node); + + await teardownHarper(node.ctx); + + ok(existsSync(node.dataRootDir), 'teardown must not delete the root under a replacement that may still come up'); + } finally { + restoreEnv('HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS', previousWait); + await cleanupFakeHarperNode(node); + } +}); + test('readHarperPid accepts only a plain pid a group signal cannot widen', async () => { const dataRootDir = mkdtempSync(join(tmpdir(), 'harper-it-pidfile-')); try { From 35ea02c1ab492a4dbdc9f612bd938cb4c934ecc5 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 23 Sep 2026 10:24:28 -0600 Subject: [PATCH 04/11] fix: ignore Harper's boot-time readiness log when detecting a restart Review follow-up. With logging.stdStreams on, Harper's logger repeats the readiness line right after boot, which was taken for a restart announcement: a teardown within the relaunch window then waited for a replacement that never came and kept the install directory. Reports within 50ms of readiness now count as boot; a restart cannot report that soon because Harper waits 50ms after the restart operation before it begins. An announced replacement that has not recorded its pid now stays pending across calls, so killHarper followed by teardownHarper still keeps the install directory. killHarper warns when it cannot confirm the node stopped. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016U46s3CDVAfhBsVXHFLaPJ Dispatch-Task: integration-testing-killharper-after-restart --- README.md | 2 +- src/harperLifecycle.ts | 23 +++++++++++++---- test/harperLifecycle.test.ts | 50 ++++++++++++++++++++++++------------ 3 files changed, 52 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index ceb5dad..313d1ed 100644 --- a/README.md +++ b/README.md @@ -136,7 +136,7 @@ Like `startHarper()`, but copies a component directory into the Harper install b 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. -It also stops a node that restarted itself. Harper's `restart` operation relaunches the node as a new, detached process and lets the one `startHarper` spawned exit, so once that process has exited `killHarper` terminates whichever process `/hdb.pid` names — the pid file `harper stop` uses — in the same SIGTERM, grace, SIGKILL sequence (on POSIX, waiting for its whole process group), and removes the pid file if the SIGKILL left it behind. The replacement records its pid only after its predecessor has exited, about half a second later on an idle machine. The spawned process reports ready on stdout a second time just before it relaunches, so after that report `killHarper` waits up to 5 seconds (15 under CI, or `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS`) from it for the pid file to appear. A node that stopped without relaunching reports nothing and pays no wait. Only the first relaunch is followed this way: when a replacement restarts again, wait for that restart to finish before tearing down. +It also stops a node that restarted itself. Harper's `restart` operation relaunches the node as a new, detached process and lets the one `startHarper` spawned exit, so once that process has exited `killHarper` terminates whichever process `/hdb.pid` names — the pid file `harper stop` uses — in the same SIGTERM, grace, SIGKILL sequence (on POSIX, waiting for its whole process group), and removes the pid file if the SIGKILL left it behind. The replacement records its pid only after its predecessor has exited, about half a second later on an idle machine. The spawned process reports ready on stdout a second time just before it relaunches, so after that report `killHarper` waits up to 5 seconds (15 under CI, or `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS`) from it for the pid file to appear. A node that stopped without relaunching reports nothing and pays no wait. If `killHarper` cannot confirm the node stopped — an announced replacement never recorded its pid, or a process outlived SIGKILL — it logs a warning, since its ports may still be held. Only the first relaunch is followed this way: when a replacement restarts again, wait for that restart to finish before tearing down. `options.graceMs` overrides the SIGTERM→SIGKILL grace period (default `5000`, or `HARPER_INTEGRATION_TEST_TEARDOWN_GRACE_MS`). diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index fdaea95..39c164c 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -125,6 +125,13 @@ function getRelaunchPidWaitMs(): number { const PROCESS_POLL_MS = 50; +/** + * Readiness reports this soon after startup readiness belong to the boot itself: with + * `logging.stdStreams` on, Harper's logger repeats the readiness line immediately. A restart cannot + * report this soon, since Harper waits 50ms after the `restart` operation before it even begins. + */ +const BOOT_REPORT_SETTLE_MS = 50; + /** * The runtime to use for running Harper during tests. * Set via the HARPER_RUNTIME environment variable ('node' or 'bun'). @@ -450,6 +457,7 @@ export function runHarperCommand({ let stdout = ''; let stderr = ''; let postReadinessTail = ''; + let readyAt = 0; let settled = false; let readinessDetected = false; let idleTimer: NodeJS.Timeout; @@ -472,6 +480,7 @@ export function runHarperCommand({ const succeed = () => { if (settled || readinessDetected) return; readinessDetected = true; + readyAt = Date.now(); // Left armed across registration, these would let registry-lock contention time out — and // kill — an instance that already booted successfully. clearTimers(); @@ -516,11 +525,12 @@ export function runHarperCommand({ if (startupFinished()) { // Harper's `restart` goes back through its startup path, reporting ready again just // before it hands the node to a detached replacement and exits. - // Every report refreshes the time, so one Harper logs to stdout at boot (`logging.stdStreams`) - // cannot stand in for a later restart's. if (completionMessage && readinessDetected) { const recentOutput = postReadinessTail + dataString; - if (recentOutput.includes(completionMessage)) relaunchAnnouncements.set(proc, Date.now()); + const now = Date.now(); + if (recentOutput.includes(completionMessage) && now - readyAt >= BOOT_REPORT_SETTLE_MS) { + relaunchAnnouncements.set(proc, now); + } postReadinessTail = recentOutput.slice(-(completionMessage.length - 1)); } return; @@ -896,7 +906,9 @@ const START_FROM_THE_CONTEXT = */ export async function killHarper(ctx: StartedHarperTestContext, options?: { graceMs?: number }): Promise { assertHarperTestContext(ctx, 'killHarper', WRAP_THE_NODE('killHarper')); - await stopHarperNode(ctx, options?.graceMs ?? DEFAULT_TEARDOWN_GRACE_MS); + if (!(await stopHarperNode(ctx, options?.graceMs ?? DEFAULT_TEARDOWN_GRACE_MS))) { + console.warn(`Could not confirm Harper at ${ctx.harper.dataRootDir ?? ctx.harper.hostname} stopped; its ports may still be held.`); + } } /** Resolves false when the node's processes could not be confirmed gone. */ @@ -956,13 +968,14 @@ async function killRelaunchedHarper(proc: ChildProcess, dataRootDir: string, gra let pid = await readHarperPid(dataRootDir); // Read after the await above, which lets stdout that arrived alongside the exit be processed first. const announcedAt = relaunchAnnouncements.get(proc); - relaunchAnnouncements.delete(proc); const relaunchDeadline = announcedAt === undefined ? 0 : announcedAt + getRelaunchPidWaitMs(); while (pid === undefined && Date.now() < relaunchDeadline) { await sleep(PROCESS_POLL_MS); pid = await readHarperPid(dataRootDir); } + // An announced replacement that has not shown up yet may still come, for this call and any later one. if (pid === undefined) return announcedAt === undefined; + relaunchAnnouncements.delete(proc); // A stale file can still name the spawned process, whose pid may since have been reused. if (pid === proc.pid || pid === process.pid || !isProcessTreeAlive(pid)) return true; diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index cad0279..f8d1a87 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -141,19 +141,22 @@ if (replacedPid) { serve(); process.stdin.on('data', (chunk) => { if (!String(chunk).includes('restart')) return; - rmSync(pidFile, { force: true }); - process.stdout.write('successfully started\\n'); - spawn(process.execPath, [__filename], { - detached: true, - stdio: 'ignore', - env: { ...process.env, HARPER_PARENT_PROCESS_PID: String(process.pid) }, - }).unref(); - // Real Harper exits synchronously here; lingering lets a test stop it after it announced the relaunch. - setTimeout(() => process.exit(0), Number(process.env.HARPER_FAKE_LINGER_MS || 0)); + // Harper's restart, too, waits 50ms before it begins. + setTimeout(() => { + rmSync(pidFile, { force: true }); + process.stdout.write('successfully started\\n'); + spawn(process.execPath, [__filename], { + detached: true, + stdio: 'ignore', + env: { ...process.env, HARPER_PARENT_PROCESS_PID: String(process.pid) }, + }).unref(); + // Real Harper exits synchronously here; lingering lets a test stop it after it announced the relaunch. + setTimeout(() => process.exit(0), Number(process.env.HARPER_FAKE_LINGER_MS || 0)); + }, 50); }); process.stdout.write('successfully started\\n'); // What Harper's logger adds right after readiness when \`logging.stdStreams\` is on. - if (process.env.HARPER_FAKE_BOOT_NOTIFY === '1') setTimeout(() => process.stdout.write('Harper successfully started.\\n'), 50); + if (process.env.HARPER_FAKE_BOOT_NOTIFY === '1') setTimeout(() => process.stdout.write('Harper successfully started.\\n'), 10); } `, 'orphan-runner.mjs': ` @@ -1207,7 +1210,8 @@ test('killHarper waits for a replacement that has not yet recorded its pid', asy await restartFakeHarper(node); strictEqual(existsSync(join(node.dataRootDir, 'hdb.pid')), false, 'the test must run inside the handoff gap'); - // An un-awaited killHarper followed by teardown's: neither may return before the replacement is gone. + // Two overlapping calls, as from an un-awaited killHarper and a teardown hook: neither may + // return before the replacement is gone. const firstCall = killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); @@ -1232,7 +1236,7 @@ test('killHarper escalates to SIGKILL on a relaunched Harper and clears the pid ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone after SIGKILL`); strictEqual(existsSync(join(node.dataRootDir, 'hdb.pid')), false, 'SIGKILL leaves hdb.pid behind; killHarper must remove it'); - // teardownHarper after killHarper: must neither signal anything nor wait out the handoff window again. + // A second call, as teardownHarper makes after killHarper, must neither signal anything nor wait again. const secondStart = Date.now(); await killHarper(node.ctx, { graceMs: 200 }); ok(Date.now() - secondStart < 1000, 'a second call should return immediately'); @@ -1290,14 +1294,26 @@ test('killHarper does not wait for a relaunch after a clean stop that announced } }); +test('a readiness line Harper logs at boot is not taken for a restart', async () => { + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper({ HARPER_FAKE_BOOT_NOTIFY: '1' }); + await sleep(200); + + const start = Date.now(); + await teardownHarper(node.ctx); + ok(Date.now() - start < 1000, 'teardown must not wait for a relaunch that never happened'); + strictEqual(existsSync(node.dataRootDir), false, 'teardown should have removed the install directory'); + } finally { + await cleanupFakeHarperNode(node); + } +}); + test('a readiness line logged at boot does not hide a later restart', async () => { - const previousWait = process.env.HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS; - process.env.HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS = '1000'; let node: FakeHarperNode | undefined; try { node = await startRestartableFakeHarper({ HARPER_FAKE_BOOT_NOTIFY: '1', HARPER_FAKE_RELAUNCH_DELAY_MS: '300' }); - // Let the boot-time report's own relaunch window lapse before the real restart. - await sleep(1200); + await sleep(200); await restartFakeHarper(node); await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); @@ -1305,7 +1321,6 @@ test('a readiness line logged at boot does not hide a later restart', async () = const replacementPid = await readPidFile(join(node.dataRootDir, 'replacement.pid')); ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone when killHarper resolves`); } finally { - restoreEnv('HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS', previousWait); await cleanupFakeHarperNode(node); } }); @@ -1318,6 +1333,7 @@ test('teardownHarper keeps the data root when an announced replacement never rec node = await startRestartableFakeHarper({ HARPER_FAKE_RELAUNCH_DELAY_MS: '3000' }); await restartFakeHarper(node); + await killHarper(node.ctx); await teardownHarper(node.ctx); ok(existsSync(node.dataRootDir), 'teardown must not delete the root under a replacement that may still come up'); From 61b63c649b4c6e68fce6ef98e76f8bbe3cecbfa9 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 23 Sep 2026 10:37:31 -0600 Subject: [PATCH 05/11] fix: require a relaunch announcement to precede Harper's exit Review follow-up. The 50ms boot settle measured the runner's clock, so a stalled runner could read Harper's boot-time readiness log late, take it for a restart, and leave every later teardown of that node unconfirmed. A relaunch exits within milliseconds of announcing itself, so an announcement now counts only if the spawned process exited within 1s of it. The boot-log tests stop Harper with a short grace before teardown, since the Windows SIGTERM-equivalent does not stop a background node process. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016U46s3CDVAfhBsVXHFLaPJ Dispatch-Task: integration-testing-killharper-after-restart --- src/harperLifecycle.ts | 21 ++++++++++++++---- test/harperLifecycle.test.ts | 42 ++++++++++++++++++++++-------------- 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index 39c164c..dc19b92 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -132,6 +132,12 @@ const PROCESS_POLL_MS = 50; */ const BOOT_REPORT_SETTLE_MS = 50; +/** + * Harper exits within milliseconds of announcing a relaunch (it forks the replacement and exits in one + * synchronous step), so an announcement further than this from the exit was a late-delivered boot log. + */ +const RELAUNCH_EXIT_WINDOW_MS = 1000; + /** * The runtime to use for running Harper during tests. * Set via the HARPER_RUNTIME environment variable ('node' or 'bun'). @@ -807,6 +813,7 @@ const liveHarperProcesses = new Set(); let runnerCleanupRegistered = false; const relaunchAnnouncements = new WeakMap(); +const spawnedExitTimes = new WeakMap(); /** Shared so that a concurrent `killHarper` on the same node cannot return before the first finishes. */ const relaunchKills = new WeakMap>(); @@ -824,7 +831,10 @@ function trackHarperProcess(proc: ChildProcess, instanceId: string, hostname?: s // 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)); + proc.once('exit', () => { + liveHarperProcesses.delete(proc); + spawnedExitTimes.set(proc, Date.now()); + }); if (runnerCleanupRegistered) return trackedProcess; runnerCleanupRegistered = true; @@ -968,13 +978,16 @@ async function killRelaunchedHarper(proc: ChildProcess, dataRootDir: string, gra let pid = await readHarperPid(dataRootDir); // Read after the await above, which lets stdout that arrived alongside the exit be processed first. const announcedAt = relaunchAnnouncements.get(proc); - const relaunchDeadline = announcedAt === undefined ? 0 : announcedAt + getRelaunchPidWaitMs(); + const exitedAt = spawnedExitTimes.get(proc); + const relaunched = + announcedAt !== undefined && exitedAt !== undefined && exitedAt - announcedAt <= RELAUNCH_EXIT_WINDOW_MS; + const relaunchDeadline = relaunched ? announcedAt + getRelaunchPidWaitMs() : 0; while (pid === undefined && Date.now() < relaunchDeadline) { await sleep(PROCESS_POLL_MS); pid = await readHarperPid(dataRootDir); } - // An announced replacement that has not shown up yet may still come, for this call and any later one. - if (pid === undefined) return announcedAt === undefined; + // A replacement that has not shown up yet may still come, for this call and any later one. + if (pid === undefined) return !relaunched; relaunchAnnouncements.delete(proc); // A stale file can still name the spawned process, whose pid may since have been reused. if (pid === proc.pid || pid === process.pid || !isProcessTreeAlive(pid)) return true; diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index f8d1a87..fa86528 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -156,7 +156,9 @@ if (replacedPid) { }); process.stdout.write('successfully started\\n'); // What Harper's logger adds right after readiness when \`logging.stdStreams\` is on. - if (process.env.HARPER_FAKE_BOOT_NOTIFY === '1') setTimeout(() => process.stdout.write('Harper successfully started.\\n'), 10); + if (process.env.HARPER_FAKE_BOOT_NOTIFY_MS) { + setTimeout(() => process.stdout.write('Harper successfully started.\\n'), Number(process.env.HARPER_FAKE_BOOT_NOTIFY_MS)); + } } `, 'orphan-runner.mjs': ` @@ -1294,25 +1296,33 @@ test('killHarper does not wait for a relaunch after a clean stop that announced } }); -test('a readiness line Harper logs at boot is not taken for a restart', async () => { - let node: FakeHarperNode | undefined; - try { - node = await startRestartableFakeHarper({ HARPER_FAKE_BOOT_NOTIFY: '1' }); - await sleep(200); - - const start = Date.now(); - await teardownHarper(node.ctx); - ok(Date.now() - start < 1000, 'teardown must not wait for a relaunch that never happened'); - strictEqual(existsSync(node.dataRootDir), false, 'teardown should have removed the install directory'); - } finally { - await cleanupFakeHarperNode(node); - } -}); +for (const [label, notifyMs, stopAfterMs] of [ + ['right after boot', '10', 200], + // Past the boot settle window, as when a stalled runner reads the line late. + ['late', '100', 1200], +] as const) { + test(`a readiness line Harper logs at boot, delivered ${label}, is not taken for a restart`, async () => { + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper({ HARPER_FAKE_BOOT_NOTIFY_MS: notifyMs }); + await sleep(stopAfterMs); + + // Windows gets a short grace: its SIGTERM-equivalent does not stop a background node process. + const start = Date.now(); + await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); + ok(Date.now() - start < 3000, 'killHarper must not wait for a relaunch that never happened'); + await teardownHarper(node.ctx); + strictEqual(existsSync(node.dataRootDir), false, 'teardown should have removed the install directory'); + } finally { + await cleanupFakeHarperNode(node); + } + }); +} test('a readiness line logged at boot does not hide a later restart', async () => { let node: FakeHarperNode | undefined; try { - node = await startRestartableFakeHarper({ HARPER_FAKE_BOOT_NOTIFY: '1', HARPER_FAKE_RELAUNCH_DELAY_MS: '300' }); + node = await startRestartableFakeHarper({ HARPER_FAKE_BOOT_NOTIFY_MS: '10', HARPER_FAKE_RELAUNCH_DELAY_MS: '300' }); await sleep(200); await restartFakeHarper(node); From add44ab824450c9d9892d57905ded1cba4532811 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 23 Sep 2026 10:48:50 -0600 Subject: [PATCH 06/11] fix: detect Harper's relaunch by its repeated readiness line Review follow-up. The 50ms boot settle and the 1s announce-to-exit window were timing heuristics that could misjudge a restart either way: Harper's replacement allows its predecessor up to 15s to exit while RocksDB closes, so a slow exit fell outside the window and the replacement was missed again. A relaunch goes back through Harper's startup path and prints the exact readiness line it printed at boot; the line Harper's logger adds when logging.stdStreams is on is a different line. runHarperCommand now records the readiness line and treats only an exact repeat as a relaunch, with no timing involved. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016U46s3CDVAfhBsVXHFLaPJ Dispatch-Task: integration-testing-killharper-after-restart --- README.md | 4 +-- src/harperLifecycle.ts | 50 +++++++++------------------ test/harperLifecycle.test.ts | 65 +++++++++++++++++++++--------------- 3 files changed, 56 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index 313d1ed..003673a 100644 --- a/README.md +++ b/README.md @@ -134,9 +134,9 @@ 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. 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 (it logs a warning when it could not confirm the node stopped). Does not release the loopback address or clean up the install directory. Useful for restart scenarios where the test will call `startHarper` again. -It also stops a node that restarted itself. Harper's `restart` operation relaunches the node as a new, detached process and lets the one `startHarper` spawned exit, so once that process has exited `killHarper` terminates whichever process `/hdb.pid` names — the pid file `harper stop` uses — in the same SIGTERM, grace, SIGKILL sequence (on POSIX, waiting for its whole process group), and removes the pid file if the SIGKILL left it behind. The replacement records its pid only after its predecessor has exited, about half a second later on an idle machine. The spawned process reports ready on stdout a second time just before it relaunches, so after that report `killHarper` waits up to 5 seconds (15 under CI, or `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS`) from it for the pid file to appear. A node that stopped without relaunching reports nothing and pays no wait. If `killHarper` cannot confirm the node stopped — an announced replacement never recorded its pid, or a process outlived SIGKILL — it logs a warning, since its ports may still be held. Only the first relaunch is followed this way: when a replacement restarts again, wait for that restart to finish before tearing down. +It also stops a node that restarted itself. Harper's `restart` operation relaunches the node as a new, detached process and lets the one `startHarper` spawned exit, so once that process has exited `killHarper` terminates whichever process `/hdb.pid` names — the pid file `harper stop` uses — in the same SIGTERM, grace, SIGKILL sequence (on POSIX, waiting for its whole process group), and removes the pid file if the SIGKILL left it behind. The replacement records its pid only after its predecessor has exited, about half a second later on an idle machine. The spawned process prints its readiness line on stdout a second time just before it relaunches, so after that line `killHarper` waits up to 5 seconds (15 under CI, or `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS`) from it for the pid file to appear. A node that stopped without relaunching reports nothing and pays no wait. If `killHarper` cannot confirm the node stopped — an announced replacement never recorded its pid, or a process outlived SIGKILL — it logs a warning, since its ports may still be held. Only the first relaunch is followed this way: when a replacement restarts again, wait for that restart to finish before tearing down. `options.graceMs` overrides the SIGTERM→SIGKILL grace period (default `5000`, or `HARPER_INTEGRATION_TEST_TEARDOWN_GRACE_MS`). diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index dc19b92..3f9affb 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -125,19 +125,6 @@ function getRelaunchPidWaitMs(): number { const PROCESS_POLL_MS = 50; -/** - * Readiness reports this soon after startup readiness belong to the boot itself: with - * `logging.stdStreams` on, Harper's logger repeats the readiness line immediately. A restart cannot - * report this soon, since Harper waits 50ms after the `restart` operation before it even begins. - */ -const BOOT_REPORT_SETTLE_MS = 50; - -/** - * Harper exits within milliseconds of announcing a relaunch (it forks the replacement and exits in one - * synchronous step), so an announcement further than this from the exit was a late-delivered boot log. - */ -const RELAUNCH_EXIT_WINDOW_MS = 1000; - /** * The runtime to use for running Harper during tests. * Set via the HARPER_RUNTIME environment variable ('node' or 'bun'). @@ -462,8 +449,8 @@ export function runHarperCommand({ return new Promise((resolve, reject) => { let stdout = ''; let stderr = ''; + let readinessLine = ''; let postReadinessTail = ''; - let readyAt = 0; let settled = false; let readinessDetected = false; let idleTimer: NodeJS.Timeout; @@ -486,7 +473,10 @@ export function runHarperCommand({ const succeed = () => { if (settled || readinessDetected) return; readinessDetected = true; - readyAt = Date.now(); + if (completionMessage) { + const markerStart = stdout.indexOf(completionMessage); + readinessLine = stdout.slice(stdout.lastIndexOf('\n', markerStart) + 1, markerStart + completionMessage.length); + } // Left armed across registration, these would let registry-lock contention time out — and // kill — an instance that already booted successfully. clearTimers(); @@ -529,15 +519,14 @@ export function runHarperCommand({ // returned startupOutput is a snapshot taken at readiness, and the server may run // (and log) for the rest of the suite. if (startupFinished()) { - // Harper's `restart` goes back through its startup path, reporting ready again just - // before it hands the node to a detached replacement and exits. - if (completionMessage && readinessDetected) { + // Harper's `restart` goes back through its startup path, printing the readiness line again + // just before it hands the node to a detached replacement and exits. The whole line has to + // match: with `logging.stdStreams` on, Harper's logger also reports readiness at boot, in a + // line of its own. + if (readinessLine) { const recentOutput = postReadinessTail + dataString; - const now = Date.now(); - if (recentOutput.includes(completionMessage) && now - readyAt >= BOOT_REPORT_SETTLE_MS) { - relaunchAnnouncements.set(proc, now); - } - postReadinessTail = recentOutput.slice(-(completionMessage.length - 1)); + if (recentOutput.includes(readinessLine)) relaunchAnnouncements.set(proc, Date.now()); + postReadinessTail = recentOutput.slice(-(readinessLine.length - 1)); } return; } @@ -813,7 +802,6 @@ const liveHarperProcesses = new Set(); let runnerCleanupRegistered = false; const relaunchAnnouncements = new WeakMap(); -const spawnedExitTimes = new WeakMap(); /** Shared so that a concurrent `killHarper` on the same node cannot return before the first finishes. */ const relaunchKills = new WeakMap>(); @@ -831,10 +819,7 @@ function trackHarperProcess(proc: ChildProcess, instanceId: string, hostname?: s // 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); - spawnedExitTimes.set(proc, Date.now()); - }); + proc.once('exit', () => liveHarperProcesses.delete(proc)); if (runnerCleanupRegistered) return trackedProcess; runnerCleanupRegistered = true; @@ -978,16 +963,13 @@ async function killRelaunchedHarper(proc: ChildProcess, dataRootDir: string, gra let pid = await readHarperPid(dataRootDir); // Read after the await above, which lets stdout that arrived alongside the exit be processed first. const announcedAt = relaunchAnnouncements.get(proc); - const exitedAt = spawnedExitTimes.get(proc); - const relaunched = - announcedAt !== undefined && exitedAt !== undefined && exitedAt - announcedAt <= RELAUNCH_EXIT_WINDOW_MS; - const relaunchDeadline = relaunched ? announcedAt + getRelaunchPidWaitMs() : 0; + const relaunchDeadline = announcedAt === undefined ? 0 : announcedAt + getRelaunchPidWaitMs(); while (pid === undefined && Date.now() < relaunchDeadline) { await sleep(PROCESS_POLL_MS); pid = await readHarperPid(dataRootDir); } - // A replacement that has not shown up yet may still come, for this call and any later one. - if (pid === undefined) return !relaunched; + // An announced replacement that has not shown up yet may still come, for this call and any later one. + if (pid === undefined) return announcedAt === undefined; relaunchAnnouncements.delete(proc); // A stale file can still name the spawned process, whose pid may since have been reused. if (pid === proc.pid || pid === process.pid || !isProcessTreeAlive(pid)) return true; diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index fa86528..44f9ade 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -114,6 +114,7 @@ const { join } = require('node:path'); const root = process.env.HARPER_FAKE_ROOT; const pidFile = join(root, 'hdb.pid'); const replacedPid = Number(process.env.HARPER_PARENT_PROCESS_PID || 0); +const readinessLine = 'Harper 0.0.0-fake successfully started\\n'; function serve() { if (replacedPid && process.env.HARPER_FAKE_IGNORE_TERM === '1') process.on('SIGTERM', () => {}); else process.on('SIGTERM', () => { rmSync(pidFile, { force: true }); process.exit(0); }); @@ -144,7 +145,7 @@ if (replacedPid) { // Harper's restart, too, waits 50ms before it begins. setTimeout(() => { rmSync(pidFile, { force: true }); - process.stdout.write('successfully started\\n'); + process.stdout.write(readinessLine); spawn(process.execPath, [__filename], { detached: true, stdio: 'ignore', @@ -154,10 +155,10 @@ if (replacedPid) { setTimeout(() => process.exit(0), Number(process.env.HARPER_FAKE_LINGER_MS || 0)); }, 50); }); - process.stdout.write('successfully started\\n'); - // What Harper's logger adds right after readiness when \`logging.stdStreams\` is on. + process.stdout.write(readinessLine); + // What Harper's logger adds after readiness when \`logging.stdStreams\` is on. if (process.env.HARPER_FAKE_BOOT_NOTIFY_MS) { - setTimeout(() => process.stdout.write('Harper successfully started.\\n'), Number(process.env.HARPER_FAKE_BOOT_NOTIFY_MS)); + setTimeout(() => process.stdout.write('[main/0] [notify]: Harper successfully started.\\n'), Number(process.env.HARPER_FAKE_BOOT_NOTIFY_MS)); } } `, @@ -1279,6 +1280,22 @@ test('killHarper follows a relaunch the spawned process announced before it was } }); +test('killHarper follows a relaunch whose predecessor is slow to exit', async () => { + let node: FakeHarperNode | undefined; + try { + // Harper's replacement allows its predecessor 15s to exit, for RocksDB to finish closing. + node = await startRestartableFakeHarper({ HARPER_FAKE_LINGER_MS: '1500', HARPER_FAKE_RELAUNCH_DELAY_MS: '300' }); + await restartFakeHarper(node); + + await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); + + const replacementPid = await readPidFile(join(node.dataRootDir, 'replacement.pid')); + ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone when killHarper resolves`); + } finally { + await cleanupFakeHarperNode(node); + } +}); + test('killHarper does not wait for a relaunch after a clean stop that announced none', { skip: !isPosix }, async () => { let node: FakeHarperNode | undefined; try { @@ -1296,33 +1313,27 @@ test('killHarper does not wait for a relaunch after a clean stop that announced } }); -for (const [label, notifyMs, stopAfterMs] of [ - ['right after boot', '10', 200], - // Past the boot settle window, as when a stalled runner reads the line late. - ['late', '100', 1200], -] as const) { - test(`a readiness line Harper logs at boot, delivered ${label}, is not taken for a restart`, async () => { - let node: FakeHarperNode | undefined; - try { - node = await startRestartableFakeHarper({ HARPER_FAKE_BOOT_NOTIFY_MS: notifyMs }); - await sleep(stopAfterMs); - - // Windows gets a short grace: its SIGTERM-equivalent does not stop a background node process. - const start = Date.now(); - await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); - ok(Date.now() - start < 3000, 'killHarper must not wait for a relaunch that never happened'); - await teardownHarper(node.ctx); - strictEqual(existsSync(node.dataRootDir), false, 'teardown should have removed the install directory'); - } finally { - await cleanupFakeHarperNode(node); - } - }); -} +test('the readiness line Harper logs at boot is not taken for a restart', async () => { + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper({ HARPER_FAKE_BOOT_NOTIFY_MS: '100' }); + await sleep(300); + + // Windows gets a short grace: its SIGTERM-equivalent does not stop a background node process. + const start = Date.now(); + await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); + ok(Date.now() - start < 3000, 'killHarper must not wait for a relaunch that never happened'); + await teardownHarper(node.ctx); + strictEqual(existsSync(node.dataRootDir), false, 'teardown should have removed the install directory'); + } finally { + await cleanupFakeHarperNode(node); + } +}); test('a readiness line logged at boot does not hide a later restart', async () => { let node: FakeHarperNode | undefined; try { - node = await startRestartableFakeHarper({ HARPER_FAKE_BOOT_NOTIFY_MS: '10', HARPER_FAKE_RELAUNCH_DELAY_MS: '300' }); + node = await startRestartableFakeHarper({ HARPER_FAKE_BOOT_NOTIFY_MS: '100', HARPER_FAKE_RELAUNCH_DELAY_MS: '300' }); await sleep(200); await restartFakeHarper(node); From 49392d61cc4120bad5c7b82aa5551fd08e0b82d4 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 23 Sep 2026 10:59:32 -0600 Subject: [PATCH 07/11] fix: bound the relaunch-detection tail for a one-character readiness line slice(-0) returns the whole string, so a one-character readiness line would have kept every post-readiness chunk. Slice from an explicit start instead. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016U46s3CDVAfhBsVXHFLaPJ Dispatch-Task: integration-testing-killharper-after-restart --- src/harperLifecycle.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index 3f9affb..2328e5a 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -526,7 +526,7 @@ export function runHarperCommand({ if (readinessLine) { const recentOutput = postReadinessTail + dataString; if (recentOutput.includes(readinessLine)) relaunchAnnouncements.set(proc, Date.now()); - postReadinessTail = recentOutput.slice(-(readinessLine.length - 1)); + postReadinessTail = recentOutput.slice(recentOutput.length - (readinessLine.length - 1)); } return; } From 699791aa13438c0ac54ec705fc45431dd5c1db87 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 23 Sep 2026 11:04:51 -0600 Subject: [PATCH 08/11] fix: keep the whole relaunch-detection tail when a read is short The previous fix sliced from length - (n - 1), which goes negative for a read shorter than the readiness line and then counts from the end, dropping the head of a line split across reads. Clamp the start at zero, and cover a split readiness line with a test. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016U46s3CDVAfhBsVXHFLaPJ Dispatch-Task: integration-testing-killharper-after-restart --- src/harperLifecycle.ts | 2 +- test/harperLifecycle.test.ts | 43 +++++++++++++++++++++++++++++------- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index 2328e5a..f4d98fd 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -526,7 +526,7 @@ export function runHarperCommand({ if (readinessLine) { const recentOutput = postReadinessTail + dataString; if (recentOutput.includes(readinessLine)) relaunchAnnouncements.set(proc, Date.now()); - postReadinessTail = recentOutput.slice(recentOutput.length - (readinessLine.length - 1)); + postReadinessTail = recentOutput.slice(Math.max(0, recentOutput.length - (readinessLine.length - 1))); } return; } diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 44f9ade..88ac3bb 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -145,14 +145,26 @@ if (replacedPid) { // Harper's restart, too, waits 50ms before it begins. setTimeout(() => { rmSync(pidFile, { force: true }); - process.stdout.write(readinessLine); - spawn(process.execPath, [__filename], { - detached: true, - stdio: 'ignore', - env: { ...process.env, HARPER_PARENT_PROCESS_PID: String(process.pid) }, - }).unref(); - // Real Harper exits synchronously here; lingering lets a test stop it after it announced the relaunch. - setTimeout(() => process.exit(0), Number(process.env.HARPER_FAKE_LINGER_MS || 0)); + const relaunch = () => { + spawn(process.execPath, [__filename], { + detached: true, + stdio: 'ignore', + env: { ...process.env, HARPER_PARENT_PROCESS_PID: String(process.pid) }, + }).unref(); + // Real Harper exits synchronously here; lingering lets a test stop it after it announced the relaunch. + setTimeout(() => process.exit(0), Number(process.env.HARPER_FAKE_LINGER_MS || 0)); + }; + if (process.env.HARPER_FAKE_SPLIT_READINESS !== '1') { + process.stdout.write(readinessLine); + relaunch(); + return; + } + // Two reads on the runner's side, so detection has to carry the line across chunks. + process.stdout.write(readinessLine.slice(0, 22)); + setTimeout(() => { + process.stdout.write(readinessLine.slice(22)); + relaunch(); + }, 100); }, 50); }); process.stdout.write(readinessLine); @@ -1226,6 +1238,21 @@ test('killHarper waits for a replacement that has not yet recorded its pid', asy } }); +test('a relaunch readiness line split across reads is still recognized', async () => { + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper({ HARPER_FAKE_SPLIT_READINESS: '1', HARPER_FAKE_RELAUNCH_DELAY_MS: '300' }); + await restartFakeHarper(node); + + await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); + + const replacementPid = await readPidFile(join(node.dataRootDir, 'replacement.pid')); + ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone when killHarper resolves`); + } finally { + await cleanupFakeHarperNode(node); + } +}); + test('killHarper escalates to SIGKILL on a relaunched Harper and clears the pid file it leaves', { skip: !isPosix }, async () => { let node: FakeHarperNode | undefined; try { From bd2ce408e283cb3c7441c6398a8b0d0e520ffeed Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 1 Oct 2026 11:09:08 -0600 Subject: [PATCH 09/11] fix: time the relaunch wait from the predecessor's exit; keep the install while ports are held Review follow-up. The wait for a relaunched replacement's pid started at the relaunch announcement, but the replacement records its pid only once its predecessor has exited, which Harper allows up to 15s for while RocksDB closes; killHarper stopping that predecessor can itself take the whole grace period. A predecessor slower than the wait left the replacement running and teardown keeping its directory. The wait now runs from the predecessor's exit. When Harper's ports were still held after the kill, teardown parked the address but still deleted the install directory under the process holding them. It now keeps the directory as well. Co-Authored-By: Claude Opus 5.5 Dispatch-Task: fix-kriszyp_integration-testing_35-bdb5c256 --- README.md | 6 ++-- src/harperLifecycle.ts | 32 +++++++++++++------ test/harperLifecycle.test.ts | 62 +++++++++++++++++++++++++++++++++--- 3 files changed, 83 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 003673a..aadcaf0 100644 --- a/README.md +++ b/README.md @@ -136,7 +136,7 @@ Like `startHarper()`, but copies a component directory into the Harper install b 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 (it logs a warning when it could not confirm the node stopped). Does not release the loopback address or clean up the install directory. Useful for restart scenarios where the test will call `startHarper` again. -It also stops a node that restarted itself. Harper's `restart` operation relaunches the node as a new, detached process and lets the one `startHarper` spawned exit, so once that process has exited `killHarper` terminates whichever process `/hdb.pid` names — the pid file `harper stop` uses — in the same SIGTERM, grace, SIGKILL sequence (on POSIX, waiting for its whole process group), and removes the pid file if the SIGKILL left it behind. The replacement records its pid only after its predecessor has exited, about half a second later on an idle machine. The spawned process prints its readiness line on stdout a second time just before it relaunches, so after that line `killHarper` waits up to 5 seconds (15 under CI, or `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS`) from it for the pid file to appear. A node that stopped without relaunching reports nothing and pays no wait. If `killHarper` cannot confirm the node stopped — an announced replacement never recorded its pid, or a process outlived SIGKILL — it logs a warning, since its ports may still be held. Only the first relaunch is followed this way: when a replacement restarts again, wait for that restart to finish before tearing down. +It also stops a node that restarted itself. Harper's `restart` operation relaunches the node as a new, detached process and lets the one `startHarper` spawned exit, so once that process has exited `killHarper` terminates whichever process `/hdb.pid` names — the pid file `harper stop` uses — in the same SIGTERM, grace, SIGKILL sequence (on POSIX, waiting for its whole process group), and removes the pid file if the SIGKILL left it behind. The replacement records its pid only after its predecessor has exited, about half a second later on an idle machine. The spawned process prints its readiness line on stdout a second time just before it relaunches, so after that line `killHarper` waits for the pid file to appear, for up to 5 seconds (15 under CI, or `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS`) from when the announcing process exits. Harper gives that process up to 15 seconds to exit while RocksDB closes, so the wait does not start any earlier. A node that stopped without relaunching reports nothing and pays no wait. If `killHarper` cannot confirm the node stopped — an announced replacement never recorded its pid, or a process outlived SIGKILL — it logs a warning, since its ports may still be held. Only the first relaunch is followed this way: when a replacement restarts again, wait for that restart to finish before tearing down. `options.graceMs` overrides the SIGTERM→SIGKILL grace period (default `5000`, or `HARPER_INTEGRATION_TEST_TEARDOWN_GRACE_MS`). @@ -144,7 +144,7 @@ It also stops a node that restarted itself. Harper's `restart` operation relaunc Kills Harper's process tree, releases the loopback address back to the pool, and removes the install directory. Call in a teardown/`after()` hook. -Since `killHarper` waits for the process tree to exit, its fixed ports (Operations API, HTTP/S, MQTT/S) are already released by the time the address is recycled. As a safety assertion, teardown still verifies those ports are free before recycling — the pool only guarantees the *address* is bindable, not that these specific ports are free — and if any are still held (a sign a Harper process escaped the kill) it logs a warning and does not recycle the address: the slot stays parked until this process exits, so a later suite cannot co-bind the ports. +Since `killHarper` waits for the process tree to exit, its fixed ports (Operations API, HTTP/S, MQTT/S) are already released by the time the address is recycled. As a safety assertion, teardown still verifies those ports are free before recycling — the pool only guarantees the *address* is bindable, not that these specific ports are free — and if any are still held (a sign a Harper process escaped the kill) it logs a warning and leaves both the address and the install directory in place: the slot stays parked until this process exits, so a later suite cannot co-bind the ports, and the directory is not deleted under the process still holding them. If the kill itself could not be confirmed — Harper announced a relaunch but the replacement never recorded its pid, or a process outlived SIGKILL — teardown logs a warning and leaves both the install directory and the address in place rather than deleting the directory under a process that may still be running. @@ -152,7 +152,7 @@ If the kill itself could not be confirmed — Harper announced a relaunch but th - `HARPER_INTEGRATION_TEST_TEARDOWN_GRACE_MS` - Grace period after SIGTERM before escalating to SIGKILL. Default `5000`. - `HARPER_INTEGRATION_TEST_PORT_RELEASE_TIMEOUT_MS` - Max time teardown's safety assertion waits for Harper's ports to be free before recycling the loopback address (normally instant, since the process tree is already dead). Default `5000`. -- `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS` - After Harper announces a `restart` relaunch, how long `killHarper` waits for the replacement to record its pid. Default `5000` (`15000` under CI). +- `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS` - After a Harper process that announced a `restart` relaunch exits, how long `killHarper` waits for the replacement to record its pid. Default `5000` (`15000` under CI). ### `sendOperation(context, operation)` diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index f4d98fd..bcd3892 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -98,8 +98,8 @@ export const DEFAULT_TEARDOWN_GRACE_MS = parseInt(process.env.HARPER_INTEGRATION * Time teardown's safety assertion waits for Harper's fixed ports to be free before recycling the * loopback address. Killing Harper's process tree (and waiting for exit) should free them * immediately, so this normally returns on the first check; it only matters if a child process - * escaped the kill. If the ports are still in use at the deadline, a warning is logged and the address - * is not recycled. + * escaped the kill. If the ports are still in use at the deadline, a warning is logged and neither the + * address is recycled nor the install directory removed. * * Override with `HARPER_INTEGRATION_TEST_PORT_RELEASE_TIMEOUT_MS`. Default 5s. */ @@ -112,10 +112,11 @@ const SIGKILL_EXIT_WAIT_MS = 1000; const HARPER_PID_FILE = 'hdb.pid'; /** - * How long after Harper announces a relaunch teardown waits for the replacement to record its pid. The - * replacement does that only once its predecessor has exited and it has finished loading (~0.5s on an - * idle machine), so a teardown inside that window would otherwise find no pid file. Read per call, like - * the monitor's tunables, so a test can shorten it after import. + * How long teardown waits for a relaunched Harper's replacement to record its pid, counted from when the + * process that announced the relaunch exited. The replacement records it only once that predecessor is + * gone and it has finished loading (~0.5s on an idle machine), so a teardown inside that window would + * otherwise find no pid file. Read per call, like the monitor's tunables, so a test can shorten it after + * import. * * Override with `HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS`. Default 5s (15s under CI). */ @@ -802,6 +803,7 @@ const liveHarperProcesses = new Set(); let runnerCleanupRegistered = false; const relaunchAnnouncements = new WeakMap(); +const spawnedExitTimes = new WeakMap(); /** Shared so that a concurrent `killHarper` on the same node cannot return before the first finishes. */ const relaunchKills = new WeakMap>(); @@ -819,7 +821,10 @@ function trackHarperProcess(proc: ChildProcess, instanceId: string, hostname?: s // 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)); + proc.once('exit', () => { + liveHarperProcesses.delete(proc); + spawnedExitTimes.set(proc, Date.now()); + }); if (runnerCleanupRegistered) return trackedProcess; runnerCleanupRegistered = true; @@ -963,7 +968,12 @@ async function killRelaunchedHarper(proc: ChildProcess, dataRootDir: string, gra let pid = await readHarperPid(dataRootDir); // Read after the await above, which lets stdout that arrived alongside the exit be processed first. const announcedAt = relaunchAnnouncements.get(proc); - const relaunchDeadline = announcedAt === undefined ? 0 : announcedAt + getRelaunchPidWaitMs(); + // Harper gives a restarting predecessor up to 15s to exit (RocksDB may still be closing), and the + // replacement cannot record its pid before then, so the wait runs from that exit. + const relaunchDeadline = + announcedAt === undefined + ? 0 + : Math.max(announcedAt, spawnedExitTimes.get(proc) ?? Date.now()) + getRelaunchPidWaitMs(); while (pid === undefined && Date.now() < relaunchDeadline) { await sleep(PROCESS_POLL_MS); pid = await readHarperPid(dataRootDir); @@ -1080,10 +1090,12 @@ export async function teardownHarper(ctx: StartedHarperTestContext): Promise {}); + } setTimeout(() => process.exit(0), Number(process.env.HARPER_FAKE_LINGER_MS || 0)); }; if (process.env.HARPER_FAKE_SPLIT_READINESS !== '1') { @@ -1307,11 +1314,13 @@ test('killHarper follows a relaunch the spawned process announced before it was } }); -test('killHarper follows a relaunch whose predecessor is slow to exit', async () => { +test('killHarper waits for the replacement from its predecessor\'s exit, however long that took', async () => { + const previousWait = process.env.HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS; + process.env.HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS = '500'; let node: FakeHarperNode | undefined; try { - // Harper's replacement allows its predecessor 15s to exit, for RocksDB to finish closing. - node = await startRestartableFakeHarper({ HARPER_FAKE_LINGER_MS: '1500', HARPER_FAKE_RELAUNCH_DELAY_MS: '300' }); + // The predecessor takes longer to exit than the whole relaunch wait, as Harper allows (up to 15s). + node = await startRestartableFakeHarper({ HARPER_FAKE_LINGER_MS: '1000', HARPER_FAKE_RELAUNCH_DELAY_MS: '300' }); await restartFakeHarper(node); await killHarper(node.ctx, { graceMs: isPosix ? 2000 : 200 }); @@ -1319,6 +1328,30 @@ test('killHarper follows a relaunch whose predecessor is slow to exit', async () const replacementPid = await readPidFile(join(node.dataRootDir, 'replacement.pid')); ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone when killHarper resolves`); } finally { + restoreEnv('HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS', previousWait); + await cleanupFakeHarperNode(node); + } +}); + +test('killHarper still finds the replacement when stopping a predecessor that is mid-exit outlasts the wait', { skip: !isPosix }, async () => { + const previousWait = process.env.HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS; + process.env.HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS = '500'; + let node: FakeHarperNode | undefined; + try { + node = await startRestartableFakeHarper({ + HARPER_FAKE_LINGER_MS: '5000', + HARPER_FAKE_LINGER_IGNORES_TERM: '1', + HARPER_FAKE_RELAUNCH_DELAY_MS: '300', + }); + node.ctx.harper.process.stdin!.write('restart\n'); + const replacementPid = await readPidFile(join(node.dataRootDir, 'replacement.pid')); + + // SIGTERM is ignored, so the predecessor only goes at SIGKILL, after the grace and past the wait. + await killHarper(node.ctx, { graceMs: 1000 }); + + ok(!isAlive(replacementPid), `relaunched Harper ${replacementPid} should be gone when killHarper resolves`); + } finally { + restoreEnv('HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS', previousWait); await cleanupFakeHarperNode(node); } }); @@ -1391,6 +1424,27 @@ test('teardownHarper keeps the data root when an announced replacement never rec } }); +test('teardownHarper keeps the install directory while the node\'s ports are still held', async () => { + const hostname = '127.0.0.1'; + const dataRootDir = mkdtempSync(join(tmpdir(), 'harper-it-held-port-')); + // Stands in for a Harper child that escaped the kill; a port someone else already holds serves too. + const holder = createServer(); + await new Promise((resolve) => { + holder.once('error', () => resolve()); + holder.listen(OPERATIONS_API_PORT, hostname, () => resolve()); + }); + try { + strictEqual(await isPortFree(hostname, OPERATIONS_API_PORT), false, 'the port must be held for this test'); + + await teardownHarper({ harper: markHarperNode({ dataRootDir, hostname }) } as unknown as StartedHarperTestContext); + + ok(existsSync(dataRootDir), 'teardown must not delete the install directory under a process still holding its ports'); + } finally { + await new Promise((resolve) => holder.close(() => resolve())); + rmSync(dataRootDir, { recursive: true, force: true }); + } +}); + test('readHarperPid accepts only a plain pid a group signal cannot widen', async () => { const dataRootDir = mkdtempSync(join(tmpdir(), 'harper-it-pidfile-')); try { From c9c91f0282670df34855730f85a7e67534929562 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 8 Oct 2026 15:21:52 -0600 Subject: [PATCH 10/11] fix: keep a relaunched Harper unconfirmed across calls until its group is gone Harper removes hdb.pid on SIGTERM, so when the replacement's group outlived SIGKILL, a later teardownHarper found neither a pid nor an announcement and deleted the install directory under the surviving group. Remember the replacement's pid until its group is confirmed gone. Co-Authored-By: Claude Opus 5.5 Dispatch-Task: fix-kriszyp_integration-testing_35-35bd29e9 --- README.md | 2 +- src/harperLifecycle.ts | 18 ++++++++++++++---- test/harperLifecycle.test.ts | 34 ++++++++++++++++++++++++++++++++-- 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index aadcaf0..66b38b1 100644 --- a/README.md +++ b/README.md @@ -146,7 +146,7 @@ Kills Harper's process tree, releases the loopback address back to the pool, and Since `killHarper` waits for the process tree to exit, its fixed ports (Operations API, HTTP/S, MQTT/S) are already released by the time the address is recycled. As a safety assertion, teardown still verifies those ports are free before recycling — the pool only guarantees the *address* is bindable, not that these specific ports are free — and if any are still held (a sign a Harper process escaped the kill) it logs a warning and leaves both the address and the install directory in place: the slot stays parked until this process exits, so a later suite cannot co-bind the ports, and the directory is not deleted under the process still holding them. -If the kill itself could not be confirmed — Harper announced a relaunch but the replacement never recorded its pid, or a process outlived SIGKILL — teardown logs a warning and leaves both the install directory and the address in place rather than deleting the directory under a process that may still be running. +If the kill itself could not be confirmed — Harper announced a relaunch but the replacement never recorded its pid, or a process outlived SIGKILL — teardown logs a warning and leaves both the install directory and the address in place rather than deleting the directory under a process that may still be running. This holds for a teardown after a `killHarper` that already warned: the stop stays unconfirmed until that replacement is gone. **Environment Variables:** diff --git a/src/harperLifecycle.ts b/src/harperLifecycle.ts index bcd3892..0a4c7a0 100644 --- a/src/harperLifecycle.ts +++ b/src/harperLifecycle.ts @@ -804,6 +804,8 @@ let runnerCleanupRegistered = false; const relaunchAnnouncements = new WeakMap(); const spawnedExitTimes = new WeakMap(); +/** A replacement whose group outlived SIGKILL. Harper removes `hdb.pid` on SIGTERM, so a later call may only know it from here. */ +const relaunchSurvivors = new WeakMap(); /** Shared so that a concurrent `killHarper` on the same node cannot return before the first finishes. */ const relaunchKills = new WeakMap>(); @@ -962,10 +964,11 @@ function killSpawnedHarper(proc: ChildProcess, graceMs: number): Promise { - let pid = await readHarperPid(dataRootDir); + let pid = (await readHarperPid(dataRootDir)) ?? relaunchSurvivors.get(proc); // Read after the await above, which lets stdout that arrived alongside the exit be processed first. const announcedAt = relaunchAnnouncements.get(proc); // Harper gives a restarting predecessor up to 15s to exit (RocksDB may still be closing), and the @@ -982,13 +985,20 @@ async function killRelaunchedHarper(proc: ChildProcess, dataRootDir: string, gra if (pid === undefined) return announcedAt === undefined; relaunchAnnouncements.delete(proc); // A stale file can still name the spawned process, whose pid may since have been reused. - if (pid === proc.pid || pid === process.pid || !isProcessTreeAlive(pid)) return true; + if (pid === proc.pid || pid === process.pid || !isProcessTreeAlive(pid)) { + relaunchSurvivors.delete(proc); + return true; + } signalProcessTree(pid, 'SIGTERM'); if (!(await waitForProcessTreeExit(pid, graceMs))) { signalProcessTree(pid, 'SIGKILL'); - if (!(await waitForProcessTreeExit(pid, SIGKILL_EXIT_WAIT_MS))) return false; + if (!(await waitForProcessTreeExit(pid, SIGKILL_EXIT_WAIT_MS))) { + relaunchSurvivors.set(proc, pid); + return false; + } } + relaunchSurvivors.delete(proc); // Harper removes its pid file on SIGTERM but not on SIGKILL, and a pid left behind can be reused // before a later call reads it. if ((await readHarperPid(dataRootDir)) === pid) await rm(join(dataRootDir, HARPER_PID_FILE), { force: true }); diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index fae8076..59fd72c 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -120,7 +120,11 @@ const readinessLine = 'Harper 0.0.0-fake successfully started\\n'; function serve() { if (replacedPid && process.env.HARPER_FAKE_IGNORE_TERM === '1') process.on('SIGTERM', () => {}); else process.on('SIGTERM', () => { rmSync(pidFile, { force: true }); process.exit(0); }); - if (replacedPid && process.env.HARPER_FAKE_DESCENDANT) { + if (replacedPid && process.env.HARPER_FAKE_DESCENDANT === 'zombie') { + // Stands in for a member stuck in uninterruptible I/O: a child that exits at once under a parent that + // then leaves the group without reaping it, so the group outlives SIGKILL until that parent goes. + spawn('perl', ['-e', 'use POSIX; exit 0 unless fork(); POSIX::setsid(); open(my $f, ">", $ARGV[0]) or die; print $f $$; close($f); sleep 1000', join(root, 'zombie-holder.pid')], { stdio: 'ignore' }); + } else if (replacedPid && process.env.HARPER_FAKE_DESCENDANT) { const script = process.env.HARPER_FAKE_DESCENDANT === 'ignore-term' ? "process.on('SIGTERM', () => {}); setInterval(() => {}, 1000)" : 'setInterval(() => {}, 1000)'; @@ -1196,7 +1200,7 @@ async function restartFakeHarper(node: FakeHarperNode): Promise { async function cleanupFakeHarperNode(node: FakeHarperNode | undefined): Promise { if (!node) return; - for (const name of ['replacement.pid', 'descendant.pid']) { + for (const name of ['replacement.pid', 'descendant.pid', 'zombie-holder.pid']) { const pid = Number(await readFile(join(node.dataRootDir, name), 'utf-8').catch(() => '')); if (pid > 0) forceKill(pid); } @@ -1424,6 +1428,32 @@ test('teardownHarper keeps the data root when an announced replacement never rec } }); +test('a replacement group that outlived SIGKILL keeps a later teardown from cleaning up under it', { skip: process.platform !== 'linux' && 'relies on a zombie keeping a Linux process group alive' }, async () => { + let node: FakeHarperNode | undefined; + // Held here too: a teardown that wrongly cleans up deletes the file cleanup would read it from. + let holderPid: number | undefined; + try { + node = await startRestartableFakeHarper({ HARPER_FAKE_DESCENDANT: 'zombie' }); + await restartFakeHarper(node); + await readPidFile(join(node.dataRootDir, 'hdb.pid')); + holderPid = await readPidFile(join(node.dataRootDir, 'zombie-holder.pid')); + + await killHarper(node.ctx, { graceMs: 200 }); + strictEqual(existsSync(join(node.dataRootDir, 'hdb.pid')), false, 'the leader should have removed hdb.pid on SIGTERM'); + + await teardownHarper(node.ctx); + ok(existsSync(node.dataRootDir), 'teardown must not delete the root under a group that outlived SIGKILL'); + + // Once the group is gone, the stop is confirmed after all. + forceKill(holderPid); + await teardownHarper(node.ctx); + strictEqual(existsSync(node.dataRootDir), false, 'teardown should clean up once the surviving group is gone'); + } finally { + forceKill(holderPid); + await cleanupFakeHarperNode(node); + } +}); + test('teardownHarper keeps the install directory while the node\'s ports are still held', async () => { const hostname = '127.0.0.1'; const dataRootDir = mkdtempSync(join(tmpdir(), 'harper-it-held-port-')); From 6591eb91d308b207a872e75929900f3902e9cd10 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 8 Oct 2026 15:31:52 -0600 Subject: [PATCH 11/11] test: skip the surviving-group teardown test where perl is unavailable Co-Authored-By: Claude Opus 5.5 Dispatch-Task: fix-kriszyp_integration-testing_35-35bd29e9 --- test/harperLifecycle.test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/harperLifecycle.test.ts b/test/harperLifecycle.test.ts index 59fd72c..be2f46d 100644 --- a/test/harperLifecycle.test.ts +++ b/test/harperLifecycle.test.ts @@ -1,6 +1,6 @@ import { test, before, after } from 'node:test'; import { ok, strictEqual, match, doesNotMatch, rejects } from 'node:assert'; -import { spawn, type ChildProcess } from 'node:child_process'; +import { spawn, spawnSync, type ChildProcess } from 'node:child_process'; import { once } from 'node:events'; import { mkdtempSync, writeFileSync, rmSync, existsSync } from 'node:fs'; import { readFile, writeFile as writeFileAsync } from 'node:fs/promises'; @@ -1428,7 +1428,10 @@ test('teardownHarper keeps the data root when an announced replacement never rec } }); -test('a replacement group that outlived SIGKILL keeps a later teardown from cleaning up under it', { skip: process.platform !== 'linux' && 'relies on a zombie keeping a Linux process group alive' }, async () => { +// The fixture's zombie holder needs perl, and a zombie keeping a process group alive is verified only on Linux. +const canHoldZombieGroup = process.platform === 'linux' && spawnSync('perl', ['-e', '1']).status === 0; + +test('a replacement group that outlived SIGKILL keeps a later teardown from cleaning up under it', { skip: !canHoldZombieGroup && 'needs Linux and perl for a group that outlives SIGKILL' }, async () => { let node: FakeHarperNode | undefined; // Held here too: a teardown that wrongly cleans up deletes the file cleanup would read it from. let holderPid: number | undefined;