Repository navigation
Conversation
|
Status: ready for review. Stacked on #42176, so the diff shows only the listener and its tests. Once #42176 lands this rebases onto main unchanged. Reproduced on bun 1.4.3 and on main with the scripts in the Notes table: The listener applies to user streams only. A |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one listener wired at the same attach point as the existing 'close' forwarder, routed through the existing _emitTLSError.
What was reviewed:
- Confirmed
_emitTLSErroralready exists onTLSSocket.prototype(tls.ts:858) and is used elsewhere in net.ts, so the client/server routing is inherited, not reimplemented here. - The new line matches the sibling
connection.on("close", ...)on the line above — same arrow style, same non-primordial.onon a user Duplex, consistent with the five other listeners in this helper. - Tests: added to the existing file,
it.concurrentsubprocess spawns withbunExe()/bunEnv/-e,Promise.alldrain, combined-objecttoEqualon exact event lists, no network, no sleeps; thelisteners:1anduncaught:capture make the unfixed build fail on the event list rather than a timeout. - No CODEOWNERS coverage on the touched paths; the fd-adoption arm is explicitly scoped out with a stated reason.
Extended reasoning...
Overview
The runtime change is a single line in src/js/node/net.ts inside attachUpgradedDuplex: connection.on("error", err => self._emitTLSError(err)), plus a two-line why-comment and an added Node source-line citation. This helper already wires five other listeners (close, data, end, drain, close) from a wrapped Duplex transport onto the TLS socket that upgraded it; the new listener is the missing 'error' forwarder. _emitTLSError is an existing TLSSocket.prototype method (tls.ts:858) that already implements Node's routing ('_tlsError' while the server owns the socket, 'error' once control is released), and it is already called from net.ts:1110, so this adds no new mechanism. The rest of the diff is ~90 lines of tests in the existing test/js/node/tls/node-tls-connect.test.ts.
Security risks
None identified. The change is in the TLS module but touches only error-event propagation from an already-failed transport — no certificate validation, handshake logic, key material, or rejectUnauthorized handling is altered. The pre-fix behavior (an unhandled 'error' on the transport escalating to uncaughtException and leaving https.request hung forever) is strictly worse than the post-fix behavior. The listener does not swallow or downgrade any error; it routes it to the same channel Node uses.
Level of scrutiny
Low-to-moderate. The production delta is one line that mirrors the immediately preceding line's shape and calls an existing, already-used helper. The behavior is pinned to Node v26.3.0 with source-line citations to internal/tls/wrap.js L976-977 and internal/js_stream_socket.js L65. The variant matrix in the tests (client vs server wrap, three failure timings, and the https.request layer on top) covers the sibling paths REVIEW.md asks for, and the PR description explicitly names and justifies the one excluded arm (fd-adoption) as a follow-up. The bug hunter ran to dry_streak with no findings and no ruled-out candidates.
Other factors
Test conventions are followed closely: existing test file, bunExe()/bunEnv with -e, it.concurrent for independent subprocess cases, Promise.all to drain stdout/stderr/exited, a single toEqual on { events, stderr, exitCode }, no ports, no external network, no sleep/setTimeout waits (failure is triggered via queueMicrotask/setImmediate inside the child and observed via process.on("exit")). The listeners:1 assertion and the in-process uncaughtException recorder mean the unfixed build fails on the asserted event list (listeners:0 / uncaught:EBOOM) rather than by hanging. .github/CODEOWNERS covers neither changed path. No prior reviews or outstanding objections appear in the timeline.
A TLS socket that runs on the stream-level engine over a user stream
(tls.connect({ socket: duplex }), new tls.TLSSocket(duplex, { isServer: true }))
attached no 'error' listener to that stream. A transport failure became a
process-level uncaughtException and the TLS socket never learned about it:
https.request() over such a socket never settled.
attachUpgradedDuplex now forwards the stream's 'error' through
_emitTLSError, like node's JSStreamSocket: a client socket reports it as
'error', a server wrap keeps it on '_tlsError', which a tls.Server turns
into 'tlsClientError'.
A net.Socket transport (TLS over TLS, a named pipe, queued plain writes) is
left as it is. Its close paths report a peer reset only when the socket has
an 'error' listener, so the forwarder would surface teardown resets that are
silent today (test-tls-inception.js on Windows and macOS).
27884d4 to
6fcfb63
Compare
|
Thanks for the review. One thing changed after the first review pass, so it is worth a second look. The first push attached the The current head (6fcfb63) attaches the listener only when the transport is not a The branch is also rebased onto the new head of #42176 (dc42eb4). |
…or fixture Folds in the cases from #42240 that this branch did not have. The fixture takes SIDE=client|server|https and also prints '_tlsError' and the 'close' hadError flag. Every expected list is what node v26.3.0 prints.
|
Closing in favor of #42235, which carries the same fix: an The cases that only this PR had are folded into #42235's fixture: a server wrap at both timings, |
Builds on #42176, which is merged. The base of this PR is `main`. Consolidates #42240 (the same fix). ### Problem - A TLS upgrade over a generic `Duplex` listens for the transport's `data`, `end`, `drain` and `close`, but not its `error`. So `transport.destroy(err)` after the upgrade throws `err` as an uncaught exception. Node emits it on the TLS socket. - `transport.listenerCount('error')` is 0 in bun and 1 in node v26.3.0. An `https.request` over such a socket kills the process and does not fail the request. ### Fix - `forwardUpgradedError` (`src/js/node/net.ts`) forwards the transport's `error` through `_emitTLSError`. The four stream-engine attach sites call it. A client sees `'error'`. A server wrap keeps it on `'_tlsError'` (`'tlsClientError'` on a `tls.Server`). The transport's `'close'` then ends the socket (#42176). - A `net.Socket` transport (a named pipe, unflushed writes, TLS over TLS) is excluded. Its close paths synthesize a read `ECONNRESET` as soon as anything listens for `'error'`. See Notes. - Verified: `test/js/node/tls/node-tls-connect.test.ts`, 5 new tests. One runs `node-tls-duplex-transport-error-fixture.ts` through `bunRun` (six wrap cases in one subprocess). All 5 fail on `main`. Also `test/js/node/tls` and 9 vendored `test-tls-*` tests. ### Background - A stream with no fd cannot use the kernel TLS path. `upgradeDuplexToTLS` runs a BoringSSL engine over the stream, driven by four native thunks on the stream's events. None is an error thunk. - Node wraps the stream in a `JSStreamSocket`, which re-emits the stream's `'error'`. `TLSSocket._init` forwards that error with `_emitTLSError`. - `_emitTLSError` already exists in `src/js/node/tls.ts`. It emits `'_tlsError'` always, and `'error'` only once `_releaseControl` has run. `tls.connect()` releases control at once, a server socket on `'secure'`. <details><summary>Notes</summary> #### Measurements Each case wraps a `Duplex`, then calls `transport.destroy(new Error("transport failed"))`. Events on the TLS socket (or the request), linux x64, debug+ASAN. The first six rows are `node-tls-duplex-transport-error-fixture.ts`, and the node column is v26.3.0 with the same fixture. The `https.request` row runs in the test process. | case | `main` (has #42176) | this branch | node v26.3.0 | | --- | --- | --- | --- | | `tls.connect({ socket })`, destroy in the same tick | uncaught `transport failed`, non-zero exit | `_tlsError`, `error`, `close:false` | same | | `tls.connect({ socket })`, destroy after the ClientHello | uncaught, non-zero exit | `_tlsError`, `error`, `close:false` | same | | server wrap, destroy in the same tick | uncaught, non-zero exit | `_tlsError`, `close:false` | same | | server wrap, destroy after the engine started | uncaught, non-zero exit | `_tlsError`, `close:false` | same | | `tlsServer.emit("connection", duplex)`, destroy in the same tick | uncaught, non-zero exit | `tlsClientError` with the `TLSSocket`, `close:false` | same | | `tlsServer.emit("connection", duplex)`, destroy after the engine started | uncaught, non-zero exit | `tlsClientError` with the `TLSSocket`, `close:false` | same | | `https.request` over the wrap | uncaught, non-zero exit | `req.error`, `req.close` with `req.destroyed === true` | same | A transport that errors without closing also matches node: the error reaches the TLS socket and the socket stays alive (`destroyed === false`). Two more cases from #42240, measured by hand on this branch (no test): | case | bun 1.4.3-canary.1 | this branch | node v26.3.0 | | --- | --- | --- | --- | | `tlsSocket.destroy()` over `Duplex.from({ readable, writable })` | uncaught `ABORT_ERR` | `error:ABORT_ERR`, `close:false` | same | | transport whose `write` calls back with an error, `s.write(data, cb)` queued | uncaught `EBOOM`, `cb` never called, no `'close'` | `error:EBOOM`, `cb(ERR_SOCKET_CLOSED)`, `close:false` | `error:EBOOM`, `cb(ECANCELED)`, `close:false` | The callback code in the last row differs because node cancels the queued write through `JSStreamSocket.doClose`. #35386 covers `ECANCELED` for cancelled TLS writes. #### Consolidation of #42240 #42240 carried the same listener with the same `net.Socket` guard. This branch keeps the test shape the review here asked for (`bunRun`, a fixture file, a one-line comment plus the node links). The cases that only #42240 had are now in the fixture: a server wrap at both timings, `https.request` over the wrap, `'_tlsError'`, and the `'close'` `hadError` flag. #42176 dropped the `attachUpgradedDuplex` helper that both PRs had patched. It now attaches the four thunks inline at each site. So the listener moved to `forwardUpgradedError`, which each site calls after it attaches the four thunks. #### Scope: every `net.Socket` transport is left out Two arms wrap a `net.Socket`, and neither gets the listener: - The fd-adoption arm. `tls.connect({ socket: connectedNetSocket })` with no queued writes hands the fd to a native TLS pair (`upgradeTLS`) and attaches nothing to the `net.Socket`. - The stream-level engine over a `net.Socket`: TLS over TLS, a named pipe, or a socket with queued plain writes. These reach `forwardUpgradedError` and the `instanceof Socket` check skips them. Node attaches the same listener in both, so `sock.destroy(err)` on such a transport is still uncaught in bun. The reason is one mechanism. `SocketEmitEndNT`, `SocketHandlers2.close`, `failWrite` and `ServerHandlers.error` turn a peer reset into `destroy(ECONNRESET)` only when the socket has an `'error'` listener, and stay silent otherwise. A forwarder counts as a listener. The first push of #42240 attached it to every transport. `test-tls-inception.js` (TLS over TLS, no `'error'` listener anywhere) then failed on Windows x64, Windows arm64 and macOS arm64 with an uncaught `read ECONNRESET`. On Windows x64 the base passed 5 of 5 runs, that push failed 5 of 5, and the narrowed listener passed 6 of 6. Those arms also have a second route to the same failure (the raw socket's own dispatch), so they need the de-duplication that #36534 and #38122 are about. `new tls.TLSSocket(stream)` with no `connect()` wraps nothing today, so nothing reaches it there either. That is #37664's subject. `_http2_upgrade.ts` has a fifth copy of the same four `.on()` calls. #38124 routes its transport error to the h2 session. #### Fixture startup The fixture runs its six cases in one subprocess and takes the key and the cert from `KEY` and `CERT`, which the test sets. An earlier shape ran five subprocesses at once, and each imported `harness`. A debug build needs about 1.5 s to load `node:tls` and about 1 s more for `harness`, so on a loaded machine (load average 150 on 16 cores) two of the five reached the 5 s test timeout. The one subprocess takes 2.1 to 2.8 s on the same machine. #### Suites `test/js/node/tls/node-tls-connect.test.ts`: 84 pass, 0 fail. `test/js/node/tls/`: 391 pass, 3 fail, none on the Duplex wrap path. `SNICallback runs even when the requested servername matches the bind hostname` binds `localhost` and connects to `127.0.0.1`, and fails the same way on the released binary in this container. `tls.Server socket destroySoon > delivers the whole stream when destroySoon follows end` (64 rounds of 2 MB) times out at 5 s on this debug build, and does the same with `main`'s `net.ts`. `root certificate initialization > concurrent Workers all see the same CA certificate lists` times out in some runs on this loaded machine. Vendored, all exit 0: `test-tls-inception`, `test-tls-js-stream`, `test-tls-connect-given-socket`, `test-tls-delayed-attach-error`, `test-tls-socket-failed-handshake-emits-error`, `test-tls-over-http-tunnel`, `test-tls-starttls-server`, `test-tls-destroy-stream`, `test-https-agent-create-connection`. </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: 7 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 [4.73ms] (pass) should thow ECONNRESET if FIN is received before handshake [534.04ms] (pass) initializes authorizationError to null in the TLSSocket constructor [6.87ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [120.38ms] (pass) should be able to grab the JSStreamSocket constructor [14.03ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [116.64ms] (pass) tls.connect > should have peer certificate when using self asign certificate [150.83ms] (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: 11 failed, 18 skipped bun test v1.4.3-canary.1 (367d939) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [0.03ms] (pass) should thow ECONNRESET if FIN is received before handshake [6.17ms] (pass) initializes authorizationError to null in the TLSSocket constructor [0.16ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [3.30ms] (pass) should be able to grab the JSStreamSocket constructor [0.21ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [5.48ms] (pass) tls.connect > should have peer certificate when using self asign certificate [4.83ms] (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) tls.c ... (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.79ms] (pass) should thow ECONNRESET if FIN is received before handshake [386.28ms] (pass) initializes authorizationError to null in the TLSSocket constructor [6.71ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [102.58ms] (pass) should be able to grab the JSStreamSocket constructor [15.93ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [97.47ms] (pass) tls.connect > should have peer certificate when using self asign certificate [122.29ms] (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 a ... (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 9098c05 features lto, baseline 23 deps, 136 codegen, 1176 objects in 977ms 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 codegen [2/1499] mkdir stamps [3/1499] install /workspace/bun bun install v1.4.3-canary.1 (367d939) Checked 26 installs across 65 packages (no changes) [15.00ms] [4/1499] install /workspace/bun/packages/bun-error bun install v1.4.3-canary.1 (367d939) Checked 1 install across 2 packages (no changes) [4.00ms] [5/1499] install /workspace/bun/src/node-fallbacks bun install v1.4.3-canary.1 (367d939) Checked 111 installs across 104 packages (no changes) [6.00ms] [6/1499] rustc unicode_ident [7/1499] gen node-fallbacks ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/node/net.ts | 10 ++++ test/js/node/tls/node-tls-connect.test.ts | 45 ++++++++++++++++- .../tls/node-tls-duplex-transport-error-fixture.ts | 58 ++++++++++++++++++++++ 3 files changed, 112 insertions(+), 1 deletion(-) ``` </details> **gate history** · 5 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/node/net.ts 13 12 41 test/js/node/tls/node-tls-connect.test.ts 3 4 39 …/js/node/tls/node-tls-duplex-transport-error-fixture.ts 1 3 44 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <robobun@bun.sh> Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
Stacked on #42176, which names this gap as its hand-off.
Problem
tls.connect({ socket: duplex }),new tls.TLSSocket(duplex, { isServer: true })) never hears that stream's'error'.transport.listenerCount("error")is 0 after the wrap (node: 1).transport.destroy(err)becomes anuncaughtException, andhttps.request()over such a socket never settles.attachUpgradedDuplex(src/js/node/net.ts:2303) wires only the engine's thunks. Node routes the stream's error:wrap.on('error', err => this._emitTLSError(err)).Fix
attachUpgradedDuplexaddsconnection.on("error", err => self._emitTLSError(err))when the transport is not anet.Socket._emitTLSErroralready exists intls.tswith node's routing: a client reports'error', a server wrap keeps it on'_tlsError'.'close'listener destroys the socket afterwards, so every case prints node v26.3.0's event list.net.Sockettransport is unchanged. Its close paths report a peer reset only whenlistenerCount("error") > 0, so the listener made silent teardown resets uncaught (test-tls-inception.js). See Notes.test/js/node/tls/node-tls-connect.test.ts("hears that transport's error", 5 cases, all fail on the base branch and on main). Alsotest/js/node/tls/, 245 vendoredtest-tls-*/test-https-*files, and the tls suites on Windows.Background
upgradeDuplexToTLS), fed by four native thunks on the stream's events._controlReleasedis node's ownership flag.tls.connect()releases control at once, a server socket on'secure'. Until then errors stay on'_tlsError', which atls.Serverturns into'tlsClientError'.Notes
node v26.3.0 vs this branch, same scripts
tls.connect({ socket }),transport.destroy(err)same tickuncaught:EBOOM, socket never closeserror:EBOOM,close:falseuncaught:EBOOMerror:EBOOM,close:falseuncaught:EBOOM_tlsError:EBOOM,close:falseuncaught:EBOOM_tlsError:EBOOM,close:falsehttps.requestover the wrapuncaught:EBOOM,req.destroyed === falsefor goodreq.error:EBOOM,req.closetlsSocket.destroy()overDuplex.from({ readable, writable })ABORT_ERRerror:ABORT_ERR,close:falsewritecalls back with an error,s.write(data, cb)queuedcbnever callederror,cb(ERR_SOCKET_CLOSED),close:falseerror,cb(ECANCELED),close:falseThe last row's callback code differs (node cancels the queued write through
JSStreamSocket.doClose). #35386 covers ECANCELED for cancelled TLS writes.Left out on purpose: every
net.SockettransportTwo arms wrap a
net.Socket, and neither gets the listener here:tls.connect({ socket: connectedNetSocket })with no queued writes hands the fd to a native TLS pair (upgradeTLS,net.tsaround lines 2028 and 2426) and attaches nothing to thenet.Socket.net.Socket: TLS over TLS, a named pipe, or a socket with queued plain writes. These go throughattachUpgradedDuplexand are skipped by theinstanceof Socketcheck.Node attaches the same listener in both, so
sock.destroy(err)on such a transport is still uncaught in bun. The reason is one mechanism.SocketEmitEndNT,SocketHandlers2.close,failWriteandServerHandlers.errorturn a peer reset intodestroy(ECONNRESET)only when the socket has an'error'listener, and stay silent otherwise. A forwarder counts as a listener. The first push of this PR attached it to every transport, andtest-tls-inception.js(TLS over TLS, no'error'listener anywhere) then failed on Windows x64, Windows arm64 and macOS arm64 with an uncaughtread ECONNRESET: the proxy's teardown reset on the inner socket, silent before, reached the outer socket. On Windows the base branch passes 5 of 5 runs, the first push failed 5 of 5, and this head passes 6 of 6.Covering these transports needs those four gates to look through the forwarder to the TLS socket's own listeners, and that changes what an HTTPS-proxy flow with an
'error'listener sees at teardown. That wants its own cross-platform validation. It overlaps #38122 (connect failure of a wrapped socket) and #36534 (tieToWrappedStream, which forwards withdestroy(err)rather than_emitTLSError). Tracked as a follow-up.Self-review
Concerns raised: conflicting helper with #42176 (stacked on it instead), server test loosened with
slice(0, 2)(now exact lists), no same-tickdestroy(err)case (added), wrong wrap.js citation (now L976-L977 and js_stream_socket.js L65), reach claim too broad (narrowed to user streams), fd-adoption arm uncovered (left out, above).Related open PRs
'close'destroys the TLS socket, plus the staged native close.new TLSSocket(socket)upgrade. It carries adestroy(err)forwarder for the duplex arm as a side change. The_emitTLSErrorrouting here is what node does, and it keeps a server wrap's transport error off the public'error'.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file