jsc: report the flatten exception from JSString conversions; answer onResolve once

[robobun]

Problem

• An onResolve plugin that returns a path or namespace string JSC cannot flatten makes a debug build abort with ASSERTION FAILED: Unexpected exception observed ... Error Exception: Out of memory / !exception() in JSC::ExceptionScope::releaseAssertNoException() once jsBundlerPluginFunction_onResolveAsync returns.
• Release builds are worse than "the path is read as empty": the exception left on the VM rejects the async wrapper in runOnResolvePlugins (src/js/builtins/BundlerPlugin.ts), whose rejection handler calls addError for the same request, so the one Resolve is answered twice (Success with an empty string, then Err). Running the repro below 40 times in a loop on the current canary (da3851e) segfaults or panics; a single run reports success: true or the error depending on timing.
• Cause: JSC__JSString__toZigString (src/jsc/bindings/bindings.cpp) calls JSString::value(), which on a failed flatten throws and returns the null string; the shim hands back an empty ZigString with the exception still pending, and JSString::to_zig_string (src/jsc/JSString.rs) never checks for it. So get_zig_string, view, to_slice and to_slice_clone on JSString (and JSValue::to_slice_clone, which goes through it) reported success with an empty string. The .expect("Unexpected: path is not a string") in JSBundlerPlugin__onResolveAsync (src/runtime/api/JSBundler.rs) never fired because the helper never reported the failure.
• The same swallow is reachable from the other callers of these helpers. For example ws.send(str) / ws.publish(topic, str) / server.publish(topic, str) wrote an empty text frame to the peer before the error surfaced, and Bun.semver, zlib.crc32, util.parseEnv, dns.lookup, udpSocket.send, http2 header encoding and FileSink/ArrayBufferSink.write all carried on with an empty string. JSValue::to_zig_string (the JSValue flavour of the same helper) already handled this correctly, so JSValue::get_zig_string / to_slice callers were not affected.

Fix

• JSString::to_zig_string wraps the FFI call in from_js_host_call_generic, exactly as JSValue::to_zig_string does, and returns JsResult<()>; get_zig_string, view and to_slice become JsResult too and to_slice_clone propagates instead of always returning Ok. This is correct because flattening a rope is a fallible JSC operation and this wrapper is the layer that turns a pending exception into a JsResult; a C++ shim leaving the exception on the VM is the normal JSC convention.
• Every caller now propagates the error: host functions use ? (their callers already unwind on a pending exception), the JsError-returning helpers in JSGlobalObject.rs / validators.rs return the pending exception instead of building a second error on top of it, and the two non-throwing contexts keep their existing behaviour for "could not read the string" (wrap_unhandled_rejection_error_for_uncaught_exception falls through to its existing undefined message, import_windows_socket_payload clears and returns None, matching the Err arm a few lines above it).
• JSBundlerPlugin__onResolveAsync turns an Err into ResolveValue::Err(plugin_msg_from_js(...)), which is what JSBundlerPlugin__addError does for an exception thrown by the callback itself, and still reaches on_resolve_async. Taking the exception (take_exception) is what removes the second answer: the host function returns with nothing pending, so runOnResolvePlugins no longer rejects. The build fails with error: Out of memory at the importing file, and the next Bun.build in the process works. This keeps the JSBundler.rs change to the one branch since bundler: touch outstanding plugin requests through their pointer, not as &mut #37746 / bundler: stop the plugin hops from reaching back into the pass that posted them #37709 restructure these thunks; bundler: fail the build instead of panicking when onLoad contents cannot be converted #37858 does the equivalent for the onLoad thunk and explicitly leaves this one alone.
• Verified with test/bundler/bundler_plugin.test.ts (plugin/ResolvePathStringConversionThrows, plugin/ResolveNamespaceStringConversionThrows) and test/js/bun/websocket/websocket-server.test.ts (send()/publish() with a string that cannot be flattened throw and send nothing, which covers ws.send, ws.sendText, ws.publish, ws.publishText and server.publish). Without the src change, a debug build aborts on the assertion in all three tests; the release canary fails the path test (success: true; the namespace variant depends on which of the two answers the bundler thread sees first) and the websocket test (five empty frames delivered before "done"). With it, all pass, also under BUN_JSC_validateExceptionChecks=1.
• Also run with the change: the rest of bundler_plugin.test.ts and websocket-server.test.ts, test/cli/install/semver.test.ts, test/js/node/zlib/zlib.test.js, test/js/node/util/util.test.js, test/js/bun/util/arraybuffersink.test.ts, test/js/bun/test/expect-extend-preload.test.ts, test/js/bun/udp/udp_socket.test.ts, test/js/node/http2/node-http2.test.js, and the node parallel tests for zlib.crc32, util.parseEnv and unhandled rejections. cargo check of bun_runtime for x86_64-pc-windows-msvc and aarch64-apple-darwin covers the two cfg(windows) call sites; cargo fmt and clippy are clean on the touched files.

Background

• Rope strings: JSC represents the result of string concatenation as a tree of fibers (JSRopeString) and only builds the flat character buffer when something needs to read it ("flattening", JSRopeString::resolveRope). Building a rope of length 2^31 - 1 only allocates tree nodes (the repro peaks around 30 MB RSS), while flattening it as 16-bit text needs a buffer larger than WTF::StringImpl allows (isValidLength<char16_t> caps it a few characters below String::MaxLength), so resolveRope throws a RangeError with the message Out of memory without attempting an allocation. It is an ordinary catchable exception about one oversized value, not a process-level allocation failure, which is why failing the call (or the build) is the proportionate response. An 8-bit rope of the same length would instead try a real 2 GB allocation, hence the \u0100 characters in the tests.
• Pending exceptions: JSC reports a throw by storing the exception on the VM; the C++ caller is expected to check it (RETURN_IF_EXCEPTION). On the Rust side this check is from_js_host_call_generic / TopExceptionScope, which turns a pending exception into Err(JsError::Thrown). A host function that returns normally with an exception pending is an assertion failure in debug builds; in release JSC throws it into the calling JS frame after the host function returns, which is what produced the second addError answer here.
• Plugin resolve requests: the bundler thread parks a Resolve and blocks on it until the JS thread answers exactly once by setting resolve.value and calling on_resolve_async, which hands it back to the bundler thread. JSBundlerPlugin__addError is the existing answer path for a callback that threw; plugin_msg_from_js converts the exception into the logger.Msg that on_resolve logs against the importing file.

Repro (entry.ts contains import "huge";)

let huge = "\u0100";
for (let i = 0; i < 30; i++) huge = huge + huge + "\u0100"; // length 2**31 - 1, never flattened by JS
const r = await Bun.build({
  entrypoints: ["./entry.ts"],
  throw: false,
  plugins: [{ name: "x", setup(b) { b.onResolve({ filter: /^huge$/ }, () => ({ path: huge, external: true })); } }],
});
console.log(r.success, r.logs.map(l => l.message));

external: true matters: with it the JS side never reads the string, so the first flatten happens in onResolveAsync. { path: "/x", namespace: huge, external: true } behaves the same.

Before, debug build:

ASSERTION FAILED: Unexpected exception observed on thread ...
Error Exception: Out of memory
!exception()
.../JavaScriptCore/ExceptionScope.h(62) : void JSC::ExceptionScope::releaseAssertNoException()

Before, release canary da3851e, single run: true []. The same build in a for loop of 40 iterations:

panic: Segmentation fault at address 0x0

(sometimes panic: infallible: runtime export, depending on which answer the bundler thread sees first).

After (debug and release): false [ "Out of memory" ], reported at entry.ts, 40 out of 40 iterations, and a following Bun.build in the same process succeeds.

Other callers, before the change on the release canary, ws.send(huge) delivered an empty text message to the client and then threw; after the change it throws without sending anything (websocket-server.test.ts asserts the client only receives the "done" sent afterwards). Bun.semver.order(huge, "1.0.0") aborted debug builds on the same assertion before and throws the RangeError now.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b87c94f3-c184-4b9e-8d0f-6c053ab6e7e6

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and 55b4524.

📒 Files selected for processing (20)

• src/jsc/JSGlobalObject.rs
• src/jsc/JSString.rs
• src/jsc/VirtualMachine.rs
• src/runtime/api/JSBundler.rs
• src/runtime/api/bun/h2_frame_parser.rs
• src/runtime/dns_jsc/dns.rs
• src/runtime/ipc.rs
• src/runtime/node/node_cluster_binding.rs
• src/runtime/node/node_util_binding.rs
• src/runtime/node/node_zlib_binding.rs
• src/runtime/node/util/validators.rs
• src/runtime/server/ServerWebSocket.rs
• src/runtime/server/server_body.rs
• src/runtime/socket/udp_socket.rs
• src/runtime/test_runner/expect.rs
• src/runtime/valkey_jsc/js_valkey.rs
• src/runtime/webcore/Sink.rs
• src/semver_jsc/SemverObject.rs
• test/bundler/bundler_plugin.test.ts
• test/js/bun/websocket/websocket-server.test.ts

---

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

[robobun]

Status: fix is up in this PR.

Reproduced on the current canary (da3851e) and on a debug build at 9a543cc with an onResolve plugin returning { path: <2^31-1 char 16-bit rope>, external: true }: the debug build aborts on releaseAssertNoException, the release build answers the resolve twice and segfaults when the build is repeated in a loop. With this branch both report Out of memory as a build error once and the VM stays usable.

Tests: test/bundler/bundler_plugin.test.ts (two new plugin/Resolve*StringConversionThrows cases) and test/js/bun/websocket/websocket-server.test.ts (send/publish case for the shared string helper); both fail without the src change and pass with it.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes a core JSC binding signature (JSString::to_zig_string → JsResult) across 16 caller files and adds bespoke double-answer handling in JSBundlerPlugin__onResolveAsync (the segfault fix), a human look would still be worthwhile.

What was reviewed:

• Every updated caller propagates via ? in a JsResult context, or returns the pending JsError directly (JSGlobalObject.rs, validators.rs) — no second exception layered on top.
• The two clear_exception() calls (VirtualMachine.rs, ipc.rs) match existing local fallback patterns for the same "could not read" case.
• onResolveAsync's new Err arm takes the exception before on_resolve_async, so runOnResolvePlugins no longer sees a pending throw and addError is not called a second time.
• Tests wire error/close to reject, assert no empty frames precede "done", and the rope construction is length-checked so the flatten is guaranteed to fail.

Extended reasoning...

Overview

This PR makes JSString::to_zig_string (and its wrappers get_zig_string, view, to_slice, to_slice_clone) return JsResult so that JSC's rope-flatten RangeError is observed rather than swallowed as an empty string with a pending exception. It updates 16 caller files across the runtime (bundler plugins, WebSocket server, HTTP/2, DNS, UDP, IPC, semver, zlib, util, valkey, sinks, test runner) and adds targeted tests for the two most consequential paths: bundler onResolve (the segfault) and WebSocket send/publish (the empty-frame leak).

The critical piece is JSBundlerPlugin__onResolveAsync: instead of .expect() on to_slice_clone, it now matches on the result, and on Err takes the exception via take_exception and answers with ResolveValue::Err(plugin_msg_from_js(...)) — the same shape JSBundlerPlugin__addError produces. This prevents the double-answer (once as Success with an empty string, then again as Err when the pending exception rejects runOnResolvePlugins) that caused segfaults in release builds.

Security risks

None identified. The change tightens error handling — inputs that previously silently degraded to empty strings now throw. No new trust boundaries, no parsing of untrusted data, no auth/crypto paths touched.

Level of scrutiny

High. This is a signature change to a core JSC string-conversion helper with ~30 call sites across memory-safety-adjacent subsystems (bundler thread synchronization, WebSocket frame writes, HTTP/2 header encoding). The bundler double-answer fix is the difference between a crash and a build error, and the take_exception placement is load-bearing. The two clear_exception() sites deserve a maintainer's eye given REVIEW.md's general guidance against clearing — both are justified (non-throwing fallback contexts matching existing local patterns in the same file), but that's exactly the kind of judgment call a human should sign off on.

Other factors

• The mechanical ? propagations are straightforward and each caller is already in a JsResult-returning host function.
• The JsError-returning helpers in JSGlobalObject.rs and validators.rs correctly return the flatten error directly rather than building an INVALID_ARG_TYPE on top of a pending exception.
• Test coverage is good: bundler tests assert build.success === false and the specific error location; the WebSocket test asserts both that all five variants throw RangeError: Out of memory and that only the sentinel "done" frame arrives. Failure paths (client error/close) reject the awaited promise.
• The PR description documents cross-target cargo check for the two cfg(windows) sites and BUN_JSC_validateExceptionChecks=1 runs.
• No prior human reviews on the timeline to consider.