node:http2: destroy the accepted socket when an injected connection's TLS proxy is destroyed

[robobun]

Problem

• A connection handed to an http2.createSecureServer() instance with h2Server.emit("connection", rawSocket) (the pattern used by grpc-js's connection injector, http2-wrapper and similar) is never released once the server side gives up on it: after a failed TLS handshake 'tlsClientError'/'clientError' fire and the peer gets a FIN, but rawSocket.destroyed stays false, rawSocket never emits 'close', the net.Server that accepted it keeps reporting it in getConnections() and netServer.close(cb) never calls back. Node v26.3.0 destroys the accepted socket right away (probe below).
• Same leak after a successful handshake whenever the server side tears the connection down: session.destroy(), a rejected client certificate (requestCert + rejectUnauthorized), or an 'unknownProtocol' client being destroyed after unknownProtocolTimeout. A peer that keeps its side of the TCP connection open keeps the accepted socket (and the connection count) alive indefinitely.
• Cause: upgradeRawSocketToH2 (src/js/node/_http2_upgrade.ts) runs the TLS engine over a proxy Duplex. When the proxy is destroyed, tlsSocketDestroy (_http2_upgrade.ts:106) only closed the native TLS handle, and the native side of a duplex upgrade only end()s the underlying stream (UpgradedDuplex::on_close in src/runtime/socket/UpgradedDuplex.rs, call_write_or_end(None)), so the raw socket was half-closed and left waiting for a FIN the peer never has to send. Nothing ever called rawSocket.destroy().

Fix

• tlsSocketDestroy destroys ctx.rawSocket (if not already destroyed) after closing the native handle. Every teardown of the injected connection ends in the proxy's _destroy: the handshake-failure and certificate-rejection branches of socketHandshake, socketError, Http2SecureServer's own 'tlsClientError' handler, the session's socket teardown, the unknownProtocol timer, and the native close callback (socketClose) that runs when the peer's close_notify or EOF arrives. So the raw socket is released in all of them from one place.
• This is node's behavior: TLSWrap.prototype.close destroys _parentWrap (the socket the TLSSocket was built on) whenever the TLSSocket's handle closes, however it went down. The raw socket is destroyed without an error, so its 'close' reports hadError === false, as node's _parentWrap.destroy() does.
• Timing is unchanged, only the final step differs: the proxy is destroyed at the same points as before (and the engine's writes to rawSocket.write() are plain TCP sends that complete before it), and on the graceful paths (session.close(), HTTP/1 fallback res.end(), proxy.end()) the engine only reports close once the peer's close_notify or EOF has arrived, which is also when node destroys its TLSSocket and the socket under it. Checked that the HTTP/1 fallback (allowHTTP1: true) over an injected connection still delivers its response with the change.
• 'tlsClientError'/'clientError' are still emitted before the proxy is destroyed, so their listeners see the connection intact, as before.
• Companion to node:tls: destroy the wrapped net.Socket when a TLS wrap is destroyed #38028, which fixes the same leak for the wraps node:net/node:tls create (new TLSSocket(raw), tls.connect({ socket }), tls.Server#emit('connection')); those go through a different mechanism and that change does not reach this path.
• Tests: test/js/node/http2/node-http2-upgrade.test.mts, new block "the accepted socket is released when the server side goes down": failed handshake with and without a 'clientError' listener (the two destroy sites on that path), session.destroy(err) and session.destroy(), rejected client certificate, unknownProtocol teardown, plus a control that a live session does not release the connection. In each case the peer holds its TCP side open (for the TLS cases the client runs over an in-memory carrier on top of a held-open net.Socket), and the test waits for the accepted socket's 'close', checks getConnections() === 0 and that netServer.close(cb) calls back. The file also re-runs itself under node (existing "tests should run on node.js" case); all new cases pass on node v26.3.0.
• Without the src/ change (main debug build and released bun 1.4.0) the six teardown cases time out waiting for the accepted socket's 'close'; with it the whole file passes. Also run with the change: the vendored test-http2-socket-close, -client-connection-tunnelling, -autoselect-protocol, -client-proxy-over-http2, -backpressure, -generic-streams(-sendfile), -sensitive-headers, -session-unref, -write-finishes-after-stream-destroy (all exit 0) and grpc-js's (currently describe.todo) connection-injector test, which passes when enabled.

Background

• Injected connections: Http2SecureServer extends tls.Server, and node lets you feed it already-accepted plain sockets via server.emit("connection", socket); the server then performs the TLS handshake itself. In bun, Http2SecureServer#emit routes a non-TLS socket to upgradeRawSocketToH2, which creates a proxy Duplex that the HTTP/2 session (or the HTTP/1 fallback) treats as its TLS socket, and starts the native TLS engine over the raw socket with upgradeDuplexToTLS. Ciphertext flows raw socket -> engine -> proxy push(), and proxy _write() -> engine -> rawSocket.write(). The proxy's _destroy (tlsSocketDestroy) is the one place every teardown of such a connection passes through.
• net.Server connection accounting: an accepted socket is counted in server._connections until its _destroy runs, and server.close(cb) emits 'close' (calling cb) only when that count reaches zero. A socket that is only end()ed stays counted.
• end() vs destroy() on a socket: end() sends a FIN and leaves the socket open to keep reading until the peer also closes; destroy() closes the descriptor and emits 'close' regardless of the peer. Node's TLSWrap.close uses the latter on the wrapped socket.

Probe from the report (peer keeps its side open after the server's FIN; state sampled after 'tlsClientError')

node v26.3.0: ["clientError","tlsClientError","raw-close","client-got-fin","connections=0","raw.destroyed=true","server-close-cb"]
bun 1.4.0:    ["clientError","tlsClientError","client-got-fin","connections=1","raw.destroyed=false","TIMEOUT"]
bun + fix:    ["clientError","tlsClientError","raw-close","client-got-fin","connections=0","raw.destroyed=true","server-close-cb"]

The same probe with a TLS peer that completes the handshake (carrier over a held-open TCP socket) and the server calling session.destroy(err) / session.destroy(), rejecting the client certificate, or turning away a client without ALPN: node and bun + fix release the connection; bun 1.4.0 and main time out in every case. (The error code bun reports for the non-TLS peer, UNABLE_TO_GET_ISSUER_CERT where node reports ERR_SSL_HTTP_REQUEST, is unchanged here and tracked separately.)

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 29 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 70a6fcd2-ee28-4da0-a0bb-e4ef03368b51

📥 Commits

Reviewing files that changed from the base of the PR and between b7a0431 and 74d59f8.

📒 Files selected for processing (2)

• src/js/node/_http2_upgrade.ts
• test/js/node/http2/node-http2-upgrade.test.mts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status

• Reproduced on released bun 1.4.0 and on a main debug build with the probe from the report (h2.emit("connection", raw) + a non-TLS peer that keeps its side open): connections=1, raw.destroyed=false, netServer.close(cb) never fires. Node v26.3.0 releases the connection. The same holds after a successful handshake for session.destroy(), a rejected client certificate and an unknownProtocol teardown.
• Fix: src/js/node/_http2_upgrade.ts, the proxy's _destroy now destroys the raw socket (this PR).
• Tests: test/js/node/http2/node-http2-upgrade.test.mts, six teardown cases time out without the fix and pass with it; the file also passes under node.

[claude]

I reviewed this PR and didn't find any bugs. The source change is small and well-justified against Node's TLSWrap behavior, and the new tests cover the teardown matrix thoroughly (including a Node cross-check). Given it changes socket-lifecycle ordering in the TLS/HTTP2 injection path, a quick human look would still be worthwhile.

What was reviewed:

• tlsSocketDestroy now destroys ctx.rawSocket after h.close(); checked the double-destroy guard covers the existing catch block and the socketClose → destroy() re-entry.
• Verified onTlsClose listener cleanup and the events[3] raw-socket 'close' path don't race into a second destroy — both hit .destroyed guards.
• Tests: await real conditions (raw socket 'close', getConnections, close(cb)), port: 0, finally cleanup, control case for a live session, and re-run under Node.

Extended reasoning...

Overview

This PR touches two files: a ~9-line addition to tlsSocketDestroy in src/js/node/_http2_upgrade.ts that destroys the underlying raw socket when the TLS proxy Duplex is torn down, and a new 170-line describe block in test/js/node/http2/node-http2-upgrade.test.mts covering seven teardown variants (failed handshake ×2, session.destroy() ×2, rejected client cert, unknownProtocol timeout, and a live-session control). The fix addresses a leak where an injected connection's accepted net.Socket is only end()ed (never destroy()ed) when the server side gives up, so net.Server keeps counting it and close(cb) never fires.

Security risks

None identified. The change tightens resource release rather than loosening any check; no auth, crypto, or validation logic is altered. The only new behavior is calling .destroy() on a socket the server already owns and has decided to tear down.

Level of scrutiny

Medium-high. The source diff is tiny and the mechanism (mirror Node's TLSWrap.close → _parentWrap.destroy()) is clearly argued, but it lives in the TLS/HTTP2 teardown path where ordering matters: h.close() triggers the native side to end() the raw socket (close_notify + FIN), and immediately after we now destroy() it. The PR description asserts the engine's outbound writes are plain TCP sends that complete before the destroy, and the vendored Node http2 tests plus the file's Node re-run all pass, so the risk of truncating a final GOAWAY/close_notify appears to have been considered and empirically checked. Still, this is exactly the kind of lifecycle change a maintainer familiar with UpgradedDuplex should glance at.

Other factors

• No CODEOWNERS entry covers these paths.
• No prior human or bot review comments beyond a CodeRabbit rate-limit notice.
• I traced re-entrancy: the pre-existing catch block calls rawSocket.destroy(e) before tlsSocket.destroy(e), and socketClose (native close, possibly triggered by the raw socket's own 'close' via events[3]) calls this.destroy() — in both cases the new !rawSocket.destroyed guard or the proxy's own destroyed flag prevents a second destroy.
• Tests meet the repo's bar: they await observable conditions rather than sleeping, use port: 0, hold the peer's TCP side open via allowHalfOpen + an in-memory carrier so the fix is the only way the accepted socket can close, include a negative control, and are verified to time out on the unfixed build and pass under Node v26.3.0.
• The PR is a companion to #38028 (same leak in the node:tls wrap path), which is a design-level detail a human should be aware of when landing.

Deferring rather than approving because socket-lifecycle changes in TLS/HTTP2 warrant a human sign-off even when the diff is small and the tests are strong.

[robobun]

^{Updated 5:05 AM PT - Aug 13th, 2026}

❌ @robobun, your commit 74d59f8 has 1 failures in Build #94562 (All Failures):

• test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts - unchecked exception at JSC__JSModuleLoader__loadAndEvaluateModule @ src/jsc/bindings/bindings.cpp:3788 (thrown from vendor/WebKit/Source/JavaScriptCore/runtime/VMTraps.cpp:443) on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 38154

That installs a local version of the PR into your bun-38154 executable, so you can run:

bun-38154 --bun

[Jarred-Sumner]

Superseded by #44618, which consolidates the open TLS pull requests. This fix and its tests are in there, either as written, rewritten smaller, or merged with the other PRs that patched the same cause (see the "By area" list in that PR). Closing in favor of it.