webcore: propagate exceptions from readableStreamTee instead of reporting them as uncaught - #32786
Conversation
…ting them as uncaught ReadableStream__tee invoked the readableStreamTee builtin through a helper that, under ASSERT_ENABLED, passed any pending exception to Bun__reportError. readableStreamTee legitimately throws a TypeError when the stream is already locked, which is reachable from Request.clone() and Response.clone() on a body whose stream the user has called getReader() on. Routing that exception through the uncaught-exception handler had three bad effects in debug and asan builds: - the error was printed as if uncaught even though the caller catches it - the process exit code was set to 1 - the pending exception was cleared, so control fell through to the IDLSequence conversion on an empty JSValue, and clone() surfaced a bogus "Value is not a sequence" TypeError instead of the real one Also drop the EXCEPTION_ASSERT on the same line, which asserted the builtin can never throw a non-termination exception. Release builds were unaffected because the whole block was compiled out.
|
Reproduced on a debug build of Status: the diff is green. None of the CI red on build 65138 is caused by this PR. No lane reported a failure in
Ready for review. |
|
Warning Review limit reached
More reviews will be available in 21 minutes and 18 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The only hard failure on build 65126 was a buildkite-agent artifact download timeout on the windows-aarch64-build-bun step before any tests ran. The remaining red is auto-retried flake in bun-install-registry.test.ts and dev-server-ssr-100.test.ts, neither of which this diff touches.
There was a problem hiding this comment.
LGTM — the comment nit was addressed in 7d400b0, and the fix itself is a clean removal of an incorrect debug-only invariant.
Extended reasoning...
Overview
This PR removes 6 lines from ReadableStream__tee in src/jsc/bindings/webcore/ReadableStream.cpp: an #if ASSERT_ENABLED block that called Bun__reportError on any pending exception, plus the EXCEPTION_ASSERT that encoded the same false invariant (that readableStreamTee never throws non-termination exceptions). It replaces them with a 3-line comment and lets the existing RETURN_IF_EXCEPTION(scope, {}) propagate the exception. A new test.each(["Request", "Response"]) regression test in body-clone.test.ts spawns a subprocess to assert the caught error message and exit code.
Security risks
None. The change is purely subtractive in debug/asan builds and a no-op in release builds (the removed block was #if ASSERT_ENABLED, and EXCEPTION_ASSERT compiles away in release). No new code paths, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-medium. The C++ change only deletes diagnostic code that was actively wrong — readableStreamTee legitimately throws TypeError("ReadableStream is locked") when called via Request/Response.clone() on a locked body, so reporting it as uncaught (which clears the exception and sets exit code 1) was incorrect. Release builds already behaved correctly; this aligns debug/asan with release. The outer RETURN_IF_EXCEPTION(scope, false) in ReadableStream__tee already handles the propagated exception.
Other factors
- My prior inline nit (5-line comment → 3 lines per CLAUDE.md) was addressed in 7d400b0 and the thread is resolved.
- The new test follows the existing subprocess pattern already used in the adjacent test in the same file.
- CI failures (
scripts/build/ci.tsmusl build,node-tls-connect.test.ts) are unrelated to a debug-only assertion removal in the ReadableStream tee path. - No CODEOWNERS entries for the affected paths.
- Bug-hunting system found no issues.
|
A follow-up fuzz report surfaced a second symptom of the same two lines, recording it here since the PR body only covers the wrong-error one. If the script has installed a process.on("uncaughtException", () => {});
const w = new Request("http://h.invalid/x", {
method: "POST",
duplex: "half",
body: new ReadableStream({ start(c) { c.enqueue(new Uint8Array(8)); c.close(); } }),
});
w.body.getReader();
try { w.clone(); } catch {}Re-verified on main (163e6bc): removing the An equivalent variant that also folds the duplicated lambda into the file-level |
… body (#33129) ### Problem `Request.prototype.clone()` and `Response.prototype.clone()` never perform step 1 of the fetch spec's clone algorithms: "If this is unusable, then throw a `TypeError`", where unusable means the body is non-null and its stream is disturbed or locked (https://fetch.spec.whatwg.org/#body-unusable). ```js const q = new Request("http://x/", { method: "POST", body: "hello world" }); await q.text(); // body consumed const c = q.clone(); // node, Deno, browsers: TypeError. Bun: succeeds await c.text(); // "" (silent empty body) ``` What Bun does instead depends on the body's internal representation: - Consumed string, Blob, and FormData bodies: `clone()` succeeds and the clone resolves to an empty body. - Consumed or locked stream bodies: `clone()` succeeds and an error surfaces later, from the clone's own read or from the internal tee, never from `clone()` itself. The check exists so that clone-after-read, which is always a bug in the caller, fails loudly in development. Without it, proxy and retry middleware that does `clone()` then forwards the request silently forwards an empty body whenever the order of operations is wrong. Node (undici) throws a `TypeError` synchronously from `clone()` in each case: ``` $ node repro.mjs 1 req-consumed-string: clone() THREW TypeError: unusable 2 req-consumed-stream: clone() THREW TypeError: unusable 3 req-locked(unread): clone() THREW TypeError: unusable 4 res-consumed-string: clone() THREW TypeError: Response.clone: Body has already been consumed. 5 res-locked(unread): clone() THREW TypeError: Response.clone: Body has already been consumed. 6 res-consumed-blob: clone() THREW TypeError: Response.clone: Body has already been consumed. 7 req-consumed-formdata: clone() THREW TypeError: unusable $ bun repro.mjs # 1.4.0 and main 1 req-consumed-string: clone() OK, clone.text() -> "" 2 req-consumed-stream: clone() OK, clone.text() THREW TypeError: Body already used 3 req-locked(unread): clone() OK, clone.text() THREW TypeError: Body already used 4 res-consumed-string: clone() OK, clone.text() -> "" 5 res-locked(unread): clone() OK, clone.text() THREW TypeError: Body already used 6 res-consumed-blob: clone() OK, clone.text() -> "" 7 req-consumed-formdata: clone() OK, clone.text() -> "" ``` ### Fix Add `BodyMixin::throw_if_body_unusable` and call it at the top of both `do_clone` entry points (`src/runtime/webcore/Request.rs`, `src/runtime/webcore/Response.rs`). Request has a third JS entry point: `BunRequest.prototype.clone`, the subclass that `Bun.serve` `routes:` handlers receive. It dispatches through `JSBunRequest::clone` -> `Request__clone` -> `Request::ffi_clone` rather than `do_clone`, so it gets the same check at the top of `ffi_clone`. Response has no such subclass. `Request::clone` and `Response::clone` have no other callers, so every JS-visible `clone()` is covered and no internal clone path changes. The unusable predicate is the existing `bodyUsed` walk with `ReadableStream::is_locked` OR'd in, so `get_body_used` is refactored to share a parameterized helper (`body_stream_check`) instead of duplicating the match. The error is `ERR_BODY_ALREADY_USED`, an instance of `TypeError` like node and browsers, with the message `Body is disturbed or locked` (WebKit's wording, which covers both halves of the predicate). Only the two `clone()` entry points are guarded. `new Request(usedRequest)` has the same spec check (Request constructor step 36.1) and is also missing in Bun, but it is a separate algorithm with a much wider blast radius in `Bun.serve` middleware, so it is intentionally not part of this PR. `fetch(usedRequest)` already throws. ### Tests `test/js/web/fetch/body-clone.test.ts`: - New `describe("clone() throws when the body is disturbed or locked")`: consumed string / user-stream / Blob / FormData bodies, locked bodies, an in-flight read, a `fetch()` response disturbed by a reader, a consumed `Bun.serve` incoming request (catch-all `fetch` handler), and a consumed `BunRequest` from a `routes:` handler (both a `/:param` route and a static one). All fail on `main`. - Negative coverage: null bodies, an unread `Bun.serve` request (both handler kinds), and a body materialized by the `body` getter still clone. - The existing "`clone()` on a locked stream body throws a catchable TypeError" subprocess test now asserts the new message. Since the usability check fires before the stream is teed, that test no longer reaches the `readableStreamTee` exception-propagation path it was added for in #32786, so a sibling test drives the same path through `new Request(lockedRequest)`, which still tees, and keeps that coverage. With the fix, `bun bd test test/js/web/fetch/body-clone.test.ts` passes 44/44. The surrounding `body.test.ts`, `response.test.ts`, `body-stream.test.ts`, `blob.test.ts`, and `test/js/web/request/` suites are unchanged.
2 participants
Problem
Calling
.clone()on aRequestorResponsewhose body stream is locked is supposed to throw a single, catchableTypeError. InASSERT_ENABLEDbuilds (debug, and the asan release lane) it instead did three wrong things at once:Release builds behaved correctly (
caught: ReadableStream is locked, exit 0).Response.clone()hits the same path.Cause
ReadableStream__teeinvokes thereadableStreamTeebuiltin through a local helper insrc/jsc/bindings/webcore/ReadableStream.cpp. That builtin throws aTypeError("ReadableStream is locked")synchronously when the stream is already locked, which is exactly whatRequest.clone()relies on.The helper contained this after the call:
Bun__reportErrorroutes throughVirtualMachine::uncaught_exception, which prints the error, sets the process exit code to 1, and clears the pending exception. With the exception gone,RETURN_IF_EXCEPTIONno longer fired, so control reachedconvert<IDLSequence<...>>on the emptyJSValuereturned by the failedJSC::call, which is where the bogus"Value is not a sequence"came from.The
EXCEPTION_ASSERTon the next line encodes the same false invariant (thatreadableStreamTeecan never throw a non-termination exception); without theBun__reportErrorclearing the exception it would have aborted instead.Fix
Drop both lines and let
RETURN_IF_EXCEPTIONpropagate the exception.ReadableStream__teealready returnsfalsewith the exception pending, and the Rust caller (from_js_host_call_genericinReadableStream::tee) already converts that into anErrthat reaches the user'scatch. This makes debug and asan builds match what release builds already did.ReadableStream.prototype.tee()was never affected: it goes through the JS builtin directly and does not use this C++ wrapper.Verification
New
test.each(["Request", "Response"])cases intest/js/web/fetch/body-clone.test.tsspawn a child, lock the body,clone(), and assert the exact caught message and exit code 0.Before (debug build):
After: both pass, and the existing
body-clone.test.ts(27),body.test.ts/body-stream.test.ts/body-mixin-errors.test.ts(9434), andstreams.test.jssuites still pass.