Conversation
ServerHttp2Stream#respond() passed the caller's options object to the native HEADERS writer. ClientHttp2Session.request() uses the same writer, so it read parent, weight, exclusive, silent and signal from the object. An out-of-range parent or weight closed the stream with no frame on the wire, and the client request never completed. A value of the wrong type threw after the header block was in the HPACK encoder, and the next response on the session failed to decode. respond() now copies its options and reads endStream, waitForTrailers and sendDate from the copy by truthiness, as node does. It gives the native writer a new object that holds endStream, waitForTrailers and paddingStrategy. Options that are not an object throw ERR_INVALID_ARG_TYPE, as in node. respondWithFile() and respondWithFD() go through respond(). They now check the options the same way and do not read endStream, as in node. Before, endStream: true sent the HEADERS frame with END_STREAM and no file.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for one more day.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 13 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for a maintainer. The diff has no CI failure of its own. How I reproduced it: a
Test: CI, build 118474 at 25f7f43, finished: 180 of 181 jobs passed.
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/http2.ts— A statCheck callback that setsoptions.endStream = trueon the options it receives makes respondWithFile/respondWithFD send END_STREAM plus a content-length and no DATA, a length mismatch strict clients treat as a protocol error; node ignores endStream on the file responders entirely. The diff forcesendStream: falseat src/js/node/http2.ts:3338 and :3410, but the copy is the same object handed to statCheck at :3022 and then to respond() at :3043, which re-reads endStream by truthiness at :3522. Fix: the file responders must not let endStream reach respond() at all (strip it after statCheck, or pass respond() an object built from waitForTrailers/sendDate only) so the PR's stated invariant that the file is the payload holds for every input.Extended reasoning...
The PR says respondWithFile/respondWithFD never read endStream and pins that with a test for the caller's own option. The forced endStream: false lives on the copied options object at src/js/node/http2.ts:3338 (respondWithFile) and :3410 (respondWithFD). doSendFileFD calls options.statCheck.$call(this, stat, headers, options) at :3022 with that same object; node passes a fresh {offset, length} statOptions object, so user code that mutates options there runs on both runtimes. If statCheck assigns options.endStream = true, this.respond(headers, options) at :3043 copies it, endStream at :3522 becomes true, the condition at :3613 forces END_STREAM on the HEADERS frame and this.end() at :3655 runs; the content-length header set at :3037 is still present, so the frame declares a body it never sends. The file pipe then writes into a half-closed stream. On node the same statCheck has no effect because respond() there receives streamOptions computed before statCheck and never reads endStream. The dismissing finder noted the base also honored it, but the diff's own claim and test assert the opposite…
Verification: pre-existing (nit): triggers only when a user statCheck assigns
endStream = trueto its third argument. Mechanism verified: respondWithFile/respondWithFD build the copy at src/js/node/http2.ts:3338 / :3410 (options = { ...options, endStream: false }) and hand that same object to doSendFileFD; at :3022 it is passed to the user callback (`options.statCheck.$call(this, stat, headers,…
respondWithFile() and respondWithFD() gave respond() the same options object that they give to statCheck as its third argument. A statCheck that set endStream on it made respond() put END_STREAM on the HEADERS frame, next to a content-length, and no file was sent. doSendFileFD() now reads waitForTrailers, sendDate and paddingStrategy before statCheck runs and gives respond() an object with only those. node does the same: it computes the stream options before statCheck and never reads endStream for a file response. A statCheck can no longer turn waitForTrailers on or sendDate off, as in node. This replaces the endStream: false that the two methods put on their options copy. That key also showed up in the statCheck argument.
Each comment keeps the fact that the code cannot show: the options copy is deliberate, paddingStrategy is a bun extension, the native writer is shared with request(), and statCheck can write to the options it gets. The paragraph from #29075 keeps its text. Only the sentence that named a guard this branch removed is new.
|
Updated 2:38 PM PT - Sep 19th, 2026
❌ @robobun, your commit 25f7f43 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 43478That installs a local version of the PR into your bun-43478 --bun |
|
On the I reproduced it on the first push (6e0431d): a Fixed in db3a379. After the fix these rows answer The test |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/http2.ts— pre-existing: a server using respondWithFile()/respondWithFD() with a statCheck that adjusts the range through its third argument serves the whole file instead of the range it set, unlike node. Node hands statCheck itsstatOptions({ offset, length }, defaulted to 0/-1) and uses the values statCheck leaves there; http2.ts:3028 hands the copied useroptionsafterstatOptionswas already built at http2.ts:3008, so writes to offset/length are dropped and unset keys read as undefined. Fix: passstatOptionsas the third argument and derive the transfer range from it after statCheck returns, keeping the newrespondOptionssnapshot for the response flags; the comment at http2.ts:2960 should then no longer describeoptionsas what statCheck receives.Extended reasoning...
Node's doSendFileFD (lib/internal/http2/core.js) builds statOptions = { offset: options.offset ?? 0, length: options.length ?? -1 }, calls ReflectApply(options.statCheck, this, [stat, headers, statOptions]), then computes statOptions.length and passes statOptions.offset/length to respondFD. The in-tree port of node's test (test/js/node/test/parallel/test-http2-respond-file-fd-range.js:40-44) reads options.length and options.offset from that third argument, showing the contract. In bun, respondWithFD() copies the user options at http2.ts:3414 and binds that copy into doSendFileFD. http2.ts:3008-3011 builds statOptions from the copy first. http2.ts:3028 then calls options.statCheck.$call(this, stat, headers, options) with the copy, not statOptions. Any
options.length = noroptions.offset = nthe callback performs lands on the copy that is never read again; http2.ts:3036-3039 and the createReadStream at http2.ts:3067-3073 use the stale statOptions. When the caller passed no offset/length, the callback also sees offset/length as undefined where node gives 0 and -1, so arithmetic like…Verification: pre-existing — the base has the same statCheck call shape; this PR touches the surrounding lines (adds the
respondOptionssnapshot at /home/claude/bun/src/js/node/http2.ts:2960-2965 with a comment "statCheck is handedoptionsand may write to it") but leaves the mechanism in place. Trigger: a server calls respondWithFile()/respondWithFD() with astatCheckthat sets… | pre-existing — the… -
🟣
src/js/node/http2.ts— pre-existing:pushStream()still accepts a non-objectoptionsand honors an inheritedendStream, while the three sibling methods this PR fixes now follow node.pushStream(headers, "x", cb)succeeds here but throws ERR_INVALID_ARG_TYPE on node, andpushStream(headers, Object.create({ endStream: true }), cb)ends the pushed writable on bun where node ignores the inherited key. Fix: apply the sameassertIsObject(options, "options")plus own-enumerable copy and!!options.endStreamread that respond() now uses (http2.ts:3519-3524) to pushStream at http2.ts:3207-3210/3312, so all four options-reading response methods validate identically.Extended reasoning...
The PR description says pushStream was checked and left unchanged because it does not forward options to the native writer; that covers the priority-leak half of the fix but not the validation half added to respond/respondWithFile/respondWithFD in this diff. Node's pushStream (lib/internal/http2/core.js) runs assertIsObject(options, 'options'), copies options and reads endStream by truthiness from the copy. Bun's pushStream at http2.ts:3207-3210 only reroutes a function argument, then at 3312 reads
options?.endStreamdirectly off the caller's object. Consequences: (1)pushStream(h, null, cb),pushStream(h, 5, cb)orpushStream(h, [], cb)succeed on bun and throw on node, so code relying on the node error path behaves differently. (2) An object with a non-own or non-enumerableendStream: true(Object.create({endStream:true}), a class accessor) makes bun call pushedStream.end() at 3313 before respond(); node sends the body. That ended pushed stream then hits the already-posted respond()-forcing gap. The PR's own test row 'inherited endStream: true' pins this rule for respond()…Verification: pre-existing — the base branch has the same
pushStreambehavior by the same route; this PR leavespushStreamuntouched while adding the node-style validation to its three sibling ServerHttp2Stream methods. Trigger: a server callsstream.pushStream(headers, <non-object>, cb)or passes an options object whoseendStreamis inherited/non-own. Mechanism verified in… | pre-existing — the base…
pushStream() accepted options that are not an object, and it read endStream from the caller's object, so an inherited endStream ended the pushed writable. node throws ERR_INVALID_ARG_TYPE for the first and sends the push body for the second. pushStream() now calls assertIsObject() and reads endStream from a copy, as respond(), respondWithFile() and respondWithFD() do on this branch. The check sits after the push-allowed and nested-push checks, so those errors still win, as in node.
There was a problem hiding this comment.
I re-reviewed the latest push (the pushStream() check-and-copy commit plus the two new test rows) and did not find any bugs; since this rewires what every server response hands to the shared native HEADERS writer, a maintainer look before merge is still worthwhile.
What was reviewed:
- The always-passed
{ endStream, waitForTrailers, paddingStrategy }object againstH2FrameParser::requestinsrc/runtime/api/bun/h2_frame_parser.rs(6891-7046):paddingStrategy: undefinedandwaitForTrailers: falseare no-ops there, and the priority/silent/signal branches are no longer reachable fromrespond(). assertIsObjectplacement in all four methods against node'score.jsordering (after the stream-state throws, before the copy), and thatHttp2ServerResponse[kBeginSend](http2.ts:1166) still passes only keysrespond()reads.doSendFileFD'srespondOptionssnapshot is taken beforestatCheckruns and feeds all threerespond()calls; the callers' own{ ...options }copies keep the test fixtures' shared shapes unmutated across the file/FD runs.- I could not run the new tests here (no debug build in this checkout), so CI is the check on the wire-level assertions.
Extended reasoning...
Overview
The PR changes four ServerHttp2Stream methods in src/js/node/http2.ts: respond() now validates options with the module's assertIsObject, reads a spread copy, coerces endStream/waitForTrailers by truthiness, and passes only { endStream, waitForTrailers, paddingStrategy } to the native writer; pushStream(), respondWithFile() and respondWithFD() gain the same assertIsObject; doSendFileFD() snapshots waitForTrailers/sendDate/paddingStrategy before statCheck can mutate the options object. The test file adds a describe("http2 response options") block with six tests covering ignored request-only options, truthiness reads, inherited-key exclusion, file-response endStream handling, non-object rejection across all four methods, and a raw-socket check of HEADERS frame flags.
Security risks
None specific to this change. The options object is user-supplied server-side code, not peer input. The prior behavior did let weight/parent/silent/signal from respond() reach the native priority path, which could close a stream after the header block was already in the HPACK encoder and desync the connection; removing that is a robustness improvement. assertIsObject uses $isObject/$isArray intrinsics rather than user-overridable globals, consistent with the module's other call sites.
Level of scrutiny
Moderate. The diff is small and mirrors node's core.js, but it changes the argument every server response passes to a shared native writer, so I traced the native reads: paddingStrategy is only applied when a number, waitForTrailers only when boolean, endStream: false sets no flag, and none of the priority/silent/signal branches are reachable from the new fixed-shape object. The 204/205/304/HEAD forcing block preserves the earlier invariant (endStream true, waitForTrailers false) and the bunHTTP2WaitForTrailers assignment is equivalent to the old options?.waitForTrailers && !endStream guard because waitForTrailers is already cleared whenever endStream is forced. The compat-layer Http2ServerResponse[kBeginSend] only passes keys respond() still reads. I did not have a debug build available in this checkout, so I did not execute the new tests; that, plus the breadth of node:http2 server users affected, is why I am deferring rather than approving.
Other factors
The bug hunt ran dry with no findings on this version. Both of my earlier inline notes were pre-existing issues the author replied to and pointed at other open PRs, and they remain outside this change's scope. Test quality looks reasonable: failures on either side resolve the race promise with a descriptive string rather than hanging, the non-object rows throw before any file descriptor or path is used, and the frame-flag test buffers to HTTP/2 framing before asserting.
|
On the two additional findings:
While I checked the trackers I also found that the |
Problem
respond(headers, options)on anode:http2server breaks on options that node ignores.{ weight: 0 }or a non-objectoptionsclose the stream with no frame sent, so the client hangs.{ silent: 1 }or{ endStream: 0 }throwERR_INVALID_ARG_TYPE, and the session's next response fails withERR_HTTP2_ERROR: Protocol error.respond()(src/js/node/http2.ts) passes the caller's object to the native HEADERS writer (H2FrameParser::request,src/runtime/api/bun/h2_frame_parser.rs:6891).ClientHttp2Session.request()shares that writer, so it readsparent,weight,exclusive,silentandsignal. It rejects a bad value after the header block is in the HPACK encoder.respondWithFile()andrespondWithFD()go throughrespond(), andendStream: truemakes them send no file.pushStream()skips node's options check.Fix
respond()copies its options and readsendStream,waitForTrailersandsendDateby truthiness. The native writer gets onlyendStream,waitForTrailersandpaddingStrategy.ERR_INVALID_ARG_TYPEfor non-objectoptions, with node's message, and read a copy. The file methods read theirs beforestatCheckruns and never passendStream.lib/internal/http2/core.js). Every repro row matches node v26.3.0.test/js/node/http2/node-http2.test.js(6 new tests, all fail on bun 1.4.3). Also node's 256test-http2-*.jsfiles (one was rerun alone, see Notes).Background
parent,weight,exclusiveandsilentare priority options ofrequest().signalaborts a request. A response has none.paddingStrategyper response is a bun extension, kept here.Notes
Repro (
bun file.mjs, compare withnode file.mjs). Each row sends two requests on one session.node v26.3.0 and this branch: every row prints
first: response 200 | second: response 200, with no error on either side.bun 1.4.3-canary.1+367d939d9:
Why the copy matters. Node reads a spread copy of the options, so only own enumerable keys count.
respond(h, Object.create({ endStream: true })), a non-enumerableendStream, and a class accessor all send the body on node. An own getter on an ignored key still runs, and a throw from it propagates. A wider probe of these four shapes found that a first version of this fix, which read the keys straight from the caller's object, ended the stream in the first three. The copy fixes that, and one test row pins it.File methods. Node's
respondWithFD()andrespondWithFile()readwaitForTrailers,offset,length,statCheck(andonError). They never readendStream. Before this PR,respondWithFile(path, h, { endStream: true })sentcontent-length: 9with END_STREAM on the HEADERS frame and no DATA. A strict client treats that length mismatch as a protocol error.statCheck. Bun gives
statCheckthe options object as a third argument. Node gives it{ offset, length }(respondWithFD) or nothing (respondWithFile), and it computes its stream options beforestatCheckruns. The first push of this PR putendStream: falseon the options copy, but gave that same object tostatCheckand then torespond(). AstatCheckthat setendStream = truestill sent no file. The review on this PR found that.doSendFileFD()now builds the object forrespond()beforestatCheckruns, so all of these rows answer200with the file, as on node:statChecksetsendStream = true,waitForTrailers = true,sendDate = false, orweight = 0. One behavior change follows: astatCheckcan no longer turnwaitForTrailerson orsendDateoff through its third argument. Node never allowed it. WhatstatCheckreceives is unchanged by this PR.Checked and left unchanged.
pushStream()does not pass its options to the native writer, so the hang and the desync never applied to it. It did skip node'sassertIsObjectand options copy. The review on this PR pointed that out, and the last commit adds both.pushStream(h, 5, cb)now throws as on node, andpushStream(h, Object.create({ endStream: true }), cb)sends the push body as on node. Before, it ended the pushed writable.assertIsObjecthelper accepts a function, where node's rejects it.respond(h, () => {})therefore answers 200 here and throws on node.connect()andcreateServer()use the same helper, so this PR does not change it.stream.end(chunk)after the stream has ended emitsERR_STREAM_WRITE_AFTER_ENDon node and nothing on bun. That is inHttp2Stream.end(). Issue node:http2: stream.end(chunk) after end() drops the chunk silently, node emits ERR_STREAM_WRITE_AFTER_END #43546 tracks it.Related open PRs. None of them changes what
respond()passes to the native writer.maxSendHeaderBlockLength,maxSessionMemory).signalhandling forrequest()out of the native writer.respond()calls from the two error branches ofdoSendFileFD. This PR changes the argument of those same two calls, so the PR that lands second needs a small rebase.respond()put END_STREAM on the HEADERS frame of a push that was ended beforerespond(). It adds a condition to the sameifthat this PR edits. The two conditions are independent.Found by the review, not fixed here. All three exist on
mainwithout this PR. The first two have an open PR, the third has an issue.pushStream(h, { endStream: true })thenrespond()with noendStream: the pushed stream never ends. Same result on bun 1.4.3 and on this branch. Node ends it. Owner: node:http2: end a pushed response on its HEADERS frame when the stream was ended before respond() #38104.respondWithFD(directoryFd)with noonError, then the stream is destroyed beforefstatcalls back:respond()throwsERR_HTTP2_INVALID_STREAMinside thefscallback, an uncaught exception. Same result on bun 1.4.3 and on this branch. Node throws nothing. Owner: node:http2: do not call respond() when a file response fails before the headers are sent #43463.statCheckofrespondWithFD()that setsoffsetandlengthon its third argument: bun sends the whole file, node sends the range. Bun givesstatCheckthe options copy, node gives it{ offset, length }(and nothing at all forrespondWithFile()). That changes what a user callback receives, so it is not part of this PR. Issue node:http2: respondWithFD() ignores the offset and length that statCheck sets on its third argument #43547 tracks it.Test runs. All on a debug build with ASAN, on the final code of this branch unless a line says otherwise.
node-http2.test.js: the 6 new tests pass. On bun 1.4.3 all 6 fail, and each failure names its rows (server stream error ERR_HTTP2_STREAM_ERROR,threw ERR_INVALID_ARG_TYPE,did not throw,END_HEADERS | PRIORITY,the pushed writable was already ended).statCheckrow was also run against the first push of this branch. It fails there with["200", "200"].test-http2-*.jsfiles: 255 pass in my parallel runner.test-http2-forget-closed-streams.js(10,000 requests) ran into the runner's 180 s limit and passes when run alone. In earlier runs the one file that needed a run alone wastest-http2-pipe.js, because a sibling test deleted its shared scratch directory. The host load average was between 90 and 600 during these runs.test-server,test-server-errors,test-server-deadlines,test-server-interceptors,test-retryandtest-deadline: 105 pass, 0 fail.node-http2.test.jsand the other files intest/js/node/http2/. The full file had 6 failures, all 5 s timeouts inshould receive goawayandECONNREFUSED. Those 13 tests pass when run alone. The other files passed.Cost of the options object.
respond()now always gives the native writer a small object, also forrespond(headers)with no options, wheremainpasses nothing. I measured the loop of themaxSessionMemorystress test (sequential POSTs, 3000 per run, debug build with ASAN, 4 interleaved runs per variant, host load average 100 to 180). Per request,respond(h, { endStream: true }): this branch 3145 to 3315 us,main3058 to 3353 us.respond(h): this branch 3053 to 3187 us,main2938 to 3351 us. The means differ by 2 to 3 percent, which is inside the spread of either variant. I kept the single call shape. If the no-options call should stay free, the old four-argument call can come back for the case where all three values are unset. The native side treats the two the same.CI, build 118474. 180 of 181 jobs passed. The red test is
test/js/bun/spawn/spawn.test.tsondebian 13 x64-asan, which CI marks as also failing onmain. Four files passed on a retry or alone. One of them isnode-http2.test.jsondarwin aarch64: themaxSessionMemorystress test (10,000 requests, from before this PR) reached its 15 s limit once. None of the 6 new tests failed on any lane. I did not find that stress test in the annotations of the last 25mainbuilds, so I cannot show that it fails the same way onmain. The measurement above is my evidence that this PR does not slow it. Two other files needed one retry on the same macOS lane in this build (node-http-uaf.test.tswithEADDRINUSEfromlisten(0), andmodule-graph-gc.test.ts).Self-review. The automated self-review of this diff did not complete. The environment stopped it three times before it produced a result, so this PR carries no self-review verdict. Three defects in earlier pushes of this branch were found another way. My own comparison against node found the options copy. The review on this PR found the
statCheckpath and the missingpushStream()check.