net,tls: port Node.js net/tls compatibility tests and fix the gaps they surface - #31148
Closed
cirospaciari wants to merge 17 commits into
Closed
net,tls: port Node.js net/tls compatibility tests and fix the gaps they surface#31148cirospaciari wants to merge 17 commits into
cirospaciari wants to merge 17 commits into
Conversation
Collaborator
|
Updated 12:02 PM PT - May 29th, 2026
❌ @autofix-ci[bot], your commit 0b57999 has 10 failures in
🧪 To try this PR locally: bunx bun-pr 31148That installs a local version of the PR into your bun-31148 --bun |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request refactors the Node.js net module's server-side socket acceptance through a dedicated ChangesServer connection acceptance and socket lifecycle
OpenSSL half-close state management
TLS option validation and protocol handling
Validation error message standardization
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/net.ts`:
- Around line 524-526: The code currently always calls _socket.resume() after
emitting the 'connection' event which can undo a user calling _socket.pause()
inside their connection listener; change the resume logic to only call
_socket.resume() when pauseOnConnect is false, not TLS, and the socket is not
currently paused by the user. Concretely, after self.emit("connection", _socket)
replace the unconditional resume with a guarded call that checks the socket's
paused state (use _socket.isPaused() if available) so: if (!pauseOnConnect &&
!isTLS) { if (typeof _socket.isPaused !== "function" || !_socket.isPaused())
_socket.resume(); } to preserve explicit pauses made inside the 'connection'
callback.
In `@src/js/node/tls.ts`:
- Around line 873-874: The code currently applies a truthy default with "||"
before validation so falsy but valid values (like 0) are lost and invalid falsy
values bypass validateNumber; change it to first read the raw value (e.g. const
rawHandshakeTimeout = options && Object.prototype.hasOwnProperty.call(options,
"handshakeTimeout") ? options.handshakeTimeout : undefined), call
validateNumber(rawHandshakeTimeout, "options.handshakeTimeout"), then assign the
final handshakeTimeout = rawHandshakeTimeout === undefined ? 120 * 1000 :
rawHandshakeTimeout so explicit 0 is preserved and invalid values are still
validated; update uses of handshakeTimeout accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce8d1bbd-d9ce-4f6b-a957-e8c20863d8c6
📒 Files selected for processing (19)
packages/bun-usockets/src/crypto/openssl.csrc/js/node/net.tssrc/js/node/tls.tssrc/jsc/bindings/NodeValidator.cpptest/js/node/http2/node-http2.test.jstest/js/node/test/parallel/test-http2-server-shutdown-options-errors.jstest/js/node/test/parallel/test-net-allow-half-open.jstest/js/node/test/parallel/test-net-bytes-stats.jstest/js/node/test/parallel/test-net-connect-options-allowhalfopen.jstest/js/node/test/parallel/test-net-end-destroyed.jstest/js/node/test/parallel/test-net-large-string.jstest/js/node/test/parallel/test-net-pause-resume-connecting.jstest/js/node/test/parallel/test-net-server-keepalive.jstest/js/node/test/parallel/test-net-server-nodelay.jstest/js/node/test/parallel/test-net-socket-setnodelay.jstest/js/node/test/parallel/test-tls-basic-validations.jstest/js/node/test/parallel/test-tls-buffersize.jstest/js/node/test/parallel/test-tls-client-reject.jstest/js/node/test/parallel/test-vm-module-errors.js
cirospaciari
force-pushed
the
claude/port-node-net-tls-tests
branch
from
May 20, 2026 20:27
819d2a2 to
27908f3
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
cirospaciari
force-pushed
the
claude/port-node-net-tls-tests
branch
from
May 20, 2026 20:49
27908f3 to
2e08dac
Compare
cirospaciari
marked this pull request as draft
May 20, 2026 20:54
cirospaciari
force-pushed
the
claude/port-node-net-tls-tests
branch
4 times, most recently
from
May 20, 2026 23:30
24ddb9b to
5f125f1
Compare
cirospaciari
marked this pull request as ready for review
May 21, 2026 00:18
cirospaciari
force-pushed
the
claude/port-node-net-tls-tests
branch
from
May 21, 2026 00:23
5f125f1 to
1134d61
Compare
Member
Author
|
@robobun adopt |
Collaborator
|
👀 Adopted. Local validation (debug+ASAN): all 11 ported tests pass, full CI Follow-ups pushed: |
robobun
force-pushed
the
claude/port-node-net-tls-tests
branch
from
May 21, 2026 02:21
0bb6de2 to
19330e1
Compare
…ey surface Ports 11 net/tls tests from the Node.js test suite into the node-compat parallel directory, verbatim, and fixes the runtime divergences they expose: net.Socket / net.Server: - Accepted server sockets inherit the server's keepAlive, noDelay, allowHalfOpen and highWaterMark; the Socket constructor honors options.handle. - The native socket's half-open flag follows the Duplex's allowHalfOpen (default false closes on peer FIN so 'close' fires; true keeps the writable side open), matching Node/libuv. - pause() is honored while a socket is still connecting. - setNoDelay() forwards to the handle; Server coerces noDelay with Boolean() like keepAlive. - _write defers its callback to the next tick only for TLS sockets (the SSL engine batches, so bufferSize/writableLength must reflect the queued bytes); plain TCP completes synchronously so a tight write() loop backpressures at the kernel rather than the JS highWaterMark. tls: - validateSecureContextOptions (ciphers/passphrase/ecdhCurve/min-max version/ ticketKeys/sessionTimeout) and a simplified convertALPNProtocols. - TLS clients emit the 'session' event. Errors: - validateBuffer reports "must be an instance of Buffer, TypedArray, or DataView" to match Node. Two existing tests that asserted the old validateBuffer wording are updated to the upstream Node wording to match. Comments that mirror Node behavior link the corresponding upstream source.
- Server constructor now validates options.keepAliveInitialDelay with validateNumber before the ~~(/1000) coercion, matching Node's lib/net.js and Bun's own Socket constructor. Non-number values now throw ERR_INVALID_ARG_TYPE instead of silently coercing to 0. - Rewrote the half-open comments at onconnection / kConnectTcp / kRealListen, which still described the earlier 'native is always half-open' design; the native flag now follows the Duplex/Server's allowHalfOpen.
bun-debug has ASAN enabled but isASAN (which checks for 'bun-asan' in the binary name) is false, so the 10k-request stress test ran with the release 15s timeout and always timed out under `bun bd test`. Use isDebug to apply a larger multiplier there; CI's release+ASAN lane (isASAN=true) keeps the existing 3x.
cirospaciari
force-pushed
the
claude/port-node-net-tls-tests
branch
from
May 21, 2026 21:00
5ca55ec to
f49ffa0
Compare
TLSSocket wrap, session/keylog, SNICallback/ALPNCallback, pfx, OpenSSL error shapes, addCACert isolation, setDefaultCACertificates, local binding, and the close-timing/teardown fixes they surfaced (+305 ported tests) Squash of the iterative work on top of the original test-porting branch: native allowHalfOpen, reset/ECONNRESET semantics, the FIN-terminated response close path, EPOLLRDHUP/writable polling fixes, the per-loop BIO state protection around in-handshake callbacks, ALPN and SNI server dispatches, session resumption and keylog events, pfx parsing, OpenSSL error decomposition, exclusive SSL_CTX ownership for createSecureContext with cached internal paths, the setDefaultCACertificates override on every construction path, listen error address/port, per-callback server handler tables, and the expanded node test suites for net and tls.
Member
Author
cirospaciari
force-pushed
the
claude/port-node-net-tls-tests
branch
from
May 28, 2026 22:37
1e7bfcb to
f49ffa0
Compare
…tls-tests-2 # Conflicts: # src/js/node/tls.ts # src/runtime/node/node_net_binding.rs # src/runtime/socket/tls_socket_functions.rs # src/uws_sys/SocketKind.rs # src/uws_sys/us_socket_t.rs # test/js/node/tls/node-tls-server.test.ts
Comment on lines
+368
to
+391
| validateSecureProtocol(secureProtocol); | ||
| if (ciphers !== undefined && ciphers !== null) validateString(ciphers, "options.ciphers"); | ||
| if (passphrase !== undefined && passphrase !== null) validateString(passphrase, "options.passphrase"); | ||
| if (ecdhCurve !== undefined && ecdhCurve !== null) validateString(ecdhCurve, "options.ecdhCurve"); | ||
| // clientCertEngine must be a string (engine name); a provided engine then | ||
| // fails because BoringSSL (which Bun always uses) has no OpenSSL ENGINE | ||
| // support, matching Node's setClientCertEngine. Node: | ||
| // https://github.com/nodejs/node/blob/614050b657e9757c1097aa85f92f2cb51149dc0d/lib/internal/tls/secure-context.js#L296 | ||
| if (clientCertEngine !== undefined && clientCertEngine !== null) { | ||
| if (typeof clientCertEngine !== "string") { | ||
| throw $ERR_INVALID_ARG_TYPE("options.clientCertEngine", ["string", "null", "undefined"], clientCertEngine); | ||
| } | ||
| throw $ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED("Custom engines not supported by this OpenSSL"); | ||
| } | ||
| // BoringSSL (always used by Bun) has no automatic DH parameter selection. | ||
| // Matches Node's setDHParam('auto') throwing ERR_CRYPTO_UNSUPPORTED_OPERATION. | ||
| // https://github.com/nodejs/node/blob/614050b657e9757c1097aa85f92f2cb51149dc0d/lib/internal/tls/secure-context.js#L254 | ||
| if (dhparam === "auto") { | ||
| throw $ERR_CRYPTO_UNSUPPORTED_OPERATION("Automatic DH parameter selection is not supported"); | ||
| } | ||
| if (minVersion != null && !VALID_TLS_VERSIONS.has(minVersion)) | ||
| throw $ERR_TLS_INVALID_PROTOCOL_VERSION(String(minVersion), "minimum"); | ||
| if (maxVersion != null && !VALID_TLS_VERSIONS.has(maxVersion)) | ||
| throw $ERR_TLS_INVALID_PROTOCOL_VERSION(String(maxVersion), "maximum"); |
Contributor
There was a problem hiding this comment.
🟡 Node throws ERR_TLS_PROTOCOL_VERSION_CONFLICT when secureProtocol is combined with minVersion or maxVersion, but validateSecureContextOptions() validates each individually and never checks the combination — newNativeSecureContext() then silently lets secureProtocolToVersionRange() overwrite the user's min/maxVersion. The $ERR_TLS_PROTOCOL_VERSION_CONFLICT builtin already exists (builtins.d.ts:651) but is unused; this is the one piece of Node's secureProtocol/version validation block that wasn't ported here.
Extended reasoning...
What's missing
Node's createSecureContext (lib/internal/tls/secure-context.js) does:
if (secureProtocol) {
if (minVersion != null)
throw new ERR_TLS_PROTOCOL_VERSION_CONFLICT(minVersion, secureProtocol);
if (maxVersion != null)
throw new ERR_TLS_PROTOCOL_VERSION_CONFLICT(maxVersion, secureProtocol);
}This PR's new validateSecureContextOptions() (src/js/node/tls.ts:355-414) destructures secureProtocol, minVersion and maxVersion and validates each individually — validateSecureProtocol(secureProtocol) at line 368, then the VALID_TLS_VERSIONS membership checks at lines 388-391 — but never checks for the conflict between them. The $ERR_TLS_PROTOCOL_VERSION_CONFLICT builtin is declared in src/js/builtins.d.ts:651 and registered in ErrorCode.ts/ErrorCode.rs, but a grep across src/js/ shows no call site.
Code path that exhibits the divergence
Downstream, newNativeSecureContext() translates the three options to the integer protocol range:
let minVersion = tlsStringToProtocolVersion(optMinVersion);
let maxVersion = tlsStringToProtocolVersion(optMaxVersion);
const range = secureProtocolToVersionRange(optSecureProtocol);
if (range) {
minVersion = range[0]; // overwrites the user's minVersion
maxVersion = range[1]; // overwrites the user's maxVersion
}So when both are passed, secureProtocol silently wins. The same logic is duplicated in Server.prototype[buntls] with the comment "secureProtocol wins, like Node's SecureContext::Init" — which is true of Node's native init order, but Node never reaches that init because the JS-side conflict check throws first.
Why nothing else catches it
The PR-added test/js/node/test/parallel/test-tls-min-max-version.js does assert this exact case at lines 123-130:
// Cannot use secureProtocol and min/max versions simultaneously.
test(U, U, U, U, 'TLSv1.2', 'TLS1_2_method',
U, U, 'ERR_TLS_PROTOCOL_VERSION_CONFLICT');But the test early-returns at lines 8-11 under BoringSSL (process.features.openssl_is_boringssl → runs common/boringssl.js's testLegacyProtocolUnsupported() instead and returns), so those assertions never execute in Bun. test-tls-basic-validations.js does not cover this combination either.
Step-by-step proof
- Call
tls.createSecureContext({ secureProtocol: 'TLSv1_2_method', maxVersion: 'TLSv1.3' }). InternalSecureContext→validateSecureContextOptions(options). Line 368:validateSecureProtocol('TLSv1_2_method')— valid, returns. Line 390:'TLSv1.3'is inVALID_TLS_VERSIONS— passes. No conflict check.newNativeSecureContext(options):optMaxVersion = 'TLSv1.3'→maxVersion = 0x0304; thensecureProtocolToVersionRange('TLSv1_2_method')returns[0x0303, 0x0303], which overwritesmaxVersion = 0x0303.- Bun returns a context pinned to TLS 1.2; Node throws
TypeError [ERR_TLS_PROTOCOL_VERSION_CONFLICT].
Why this is in scope
This PR introduces the entire secureProtocol/minVersion/maxVersion plumbing — validateSecureProtocol, secureProtocolToVersionRange, tlsStringToProtocolVersion, the VALID_TLS_VERSIONS set, and the translation block in newNativeSecureContext are all new in this diff. The conflict check is the one piece of Node's validation block for these three options that wasn't ported alongside them.
Impact
Nit-level. It only affects callers passing an explicitly contradictory combination (already API misuse), the silent fallback (secureProtocol wins) is deterministic and matches what the in-code comment says, and it has no security/correctness impact on valid inputs. It's purely a missing user-facing diagnostic.
Fix
Add to validateSecureContextOptions, right after validateSecureProtocol(secureProtocol):
if (secureProtocol) {
if (minVersion != null) throw $ERR_TLS_PROTOCOL_VERSION_CONFLICT(minVersion, secureProtocol);
if (maxVersion != null) throw $ERR_TLS_PROTOCOL_VERSION_CONFLICT(maxVersion, secureProtocol);
}A pfx-only client folded the bundle's CA into the explicit ca option, which under the new CA semantics replaced the default trust store and broke verification against the default/NODE_EXTRA_CA_CERTS roots. The embedded CAs are now applied through addCACert on an uncached context, extending the default trust set the way Node loads PKCS#12 bundles. Drops the temporary expectations entry and the duplicated drain handler.
… API A send() failing with ECONNRESET/EPIPE while a response was still queued was reported as would-block, so the node:net write layer waited forever for a drain that could never come - the FIN-terminated http response tests and test-net-GH-5504 hung on every Linux target. A new us_socket_write_check_error reports the fatal condition to callers that opt in; the node:net write funnel drops undeliverable buffered data, closes the socket through the normal native path, and a pending write callback is completed when the socket goes away. The existing us_socket_write contract is untouched, so the HTTP and WebSocket stacks keep their current behavior.
…e:tls handlers The handshake callback's second argument is authorized (handshake plus verification plus hostname), matching the public Bun.connect contract. The node:tls client handlers previously destroyed the socket for any unauthorized result, which tore down sessions whose certificate merely failed verification even though rejectUnauthorized / checkServerIdentity decide that in JS. Keep the fatal teardown only for protocol-level failures - reported as EPROTO carrying the OpenSSL reason string or an already decomposed ERR_SSL_*/ERR_OSSL_* code - and let verification results flow into the JS authorization handling. The server-side handler keeps its raw !success teardown because client-certificate verification is reported separately there.
cirospaciari
force-pushed
the
claude/port-node-net-tls-tests
branch
from
May 29, 2026 18:37
826ce7e to
11bacc5
Compare
…back - feed oversized TLS buffers to the C layer in i32-sized chunks instead of truncating the length cast, stopping if the socket closes mid-feed - never panic on FFI-provided lengths in the TLS dispatch callbacks - check SSL_set1_chain's return value in setKeyCert - add the ERR_TLS_INVALID_STATE / ERR_TLS_RENEGOTIATION_UNSUPPORTED aliases to the Rust error-code table - PerformanceObserverEntryList for node-only entry types: chronological ordering, optional type filter on getEntriesByName, drop an unreachable observe() branch - test hygiene: skip the getPeerCertificate RSS check on local debug builds instead of passing over-threshold runs, preserve harness ASAN_OPTIONS, hoist requires in ssl-ctx-cache and fix its stale createSecureContext comment, narrow swallowed socket errors in raw test servers, restore the server-side socket.authorized coverage alongside the SNICallback test
…l-send-error write path - detect a transient send failure with bsd_would_block() (errno on POSIX, WSAGetLastError() on Windows) instead of reading errno on every platform; bsd_send already retries EINTR internally - declare us_socket_write_check_error in libusockets.h next to the other write entry points - satisfy the lint gate in the Rust binding: SAFETY comment, explicit raw pointer for the out-parameter, and clamp the length cast
cirospaciari
force-pushed
the
claude/port-node-net-tls-tests
branch
from
May 29, 2026 19:01
0b57999 to
d32fcdd
Compare
…ites after upgradeTLS
- Server.setSecureContext() folds PKCS#12-embedded CAs into the ca it hands
the native listener, so createServer({pfx, requestCert}) verifies client
certificates against the bundle's own CA again (regression test added)
- write_maybe_corked() routes BYPASS_TLS sockets through the raw write path
again instead of the SSL-encrypting write_check_error, restoring the
[raw, tls] upgrade contract
internal_flush() now uses the same fatal-send-error detection as the initial write: once the peer is gone the kernel rejects every retry, so the parked node:net buffer is dropped and the socket closes instead of waiting forever on a writable event that can never make progress. The BYPASS_TLS upgrade twin keeps the raw write path and TLS errors keep propagating through the SSL layer.
…lished ECONNRESET and protocol-level failures already returned earlier in the handshake handlers, so reaching the establishment point means the TLS session exists even when authorized is false purely because of the native hostname verdict (which arrives with no error object). Keeps exportKeyingMaterial() usable and the parked TLS 1.3 session tickets flushing on rejectUnauthorized:false connections to a mismatched host.
Comment on lines
+3046
to
+3053
| if (this.listenerCount("error") === 0) { | ||
| // An unobserved transport teardown (the peer dropped a connection | ||
| // nobody is listening to anymore): destroy quietly - the destroy still | ||
| // errors any remaining streams - instead of re-emitting on a session | ||
| // with no 'error' listener and crashing the process. | ||
| this.destroy(); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🔴 This swallows every socket error on a ClientHttp2Session with no 'error' listener — TLS failures, ETIMEDOUT, mid-transfer ECONNRESET — not just the benign post-GOAWAY teardown it's meant to absorb. Node's socketOnError only filters the narrow error.code === 'ECONNRESET' && session[kState].goawayCode !== null case; everything else goes through session.destroy(error) and crashes if unhandled, per the EventEmitter contract. If this is compensating for the new SocketEmitEndNT RST surfacing in net.ts, narrowing the check to error?.code === 'ECONNRESET' (or matching Node's GOAWAY guard) would cover that without hiding real failures. (The matching ServerHttp2Session.#onError change at 3046 is effectively dead — connectionListener always attaches sessionOnError at line 4101, so listenerCount('error') is never 0 there.)
Extended reasoning...
What the change does
Both ServerHttp2Session.#onError (http2.ts:3045–3053) and ClientHttp2Session.#onError (http2.ts:3625–3631) now check this.listenerCount('error') === 0 and call this.destroy() with no argument instead of this.destroy(error). The in-code comment frames this as absorbing "an unobserved transport teardown" so it doesn't crash the process. The change was added as a knock-on of this PR's net.ts work: SocketEmitEndNT now surfaces a peer RST as destroy(new ConnResetException('read ECONNRESET')) on the underlying socket, which the http2 session's socket.on('error', #onError) receives; without compensation, every http2 test where a peer hard-closes would now throw an uncaught 'read ECONNRESET' on the session.
Where it diverges from Node
Node's equivalent is socketOnError in lib/internal/http2/core.js:
function socketOnError(error) {
const session = this[kSession];
if (session !== undefined) {
if (error.code === 'ECONNRESET' && session[kState].goawayCode !== null)
return session.destroy();
session.destroy(error);
}
}Node only swallows one case: an ECONNRESET that arrives after a GOAWAY has been exchanged (i.e. the connection was already shutting down cleanly and the RST is teardown noise). For every other socket error — ETIMEDOUT, EPIPE, TLS protocol failures, ECONNRESET before GOAWAY (peer crashed mid-transfer) — Node calls session.destroy(error), which emits 'error' on the session. If no listener is attached, that's an uncaught exception per the documented EventEmitter contract — the same fail-loud behavior every other Node EventEmitter has.
This PR's filter is listenerCount('error') === 0 with no check on error.code or session state, so it swallows all of the above whenever the user hasn't attached a listener.
Step-by-step proof (client side)
const session = http2.connect('https://example.com')— user attaches no'error'listener.- Mid-connection, the underlying TLS socket fails with e.g.
ETIMEDOUTor the server crashes and the kernel reportsECONNRESETbefore any GOAWAY. - net.ts surfaces this on the socket → http2's
socket.on('error', #onError)fires with the error. #onErrorreaches line 3625:this.#closedis false (noclose()was called),listenerCount('error') === 0→this.destroy()with no argument.destroy()errors any active streams (so an in-flight request does see an error), then emits'close'on the session — but the session-level error is gone. If the session was idle (e.g. a pooled connection being kept warm), the only observable signal is a bare'close'with no indication of why.- In Node, step 4 would instead call
session.destroy(error)→process.nextTick(emit, session, 'error', error)→ uncaught exception, surfacingError: read ECONNRESET(or the TLS error) and crashing the process.
Addressing the "this is intentional / not a bug" objection
It is clearly intentional — but the PR's stated goal is Node compatibility, and this introduces a divergence in error-surfacing semantics that is strictly broader than what Node does. Specifically:
- "The session still emits 'close'" — true, but
'close'carries no error. A user debugging "why did my http2 connection drop?" gets nothing. - "Active streams are still errored" — true, and that mitigates the in-flight-request case. But an idle session (the comment's own "idle pooled connection" scenario) has no streams to error, so the failure is fully silent.
- "Line 3621 already swallowed errors when
#closed" — that pre-existing check is much narrower (the user already calledclose(), so they've signaled they're done). The new check applies regardless of session state. - "The server-side branch is dead code" — correct:
connectionListener(http2.ts:4099–4101) always attachessession.on('error', sessionOnError), solistenerCount('error')is never 0 on aServerHttp2Session. That makes the 3046–3053 hunk a no-op (worth deleting, but harmless). The client-side hunk at 3625 is the one that actually changes behavior.
Impact
For http2.connect() users who don't attach a session 'error' listener (relying on stream-level errors or just letting unhandled errors crash, which is the Node default), genuine transport failures on an idle session are now invisible. The session emits 'close' with no diagnostic, and the process keeps running where Node would have crashed. This makes "my http2 connection silently dies" reports harder to debug and diverges from Node's documented EventEmitter behavior.
Fix
Narrow the filter to the case it's actually compensating for — the new RST-on-close from this PR's net.ts changes — by matching Node's check:
#onError(error: Error) {
this[bunHTTP2Socket] = null;
if (this.#closed) {
this.destroy();
return;
}
// Match Node's socketOnError: only an ECONNRESET after GOAWAY is teardown noise.
if (error?.code === 'ECONNRESET' && this[kState]?.goawayCode != null) {
this.destroy();
return;
}
this.destroy(error);
}If Bun doesn't track goawayCode on the session state, a coarser error?.code === 'ECONNRESET' would still be much closer to Node than swallowing everything. The dead ServerHttp2Session hunk can be dropped entirely.
cirospaciari
added a commit
that referenced
this pull request
Jun 17, 2026
…ey surface — half-open/reset/write semantics, server TLSSocket wrap, session/keylog, SNICallback/ALPNCallback, pfx, OpenSSL error shapes, addCACert, local binding (+305 tests) (#31155) [publish images] Brings node:net and node:tls compatibility in line with Node by porting upstream tests verbatim and fixing the native gaps they expose. | | parallel | sequential | **total** | |---|---|---|---| | **net** | 141/148 (95.3%) | 10/12 | **151/160 (94.4%)** | | **tls** | 150/215 (69.8%) | 4/4 | **154/219 (70.3%)** | 305 verbatim upstream tests added. ### Behavior changes - **Half-open semantics**: `socket.end()` now half-closes (FIN) instead of full-closing, so a server's response after a client `end()` is delivered before close. - **Reset/write semantics**: `socket.resetAndDestroy()`, `server.close({ resetConnections })`, write-after-end / write-after-destroy errors, and the post-write callback contract match Node. - **Close-time read errors**: a reset delivered with the close destroys the socket with Node's `read ECONNRESET` shape when an error listener is attached and a clean EOF has not already been delivered; a codeless close error that carries an errno derives its `code` from it. A reset that lands after the exchange already ended cleanly stays a graceful close. - **Local binding**: `net.connect({ localAddress, localPort })` binds before connecting on every connect path including deferred DNS resolution. - **The `'session'` and `'keylog'` events**, end-to-end through the native handler-slot chain; the `--tls-keylog` flag. - **OpenSSL error decomposition**: handshake/context-creation failures carry Node's `code`/`library`/`function`/`reason` properties (`ERR_SSL_<REASON>`, `ERR_OSSL_<LIB>_<REASON>`). - **`secureContext.context.addCACert()`** extends the full default trust set (bundled roots, `NODE_EXTRA_CA_CERTS`, system CAs when enabled) on the context's own store, and chain verification then uses that store; `tls.createSecureContext()` returns a context that owns its SSL_CTX exclusively so a CA appended to one cannot affect another (the internal connect/listen paths keep the per-digest cache). - **`tls.setDefaultCACertificates()`** applies on every secure-context construction path (plain `tls.connect()`, `addContext`, `setSecureContext`), not just the public `createSecureContext()`. - **The `SNICallback` and `ALPNCallback` server dispatches**, resolved per connection with Node's semantics; `tlsSocket.setKeyCert()`; `ERR_TLS_ALPN_CALLBACK_WITH_PROTOCOLS`. - **The OpenSSL cipher-list selector grammar** (`PSK+HIGH`, `!aNULL`, `@SECLEVEL`, …) is accepted/rejected the way BoringSSL evaluates it; a mixed EC/RSA multi-identity configuration is rejected with Node's decomposed `KEY_TYPE_MISMATCH`. - **The `pfx` option**: PKCS#12 blobs (single or array, per-entry passphrases) are parsed into key/cert with Node's error messages; CAs embedded in the bundle extend the trust set (they are not treated as an explicit `ca` replacement). - **Server-side `TLSSocket` wrapping** of an accepted socket and `tls.connect({ socket: duplex })` over a generic Duplex, including `connecting` parity and synchronous teardown of the wrapped duplex on destroy. - **Memory safety**: a deferred-close protocol so a destroy issued from inside an SNI/ALPN/keylog callback cannot free the SSL out from under BoringSSL; the per-loop BIO state is snapshotted/restored across in-handshake JS so cross-socket I/O cannot misroute the in-flight handshake; the GC-rooting of the detailed peer-certificate chain. - **`net` perf_hooks observer**, `options.handle`, `pause()`/`unref()` accounting, `SocketAddress`/BlockList parity, the `autoSelectFamily` flag plumbing. ### Known limitations / follow-ups - An asynchronous `SNICallback` now suspends the handshake (BoringSSL select-certificate retry) until its callback resolves - the upstream `test-tls-sni-option.js` passes; synchronous callbacks behave as before. - Two linked follow-ups around mid-handshake teardown: (1) a server connection raw-closed (RST) while its handshake is suspended leaves the JS connection count stale until the socket is GC'd - `server.close()` waits longer than it should in that edge case; (2) fixing it by dispatching the handshake failure from the close path requires first aligning `tlsHandshakeError`'s no-listener behavior with Node's silent-destroy semantics, otherwise routine client-initiated mid-handshake teardowns (h2 connection management) surface as spurious ECONNRESET errors. - An `SNICallback` that reports an error, returns something that is not a SecureContext, or throws aborts the handshake (synchronously or asynchronously): the connection is dropped without a TLS alert and the server emits `'tlsClientError'` with the callback's error, matching Node. - `setKeyCert()` from inside `ALPNCallback` is too late under BoringSSL's TLS 1.3 (the credential is already chosen); calling it from `SNICallback` works. - `tls.DEFAULT_MIN/MAX_VERSION` are now honored at context-construction time (assignment through the module exports works the way Node's does), and a TLS socket that ends first keeps reading the peer's in-flight data - BoringSSL has no TLS half-close, so `end()` defers the close_notify (flushing pending session tickets first) and half-closes at the TCP level instead. - `test-net-perf_hooks.js` is intermittently divergent on Ubuntu 25.04 (dual-stack `localhost` resolution). - A response written to a peer that has already gone away used to be retried as would-block on Linux, hanging the FIN-terminated-response teardown tests there; node:net writes now go through an opt-in fatal-send-error path (`us_socket_write_check_error`) that fails the pending write and closes the socket instead. - The fetch/h2 suites on Windows still see teardown resets surfaced between tests; under investigation alongside the write-error work. - An http2 session with no `'error'` listener swallows `ECONNRESET` transport teardown noise (destroying the session quietly) instead of crashing the process the way Node's EventEmitter contract would; all other unobserved errors still surface. Fixes #28638 Fixes #28641 Fixes #26418 Fixes #20642 --- ## Origin (consolidated from #31148) ## What Ports 11 `net`/`tls` tests from the Node.js test suite into `test/js/node/test/parallel/` (verbatim) and fixes the runtime divergences they surfaced. ### Tests added (verbatim from upstream Node) **net:** `test-net-pause-resume-connecting`, `test-net-server-keepalive`, `test-net-server-nodelay`, `test-net-socket-setnodelay`, `test-net-connect-memleak` **tls:** `test-tls-basic-validations`, `test-tls-buffersize`, `test-tls-client-reject`, `test-tls-net-socket-keepalive`, `test-tls-secure-session`, `test-tls-connect-memleak` ### Fixes **`net.Socket` / `net.Server`** - Accepted server sockets inherit the server's `keepAlive`, `noDelay`, `allowHalfOpen` and `highWaterMark`; the `Socket` constructor honors `options.handle`. - The native socket's half-open flag follows the Duplex's `allowHalfOpen` (default `false` closes on peer FIN so `'close'` fires; `true` keeps the writable side open), matching Node/libuv. - `pause()` is honored while a socket is still connecting. - `setNoDelay()` forwards to the handle; `Server` coerces `noDelay` with `Boolean()` like `keepAlive`. - `_write` defers its callback to the next tick only for TLS sockets (the SSL engine batches, so `bufferSize`/`writableLength` reflect the queued bytes); plain TCP completes synchronously so a tight `write()` loop backpressures at the kernel rather than the JS `highWaterMark`. **`tls`** - `validateSecureContextOptions` (ciphers / passphrase / ecdhCurve / min-max version / ticketKeys / sessionTimeout) and a simplified `convertALPNProtocols`. - TLS clients emit the `'session'` event. **Errors** - `validateBuffer` reports `"must be an instance of Buffer, TypedArray, or DataView"` to match Node. Two existing tests that asserted the old wording are re-synced to upstream. Comments that mirror Node behavior link the corresponding upstream source. ## Testing Validated locally with the debug build before pushing: - All 11 ported tests pass 20× with no flakes and are byte-identical to upstream (flaky candidates were dropped). - Full `test-{net,http,tls}-*` parallel sweep: 0 failures. - `test/js/node/http/node-http.test.ts` (incl. HTTP server security tests): 78 pass, 0 fail. ## Notes The strictly Node-correct half-close `_final` (`shutdown()` rather than `$end()`) is left for a follow-up: it exposes a uWS TLS-handshake edge case where a successful handshake immediately followed by a `close_notify` is reported as `ECONNRESET`. Until that's addressed, `_final` uses `$end()`. --- *This PR consolidates the original test-porting branch (`claude/port-node-net-tls-tests`, #31148) and the follow-up branch (`claude/port-node-net-tls-tests-2`); both branches now point to the same squashed content.*
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
2 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.
What
Ports 11
net/tlstests from the Node.js test suite intotest/js/node/test/parallel/(verbatim) and fixes the runtime divergences they surfaced.Tests added (verbatim from upstream Node)
net:
test-net-pause-resume-connecting,test-net-server-keepalive,test-net-server-nodelay,test-net-socket-setnodelay,test-net-connect-memleaktls:
test-tls-basic-validations,test-tls-buffersize,test-tls-client-reject,test-tls-net-socket-keepalive,test-tls-secure-session,test-tls-connect-memleakFixes
net.Socket/net.ServerkeepAlive,noDelay,allowHalfOpenandhighWaterMark; theSocketconstructor honorsoptions.handle.allowHalfOpen(defaultfalsecloses on peer FIN so'close'fires;truekeeps the writable side open), matching Node/libuv.pause()is honored while a socket is still connecting.setNoDelay()forwards to the handle;ServercoercesnoDelaywithBoolean()likekeepAlive._writedefers its callback to the next tick only for TLS sockets (the SSL engine batches, sobufferSize/writableLengthreflect the queued bytes); plain TCP completes synchronously so a tightwrite()loop backpressures at the kernel rather than the JShighWaterMark.tlsvalidateSecureContextOptions(ciphers / passphrase / ecdhCurve / min-max version / ticketKeys / sessionTimeout) and a simplifiedconvertALPNProtocols.'session'event.Errors
validateBufferreports"must be an instance of Buffer, TypedArray, or DataView"to match Node. Two existing tests that asserted the old wording are re-synced to upstream.Comments that mirror Node behavior link the corresponding upstream source.
Testing
Validated locally with the debug build before pushing:
test-{net,http,tls}-*parallel sweep: 0 failures.test/js/node/http/node-http.test.ts(incl. HTTP server security tests): 78 pass, 0 fail.Notes
The strictly Node-correct half-close
_final(shutdown()rather than$end()) is left for a follow-up: it exposes a uWS TLS-handshake edge case where a successful handshake immediately followed by aclose_notifyis reported asECONNRESET. Until that's addressed,_finaluses$end().