Conversation
Socket.prototype.unref applied kUserUnrefed to the handle as soon as it was created (initSocketHandle), so a socket unref'd before or during connect() released its hold on the event loop while the connection was still pending and the process exited silently. Node keeps the loop alive until the connect completes because the pending uv_connect_t is an active request; our handle has no request concept, so the unref must wait for the connect event. unref() now defers via a flag-aware connect listener whenever the socket has no handle or is still connecting, which also makes an autoSelectFamily retry handle inherit the unref (the case the initSocketHandle line was added for), and a later ref() correctly cancels a deferred unref. Fixes #37086
|
Warning Review limit reached
Next review available in: 3 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 |
A keep-alive agent calls unref() on its free socket after every request; when the socket still reports connecting, each call added another once(connect) listener and tripped MaxListenersExceededWarning (test-http-client-request-listeners-leak.js). The deferred listener re-reads kUserUnrefed when connect fires, so one pending listener per direction is enough.
|
CI status: the only red is test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js (SIGABRT, JSC !exception() assertion) on the x64-asan lane, which fails identically on the previous run, passes locally under an ASAN debug build of this branch, and is unrelated to this node:net change (reported separately for main triage). fetch-leak.test.ts passed on retry. The diff itself is green: the regression tests plus the node net/tls suites pass locally. |
There was a problem hiding this comment.
No bugs found. The fix is small and the root-cause analysis is convincing, but this changes event-loop ref/unref semantics on net.Socket (deferring handle.unref() past the connecting window and removing the immediate unref from initSocketHandle), which interacts with autoSelectFamily retries, TLS upgrade, and the pause/resume ref bookkeeping — worth a human sanity check.
What was reviewed:
- Traced
unref/refdeferral against allemit("connect")sites andconnectingtransitions; the deferred listener applies to the final handle after autoSelectFamily retries, so removing theinitSocketHandleunref is covered. - Checked
kPendingDeferred*guards prevent duplicateoncelisteners and thatapplyDeferredUnref/applyDeferredRefre-checkkUserUnrefedso ordering (unref→ref before connect, and vice versa) resolves to the last call. - Ruled out: TLS-over-socket (
kupgraded) path never emittingconnectfor the deferred listener — pre-PR code used the sameonce("connect", ...)pattern, so no new gap is introduced.
Extended reasoning...
Overview
Fixes a regression from #36619 where socket.unref() before or during connect() released the event loop hold while the connection was still pending, causing the process to exit silently before the connect callback ran (breaks testcontainers). The change touches only src/js/node/net.ts (~30 lines): Socket.prototype.unref now defers to the connect event when !this._handle || this.connecting, guarded by a new kPendingDeferredUnref flag so at most one listener is registered; ref() gets a symmetric kPendingDeferredRef guard; and the immediate handle.unref?.() in initSocketHandle (added by #36619) is removed. Three regression tests are added covering unref-before-connect, unref-while-connecting, and ref-after-unref ordering.
Security risks
None. This is event-loop-liveness bookkeeping in the Node net compat layer — no parsing of untrusted input, no auth/crypto, no new external surface.
Level of scrutiny
Medium-high. The diff is small and the mechanism is clear (Node keeps the loop alive via the pending uv_connect_t; Bun has no request concept so must defer the unref), but ref/unref correctness in node:net is subtle: getting it wrong produces either process hangs or premature exits, and the file already juggles kUserUnrefed, kPausedUnref, kended-driven re-refs, and autoSelectFamily handle swaps. I traced the new deferral against every emit("connect") site and every connecting = false transition — the deferred listener fires on the final handle after retries, so the removed initSocketHandle line is subsumed. The ref() path intentionally does not add || this.connecting (an immediate socket.ref() while connecting is harmless — new handles are ref'd by default — and kUserUnrefed = false neutralizes any pending deferred unref).
Other factors
- The evidence block shows the new tests fail on the ASAN/debug build without the fix and pass with it; on the release build the loopback connect completed fast enough that the unfixed build also passed, so the regression is timing-dependent — but the fix is not.
- The full node net/tls suite (
node-net.test.ts,node-tls-connect.test.ts, autoSelectFamily tests, etc.) was reported green, and the existingunref survives an autoSelectFamily retrytest still covers the case the removedinitSocketHandleline was originally added for. - The one candidate the bug hunter raised (TLS-over-socket deferring to a
connectevent that never fires) was ruled out: the pre-PR code used the identicalonce("connect", ...)deferral, so no new gap. - Deferring rather than approving because event-loop-liveness changes in
node:netare the kind of thing a maintainer familiar with the #36619 history should eyeball; the change looks correct to me.
|
Thanks for the review. To confirm the two subtle points: the deferred listener fires on whatever handle is current when connect is emitted, so autoSelectFamily retries inherit the unref (the existing "unref survives an autoSelectFamily retry" test covers this and passes under the debug build with this change, where it failed before), and ref() stays immediate while connecting since a fresh handle is ref'd by default and clearing kUserUnrefed neutralizes any pending deferred unref. Ready for a maintainer look; the only CI red is the unrelated worker-terminate assertion noted above. |
|
Data point for the existing-test part of this PR: on current main (8326d1b), a linux-x64 debug (ASAN) build fails With this PR's |
|
Closing in favor of #39856. #39856 fixes this at the native layer. The connect attempt holds the event loop until the socket is established, and a Nothing has landed on main yet, so #37086 stays open until #39856 merges. |
Fixes #37086
Repro
Node (and Bun 1.3.14) prints
CONNECTEDand exits 0. Bun 1.4.0 canary exits 0 silently before the connection completes. This breaks testcontainers: the Ryuk reaper callssocket.unref()before.connect(), so every testcontainers program exits during container startup.Cause
Regression from #36619, which added
if (self[kUserUnrefed]) handle.unref?.()toinitSocketHandleso an autoSelectFamily retry handle inherits a priorunref(). ButinitSocketHandlealso runs whenconnect()creates the initial handle, so a socket unref'd before (or during)connect()released its hold on the event loop while the connection was still pending. Node keeps the loop alive in that window because the pendinguv_connect_tis an active request; Bun's handle has no request concept, so an immediatehandle.unref()lets the process exit.Fix
Socket.prototype.unrefnow defers to theconnectevent whenever the socket has no handle or is still connecting, and the immediate unref ininitSocketHandleis removed. The deferred listener applies to whatever handle is current whenconnectfires, which covers the autoSelectFamily retry inheritance that the removed line was for. The listener re-checkskUserUnrefed(andref()'s deferral re-checks symmetrically) so aref()issued after a pending deferredunref()wins regardless of ordering.This also fixes the existing
unref survives an autoSelectFamily retrytest innode-net.test.tsfailing under the debug build when the refuse-and-retry takes longer than the test's 500ms sentinel timer: previously the process exited mid-retry without ever connecting.Verification
test/regression/issue/37086.test.tscovers unref-before-connect, unref-while-connecting, and ref-after-unref ordering. The first two fail on bun 1.4.0 canary (process exits with no output) and pass with this change. Also ran the node net/tls suites (node-net.test.ts,node-net-server.test.ts,double-connect.test.ts,connect-autoselectfamily-stale-timer.test.ts,socket-reconnect-live.test.ts,node-tls-connect.test.ts,node-tls-server.test.ts,node-tls-upgrade.test.ts,node-net-allowHalfOpen.test.js): no new failures vs main.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file
root cause · written by the author bot
When unref() was called before connect(), the recently added kUserUnrefed flag was recorded immediately and consulted by the connect path as soon as the native handle was created, so the handle was unreffed while the connection was still pending and the event loop had nothing keeping it alive, letting the process exit silently before the connect callback ever ran. Node instead defers a pre-connect unref until the connection is established, so the in-flight connection keeps the process alive. The fix restores that behavior by not applying the user's unref to the handle during connection setu…