node:http2: add the goawayCode and goawayLastStreamID session getters - #37550
Conversation
Node's Http2Session exposes the error code and Last-Stream-ID of the GOAWAY frame a session received as goawayCode and goawayLastStreamID, reporting 0 for both until one arrives. Bun's client and server sessions had neither property. The received code was already kept under kGoawayCode; store the received Last-Stream-ID alongside it in both onGoAway handlers and define the two getters on the shared Http2Session base class, where node defines them.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: merged. Reproduced on bun 1.4.0 and on main (da3851e) with the script in the PR body: both session classes returned |
There was a problem hiding this comment.
LGTM — straightforward Node.js compat addition mirroring node's Http2Session getters.
What was reviewed:
- Getters on the shared
Http2Sessionbase use|| NGHTTP2_NO_ERROR/|| 0, matching node'slib/internal/http2/core.jsexactly;NGHTTP2_NO_ERRORis destructured into module scope at http2.ts:1747 so it's in scope. - Both
onGoAwayhandlers (server + client) setkGoawayLastStreamIDbefore emitting'goaway', alongside the pre-existingkGoawayCodewrite — no sibling site missed. - Tests cover both directions, initial state, getter descriptors, persistence after destroy, and graceful GOAWAY; they use
port: 0, local servers,describe.concurrent, and try/finally cleanup.
Extended reasoning...
Overview
This PR adds two getters, goawayCode and goawayLastStreamID, to the shared Http2Session base class in src/js/node/http2.ts, plus a new kGoawayLastStreamID symbol and one assignment in each of the two goaway native-callback handlers (server and client). ~10 lines of production code and ~130 lines of tests in test/js/node/http2/node-http2.test.js.
Security risks
None. This exposes read-only getters over state that was already passed to the userland 'goaway' event; no new inputs are parsed, no auth/crypto/permissions paths touched.
Level of scrutiny
Low-to-moderate. This is a pure Node.js API-surface compat gap: node defines these getters on Http2Session and bun did not. The implementation is a direct transcription of node's (this[kState].goawayCode || NGHTTP2_NO_ERROR, etc.). The kGoawayCode symbol was already being set in exactly the same place for stream teardown; this PR adds the sibling kGoawayLastStreamID write next to it and surfaces both via getters. No behavior change to existing code paths — the new symbol has no other readers.
Other factors
- Verified
NGHTTP2_NO_ERRORis a bare module-scope const (destructured at http2.ts:1747), so the getter body resolves it. - Checked that both native-callback
goawayhandlers (ServerHttp2Session at :4462 and ClientHttp2Session at :5458) received the symmetric change — the "fix the whole class" rule is satisfied. - The
||fallback (vs??) matches node verbatim and is observably equivalent here since the fallback values (0) equal the only falsy value the fields can hold. - Tests follow harness conventions:
describe.concurrent,port: 0, awaiting real events (no sleeps),try/finallycleanup registered before assertions, and assert the strongest invariant (exact getter values inside the event, after destroy, and that the sending side's getters stay0/0). The descriptor test walks the prototype chain rather than assuming placement, so it's robust to where bun vs node define the accessor. - No CODEOWNERS entry for this file. No outstanding reviewer comments.
|
@robobun are there any tests upstream in the Node.js test suite we can add now that pass? |
|
Checked, and the answer is no for this change specifically: nothing in node's test suite reads Since I was looking anyway, I diffed upstream
|
Repro
node v26.3.0 (and this branch):
bun 1.4.0 and main:
Cause
Node's
Http2Session(lib/internal/http2/core.js) has two getters describing the GOAWAY frame the session received:onGoawayDatafills both in before emitting'goaway', and they keep their values after the session is destroyed. NeitherClientHttp2SessionnorServerHttp2Sessioninsrc/js/node/http2.tsdefined them. The received code was already tracked (kGoawayCode, used to pick the rst code of streams torn down by the GOAWAY); the received Last-Stream-ID was passed to the event and then dropped.Fix
Store the received Last-Stream-ID next to the code in both
onGoAwayhandlers (before'goaway'is emitted, as in node) and define the two getters once on the sharedHttp2Sessionbase class, where node defines them. They describe received GOAWAYs only: agoaway()this side sends does not touch them, same as node.This is a plain node compat gap. Code written for node that reads these (for example
session.goawayCode !== NGHTTP2_NO_ERRORto tell a graceful shutdown from an errored one) getsundefinedon bun today, so such a check treats every session as errored. No type or docs changes: node does not document these getters and@types/nodedoes not declare them, they are just part of theHttp2Sessionsurface.Verification
Four tests added to
test/js/node/http2/node-http2.test.js(describe "http2 session goawayCode / goawayLastStreamID"). All four fail on bun 1.4.0 and pass with this change; the same scenarios run as a standalone script under node v26.3.0 and under the fixed debug build print identical output.0/0on a fresh client and server sessiongoaway(ENHANCE_YOUR_CALM, 1): the client reports11/1inside the'goaway'event and still after it was destroyed; the server's own getters stay0/0goaway(CANCEL, 2): the server session reports8/2, with the same persistence checkgoaway(): the client reports0/1(the implicit last processed stream id)With the debug build,
node-http2.test.js(351 pass, 6 skip),h2-conformance.test.tsand the vendored node http2 goaway/shutdown tests pass.