webcore: clone() keeps the Blob behind an unread native body stream instead of teeing it - #42053
Conversation
…b instead of teeing it Response.clone() and Request.clone() teed every stream-backed body through JS. When the stream was an unread Bun.file() or Blob stream, both branches became plain JS streams: the file store, its type, and the sendfile path were gone for the clone and for the original. The readers and Bun.serve already move such a stream back into its Blob; clone() now does the same and dupes that Blob. The stream object is detached, so a reference the caller holds reads as locked, as after a tee. JS streams and partly read bodies are still teed.
… path), dupe the rest A path that stat() says is not a regular file (a FIFO) hung on the second open when clone() duped it, and a Blob body over Bun.stdin or Bun.file(fd) already lost its bytes to whichever body read first. One predicate, store_reads_repeatably(), now decides dupe versus tee for both the stream arm and the Blob arm of clone().
|
Warning Review limit reached
On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file. Or wait 1 minute for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 1:14 PM PT - Sep 8th, 2026
❌ @robobun, your commit 9f4a7cf has 3 failures in
🧪 To try this PR locally: bunx bun-pr 42053That installs a local version of the PR into your bun-42053 --bun |
|
Reproduction (bun 1.4.3, Linux x64), before this change: import fs from "node:fs";
const d = fs.mkdtempSync("/tmp/ct-");
fs.writeFileSync(d + "/a.html", "<p>hi</p>");
const mk = () => new Response(Bun.file(d + "/a.html").stream());
console.log((await mk().blob()).type); // "text/html;charset=utf-8"
console.log((await mk().clone().blob()).type); // "" (this PR: "text/html;charset=utf-8")
const s = Bun.serve({ port: 0, fetch: req => (new URL(req.url).pathname === "/clone" ? mk().clone() : mk()) });
for (const p of ["/direct", "/clone"])
console.log(p, (await fetch(new URL(p, s.url))).headers.get("content-type"));
// /direct text/html;charset=utf-8
// /clone null (this PR: text/html;charset=utf-8)
s.stop(true);And the fd case, piping const r = new Response(Bun.stdin);
const c = r.clone();
console.log(await Promise.all([r.text(), c.text()])); // [ "hello world", "" ] (this PR: both "hello world")The new tests in CI (build 113145): |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks Body::clone() semantics — including stream lifecycle (force_detach, cached .body/.stream slot clearing) and a per-store dupe-vs-tee decision that the PR itself flags has an order-dependent header side effect and one open "maintainer call" — a human look would still be worthwhile.
What was reviewed:
store_reads_repeatably: fd → always false, path stat'd on demand, missing path stays repeatable — matches the stated intent andresolve_file_stat.take_blob_from_unread_stream: guards onpromise/on_receive_value, checks the file-reader store beforeto_any_blob, thenforce_detach+readable.deinit()— mirrors existingto_any_blob_allow_promiseshape.sync_body_stream_caches: the non-Lockedarm clears both cached slots so.bodyrebuilds from the Blob; both call sites (sync_cloned_body_stream_caches,clone_body_value_via_cached_stream) were previously duplicated and now share this.- Tests:
port: 0, harness helpers, subprocess pipes drained viaPromise.all, stdout asserted before exit code, FIFO case gatedskipIf(isWindows).
Extended reasoning...
Overview
This PR changes how Request/Response .clone() handles bodies backed by native Blob/file stores. In src/runtime/webcore/Blob.rs it adds store_reads_repeatably() to classify a store as safely re-readable (in-memory bytes, S3, regular file by path) versus not (any fd, or a non-regular file by path — stat'd on demand). In src/runtime/webcore/Body.rs, Value::clone_with_readable_stream() now (a) for a Locked body, first tries take_blob_from_unread_stream() to move an unread native/file/byte stream back into its Blob (force-detaching the caller-visible stream so it reads as locked) before falling back to the existing tee, and (b) for a Value::Blob over a non-repeatable store (pipe/fd/FIFO), converts to a ReadableStream and tees so both bodies see all bytes. The cached-slot resync is factored into sync_body_stream_caches() which additionally handles the new "reverted to Blob" state by zeroing both .stream and .body cache slots. Thirteen tests are added to test/js/web/fetch/body-clone.test.ts covering content-type preservation through clone (including via Bun.serve), the locked-original-stream contract, and stdin/FIFO tee behavior.
Security risks
No direct security surface. There is no untrusted-input parsing, no auth/crypto, no path traversal exposure — the added stat() is on a path the caller already handed to Bun.file(). The change is purely about which of two existing body-duplication strategies (Blob dupe vs. JS tee) is chosen per store kind.
Level of scrutiny
Medium-high. While the individual pieces reuse established helpers (to_any_blob, force_detach, readable.deinit(), resolve_file_stat), the change sits at the intersection of several subtle mechanisms: JSC GC-tracked cache slots (body_set_cached/stream_set_cached), Strong/weak readable-stream ownership migration via check_body_stream_ref, and the Value state machine. The PR description itself flags two things a maintainer should weigh: (1) an intentional non-change to Value::from_js eagerness for file streams because making it eager would regress fetch() upload streaming, and (2) a known order-dependent side effect where calling .clone() before the first .headers access now surfaces a content-type header that would not have appeared otherwise. Adding a synchronous stat() to .clone() for path-backed file bodies is also a small performance trade-off. These are design judgments, not bugs, but they are exactly the kind of decision REVIEW.md says to pre-emptively justify to a human.
Other factors
Test coverage is good and follows repo conventions: harness helpers (bunExe/bunEnv/tempDirWithFiles), port: 0, concurrent pipe draining with Promise.all, output asserted before exit code, .toEqual on structured objects, skipIf(isWindows) on the mkfifo case. The Value::Blob → tee path passes None for owned_readable, which is correct since a Value::Blob entry cannot have a live cached .body stream (the .body getter would have already converted it to Locked). No CODEOWNERS coverage on the changed paths. The exit reason was dry_streak, so the hunt ran to completion. Given the lifecycle subtlety and the author-flagged open questions, deferring to a human is the right call rather than approving outright.
… empty Blob from a closed stream - take_blob_from_unread_stream no longer detaches by hand: to_any_blob marks the stream consumed itself. - Bun.readableStreamTo*() on a stream a Body already consumed rejects with 'has already been used' rather than 'is locked'. - A closed, never-read stream lifts back into the store-less empty Blob that new Response(new Blob([])) holds, so json() on a touched empty body still rejects with a SyntaxError instead of resolving null. - Trim comments; wire socket error events in the new tests.
### Problem - `new Response(p.body, p)` adopted a JS-backed stream but pulled the Blob out of a blob-backed one (`Value::from_js`): `p` came out locked and used, and `r.body !== p.body`. - After `text()`/`json()`/`blob()`/... the body's stream was unlocked and `getReader()` worked. undici and Chromium keep it locked. - Zero-length string/`Uint8Array` bodies were never used up: `blob()` left `bodyUsed` false, and `.body` handed out a stream the body did not track. ### Fix - `Value::from_js` adopts every stream. `to_blob_if_possible` still lifts a blob/file-backed stream back into a blob when the body is consumed, served, or uploaded, so the Content-Length framing stays. - New `m_consumedAsBody` bit on `JSReadableStream`, part of `nativeHandleDetached()` and so of `isReadableStreamLocked()`. `set_promise` sets it once the consumer has started, `to_any_blob` after taking a native source's bytes. `Bun.readableStreamToText()` is unchanged. - `.body` on `Empty` stores its stream like the other arms, `use_()` marks `Empty` used, `to_any_blob` turns a closed never-read stream into an empty blob, the getters reject a locked stream up front, and `Response.redirect()`/`error()` get a null body. - Verified: `test/js/web/fetch/body.test.ts` (145 new cases, 128 fail on 1.4.3, all checked against node v26.3.0), plus the suites in Notes. Self-reviewed: 4 concerns, 3 addressed, the #33461 overlap is noted below. ### Background - A body is a `Body::Value`: string, `Blob`, bytes, `Empty`, `Null`, or `Locked` (a stream). Reading `.body` makes a non-null body `Locked`. - Blob- and file-backed streams keep a native source. Until something reads them, `to_any_blob` can take the payload back without running the stream. The fetch spec reads a body through a reader it never releases, so a consumed body's stream stays disturbed and locked. <details><summary>Notes</summary> - Ledger members: #44053 (eager transfer), #44054 and #44171 (unlocked after consume, JS-stream close and error paths), #44055 (zero-length bodies). Not in this PR: #44057 is covered by #33499, and the "`getReader()` alone marks a native body used" cascade (#921) by #33461. #44059, #44060 and #44061 are separate mechanisms. - Overlap with #33461: both touch the body getter prologues, `ReadableStream::to_any_blob`'s guard and `Value::from_js`. If this lands first, #33461 keeps its `m_nativeSourceMaterialized` gating and drops its getter and `from_js` hunks on rebase. `to_any_blob` then wants `is_native_source_consumed || is_locked` as its guard. - `ReadableStream__detach`/`force_detach` had no other caller and is removed. `m_consumedAsBody` takes over both halves of what the `-1` handle sentinel did there: the stream reads as locked, and its native handle is neither started by `getReader()` nor handed to `Readable.fromWeb()`'s fast path. Unlike the sentinel it leaves `m_nativePtr` alone, so the handle stays rooted while an async consumer runs. - `Readable.fromWeb()` now throws `ERR_INVALID_STATE` for any locked stream before it does anything else, as Node does (Node acquires the reader at that point). Before, a locked native-backed stream had its handle taken anyway. - `ReadableStream__isClosedUnread`: `ReadableStream{Default,Byte}ControllerClose` only moves a stream to `Closed` once its queue is empty, so `Closed && !disturbed && !locked` means the stream can never yield a byte. This keeps a touched empty body (`new Response(""); r.body`) framing and typing exactly like an untouched one, and `new Response(new Blob([]))` takes the same path. - A locked (not disturbed) body stream now rejects from the getter with `TypeError: Invalid state: ReadableStream is locked`, the same error the C++ helper produced before, and no longer records a pending read first. For JS-stream bodies `getReader(); releaseLock(); await r.text()` works and `bodyUsed` stays false while only locked, as in undici. Native-backed bodies still mark themselves disturbed on `getReader()`; that is #33461's subject. - `fetch()` upload framing: a blob/bytes-backed or closed-empty stream body goes out with a Content-Length (as 1.4.3 did for the blob case through the eager transfer, and as undici does for bodies whose source it knows). A file-backed stream keeps streaming chunked, as today, because its length may not be knowable (FIFO, device). A JS stream streams chunked. - `Response.redirect()`, `Response.error()` and the S3 `new Response(s3file)` redirect used `Value::Empty`. The spec body is null. With `Empty` now tracked like any other body they would have become visibly one-shot, so they are `Value::Null` here (the same three-line change sits in #33125). - `Bun.readableStreamToText()` and the other helpers on a stream a Body already consumed reject with `ERR_INVALID_STATE` "ReadableStream has already been used" (a stream held by someone else's reader still says "is locked"). `test/js/web/streams/readable-stream-blob-consumed.test.ts` asserted `ERR_BODY_ALREADY_USED` from the old blob-loader path and is updated; its point (no crash, a rejected promise) is unchanged. - Rebased onto #42053: its `take_blob_from_unread_stream` used `force_detach`; `to_any_blob` now marks the stream consumed itself. - Suites run on the debug build: `body.test.ts`, `body-stream.test.ts` (9086), `body-clone`, `body-mixin-errors`, `body-async-iterator`, `body-stream-excess`, `serve.test.ts`, `bun-server`, `bun-serve-static`, `bun-serve-file`, `bun-serve-body-json-async`, `serve-if-none-match`, `proxy.test.ts`, `cookie.test.ts`, `html-rewriter.test.js`, `bun-write.test.js`, `spawn-stdin-readable-stream`, `streams.test.js`, `readable-stream-blob-consumed`, `native-source-onclose-leak`, `sync-pull-fast-path`, `request.test.ts`, `response.test.ts`, `client-fetch`, `content-length`, `fetch.stream`, `fetch.test.ts`, `fetch-abort-stream-body`, `fetch-keepalive`, `fetch-backpressure`, `node-stream.test.js`, `direct-readable-stream`, the node `test-readable-from-web-*` files, regression 07001 and 09555. The failures left in `fetch.test.ts`/`serve.test.ts`/`bun-server`/`fetch-backpressure` are environment-only here (IPv6, running as root, no internet or S3 egress, ASAN timeouts, and the ASAN RSS bound in "bounds memory when a handler forwards req.body") and reproduce with `origin/main`'s `src/`. </details>
ReadFile skipped the read when its fstat said st_size 0 and the store's cached mode said regular file. That cache is only filled once something stats the shared store, which clone() does since #42053 through store_reads_repeatably. new Response(Bun.file('/proc/version')).clone() then made both copies and the Bun.file itself read as "": a procfs file is a regular file with st_size 0 and real content. Drop the shortcut and read to EOF. A truly empty file costs one read() that returns 0.
ReadFile skipped the read when its fstat said st_size 0 and the store's cached mode said regular file. That cache is only filled once something stats the shared store, which clone() does since #42053 through store_reads_repeatably. new Response(Bun.file('/proc/version')).clone() then made both copies and the Bun.file itself read as "": a procfs file is a regular file with st_size 0 and real content. Drop the shortcut and read to EOF. A truly empty file costs one read() that returns 0.
2 participants
Problem
new Response(Bun.file("a.html").stream())answersblob().type === "text/html;charset=utf-8"and Bun.serve sends that Content-Type. After.clone()both bodies answer""and Bun.serve sends none. ABun.file()or typedBlobbody loses it too once.bodywas observed first.Value::clone_with_readable_stream(src/runtime/webcore/Body.rs) tees every stream-backed body through JS, so the store behind an unread native stream (MIME type, sendfile path) is gone on both sides. The Blob arm has the mirror bug:new Response(Bun.stdin).clone()dupes a Blob over fd 0 and the clone reads"".Fix
clone()decides per store. A store that reads the same twice (memory, S3, a regular file by path) is duped. An unread native stream first moves back into its Blob throughReadableStream::to_any_blob, as the readers and Bun.serve already do. A store that yields its bytes once (any fd, a FIFO by path) is read as one stream and teed. JS streams and partly read bodies tee as before..bodyis cleared and rebuilt from the Blob (the fetch: tee the body stream in clone() instead of detaching the cached .body #33779 guarantees hold).test/js/web/fetch/body-clone.test.ts(13 new tests, 6 fail on stock bun) and the neighbouring body, response, request, FormData, blob, serve and fetch suites. Self-reviewed: 2 concerns raised, both addressed.Background
Valueis aBlob(memory,Bun.file()store, S3), a string or byte buffer, orLocked(aReadableStream).new Response(stream)isLocked, as is any body once.bodyran.Store.to_any_blobturns it back into a Blob without reading it..bodyand its stream. A clone that changes what the body holds must resync both (fetch: tee the body stream in clone() instead of detaching the cached .body #33779).Notes
new Response(Bun.file(p).stream()): type recovered for the direct return, dropped by.clone(); Bun.serve/direct text/html,/clone null". Expected: clone == original.store_reads_repeatably()(Blob.rs, next toresolve_file_stat): Bytes and S3 are repeatable; aFileover an fd never is (reads share the fd offset, and a pipe's bytes are gone once read:new Response(Bun.file(fd)).clone()read""on the second body even for a regular file); aFileover a path is stat'd once ifseekableis unknown and is repeatable unless stat says it is not a regular file. A missing path stays repeatable (both bodies fail the same way).new Response(Bun.file(fifoPath).stream()).clone()hung on the second open while stock's tee delivered the bytes to both;new Response(Bun.stdin).clone()gives["hello world", ""]on stock. Both are tests now (posix for the FIFO).new Response(new Blob([..], { type }).stream())puts the type into the header list at construction (undici:null) becauseValue::from_jsmoves a memory-blob stream into its Blob eagerly, while a file stream staysLockeduntil first use, so its headers staynull. Making file streams eager too would routefetch(url, { body: Bun.file(p).stream() })through the file-blob upload path (a synchronous whole-file read over https,fetch.rs"TODO: make this async + lazy") instead of a streamed upload, so it is not done here.Responsederives a missingContent-Typeheader lazily from a Blob body on first.headersaccess. Fornew Response(file.stream()), calling.clone()before the first.headersaccess therefore makescontent-type: text/htmlappear on both. Header derivation is already order-dependent today (.bodyor.text()before.headersdrops it, see webcore: fix Content-Type lost when Request/Response body is read before headers #32913); this adds no new mechanism.request-clone-leak.test.tsandrequest-method-getter.test.tstime out at 5 s per test on this build for the constructor-only cases too; unrelated.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/body-clone.test.ts