Conversation
A respond() whose HEADERS frame carries END_STREAM (204, 205, 304, a HEAD request, or endStream: true) ended the writable side only after the native request() call. When the peer had already half-closed, that call drives the stream straight to closed and dispatches onStreamEnd synchronously, so _destroy ran with the writable side still open and treated the teardown as a client abort: a spurious 'aborted' event and stream.aborted === true. End the writable side before submitting the frame, as node does. _final parks its callback instead of writing an empty DATA frame, and the onStreamEnd dispatch settles it.
|
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)
WalkthroughThis PR fixes HTTP/2 stream shutdown sequencing for cases where respond() submits HEADERS with END_STREAM. A new StreamState.EndStreamOnHeaders flag prevents an extra empty DATA frame from _final, and respond() logic is reordered to end the writable side before native request() dispatch. Regression tests validate correct lifecycle event ordering. ChangesHTTP/2 END_STREAM shutdown sequencing
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:09 PM PT - Jul 5th, 2026
✅ @robobun, your commit 69433a5af677fe10fdbefefcbd1457b4ea75fde2 passed in 🧪 To try this PR locally: bunx bun-pr 33375That installs a local version of the PR into your bun-33375 --bun |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
I didn't find any bugs, but this reorders writable-side teardown against a synchronous native dispatch in the HTTP/2 stream state machine — worth a human sanity-check on the _final parking path.
Extended reasoning...
Overview
This PR fixes a spurious 'aborted' event on ServerHttp2Stream when respond() sends a HEADERS frame carrying END_STREAM (status 204/205/304, HEAD requests, or { endStream: true }) after the peer has already half-closed. The fix moves this.end() to before the native request() call (matching Node's ordering in lib/internal/http2/core.js), and introduces a new StreamState.EndStreamOnHeaders bit so that _final parks its callback rather than writing an empty DATA frame ahead of the HEADERS. A follow-up markWritableDone(this) after request() settles the parked callback idempotently in case the native layer didn't dispatch.
Two files touched: ~25 net lines in src/js/node/http2.ts (new enum bit, a guard in _final, reordered end() in respond()), and ~90 lines of new tests in test/js/node/http2/node-http2.test.js covering the reported statCheck veto plus the full class of end-stream response shapes.
Security risks
None identified. This is stream-lifecycle event ordering in the Node compat layer; no untrusted input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Medium-high. HTTP/2 stream state is notoriously order-sensitive, and this change threads a new state bit through three interacting sites (respond(), _final, markWritableDone) while relying on whether end() → _final runs synchronously relative to a native call that can itself synchronously destroy the stream. The author's analysis is thorough and the fix mirrors upstream Node, but the interaction between the parked _final callback and the various markWritableDone call sites (in markStreamClosed, in the new post-request() call, and in the destroy path) is subtle enough that a maintainer familiar with this file's history should confirm no path leaves the callback unsettled.
Other factors
The PR description demonstrates before/after test failures, a clean full-file run (296 pass / 0 fail), and identical Node parallel-test failure sets pre/post. The bug-hunting pass found nothing. The tests look solid — they assert exact event sequences and stream.aborted, cover five end-stream variants, and use port: 0 with proper cleanup. Still, per the approval guidelines this is not a simple/mechanical change: it's a state-machine ordering fix in a Node-compat hot path, so I'm deferring rather than auto-approving.
markWritableDone only needs to run from the native onStreamEnd dispatch, so _final can call its callback straight back instead of parking it: 'finish' is queued on the next tick either way, after request() has put the HEADERS frame on the wire. This is what node's _final does here (handle.shutdown() reports a synchronous finish and the callback runs inline), and it means no path can leave the callback unsettled if request() throws.
|
Good call on the Parking the callback bought nothing. That also matches node more closely. Its The new Same verification as before on the simplified version: For the record on the earlier red build (#68558): both failures were agent infra, neither ran the tests. |
…nd() Ending the writable side before the native request() call, the way node does, does not hold in bun: node validates headers in JS before ending, while bun validates them inside request(). A respond() that threw on a bad header left the writable side already ended, so the documented catch-and-retry recovery (respond again with a body-carrying status, then end) silently dropped its END_STREAM and hung the client. Keep this.end() where it was and record the in-flight END_STREAM on the stream status instead, cleared once the frame is out. _destroy reads it to tell a completed response apart from a client abort. A request() that threw submitted nothing, so the writable side stays genuinely open and the retry works. EndStreamOnHeaders has to be its own bit: NativeClosed and WritableClosed are also set when the peer cancels with RST_STREAM(NO_ERROR), which dispatches onStreamEnd(CLOSED) too, and suppressing 'aborted' there would swallow a real client cancel. Also shrink the maxSessionMemory stress test to 1k requests under debug+ASAN (88s -> 13s); at 10k it sat close enough to its own timeout to fail on a loaded machine.
|
You're right, and thanks - that's a real regression. Confirmed it with your exact scenario:
I went with neither of your two suggestions though. Pre-validating in JS would duplicate the native header encoder's validation on every Instead One thing worth recording, since it's the trap in this area: the bit has to be its own flag rather than reusing Added a regression test for your retry path so the reorder can't come back. Also shrank the Verification on the reworked version: 297 pass / 0 fail for the file, 2 fail on |
There was a problem hiding this comment.
LGTM — the flag-based redesign in 6f98db0 addresses my earlier concern (writable side stays open when request() throws), and the new test covers that exact case.
Extended reasoning...
Overview
This PR fixes a spurious 'aborted' event on ServerHttp2Stream when respond() sends a HEADERS frame carrying END_STREAM (204/205/304, HEAD, or endStream: true) after the peer has already half-closed. The final implementation is minimal: a new StreamState.EndStreamOnHeaders bit is set immediately before the native request() call and cleared in a finally, and _destroy checks that bit to skip the 'aborted' emission. Nothing else changes — this.end() still runs after request() exactly as before, and _final is untouched in the cumulative diff.
Response to prior review
My earlier comment flagged that the previous revision (which moved this.end() before native request()) would leave the writable side ended if native header validation threw, breaking catch-and-retry. Commit 6f98db0 abandoned the reordering entirely in favor of the flag-only approach, so this.end() no longer precedes validation. The try/finally guarantees the flag is cleared even on throw, and a new test ("a respond() that throws on invalid headers leaves the writable side open") asserts exactly the recovery scenario I described (ERR_HTTP2_INVALID_PSEUDOHEADER → respond({':status':500}); end('err') → client receives 500 + body). That fully resolves the concern.
Security risks
None. This is stream-lifecycle event bookkeeping in the Node.js http2 compat layer; no auth, crypto, parsing of untrusted data, or resource limits are touched. The try/finally is defensive only — it doesn't swallow the exception.
Level of scrutiny
Moderate. HTTP/2 stream state machines are subtle, but this change is deliberately narrow: one bit set/checked/cleared around an existing call, with no reordering of side effects relative to pre-PR behavior. The pre-PR code path already ran this.end() after a synchronous destroy; this PR only stops _destroy from mislabeling that as an abort.
Other factors
Test coverage is thorough: the reported statCheck veto, the wider class (304/204/205/HEAD/endStream:true after request-body 'end'), and the throw-then-recover edge case. The author verified 296/296 pass on the http2 test file and an identical parallel-test failure set vs main. The unrelated MAX_SESSION_MEMORY_REQUESTS change shrinks a stress-test workload under debug/ASAN rather than raising its timeout, which matches the repo's test guidance. No CODEOWNERS apply. The PR description is stale (it describes the earlier reordering approach), but the code and commit message are accurate.
|
Thanks - and the description is current now, it just got rewritten a few minutes after 6f98db0 was pushed, so the review snapshotted the old one. It describes the flag-based approach and the throw-then-recover case. For anyone looking at the red check: the only failing job on build #68587 is Same agent-side artifact-download timeout took out that lane on the previous build too. The rest of the build is still running. |
Build #68587 was green on 280 jobs; the three reds never touched this diff: darwin aarch64 (both shards) failed with 'buildkite-agent artifact download timed out after 120s' before running any test, ubuntu x64 OOM-killed v8-heap-snapshot.test.ts, and the windows 2019 agent failed to provision.
There was a problem hiding this comment.
The rework addresses my earlier concern — this.end() stays after native request() and the new EndStreamOnHeaders bit is scoped tightly with try/finally, so the throw-then-recover path is preserved (and now covered by a test). The fix is small and well-reasoned, but the HTTP/2 stream state-machine interaction with synchronous native dispatch is subtle enough that someone familiar with h2_frame_parser.rs should give it a quick look.
Extended reasoning...
Overview
This PR fixes a spurious 'aborted' event on ServerHttp2Stream when respond() sends a HEADERS frame with END_STREAM (204/205/304, HEAD, or endStream: true) after the peer has half-closed. Two files touched: src/js/node/http2.ts (~30 lines net) and test/js/node/http2/node-http2.test.js (+135 lines of tests, plus a debug-build workload reduction for an unrelated stress test).
The fix adds a StreamState.EndStreamOnHeaders bit that respond() sets immediately before calling native request() and clears in a finally. _destroy checks that bit alongside _writableState.ending to distinguish a completed body-less response from a genuine client abort. this.end() remains after the native call, unchanged from pre-PR — so a request() that throws on header validation leaves the writable side untouched and catch-and-retry recovery still works.
Prior review
I previously flagged (now-resolved inline comment) that an earlier revision moved this.end() before native request(), which regressed the throw-then-recover path: a validation error from native request() would surface with the writable side already ended, and a retry respond() + end() would silently hang the client. The author confirmed the regression empirically, reworked to the flag-based approach in 6f98db0, and added a regression test for that exact scenario. The current diff no longer has that problem — verified by reading respond() at lines 3124-3157: the flag is cleared in finally, and if (endStream) this.end() runs only after request() returns successfully.
Security risks
None identified. This is event-emission timing in the Node-compat HTTP/2 server layer; no auth, crypto, permissions, or untrusted-input parsing is touched. The change narrows when 'aborted' fires — it does not relax any validation or resource limit.
Level of scrutiny
Medium. The mechanical change is small (one enum bit, one && clause, one try/finally), but correctness depends on understanding when native request() synchronously dispatches onStreamEnd(CLOSED) → stream.destroy(), and why the new bit must be distinct from NativeClosed/WritableClosed (both are also set on RST_STREAM(NO_ERROR), which is a real abort). The author's reasoning is sound and well-documented in the PR description and code comments, and the Node parallel-test-suite comparison (identical failure set) is reassuring — but this is exactly the kind of state-machine subtlety where a second pair of eyes from someone who owns h2_frame_parser.rs is worth the cost.
Other factors
- Test coverage is thorough: the reported
statCheckveto, five END_STREAM variants responding after request-body'end', and the throw-then-recover regression I flagged. All wire failure events to reject; no sleeps. - The author verified genuine aborts (client
close(CANCEL), clientclose(NO_ERROR), serverdestroy()mid-body) still fire'aborted', matching Node. - No CODEOWNERS entry for
src/js/node/http2.ts. - The unrelated
maxSessionMemorystress-test workload reduction (10k → 1k under debug/ASAN) is a sensible flake mitigation and doesn't weaken the assertion. - CI red on the last two builds is agent-side (artifact download timeout on darwin-aarch64, DNS failure on windows-x64-baseline) per the author's note; neither ran tests.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-05 and it conflicts with main. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Repro
A server stream that answers with a body-less status emits a
'aborted'event and flipsstream.abortedtotrue, even though nothing aborted and noRST_STREAMis exchanged. The reported shape is the documentedstatCheckveto (reply 304 from a cache validator, cancel the file send):It is not specific to
statCheck. Anyrespond()whose HEADERS frame carriesEND_STREAMhits it as long as the request body has already been received:Same for
204,205, aHEADrequest, andrespond(headers, { endStream: true }).Cause
ServerHttp2Stream.respond()ended the writable side after the nativerequest()call:When the peer has already half-closed, submitting HEADERS with
END_STREAMmoves the stream toCLOSEDinsiderequest(), which dispatchesonStreamEnd(7)synchronously. That handler callsstream.destroy(), and_destroy()reads_writableState.endingto decide whether the response was cut short:this.end()had not run yet, soendingwasfalseand the clean close was recorded as a client abort. The synchronousrespond()case never showed it because there the stream is stillOPENwhen the handler runs, so theCLOSEDtransition lands afterrespond()returns.Node ends the writable side before submitting, in
lib/internal/http2/core.js:Fix
Copying that ordering does not hold in bun. Node validates headers in JS (
mapToHeaders) before it ends the writable side, so a bad header throws with the stream untouched. Bun validates inside the nativerequest(), so ending first means a throwingrespond()leaves the writable side already ended, and the documented catch-and-retry recovery silently hangs the client:So
this.end()stays where it was. Insteadrespond()records, on the stream status, that a HEADERS frame carryingEND_STREAMis in flight, and clears it once the native call returns._destroyreads that bit to tell a completed response apart from a client abort. Arequest()that threw submitted nothing, so the bit is cleared, the writable side is still genuinely open, and the retry works.EndStreamOnHeadersneeds to be its own bit rather than reusingNativeClosedorWritableClosed. Both of those are also set when the peer cancels withRST_STREAM(NO_ERROR), which dispatchesonStreamEnd(CLOSED)as well (h2_frame_parser.rs:4335) - keying off either would swallow a real client cancel.Verification
New tests in
test/js/node/http2/node-http2.test.js:statCheckveto, and the wider class (304,204,205,HEAD,endStream: trueresponding once the request body has been received). Both fail onmain, assertingstream.aborted === falseand that no'aborted'fires while'finish'still does.respond()leaves the writable side open, sorespond()+end()recovery still reaches the client.Genuine aborts were checked to still fire: a client
close(NGHTTP2_CANCEL)mid-body, a clientclose(NGHTTP2_NO_ERROR)mid-body (theonStreamEnd(CLOSED)path), and a server-sidedestroy()mid-body all still emit'aborted'withstream.aborted === true, matching node.Before / after, and no regressions
All 293
test-http2-*/test-diagnostics-channel-http2-*files undertest/js/node/test/parallel/run against the debug build: 234 pass, 56 fail, an identical failure set before and after the change.This also shrinks the
maxSessionMemorystress test to 1k sequential requests under debug+ASAN. At 10k it took ~88s against its own 150s timeout, close enough to fail on a loaded machine; release still runs the full 10k. The file now runs in 45s instead of 140s.