Repository navigation
Conversation
…ing 204 When error() returned a promise that settled after the server's microtask drain, the pending branch of process_on_error_promise registered fetch's resolve/reject callbacks and dropped the original error and status. A later fulfillment with a non-Response (the common case: an async error() that awaits and forgets to return a Response) reached render_missing_invalid_response, which responded 204 and printed fetch's invalid-return diagnostic instead of reporting the original error. Keep the original error rooted across the await and, when the deferred error() promise fulfills with a non-Response, route it through finish_running_error_handler at 500, matching the synchronous and already-settled twins.
|
Updated 4:12 PM PT - Jun 30th, 2026
❌ @robobun, your commit 6228d6f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33140That installs a local version of the PR into your bun-33140 --bun |
Walkthrough
ChangesDeferred server return-value handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/server/RequestContext.rs`:
- Around line 746-769: The deferred error in RequestContext::handle_resolve is
being unprotected too early via take_error_promise_value(), then passed unrooted
into finish_running_error_handler(). Update the error-handling flow so the
original_error stays protected for the entire reporting/rendering call, and only
unprotect or clear it after finish_running_error_handler() returns; use the
existing RequestContext::take_error_promise_value, handle_resolve, and
finish_running_error_handler symbols to locate the fix.
In `@test/js/bun/http/serve.test.ts`:
- Around line 1967-1968: The deferred-error check is ambiguous because the
generic "/deferred" substring also matches "/deferred-null", so it may not
verify the intended invariant. Update the assertion in the serve test to target
the exact deferred diagnostic line or split the checks so the plain deferred
undefined path and the deferred-null path are asserted separately using a
delimiter or more specific match in the same test block.
- Around line 1934-1946: The readiness handshake in the Bun spawn test can hang
if server.js exits before sending the IPC message. Update the test around
Promise.withResolvers and Bun.spawn so the readiness promise is rejected when
proc.exited resolves/rejects before ipc() calls resolve, instead of waiting for
the outer timeout. Also start draining stderr immediately when spawning the
process so failure output is consumed concurrently, and keep the await promise
path tied to the proc lifecycle in serve.test.ts.
🪄 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: e4061ba5-3af2-4923-bb2d-8c702bd823ec
📒 Files selected for processing (2)
src/runtime/server/RequestContext.rstest/js/bun/http/serve.test.ts
- Hold the protect() on the deferred error across finish_running_error_handler (which re-enters JS and can run GC), releasing it only after the report. - Fail the spawn test fast if the server dies before the URL handshake, and drain stderr concurrently. - Assert each deferred variant's error at a line boundary so /deferred does not also satisfy /deferred-null.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues, but this touches core RequestContext lifecycle and JSValue protect/unprotect semantics in the HTTP server hot path, so it warrants a human look before merging.
Extended reasoning...
Overview
This PR fixes #33137: a Bun.serve error() handler returning a pending Promise that resolves to a non-Response was responding 204 and printing fetch's invalid-return diagnostic instead of responding 500 and reporting the original error (as the sync and already-settled twins do). The fix adds an error_promise_value: JSValue field to RequestContext, protects it when process_on_error_promise hits the Pending arm, and routes deferred non-Response resolutions in handle_resolve through finish_running_error_handler(original_error, 500). Cleanup is added in finalize_without_deinit for the abort-while-pending path. A thorough subprocess regression test in serve.test.ts covers sync / already-settled / deferred × undefined / null, plus a deferred Response (which still renders).
The earlier coderabbit feedback (keep the error rooted across the JS-reentering report, fail-fast on subprocess exit before IPC, disambiguate the /deferred vs /deferred-null stderr assertion) was addressed in 7595fb9 — finish_running_error_handler now runs before clear_error_promise_value, and the helper is idempotent.
Security risks
None identified. This is error-path control flow and GC rooting; no new input parsing, auth, or data-exposure surface.
Level of scrutiny
High. RequestContext.rs is the per-request state machine for Bun's HTTP server, and the change adds a new GC-protected JSValue field with protect/unprotect bookkeeping across async boundaries (resolve, reject, abort, finalize). The reject path (handle_reject) does not explicitly clear the new field and relies on finalize_without_deinit for cleanup, which looks correct given has_called_error_handler prevents re-entry into process_on_error_promise, but a maintainer familiar with the full lifecycle should confirm.
Other factors
- CI shows musl build failures and Windows aarch64 agent-creation failures on this PR; they look like infrastructure rather than this change, but worth confirming.
- The fix is well-scoped, mirrors the existing
Fulfilledarm's behavior, and the test is comprehensive. I traced the protect/unprotect pairing and it appears balanced and idempotent. My deferral is purely about the criticality of the code path, not a suspected defect.
|
On the one lifecycle point worth calling out (the reject path): it is intentionally unchanged. When the deferred On the protect/unprotect balance: |
There was a problem hiding this comment.
No issues found, but this adds a GC-protected JSValue field with protect/unprotect balanced across several lifecycle paths (resolve/reject/abort/finalize) in the core Bun.serve request context — worth a human pass on the rooting invariants.
Extended reasoning...
Overview
This PR fixes #33137: a Bun.serve error() handler returning a pending Promise<undefined> was responding 204 and printing fetch's invalid-return diagnostic instead of responding 500 and reporting the original error (as the sync and already-settled paths do). The fix adds an error_promise_value: JSValue field to RequestContext, protects the original error when the error() promise is pending, and in handle_resolve routes a non-Response fulfillment through finish_running_error_handler(original_error, 500) before releasing the root. clear_error_promise_value() is idempotent and is also called from finalize_without_deinit to cover the reject and abort-while-pending paths. A thorough subprocess regression test in serve.test.ts covers sync / already-settled / deferred × undefined/null, plus a deferred Response.
Security risks
None apparent. This is error-handling control flow inside the HTTP server; no auth, crypto, or untrusted-input parsing is touched. The user-observable change is 204→500 for a misbehaving error() handler, which is strictly less information disclosed than before.
Level of scrutiny
High. RequestContext is the per-request state machine at the heart of Bun.serve, and the change introduces manual JSValue protect/unprotect across async boundaries. The protect in process_on_error_promise's Pending arm must be matched by exactly one unprotect across resolve (explicit clear_error_promise_value after finish_running_error_handler), reject (deferred to finalize_without_deinit), early-return-on-abort/upgrade (deferred to finalize_without_deinit), and the deferred-Response fall-through (explicit clear). The robobun lifecycle note and the addressed CodeRabbit comment both speak to this; the reasoning looks sound, but a leak or premature unprotect here would be a memory bug in production-critical code, so it warrants human confirmation rather than bot auto-approval.
Other factors
CodeRabbit raised three points (GC root ordering, test readiness hang, ambiguous assertion) and all were addressed in commit 7595fb9 — the current diff keeps the value protected through the JS-reentering finish_running_error_handler call, drains stderr concurrently with a fail-fast on early child exit, and matches /deferred at a line boundary. The bug-hunting system found no issues. The change is focused and well-tested; my only reason to defer is the GC-lifecycle sensitivity of the touched code path.
|
CI status: green on every lane that exercises this change.
These leak/RSS/terminal tests are timing-sensitive and pass on recent |
render_production_error wrote "Something went wrong!" for every method, so a HEAD request reaching the error path emitted a 21-byte body and desynced the keep-alive connection. This is pre-existing (the sync error() path did it too), but the deferred error() fix now routes HEAD through here as well. Route HEAD through end_without_body, mirroring the 404 arm, so no body bytes are written. Tests: add a rejected fetch() promise that routes through handle_reject into error(); a deferred fetch() non-Response that must still respond 204 (pins the is_error_promise_pending gate); and a keep-alive HEAD check proving the 500 writes no body and the next response on the socket is not desynced.
|
Pushed
The normal HEAD framing path ( |
|
Re: the "out of scope" pre-merge warning on the HEAD change. The HEAD guard is not incidental cleanup; it keeps this fix from introducing a regression. Before this PR, a HEAD request whose deferred I put it in |
There was a problem hiding this comment.
I didn't find any bugs, but this touches the core Bun.serve request lifecycle (GC rooting of a stashed JSValue, a new state flag gating the shared fetch/error resolve callback, and HEAD framing in render_production_error), so it's worth a human pass on the protect/unprotect balance and the finalize path.
Extended reasoning...
Overview
This PR fixes #33137: a Bun.serve error() handler that returns a pending Promise<non-Response> was responding 204 and emitting fetch's invalid-return diagnostic instead of reporting the original error at 500 like its sync/already-settled twins. The fix is in src/runtime/server/RequestContext.rs:
- Adds an
error_promise_value: JSValuefield onRequestContext,protect()ed inprocess_on_error_promise'sPendingarm and released via a new idempotentclear_error_promise_value()inhandle_resolve(after reporting) and infinalize_without_deinit(abort-while-pending). - Gates
handle_resolveonis_error_promise_pending: a non-Response fulfillment routes tofinish_running_error_handler(original_error, 500); a Response fulfillment clears the stash and falls through to render normally. - Separately suppresses the body on HEAD in
render_production_error(a pre-existing keep-alive desync the deferred path now reaches more often).
Tests in serve.test.ts cover the full matrix (sync / already-settled / deferred × undefined / null / Response / fetch-rejects), a raw-socket HEAD-then-GET keep-alive check, and a negative test pinning that fetch()'s own deferred non-Response still gets 204.
Security risks
None identified. This is error-path response selection and GC rooting; no auth, crypto, input parsing, or privilege boundaries are touched. The HEAD fix tightens RFC 9110 conformance (prevents a stray body desyncing a keep-alive connection), which is if anything a hardening.
Level of scrutiny
Moderate-to-high. RequestContext is the per-request state machine for every Bun.serve request, and the change introduces:
- A new
protect()/unprotect()pair whose balance depends onhas_called_error_handlerguaranteeing thePendingarm runs at most once, and on every exit path (resolve, reject→finalize, abort→finalize) reaching exactly oneclear_error_promise_value(). The author's analysis in the thread is convincing and the helper is idempotent, but GC-root lifecycle in a shared resolve callback is exactly the kind of thing a maintainer should eyeball. - A behavioral branch in
handle_resolvekeyed on a flag, where the same callback serves both fetch() and error() promises. - An HTTP framing change for HEAD on the production-error path.
None of this is a one-line config tweak; it's real logic in a hot, correctness-sensitive path.
Other factors
- The bug-hunting system found nothing.
- CodeRabbit's three findings (root-across-report, subprocess readiness reject, ambiguous
/deferredsubstring match) are all marked addressed in 7595fb9 and the current diff reflects those fixes. - CI is reported green on every lane that exercises this change; the two red lanes are unrelated flaky leak tests per the author's analysis.
- Test coverage for the fix is thorough, including a negative test that pins the
is_error_promise_pendinggate so a regression that hijacks fetch()'s own 204 path would fail.
Given the scope (core server lifecycle + manual GC rooting + framing change), I'm deferring rather than auto-approving.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Do not add another field. We don't need that.
|
Good call, dropped the field. Reworked to reuse the existing I also pulled the HEAD-framing change back out to keep this focused on the deferred |
Per review, remove the error_promise_value field. A deferred error() resolving to a non-Response already flows through render_missing_invalid_response, so gate that function's final render on the existing is_error_promise_pending flag: render 500 (like the sync and already-settled twins) instead of the fetch path's 204. No new state, no protect/unprotect. Also drop the HEAD-body change to keep this focused on the deferred error() fix; the body-on-HEAD for error 500s is pre-existing (the sync error() path hits it too) and can be addressed separately. Tests keep the deferred/sync/settled matrix (all 500), the rejected-fetch route into error(), and the negative test pinning that fetch()'s own deferred non-Response still responds 204.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/server/RequestContext.rs`:
- Around line 736-741: The deferred error branch in RequestContext’s error
handling still bypasses the shared error path, so the original `error()` value
is lost. Update the `is_error_promise_pending` branch to route through
`finish_running_error_handler(value, 500)` instead of calling
`render_production_error(500)` directly, and ensure the stored error value is
preserved so `vm.on_unhandled_rejection` and the `DEBUG_MODE` fallback page
still run.
🪄 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: 5d00956d-ba1b-4978-8956-7d7dd4b24e14
📒 Files selected for processing (2)
src/runtime/server/RequestContext.rstest/js/bun/http/serve.test.ts
| // A deferred `error()` handler that produced no Response responds 500, | ||
| // like its sync and already-settled twins, instead of the fetch path's 204. | ||
| if self.flags.is_error_promise_pending() { | ||
| self.flags.set_is_error_promise_pending(false); | ||
| return self.render_production_error(500); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant regions in RequestContext.rs and the matching test.
FILE="src/runtime/server/RequestContext.rs"
TEST="test/js/bun/http/serve.test.ts"
echo "== $FILE (around 3300-3485 and 720-760) =="
sed -n '3300,3485p' "$FILE"
echo
sed -n '720,760p' "$FILE"
echo
echo "== $TEST (around 1960-2010) =="
sed -n '1960,2010p' "$TEST"Repository: oven-sh/bun
Length of output: 11407
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all references to the error-promise flag and the helper that renders the fallback path.
FILE="src/runtime/server/RequestContext.rs"
rg -n "is_error_promise_pending|finish_running_error_handler|render_production_error|render_missing_invalid_response|on_unhandled_rejection|render_default_error" "$FILE"Repository: oven-sh/bun
Length of output: 2159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact implementations for the helper functions mentioned in the review.
FILE="src/runtime/server/RequestContext.rs"
# Prefer precise line ranges if the file is large.
nl -ba "$FILE" | sed -n '3330,3475p'
echo
nl -ba "$FILE" | sed -n '730,745p'Repository: oven-sh/bun
Length of output: 189
Deferred error path still skips the original error handling.
This returns 500, but it bypasses finish_running_error_handler(value, 500), so the original error() value never reaches vm.on_unhandled_rejection, and DEBUG_MODE loses the richer fallback page. Route this branch through the shared helper instead of calling render_production_error(500) directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/server/RequestContext.rs` around lines 736 - 741, The deferred
error branch in RequestContext’s error handling still bypasses the shared error
path, so the original `error()` value is lost. Update the
`is_error_promise_pending` branch to route through
`finish_running_error_handler(value, 500)` instead of calling
`render_production_error(500)` directly, and ensure the stored error value is
preserved so `vm.on_unhandled_rejection` and the `DEBUG_MODE` fallback page
still run.
| if self.flags.is_error_promise_pending() { | ||
| self.flags.set_is_error_promise_pending(false); | ||
| return self.render_production_error(500); | ||
| } |
There was a problem hiding this comment.
🔴 After the rework that dropped error_promise_value, the deferred path no longer reports the original error — it prints the "Expected a Response object" fetch diagnostic and calls render_production_error(500) directly, while the sync and already-settled twins still route through finish_running_error_handler → vm.on_unhandled_rejection(original_error). So the 204→500 status inconsistency is fixed, but the error-reporting half of #33137 is not, and the PR description ("After: error: boom", "asserts the original error is reported while the bogus … diagnostic is not") now describes the opposite of what the test asserts. At minimum the description needs updating; if matching the twins' diagnostics matters, the original error has to be stashed across the await again.
Extended reasoning...
What changed in the rework
Commit 6228d6f dropped the error_promise_value field and replaced the handle_resolve interception with a single check inside render_missing_invalid_response (lines 738–741): when is_error_promise_pending is set, clear it and call render_production_error(500). That correctly turns the 204 into a 500, but it never calls vm.on_unhandled_rejection with the user's thrown error — and it can't, because the original error was discarded when the Pending arm of process_on_error_promise returned.
How the three twins now diverge
Compare the three entry points for "error() produced no Response":
| Path | Code | Reports original error? |
|---|---|---|
Sync error() returns undefined |
run_error_handler_… falls through to finish_running_error_handler(value, 500) at line 3425 → (vm.on_unhandled_rejection)(vm, global_this, value) at line 3379 |
✅ error: boom |
Already-settled Promise.resolve(undefined) |
process_on_error_promise Fulfilled arm, line 3464–3465 → finish_running_error_handler(value, status) |
✅ error: boom |
Deferred pending Promise<undefined> |
Pending arm (3441–3450) drops value; later handle_resolve → render_missing_invalid_response prints the fetch diagnostic at 709–712, then 738–741 → render_production_error(500) |
❌ prints Expected a Response object, but received 'undefined' |
So after this PR, sync/settled print the user's actual error to stderr; deferred prints the misleading "your fetch handler returned the wrong thing" diagnostic for a value that came from error(), and the original Error("boom") is never surfaced.
Step-by-step on the PR's own repro
fetch() { throw new Error("boom"); },
async error(e) { await new Promise(r => setImmediate(r)); /* no return */ },fetchthrows →run_error_handler_with_status_code_dont_check_respondedcallserror(boom); result is a pending promise.process_on_error_promise→Pendingarm setsis_error_promise_pending, registersON_RESOLVE/ON_REJECT, and returns.value(theError("boom")) is not stored anywhere.setImmediatefires, promise fulfills withundefined→on_resolve→handle_resolve(undefined).undefinedis empty/null-ish →render_missing_invalid_response(undefined)printsExpected a Response object, but received 'undefined'(line 710).is_error_promise_pendingis set → clear it,render_production_error(500).on_unhandled_rejectionis never called.
stderr: Expected a Response object, but received 'undefined' — not error: boom.
The PR description and test are out of sync with this
The description's "What" section lists "never reports the original error" and "stderr instead prints fetch's invalid-return diagnostic" as the bug being fixed, the "After:" block shows error: boom, and the Verification section says the test "asserts the original error is reported while the bogus Expected a Response object diagnostic is not." All three describe the dropped field-based approach. The actual test now asserts expect(stderr).toContain("Expected a Response object") with the comment "The deferred 500s come with the invalid-return diagnostic" — the exact opposite.
Why existing code doesn't prevent it
finish_running_error_handler is the only place that calls (vm.on_unhandled_rejection)(…, original_error) for this flow (line 3379). The new branch bypasses it and goes straight to render_production_error, and there is no longer any field holding the original error to pass in even if it wanted to.
Impact and fix
The user-facing HTTP behavior (status 500, body Something went wrong!) is now consistent across all three shapes — that's the important part of #33137 and it's fixed. The remaining inconsistency is diagnostic-only: a developer whose async error() forgets to return a Response sees a confusing "Expected a Response object" instead of their real error, while the sync version of the same handler shows the real error.
If that trade-off is acceptable for the no-new-state constraint, the PR description and Verification section should be updated to say so (and drop the stale "After: error: boom" block). If the diagnostics should match the twins, the original error needs to be stashed across the await again — i.e. bring back something like error_promise_value (with the GC-rooting fix from 7595fb9) so handle_resolve can call finish_running_error_handler(original_error, 500) instead of falling through to the fetch diagnostic.
|
Description updated to match the field-free version. On the divergence both bots flag: dropping the field means the deferred path can no longer re-report the original thrown error, so it surfaces the accurate @Jarred-Sumner fine to keep it this way, or if you want the deferred path to report the thrown error like the twins I will retain it across the await. Your call. |
|
Stale PR review: keep open, rework. The fix is wanted. The bug is live on a 1.4.3 canary build (367d939). An The current diff still takes the wrong exit:
The wanted shape: when |
What
A
Bun.serveerror()handler that returns aPromisewhich resolves to a non-Responseafter the server's microtask drain (the common shape: anasync error()that awaits something and forgets toreturnaResponse) responded 204 with an empty body, while the synchronous and already-settled twins responded 500.Cause
RequestContext::process_on_error_promise'sPendingarm registers fetch's resolve callback. When theerror()promise later fulfills with a non-Response,handle_resolveroutes it torender_missing_invalid_response, which renders 204 (the fetch path's "handler returned nothing" behavior). TheFulfilledarm (already-settled) and the synchronous path both fall back tofinish_running_error_handler(..., 500).Fix
render_missing_invalid_responsenow checks the existingis_error_promise_pendingflag (already set in thePendingarm) and renders 500 when it is set, matching the status of the sync and already-settled twins. No new state is retained. The accompanying "Expected a Response object, but received 'undefined'" diagnostic is accurate for a deferrederror()that returned a non-Response.Verification
New subprocess tests in
serve.test.ts:undefinedandnull, plus a rejectedfetch()promise routed througherror(), all respond 500;error()that does return aResponsestill renders it (503);fetch()(noterror()) resolving to a non-Responsestill responds 204, pinning theis_error_promise_pendinggate so it cannot hijack fetch's own path.Fail-before: the deferred case responds
204with an empty body on the released binary; it passes at500with the fix.Fixes #33137.