Response.clone(): share tee chunk refs instead of StructuredClone per branch - #35843
Conversation
|
Status: diff is green. All review feedback addressed through 39c18f8; CI build 82012: 135 jobs passed, 5 Repro: 10 MB streaming body cloned 100 deep: Ready for review/merge; needs a maintainer to push past the build-lane timeouts. |
WalkthroughChangesStream-specific structured-clone bindings were removed. Readable stream teeing now shares materialized chunks between branches. Fetch tests verify identity, concurrent consumption, and bounded memory use across deep clone chains. Stream clone removal
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:56 PM PT - Jul 25th, 2026
❌ @robobun, your commit d28dea9 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35843That installs a local version of the PR into your bun-35843 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nits, I confirmed ReadableStream__tee is only reached via the Rust Body clone path (ReadableStream.rs), so no other consumer's semantics change; and readableByteStreamTee takes no cloneForBranch2 parameter, so byte-controller tee is unaffected as the description states. Deferring to a human because this is an observable spec deviation on a Web API (chunk mutations now alias across branches) — well-justified against Node/Chrome/Firefox/Deno, but worth a maintainer sign-off.
Extended reasoning...
Verified the two claims the correctness of the one-line change rests on: (1) grep shows ReadableStream__tee is declared in WebStreamsInternals.h, defined in WebStreamsExports.cpp, and called only from src/runtime/webcore/ReadableStream.rs's tee(), which is the body-clone helper — so flipping cloneForBranch2 here cannot affect any path other than Request/Response body cloning; (2) readableByteStreamTee has no cloneForBranch2 parameter, so type: "bytes" streams keep their per-spec copy behavior. The two inline findings (now-dead structuredCloneChunk/m_shouldClone path; the backing assertion in the rewritten fetch test no longer measures the clone branch) cover the remaining review feedback.
|
@robobun fix the review comments but also show a benchmark wiht memory usage comparisons for bun, node, and deno in the PR body for the same code. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@Jarred-Sumner done. Review comments addressed in 25f5387 and 6c4deb9 (dead Measured RSS delta for a 10 MB streaming body, reading only the last clone in the chain:
Deno turns out to implement the spec's per-chunk StructuredClone, so it shows the same growth bun did; I've corrected the code comments accordingly. Node shares the chunk reference and stays roughly flat; bun now enqueues the same |
There was a problem hiding this comment.
All three rounds of my earlier feedback are addressed in 25f5387/6c4deb9/39c18f8 and no new issues surfaced this pass. Deferring only because this is a user-observable spec deviation on a Web API (cloned branches now share chunk references, so mutating a chunk in one branch is visible in the other) — worth an explicit maintainer sign-off on that semantic.
What was reviewed:
- Confirmed
readableStreamTee/readableStreamDefaultTeehave exactly the two updated call sites; no stragglers with the oldboolarg. - Confirmed the only remaining
cloneArrayBuffersymbol is the unrelated file-static in JSReadableByteStreamController.cpp. m_shouldClone,structuredCloneChunk, and both private-global plumbing chains are fully gone.- New RSS test branches on
isASAN/isDebugwith a bound (80/120 MB) well below the ~200 MB unfixed baseline.
Extended reasoning...
Overview
The PR flips Response.clone()/Request.clone()'s tee from cloneForBranch2 = true (StructuredClone every chunk into branch2) to sharing the same JSValue in both branches, matching Node/Chrome/Firefox and fixing O(depth × bytes) retention on clone chains. Follow-up commits deleted every piece of the now-dead cloneForBranch2 = true machinery: structuredCloneChunk, JSStreamTeeState::m_shouldClone, the $structuredCloneForStream and $cloneArrayBuffer private globals (host functions, builtin names, GlobalPropertyInfo entries, .d.ts declarations), and the bool cloneForBranch2 parameter itself.
Security risks
None identified. Sharing chunk references between tee branches means user code can mutate a chunk and observe it in the sibling branch, but that's already the behavior of ReadableStream.prototype.tee() and of Node/Chrome/Firefox for body clones — it's not a new capability.
Level of scrutiny
Medium-high. The core change is one line, but it's an intentional spec deviation (fetch spec says cloneForBranch2 = true; whatwg/streams#1156 tracks the disagreement) with user-observable semantics, and it triggered ~150 lines of dead-code deletion across the C++ streams implementation and private-global plumbing. Jarred has been directing the follow-up work but hasn't formally approved; the semantic change deserves an explicit maintainer ack.
Other factors
All feedback from my three prior review passes is addressed. Repo-wide grep confirms no orphaned callers of the resignatured tee functions and no remaining references to the deleted private globals beyond the unrelated file-static cloneArrayBuffer in JSReadableByteStreamController.cpp. The new RSS regression test correctly branches its threshold on isASAN/isDebug per REVIEW.md, and the reworked fetch test asserts reference identity (toBe) which is the strongest invariant for this change. The comment-cop bot's flags on the spec-deviation comments were reasonably rebutted — they document a deliberate upstream deviation with a citation, which is exactly what REVIEW.md says comments should carry.
There was a problem hiding this comment.
All three earlier review nits are addressed as of 39c18f8; nothing new found this pass. Deferring because this is an intentional spec deviation on a user-facing Web API (both clone branches now receive the same chunk object, so a mutation on one is observable on the other) — worth a maintainer sign-off on the semantics.
What was reviewed:
- No dangling references to the deleted
$cloneArrayBuffer/$structuredCloneForStreamplumbing; the survivingcloneArrayBufferin JSReadableByteStreamController.cpp is the unrelated file-static helper. - Both
readableStreamTeecallers (prototype.tee()andReadableStream__tee) match the new parameterless signature. readableByteStreamTeestill copies viacloneAsUint8Array, sotype: "bytes"tee behavior is unchanged.
Extended reasoning...
Overview
The PR changes Response.clone() / Request.clone() on a stream body to share chunk references between tee branches instead of structured-cloning each chunk into branch2. The one-line semantic change is in ReadableStream__tee (WebStreamsExports.cpp), and the rest is deletion of the now-unreachable cloneForBranch2=true machinery: structuredCloneChunk, m_shouldClone, the $structuredCloneForStream and $cloneArrayBuffer private globals, their builtin-name entries, and the cloneForBranch2 parameter on readableStreamTee / readableStreamDefaultTee. Tests in body-clone.test.ts now assert reference identity between branches and add an RSS-bounded 50-deep clone-chain regression test.
Security risks
None identified. The change strictly removes a per-chunk allocation/copy path; no new user-controlled data reaches native code, and the byte-controller tee (which handles BYOB views and buffer transfers) is untouched. Sharing a Uint8Array between two JS consumers of the same body is what ReadableStream.prototype.tee() already did.
Level of scrutiny
Medium-high. The C++ diff is almost entirely deletions and a signature simplification, which is low-risk mechanically, and I verified every deleted symbol has no remaining references. But this is a deliberate deviation from the fetch spec's "clone a body" step (specced as cloneForBranch2 = true), aligning with Node/Chrome/Firefox per whatwg/streams#1156. That's a user-observable behavior change — code that mutates a chunk from one clone branch will now see the mutation in the other — and REVIEW.md's "Never change a Bun-native default to fix Node compatibility" principle makes Web-API semantic shifts a maintainer call.
Other factors
All three of my earlier inline comments (dead cloneForBranch2=true path, vacuous backing assertion in the fetch test, orphaned $cloneArrayBuffer global) were addressed across 25f5387 / 6c4deb9 / 39c18f8, and the comment-cop flags were resolved. The maintainer is already engaged (requested and received the cross-runtime benchmark table), so deferring rather than approving keeps the sign-off with them. The new RSS test branches its threshold on isASAN/isDebug with ~80 MB of headroom below the pre-fix ~200 MB, which looks reasonable per the leak-test guidance.
… branch ReadableStream__tee (the extern called from Rust for Request/Response body clone) passed cloneForBranch2 = true, which StructuredClone-copies every chunk into the second tee branch. An N-deep clone chain therefore retained N independent copies of every body chunk: a 10 MB streaming body cloned 100 times held ~1 GB of duplicated buffers. Node (undici), Chrome, Firefox, and Deno all share the chunk reference between tee branches when cloning a body. The fetch spec text asks for cloneForBranch2 = true but no engine ships that; see whatwg/streams#1156. This flips the extern to cloneForBranch2 = false. The cloned branch now receives the same Uint8Array the original branch does, so a clone chain retains one copy of the body regardless of depth. The #33130 tests that asserted exact-size copies are updated to assert reference identity, which is the stronger invariant (zero copies rather than right-sized copies).
With both callers of readableStreamTee passing cloneForBranch2 = false, the default tee's per-chunk StructuredClone branch is unreachable. Remove structuredCloneChunk(), the m_shouldClone bitfield and if-block, the $structuredCloneForStream private global + host function, and the CloneMode::Full branch in StructuredClone.cpp that only it reached. Keep the cloneForBranch2 parameter on readableStreamDefaultTee to mirror the spec signature (asserted false). Also drop the backing-buffer-sum assertion from the fetch() clone test; it now measured branch1's receive buffers rather than anything the clone path controls. The per-chunk b.value === a.value check is the load-bearing invariant. Correct the Deno claim in comments: Deno passes cloneForBranch2 = true like the spec.
No caller passes true after the previous commit; remove the parameter from readableStreamTee / readableStreamDefaultTee and both call sites rather than keeping it with a comment explaining why it's unused.
The previous commit refactored cloneArrayBufferImpl into the cloneArrayBuffer host function, but $cloneArrayBuffer has had no JS-builtin caller since the streams implementation moved to C++. Delete the host function, its header declaration, builtin name, private-global registration, and type declaration.
d28dea9 to
0edb264
Compare
There was a problem hiding this comment.
All three rounds of earlier feedback are addressed (dead cloneForBranch2=true path, m_shouldClone, structuredCloneForStream/cloneArrayBuffer plumbing all removed; fetch test tightened to reference-identity). I didn't find any correctness issues, but leaving final sign-off to a maintainer since this is a user-observable spec deviation on Response/Request.clone() (chunk identity is now shared across tee branches).
What was reviewed:
- Confirmed both
readableStreamTeecallers updated and no stale references to the deleted symbols remain (the survivingcloneArrayBufferinJSReadableByteStreamController.cppis an unrelated file-static helper). defaultTeeChunkStepsMicrotasknow enqueues the samechunkinto both controllers; byte-tee'scloneAsUint8Arraypath is untouched.- New RSS test branches its threshold on
isASAN/isDebugand sits well below the ~200 MB pre-fix baseline; the comment/threshold mismatch flagged by a finder was checked and is fine.
Extended reasoning...
Overview
The PR flips ReadableStream__tee (the Rust FFI entry used by Body clone) from cloneForBranch2 = true to sharing chunk references, matching Node/undici, Chrome, and Firefox. With both callers now passing false, the entire clone-for-branch2 machinery is deleted: structuredCloneChunk(), JSStreamTeeState::m_shouldClone, the structuredCloneForStream and cloneArrayBuffer host functions, their BunBuiltinNames.h / ZigGlobalObject.cpp / builtins.d.ts plumbing, and the cloneForBranch2 parameter itself. Tests are updated from copy-equality to reference-identity assertions and a 50-deep clone-chain RSS regression is added.
Security risks
None identified. No untrusted-input parsing, auth, or crypto is touched. The change removes an allocation/copy path; the only new observable is that mutating a chunk read from one clone branch is now visible to the other — the same semantics Node and browsers already ship, discussed in whatwg/streams#1156.
Level of scrutiny
Moderate-to-high. The implementation itself is a small, well-understood simplification (delete a branch, enqueue the same JSValue twice), and the dead-code removal is mechanical — I grepped for every deleted symbol and confirmed no remaining references outside the unrelated file-static cloneArrayBuffer in JSReadableByteStreamController.cpp. What warrants a maintainer's eye is the decision: this is a deliberate deviation from the fetch spec's "clone a body" step on a public Web API, and while it aligns with every other major runtime, it changes user-observable behavior (clonedChunk === originalChunk where it was previously a fresh copy).
Other factors
Jarred already engaged on the thread and directed the follow-up work (benchmark table, review-comment fixes), so the direction has implicit maintainer awareness. All three of my earlier inline comments are resolved in 25f5387 / 6c4deb9 / 39c18f8. The bug-hunting system found nothing this run; one finder flagged the RSS test's "well under 50 MB" comment vs. the 80/120 threshold, which verifiers correctly ruled out — the threshold is set well below the ~200 MB unfixed baseline per REVIEW.md's leak-test guidance. CI is green on the diff; the failing lanes are pre-existing build timeouts and known flakes unrelated to this change.
### Problem - GitHub closes only the first reference after a keyword, so "Fixes #1, #2" leaves #2 open. "Supersedes #3" links nothing, and no reference closes a pull request. - The last 1000 merged PRs name 274 such references. PR #32292 is open although merged #36135 says "Supersedes #32292". ### Fix - `.github/workflows/close-linked-issues.yml` runs on `pull_request_target` `closed` (a merge into the default branch of `oven-sh/bun`) and on `workflow_dispatch` with a PR number and `dry_run`. Everything is inline in one `actions/github-script` step, with no checkout. - Each open target is closed as `completed` with the comment "Closed as completed by #N." or "Superseded by #N.". Closed or missing targets, the PR itself and other repositories are skipped. - The parser has no regex. A closing keyword (close, fix, resolve, supersede, replace, any tense) must lead the reference, alone or in a list. A negated, hedged or noun keyword, or one whose subject is another reference, does not count ("may fix", "the rm fix #1", "#100 supersedes #1"). - Verified: `test/internal/close-linked-issues.test.ts` (333 cases) runs the YAML's script against fake `github`, `context` and `core`. Also the 1000-PR parse (Notes). ### Background - GitHub's own keywords are close, fix and resolve (-s, -ed). Each links one reference, and only a merge into the default branch closes it. - `pull_request_target` runs in the base repository with a write token, also for fork PRs. That is safe only when no PR-controlled code runs. Here the description is the only PR input, parsed as text. <details><summary>Notes</summary> A close through the API does not create the "closed this in #N" timeline link that GitHub makes for its own closes. The comment carries the PR number instead. How the parser was calibrated. I pulled the descriptions of the last 1000 merged PRs and listed every line with a keyword next to a reference. The keyword families, list shapes and reference forms in the script are the ones that appear there. A reference is `#1`, `owner/repo#1`, an issue or pull URL (bare or in `<>`), or a markdown link. Four lines would have been wrong with a plain keyword-then-reference rule, and each led to a rule: - "the open `rm` fix #37521" (#38379): "fix" as a noun. Base forms (fix, close, resolve, supersede, replace) count only at the start of a sentence or line, or after will, should, does, and, and a few similar words. "to" is not one of them ("unable to fix #1", "how to fix #1"). - "May also fix #12318 / #10046, untested" (#38242): hedged. may, might, could, would, partially and the negations disqualify the keyword, looking past adverbs such as "also". - "Supersedes the closed #26040" (#36289) and "a comment on closed #35351" (#35365): "closed" as an adjective. A determiner or preposition before the keyword disqualifies it. - "supersedes #33130's optimisation" (#35843): a number that continues into a word is not a reference. Review added: a reference before the keyword is the subject ("#100 supersedes #1"), also through "which" or "that" ("reverts #100, which fixed #1") and across a removed span ("#100 ~~also~~ fixes #1"). A hedge two words before the keyword disqualifies it ("hopefully this fixes #1", "could this fix #1?"). A clause that starts with if, when, once, until or unless is not a statement. The tokenizer keeps a line break as a token so that "Fixes #1" on one line and "Fixes #2" on the next stay two statements. Code spans, fences, indented code, blockquotes, HTML comments and strikethrough are skipped. The block stripping follows CommonMark for fences (also inside a blockquote), indented code, blockquotes with lazy continuation, setext underlines and HTML comments, and GFM for `~~` flanking. Result over the 1000 descriptions: 274 distinct references in 135 PRs. I checked the current state of all of them through GraphQL. All but one are closed (202 issues completed, 5 duplicates, 66 pull requests). The one open target is PR #32292, superseded by merged #36135. No open target is a false positive. Every review change kept this result. Patterns that are deliberately not handled: a bulleted list under "Closes:" on its own line (not seen in the sample), references separated by whitespace only ("#1 #2"), "fix for #1", and GH-1 style references. A `?` after the list is not treated as a question. The block parser tracks no list containers, so a second paragraph of a list item indented by four spaces is read as an indented code block and skipped. A removed span or inline comment reads as one word, so "Fixes <!-- n --> #1" finds nothing. The test suite covers: the phrases above, stopping at the right place in real sentences, CRLF descriptions, URLs with fragments or a `/files` suffix, case-insensitive `Owner/Repo#1`, the fake API where a lookup, an update or a comment fails, the `dry_run` input, an invalid `pr_number` input, an unmerged PR, a PR merged into a non-default branch, the merge event body against a later edit, and a description with no closing statement. The first revision of this PR checked out the repository and ran `scripts/close-linked-issues.ts`. Jarred asked for no checkout and no script file, so the script moved inline into the workflow and the test now reads it out of the YAML. </details> <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 9 · 2 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/internal/close-linked-issues.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/internal/close-linked-issues.test.ts bun test v1.4.1 (4448a2e) test/internal/close-linked-issues.test.ts: (pass) finds "Fixes #39852" [176.21ms] (pass) finds "Closes #31772. Fixes #31771." [22.28ms] (pass) finds "- Fixes #39930" [12.28ms] (pass) finds "Fixes: #30429" [10.46ms] (pass) finds "FIXES #1" [7.86ms] (pass) finds "(Fixes #1)" [8.97ms] (pass) finds "**Fixes #1**" [10.20ms] (pass) finds "__Fixes #1__" [9.83ms] (pass) finds "_Fixes #1_" [11.25ms] (pass) finds "Fixes **#1**" [9.72ms] (pass) finds "**Fixes** #1" [7.13ms] (pass) finds "**Fixes:** #1" [8.11ms] (pass) finds "Fixes #1 and **#2**" [11.47ms] (pass) finds "Fixes **#1**, **#2**" [9.13ms] (pass) finds "## Why (fixes #13771, closes #30543)" [16.08ms] (pass) finds "Closes #11418" [19.46ms] (pass) finds "Resolves #1. Resolved #2. Resolve #3." [12.09ms] (pass) finds "Fixes #34055, #30327, #24394, #20816, #32403, #11898, #10056." [17.11ms] (pass) finds "Fixes #18192 and #31675 as a consequence" [10.45ms] (pass) finds "Fixes #1, #2, and #3" [10.96ms] (pass) finds "Fixes #1 & #2" [7.63ms] (pass) finds "Closes #33280, Closes #32864 and Closes #29696 (the timer in #32949 is orthogonal)" [20.29ms] (pass) finds "Closes #33182 and #32947 on top of current main (which already has #36304 for catalogs)." [16.12ms] (pass) finds "Fixes #1,\n#2" [7.76ms] (pass) finds "Fixes #1, #2,\nand #3" [9.27ms] (pass) finds "Fixes #1\nand #2" [8.57ms] (pass) finds "Fixes #1\n& #2" [6.80ms] (pass) finds "Fixes #1 and\n#2" [7.31ms] (pass) finds "Supersedes #39908 (same change, moved from a fork branch)" [13.21ms] (pass) finds "Supersedes #38778 and #38391. Carries the entry point arm of #35053." [14.43ms] (pass) finds "Supersedes #39193 and keeps its three tests." [11.48ms] (pass) finds "This supersedes #33306 and #32803. Their tests are kept here." [13.73ms] (pass) finds "- This replaces #33793. Its ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` .github/workflows/close-linked-issues.yml | 950 ++++++++++++++++++++++++++++++ test/internal/close-linked-issues.test.ts | 598 +++++++++++++++++++ 2 files changed, 1548 insertions(+) ``` </details> **gate history** · 29 passed · 0 rejected · iteration 9 <details><summary>evidence per changed file</summary> ``` file reads edits tests .github/workflows/close-linked-issues.yml 6 12 0 test/internal/close-linked-issues.test.ts 3 11 0 ``` </details> <!-- robobun:evidence:end -->
2 participants
What
Response.clone()/Request.clone()on a stream body deep-copied every chunk into the second tee branch. An N-deep clone chain therefore retained N independent copies of every body chunk.Memory (RSS delta, 10 MB streaming body, only the leaf clone is read)
bun is now flat across depth and below node (node still allocates one
Uint8Arraywrapper per tee level; bun enqueues the sameJSValue).Cause
ReadableStream__tee(the extern called from RustBodyclone) passedcloneForBranch2 = truetoreadableStreamTee, which routed each chunk throughstructuredCloneForStream(ArrayBuffer::slicememcpy) before enqueuing into branch2. In a clone chain each level copies again, so a single source chunk ends up duplicated once per tee level.The fetch spec's "clone a body" step does say
ReadableStreamTee(stream, true), but Node (undici), Chrome, and Firefox all share the chunk reference between branches instead. Deno follows the spec and exhibits the same O(depth × bytes) growth. See whatwg/streams#1156 for the spec discussion.Fix
Pass
cloneForBranch2 = falseinReadableStream__tee. The cloned branch now receives the exact sameUint8Arrayobject the original branch does, so a clone chain retains one copy of the body regardless of depth.This supersedes #33130's right-sized-copy optimisation: instead of copying only the view's bytes, we don't copy at all. Branch1 always received the original reference, so branch2 sharing that reference never retains more than branch1 already did. The now-unreachable
cloneForBranch2 = truepath (structuredCloneChunk,m_shouldClone, the$structuredCloneForStreamprivate global and host function, andCloneMode::Full) is deleted; thecloneForBranch2parameter is kept onreadableStreamDefaultTeeto mirror the spec signature and asserted false.ReadableStream.prototype.tee()was already passingfalse; only the body-clone path changes. Byte-controller tee (type: "bytes") is unchanged and still copies per spec.Verification
test/js/web/fetch/body-clone.test.tsgains a 50-deep clone chain RSS check (fails at ~219 MB on main, passes at <50 MB with this change) and the existing per-chunk tests now assertclonedChunk === originalChunk. All 63 tests in the file pass.[review] gate passed · iteration 5 · 11 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 5
evidence per changed file