Conversation
doSendFileFD is shared by respondWithFile() and respondWithFD(). It called options.onError for both. Node reads onError only in respondWithFile() (afterOpen and doSendFileFD). Its doSendFD and processRespondWithFD never read it. respondWithFD() now clears onError on its own copy of the options. respondWithFD() without statCheck ends the writable side when it is called. A response that the onError handler sent after that never ended, so the client request did not close. A header error from respond() now destroys the stream for respondWithFile() too. Node's processRespondWithFD does the same for both entry points.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughThe HTTP/2 implementation adds guarded response teardown and clears ChangesHTTP/2 response error handling
Suggested reviewers: Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:28 AM PT - Sep 20th, 2026
❌ @robobun, your commit f36c3e4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 43587That installs a local version of the PR into your bun-43587 --bun |
|
Status: ready for a maintainer. The diff is green. CI is red only for tests that this diff does not touch. CI:
Before you merge: read the merge order in the PR notes (#43564 and #43526). How I reproduced it: the script in the Notes block of the PR body, run with Node v26.3.0 and with Bun 1.4.3-canary.1+367d939d9 on Linux x64. Node never calls PR: #43587 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether options.onError = undefined in respondWithFD() leaks to the caller's object — it does not, respondWithFD() spreads options into a fresh copy at src/js/node/http2.ts:3405 before the assignment. The onError local in doSendFileFD is still read on the fstat-error and non-regular-file arms, so respondWithFile() keeps calling the handler there as before.
Extended reasoning...
Inline findings were posted, so this note only records what else was examined. The per-call options copy at http2.ts:3405 precedes the new options.onError = undefined at 3422, so a caller-shared options object (e.g. reused across respondWithFD and a later respondWithFile) is unaffected. The header-rejection catch now destroys unconditionally for both entry points, which matches the cited Node lines; the earlier onError arms at 2966 and 2985 remain reachable only for respondWithFile, as intended. The unguarded this.respond() calls on those arms for respondWithFD callers and the fd-ownership mismatch are covered by the inline comments and not restated here.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
src/js/node/http2.ts— Servers that pass onError to respondWithFD() can now crash with an uncaught exception where the base called their handler. With onError cleared at http2.ts:3422, an fstat error or a directory fd takes the arm at http2.ts:2968 / 2987, which calls this.respond() unguarded inside the fs callback. respond() throws ERR_HTTP2_INVALID_STREAM if the client reset the stream before fstat returned, or ERR_HTTP2_INVALID_PSEUDOHEADER for rejected headers; nothing catches it. Fix: guard both respond() calls (try/catch then this.destroy(err), or skip respond when this.destroyed), so every no-onError arm settles the stream instead of throwing from the callback. The PR notes this as #43448, but neither fix is in this tree.Extended reasoning...
On the base branch respondWithFD(fd, headers, { onError }) reads onError at http2.ts:2959 and calls it at 2966 or 2985, so the throwing respond() call is never reached for those callers. After this change http2.ts:3422 sets options.onError = undefined for every respondWithFD call, so the same callers now take the else arm. That arm calls this.respond(headers, options) at http2.ts:2968 (fstat error) or 2987 (directory or non-regular fd). respond() at http2.ts:3505-3507 throws $ERR_HTTP2_INVALID_STREAM when this.destroyed is true. A client that sends RST_STREAM while fstat is pending destroys the stream before the callback runs, so a closed or bad descriptor plus a client reset throws from the fs.fstat callback. With headers containing a request pseudo-header respond() throws ERR_HTTP2_INVALID_PSEUDOHEADER on the same lines. A throw from an fs callback is an uncaughtException and terminates the process unless the user installed a handler. The header-error catch at http2.ts:3043-3053 only covers the success path respond() at 3044, not 2968 or 2987. The PR description acknowledges this as…
Verification: normal, acknowledged in diff: the PR description's Notes say "respondWithFD() callers that pass it reach the respond() calls in the arms without onError. Those calls are not guarded... throws from the fs callback. That is #43448 ... #43463 and #43564 both fix it. Prefer to merge one of them first." The bound is accurate for onError-less callers (base already fails that way) but the claim that…
doSendFileFD calls respond() in the two arms that run without onError. respond() throws when the stream is already destroyed and when it rejects the headers. The throw left the fstat callback as an uncaught exception. respondWithFD() callers that pass onError now reach these arms, so the error destroys the stream there, like the arm that sends the file. The test covers a bad descriptor together with rejected headers. Its onError handler for the cases that must not call it destroys the stream, so an unwanted call fails an assertion and does not wait for the timeout.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the updated push and found no bugs; the second commit addresses the hang-instead-of-fail nit from my earlier pass. Because this changes user-visible node:http2 semantics (when options.onError fires) and overlaps with the other open doSendFileFD PRs named in the description, a human look at the intended ordering is still worthwhile.
What was reviewed:
options.onError = undefinedinrespondWithFD()writes to the per-call copy made atsrc/js/node/http2.ts:3412, not the caller's object.respondOrDestroycallingdestroy(err)whenrespond()throws on an already-destroyed stream is a no-op (Duplex destroy is idempotent), so no double-error emit.- The
respondWithFilecontrol cases still work because onlyrespondWithFDwithoutstatCheckcloses the writable side beforefstatreturns. - Could not run the test here (no debug build in this checkout), so the pass/fail claims in the description are unverified by me.
Extended reasoning...
Overview
The PR touches two files: src/js/node/http2.ts (the shared doSendFileFD helper plus ServerHttp2Stream#respondWithFD) and test/js/node/http2/node-http2.test.js. The runtime change is three-fold: respondWithFD() clears onError on its options copy so the shared helper never calls it for that entry point; a new respondOrDestroy helper wraps the two bare respond() calls in the fstat-error and non-regular-file arms so a throw there destroys the stream rather than escaping the fs.fstat callback as an uncaught exception; and the catch around the main respond() now always destroys the stream instead of routing header errors to onError. The second commit (6b68372) is what added respondOrDestroy and changed the test's "must not run" handler to destroy the stream, which is the fix for the hang concern I raised in the earlier run.
Security risks
None identified. The change only affects which callback runs on a server-side error path and does not touch header parsing, TLS, auth, or any input that crosses a trust boundary. The mutation of options.onError is confined to the object created by { ...options } at line 3412, so no caller-visible mutation or prototype interaction is introduced.
Level of scrutiny
Moderate. The diff is small and each branch maps to a cited Node v26.3.0 line, but it is a deliberate behavioral change to a Node-compat API: code that passed onError to respondWithFD() and relied on it will now see the stream destroyed instead. That is the correct Node behavior per the cited source, but it is the kind of compat decision a maintainer should consciously accept, especially since the description names two other open PRs (#43463, #43564) rewriting the same function with stated merge-order preferences. Those ordering trade-offs are a human call, not something the code alone settles.
Other factors
The test covers the variant matrix reasonably: nine failing scenarios each run with and without onError, plus two positive controls for respondWithFile. Every awaited condition is an event (req 'close' and server stream 'close'), the unwanted-call handler now destroys the stream so a regression fails an assertion rather than waiting for the file timeout, and cleanup is in finally. The one caveat is runtime: it is a single it issuing 20 sequential requests on one connection, which is fine on release but will be slow under debug+ASAN; there is no per-test timeout set, so it stays within the file default. I was not able to execute the test in this environment (no debug build present and test execution was not permitted), so the description's pass/fail claims remain unverified by me. No CODEOWNERS entry covers the changed paths.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
The new test case starts respondWithFile() on a stream while the fstat of respondWithFD() is pending. A check of the per-stream kOwnsFd flag in doSendFileFD would call the onError of respondWithFD() there. The clear on the options copy of the call does not.
|
Follow-up to the review:
The guard conflicts with #43463 and #43478 in those two arms. The PR notes say how to resolve each one. |
There was a problem hiding this comment.
I reviewed the two newer pushes and found no bugs; the earlier hang concern is addressed (the "must not run" handler now destroys the stream, so an unexpected onError call fails the toEqual instead of stalling the file). Because this changes node:http2 response semantics inside a function that #43463 and #43564 also rewrite, a maintainer should still decide merge order.
What was reviewed:
options.onError = undefinedinrespondWithFDlands on the{ ...options }copy made at http2.ts:3410, so the caller's object is untouched andafterOpen(respondWithFile) still reads its own copy'sonError.respondOrDestroynow guards both no-onErrorfallback arms; arespond()throw destroys with the header error and thefalsereturn skips the seconddestroy(), so the stream is not destroyed twice with different errors.- The
bothInFlightcase:respondWithFileflippingkOwnsFdbefore the pendingfstat(badFd)returns cannot resurrectonError, since the flag is no longer what selects it; the EBADF arm still skipstryClose.
Extended reasoning...
Overview
The PR touches src/js/node/http2.ts (respondWithFD, doSendFileFD, and a new respondOrDestroy helper) and adds one test to test/js/node/http2/node-http2.test.js. Runtime changes are about 30 lines: respondWithFD clears onError on its per-call options copy, the two fallback arms in doSendFileFD that previously called this.respond() unguarded now go through respondOrDestroy, and the catch around the main respond() always destroys instead of routing to onError. Since my last review, commit 6b68372 added respondOrDestroy and the loud-failure onError handler in the test, and fa960e6 added the bothInFlight case and trimmed comments.
Security risks
None identified. The change only affects which error path a server-side stream takes when a file descriptor or header set is rejected; no user-controlled data reaches a new parser, path, or allocation, and the $call intrinsic is used for the helper as src/js/ requires.
Level of scrutiny
Moderate. The diff is small and each branch maps to a cited Node v26.3.0 line, and I traced the three paths in the summary (options aliasing, double-destroy, kOwnsFd race). However this is a behavioral change in a Node-compat module, and the PR description itself says two other open PRs (#43463, #43564) rewrite the same function, with a stated conflict against #43564. That sequencing decision, plus the still-open pre-existing fd-ownership thread on kOwnsFd, is why a human look is worthwhile rather than a full approve.
Other factors
The multi-agent hunt exited on dry_streak with no findings. I could not run the new test locally (no debug build present in this checkout), so the "stock bun fails it" claim is taken from reading the test: without the fix, respondWithFD(badFd, ok, { onError }) would push EBADF into events.onError and the toEqual({ ...withoutOnError, onError: [] }) assertion would fail, so the test is discriminating. The test uses port: 0, awaits real close events on both sides, and closes the fds and server in finally, matching the harness conventions.
respond() still rejects header values that node accepts, for example undefined. A respondWithFile() caller with an onError handler gets that error today and can answer with a 500. Without the handler call the stream is destroyed, and a stream with no 'error' listener ends the process. This arm can follow node after respond() accepts what node accepts. respondWithFD() still never calls onError.
robobun
changed the title
There was a problem hiding this comment.
I reviewed the latest push (through f36c3e4) and found no bugs, but a human look is still worthwhile: the last commit deliberately keeps onError for a respond() header error in respondWithFile() (src/js/node/http2.ts:3045), so the PR description's "A header error from respond() now destroys the stream for respondWithFile() too" no longer matches the code, and a maintainer should decide whether that retained Node deviation is acceptable and how this sequences with #43463/#43564.
What was reviewed:
options.onError = undefinedinrespondWithFD()mutates the copy made at http2.ts:3413, not the caller's object;doSendFileFDreads it once and bothif (onError)/typeof onError === "function"checks handleundefined.respondOrDestroyon the two no-onErrorarms: a throw fromrespond()on an already-destroyed stream maps to a no-opdestroy(), so no double-destroy or uncaught exception from the fstat callback.- Test matrix pairs each failing
respondWithFDcall with and withoutonError, wires the must-not-run handler tostream.destroy()so a regression fails instead of hanging, and cleanup runs infinally.
Extended reasoning...
Overview
The change is confined to src/js/node/http2.ts (ServerHttp2Stream.respondWithFD() and the shared doSendFileFD helper plus a new respondOrDestroy helper) and one new test in test/js/node/http2/node-http2.test.js. respondWithFD() now clears onError on its private copy of options so the shared fstat callback never invokes it for that entry point, matching Node's doSendFD/processRespondWithFD. The two error arms that previously called this.respond() unguarded from inside the fs callback now go through respondOrDestroy, which converts a synchronous throw from respond() into this.destroy(err) and skips the follow-up destroy so the stream is not destroyed twice with different errors.
Security risks
None identified. The code path handles server-side file responses; the change does not alter fd ownership (the kOwnsFd logic is untouched), does not add any path handling, and the only new side effect is on an already-copied options object. The test opens a bad fd, a directory fd and the test file itself, all local, and closes them in finally.
Level of scrutiny
Moderate. This is Node compat code in a built-in module with a history of several overlapping in-flight PRs on the same function (the author names #43463, #43478 and #43564). The logic itself is small and I traced each arm: EBADF on a bad fd, directory fd, and rejected headers, each with and without statCheck, and the destroyed-stream case where respond() throws ERR_HTTP2_INVALID_STREAM and destroy() becomes a no-op. What warrants a human is not a bug but a judgment call: the final commit (f36c3e4) reversed the earlier version's Node-matching behavior for respondWithFile() header errors, keeping the onError call at http2.ts:3045 with a stated reason (Bun's respond() rejects header values Node accepts, and destroying a stream with no 'error' listener would crash the process). That is a reasonable interim choice, but it is a documented deviation from Node that the PR description now contradicts, and it removed the corresponding test row, so a maintainer should confirm they agree with it.
Other factors
The new test is well-formed: it awaits both the client 'close' and the server stream 'close', uses port: 0, routes the must-not-run handler to stream.destroy() so a regression fails an assertion rather than hanging, includes the bothInFlight ordering case, and keeps two positive controls for respondWithFile() (ENOENT, ERR_HTTP2_SEND_FILE) so the assertion set cannot pass vacuously. Prior reviews on this PR (an onError-hang nit and a pre-existing fd-leak note) were resolved by the author; the hang nit is addressed in the current test, and the fd leak is explicitly deferred to #43564, which is a scoping decision rather than a defect in this diff. The changed files are not covered by CODEOWNERS.
|
The PR description was updated after f36c3e4, so that sentence is gone. The Fix section now says that |
This branch has not been deployed
1 participant
Problem
ServerHttp2Stream#respondWithFD()callsoptions.onErrorwhen thefstatfails, when the descriptor is a directory, and whenrespond()rejects the headers. Node v26.3.0 never readsonErrorthere and destroys the stream.statCheck, a response that theonErrorhandler sends never ends, so the client request hangs.doSendFileFD(src/js/node/http2.ts:2957).respondWithFile()andrespondWithFD()share it, and it readsoptions.onErrorfor both.Fix
respondWithFD()clearsonErroron its copy of the options, sodoSendFileFDtakes the arms withoutonErrorand destroys the stream.fstatcallback whenrespond()fails (respondOrDestroy). The error destroys the stream.doSendFDandprocessRespondWithFD(lib/internal/http2/core.js, v26.3.0) never readonError.respondWithFile()does not change.test/js/node/http2/node-http2.test.js(new test, stock bun fails it), and 23 vendoredtest-http2-*files that use these methods. Self-reviewed: 13 concerns raised, 11 addressed. Notes name the other 2 and the merge order.Background
respondWithFile(path)opens the file itself.respondWithFD(fd)sends a descriptor that the caller owns. Each call copiesoptions.onErroris an option ofrespondWithFile()only. Node calls it before any headers, for a failedopenorfstat, or for a file that is not regular.statCheck,respondWithFD()closes the writable side before it returns, like Node. Bun stats the descriptor after that, so the handler'sstream.end()does nothing.Notes
Repro. Run with
nodeand withbun:Node v26.3.0 and this branch:
Bun 1.4.3-canary.1+367d939d9 prints this and never closes the request:
Merge order. Read this before you merge.
doSendFDandprocessRespondWithFD, and it fixes the cause of this class of hang:respondWithFD()no longer goes throughfstatwithout astatCheck. If it lands first, most source lines of this PR go away with the code they patch. One thing stays open there: itsprocessRespondWithFDstill callsonErrorfor a header error, forrespondWithFD()too. The test of this PR fails on its current head at the rows with rejected headers. I will then cut this PR down to that one condition plus the test.http2 file responses reject a :status that statCheck set to a value that coerces to 0expects threeonErrorcalls fromrespondWithFD(). Node makes none of them, and this PR removes them. Whichever of the two lands second must change that test, ornode-http2.test.jsgoes red on main.onError, and node:http2: ignore request() options in respond(), like node #43478 changes the options that they pass torespond(). Both conflict withrespondOrDestroyin those arms. For node:http2: do not call respond() when a file response fails before the headers are sent #43463, take its arms and droprespondOrDestroy, because its helper has the same guard. I checked that combination: both tests pass.What this PR leaves alone: a header error in
respondWithFile(). Node destroys the stream there too and never callsonError(core.js lines 2714 to 2719). An earlier version of this PR did the same. I took it out, becauserespond()still rejects header values that Node accepts. Example: the pattern from the Node docs,respondWithFile(file, { "content-type": mime[ext] }, { onError }), with an unknown extension. Node skips theundefinedvalue and serves the file. Bun throwsERR_HTTP2_INVALID_HEADER_VALUE, and today theonErrorhandler answers with a 500. Without the handler call the stream is destroyed, and a stream with no'error'listener ends the process. That arm can follow Node after #41614, which makesrespond()accept what Node accepts.Why the options copy and not the
kOwnsFdflag.kOwnsFdis a field of the stream, and a laterrespondWithFile()orrespondWithFD()on the same stream overwrites it before the firstfstatreturns. Node decides per call, becausedoSendFDanddoSendFileFDare different functions. A first version of this change readkOwnsFdindoSendFileFD. WithrespondWithFile(dir, h, { onError })followed byrespondWithFD(fd)in the same tick it dropped theonErrorthat Node and stock Bun call. The options copy is per call, so both orders now match Node. ThebothInFlightcase of the test pins this: it fails with akOwnsFdcheck.respondOrDestroy. The two arms withoutonErrorcalledrespond()with no guard.respond()throws when the stream is already destroyed and when it rejects the headers, and the throw left thefstatcallback as an uncaught exception (the second point of #43448).respondWithFD()callers that passonErrornow reach these arms, so this PR guards them. With the guard, a probe of 19 calls (bad descriptor, directory descriptor, rejected headers,destroy()right after the call, both methods on one stream) has no uncaught exception and no request that stays open.onErrormatches Node in everyrespondWithFD()call of the probe.The test. It runs each failing
respondWithFD()call two times, withoutonErrorand with it. It expects the same events both times, noonErrorcall, andNGHTTP2_INTERNAL_ERRORon the client. The handler for these cases destroys the stream, so a call that must not happen fails an assertion and does not wait for the timeout. The test pins the server error code only for the header errors. #43463 and #43564 change the server error code and the200of the other cases, and the test passes before and after #43463. Two control cases check thatrespondWithFile()still callsonErrorfor a missing file and for a directory, and that the handler's 404 arrives.The 2 concerns I did not address here.
respondWithFD()never callsstatCheck, so there is no hook left to send another response. Node callsstatCheckfor every descriptor type. node:http2: make respondWithFD() follow node with and without a statCheck #43564 fixes that. The same PR owns the descriptor leak when both methods run on one stream, and the requests that stay open whenfs.fstatthrows at once (descriptor-1,1.5,2 ** 31).Suites run with the debug build. The new test (runs in a row, runs under load, runs with
BUN_JSC_validateExceptionChecks=1, and a run on Windows x64 for the earlier head). The fullnode-http2.test.js(388 pass, 6 skip, 0 fail, on the earlier head).test-http2-respond-file*.js(17 files),test-http2-respond-with-file-connection-abort.js,test-http2-respond-no-data.js,test-worker-terminate-http2-respond-with-file.js,test-http2-error-order.js,test-http2-generic-streams-sendfile.js, andsequential/test-http2-timeout-large-write-file.js.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file