node:http: send 500 instead of 200 when handler throws synchronously

[robobun]

Reproduction

const http = require("node:http");
process.on("uncaughtException", () => {}); // keep process alive
const server = http.createServer((req, res) => {
  throw new Error("boom");
});
server.listen(0, async () => {
  const res = await fetch(`http://127.0.0.1:${server.address().port}/`);
  console.log(res.status); // before: 200, after: 500
});

Root cause

When a node:http request handler throws (or its returned promise rejects) before the response has completed, onNodeHTTPRequestWithUpgradeCtx in src/bun.js/api/server.zig cleans up by writing a status and ending the response. The branch that writes "500 Internal Server Error" was guarded by raw_response.state().isHttpStatusCalled() — i.e. it only ran when a status line had already been written, which makes writeStatus a no-op. When no status had been written, control fell into the else arm and called endStream(true), which bottoms out in uWS sendTerminatingChunk() → writeStatus(HTTP_200_OK).

The condition is simply inverted; the equivalent rejection handler in NodeHTTPResponse.Bun__NodeHTTPRequest__onReject uses !isHttpStatusCalled() correctly.

Fix

Invert the condition so that:

• if no status has been sent yet → writeStatus("500 Internal Server Error") + endWithoutBody(true)
• if a status was already sent → endStream(true) to terminate the in-flight chunked response

Verification

New test test/js/node/http/node-http-sync-throw-500.test.ts:

• before fix (and with USE_SYSTEM_BUN=1):

  Expected: "STATUS_LINE:HTTP/1.1 500 Internal Server Error"
  Received: "STATUS_LINE:HTTP/1.1 200 OK"
  

• after fix: both tests pass — the sync-throw case returns 500, and the throw-after-writeHead(201) case still preserves the 201 status while cleanly terminating the stream.

The existing node-http.test.ts suite passes (the single request via http proxy failure is pre-existing ECONNREFUSED flake, reproduces without this change).

[coderabbitai]

Walkthrough

A bug fix inverts the condition that guards writing a "500 Internal Server Error" status when a JS exception occurs during HTTP request handling, ensuring the status is written only when no prior status has been set. New tests validate the corrected behavior for both synchronous throws before and after writeHead.

Changes

HTTP Error Status Handling

Layer / File(s)	Summary
Bug Fix src/runtime/server/server.zig	In onNodeHTTPRequestWithUpgradeCtx, the condition for writing "500 Internal Server Error" is inverted from isHttpStatusCalled() to !isHttpStatusCalled(), ensuring 500 status is written only when no HTTP status has been set.
Test Coverage test/js/node/http/node-http-sync-throw-500.test.ts	Two concurrent test cases validate the corrected behavior: synchronous throw before any status writes a 500 response; synchronous throw after writeHead(201) and flushHeaders() preserves the 201 status line.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main fix: inverting a condition to send 500 instead of 200 when a node:http handler throws synchronously.
Description check	✅ Passed	The PR description is comprehensive, covering reproduction case, root cause analysis, the fix, and verification with test results. It exceeds the template requirements.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Review rate limit: 3/5 reviews remaining, refill in 22 minutes and 20 seconds.}

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

[robobun]

^{Updated 3:07 PM PT - May 4th, 2026}

❌ @robobun, your commit 3c8651d has 2 failures in Build #51204 (All Failures):

• test/js/web/fetch/fetch-http2-client.test.ts - pid 7635 internal assertion failure on 🐧 13 x64-asan
• test/js/bun/test/parallel/test-http-should-emit-close-when-connection-is-aborted.ts - timeout on 🪟 11 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 30075

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

bun-30075 --bun

[claude]

LGTM — one-character condition inversion that brings the sync-throw cleanup path in line with the already-correct async rejection handler, with tests covering both branches.

Extended reasoning...

Overview

This PR is a single-character logic fix in src/bun.js/api/server.zig (line 2147): inverting if (raw_response.state().isHttpStatusCalled()) to if (!raw_response.state().isHttpStatusCalled()) in the node:http handler exception/rejection cleanup path. It also adds a new test file with two tests covering both arms of the branch (throw before any status → 500, throw after writeHead(201) → 201 preserved + stream terminated).

Correctness

The fix is verifiably correct by inspection. writeStatus("500 Internal Server Error") only makes sense when no status line has been written yet, i.e. when isHttpStatusCalled() is false. The parallel async-rejection handler at src/bun.js/api/server/NodeHTTPResponse.zig:680 already uses !isHttpStatusCalled() to guard the same writeStatus call, confirming the original condition here was simply inverted. The previous behavior — endStream(true) falling through to uWS's default HTTP_200_OK when the handler threw — was clearly a bug.

Security risks

None. This changes only the HTTP status code emitted on an error path from an incorrect 200 to a correct 500. No auth, crypto, input parsing, or memory-safety surface is touched.

Level of scrutiny

Low. This is a mechanical one-token bug fix with a clear root-cause analysis, a sibling code path that already does it correctly, and regression tests that exercise both the fixed branch and the unchanged branch. No design decisions are involved.

Other factors

No CODEOWNERS entry covers this path. No outstanding human reviewer comments. The bug-hunting system found no issues. The new tests use raw TCP sockets to assert the exact status line, which is robust against client-side normalization.

[robobun]

CI status — all failures are pre-existing/infra, unrelated to this one-line node:http change. node-http-sync-throw-500.test.ts passes on every shard that ran it.

Build 51204 — ongoing CI agent outage: 21 jobs expired, 106 waiting_failed. Actual test failures:

• test-http-should-emit-close-when-connection-is-aborted.ts timeout on Win11 aarch64 — also failing on builds 51275, 51279, 51195, 51188, 51187, 51186, 51185
• fetch-http2-client.test.ts ASAN assertion (experimental HTTP/2 client) — also failing on build 51185; has prior deflake history (test(fetch-http2-client): deflake 'client RSTs on local error' via same-session barrier #29954)

Earlier builds

Build 51002 — full CI agent outage, nothing compiled. Same hit main build 50956.

Build 49828 — pre-existing flakes (bake/dev-and-prod.test.ts, no-orphans.test.ts, s3-storage-class.test.ts). New test passed on darwin (61ms/69ms) and all Linux/Windows lanes.

[claude]

LGTM — one-character logic inversion that matches the existing async-rejection path, with tests covering both branches.

Extended reasoning...

Overview

This PR flips a single boolean condition in src/runtime/server/server.zig (onNodeHTTPRequestWithUpgradeCtx's exception/rejection cleanup) from if (raw_response.state().isHttpStatusCalled()) to if (!raw_response.state().isHttpStatusCalled()). The original condition was clearly inverted: it tried to writeStatus("500 Internal Server Error") only when a status had already been written (a no-op), and otherwise fell through to endStream(true) which defaults to 200 OK. The fix is accompanied by a new test file with two cases: handler throws before any status → expect 500, and handler throws after writeHead(201) + flushHeaders() → expect 201 preserved with the stream cleanly terminated.

Security risks

None. This only affects which HTTP status code is emitted on the error path when a node:http handler throws synchronously. No new data is exposed; if anything, returning 500 instead of a misleading 200 is the safer/more correct behavior.

Level of scrutiny

Low. The change is a single ! and the corrected logic is self-evidently right ("write a 500 status only if no status has been written yet"). I cross-checked the parallel async path Bun__NodeHTTPRequest__onReject in src/runtime/server/NodeHTTPResponse.zig:680, which already uses !isHttpStatusCalled() for the identical purpose — this PR brings the sync path into agreement with it.

Other factors

• The new test exercises both arms of the corrected branch via raw TCP so it asserts the actual wire status line, and CI confirms it passes on all 15+ platforms that ran it.
• No bugs were flagged by the bug-hunting system.
• No outstanding human review comments; CI failures listed by robobun are documented pre-existing flakes unrelated to this change.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/js/node/http/node-http-sync-throw-500.test.ts`:
- Around line 25-35: In the server.listen callback, remove the explicit
process.exit(0) call and instead await server.close() so the server fully closes
before the script naturally exits; update the callback that currently calls
server.close(); process.exit(0) to use await server.close() (or return after
awaiting server.close()) in both embedded scripts so stdout/stderr flush
reliably and the process terminates naturally.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 817439d9-2c2a-460c-b49a-882a91519132

📥 Commits

Reviewing files that changed from the base of the PR and between 0a7bed5 and 3c8651d.

📒 Files selected for processing (2)

• src/runtime/server/server.zig
• test/js/node/http/node-http-sync-throw-500.test.ts

[claude]

LGTM — one-character condition inversion that matches the existing async-rejection handler, with tests covering both branches.

Extended reasoning...

Overview

This PR fixes a single inverted boolean condition in src/runtime/server/server.zig (onNodeHTTPRequestWithUpgradeCtx's exception/rejection cleanup path). The previous code wrote 500 Internal Server Error only when isHttpStatusCalled() was true — i.e., when a status had already been sent and writeStatus is a no-op — and otherwise fell through to endStream(true), which defaults to 200 OK. The fix adds a ! so the 500 is written only when no status line has been sent yet, and the in-flight response is terminated via endStream otherwise.

I verified the PR's claim that the parallel async-rejection handler Bun__NodeHTTPRequest__onReject in src/runtime/server/NodeHTTPResponse.zig:680 already uses !isHttpStatusCalled() with the same write-500 / end-stream split. This change simply brings the sync path to parity with that established pattern.

A new test file exercises both branches: (1) handler throws before any write → expects HTTP/1.1 500 Internal Server Error, and (2) handler throws after writeHead(201) + flushHeaders() → expects the 201 status and header to be preserved while the connection still terminates cleanly. CI confirms the new test passes on all shards; reported failures are pre-existing flakes unrelated to node:http.

Security risks

None. The change only affects which HTTP status code is emitted on the error path of a node:http handler that throws. No new data is exposed, no auth/permission logic is touched, and the error message body remains empty (endWithoutBody).

Level of scrutiny

Low. This is a one-token logic fix whose correctness is self-evident from the semantics of writeStatus (only meaningful before a status has been sent) and is corroborated by an identical existing code path elsewhere in the codebase. The blast radius is limited to the already-broken error-handling branch.

Other factors

• No CODEOWNERS entry covers this file.
• No outstanding human review comments; only bot/CI status comments on the timeline.
• The bug-hunting system found no issues.
• Both code branches are now covered by regression tests that read the raw status line over a TCP socket, so the behavior is pinned.