ExistingProcessWrapper: unref() and the 'exit' event are mutually exclusive, and the liveness interval is never unref'd

[kriszyp]

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':

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:

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:

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.