Stream network bodies to disk in Bun.write instead of buffering them#31739
Stream network bodies to disk in Bun.write instead of buffering them#31739alii wants to merge 9 commits into
Conversation
|
Updated 1:05 AM PT - Jun 10th, 2026
❌ @robobun, your commit 9b99c2e has some failures in 🧪 To try this PR locally: bunx bun-pr 31739That installs a local version of the PR into your bun-31739 --bun |
alii
force-pushed
the
ali/bun-write-stream-to-disk
branch
from
1068bd3 to
8a5de24
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@robobun adopt and fix any ci and review comments |
|
Adopted. All review threads on this PR and #31683 are addressed and resolved; the stack is current with main (incl. the #32042 bunx skip). Build 61686 final: 283 jobs passed, zero test or build failures. The "failed" verdict is entirely from 3 jobs that expired un-run waiting for scarce agents (2 darwin-14-aarch64, 1 windows-11-aarch64) during last night's queue congestion. Retrying those 3 from the Buildkite UI (write access needed) keeps the 283 greens; a fresh build would also work but re-rolls everything. Earlier history: fixed the Windows |
There was a problem hiding this comment.
Thanks — the O_TRUNC and Windows mkdirp fixes look right and the new tests cover them. Nothing further from me, but this reroutes a core Bun.write path through unsafe FileSink/refcounting code with platform-specific arms and a user-visible resolve-value change, so it should get a human pass (and it's stacked on #31683).
Extended reasoning...
Overview
This PR diverts Bun.write(path, response|request) for network-fed bodies from the buffered WriteFile path to a streaming FileSink path via pipe_readable_stream_to_blob. Touches src/runtime/webcore/Blob.rs (~200 lines: body_dispatch restructure, Windows open arm with O_TRUNC + mkdirp recovery, FileSinkPipe resolve-value fixes), src/runtime/webcore/FileSink.rs (~60 lines: Options::flags() honors truncate, new mkdirp field with ENOENT-retry in setup(), truncate default flipped true→false), plus new tests in bun-write.test.js and an updated assertion in s3-write-to-local-file.test.ts.
Prior review status
I previously flagged two correctness bugs (missing O_TRUNC; Windows arm ignoring mkdirp_if_not_exists) and two test nits. All four are addressed in 4eff907: Options::flags() now gates O_TRUNC on self.truncate, the Bun.write arm opts in (truncate: true, mkdirp: …), the Windows arm opens with O_TRUNC and does the ENOENT→mkdir_recursive→retry recovery, and the RSS test that prompted the nits was replaced with a deterministic mid-stream-gate test. I verified the truncate default flip is behavior-preserving for the other FileSinkOptions constructors (streams.rs:235/255/269, Blob.rs:2033/2042 — the .writer() paths) since the field was dead before this change. The bug-hunting pass on the current revision found nothing.
Security risks
None apparent — no auth/crypto/permissions surface. The change is I/O routing within an existing trust boundary (local filesystem writes the user already requested).
Level of scrutiny
High. This is a behavioral rewrite of a core primitive on a hot path, with: (a) substantial unsafe Rust — raw *mut reborrows of body values, intrusive refcounting on FileSink, (*self.sink).written.get() reads in the resolve callbacks; (b) divergent Windows/POSIX implementations; (c) a user-visible semantic change (ReadableStream-backed Bun.write now resolves with byte count instead of 0, including the pre-existing S3→file path); (d) a dependency on stacked PR #31683's FileSinkPipe. None of those are red flags individually, but together they put this well outside "simple/mechanical."
Other factors
Test coverage for the new path is solid (sized + chunked content/count, createPath both ways, truncation over a larger file, deterministic mid-stream proof). No CODEOWNERS on the touched paths. Deferring rather than approving because the scope and unsafe-code surface warrant a human reviewer, not because of any specific remaining concern.
Bun.write(path, response) and Bun.write(path, request) buffered the entire body in memory before writing: the Locked-body destination arm parked a WriteFileWaitFromLockedValueTask on the body and let the ValueBufferer accumulate every chunk, then performed one write. A 256MB streamed response grew RSS by ~1.4GB before any byte reached the disk, and larger-than-memory downloads could not complete at all. Network-fed bodies now stream through the FileSink: the destination arm extracts the body's ReadableStream the same way the S3 destination arm does and routes it through pipe_readable_stream_to_blob, which pipes ByteStream-backed sources native-to-native. Peak memory becomes the sink's high-water mark, and disk writes start with the first chunk. Only bodies that install the on_start_streaming drain hook divert - fetch responses and server request bodies, which are exactly the unbounded cases. The check runs before the stream is materialized: bodies driven by other producers (HTMLRewriter transforms) depend on the ValueBufferer to pump them, and materializing a stream first silently breaks that, so they keep the buffered path. Supporting changes: - FileSink::Options gains mkdirp: on ENOENT for a path open, create the missing parent directories and retry once - the recovery the buffered path already performed. Bun.write's createPath option (default true) flows through pipe_readable_stream_to_blob into the sink; .writer() keeps plain open semantics. - The streaming completions now resolve with the total bytes written (FileSinkPipe's synchronous arm resolved with the final flush's count, the JS-sink wrapper resolved with 0), matching the buffered paths and the documented return value. Verified against the previous implementation: body bytes, resolve counts, createPath default and opt-out behavior (including the ENOENT error), chunked and sized bodies, request bodies, and Bun.file destinations are identical. New tests cover content/count for sized and chunked responses, createPath both ways, and an RSS ceiling test that fails on the buffered implementation (delta >= body size) and passes streaming (a fraction of it).
The base branch pinned Bun.write's streaming resolve value at 0 (parity with the JS streaming loop). This branch resolves with the total bytes written across every settle path, so the pins assert the count.
The FileSink-backed destination arm diverged from the buffered WriteFile path it replaces: - No O_TRUNC: writing a streamed body over a larger existing file left stale trailing bytes. Options::flags() now honors the (previously dead) truncate field; Bun.write destinations opt in, .writer() keeps its open-in-place semantics. - The Windows arm never consumed mkdirp_if_not_exists (unused-variable error under deny(warnings)) and rejected with ENOENT instead of honoring createPath. It now opens with O_TRUNC and retries once after mkdir_recursive, mirroring WriteFileWindows. Replace the RSS-ceiling test with a deterministic one: hold the body open mid-stream behind a gate and assert the first chunk already landed on disk (the buffered path never touches the destination before the body completes). RSS deltas of a same-process server/client transfer on release mimalloc measured 155-173MB for a 64MB body on CI — not a sound proxy for streaming. Also add a truncation regression test and raise the file's default test timeout for loaded debug/ASAN runners. bun_core::PathString was deleted on main; the mkdirp block now uses CowSlice like mkdir_if_not_exists does.
Two fixes in the streaming Bun.write destination path: Aborted-body panic: to_readable_stream runs the producer's drain callback, which can observe an abort and replace the Locked body with Null instead of handing back a stream. The fallthrough then parked the buffered task behind a Locked-only re-match and panicked on unreachable. Settle non-Locked variants before the task: an Error body rejects with its error, Null/Empty reject with the abort reason - matching what the buffered path's ValueBufferer would have delivered. Reproducible with AbortController + fetch + Bun.write(path, response), and with Bun.write(path, req) after a client disconnect. Wrong resolve value on pollable-fd destinations: Bun.write resolved with FileSink.written, which is accumulated from the writer's drain callbacks - those mix at-accept and at-drain reports for pollable fds and undercount (CI observed 3.7MB resolved for a fully-delivered 32MB body). Count the bytes the pipe hands to the sink instead and resolve with that on every settle path (synchronous, fulfilled, and the then-shims). Adds an abort regression test; the pending-flush pipe test now passes deterministically with the full byte count.
alii
force-pushed
the
ali/bun-write-stream-to-disk
branch
from
4eff907 to
5f43710
Compare
FileSinkPipe resolves with its own accepted total because FileSink.written mixes at-accept and at-drain reports on pollable fds. The JSSink-loop wrapper still resolves with written: readStreamIntoSink discards per-chunk write results, so no accepted total exists there. Regular-file destinations are force_sync and exact either way.
|
@robobun fix review comments |
# Conflicts: # src/runtime/webcore/Blob.rs
|
All review threads here were already addressed and resolved (5/5: O_TRUNC, Windows createPath/mkdirp, the two test nits, and the resolve-count doc comment from the last claude pass) — nothing was left open on this PR or #31683. What was actually blocking was CI, so I refreshed the stack instead: merged current main into |
2 participants
What does this PR do?
Bun.write(path, response)/Bun.write(path, request)buffered the entire body in memory before writing anything: a 256MB streamed response grew RSS by ~1.4GB before the first byte reached disk, and larger-than-memory downloads could not complete at all. Worse, if the body stalled mid-stream afterBun.writewas called, the buffered wait could deadlock and the returned promise never settled (reproducible on released 1.3.5 with a server that pauses between chunks).Network-fed bodies now stream through the
FileSink: the destination arm extracts the body's stream the same way the S3 destination arm does and routes it throughpipe_readable_stream_to_blob, which pipes ByteStream-backed sources native→native (#31683'sFileSinkPipe). Disk writes start with the first chunk, and peak memory no longer includes a full buffered copy of the body.Which bodies divert
Only bodies that install the
on_start_streamingdrain hook — fetch responses and server request bodies, which are exactly the unbounded cases. The check runs before the stream is materialized: bodies driven by other producers (e.g.HTMLRewriter.transformoutputs) depend on theValueBuffererto pump them, and materializing a stream first silently breaks that, so they keep the buffered path.Bun.write semantics preserved
WriteFilepath opens withO_TRUNC; the FileSink path didn't, which would leave stale trailing bytes when writing a smaller body over a larger existing file.FileSink::Options::flags()now honors the (previously dead)truncatefield —Bun.writedestinations opt in,.writer()keeps its current open-in-place behavior — and the Windows arm opens withO_TRUNCdirectly.createPath(defaulttrue) —FileSink::Optionsgainsmkdirp: onENOENTfor a path open, create the missing parent directories and retry once. The Windows arm performs the same recovery (it previously ignored the parameter, which also failedbuild-rustunderdeny(warnings):unused variable: mkdirp_if_not_exists).FileSinkPipe's synchronous arm resolved with only the final flush's count; the JS-sink wrapper resolved with0), matching the buffered paths and the documented return value. Also corrects the S3→file resolve value from Pipe S3 downloads into local files natively #31683.Tests
In
test/js/bun/io/bun-write.test.js:createPathboth waysSuites run green:
test/js/bun/io/,body.test.ts,streams.test.js,filesink.test.ts,test/js/bun/s3/, HTMLRewriter.Stacked on #31683 (uses
FileSinkPipe); review that first. Both branches carry a merge of currentmainso the binary-size check compares against the same WebKit.