fix(http2): propagate session.destroy(error) to in-flight stream 'error' listeners#29056
fix(http2): propagate session.destroy(error) to in-flight stream 'error' listeners#29056cirospaciari wants to merge 3 commits intomainfrom
Conversation
…or' listeners Node's closeSession destroys open streams with the original session error (lib/internal/http2/core.js:1222-1224). Bun's destroy() called parser.emitErrorToAllStreams(code), which surfaced to streams as a numeric code that emitStreamErrorNT mapped to ERR_HTTP2_STREAM_ERROR (or nothing for code 0), losing the original Error. Now destroy() stores the Error on the session via bunHTTP2SessionDestroyError before tearing down streams; emitStreamErrorNT reads it and uses that as the stream's error_instance when an 'error' listener exists, matching Node semantics. Partial step toward Node tests test-http2-propagate-session-destroy-code.js and test-http2-server-shutdown-before-respond.js (those also need session.closed timing changes).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughPersist a session-scoped destroy-time error in HTTP/2 sessions and prefer that stored error when emitting per-stream Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 10:09 PM PT - Apr 13th, 2026
❌ @cirospaciari, your commit 7ed9d91 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 29056That installs a local version of the PR into your bun-29056 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean targeted fix with appropriate test coverage.
Extended reasoning...
Overview
The PR modifies src/js/node/http2.ts to fix error propagation when session.destroy(error) is called with an Error object. It introduces a module-private symbol bunHTTP2SessionDestroyError, stores the original error on the session before tearing down streams, then reads it back in emitStreamErrorNT so stream error listeners receive the original Error rather than a numeric code. A new test file is added to verify the behavior.
Security risks
No security concerns. The fix deals with error propagation semantics in HTTP/2 stream teardown and does not touch auth, crypto, or permissions. The new symbol is not exported via Symbol.for, keeping it module-private.
Level of scrutiny
Low. This is a small, targeted bug fix (two nearly identical one-liners in the destroy paths, one conditional reordering in emitStreamErrorNT) that aligns Bun's behavior with Node's documented semantics. The change is mechanical and the test directly validates the new behavior.
Other factors
No bugs were flagged by the automated hunting system. The test cleanly exercises the fix using Promise.withResolvers and properly tears down both client and server. The PR description transparently notes what is left out of scope for follow-up work.
There was a problem hiding this comment.
LGTM — clean, well-tested Node-semantics fix for HTTP/2 session destroy error propagation.
Extended reasoning...
Overview
Modifies src/js/node/http2.ts to propagate the original Error passed to session.destroy(error) down to in-flight streams' error listeners, matching Node.js behavior. The change is ~10 lines across three call sites: a new private symbol bunHTTP2SessionDestroyError, a write in both ServerHttp2Session.destroy and ClientHttp2Session.destroy, and a read in emitStreamErrorNT. A new test file validates same-instance identity of the error.
Security Risks
None. This is purely error-propagation logic with no auth, crypto, or permission-model involvement.
Level of Scrutiny
Low. The change is small, symmetrically applied to both session classes, and has a direct test. The logic is straightforward and the description clearly references the upstream Node.js source for the intended behavior.
Other Factors
No bugs found by the automated bug-hunting system. CodeRabbitAI also found no actionable issues. The CI build failures in the timeline are cpp/zig failures on the preceding commit; this PR only touches TypeScript, so those failures are unrelated. The autofix.ci commit resolved formatting concerns.
| if (stream) { | ||
| let error_instance: Error | number | undefined = undefined; | ||
| if (stream.listenerCount("error") > 0) { | ||
| if (typeof error === "number") { | ||
| const sessionErr = self?.[bunHTTP2SessionDestroyError]; | ||
| if (sessionErr !== undefined) { | ||
| error_instance = sessionErr; | ||
| if (typeof error === "number") stream.rstCode = error; | ||
| } else if (typeof error === "number") { | ||
| stream.rstCode = error; | ||
| if (error != 0) { | ||
| error_instance = streamErrorFromCode(error); |
There was a problem hiding this comment.
🔴 The session-level symbol bunHTTP2SessionDestroyError is set before emitErrorToAllStreams is called but is never cleared afterward. Any emitStreamErrorNT callbacks that were already queued via process.nextTick before session.destroy(myErr) was called will incorrectly receive myErr as their error instead of the stream's actual close reason (e.g., the RST_STREAM code). To fix this, either clear the symbol immediately after emitErrorToAllStreams returns, or pass the error explicitly as a parameter through the call chain rather than via a session-level side channel.
Extended reasoning...
What the bug is and how it manifests
The PR introduces bunHTTP2SessionDestroyError, a session-scoped symbol set on the session object before parser.emitErrorToAllStreams(code) is called. The function emitStreamErrorNT reads this symbol to decide which error to deliver to stream 'error' listeners. The symbol is never cleared after emitErrorToAllStreams returns.
The specific code path that triggers it
Native stream close/error callbacks (streamClose/streamError) schedule emitStreamErrorNT asynchronously:
process.nextTick(emitStreamErrorNT, self, stream, error, true, ...)
These nextTick entries are queued immediately when the native parser fires its callback, before the current synchronous call stack unwinds.
Why existing code doesn't prevent it
If a stream was already closing (its streamClose/streamError native callback had already fired) before session.destroy(myErr) is invoked in the same synchronous turn, the nextTick for that stream is already in the queue. When destroy() then sets session[bunHTTP2SessionDestroyError] = myErr and calls emitErrorToAllStreams, the native layer may skip the already-closed stream. When the pre-queued nextTick fires, emitStreamErrorNT reads the still-set symbol and substitutes myErr — completely overwriting the stream's original error reason.
Concrete step-by-step proof
- Server sends RST_STREAM(CANCEL) for stream A → native parser fires
streamClose(session, streamA, NGHTTP2_CANCEL)synchronously → JS callback schedulesprocess.nextTick(emitStreamErrorNT, session, streamA, NGHTTP2_CANCEL, ...). - In the same I/O callback (or a synchronous event handler triggered by it, e.g.,
streamEndcallingself.destroy()as already done at line ~2869), user code callsclient.destroy(myErr). ClientHttp2Session.destroy()setssession[bunHTTP2SessionDestroyError] = myErr(line 3811) and callsparser.emitErrorToAllStreams(0). Stream A is already closed at the native layer, so it is not re-processed.- The nextTick from step 1 fires.
emitStreamErrorNTreadsself[bunHTTP2SessionDestroyError]which is stillmyErr. It setserror_instance = myErrand emitsmyErrto stream A's'error'listener. - Stream A reports
myErr(e.g.,ERR_HTTP2_SESSION_ERROR) instead ofERR_HTTP2_STREAM_ERROR/CANCEL— the wrong error for a stream that closed before the session was destroyed.
Impact
Application code relying on the stream error's code or message to distinguish why a stream failed will receive incorrect information. A stream that received a valid server RST_STREAM (e.g., rate limiting, request cancellation) would show the client-side session destroy error instead, making error handling and diagnostics incorrect.
How to fix it
Option A (simpler): Clear the symbol immediately after emitErrorToAllStreams returns:
if (streamDestroyError \!== undefined) this[bunHTTP2SessionDestroyError] = streamDestroyError;
parser.emitErrorToAllStreams(code || constants.NGHTTP2_NO_ERROR);
this[bunHTTP2SessionDestroyError] = undefined; // clear after useThis prevents any pre-queued nextTicks from picking up the symbol since they fire after the current synchronous block.
Option B (more robust): Pass the error explicitly through emitErrorToAllStreams → emitStreamErrorNT as a parameter instead of using a session-level side channel, eliminating the shared mutable state entirely.
2 participants
Node's
closeSessiondestroys open streams with the original session error (lib/internal/http2/core.js:1222). Bun'sdestroy()calledparser.emitErrorToAllStreams(code), which surfaced to streams as a numeric code thatemitStreamErrorNTmapped toERR_HTTP2_STREAM_ERROR(or nothing for code 0), losing the originalError.Now
destroy()stores the Error on the session via a private symbol before tearing down streams;emitStreamErrorNTreads it and uses that as the stream's error when an'error'listener exists, matching Node semantics.This is a partial step toward Node tests
test-http2-propagate-session-destroy-code.jsandtest-http2-server-shutdown-before-respond.js(those also needsession.closedtiming changes shipped separately).