Bun.write: don't cap a file-to-file copy at the destination's cached size - #37877
Bun.write: don't cap a file-to-file copy at the destination's cached size#37877robobun wants to merge 1 commit into
Conversation
…size Bun.write(Bun.file(dest), Bun.file(src)) passed the destination blob's size to CopyFile / CopyFileWindows as the number of bytes to copy. For an unsliced Bun.file() that field is only the stat size cached by .size, exists(), toHaveLength() or structuredClone(), so once any of those had run the copy was cut to the destination's previous length, and the exists() guard pattern produced an empty file. Record on the blob whether size is a window the caller asked for with slice(); the copy only takes the window in that case and otherwise copies the whole source. Structured clones carry the window only for sliced blobs, so a clone of a primed whole-file blob stays a whole-file blob and a clone of a slice stays a slice. Fixes #4930
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Reproduced on released 1.4.0 (Linux) with the repro in the description and with the original scripts from #4930 and #22456: after Fix: |
|
Updated 2:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 5f4d4c8 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37877That installs a local version of the PR into your bun-37877 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds a new discriminator field to the core Blob struct and changes file→file copy semantics across all three platforms — and the description notes several overlapping in-flight PRs (#31515 / #32859 / #33360 / #33659) touching the same size semantics — a human look at the approach would still be worthwhile.
What was reviewed:
- All three
Blobliteral sites (Default,dupe_with_content_type, and the inline copy at Blob.rs:3316) initialize the new field; Rust would fail the build otherwise. get_slice_fromunconditionally overwrites thedupe()'d flag withlen != MAX_SIZE, so a whole-fileslice()on an unresolved blob correctly stays unbounded and a bounded re-slice stays bounded.- Structured-clone serialize now writes
MAX_SIZEfor unsliced blobs and deserialize sets the flag on a concrete size — no wire-format change, and the round-trip guard tests cover both directions. - The Windows and POSIX arms of
write_file_with_source_destinationboth consume the newmax_length.
Extended reasoning...
Overview
The PR fixes #4930 / #22456: Bun.write(Bun.file(dest), Bun.file(src)) was capped to the destination's cached stat size once anything (.size, exists(), toHaveLength, structured clone) had run resolve_size() on the destination blob. It adds Blob::size_is_explicit: Cell<bool> in src/jsc/webcore_types.rs, sets it in slice() / dupe() / the one other field-copy site / structured-clone deserialize, gates the v4 structured-clone size field on it, and has write_file_with_source_destination in src/runtime/webcore/Blob.rs pass MAX_SIZE to CopyFile::create / CopyFileWindows::init unless the destination was explicitly sliced. 17 new test cases in test/js/bun/io/bun-write.test.js cover the primer matrix, the #4930 missing-destination shape, an fd destination, the Windows over-truncate direction, and guards that a real slice(0, 4) (and its clone) still bounds the copy.
Security risks
None identified. This is a data-loss correctness fix in the file-copy path; no untrusted-input parsing, auth, or privilege boundaries are touched. The new flag defaults to false (unbounded copy, i.e. bounded by the source), which is the pre-existing behavior for a fresh Bun.file(dest).
Level of scrutiny
Medium-high. The diff itself is small and the mechanism is a single boolean discriminator, but it changes semantics in the core Blob type that flows through every file-backed I/O path on Linux, macOS, and Windows, and it adjusts what structured-clone puts on the wire (within the existing v4 format). The PR description explicitly calls out four other open PRs converging on the same Blob.size ambiguity with different approaches; a maintainer should confirm this is the direction they want relative to those.
Other factors
The bug hunting system found nothing. I confirmed every Blob { .. } literal in the tree carries the new field (grep on the adjacent is_jsdom_file: line), that get_slice_from overwrites the flag after dupe() so the parent's value can't leak through incorrectly, and that both the #[cfg(windows)] and non-Windows arms consume max_length. Test coverage is thorough (six primers × two existing-file states, plus fd, missing-file, shrink, and slice-still-bounds guards) and follows harness conventions (tempDir, Buffer.alloc over .repeat, isLinux gate on the resolved byte count with a stated reason). The existing Bun.file(fd).slice(0, 5) and Bun.stdout.slice(0, 100) tests in the same file continue to exercise the bounded path. Given the cross-platform reach and the coordination question with the sibling PRs, deferring to a human reviewer rather than auto-approving.
Problem
Bun.write(Bun.file(dest), Bun.file(src))copies only as many bytes asdestused to have, once anything has readdest's size (.size,exists(),toHaveLength(), structured clone). With the usualexists()guard on a missing file, the copy leaves an empty file behind.slice()window and the cached stat size. The copy treated whatever was in it as a window, so a destination that had merely been stat'ed was written as if it weredest.slice(0, oldSize).BunFile.existsmakesBun.writewrite nothing #4930Fix
slice(). The copy is bounded by the destination only in that case; otherwise it is bounded by the source, as a freshBun.file(dest)or a path destination already was.Background
Bun.file(path)is a lazy blob: it holds a path or fd and does not stat the file until asked..size,exists(),toHaveLength()and structured clone all stat it and cache the size on that blob object.blob.slice(start, end)is a new blob over a byte window of the same file. As aBun.writedestination it means "write into these bytes only", and existing tests rely on it capping a copy.MAX_SIZE, the sentinel for unknown. The per-platform copy routines (Linux, macOS, Windows) take a max length, andMAX_SIZEthere means the source decides; all three were being handed the destination's size field.Original description
Bun.write(Bun.file(dest), Bun.file(src))copies only as many bytes asdestused to have once anything has resolveddest's size. The common shape is anexists()guard, which leaves an empty file behind (#4930, open since 1.0); the other is overwriting a shorter file afterexists()/.size(#22456).Repro
Without the
.size/exists()line both copies are complete. Only the file to file path is affected; string, buffer and Blob sources go throughWriteFile, which never looks at the destination's size. Reproduces on 1.4.0 on Linux; the macOS (clonefile/fcopyfile+ftruncate) and Windows (CopyFileWindows::on_completetruncate) arms consume the same value. On Windows the cached size also works the other way round: copying a shorter file into a longer one padded it back out to the old length.Cause
Blob.sizeholds two different things for a file-backed blob: the window set byslice(), and, onceresolve_size()has run, the file's stat size..size,exists(),expect(f).toHaveLength()and structured-clone serialization all runresolve_size()on the blob itself.write_file_with_source_destination(src/runtime/webcore/Blob.rs) then handeddestination_blob.sizetoCopyFile::create/CopyFileWindows::initas the copy length, so a destination whose stat size merely got cached was copied into as if the caller had writtenBun.file(dest).slice(0, oldSize).Fix
Add
Blob::size_is_explicit, set byslice()(and bydupe()/ the other field-copy), and have the copy take the destination window only when it is set; otherwise it passesMAX_SIZEand the copy is bounded by the source like a freshBun.file(dest)or a path destination already is.slice()with no bounds on an unresolved file blob producesMAX_SIZE, which is not a window, so that case stays unbounded too.Structured clone writes the v4 size field only for sliced blobs (the format already defines
MAX_SIZEthere as "receiver stats it itself") and sets the flag when it reads a concrete one. Without that, a clone of a primed whole-file blob would arrive looking like a slice of the old length, and a clone of a real slice would lose its window under the new check. No wire format change.An explicitly sliced destination behaves exactly as before (
bun-write.test.jsalready relies onBun.file(fd).slice(0, 5)andBun.stdout.slice(0, 100)capping the copy), so unlike the attempts that changed what a destination slice means, this only changes the cases that were losing data. It is also independent of which size ends up being consulted later: #31515 / #32859 (copy the source slice instead) would make the call-site hunk here redundant, and #33360 / #33659 add the same field for the read paths and the getters; whichever of those lands second has a one-line conflict on the field declaration.Verification
test/js/bun/io/bun-write.test.js, newdescribenext to the existing file to file test: the six ways of priming the destination (.size,exists(),toHaveLength(), serializing it, writing to a clone of a primed blob, writing to a whole-fileslice()) against a shorter and an empty existing file, the missing-destinationexists()guard from #4930, an fd destination, a shorter source over a longer destination, and guards thatslice(0, 4)andstructuredClone(slice(0, 4))destinations are still bounded. The resolved byte count is only asserted on Linux becauseuv_fs_copyfileandfcopyfileover an existing file resolve with 0 independently of this (#33715).Also green with the debug build: the rest of
bun-write.test.js(the subprocess tests there only time out when run concurrently under ASAN here, same as on main),structured-clone-blob-file.test.ts,blob.test.ts,blob-write.test.ts,blob-cow.test.ts,bun-file*.test.ts,bun-serve-file.test.tsand theserve.test.tsContent-Range suite.cargo check -p bun_runtimepasses forx86_64-pc-windows-msvc.Fixes #4930
Fixes #22456