Conversation
…rocess stop() was calling this.process.kill(0). In the Node child_process API, signal 0 is a liveness probe: it checks whether the process exists and returns true/false, but sends no signal. The forked verdaccio process therefore kept running after every test file's afterAll(), leaking one process per file that uses VerdaccioRegistry. This restores the default SIGTERM (the behavior before #16540) and suppresses the exit listener's 'exited with signal' log when the exit was triggered by stop(), so normal teardown stays quiet. A new test starts a registry, calls stop(), and asserts the child's exit event fires.
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Reproduced by starting a CI status: the diff is green. The new test passed on every lane in both build 85129 and build 85143. Remaining red on 85143 is 11 tests all tagged flaky by the annotation tooling (napi threadsafe finalizer, spawn-streaming-stdout, svelte client-side, grpc-js subchannel pool, etc.), each of which passed alone or on retry and none of which import Self-review raised no surviving concerns; the |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re: duplicate check pointing at #34687. That PR does include the same |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-harness-only fix; kill(0) → kill() is the correct restoration of pre-#16540 behavior. The inline nit about resetting #stopped in start() is non-blocking (no current caller restarts a registry, and the only casualty would be a suppressed diagnostic log).
Extended reasoning...
Overview
This PR touches two files, both under test/:
test/harness.ts: changesVerdaccioRegistry.stop()fromthis.process?.kill(0)tothis.process?.kill(), and adds a#stoppedflag so theexitlistener stays quiet during intentional teardown while still logging unexpected mid-test crashes.test/cli/install/verdaccio-registry-lifecycle.test.ts: new test that starts a registry, callsstop(), and asserts the child'sexitevent fires (viaonce(child, "exit")raced against a bounded 3s sleep), with anafterAllSIGKILL fallback for cleanup.
No runtime (src/) code is touched.
Security risks
None. This is test infrastructure; the change sends SIGTERM to a test-spawned child process instead of a no-op signal-0 probe.
Level of scrutiny
Low. The core fix is a one-token change whose correctness follows directly from Node's documented child_process.kill() semantics — signal 0 only tests process existence and never delivers a signal. The #stopped flag is a small quality-of-life addition whose only effect is suppressing a console.error during normal teardown. The new test follows harness conventions: awaits the actual observable condition (exit event) rather than sleeping, uses a bounded race for the timeout branch, and cleans up with SIGKILL in afterAll if the assertion fails.
Other factors
The bug-hunting system flagged one nit: #stopped is set in stop() but never reset in start(), so a start→stop→start sequence would carry the stale flag into the second lifecycle. I confirmed via the finding's own analysis that no current test/ consumer restarts a VerdaccioRegistry instance, and the worst case is a suppressed diagnostic log — no test would pass or fail incorrectly. This is worth a one-line follow-up but does not block the fix, which resolves a real process leak affecting every install test file today.
Keeps the flag's invariant correct if an instance is ever restarted.
|
Updated 9:31 AM PT - Jul 29th, 2026
✅ @robobun, your commit d652713ce2dba55159662963c02e7996149669ab passed in 🧪 To try this PR locally: bunx bun-pr 36352That installs a local version of the PR into your bun-36352 --bun |
There was a problem hiding this comment.
LGTM — the kill(0) → kill() fix is unambiguously correct and the change is entirely test-harness code. The remaining inline nit is about a spurious console.error in a restart sequence no current test exercises; it doesn't affect the fix and can be a follow-up.
Extended reasoning...
Overview
This PR fixes VerdaccioRegistry.stop() in test/harness.ts, which was calling this.process?.kill(0) — a liveness probe that sends no signal — instead of kill(), so the forked verdaccio child leaked past every afterAll(). The fix restores the default SIGTERM, adds a #stopped flag to suppress the resulting exit-log noise on intentional teardown, and adds test/cli/install/verdaccio-registry-lifecycle.test.ts which awaits the child's exit event after stop() and fails on the old code.
Security risks
None. Test-harness only; no runtime, network, auth, or crypto surface touched.
Level of scrutiny
Low. Both changed files live under test/. The functional change is a single well-understood Node child_process semantics fix (kill(0) vs kill()), and the rest is diagnostic-log plumbing plus a regression test. Nothing here affects the shipped binary.
Other factors
- My previous review's nit (reset
#stoppedinstart()) was addressed in e9a722c. This run's inline comment points out that the reset now races the previous child's asyncexitevent in a tightstop(); await start()sequence, so child A's SIGTERM would still log. That is real but nit-level: the only casualty is aconsole.errorin test-harness code, and a grep confirms every current consumer uses a singlebeforeAll(start)/afterAll(stop)lifecycle with no restart. It does not affect the actual process-leak fix. - The new test awaits the actual observable condition (
once(child, 'exit')) raced against a 3s bound rather than sleep-then-check, and has anafterAllSIGKILL safety net so a failure doesn't itself leak a process. - The duplicate-PR bot flagged #34687, which contains the same one-liner inside a much larger refactor; the author acknowledged and this focused fix can land independently.
There was a problem hiding this comment.
LGTM — the kill(0) → kill() fix is correct and the #stopped flag handles the one lifecycle pattern in actual use. The remaining restart-ordering nit (resolved above) is out of scope for this focused fix and only affects a console.error in a codepath nobody exercises.
What was reviewed:
kill()semantics — argless defaults to SIGTERM, restoring pre-#16540 behavior- New test awaits the actual
exitevent with a bounded race and has anafterAllSIGKILL guard so a failure can't leak the child #stoppedflag lifecycle acrossstart()/stop()for the single-use pattern all ~15 consumers follow
Extended reasoning...
Overview
Test-harness-only change: VerdaccioRegistry.stop() in test/harness.ts was calling this.process?.kill(0), which in Node's child_process API only probes liveness without sending a signal. The fix drops the argument so the default SIGTERM is sent. A private #stopped flag suppresses the exit listener's diagnostic log during intentional teardown, and start() resets it. A new test file starts a registry, calls stop(), and asserts the child's exit event fires within 3s.
Security risks
None. Entirely under test/; no production code, no network exposure, no auth/crypto paths.
Level of scrutiny
Low. This is test infrastructure with a well-understood one-line semantic fix (kill(0) vs kill() is textbook Node behavior). The supporting #stopped flag only gates a console.error — worst-case failure mode is a spurious or missing log line, not incorrect test results.
Other factors
I left two inline nits on earlier revisions of this PR, both now resolved: the first (#stopped never reset) was fixed in e9a722c; the second (reset-vs-async-exit ordering on restart) was reasonably declined as out of scope since no consumer restarts a registry instance and the only effect is one extra log line. The new test follows the harness conventions — awaits the event rather than sleeping, uses a bounded Promise.race deadline, and cleans up with SIGKILL in afterAll so a failing assertion can't leak the process. The duplicate-PR check flagged #34687, which the author addressed: that PR bundles the same one-liner inside a larger refactor without a covering test, so this focused fix can land independently.
|
Extra weight for this fix: the leaked verdaccio processes are actively breaking an unrelated test on darwin. One more hardening worth considering while in this code: |
`test/js/bun/http/bun-server.test.ts` has been flaking on darwin lanes since the graceful `stop()` drain work landed (#35130 on Aug 5, #37074 on Aug 6): about 30 failing builds in the 36 hours after the second merge, plus the 90s-timeout reported red in build [90105](https://buildkite.com/bun/bun/builds/90105). Three distinct failure modes, each with a verified cause. This PR is test-only; the code paths the tests pin are unchanged and still covered (see Verification). ## 1. GC churn test answered by a foreign server `request on a connection surviving graceful stop() never reaches a collected handler` failed with ``` FAIL fetch round 11: bad initial response [{"status":200,"body":"\n <!DOCTYPE html>\...//x/-/static/main.55cdd4aea43adabcb109.js\"></script>..."}] ``` That body is a verdaccio web UI page; the `routes` variant got verdaccio's 404 `{"error":"no such package available"}`. A freshly dialed connection to the fixture's own freshly bound port was answered by an npm test registry. Cause, reproduced on a macOS 14 CI host: the ephemeral port allocator on macOS honors only exact-address conflicts. A wildcard `Bun.serve({ port: 0 })` bind gets handed a port that another process already holds at `127.0.0.1` (one collision per ~16,384 binds, i.e. once per wrap of the ephemeral range; a loopback-bound holder under both a real node and a bun process, same result). Connects to `127.0.0.1:port` then reach the more specific foreign listener. The churn fixture performs hundreds of wildcard binds per run, so it finds such a port regularly, and leaked verdaccio processes supply the listeners: `VerdaccioRegistry.stop()` calls `kill(0)`, which is a liveness probe rather than a kill (regressed in #16540; fix open in #36352). Fix: bind the fixture's servers (and decoys) to `127.0.0.1`, the address the parks dial. Verified on the same host: 36,000 loopback-bound port-0 binds against a live loopback holder, zero collisions. This protects the test regardless of what else leaks on the machine; #36352 independently removes the main leak source. ## 2. Parse-window test 90s timeout (the build 90105 red) ``` ✗ server.stop() drain promise counts open connections > a response completing inside another socket's parse window still closes its drained connection [90001.99ms] ^ this test timed out after 90000ms. ``` The fixture wrote a partial request head on connection A and waited 20 `setImmediate` ticks before `stop(false)`. A partial head gives the server nothing observable, and on a loaded darwin host those ticks can elapse before the server has even accepted the socket. Verified on a macOS 14 CI host: when `stop(false)` runs while a handshake-completed connection is still waiting in the accept queue, macOS strands it - never accepted, never counted, never closed, and the client side stays silently open. (Linux keeps the pending accept, which is why this never fired there.) In the fixture that means `/poke` never dispatches, `releaseB` never resolves, B stays parked, and `while (!a.closed || !b.closed)` spins until the test timeout with no other output - exactly the observed signature. Fix: A's poke is now a `POST` with a held body. The handler's dispatch is awaited before `stop()` (so the sweep provably sees a busy connection), and writing the 2-byte body afterwards completes the request. The body's fin chunk is delivered inside A's parse window, so B's completion still runs in that window's microtask drain - the per-socket close-gate property the test exists to pin is exercised exactly as before. ## 3. Mid-request sparing test resolving early Build [89946](https://buildkite.com/bun/bun/builds/89946) hit `error: stop() resolved while a mid-request connection was open` in `a connection mid-request survives stop() until the client closes`. Same root cause: the connection's partial head had not reached the server when `stop()` ran, so the server (correctly) had nothing to count and the drain promise resolved. Fix: the mid-request state is staged as a partial second head on a keep-alive connection that already completed a full request, so the server demonstrably owns the socket. If the sweep still closes it, the head had not arrived and the connection was legitimately idle - that round proves nothing about sparing, so it is voided and retried on a fresh server (bounded, fails loudly if every round races). A resolution while the socket is left open is still reported as the bug it would be. Confirmed-spared rounds run in two variants, and the test requires one success of each: - destroy: the client hangs up and that resolves the drain (the original assertion). - complete: the client finishes the head after `stop()`. The request must still dispatch and be answered, which requires the close-when-idle mark to survive the dispatch's response-state reset (`HTTP_CONNECTION_SCOPED` in uWS `HttpResponseData.h`), and the mark must then close the served connection. This preserves the dispatch-after-stop coverage the old parse-window staging provided incidentally (its partial head completed after `stop()`), which the redesign in section 2 otherwise moves ahead of the stop. ## Verification - `bun bd test test/js/bun/http/bun-server.test.ts`: 74 pass / 3 fail, the 3 failures (`parse source map and fetch small stream`, `rejected promise handled by error method`, `abrubtly close a upload request`) fail identically on an unmodified checkout in this environment. - Drain block run 5x, churn test 2x locally: all pass. - Both redesigned fixtures extracted and run 30x on a macOS 14 CI host under the exact canary from build 90105 (`89d30ad11`): 60/60 pass, plus 40x under a 14-way CPU-spin load: all pass. - Coverage check for the parse-window test: patching the `internalEnd` close gates back to the context-wide `isParsingHttp` bit (the bug #37074 guards against) makes the redesigned test fail by timeout, and restoring the per-socket gate makes it pass. - Coverage check for the mid-request test's complete variant: dropping `HTTP_CLOSE_WHEN_IDLE` from `HTTP_CONNECTION_SCOPED` (so the mark is wiped when the post-stop request dispatches) makes it fail by timeout; restored, it passes. The updated fixture also runs 30/30 on the macOS 14 CI host under the build 90105 canary, and hangs as expected under a pre-#37074 build. The remaining hazard - any long-running wildcard port-0 listener on darwin CI can have its loopback traffic stolen by a later explicit `127.0.0.1` bind such as `VerdaccioRegistry`'s `randomPort()` (range 1024-65535 overlaps the kernel's ephemeral range) - is worth addressing in the harness separately; noted on #36352. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test-only change; deferring to CI. <!-- robobun:evidence:end -->
…37141) `test/js/bun/http/bun-server.test.ts` has been flaking on darwin lanes since the graceful `stop()` drain work landed (oven-sh#35130 on Aug 5, oven-sh#37074 on Aug 6): about 30 failing builds in the 36 hours after the second merge, plus the 90s-timeout reported red in build [90105](https://buildkite.com/bun/bun/builds/90105). Three distinct failure modes, each with a verified cause. This PR is test-only; the code paths the tests pin are unchanged and still covered (see Verification). ## 1. GC churn test answered by a foreign server `request on a connection surviving graceful stop() never reaches a collected handler` failed with ``` FAIL fetch round 11: bad initial response [{"status":200,"body":"\n <!DOCTYPE html>\...//x/-/static/main.55cdd4aea43adabcb109.js\"></script>..."}] ``` That body is a verdaccio web UI page; the `routes` variant got verdaccio's 404 `{"error":"no such package available"}`. A freshly dialed connection to the fixture's own freshly bound port was answered by an npm test registry. Cause, reproduced on a macOS 14 CI host: the ephemeral port allocator on macOS honors only exact-address conflicts. A wildcard `Bun.serve({ port: 0 })` bind gets handed a port that another process already holds at `127.0.0.1` (one collision per ~16,384 binds, i.e. once per wrap of the ephemeral range; a loopback-bound holder under both a real node and a bun process, same result). Connects to `127.0.0.1:port` then reach the more specific foreign listener. The churn fixture performs hundreds of wildcard binds per run, so it finds such a port regularly, and leaked verdaccio processes supply the listeners: `VerdaccioRegistry.stop()` calls `kill(0)`, which is a liveness probe rather than a kill (regressed in oven-sh#16540; fix open in oven-sh#36352). Fix: bind the fixture's servers (and decoys) to `127.0.0.1`, the address the parks dial. Verified on the same host: 36,000 loopback-bound port-0 binds against a live loopback holder, zero collisions. This protects the test regardless of what else leaks on the machine; oven-sh#36352 independently removes the main leak source. ## 2. Parse-window test 90s timeout (the build 90105 red) ``` ✗ server.stop() drain promise counts open connections > a response completing inside another socket's parse window still closes its drained connection [90001.99ms] ^ this test timed out after 90000ms. ``` The fixture wrote a partial request head on connection A and waited 20 `setImmediate` ticks before `stop(false)`. A partial head gives the server nothing observable, and on a loaded darwin host those ticks can elapse before the server has even accepted the socket. Verified on a macOS 14 CI host: when `stop(false)` runs while a handshake-completed connection is still waiting in the accept queue, macOS strands it - never accepted, never counted, never closed, and the client side stays silently open. (Linux keeps the pending accept, which is why this never fired there.) In the fixture that means `/poke` never dispatches, `releaseB` never resolves, B stays parked, and `while (!a.closed || !b.closed)` spins until the test timeout with no other output - exactly the observed signature. Fix: A's poke is now a `POST` with a held body. The handler's dispatch is awaited before `stop()` (so the sweep provably sees a busy connection), and writing the 2-byte body afterwards completes the request. The body's fin chunk is delivered inside A's parse window, so B's completion still runs in that window's microtask drain - the per-socket close-gate property the test exists to pin is exercised exactly as before. ## 3. Mid-request sparing test resolving early Build [89946](https://buildkite.com/bun/bun/builds/89946) hit `error: stop() resolved while a mid-request connection was open` in `a connection mid-request survives stop() until the client closes`. Same root cause: the connection's partial head had not reached the server when `stop()` ran, so the server (correctly) had nothing to count and the drain promise resolved. Fix: the mid-request state is staged as a partial second head on a keep-alive connection that already completed a full request, so the server demonstrably owns the socket. If the sweep still closes it, the head had not arrived and the connection was legitimately idle - that round proves nothing about sparing, so it is voided and retried on a fresh server (bounded, fails loudly if every round races). A resolution while the socket is left open is still reported as the bug it would be. Confirmed-spared rounds run in two variants, and the test requires one success of each: - destroy: the client hangs up and that resolves the drain (the original assertion). - complete: the client finishes the head after `stop()`. The request must still dispatch and be answered, which requires the close-when-idle mark to survive the dispatch's response-state reset (`HTTP_CONNECTION_SCOPED` in uWS `HttpResponseData.h`), and the mark must then close the served connection. This preserves the dispatch-after-stop coverage the old parse-window staging provided incidentally (its partial head completed after `stop()`), which the redesign in section 2 otherwise moves ahead of the stop. ## Verification - `bun bd test test/js/bun/http/bun-server.test.ts`: 74 pass / 3 fail, the 3 failures (`parse source map and fetch small stream`, `rejected promise handled by error method`, `abrubtly close a upload request`) fail identically on an unmodified checkout in this environment. - Drain block run 5x, churn test 2x locally: all pass. - Both redesigned fixtures extracted and run 30x on a macOS 14 CI host under the exact canary from build 90105 (`89d30ad11`): 60/60 pass, plus 40x under a 14-way CPU-spin load: all pass. - Coverage check for the parse-window test: patching the `internalEnd` close gates back to the context-wide `isParsingHttp` bit (the bug oven-sh#37074 guards against) makes the redesigned test fail by timeout, and restoring the per-socket gate makes it pass. - Coverage check for the mid-request test's complete variant: dropping `HTTP_CLOSE_WHEN_IDLE` from `HTTP_CONNECTION_SCOPED` (so the mark is wiped when the post-stop request dispatches) makes it fail by timeout; restored, it passes. The updated fixture also runs 30/30 on the macOS 14 CI host under the build 90105 canary, and hangs as expected under a pre-oven-sh#37074 build. The remaining hazard - any long-running wildcard port-0 listener on darwin CI can have its loopback traffic stolen by a later explicit `127.0.0.1` bind such as `VerdaccioRegistry`'s `randomPort()` (range 1024-65535 overlaps the kernel's ephemeral range) - is worth addressing in the harness separately; noted on oven-sh#36352. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test-only change; deferring to CI. <!-- robobun:evidence:end -->
|
One more data point for this fix, from a long-lived container rather than CI. Still reproduces on current main (8326d1b): Because every file that does For cross-reference: #34687 carries the same |
|
This leak is now breaking main. In build 108576 In both shards three This branch merges cleanly onto main today (a6c4cc2). I rebuilt the same fix on top of current main in robobun/7d7bb0a5/verdaccio-registry-stop (stop() awaits the child's exit, start() rejects if verdaccio exits before it listens, randomPort() takes a kernel-assigned port). I am not opening a second PR for it. #38891 covers the port side from the other direction. |
This branch has not been deployed
1 participant
Problem
VerdaccioRegistry.stop()intest/harness.tscallsthis.process?.kill(0). In Node'schild_processAPI, passing signal0tokill()only probes whether the process exists; it does not send a signal. As a result, the forked verdaccio process survives everyafterAll()teardown and leaks for the rest of the test run.This was introduced in #16540, which changed
kill()tokill(0).Reproduction
Observed in practice as stacks of stale
verdaccioprocesses after running install test files locally.Fix
Call
kill()with no argument so the defaultSIGTERMis sent, restoring the pre-#16540 behavior. Now that the process actually exits on teardown, theexitlistener would logVerdaccio exited with code null and signal SIGTERMfor every test file; a#stoppedflag suppresses that message when the exit was initiated bystop(), keeping normal teardown quiet while still surfacing unexpected mid-test crashes.Verification
New test
test/cli/install/verdaccio-registry-lifecycle.test.tsstarts a registry, callsstop(), and asserts the child'sexitevent fires.Before the fix:
After: passes in ~1s,
psshows no lingering verdaccio.Note: the change is entirely under
test/, so the automated fail-before check (which stashessrc/) cannot distinguish before/after. The fail-before output above was captured by revertingkill()back tokill(0)with the new test in place.[stamp-90s] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file