node:http: convert write()/end() arguments before reading response state

[Jarred-Sumner]

What

node:http ServerResponse.write(chunk, encoding) / end(...): NodeHTTPResponse::write_or_end read the response state (ended / socket closed / pending), then converted encoding and the chunk (Encoding::from_js, StringOrBuffer), which can run user JS (toString / Symbol.toPrimitive) that destroys the response. It then continued into the zero-copy write path with a closed socket and a null this value (UBSan: member call on null JSCell; stock SEGV at 0x28).

write_or_end now converts all arguments first and reads the response state once, after that (this is also Node's order: write_() validates the chunk before the write-after-end / destroyed checks).

Repro (before)

import http from "http";
const server = http.createServer((req, res) => {
  const enc = Object.assign(new String("hex"), { [Symbol.toPrimitive]() { res.destroy(); Bun.gc(true); return "hex"; } });
  res.write("41".repeat(20000), enc);   // crash
  setTimeout(() => process.exit(0), 100);
});
server.listen(0, () => fetch(`http://127.0.0.1:${server.address().port}/`).catch(() => {}));

Tests

test/js/node/http/node-http.test.ts — "ServerResponse.write() with an encoding whose toPrimitive destroys the response does not crash". Fails on the ASan canary and debug main; passes here.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 25 minutes

Limit details: You’ve used all 1 included review currently available under your plan. You completed 71 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6ec365f5-c041-4ac5-8089-4911a74b42ad

📥 Commits

Reviewing files that changed from the base of the PR and between fea1829 and df8ca56.

📒 Files selected for processing (2)

• src/runtime/server/NodeHTTPResponse.rs
• test/js/node/http/node-http.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 9:30 PM PT - Aug 16th, 2026}

✅ @Jarred-Sumner, your commit df8ca5696d615e1cfb4e600df0b1aaa02a059cff passed in Build #99838! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 39386

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

bun-39386 --bun

[claude]

🟡 The it.each only passes a hostile encoding to res.write()/res.end() — both route into write_or_end. Neither row sets a hostile res.statusMessage (so write_head_impl's to_bun_string at :948 stays on the is_undefined() short-circuit) and neither reaches write_informational, so reverting either of those two reorderings would not break any test in this PR (REVIEW.md: "Confirm deleting each load-bearing clause of your fix breaks at least one test"). Consider a third row, e.g. ["writeHead via statusMessage", 'res.statusMessage = enc; result = res.write("x");', "threw ERR_STREAM_ALREADY_FINISHED"], to pin the write_head_impl fix.

Extended reasoning...

What the gap is

This PR now applies the same "convert arguments before reading response state" reordering to three functions in NodeHTTPResponse.rs:

1. write_or_end<IS_END> — the original crash fix.
2. write_head_impl — state checks (is_requested_completed_or_ended(), SOCKET_CLOSED, raw_response, handle_ended_if_necessary) moved from :912 down to :959–978, after status_message_value.to_bun_string(global_object)? at :948.
3. write_informational — is_done() / raw_response / handle_ended_if_necessary moved to :1251–1259, after Encoding::from_js / from_js_with_encoding_into.

The last two were added in response to the earlier "fix the whole class in the same PR" review comment. But the new it.each at node-http.test.ts:2284-2325 still only exercises the first.

Step-by-step: why neither row reaches the sibling fixes

["write", …] row: res.write(payload, enc) enters the JS write path → handle.cork(() => { handle.writeHead(this.statusCode, this[kSnapshotStatusMessage] ?? this.statusMessage, headers); handle.write(chunk, enc, …) }). The fixture never assigns res.statusMessage, so it is undefined and kSnapshotStatusMessage is unset (only set inside an explicit writeHead()). Native write_head_impl receives status_message_value = undefined, and at :946 !status_message_value.is_undefined() is false — to_bun_string() at :948 never runs, so the moved re-check at :959–978 is never the thing that observes the destroyed state. The hostile enc only fires later inside handle.write → write_or_end<false>, which is the covered path.

["end", …] row: res.flushHeaders() calls writeHead with the same statusMessage = undefined (benign), then res.end(payload, enc) sees headersSent === true and goes straight to write_or_end<true> without re-entering write_head_impl. Again only write_or_end is exercised.

Neither row calls anything that reaches write_informational (that is only entered via res._writeRaw for 1xx responses).

Why REVIEW.md flags this

REVIEW.md, Tests reviewers reject: "Every behavioral change ships an automated test in the same PR" and "Confirm deleting each load-bearing clause of your fix breaks at least one test — a test that passes both ways is worse than no test." The reordering in write_head_impl and write_informational is a behavioral change (it turns a use-of-stale-raw_response into a clean early-return/throw), but reverting either block to its pre-PR position would leave every test in this PR green.

This is not a duplicate of the existing timeline comments: the earlier sibling-site comment asked for the fix (now applied), and the earlier "cover end()" comment asked for the IS_END = true arm (now the second row). This is the remaining gap: tests for the two applied sibling fixes.

Suggested fix

The fixture already builds a hostile enc = Object.assign(new String("hex"), { [Symbol.toPrimitive]() { res.destroy(); Bun.gc(true); return "hex"; } }), so a third row can reuse it as the status message:

["writeHead via statusMessage", `res.statusMessage = enc; result = res.write("x");`, "threw ERR_STREAM_ALREADY_FINISHED"],

Trace: res.statusMessage = enc (plain data property, no validating setter in http1) → res.write("x") → handle.cork → handle.writeHead(200, enc, …) → write_head_impl :946 !is_undefined() is true → :948 to_bun_string invokes Symbol.toPrimitive → res.destroy() → abort() sets SOCKET_CLOSED and, via on_request_complete(), REQUEST_HAS_COMPLETED → back at the moved :959 is_requested_completed_or_ended() is true → throws ERR_STREAM_ALREADY_FINISHED → propagates out of cork → fixture's catch prints threw ERR_STREAM_ALREADY_FINISHED. (Confirm the exact expected string with bun bd test; if the SOCKET_CLOSED branch at :972 wins instead, the expected becomes "returned boolean" — either way the row pins the reordering.)

Covering write_informational from public API is harder (it's reached via internal _writeRaw for 1xx); if the author considers it not publicly reachable with a hostile object, saying so in the PR description satisfies REVIEW.md's "if a site is intentionally excluded, say so in the PR."

Severity

Nit. The primary crash (write_or_end) is properly tested for both write() and end(), and the two sibling reorderings are structurally identical moves of the same guard block — the risk of one being wrong while write_or_end is right is low. The PR is strictly safer than before; this only tightens the mutation-testing bar the repo's own review guide sets.