node:http2: deliver the destroy() GOAWAY on sessions whose socket has no native handle

[robobun]

Problem

• session.destroy() on an HTTP/2 session whose socket has no native handle never gets its GOAWAY to the peer. This is the transport used for every connection injected into an Http2SecureServer with h2.emit('connection', rawSocket) (the http2-wrapper / crawlee pattern, src/js/node/_http2_upgrade.ts), for a Duplex passed to server.emit('connection'), and for a client createConnection that returns a Duplex.
• A peer that completes the TLS handshake and then records the frame types it reads until the server hangs up sees [SETTINGS, GOAWAY] from node and from a directly listening bun server. On the injected-connection path bun sends [SETTINGS] when destroy() runs on an established session, and nothing at all when it runs synchronously from the 'session' event.
• Http2Session#destroy() sets #connected = false and ends the socket first, and only then calls parser.detach() (on main, src/js/node/http2.ts L4933 / L4947-4957 / L4972 for the server session and L6007 / L6035-6042 / L6061 for the client session). The GOAWAY that this.goaway() queued is still sitting in the parser's cork at that point; detach() is what writes it out. On a socket with a native handle the parser writes to the handle directly, so the order did not matter. Without one, the write goes through the session's write handler (#Handlers.write), which returns -1 when socket.writableEnded or !#connected, so the bytes are dropped and then discarded by detach(). When destroy() runs in the tick that created the session, the preface SETTINGS is still in the same cork and is lost the same way.

Fix

• In both ServerHttp2Session#destroy() and ClientHttp2Session#destroy(): queue the GOAWAY, tear down the streams and detach the parser as before, and only then set #connected = false and end the socket. detach()'s write now happens while the write handler still accepts it, so the GOAWAY goes into the socket ahead of the FIN.
• Nothing else about the teardown moves: the GOAWAY is still queued before the streams are torn down (so the wire order on native sockets is unchanged), the error path still ends the socket and destroys it a tick later, and the graceful path still resume()s before end().
• Reordering, rather than adding a parser.flush() after goaway() the way close() does, because flush() also drains the streams' queued DATA frames and runs their write callbacks in the middle of the teardown. nghttp2 discards everything but the GOAWAY once a session is terminated, and detach()'s write is exactly the frames that were already serialized, so keeping detach() as the writer keeps the native-socket output byte for byte what it is today.
• This is node's order as well: closeSession() destroys the streams, handle.destroy() writes the GOAWAY, and finishSessionClose() ends the socket afterwards. Node sends INTERNAL_ERROR for destroy(err) and NO_ERROR for destroy(); the tests check the codes and pass unchanged under node v26.3.0.
• Tests:
  • test/js/node/http2/node-http2-upgrade.test.mts: the injected-connection server, destroyed with an error from the 'session' event, with an error once the peer has the preface, and without an error. The file also runs itself under node.
  • test/js/node/http2/node-http2.test.js: a server session on a duplexPair side injected with server.emit('connection') (same three shapes, with and without an error) and a client session on a createConnection Duplex (destroy(err) and destroy()), checking the transport received the GOAWAY before it was ended.
  • All nine new tests fail on the current release and pass with the fix.
• Also run with the fix: the 278 upstream test-*http2* files in test/js/node/test/parallel (all pass), the rest of test/js/node/http2 (474 pass), and the grpc-js and http2-wrapper suites (same results as without the change; their failures here need public DNS or a localhost that resolves to 127.0.0.1).

Background

• The parser (H2FrameParser, src/runtime/api/bun/h2_frame_parser.rs) does not write each frame as it is produced. Frames written during one tick are corked into a per-thread buffer and written out in one piece by a deferred flush, by an explicit flush(), or by detach() as its last act. destroy() relies on the last of these.
• The parser has two ways to reach the wire. If the session's socket has a native handle (socket._handle, a plain net.Socket or TLSSocket), it writes to the handle itself. Otherwise every write is dispatched to the session's write handler in http2.ts, which calls socket.write() on whatever stream the session was given.
• Http2SecureServer#emit('connection', rawSocket) is handled by _http2_upgrade.ts: it runs TLS over the raw socket's events and hands the session a plain Duplex carrying the decrypted bytes. That Duplex deliberately has no _handle, so the session is always on the handler path there.
• A GOAWAY is the frame a session sends to tell the peer it is shutting down, carrying an error code; RFC 9113 asks endpoints to send it before closing the connection so the peer can tell a deliberate shutdown from a dropped connection.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 12 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: 79bd8f1a-fbb7-4d97-8cda-5a71d3763c88

📥 Commits

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

📒 Files selected for processing (3)

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

---

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

[robobun]

Status: fix pushed, waiting on CI.

Reproduced on the current release with an Http2SecureServer fed by net.createServer(s => h2.emit('connection', s)) and a TLS peer (ALPN h2, no preface) that records the frame types it reads until the server hangs up:

teardown	node v26.3.0	bun (release)	bun (this PR)
session.destroy(err) inside 'session'	SETTINGS, GOAWAY	nothing	SETTINGS, GOAWAY
session.destroy(err) on an established session	SETTINGS, GOAWAY	SETTINGS	SETTINGS, GOAWAY
directly listening createSecureServer (either of the above)	SETTINGS, GOAWAY	SETTINGS, GOAWAY	SETTINGS, GOAWAY

The same loss reproduces for a server session on a Duplex passed to server.emit('connection') and for a client session on a createConnection Duplex, which is why both destroy() implementations are changed and both shapes are covered by the tests. Details in the PR description.

[claude]

I reviewed this PR and didn't find any bugs. The reordering looks correct and matches Node's teardown order, but since it moves parser.detach() and the stream-teardown callbacks to run while #connected is still true and the socket hasn't been ended, a human look at the re-entrancy implications would be worthwhile.

Checked: #Handlers.write gates on !socket.writableEnded && self.#connected (L4494/L5531), confirming the mechanism; goaway()'s this.destroyed guard is bunHTTP2Socket === null, still set at the call site so no new throw; the catch retry path is no worse (server-side it's now actually better — #connected stays true if goaway() throws); duplexPair/Duplex are already imported in node-http2.test.js.

Extended reasoning...

Overview

This PR reorders the teardown sequence in ServerHttp2Session#destroy() and ClientHttp2Session#destroy() in src/js/node/http2.ts. Previously: set #connected = false → end socket → parser.detach(). Now: queue GOAWAY → tear down streams → parser.detach() → set #connected = false → end socket. The point is that detach() flushes the corked GOAWAY through #Handlers.write, which drops writes once #connected is false or the socket is ended — so on sockets without a native handle (the _http2_upgrade.ts Duplex, a createConnection Duplex, an injected duplexPair side) the GOAWAY was silently discarded. Nine new tests cover server/client × synchronous-in-session-event/after-preface × error/no-error, plus the TLS upgrade path (which also runs under Node).

Security risks

None identified. This is protocol-teardown ordering; no auth, crypto, or untrusted-input parsing is touched. The frame parsers in the tests read only bytes the test itself produced.

Level of scrutiny

Medium-high. HTTP/2 session destruction is a well-trodden source of ordering bugs (the file's own comments and git history attest to it), and the reorder means parser.forEachStream(destroyStreamForSessionDestroy) and parser.emitErrorToAllStreams() — both of which invoke user callbacks — now run while the socket is still writable and #connected is true. On native-handle sockets the parser already wrote directly to the handle regardless of #connected, so this makes the JS-write path consistent rather than introducing new behavior, and it matches Node's closeSession → handle.destroy → finishSessionClose order. The #destroying and #closed latches guard re-entry. Still, the interaction surface (stream _destroy, user 'close'/'error' listeners firing mid-teardown with the socket not yet ended) is exactly the class of thing a maintainer familiar with prior http2 teardown regressions should sanity-check.

Other factors

The PR description reports the full upstream test-*http2* parallel suite (278 files) and the rest of test/js/node/http2 (474 tests) pass, plus grpc-js/http2-wrapper unchanged. The comment fix in the client (See the client session → See the server session) corrects a pre-existing copy-paste. I verified the goaway() call's this.destroyed guard (bunHTTP2Socket === null) is unaffected by the move, and that the catch-and-reset-#destroying path leaves the session in a retryable state under the new ordering (on the server it's actually improved: #connected is no longer cleared before the throwing goaway()). No bugs surfaced, but this is not a mechanical change — deferring for a human pass on the callback-ordering implications.

[robobun]

On the re-entrancy question, here is what actually changes for user code that runs during destroy(), for whoever takes the human pass:

• The stream teardown callbacks (forEachStream(destroyStreamForSessionDestroy), emitErrorToAllStreams) already ran before parser.detach() on main; the only difference is that #connected is still true and the socket is not ended while they run. #connected is read by the write handler and, on the client, by request(); nothing else. So a ping() / goaway() / settings() / stream write issued from one of those callbacks is now written out by detach() on a JS transport, which is what already happened on a native socket (the parser writes to the handle without consulting #connected). Re-entering destroy() is still stopped by #destroying, and close() by #closed, both set before any callback runs, as on main.
• What is genuinely new is that a JS transport's own _write now runs inside destroy() (it could not before, since the handler bailed out first). I probed that with the debug/ASAN build: a createConnection Duplex whose _write calls session.ping(), session.goaway(), session.settings(), session.request(), session.destroy() or transport.destroy() from inside the write that destroy() triggers, followed by a few more sessions corking and flushing afterwards. No crash, every session ends up destroyed, and the GOAWAY reaches the transport in all six cases. Frames queued from inside that write are best effort: ping() / goaway() land in a cork that the teardown releases without writing, settings() (which flushes explicitly) and request() go out after the GOAWAY. Node discards frames submitted after terminate_session too.
• The existing "frames issued from inside a user-supplied Duplex transport's _write" cases in node-http2.test.js exercise the same re-entry during normal operation and still pass, as do the upstream test-http2-* files that tear sessions down from stream callbacks (test-http2-server-stream-session-destroy, test-http2-client-destroy, test-http2-many-writes-and-destroy, test-http2-large-write-destroy, and the rest of the 278).

[robobun]

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

❌ @robobun, your commit c3c4c73 has some failures in Build #94601 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38158

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

bun-38158 --bun

[robobun]

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

@robobun, your commit c3c4c73 is building: #94601

[robobun]

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

@robobun, your commit c3c4c73 is building: #94601