socket/websocket: guard the error-handler dispatch against re-entering JS with a pending termination exception

[robobun]

What

test/js/node/test/parallel/test-http2-reset-flood.js started intermittently aborting on the asan lane with:

ASSERTION FAILED: (null)
!exception()
vendor/WebKit/Source/JavaScriptCore/runtime/ExceptionScope.h(61) : void JSC::ExceptionScope::assertNoException()

after #32488 landed (which changed the http2 teardown timing enough to surface this in CI; the underlying bug predates it).

Cause

When a Bun.listen / Bun.serve socket handler runs inside a worker and worker.terminate() fires while the handler is mid-call, the termination exception is raised at a JS safepoint inside the handler. The socket dispatch path (socket_body.rs on_close/on_data/etc., and ServerWebSocket.rs on_message/on_close/etc.) sees the handler return Err and follows up with the error handler:

if let Err(e) = callback.call(&global, this_value, &[...]) {
    let _ = handlers.call_error_handler(this_value, &[this_value, global.take_error(e)]);
}

take_error calls JSGlobalObject__tryTakeException, whose tryClearException() is a no-op for termination, so the exception stays pending on the VM. The error-handler dispatch then calls on_error.call(), which enters Interpreter::executeCallImpl:

auto scope = DECLARE_THROW_SCOPE(vm);
scope.assertNoException();  // aborts: termination is still pending

Found by instrumenting every Bun__JSValue__call entry with per-thread ring-buffer markers and stressing under contention; the last recorded callers were socket_body.rs:2058 (on_close's callback.call) followed directly by Handlers.rs:297 (on_error.call) with the exception already pending.

Fix

Add a has_exception() guard to the two error-handler dispatch functions that call JS after a preceding callback.call() -> Err:

• Handlers::call_error_handler (src/runtime/socket/Handlers.rs), reached from every Bun.listen/Bun.connect socket lifecycle handler's Err branch
• WebSocketServerContext::run_error_callback (src/runtime/server/WebSocketServerContext.rs), reached from every Bun.serve websocket handler's Err branch

This mirrors the existing guard in UDPSocket::call_error_handler (src/runtime/socket/udp_socket.rs:673) and EventLoop::run_callback (src/jsc/event_loop.rs:387). When the worker is terminating there is no point calling the error handler anyway; the VM is tearing down.

Verification

New test/js/bun/net/socket-handler-worker-terminate.test.ts reproduces the abort deterministically via Atomics coordination for both dispatch paths: a worker's handler signals the parent it is running, the parent terminates it, the handler spins on safepoints until termination lands. The test throws if the handler never fired or if terminate never landed inside it in any iteration, so a vacuous pass is impossible. Both variants fail on main with the exact assertion above (exit 134), and both pass with this patch.

Under the original stress harness (8 parallel runs of test-http2-reset-flood.js), main fails within ~10-300 rounds; with this patch no failure in 3200+ rounds.

---

no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/net/socket-handler-worker-terminate.test.ts

[robobun]

^{Updated 9:49 PM PT - Jul 20th, 2026}

✅ @Jarred-Sumner, your commit 69f0a7501d91881674539d7a0be022a159ceeeb0 passed in Build #76575! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 34414

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

bun-34414 --bun

[claude]

LGTM — small guard mirroring the established has_exception() pattern from EventLoop::run_callback.

What was reviewed:

• Verified the guard matches event_loop.rs:387/:422 and the analogous is_termination_exception() check in UDPSocket::call_error_handler (udp_socket.rs:673).
• Checked all call_error_handler callers in socket_body.rs — non-termination paths clear the exception via take_error before reaching here, so the new guard only short-circuits the sticky-termination case.
• Test uses Atomics-coordinated terminate-mid-handler, spawned subprocess, drains pipes concurrently, asserts on {stdout, exitCode}.

Extended reasoning...

Overview

This PR adds an 8-line guard (mostly comment) to Handlers::call_error_handler in src/runtime/socket/Handlers.rs: if global_object.has_exception() is true on entry, return false instead of re-entering JS. It also adds a regression test at test/js/bun/net/socket-handler-worker-terminate.test.ts that reproduces the SIGABRT from test-http2-reset-flood.js by terminating a worker while its Bun.listen close handler is on-stack.

Security risks

None. This is a defensive early-return that prevents entering the JS interpreter with a pending (unclearable) termination exception. No new user-controlled inputs, no auth/crypto/permission surface.

Level of scrutiny

Low-to-moderate. The change is a single conditional early-return following an already-established pattern in the codebase. I confirmed:

• EventLoop::run_callback (src/jsc/event_loop.rs:387) and run_callback_with_result (:422) have byte-identical if global_object.has_exception() { return } guards with the same rationale comment about executeCallImpl's assertNoException.
• UDPSocket::call_error_handler (src/runtime/socket/udp_socket.rs:673) has the equivalent if err.is_termination_exception() { return } check.
• Audited the ~12 callers of Handlers::call_error_handler in socket_body.rs. Callers that reach here after callback.call() -> Err pass global.take_error(err) as args[1]; for ordinary thrown exceptions take_error clears the VM exception, so has_exception() is false and behavior is unchanged. For JsError::Terminated, the termination exception is sticky (JSC does not clear it via clearException), so has_exception() remains true and the new guard correctly bails. Callers that synthesize a fresh error value (e.g. SystemError.to_js) have no pending exception and are unaffected.

Other factors

The test is well-constructed per repo conventions: spawned subprocess with bunExe()/bunEnv, concurrent Promise.all pipe draining, asserts on a combined {stdout, stderr, exitCode} object rather than exact stderr, uses SharedArrayBuffer + Atomics.wait/notify to deterministically land terminate() inside the handler instead of sleep-and-hope, bounded spin loop so a missed terminate cannot hang, and test.skipIf(isWindows) with a stated reason. The 60s timeout is generous but reasonable for 8 worker spawn/terminate iterations under debug+ASAN. The comment in the source change is 5 lines (repo guideline says 3 max) but it names the exact assert and cross-references both precedents, which is the kind of durable non-obvious content the guidelines carve out for.

[claude]

No bugs found, but deferring to a human: this adds a pending-exception early-return to Bun__JSValue__call, the chokepoint every Rust JSValue::call() flows through, and the caller-site → chokepoint move is a design call worth a maintainer's eyes.

What was reviewed:

• Return contract: return {} with a pending exception satisfies call_zero_is_throw's zero-iff-thrown invariant, so Rust callers get the same Err(JsError::Thrown) they already handle.
• Checked that the guard doesn't conflict with the Rust-side ExceptionValidationScope opened in from_js_host_call before the FFI call.
• Test uses SharedArrayBuffer/Atomics to deterministically land terminate() mid-handler and asserts its own precondition (throws on 0/8 hits), so it can't pass vacuously.

Extended reasoning...

Overview

The PR fixes a SIGABRT that surfaces when worker.terminate() fires while a Bun.listen socket handler is executing inside the worker. The termination exception raised at a JS safepoint cannot be cleared (tryClearException is a no-op for termination), so when the socket dispatch path routes the resulting Err to the error handler and calls back into JS, Interpreter::executeCallImpl's assertNoException aborts. Originally seen as an intermittent SIGABRT in test-http2-reset-flood.js on the ASAN lane.

Two commits: the first (c7c2bad) added a has_exception() gate in Handlers::call_error_handler (mirroring existing guards in UDPSocket::call_error_handler and EventLoop::run_callback); the second (f0a5758) reverted that and moved the guard down into Bun__JSValue__call in bindings.cpp — the single FFI entry point every Rust JSValue::call() uses — on the grounds that the same call→Err→secondary-call shape exists in WebSocketServerContext::run_error_callback, RequestContext's error handler, and the on_open→mark_inactive→on_close chain.

Security risks

None. This is worker-termination / exception-propagation plumbing with no auth, crypto, or untrusted-input parsing involved.

Level of scrutiny

High. Bun__JSValue__call is one of the hottest and most widely-reached functions in the runtime — every Rust-side JS callback invocation goes through it. The change itself is two lines plus a comment and an [[unlikely]] hint (a single vm.m_exception load), so perf impact is negligible, but the placement is a design decision:

• Guarding at the chokepoint covers every current and future call → Err → secondary call site in one place, which matches "fix at the layer that owns the invariant" (executeCallImpl is what enforces no-pending-exception).
• On the other hand, it broadens the guard from termination-only to any pending exception. A caller that reaches JSValue::call with a pending non-termination exception is a caller bug that executeCallImpl's debug assertion would previously have surfaced loudly; now it becomes a quiet Err. The existing per-caller guards (UDPSocket, EventLoop::run_callback) chose the narrower placement, and the first commit here did too before the follow-up moved it.

I verified the mechanics are sound — return {} with the exception still pending is exactly the zero_is_throw contract that from_js_host_call → call_zero_is_throw_at expects (src/jsc/TopExceptionScope.rs:628), and assert_exception_presence_matches(true) passes — so there's no correctness bug. But whether the chokepoint is the right layer vs. per-caller (or a termination-only check like vm.hasPendingTerminationException()) is a call I'd rather a maintainer make.

Other factors

The regression test is well-constructed: it uses a SharedArrayBuffer + Atomics.wait/notify handshake so the parent terminates the worker precisely while the close() handler is on-stack, spins on allocation safepoints (bounded, no timers), asserts its own precondition (throws if close() never fired or if 0/8 iterations landed mid-handler), drains stdout/stderr/exited concurrently, and skips on Windows with a stated reason. It follows the harness conventions in CLAUDE.md closely.

[robobun]

Narrowed the guard to termination-only (via assertNoExceptionExceptTermination() inside the branch): a non-termination exception pending at this point is a caller bug the assert should still surface, same as before; termination is the one case a caller cannot avoid because tryClearException is a no-op for it. This keeps the behaviour change scoped to exactly the case the test reproduces.

[claude]

LGTM — the termination-only narrowing addresses my prior concern; non-termination pending exceptions still assert loudly in debug via assertNoExceptionExceptTermination(), and the release-build path (early Err instead of entering executeCallImpl with a pending exception) is strictly safer than before.

Extended reasoning...

Overview

Follow-up commit 6884195 on top of what I last reviewed at f0a5758. The guard in Bun__JSValue__call (bindings.cpp:2806) now calls scope.assertNoExceptionExceptTermination() inside the if (scope.exception()) branch before returning {}. Net diff vs main is 4 lines of code + 7 lines of comment in bindings.cpp, plus the regression test (unchanged since my last review).

Security risks

None — worker-termination / exception-propagation plumbing, no auth/crypto/untrusted-input surface.

Level of scrutiny

My previous deferral raised two points. The substantive one — that guarding on any pending exception would quietly convert caller bugs into Err instead of surfacing them via executeCallImpl's debug assert — is resolved: assertNoExceptionExceptTermination() (an established pattern with ~20 existing uses in the bindings) preserves the debug-build abort for non-termination exceptions, so caller-bug diagnostics are unchanged. In release builds the assert compiles out and any pending exception returns {}, which is strictly safer than the prior behavior of proceeding into JS with a pending exception. The remaining chokepoint-vs-per-caller placement question is answered by the author's rationale (fix at the layer that owns the invariant; covers WebSocketServerContext / RequestContext / on_open→on_close sibling shapes) and is now low-stakes given the guard only alters behavior for the one case — termination — that a well-behaved caller cannot avoid.

I re-confirmed the return contract: return {} with the (still-pending) termination exception satisfies call_zero_is_throw_at's zero-iff-thrown invariant (TopExceptionScope.rs:628), so Rust callers see Err(JsError::Thrown) exactly as they already handle.

Other factors

Test unchanged since last review and still well-constructed (Atomics-coordinated terminate-mid-handler, asserts its own precondition, concurrent pipe drain, Windows skip with reason). No bugs from the bug-hunt pass. No human reviews or outstanding line comments on the thread.

[coderabbitai]

Walkthrough

Changes

Worker termination can leave a pending JavaScript exception during socket and WebSocket error handling. Both callback paths now return before invoking on_error, and new repeated worker tests cover socket-close and WebSocket-message termination scenarios.

Termination exception handling

Layer / File(s)	Summary
Callback exception guards src/runtime/server/WebSocketServerContext.rs, src/runtime/socket/Handlers.rs	Error callbacks return early when global_object already has a pending exception.
Worker termination regression tests test/js/bun/net/socket-handler-worker-terminate.test.ts	Repeated worker tests terminate active Bun.listen and Bun.serve handlers, then validate interruption counts and successful process exits.

Possibly related PRs

• oven-sh/bun#34346: Also changes WebSocket error-callback execution to avoid problematic re-entry during error handling.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title accurately summarizes the main fix: guarding error-handler dispatch against re-entering JS with a pending termination exception.
Description check	✅ Passed	It covers the bug, fix, and verification, though the headings don't match the template exactly.

---

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

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@test/js/bun/net/socket-handler-worker-terminate.test.ts`:
- Around line 1-180: Move the Bun.listen close-handler regression scenario from
this standalone file into the existing socket test file, and move the Bun.serve
WebSocket message-handler scenario into the existing WebSocket test file.
Preserve each scenario’s worker setup, termination assertions, platform skip
behavior, and test names; remove this standalone regression file.
- Around line 157-177: Remove the explicit 60_000 timeout argument from the test
declaration surrounding the Bun.spawn assertion, allowing the runner’s
build-aware timeout to apply. Keep the test name, async callback, process
handling, and expectations unchanged.

🪄 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: c7c42ddf-3bad-4d7a-bb40-3a661849a765

📥 Commits

Reviewing files that changed from the base of the PR and between a85068c and 6962a78.

📒 Files selected for processing (3)

• src/runtime/server/WebSocketServerContext.rs
• src/runtime/socket/Handlers.rs
• test/js/bun/net/socket-handler-worker-terminate.test.ts

[robobun]

CI on 69f0a75: all four test variants pass on every lane; every red is [flaky] (passed on retry). Diff is green and ready to merge.

[claude]

Beyond the two test-convention nits inline: I checked all ~20 callers of call_error_handler / run_error_callback in socket_body.rs and ServerWebSocket.rs — each does take_error/take_exception (or passes a synthesized error) before the call, so has_exception() being true at the guard can only mean termination; the broad check is functionally termination-only here and matches the event_loop.rs:387 precedent exactly. The return false on the new early-return path is safe — every caller discards the result with let _ =.

Extended reasoning...

The native change is small and follows the identical existing guard in EventLoop::run_callback (src/jsc/event_loop.rs:387), and the UDPSocket variant at udp_socket.rs:673 (which checks err.is_termination_exception() on the value rather than VM state — a slightly different but equivalent gate given callers always take_exception first). I traced every call site of both patched functions to confirm none can reach them with a non-termination exception still pending, so the broad has_exception() check does not swallow any error a narrower termination-only check wouldn't. The false return from Handlers::call_error_handler on the new path is inert (all callers use let _ =). Not approving because this is JSC exception-handling in a runtime-critical dispatch path and the PR iterated through several guard placements/scopings — a maintainer should ratify the final choice — but the core fix looks correct.

[claude]

No issues found, but I'd like a human to confirm the guard shape here — the fix touches JSC exception-handling at the socket/websocket error-dispatch boundary, and the design iterated during the PR (chokepoint → termination-only → call-site has_exception()).

Checked: the new has_exception() guard vs. the cited precedents — event_loop.rs:387 matches exactly; udp_socket.rs:673 uses err.is_termination_exception() on the value instead, which is narrower. Traced all ~20 callers of call_error_handler/run_error_callback in socket_body.rs and ServerWebSocket.rs — each either goes through take_error/take_exception first or builds the error value fresh, so a pending exception at the guard should only be termination. Test harness looks solid: Atomics coordination guarantees the handler fired and terminate landed mid-frame, so a vacuous pass is ruled out.

Extended reasoning...

Overview

The PR adds a global_object.has_exception() early-return to two error-handler dispatch functions — Handlers::call_error_handler (src/runtime/socket/Handlers.rs) and WebSocketServerContext::run_error_callback (src/runtime/server/WebSocketServerContext.rs) — plus a new deterministic worker-terminate regression test. Total production change: 10 lines across 2 files. It fixes an intermittent assertNoException() abort when worker.terminate() lands mid-handler and the dispatch path then tries to invoke the user's error callback with the (unclearable) termination exception still pending on the VM.

Security risks

None. This is internal JSC exception-scope hygiene during VM teardown; no user-controlled input, auth, or data-exposure surface is touched.

Level of scrutiny

Moderate-to-high. The change is small and additive (strictly one more early return), but it sits in the JSC exception-handling path — the category CLAUDE.md flags as most-blocked. The PR's own commit history shows the design was non-obvious: it started at the Bun__JSValue__call chokepoint, was narrowed to termination-only there, then moved to the two call sites with a broad has_exception() check. I believe the broad check is correct at the call sites (every caller has already run take_error/take_exception, which clears non-termination exceptions, so has_exception() here effectively means termination), but the cited UDPSocket precedent uses the narrower err.is_termination_exception() check on the value rather than the global — a maintainer should confirm the broader form is intentional and preferred here.

Other factors

• The test is well-constructed: Atomics-coordinated, throws on 0/8 hits so it cannot vacuously pass, spawned in a subprocess so the abort is observable, both dispatch paths covered. My prior nit about test.concurrent was applied in 86f2349; the 60_000 timeout was retained with reasonable justification (worker-spawn cost floor under debug+ASAN, matching neighbouring socket.test.ts precedent).
• CI on 6962a78 passed the new test on every lane; the one non-flaky red is a pre-existing unrelated LSAN leak.
• ~20 call sites feed into these two functions across socket_body.rs and ServerWebSocket.rs; the guard changes behaviour for all of them. I traced them and each either clears the exception first or has no prior JS call, so I don't see a false-positive path — but that's the kind of enumeration a maintainer familiar with this code should sanity-check.

[claude]

No bugs found, but this touches JSC exception-scope handling on the worker-termination path and the fix location iterated a few times (Bun__JSValue__call chokepoint → termination-only → per-call-site), so a human should confirm the final placement and predicate.

What was reviewed:

• The two has_exception() guards match the established pattern at src/jsc/event_loop.rs:387/:422; checked that the fallback uncaught_exception path being skipped is only reachable for termination (normal exceptions are already cleared by take_error before these functions run).
• Compared against the UDP precedent at udp_socket.rs:673, which uses err.is_termination_exception() instead — functionally equivalent here since only termination survives tryClearException, but worth a maintainer's eye on which predicate is preferred.
• Test: Atomics two-phase handshake makes the window build-speed-independent; hits === 0 throw prevents a vacuous pass; both prior inline nits are resolved (test.concurrent applied, 60_000 timeout kept with cited neighboring precedent).

Extended reasoning...

Overview

Two 5-line defensive guards adding if global_object.has_exception() { return } at the top of the socket and websocket error-handler dispatch functions (Handlers::call_error_handler, WebSocketServerContext::run_error_callback), plus a 152-line deterministic regression test. The guards prevent re-entering Interpreter::executeCallImpl while a termination exception is pending on the VM, which trips a RELEASE_ASSERT and SIGABRTs.

Security risks

None. The change only adds early returns on a path that would otherwise abort the process; no new inputs are accepted, no data is exposed, no auth/crypto touched.

Level of scrutiny

Medium-high. The native change is tiny and purely defensive — it cannot introduce a new failure mode, since entering JS with a pending exception aborts anyway. It follows the exact precedent at event_loop.rs:387 (run_callback) with matching comment. However, this is JSC exception-scope handling on a concurrency-sensitive teardown path, and the commit history shows the fix location was iterated on three times (guard in Bun__JSValue__call → narrowed to termination-only there → moved to the two call sites). That iteration reflects real design judgment about where the guard belongs and how narrow the predicate should be, which a maintainer should ratify.

One specific point worth a human's eye: the cited UDP precedent (udp_socket.rs:673) uses err.is_termination_exception() on the error value, whereas this PR and event_loop.rs use global_object.has_exception() on VM state. At these call sites they're equivalent in practice (callers invoke take_error first, which clears everything except termination), but the divergence is worth noting.

Other factors

• Bug hunter found nothing.
• CI on 6962a78 passed the new test on every lane; the one non-flaky red is a pre-existing unrelated LSAN leak.
• All four inline review threads (2 CodeRabbit, 2 mine) are resolved: test.concurrent was applied; the standalone test file and 60_000 timeout were kept with cited precedent from neighboring files, which both reviewers accepted.
• The test has explicit vacuous-pass protection (if (hits === 0) throw) and the two-phase Atomics handshake in 5cb30c7 makes the terminate-lands-mid-handler window independent of build speed.

[claude]

The on_open→on_close re-entry gap I flagged is now covered (guards in ServerWebSocket::on_close and socket_body.rs::on_close, plus two new test variants). Deferring to a human for the layer question the author left open — whether has_exception() at each dispatch entry point is the right shape vs. a shared chokepoint — and because these are hot native JSC dispatch paths.

What was reviewed:

• The four new has_exception() guards mirror the existing pattern in UDPSocket::call_error_handler / EventLoop::run_callback; each early-return preserves the surrounding cleanup (drop(cleanup) in socket_body, scopeguard-deferred signal/downgrade in ServerWebSocket::on_close).
• The four test scenarios each throw on hits === 0, so a run where terminate never lands mid-handler cannot pass silently.
• Checked that the broader has_exception() (vs. the termination-only variant an earlier revision used) does not swallow ordinary user exceptions on the happy path — those are cleared by take_exception before this guard is reached; only termination survives to trip it.

Extended reasoning...

Overview

Four has_exception() guards added ahead of JS re-entry after a preceding callback.call() -> Err: two on the error-handler dispatch (Handlers::call_error_handler, WebSocketServerContext::run_error_callback) and two on the close-handler dispatch (ServerWebSocket::on_close, socket_body.rs::on_close). The latter pair addresses my prior finding that on_open's error branch synchronously re-enters on_close before the error-handler guard is reached. A new 4-scenario test file drives each path deterministically via a two-phase Atomics handshake between a parent and a worker.

Security risks

None. This is a debug-assertion abort during worker teardown; no user-controlled input reaches the changed code.

Level of scrutiny

High. These are the socket/websocket lifecycle dispatch paths — every Bun.listen/Bun.connect connection and every Bun.serve websocket routes through them, and the change interacts with JSC's exception scope machinery and worker termination timing. The author explicitly asked for maintainer direction on placement ("Waiting on direction about which layer the has_exception() guard belongs at"; commit 9dda1f1 shows a chokepoint approach was tried and reverted), and an earlier revision narrowed the check to termination-only via assertNoExceptionExceptTermination() before broadening back to has_exception(). Those are design decisions a maintainer should sign off on.

Other factors

• The unresolved inline thread on WebSocketServerContext.rs:92 is addressed by 69f0a75; the author's reply left it open pending layer direction rather than because the fix is incomplete.
• Test-convention nits (test.concurrent, per-test timeout, standalone file, error-event wiring) were all discussed and either applied or declined with stated rationale; nothing outstanding there.
• The openWorkerSource variant uses net.createServer (which routes through the same socket_body.rs NewSocket dispatch) rather than Bun.listen, so it does exercise the intended on_open -> mark_inactive -> on_close path.