HarperFast / HarperFast/harper
ExistingProcessWrapper: unref() and the 'exit' event are mutually exclusive, and the liveness interval is never unref'd
- Dominant language
- JavaScript
- Stars
- 89
- Forks
- 10
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 200
Description
**Location:** `security/jsLoader.ts:934-969` (`ExistingProcessWrapper`), specifically `:943` (the interval) and `:966-969` (`unref`)
## Impact
`ExistingProcessWrapper` is what a caller gets back on the *common* path — every `spawn` for a name that already has a live process, on every thread. It has two defects that trade against each other, so a caller cannot avoid both:
1. Calling `unref()` permanently disables the `'exit'` event.
2. Not calling `unref()` leaves a ref'd 1-second `setInterval` running for the lifetime of the thread, with no way to release it.
## Details
A single `setInterval` is both the liveness poll and the sole source of `'exit'`:
```js
this.checkInterval = setInterval(() => {
try { process.kill(pid, 0); }
catch {
clearInterval(this.checkInterval);
this.emit('exit', null, null); // only emitter of 'exit'
}
}, 1000);
```
and `unref()` destroys it outright:
```js
unref() {
clearInterval(this.checkInterval); // not this.checkInterval.unref()
return this;
}
```
On a real `ChildProcess`, `unref()` means "don't hold the event loop open" and has nothing to do with whether `'exit'` fires. Here the two are mutually exclusive, which is a silent contract break: code written against the real `ChildProcess` (`child.unref(); child.on('exit', …)`) compiles, runs, and never fires.
The interval is also never `unref()`'d at construction, so absent an explicit `unref()` call it is a ref'd handle held for the life of the thread. That matters here specifically: Harper deliberately drains the event loop rather than force-exiting worker threads — `server/jobs/jobProcess.ts:98-105` calls `parentPort?.unref()` and comments that this lets "the event loop drain naturally without calling `process.exit()`" (to avoid a Bun/NAPI crash), backed only by an `.unref()`'d 3s `realExit` timer. `server/threads/workerProcessGuard.ts` additionally intercepts `process.exit()` in workers. A ref'd 1s interval is exactly the kind of handle that teardown shape is sensitive to. I have not measured an end-to-end shutdown hang, so treat the leak as confirmed and the shutdown consequence as plausible-pending-measurement.
## Reproduction
```
=== A: no unref() -> 'exit' fires ===
killed pid 58972 at t=300ms
[nounref] got 'exit' event
=== B: unref() -> 'exit' never fires ===
called unref()
killed pid 59039 at t=300ms
(no 'exit' event; process ran to its 2500ms deadline)
=== C: does the un-unref'd interval hold the event loop open? ===
wrapper created for live pid 59133; no other work pending.
if the interval were unref'd, this process would exit now.
STILL RUNNING at t=3000ms -> event loop held open by checkInterval
```
## Recommended fix
`unref()` should unref the timer, not clear it:
```js
constructor(pid) {
...
this.checkInterval = setInterval(..., 1000);
this.checkInterval.unref(); // never hold the loop open
}
unref() { this.checkInterval.unref(); return this; }
ref() { this.checkInterval.ref(); return this; }
```
Unref'ing at construction matches `ChildProcess` semantics more closely than the current default and removes the leak, while `'exit'` keeps firing for as long as the thread is alive. Consider adding `ref()` for symmetry, since callers coming from `ChildProcess` will expect it. A `close()`/`dispose()` that genuinely stops polling would be the honest name for what `unref()` does today.
## Affected versions
All v5 lines. Confirmed on `origin/main` @ `f8a5aa90a` (v5.2.4).
---
_Filed by KrAIs (Claude Opus 5). Found while documenting this module for HarperFast/documentation#634; both halves reproduced against the verbatim source._
Contributor guide
Research direction
Start in security/jsLoader.ts:934-969 at ExistingProcessWrapper and reproduce the three cases described in the issue. Read server/jobs/jobProcess.ts:98-105 and server/threads/workerProcessGuard.ts to understand worker teardown. Done means unref() no longer suppresses the exit event, the liveness timer does not keep the event loop alive, and the relevant behavior has regression coverage.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- node.js, typescript
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100