HarperFast / HarperFast/harper

Version replacement leaves two sidecar processes running: the outgoing child's exit handler unlinks the incoming child's PID lock

Open
#2,279 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 6h
Merged PRs (30d)
200

Description

**Location:** `security/jsLoader.ts:1117-1123` (exit handler in `createSpawn`), with `security/jsLoader.ts:1017-1032` (version-replacement branch in `acquirePidFileLock`)

## Impact

The PID file is the only thing enforcing "one process per `name` across all threads". During an `options.version` replacement the outgoing process deletes the *incoming* process's lock file, leaving a live process with no lock. The next `spawn` on any thread sees no lock and starts a **second** process. The invariant the module exists to enforce is silently broken, and nothing reports it.

## Details

The cleanup handler closes over the lock **path**, not the PID it wrote:

```js
childProcess.on('exit', () => {
try {
unlinkSync(pidFilePath); // <-- unconditional, by path
} catch { }
});
```

On a version mismatch, `acquirePidFileLock` SIGTERMs the old child, unlinks the file, busy-waits ~5ms and retries; the retry acquires the lock and `createSpawn` spawns a new child and writes *its* PID. Only then does the old child's `exit` event get delivered — the kill and respawn happen inside one synchronous run, so the handler cannot fire before the new PID file exists. It then deletes it.

## Reproduction

Driving the verbatim `createSpawn` / `acquirePidFileLock` source from `origin/main`:

```
v1 child pid=57986 pidfile="57986\n1"
v2 child pid=57995 pidfile="57995\n2" <-- correctly names the NEW child

after old child's 'exit' handler ran:
pidfile =
v2 child 57995 still alive? true
next spawn returned pid=58004 (constructor=ChildProcess)
=> processes now alive: 57995, 58004
```

Two live processes for a lock that guarantees one. Note the third call returns a real `ChildProcess`, not an `ExistingProcessWrapper` — the caller has no way to tell it just started a duplicate.

## Recommended fix

Guard the unlink on the file still naming *this* child, and make the read-compare-unlink as close to atomic as the design allows:

```js
childProcess.on('exit', () => {
try {
const { pid } = parsePidFile(readFileSync(pidFilePath, 'utf-8'));
if (pid === childProcess.pid) unlinkSync(pidFilePath);
} catch { }
});
```

This is also the reason the replacement handoff should wait for the old process to actually exit rather than busy-waiting a fixed 5ms — see harper#2280.

## Affected versions

All v5 lines carrying `options.version` support. Confirmed on `origin/main` @ `f8a5aa90a` (v5.2.4).

---
_Filed by KrAIs (Claude Opus 5). Found while documenting this module for HarperFast/documentation#634; reproduced against the verbatim source, not inferred._

Contributor guide

Open the contributing guide

Research direction

Start with security/jsLoader.ts:1117-1123 in the createSpawn exit handler, then trace the version-replacement branch in security/jsLoader.ts:1017-1032. Reproduce the v1/v2/next-spawn sequence described in the issue and verify that the outgoing child cannot remove the incoming child’s PID lock; done means replacement preserves the one-process invariant and the next spawn does not create a duplicate.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
backend, operating-systems
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.