Bun.serve: don't finalize the request again when error() upgraded or ended it

[Jarred-Sumner]

What

server.upgrade(req) called from inside Bun.serve's error(err) handler (after fetch threw for a WebSocket-upgrade request) completed the upgrade — which releases the RequestContext's base ref and detaches resp — and then the synchronous error-handler path fell through into finish_running_error_handler → (dev mode) render_default_error, whose resp == None arm finalizes and derefs again: assertion failed: ref_count > 0 on debug/assert builds, a refcount underflow on release. The async error-handler path already returned early on did_upgrade_web_socket(); only the sync path was missing it.

After calling on_error, run_error_handler_with_status_code_dont_check_responded now returns if is_aborted_or_ended() || did_upgrade_web_socket() — the same guard every sibling site uses after re-entering user JS (handle_resolve, on_response, on_upgrade) — which also covers error() handlers that end the request another way (e.g. server.stop(true)). The now-redundant check in process_on_error_promise's Fulfilled arm is removed. Semantics: upgrading from error() works (101 + open fires), matching what the async path already did.

Repro (before)

let server = Bun.serve({ port: 0, development: true,
  fetch(req) { throw Object.assign(new Error("boom"), { req }); },
  error(err) { if (err.req && server.upgrade(err.req, { data: {} })) return; return new Response("err", { status: 500 }); },
  websocket: { open(ws) { ws.send("opened"); }, message(ws, m) { ws.send(m); } } });
const ws = new WebSocket(`ws://127.0.0.1:${server.port}/`); ws.onmessage = e => console.log(e.data);
// debug/ASan+assert: panic: assertion failed: ref_count > 0 (RequestContext.rs deref ← render_default_error)

Tests

test/js/bun/websocket/websocket-server.test.ts — "server.upgrade() from the error() handler after fetch() threw completes the handshake". 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: 59 minutes

Limit details: You’ve used all 1 included review currently available under your plan. You completed 72 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: b0906be7-42a6-4624-8381-a35884d7e628

📥 Commits

Reviewing files that changed from the base of the PR and between 771c7e6 and f689319.

📒 Files selected for processing (2)

• src/runtime/server/RequestContext.rs
• test/js/bun/websocket/websocket-server.test.ts

---

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

[robobun]

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

✅ @Jarred-Sumner, your commit f689319d2121a112f3bdc94a98986ea0dc1f32ae passed in Build #99854! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 39409

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

bun-39409 --bun

[claude]

LGTM — the new guard mirrors the exact is_aborted_or_ended() || did_upgrade_web_socket() check that handle_resolve and on_response already apply after re-entering user JS, and the removed Fulfilled-arm check is provably dead behind it.

What was reviewed

• Traced the double-deref: upgrade nulls resp, then render_default_error's resp == None arm hits finalize_without_deinit(); deref() on an already-released ctx — the new early return cuts that path.
• Confirmed the removed check in process_on_error_promise is redundant: its only caller is now guarded, and promise.unwrap(.., MarkHandled) runs no user JS between the new guard and the old check.
• Verified the async path (Pending → ON_RESOLVE → handle_resolve) already has the same guard, so the sync/async paths now agree.
• Test spawns a subprocess with development: true and asserts the full 101 → open → echo → close-1000 sequence, so it fails on the unfixed assert-crash rather than passing vacuously.

Extended reasoning...

Overview

Five-line guard added to run_error_handler_with_status_code_dont_check_responded in src/runtime/server/RequestContext.rs, returning early if the request was aborted/ended or upgraded to a WebSocket during the synchronous error() callback. An eight-line now-redundant did_upgrade_web_socket() check is removed from process_on_error_promise's Fulfilled arm. One subprocess test added to test/js/bun/websocket/websocket-server.test.ts.

Security risks

None. This is a lifecycle/refcount fix — the added guard only prevents a second finalize+deref on a context whose base ref was already released by server.upgrade(). No new user-controlled input is parsed and no auth/crypto/permissions surface is touched.

Level of scrutiny

High, because this is refcount-sensitive native code in the HTTP server request lifecycle. I traced both directions:

• Can the new early return leak? No — the paths that set Upgraded or clear resp (upgrade, abort, server.stop(true)) each already release the context's base ref; returning here just stops the second release.
• Is the removed check truly dead? Yes — process_on_error_promise is called only at RequestContext.rs:3510, immediately after the new guard, and promise.unwrap() on an already-fulfilled promise reads state without invoking user JS, so did_upgrade_web_socket() cannot flip between the two points. The Pending arm still routes through handle_resolve which retains its own guard (line 730).

The change is not novel logic; it copies verbatim the guard shape from handle_resolve (line 730), on_response (lines 2563/2624), and the abort check at 998, which is exactly what REVIEW.md's "re-validate liveness guards after every callback" rule prescribes.

Other factors

• No CODEOWNERS entry covers src/runtime/server/.
• Test is placed in the existing suite file (not a new file), uses it.concurrent, spawns via bunExe() -e with bunEnv, drains stdout concurrently with exited, and asserts exact stdout before exit code — matches harness conventions. development: true is set so the crashing render_default_error path is exercised.
• Bug-hunting system found nothing.