Skip to content

node:http2: end a server response on its HEADERS frame when the stream was ended before respond() - #38170

Closed
robobun wants to merge 1 commit into
mainfrom
farm/325a0c59/http2-end-before-respond
Closed

robobun wants to merge 1 commit into
mainfrom
farm/325a0c59/http2-end-before-respond

Conversation

@robobun

@robobun robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • On a node:http2 server, stream.end() before stream.respond() on a regular (client-initiated) stream puts an empty END_STREAM DATA frame on the wire for a stream that has not sent any HEADERS. Frames on stream 1 for a GET, against node v26.3.0 (0x4 = END_HEADERS, 0x5 = END_HEADERS | END_STREAM):
    s.end(); s.respond({ ":status": 200 })     node: HEADERS 0x5        bun: DATA 0x1 len=0, HEADERS 0x4
    s.end()                                    node: (nothing)          bun: DATA 0x1 len=0
    
  • User-visible: bun's own client gets 'end'/'close' but never 'response'; a respond() a tick later throws ERR_HTTP2_INVALID_STREAM (the frame already closed the stream); on 1.4.0 the same-tick respond() also raises an uncaught ERR_HTTP2_SESSION_ERROR on the server once the client rejects the late HEADERS. For a strict peer, DATA before the response HEADERS is a protocol error (RFC 9113 8.1).
  • Cause: Http2Stream#_final (src/js/node/http2.ts) handled ended-before-respond only for pushed (even-id) streams; everything else fell through to native.writeStream(id, "", ..., true), i.e. the empty END_STREAM DATA frame. Node's _final (Http2Stream::DoShutdown) only marks the stream unwritable, and SubmitResponse then submits the response without a body, so END_STREAM rides on the HEADERS frame.
  • Second, pre-existing bug this exposes: close()/destroy() before respond() use the same _final, so the server now sends just RST_STREAM(NO_ERROR) for them (as node does), but a bun client receiving RST_STREAM(NO_ERROR) emitted 'close' without 'end'. on_stream_reset in src/runtime/api/bun/h2_frame_parser.rs (the inbound engine's bridge) dispatched every non-CANCEL code, NO_ERROR included, as onStreamError, which destroys the stream before its readable can end. Reproducible on main against a node server doing stream.close() (node's client: end, close; bun's: close), and it made the vendored test-http2-compat-write-head-after-close.js hang once the server side was fixed.

Fix

  • _final on a server stream without headers sent: a client-initiated stream sets FinalCalled and completes its callback without writing anything, so the writable finishes right away like node's, whether or not a respond() follows. The pushed-stream branch (parked callback) is unchanged.
  • respond() forces endStream when FinalCalled is set, so END_STREAM goes on the HEADERS frame; that branch already strips waitForTrailers, and node likewise never asks for trailers on a response submitted without a body.
  • Why FinalCalled is the right signal: it is the analog of node's !is_writable(). Data written before respond() triggers the implicit respond() in _write/_writev, so when respond() is still allowed to run, _final having run means end() was called with nothing written at all. writableEnded would be wrong: it is already true during that implicit respond() for chunks buffered behind cork() when end() is called before uncorking, and END_STREAM on the HEADERS would then put the body on a half-closed stream.
  • on_stream_reset: a peer RST_STREAM(NO_ERROR) dispatches onStreamEnd(CLOSED), the routing the local end_stream (and the dead legacy inbound handle_rst_stream_frame) already use; the streamEnd handlers push EOF and read, so the stream emits 'end' then 'close' with rstCode 0, matching node. Other codes unchanged.
  • Result: every end()/close()/destroy()/session.destroy()-before-respond() variant measured now sends the same frames as node (table below). Streams whose headers were already sent are untouched.
  • Tests, test/js/node/http2/h2-conformance.test.ts (raw-frame client against a real server): end(); respond() sends exactly one HEADERS carrying END_STREAM and the stream finishes and closes; respond() from 'finish' works; end() alone sends nothing and still finishes; waitForTrailers variant emits no 'wantTrailers'; request body still open (END_STREAM on HEADERS half-closes, the request's END_STREAM closes); body buffered behind cork() keeps END_STREAM off HEADERS (pins the signal choice, passes before and after); bun's client sees response (flags 0x5), end, close; a client fed RST_STREAM(NO_ERROR) by a raw server emits end, close rstCode=0. 7 of the 8 new cases fail on main.
  • The re-entrant sendTrailers inline snapshot in the same file loses its req error ERR_HTTP2_STREAM_CANCEL line. That fixture feeds the client a RST_STREAM(NO_ERROR) before client.destroy(); the error was the deferred error dispatch of that reset picking up the session's cancel error. The stream now closes cleanly, as node's does for a stream the peer already reset with NO_ERROR. The test's point (exit 0, no signal) is unchanged.
  • Also run: all 261 vendored upstream test-http2-* files pass (compat-write-head-after-close needs the on_stream_reset hunk); node-http2.test.js (357 pass) and the other test/js/node/http2/* files; the http2 regression tests; grpc-js test-server/test-metadata; undici-h2 (11/11).
  • Related open PRs, neither covering the plain end() path: node:http2: end a pushed response on its HEADERS frame when the stream was ended before respond() #38104 does the same END_STREAM-on-HEADERS for pushed streams, keyed on the parked callback (the conditions are additive; one-line rebase for whichever lands second). node:http2: send RST_STREAM when a server stream is reset #33380 owns the close()/destroy() reset semantics (including the missing closed guard in respond()); it independently carries the same RST_STREAM(NO_ERROR) routing hunk and a bare-end() test, which this PR needs in order to stand on its own.

Background

  • A response begins with a HEADERS frame; END_STREAM is a flag on the last frame the sender emits for the stream, either a DATA frame or, for a body-less response, the HEADERS frame itself (RFC 9113 8.1).
  • _final is the stream.Writable hook run once end() was called and all buffered chunks were written; 'finish' is emitted (on a later tick) after its callback is invoked. ServerHttp2Stream#_write/_writev call respond() implicitly when data is written before it.
  • StreamState in http2.ts is a per-stream bit set. FinalCalled was already set by the other _final paths; this is its first reader. The pushed-stream path instead parks the callback in bunHTTP2StreamFinal and completes it from the native END_STREAM dispatch (markWritableDone).
  • RST_STREAM closes a stream abruptly; with code NO_ERROR it is the normal way to drop a stream without a complete response (node's stream.close() defaults to it). Inbound frames are parsed by the rewritten engine (h2/connection.rs), which calls into the Sink bridge in h2_frame_parser.rs; on_stream_reset is the bridge for a received RST_STREAM. onStreamEnd and onStreamError are the two JS dispatches it can choose between, handled by the streamEnd (end-of-stream bookkeeping, destroy once both halves are done) and streamError (destroy with an error) handlers in http2.ts.
Frames the server sends on stream 1 (GET unless noted): node v26.3.0, bun 1.4.0, this branch

Measured with a TCP proxy logging frame type/flags between bun's client and the server.

server does node bun 1.4.0 this branch
end(); respond() HEADERS 0x5 DATA 0x1, HEADERS 0x4 (+ session error) HEADERS 0x5
end(); respond() (POST, body open) HEADERS 0x5 DATA 0x1, HEADERS 0x4 HEADERS 0x5
end() only nothing, 'finish' DATA 0x1 nothing, 'finish'
end(), then respond() on 'finish' / later HEADERS 0x5 DATA 0x1, HEADERS 0x5 / throws HEADERS 0x5
end(); respond({ waitForTrailers }) HEADERS 0x5, no wantTrailers DATA 0x1, HEADERS 0x4 HEADERS 0x5, no wantTrailers
additionalHeaders(102); end(); respond() HEADERS 0x4, HEADERS 0x5 DATA 0x1 HEADERS 0x4, HEADERS 0x5
end(); respondWithFD(fd) HEADERS 0x5 DATA 0x1 HEADERS 0x5
respondWithFD(fd); end() HEADERS 0x4, DATA, DATA 0x1 same same
destroy() RST_STREAM(0) DATA 0x1 RST_STREAM(0)
close() RST_STREAM(0) DATA 0x1, RST_STREAM(0) RST_STREAM(0)
close(REFUSED_STREAM) RST_STREAM(7), client errors DATA 0x1 (reset lost, client sees a clean end) RST_STREAM(7), client errors
close(); respond() throws, RST_STREAM(0) DATA 0x1, HEADERS 0x4, RST_STREAM(0), session errors HEADERS 0x5, RST_STREAM(0) (the throw is #33380)
end(); session.destroy() nothing DATA 0x1 nothing
cork(); write("hello"); end() HEADERS 0x4, DATA(5) 0x1 HEADERS 0x4, DATA(5) 0x0, DATA(0) 0x1 unchanged
respond(); close(code) (headers sent) not touched by this PR

…m was ended before respond()

Http2Stream#_final on a client-initiated server stream that has not sent
headers yet wrote an empty END_STREAM DATA frame, ahead of any HEADERS.
It now just finishes the writable and marks FinalCalled, and respond()
puts END_STREAM on the HEADERS frame for such a stream, which is what
node sends.

With that frame gone, close()/destroy() before respond() put a
RST_STREAM(NO_ERROR) on the wire like node does, so the inbound engine
bridge now routes a peer RST_STREAM(NO_ERROR) to onStreamEnd (as the
local end_stream path already did) instead of onStreamError, so the
receiving stream still emits 'end'.
@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on bun 1.4.0 and a debug build of main with a frame-logging TCP proxy (end(); respond() on a server stream sends DATA flags=0x1 then HEADERS flags=0x4; node v26.3.0 sends one HEADERS flags=0x5). Fix and tests are in this PR; the new cases in h2-conformance.test.ts fail on main and pass with the change, and the 261 vendored upstream http2 tests pass with it.

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

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

Next review available in: 58 seconds

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: 067ba3b7-62f6-4511-a4f8-47590e655881

📥 Commits

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

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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it changes HTTP/2 wire-level frame semantics (both the server _final/respond() state machine and inbound RST_STREAM(NO_ERROR) routing) and loosens an existing snapshot, a human look would still be worthwhile.

What was reviewed:

  • _final no longer writes an empty END_STREAM DATA before HEADERS on client-initiated server streams; FinalCalled gates endStream in respond() — checked that the cork() test pins why writableEnded would be wrong.
  • on_stream_reset now routes NO_ERROR through onStreamEnd(CLOSED) — confirmed this matches the legacy handle_rst_stream_frame (line ~4645) and end_stream (line ~2524) paths.
  • The dropped req error ERR_HTTP2_STREAM_CANCEL snapshot line — the fixture feeds RST_STREAM(NO_ERROR), so the clean-close routing legitimately removes that error; the test's actual invariant (exit 0, no signal) is preserved.
Extended reasoning...

Overview

This PR fixes RFC 9113 §8.1 conformance for node:http2 server streams that are end()ed before respond(). Three files change: src/js/node/http2.ts (Http2Stream#_final gains a client-initiated-stream branch that sets FinalCalled and completes the callback without writing; ServerHttp2Stream#respond forces endStream when FinalCalled is set), src/runtime/api/bun/h2_frame_parser.rs (on_stream_reset routes RST_STREAM(NO_ERROR) to onStreamEnd(CLOSED) instead of onStreamError), and test/js/node/http2/h2-conformance.test.ts (8 new wire-level tests plus one updated inline snapshot).

Security risks

None identified. This is protocol-conformance work in the Node.js compatibility layer; no auth, crypto, or untrusted-input parsing surface changes. The on_stream_reset change narrows an error path to a clean-close path for one specific code, which if anything reduces error surface.

Level of scrutiny

Medium-high. HTTP/2 frame ordering and stream-state transitions are a critical, subtle code path with many interacting variants (push vs client-initiated, half-close ordering, cork buffering, trailers). The change is small and well-argued, but it alters wire semantics observable by any peer, changes inbound RST_STREAM handling for all clients, and removes a line from an existing snapshot. The PR description is unusually thorough (frame tables measured against node v26.3.0, explicit reasoning for the FinalCalled vs writableEnded choice with a pinning test), and 261 vendored upstream http2 tests plus grpc-js/undici-h2 were run — but the interaction surface is large enough that a maintainer familiar with the h2 rewrite should confirm the state-machine reasoning.

Other factors

I verified the Rust change is consistent with the two existing paths it cites: handle_rst_stream_frame at line ~4645 and end_stream at line ~2524 both already dispatch onStreamEnd for NO_ERROR, so on_stream_reset was the odd one out. The streamEnd handler (line ~4263) handles state === 7 (CLOSED) by pushing null to the readable and destroying, which produces the 'end' then 'close' sequence the test asserts. The removed snapshot line is explained: the fixture pushes RST_STREAM(NO_ERROR) into the client during a re-entrant toString, and with the new routing that stream closes cleanly before client.destroy() cancels it — the test's purpose (no UAF, clean exit) is intact. Two related open PRs (#38104, #33380) overlap with parts of this change, which the description flags for whoever lands them.

@robobun

robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 10:28 AM PT - Aug 13th, 2026

✅ @robobun, your commit 8538bf55703061d742328c8930e697e348bada12 passed in Build #94639! 🎉


🧪   To try this PR locally:

bunx bun-pr 38170

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

bun-38170 --bun

@robobun

robobun commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: #43558 carries this fix, rebased on main. It also adds the client check that needs it (the client now refuses DATA before the response HEADERS), and it ends the writable side when the peer sends RST_STREAM(NO_ERROR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant