Skip to content

clone(): keep the stat of an unread Bun.file().stream() body out of the shared Store - #46666

Open
robobun wants to merge 2 commits into
mainfrom
robobun/7c60697c/clone-file-stream-no-store-stat
Open

robobun wants to merge 2 commits into
mainfrom
robobun/7c60697c/clone-file-stream-no-store-stat

Conversation

@robobun

@robobun robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

Background

Downsides

Notes

Repro (the file grows from 5 to 9 bytes after clone(), and 3 of 4 lines differ on 1.4.4)

import fs from "node:fs"; import os from "node:os"; import path from "node:path";
const d = fs.mkdtempSync(path.join(os.tmpdir(), "sb-")); let n = 0;
const drain = async b => { let s = ""; for await (const c of b.body) s += Buffer.from(c); return s; };
const mk = clone => { const p = path.join(d, "f" + n++); fs.writeFileSync(p, "12345"); const f = Bun.file(p), r = new Response(f.stream()), c = clone && r.clone(); fs.writeFileSync(p, "123456789"); return { f, r, c }; };
{ const { f, r } = mk(false); console.log("no clone():", f.size, await drain(r)); }
{ const { f, r, c } = mk(true); console.log("clone():", f.size, await drain(c), await drain(r)); }
{ const { f } = mk(true); console.log("clone(), the program's own f:", f.size, await f.text()); }
const P = "/proc/version", L = fs.readFileSync(P).length, f = Bun.file(P), r = new Response(f.stream()), c = r.clone();
console.log("/proc/version whole:", [await c.text(), await r.text(), await f.text()].map(t => t.length === L).join(" "));
1.4.2 and this PR 1.4.4
no clone() (control) 9 123456789 9 123456789
clone(): f.size, the clone, the original 9 123456789 123456789 5 12345 12345
clone(): the program's own f.size, f.text() 9 123456789 5 12345
/proc/version whole: clone, original, f true true true false false false

A longer script with ten rows (Request bodies, clone.blob(), a fetch upload of clone.body, a file that is empty at clone(), a deleted file, utimes): 1.4.2 has 10 right, 1.4.4 has 2 right, this PR has 10 right.

The change. store_reads_repeatably keeps its rule: tee an fd and every path that is not a regular file, lift a regular file or a path whose stat fails. It takes the mode from mode_for_clone: what a stat of the Bun.file() cached if there is one, else one stat per Store kept in mode_seen_by_clone. store_yields_bytes_once calls the same helper, so the code it had inline is now in one place.

Tests. The describe clone() of a Bun.file() body does not change what the Bun.file() answers from #44504 is now a describe.each over the body forms Bun.file() and Bun.file().stream(): 28 tests and 6 test.todo. New in both forms: a file that is empty at clone() and written afterwards, and a cloned procfs body read through arrayBuffer(), bytes() and text() of its blob(). On the 1.4.4 release the 14 with a Bun.file() body pass, and 13 of the 14 with a .stream() body fail. The fourteenth, exists() after a file was created, passes there only because clone() writes the cache. The 6 test.todo are the three procfs readers of the next section, in both forms.

The procfs readers (a cloned body over Bun.file("/proc/sys/kernel/ostype").stream(), the clone and the original give the same result in every cell, Linux x64)

reader 1.4.2 1.4.4 this PR this PR merged with #41593
text(), arrayBuffer(), bytes() whole "" whole whole
text() of its blob() whole "" whole whole
for await of .body whole "" "" whole
size of its blob() 6 0 0 0
returned from Bun.serve whole "", Content-Length: 0 "", Content-Length: 0 "", Content-Length: 0

The last three readers take the length of a file body from its stat, and procfs reports st_size 0. They give the same empty result for a Bun.file() body that nothing cloned, on 1.4.2 too. In 1.4.2 the clone of a .stream() body was a tee, which copied the bytes, so those readers did not see a file. The three are test.todo in the procfs block of body-clone.test.ts, each with the observed result.

Syscalls (newfstatat/fstat/openat, N iterations minus the N = 0 run, counted with a seccomp and ptrace counter because strace is not installed here, on the 1.4.4 release binary and this PR's debug build)

1.4.4 this PR
1000 clone() of a .stream() body, a fresh Bun.file() each 1000/1/1 1000/2/2
1000 clone(), one shared Bun.file() 1/1/1 1/1/1
1000 clone() of a slice().stream() body 1000/1/1 1000/2/2
1000 clone() of a Bun.file() body, fresh / shared 1000/1/1, 1/1/1 1000/1/1, 1/1/1
200 clone() + drain of clone.body, fresh 200/208/208 400/208/211
200 clone() + drain of clone.body, shared 1/208/208 2/208/209
200 clone() + clone.text(), fresh 200/201/201 200/201/201
200 clone() + console.log(clone), fresh / shared 200/1/1, 1/1/1 400/1/1, 2/1/1

The FIFO order (one shared Bun.file(p): p is a regular file at the first act, then becomes a FIFO that gets 19 bytes, then a .stream() body is cloned and both bodies are read)

first act 1.4.2 1.4.4 this PR
nothing 19, 19 19, 19 19, 19
a Bun.file() body is cloned 19, 19 19, 19 never settles
a .stream() body is cloned 19, 19 never settles never settles
f.size 4, 4 4, 4 4, 4

The memo lives as long as the Store, as #44504 chose. On 1.4.4 the two arms keep separate answers, so only the second row hangs there. Here both arms share mode_seen_by_clone.

Differential against 1.4.2 and main, measured on 2026-10-08 with this same change on 620b50f (10,200 rows: 5 ways to clone x 5 body forms x 12 file states x a fresh or already sized Bun.file() x 17 observations, and 0 rows differed run to run). 245 rows change from main. 185 take the value of 1.4.2. 60 match neither build, and each equals what main answers for a Bun.file() body: 40 printed sizes (the size on disk at print time, not the size at clone()) and 20 clone.blob() sizes (the size on disk, with the type main keeps). No row changes for a Bun.file() body, for a body whose .body was read before the clone, for a directory, or for /dev/null.

Not fixed here

Suites on the debug build, all pass: body-clone (118), body, response, blob, blob-write, wasm-streaming, FormData, bun-file, bun-file-exists, bun-file-read, inspect, console-log, bun-serve-file, bun-serve-static.

Credit: the once-per-Store memo mode_seen_by_clone is from #44504.

… clone-only memo

`store_reads_repeatably` called `resolve_file_stat`, which writes
`max_size`, `mode`, `seekable` and `last_modified` on the `Store` that
the body shares with the program's `Bun.file()`. It now takes the mode
from `mode_for_clone`, the lookup that `store_yields_bytes_once` had
inline. That lookup stats a path once per `Store` into
`mode_seen_by_clone`.

The rule of the arm is the same: an fd and every path that is not a
regular file are teed.
@robobun

robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:03 PM PT - Oct 24th, 2026

✅ @robobun, your commit 0286dee380169c1bf5ebcd0a30a79cdafd9eb508 passed in Build #132106! 🎉


🧪   To try this PR locally:

bunx bun-pr 46666

That installs a local version of the PR into your bun-46666 executable, so you can run:

bun-46666 --bun

@robobun

robobun commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on the 1.4.3 and 1.4.4 release binaries. bun 1.4.2 is right.

import fs from "node:fs"; import os from "node:os"; import path from "node:path";
const p = path.join(fs.mkdtempSync(path.join(os.tmpdir(), "clone-")), "log.txt");
fs.writeFileSync(p, "12345");
const f = Bun.file(p);
const original = new Response(f.stream());
const clone = original.clone(); // nothing reads a body
fs.writeFileSync(p, "123456789");
console.log(f.size, await clone.text(), await Bun.readableStreamToText(original.body));
// 1.4.4: 5 123456789 12345
// 1.4.2 and this PR: 9 123456789 123456789

The regression tests are in test/js/web/fetch/body-clone.test.ts. Run them with bun bd test test/js/web/fetch/body-clone.test.ts.

Not fixed here: a cloned procfs body is still empty through .body and Bun.serve, and its blob() has size 0. These three readers are test.todo in the same file. #41593 reads .body whole (measured on a merge of its branch with this one, run with --todo). #43974 owns Bun.serve. The notes in the description have the table.

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 829a5e9 and 0286dee.

Files selected for processing: 1
  • test/js/web/fetch/body-clone.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Walkthrough

File-backed body repeatability now uses file mode through mode_for_clone. Clone tests cover Bun.file() and .stream() bodies across Response and Request cases, file growth, and procfs reads.

Changes

File body cloning

Layer / File(s) Summary
Determine file repeatability
src/runtime/webcore/Blob.rs
store_reads_repeatably uses mode_for_clone to classify file stores. The helper caches path mode without updating shared file-stat fields.
Validate cloned file bodies
test/js/web/fetch/body-clone.test.ts
Tests exercise Bun.file() and .stream() bodies across Response and Request clone paths, file growth, existence and inspection-size checks, and Linux procfs reads. The tests add whole-file reads through arrayBuffer(), bytes(), and blob().text(). Related .body, blob-size, and Bun.serve tests remain TODOs.

Possibly related PRs

  • oven-sh/bun#46688: Both PRs cover procfs files with zero st_size; this PR tests cloned bodies, while the related PR changes synchronous Blob reads.
  • oven-sh/bun#43871: Both PRs address procfs reads where file metadata reports zero size; this PR adds clone coverage.

Suggested reviewers: dylan-conway, jarred-sumner

Priority: Normal

Merge Risk: Minimal · up to 0286d

The clone regression has targeted coverage, and no demonstrated new merge-blocking issue remains. The reported FIFO and procfs limitations should remain visible to owners.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the main change: preventing an unread Bun.file().stream() body from sharing the cached stat with the Store.
Description check Passed The description explains the problem, fix, verification, tradeoffs, and remaining limitations. It does not use the template headings exactly, but it provides the required information in equivalent sec…
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.
Full details: Description check

Explanation

The description explains the problem, fix, verification, tradeoffs, and remaining limitations. It does not use the template headings exactly, but it provides the required information in equivalent sections.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I diffed the old and new store_reads_repeatably predicate on a path store: a failed stat returned true before as well (seekable stayed None, so != Some(false) held), so the only behavior change there is dropping the resolve_file_stat side effect and sharing the memo with store_yields_bytes_once. I also checked store_yields_bytes_once after the extraction — the fd resolve_file_stat pre-step, the None for an fd whose fstat failed, and the final predicate are unchanged from the old inline code.

Extended reasoning...

The change touches src/runtime/webcore/Blob.rs (extracting mode_for_clone and rewriting store_reads_repeatably to use it instead of resolve_file_stat) and parameterizes the clone test block in test/js/web/fetch/body-clone.test.ts over Bun.file() and Bun.file().stream() bodies. No security-sensitive surface is involved. A confirmed inline finding (the shared mode_seen_by_clone memo causing a hang when a path becomes a FIFO between two clones, which the author also disclosed as a known downside) means a human should weigh the tradeoff, so this is not an approve.

Comment thread src/runtime/webcore/Blob.rs
A cloned procfs body reads whole through arrayBuffer(), bytes() and text()
of its blob(). Add those next to the text() test.

Three readers take the length of a file body from its stat, and procfs
reports st_size 0. They see the cloned body as empty, as they see a
Bun.file() body that nothing cloned: .body, the size of the blob(), and
Bun.serve. Add them as todo with the observed result.
Comment on lines +5752 to +5753
/// opens it again). A file descriptor never does: its offset, and for a
/// pipe its bytes, are shared.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Comment on lines +5764 to +5765
/// `st_mode` for a body's `clone()`. A path is stat'd once into `mode_seen_by_clone`, not through `resolve_file_stat`:
/// the `Bun.file()` sharing the store answers from what that caches.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants