node:http2: emit 'trailers' on a server stream after its session is closed

[robobun]

Fixes #43488

Problem

• A node:http2 server closes a session gracefully while a request is open. The request trailers arrive after that. The stream never emits 'trailers', and req.trailers stays {} in the compat API. node v26.3.0 delivers the trailers.
• session.close(), server.close(), and a client that calls close() all cause it.
• The cause is the guard of ServerHttp2Session.#Handlers.streamHeaders (src/js/node/http2.ts:4111). It returned for every header block when self.closed was true.

Fix

• The handler applies the self.closed check only to a stream that the 'stream' event has not delivered yet (StreamResponded is not set). The stream.closed check stays.
• A graceful close only stops new streams. node's onSessionHeaders checks session.closed only in the branch that creates a stream (core.js#L373-L381).
• Verified: four new tests in test/js/node/http2/node-http2.test.js. bun 1.4.3 fails three. The fourth guards that a stream opened after session.close() is still not delivered. All four pass on node v26.3.0.
• Self-reviewed: 9 concerns raised, 7 addressed. The Notes name the other two.

Background

• session.close() is the graceful shutdown of a session. It sends a GOAWAY frame, accepts no new streams, and lets the open streams finish. server.close() calls it on every session. A server session also calls it when the client sends a GOAWAY with NO_ERROR.
• Trailers are a second HEADERS block that ends a request or a response after the body.
• The native frame parser calls streamHeaders for every complete header block of a server stream. The first block becomes the 'stream' event and sets the StreamResponded bit. A later block becomes 'trailers'.

Notes

Reproduction (bun repro.js, compare with node repro.js):

const http2 = require("http2");
const ev = [];
const server = http2.createServer();
server.on("stream", stream => {
  ev.push("stream");
  stream.on("trailers", t => ev.push("trailers " + t["x-trailer"]));
  stream.on("data", d => ev.push("data " + d));
  stream.on("end", () => { ev.push("end"); stream.respond({ ":status": 200 }); stream.end("ok"); });
  stream.on("close", () => ev.push("close rstCode=" + stream.rstCode));
  stream.session.close();
});
server.listen(0, "127.0.0.1", () => {
  const client = http2.connect("http://127.0.0.1:" + server.address().port);
  client.on("error", () => {});
  const req = client.request({ ":method": "POST", ":path": "/" }, { waitForTrailers: true });
  req.on("wantTrailers", () => req.sendTrailers({ "x-trailer": "1" }));
  req.on("response", () => ev.push("client response"));
  req.resume();
  req.on("close", () => setTimeout(() => { console.log(JSON.stringify(ev)); client.close(); server.close(); }, 100));
  setTimeout(() => req.end("body"), 100);
});

• node v26.3.0 and this PR: ["stream","data body","trailers 1","end","close rstCode=0","client response"]
• bun 1.4.3: ["stream","data body","end","close rstCode=0","client response"]

The client-side trigger is the common one: client.close() right after client.request(). The client sends a GOAWAY with NO_ERROR, the server session calls close() on itself, and the trailers of the open request are lost. The second test covers it. The 'trailers' listener records stream.session.closed, so each test proves that the session was closed when the trailers arrived.

The new tests do not use a timer. In the server-side tests the client sends the body and the trailers from its 'goaway' listener. In the client-side test the client calls close() and then req.end() from its 'response' listener, so the GOAWAY is on the wire before the body and the trailers.

Mutation check: without the closed-session clause, the guard test fails with delivered: [1, 3].

History of the guard: #19558 added self.closed || stream.closed to the client handler and to the server handler. #19616 reverted that PR because a client that called close() no longer got 'response' (test-http2-client-upload.js). #19630 landed it again with the client check changed to stream.rstCode. The server check kept self.closed. So the client handler needs no change here.

Self-review. Addressed: the comment now names the StreamResponded branch, the tests assert one combined object per case, the guard test checks the GOAWAY payload, a test for the client-side close exists, the server responds before 'end' so no listener can throw, the compat test listens on req.stream for errors, and cleanup uses destroy(). Rejected: merging the stream.closed read into the status read. It is a micro-optimization and it is not part of this fix. Out of scope: pushStream() and ping() still work after session.close(), and node refuses both. #43486 tracks that.

Relation to #43475: that PR handles a stream that opens after close() and edits the same if. It keeps the old behaviour for delivered streams. The two changes are independent. The second one to merge needs a small rebase.

Local runs with the debug build:

• The four new tests: 32 of 32 pass with --rerun-each 8. With bun 1.4.3, 15 of 20 fail with --rerun-each 5 (the guard test passes).
• bun bd test test/js/node/http2/node-http2.test.js (first version of this change): 373 pass, 17 fail. All 17 are 5 s timeouts of tests that spawn a bun-debug child. The debug binary needs about 4 s to start on this machine. None of them sends request trailers or closes a session.
• bun bd test test/js/node/http2/h2-conformance.test.ts: 69 pass, 1 timeout ("releases server stream objects once the peer resets their streams").
• 29 upstream test-http2-* files that use trailers, close, GOAWAY, or shutdown: 28 exit 0. test-http2-forget-closed-streams.js (10,000 requests) exceeds 120 s in the debug build.

[coderabbitai]

Warning

Review paused — included plan limit reached

Keep your review moving with free on-demand reviews.

• Run this review for free

On-demand reviews are free for one more day.

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Promotion and pricing details

On-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file.

Review limit details

Or wait 11 minutes for your next included review.

Check out review usage here.

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 74382f6f-5fd1-4cda-842c-8663dac59a6d

📥 Commits

Reviewing files that changed from the base of the PR and between 26e7a4b and 431e72f.

📒 Files selected for processing (2)

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

---

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

[robobun]

Closing: this is a duplicate of #43507, which was opened first for the same bug (#43488) with the same fix. The extra tests on this branch (client-side close(), the compat API, and a guard for a stream that opens after close()) are offered there.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[claude]

🟣 pre-existing, not blocking: Servers that call session.close() or server.close() while a client pipelines one more request never finish shutting down: the session's 'close' event and the server.close() callback never fire, and the client waits forever for that request. The new guard at src/js/node/http2.ts:4117 drops the undelivered stream after streamStart at line 4007 already counted it in #connections, and nothing ever decrements it or resets the stream. Fix: on a closed session, refuse the new stream (rstStream REFUSED_STREAM, decrement #connections and #peerInitiatedStreams) instead of returning, at this guard and the same drop for the maxConcurrentStreams path.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Extended reasoning...

The dismissing finders read the base guard and called it pre-existing; the base did drop every block, but the PR rewrites exactly this guard and adds a test that cements the drop, so the pin is now the intended contract. Trace: server calls close() (line 4669) with one stream open, so #connections is 1 and no destroy is scheduled. The client already sent HEADERS for stream 3 before it read the GOAWAY (any browser or grpc client pipelining requests). Native handle_headers (h2/connection.rs:986-995) has no post-GOAWAY refusal (going_away is written at 380 and 823 and never read), so on_stream_open fires. streamStart (line 4007) does #connections++ and creates a ServerHttp2Stream. streamHeaders returns at line 4117 because status lacks StreamResponded. No RST_STREAM leaves; the native stream sits in Open or HalfClosedRemote forever. The delivered stream finishes: streamEnd (line 4066) decrements to 1, so the #connections === 0 && self.#closed branch at line 4085 never runs and destroyIfNotDestroyedNT is never scheduled. The session 'close' event never fires, server.close(cb) never calls…

Verification: pre-existing — the base already fails by exactly the same route, and the PR's rewrite of the guard leaves the new-stream-after-close path byte-for-byte equivalent (it only lets trailers of delivered streams through). Triggering condition: a server session that has called close() (or server.close() → closeAllSessions, /home/claude/bun/src/js/node/http2.ts:6370-6376) while at least one stream is…