node:http2: check fd in respondWithFD() like node, after the stream state and the options

[robobun]

Problem

• ServerHttp2Stream#respondWithFD(fd, headers, options) checks fd first (src/js/node/http2.ts, top of respondWithFD). A bad fd then hides every other error. After stream.respond(), stream.respondWithFD("x") throws ERR_INVALID_ARG_TYPE ("fd"). Node v26.3.0 throws ERR_HTTP2_HEADERS_SENT.
• Node checks the stream state, then options, then fd, then the headers (lib/internal/http2/core.js, respondWithFD).
• respondWithFD() and respondWithFile() test only this.destroyed. Node also tests this.closed. After stream.close(), node throws ERR_HTTP2_INVALID_STREAM and bun does not throw.

Fix

• The fd check moves below the options checks and above the headers checks.
• The fd check is now node's: a number, or a FileHandle instance. Before, any object with a numeric fd passed, and the read of it could run a user getter.
• Both methods throw ERR_HTTP2_INVALID_STREAM on a closed stream.
• Verified: new test in test/js/node/http2/node-http2.test.js, with node v26.3.0's output as the expected strings. It fails without the src/ change. All 22 pairs of failing checks I compared now equal node. Also ran the 22 vendored node tests that call these methods.

Background

• This PR is stacked on node:http2: validate the headers and options arguments like node #43491, which moves the options checks above the headers checks. node:http2: prepare the final response headers like node #43526 (other base) also moves the fd check to this position, without the closed and FileHandle checks. See Notes.
• respondWithFD() sends a file from an open descriptor as the response body. respondWithFile() opens a path first. A FileHandle is what fs.promises.open() returns.
• A stream is closed after close(). It is destroyed later, when teardown completes. Node refuses a response in both states.

Notes

Review. Three review passes before the PR (correctness, test, shape), no blocker, two test findings applied. After the first push, the automatic review reported that the moved fd block let a user getter run after the state checks, and that the block built its error by hand. The second commit replaces the block with node's check. Measured after it: an object with an fd getter gets ERR_INVALID_ARG_TYPE as in node, and the getter does not run (test row fdGetterRan). String(err) is TypeError [ERR_INVALID_ARG_TYPE]: ... as in node (before: TypeError: ...). A real FileHandle still works (test-http2-respond-file-filehandle.js). The unreachable fd instanceof FileHandle branch at the end of the method is removed.

Behavior change to know about. respondWithFD({ fd: 3 }) with a plain object threw nothing before. It now throws ERR_INVALID_ARG_TYPE, as in node.

Overlap with open PRs on the same lines.

• node:http2: prepare the final response headers like node #43526 (stacked on node:http2: validate the server stream :status like node #43453, opened the same day) moves the fd block in respondWithFD() to the same position, as part of a change to how the response headers are prepared. Its test has two rows for that order. I found it only after I opened this PR. The two PRs change the same lines from different bases, so the second one to land needs a rebase there. Only this PR has: the closed check in both methods, the FileHandle check, the rows for a bad fd on a stream that already responded, is closed, or is destroyed, and the row for options that is not an object.
• node:http2: throw getUnpackedSettings/respondWithFD ERR_INVALID_ARG_TYPE through the shared error machinery #38644 (open since August, now in conflict with main) makes the same change to the content of the fd block, and does not move it. This PR now contains that half of it. Its other half (getUnpackedSettings()) is not touched here.
• node:http2: send RST_STREAM when a server stream is reset #33380 adds || this.closed || this.session === undefined to the same state checks, and to respond().
• node:http2: validate the server stream :status like node #43453 edits the :status lines below the headers check. No shared lines.
• node:http2: ignore request() options in respond(), like node #43478 adds assertIsObject(options, "options") at the place where node:http2: validate the headers and options arguments like node #43491 adds it.

Not fixed here: respond() on a closed stream. respond() still tests only destroyed. Measured on main: after stream.close(), stream.respond() does not throw (node: ERR_HTTP2_INVALID_STREAM), and a bun client then fails the whole session with ERR_HTTP2_ERROR: Stream was already closed or invalid, so later requests on it fail too. I did not add the check here because respond() is also called from the fs.fstat callback of the file methods, before that callback tests closed. A throw there is an uncaught exception. #43463 changes those call sites and #33380 adds the check.

Not fixed here: headersSent after respondWithFD(). No open PR covers it as far as I found. Node runs respondWithFD() synchronously when options.statCheck is not given, so stream.headersSent is true when the call returns and a later respond() throws ERR_HTTP2_HEADERS_SENT. Bun always waits for fs.fstat, so headersSent is false, respond() does not throw, and the file is never sent. In my run the client request then never emitted 'close'.

import http2 from "node:http2";
import fs from "node:fs";
import { fileURLToPath } from "node:url";
const fd = fs.openSync(fileURLToPath(import.meta.url), "r");
const srv = http2.createServer();
srv.on("stream", st => {
  st.respondWithFD(fd);
  console.log("headersSent:", st.headersSent); // node: true, bun: false
  try { st.respond(); console.log("no throw"); } catch (e) { console.log(e.code); } // node: ERR_HTTP2_HEADERS_SENT
});
srv.listen(0, "127.0.0.1", () => {
  const r = http2.connect("http://127.0.0.1:" + srv.address().port).request();
  r.resume();
  r.on("close", () => process.exit(0));
});

[robobun]

Status: ready for review. The fix and the test are pushed (3 commits). This PR is stacked on #43491 and needs it to merge first. All review threads are answered and resolved.

CI (build 118552, ec43141, finished): 180 of 181 jobs passed. The four macOS test jobs passed. One job failed: debian 13 x64-asan, in test/js/bun/spawn/spawn.test.ts ("stdout reader of an unref'd child and process lifetime"). The CI report marks it as pre-existing: the same failure is in the final builds of recently merged PRs. This PR changes only node:http2. I reported the test for triage. The failure report lists no test in test/js/node/http2/. test/js/bun/http/serve-http2-lifecycle.test.ts timed out once in a parallel batch on one lane and passed alone. It tests Bun.serve and does not call the two methods that this PR changes.

Reproduction (bun repro.mjs, compare with node repro.mjs, node v26.3.0):

import http2 from "node:http2";
const d = fn => { try { fn(); return "no throw"; } catch (e) { return `${e.name} ${e.code}`; } };
const srv = http2.createServer();
srv.on("stream", st => {
  st.on("error", () => {});
  console.log("respondWithFD('x', {}, {offset: 'x'})   ", d(() => st.respondWithFD("x", {}, { offset: "x" })));
  st.respond();
  console.log("after respond(): respondWithFD('x')     ", d(() => st.respondWithFD("x")));
  st.end();
});
srv.listen(0, "127.0.0.1", () => {
  const c = http2.connect("http://127.0.0.1:" + srv.address().port);
  c.on("error", () => {});
  const r = c.request();
  r.on("error", () => {});
  r.resume();
  r.on("close", () => { c.destroy(); srv.close(); });
});

node prints TypeError ERR_INVALID_ARG_VALUE, then Error ERR_HTTP2_HEADERS_SENT. bun 1.4.3 prints TypeError ERR_INVALID_ARG_TYPE (the "fd" error) for both lines. With this PR, bun prints the same as node.

Overlap: #43526 also moves the fd check in respondWithFD() to the same position, from another base. I found it after I opened this PR. The PR body (Notes) says what each PR has that the other does not.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked whether the new || this.closed guard could reject a live server stream: the Closed bit is only set by close(), the rst/error path in the stream-error handler, native stream state 7, and push streams after their writable side has already settled — none of these fires on a stream that has not yet responded, so a legitimate respondWithFD/respondWithFile call is not affected.

Extended reasoning...

Findings were reported inline, so this note only records what else was examined. I traced every markStreamClosed call site in /home/claude/bun/src/js/node/http2.ts (close() at ~2539, _destroy at ~2603, the stream error/rst path at ~3717, the native state-7 handler at ~4042/~5037, and the push-stream half-closed-local bookkeeping at ~2071/~2749). Each runs only after the stream has been explicitly closed, reset, or has already finished its response, so the tightened state check in respondWithFD/respondWithFile cannot reject a stream that node would accept. The inline findings (re-entrant fd getter ordering, respond() still lacking the same closed guard, asynchronous headersSent after respondWithFD, and the hand-rolled ERR_INVALID_ARG_TYPE) remain the reason a human should look.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🟣 src/js/node/http2.ts — Servers that call stream.respond() (or read stream.headersSent) right after stream.respondWithFD() send only headers and never the file; node throws ERR_HTTP2_HEADERS_SENT there. respondWithFD at src/js/node/http2.ts:3404-3414 ends the user writable synchronously via closeWritableForFileResponse but defers respond() to the fs.fstat callback, so headersSent stays false until fstat completes. A later respond() wins, and doSendFileFD then returns silently at :3022 because headersSent is true. Fix: when options.statCheck is undefined, mark the response as initiated synchronously (node runs processRespondWithFD synchronously) so headersSent is true when respondWithFD returns and a second respond() throws.

  Extended reasoning...

  Handler calls stream.respondWithFD(fd) with no statCheck.
  Line 3404-3408: closeWritableForFileResponse(this) runs stream.end() synchronously, but respond() is not called yet.
  Line 3413: fs.fstat is asynchronous; headersSent remains false when respondWithFD returns.
  Handler (or a generic error/timeout helper that checks if (!stream.headersSent)) calls stream.respond({':status': 500}) or stream.respond().
  respond() at :3487 sees headersSent false and submits those headers; no throw.
  fstat completes; doSendFileFD at :3020-3025 sees this.headersSent true and returns; the file is never sent.
  The client receives the second response's headers with END_STREAM from the ended writable, no body.
  Node's processRespondWithFD runs synchronously and sets headersSent, so the second respond() throws ERR_HTTP2_HEADERS_SENT and the file is sent.
  The PR description itself shows this repro and marks it found-and-not-fixed; the code at :3404-3414 is identical on the base commit, so it is pre-existing, but it is a real divergence with a concrete trigger.
  Remedy: set the headers-sent state…

  Verification: pre-existing (the base commit 4ed357f has byte-identical code for this tail of respondWithFD; the PR only moves the fd type check and adds || this.closed, and its own description lists this under "Found and not fixed here"). Trigger: a handler calls stream.respondWithFD(fd) without options.statCheck and then, synchronously or before fstat completes, calls stream.respond(...) (or any…

[robobun]

^{Updated 4:14 PM PT - Sep 19th, 2026}

❌ @robobun, your commit ec43141 has 1 failures in Build #118552 (All Failures):

• test/js/bun/spawn/spawn.test.ts - code 1: stdout reader of an unref'd child and process lifetime > an idle reader stopped at the highwater … on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 43550

That installs a local version of the PR into your bun-43550 executable, so you can run:

bun-43550 --bun

[claude]

LGTM — the second push (aa85f1a) addresses the two actionable points from my earlier pass: only a real FileHandle is unwrapped, so no user getter runs after the state checks, and the fd error now goes through $ERR_INVALID_ARG_TYPE.

What was reviewed:

• respondWithFD check order (state → options → fd → headers) and the instanceof FileHandle unwrap now match node's lib/internal/http2/core.js line for line; ERR_INVALID_ARG_TYPE("fd", ["number", "FileHandle"], ...) renders "of type number or an instance of FileHandle" in ErrorCode.cpp, matching the test's expected strings.
• doSendFileFD and fs.createReadStream({ fd }) only ever see the numeric fd now, so dropping the fd instanceof FileHandle branch at the fstat call is behavior-preserving; receivedValueLabel still has a caller (line 281), so nothing went dead.
• Test: hermetic (listen(0), awaits req close, try/finally cleanup), every row is doubly-invalid so it can only pass with the new ordering, and destroyedAfterClose: false proves the closed rows exercise the new check rather than destroyed.

One small note: the PR description still says the fd block's "content is unchanged" and that #38644 rewrites it — after aa85f1a this PR does that rewrite itself, so the description/overlap notes are a bit stale.

Extended reasoning...

Overview

The PR touches two files: src/js/node/http2.ts (the ServerHttp2Stream.respondWithFile / respondWithFD argument validation) and test/js/node/http2/node-http2.test.js (one new test). In respondWithFD the fd check moves below the stream-state and options checks, unwraps only a real FileHandle ($data.FileHandle from node:fs/promises, imported at line 37), and throws via $ERR_INVALID_ARG_TYPE. Both methods now reject this.closed in addition to this.destroyed. The later fs.fstat(fd, ...) call is collapsed to the numeric fd, which is correct since the unwrap now always happens first.

Security risks

None identified. The change removes a path where a user-supplied getter (fd.fd on a duck-typed object) could run between the headersSent guard and the response, which was the main behavioral risk flagged on the previous push; it is now closed by the instanceof check. No new user-controlled data flows into native code — doSendFileFD receives the same numeric fd it previously did after the old duck-typed unwrap.

Level of scrutiny

Moderate. This is a node-compat ordering fix in built-in JS, small and self-contained, with a direct reference implementation (node's respondWithFD), which I compared against: node checks destroyed || closed, then headersSent, then assertIsObject(options) and the offset/length/statCheck value checks, then fd instanceof FileHandle ? fd = fd.fd : typeof fd !== 'number' -> ERR_INVALID_ARG_TYPE('fd', ['number','FileHandle'], fd), then headers. The diff matches that exactly. I also read ERR_INVALID_ARG_TYPE in src/jsc/bindings/ErrorCode.cpp to confirm ["number", "FileHandle"] renders as "of type number or an instance of FileHandle" and that a plain object renders as "Received an instance of Object", so the expected strings in the test are what this build will produce. I could not run the test locally (no debug build present in this checkout), so correctness of the expected strings rests on that reading plus the author's stated node v26.3.0 comparison.

Other factors

My prior-run inline findings were: (1) fd getter re-entry after the state checks — fixed by instanceof FileHandle; (2) respond() still lacking the closed check — pre-existing, explicitly deferred to #33380 in the description, not blocking; (3) hand-rolled TypeError with assigned .code — fixed by $ERR_INVALID_ARG_TYPE. receivedValueLabel remains live via its use at line 281. The new test is hermetic (listen(0, "127.0.0.1"), awaits the client request close event, try/finally cleanup of client and server), every row passes multiple bad arguments so the observed error proves which check ran first, and the destroyedAfterClose: false row demonstrates that the closed rows exercise the new predicate rather than the pre-existing destroyed one. No CODEOWNERS concern was identified for these paths, and there are no outstanding third-party objections in the timeline. The only remaining nit is a stale PR description ("fd block content unchanged" / "#38644 rewrites it") that no longer reflects the second commit.

Still open from earlier reviews (1):

• Unresolved: 1 minor or pre-existing.

[robobun]

Replies to the two automatic reviews and to the comment check. Two commits followed the first review: aa85f1a and ec43141.

1. A user fd getter ran after the state checks (introduced by my first commit). Fixed in aa85f1a. I knew about this case before I opened the PR and documented it as acceptable. The review was right that it must not ship: on main that call threw, and after my move it did not. respondWithFD() now uses node's check: fd instanceof FileHandle, else typeof fd !== "number" throws. No property of another object is read. Measured on the new build: the two getter cases from my probe give ERR_INVALID_ARG_TYPE as in node, and the test row fdGetterRan is false. A real FileHandle still works (test-http2-respond-file-filehandle.js passes). Behavior change: respondWithFD({ fd: 3 }) with a plain object now throws, as in node.

2. respond() has no closed check. Not changed in this PR. The reason is in the reply on that thread. The PR description lists it under "Not fixed here", with what I measured on main.

3. The fd error was built by hand. Fixed in aa85f1a with $ERR_INVALID_ARG_TYPE("fd", ["number", "FileHandle"], fd). Measured: String(err) was TypeError: The "fd" argument ... and is now TypeError [ERR_INVALID_ARG_TYPE]: The "fd" argument ..., the same as node v26.3.0. test-http2-respond-file-fd-errors.js passes, so the message is the same for the six value types it tries. The unreachable fd instanceof FileHandle branch before the fstat call is removed.

4. headersSent stays false after respondWithFD(). Not fixed in this PR. I measured the same result on main and with this PR. I found no open PR that covers it. The PR description has the repro under "Not fixed here".

Code comment above the fd check. Removed in ec43141. The test rows fdPlainObject and fdGetterRan now guard that behavior.

Stale description. Correct at the time of the second review: the description still said that the content of the fd block was unchanged. I updated the title and the description after that push. They now describe both commits, and the overlap note for #38644 says that this PR contains the respondWithFD() half of it.

Checks I ran on aa85f1a (ec43141 only removes a comment, and the new test passes with it): the new test fails with the src/ of the base branch and passes with this one. The 22 vendored node tests that call respondWithFD or respondWithFile pass. All 22 pairs of failing checks that I compared give the same error as node v26.3.0.

[claude]

LGTM — the two follow-up commits address the earlier feedback (the fd block now unwraps only a real FileHandle via instanceof and throws through $ERR_INVALID_ARG_TYPE), and the reordered checks match node's respondWithFD.

What was reviewed:

• Check order in respondWithFD/respondWithFile at src/js/node/http2.ts:3314-3403 against node's core.js: state, headersSent, options, fd, headers — matches.
• $ERR_INVALID_ARG_TYPE("fd", ["number", "FileHandle"], fd) rendering: ErrorCode.cpp groups FileHandle as a class name, so the message is node's verbatim; determineSpecificType reads only constructor, so a user fd getter never runs.
• Passing the unwrapped number (not the FileHandle) into doSendFileFD/createReadStream: kOwnsFd is false so autoClose stays off and the caller keeps ownership, same as node.
• receivedValueLabel still has a caller at :281, so nothing became dead.

Extended reasoning...

Overview

The diff touches two files: src/js/node/http2.ts (the respondWithFile and respondWithFD methods of ServerHttp2Stream) and test/js/node/http2/node-http2.test.js (one new test). In respondWithFD, the hand-rolled fd TypeError that used to run first is replaced by node's fd instanceof FileHandle ? fd = fd.fd : typeof fd !== "number" → $ERR_INVALID_ARG_TYPE, positioned after the state/headersSent/options checks and before the headers check. Both file methods now also reject closed streams (this.destroyed || this.closed), and the duplicate instanceof FileHandle branch before fs.fstat is collapsed since fd is already a number at that point. The new test drives a server stream through doubly-invalid calls and asserts a single results object with exact code+message strings.

Security risks

None identified. The change narrows what respondWithFD accepts (only numbers and real FileHandle instances, which is node's contract) and removes the read of an arbitrary user object's fd property, so less user code runs during validation than before. FileHandle is captured at module load from require("node:fs/promises").$data, not from a user-overridable global. The $ERR_INVALID_ARG_TYPE message builder only reads constructor/name on the rejected value, the same as node's determineSpecificType.

Level of scrutiny

Low-to-moderate. This is a small, self-contained node-compat reorder in built-in JS, and the two commits since my prior review directly address the two points I raised then (user getter running after the state guards; hand-assigned err.code). I verified from the code, not the PR description, that both are fixed. I checked that unwrapping the FileHandle to a number before doSendFileFD does not change ownership semantics: kOwnsFd is set to false in respondWithFD, so createReadStream gets autoClose: false and the fd stays the caller's, matching node's own fd = fd.fd unwrap. The remaining pre-existing gap (respond() on a closed stream) was noted earlier as non-blocking and is explicitly scoped out in the PR description with references to other PRs that cover it.

Other factors

The new test asserts its precondition (destroyedAfterClose: false) so the closed rows genuinely exercise the new || this.closed clause, wires the client/req error events, awaits the request close event rather than sleeping, and releases resources in finally. The vendored node tests test-http2-respond-file-fd-errors.js, -fd-invalid.js, and -filehandle.js cover the same surface (every non-number type, options type matrix, real FileHandle), which gives additional guard against regressions in the message text and the FileHandle path. I did not build and run the tests in this session (no debug build present); my conclusion rests on reading the diff, the ErrorCode.cpp message renderer, and the doSendFileFD/close() paths. The PR overlaps textually with other open PRs on the same lines, which is a rebase/ordering matter rather than a correctness concern.