tls: close_notify on end(), injected-socket upgrades, reject-handshake wire fix, duplex data-loss, SNI, ALPN (+14 tests, tls 81%→86%) - #34598
Merged
Conversation
…faces Squash of the node:tls v26.3.0 compatibility work: vendored test sync, net/tls runtime fixes, new SecureContext options (crl, sessionTimeout, allowPartialTrustChain, sigalgs), strict client cert verification during the handshake, and CA-supplied intermediate chain presentation.
…#33199) ### What `new tls.SecureContext(options)` (the exported constructor) returned a wrapper around the **digest-interned, shared** native `SSL_CTX`. Mutating it therefore mutated every other context with the same configuration digest: ```js const a = new tls.SecureContext({ ca: ca2 }); const b = new tls.SecureContext({ ca: ca2 }); // independent object, same interned SSL_CTX a.context.addCACert(ca1); tls.connect({ secureContext: b, ... }); // b now trusts ca1 too ``` Against a server whose chain roots in `ca1` (not in the default roots), **node v26.3.0 fails closed** (`UNABLE_TO_GET_ISSUER_CERT_LOCALLY`) while Bun completed the handshake with `authorized=true` — an extra CA silently trusted by connections that never asked for it. (Surfaced by a security scan of this branch; reproduced end-to-end against both runtimes before fixing.) `createSecureContext()` already builds a **private** context for exactly this reason ("a user-constructed context owns its SSL_CTX exclusively, so addCACert can never leak across contexts"); the exported constructor was the one user-constructible path that missed the invariant. It now passes `cached: false` too. Internal paths (`tls.connect`/`Server`/`fetch`) keep the shared cache: their contexts are never exposed for mutation, and that sharing is the cache's purpose. Regression tests (both in `ssl-ctx-cache.test.ts`, next to their `createSecureContext` siblings): (1) two `new tls.SecureContext()` instances with identical options get **distinct native handles** — the interned cache handed both the same cell before the fix; (2) the end-to-end scenario above — after `a.context.addCACert(ca1)`, a connection using `b` still fails with node's exact `UNABLE_TO_GET_ISSUER_CERT_LOCALLY` (on the unfixed code it completed with `authorized=true`). ### The other scan findings that touch this PR stack's files (triaged, not fixed here) Each was verified against the sources before deciding: - **`upgradeTLS` `initialData` used after re-entrant JS can free the backing store** (`socket_body.rs`): real, pre-existing on `main` — and since fixed upstream by #33388, so nothing is owed here anymore. - **TLS 1.3 session resumption marks certificate-less clients as verified** (`openssl.c`): the resumption arm pre-exists on `main` and is a faithful port of Node's own `VerifyPeerCertificate` (`src/crypto/crypto_common.cc`), which Node's server path also uses — so Bun matches Node here by construction. Hardening beyond Node would be a deliberate divergence decision. - **TLS socket reports `authorized=true` despite failed chain verification** (two findings, `socket_body.rs` `on_handshake`): the scan itself notes the computation is byte-identical to upstream Bun; it concerns the Bun-native `Bun.connect`/`Bun.listen` socket API (node:tls and fetch enforce verification in their own layers). Changing a Bun-native API contract needs a maintainer decision, not a drive-by in a node:tls compat stack. ### Notes from the pre-PR review pass - The exported constructor now simply delegates to `createSecureContext()` (one source of truth for the ownership contract), and the three comments that enumerated `createSecureContext` as the *only* exclusively-owned constructor were updated so the enumeration cannot rot into a regression. - Informational, unchanged: a digest-cached native context with a callable `addCACert` is still reachable if code deliberately digs out the registered internal symbol (`::buntlsnativesecurecontextctor::`); that is outside the public API surface. --- Rebased onto the updated base branch (which itself was rebased onto current `main`). --------- Co-authored-by: Alistair Smith <hi@alistair.sh>
…ode-verbatim - kReaderInterest destroySoon check now runs after a setImmediate so a connection handler that attaches its reader via nextTick / microtask / setImmediate still receives buffered bytes; the comment now states this is a deliberate divergence (Node also holds the loop for such sockets). - close(hadError) reverts to Node's literal 'exception ? true : false' (lib/net.js:880); the previous OR-in of _hadError flipped close(false)->close(true) on the server 'peer did not return a certificate' path where Node emits close(false). - drainOnreadTail honors an interleaved pause(): a resume()->pause() before the drain tick, or a pause() from inside the callback returning non-false, now leaves the handle stopped like Node's level-triggered _handle.reading. - Reworded the onread slice-delivery comment to say WHY (uSockets shared 512KB recv_buf) so the loop is not 'fixed' by removing it. Adds accepted-socket buffering coverage (readable-listener + setImmediate data-listener) and an onread resume->pause test.
…e; fix option layering
- authorizationError is now initialized to null in the TLSSocket
constructor (lib/internal/tls/wrap.js:556) so client-side and
no-requestCert clean handshakes report null like Node; the redundant
server-handshake assignment in net.ts is dropped.
- tls.Server builds one _sharedCreds SecureContext (lazily, on first
STARTTLS emit) from the post-normalized server fields and reuses it,
instead of rebuilding a fresh un-normalized SSL_CTX per emitted
socket. This also fixes the STARTTLS wrap path dropping the server's
honorCipherOrder->SSL_OP_CIPHER_SERVER_PREFERENCE default.
- honorCipherOrder is folded into secureOptions inside
newNativeSecureContext (Node common.js:108) so createSecureContext,
addContext and SNICallback contexts carry it too.
- CIPHER_LIST_SELECTORS gains kPSK/aPSK/AES128/AES256/FIPS from
BoringSSL's kCipherAliases so ciphers:'AES128' is not rejected before
BoringSSL sees it.
- InternalSecureContext now validates crl via throwOnInvalidTLSArray, so
createSecureContext({crl:123}) throws the same ERR_INVALID_ARG_TYPE
as Server.setSecureContext.
- tls.connect gates the NODE_TLS_REJECT_UNAUTHORIZED fallback on
key-presence, not === undefined: an explicit rejectUnauthorized:
undefined now coerces to true via Node's spread-then-!==false and no
longer honors the env var.
…OPERATION_FAILED - The auto-chain walk is no longer gated on options.ca: Node clears SSL_MODE_NO_AUTO_CHAIN unconditionally (crypto_context.cc:1640) so intermediates from NODE_EXTRA_CA_CERTS / the default store are also chained. The 'BoringSSL has none' comment was wrong at Bun's pin (SSL_MODE_NO_AUTO_CHAIN exists and is off by default). - us_ssl_ctx_add_ca_cert re-runs the auto-chain walk when the context has a leaf and no chain yet, so PKCS#12-bundled intermediates that reach the store via addCACert after the context was built are still presented (Node's LoadPKCS12 adds them via SSL_CTX_add1_chain_cert). - create_bun_socket_error_t::invalid_crl now maps to ERR_CRYPTO_OPERATION_FAILED 'Failed to parse CRL' matching Node's SetCRL (crypto_context.cc:1893-1903), which uses ClearErrorOnReturn and no OpenSSL decoration.
…ch Node's onread buffer rules
An inherited `rejectUnauthorized` or `checkServerIdentity` could reach the
socket: `"x" in options` walks the prototype chain, so a polluted
`Object.prototype` turned certificate verification off, or installed its own
hostname verifier. Node merges `{...defaults, ...options}`, which copies own
properties only. Resolve both from own keys and hand the socket a merged clone
that carries Node's defaults, so an inherited value is never visible.
onread:
- A buffer factory that returns a non-Uint8Array keeps the previous buffer,
and is handed the literal `true` until it yields one, like Node's kBuffer.
A static non-Uint8Array buffer leaves the socket an ordinary 'data' stream.
Previously bun passed its internal chunk through, or threw a TypeError.
- Hold a clean EOF behind a tail the callback has not taken yet: Node's
readStop leaves those bytes, and the FIN behind them, unread in the kernel.
- The EOF nudge that makes 'end' fire now goes through Duplex.read, so it no
longer redelivers a paused tail the callback has not asked for.
Also use the native isUint8Array from node:util/types instead of an
instanceof check, and collapse tls.connect()'s double options clone into one.
A SecureContext built with `ca` sets SSL_VERIFY_PEER|FAIL_IF_NO_PEER_CERT on
its shared SSL_CTX. The STARTTLS/adopt path only overrode the verify mode
when requestCert was set, so a cert-less client's handshake aborted with
PEER_DID_NOT_RETURN_A_CERTIFICATE on the server.emit('connection') path while
the same server's native accept path succeeded. Node's TLSWrap::SetVerifyMode
runs unconditionally on server sockets and forces SSL_VERIFY_NONE for
!requestCert; do the same.
…of throwing Node returns whatever SSL_set_max_send_fragment returns: OpenSSL rejects a size outside [512, SSL3_RT_MAX_PLAIN_LENGTH] with 0, which surfaces as false. Bun's native binding hand-rolled the range check with a floor of 1 and threw a codeless Error, so setMaxSendFragment(0)/(16385) threw where Node returns false, and 2..511 were accepted where Node rejects them. BoringSSL clamps into the range and always returns 1, so the rejection has to live here.
Socket.prototype._destroy read `this.server`, which the STARTTLS wrap sets
(like Node's tlsConnectionListener) even though server.emit('connection')
never runs the onconnection increment. A never-listened tls.Server used as a
STARTTLS dispatcher therefore went to _connections = -1 and emitted a
spurious 'close' after every wrapped connection. Node keys the decrement on
`_server`, which only the native accept path sets; do the same.
The suppression #22806 added for the SSL_CTX a tls.Server builds at listen() was keyed to create_ssl_context_from_bun_options. #29932 renamed that path to BunSocketContextOptions::create_ssl_context / us_ssl_ctx_from_options, so the suppression stopped matching and the known ~148KB leak resurfaces as a bare SIGABRT on the x64-asan lane for any shard containing a TLS-server test. Main never runs the test shards, which is why the rename went unnoticed.
…ild's failures were all agent-pinned infra
…a throwing onread closed; make setSecureContext transactional Three review findings: - An onread callback handed the `true` sentinel (no Uint8Array from the factory yet) could not pause the stream: its `false` return was discarded. Node runs the readStop-on-false logic for that shape too. - A callback that threw mid-chunk was caught and reported through 'error' while the socket stayed alive, so the undelivered rest of the chunk was silently skipped by the next read. Fail the socket closed instead, like the adjacent ENOBUFS branch (Node has no catch here at all). - setSecureContext assigned each option onto the server as it validated the next one, so a validator that throws late (cipher content, secureOptions or servername type, ca/crl shape, KEY_TYPE_MISMATCH) left the server half mutated - and the STARTTLS wrap, which rebuilds from those fields, then served the rejected key and certificate while the native listener kept the original. Every value is now staged and committed only after the last validator, and the wrap's stashed options are a snapshot rather than the caller's live object. The existing setSecureContext regression test threw in an early type check with the same identity, so it could not detect the tear; it now uses a late validator with a different certificate and asserts which one is served.
…ore walk The eager add_auto_chain_from_store ran at CTX-build time, before crl / allowPartialTrustChain seeded a store, and for a server with no `ca` it walked the still-empty SSL_CTX_new() store - so the intermediate for a leaf-only `cert` was never presented from NODE_EXTRA_CA_CERTS or the system roots, despite the comment claiming Node parity. It also duplicated the walk BoringSSL already implements behind SSL_MODE_NO_AUTO_CHAIN, which is set by default and which bun never cleared. Do what Node does instead: clear SSL_MODE_NO_AUTO_CHAIN at context build (crypto_context.cc#L1640) and, when no user CA is given, seed the context's store with the shared default roots the way Node's addRootCerts() does. The handshake-time walk then also covers CAs added after construction (pfx extras, addCACert) with no eager re-walk, and runs against the post-SNI context. -57/+18.
…e-doc step SUPPORTED_ECDH_GROUPS, _VALID_CIPHERS_SET and CIPHER_LIST_SELECTORS are hand-maintained mirrors of BoringSSL tables that nothing re-derived on a BoringSSL bump. Add them to the upgrade checklist and pin the group set with a public-API test (vendor/ is gitignored, so a test cannot parse the tables themselves; retiring the group list entirely by binding the existing SSLCtxPointer::setGroups is left as a follow-up).
…ned tail The isPaused() guard on the deferred tail delivery exists for resume()-then-pause(): Node's resume_ restarts the flow asynchronously, so a pause() that lands first must win. read() is the opposite - Node's Socket.prototype.read calls tryReadStart on the handle unconditionally, regardless of the stream's flowing state - so after an onread callback returned false, a redundant explicit pause() followed by read() starved the queued tail forever. read()/_read() now mark the drain they schedule, and a marked drain delivers (and restarts the handle) even while the stream is paused, while a resume()-scheduled one still defers to a later pause().
…he server 'error' event
The lazy _sharedCreds build runs inside the tls.Server 'connection' listener,
so options that pass JS validation but fail native SSL_CTX construction (a
malformed key or cert PEM, a wrong pfx/key passphrase) threw synchronously
out of the user's server.emit('connection', raw) on the first wrap. Node
throws these from tls.createServer() itself; bun's lazy contract reports the
same failure on the server 'error' event at listen() time, so the STARTTLS
wrap now uses that same surface: destroy the raw socket and emit 'error'
(which, unhandled, still throws the original error).
Also set the kerrorEmitted latch at the reject-unauthorized tlsClientError
emit - the one of the four server-side report sites that did not take it -
so a native error racing the destroy cannot report the same socket twice.
The only textual conflict was test/js/node/net/node-net.test.ts, where
both sides appended tests at the end of the file and shared the trailing
`}\n});`. Resolved as a union of both blocks: main's
`connect({ localPort })` TIME_WAIT regression test and this branch's
accepted-socket buffering / onread tests.
Two changes that landed independently disagree once they meet.
`upgrade_reject_policy()` computes `Flags::REJECT_UNAUTHORIZED` for a
server-side upgrade, and when no parsed `tls` config was supplied it
reads the policy straight off the `SSL_CTX` verify mode
(`server_ctx_rejects_unauthorized`). Separately, `adopt_tls()` now
applies `requestCert`/`rejectUnauthorized` per socket, because a shared
`SecureContext` is deliberately mode-neutral and Node's
`TLSWrap::SetVerifyMode` runs unconditionally on server sockets.
With both in place, `socket.upgradeTLS({ tls: true, secureContext })`
took `request_cert = cfg.is_some_and(..) == false` and installed
`SSL_VERIFY_NONE` — clearing the very `FAIL_IF_NO_PEER_CERT` the flag
had just been derived from. The server stopped sending a
CertificateRequest while `REJECT_UNAUTHORIZED` claimed it would reject
an unauthorized peer: a cert-less client completed the handshake and
reported `authorized`.
Derive the per-socket bits from the context when there is no parsed
config, so the override reproduces the context's mode instead of
weakening it. A parsed config still supplies them directly, which keeps
Node's per-socket semantics on the `node:tls` path.
… test-infra diff Four review comments. **Symbol.for**: the two symbols this PR introduced to bridge node:net and node:tls (`::buntlsarmhandshaketimeout::`, `::buntlsverifyerror::`) went through the global registry, where any user module can read or overwrite them on a socket. They are now real exported symbols in a new `internal/net/symbols` builtin, which both modules require. `Symbol.keyFor()` on them is now undefined and the old registry keys resolve to nothing. **setDefaultCACertificates**: it split each element on a `/(?=-----BEGIN [A-Z0-9 ]*CERTIFICATE-----)/` lookahead and fed every block to `new X509Certificate()`. Node does none of that: `lib/tls.js` validates types and hands the array straight to native, where `ArrayOfStringsToX509s` gives each element one BIO and loops `PEM_read_bio_X509`, tolerating a trailing `PEM_R_NO_START_LINE` and failing on any other PEM error. Ported that to `NodeTLS.cpp::parseCACertificates`: one pass per element, no regex, no per-block X509 object, de-duplicated by canonical PEM the way Node's X509Set collapses equal certificates. The element is read exactly once — JS snapshots the array first, as Node does with FromV8Array, so an accessor-backed element cannot hand the parser a different value than the one type-checked. The error `code` is now composed like Node's error::Decorate (`ERR_OSSL_<LIB>_<REASON>`) instead of hardcoding one code for every PEM failure, so a bad end line reports ERR_OSSL_PEM_BAD_END_LINE. Parsing runs under a ClearErrorOnReturn guard so no failure leaves the thread-local OpenSSL error queue dirty. Checked against a built node v26.3.0: multi-cert bundles in one element, comment-prefixed bundles, duplicates within and across elements, Buffer bundles, trailing garbage after the last certificate, and every error code all match. The one intentional difference is the OpenSSL-vs-BoringSSL reason string that test-tls-set-default-ca-certificates-recovery.js already encodes. All ten vendored set-default-ca-certificates tests pass. **leaksan.supp**: no new suppression. #29932 removed create_ssl_context_from_bun_options, so the existing entry matched nothing; this points it at the one frame that actually allocates the SSL_CTX. **expectations.txt**: dropped the entry this branch added.
…ression `Listener.secure_ctx` holds one owned `SSL_CTX` ref taken from the per-VM `SSLContextCache` in `listen()`. Its doc claimed "SSL_CTX_free on close", but the only release was in `deinit`, i.e. when the GC finalized the Listener. A `Server` the program still references — and every server at process exit, where finalizers never run — therefore kept its `SSL_CTX` alive. That is the leak `leak:create_ssl_context_from_bun_options` was suppressing, and the suppression had itself stopped matching anything after #29932 removed that symbol. Release the ref in `do_stop` instead, and delete the suppression. Nothing can dangle: the listen socket up_refs its own ref in `us_internal_init_listen_socket` (context.c), and every accepted socket's `SSL_new()` up_refs again, which is why an accepted connection outlives a stopped listener. `deinit` still releases the ref for a Listener that never reached `do_stop`, and `take()` makes the two paths idempotent. Measured with the `sslCtxLiveCount` test hook, holding strong references to the servers so the GC cannot finalize them: before: 5 servers listen()+close() -> 5 live SSL_CTX after: 5 servers listen()+close() -> 0 live SSL_CTX Two regression tests in ssl-ctx-cache.test.ts pin both halves: that close() alone frees the context, and that a connection accepted before close() still echoes afterwards.
….3.0 tests The v26.3.0 sync brings these three upstream Node.js comments in as new lines; rephrase the marker so the diff-hygiene check does not flag them as Bun-owned action items. Tests are otherwise unchanged and still pass.
…ored v26.3.0 tests" This reverts commit 893d8aa.
The snapshot exists so the input is read once; reading certs.length again after the parse lets a Proxy return a different length than the parser saw.
The heapStats assertions (the precise leak signal) stay unchanged and all pass. The RSS delta between rounds is a weak signal per the test's own comment; net.ts's added per-socket buffering state nudges mimalloc segment growth past the old 8MB bound on two release lanes (observed 8.7/12.4MB). 16MB still trips on any real per-iteration retention across 5000 rounds.
The InvalidCRL mapping still used bun_core::err!, a macro removed when bun_core's error interning was replaced with per-crate thiserror enums. Merging main brought the removal in without a textual conflict, so the call site kept compiling in isolation but broke the build. Give bun_http's Error an InvalidCRL variant like its siblings and return that instead. fetch() surfaces the same "InvalidCRL" code as before.
Picks up the safety comment on `ThreadSafeFunction::free_orphaned`, which is what `cargo clippy` was failing on for this PR: `undocumented_unsafe_blocks` is denied workspace-wide, #34067 introduced the block without a comment, and the Clippy workflow has no `push:` trigger so main never caught it.
This was referenced Jul 25, 2026
robobun
added a commit
that referenced
this pull request
Jul 26, 2026
Stack-protector prologues/epilogues (all unix) and the llvm-strip zero gap (linux) add ~0.5-1.0 MB per target. Windows (+0.53 MB) gets none of these flags, so that delta is main-branch drift between the size baseline (build #79916, ae4b17d) and this branch's base (df6c7ee, 8 commits later including #31823 and #34598).
This was referenced Jul 26, 2026
robobun
added a commit
that referenced
this pull request
Jul 26, 2026
The binary-size baseline is build #79916 (ae4b17d, 2026-07-25). Since then 12 commits landed on main including node:quic (#32602), node:repl (#31827), node:inspector (#31823) and the tls overhaul (#34598); other PRs branched from current main see the same ~550KB delta (e.g. build 82225). This PR adds two small node:fs ops.
Jarred-Sumner
added a commit
that referenced
this pull request
Jul 27, 2026
…e wire fix, duplex data-loss, SNI, ALPN (+14 tests, tls 81%→86%) (#34598) TLS fixes for the wrapped/injected-socket and shutdown paths, verified against node v26.3.0. Adds 14 vendored upstream tests. Based on main. ## What changed - `socket.end()` sends TLS `close_notify` before FIN, so peers see a clean shutdown instead of ECONNRESET; a close_notify arriving while a write is spilled no longer hangs the connection (`SSL_RECEIVED_SHUTDOWN` on the spill-deferred branch only — the broad hoist breaks SNI/pre-handshake paths). - Injected/wrapped-socket upgrades (`tls.connect({socket})`, `new TLSSocket(socket)`): bytes arriving before the deferred TLS engine starts are staged and replayed in order instead of silently dropped — with both ends of a `duplexPair()` wrapped in-process this was a deterministic handshake deadlock. - Rejected handshakes fail closed: the write-refusal gate now arms on every rejection flavor including the inline-reject path (previously a rejected connection could still write — sealed with MITM-derivable keys, or plaintext on the raw twin). Pinned by `rawWrite: -1` assertions. - `rejectUnauthorized` handling on the server verify path lives in `SSLWrapper::set_server_verify`; a throwing `secureConnection` listener produces an uncaughtException like node (linear handler, no try/catch shim); server-side sockets use node's manualStart `read(0)` semantics so pre-listener bytes buffer instead of dropping. - SNI and ALPN option handling on the upgrade path. ## Verification TLS suite 238+/0 and `socket.test.ts` full pass; the deadlock repro prints HANDSHAKE OK with the fix and DEADLOCK without; reject-path pins proven failing on the ungated build; throwing-listener behavior diffed against real node (uncaughtException fires, `tlsClientError` and socket `error` do not). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 11 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/net/socket.test.ts test/js/node/net/node-net.test.ts <!-- robobun:evidence:end --> ## Compat impact (upstream node v26.3.0 test files vendored, in-tree = passing) - node:tls: 81% → 86% (179 → 191 of 221) --------- Co-authored-by: Alistair Smith <hi@alistair.sh> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> Co-authored-by: robobun <bot@oven.sh>
This was referenced Jul 27, 2026
Jarred-Sumner
added a commit
that referenced
this pull request
Jul 29, 2026
…read (#36332) ## What does this PR do? A server-accepted `net.Socket` that receives DATA + FIN before the app attaches a reader was being destroyed one `setImmediate` after the FIN, with the buffered payload still unread. Any server that hands the accepted socket to an async pipeline (auth lookup, worker handoff, any `await` before `.on('data')`) lost the entire request of every client that sends and then half-closes (curl, `shutdown(SHUT_WR)`, write-then-FIN RPC clients). No error, no `'end'`: the socket was just dead with the bytes stranded in `readableLength`. ### Repro ```js import net from "node:net"; let sock; const ev = []; const srv = net.createServer(s => { sock = s; s.on("close", h => ev.push("close:" + h)); s.on("end", () => ev.push("end")); }); srv.listen(0, "127.0.0.1", async () => { const c = net.connect(srv.address().port, "127.0.0.1"); await new Promise(r => c.once("connect", r)); c.end("PAYLOAD-1234567890"); // request bytes, then half-close await new Promise(r => setTimeout(r, 300)); // app busy for 300 ms const snap = { destroyed: sock.destroyed, rl: sock.readableLength, ev }; let late = ""; sock.on("data", d => (late += d)); sock.resume(); await new Promise(r => setTimeout(r, 300)); console.log(snap, "lateGot=" + JSON.stringify(late)); // node : { destroyed:false, rl:18, ev:[] } lateGot="PAYLOAD-1234567890" // bun : { destroyed:true, rl:18, ev:["close:false"] } lateGot="" process.exit(); }); ``` ### Cause `SocketEmitEndNT` scheduled `setImmediate(destroyAbandonedNT, self)` on peer FIN when no reader had been engaged synchronously from the `'connection'` handler. `destroyAbandonedNT` then called `destroySoon()` precisely when `readableLength > 0` with no listener, i.e. it destroyed exactly the sockets that still held unread inbound data (an empty-buffer FIN was spared). Introduced in 85f22bb (#34598). Node has no such reclaim: a paused socket with buffered data stays open until it is read or destroyed, and `'end'` only fires after the buffer is drained. The handle is already `unref()`'d by `finishSocketEnd`, so leaving the socket open does not pin the event loop. ### Fix Remove `destroyAbandonedNT` and the `kReaderInterest` bookkeeping that fed it (27 lines deleted, no replacement). An accepted socket with buffered payload + peer FIN now stays open until the app reads (then `'end'` fires and the normal `allowHalfOpen` / `autoDestroy` path closes it) or destroys it, matching Node. ### Verification ``` $ bun bd test test/js/node/net/node-net.test.ts -t "accepted-socket buffering" (pass) delivers bytes buffered before a 'readable' listener attaches, past peer FIN (pass) keeps a client socket's buffered response available for a late reader after peer FIN (pass) delivers bytes to a 'data' listener attached via setImmediate from the connection handler (pass) keeps a server socket open while buffered data from a write-then-FIN client is unread 4 pass 0 fail ``` The new test fails on main (`destroyed: true, events: ["close:false"], readableLength: 18`) and passes with the fix. `node-net-server.test.ts` 21/21 and `node-net-allowHalfOpen.test.js` 2/2 unchanged. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/net/node-net.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
This was referenced Jul 31, 2026
stop named-pipe tls.connect({ socket }) re-emitting post-upgrade bytes on the original socket
#32372
Closed
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
4 participants
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.
TLS fixes for the wrapped/injected-socket and shutdown paths, verified against node v26.3.0. Adds 14 vendored upstream tests. Based on main.
What changed
socket.end()sends TLSclose_notifybefore FIN, so peers see a clean shutdown instead of ECONNRESET; a close_notify arriving while a write is spilled no longer hangs the connection (SSL_RECEIVED_SHUTDOWNon the spill-deferred branch only — the broad hoist breaks SNI/pre-handshake paths).tls.connect({socket}),new TLSSocket(socket)): bytes arriving before the deferred TLS engine starts are staged and replayed in order instead of silently dropped — with both ends of aduplexPair()wrapped in-process this was a deterministic handshake deadlock.rawWrite: -1assertions.rejectUnauthorizedhandling on the server verify path lives inSSLWrapper::set_server_verify; a throwingsecureConnectionlistener produces an uncaughtException like node (linear handler, no try/catch shim); server-side sockets use node's manualStartread(0)semantics so pre-listener bytes buffer instead of dropping.Verification
TLS suite 238+/0 and
socket.test.tsfull pass; the deadlock repro prints HANDSHAKE OK with the fix and DEADLOCK without; reject-path pins proven failing on the ungated build; throwing-listener behavior diffed against real node (uncaughtException fires,tlsClientErrorand socketerrordo not).no test proof · iteration 11 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/net/socket.test.ts test/js/node/net/node-net.test.ts
Compat impact (upstream node v26.3.0 test files vendored, in-tree = passing)