Repository navigation
Conversation
A TLSSocket layered over an existing socket (new TLSSocket(socket,
{ isServer: true }), tls.connect({ socket }), tls.Server's
emit('connection') wrap) only released the wrapped net.Socket from its
'end' event, and Socket.prototype._destroy skipped net.Socket transports.
A handshake failure destroys the TLS socket without emitting 'end', so the
wrapped socket never emitted 'close', an accepted one stayed in its
net.Server's connection count and server.close(cb) never called back.
Tear the wrapped transport down from _destroy instead, the way node's
TLSWrap close() destroys its parent wrap. A net.Socket whose handle is the
raw twin of the TLS handle shares the fd that the TLS handle's close tears
down, so it is detached before being destroyed; any other transport still
owns its fd and is destroyed outright, which also covers a wrap destroyed
before the deferred adoption ran and wraps running on the stream-level
engine.
Contributor
|
Warning Review limit reached
Next review available in: 39 seconds 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 (3)
Comment |
Collaborator
Author
|
Updated 4:05 PM PT - Aug 13th, 2026
❌ @robobun, your commit 585f684 has some failures in 🧪 To try this PR locally: bunx bun-pr 38028That installs a local version of the PR into your bun-38028 --bun |
Collaborator
Author
|
Reproduced on main (f426a8e): a standalone Fix: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Because it changes Socket.prototype._destroy teardown semantics for every TLS wrap (both the graceful path — the removed 'end'-time kCloseRawConnection — and the error path) and relies on the shared-fd twin ownership rules around kAdoptedTLSRaw, a human familiar with the net/tls lifecycle should sign off.
What was reviewed:
destroyUpgradedTransport— thekAdoptedTLSRawdetach-before-destroy so the twin's fd isn't double-closed, and that non-adopted transports (duplex / not-yet-adopted socket) still close their own fd.- The removed
kCloseRawConnectionhandler'sunref()/connecting = false— both are covered by the wrapped socket's own_destroyoncedestroy()runs. - Test coverage: handshake failure, destroy-before-adoption, stream-level engine,
emit('connection')rejection, and the client-sidetls.connect({ socket })variants — each awaits the actual'close'and assertsgetConnections() === 0.
Extended reasoning...
Overview
The PR replaces the 'end'-only release of a TLS wrap's underlying net.Socket with an unconditional destroyUpgradedTransport(this) call in Socket.prototype._destroy. It removes the kCloseRawConnection symbol/method and the three this.once('end', ...) registrations at the adoption sites (client connect over connected socket, client connect over connecting socket, server-side bunUpgradeServerTLS). The old _destroy branch that only destroyed non-Socket upgraded transports is generalized to destroy any kupgraded transport, detaching _handle first when it is the kAdoptedTLSRaw twin so only the wrapped socket's _destroy bookkeeping runs (no double fd close). Seven new tests cover server-side and client-side wraps across handshake failure, destroy-before-adoption, stream-level engine, and STARTTLS-via-emit('connection') rejection.
Security risks
None identified. This is teardown/lifecycle bookkeeping; it does not change verification, credential handling, or any security gate. The change makes cleanup more thorough (closing an fd that was previously leaked on handshake failure), which is defensively better.
Level of scrutiny
High. Socket.prototype._destroy runs for every socket teardown, and this change alters the behavior for every TLS-wrapped socket — including the graceful-shutdown path, which previously ran kCloseRawConnection from 'end' and now runs destroyUpgradedTransport from _destroy. The kAdoptedTLSRaw shared-fd semantics (nulling _handle before destroy() so the twin is not double-closed while the TLS handle's own close tears down the fd) are subtle and depend on native-layer ownership rules that a maintainer should confirm. The PR description is exceptionally thorough (probe output, node reference, wide test-suite runs), and the bug-hunting pass found nothing, but this is core networking lifecycle code with observable ordering effects (raw 'close' now fires before the TLS socket's on the graceful path too).
Other factors
- The removed
kCloseRawConnectionalso didconnection.unref()andconnection.connecting = false. Both are covered by the wrapped socket's own_destroy(it setsthis.connecting = falseand closing/nulling the handle drops the ref), so no behavioral loss there. - Tests are well-structured: they await concrete conditions (
'close'events,getConnections()), assert exact error codes, cover the variant matrix, and clean up infinally. - No prior human review on this PR; CI is still building.
Run the wrapped transport's teardown from a nextTick queued after
_destroy's callback instead of at the top of _destroy, so the TLS
socket's 'error' (and a tls.Server's 'tlsClientError') still observe the
wrapped socket intact and its 'close' lands between the TLS socket's
'error' and 'close', which is node's order. A handle-less TLS socket
queues its own 'close' behind the transport's for the same reason.
Record kupgraded before the client upgrade branches on whether the
socket is still connecting, so destroying tls.connect({ socket }) while
the socket is connecting destroys it as well instead of leaving it to
connect (and, for a failed connect, to raise an unhandled error).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I reviewed this PR and didn't find correctness bugs. Because it reworks the TLS-over-net.Socket teardown path in Socket.prototype._destroy — replacing the 'end'-keyed release with an unconditional one, re-sequencing the wrapped socket's 'close' between the wrap's 'error' and 'close', and detaching the shared-fd raw twin before destroying it — a human look at the lifecycle and event-ordering changes is worthwhile.
What was reviewed
- The
kAdoptedTLSRawdetach-then-destroy: nulling_handlebeforedestroy()so the twin's fd isn't double-closed while_destroybookkeeping (server_connections--,'close') still runs. - Moving
this[kupgraded] = connectionahead of the deferred'connect'upgrade so a same-tickdestroy()still tears the connection down; the removedconnection.destroy()in that callback is now covered by the_destroypath. - The
nextTickordering in_destroy(transport destroy queued aftercallback(err), wrap'close'after the transport's) against Node'sTLSWrap.prototype.closeparent-wrap semantics. - The 7 new tests: each awaits an observable event log rather than sleeping, wires failure paths, and asserts exact event sequences; two intentionally sort where ordering isn't part of the contract.
Extended reasoning...
Overview
This PR changes how a TLS socket layered over an existing net.Socket (tls.connect({ socket }), new tls.TLSSocket(raw, { isServer: true }), tlsServer.emit('connection', raw)) tears down that wrapped socket. Previously the wrapped socket was released only from the wrap's 'end' listener (kCloseRawConnection), and _destroy only tore down non-Socket duplex transports. A handshake failure destroys the wrap without emitting 'end', so the wrapped socket leaked (never emitted 'close', stayed in the accepting server's connection count, blocked server.close(cb)). The fix moves the release into Socket.prototype._destroy unconditionally via two new helpers (destroyUpgradedTransportNT, destroyUpgradedTransportAndCloseNT), records kupgraded before the deferred upgrade runs, and removes the now-redundant 'end' listeners and per-branch kupgraded assignments. Seven new tests cover server-side wraps (handshake failure, emit('connection') rejection, destroy-before-adoption, handle-less stream engine) and client-side tls.connect({ socket }) (non-TLS peer, cert-verify failure, destroy-while-connecting).
Security risks
None introduced. The change is teardown-only: it makes the wrapped transport go down with the TLS socket, matching Node. It does not weaken any verification, does not touch auth/crypto decisions, and the kAdoptedTLSRaw detach avoids a double-close on the shared fd rather than opening a new path.
Level of scrutiny
High. src/js/node/net.ts is the core socket implementation shared by every net.Socket and tls.TLSSocket; _destroy runs on every socket teardown. The change alters event ordering (queuing the transport destroy on nextTick after callback(err) so the wrap's 'error'/'tlsClientError' still see the transport intact, then the transport's 'close', then the wrap's) and replaces an existing mechanism wholesale. The shared-fd twin handling (null _handle before destroy()) is subtle and load-bearing. Regressions here would surface as hangs, double-closes, or reordered events across the entire TLS-wrap surface, so this is not a change I'd approve without a maintainer's eyes.
Other factors
- The comment-cop bot has flagged five multi-line explanatory comments in the diff; those are already visible on the PR and I'm not restating them here.
- The PR description reports the vendored
test-tls-*/test-net-*/proxy suites were run with only pre-existing failures, and the new tests are stated to fail on the unfixed build — good signal, but CI (#94445) was still building at the time of the timeline snapshot. - The candidate issue about the multi-line comment style was raised and ruled out by verifiers as not a functional bug (it's a style flag the bot already surfaced).
Contributor
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks Socket.prototype._destroy teardown ordering for every TLS wrap flavor (adopted-fd twin, still-connecting, duplex-engine, handle-less) and replaces the shipped 'end'-listener release with a nextTick-deferred path, a human look would still be worthwhile.
Reviewed: the kupgraded gate keeps the new nextTick to wraps only; the kAdoptedTLSRaw detach before destroy() correctly skips when the twin was never installed (destroy-before-adoption); kCloseRawConnection has no remaining references. Checked that removing the synchronous non-Socket upgraded.destroy?.() at the top of _destroy is subsumed by destroyUpgradedTransportNT (generic duplexes fall through the kAdoptedTLSRaw guard to upgraded.destroy()). Two comment-cop lints (lines 255, 2001) are still open on the current revision.
Extended reasoning...
Overview
Reworks how a TLS wrap tears down the net.Socket (or generic duplex) it was layered over. Socket.prototype._destroy now unconditionally destroys this[kupgraded] on a nextTick queued after the destroy callback (so the wrap's 'error' fires with the transport still intact), replacing the previous 'end'-listener release (kCloseRawConnection, deleted) and the synchronous !(upgraded instanceof Socket) early-destroy. When the wrapped socket's handle is the shared-fd raw twin (kAdoptedTLSRaw), it is detached before destroy() so only bookkeeping runs. tls.connect({ socket }) records kupgraded before waiting for 'connect', so a destroy while connecting also tears the socket down. Eight new tests assert ordered event logs against node's behavior across handshake failure, cert rejection via emit('connection'), destroy-before-adoption, handle-less duplex engine, and the client-side variants.
Security risks
None identified. This is fd/handle lifecycle and event ordering on the teardown path, not TLS verification, credential handling, or crypto. The change destroys the wrapped transport more eagerly (fixing a leak), not less.
Level of scrutiny
High. Socket.prototype._destroy runs for every net.Socket teardown; the change is gated on kupgraded, but it also removes an existing synchronous destroy of wrapped generic duplexes and defers it to nextTick, which is a timing change to a shipped path. The correctness depends on subtle event-loop-phase reasoning (nextTick after callback(err) vs. the handle close's setImmediate vs. _closeAfterHandlingError's microtask) across four wrap flavors. The PR description and tests are thorough (vendored test-tls-*/test-net-*/proxy suites reported clean), but this is exactly the kind of ordering-sensitive core change a maintainer familiar with the TLS wrap and shared-fd twin machinery should validate.
Other factors
- Two unresolved comment-cop lints on the current revision (net.ts:255 — the 5-line
destroyUpgradedTransportNTcomment; net.ts:2001 — the 2-linekupgradedhoist comment). The author already shortened once in 50c0b69; the remaining ones document real ordering invariants and may be acceptable, but they're technically open. - The tests intentionally do not pin event order for the not-yet-adopted and still-connecting cases (they
.sort()the log), which the PR description acknowledges as a known divergence from node's order for those two flavors. - No prior review from me on this PR.
…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.
server.listen(0, "localhost") binds whichever family localhost resolves to first, while the client resolved it independently and could pick the other one (ECONNREFUSED on hosts where that order differs). Connect to server.address() and request the bind hostname via servername instead.
Contributor
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewires Socket.prototype._destroy — a path every socket passes through — and changes the wrapped-transport teardown timing that TLS wraps depend on, a human look would still be worthwhile.
Checked: the removed kCloseRawConnection 'end' listener and the !(upgraded instanceof Socket) generic-duplex teardown are both subsumed by the new destroyUpgradedTransportNT path (graceful shutdown still reaches _destroy, and the new path handles duplexes too). Verified the kAdoptedTLSRaw detach avoids a double close of the shared fd, and that hoisting this[kupgraded] = connection before the still-connecting branch is what makes the destroy-while-connecting case release the socket. The nextTick placement after callback(err) correctly orders the wrap's 'error' before the transport's teardown.
Extended reasoning...
Overview
The PR fixes a resource leak in the node:tls wrap-over-existing-socket paths: when a TLS socket layered over a net.Socket (server-side new TLSSocket(raw, {isServer: true}), tlsServer.emit('connection', raw), or client-side tls.connect({ socket })) is destroyed by a handshake failure, the wrapped net.Socket was left alive — never emitting 'close', staying in the accepting server's connection count, and blocking server.close(cb). The fix moves the transport teardown from an 'end'-event listener (which never fires on error) into Socket.prototype._destroy, deferred one tick so the TLS socket's 'error' still observes the transport intact. It also hoists the kupgraded assignment in tls.connect({ socket }) above the connecting/connected branch so a wrap destroyed before the underlying connect completes still tears down the socket.
Files touched: src/js/node/net.ts (the fix, ~40 lines net), plus ~280 lines of new tests across test/js/node/tls/node-tls-server.test.ts and test/js/node/tls/node-tls-upgrade.test.ts, and a small unrelated flake fix in an existing SNI test (connect to the bound address instead of re-resolving "localhost").
Security risks
None identified. The change tears down more resources than before, not fewer; there's no new trust decision, credential handling, or input parsing. The wrapped socket is destroyed without an error, matching node's _parentWrap.destroy().
Level of scrutiny
High. Socket.prototype._destroy is on the teardown path of every net.Socket in the runtime, and the change alters observable event ordering for TLS wraps (the transport's 'close' now lands between the wrap's 'error' and its 'close'). The PR description shows this ordering was arrived at deliberately — a first revision that ran the teardown synchronously at the top of _destroy broke the raw.destroyed=false-inside-'error' invariant — but that same care is why a maintainer should confirm the tick placement and the kAdoptedTLSRaw detach interact correctly with the native handle-close paths across all platforms and the _closeAfterHandlingError arm.
Other factors
The test coverage is thorough (10 new cases: server + client, error + graceful, adopted + unadopted, connected + still-connecting), each asserting the full ordered event log rather than a single boolean, and the description documents that 8 of them fail without the net.ts change. The vendored test-tls-* / test-net-* / proxy suites were re-run. The comment-cop feedback on comment length was addressed in 50c0b69 and all threads are resolved. No human has reviewed yet. Given the reach of _destroy and the subtlety of the shared-fd twin detach, this is a defer-to-human rather than an auto-approve.
Collaborator
Author
|
Pushed c47dff8 while closing #34507 as superseded by this PR: two more cases in the One observation from comparing the block's expectations with node v26.3.0, same Node destroys the parent socket at the moment the TLS handle is closed: synchronously inside |
Member
|
@robobun please run the tests this PR adds or changes on Node.js v26.3.0 and report whether each one passes or fails. |
This was referenced Sep 11, 2026
alii
added a commit
that referenced
this pull request
Sep 24, 2026
…cket it wraps (#42487) ### Problem - `new tls.TLSSocket(socket, { isServer: true })` and `tls.connect({ socket })` wrap a connected `net.Socket`. When the connection closes, the wrapped socket reports events that belong to the TLS socket: `'end'` and `'finish'` on a close, `'error'` on a peer reset. Node emits only `'close'` on it. - Its `'close'` then destroys the TLS socket early. After `raw.end()`, `finished(tlsSocket)` reports `ERR_STREAM_PREMATURE_CLOSE`. Regression from #39066 and #42265. - Cause: the native close calls the wrapped socket's close handler first (`ServerHandlers.close`, `src/js/node/net.ts:1052`). It reports its own EOF or read error and closes. Its `'close'` runs `onUpgradedClose` (`net.ts:413`), which destroys the TLS socket. ### Fix - A wrapped socket's close handler reports nothing (`closeWithTLSSocket`). The TLS socket's close handler runs next and reports the EOF or the error. - The TLS socket closes the wrapped socket at `'end'`, or from `_destroy` after it queued its `'error'`. `destroy(err)` gives node's order: `tls error`, `raw close`, `tls close`. - Node does the same: `TLSWrap` owns the reads ([wrap.js#L723-L727](https://github.com/nodejs/node/blob/v26.3.0/lib/internal/tls/wrap.js#L723-L727)) and `TLSWrap.close()` destroys the wrapped socket ([wrap.js#L676-L688](https://github.com/nodejs/node/blob/v26.3.0/lib/internal/tls/wrap.js#L676-L688)). - Verified: two `node:test` files, `node-tls-wrapped-socket-close.test.ts` (14 cells) and `node-tls-raw-end.test.ts` (4 tests, by @alii). Node v26.3.0 passes all 18. Bun 1.4.2 fails 11. Main's `src/` fails all 18. This branch passes all. ### Background - The TLS socket adopts the fd. The wrapped socket keeps a raw handle (`kAdoptedTLSRaw`). - Only the TLS handle gets native events. Its `on_close` (`src/runtime/socket/socket_body.rs`) calls the raw handle's close handler, then the TLS socket's. - A `close_notify` reaches only the TLS socket. These closes have none: after the TLS socket's own FIN, a peer reset, `destroy()` by the owner. <details><summary>Notes</summary> **Scope.** #39066 made the `'close'` of the wrapped socket destroy the TLS socket. #42265 emptied the buffer of the wrapped socket, so its `'end'` is no longer held back by unread TLS bytes. Two PRs landed on main after this one opened and repaired parts of the damage. #42293 removed the tick drain between the two close handlers, so the TLS socket gets its `'end'` and its reset error again. #42176 keeps unread data when the wrapped socket closes first. What is left on main: the wrapped socket still reports events of its own, a reset is still reported on it first with `hadError=true`, and `finished(tlsSocket)` fails after `raw.end()`. None of these PRs is in a release. **Test matrix.** Both files use `node:test` and `node:assert` only. Under bun, the last test of `node-tls-wrapped-socket-close.test.ts` runs the same file in Node.js. | | `node-tls-wrapped-socket-close.test.ts` (14 cells) | `node-tls-raw-end.test.ts` (4 tests) | |---|---|---| | Node.js v26.3.0, `node --test` | 14 pass (25 of 25 runs) | 4 pass | | Bun 1.4.2, `bun test` | 9 fail, 5 pass | 2 fail, 2 pass | | main b2ad29d, debug build of its `src/` | 14 fail | 4 fail | | this branch, debug + ASAN | 14 pass | 4 pass | The 5 cells that pass on Bun 1.4.2 are the `end()` cells with a TLS peer or after the handshake, and the unread-data cell. They broke on main after 1.4.2 (#39066, #42265). The other 9 were never right: 1.4.2 reports a reset on the wrapped socket, emits `raw end` and `raw finish` on `destroy(err)`, and in 4 cells never closes one of the sockets (those time out). **Traces.** Events of the two sockets on the observed side, in order. This branch prints node's trace in every cell. | cell | node v26.3.0 and this branch | main b2ad29d | |---|---|---| | server wrap, `end()` one tick or one `setImmediate` after the wrap, TLS or plain peer (4 cells) | `tls finish, tls end, raw close, tls close` | `tls finish, raw end, tls end, raw close, tls close` | | server wrap, `end()` after the handshake, peer answers `'end'` with `destroy()` | same as above | same as above | | `tls.connect({ socket })`, `end()` after the handshake, peer answers `'end'` with `destroy()` | same as above | same as above | | server wrap, peer answers `'end'` with 4 bytes then `destroy()`, nothing reads until the connection closed | `tls finish, (closed, unread=4), tls data late, tls end, raw close, tls close` | `tls finish, raw end, raw finish, raw close, (closed, unread=4), tls data late, tls end, tls close` | | server wrap, peer reset before or after the handshake (2 cells) | `tls error ECONNRESET, raw close, tls close hadError=true` | `raw error ECONNRESET, raw close hadError=true, tls error ECONNRESET, tls close hadError=true` | | `tls.connect({ socket })`, peer reset after the handshake | same as above | `raw error ECONNRESET, tls error ECONNRESET, tls close hadError=true, raw close hadError=true` | | `tlsServer.emit('connection', socket)`, peer reset before the handshake | `tlsClientError ECONNRESET, raw close, tls close hadError=true` | `raw error ECONNRESET, raw close hadError=true, tlsClientError ECONNRESET, tls close hadError=true` | | server wrap, owner calls `tlsSocket.destroy(err)` before or after the handshake (2 cells) | `tls error, raw close, tls close hadError=true` | `raw end, tls error, raw finish, raw close, tls close hadError=true` | | `tls.connect({ socket })`, owner calls `tlsSocket.destroy(err)` after the handshake | same as above | `raw end, tls error, raw finish, tls close hadError=true, raw close` | | server wrap, owner calls `raw.end()` (`node-tls-raw-end.test.ts`) | node: `raw finish, tls end, tls finish, finished ok, raw close, tls close`. This branch: `raw finish, tls end, raw close, tls finish, finished ok, tls close` | `raw finish, raw end, tls end, raw close, tls close, finished ERR_STREAM_PREMATURE_CLOSE` | In the last row this branch still closes the wrapped socket at the `'end'` of the TLS socket, so `raw close` comes before `tls finish`. Node closes it when the TLS socket is destroyed. That order is the same on main and is listed under "Not changed". **The order for `destroy(err)`.** The first version of this PR closed the wrapped socket from inside the native close, which runs inside `_destroy` before `_destroy` queues the `'error'`. The events were `raw close, tls error, tls close`. Now the close handler of the wrapped socket only records that the TLS socket owes the close (`kOwesRawClose`). `_destroy` pays it after `callback(err)`. A `_destroy` that deferred the close of its handle (a handshake failure closes it from a microtask) has returned by then, so the next tick pays it. In both cases the `'error'` is already queued. **Reset matrix.** Measured on the first commit against main b993710, before #42293. A matrix of 24 cells of resets. Three shapes: server wrap, socket injected into a `tls.Server`, client wrap. Two phases: before and after the handshake. Four placements of the `'error'` listener: on the wrapped socket, on the TLS socket, on both, on neither. The peer is always a node process. This branch printed node's trace in 20 cells. The 4 cells that differ are client wraps whose TLS socket has no `'error'` listener: node throws the ECONNRESET as an uncaught exception. Bun closes a socket with no `'error'` listener without an error, on every kind of socket. This PR does not change that. **Related PRs.** - #42293 (merged) made the TLS socket report a peer reset. Its two tests pass on this branch. - #42176 (merged) changed `onUpgradedClose` so that unread data survives. This PR does not touch that function. With this PR the wrapped socket no longer closes first in these cells, so that path is not reached. - #38028 moves the teardown of the wrapped socket into the TLS socket's `_destroy` for every wrap. It has conflicts with main. The `_destroy` call here covers only a wrapped socket whose fd has already closed. - #36534 changes the native upgrade so that the raw handle gets no JS dispatch. It has conflicts with main. If it lands, `closeWithTLSSocket` can go. The cells still apply to it, because they assert node's events only. **Not changed, same as main.** - `'end'` after a plain `destroy()`: bun emits one on plain `net` and `tls` sockets too. The fixture of #42181 documents it. A TLS socket that its owner destroys with no error still shows `tls end`. The wrapped socket no longer shows `raw end`. - A wrapped socket that its owner destroys directly (`raw.destroy()`): `tls end, tls finish, tls close, raw close`. Node: `raw close, tls close`. - When the TLS socket gets its `'end'` while the fd is still open (a peer that closes first with a `close_notify`, or `raw.end()`), `raw close` comes before `tls finish`. Node has `tls finish` first. - `destroy()` in the same tick as the wrap, before the fd is adopted, does not close the wrapped socket. That is #38028. - `end()` in the same tick as the wrap sends no FIN. That is #42339. **Platforms.** The first commit (11 cells, as a fixture spawned on bun and on node) was also built and run on Windows x64: a canary of main failed the 11 bun cells, the branch passed all. The `destroy(err)` cells and the `node:test` files were run on Linux x64 only. **Other suites on this branch.** All of `test/js/node/tls/`, `node-http2-upgrade.test.mts`, `socket-retention.test.ts`, and 508 vendored `test-tls-*`, `test-https-*`, `test-http2-*` files: 506 pass. The two others also fail on main: `test-https-timeout.js` (debug build only) and `test-tls-client-allow-partial-trust-chain.js` (needs the test runner). Also on main in this container: `node-tls-server.test.ts` "SNICallback runs even when the requested servername matches the bind hostname" (`localhost` resolves to `::1` first). </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 3 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 14 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-upgrade.test.ts bun test v1.4.3 (09bb546) test/js/node/tls/node-tls-upgrade.test.ts: (pass) should be able to upgrade a paused socket and also have backpressure on it #15438 [1654.06ms] (pass) tls.connect({ socket }) over a net.Socket with readable: false keeps the TLS bytes off the wrapped socket [334.63ms] (pass) tls.connect({ socket }) over a net.Socket with an onread buffer keeps the TLS bytes off the wrapped socket [113.54ms] (pass) tls.connect({ socket }) over a net.Socket with no reader keeps the TLS bytes off the wrapped socket [59.53ms] (pass) a STARTTLS exchange hands no TLS bytes to the 'data' listeners of the wrapped sockets (#32239) [164.21ms] 182 | ["net", "process.nextTick"], 183 | ["net", "setImmediate"], 184 | ])( 185 | "new TLSSocket(socket, { isServer }) end()s before the handshake completes, %s peer, from %s", 186 | async (peer, when) => { 187 | expect(await run("end-before-handshake", peer, when)).toEqual(eof); ... (truncated) release without fix: 14 FAILED bun test v1.4.3-canary.1 (09bb546) test/js/node/tls/node-tls-upgrade.test.ts: (pass) should be able to upgrade a paused socket and also have backpressure on it #15438 [55.44ms] (pass) tls.connect({ socket }) over a net.Socket with readable: false keeps the TLS bytes off the wrapped socket [6.04ms] (pass) tls.connect({ socket }) over a net.Socket with an onread buffer keeps the TLS bytes off the wrapped socket [2.90ms] (pass) tls.connect({ socket }) over a net.Socket with no reader keeps the TLS bytes off the wrapped socket [2.57ms] (pass) a STARTTLS exchange hands no TLS bytes to the 'data' listeners of the wrapped sockets (#32239) [3.81ms] 182 | ["net", "process.nextTick"], 183 | ["net", "setImmediate"], 184 | ])( 185 | "new TLSSocket(socket, { isServer }) end()s before the handshake completes, %s peer, from %s", 186 | async (peer, when) => { 187 | expect(await run("end-before-handshake", peer, when)).toEqual(eof); ^ error: expect(received).toEqual(expected) [ "tls finish", - "tls end", + "raw end", + "raw finish", "raw close hadError=false", "tls close ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-upgrade.test.ts bun test v1.4.3 (09bb546) test/js/node/tls/node-tls-upgrade.test.ts: (pass) should be able to upgrade a paused socket and also have backpressure on it #15438 [1471.02ms] (pass) tls.connect({ socket }) over a net.Socket with readable: false keeps the TLS bytes off the wrapped socket [236.85ms] (pass) tls.connect({ socket }) over a net.Socket with an onread buffer keeps the TLS bytes off the wrapped socket [84.87ms] (pass) tls.connect({ socket }) over a net.Socket with no reader keeps the TLS bytes off the wrapped socket [75.39ms] (pass) a STARTTLS exchange hands no TLS bytes to the 'data' listeners of the wrapped sockets (#32239) [188.67ms] (pass) the close of a connection under a TLS socket and the net.Socket it wraps (bun) > new TLSSocket(socket, { isServer }) end()s before the handshake completes, tls peer, from setImmediate [2170.33ms] (pass) the close of a connection under a TLS socket and the net.Socket it wraps (bun) > new TLSSocket(socket, { isServer }) end()s before the handshake comp ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 1157ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/125] gen JS modules (bundle-modules) Preprocess modules (8646ms) Bundle modules (67ms) Postprocesss modules (24ms) Bundle Functions (433ms) Generate Code (28ms) [9.20s] Bundled "src/js" for production 2600 kb 197 internal modules 13 native modules 50 internal functions across 16 files [1/8] cargo bun_runtime → libbun_runtime.a ^[[1m^[[92m Compiling^[[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) ^[[1m^[[92m Compiling^[[0m bun_errno v0.0.0 (/workspace/bun/src/errno) ^[[1m^[[92m Compiling^[[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) ^[[1m^[[92m Compiling^[[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) ^[[1m^[[92m Compiling^[[0m bun_safety v0.0.0 (/workspace/bun/src/safety) ^[[1m^[[92m Compiling^[[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) ^[[1m^[[92m Compiling^[[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) ^[[1m^[[92m Compiling^[[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) ^[[1m^[[92m Compiling^[[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) ^[[1m^[[92m Compi ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/node/net.ts | 37 +++- test/js/node/tls/node-tls-upgrade.test.ts | 100 ++++++++++- .../node/tls/tls-wrapped-socket-close-fixture.mjs | 193 +++++++++++++++++++++ 3 files changed, 321 insertions(+), 9 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/node/net.ts 19 21 51 test/js/node/tls/node-tls-upgrade.test.ts 9 6 45 test/js/node/tls/tls-wrapped-socket-close-fixture.mjs 4 6 52 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Alistair Smith <hi@alistair.sh>
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 1, 2026
…ists (#44196) A TLS socket over a `stream.Duplex` (`tls.connect({ socket })`, `new TLSSocket(duplex)`) has no fd. Bun runs a TLS engine over the stream, and a queued task starts that engine after the wrap. "Before the engine" below is the rest of the script, `process.nextTick`, microtasks and promise jobs. The lost EOF needs the close hook of #39066. The freed listener functions need the `pending_close` branch of #42176. No release has either. ### What the TLS socket emits | The transport, before the engine | Node v26.10 | main | This PR | |---|---|---|---| | Client: EOF, close | `end`, ECONNRESET, `close` true | `close` false | as Node | | Server wrap: EOF, close | `end`, `close` false | `close` false | as Node | | Client: bytes, EOF, close | `end`, ECONNRESET, `close` true | `close` false | as Node | | Server wrap: bytes, EOF, close | `end`, `close` false | `close` false | as Node | | Client: non-TLS bytes, EOF | ERR_SSL_HTTP_REQUEST, `close` true | `secureConnect`, `end`, `close` false | `end`, ECONNRESET, `close` true | | Server wrap: a complete ClientHello, EOF | `end`, 1 write | `end`, 1 write | as Node | | Server wrap: a complete ClientHello, EOF, close | `close` false | `close` false | `end`, `close` false | Last row: Node writes its answer to the ended transport, and that failure closes the socket ahead of `end`. The same rows after the engine started do not change. ### What happens to the transport | Case | Node v26.10 | main | This PR | |---|---|---|---| | `tls.connect({ socket: netSocket }).destroy()` in the same tick | raw socket closed, peer gets a FIN | raw socket open, no FIN | as Node | | The socket or the transport is destroyed after the engine started | no `end()`, `writableEnded` false | `end()` on the destroyed transport, `writableEnded` true | as Node | | 16 wraps closed before the engine, a GC, then 8 new wraps | | 7 of 8 never reach `secureConnect` | 8 of 8 answer | Last row: the close left the four listener functions of the context set. The GC freed them, and the task that frees the context then wrote into the freed cells. ### What the transport throws | Case | Node v26.10 | main | This PR | |---|---|---|---| | The `write` getter throws (the ClientHello) | `uncaughtException` | the exception stays pending on the VM (debug build: panic) | `error` on the socket, `close` true | | The `end` getter throws, close after the engine started | `end` is not read | the exception stays pending on the VM (debug build: assertion) | `uncaughtException` | | `end()` throws, close after the engine started | `end()` is not called | dropped | `uncaughtException` | | `end()` or its getter throws, close before the engine | `end()` is not called | `end()` is not called | `uncaughtException` | The `end` rows need a transport that is not ended and not destroyed when the socket closes. ### Behavior changes | Case | Before | After | |---|---|---| | Close before the engine, transport not ended and not destroyed | the transport is left as it is | `end()` is called on it | | `end()` of the transport throws during the close | dropped | uncaught exception | | Close after the engine started, transport destroyed | `end()` is called, `writableEnded` true | no call, `writableEnded` false | | EOF before the engine | held until the engine starts | reported inside the transport's `'end'` | | Client, EOF only, before the engine | 1 write (the ClientHello) | 0 writes (Node: 1) | | Client, bytes ahead of an EOF, before the engine | the engine reads them | not read, so an SSL error in them shows as ECONNRESET | | Server wrap, `socket.end()` in an `'end'` listener | transport `writableEnded` true, `final` not called | `writableEnded` false, `final` not called (Node: true, `final` called, see #42350) | ### Not in this PR - #32929: non-TLS bytes give a client `secureConnect`. - #38058: a server wrap whose handshake fails gets no `'close'`. - #43392: an extra `'end'` ahead of `'close'` when the socket is destroyed after the engine started. - #42350: `socket.end()` ends the transport. - #38028: destroy a wrapped `net.Socket`, as Node does. This PR ends it. ### Tests `node-tls-connect.test.ts` and `node-tls-duplex-close-throw-uaf.test.ts`: 25 fail on main (9f70da0), 0 fail with this PR. `test/js/node/tls` and `test/js/node/http2`: 1077 pass, 0 fail. Debug + ASAN, macOS arm64. <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 3 · 4 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 15 failed, 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-connect.test.ts test/js/node/tls/node-tls-duplex-close-throw-uaf.test.ts bun test v1.4.3 (367d939) test/js/node/tls/node-tls-duplex-close-throw-uaf.test.ts: (pass) tls.connect({socket: Duplex}) does not read freed Handlers > when a pre-open duplex error races StartTLS [1332.69ms] (pass) tls.connect({socket: Duplex}) does not read freed Handlers > when duplex.end() throws after close [1443.48ms] 25 | const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); 26 | 27 | // On failure stderr carries the ASAN "use-after-poison" report; include 28 | // it in the assertion so the diff shows the crash rather than just an 29 | // empty stdout. 30 | expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ stdout: expected, stderr: "", exitCode: 0 }); ^ error: expect(received).toEqual(expected) { "exitCode": 0, "stderr": "", "stdout": - "the duplex closes, end() calls: 1 - the soc ... (truncated) release without fix: 34 failed, 22 skipped bun test v1.4.3-canary.1 (367d939) test/js/node/tls/node-tls-duplex-close-throw-uaf.test.ts: (skip) tls.connect({socket: Duplex}) does not read freed Handlers > when duplex.end() throws after close (skip) tls.connect({socket: Duplex}) does not read freed Handlers > when a pre-open duplex error races StartTLS (skip) tls.connect({socket: Duplex}) does not read freed Handlers > when duplex.end() throws after a close that comes before StartTLS (skip) tls.connect({socket: Duplex}) does not read freed Handlers > when an EOF listener destroys the socket and throws before StartTLS test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [0.20ms] (pass) should thow ECONNRESET if FIN is received before handshake [8.07ms] (pass) initializes authorizationError to null in the TLSSocket constructor [0.16ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [3.07ms] (pass) should be able to grab the JSStreamSocket constructor [0.23ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [42.60ms] (pass) tls.connect > should have peer certificate when ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-connect.test.ts test/js/node/tls/node-tls-duplex-close-throw-uaf.test.ts bun test v1.4.3 (367d939) test/js/node/tls/node-tls-duplex-close-throw-uaf.test.ts: (pass) tls.connect({socket: Duplex}) does not read freed Handlers > when duplex.end() throws after close [2203.98ms] (pass) tls.connect({socket: Duplex}) does not read freed Handlers > when a pre-open duplex error races StartTLS [1670.45ms] (pass) tls.connect({socket: Duplex}) does not read freed Handlers > when duplex.end() throws after a close that comes before StartTLS [2263.36ms] (pass) tls.connect({socket: Duplex}) does not read freed Handlers > when an EOF listener destroys the socket and throws before StartTLS [2238.29ms] test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [2.07ms] (pass) should thow ECONNRESET if FIN is received before handshake [276.06ms] (pass) initializes authorizationError to null in the TLSSocket constructor [6.64ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without th ... (truncated) release with fix: 22 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 562c022 features lto, baseline 23 deps, 136 codegen, 1176 objects in 4630ms ninja: Entering directory `/workspace/bun/build/release' [1/4] fetch lolhtml [lolhtml] up to date [2/4] fetch rust-argon2 [rust-argon2] up to date [2/4] cargo plan → /workspace/bun/build/release/rust-target/plan.json 244 units: 172 lib, 16 proc-macro (host), 19 custom-build (host), 15 run custom-build, 17 lib (host), 4 run custom-build (host), 1 rlib [3/4] reconfigure [1/1499] mkdir stamps [2/1499] mkdir codegen [3/1499] install /workspace/bun bun install v1.4.3-canary.1 (367d939) Checked 26 installs across 65 packages (no changes) [730.00ms] [4/1499] rustc unicode_ident [5/1499] rustc build_script_build [6/1499] rustc heck [7/1499] install /workspace/bun/packages/bun-error bun install v1.4.3-canary.1 (367d939) Checked 1 install across 2 packages (no changes) [110.00ms] [8/1499] rustc build_script_build [9/1499] rustc unicode_xid [10/1499] rustc build_script_build [11/1499] rustc build_s ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/socket/UpgradedDuplex.rs | 41 +-- src/runtime/socket/socket_body.rs | 2 +- test/js/node/tls/node-tls-connect.test.ts | 332 ++++++++++++++++++++- .../tls/node-tls-duplex-close-throw-uaf.test.ts | 103 ++++++- 4 files changed, 451 insertions(+), 27 deletions(-) ``` </details> **gate history** · 1 passed · 3 rejected · iteration 3 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/socket/UpgradedDuplex.rs 8 6 68 src/runtime/socket/socket_body.rs 7 2 68 test/js/node/tls/node-tls-connect.test.ts 6 5 54 test/js/node/tls/node-tls-duplex-close-throw-uaf.test.ts 2 1 43 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
cirospaciari
pushed a commit
that referenced
this pull request
Oct 2, 2026
…t wraps (#37664) ### Problem - `new tls.TLSSocket(socket)` on the client side (STARTTLS) does nothing on main. A write throws `TypeError: socket.@Write is not a function`. `_start()` throws `ERR_MISSING_ARGS`. - Since #42181 (not released), `end()` throws an uncaught `TypeError: socket.shutdown is not a function` at `endNT (node:net)`. - Cause: the constructor stored the wrapped stream as `_handle` (`src/js/node/tls.ts`). Nothing replaced it with a TLS handle. ### Fix - The constructor runs the upgrade of `tls.connect({ socket })` (`kUpgradeClientTLS`, `src/js/node/net.ts`). `_handle` is never the stream. `_start()` is a no-op. - The wrap completes like node's `_finishInit`: `'secure'` and `ssl.verifyError()`. It gets no hostname check and no `'secureConnect'`, and `authorized` stays `false`. - Verified: `test/js/node/tls/node-tls-connect.test.ts`. 16 of its 21 new tests fail on main. Also `test/js/node/tls/` and 657 vendored node tests. ### Background - STARTTLS changes a plaintext connection to TLS in place. The `mysql` driver 2.18.1 does it with this constructor. No user filed an issue for it. - In `node:net`, `_write`, `_final` and `_destroy` call into `_handle` as a native handle. - Only `tls.connect()` adds node's `onConnectSecure` (hostname check, `authorized`, `'secureConnect'`). A wrap gets `_finishInit` only. - Considered a start on `_start()`, as in node. Each handle call then needs a guard. ### Downsides - An unused wrap now sends a ClientHello of 1450 bytes (main and node: 0). `setServername()` and `setSession()` after construction have no effect on that handshake. - Unlike node, a wrap rejects an untrusted certificate unless the caller passes `rejectUnauthorized: false`. An app that does its own check must pass `false`. <details><summary>Notes</summary> **Scope.** This head is the core only, as the review of 2026-09-24 asked. The same review decided that a wrap rejects an untrusted certificate by default. Two parts of the earlier head are gone, because other changes own them. #42235 landed the forwarding of the `'error'` of a `Duplex`, and #43791 owns it for a `net.Socket`. #38028 owns the destroy of a wrapped socket that has not connected yet. The `UpgradedDuplex.rs` hunk landed with #36909. The review of 2026-09-25 asked for three more changes: commits 6657bc3 and ae669bf, and this body. The review of 2026-09-30 asked for one more: commit a460a9d. **Changes since the earlier head that the review did not list.** - The `open` handler of an upgraded socket applies the `session` option on the native socket. See "Sessions" below. - A wrap gives the `NODE_TLS_REJECT_UNAUTHORIZED=0` warning of `tls.connect()`. - `authorized` and `authorizationError` keep their initial values on a wrap, as in node. The earlier head set `authorized = true` for a good chain. The wrap checks no host name, so that value accepted a certificate of any host. The verdict is `ssl.verifyError()`. - A wrap does not get `onConnectEnd`. A peer that closes during the handshake gives `'end'`, `'finish'`, `'close'` and no `ECONNRESET`, as in node. `tls.connect({ socket })` keeps its `ECONNRESET`. - `servername` is passed into the upgrade. Before, it reached the ClientHello only when the caller gave no `secureContext`. - `kStandaloneWrap` is initialised in the `Socket` constructor. **Differences from node v26.3.0 that stay.** The review kept the start of the handshake in the constructor. The `mysql` driver, the main user of this API, calls `_start()` right after the constructor, so it sees no difference. | shape | node | this PR | | --- | --- | --- | | wrap that is never used | sends nothing | sends a ClientHello (1450 bytes) | | `setServername()` after the constructor | applies, the handshake starts later | too late. Pass `servername` as an option. | | `setSession()` after the constructor | applies | no effect. Pass `session` as an option. | | untrusted certificate, `rejectUnauthorized` absent or `true` | `'secure'`, the caller must read `ssl.verifyError()` | destroy with the verify error, `'_tlsError'`, no `'secure'` | | untrusted certificate, `rejectUnauthorized: false` | `'secure'`, data flows | the same | | `new TLSSocket(raw)`, then `tls.connect({ socket: raw })` | `EALREADY` on the wrap | `Invalid socket` on the `tls.connect` client (main: works, because its wrap does nothing) | Node never rejects a certificate on a wrap. It leaves the check to the app, so an app that forgets the check accepts any certificate. Here the wrap uses the rule of `tls.connect()`: it rejects unless the caller passes `rejectUnauthorized: false`, or `NODE_TLS_REJECT_UNAUTHORIZED` is `0`. Before commit 4d8b9e5, only `rejectUnauthorized: true` rejected, and a wrap with default options accepted each certificate, as in node. The `mysql` driver listens to `'secure'` and to `'_tlsError'`. If the wrap emitted `'secure'` and then destroyed itself, the driver would report a bad chain two times. The replay test asserts one report. **Sessions.** BoringSSL's `SSL_set_session` calls `abort()` when the handshake has started, in release builds too. `TLSSocket.prototype.setSession()` calls the native function at once, on main and on this head. #41671 puts the guard in the native function, for each caller: a late `setSession()` then throws `Already started.`. An earlier head of this PR made `setSession()` only store the session. The review asked to take that rule out, and commit ae669bf did. For a client-side wrap the review then asked for one narrow rule, in commit a460a9d: the `TLSSocket` constructor sets `kStandaloneWrap`, and `setSession()` returns at once when it is set. | shape | node | main | this PR | | --- | --- | --- | --- | | `new TLSSocket(raw)`, `setSession()`, `_start()` | resumes | throws `ERR_MISSING_ARGS` | no effect, full handshake | | `tls.connect({ socket })`, then `setSession()` | no effect | process aborted | process aborted | | `setSession()` inside `'secureConnect'` | no effect | process aborted | process aborted | | `tls.connect({ port })`, then `setSession()` before it connects | resumes | resumes | resumes | | `tls.connect({ port })`, then `setSession()` in the `'connect'` listener | full handshake | resumes | resumes | | `tls.connect({ socket, session })` | resumes | full handshake | resumes | | `new TLSSocket(raw, { session })` | resumes | throws `ERR_MISSING_ARGS` | resumes | In the first row, the wrap has sent its ClientHello when `setSession()` runs. Without the check in `setSession()`, that row aborts the process (exit 134). The rule also holds for a wrap over a socket that is still connecting: node resumes there, and this PR runs a full handshake. The last two rows come from one hunk that stays. `SocketHandlers2.open` applies the `session` option on the native socket. An fd upgrade assigns `_handle` after `open`, so `self.setSession()` dropped the option there. **Two bugs of main that this PR does not fix. An open PR owns each.** - The native `setSession()` has no check of the handshake state. `socket.setSession()` in the `handshake` callback of a `Bun.connect` socket aborts the process (exit 134, release build of main). #41671 fixes it in `set_session` in `src/runtime/socket/tls_socket_functions.rs`, for each caller. It makes a late `setSession()` throw `Already started.`. - Over a `Duplex`, TLS inside TLS, or a named pipe, a handshake that fails is reported as success. A peer that answers the ClientHello with plaintext gives `'secureConnect'` for `tls.connect({ socket: duplex, rejectUnauthorized: false })` on main, and `'secure'` for a wrap with `rejectUnauthorized: false` here. Node gives `ERR_SSL_WRONG_VERSION_NUMBER`. Over a TCP socket the result is correct. The stream engine in `src/uws/lib.rs` reports no protocol error. #32929 fixes it there. One more door of the same bug: with `rejectUnauthorized: true` and the `session` of an earlier verified connection, `tls.connect({ socket: duplex })` emits `'secureConnect'` with `authorized` true on main, and a wrap emits `'secure'` with `ssl.verifyError()` null here. A write after that fails with `ERR_SOCKET_CLOSED`, and no byte reaches the transport. Reproduction for the first one (needs a key and a certificate, for example `test/js/node/tls/fixtures/agent1-*.pem`): ```ts const server = Bun.listen({ hostname: "127.0.0.1", port: 0, tls: { key, cert }, socket: { data() {}, open() {}, error() {} } }); await Bun.connect({ hostname: "127.0.0.1", port: server.port, tls: { rejectUnauthorized: false }, socket: { data() {}, error() {}, handshake(socket) { socket.setSession(socket.getSession()); /* the process aborts here */ } }, }); ``` Reproduction for the second one: ```js const tls = require("tls"), { Duplex } = require("stream"); let answered = false; const raw = new Duplex({ read() {}, write(chunk, encoding, callback) { callback(); if (!answered) { answered = true; setImmediate(() => this.push(Buffer.from("HTTP/1.1 400 Bad Request\r\n\r\n"))); } }, }); const socket = tls.connect({ socket: raw, rejectUnauthorized: false }); socket.on("secureConnect", () => console.log("secureConnect")); // main prints this socket.on("error", error => console.log(error.code)); // node prints ERR_SSL_WRONG_VERSION_NUMBER ``` **Gaps that this PR does not close.** Each one also exists on main for `tls.connect({ socket })`, with the same result. | shape | node | this PR | owner | | --- | --- | --- | --- | | `end()` or `destroySoon()` before the socket connects | waits for `'connect'`, then sends the FIN | `'finish'` at once, no FIN | #42339 | | refused connection under a wrap | `'_tlsError'`, `'close'` | uncaught `ECONNREFUSED`, the wrap stays open | #38122 | | `end()` over a `Duplex` before the handshake completes | runs the `final()` of the `Duplex` | `'finish'`, no `final()` | #42350 | A wrapped `Duplex` that fails when it is read was in this list. #42235 landed, and the wrap now reports `'_tlsError'` and then `'close'`, as node does (measured on d31efd7). **Inherited options.** `kUpgradeClientTLS` passed a plain `{ socket, servername }` object to `Socket.prototype.connect`, and that function reads `rejectUnauthorized` through the prototype chain. With `Object.prototype.rejectUnauthorized = false`, the wrap accepted an untrusted certificate, also with an explicit `rejectUnauthorized: true`. Commit 6657bc3 passes the decision of the constructor as an own property, as `tls.connect()` does. Measured on this head under that pollution: a wrap with default options, with `{}` and with an explicit `true` rejects, and an own `false` accepts. `new TLSSocket(raw, { rejectUnauthorized: undefined })` with `NODE_TLS_REJECT_UNAUTHORIZED=0` rejects. **`'finish'`.** `new TLSSocket(new PassThrough()).end()` emits `'finish'` and `'close'` on this head. The shutdown cells do not assert `'finish'`. A separate check reports that `'finish'` is lost there when #43962 is applied on top of this PR. This session did not build that combination. Cell by cell for the 21-cell matrix of #42330 on the earlier head: #37664 (comment) **Releases.** Earlier comments in this thread measured the same failures on Bun 1.4.0 and 1.4.3. This session measured main only. **User.** `Connection.prototype._startTLS` in mysql 2.18.1 (`lib/Connection.js`) is the known caller of the client-side constructor. The replay test matches that function line by line, and `lib/protocol/sequences/Handshake.js` sends the SSLRequest and starts TLS with no reply in between. The xmpp report in this thread is for `tls.connect({ socket })`, a different path. A search of the open and closed issues finds no report for the constructor. **Guard design.** #42330 kept the stream as `_handle` and added a guard at 2 of the 6 places that call it as a native handle. **Signatures on main.** `destroy()` on the wrap fails with `handle.close is not a function`. `end()`, `end(cb)` and `destroySoon()` throw `socket.shutdown is not a function` from `process.nextTick`. **Tests.** All are in `test/js/node/tls/node-tls-connect.test.ts`, block `new tls.TLSSocket(socket) on the client side`. - Four reports come from `node-tls-client-wrap-fixture.mjs`. Bun calls its functions in the test process. Node runs the same file as a script. The expected report is the same for both. - `shutdown`: 16 cells. The methods are `end()`, `end(cb)`, `destroySoon()` and `destroy()`. The streams are a connected, a connecting and a never-connected `net.Socket`, and a `Duplex`. Each cell calls the method and then `destroy()`. It asserts no throw, no `'error'` and `'close'`. The cells run together. On main each cell fails with `socket.shutdown is not a function` or `handle.close is not a function`. - `mysql`: the calls of `Connection.prototype._startTLS` in mysql 2.18.1, in the driver's order and at its time. The driver writes the SSLRequest and starts TLS in the same turn, and the server sends no reply in between. Three configurations: `rejectUnauthorized: false`, the CA of the server, no CA. `onSecure` runs one time in each. - `peerCloses`: the peer closes when the ClientHello arrives. - `session`: the `session` option on the three paths, and `setSession()` before the socket connects. Each one resumes. - Both sides of `rejectUnauthorized` run in the test process only, because node accepts in each case. `unlike node, an untrusted certificate destroys the wrap with the verify error` has one case for default options and one for `true`. `with rejectUnauthorized: false, 'secure' fires for an untrusted certificate and a write goes out over TLS` is the other side. `NODE_TLS_REJECT_UNAUTHORIZED=0 turns the default off, as for tls.connect()` pins the environment variable. - `an inherited rejectUnauthorized cannot turn the check of a wrap off` runs in a child process with `Object.prototype.rejectUnauthorized = false`: default options and an own `true` reject, an own `false` accepts. It fails on d31efd7. ``an own `rejectUnauthorized: undefined` still rejects with NODE_TLS_REJECT_UNAUTHORIZED=0, as for tls.connect()`` pins that rule. - `setSession() on a wrap has no effect: it does not abort the process and does not throw` runs in a child process, because the failure is a process abort. Without the check it gets exit code 134. No test covers a late `setSession()` on a `tls.connect()` socket: it aborts the process until #41671 lands. - On main (canary 367d939, release build), 16 of the 21 tests in the block fail. 8 fail at once, and 8 fail by the timeout, because main starts no handshake. The 5 that pass are the http2-wrapper guard and the 4 rows that run node. - The SNI test fails when only the `servername` argument is reverted (`Expected: "sni.example"`, `Received: undefined`). **Cost for callers that never wrap a socket,** from the diff: - per `net.Socket`: one more property store in the constructor. - per TLS `connect()`, per client handshake and per `setSession()`: one more property read and branch. - per `internalConnect` and `internalConnectMultiple`: two property reads and branches fewer. Each `[buntls]` options object has one property fewer. - `tls.connect({ socket })` puts the same bytes on the wire as on main (1452). - This session did not measure instructions, syscalls or binary size: `perf`, `valgrind`, `strace` and `bloaty` are not in the test container. A separate differential check of d31efd7 merged onto main reports equal instruction counts: 10,162,756 (main) and 10,159,922 (this PR) for each TLS connection, and 2,195 and 2,200 for `new net.Socket()`. **Suites run with a debug build of this head.** - On the head a460a9d: `test/js/node/tls/node-tls-connect.test.ts` gives 107 pass, 18 skip, 0 fail with a 30 s limit, in 2 of 2 runs. With the default 5 s limit, 4 to 8 tests reach the timeout in each run on this machine, and the set differs from run to run. Most of them came from main. The load average was 450 to 970 on 16 cores. - One of those tests from main (`server write() and end(data) from inside ALPNCallback`) takes the same time with the source of main and with this PR: 3.9 to 6.4 s and 3.8 to 6.0 s, 10 runs each. - `tsc --noEmit -p src/js/tsconfig.json` and `bun lint` pass on a460a9d. - The suites below ran on the head c81cee3, before the merge of main. - `test/js/node/tls/` (28 files): 2 failures in each run, and this change causes neither. `SNICallback runs even when the requested servername matches the bind hostname` fails on the release build of main too. `concurrent Workers all see the same CA certificate lists` fails 5 of 5 times with the `net.ts` and `tls.ts` of main on the same debug build. In the last run the machine was overloaded, and 2 more tests reached the 5 s timeout. Both pass alone in 3 of 3 runs, and neither builds a client-side wrap. - `test-tls-*`, `test-https-*`, `test-net-*`, `test-http2-*` in `test/js/node/test/parallel` (657 files): no failure from this change. 10 files fail on main too (`test-https-proxy-request*.mjs`, `test-https-request-proxy-post.mjs`, `test-tls-client-allow-partial-trust-chain.js`). `test-https-timeout.js` hangs on a debug build, with the source of main too. - `test/js/bun/net/socket.test.ts`: 94 pass, 1 fail. The failure needs DNS for `www.example.com` and fails on main too. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 16 failed, 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-connect.test.ts bun test v1.4.3 (367d939) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [3.32ms] (pass) should thow ECONNRESET if FIN is received before handshake [351.98ms] (pass) initializes authorizationError to null in the TLSSocket constructor [9.71ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [175.18ms] (pass) should be able to grab the JSStreamSocket constructor [18.49ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [114.28ms] (pass) tls.connect > should have peer certificate when using self asign certificate [269.78ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port ... (truncated) release without fix: 34 failed, 18 skipped bun test v1.4.3-canary.1 (367d939) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [20.95ms] (pass) should thow ECONNRESET if FIN is received before handshake [48.67ms] (pass) initializes authorizationError to null in the TLSSocket constructor [0.39ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [5.88ms] (pass) should be able to grab the JSStreamSocket constructor [0.30ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [5.44ms] (pass) tls.connect > should have peer certificate when using self asign certificate [24.26ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port and host correctly (skip) tls.connect > should process port, host, and callback correctly (skip) tls.connect > should handle the absence of a callback gracefully (skip) tl ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-connect.test.ts bun test v1.4.3 (367d939) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [3.27ms] (pass) should thow ECONNRESET if FIN is received before handshake [394.13ms] (pass) initializes authorizationError to null in the TLSSocket constructor [8.43ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [196.56ms] (pass) should be able to grab the JSStreamSocket constructor [33.22ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [298.86ms] (pass) tls.connect > should have peer certificate when using self asign certificate [101.96ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port ... (truncated) release with fix: 18 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision d31efd7 features lto, baseline 23 deps, 136 codegen, 1176 objects in 6018ms ninja: Entering directory `/workspace/bun/build/release' [1/4] fetch lolhtml [lolhtml] up to date [2/4] fetch rust-argon2 [rust-argon2] up to date [2/4] cargo plan → /workspace/bun/build/release/rust-target/plan.json 244 units: 172 lib, 16 proc-macro (host), 19 custom-build (host), 15 run custom-build, 17 lib (host), 4 run custom-build (host), 1 rlib [3/4] reconfigure [1/1499] mkdir stamps [2/1499] mkdir codegen [3/1499] install /workspace/bun bun install v1.4.3-canary.1 (367d939) Checked 26 installs across 65 packages (no changes) [231.00ms] [4/1499] rustc unicode_xid [5/1499] rustc heck [6/1499] rustc build_script_build [7/1499] rustc build_script_build [8/1499] rustc unicode_ident [9/1499] rustc build_script_build [10/1499] rustc build_script_build [11/1499] install /workspace/bun/packages/bun-error bun install v1.4.3-canary.1 (367d939) Checked 1 install across 2 packages (no changes ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/internal/net/symbols.ts | 2 + src/js/node/net.ts | 68 ++-- src/js/node/tls.ts | 47 +-- test/js/node/tls/node-tls-client-wrap-fixture.mjs | 387 ++++++++++++++++++++++ test/js/node/tls/node-tls-connect.test.ts | 356 +++++++++++++++++++- 5 files changed, 806 insertions(+), 54 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/internal/net/symbols.ts 0 0 72 src/js/node/net.ts 8 0 73 src/js/node/tls.ts 1 0 74 test/js/node/tls/node-tls-client-wrap-fixture.mjs 2 3 77 test/js/node/tls/node-tls-connect.test.ts 0 0 69 ``` </details> <!-- robobun:evidence:end -->
Collaborator
|
Superseded by #44618, which consolidates the open TLS pull requests. This fix and its tests are in there, either as written, rewritten smaller, or merged with the other PRs that patched the same cause (see the "By area" list in that PR). Closing in favor of it. |
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 6, 2026
…t when an engine is attached (#38028, #38122, #38076) Node ties the two sockets together in the TLSSocket constructor, whatever state the wrapped one is in: its 'close' destroys the wrap, its 'error' is re-emitted, and closing the TLS handle destroys it. Bun made those links at the moment a TLS engine was attached, in six copies, and the last one only for a generic Duplex or an adopted fd. So a wrap with no engine yet (a client waiting for 'connect', the server's one-tick deferral, a server over a connecting socket) and every stream-level wrap over a net.Socket had none: the fd leaked and server.close() never called back, a refused connect was uncaught or reported nowhere, a socket dialed again was upgraded by its dead wrap, and a server wrap adopted a half-connected socket, which kept the process alive. linkUpgraded() makes the links once, before any branching. The client and server attach code is one function each over two shared helpers. _destroy takes down any transport that is not the raw twin of an adopted fd, where node does it, next to the handle close, so it is still intact inside 'error'. A server wrap over a connecting socket waits for 'connect', and named pipes take the stream-level engine there as they do on the client. The constructor refuses a secureContext that is not one (ERR_TLS_INVALID_CONTEXT) instead of throwing out of nextTick.
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 7, 2026
…t when an engine is attached (#38028, #38122, #38076) Node ties the two sockets together in the TLSSocket constructor, whatever state the wrapped one is in: its 'close' destroys the wrap, its 'error' is re-emitted, and closing the TLS handle destroys it. Bun made those links at the moment a TLS engine was attached, in six copies, and the last one only for a generic Duplex or an adopted fd. So a wrap with no engine yet (a client waiting for 'connect', the server's one-tick deferral, a server over a connecting socket) and every stream-level wrap over a net.Socket had none: the fd leaked and server.close() never called back, a refused connect was uncaught or reported nowhere, a socket dialed again was upgraded by its dead wrap, and a server wrap adopted a half-connected socket, which kept the process alive. linkUpgraded() makes the links once, before any branching. The client and server attach code is one function each over two shared helpers. _destroy takes down any transport that is not the raw twin of an adopted fd, where node does it, next to the handle close, so it is still intact inside 'error'. A server wrap over a connecting socket waits for 'connect', and named pipes take the stream-level engine there as they do on the client. The constructor refuses a secureContext that is not one (ERR_TLS_INVALID_CONTEXT) instead of throwing out of nextTick.
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 8, 2026
…t when an engine is attached (#38028, #38122, #38076) Node ties the two sockets together in the TLSSocket constructor, whatever state the wrapped one is in: its 'close' destroys the wrap, its 'error' is re-emitted, and closing the TLS handle destroys it. Bun made those links at the moment a TLS engine was attached, in six copies, and the last one only for a generic Duplex or an adopted fd. So a wrap with no engine yet (a client waiting for 'connect', the server's one-tick deferral, a server over a connecting socket) and every stream-level wrap over a net.Socket had none: the fd leaked and server.close() never called back, a refused connect was uncaught or reported nowhere, a socket dialed again was upgraded by its dead wrap, and a server wrap adopted a half-connected socket, which kept the process alive. linkUpgraded() makes the links once, before any branching. The client and server attach code is one function each over two shared helpers. _destroy takes down any transport that is not the raw twin of an adopted fd, where node does it, next to the handle close, so it is still intact inside 'error'. A server wrap over a connecting socket waits for 'connect', and named pipes take the stream-level engine there as they do on the client. The constructor refuses a secureContext that is not one (ERR_TLS_INVALID_CONTEXT) instead of throwing out of nextTick.
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 10, 2026
…t when an engine is attached (#38028, #38122, #38076) Node ties the two sockets together in the TLSSocket constructor, whatever state the wrapped one is in: its 'close' destroys the wrap, its 'error' is re-emitted, and closing the TLS handle destroys it. Bun made those links at the moment a TLS engine was attached, in six copies, and the last one only for a generic Duplex or an adopted fd. So a wrap with no engine yet (a client waiting for 'connect', the server's one-tick deferral, a server over a connecting socket) and every stream-level wrap over a net.Socket had none: the fd leaked and server.close() never called back, a refused connect was uncaught or reported nowhere, a socket dialed again was upgraded by its dead wrap, and a server wrap adopted a half-connected socket, which kept the process alive. linkUpgraded() makes the links once, before any branching. The client and server attach code is one function each over two shared helpers. _destroy takes down any transport that is not the raw twin of an adopted fd, where node does it, next to the handle close, so it is still intact inside 'error'. A server wrap over a connecting socket waits for 'connect', and named pipes take the stream-level engine there as they do on the client. The constructor refuses a secureContext that is not one (ERR_TLS_INVALID_CONTEXT) instead of throwing out of nextTick.
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 10, 2026
…ps, WebSocket, SQL) (#44618) ### What does this PR do? Consolidates the open TLS pull requests into one. Each was reproduced on `main` and, for `node:*` behavior, on Node v26.3.0 first. About a third are ported as written, the rest are rewritten smaller or merged into one fix where several PRs patched the same cause. One commit per fix, so it can be read commit by commit. Fixes #43520, fixes #31396, fixes #43635, fixes #37193, fixes #43846, fixes #17932, fixes #41061, fixes #36887, fixes #31810, fixes #35240, fixes #32234, fixes #44365, fixes #43807, fixes #42280, fixes #44517. Addresses #41856 (SNI and `servername`; not `checkServerIdentity` for SQL), #24845 (the spin is gone, shown with fault injection on Linux; not run on macOS), #19754 (node-fetch forwards the agent's TLS options; the Kubernetes client itself was not run). #### The ones that matter most | | On `main` | PRs | |---|---|---| | Client certificate disclosure | `https.request()` with a client certificate sends it to a server it then refuses (wrong name, `checkServerIdentity`, `destroy()` in `'secureConnect'`, `terminate()` in `handshake`). A server can force it with a junk record behind its Finished | #43946 | | False `authorized` | Over a Duplex, `secureConnect` with `authorized === true` for a peer that failed the key proof; `secureConnect` for a plaintext peer with `rejectUnauthorized: false` | #44422, #32929 | | Cleartext https | `https.createServer()` without a usable key/cert answers plain HTTP | #41672, #33539 | | Revoked client certificates | An https mTLS server never sees `crl`, so a revoked client is `authorized` | #41641 | | Pooled sockets | Requests with different client certificates or CAs share an `https.Agent` socket and session | #42498 | | Silent plaintext | `tls: [...]` given to `Bun.listen` / `Bun.connect` is plain TCP | #41490 | | Server weakened by a client knob | `NODE_TLS_REJECT_UNAUTHORIZED=0` turns off a server's client-certificate enforcement | #35245 | | Pins never checked | `WebSocket` never calls `tls.checkServerIdentity` and ignores `tls.serverName` | #41648 | | `verify-full` dropped | `PGSSLMODE=verify-*` is lost next to a `TLS_*` URL variable; `tls: true` sends no SNI | #44498 | | Crashes | use-after-free from `destroy()` in `ALPNCallback` over a Duplex; `abort()` on a late `setSession()`; SIGABRT in `fetch` with an https proxy from the environment and a `Bun.file()` body | #44462, #41671, #44458 | | Stream corruption | A TLS `write()` can lose 16 KiB it reported as written while another socket on the loop is stalled | #44529 | | Hangs and spins | 100% CPU on a failing `send()`; a fatal `SSL_write` leaves the socket open forever; `idleTimeout` never sheds a TLS client that ignores `close_notify` | #34510, #38176, #42336 | | Wrong certificate (regression since 1.3.14) | Connections accepted before `stop()` / `close()` get the default certificate and skip their entry's `requestCert` / `ca` | #42355 | | Quadratic Duplex / proxy tunnel | Reading one chunk over a Duplex, CPU: 8 MB 0.88 s → 0.14 s, 16 MB 3.18 s → 0.23 s, 32 MB 11.75 s → 0.39 s; `fetch` upload through CONNECT: 1.7 s → 0.18 s (debug build) | #44464 | #### By area - **fd engine, write path** (`openssl.c`, `socket.c`): #42352, #34510 + #38176 + #42336 as one change, #44529, #44458, #44192. A rejected `send()` ends the write side only and closes at the next writable event unless the peer's bytes are still queued (a 413 sent before a reset is still read). No new per-socket state. Also, on kqueue, **a FIN no longer ends a socket that waits in the low-priority queue** (`loop.c`): with more than 5 TLS handshakes at once, a client that ended right after its handshake could be reset and its server socket report `socket hang up`, because the eof that the sentinel read knote reports was acted on ahead of the unread Finished. That is on `main` too (the macOS entry for `node-tls-server.test.ts` in `test/flaky-tests.txt`: 7 of 48 recent builds of other branches), and this branch made it likelier (6 of 8 builds), since Finished now leaves in one segment with the close_notify. - **Error reporting, both engines**: #44422, #32929, #44516, #37094, #41272 + #42324 + #44223 as one change, #44021, #37472, #43946, #33630. One channel: a fatal error on an established session is reported, then **the engine closes the connection itself**, whatever the owner does with the report. `test/js/bun/net/tls-fatal-error-closes.test.ts` asserts closed-and-nothing-delivered for every owner (node:tls, `Bun.connect`, `Bun.listen`, `fetch` direct and through CONNECT, `Bun.serve`, `WebSocket` direct and through a proxy, Postgres, MySQL, Valkey, Duplex). - **Duplex engine** (`SSLWrapper`, `UpgradedDuplex`): #44462, #43529, #42332, #44464. #43877 + #44394 were in and are **out again**, see "Worth a look" 5. - **node:tls wrap lifecycle** (`net.ts`, `tls.ts`): #38007, #38058, #38028 + #38122 + #38076 as one change (six copies of the attach code become two helpers), #38311, #39008, #38154, #42340 + #42343 + #42339 + #42453 as one change, #43791, #42425, #44085, #42683, #39088, #39040, #40375, and what was still real of #36534. - **SNI, ALPN, server contexts**: #43080, #42050, #37195 + #43849 as one change (**one** SNI matcher for TCP and HTTP/3), #42355, #42285, #33253, part of #37896, part of #37013. A `tls.Server` has one `SSL_CTX`. - **Verification and options**: #44738, #41490, #37005 + the cwd pin of #40984, #31811, #43982, #33483 + #35245, #41810, #32235, #44441, #38092. - **node:tls API and CA store**: #41671, #38145, #32824, #43594, #39997, #41696, #33534, #34748, #42991, #42996, #42970. - **node:https, Agent, `ws`, node-fetch**: #41672 (https half), #41641, #38261, #42498, #44346, #35609, #31397, #42325. - **WebSocket client**: #41648, #37487 + #43048 as one change. - **SQL, Redis**: #33666, #41711, #44498, part of #42054. - **Tests only**: #41426, #40040, #44395, #44016, #37860, #40591, #44440, #41424. Found on the way and fixed here: an upload that a TLS 1.2 server interrupts with a renegotiation never completes on `main` (0 of 32 runs over `https.request`, `fetch`, `node:tls` and `Bun.connect`: the renegotiation ClientHello lands inside an application record that is still unsent, or the socket gets no `drain` again) and completes here, with two tests from robobun; the fix for #40653 (final flight and first write in one segment) stopped working whenever another TLS socket on the loop was stalled, on `main` too; the `tls.Server` prototype pinned the last server constructed and every `SSL_CTX` it owned; `Object.create(process.env).NODE_TLS_REJECT_UNAUTHORIZED = "0"` turned verification off process-wide once a `SHARE_ENV` worker existed; two debug panics when wrapping a shut-down or still-connecting socket; a `fetch` POST through a proxy sent its headers twice when the origin renegotiated; `BlockList` ignored IPv6 zone ids; a test now ties `root_certs.der` to `certdata.txt`. #### Behavior changes - **A server's `ca` without `requestCert` no longer asks for a client certificate** (`Bun.serve`, `Bun.listen`, HTTP/3, node:tls). It matches the docs and Node. On `main` such a server refused clients with no certificate but served any unrelated self-signed one, so it was never authentication. **Set `requestCert: true` to require a certificate.** A matrix test pins that `requestCert: true` still refuses no certificate and an untrusted one on 8 kinds of server, TLS 1.2 and 1.3, with `NODE_TLS_REJECT_UNAUTHORIZED` unset and `0`. - `NODE_TLS_REJECT_UNAUTHORIZED=0` no longer relaxes a server. - `Bun.connect` / `Bun.listen` hear of a fatal TLS error after the handshake through `error(socket, err)`. With no `error` handler the socket just closes. - HTTP/3 server names match like TCP: `*.` covers exactly one label, case is ignored, a trailing dot is ignored, the last registration of a name wins. - `requestCert` on node:https is `=== true`, as in Node. - An array where a generated options dictionary is expected throws (`tls: []`, `jest.useFakeTimers([])`). - `key` / `cert` arrays serve every identity. A client that can use both gets ECDSA, where `main` served whichever pair came last. - `ecdhCurve` is forwarded by node:https, `ws` and node-fetch now, so a group BoringSSL lacks (`X448`) throws there as it already does in `tls.createServer`. - A wrapped socket's error is re-emitted on the TLS socket as in Node, so `raw.destroy(err)` with a listener on `raw` only is uncaught, as in Node. - `sql.options.tls` is always an object, never `true`. `RedisClient` sends SNI. - `tls: { secureContext }` alone asks for TLS on `Bun.listen` / `Bun.connect` (it was plain TCP), and a value that is not a `SecureContext` throws. The context is served as it is: the `requestCert` / `rejectUnauthorized` it was created with hold whatever the options next to it say, and `requestCert` in the options over a context that does not ask throws at `listen()`. - `tls.DEFAULT_CIPHERS` reaches every client once assigned (`fetch`, `WebSocket`, `Bun.connect`, `RedisClient`, `Bun.SQL`, `S3Client`, proxy tunnels) and servers again. A list that selects no cipher throws `ERR_SSL_NO_CIPHER_MATCH` at the assignment. `fetch.preconnect()` dials nothing after an assignment. - The warning for an unreadable `NODE_EXTRA_CA_CERTS` is Node's one line, without the `warn:` prefix. - `BUN_CONFIG_WS_CLOSE_TIMEOUT` (default 30 s): how long a `WebSocket` client waits for the server to close the connection after the closing handshake. #### Worth a look in review 1. **#44529**: the kernel-refused remainder of a TLS write moves from the loop's one slot onto the connection (in the existing rare struct), so the write BIO never refuses a sealed record. Nothing is allocated on an unstalled path (200 writes: 0 appends, same `send()` count as `main`), memory with 16 stalled writers is lower than on `main` (276 KB vs 340 KB, which `main` holds inside BoringSSL's buffers), `us_socket_t` stays 80 bytes. It needs a bound on how long a deferred close waits, or a peer that stops reading pins the fd past `destroy()`: `US_SSL_CLOSE_AFTER_SPILL_TIMEOUT` is a fixed 10 s, not re-armed on progress. Separate commits, but the fix that keeps the client certificate off the wire beside a stalled socket builds on them. 2. **The default name check of node:tls also runs inside the handshake**, so a wrong-name server gets no client certificate on TLS 1.2 either. JS still runs it after every successful handshake, so a difference between the two matchers can only refuse. Error objects are byte-identical. 3. **#44441** widens trust by design: a self-issued leaf whose `keyUsage` lacks `keyCertSign` (`dotnet dev-certs`) is its own anchor when the store holds a byte-identical copy. No BoringSSL change. Expired pin, same subject with another key, wrong EKU and a pinned intermediate are tested to fail. 4. **#32235** only adds Ed25519 and ECDSA P-521 to the verify list. A captured ClientHello shows `main`'s list with the two inserted; `rsa_pkcs1_sha1` stays. 5. **A stream that a TLS socket wraps, when that TLS socket closes.** An earlier state of this branch lost data here while CI was green (found by #44709's report): with the peer closing first, 4 of 8 MiB arrived with TLS in TLS, 4 of 32 MiB on the http2 `emit("connection")` path, and a `write()` with no `'error'` listener ended the process. Three Node-parity changes only hold together: destroying the wrapped stream at the close (#38028 + #38122 + #38076, #38154) is safe only if every write has really completed (#43877), which in turn needs Node's handling of the peer's close_notify, which needs half-open sockets that the GC can collect. So: - #43877 + #44394 are reverted and reopened. A write over a stream completes once the stream has taken the ciphertext, as on `main`. - Until the verdict on the peer lets the session through, the application cannot have written over it. There the wrapped stream is destroyed as in Node, with the sessions below it. That keeps the release of the connection after a failed handshake, a rejected certificate and an early `destroy()`. The same for an http2 socket the application never got, and for `resetAndDestroy()`. - After that it is `main`'s teardown: a `net.Socket` only gets the engine's `end()`, closes at its peer's FIN, keeps its own timeout and reports its own errors. Any other stream is destroyed with the TLS socket. The regular suites cannot see any of this (999 files were green on every broken variant), so it was steered by eleven seeded differential fuzzers run on this build, `main`, Node v26.3.0 and the earlier state: close, `end()`, `destroy()`, `destroySoon()`, resets, hung and half-open peers, paused writers, timeouts, two and three sessions deep, over TCP and over Duplexes, before, at and after the handshake, and http2 requests. See "How did you verify". #### Known limits - `fetch` with a `checkServerIdentity` function still sends the client certificate (not the request) to a server the function refuses. On TLS 1.2 any verdict a JS callback gives is too late, as in Node. - `addContext()` / `SNICallback` still do not apply to a server-side socket on the stream engine (`emit("connection", duplex)`, TLS in TLS, unflushed writes, named pipes), as on `main`. - A CA bundled in a pfx extends an explicit `ca` only, for `ws` / node-fetch / `WebSocket`: the native `ca` can only replace the default store, and that store keeps `SSL_CERT_FILE` / `SSL_CERT_DIR`. - P-521 leaves work on TLS 1.3 only. TLS 1.2 needs secp521r1 in every ClientHello (`it.todo`). - Once `tls.DEFAULT_CIPHERS` is assigned, `fetch(url, { protocol: "http3" })` is `HTTP3Unsupported`, as with an explicit `ciphers`. - `addCACert()` by hand does not extend the chains of a context with several identities. - A throwing `ALPNCallback` sends `no_application_protocol` on both engines. Node sends nothing and its client sees `ECONNRESET`. - TLS in TLS, peer FIN while the outer handshake runs: the inner socket gets one `write EPIPE`, where Node gives `ECONNRESET` (`main` gives it no error at all). - `@SECLEVEL` in `ciphers` is dropped by the `ws` / node-fetch shims, which used to ignore `ciphers`. node:tls keeps throwing `ERR_SSL_INVALID_COMMAND`. - Beside a stalled TLS socket only the first record (16 KiB) of the first write leaves with the handshake flight. The rest goes record by record, which is what bounds the memory of stalled writers. - After a fatal error on an established session the socket emits `'error'` and then `'close'`. Node emits `'error'` and leaves the socket open. - A paused reader whose own write the kernel rejects loses what it had not read yet, with an `EPIPE`, as on Node. `main` reports no error there and delivers it. - On `main` too: a `Bun.listen` socket without `allowHalfOpen` that has unsent ciphertext when the client's `shutdown()` arrives loses that ciphertext (32 KiB), and over plain TCP `end()` with the peer still sending is a close over unread input, so a reset. - Differences from both `main` and Node that the differential runs below found and that stay, all with a peer that aborts: `ECONNRESET` instead of a clean `'end'` after the socket's own `'finish'` when the peer destroyed with unread data; under TLS 1.2, a zero-length `write()` followed by `destroy()` in `'secureConnection'` leaves the client without `'secureConnect'` (a plain `destroy()` there matches Node); a TLS 1.2 client that destroys in `'secureConnect'` gets no `'session'`; a `ClientRequest` whose handshake fails with an alert emits `'error'` and `'close'` but no `'finish'` (`writableFinished` is true). - Once `tls.DEFAULT_CIPHERS` is assigned, `fetch.preconnect()` opens nothing: `fetch()` then uses a context of its own, and a socket warmed under the default one would never be picked up. - A TLS `send()` that the kernel refuses outside a `write()` call (the drain of unsent ciphertext) is reported with the close, as `read EPIPE` / `read ECONNRESET`. Node says `write EPIPE`. `main` does not report it at all. - Once the application has a session over a `net.Socket` (TLS in TLS, http2 `emit("connection")`), a peer that never sends its FIN holds that socket after the TLS socket closed, as on `main`. Node destroys it. Two tests of #38154 are `todo` for this. Closing it any earlier (at its `'finish'`, say) makes the kernel drop what it has not sent yet as soon as the peer's close_notify arrives. - Plaintext that was queued on a socket before it was wrapped (STARTTLS with a backlog) is dropped when the TLS socket is destroyed, or its handshake fails, before the session is accepted. Node drops it too, except on `destroySoon()`. `main` sends it. - Over a stream that is no `net.Socket`, `end()` can still cut what that stream has buffered, and there is no backpressure, both as on `main` (#43877). - `tls.secureContext` (the undocumented door node:tls uses) is not read by a Windows named pipe listener, which builds its context from the options. On `upgradeTLS({ isServer: true })` the options next to it are the policy, as with Node's `SetVerifyMode`. - `selectServerName()` rebuilds the name tree per ClientHello for injected sockets of a server with `addContext()` entries: 0.4 µs for 1 entry, 3.7 µs for 10, 41 µs for 100, against 631–1111 µs for a handshake. #### Not included Left open, because they need a decision or are not TLS: #43877 + #44394 (see "Worth a look" 5; #43874 stays open with them), #38548, #38591 (both shrink who is trusted), #41589 (`verify-full` vs `NODE_TLS_REJECT_UNAUTHORIZED=0`), #37197, #41706, #43216, #33487, #33545, #36707, #32435, #37255, #28691, #40275, #30314 (features), #38120 (needs the BoringSSL fork, as did #33517, which the stale bot has closed since), #38529 (needs a Windows measurement), #34342, #38232, #43089, #44454, #40451, #42710, #44527, #38088, #38093, #41898. #37896, #42054 and #37013 stay open for the halves not taken. `http.createServer({ key, cert })` keeps serving TLS on purpose. One open question: `tls: {}` (an object that names no TLS option) is plain TCP on `Bun.listen` / `Bun.connect`, here and on `main`. It is the same trap as `tls: []`, but changing it changes a Bun default, so it is left alone. ### How did you verify your code works? - Every new test fails on `main` for the stated reason and passes here, except guards that pin existing behavior, each shown to fail when its clause is removed. `node:*` tests also pass on Node v26.3.0; the few that cannot say which Node version has the behavior. - 212 test files that touch TLS, sockets, http, http2, fetch, WebSocket, SQL, Valkey and workers: 6154 pass, 2 fail. Both are seen on `main` too: `serve.test.ts` "root range port" (the box runs as root), and `worker_threads.test.ts` "terminate(): nothing of the worker's runs after the request", which is flaky there and passed in the run below. - 58 of those files the way the ASAN lane runs them (LeakSanitizer + `BUN_JSC_validateExceptionChecks`): 58 files, 48 of them with leak checking, 4132 pass, 3 fail. All three also fail on `main`: `serve.test.ts` "root range port", `node-net.test.ts` "should not leak when connect({path}) fails synchronously on a reused handle" (times out under this environment), `worker_threads.test.ts` "process.exit() with a shell cp in flight" (a `ShellCpTask` leak). - 647 vendored `test-tls-*`, `test-https-*`, `test-net-*`, `test-http2-*`: the only two failures also fail on `main`. - The SNI matcher was diffed against both old matchers: 3 seeds × 1.23 M lookups × 3 registration flavours, every difference in one of the intended classes, TCP and HTTP/3 identical on every lookup. - The headline rows were also driven by hand with scripts against this build, `main` and Node v26.3.0: cleartext https, `crl`, `tls: []` / `{ secureContext }`, the `ca` / `requestCert` matrix, the client certificate on a wrong-name server, late `setSession()`, `destroy()` in `ALPNCallback`, `[rsa, ec]` identities with an intermediate from `ca`, `WebSocket` `checkServerIdentity`, a corrupted record, the Duplex read above, `tls.DEFAULT_CIPHERS`. - The `setSession()` guard was checked against the real `abort()` at 43 handshake states. - `bun run rust:check-all`: 12 of 12 targets. `tsc`, oxlint, source lints, prettier, rustfmt, mordant clean. - usockets' `_Nonnull` is compiled out of debug builds, so 105 of those files were also run on a local release ASAN build with the CI runner's environment (92 with leak checking): 4595 pass, 1 fail, `child_process.test.ts` "spawn reports EPERM after dropping privileges", which cannot pass as root and fails on `main` too. - The close of a TLS socket over another stream ("Worth a look" 5): eleven seeded differential fuzzers, 8,424 scenarios compared, each run on a release ASAN build of this branch, on `main`, on Node v26.3.0 and on the earlier state of the branch. Against `main`: - Data that `main` delivers in full is cut in 5 scenarios, and about 150 that `main` cuts arrive in full. Of the 5, in 2 `main` never notices the peer's close and keeps the socket for good, 2 call `end()` on the middle one of three sessions over an in-memory Duplex, and 1 does the same on Node. - No dead timeout, no silent reset and no uncaught error that `main` does not have (4 uncaught errors fewer). - A socket stays open where `main` closes it in 109, and closes where `main` keeps it in 295. 92 of the 109 do the same on Node or on the earlier state (a `destroy()` that an in-memory Duplex does not show its peer, half-open peers). 14 wait for a peer that paused reading and so does not read the FIN (#42332's backpressure, as in Node); the socket's own timeout fires there. 3 are left: one on a 5 ms timer, two with three sessions over an in-memory Duplex. - The earlier state of the branch cut data in 173 of the 400 scenarios of one of them, where `main` cuts none and this cuts none. - 23 new tests pin what they found. Each earlier attempt at this fix fails the ones that describe it, the earlier state of the branch fails 7, and all pass on Node. - After that change: 999 test files on the release ASAN build (20,246 pass; the 11 files that fail need a database, Docker, DNS or a non-root user, or share a temp directory with a parallel run and pass alone), 61 on the debug build. - TLS over a file descriptor (`openssl.c`, the path of `fetch`, `Bun.serve`, `tls.connect`, `Bun.connect`) got the same treatment after the rebase: seeded differential fuzzers on CI's release build of this branch, on `main` and, for `node:*`, on Node v26.3.0. Every runtime also against itself for the noise floor, injected faults and known bugs of `main` as positive controls, and a difference counts only if it shows in 5 of 5 fresh processes. - `node:tls` over TCP: 11,500 scenarios (one connection with Node as the oracle line by line; 2 to 60 connections beside stalled neighbours; raw peers that break the handshake). HTTPS: about 136,000 runs over `Bun.serve` + `fetch`, `node:https`, `node:http2` and `wss://`, also with the two ends in different runtimes. `Bun.connect` / `Bun.listen` / `upgradeTLS`: 11,500 scenarios and 720 slow connections, with writers driven by what `write()` returns, beside up to 6 stalled, dripping, closing or resetting neighbours, and plain TCP as a second oracle. No crash, hang, duplication, reordering or silent truncation, and no change in time or in connection reuse. - They found six things that `main` does better, none of which any test showed. All are fixed, each with a test that fails on the build before: what the peer sent lost behind a rejected `send()` (23 scenarios, and an early HTTPS response lost with only `EPIPE`), the same silently for a paused reader, `server.close()` never calling back on a half-open server after a ClientHello and a reset (17), `closeAllConnections()` taking 12 s with a stalled client, `end()` losing up to 1.3 of 4 MiB that `write()` had reported while the peer still uploads, and `end()` a little after a stall never closing beside other stalled TLS sockets. The last two fixes also deliver the 1 to 2 MiB that `main` loses there, and close the socket that `main` keeps for good without such neighbours. - All of them again after every fix, on CI's release build of it. That caught one regression of a fix itself (a reader stopped for backpressure lost 86,385 bytes, 1 of 6,000 scenarios), fixed too. On the last build: scenarios that lose data where `main` does not 23 → 2, and Node loses it in both, with the same `EPIPE`; `server.close()` that never calls back 17 → 0; connections held 4 → 0; requests that end in an error only where `main` has a response 6 → 0. With a Node server in another process, a request ends in an error only in 8 and 10 of 1,500 scenarios here, 5 and 3 on `main`, 10 with Node as the client. - `Bun.connect` / `Bun.listen` on the last build against `main`, in scenarios: hangs 0 against 1,031, sockets and fds never released 0 against 965, corrupted data 0 against 345, `abort()` 0 against 26 (`setSession()` after the handshake), writers that never close 0 against 101 of 720 connections. No kind of failure shows here and not on `main`. About a third of the slow connections close later than on `main`, in 1 to 16 s instead of at once, waiting for unsent ciphertext or for the peer's close_notify, and 79 more of them deliver all that `write()` reported. RSS and time with 16 to 256 stalled writers are the same. - What they found that `main` does worse: a `WebSocket` that calls `close()` with sends pending loses messages in 81 of 999 scenarios (0 here), 37 server sockets left open, 10 `server.close()` that never call back, 20 write callbacks that never run. - The kqueue fix cannot be run on Linux. The `connectionListener` count test now says what became of a missing connection, which is how the cause was found (`'tlsClientError'` "socket hang up", then `read ECONNRESET` at the client of the same port, after its `'secureConnect'`). On macOS x64 it failed every attempt of the three builds before the fix and passed at the first attempt of the build with it. - Windows and macOS were only run by CI. Four new tests asserted what only the Linux kernel does (a FIN read ahead of a reset, unread bytes surviving a reset, loopback buffer sizes, `fstat()` on a socket) and now say so per platform. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
net.Socket(new tls.TLSSocket(raw, { isServer: true }),tls.connect({ socket }), ortlsServer.emit("connection", raw)) that is destroyed by a handshake failure leaves the wrappednet.Socketalive: it never emits'close',raw.destroyedstaysfalse, the acceptingnet.Serverkeeps it in its connection count (getConnections()reports 1) andserver.close(cb)never calls back. Node destroys the wrapped socket in every case (probe output below).'end'event (src/js/node/net.ts, thekCloseRawConnectionlistener installed at the three adoption sites), andSocket.prototype._destroytore down the wrapped transport only when it was not anet.Socket(!(upgraded instanceof Socket)). A handshake failure destroys the TLS socket with an error and never emits'end', so neither path ran.tls.connect({ socket })on a still-connecting socket only recorded the socket once it had connected, so destroying the TLS socket left the connect running and a failed connect surfaced as an unhandled error), and for a wrap running on the stream-level TLS engine.Fix
Socket.prototype._destroydestroys the transport recorded inkupgradedfor every wrap (destroyUpgradedTransportNT), and the'end'listeners are removed: a clean shutdown still ends indestroy(), so one path covers both the graceful and the error teardown.nextTickqueued after_destroy's callback, i.e. after the TLS socket's'error'has been queued. This keeps node's observable order: the TLS socket's'error'(and atls.Server's'tlsClientError') fire while the wrapped socket is still intact (itsremoteAddressis still readable, a listener that detaches handlers on the wrapped socket's'close'does not lose the error), then the wrapped socket's'close', then the TLS socket's'close'(which comes from its own handle close, asetImmediate). A TLS socket that no longer has a handle queues its own'close'behind the transport's teardown for the same reason (destroyUpgradedTransportAndCloseNT).kAdoptedTLSRaw), it is detached beforedestroy(): the twin shares the fd that the TLS socket's own handle close tears down, so the wrapped socket only runs its_destroybookkeeping ('close'on the next tick,server._connections--). Without the detach its_destroywould take the shared-fd arm and emit'close'two check phases later, after the TLS socket's. Any other transport (a generic duplex, a socket whose adoption has not run yet, a socket driving its own fd under the stream-level engine) still owns its fd and is destroyed outright, as the generic-duplex branch already did; for those the transport's'close'follows its handle close and may come after the TLS socket's (node emits it first), which the tests for these two cases deliberately do not pin.tls.connect({ socket })recordskupgradedbefore branching on whether the socket is still connecting (as the server wrap already did), so destroying the TLS socket at any point destroys the socket; the'connect'listener's own destroy of the socket becomes a plain early return.'close'reportshadError === false), matching node's_parentWrap.destroy().tls.connect({ socket })side with a_destroychange that predates the shared-fd twin handling and no longer applies to main. Its tests and the repro from its description pass on this branch (the wrapped socket's'close'fires anddestroyedistrue, same output as node); its caller-initiated teardown case is carried over below as thedestroy()case.Http2SecureServer.emit("connection", raw)goes through its own upgrade path in_http2_upgrade.ts, has the same leak, and is not touched here (reported separately).getConnections() === 0andserver.close()completing where anet.Serveris involved:test/js/node/tls/node-tls-server.test.ts, "destroying a server-side TLSSocket wrap destroys the wrapped net.Socket": handshake failure (the_closeAfterHandlingErrorpath),tls.Serverrejecting a client certificate on a connection injected viaemit('connection')(the plaindestroy(err)path, with'tlsClientError'observing the intact socket), destroy before adoption, stream-level engine over a handle-less socket.test/js/node/tls/node-tls-upgrade.test.ts, "destroying tls.connect({ socket }) destroys the wrapped net.Socket": non-TLS peer over a connected and over a still-connecting socket, certificate verification failure,destroy()while the socket is still connecting (against a port nothing listens on, so a leaked socket would also raiseECONNREFUSED),destroy()right after wrapping a connected socket (the adopted fd has to be closed, so the peer's'close'is awaited too), and a gracefulend()after the handshake (the path the removed'end'listener used to cover).net.tschange (the wrapped socket's'close'never appears in the log). Thedestroy()-after-wrapping and gracefulend()cases pass without it as well: they pin the two paths whose release this PR moves from the'end'listener and the native close into_destroy. The 6 error-path and connecting-socket tests also fail against the first revision of this PR, which ran the teardown synchronously at the top of_destroy(raw.destroyed=trueinside the error listeners, wrapped socket's'close'before the TLS socket's'error').test-tls-*.js(186 files) andtest-net-*.js(139), the vendoredtest-http(s)-proxy-*/*-proxy-*suites (48 files, the https agent'stls.connect({ socket })tunnel path),test/js/node/tls/,node-net.test.ts,node-http2-upgrade.test.mts,node-http-connect.test.ts,node-http-proxy-url.test.ts. The only failures are ones that fail identically without this change (localhost bind/connect family mismatch, sandbox proxy env vars, debug-build timeouts).Background
net.Socket's fd into a TLS socket natively (upgradeTLS) and hands back two handles over the same fd: the TLS handle, which becomes the TLS socket's_handle, and a raw twin, which replaces the wrapped socket's_handleand is taggedkAdoptedTLSRaw. Closing either one closes the shared fd; closing the TLS handle also retires the twin natively, so the twin never needs a close of its own. The wrapping socket records the wrapped one inkupgraded.upgradeDuplexToTLS) and the wrapped socket keeps driving its own fd._destroyemits a socket's'close'in one of two ways: from its handle close (asetImmediate, or a microtask followed by asetImmediatewhen_closeAfterHandlingErrorkeeps the handle around for the error listeners), or, for a socket without a handle, from anextTick. The stream layer emitsdestroy(err)'s'error'from anextTickqueued by_destroy's callback. Node'sTLSWrap.prototype.closedestroys its parent wrap when the TLSSocket's handle is closed and emits the TLSSocket's'close'once the parent has closed, which is where the ordering above comes from.net.Servercounts an accepted socket in_connectionsand decrements it in that socket's_destroy;server.close(cb)emits'close'(and callscb) only once the count reaches zero, which is why a wrapped socket that is never destroyed keepsserver.close(cb)from completing.Probe: standalone server wrap, node v26.3.0 vs bun (events after the client goes away)
ok: TLS client connects and disconnects after the handshake.reject: wrap created withrequestCert+rejectUnauthorizedand the client presents an unverifiable certificate (bun rejects on a standalone wrap, node leaves that to the application, so there is no node line for it).garbage: a plain TCP client writes non-TLS bytes.rawis the acceptednet.Socket,connectionsisrawServer.getConnections()beforerawServer.close().State seen inside the error listeners, node vs the fix (identical):
tls.Server+emit('connection')fed HTTP bytes, inside'tlsClientError':raw.destroyed=false raw.remoteAddress=127.0.0.1;tls.connect({ socket })against a non-TLS peer, inside the TLS socket's'error':plain.destroyed=false, followed by the plain socket's'close', then the TLS socket's.Further cases checked the same way (node's end state reached with the fix,
destroyed=false/ count stuck at 1 or, for the connecting case, an unhandledECONNREFUSEDwithout it): wrap destroyed synchronously after construction, wrap over a socket with unflushed writes (stream-level engine),tls.Serveremit('connection')with a rejected client, and on the client sidetls.connect({ socket })against a non-TLS peer, with a failing certificate verification, withdestroy()andresetAndDestroy()after the handshake, with the server ending the connection, anddestroy()while the socket is still connecting (to a listening and to a refused port).[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The failing SNICallback test bound a TLS server to the hostname localhost while the client independently resolved localhost for its connection, so on hosts where /etc/hosts lists both ::1 and 127.0.0.1 the client could pick a different address family than the server bound, producing an ECONNREFUSED that had nothing to do with SNI behavior. The fix makes the test connect to the literal address reported by server.address() and passes the bind hostname through the servername option instead. This removes the dependency on the host's name resolution order while still exercising the intended case…