Repository navigation
Conversation
|
Status: ready for review. The branch is rebased on main (a4f1429). The change is green in CI. The two red tests do not come from this change. How to reproduce, with a release build of main on Linux x64: printf '0123456789' > /tmp/e1.txt
bun -e "const f = Bun.file('/tmp/e1.txt'); f.size; Bun.write(f, 'NEW'); process.exit(0)"
cat /tmp/e1.txtThe file holds A debug build exits slowly, so the bytes usually land there. CI, build 121354 at 1890c97: 179 of 181 jobs pass.
Both are reported as breaks outside this PR. A new CI run gives the same result for the first one until it is repaired, so this PR has no retrigger commit. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughOn non-Windows runtimes, offset-zero regular-file Blob destinations with a nonzero mode now qualify for existing small-input fast writes. Tests cover write completion, size thresholds, ordering, failures, process exit, and output to files, pipes, and terminals. ChangesRegular-file write completion
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Small Bun.write calls to a file that was previously statted now complete inline instead of going to the thread pool. This fixes the case where a write followed by an immediate process exit could leave old file contents. No merge-blocking risk was found. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether routing stat'ed fd handles (Bun.stdout/stderr redirected to a file) through the inline path can clobber the file on a non-empty write — it cannot, since write_string_to_file_fast/write_bytes_to_file_fast only arm the ftruncate guard when NEEDS_OPEN or the payload is empty (the empty case is the posted finding). The Windows path is unaffected: the changed gate sits under #[cfg(not(windows))] and libuv handles every write there.
Extended reasoning...
The change removes the regular-file/stat clause from the fast-path gate in write_file_internal in src/runtime/webcore/Blob.rs and adds a ~280-line test block; it touches no auth, crypto, or injection surface. Verified findings are being posted inline (including a fast-path double-write on EAGAIN for mid-size pipe payloads and the empty-write truncate-failure widening), and three further verified findings were withheld, so this is not an approve. The note above records the two adjacent concerns examined and ruled out from the code.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/webcore/Blob.rs— Callers writing a 64 KiB to 256 KiB string to a non-blocking pipe fd can get the payload's prefix written twice, with the promise still resolving to the full length. The loop at Blob.rs:5194-5207 counts a partial write intowritten, then on EAGAIN setsneeds_asyncand returns; write_file_internal at Blob.rs:4718 falls through to the pool, which rewrites the whole payload from byte 0 (write_file.rs:456-470). Fix: when any bytes were already written before EAGAIN, either hand the pool the remaining slice, or finish the write inline with a poll; apply the same to write_bytes_to_file_fast at Blob.rs:5278.Why this was flagged
Trigger: Bun.write(Bun.stdout, s) or Bun.write(Bun.file(pipeFd), s) with a non-blocking pipe/socket whose buffer (64 KiB) is smaller than the payload (< 256 KiB). write(2) accepts part, then returns EAGAIN. Blob.rs:5197 has already advanced
written; Blob.rs:5205-5207 sets needs_async and returns ZERO without reporting how much landed. Blob.rs:4718 then continues to the pool path, which writes the fullbytes_blob(write_file.rs:456-470, do_write_loop), so the consumer receives the first chunk twice. The dismissal argued the newly fast-pathed population (regular files) never returns EAGAIN, which is true, but this PR is the change that touches the routing gate at Blob.rs:4685-4687 and the same commit is the moment the duplicated-prefix mechanism should be fixed rather than left in the path it just widened; the fd-backed stdio population (Bun.stdout when spawned with a pipe) reaches it at production rate. Remedy: pass the remaining slice / written count to the pool task, or drain inline with a writable poll.Verification: pre-existing (base already fails by the same route; this PR neither adds nor widens the EAGAIN-capable destinations). Trigger:
Bun.write(Bun.file(pipeFd), s)/Bun.write(Bun.stdout, s)where the fd is a non-blocking pipe or socket andsis between the pipe buffer size (~64 KiB) and 256 KiB. Mechanism verified in /home/claude/bun/src/runtime/webcore/Blob.rs:write_string_to_file_fast…
|
Updated 3:16 PM PT - Sep 28th, 2026
❌ @robobun, your commit 1890c97 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 44056That installs a local version of the PR into your bun-44056 --bun |
|
Reply to the review of 46571be. The finding outside the diff (a payload of 64 KiB to 256 KiB to a non-blocking pipe fd, prefix written twice after The three inline comments have a reply each. 578f5c8 changes the two tests: the terminal test fails when the PTY closes with no output and releases the terminal with |
There was a problem hiding this comment.
I re-reviewed after 578f5c8 and found no bugs; the two test nits from the earlier pass (terminal exit now rejects sawOutput under await using, SIGXFSZ handler gone) are addressed. A human look is still worthwhile because the fix deletes a gate whose original intent is ambiguous — the upstream clause reads like an inverted "known pipe, avoid blocking the main thread" check, and removing it versus inverting it is a design call with user-visible consequences (inline partial writes on EFBIG, empty writes to read-only fds now fulfil, more JS-thread time per statted small write).
What was reviewed: the widened fast_path_ok at Blob.rs:4685-4687 against both write_string_to_file_fast/write_bytes_to_file_fast variants — the fd (NEEDS_OPEN=false) path was already reachable for un-statted Bun.file(fd) handles, so a statted handle now follows an existing, exercised path rather than a new one; the as_file() calls below still cannot see a Bytes store (callers reject it first, unchanged). Checked the offset != 0, S3, and 256 KiB exclusions remain intact, and that last_modified reset and the EAGAIN/ENOENT needs_async fallback are unaffected. Test matrix asserts promise state plus on-disk bytes with toEqual on whole objects, drains pipes with Promise.all, and skips on Windows where the block is cfg'd out.
Extended reasoning...
The diff is a three-line change to the non-Windows synchronous fast-path gate in src/runtime/webcore/Blob.rs::write_file_internal plus ~280 lines of new tests in test/js/bun/io/bun-write.test.js; it touches no auth, crypto, or injection surface. The removed clause routed small writes to a stat'ed regular-file Blob to the thread pool; the PR argues that made writes lost at process.exit(), and the code now treats stat'ed handles exactly like fresh ones. The second commit addressed both of my prior test nits, and the third prior comment (ignored ftruncate result) was pre-existing. What keeps this from an approve is that the deleted heuristic has an upstream origin whose intent may have been the opposite polarity, so choosing removal over inversion is a maintainer decision, and the change measurably shifts EFBIG/read-only-fd/JS-thread-latency behavior for statted handles. No CODEOWNERS entry covers the changed files.
|
@robobun wake up!! |
|
@robobun wake up!! |
…of the Bun.file() handle The gate of the inline write path in write_file_internal refused a destination whose store had cached the mode of a regular file. Every use of the handle that stats the file fills that cache: f.size, exists(), lastModified, structuredClone(f), a Response or FormData body that holds f. Bun.stdout and Bun.stderr on a regular file start with it. After that, a write of a string or a typed array under 256 KiB went to the thread pool. The gate no longer reads the cached mode. Such a write to a destination at offset 0 is made on the calling thread, as it is for a path string and for a handle that nothing has statted. A payload of 256 KiB or more, a Blob or Response source, a destination with an offset, and an S3 destination keep their path.
…he SIGXFSZ handler The terminal test rejects its wait when the PTY closes before the output arrives, and it releases the terminal with `await using`. Bun ignores SIGXFSZ from startup (src/runtime/bin_entry/mod.rs), so the handler in the script of the file size limit test had no effect.
578f5c8 to
1890c97
Compare
|
I am here. State of this PR:
Two decisions are for a maintainer. Both are in Notes of the PR body.
|
Problem
f.size; Bun.write(f, "NEW"); process.exit(0)leaves the old bytes or an empty file (release build, 10 of 10). Withoutf.sizethe file holdsNEW.fast_path_okgate inwrite_file_internal(src/runtime/webcore/Blob.rs:4679) sends a small write to the thread pool.process.exit()does not wait for it.Fix
process.exit().test/js/bun/io/bun-write.test.js(main fails 16),blob.test.ts,filesink.test.ts,image.test.ts.it.todofor aBlobsource that Bun.write: copy a file to an O_APPEND destination fd with the read/write loop #41685 owns.Background
Bun.file()handle caches onestat..size,exists(),structuredClone(f)fill it.Bun.stdouton a file has it.Bun.write(Bun.stdout, x) | cat), wait at exit (hangs on a full pipe). The predicate is from Bun.write: keep stdio writes in call order when stdout/stderr is a regular file #35949.Downsides
EFBIG) and fulfils an empty write to a read-only fd (wasEINVAL). Notes lists each case.Notes
What selects the path on main
A use of the handle that runs
resolve_file_statstoresst_modeon the file store that each view of the handle shares. After that, each write of a string or a typed array under 256 KiB to that handle goes to the thread pool. Measured withBun.write(f, "NEW"); process.exit(0)after the use, release build of main, 10 runs per line:NEWawait f.text(),await f.stat()f.size,await f.exists(),f.lastModifiedstructuredClone(f),new Blob([f]).size,new Response(f).clone()(await new Response(f).blob()).size, one read ofnew Response(f).body, aFormDatabody that holdsfawait new Response(f).clone().text()With this PR each line is 10, 0, 0. The empty file is an observed outcome. Its cause is read from the code and not traced: the pool path opens with
O_TRUNC.Bun.peek.status(Bun.write(f, "NEW"))ispendingfor each losing line andfulfilledfor the others, on a debug build too. On a debug build the exit is slow and the bytes land in 130 of 130 runs on main. So each test asserts the promise state at return and the bytes.One
process.stdout.write()between the write and the exit hides the loss on a release build. The children in the exit tests do no I/O there and report through the exit code.Same gate, other symptoms on main
Each row of this table has a test.
awaittoBun.stdoutand toBun.stderron a regular file, thenprocess.exit()awaitto a stattedBun.file(fd)C0 C2 C1 C4 C3C0 C1 C2 C3 C4"a", 10-byte fileulimit -f 1, 3000-byte file, statted handle (rejectsEFBIGon both)These rows are measured by hand and have no test. The first one has the same mechanism as the
Bun.stderrrow above.--inspectbanner with2> file, 12 runs (src/js/internal/debugger.ts:235callsBun.write(Bun.stderr, ...)with noawait)Bun.write(Bun.stdout, "hello\n"); process.exit(0)with> out.txt, 20 runs with the line in the fileulimit -f 0, 3000-byte file, statted handleawait, thenprocess.chdir(): rounds of 30 where the file lands in the new directoryIn each row the value of this PR is the value that a handle with no stat has on main.
Bun.file(path)andBun.file(fd)never reach the stdio branch that #37128 adds. When #37128 lands, the rows forBun.stdoutandBun.stderrbelong to it.Not changed
Routes to the pool that remain for each handle: a payload of 256 KiB or more, a
Blob,Response,RequestorArchivesource, a destination with an offset, and the fallback afterENOENTorEAGAINfrom the inline path. For these routes:process.exit()right after the write loses it. A 300 KiB string and aBlobsource to a fresh handle, a 300 KiB string to a path string and to a statted handle: written in 0 of 20 runs each, on main and here.awaitto one fd can land out of call order (Blobsources: 4 of 10 rounds on main, 10 of 10 here, a race on both). Call order holds among inline writes.BloborResponseto an fd opened with"a"gives 4000 bytes with 1990 NUL, and a 300 KiB string gives 614,400 bytes. Bun.write: copy a file to an O_APPEND destination fd with the read/write loop #41685 owns thefallocatein the pool path.The gate still reads the offset of the view.
slice()resolves a negative start against the cached size, sof.slice(-20)of a 10-byte file has offset 0 only after a stat. For that destination a stat still selects the path:fulfilledafterf.size,pendingwith no stat (main:pendingfor both). The pool path does not use the offset, so the bytes are the same on each path. The offset clause dates from #7470. Its removal is a separate decision for a maintainer, and this PR does not make it.Other code that reads the cached mode:
read_file.rs:757st_size0write_file.rs:427could_blockfor an fd destinationBlob.rs:1275exists()read_file.rs:551recvfor a socketcopy_file.rs:815,copy_file.rs:830The cached size as a byte budget (
Bun.write(dest, Bun.file(src))writes 10 bytes into a 10-byte file afterdest.size) is #4930. #43910 owns it.A pipe or a terminal destination was inline and stays inline. Two tests pin that. Windows: the block is under
#[cfg(not(windows))]and each write goes through libuv. Read from the code, not measured.Image.writeto a stattedBun.file()callswrite_file_internal(src/runtime/image/Image.rs:1909) and now writes inline under 256 KiB, as it does for a path string.Behaviour of a statted handle that moves to the behaviour of a fresh handle
pendingfulfilledfulfilledf.lastModifiedread between the call and theawaitopenatflagsO_WRONLY O_CREAT O_TRUNC O_NONBLOCK O_CLOEXECO_TRUNC, thenftruncatefallocatefor a payload over 1024 bytesEINVAL{ mode: 0o600 }, file removed after the stat, umask 022The
modeoption has no effect on a string or a typed array that is not empty, on main and here (#32885). No value of a fresh handle changed.The last row is a race with the reader. The inline path writes until the pipe is full, gets
EAGAIN, and the pool path writes the payload again from the start. #35953 and #36025 own that path. A ledger check of this PR found the case.Measurements
Release builds of main (29d9638) and of this PR on that base, Linux x64, JIT off for the instruction counts. The branch is now rebased on a4f1429. Between the two commits main changed no POSIX code of the gate or of the two inline writers, and the numbers are not measured again. Tools:
gdb(stepi, breakpoint hit counts), aptracesyscall counter written for this,size,nm.strace,perf,valgrind,bloatyandhyperfineare not on the test machine.write_file_internal, path string, string payloadUint8ArrayUint8ArrayUint8ArrayUint8Array, instructions before the hand-offBlobsource.text, bytes (size -A)write_file_internal, bytes (nm -S)WriteFilejobs for 5 small statted writes (debug build)futex,epoll_pwait2)openat,write,ftruncate,close)Bun Poolthreads after 1 statted write / after 100VmSizeafter those 100 writes, kBThe path string rows grow by 1 and 2 instructions because the compiler allocates the registers of the smaller function in another way. The source of that path did not change. The counts before the hand-off leave out each callee that runs over 3000 instructions (the copy of the payload), the same callees on both builds.
JS-thread time for a burst of 100 writes with no
awaitto a statted handle. Interleaved runs of the two builds, 10 rounds, minimum. The same measurement on a fresh handle differs by at most 2.4 % between the builds, which is the noise floor.For the large payload main spends more time in the calls, because the pool path copies each payload into a new store on the JS thread.
New tests: 5 child spawns and 144 in-process cells. Median of 5 runs of the new block on a debug build: 7.2 s, at a host load average of 260 to 330.
Other designs
Bun.write(Bun.stdout, x) | catkeeps its order because of that.process.exit()wait for pool writes. A write to a full pipe that nothing drains then blocks the exit. Node does not wait either.await. Awaited code still gets the NUL bytes on an append fd and the changed order on an fd.Related PRs
Bun.file()handle. Its tests coveredBun.stdoutandBun.stderronlyBun.write(Bun.stdout | Bun.stderr)through the stdio sink, before this gatefallocateon an append fd in the pool path. Without its fix, its testappends a string above the preallocate threshold to stdout redirected with >>fails on main and passes with this PR, because the string no longer reaches the pool. ABlobsource keeps that test on the pool pathBun.write(Bun.stdout, "")empties the file that stdout points to). Without its fix, its testdoes not drop concurrent in-flight writes when stdout is a filefails on main and passes with this PR, for the same reasonftruncatein the inline writersEAGAINfrom the inline path, andcould_block. Edits the same block and not the clausemodeoption. Edits the same block and not the clauseSuites
The test machine had a host load average of 110 to 400 during these runs.
Release build of this PR, Linux x64: the whole
bun-write.test.jsgives 103 pass, 3 skip, 0 fail in 4 of 4 runs at a load average of 150. In 4 other runs at a load average over 250, 17 to 20 tests went over their 5 s limit. The test file of main did the same in 1 of 4 runs (20 tests).Debug build of this PR: the 21 new tests pass in 10 of 10 runs. The whole
bun-write.test.jsgives 101 pass and 5 fail in 4 of 5 runs (the other run: 100 and 6). The test file of main gives 80 pass and 5 fail in 3 of 3 runs. Each failure is a test that goes over its 5 s limit, in most runs these 5:should work when copyFileRange is not available > on large files,Bun.write(Bun.stdout, Bun.stdin) copies the whole pipe, and the three tests ofdoes not truncate the fd.blob.test.ts110 pass.filesink.test.ts69.image.test.ts103 pass, 5 skip.bun-file.test.ts6.bun-file-exists.test.js1.bun-file-read.test.ts5.bun-file-fd-read.test.ts3.vm-thread-door.test.ts46.comment-cop.test.ts2.bun-write-leak.test.tsfails on a local debug build with and without this change (320 MB at the first iteration against a limit of 256 MB, because the fixture detects ASAN by the binary namebun-asan). It passes on the release build of this PR.No Windows or macOS machine ran a test for this before the push. In CI the new tests pass on each lane that runs them: Linux x64 and aarch64 (glibc, musl, ASAN) and macOS x64 and aarch64. The new tests skip Windows.
no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/io/bun-write.test.js