node:http2: end a pushed response on its HEADERS frame when the stream was ended before respond()

[robobun]

Problem

• On a node:http2 server, a pushed stream whose writable side is ended before respond() never gets its response ended on the wire. Two ways to get there:
  • stream.pushStream(headers, { endStream: true }, (err, push) => push.respond(...))
  • stream.pushStream(headers, (err, push) => { push.end(); push.respond(...); })
• Frames the server sends for the pushed stream, against node v26.3.0 (HEADERS flags 0x4 = END_HEADERS, 0x5 = END_HEADERS | END_STREAM):

  node: PUSH_PROMISE(1)  HEADERS sid=2 flags=0x5
  bun:  PUSH_PROMISE(1)  HEADERS sid=2 flags=0x4   (nothing follows on stream 2)
  

• Consequences: the client's pushed stream never emits 'end'/'close', the server push stream's writable never finishes ('finish' never fires, writableFinished stays false), and a client.close() waiting on the pushed stream never completes. The :method HEAD variant of the same thing works.
• Cause: Http2Stream#_final (src/js/node/http2.ts:2887) handles an ended-but-unresponded push stream by parking its callback in bunHTTP2StreamFinal instead of writing an empty DATA frame ahead of the response HEADERS, and relies on respond() to put END_STREAM on the HEADERS frame. ServerHttp2Stream#respond() (:3807) only did that for options.endStream, status 204/205/304 and headRequest, so for a plain push it sent HEADERS without END_STREAM and the parked callback was never completed.

Fix

• respond() also forces endStream when _final has parked its callback. The HEADERS frame then carries END_STREAM, the native request() dispatches onStreamEnd, and the existing handler completes the parked callback through markWritableDone (the path HEAD pushes already take today).
• Matches node: Http2Stream::SubmitResponse submits the response without a data provider whenever the stream's writable half has been shut (!is_writable()), which is the state pushStream({ endStream }) and push.end() leave the stream in, so nghttp2 sets END_STREAM on the HEADERS frame.
• The parked callback is the right signal for this (rather than writableEnded): it is set from _final, i.e. only once end() was called and every buffered chunk has been written. writableEnded is already true during the implicit respond() that _write/_writev issue for data buffered behind a cork() when end() was called before uncorking, and forcing END_STREAM there would put the body on a half-closed stream.
• respond(headers, { waitForTrailers: true }) on such a stream goes through the same branch, which already strips waitForTrailers, so no 'wantTrailers' is emitted; node also never asks for trailers on a response that has no body.
• Related, independent: node:http2: release server push streams once their response ends #38082 makes the native side treat an even-id server stream as HALF_CLOSED_REMOTE so a completed push is released and emits 'close'. It keeps the parked-callback branch in _final and does not touch respond(), so this bug is present with or without it; with both, an endStream push closes exactly like node's. node:http2: send RST_STREAM when a server stream is reset #33380 covers the neighbouring close()/destroy()-before-respond() paths.
• Tests: test/js/node/http2/node-http2.test.js, describe("http2 pushStream: response ended before respond()"). One real client/server session per case; the flags argument of the client's 'push' event is the flags byte of the pushed response's HEADERS frame, so END_STREAM placement is asserted directly, then the pushed stream's 'end'/'close' (rstCode 0) and the server push stream's 'finish'. Cases: pushStream({ endStream: true }), push.end() before respond(), the same with waitForTrailers, a HEAD push (passes before too; pins that the two paths agree), and a push that is still writable at respond() time (END_STREAM must stay on its DATA frame). The first three fail on main at the flags assertion and pass with the fix.
• Also run with the fix: the whole of node-http2.test.js (362 pass), h2-conformance.test.ts (61 pass), and all 261 upstream test-http2-* tests under test/js/node/test (all pass).

Background

• Server push: the server announces a request it is going to answer unasked with a PUSH_PROMISE frame on the client's stream, reserving a new even-numbered stream id, and then sends the response on that stream. In node:http2 that is stream.pushStream(), whose callback receives a ServerHttp2Stream for the reserved id; the response is sent with the normal respond()/end() API.
• END_STREAM: the HTTP/2 flag that closes the sender's side of a stream. It rides on the last frame the sender emits for the stream, either a DATA frame or, for a response with no body, the HEADERS frame itself. A response whose HEADERS lacks it is, as far as the peer is concerned, still in progress until a frame carrying it arrives.
• _final and the parked callback: _final is the stream.Writable hook that runs once end() was called and every buffered write has been flushed; the writable emits 'finish' when its callback is invoked. For a push stream that was ended before respond(), bun's _final cannot send anything yet (a response has to start with HEADERS), so it stores the callback in the bunHTTP2StreamFinal slot; markWritableDone, run from the onStreamEnd dispatch, invokes it once the END_STREAM has gone out.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 9 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: afb92925-74ea-48e8-a217-641f1df198b2

📥 Commits

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

📒 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]

^{Updated 7:01 AM PT - Aug 13th, 2026}

❌ @robobun, your commit 10d7edd has 1 failures in Build #94378 (All Failures):

• test/js/web/timers/setInterval.test.js - code 1 on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 38104

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

bun-38104 --bun

[robobun]

Status: reproduced on main (04148c8) and fixed here.

Reproduced with a frame-level sniff of the server's output for pushStream(h, { endStream: true }) and for push.end() before push.respond(): the pushed stream's HEADERS frame goes out with flags 0x4 (no END_STREAM) and nothing follows, so the client's pushed stream never ends; node v26.3.0 sends flags 0x5 for both. A :method HEAD push already sent 0x5.

New cases in test/js/node/http2/node-http2.test.js (describe("http2 pushStream: response ended before respond()")): the three affected shapes fail on main at the END_STREAM flag assertion and pass with this change; the HEAD push and the still-writable push pass both ways and pin the surrounding behaviour. Whole file, h2-conformance.test.ts and the upstream test-http2-* suite pass with the change.

[claude]

LGTM — narrowly scoped one-condition fix with thorough coverage.

What was reviewed:

• Traced every _final path that parks bunHTTP2StreamFinal; only the even-id push-before-respond branch (:2892) can leave it set when respond() runs (guarded by headersSent at :3713), so the new condition cannot fire on odd-id or already-responded streams.
• Confirmed the branch already strips waitForTrailers and the parked callback is completed via onStreamEnd → markWritableDone, matching the existing HEAD-push path.
• Tests: END_STREAM flag asserted directly from the client 'push' event before any wait; negative case pins that a still-writable push keeps END_STREAM on the DATA frame; all failure events wired to reject.

Extended reasoning...

Overview

The PR adds a single OR clause to ServerHttp2Stream#respond()'s endStream-forcing condition (src/js/node/http2.ts:3819): when _final has parked its callback in bunHTTP2StreamFinal, the response HEADERS frame carries END_STREAM. This closes the gap where a pushed stream ended before respond() (via pushStream({ endStream: true }) or an explicit push.end()) never had its response terminated on the wire, leaving both peers hung. Five new test cases in node-http2.test.js cover both entry points, the waitForTrailers interaction, the HEAD baseline, and a negative case proving still-writable pushes are unaffected.

Security risks

None. This is HTTP/2 server-push frame-flag placement; no parsing of untrusted input, no allocation, no auth/crypto surface.

Level of scrutiny

Moderate — HTTP/2 stream state is subtle, but the change is one boolean clause added to an existing well-commented branch. I audited every site that writes bunHTTP2StreamFinal (:2879, :2892, :2912, :2967) against respond()'s entry guards (destroyed, assertSession, headersSent). The :2879 path requires EndStreamSent (implies data was written → implicit respond() already set headersSent); :2912 requires bunHTTP2WaitForTrailers (only set at :3855 after respond()); :2967 requires a dead session (fails assertSession). Only :2892 — the even-id push ended before respond — can reach the new clause, which is exactly the intended target. The PR's choice of the parked callback over writableEnded as the signal is correct: writableEnded flips before buffered cork'd writes flush, and forcing END_STREAM there would half-close before the body goes out.

Other factors

The tests follow the repo's harness conventions closely: every error/sessionError/stream error is wired to a shared reject, each wait is Promise.raced against it, the END_STREAM flag is asserted from the client's 'push' event flags byte before awaiting close so the pre-fix hang surfaces as an assertion failure rather than a timeout, and cleanup is in finally. The negative test ("still writable at respond()") pins that the fix does not over-apply. The PR reports the full node-http2.test.js (362), h2-conformance.test.ts (61), and all 261 upstream test-http2-* tests pass. The interaction with the independent #38082 is documented and orthogonal (that PR touches native even-id lifecycle, not respond()).