Repository navigation
node:tls: report a peer reset on a wrap of an accepted socket - #42293
Merged
Merged
Conversation
Since #39066 the wrapped socket's 'close' destroys the TLS socket. On an fd upgrade the two sockets share one fd. When a peer reset closes it, the native dispatch runs the wrapped socket's close callback, drains the tick queue (the wrapped socket's 'error' and 'close'), and only then runs the TLS socket's own close callback, which carries ECONNRESET. The destroy from 'close' won that race: the TLS socket closed with hadError false and no 'error', and a tls.Server lost 'tlsClientError'. The fd upgrade sites now arm onAdoptedClose. Between the two native close callbacks it defers the destroy one setImmediate, so the TLS socket's own close reports the reset first.
Collaborator
Author
|
Status
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughThe change adds deferred close handling for adopted TLS sockets and applies it to three TLS upgrade paths. Tests cover ChangesAdopted TLS close handling
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The added reset-ordering coverage validates the intended TLS error and close behavior without introducing an identified merge risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…waited promises A lost 'tlsClientError' or a wrap that closes before it gets data now fails the test at once with a message, not through the runner's timeout.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — the test rejection-path feedback from the earlier review is addressed in 080e7ee.
What was reviewed:
onAdoptedClose/destroyWhenAdoptedClosesmirror the adjacentonUpgradedClosepattern exactly (samekOnUpgradedCloseslot,.bind, baresetImmediateper file convention), so existing listener-removal paths still find the handler.- The
connection[kclosed] && !self[kclosed]gate only defers in the between-callbacks window; every other path falls through to the original synchronous destroy, so half-open teardown and the stream-engine sites (still ondestroyWhenUpgradedCloses) are unchanged. - Both new tests now reject their awaited resolvers on the relevant failure events (wrapped
closebefore data, rawclose+ bounded setImmediate turns), so a regression fails fast instead of timing out.
Extended reasoning...
Overview
The PR adds a 7-line helper pair (onAdoptedClose/destroyWhenAdoptedCloses) in src/js/node/net.ts and swaps three fd-upgrade call sites from destroyWhenUpgradedCloses to it. The helper defers the wrapped-socket-driven destroy() by one setImmediate only when connection[kclosed] && !self[kclosed] — the exact window between the two native close callbacks — so the TLS socket's own close callback can surface ECONNRESET first. Two describe.concurrent tests cover the new TLSSocket(raw, {isServer}) and tlsServer.emit('connection', raw) shapes.
Security risks
None. This is error-event ordering, not TLS verification, certificate handling, or crypto. rejectUnauthorized: false appears only on the test client connecting to the test's own self-signed server. No new user-controlled input is parsed.
Level of scrutiny
Moderate. net.ts is a hot-path builtin, but the change is tiny and structurally identical to the adjacent destroyWhenUpgradedCloses (same kOnUpgradedClose storage, .bind(null, ...), named module-level functions, bare setImmediate matching lines 259/263/2260). The four stream-engine upgrade sites are deliberately left on the original helper, and the PR description explains why. The gate is conservative: if the condition doesn't hold, behavior is byte-identical to before.
Other factors
My earlier inline comment (unwired rejection paths would make the tests hang on regression) was addressed in commit 080e7ee: gotData now rejects from the wrapped socket's close and both client-side error events; clientError rejects after a bounded 10-setImmediate window from the raw socket's close; injected rejects on the raw client's error. Tests use port: 0 via listen(), 127.0.0.1, describe.concurrent, and try/finally cleanup registered before assertions. No outstanding third-party objections in the timeline.
The previous commit deferred the destroy in net.ts when it saw the state between the two native close callbacks of an fd upgrade. Fix the ordering at its source instead and put net.ts back. on_close calls the raw twin's close handler as a full dispatch before the TLS socket's own close handler. That dispatch was an outermost enter()/exit() pair, so its exit drained the tick queue: the raw socket's 'error' and 'close' events ran ahead of the TLS socket's close handler, the one that gets the read error. on_close now holds the event loop entered across both handlers, so the queue drains after both have run. The TLS socket is already destroyed with ECONNRESET when the raw socket's 'close' listener looks at it. The order of the JS events does not change. CloseTeardown is created ahead of the scope so that it drops after it: the drain still comes ahead of the teardown, as for a socket with no twin.
Collaborator
Author
|
Cross-reference: #42487 changes |
Jarred-Sumner
deleted the
robobun/d14cf5ae/tls-wrap-peer-reset-error
branch
September 14, 2026 23:09
usrbinkat
pushed a commit
to usrbinkat/bun
that referenced
this pull request
Sep 15, 2026
…h#42293) ### Problem - A server-side wrap of an accepted socket (`new tls.TLSSocket(raw, { isServer: true })`, `tlsServer.emit('connection', raw)`) no longer reports a peer RST. The TLS socket emits `'close'` with `hadError === false` and no `'error'`. During the handshake the `tls.Server` gets no `'tlsClientError'`. Bun 1.4.2 and Node report ECONNRESET. - Regression from oven-sh#39066: the wrapped socket's `'close'` now destroys the TLS socket (`onUpgradedClose`, `src/js/node/net.ts:273`). The native `on_close` (`src/runtime/socket/socket_body.rs:2122`) runs the raw socket's close callback, drains the tick queue (raw `'close'`, so the destroy), and only then runs the TLS socket's own close callback with ECONNRESET. ### Fix - `on_close` holds the event loop entered (`EventLoop::enter_scope`) across both close callbacks of an `upgradeTLS` pair. The tick queue drains after both ran, not between them. - The TLS socket's own close callback now destroys it with ECONNRESET before the raw socket's `'close'` event runs. That listener then finds the socket destroyed. `net.ts` does not change. - The JS events keep the 1.4.2 order: `raw error, raw close, tls error, tls close`. - Verified: `test/js/node/tls/node-tls-socket-allow-half-open-option.test.ts`, 2 new tests, both fail on main's `src/`. Also `test/js/node/tls/`, `net/`, `http2/`, `test/js/bun/net/`, vendored `test-tls-*` and `test-https-*`. ### Background - A wrap is TLS over an existing socket. For a connected `net.Socket` Bun adopts the fd: `upgradeTLS` returns a raw handle, which stays on the wrapped socket, and a TLS handle for the TLS socket. - Only the TLS handle gets native events. Its `on_close` first calls the raw handle's close callback (the "twin") as a full dispatch of its own. - Each dispatch brackets its JS callback with `enter()` and `exit()` on the event loop. The outermost `exit()` drains `process.nextTick` and microtasks. The twin's dispatch was an outermost pair, so it drained before the TLS callback ran. <details><summary>Notes</summary> #### Scope The raw socket needs an `'error'` listener of its own for this to show, the usual STARTTLS server shape. Without one it is not destroyed on the reset, so it emits no `'close'`. `CloseTeardown` is now created ahead of the scope, so it drops after it. The drain still comes ahead of the teardown (`mark_inactive`, the final `deref`), as it does for a socket with no twin. The three early returns lose their explicit `drop(cleanup)` for the same reason: the natural drop order is the scope first, then the teardown. The change applies to every `upgradeTLS` pair, also the ones from `Bun.connect` and `socket.upgradeTLS()`. What moves is when the ticks queued by the raw socket's `close` handler run: after the TLS socket's `close` handler, not before it. An earlier revision of this PR deferred the destroy in `net.ts` with a `setImmediate` when it saw the state between the two callbacks. Review asked to fix the ordering at its source, which is this version. Since oven-sh#39066 a wrap takes `allowHalfOpen` from the wrapped socket. A half-open wrap does not end itself after EOF, so the wrapped socket's `'close'` must destroy it. This change keeps that. #### Measurements Linux x64. "main" is a debug+ASAN build of 81f97bb. "before" is bun 1.4.3-canary.1+4ff919377, which predates oven-sh#39066 and prints what 1.4.2 prints. Node is v26.3.0. The server wraps the accepted socket and also listens for `'error'` on it. The client does `resetAndDestroy()`. | case | node | before | main | this PR | | --- | --- | --- | --- | --- | | `new TLSSocket(raw)`, after the handshake | tls error ECONNRESET, raw close, tls close:true | raw error, raw close, tls error ECONNRESET, tls close:true | raw error, raw close, tls close:false | same as before | | `tlsServer.emit('connection', raw)`, after the handshake | same | same | same | same as before | | `new TLSSocket(raw)`, during the handshake | tls error ECONNRESET, raw close, tls close:true | raw error, raw close, tls error ECONNRESET, tls close:true | raw error, raw close, tls close:false | same as before | | `tlsServer.emit('connection', raw)`, during the handshake | tlsClientError ECONNRESET | tlsClientError ECONNRESET | nothing | tlsClientError ECONNRESET | `tls.connect({ socket })` over a `net.Socket` it dialed itself was not affected. Its raw socket keeps its handle through the close callback, so `_destroy` takes the `kAdoptedTLSRaw` branch and `'close'` comes two check phases later (`closeAdoptedTLSRawNT`). An accepted socket's callback (`ServerHandlers.close`) detaches the handle first, so `'close'` comes from `process.nextTick`. #### The trace `BUN_DEBUG_Socket=1 BUN_DEBUG_JS=1` on main, after the client's RST: ``` [socket] onClose C <- TLS handle [socket] onClose S <- raw handle, dispatched from inside the first [net] Bun.Server close <- raw socket's callback: destroy(ECONNRESET) [events] Socket.emit error [events] Socket.emit close <- onUpgradedClose: tlsSocket.destroy() [net] Socket.prototype._destroy [net] Bun.Server close <- TLS socket's callback, socket already destroyed ``` #### Relation to oven-sh#42176 oven-sh#42176 now carries the other face of the oven-sh#39066 regression: a TLS socket over a `stream.Duplex` that loses unread data when the duplex closes. That is the stream-engine path and it changes `onUpgradedClose`. This PR touches neither that function nor `net.ts`, so the two do not conflict. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 2 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-socket-allow-half-open-option.test.ts bun test v1.4.3 (4ff9193) test/js/node/tls/node-tls-socket-allow-half-open-option.test.ts: (pass) TLSSocket allowHalfOpen > new TLSSocket(socket) takes the wrapped socket's allowHalfOpen, ignoring the option [228.65ms] (pass) TLSSocket allowHalfOpen > without a socket to wrap, the option is honored [10.30ms] (pass) TLSSocket allowHalfOpen > tls.connect({ socket }) takes the given socket's allowHalfOpen, ignoring the option [207.34ms] (pass) TLSSocket allowHalfOpen > over a connection > a server-side wrap of a half-open socket stays writable after the peer ends [946.31ms] (pass) TLSSocket allowHalfOpen > over a connection > a server-side wrap of a regular socket ends itself after the peer ends, even with allowHalfOpen: true [836.51ms] (pass) TLSSocket allowHalfOpen > over a connection > a socket injected into a tls.Server keeps its own allowHalfOpen [845.82ms] (pass) TLSSocket allowHalfOpen > over a connection > tls.connect({ socket }) over a half-open socket stays writab ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (00fab6991) test/js/node/tls/node-tls-socket-allow-half-open-option.test.ts: (pass) TLSSocket allowHalfOpen > new TLSSocket(socket) takes the wrapped socket's allowHalfOpen, ignoring the option [8.83ms] (pass) TLSSocket allowHalfOpen > without a socket to wrap, the option is honored [0.16ms] (pass) TLSSocket allowHalfOpen > tls.connect({ socket }) takes the given socket's allowHalfOpen, ignoring the option [4.39ms] (pass) TLSSocket allowHalfOpen > a peer reset under a server-side wrap > a socket injected into a tls.Server: a reset during the handshake is a 'tlsClientError' [20.27ms] (pass) TLSSocket allowHalfOpen > the wrapped socket closing > new TLSSocket(duplex, { isServer }): the duplex closing [36.67ms] (pass) TLSSocket allowHalfOpen > over a connection > a server-side wrap of a half-open socket stays writable after the peer ends [48.42ms] (pass) TLSSocket allowHalfOpen > over a connection > a server-side wrap of a regular socket ends itself after the peer ends, even with allowHalfOpen: true [44.45ms] (pass) TLSSocket allowHalfOpen > over a connection > a socket injected into a tls.Server keeps its own allowHalfOpen [44.65ms] (pass) TL ... (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-socket-allow-half-open-option.test.ts bun test v1.4.3 (4ff9193) test/js/node/tls/node-tls-socket-allow-half-open-option.test.ts: (pass) TLSSocket allowHalfOpen > new TLSSocket(socket) takes the wrapped socket's allowHalfOpen, ignoring the option [215.09ms] (pass) TLSSocket allowHalfOpen > without a socket to wrap, the option is honored [9.47ms] (pass) TLSSocket allowHalfOpen > tls.connect({ socket }) takes the given socket's allowHalfOpen, ignoring the option [206.47ms] (pass) TLSSocket allowHalfOpen > over a connection > a server-side wrap of a half-open socket stays writable after the peer ends [953.62ms] (pass) TLSSocket allowHalfOpen > over a connection > a server-side wrap of a regular socket ends itself after the peer ends, even with allowHalfOpen: true [840.71ms] (pass) TLSSocket allowHalfOpen > over a connection > a socket injected into a tls.Server keeps its own allowHalfOpen [839.34ms] (pass) TLSSocket allowHalfOpen > over a connection > tls.connect({ socket }) over a half-open socket stays writabl ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 692ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/6] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [1/6] 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 Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compiling�[0m bun_paths v0.0.0 (/workspace/bun/src/paths) �[1m�[92m Compiling�[0m b ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/socket/socket_body.rs | 21 +++--- .../node-tls-socket-allow-half-open-option.test.ts | 77 ++++++++++++++++++++++ 2 files changed, 89 insertions(+), 9 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/socket/socket_body.rs 4 1 23 …node/tls/node-tls-socket-allow-half-open-option.test.ts 4 4 23 ``` </details> <!-- robobun:evidence:end -->
This was referenced Sep 22, 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>
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
new tls.TLSSocket(raw, { isServer: true }),tlsServer.emit('connection', raw)) no longer reports a peer RST. The TLS socket emits'close'withhadError === falseand no'error'. During the handshake thetls.Servergets no'tlsClientError'. Bun 1.4.2 and Node report ECONNRESET.'close'now destroys the TLS socket (onUpgradedClose,src/js/node/net.ts:273). The nativeon_close(src/runtime/socket/socket_body.rs:2122) runs the raw socket's close callback, drains the tick queue (raw'close', so the destroy), and only then runs the TLS socket's own close callback with ECONNRESET.Fix
on_closeholds the event loop entered (EventLoop::enter_scope) across both close callbacks of anupgradeTLSpair. The tick queue drains after both ran, not between them.'close'event runs. That listener then finds the socket destroyed.net.tsdoes not change.raw error, raw close, tls error, tls close.test/js/node/tls/node-tls-socket-allow-half-open-option.test.ts, 2 new tests, both fail on main'ssrc/. Alsotest/js/node/tls/,net/,http2/,test/js/bun/net/, vendoredtest-tls-*andtest-https-*.Background
net.SocketBun adopts the fd:upgradeTLSreturns a raw handle, which stays on the wrapped socket, and a TLS handle for the TLS socket.on_closefirst calls the raw handle's close callback (the "twin") as a full dispatch of its own.enter()andexit()on the event loop. The outermostexit()drainsprocess.nextTickand microtasks. The twin's dispatch was an outermost pair, so it drained before the TLS callback ran.Notes
Scope
The raw socket needs an
'error'listener of its own for this to show, the usual STARTTLS server shape. Without one it is not destroyed on the reset, so it emits no'close'.CloseTeardownis now created ahead of the scope, so it drops after it. The drain still comes ahead of the teardown (mark_inactive, the finalderef), as it does for a socket with no twin. The three early returns lose their explicitdrop(cleanup)for the same reason: the natural drop order is the scope first, then the teardown.The change applies to every
upgradeTLSpair, also the ones fromBun.connectandsocket.upgradeTLS(). What moves is when the ticks queued by the raw socket'sclosehandler run: after the TLS socket'sclosehandler, not before it.An earlier revision of this PR deferred the destroy in
net.tswith asetImmediatewhen it saw the state between the two callbacks. Review asked to fix the ordering at its source, which is this version.Since #39066 a wrap takes
allowHalfOpenfrom the wrapped socket. A half-open wrap does not end itself after EOF, so the wrapped socket's'close'must destroy it. This change keeps that.Measurements
Linux x64. "main" is a debug+ASAN build of 81f97bb. "before" is bun 1.4.3-canary.1+4ff919377, which predates #39066 and prints what 1.4.2 prints. Node is v26.3.0. The server wraps the accepted socket and also listens for
'error'on it. The client doesresetAndDestroy().new TLSSocket(raw), after the handshaketlsServer.emit('connection', raw), after the handshakenew TLSSocket(raw), during the handshaketlsServer.emit('connection', raw), during the handshaketls.connect({ socket })over anet.Socketit dialed itself was not affected. Its raw socket keeps its handle through the close callback, so_destroytakes thekAdoptedTLSRawbranch and'close'comes two check phases later (closeAdoptedTLSRawNT). An accepted socket's callback (ServerHandlers.close) detaches the handle first, so'close'comes fromprocess.nextTick.The trace
BUN_DEBUG_Socket=1 BUN_DEBUG_JS=1on main, after the client's RST:Relation to #42176
#42176 now carries the other face of the #39066 regression: a TLS socket over a
stream.Duplexthat loses unread data when the duplex closes. That is the stream-engine path and it changesonUpgradedClose. This PR touches neither that function nornet.ts, so the two do not conflict.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file