Repository navigation
Conversation
… is destroyed
A net.Socket passed to tls.connect({ socket }) whose handshake failed was
left with destroyed=false and never emitted 'close'. The wrapped socket's
only release paths were once('end', kCloseRawConnection) and the _destroy
teardown that was gated on !(upgraded instanceof Socket), so a handshake
that failed via error -> close (no 'end') orphaned the caller's socket.
_destroy now tears down a wrapped net.Socket the same way kCloseRawConnection
does on the graceful path: null the raw half (its fd is released with the
TLS handle) and destroy the JS Socket so its 'close' fires.
|
Reproduced on main (a215285): CI on 0e4cd7d: the diff-touched lanes are green. Remaining red is unrelated: |
|
Updated 12:05 AM PT - Jul 18th, 2026
❌ @robobun, your commit 0e4cd7d has 1 failures in 🧪 To try this PR locally: bunx bun-pr 34507That installs a local version of the PR into your bun-34507 --bun |
WalkthroughChangesThe socket teardown path now safely destroys upgraded TLS socket teardown
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
The wrapped net.Socket's close + destroyed is the regression being tested; checking tlsSock close afterwards added a needless event-ordering dependency.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Nulling upgraded._handle before destroy is only correct for the upgradeTLS path (where _handle is the raw twin of this._handle). For a duplex-wrapped net.Socket (Windows named pipe), _handle is the original live pipe handle and nulling it would orphan that handle. Closing the raw twin via its own _destroy is safe: us_socket_close() guards on is_closed, so the subsequent closeSocketHandle(this) is a no-op on the shared us_socket_t. This also makes the netSock 'close' / tlsSock 'close' ordering match Node's (both via setImmediate, netSock first).
There was a problem hiding this comment.
44ca65f addresses my earlier note — dropping _handle = null and relying on us_socket_close()'s is_closed guard (socket.c:280) is cleaner and keeps the named-pipe path correct. I didn't find anything further, but the raw/tls twin-handle close ordering in _destroy is subtle enough that someone who owns this path should confirm before merge.
What was reviewed:
- Verified
us_internal_socket_close_rawguards onis_closedbefore touching the fd or the loop'sclosed_headlist, so the raw twin's close ahead ofcloseSocketHandle(this)is idempotent at the usockets layer. - Checked
kCloseRawConnection(still nulls_handle) vs. this new error path (does not) — the divergence is benign given the guard, andupgraded.destroyedprevents double-destroy when both paths could run. - Confirmed
onSocketHandleClosedis a no-op, so callinghandle.close()on both halves has no JS-side callback duplication.
Extended reasoning...
Overview
The PR removes the !(upgraded instanceof Socket) guard from Socket.prototype._destroy in src/js/node/net.ts, so a net.Socket wrapped via tls.connect({ socket }) is destroyed (and emits 'close') when the TLS socket is torn down on the error path — previously only generic Duplex streams were torn down here, and real net.Socket instances relied solely on the 'end'-listener path that never fires on a failed handshake. Two new parametrized tests in test/js/node/tls/node-tls-upgrade.test.ts cover the connecting and already-connected cases; both time out on main and pass with the fix.
My earlier review on 18bb2fd flagged that the intermediate revision's upgraded._handle = null would orphan a Windows named-pipe handle on the upgradeDuplexToTLS path. The author responded in 44ca65f by dropping the nulling entirely and instead relying on us_socket_close() idempotency for the upgradeTLSDeferred twin-handle case. That inline thread is resolved.
Security risks
None identified. This is teardown-path lifecycle plumbing; no auth, crypto config, or user-input parsing is touched.
Level of scrutiny
Medium-high. node:net / node:tls socket teardown is a heavily-exercised Node compat path where handle-lifecycle bugs surface as intermittent hangs, double-frees, or leaked fds under load. The runtime change itself is a one-clause guard removal, but its correctness now depends on native-level ordering: netSock._destroy synchronously calls raw.close() (which per us_internal_socket_close_raw can dispatch on_close back into JS) while tlsSock._destroy is still mid-execution and hasn't yet called closeSocketHandle(this) on the tls half. I verified the is_closed guard the author cites, and tlsSock.destroyed is already true so re-entrant destroys short-circuit — but I'd want a maintainer who owns the raw/tls twin-handle pair (upgrade_tls_impl in socket_body.rs) to confirm the raw half's synchronous on_close dispatch doesn't disturb the tls half before its own close runs.
Other factors
- The graceful path (
kCloseRawConnection, net.ts:1691) still nulls_handlebefore destroying; this new error path does not. That divergence is now intentional per the author's reply, and looks safe given the usockets guard, but it does mean two paths now release the wrapped socket via different mechanics. - Evidence block: ASAN debug fails without the fix and passes with it. The "release without fix: all passed" line is a canary binary at a different revision (1498d7b), not a locally-built release from the same checkout, so it doesn't undermine the fails-without-fix claim.
- Vendored Node tests (
test-tls-connect-given-socket,test-tls-destroy-whilst-write, etc.) are reported passing. - Tests use
await using server,once()for every awaited event,port: 0, and assert the specific error code — they meet the harness conventions.
There was a problem hiding this comment.
44ca65f resolves the earlier named-pipe concern — dropping the _handle = null means a duplex-wrapped net.Socket keeps its live pipe handle and closes it in its own _destroy. Beyond the inline nit, I also re-checked the upgradeTLSDeferred double-close question: the raw twin's close() and this socket's closeSocketHandle both land in us_internal_socket_close_raw, which guards on is_closed, so no double-free of the shared us_socket_t.
Extended reasoning...
The author addressed my prior review by removing the _handle = null line in 44ca65f, so both upgrade paths (upgradeTLSDeferred and upgradeDuplexToTLS/named-pipe) now go through the wrapped socket's own _destroy with its handle intact. The remaining inline nit (resetAndDestroy → FIN instead of RST because upgraded.destroy() runs before the resetAndClosing branch) is niche but real; leaving the approve/merge call to a human given this is TLS socket-lifecycle code.
us_socket_close is first-close-wins on the shared us_socket_t, and
SO_LINGER{1,0} is only armed when that first close carries the reset code.
With the teardown block before the resetAndClosing branch, upgraded.destroy()
reached raw.close() first and terminate() became a no-op. Moved the block to
after this._handle is closed so terminate() stays first-close.
Added a resetAndDestroy() case asserting the wrapped socket is still
destroyed.
There was a problem hiding this comment.
No new issues found on 0e4cd7d. Deferring for a human sign-off since Jarred is already engaged and the resetAndDestroy test question (peer ECONNRESET assertion vs. the weaker destroyed-only check) is still open on his end.
What was reviewed:
- The moved
kupgradedblock now runs afterthis._handle's close/terminate, soresetAndDestroy()keepsterminate()as first-close on the sharedus_socket_t. - The
_closeAfterHandlingErrorbranch (queueMicrotask'dcloseSocketHandle) —upgraded.destroy()runs first there, but that path is error-only and never combines withresetAndClosing, so no RST is expected anyway. - Generic-duplex teardown is preserved: the old
!(upgraded instanceof Socket)branch's behavior is subsumed by the new unconditionalupgraded.destroy?.(). - The Windows named-pipe (
upgradeDuplexToTLS) path —upgraded._handleis left intact and its own_destroycloses the pipe.
Extended reasoning...
Overview
The PR changes Socket.prototype._destroy in src/js/node/net.ts so that a wrapped transport (tls.connect({ socket })) is destroyed on every teardown path, not just for generic duplexes. The block moved from before the _handle close to after it (per maintainer feedback) so terminate() remains the first close on the shared us_socket_t and resetAndDestroy() still arms SO_LINGER{1,0}. Three new tests cover failing-handshake teardown (connecting / already-connected) and resetAndDestroy() on a wrapped socket.
Security risks
None. This is JS-side stream lifecycle bookkeeping — no auth, crypto, or untrusted-input parsing is touched. The underlying fd was already being released; the fix is about the JS net.Socket object's destroyed flag and 'close' event.
Level of scrutiny
High. Socket.prototype._destroy is a shared teardown path for every net/tls socket, and this PR has already been through two rounds of subtle ordering bugs (the named-pipe _handle = null orphan and the RST-vs-FIN first-close race), both caught in review. The final shape looks correct, but a maintainer (Jarred-Sumner) is actively reviewing and has an open item.
Other factors
Jarred requested three changes: move the block (done in 0e4cd7d), add a peer-observes-ECONNRESET test (author explained it isn't observable on loopback because us_internal_ssl_close queues an alert before the RST fires, and Node throws ERR_INVALID_HANDLE_TYPE for the same call — a weaker destroyed-only test was added instead), and drop the ordering claim from the PR body (done). Whether the weaker test is acceptable is Jarred's call, so I'm not auto-approving over an in-progress human review.
|
Superseded by #38028, which moves the wrapped-socket teardown into |
…t({ socket })
Adds the two teardown paths of a wrap over an already connected socket
that the describe block did not exercise: destroy() right after wrapping
(the adopted fd has to be closed and the plain socket released, the case
from #34507) and a graceful end() after the handshake, whose release used
to come from the removed 'end' listener. Both assert node's event order.
|
Closing in favor of #38028. Verified on its branch: the three tests from this PR and the reproduction above pass there (the wrapped socket emits |
|
Heads up: #38232 makes |
Reproduction
Node v26.3.0 and pre-rewrite Bun:
Bun on main (a215285):
The kernel fd is released (no fd leak), but the JS
net.Socketis stranded withdestroyed === falseand its'close'handlers never run. Thesocket:option exists for STARTTLS-style upgrades where callers own that socket and key their cleanup on its'close', so every failed upgrade leaked one zombie Socket.Cause
After
tls.connect({ socket: ns }), the wrappednshas exactly two release paths innet.ts:this.once("end", this[kCloseRawConnection]): only fires on a graceful close; a handshake failure iserror->closewith noend.Socket.prototype._destroy'sthis[kupgraded]teardown: gated on!(upgraded instanceof Socket), so it only ever tore down generic duplexes.Neither fires on the error path for a real
net.Socket, so it is never destroyed.Fix
_destroydrops the!(upgraded instanceof Socket)guard and callsupgraded.destroy?.()for any wrapped transport. The teardown runs afterthis._handleis closed soresetAndDestroy()'sterminate()stays the first close on the sharedus_socket_t(us_socket_closeis first-close-wins, socket.c:280); the raw twin's ownclose()then no-ops onis_closed. On theupgradeDuplexToTLSpath (Windows named pipe)upgraded._handleowns its own fd and its_destroycloses it.This also covers
tlsSock.destroy()mid-stream and the server-sidenew TLSSocket(socket, { isServer: true })wrap.Verification
The failing-handshake tests time out without the fix (waiting on the wrapped socket's
'close'). Vendored Node teststest-tls-connect-given-socket,test-tls-destroy-whilst-write,test-tls-inception,test-tls-on-empty-socket,test-tls-net-socket-keepalive,test-tls-js-streamall pass.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file