node:http2: drop empty DATA frames that carry no END_STREAM, pass the stream id to frameError

[robobun]

Problem

• A node:http2 stream that calls end() with trailers pending, or write("") / write(Buffer.alloc(0)), puts a zero-length DATA frame with no flags on the wire. Node never sends one.
• That frame carries no information, and receivers (node's maxSessionInvalidFrames, Bun's own inbound parser) count it against the session's invalid-frame allowance, so body-less waitForTrailers requests slowly burn the peer's budget.
• Cause: the outbound path wrote a DATA header for every empty payload and only chose the flags based on whether trailers were pending, on both the direct write path and the queued (backpressure) path.
• Separately, the stream-level 'frameError' event was emitted as (type, code); node emits (type, code, id).

Fix

• An empty payload is written as a DATA frame only when it can carry END_STREAM. Otherwise nothing goes on the wire, and the write callback and wantTrailers dispatch still run. The queued path applies the same rule when it drains, so ordering behind earlier frames is unchanged.
• Correct because END_STREAM is the only thing a zero-length DATA frame can convey; noTrailers() / sendTrailers({}) still terminate with a single DATA(END_STREAM), as node does.
• 'frameError' now passes the stream id as its third argument, node's documented signature.
• Verification: six new wire-level tests (direct path, queued path, client and server frameError args) fail on the unfixed binary and pass with the fix. The existing http2 suite and the 256 upstream test-http2-* files still pass; the manual repro against node v26.3.0 is in the original.

Background

• HTTP/2 sends a body as DATA frames on a stream; the END_STREAM flag marks the last frame. A zero-length DATA frame with END_STREAM is the normal way to finish when there is nothing left to send.
• waitForTrailers: with this option end() does not close the stream. The stream emits wantTrailers, and the trailer HEADERS frame written by sendTrailers() carries END_STREAM instead; sendTrailers({}) falls back to an empty DATA(END_STREAM).
• Invalid-frame allowance: HTTP/2 receivers (nghttp2 in node, Bun's own parser) tolerate a bounded number of meaningless or malformed frames per session before tearing it down, and a flagless empty DATA frame counts.
• Outbound queue: when the session has socket backpressure or frames already waiting on flow control, a write is queued and written later by the flush path, so the empty-frame rule has to hold in two places.
• 'frameError' is the per-stream event fired when a frame cannot be encoded (here, trailers larger than the max frame size); the stream is then reset with FRAME_SIZE_ERROR.

Original description

What

Two node compat fixes on the node:http2 outbound path, both visible with a waitForTrailers request against a raw server that logs the frames it receives:

1. Bun wrote a zero-length DATA frame with no flags whenever the outbound payload was empty but END_STREAM could not be set: req.end() with trailers pending (END_STREAM rides on the trailer HEADERS), and every write("") / write(Buffer.alloc(0)). Node never sends such a frame. An empty DATA frame without END_STREAM carries no information, and receivers count each one against the session's invalid-frame allowance (node's maxSessionInvalidFrames; Bun's own inbound engine does the same in h2/connection.rs), so a Bun client doing body-less waitForTrailers POSTs slowly burns the peer's budget.
2. The stream-level 'frameError' event was emitted as (type, code). Node emits (type, code, id).

Repro

Raw net server that sends SETTINGS after the preface, ACKs the client's SETTINGS and logs {type, flags, len} of every frame on stream 1:

const req = client.request({ ":method": "POST", ":path": "/" }, { waitForTrailers: true });
req.on("wantTrailers", () => req.sendTrailers({ "x-big": Buffer.alloc(64 * 1024 + 1, "x").toString() }));
req.on("frameError", (type, code, id) => console.log("frameError", type, code, id));
req.end();

	node v26.3.0	bun 1.4.0
frames on stream 1	HEADERS(4) RST_STREAM	HEADERS(4) DATA(flags=0,len=0) RST_STREAM
frameError args	1 6 1	1 6 undefined

With a small trailer the stray frame is the same: node sends HEADERS(4) then HEADERS(5), bun sent HEADERS(4) DATA(0,0) HEADERS(5); with no wantTrailers listener node sends HEADERS(4) DATA(flags=1,len=0), bun sent HEADERS(4) DATA(0,0) DATA(1,0). req.write("") on a plain POST also produced a DATA(0,0) per call. After this change every one of these flows produces the same frame sequence as node (the request flows that already carried a body were already identical and are unchanged).

Cause

H2FrameParser::send_data ("empty payload we still need to send a frame") wrote a DATA header for every empty payload and only decided the flags based on wait_for_trailers; Stream::flush_queue did the same for a zero-length queued entry (the path taken when the session already has frames queued behind flow control or socket backpressure). The server-side _final in http2.ts had grown a JS workaround for this; the client _final and plain empty writes went straight to native.

emitFrameErrorEventNT in http2.ts (shared by the client and server handlers) simply never passed the id.

Fix

• send_data: an empty payload is written only when it can carry END_STREAM. When it cannot (trailers pending, or a plain empty write) nothing goes on the wire; the write callback and the wantTrailers dispatch still run exactly as before. The queued branch is unchanged so ordering relative to frames ahead of it is preserved.
• flush_queue: a zero-length queued entry is written only when it ends up carrying END_STREAM; otherwise it is consumed for its callback / wantTrailers bookkeeping alone.
• emitFrameErrorEventNT passes stream.id.

Why this is right: END_STREAM is the only thing a zero-length DATA frame can convey, so a frame that will not carry it has no receiver-visible purpose, and both nghttp2 (observed above) and Bun's own receiver treat it as noise to be rate-limited. noTrailers() / sendTrailers({}) still terminate with a single DATA(END_STREAM, len 0), as node does. The frameError argument order is the documented node signature.

Not changed here (separate, already tracked): the ERR_HTTP2_STREAM_CANCEL vs ERR_HTTP2_STREAM_ERROR code on this path (#36389), and a client endStream + waitForTrailers request, where node puts END_STREAM on the request HEADERS and never asks for trailers.

Verification

New outbound empty DATA frames and trailer encode failures block in test/js/node/http2/h2-conformance.test.ts, all wire-level against the file's raw server:

• end() with trailers pending: stream frames are exactly HEADERS, trailer HEADERS(END_STREAM)
• same with no wantTrailers listener: exactly HEADERS, DATA(END_STREAM, len 0)
• write(""), write(Buffer.alloc(0)), write("payload"), write(""), end(): exactly HEADERS, DATA(7), DATA(END_STREAM, 0)
• the queued path: stream 1 exhausts both send windows so stream 3's end() goes through the outbound queue; after a connection WINDOW_UPDATE stream 3 shows exactly HEADERS, trailer HEADERS(END_STREAM) (this is the flush_queue path; the direct send_data branch is not involved while another stream's data is queued)
• client trailers that cannot be encoded: frameError args are [HEADERS, FRAME_SIZE_ERROR, 3] and the wire is HEADERS, RST_STREAM(FRAME_SIZE_ERROR) with no DATA
• server trailers that cannot be encoded: frameError args are [HEADERS, FRAME_SIZE_ERROR, 3]

All six fail on the unfixed binary (the extra {type: 0, flags: 0, length: 0} entries / two-element frameError args) and pass with the fix. bun bd test test/js/node/http2/ otherwise passes apart from two tests that only time out under the debug build's 5 s default when the whole directory runs (node-http2-streams-rehash forEachStream, and the TLS-over-Duplex "tail" case in node-http2.test.js); both pass when run on their own and neither touches the changed paths. All 256 upstream test-http2-* files in test/js/node/test/parallel exit 0 with the fix (including test-http2-write-empty-string, test-http2-zero-length-write, test-http2-write-callbacks, test-http2-trailers, test-http2-exceeds-server-trailer-size and the compat trailer tests); test-http2-forget-closed-streams needs about 1.5 minutes under the debug build but passes.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 10 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: af1aa11a-4de5-463f-8eaf-b242937c1a47

📥 Commits

Reviewing files that changed from the base of the PR and between 0825a8b and 59c579e.

📒 Files selected for processing (3)

• src/js/node/http2.ts
• src/runtime/api/bun/h2_frame_parser.rs
• test/js/node/http2/h2-conformance.test.ts

---

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

[robobun]

^{Updated 8:41 PM PT - Aug 11th, 2026}

❌ @robobun, your commit 59c579e has 2 failures in Build #92746 (All Failures):

• test/js/third_party/grpc-js/test-tonic.test.ts - code 1 on 🍎 14 aarch64
• test/js/node/http2/h2-conformance.test.ts - code 1: error: timed out waiting for frame on 🍎 14 aarch64
• test/js/node/http2/h2-conformance.test.ts - code 1: error: timed out waiting for frame on 🍎 26 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 37712

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

bun-37712 --bun

[robobun]

Status

Reproduced both divergences against node v26.3.0 with a raw frame-logging server: a waitForTrailers POST ended with no body put a DATA(flags=0, len=0) on the wire before the trailers (or the RST_STREAM when the trailers fail to encode), and 'frameError' fired with two arguments instead of (type, code, id). Empty write() calls produced the same stray frame.

Fix is in this PR: the native send and queued-flush paths only write an empty DATA frame when it carries END_STREAM, and frameError passes the stream id. Six new wire-level tests in test/js/node/http2/h2-conformance.test.ts fail on the unfixed binary and pass with the fix.

[claude]

I reviewed this PR and didn't find any bugs. The native change is small and I traced the bookkeeping on both paths, but since it alters HTTP/2 outbound wire framing a human look would still be worthwhile.

What was reviewed:

• send_data empty-payload branch: callback / onWantTrailers / close-state dispatch in the !enqueued tail still runs when nothing is written; the queued-behind-backpressure branch is unchanged.
• flush_queue zero-length branch: owned_frame is set before the early break so outbound_queue_size decrement, write callback, and onWantTrailers still fire from the defer block.
• emitFrameErrorEventNT: both call sites (server handler at ~4224, client withStreamFrame at ~5218) pass an Http2Stream whose .id is the stream id; matches Node's documented (type, code, id).

Extended reasoning...

Overview

Two Node-compat fixes on the node:http2 outbound path: (1) stop writing zero-length DATA frames that carry no END_STREAM (in both H2FrameParser::send_data and Stream::flush_queue in src/runtime/api/bun/h2_frame_parser.rs), and (2) pass stream.id as the third argument to the stream-level 'frameError' event in src/js/node/http2.ts. A comment on the server _final JS workaround is rewritten now that native handles the same case. Six new wire-level tests are added to test/js/node/http2/h2-conformance.test.ts.

Security risks

None. This strictly reduces what goes on the wire (a no-op frame is dropped) and adds a numeric argument to an event. No new parsing of untrusted input, no auth/crypto/permission surface.

Level of scrutiny

Moderate-to-high. The native diff is only ~25 lines and the logic is simple ("only write an empty DATA frame when it will carry END_STREAM"), but it sits on the HTTP/2 outbound framing path. I traced control flow through both modified functions to confirm the callback dispatch, onWantTrailers dispatch, outbound_queue_size decrement, and stream-state transitions are preserved on every branch (empty write with close=false, close=true + wait_for_trailers, and close=true without trailers), for both the direct-write and queued paths. The change is a strict subset of previous behavior — a frame that conveyed no information is no longer sent — so it cannot break a compliant peer, and the PR verified it against Node's own trailer test suite.

Other factors

The test coverage is unusually thorough: it exercises the direct send_data branch, the flush_queue branch (via a stream blocked behind an exhausted send window), the no-wantTrailers-listener path, plain empty writes interleaved with real payload, and both client- and server-side frameError on a non-default stream id. Tests follow the file's raw-server harness conventions, use Buffer.alloc(n, fill).toString() per repo guidance, and await observable wire events rather than sleeping. The PR description documents that all six fail on the unfixed binary and pass with the fix, and lists ten upstream Node parallel tests that continue to pass. No CODEOWNERS cover these paths and there are no outstanding review comments. I'm deferring only because outbound HTTP/2 framing is a critical enough path that a maintainer glance is warranted.