Bun.write: deliver the full payload to a FIFO named by path - #36028
Bun.write: deliver the full payload to a FIFO named by path#36028robobun wants to merge 3 commits into
Conversation
Writing to a FIFO via Bun.write(path, data) had two failure modes:
(1) For string/TypedArray < 256 KiB the synchronous fast path opened
the FIFO O_WRONLY|O_NONBLOCK, looped write(), and on EAGAIN set a
needs_async flag and returned. The close-on-drop guard closed the
fd, which removed the FIFO's only writer, so the reader's blocked
read() saw EOF and exited with only the pipe-buffer-sized prefix.
The async fallback then reopened O_WRONLY|O_NONBLOCK with no
reader present and rejected ENXIO.
(2) For Blob/Response or any source >= 256 KiB the thread-pool
WriteFile path treated every path-opened destination as
could_block=false and its do_write() matched a write() result
hoisted outside the loop, so on EAGAIN the !could_block arm
'continue'd against the same stale error forever: 100% CPU on a
pool worker, zero syscalls, promise never settles.
Fix:
- write_{string,bytes}_to_file_fast: after opening a path, fstat the
fd; when not a regular file hand the already-open fd to the async
WriteFile via a new opened_fd parameter so the FIFO is never closed
and reopened mid-payload. Regular-file writes keep the existing
synchronous loop.
- WriteFile::do_write: drop the stale-result loop. EAGAIN on a regular
file is impossible, so set could_block=true and wait_for_writable().
- WriteFile::run_with_fd: fstat a path-opened fd to derive could_block
so FIFOs use the POLLOUT wait from the start and skip
preallocate_file.
- write_file_with_source_destination / WriteFile::create: thread the
pre-opened fd through; FileOpener::get_fd already short-circuits
when opened_fd is set and is_allowed_to_close() (path-backed) closes
it on finish.
Test: four spawn cases (200 KiB string via the fast path; 1 MiB
string/Uint8Array/Blob via the thread-pool path) mkfifo, pre-open the
read end synchronously so the write-side O_NONBLOCK open has a
reader, drain through a child, and assert Bun.write resolves with the
full byte count and the reader receives every byte.
|
Warning Review limit reached
Next review available in: 10 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Only the fast path in `write_file_internal` supplies a pre-opened fd, and | ||
| // it only runs for string/ArrayBuffer (→ Bytes) sources into a path-backed | ||
| // File destination, so the fd is consumed on the File+Bytes arm below. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // If the fast path handed us an open fd and anything below throws before | ||
| // `WriteFile` adopts it, close it instead of leaking. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A FIFO/socket/chardev opened O_NONBLOCK will EAGAIN once the pipe buffer | ||
| // fills. Closing after a partial write delivers EOF to the reader and the | ||
| // async fallback's reopen then fails ENXIO, so hand the open fd to the | ||
| // async `WriteFile` (which owns the POLLOUT drain) instead of writing here. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // See `write_string_to_file_fast`: hand a non-regular destination's open | ||
| // fd to the async `WriteFile` instead of risking a torn partial write. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // write(2) on a regular file never returns EAGAIN, so reaching | ||
| // this arm means the fd is pollable regardless of what the | ||
| // open-time classification guessed. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // This is mostly to handle pipes which were passsed to the process somehow | ||
| // such as stderr, stdout. Bun.stdin and Bun.stderr will automatically set `mode` for us. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // We just opened the path with O_NONBLOCK. For a FIFO/socket/ | ||
| // character device that open succeeds and every write can | ||
| // EAGAIN, so classify from fstat instead of assuming regular. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
| // Hand a FIFO/socket/chardev fd to the async WriteFile: closing here after a | ||
| // partial write would EOF the reader and make the async reopen fail ENXIO. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Duplicate of #36025 (same fd-handoff + fstat + |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| describe.skipIf(isWindows)("Bun.write to a FIFO by path", () => { | ||
| // 200 KiB exercises the <256 KiB sync fast path; 1 MiB exercises the | ||
| // thread-pool WriteFile path (string/ArrayBuffer) and the Blob path. | ||
| it.each([ | ||
| ["200 KiB string", "string", 200 * 1024], | ||
| ["1 MiB string", "string", 1 << 20], |
There was a problem hiding this comment.
🟡 The test matrix has no <256 KiB Uint8Array case, so the new fd-handoff block in write_bytes_to_file_fast (Blob.rs:5548-5558) is never executed — the 1 MiB Uint8Array exceeds the 256 KiB threshold at Blob.rs:5109 and skips the fast path entirely. Add ["200 KiB Uint8Array", "u8", 200 * 1024] to the it.each matrix so both sibling entry points receiving the same fix are covered; without it, deleting that handoff block would not break any test.
Extended reasoning...
What the gap is
This PR adds identical fd-handoff logic to two sibling fast-path functions:
write_string_to_file_fast::<true>— the new block at Blob.rs:5445-5457write_bytes_to_file_fast::<true>— the new block at Blob.rs:5548-5558
Both blocks do the same thing: after opening a path with O_WRONLY|O_NONBLOCK, fstat the fd, and if it's not a regular file, hand the open fd to the async WriteFile via *handoff_fd instead of writing synchronously and risking a torn partial write on EAGAIN.
The new test's variant matrix at bun-write.test.js:738-743 is:
it.each([
["200 KiB string", "string", 200 * 1024],
["1 MiB string", "string", 1 << 20],
["1 MiB Uint8Array", "u8", 1 << 20],
["1 MiB Blob", "blob", 1 << 20],
])Why the second handoff block is never reached
The ArrayBuffer fast path is gated at Blob.rs:5109:
} else if let Some(buffer_view) = data.as_array_buffer(global_this) {
if buffer_view.byte_len < 256 * 1024 {
...
write_bytes_to_file_fast::<true>(...)The only Uint8Array case in the matrix is 1 MiB (1 << 20 = 1,048,576 bytes), which is ≥ 256 KiB, so write_bytes_to_file_fast is never called. That case falls straight through to the thread-pool WriteFile path — which exercises the do_write / run_with_fd fixes in write_file.rs, but not the fast-path handoff. The 200 KiB string case exercises write_string_to_file_fast's handoff; the 1 MiB string and Blob cases both exercise the thread-pool path. Nothing exercises write_bytes_to_file_fast's handoff.
Step-by-step: what happens if the handoff block at 5548-5558 is deleted
Take a hypothetical 200 KiB Uint8Array to a FIFO with a slow reader:
buffer_view.byte_len= 204,800 < 262,144 → enterswrite_bytes_to_file_fast::<true>.- Opens the FIFO
O_WRONLY|O_CREAT|O_TRUNC|O_NONBLOCK→ fd. - Without the handoff block, execution falls through to line 5564:
let _close = NEEDS_OPEN.then(|| bun_sys::CloseOnDrop::new(fd));— the close-on-drop guard is armed. - The sync
write()loop writes ~64 KiB (Linux default pipe buffer) then getsEAGAIN. - The
EAGAINarm at lines ~5578-5580 sets*_needs_async = trueand returns. _closedrops, closing the fd. The FIFO's only writer is gone → the reader's blockedread()returns 0 (EOF) and it exits with only the pipe-buffer-sized prefix.- Back in
write_file_internal,dest_opened_fdis stillFd::INVALID(the handoff never set it), so the async fallback reopensO_WRONLY|O_CREAT|O_TRUNC|O_NONBLOCKwith no reader present →ENXIO, and the promise rejects.
This is exactly the torn-write bug the PR fixes for strings — silently unfixed for the ArrayBuffer entry point if that block were removed. But no test in this PR would catch it: the only <256 KiB case is a string.
Why this matters per REVIEW.md
REVIEW.md's test rules (which per the doc's own header "have blocked merges"):
Cover the variant matrix, not just the repro. Every sibling entry point receiving the same fix
Confirm deleting each load-bearing clause of your fix breaks at least one test — a test that passes both ways is worse than no test.
Deleting the load-bearing handoff block in write_bytes_to_file_fast breaks zero tests. Both sibling entry points received the same fix; only one is tested.
Fix
One line in the it.each matrix:
it.each([
["200 KiB string", "string", 200 * 1024],
["200 KiB Uint8Array", "u8", 200 * 1024], // ← add: exercises write_bytes_to_file_fast handoff
["1 MiB string", "string", 1 << 20],
["1 MiB Uint8Array", "u8", 1 << 20],
["1 MiB Blob", "blob", 1 << 20],
])The comment above the matrix ("200 KiB exercises the <256 KiB sync fast path") already states the intent — it just wasn't applied to the u8 kind.
Severity
Marking this nit: the production fix is present and correct on both code paths, so merging as-is causes no user-facing failure. The gap is purely test-coverage — a one-line matrix addition — but it does leave one of the two load-bearing hunks in this PR untested, which REVIEW.md explicitly calls out.
2 participants
Repro
Cause
Two independent faults in the path-destination write paths:
Torn write then ENXIO (string/TypedArray < 256 KiB).
write_{string,bytes}_to_file_fast::<true>opens the FIFOO_WRONLY|O_NONBLOCK, loopswrite(), and onEAGAINsetsneeds_asyncand returns. ItsCloseOnDropguard closes the fd, which removes the FIFO's only writer, so the reader's blockedread()returns 0 and exits with only the pipe-buffer-sized prefix. The async fallback reopensO_WRONLY|O_CREAT|O_TRUNC|O_NONBLOCKwith no reader present and the promise rejectsENXIO.Never settles (Blob/Response or any source >= 256 KiB).
WriteFile::run_with_fdleftcould_block=falsefor every path-opened destination, anddo_writecomputedsys::write(...)once outside aloop { match &result { ... } }; theEAGAIN && !could_block => continuearm re-matched the same stale error forever without re-issuing the syscall or registering forPOLLOUT, so a pool worker spins at 100% with zero syscalls while the reader sits inpipe_r.Bun.file(fifo).writer()already works: its open path fstats and marks pipes pollable, and its streaming writer resumes at the saved offset onPOLLOUTon the same fd.Fix
src/runtime/webcore/Blob.rs,write_{string,bytes}_to_file_fast: after opening a path,fstatthe fd; when not a regular file, hand the already-open fd to the asyncWriteFilevia a newopened_fdparameter instead of writing here, so the FIFO is never closed and reopened mid-payload. Regular-file writes keep the existing synchronous loop.src/runtime/webcore/blob/write_file.rs:do_write: drop the stale-result loop.write(2)on a regular file never returnsEAGAIN, so reaching that arm means the fd is pollable; setcould_block = trueandwait_for_writable().run_with_fd:fstata path-opened fd to derivecould_blockso pipes use thePOLLOUTwait from the start and skippreallocate_file.create/create_with_ctx: accept a pre-opened fd.FileOpener::get_fdalready short-circuits whenopened_fdis set, andis_allowed_to_close()(path-backed) closes it on finish.write_file_with_source_destinationthreads the fd through; ascopeguardcloses it if construction of the source/destination blobs throws beforeWriteFileadopts it.Verification
New
describe.skipIf(isWindows)block intest/js/bun/io/bun-write.test.jswith four cases (200 KiB string via the fast path; 1 MiB string/Uint8Array/Blob via the thread-pool path). Each spawns a child thatmkfifos, opens the read end synchronously (O_RDONLY|O_NONBLOCK) so the write-side open has a reader, hands that fd to a draining grandchild, and assertsBun.writeresolves with the full byte count and the reader receives every byte. Without the fix the 200 KiB case rejectsENXIOwith a short delivery and the 1 MiB cases are killed by the 20 s spawn timeout with a pool worker spinning.Related: #35953 fixes the same
do_writespin forBun.stdoutpipes and dups caller-supplied fds for epoll; this PR covers the FIFO-by-path entry and the fast-path fd handoff. Thedo_writehunk is the same shape and will merge cleanly with whichever lands second.no test proof · iteration 0 · 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