Bun.write: honour the destination window on the Linux read/write fallback - #37856
Bun.write: honour the destination window on the Linux read/write fallback#37856robobun wants to merge 1 commit into
Conversation
…back Bun.write(Bun.file(path).slice(0, n), Bun.file(src)) copies n bytes via copy_file_range, but when that degrades to the read()/write() fallback (EXDEV, ENOSYS, ENOTSUP, BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1) a path destination went through copy_file_using_read_write_loop, which treats its size argument as a hint and drains the source to EOF, and the ftruncate that followed pinned the file at the full source length. Route path destinations through the same bounded loop the fd arm uses, keeping the ftruncate for the case where run_async fallocate'd the file longer than what was copied. The POSIX_FADV_SEQUENTIAL hint moves into the bounded loop so the fallback keeps it.
|
Warning Review limit reached
Next review available in: 4 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 (2)
Comment |
|
Status: reproduced on release 1.4.0 and current main (Linux 6.17) with the snippet in the description, both via |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is small and the test matrix is thorough, but since it reroutes the Linux Bun.write fallback through a different copy loop (64 KiB + fadvise instead of the 8 MiB-slab helper) on a path reachable in production via EXDEV, a maintainer sign-off on that trade-off would be worthwhile.
Checked: read_write_loop_capped still reads to EOF when cap == MAX_SIZE (the /proc/version unsliced case); ftruncate is still gated on bun_opened_dest and skipped on error, matching the old behavior; the posix_fadvise cfg matches node_fs.rs exactly and is compiled out on macOS; node_fs import is still live via DEFAULT_PERMISSION and the macOS/FreeBSD arms.
Extended reasoning...
Overview
The PR fixes read_write_fallback in src/runtime/webcore/blob/copy_file.rs (Linux/Android only). Previously, when the destination was a path Bun opened itself, the fallback delegated to NodeFS::copy_file_using_read_write_loop, which treats its size argument as a hint and reads to EOF — so a .slice(0, n) destination window was ignored whenever copy_file_range was unavailable (EXDEV, ENOSYS, ENOTSUP, or the debug env var). The fix routes both the path- and fd-destination arms through the existing read_write_loop_capped, keeping the ftruncate for the path arm to trim any fallocate overshoot. The POSIX_FADV_SEQUENTIAL hint that lived in the removed helper is moved into read_write_loop_capped under the same cfg(linux|android|freebsd). Seven new test cases in bun-write.test.js cover both syscall paths, window-past-EOF, the preallocate threshold, and st_size==0 sources.
Security risks
None. No untrusted input parsing, no path handling changes, no new syscalls beyond an advisory posix_fadvise hint that was already issued on this path via the removed helper.
Level of scrutiny
Medium-high. This is core Bun.write file-copy runtime code, and the fallback is reachable in production on any cross-filesystem copy (EXDEV). The correctness change itself is straightforward — one function call swapped for another that already exists and is already used by the sibling arm — but it also swaps the large-file buffer strategy (8 MiB slab → 64 KiB loop with readahead hint). The PR description benchmarks this as neutral-to-better, referencing #34825, but that's a performance claim on a hot-ish path that a maintainer should confirm.
Other factors
I verified the node_fs import remains live (used by DEFAULT_PERMISSION, the macOS fcopyfile fallback, the FreeBSD arm, and Windows), that the new posix_fadvise cfg exactly matches the one in node_fs.rs:4723, and that the unknown-size / read-to-EOF semantics are preserved (cap = MAX_SIZE → loop until read() returns 0). The ftruncate gating and error-path skip are unchanged from before. FreeBSD's fd-destination arm now also gets the fadvise hint via do_read_write_loop_capped, which is new but harmless. Tests follow harness conventions (tempDir, bunEnv spread, combined-object assert before exitCode, it.each matrix) and the author confirmed fail-before with USE_SYSTEM_BUN=1. No CODEOWNERS cover these paths.
|
Updated 1:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit f35d99d has some failures in 🧪 To try this PR locally: bunx bun-pr 37856That installs a local version of the PR into your bun-37856 --bun |
Problem
Bun.write(Bun.file("dest.bin").slice(0, 5000), Bun.file("src.bin"))with a 300000 byte source writes 5000 bytes on thecopy_file_rangepath but 300000 bytes on the Linux read/write fallback. Reproduced on 1.4.0 and main.BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1or whencopy_file_rangereturnsEXDEV,ENOSYSorENOTSUP, so the same call copies a different number of bytes depending on whether the two files share a filesystem.fs.copyFileread loop, which treats the size it is given as a hint and reads to EOF, then truncated the destination to whatever it had copied. The window was never applied.Fix
ftruncateafter the copy stays for destinations Bun opened itself: the file may have been preallocated to the source's stat size, which is too long if the source shrank during the copy. It is a no-op otherwise.POSIX_FADV_SEQUENTIALhint (sys: hint POSIX_FADV_SEQUENTIAL on the copy-file read/write fallback #34825) lived in the old loop, so it moves into the capped loop; per the numbers in sys: hint POSIX_FADV_SEQUENTIAL on the copy-file read/write fallback #34825 that keeps the 64 KiB loop on par with the old 8 MiB buffer, and the existing hint test still passes.Background
Bun.write(dest, Bun.file(src))copies file to file inside the runtime. Slicing the destination,Bun.file(path).slice(0, n), hands the copy a window of n bytes that every platform arm is expected to apply.copy_file_range(an in-kernel copy) and drops to a userspace read()/write() loop when the kernel refuses:EXDEVacross filesystems,ENOSYSorENOTSUPwhere unsupported, or the env var above.ftruncate), and a file descriptor supplied by the caller. Only the path arm was affected.POSIX_FADV_SEQUENTIALtells the kernel the source will be read front to back so it can read ahead, which is what lets a small buffer keep pace with a large one.Original description
Repro
The env var is only the easiest way in. The same fallback is taken when
copy_file_rangereturnsEXDEV,ENOSYSorENOTSUP, so on a 6.x kernel the same call copies 5000 bytes when both files are on one filesystem and 300000 when they are not (reproduced here with overlayfs ->/dev/shm, and/proc/version-> tmpdir, both with no env var set). Release 1.4.0 and current main behave the same.Cause
read_write_fallbackinsrc/runtime/webcore/blob/copy_file.rssent a path destination throughNodeFS::copy_file_using_read_write_loop(.., stat_size = cap, ..). That helper is written forfs.copyFile, where the stat size is a hint: it readsstat_sizebytes and then, unless it hit EOF, keeps reading until it does. So the cap was ignored whenever the source was longer than it, and theftruncate(dest, total)that followed then pinned the file at the full source length.Every other implementation of the copy honours the window already: the Linux
copy_file_range/sendfile/spliceloop stops atremain, the fd-destination arm of this same function usesread_write_loop_capped(#36758), macOS (clonefile,fcopyfile) and FreeBSD truncate the destination tomax_length, andCopyFileWindows::on_completetruncates tosize. This was the one arm that did not, and whichever cap the caller ends up passing (the destination window today, a source slice if #31515 lands)CopyFileneeds every arm to apply it the same way.Fix
Route the path destination through
read_write_loop_cappedas well, soread_write_fallbackis one bounded loop plus, for a destination Bun opened itself, the existingftruncate(dest, total). The truncate is kept becauserun_asyncmay havefallocated the destination to the stat'd length before the copy, which is longer than the copy if the source shrank in between; it is a no-op otherwise. The fixing line is theread_write_loop_cappedcall replacing thecopy_file_using_read_write_loopcall.copy_file_using_read_write_loopwas also where the fallback picked up itsPOSIX_FADV_SEQUENTIALhint (#34825), so the hint moves intoread_write_loop_capped(same cfg as the original) and the fd arm gets it too; the existing LD_PRELOAD test for the hint still passes. Per the numbers in #34825, a 64 KiB loop with the hint is on par with the 8 MiB slab that helper used for large files; a 256 MiB fallback copy with this debug build took 0.6 to 0.9 s here against 1.0 s or more for the release slab.Unchanged: the fd-destination arm (already bounded), the kernel syscall loop, and the macOS/FreeBSD/Windows arms (all truncate to the window).
cargo check -p bun_runtimepasses forx86_64-apple-darwinandx86_64-unknown-freebsd, which shareread_write_loop_capped.Verification
New
describeintest/js/bun/io/bun-write.test.js(Linux block), 7 cases: the window on both thecopy_file_rangepath and the fallback, a window past EOF on both, a 2.5 MiB window (above the 2 MiBfallocatethreshold, so the keptftruncateis exercised), and an st_size == 0 source (/proc/version) both unsliced (must still read to EOF) and sliced.The rest of the file passes with this build apart from
should work when copyFileRange is not available > on large files, which exceeds the 5 s timeout on this machine on unmodified main too (and starves whichever concurrent siblings are in flight); #37792 is the fix for that test.Related
While checking blast radius: on the normal
copy_file_rangepath (and the macOS/Windows arms), readingf.sizeor awaitingf.exists()on the destination beforeBun.write(f, Bun.file(src))caps the copy at the destination's previous size, becauseresolve_size()stores the stat size in the same fieldslice()uses andwrite_file_with_source_destinationpasses that field as the window. That is a call-site problem inBlob.rswhich this PR does not change (after this PR the fallback matches the other arms there too); it is being tracked separately. #35815 is about sliced destinations with a non-zero offset and leavesslice(0, n)as is.