Bun.write: stream ReadableStream sources instead of stringifying them - #31689
Bun.write: stream ReadableStream sources instead of stringifying them#31689robobun wants to merge 16 commits into
Conversation
WalkthroughAdds ReadableStream, AsyncIterable/AsyncGenerator (and thunk) inputs to Bun.write, BunFile.write, and S3 write APIs; implements stream piping with mkdirp/mode handling, rejects disturbed/locked/errored streams, and returns accurate written byte counts. Tests cover stream shapes, error cases, truncation, and mode/createPath behavior. ChangesReadableStream and async iterable support for write operations
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:36 PM PT - Jul 28th, 2026
❌ @robobun, your commit 4d5399c has some failures in 🧪 To try this PR locally: bunx bun-pr 31689That installs a local version of the PR into your bun-31689 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/runtime/file-io.mdx (1)
91-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd async iterable and async generator types to the supported inputs list.
The layer description and PR objectives indicate that
Bun.writeshould acceptAsyncIterable,AsyncGenerator, and async generator factory functions in addition toReadableStream. The documentation currently only mentionsReadableStream(line 98).📝 Suggested additions to complete the documentation
- `TypedArray` (`Uint8Array`, et. al.) - `Response` - `ReadableStream` +- `AsyncIterable` +- `AsyncGenerator`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/runtime/file-io.mdx` around lines 91 - 99, Update the supported-inputs list for Bun.write to include async iterables and async generators: add "AsyncIterable" and "AsyncGenerator" (and note factory functions that return async generators / async iterables) alongside ReadableStream as accepted second-argument types; ensure the prose around the second argument clarifies Bun.write accepts AsyncIterable/AsyncGenerator/async generator factory functions in addition to the existing types and matches the layer/PR objectives for Bun.write.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/runtime/file-io.mdx`:
- Around line 281-284: The write(...) signature currently lists ReadableStream
but omits supported async input types; update the input union for the write
function signature to also include AsyncIterable and AsyncGenerator forms (e.g.,
AsyncIterable<...> and AsyncGenerator<...>) to match the runtime and docs; when
adding them, match the exact generic constraint used in your TypeScript defs
(confirm whether the project uses Uint8Array, ArrayBufferView, or an
unconstrained generic in packages/bun-types/bun.d.ts) and ensure the symbol is
the same write(destination: string | number | BunFile | URL, input: ...)
declaration so consumers see the full supported async types.
In `@src/runtime/webcore/Blob.rs`:
- Around line 5436-5466: Add a preflight check for an already-errored
ReadableStream like the existing disturbed/locked guards: after obtaining
`stream` from `ReadableStream::from_js` and before calling
`destination_blob.pipe_readable_stream_to_blob`, check
`stream.is_errored(global_this)` || `stream.value.get(global_this,
"errored")?.is_some_and(|v| v.to_boolean())`; if true call
`destination_blob.detach()` and return an error via
`global_this.throw_invalid_arguments(format_args!("ReadableStream has
errored"))`. This mirrors the `stream.is_locked` logic and prevents truncation
when the stream has already errored.
- Around line 4552-4570: The current mkdirp_parent_of(dest_path: &[u8]) -> bool
hides all errors from NodeFS::mkdir_recursive and always yields false on
failure, causing callers (like pipe_readable_stream_to_blob/open code paths) to
report earlier ENOENT instead of the real error (EACCES, ENOTDIR, etc.); change
mkdirp_parent_of to return a Result<(), bun_sys::Result> (or the same
result/error type used by mkdir_if_not_exists), propagate and return the exact
error from node_fs.mkdir_recursive rather than collapsing it to false, and
update callers to handle the Result (mirroring the existing mkdir_if_not_exists
error propagation behavior).
In `@test/js/bun/io/bun-write.test.js`:
- Line 842: The test is asserting an async stream error with a synchronous
matcher; update the assertion to use the promise rejection matcher for
Bun.write. Replace the current expect wrapper around an async function (the one
calling Bun.write with filePath and stream) with a promise-based assertion using
expect(Bun.write(filePath, stream)).rejects.toThrow("stream go boom") so the
asynchronous stream rejection is correctly tested.
---
Outside diff comments:
In `@docs/runtime/file-io.mdx`:
- Around line 91-99: Update the supported-inputs list for Bun.write to include
async iterables and async generators: add "AsyncIterable" and "AsyncGenerator"
(and note factory functions that return async generators / async iterables)
alongside ReadableStream as accepted second-argument types; ensure the prose
around the second argument clarifies Bun.write accepts
AsyncIterable/AsyncGenerator/async generator factory functions in addition to
the existing types and matches the layer/PR objectives for Bun.write.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3ec7f6e-7dee-4010-b2a8-d43661e7a4a2
📒 Files selected for processing (6)
docs/runtime/file-io.mdxpackages/bun-types/bun.d.tspackages/bun-types/s3.d.tssrc/runtime/webcore/Blob.rssrc/runtime/webcore/FileSink.rstest/js/bun/io/bun-write.test.js
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/webcore/Blob.rs:5436-5459— The locked-stream pre-check added in f0ed335 doesn't cover streams that are already in the errored state —new ReadableStream({ start(c) { c.error(...) } })hasdisturbed=falseandlocked=false, so it passes both guards here, thenpipe_readable_stream_to_blobopens the destination withO_TRUNCbeforeassign_to_streamobserves the errored state and rejects, irreversibly truncating an existing destination to 0 bytes. This was noted as a remaining gap in the (now-resolved) locked-stream thread; the new "rejects when the stream errors" test usespull(notstart) on a non-pre-existing path so it doesn't observe the truncation. Fix: either inspect the stream's[[state]]before opening, or defer theO_TRUNCopen until afterassign_to_streamhas acquired a reader (which would also harden the locked case).Extended reasoning...
What the bug is
Commit f0ed335 added pre-checks in
write_file_internal(Blob.rs:5437-5458) so aReadableStreamsource rejects beforepipe_readable_stream_to_blobopens the destination withO_TRUNC. It guards onis_disturbedandlocked. But a stream whosestart()synchronously callscontroller.error(...)is in the WHATWGerroredstate with[[disturbed]] = false(only read/cancel set that) andlocked = false(no reader acquired) — so it passes both guards and enterspipe_readable_stream_to_blob, which opens the destination withO_TRUNCfirst (POSIX:FileSinkOptions { truncate: true }→Options::flags()returns… | O_TRUNC; Windows:bun_sys::open(..., O::TRUNC, ...)) and only then callsJSSink::assign_to_stream, which rejects on the firstread(). The destination is already 0 bytes by the time the promise rejects.The code path
write_file_internal(Blob.rs:5436):ReadableStream::from_jssucceeds;is_disturbed→false;is_lockedand thelockedgetter →false(an errored stream has no reader). Both guards pass.pipe_readable_stream_to_blobopens the destination — POSIX:(*sink).start(&stream_start)withtruncate: true→FileSink::setup→open_for_writingwithO_NONBLOCK|O_CLOEXEC|O_CREAT|O_WRONLY|O_TRUNC; Windows:bun_sys::open(path, WRONLY|CREAT|TRUNC|NONBLOCK, mode). The kernel truncates the existing file to 0 bytes on open.- Only then does
JSSink::assign_to_streamrun.readStreamIntoSinkcallsgetReader()(allowed on errored streams), and the firstreader.read()returns a rejected promise. Afterdrain_microtaskstheRejectedarm fires andBun.writereturns a rejected promise — but the file on disk is already empty.
Why existing code/tests don't catch it
The two new pre-checks only cover
disturbedandlocked; per the Streams spec,ReadableStreamDefaultControllerErrorsets[[state]] = "errored"and[[storedError]]but neither disturbs nor locks the stream. The PR's new"rejects when the stream errors"test errors frompull(the stream is still"readable"at the time of the guard check) and writes to a path inside a freshtempDirthat does not pre-exist, so the truncation has no observable effect there. This gap was explicitly called out in the extended reasoning of the now-resolved locked-stream thread ("The synchronously-errored-stream case is harder to pre-validate without inspecting[[state]]"), but f0ed335 only addressed the locked case before the thread was resolved.Step-by-step proof
await Bun.write("data.bin", Buffer.alloc(1_000_000)); // 1 MB on disk const s = new ReadableStream({ start(c) { c.error(new Error("boom")); } }); // s.locked === false, ReadableStream is not disturbed await Bun.write("data.bin", s).catch(e => console.log(e.message)); // "boom" console.log((await Bun.file("data.bin").stat()).size); // 0 — original 1 MB destroyed
sis constructed;startruns synchronously,controller.error(new Error("boom"))transitions[[state]]to"errored".[[disturbed]]staysfalse, no reader →locked === false.write_file_internal:is_disturbed→ false (guard passes);is_locked→ false andstream.locked→ false (guard passes).pipe_readable_stream_to_blobopensdata.binwithO_TRUNC→ file is now 0 bytes.assign_to_stream→readStreamIntoSink→getReader()succeeds →read()rejects withError("boom")→ afterdrain_microtasks, theRejectedarm returns a rejected promise.- The user's
awaitrejects with"boom", butdata.binis permanently empty.
Impact
Irreversible data loss on an existing destination file, triggered by a programmer error in the source argument. Both halves of the destructive ordering (the
O_TRUNCwiring and the user-reachableReadableStreamcall site) are introduced by this PR, so this is PR-introduced. The trigger (passing a synchronously pre-errored stream) is narrower than the locked-stream case, but it's the same data-loss class that justified fixing the locked case in f0ed335.Fix
Two options:
- Pre-validate: before entering
pipe_readable_stream_to_blob, inspect the stream's[[state]](e.g. via the internal@stateslot or by readingstream.value's state) and reject with the stored error if it's already"errored". - Defer the open (more robust): move the
O_TRUNCopen until afterassign_to_streamhas successfully acquired a reader. This covers the errored case, hardens the locked case without needing the dualis_locked/locked-getter check, and protects against any future synchronous-failure mode inassignToStream.
The
"rejects when the stream errors"test should also be extended (or a new test added) that errors fromstartinstead ofpull, pre-populates the destination, and asserts its contents survive — mirroring the updated"throws on a locked stream without touching the destination"test.
|
Re the last review's 🔴 on pre-errored streams: that landed in 37aa69a (pushed while the review was running) — |
|
CI status for maintainers, build 84278 (final) at Review follow-ups on top of the rebased branch: hermetic mock-S3 test for See "Rebase notes" in the PR description for the resolutions against #33193 and #33538. Ready for review/merge. |
15d9e74 to
344ab84
Compare
|
I root-caused One gap: the fix covers S3 destinations (it is all in the shared main...farm/0b76023f/fix-s3-write-readablestream The relevant piece is the |
4490af8 to
c5eae7e
Compare
The locked-stream preflight consulted the public locked getter, which user code can shadow with Object.defineProperty, letting a locked stream reach the O_TRUNC open before the pipe's internal reader acquisition throws. Add ReadableStream__hasReader, which reads the $reader private slot the way $isReadableStreamLocked does, and drop the shadowable fallback.
c5eae7e to
b7846b9
Compare
|
Rebased onto main at 789be97 (523 commits). No conflicts; Verification after rebase (
Repro from the report, with this build: The plain-object case is unchanged: |
There was a problem hiding this comment.
Beyond the inline nit, two things were checked and ruled out this pass: (1) the Options::truncate default flip from true→false is behavior-preserving for Bun.file().writer() because flags() previously discarded the field entirely (let _ = self), so the three ..Default::default() sites in streams.rs get no O_TRUNC before or after; (2) the early-reject paths in write_file_internal (disturbed/locked/errored) not cancelling the stream — they run before any reader is acquired, so there is nothing to cancel.
Extended reasoning...
The one open finding is a nit; the PR itself is a substantial feature addition (new Bun.write/BunFile.write/S3 input types, FileSink open-flags change, new C++ FFI export, cross-platform mkdirp retry) that has been through several review rounds with all prior threads resolved. Not approving given the API-surface and core-runtime scope.
…he caller's frame
There was a problem hiding this comment.
No new findings this pass; every prior inline comment (locked/errored preflights, createPath/mode threading, sync-direct-stream byte count, simdutf UTF-16 length, S3 coverage, #[inline(never)]) has been addressed. Deferring to a maintainer because this adds user-facing API surface and touches refcounted native I/O across POSIX/Windows arms — worth a human sign-off.
What was reviewed:
Options::truncatedefault flip (true→false) is safe:flags()previously ignoredself, so no existing caller changes behavior; only the new pipe path opts intoO_TRUNC.received_bytescounting vs #33538'sbytes_accepted— independent counters, no double-credit;Done(n)correctly credits 0.ReadableStream__getStoredErrorreads C++m_state/m_storedErrordirectly (tamper-proof), andis_lockednow goes throughisReadableStreamLocked— the shadowed-getter bypass is closed.write_file_with_source_destination's existingpipe_readable_stream_to_blobcall site updated with the two new args; no other callers.
Extended reasoning...
Overview
Adds ReadableStream / async-iterable as accepted Bun.write sources by routing them through the existing pipe_readable_stream_to_blob. Touches Blob.rs (~130 lines: stream detection in write_file_internal, mkdirp_parent_of helper, two new params on pipe_readable_stream_to_blob, byte-count reads from FileSink.received_bytes), FileSink.rs (received_bytes counter, Options::truncate wired into flags(), default flipped to false), ReadableStream.rs + WebStreamsExports.cpp (stored_error accessor), five .d.ts signatures, docs, 19 new file-destination tests, and a hermetic mock-S3 test.
Security risks
None identified. The locked/disturbed/errored preflights read internal C++ stream state (not user-overridable getters), so a hostile stream cannot trick the code into truncating a destination it shouldn't. mkdirp_parent_of uses bun_core::dirname on the caller-supplied destination path — same trust model as the sibling mkdir_if_not_exists.
Level of scrutiny
High. This is native runtime code on a hot user-facing path with raw *mut FileSink derefs, intrusive refcount management, and separate Windows/POSIX open arms. It also changes the semantics of a previously-dead Options field and adds public API surface (types + docs). None of that is mechanical.
Other factors
The PR has been through ~14 rounds of inline findings (mine and CodeRabbit/comment-cop), all resolved and all marked so on the timeline. Test coverage is thorough for file destinations (fetch body, plain/pull/direct/sync-direct/empty streams, async iterable + generator fn, disturbed/locked/shadowed-locked/pre-errored preflights, createPath true/false, mode option, truncation) plus a hermetic S3 body assertion. The rebase notes explain the interaction with #33193 (webstreams C++ rewrite) and #33538 (backpressure accounting) cleanly. The remaining reason to defer is scope, not any specific concern: new API + native memory-management changes across platforms is the kind of thing a maintainer should approve.
|
Still needed as of main 165dc9f: |
Fixes #31681
Fixes #31682
Fixes #37658
Repro
Passing a raw
ReadableStream(a fetch body, a plainnew ReadableStream(...), or viaBun.file(path).write(stream)) wrote the literal string"[object ReadableStream]"and resolved successfully — silent data corruption.Cause
write_file_internalspecial-casesResponse,Request, andArchivesources, then falls through toBlob::get, whose joiner string-coerces unknown objects. A rawReadableStreammatched none of the special cases.Fix
ReadableStream(and async iterable) sources inwrite_file_internaland pipe them into the destination withpipe_readable_stream_to_blob— the same machinery already used for S3→file writes. Covers file, fd, and S3 destinations. Already-used streams throwReadableStream has already been used, matching the Response/Request body arms.0.FileSink.writtenmixes "buffered" and "flushed" reports and can double-count, so this addsFileSink.received_bytes, counting every accepted chunk exactly once (UTF-8 re-encoded length for string chunks, via simdutf).Bun.write's replace semantics.FileSink's open never appliedO_TRUNC(Options.truncatewas dead); it is now wired intoflags()with afalsedefault soBun.file(path).writer()keeps its established in-place overwrite behavior, and the pipe path opts in.assignToStreampromise handled when it rejects synchronously, so a locked/errored stream rejects the returned promise without also surfacing a spurious unhandled rejection.Types (
Bun.write,BunFile.write, S3 writes) and docs updated to includeReadableStream.Verification
New tests in
test/js/bun/io/bun-write.test.js(Bun.write > ReadableStream source): fetch body (256 KiB), plain/pull-based/direct/empty streams, byte-count accuracy, truncation of longer pre-existing files,Bun.file().write(stream), async iterables, disturbed-stream throw, locked-stream and errored-stream rejections.bun bd test test/js/bun/io/bun-write.test.js→ 46 pass (11 new tests fail without the fix)test/js/bun/util/filesink.test.ts(42),test/js/web/streams/streams.test.js(70), spawn stdin stream suites — all passBun.file(path).writer()on an existing longer file still overwrites in place (unchanged behavior)Related issues
Bun.write(Bun.file(path), s3file)leaving stale trailing bytes is the same missing-O_TRUNCopen inpipe_readable_stream_to_blob— fixed by the truncation change (both the POSIXFileSinkopen and the Windows arm).ReadableStreamsandAsyncIterablestoBun.write#17459: the direct forms requested there (Bun.write(dest, readableStream)/Bun.write(dest, asyncIterable)) now work. Not closing it here because its example wraps the generator in aResponse, which hits the separateLocked-body hang (Bun.write: pump the stream when Response/Request body is already a ReadableStream #28988's territory) that this PR does not touch.FileSinkdoes not truncate existing file #25968 (Bun.file().writer()should truncate) is deliberately not changed:writer()keeps its long-standing in-place overwrite behavior; onlyBun.write's replace-style pipe path opts intoO_TRUNC.Rebase notes
Two main changes landed between reviews that touched the same code:
src/jsc/bindings/webcore/ReadableStream.cppand replaced the JS-builtins streams with C++ classes.ReadableStream__getStoredErrorwas ported toWebStreamsExports.cppand now readsJSReadableStream::m_state/m_storedErrordirectly. The separateReadableStream__hasReaderhelper was dropped: the rewrite'sisReadableStreamLocked()already reports reader, reader-less, and detached-native locks correctly, so the preflight inwrite_file_internalusesstream.is_locked()directly. The shadowed-locked-getter test is kept (the C++ lock check reads internal state, not the public getter, so it still passes).write/write_latin1/write_utf16tails tobytes_accepted(buffered_before, &rc)+to_result(rc, accepted). That accounting (per-call pending-promise credit) and this PR'sreceived_bytescounter (cumulative bytes forBun.write's resolved value) are independent; both are kept side by side.After rebase: bun-write 54/0, filesink 46/0, streams 151/0.
no test proof · iteration 11 · 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