Repository navigation
Conversation
An empty Blob/Response/BlobPart[] source routed through write_file_with_empty_source_to_destination, which unconditionally called truncate on the destination path or fd. On character devices and pipes (for example /dev/null or a piped stdout) truncate fails with EINVAL and the promise was rejected, while the equivalent "" and new Uint8Array(0) sources resolved via the fast path. On an fd backed by a regular file (for example Bun.stdout redirected to a file) truncate succeeded and silently wiped everything already written to it. For fd-backed destinations, skip truncation entirely and resolve 0. For path destinations, keep truncation (it creates/empties regular files) but treat an EINVAL from truncate as a successful zero-byte write so non-regular-file paths match the non-empty-source behaviour. Drop the || is_empty() term from the fast-path truncate condition so that writing "" or new Uint8Array(0) to a user-supplied fd no longer truncates it either.
|
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 configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughEmpty-source writes to file descriptors now return zero without truncating existing contents. Writes to paths still truncate existing files or create empty files. The tests cover descriptor APIs, stdout and stderr, redirected output, and queued stdout writes. ChangesEmpty-source writes
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Empty writes to stdout, stderr and other caller-provided file descriptors no longer truncate their contents. Writes to paths behave as before. No merge-blocking risk was identified, and cross-platform behavior is left to CI. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:47 PM PT - Oct 2nd, 2026
❌ @robobun, your commit f5de349 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32890That installs a local version of the PR into your bun-32890 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs and the fix looks correct, but this changes user-visible Bun.write semantics (fd-backed empty writes become no-ops, path-based EINVAL from truncate is swallowed), so it's worth a human confirming the intended behavior.
Extended reasoning...
Overview
This PR fixes Bun.write(dest, <empty source>) so it no longer truncates fd-backed destinations or rejects with EINVAL on non-regular files. It touches src/runtime/webcore/Blob.rs in three places — write_file_with_empty_source_to_destination (early-return for fd destinations, swallow EINVAL from truncate on paths, remove the now-dead fd arm of the ENOENT recovery) and the two fast paths write_string_to_file_fast / write_bytes_to_file_fast (drop the || input.is_empty() term so truncation is gated solely on NEEDS_OPEN). Five new test cases in bun-write.test.js cover /dev/null, user fds, stdout-to-file, stdout-to-pipe, and two regression guards that path-based empty writes still truncate/create.
Security risks
None identified. No auth, crypto, or permissions surface; the change narrows when ftruncate/truncate is called rather than adding new syscalls or expanding access.
Level of scrutiny
Medium-high. Bun.write is a core, documented I/O API and this is an intentional semantic change: Bun.write(Bun.file(fd), "") used to ftruncate(fd, 0) and now resolves 0 without touching the file. The PR argues convincingly that the old behavior was a bug (it silently wiped redirected stdout and contradicted the adjacent comment), and the regression guards confirm path destinations still truncate. But a maintainer should sign off on the design call — particularly that swallowing EINVAL from path-based truncate is the right trade-off and that no callers were relying on the fd-truncate side effect.
Other factors
The Rust changes are surgical and internally consistent: the early Fd return makes the later file.pathlike.path() call safe (it panics on Fd, but that arm is now unreachable), and the was_eperm bookkeeping it carried is correctly removed as dead code. Test coverage is thorough, including a spawned-child check for both file-redirected and piped stdout. CI is still building; no prior human or bot review on the thread.
There was a problem hiding this comment.
I didn't find any issues with the logic, but this is a behavioral change to Bun.write's empty-source semantics on a core I/O path (and CI is currently red on 919f107), so it's worth a human look before merging.
Extended reasoning...
Overview
This PR changes how Bun.write(dest, source) handles an empty source in three places in src/runtime/webcore/Blob.rs:
write_file_with_empty_source_to_destination: now early-returns0for fd-backed destinations instead of callingftruncate, and swallowsEINVALfromtruncate(2)on path-backed destinations (treating it as a no-op for non-regular files). The now-unreachableFdarm of the ENOENT-recovery match and itswas_epermbookkeeping are removed, with the path extraction collapsed tofile.pathlike.path().slice()(safe —Fdis early-returned above, and.path()is documented to panic only onFd).write_string_to_file_fast/write_bytes_to_file_fast: drop the|| input.is_empty()term from thetruncateflag so it's gated solely onNEEDS_OPEN, matching the existing comment about not truncating caller-supplied fds.
Tests in bun-write.test.js cover all five empty-source spellings against /dev/null, a user-opened fd, a regular-file path (regression guard for truncate-to-empty), a new path (regression guard for create), and Bun.stdout via a spawned child with both file-redirected and piped stdout.
Security risks
None. The change reduces destructive behavior (stops wiping data on fd-backed destinations) and doesn't introduce any new file creation, permission, or path-handling surface.
Level of scrutiny
Medium-high. Bun.write is a core, widely-used I/O API, and this is an intentional behavioral change: writing an empty source to Bun.file(fd) used to truncate the underlying file and now does nothing. The PR argues convincingly that the old behavior was a bug (it contradicted the adjacent comment and destroyed data on redirected stdout), and the regression-guard tests confirm path-backed destinations still truncate/create as before — but a maintainer should sign off on the semantic shift and on swallowing EINVAL from truncate for path destinations.
Other factors
- Buildkite #65674 is currently reporting failures on the head commit; worth confirming whether they're related before merging.
- No CODEOWNERS entry covers this path.
- The bug-hunting pass found no issues; I verified that the
.path()unwrap is guarded by the newFdearly-return and that thewas_epermremoval is sound (the only consumer was the now-deadFdarm).
…bun-write-empty-blob-no-truncate
… regression When stdout is redirected to a regular file, a burst of fire-and-forget Bun.write(Bun.stdout, chunk) calls racing with a single Bun.write(Bun.stdout, "") used to leave the file correctly sized but mostly NUL-filled: the empty write synchronously ftruncate'd fd 1 while thread-pool writes were in flight, discarding their bytes without resetting the kernel file offset, so the remaining writes landed past a sparse hole. Every promise still fulfilled with its full byte count.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Pushed an additional regression test for the concurrent-write variant of this bug: a burst of fire-and-forget // bun r.mjs > out.txt ; then: tr -cd '\\0' < out.txt | wc -c -> ~53 000 NULs of 58 890 bytes
const ps = [];
for (let i = 0; i < 10000; i++) ps.push(Bun.write(Bun.stdout, "C" + i + "\n"));
await Bun.write(Bun.stdout, "");
const r = await Promise.allSettled(ps);
process.stderr.write("fulfilled=" + r.filter(x => x.status === "fulfilled").length +
" bytes=" + r.reduce((a, x) => a + (x.value || 0), 0) + "\n"); // fulfilled=10000 bytes=58890Also merged current |
The binary-size gate compares against the last passing main canary (#79916 = ae4b17d). This branch has 12 newer main commits merged in; the ~550KB growth is from those, not the ~45-line Blob.rs change here.
XNU doesn't serialize the file offset for concurrent write(2) on a shared fd without O_APPEND, so the strict size/line-set equalities could flake on macOS. Assert only what the fix guarantees: every promise fulfils with its byte count and no NUL hole appears. Also advance the offset with a synchronous writeSync before the empty write so the pre-fix ftruncate deterministically produces a hole regardless of thread-pool timing. [skip size check]
XNU's dofilewrite updates fg_offset with a non-atomic += bytecnt RMW, so concurrent write(2) on a shared fd without O_APPEND can double-count the increment and leave a sparse NUL hole independently of the ftruncate fix under test. Linux's f_pos_lock serializes the offset update so the nulBytes === 0 invariant holds there; gate the test with skipIf(isMacOS) to keep that check on platforms where it is deterministic. [skip size check]
There was a problem hiding this comment.
The two macOS-flakiness concerns I raised earlier are addressed and I found nothing new this pass, but this changes the user-visible contract of Bun.write(Bun.file(fd), <empty>) from "truncate" to "no-op" — worth a maintainer confirming that's the intended semantics before merge.
What was reviewed:
file.pathlike.path()in the ENOENT-recovery branch is safe: the new early return guarantees thePatharm.- Path-backed empty writes still truncate/create — verified via the
NEEDS_OPEN=truefast path (opens withoutO_TRUNC, thenftruncate(fd, 0)) and the retainednode_fs.truncatecall; both regression-guarded. - The concurrent-stdout test now skips macOS and its remaining assertions (
nulBytes===0,fulfilled=N) hold on Linux (f_pos_lock) and Windows (I/O manager serializesCurrentByteOffsetper FILE_OBJECT).
Extended reasoning...
Overview
Two-file change: src/runtime/webcore/Blob.rs and test/js/bun/io/bun-write.test.js. In write_file_with_empty_source_to_destination, an fd-backed destination now resolves 0 immediately instead of calling node_fs.truncate on it; path-backed destinations still truncate but treat EINVAL (non-regular file) as "wrote 0 bytes". The now-unreachable Fd arm of the ENOENT-recovery block and its was_eperm bookkeeping are removed, and file.pathlike.path() is called directly (safe because Fd early-returned above; .path() panics on the Fd variant per src/jsc/node_path.rs:331-335). In the two POSIX fast paths, truncate is now gated on NEEDS_OPEN alone — dropping the || input.is_empty() term that contradicted the adjacent comment about caller-supplied fds.
Tests add a five-spelling matrix ("", Uint8Array(0), Blob([]), Response(""), []) across four destination shapes (/dev/null path, user fd on a regular file, existing regular-file path, new path), plus a spawned-child test covering Bun.stdout as both a redirected file and a pipe, plus a concurrent-writes-then-empty regression test.
Security risks
None. No untrusted-input parsing, no auth/crypto surface. The change narrows what Bun.write does to a user's fd (stops silently discarding their data via ftruncate), which is strictly safer than before.
Level of scrutiny
Medium-high. Bun.write is a core, documented, hot-path API. The fix is small and the mechanism is well-argued, but it changes observable behaviour: Bun.write(Bun.file(fd), "") previously ftruncated the fd (silently, with the error discarded) and now does nothing. That is almost certainly what users want — the existing code comment already said fd destinations should get "manual control", and #13477 is a real user hitting the wrong behaviour — but a maintainer should sign off on the contract change rather than a bot.
Other factors
- My two earlier inline comments on the concurrent-stdout test (XNU
f_pos_lockabsence, then thefg_offset += bytecntdouble-count producing NUL holes) were both addressed; the test now skips macOS with a comment naming the exact race, and the remaining assertions are the ones the fix actually guarantees on Linux/Windows. - The two fast paths are
#[cfg(not(windows))]; on Windows the empty-source-to-fd case is covered by the early return inwrite_file_with_empty_source_to_destination(which is not cfg-gated). The new "to a user-provided fd does not truncate" and "Bun.stdout does not truncate" tests run on Windows and should exercise that path. - CI is still building; the binary-size failure is flagged in the commit log as inherited from merged-in main (
[skip size check]), not from this ~30-line diff. - The comment-cop bot flagged the rationale comments; robobun replied that they document a behavioural contract (not a workaround) and trimmed them in 96b94b1 — the surviving one-liners look proportionate.
|
Status for f5de349: CI is green (build #123146), there is no conflict with main, and every review thread is resolved. Ready for a maintainer review. f5de349 is tests only on top of aa30dd7, which has main bc7a813 merged in. What the last two pushes changed:
How to reproduce (Linux x64, stock 1.4.3-canary.1+367d939d9 against a debug build of this branch): printf 'line 1\nline 2\n' > a.log
bun -e 'await Bun.write(Bun.stdout, "")' >> a.log; wc -c < a.log # stock: 0 this branch: 14
(echo header; bun -e 'await Bun.write(Bun.stdout, "")'; echo footer) > b.log
od -c b.log | head -1 # stock: 7 NUL bytes, then footer this branch: header, footer// an answer with no body erases the log behind the fd
const fd = require("fs").openSync("app.log", "a");
await Bun.write(Bun.file(fd), await fetch(url204)); // stock: app.log is empty this branch: app.log is intact
Local test run: the eight tests of this PR and the 12 tests of the |
…bun-write-empty-blob-no-truncate # Conflicts: # test/js/bun/io/bun-write.test.js
…-mode fd An empty source written to Bun.stdout/Bun.stderr when fd 1/2 were inherited in append mode (bun app.js >> app.log) used to ftruncate the log and wipe output from earlier processes. Same for Bun.file(fd) on an fd opened with 'a'. Both resolve 0 and leave the file intact now.
robobun
changed the title
…bun-write-empty-blob-no-truncate Conflict in test/js/bun/io/bun-write.test.js: both sides kept. Blob.rs: write_file_with_empty_source_to_destination now takes `cx: &JsThread` on main, so the two early returns pass `cx.global()`.
Drop the arm that mapped EINVAL from the path truncate to a resolved 0. On Windows the truncate of a directory also reports EINVAL, and `Bun.write(dir, new Blob([]))` must keep rejecting there. How an empty source treats a path that is not a regular file is a separate question from the fd truncation, so this change no longer touches it. Keep the fast path change to the one expression.
…size in the concurrent case `Bun.write(fd, src)` and `Bun.file(fd).write(src)` also truncated the fd on an empty source. Path destinations keep their contract: an empty source still empties or creates the file (and its directory), and a shorter write still trims the file. The concurrent stdout test now checks the file size. Without it the test passed on an unfixed build when every queued write had finished before the truncate.
robobun
changed the title
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 finding, I also checked: the now-unconditional file.pathlike.path() in the ENOENT/mkdirp loop (src/runtime/webcore/Blob.rs:4189, 4210) is safe because the Fd arm returns at :4150 before it; the async write path at Blob.rs:1546 already sets truncate only for Path destinations, so no sibling site was missed; and the early return is not cfg-gated, so Windows (which skips the fast paths) gets the same no-op. One mismatch to tidy: the PR description still says /dev/null EINVAL from truncate(2) resolves 0 and lists an empty source > to /dev/null resolves 0 test, but after "leave path destinations as they are on main" neither is in the diff — Bun.write(Bun.file("/dev/null"), new Blob([])) still rejects.
Extended reasoning...
The change touches two files: a ~34-line edit to src/runtime/webcore/Blob.rs that makes an empty-source Bun.write a no-op for fd-backed destinations (early return in write_file_with_empty_source_to_destination, truncate = NEEDS_OPEN in both sync fast paths, removal of the dead Fd arm) and ~224 lines of new tests in test/js/bun/io/bun-write.test.js. No security-sensitive surface (no auth, injection, or data exposure); the risk is correctness of fd-vs-path truncation semantics. An inline nit was posted and one further verified finding was withheld, and the PR description no longer matches the code after the latest push, so this is a defer rather than an approve.
… tests concurrently `Bun.write(fd, await fetch(url))` with an answer that has no body, and a Response or Request whose stream closed before it gave a chunk, reach the same empty-source route and truncated the fd. The two tests that redirect a child's stdout now run in a concurrent describe block, like the other groups of this file that spawn a child.
|
I reproduced the reported Windows failure on Bun The fix in this PR addresses the failing behavior. I added focused Windows regression cases locally for both piped descriptors, a shared redirected file, and path truncation. They fail before the fix (10 pipe/file cases) while the five path-preservation checks pass. I could not finish a debug build here: Windows This confirms the issue on Windows and gives additional descriptor coverage for the existing change. |
Fixes #13477
Problem
Bun.write(fd, <empty source>)callsftruncate(fd, 0)and resolves0. It applies toBun.stdout,Bun.stderr,Bun.file(fd)and a numeric fd, for"",Uint8Array(0),Blob([]),[]and an emptyResponse(a 204 fromfetch(), a closed stream).bun app.js >> app.logwith oneBun.write(Bun.stdout, "")empties the log. With writes in flight the file fills with NUL bytes. On Windows the call rejects withEISDIR.write_file_with_empty_source_to_destination(src/runtime/webcore/Blob.rs:4127) truncates every destination. The fast paths settruncate = NEEDS_OPEN || input.is_empty(): the second term fires only for an fd.Fix
0with no syscall. The fast paths settruncate = NEEDS_OPEN: Bun resizes only a file it opened.Bun.write(path, "")still empties or creates the file.test/js/bun/io/bun-write.test.js. Five new tests fail on 1.4.3-canary, three guard path behavior, all pass here.Background
Blobwith noStore, which routes it towrite_file_with_empty_source_to_destination. A destination isPathOrFileDescriptor::Fdor::Path.ftruncatechanges the file length, not the fd offset, so later writes land past a hole of NUL bytes.O_TRUNCat open, fd untouched). That changes timing and errors for path destinations: separate change.Downsides
fs.ftruncateSync(fd, 0).Bun.write(Bun.file("/dev/null"), new Blob([]))rejects withEINVAL, while""resolves0.Notes
Verified on stock
1.4.3-canary.1+367d939d9against a debug build of this branch (Linux x64):Entry points:
Bun.write(Bun.file(fd), src),Bun.write(fd, src)andBun.file(fd).write(src), with six empty sources ("",Uint8Array(0),ArrayBuffer(0),Blob([]),Response(""),[]) and an fd openedr+ora. Stock truncates the file in all 36 cases. The fix keeps the 8 bytes in all 36.Empty bodies: 19 shapes of an empty
ResponseorRequest(a stream closed instart()or in the firstpull(), a byte stream,Blob([]).stream(), aclone(),Response(null),Response(),Response(""),Response(Blob([])),fetch()withContent-Length: 0, 204,HEAD, chunked with no chunk, and five shapes that close later) written toBun.file(fd)onaandr+and to a numeric fd. Stock truncates the file for 14 of the 19 shapes on all three destinations. The fix keeps it in 57 of 57 cases, in 3 of 3 runs. Each of these becomes aBlobwith noStoreand reaches the same early return. A path destination is still emptied by all 19. The stream shapes regressed between the canaries of 09-06 and 09-15, and thefetch()shapes truncate on 1.3.14 and later.Concurrent case: 2000 un-awaited
Bun.write(Bun.stdout, "C" + i + "\n")calls, onefs.writeSync(1, ...), then one awaitedBun.write(Bun.stdout, ""), with stdout redirected to a file. Stock leaves either a hole of NUL bytes or an empty file, and every promise still fulfills with its byte count. The test checks the exact size and the NUL count. It is skipped on macOS: XNU updates the shared offset of concurrentwrite(2)calls withoutO_APPENDnon-atomically, which can lose bytes with or without this change.History:
eebcd30067(2022-10, "Bun.write- truncate empty files") replaced an early "length 0: resolve 0" return withtruncate = needs_open or str.len == 0, so thatBun.write(path, "")empties the file. Theorterm has no effect for a path (needs_openis already true) and truncates a caller's fd. #18561 added the empty-source function.Path destinations that are not regular files:
Bun.write(Bun.file("/dev/null"), src)resolves0for""andUint8Array(0)(fast path, theftruncateerror is ignored) and rejects withEINVAL (truncate)forBlob([]),Response("")and[]. An earlier revision of this PR mapped thatEINVALto a resolved0. It is removed: on Windows the truncate of a directory also reportsEINVAL, andBun.write(dir, new Blob([]))must keep rejecting there (test blockBun.write early rejections are tracked). The mapping also resolved0where a non-empty write fails atopen(a FIFO with no reader, a socket path).An empty write to a closed or read-only fd resolves
0and does not touch the fd. Before,""andUint8Array(0)also resolved0(the fast path ignored theftruncateerror), and theBlob/Response/array sources rejected withEBADForEINVAL.Related open PRs: #41745 documents the old exception ("an empty
datatruncates the file to zero length"), and that sentence must go when this lands. #38167 and #42618 edit the same functions, the conflicts are textual.Windows and macOS are not run locally. The stdout test and the fd test run on both in CI.
Tests in
test/js/bun/io/bun-write.test.js:empty source > to a user-provided fd does not truncateempty source > an empty Response or Request body to a user-provided fd does not truncateempty source > to inherited O_APPEND stdout/stderr leaves earlier content intactempty source > to a regular-file path still empties the file(guard)empty source > to a new path still creates an empty file, and its directory(guard)empty source > a shorter write to a path still trims the file(guard)Bun.write(Bun.stdout, <empty source>) > does not truncate the destination(stdout as a file and as a pipe)Bun.write(Bun.stdout, <empty source>) > does not drop concurrent in-flight writes when stdout is a fileno 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