socket: don't leave exceptions pending when a connect promise settle or TLS session/keylog dispatch fails

[robobun]

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.

---

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

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Changes

The socket runtime now reports callback and promise failures as unhandled exceptions or propagates them through connection failures. TLS session and keylog dispatch use unit-returning handlers. Fault injection and tests cover these paths.

Socket error handling

Layer / File(s)	Summary
Connection error propagation src/runtime/socket/socket_body.rs, src/runtime/socket/Listener.rs, src/runtime/socket/WindowsNamedPipeContext.rs, src/runtime/socket/mod.rs	Connection-error handlers report promise rejection failures and preserve callback exceptions through synchronous connection paths.
Callback dispatch and cleanup src/runtime/socket/socket_body.rs, src/runtime/socket/uws_dispatch.rs, src/runtime/socket/WindowsNamedPipeContext.rs	Open, handshake, session, keylog, and close handlers use unit-returning dispatch. Active exceptions are reported before cleanup.
TLS buffer fault injection packages/bun-usockets/src/internal/fault_inject.h, src/js/internal-for-testing.ts, src/runtime/socket/socket_body.rs, src/uws_sys/lib.rs	The session_buffer fault-injection target and native bindings support simulated TLS buffer allocation failures.
Socket error tests test/js/bun/net/socket.test.ts, test/js/bun/net/socket-session-oom-fixture.ts	Tests cover TLS Buffer delivery, callback exceptions, allocation failures, open-handler exceptions, Unix-socket failures, and connectError rejection propagation.

Possibly related PRs

• oven-sh/bun#36140: Both changes update socket connection-error handling.
• oven-sh/bun#36579: Both changes update socket event-dispatch behavior.
• oven-sh/bun#36909: Both changes update socket and TLS error propagation.

Suggested reviewers: cirospaciari, 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 clearly summarizes the main fix for pending exceptions during socket promise settlement and TLS session/keylog dispatch.
Description check	✅ Passed	The description explains the problem, implementation, affected paths, and verification results, although its headings differ from the template.

---

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

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[claude]

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

• 🔴 src/runtime/socket/WindowsNamedPipeContext.rs:251-255 — fail_and_release still discards handle_connect_error's result with _ =, but its callers (WindowsNamedPipeContext::open()/connect() via FailAndRelease::drop) run from connect_inner's named-pipe branch, which sets the connect promise before the call — so handle_connect_error can return Err with a pending exception here, and connect_inner then does Err(_) => return Ok(promise_value). The PR's stated exemption ("duplex TLS upgrade teardown … no connect promise") doesn't cover this site. Same fix as on_error a few lines up: read (*this).global_this and report_active_exception_as_unhandled(e) on Err.

  Extended reasoning...

  What the bug is

  WindowsNamedPipeContext::fail_and_release still uses the discard pattern this PR is eliminating:

  match_socket!(unsafe { (*this).socket }, |s: NewSocket<SSL>| _ =
      NewSocket::handle_connect_error(s, SystemErrno::ENOENT as i32, 0));

  This is the same file, and the same pattern, as on_error — which this PR did fix (lines 246–255) by capturing global_this and calling report_active_exception_as_unhandled(e) on Err.

  Code path that triggers it

  Step by step:

  1. Listener::connect_inner (Windows named-pipe branch, both TLS and TCP arms) does:

     let promise = jsc::JSPromise::create(global);
     let promise_value = promise.to_js();
     handlers.set_promise(global, promise_value);   // ← promise IS set
     // ...
     let named_pipe_result = ... WindowsNamedPipeContext::connect(...) / ::open(...);
     let named_pipe = match named_pipe_result {
         Ok(p) => p,
         Err(_) => return Ok(promise_value),         // ← returns Ok from host call
     };

  2. Inside WindowsNamedPipeContext::connect/open, a FailAndRelease guard is armed via Self::armed(this). If named_pipe.open(...)? or named_pipe.connect(...)? fails (e.g. uv_pipe_init or the TLS wrapper init fails), the ? returns Err and the guard's Drop runs fail_and_release(this).
  3. fail_and_release calls NewSocket::handle_connect_error(s, ENOENT, 0). The socket's handlers is an Rc::clone of the same Handlers on which step 1 called set_promise, so the promise is present.
  4. After this PR, handle_connect_error's only Err-returning sites are js_promise.reject(...)? (line 1187), handlers.reject_promise(err_val)? (line 1212 — the very site this PR changed), and promise.reject_as_handled(...)? (line 1223). All three require a promise, which this path has. If any of those rejects fails (allocation failure), Err is returned with the exception still pending on the VM.
  5. fail_and_release discards that Err with _ =, then connect_inner sees Err(_) from WindowsNamedPipeContext::connect/open and does return Ok(promise_value) — returning Ok from the Bun.connect() host call with a non-termination exception pending.

  That is exactly the !scope.exception() || vm.hasPendingTerminationException() assertion trigger the PR description names.

  Why the PR's stated exemption doesn't apply

  The description says:

  The two handle_connect_error calls in the duplex TLS upgrade teardown keep their let _ =: that flow has no connect promise, and every Err path in handle_connect_error requires one, so no exception can escape there.

  That reasoning is sound for the two DuplexUpgradeContext sites (socket_body.rs ~4342/4447), but it does not cover fail_and_release: its only callers set the connect promise before invoking it, so an Err can escape. The site is not listed among the intentional exclusions.

  Impact

  Same as the other sites this PR fixes: debug/ASAN assertion in JSObjectInlines.h on the next native get, or in release the pending exception is misattributed to whatever runs next. Windows-only (the file is cfg-gated) and requires an allocation failure inside a promise reject, so like the rest of this PR it has no deterministic repro — but per REVIEW.md "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep)… If a site is intentionally excluded, say so in the PR", leaving one sibling of the fixed pattern in the same file makes the fix incomplete.

  Fix

  Mirror the on_error change this PR already made ~60 lines above:

  fn fail_and_release(this: *mut Self) {
      // SAFETY: `this` is live; `global_this` is a Copy field read.
      let global = unsafe { (*this).global_this };
      match_socket!(unsafe { (*this).socket }, |s: NewSocket<SSL>| {
          if let Err(e) = NewSocket::handle_connect_error(s, SystemErrno::ENOENT as i32, 0) {
              global.report_active_exception_as_unhandled(e);
          }
      });
      unsafe { Self::deref(this) };
  }

  (? isn't available here because it runs from Drop, so the event-loop-dispatch idiom is the right one — same choice the PR made for on_error and on_connect_error.)

[robobun]

Re the fail_and_release finding: also addressed by 9f9f84e, which made handle_connect_error infallible. Each failed promise settle now reports its pending exception via report_active_exception_as_unhandled at the failure site, inside the dispatch's scope guards, so fail_and_release (and every other caller) has nothing to discard anymore; the _ = there is gone. Handling it inside handle_connect_error rather than at each caller also keeps the exception from reaching the microtask checkpoint that ScopeExit runs on drop, which a caller-side report would be too late for.

The named-pipe analysis is right that the connect promise is set before fail_and_release runs, so that path was indeed reachable; it is covered by the same internal reporting now.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

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/socket_body.rs (1)

1902-1912: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move the session doc block onto on_session.

The doc comment at Lines 1902-1907 describes a TLS session arriving and the session handler dispatch. It now attaches to create_dispatch_buffer, because on_session moved below the helper. A reader of create_dispatch_buffer gets a description of the session dispatch instead of the buffer allocation.

Move those lines to on_session (Line 1934) and keep only the buffer description on the helper.

♻️ Proposed doc relocation

-    /// A new resumable TLS session arrived (the peer's NewSessionTicket was
-    /// processed during an earlier `SSL_read`). Hands the serialized session
-    /// to the JS `session` handler, mirroring Node's `onnewsession` callback.
-    /// Dispatched from `ssl_flush_pending_session()` after the SSL stack has
-    /// unwound, so the JS handler may safely destroy the socket.
-    ///
     /// The JS `Buffer` for an `on_session`/`on_keylog` payload. The
     /// `session_buffer` fault rule simulates the allocation throwing: that
     /// failure needs JSC heap exhaustion, so it is unreachable from a test
     /// without injection.
     fn create_dispatch_buffer(global: &JSGlobalObject, len: usize) -> JsResult<JSValue> {

Then prepend the moved block to on_session:

+    /// A new resumable TLS session arrived (the peer's NewSessionTicket was
+    /// processed during an earlier `SSL_read`). Hands the serialized session
+    /// to the JS `session` handler, mirroring Node's `onnewsession` callback.
+    /// Dispatched from `ssl_flush_pending_session()` after the SSL stack has
+    /// unwound, so the JS handler may safely destroy the socket.
+    ///
     /// Takes `ThisPtr<Self>` for the same re-entrancy reason as `on_writable`.
     pub(crate) fn on_session(this: bun_ptr::ThisPtr<Self>, session: &[u8]) {

🤖 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/socket_body.rs` around lines 1902 - 1912, Move the TLS
resumable-session dispatch documentation from above create_dispatch_buffer to
directly above on_session. Keep only the JS Buffer allocation and session_buffer
fault-rule documentation attached to create_dispatch_buffer, preserving the
existing wording and 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/socket/socket_body.rs`:
- Around line 1902-1912: Move the TLS resumable-session dispatch documentation
from above create_dispatch_buffer to directly above on_session. Keep only the JS
Buffer allocation and session_buffer fault-rule documentation attached to
create_dispatch_buffer, preserving the existing wording and behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: be299eba-12b1-4e2f-95d5-65eff924addd

📥 Commits

Reviewing files that changed from the base of the PR and between 45be4ac and ea27963.

📒 Files selected for processing (10)

• packages/bun-usockets/src/internal/fault_inject.h
• src/js/internal-for-testing.ts
• src/runtime/socket/Listener.rs
• src/runtime/socket/WindowsNamedPipeContext.rs
• src/runtime/socket/mod.rs
• src/runtime/socket/socket_body.rs
• src/runtime/socket/uws_dispatch.rs
• src/uws_sys/lib.rs
• test/js/bun/net/socket-session-oom-fixture.ts
• test/js/bun/net/socket.test.ts

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

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 `@test/js/bun/net/socket-session-oom-fixture.ts`:
- Around line 18-21: Update the uncaughtException handler around
gotUncaught/onUncaught so only the first exception resolves the promise; report
every subsequent uncaught exception to stderr or another visible error channel
instead of swallowing it, while preserving the fixture’s existing first-error
behavior.

🪄 Autofix

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: 5922566e-df39-4b82-9b3d-589ca9f4a2b5

📥 Commits

Reviewing files that changed from the base of the PR and between b3941fc and 118d50b.

📒 Files selected for processing (10)

• packages/bun-usockets/src/internal/fault_inject.h
• src/js/internal-for-testing.ts
• src/runtime/socket/Listener.rs
• src/runtime/socket/WindowsNamedPipeContext.rs
• src/runtime/socket/mod.rs
• src/runtime/socket/socket_body.rs
• src/runtime/socket/uws_dispatch.rs
• src/uws_sys/lib.rs
• test/js/bun/net/socket-session-oom-fixture.ts
• test/js/bun/net/socket.test.ts

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[robobun]

CI status: every failure in the latest runs (89690, 89695) is outside this diff. The one red test, test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts (SIGABRT on the x64-asan lane), is a pre-existing failure on main and has been reported separately; the rest are Windows-lane flakes that passed on retry (http3 handshake, bake deinitialization, terminal gaps, install registry). The socket, net, and tls suites, including the new fault-injection test on the asan lane, are green everywhere. Ready for review.

[claude]

All prior review findings (mine and CodeRabbit's) are addressed at 41859c3; nothing new this pass. Deferring to a human given the scope — this reworks exception discipline across the socket dispatch layer (connect-error, open, handshake, close, session/keylog) plus the Windows named-pipe and duplex-upgrade call sites.

What was reviewed:

• Every let _ = / .unwrap_or(true) swallow named in the description is gone; each now-infallible dispatch (handle_connect_error, on_connect_error, on_open, on_handshake, on_close, on_session, on_keylog) reports before exit_scope's microtask checkpoint.
• report_active_exception_as_unhandled ordering vs exit_scope / has_exception() guards checked at each site; matches the ipc/h2/dns precedent.
• New session_buffer fault hook: enum value 12 consistent across fault_inject.h / uws_sys / testing_apis; us_fault_hit extern signature matches the C header (ssize_t* ↔ *mut isize); fd-rejection and error-string updated together.
• Tests: fault-injection fixture asserts exactly one uncaught + a post-failure round trip; behavior tests cover session/keylog delivery, handler throws, sync unix ENOENT, and connectError-throw-as-rejection.

Extended reasoning...

Overview

The PR eliminates a class of stale-pending-exception bugs in src/runtime/socket/: dispatch sites that called a fallible promise-settle or Buffer-allocation, discarded the Err, and returned to the uSockets/libuv loop with a JS exception still set on the VM. Six dispatch functions (handle_connect_error, on_connect_error, on_open, on_handshake, on_close, on_session, on_keylog) are made infallible, with each internal failure reported via report_active_exception_as_unhandled before the scope-exit microtask checkpoint. Every call site across mod.rs, uws_dispatch.rs, WindowsNamedPipeContext.rs, Listener.rs, and the DuplexUpgradeContext in socket_body.rs drops its let _ =. A new session_buffer fault-injection rule makes the OOM-only Buffer-allocation failure deterministically testable, wired through fault_inject.h, uws_sys/lib.rs, internal-for-testing.ts, and the testing_apis parser. ~265 lines of new tests in socket.test.ts plus a spawned fixture cover every touched dispatch path.

Security risks

None identified. No auth/crypto/permission logic changes; the TLS surface touched is dispatch plumbing (session ticket / keylog delivery), not verification or trust decisions. The new fault-injection hook is compile-gated behind cfg(socket_fault_injection) and reachable only through bun:internal-for-testing.

Level of scrutiny

High. This is production-critical socket/TLS dispatch code that runs for every Bun.connect/Bun.listen connection, with cross-platform branches (Windows named pipes, duplex TLS upgrade) and release-build behavioral impact (exception misattribution, not just a debug assert). The change is mechanically consistent — the same report_active_exception_as_unhandled idiom applied at each site, matching existing precedent in ipc/h2/fs-watcher/dns — but the ordering vs exit_scope and the interaction with termination exceptions is subtle enough that it benefits from a maintainer's eye.

Other factors

I reviewed this across three prior iterations and raised four findings (the missed on_open sites, the still-fallible on_handshake/on_close, a misplaced doc comment, and the ungated us_fault_hit call); all were addressed, as was CodeRabbit's fixture-hardening nit. The bug-hunting system found nothing this run. Test coverage is strong: a fault-injection fixture that reproduces the exact assertion class the fuzzer hit, plus behavior tests for every touched dispatch path (including previously-untested TLS session/keylog delivery). The PR description states Windows named-pipe tests and cargo check on all 10 targets pass; I can't verify the Windows paths directly. Given the scope and the number of call sites across three transports, a human sign-off is warranted.