Skip to content

worker: guard the remaining NewSocket / ServerWebSocket / Bun.serve / Bun.$ dispatch sites against a pending termination - #36808

Open
robobun wants to merge 9 commits into
mainfrom
farm/a8e86549/worker-terminate-guards-round-2
Open

worker: guard the remaining NewSocket / ServerWebSocket / Bun.serve / Bun.$ dispatch sites against a pending termination#36808
robobun wants to merge 9 commits into
mainfrom
farm/a8e86549/worker-terminate-guards-round-2

Conversation

@robobun

@robobun robobun commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #36579. That PR upgraded the multi-JS-entry NewSocket callbacks and several C++ emit paths to the worker-aware script_execution_status() != Running gate; this one covers the remaining dispatch sites the fuzz ledger has since hit with the same assertNoException() / empty-argument aborts.

NewSocket remaining callbacks (socket_body.rs)

on_writable / on_data / on_end / on_timeout / on_close / handle_error / the ALPN selector still gated on is_shutting_down(). A single uSockets poll sweep can dispatch several callbacks back to back (e.g. us_internal_ssl_on_dataon_handshakessl_retry_parked_writeon_writable, or a burst of FINs → N× on_end), so a terminate() trap raised inside one socket's handler leaves the TerminationException pending for the next socket's dispatch in the same tick:

ASSERTION FAILED: !exception()
ExceptionScope.h(61) : void JSC::ExceptionScope::assertNoException()
  <- Interpreter::executeCallImpl <- JSC::call <- Bun__JSValue__call
  <- NewSocket::on_end<false> (callback.call of the JS `end` handler)
  <- us_internal_dispatch_ready_poll (allow_half_open EOF branch)
  <- us_loop_run_bun_tick <- auto_tick_active <- WebWorker::spin

Unify every remaining is_shutting_down() gate in the NewSocket dispatch table on the predicate the rest of #36579 uses.

ServerWebSocket (ServerWebSocket.rs)

on_open / on_message / on_drain / on_ping / on_pong / on_close had the same is_shutting_down() gate, and on_message / on_ping / on_pong swallowed a failed payload-buffer creation into .unwrap_or(JSValue::ZERO), which Bun__JSValue__call asserts on:

ASSERTION FAILED: arguments[1] is JSValue.zero. This will cause a crash.
!JSValue::decode(arguments[i]).isEmpty()   bindings.cpp(2901) : Bun__JSValue__call
  <- ServerWebSocket::on_ping (callback.call of the JS `ping` handler)
  <- uWS::WebSocketContext::handleFragment (PING) <- ... <- WebWorker::spin

Upgrade the gates and early-return when the payload conversion fails.

Bun.serve (mod.rs / RequestContext.rs)

on_request's fetch-handler call can take the termination trap; on_response then routes the pending TerminationException into the user's error() handler at run_error_handler_with_status_code_dont_check_responded, tripping assertNoException(). Gate both the on_request entry (respond 503 like the stale-wrapper path) and the error-handler dispatch.

Bun.$ (shell/interpreter.rs)

Interpreter::finish builds the stdout/stderr Buffers (DECLARE_TOP_EXCEPTION_SCOPE in JSBuffer__bufferFromPointerAndLengthAndDeinit) then resolve.call() with no gate; on a terminating worker, drop the keepalive and root-io and skip the JS resolve.

Verification

One new skipIf(!isDebug) test (release WebKit compiles these asserts out). On a debug build, without the src/ changes:

(fail) terminate() while a worker's Bun.listen end handler is firing ...  stderr: "ASSERTION FAILED: !exception() ... ExceptionScope.h(61)"

With the src/ changes it passes (~33s on debug+ASAN). The on_writable<true> (node:tls), ServerWebSocket::on_ping, Bun.serve error(), and Bun.$ sites each reproduce on a release-asan build (see the commit messages); they use the same script_execution_status() gate proven by the on_end test and by #36579's three tests, and their repros are heavy (self-signed TLS, raw WS PING frames, cat/head pipelines) for a committed test.


no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/workers/worker-terminate-lifetime.test.ts

robobun added 2 commits August 3, 2026 05:10
…ution_status()

#36579 upgraded the multi-JS-entry callbacks (on_open/keylog/session/
handshake/handle_connect_error) from is_shutting_down() to the
worker-aware script_execution_status() gate, leaving the single-entry
siblings (on_writable/on_data/on_end/on_timeout/on_close/handle_error/
ALPN) on the weaker predicate. That split is insufficient: uSockets can
dispatch several callbacks in one poll sweep, and a terminate() trap
raised inside the first socket's handler leaves the TerminationException
pending for the next socket's dispatch in the same tick, tripping
Interpreter::executeCallImpl's scope.assertNoException().

Reproduces via on_end on plain TCP (worker Bun.listen with allowHalfOpen,
parent FINs the whole batch then terminates) and via on_writable<true>
through ssl_retry_parked_write under node:tls. Unify every remaining
is_shutting_down() gate in the NewSocket dispatch table on the same
predicate the rest of #36579 uses.
…sites against a pending worker termination

ServerWebSocket on_open/on_message/on_drain/on_ping/on_pong/on_close gated
on is_shutting_down() and on_message/on_ping/on_pong swallowed a failed
payload-buffer creation into JSValue::ZERO, which Bun__JSValue__call
asserts on ("arguments[i] is JSValue.zero"). Upgrade the gates and
early-return when the payload conversion fails.

Bun.serve: on_request's fetch-handler call can take the termination trap;
on_response then routes the pending TerminationException into the user's
error() handler at run_error_handler_with_status_code_dont_check_responded,
tripping assertNoException(). Gate both the on_request entry (respond 503,
like the stale-wrapper path) and the error-handler dispatch.

Bun.$: Interpreter::finish builds the stdout/stderr Buffers
(DECLARE_TOP_EXCEPTION_SCOPE in JSBuffer__bufferFromPointerAndLengthAndDeinit)
then resolve.call() with no gate; on a terminating worker, drop the
keepalive and root-io and skip the JS resolve.
@robobun

robobun commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

Status: diff is green. The x64-asan lane in build 88022 is done and the new on_end test passes. The remaining red there is unrelated: test-worker-message-port-transfer-terminate.js is the pre-existing #34095/#34690 MessagePort assertion; bun-add.test.ts "Git URL in dependencies (SCP-style)" is a git-clone network timeout (flaky on four other lanes in the same build, install code untouched here; reported to main-break triage); the rest are single-lane flakes. All review findings addressed or withdrawn; final bot review found no issues and deferred to a maintainer. Ready for review.

Scope (follow-up to #36579):

  • NewSocket dispatch table (all remaining gates), Handlers::{resolve,reject}_promise/call_error_handler, the two SNI server_name callbacks, UpgradedDuplex::call_write_or_end (+ between-probe has_exception() bail), and the Windows named-pipe accept path
  • ServerWebSocket callback table plus the unwrap_or(JSValue::ZERO) → early-return fixes
  • Bun.serve: js_value_for_dispatch() (every on_request-family entry incl. node-http / ws-upgrade id==0 / on_saved_request both arms / client-error / connection), a belt-and-braces check in prepare_js_request_context for the bake save path, and run_error_handler_* (entry gate + has_exception() bail after the user error() call)
  • Bun.$ Interpreter::finish

Between-entry bails use has_exception() (the condition assertNoException() tests); entry-of-function gates use script_execution_status() (the worker-aware predicate). Handlers::mark_inactive intentionally stays on is_shutting_down() (no JS entry). The pre-existing SavedRequest::deinit()-has-no-caller issue surfaced by review is handed off separately.

Fail-before (debug build): git checkout origin/main -- src/ && bun bd test test/js/web/workers/worker-terminate-lifetime.test.ts -t "Bun.listen end handler" shows the ASSERTION FAILED: !exception() stderr line; pass-after with src/ restored. Sanity: websocket-server.test.ts 115/115, bunshell.test.ts 418/418, serve.test.ts 280/284 (4 pre-existing environment fails on main).

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

The PR gates server, WebSocket, socket, listener, promise, duplex, and shell callbacks on Running script execution. It also stops error-handler processing after pending exceptions and adds worker-termination regression coverage.

Runtime termination guards

Layer / File(s) Summary
Server and WebSocket callback guards
src/runtime/server/RequestContext.rs, src/runtime/server/ServerWebSocket.rs, src/runtime/server/mod.rs
Server dispatch, error handlers, and WebSocket callbacks now require Running script execution. Payload conversion failures and pending exceptions stop further processing.
Socket callback dispatch guards
src/runtime/socket/*, test/js/web/workers/worker-terminate-lifetime.test.ts
Socket callbacks, promise handlers, listener callbacks, and duplex writes now stop when execution is not Running. The regression test covers concurrent TCP shutdown callbacks during worker termination.
Shell completion guard
src/runtime/shell/interpreter.rs
finish now performs cleanup and disables keep-alive without resolving the promise when execution is not Running.

Possibly related PRs

  • oven-sh/bun#36331 — Both PRs guard JavaScript callback execution during worker termination.
  • oven-sh/bun#36342 — Both PRs modify worker-termination handling in RequestContext.rs and lifetime regression coverage.
  • oven-sh/bun#36806 — Both PRs modify worker-termination handling and the worker lifetime test.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: guarding remaining dispatch sites against pending worker termination.
Description check ✅ Passed The description explains the changes and verification results, including the debug regression test and known environment-related failures.
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.

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

@github-actions github-actions Bot added the claude label Aug 3, 2026
Comment thread src/runtime/socket/socket_body.rs
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Found 6 issues this PR may fix:

  1. ASAN CI: ExceptionScope::assertNoException during worker terminate (worker-transfer-terminate-stress, separate from #34095) #34690 - ASAN CI assertNoException assertion failure during worker terminate stress tests — exactly the exception-scope violation this PR guards against
  2. ASAN CI: JSC assertion in JSObject::getOwnPropertyDescriptor during worker terminate (test-worker-message-port-transfer-terminate) #34095 - ASAN CI JSC assertion (!scope.exception() || !result) during worker terminate/transfer — pending exception during callback dispatch
  3. panic: Segmentation fault at address 0xD — "multiple threads are crashing" under Worker spawn/terminate churn (1.3.14, long-running server) #31880 - Segfault in long-running Bun.serve() with Worker pool under spawn/terminate churn — PR guards on_request to return 503 when terminating
  4. Worker create+terminate cycle aborts process after ~100k–900k iterations on macOS arm64 #30421 - Worker create+terminate cycle aborts after ~100k–900k iterations on macOS arm64 — callbacks dispatched into terminating VM
  5. Windows worker stdout/pipe write completion crash during shutdown #31224 - Windows worker stdout/pipe write completion crash during shutdown — same is_shutting_down() insufficiency this PR replaces
  6. Segfault in Bun.serve() .stop() during shutdown after a long-lived server (~8h), faulting address is ASCII text #36788 - Segfault in Bun.serve().stop() during shutdown after long-lived server — PR guards serve dispatch during teardown

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #34690
Fixes #34095
Fixes #31880
Fixes #30421
Fixes #31224
Fixes #36788

🤖 Generated with Claude Code

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/runtime/server/mod.rs (1)

1072-1091: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Guard every remaining JS dispatch path against terminated scripts.

on_saved_request, on_user_route_request, server_body::on_request_for, server_body::on_user_route_request_for, and upgrade_web_socket_user_route can dispatch into JS without checking script_execution_status(). The route dispatcher calls scope.assertNoException() before invoking the handler, so a pending TerminationException can abort the process. Return 503 with respond_stopped_503 before preparing the request or dispatching into JS on each path.

🤖 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/mod.rs` around lines 1072 - 1091, Guard every listed
dispatch path—on_saved_request, on_user_route_request,
server_body::on_request_for, server_body::on_user_route_request_for, and
upgrade_web_socket_user_route—with a script_execution_status() check before
request preparation or any JS dispatch. When the status is not Running,
immediately call server_body::respond_stopped_503 and return, matching the
existing on_request guard behavior.
🤖 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.

Outside diff comments:
In `@src/runtime/server/mod.rs`:
- Around line 1072-1091: Guard every listed dispatch path—on_saved_request,
on_user_route_request, server_body::on_request_for,
server_body::on_user_route_request_for, and upgrade_web_socket_user_route—with a
script_execution_status() check before request preparation or any JS dispatch.
When the status is not Running, immediately call
server_body::respond_stopped_503 and return, matching the existing on_request
guard behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 51f7217a-e86d-4a79-b5d1-5ff0171af3e5

📥 Commits

Reviewing files that changed from the base of the PR and between 52af832 and 2b4152e.

📒 Files selected for processing (6)
  • src/runtime/server/RequestContext.rs
  • src/runtime/server/ServerWebSocket.rs
  • src/runtime/server/mod.rs
  • src/runtime/shell/interpreter.rs
  • src/runtime/socket/socket_body.rs
  • test/js/web/workers/worker-terminate-lifetime.test.ts

Every on_request-family entry (on_request, on_user_route_request,
on_saved_request, upgrade_web_socket_user_route, and the generic
on_request_for / on_user_route_request_for) funnels through one of these
two before its first JS call, so the gate there covers the whole family
instead of only on_request.
Comment thread src/runtime/server/mod.rs Outdated
@robobun

robobun commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the sibling-dispatch finding in 74c0a24: moved the script_execution_status() gate into prepare_js_request_context (mod.rs) and prepare_js_request_context_for (server_body.rs), which every on_request-family entry (on_request, on_user_route_request, on_saved_request, upgrade_web_socket_user_route, and the generic on_request_for / on_user_route_request_for) funnels through before its first JS call. serve.test.ts still 280/284 (same four pre-existing environment failures as main).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 src/runtime/server/mod.rs:1085-1091 — The script_execution_status() gate added to on_request isn't applied to its sibling request-dispatch trampolines that also enter user JS in the same uSockets tick: on_user_route_request (mod.rs:1126, routes: { "/x": handler }), on_saved_request (mod.rs:880), on_node_http_request_with_upgrade_ctx (mod.rs:1216), and the server_body.rs h3/generic sites at ~2949/3037/3335/3418 — each still gates only on js_value_for_dispatch(). A worker running Bun.serve({ routes: {...} }) that receives a burst of requests in one poll sweep and is terminate()d inside the first handler will still trip assertNoException() on the next request via Bun__ServerRouteList__callRoute. Copy the same 5-line gate to those entry points (or fold it into js_value_for_dispatch() / prepare_js_request_context).

    Extended reasoning...

    What the bug is

    This PR's Bun.serve half adds a script_execution_status() != Running check at the top of NewServer::on_request (mod.rs:1085–1091), so a request arriving in the same uSockets poll tick as a pending TerminationException gets a 503 instead of entering the JS fetch handler with the exception pending. But on_request is only one of several uWS request-dispatch trampolines on NewServer that call into user JS. The siblings still gate only on js_value_for_dispatch():

    • on_user_route_request — mod.rs:1126–1183, calls Bun__ServerRouteList__callRoute (the routes: { "/x": handler } path)
    • on_saved_request — mod.rs:880
    • on_node_http_request_with_upgrade_ctx — mod.rs:1216 (node:http compat)
    • The server_body.rs generic paths: on_user_route_request_for (~2949), on_request_for (~3037), the h3 user-route site (~3335), and the h3 request site (~3418)
    • (Also on_client_error at ~3625 and on_connection_callback at ~3677, which call user JS behind the same gate.)

    js_value_for_dispatch() is just self.js_value.try_get() (mod.rs:557–559) — it returns None only once the JS wrapper is Finalized. It does not consult script_execution_status(), so a live wrapper on a VM whose termination trap has already fired still passes the gate.

    Code path that triggers it

    Concrete step-through with on_user_route_request:

    1. A worker runs Bun.serve({ port: 0, routes: { "/x": (req) => new Response(...) } }).
    2. In one uSockets poll sweep, N pipelined/parallel requests to /x become ready. uWS dispatches them back-to-back through trampoline::on_user_route_request<SSL,DEBUG>NewServer::on_user_route_request.
    3. Request A enters on_user_route_request: js_value_for_dispatch() is Some, so it proceeds through prepare_js_request_context and calls Bun__ServerRouteList__callRoute, which enters JS via Interpreter::executeCallImpl. While inside the user's route handler, the parent thread's worker.terminate() fires the VM's termination trap; the handler unwinds with the sticky TerminationException pending.
    4. Control returns to the uSockets loop, which immediately dispatches request B to on_user_route_request in the same tick.
    5. js_value_for_dispatch() is still Some (the wrapper is not Finalized). There is no script_execution_status() check on this path, and the PR placed the on_request gate before prepare_js_request_context rather than inside it, so nothing between the trampoline and Bun__ServerRouteList__callRoute observes the pending termination.
    6. Bun__ServerRouteList__callRouteJSC::callInterpreter::executeCallImpl hits scope.assertNoException() with the TerminationException pending → ASSERTION FAILED: !exception() → SIGABRT of the whole process.

    This is the identical failure class the PR fixes for the default fetch path, and the identical shape it fixes for NewSocket ("a burst of dispatches in one poll sweep, terminate() inside the first, next dispatch trips the assert").

    Why existing code doesn't prevent it

    The new gate at run_error_handler_with_status_code_dont_check_responded (RequestContext.rs:3585–3593) does help these siblings — they all funnel into handle_requeston_response → the error handler — but it only covers the after-call path (routing the pending exception into the user's error() handler). It cannot prevent the before-call assertNoException() on entering the route/node-http handler itself, which is what the on_request entry gate was added for.

    Impact

    Same as the on_request case this PR fixes: debug/ASAN-build SIGABRT (release WebKit compiles the assert out). It affects any worker using Bun.serve({ routes: {...} }), node:http-backed servers, or the h3 path — arguably more common than a bare fetch handler. Per REVIEW.md ("Fix the whole class in the same PR — grep for every sibling site sharing the pattern… If a site is intentionally excluded, say so in the PR"), and given this PR's own methodology for NewSocket/ServerWebSocket was exactly "upgrade every sibling gate", leaving these ungated is a same-class miss in the same file.

    Fix

    Copy the same 5-line script_execution_status() != Runningrespond_stopped_503 gate into each sibling right after the js_value_for_dispatch() check. Alternatively, fold the check into js_value_for_dispatch() itself (return None when not Running) or into prepare_js_request_context, which every JS-entering path shares — that would cover on_request, on_user_route_request, on_saved_request, on_node_http_request_with_upgrade_ctx, and the server_body.rs generics in one place.

Comment thread src/runtime/socket/socket_body.rs
Comment thread src/runtime/server/ServerWebSocket.rs
Socket module: the two SNI server_name callbacks (same BoringSSL-handshake
shape as ALPN), UpgradedDuplex::call_write_or_end, the Windows named-pipe
accept path, and Handlers::{resolve,reject}_promise/call_error_handler
still gated on is_shutting_down() before their JS entry.
Handlers::mark_inactive stays as-is (no JS entry; the check there is about
the wrapper being gone at process exit).

Server: fold the gate into NewServer::js_value_for_dispatch so the
node-http / client-error / connection-callback entry points (which do not
go through prepare_js_request_context) share it.
Comment thread src/runtime/server/mod.rs Outdated
Comment thread src/runtime/server/RequestContext.rs Outdated
Every request-family trampoline (on_request, on_user_route_request,
on_saved_request both arms, on_node_http_request_with_upgrade_ctx, the
on_web_socket_upgrade id==0 fetch fallthrough, on_client_error,
on_connection_callback, and the generic on_request_for /
on_user_route_request_for / upgrade_web_socket_user_route) calls
js_value_for_dispatch() before its first JS entry, so the gate there
covers the class. prepare_js_request_context keeps a belt-and-braces
check for the bake prepare_and_save_js_request_context path that bypasses
js_value_for_dispatch. Also merge the duplicated option match in
run_error_handler_with_status_code_dont_check_responded.
Comment thread src/runtime/server/RequestContext.rs
Comment thread src/runtime/server/mod.rs
Comment thread src/runtime/server/mod.rs
@robobun

robobun commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

Re the on_node_http_request_with_upgrade_ctx / on_web_socket_upgrade id==0 / on_saved_request Saved finding: 801fd3a (prior to that review) already folded the script_execution_status() check into NewServer::js_value_for_dispatch(), so all three are covered (each calls js_value_for_dispatch() before its first JS entry; the Saved arm reaches it at the top of on_saved_request). d91033f documents this on js_value_for_dispatch() and keeps a belt-and-braces check in prepare_js_request_context for the one bake caller (prepare_and_save_js_request_context) that bypasses js_value_for_dispatch(). The duplicated if let Some(server) in run_error_handler_* is merged in the same commit.

Comment thread src/runtime/server/mod.rs
Comment thread src/runtime/server/mod.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/mod.rs`:
- Around line 557-565: Update js_value_for_dispatch to release the saved request
context when script_execution_status is not Running: invoke
SavedRequest::deinit() or the established cancellation cleanup before returning
None. Ensure this stopped path calls RequestContext::deinit() and ultimately
on_request_complete(), while preserving the existing None return behavior.

In `@src/runtime/server/RequestContext.rs`:
- Around line 3588-3593: Re-check script execution termination immediately after
the user-JavaScript on_error.call operation in RequestContext.rs before entering
finish_running_error_handler, process_on_error_promise, or render, returning
when the status is no longer Running. Also update UpgradedDuplex.rs after each
duplex.get operation to perform the same termination check before the next
JavaScript entry.

In `@src/runtime/socket/Handlers.rs`:
- Around line 218-220: Update the handler flow around script_execution_status
and take_promise so the pending promise is detached before returning for
non-Running statuses. Discard the detached promise when the VM is stopped or the
worker terminates, while preserving reject_promise’s true terminal-cancellation
result for callbacks that can 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: a4704161-0cfe-4fc7-b8b1-f7c115a5f221

📥 Commits

Reviewing files that changed from the base of the PR and between 2b4152e and 5880eb4.

📒 Files selected for processing (5)
  • src/runtime/server/RequestContext.rs
  • src/runtime/server/mod.rs
  • src/runtime/socket/Handlers.rs
  • src/runtime/socket/Listener.rs
  • src/runtime/socket/UpgradedDuplex.rs

Comment thread src/runtime/server/mod.rs
Comment thread src/runtime/server/RequestContext.rs
Comment thread src/runtime/socket/Handlers.rs
… left a termination pending

call_write_or_end's writableEnded probe and run_error_handler_*'s
on_error.call both have a follow-up JS entry in the same function; a
has_exception() check after take_exception prevents the assert when
terminate() lands inside the first.
@robobun

robobun commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:41 AM PT - Aug 3rd, 2026

@robobun, your commit 1b9a1f8 has 2 failures in Build #88022 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 36808

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

bun-36808 --bun

Comment thread src/runtime/socket/Listener.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/runtime/socket/UpgradedDuplex.rs (1)

210-211: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Re-check VM status before each subsequent JavaScript call.

duplex.get(...) performs JSC property access and can run a getter or proxy trap. A termination request is tracked by script_execution_status(), not global.has_exception(). Check the status after the writableEnded lookup and after the write/end lookup, before write_or_end.call(...), including the non-teardown path.

🤖 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/socket/UpgradedDuplex.rs` around lines 210 - 211, Update the
JavaScript call sequence in the upgraded duplex handling to re-check
vm.script_execution_status() after the writableEnded lookup and again after the
write/end lookup, before invoking write_or_end.call(...). Apply these checks to
both teardown and non-teardown paths, returning immediately when the status is
no longer Running.

Source: Coding guidelines

🤖 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 3604-3606: After on_error.call(), update the guard in the
surrounding error-handling flow to re-check script_execution_status() before
processing result, covering both pending exceptions and worker termination
requests. Return immediately when execution is no longer live, preventing
finish_running_error_handler, process_on_error_promise, or render from entering
JavaScript again.

---

Outside diff comments:
In `@src/runtime/socket/UpgradedDuplex.rs`:
- Around line 210-211: Update the JavaScript call sequence in the upgraded
duplex handling to re-check vm.script_execution_status() after the writableEnded
lookup and again after the write/end lookup, before invoking
write_or_end.call(...). Apply these checks to both teardown and non-teardown
paths, returning immediately when the status is no longer Running.
🪄 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: 61afec87-acda-4fe7-b834-da2646b9f3bd

📥 Commits

Reviewing files that changed from the base of the PR and between 5880eb4 and 66c07c1.

📒 Files selected for processing (2)
  • src/runtime/server/RequestContext.rs
  • src/runtime/socket/UpgradedDuplex.rs

Comment thread src/runtime/server/RequestContext.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/runtime/socket/Listener.rs (1)

1919-1919: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Handle termination after both SNI callbacks.

After each callback.call(), check global.has_exception() before normal result decoding or any later JavaScript entry. A termination exception can remain pending and trigger assertNoException().

🤖 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/socket/Listener.rs` at line 1919, Update the SNI callback
handling around handlers.vm.script_execution_status() so each callback.call() is
immediately followed by a global.has_exception() check before result decoding or
any further JavaScript entry; handle termination and return through the existing
termination path, preventing a pending exception from reaching
assertNoException().

Source: Learnings

🤖 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.

Outside diff comments:
In `@src/runtime/socket/Listener.rs`:
- Line 1919: Update the SNI callback handling around
handlers.vm.script_execution_status() so each callback.call() is immediately
followed by a global.has_exception() check before result decoding or any further
JavaScript entry; handle termination and return through the existing termination
path, preventing a pending exception from reaching assertNoException().

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 71493799-61a2-4726-a12c-0ac717b2efe5

📥 Commits

Reviewing files that changed from the base of the PR and between 66c07c1 and 1b9a1f8.

📒 Files selected for processing (1)
  • src/runtime/socket/Listener.rs

@robobun

robobun commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

Re the SNI callback.call() post-check suggestion (Listener.rs:1919): decode_sni_result has no JS entry (its body is is_boolean/to_error/is_undefined_or_null/as_class_ref type checks and an out-parameter store), so there is no follow-up call for a has_exception() bail to protect. If the trap fires inside callback.call(), the Err arm take_exceptions it into result, result.to_error() is true, abort_handshake is set, and the function returns null (the correct TLS-level refusal for a dying VM). The listener-level SNI callback at ~2012 has the same shape. Leaving these as single-entry-gated.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found; every prior finding (mine and CodeRabbit's) has been addressed and resolved. Deferring to a human because the sweep touches Bun.serve request dispatch, the socket/WebSocket callback tables, and shell finish — critical paths where the non-mechanical early-return sites (run_error_handler_*, prepare_js_request_context, Interpreter::finish) deserve a maintainer's sign-off.

What was reviewed:

  • The is_shutting_down()script_execution_status() != Running swaps at every remaining src/runtime/socket / ServerWebSocket dispatch site — confirmed no JS-entering is_shutting_down() gates remain in those modules (Handlers::mark_inactive intentionally kept).
  • Interpreter::finish early return: cleanup (keep_alive.disable, deref_root_shell_and_io_if_needed(true), _decr guard) mirrors the success path; only the JS Buffer allocs and resolve.call are skipped.
  • js_value_for_dispatch new None condition: every caller already has the 503-and-return path from the pre-existing Finalized gate; prepare_js_request_context covers the one bake caller that bypasses it.
  • ServerWebSocket payload let Ok(...) else { return }: the _loop_guard is already armed, so the pending exception surfaces on scope exit; no JSValue::ZERO reaches Bun__JSValue__call.
Extended reasoning...

Overview

Follow-up to #36579 that upgrades the remaining is_shutting_down() JS-entry gates to the worker-aware script_execution_status() != Running predicate across NewSocket (socket_body.rs: on_writable/on_data/on_end/on_timeout/on_close/handle_error/ALPN), Handlers (resolve/reject_promise, call_error_handler), Listener (SNI ×2, Windows named-pipe accept), UpgradedDuplex::call_write_or_end, ServerWebSocket (on_open/message/drain/ping/pong/close), NewServer (js_value_for_dispatch, prepare_js_request_context), RequestContext::run_error_handler_*, and shell Interpreter::finish. Also fixes three unwrap_or(JSValue::ZERO) → early-return sites in ServerWebSocket that would trip Bun__JSValue__call's empty-argument assert. One new debug-only regression test in worker-terminate-lifetime.test.ts covering the on_end burst shape.

Security risks

None. This is defensive gating of JS re-entry during worker VM teardown; no auth, crypto, input parsing, or trust-boundary changes. The early-return paths run strictly less code than before.

Level of scrutiny

High — the touched functions are the hot dispatch paths for Bun.serve, Bun.listen/Bun.connect, ServerWebSocket, and Bun.$. About 15 sites are purely mechanical predicate swaps following the #36579 pattern; the remainder introduce new early-return branches with their own cleanup ordering (Interpreter::finish, prepare_js_request_context → 503, run_error_handler_* has_exception() bail, the ServerWebSocket payload-Err returns, and the between-JS-entry has_exception() rechecks in RequestContext/UpgradedDuplex added in 66c07c1). Those are the parts a maintainer should confirm.

Other factors

The PR has been through several review rounds: my earlier findings (missed SNI/Handlers/UpgradedDuplex/named-pipe siblings; on_node_http_request/on_saved_request/ws-upgrade coverage via js_value_for_dispatch; two stale doc-comments) were addressed in 801fd3a/d91033f/5880eb4/1b9a1f8. CodeRabbit's re-check-between-JS-entries finding was addressed in 66c07c1; its SavedRequest cleanup and take_promise ordering findings were withdrawn as pre-existing/out-of-scope (correctly — the early-return paths only fire on a terminating VM whose teardown releases the remaining state). The new test follows the file's existing debug-only pattern and the PR description records fail-before/pass-after. CI on 1b9a1f8 (build #88022) is still building per the timeline. Given the breadth across critical dispatch tables and the several non-mechanical early-return sites, this should get a human sign-off rather than a bot auto-approve.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No bugs found this pass; the earlier inline findings are all addressed. Deferring to a human rather than approving because the change spans dispatch entry points in four subsystems (NewSocket, ServerWebSocket, Bun.serve request/error, Bun.$) and adds new early-return paths on hot code, and build #88022 shows test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js SIGABRTing on x64-asan — worth confirming that's pre-existing on main before merge.

Checked: the is_shutting_down()script_execution_status() sweep now covers every JS-entering site in src/runtime/socket/ (SNI, ALPN, UpgradedDuplex, Handlers, named-pipe accept) and the request-family entries via js_value_for_dispatch().
Checked: the let Ok(payload) else { return } early returns in ServerWebSocket land inside _loop_guard, so a non-termination exception is reported (not left pending) on the way out.
Checked: the Interpreter::finish bail drops keep_alive and root-io before returning, matching the surrounding cleanup order.

Extended reasoning...

Overview

Follow-up to #36579: upgrades every remaining is_shutting_down() gate on a JS-dispatch entry to the worker-aware script_execution_status() != Running predicate, so a TerminationException raised inside one callback in a poll sweep does not reach the next callback's assertNoException(). Touches NewSocket (on_writable/on_data/on_end/on_timeout/on_close/handle_error/ALPN), the two SNI callbacks, Windows named-pipe accept, Handlers::{resolve,reject}_promise/call_error_handler, UpgradedDuplex::call_write_or_end, all six ServerWebSocket handlers (plus fixing the .unwrap_or(JSValue::ZERO) payload-argument aborts), Bun.serve's js_value_for_dispatch/prepare_js_request_context/error-handler dispatch, and Bun.$'s Interpreter::finish. Adds one debug-only regression test.

Security risks

None. No new user-visible surface, no auth/crypto/permissions changes; the gates are strictly more conservative than before.

Level of scrutiny

Medium-high. The individual edits are a mechanical predicate swap following #36579's proven pattern, but they add early-return paths across ~20 hot dispatch sites in four subsystems. The bail-out cleanup on each path matters (e.g. shell keep-alive, ServerWebSocket _loop_guard ordering, respond_stopped_503 on the request path), and the between-JS-entry has_exception() bails in RequestContext/UpgradedDuplex are new control flow.

Other factors

All four rounds of my earlier inline findings were addressed in 801fd3a1b9a1f8; CodeRabbit's three findings were withdrawn or fixed in 66c07c1. The one open nit (routing the binary_to_js Err through run_error_callback like on_close does) is non-blocking — _loop_guard drop reports it. Build #88022 shows test-worker-message-port-transfer-terminate.js SIGABRTing on x64-asan; this PR does not touch MessagePort, but it is in the worker-terminate area, so a human should check whether it also fails on main. bun-add.test.ts on Windows is unrelated to this diff.

Jarred-Sumner pushed a commit that referenced this pull request Aug 6, 2026
…or TLS session/keylog dispatch fails (#37067)

### What

Fuzzing (Fuzzilli) keeps hitting this debug assertion in processes
exercising sockets:

```
ASSERTION FAILED: !scope.exception() || vm.hasPendingTerminationException() || !hasProperty
JavaScriptCore/JSObjectInlines.h(137) : JSValue JSC::JSObject::get(JSGlobalObject *, PropertyName)
```

It fires when any native `JSObject::get` runs while a non-termination JS
exception is pending. Several socket dispatch sites could leave the VM
in exactly that state: they call a fallible operation whose `Err` means
"a JS exception is pending", discard the `Err`, and return to the
uSockets/libuv event loop. The next native JSC call in the same tick
then runs with the stale exception. In debug/ASAN builds that trips the
assert (`JSObject::get`, or `assertNoExceptionExceptTermination` at the
microtask-drain entry); in release the exception gets attributed to
whatever runs next.

The sites, all in `src/runtime/socket/`:

- `handle_connect_error` swallowed a failed promise reject with
`handlers.reject_promise(err_val).unwrap_or(true)` and returned `Ok`
with the exception still set (a `// TODO: properly propagate exception
upwards` site), and its other two settle sites (`reject`,
`reject_as_handled`) returned `Err` into callers that all discarded it:
`connect()`'s synchronous `do_connect()` failure branch, the
`on_connect_error` event-loop dispatch, the Windows named-pipe
`on_error`/`fail_and_release`, and the duplex TLS upgrade teardown.
- `on_open` swallowed a failed resolve of the connect promise (the other
`// TODO: properly propagate exception upwards` site) and returned to
the loop with the exception pending, and did the same `unwrap_or(true)`
swallow when rejecting with an error returned from the `open` callback.
- `us_dispatch_session` / `us_dispatch_keylog` (and the duplex and
named-pipe equivalents) discarded `on_session` / `on_keylog` results;
those return `Err` when allocating the session/keylog `Buffer` throws.

### Fix

The pending exception has to be handled at the failure site, before the
dispatch's scope guards drop: `ScopeExit` runs a microtask checkpoint on
scope exit, and entering that checkpoint with a non-termination
exception pending is itself the asserted condition, so an `Err`
propagated or discarded past the guard is already too late. And the
event-loop dispatch callers have no JS frame to propagate into in the
first place.

So each failed settle now reports its exception as unhandled right where
it fails, via `report_active_exception_as_unhandled`: the same idiom the
socket error handler, ipc, h2, fs-watcher, and dns (#37004) completions
use. That clears and reports a real throw, and leaves a termination
exception pending, which is what the event loop expects
(`assertNoExceptionExceptTermination`). With every failure handled
internally, `handle_connect_error`, `on_session`, and `on_keylog` become
infallible, and `on_handshake`/`on_close` (whose `Err` paths were
structurally dead: `verify_error_to_js` cannot fail) are made infallible
with them, so no dispatch call site can discard an `Err` from this
family again; every `let _ =` at those call sites is gone.

### Verification

The promise-settle failures (`JSPromise::resolve/reject`) only occur on
JSC heap OOM or termination mid-call, so those arms have no
deterministic reproduction; they are covered by the behavior-level tests
below plus the analysis above. The session/keylog `Buffer` allocation
has the same trigger, but for that one this PR adds a `session_buffer`
fault-injection rule (mirroring the existing `ssl_loop_buffer` rule for
the one other allocation whose failure is unreachable without
injection), which makes the fix deterministically testable:

- `socket-session-oom-fixture.ts` arms the rule, completes a TLS
handshake with a `session` handler, and asserts the injected allocation
failure surfaces as an `uncaughtException` while the event loop survives
(a TCP round trip completes afterwards and the process exits 0). With
the hook but without the fix, the same fixture dies on `ASSERTION
FAILED: ... !exception() || m_vm.hasPendingTerminationException()` in
`ExceptionScope::assertNoExceptionExceptTermination`, the fuzzer's
failure class.
- New behavior tests cover every touched dispatch path: TLS
`session`/`keylog` delivery (previously untested), throws from
`session`/`keylog`/`open` handlers reaching the socket's (or listener's)
`error` handler while `connect()` still resolves, a synchronous unix
connect failure rejecting with `ENOENT` (run in a child with a relative
socket path so macOS's 104-byte `sun_path` limit cannot clobber the
errno), and a throw from `connectError` becoming the `connect()`
rejection.
- The Windows named-pipe paths were exercised directly (named-pipe
client lifecycle tests, including the failed-connect branch this PR
touches, and the Windows connect error-code tests): all pass on a
Windows debug build.
- The existing fault-injection suites (`socket-syscall-fault`,
`tls-syscall-fault`) pass with the new rule added.
- `test/js/bun/net/`, `test/js/node/net/`, and the node-tls connect
suites show the identical failure set as a baseline build in this
sandboxed environment (local DNS/IPv6 limitations), with no new
failures. `cargo check` passes on all 10 platform targets.

Related but distinct open work: #36808 gates these dispatch sites
against an already-pending termination (entry-side), while this PR fixes
exceptions the dispatch itself creates (exit-side); #35221 changes which
errno the DNS-path `connectError` reports, not the exception discipline.

<!-- robobun:evidence:begin -->

---

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

<!-- robobun:evidence:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants