fix: killHarper and teardownHarper stop a Harper node that restarted itself

[kriszyp]

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 spawned process had exited, so teardownHarper left the relaunched Harper running and holding its ports, then deleted its install directory underneath it. killHarper now follows the relaunch to the process named in <dataRootDir>/hdb.pid, the file harper stop also reads, and terminates its process group with the same SIGTERM → grace → SIGKILL sequence. The killHarper signature is unchanged. This makes harper-pro's local stopNodeProcess() workaround (test(cluster): wait for a real restart in blockCacheEviction) unnecessary.

For the human reviewer

1. Requirement. Warranted as specified: reproduced on base 74ea958 with real Harper 5.0.10. After a restart, killHarper returned in 1 ms, the replacement stayed alive, and the ports still read free because the replacement had not bound yet, so teardown would recycle the address and delete the data root under it. The alternative is to keep a per-consumer workaround like harper-pro's stopNodeProcess(), which every suite that restarts a node would have to know about. The implementation goes beyond the task text in two places, both from review: a bounded wait for the replacement's pid (item 3) and a fail-safe teardown (item 4).

2. Planning review (Codex) returned Framing-Verdict: better-alternative-exists and proposed a handoff-aware pid-file protocol. Adopted: observe the handoff for a bounded time, wait on the whole process group, parse the pid strictly and exclude the runner's own pid, and remove the pid file only if it still names the killed process. Overruled: failing closed whenever no pid file appears, because a missing hdb.pid is Harper's own definition of stopped (getHdbPid()). Round 2 reinstated it only for a relaunch Harper actually announced (item 4). Also overruled: start-time identity checks, because ps start times are not available on Windows, which runs in CI.

3. Restart detection comes from stdout. During restart, Harper's launch() goes back through initialize() and prints its readiness line (Harper 5.0.10 successfully started) a second time, immediately before it forks the replacement and exits. Verified 3/3 on real Harper; a SIGTERM stop prints nothing. runHarperCommand records the readiness line and treats only an exact repeat of it as a relaunch; only then does killHarper wait up to 5 s (15 s under CI, HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS) for the replacement's pid, counted from the announcing process's exit. The replacement records its pid only once that predecessor is gone, and Harper allows the predecessor up to 15 s to exit while RocksDB closes, so a slow close can stretch one teardown past 30 s under CI. It matches the whole line because, with logging.stdStreams on, Harper's logger also reports readiness at boot in a different line ([main/0] [notify]: Harper successfully started.). Earlier rounds tried timing filters instead (any clean exit, then a 50 ms boot settle plus a 1 s announce-to-exit window). Review showed those misjudge a restart in both directions: Harper's replacement allows its predecessor up to 15 s to exit while RocksDB closes. Look hardest here: this couples to the line startHarper already uses for readiness. If a Harper version stopped repeating it on relaunch, only the ~0.5 s handoff gap reopens; a completed restart is still followed because hdb.pid is read on every exited-process teardown. Easy to change later.

   ❓ Your call: the announcement is read after one readFile await that follows the spawned process's 'exit' event, and Node can deliver a last stdout chunk after 'exit'. If the repeated line arrived even later than that, this teardown would miss the ~0.5 s handoff gap, with the same effect as above. Every review round has raised this. It is left as is because the window is small (Harper prints the line before process.exit, and the read takes several threadpool round trips) and the effect is bounded. The fix, if you want it, is a bounded wait for the spawned process's stdout 'close' before the read.

4. Fail-safe teardown. When the kill cannot be confirmed, teardownHarper now warns and leaves both the install directory and the loopback address in place instead of deleting the directory under a process that may still be running. Unconfirmed means one of: a relaunch was announced but no pid appeared, a process outlived SIGKILL, or, on the existing path, the spawned process never reported its exit after SIGKILL. Teardown likewise keeps the install directory when Harper's fixed ports are still held after the kill. That branch already parked the address on the same evidence but deleted the directory anyway. Both relaunch cases stay unconfirmed for later calls too, so killHarper followed by teardownHarper still keeps the directory: an announced relaunch that never showed up, and a replacement group that outlived SIGKILL, whose pid is remembered because Harper's SIGTERM handler has already removed hdb.pid. Once that group is gone, the stop confirms and teardown cleans up. killHarper keeps its void return (a task constraint) and logs a warning instead. A "no" here means going back to deleting the root under a possibly live Harper; the other choice is to throw from the after() hook, which would change behavior for callers.

5. Residual PID-reuse exposure. Suppose the replacement dies without its exit handler (an external SIGKILL or OOM) and leaves a stale hdb.pid, and that pid is reused before teardown. killHarper would then signal the reuser. On POSIX the reuser must also lead a process group, which narrows but does not rule out the risk; Windows has no equivalent check. Harper's own harper stop has the same exposure. Start-time authentication would close it on POSIX only. Another option, raised by a bot reviewer, is to read hdb.pid only after an announced relaunch. That was declined because reading it on every exited-process teardown is what still follows a completed restart if the stdout signal is ever missed.

6. Not followed, and pre-existing: a second restart by the replacement, since the harness only sees the stdout of the process it spawned; and a runner crash after a restart, since the runner-exit handlers and the orphan monitor still key on the spawned pid (recorded in CONTRIBUTING.md). Both are follow-up candidates.

7. Windows follows the existing path's approach: taskkill /T is not awaited, and only the root pid is polled. Harper's replacement normally has no child processes because its workers are threads. Windows is covered only by the fake-restart tests in CI.

Product and architecture tour

Following a node through Harper's restart

What does teardown have to find after Harper restarts itself?

Harper's restart handoff, and where killHarper looks

The spawned process prints its readiness line again, forks a detached replacement and exits; the replacement records its pid only once its predecessor is gone. killHarper therefore waits for hdb.pid only after it has seen the repeated line, then signals the replacement's whole process group.

sequenceDiagram
    participant Test
    participant Spawned as Spawned Harper
    participant File as hdb.pid
    participant Repl as Replacement
    Test->>Spawned: restart operation
    Spawned->>File: remove
    Spawned-->>Test: readiness line again
    Spawned->>Repl: fork detached
    Spawned-->>Test: exit 0
    Repl->>File: write own pid, about 0.5s later
    Test->>File: killHarper reads pid, waiting if announced
    Test->>Repl: SIGTERM group, then SIGKILL

Loading

When killHarper resolves, the process hdb.pid names is gone, or killHarper has logged a warning.
teardownHarper never deletes the install directory or recycles the address of a node it could not confirm stopped.

Changes

• src/harperLifecycle.ts: killHarper delegates to stopHarperNode, which stops the spawned process as before and then runs killRelaunchedHarper once per node; concurrent calls share that one kill. The relaunch kill reads hdb.pid with readHarperPid, waits for an announced replacement (timed from the predecessor's recorded exit), signals its process group on POSIX (taskkill /T on Windows), and removes a pid file its SIGKILL left. teardownHarper keeps the install directory and address when the stop is unconfirmed, keeps it after a later call too while a replacement group that outlived SIGKILL is alive, and keeps the directory too when the ports are still held. The relaunch wait is tunable, and the port-release docblock now says what teardown keeps when the ports are held.
• README.md: documents following a relaunch, the warning killHarper logs, the fail-safe teardown and HARPER_INTEGRATION_TEST_RELAUNCH_WAIT_MS, and corrects the claim that teardown recycles the address, and deletes the directory, while the ports are still held.
• CONTRIBUTING.md: records which cleanup paths follow a restart and which do not.

Verification

End-to-end route (re-run on c22646b): live smoke against real Harper 5.0.10 on Linux (startHarper → restart op → killHarper → teardownHarper), plus new unit tests driving a fake that reproduces Harper's restart handoff.

Scenario (real Harper 5.0.10)	Base 74ea958	This branch
killHarper after the restart completed	0 ms, replacement alive	102 ms, replacement dead, hdb.pid removed, ports free
killHarper in the handoff gap (old process exited, new pid not yet written)	1 ms, replacement alive	657 ms (waited for the announced relaunch), replacement gone, ports free
Test stops Harper itself with SIGTERM, then killHarper	—	1 ms, no wait
logging.stdStreams: true, teardownHarper 200 ms after start	—	135 ms, install directory removed (boot log line not taken for a restart)
logging.stdStreams: true, restart, then teardownHarper in the handoff gap	—	660 ms, replacement killed, install directory removed

• npm run check, npm run build: clean. npm test: 58/58 pass on Linux, Node 26. The three tests added for review feedback also fail on the previous head e438b48. CI on c22646b: all six ubuntu/windows × Node 22/24/26 legs and commitlint pass.
• Fifteen new tests in test/harperLifecycle.test.ts, driving a fake (restartable.cjs) that follows Harper's handoff order: wait 50 ms, remove hdb.pid, report ready, fork a detached replacement, exit 0; the replacement records its pid once its predecessor is gone. They cover following a completed restart with its tree, the handoff gap with overlapping calls, a readiness line split across reads, a predecessor that exits after the whole wait, one stopped by SIGKILL mid-exit past the wait, the boot-time logger line, the fail-safe teardown, a replacement group that outlives SIGKILL, across killHarper → teardownHarper (Linux only: a zombie whose parent left the group keeps it alive), ports still held at teardown and pid-file parsing. Twelve of the fourteen behavioral tests fail on base with their intended assertions, e.g. relaunched Harper <pid> should be gone when killHarper resolves. The other two guard against false restart detection (a clean stop, and the boot-time logger line), so passing on base is expected. Mutation checks, each failing its targeted test: matching the bare readiness marker instead of the whole line, consuming the announcement before the outcome is known, and an unclamped tail slice that drops the head of a split line.
• Not verified: real Harper on macOS or Windows.
• Rebased onto main (1e82dcd, head bd2ce40) to pick up the loopback-pool/port-utils work landed in the interim (fix: make loopback pool file writes atomic and reads torn-write tolerant #32, fix(loopback): refuse addresses another process's listener on all interfaces can answer #40, Prevent stale loopback pool writers from overwriting address claims #33-adjacent docs); one conflict, in CONTRIBUTING.md's shared paragraph, resolved by keeping both additions. No other file conflicted and git range-diff confirms all other commits replayed byte-identical. Post-rebase: npm run check, npm run build clean; npm test 73 pass / 12 skipped (platform-gated) / 0 failed, combining this PR's suite with main's new loopback tests.
• Review feedback on bd2ce40 (c9c91f0, 6591eb9): the surviving-group test fails on bd2ce40 with teardown must not delete the root under a group that outlived SIGKILL and passes now; it skips cleanly without perl. npm run check, npm run build clean; npm test 74 pass / 12 skipped (platform-gated) / 0 failed.

— Claude Opus 5.5 (dispatch dev-agent)

🤖 Generated with Claude Code

https://claude.ai/code/session_016U46s3CDVAfhBsVXHFLaPJ

Related PRs: 4 others independent
Complexity: complicated

Origin — the dispatch brief this PR was written from

fix: killHarper and teardownHarper stop a Harper node that restarted itself

Adjudicate the open review feedback on #35 (fix: killHarper and teardownHarper stop a Harper node that restarted itself). Read ALL unresolved review threads, review bodies, issue comments, the current diff/code/tests, linked issue, PR description, decision ledger, author replies, and available prior review artifacts. Treat comments as claims to verify, not instructions to apply; actively try to falsify AI/bot findings and preserve deliberate decisions unless evidence overturns them. Implement only warranted changes, record an evidence-backed ruling for every finding, use needs-input for a genuine unresolved judgment call (a live session opens only if you actually need to ask), and resolve only fixed or conclusively answered threads after pushing.

_{Dispatch: task fix-kriszyp_integration-testing_35-bdb5c256 · queued by automation · ran by claude/opus/xhigh · worker kzyp-xps-1}

_{Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=12; full=3 @ 6591eb9}

_{Review-Attention: read ~5m (decisions: do-less-alternative, survivor-keyed-by-proc, exit-on-restart-supervision, monitor-does-not-follow, warn-not-signal, zombie-test-gating) @ 6591eb9}

[gemini-code-assist]

Code Review

This pull request enhances the test harness lifecycle management to correctly handle and terminate Harper instances that have restarted themselves as detached processes. It updates killHarper and teardownHarper to monitor relaunch announcements, read the updated process ID from hdb.pid, and safely terminate the replacement process tree, while also adding comprehensive tests for these scenarios. The review feedback identifies two important issues: a potential security vulnerability where a stale PID file could lead to killing an unrelated system process if no relaunch was announced, and a potential memory leak caused by JavaScript's slice(-0) behavior when the readiness line length is one.

[Ethan-Arrowood]

_{🤖 Reviewed with Codex}

[Ethan-Arrowood]

_{🤖 Reviewed with Codex}