Repository navigation
Conversation
endAfterHeaders threw ERR for a stream with no id yet, and flipped from true to false once the native stream was freed. It is now a JS state flag set from the END_STREAM bit of the received header block, which is what node tracks in onSessionHeaders. The native end_after_headers flag and its getter go away. rstCode reads 0 until the stream is reset (node's NGHTTP2_NO_ERROR), not undefined. A client stream reports headersSent true from construction, as node's ClientHttp2Stream constructor does. The http2.connect() listener receives the socket as its second argument.
|
Warning Review limit reached
On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file. Or wait 2 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughChangesHTTP/2 stream lifecycle
Suggested reviewers: Merge Risk: ⚪ Minimal · up to HTTP/2 stream lifecycle getters now retain Node-compatible state and connect listeners receive the socket. The changed behavior is covered across pending, open, and closed stream states, with no outstanding merge-readiness risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
CI on 2bacef1: the only red lane is test/js/node/test/parallel/test-crypto-dh-leak.js on debian 13 x64-asan, an RSS assertion that is red on main and on every other PR build this week. It is reported for main-break triage. This diff does not touch crypto. test/js/node/http2/ passes on every lane, the three other entries are flaky retries that passed. Ready for a maintainer. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…onnect() close() before the connection completes makes #onConnect() destroy the session, which drops the socket. The 'connect' event captures it first, so the listener does too. The test wires the session's error and close events to reject.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 5:48 AM PT - Sep 8th, 2026
❌ @robobun, your commit f410867 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 41945That installs a local version of the PR into your bun-41945 --bun |
…ream node's onSessionHeaders (lib/internal/http2/core.js) writes endAfterHeaders only when the header block creates the stream object. A client request stream already exists when its response arrives, so it keeps false even when the response HEADERS frame carries END_STREAM, and a trailers block never changes the value. Drop the client-side write and cover both cases.
There was a problem hiding this comment.
LGTM — every earlier thread is addressed, and the new third test settles the trailers question against real Node: onSessionHeaders writes endAfterHeaders only when the block creates the stream, so my previous comment there was wrong.
Verified the follow-ups landed: the connect listener captures bunHTTP2Socket before #onConnect() can null it; the server test awaits the server stream's own 'close' before reading states[1]; the connect-listener test wires error/close to reject.
Native side is pure deletion — end_after_headers, its on_headers_complete write, the request-path write, and the getEndAfterHeaders host fn + .classes.ts entry have no remaining readers.
Also looked at pushed client streams keeping endAfterHeaders === false — base's native flag was set by trailers and the outgoing END_STREAM too, so that path was already wrong and this PR doesn't make it worse.
Extended reasoning...
Overview
The PR moves three Http2Stream getters (endAfterHeaders, rstCode, headersSent) from native-backed lookups or undefined defaults to JS-side class fields so they answer correctly across the whole stream lifecycle (pending → open → closed), matching Node. It also passes the raw socket as the second argument to the http2.connect() inline listener. Native side deletes the now-dead end_after_headers field, its writes in on_headers_complete and the request path, the getEndAfterHeaders host function, and its .classes.ts entry. Four tests in the existing node-http2.test.js cover client pending/open/closed, server persistence past close, the "only the opening block sets it" semantics (trailers and response HEADERS do not), and the connect-listener signature.
Security risks
None. This is Node-compat surface for read-only getters and a callback argument. No parsing of untrusted input changed; the native deletions remove code rather than add it. The connect listener already had access to session.socket; passing the raw socket alongside is what Node does.
Level of scrutiny
Moderate — node:http2 is a compat module where observed Node behavior is the spec, and the change touches both JS builtins and the Rust frame parser. This is the sixth review round; the earlier five produced four inline threads, all now addressed by commits: socket captured before #onConnect() (d9a2f91), server-close race awaited (8b74c06), connect-listener test wires failure events, and the endAfterHeaders/trailers question resolved by 1feb320 with a test that pins Node's actual semantics (my prior claim that Node overwrites on trailers was incorrect — Node only assigns in the stream === undefined branch of onSessionHeaders). The hunt exited on dry_streak.
Other factors
The native changes are strictly subtractive and satisfy REVIEW.md's "delete dead code in the same PR that makes it dead". JS-side additions follow src/js/ conventions: private symbol for internal state, class-body field defaults, .$call for the listener. Tests use port: 0, loopback only, Promise.withResolvers on events with error paths wired to reject, and try/finally cleanup registered before assertions. The one investigated-and-dismissed candidate (pushed client streams keeping endAfterHeaders === false) is not a regression against base — the removed native flag was itself set from the wrong frames (trailers, outgoing END_STREAM), so base was already divergent there and this PR doesn't widen the gap. No outstanding third-party objections in the timeline.
… carries END_STREAM (#42348) ### Problem - A `node:http2` stream emits no `'end'` when a listener calls `close(code)` with an error code (2, 7, 11) after the peer's half ended on a HEADERS frame: server `'stream'` and `'trailers'`, client `'response'` and `'trailers'`, the `pushStream()` callback. Node v26.3.0 and Bun 1.4.2 emit `'end'`, `'error'`, `'close'`. Main skips `'end'`. - Both `streamHeaders` handlers (`src/js/node/http2.ts`) emit the event inside the HEADERS dispatch. The frame's END_STREAM reaches the readable only in the later `streamEnd` dispatch, after the listener's ticks drain. By then the error has destroyed the stream and suppressed `'end'`. A pushed server stream never got an EOF. - `close()` hid this with an unconditional `push(null)` until #33607 (fc479fb) gated it. ### Fix - `endInboundHalf()` gives the readable its EOF. Both `streamHeaders` handlers call it before the event of a HEADERS frame that carries END_STREAM. `pushStream()` calls it for the new stream. Node's `onSessionHeaders` and `pushStream` do the same. - The `close()` gate stays. With the peer's half still open, an error code gives no `'end'`, as on Node. - Verified: `test/js/node/http2/node-http2-client-close.test.ts` (56 new cases, 24 fail on main, all 68 pass on Node v26.3.0). Also `test/js/node/http2/`, Node's `test-http2-*`, grpc-js, `serve-http2`, fetch HTTP/2. ### Background - The native frame parser calls JS once per event: `streamHeaders` for a header block, then `streamEnd` if the frame carried END_STREAM. The `nextTick` queue drains between the two. - `push(null)` gives a Readable its EOF. `'end'` fires a tick later, unless the stream has an error by then. - `close(code)` with a code other than NO_ERROR or CANCEL destroys the stream with `ERR_HTTP2_STREAM_ERROR`. <details><summary>Notes</summary> The regression is unreleased. A fuzz ledger found it, no user reported it. "Before" below is `1.4.3-canary.1+4ff919377`, the last build before #33607. The report measured the same sequences on 1.4.2. Server-side events for `close(2)` inside the `'stream'` listener. The client sent `request({':path':'/'}).end()` with no body: | when | Node v26.3.0 | before | main | this PR | | --- | --- | --- | --- | --- | | sync | aborted, finish, end, error, close | as Node | aborted, finish, error, close | as Node | | nextTick | aborted, end, finish, error, close | as Node | aborted, finish, error, close | as Node | | setImmediate | end, aborted, finish, error, close | as Node | as Node | as Node | | setTimeout | end, aborted, finish, error, close | as Node | as Node | as Node | Codes 7 and 11 behave like 2. Codes 0 and 8 agree on all four builds. The other sites, `close(2)` or `close(11)` in the listener or one tick later. The column says whether `'end'` fires before `'error'`: | listener | Node v26.3.0 | before | main | this PR | | --- | --- | --- | --- | --- | | server `'stream'`, after `respond({endStream: true})` | yes | yes | no | yes | | server `'trailers'` | yes | yes | no | yes | | client `'response'`, response ended on HEADERS (204) | yes | yes | no | yes | | client `'trailers'` | yes | yes | no | yes | | `pushStream()` callback | yes | yes | no | yes | Three probe scripts cover these sites with codes 0, 8, 2, 11 (and 7 for the push), sync and nextTick. Their output on this branch is identical to Node v26.3.0 on all 82 cells. Cells that #33607 moved to Node's sequence and that this PR keeps (`'end'` for `close(2)` on a server stream): | request | close(2) runs | Node v26.3.0 | before | main and this PR | | --- | --- | --- | --- | --- | | `end('hello')` | sync or nextTick in `'stream'` | no `'end'` | `'end'` | no `'end'` | | `end('hello')` | in `'data'`, or a tick after it | no `'end'` | `'end'` | no `'end'` | | never ended | sync, nextTick, setImmediate, setTimeout | no `'end'` | `'end'` | no `'end'` | END_STREAM on a DATA frame is not part of this change. nghttp2 ignores frames for a stream that `close()` already reset, so Node emits no `'end'` in the `'data'` rows above, and main already agrees. Sites left out on purpose: - The client `'headers'` event (a 1xx block). A 1xx block with END_STREAM is malformed. Node reports it as `'response'`. Nothing changes there. - The client `streamPush` handler (PUSH_PROMISE). A PUSH_PROMISE cannot carry END_STREAM. The two shapes the report proposed, and why this PR uses neither: - "Apply the gate only to client streams" brings back `'end'` on server streams whose request is still open. Node does not emit it there. It also leaves the client `'response'` and `'trailers'` cells broken. - "Apply the gate only when the readable has not received END_STREAM": in the failing cells the JS readable has not received it yet when `close()` runs. That is the defect. `close()` would have to ask the native layer, and any other code that looks at the readable inside the listener would still see the stale state. Related open PRs: - #33380 (server-side RST_STREAM) is why the client still sees `rstCode=0` and no `'error'` in the no-body cells. It does not touch these hunks. With it, `close()` no longer sends END_STREAM first, so some `'stream'` cells would pass without this change. The `after respond()` cells stay red without it on both main and #33380. - #41945 gives `rstCode` a default of 0. If it lands, the `rstCode` guard in `endInboundHalf()` is dead and can go. `endInboundHalf()` also sets `rstCode = 0` when no reset set it, as the `streamEnd` handlers did. Without that, a listener that resumes the stream inside `'stream'` reads `rstCode === undefined` in its `'end'` handler, because `'end'` now fires before the `streamEnd` dispatch. Node reports 0. The Node cross-check inside the test file is now skipped on musl. Alpine's `node` segfaults at a random point of this file under `node --test` (alpine 3.23 aarch64, also on main in build 114123, before this change). The CI runner fails a file for any new core dump, whichever process wrote it. The glibc, macOS and Windows lanes keep the cross-check. Suites run on the debug build: `test/js/node/http2/` (576 pass, 6 skip), 261 `test-http2-*` and 18 `test-diagnostics-channel-http2-*` Node tests, `serve-http2`, `serve-http2-protocol`, `serve-http2-lifecycle`, `node-http2-ping-flood-staged`, `fetch-http2-client`, `fetch-http2-adversarial`, `fetch-http2-leak`, `undici-h2`, `wpt-h2`, `grpc-js`, the http2 regression tests (25589, 24924, 26915, 29073). Local failures that also occur without the change: the `test-outlier-detection.test.ts` ejection tests (5 s timeouts on a loaded debug build, they flip between runs on main too), `test tonic server`, the grpc-js DNS tests (no network), `http2-wrapper.test.ts` (ECONNREFUSED, on the release build too), one `AsyncLocalStorage.test.ts` timing test (debug build timeout). </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http2/node-http2-client-close.test.ts <!-- robobun:evidence:end -->
This branch has not been deployed
2 participants
Problem
stream.endAfterHeadersthrowsInvalid stream idon a stream that has no id yet (a request issued before the session connects, or one queued behindmaxConcurrentStreams). Node returnsfalse. The same getter flips fromtruetofalseonce the native stream is freed, so a server stream that received an END_STREAM request reports the wrong value in its own'close'handler. Node keeps the value.stream.rstCodereadsundefineduntil the stream is reset. Node reads0(NGHTTP2_NO_ERROR). A client stream'sheadersSentreadsundefined. Node readstruefrom construction.http2.connect(url, options, listener)listener receives(session, undefined). Node passes(session, socket), the same arguments as the'connect'event.Fix
endAfterHeadersbecomes a JS state flag, set from the END_STREAM bit of the header block that opens the stream (the request block on a server). That is the one place node'sonSessionHeaderswrites it: a client request stream keepsfalseeven for an END_STREAM response, and trailers never change it. It is a plain field, so it answers before the stream has an id and after the stream closed. The nativeend_after_headersflag and itsgetEndAfterHeaders()host function are removed: nothing reads them now, and the native flag was also set by trailers and by an outgoing END_STREAM, which node never reports.rstCodeinitializes to0. Every existing check reads it for truthiness, so the reset paths are unchanged.ClientHttp2StreamdeclaresheadersSent = true(node setsSTREAM_FLAGS_HEADERS_SENTin its constructor).ServerHttp2Streamkeeps the field it already had, now inherited.connect()listener getsthis[bunHTTP2Socket]as its second argument, the raw socket, as the'connect'event already does.test/js/node/http2/node-http2.test.js(4 new tests, all fail on bun 1.4.3). Also the rest oftest/js/node/http2/and node's 256test-http2-*.jsfiles.Background
stream[kState]), so a getter keeps answering after the C++ stream is gone. Bun answers some getters by calling into the native session, which only knows about live streams, so the same read throws or changes answer at the end of the stream's life.endAfterHeadersmeans "the header block that opened this stream carried END_STREAM", so the peer will send no body. Node sets it inonSessionHeadersonly when that block creates the stream object, from the same frame flags that the'stream'event carries as itsflagsargument.rstCodeis the RST_STREAM error code.0isNGHTTP2_NO_ERROR, which is what a stream that was never reset reports.Notes
Probe, node v26.3.0 against bun 1.4.3 and this branch. A client request is issued before
'connect', so it starts with no id:Server side, a GET request (END_STREAM on the request block), read in the
'stream'handler and again in'close':POST with a body and trailers, server responds
204withendStream: true(flags 5 on the response HEADERS):Two more faces from the same report are already covered by open PRs and are not in this diff:
session.originSetthrowing inside a session'error'handler (#33792, which defersencryptedto connect) andsession.statereturningundefinedafterdestroy()(#33597).The listener's socket is the raw
net.Socket.session.socketis aProxyover the session, in node as well, so the two are not the same object in either runtime.Suites run with the debug build:
bun bd test test/js/node/http2/(502 pass, 6 skip) andtest/js/node/test/parallel/test-http2-*.js(256 pass).