Bun.stdin: reject concurrent text/bytes/arrayBuffer/json reads instead of racing on fd 0 - #35833
Bun.stdin: reject concurrent text/bytes/arrayBuffer/json reads instead of racing on fd 0#35833robobun wants to merge 6 commits into
Conversation
…d of racing on fd 0 Two concurrent Bun.stdin.text() calls each spawned a ReadFile with its own io::Poll; the second EPOLL_CTL_ADD on fd 0 returned EEXIST after the loser had already consumed bytes from the pipe, so one promise rejected with a raw 'EEXIST: file already exists, epoll_ctl' and the other resolved short. A Bun.stdin blob read alongside process.stdin split the piped bytes with no error at all. do_read_file now claims a per-thread in-flight flag for stdin and the second concurrent caller rejects ERR_INVALID_STATE up front; the claim is released in NewReadFileHandler::run so sequential reads keep working. When a ReadableStream has already been cached on the Bun.stdin wrapper (process.stdin creates one), text/json/arrayBuffer/bytes route through that stream instead of do_read_file, so a held reader produces the same ERR_INVALID_STATE as Bun.stdin.stream().getReader().
|
Warning Review limit reached
Next review available in: 8 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 (4)
Comment |
|
Updated 11:28 PM PT - Jul 25th, 2026
❌ @robobun, your commit 1c7e2b8 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35833That installs a local version of the PR into your bun-35833 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #35832; they solve the same EEXIST/truncation bug with different designs:
This PR implements the single-owner reject design (mirroring |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/webcore/Blob.rs:484-491— The reverse ordering is still open:const p = Bun.stdin.text(); process.stdin.on("data", …)— the first call falls through todo_read_file(no cached stream yet) and claims the flag, but the second reachesget_stream_with_cache, which never consultsSTDIN_BLOB_READ_IN_FLIGHT, so it creates aFileReaderon fd 0 that racesp'sReadFileand bytes split silently. Not a regression (pre-existing behaviour, and the less-common ordering), but per REVIEW.md "fix the whole class" it's worth either havingget_stream_with_cachereject stdin when the flag is set, or noting this as a known residual gap in the PR description.Extended reasoning...
What the bug is
This PR guards two of the three orderings in which multiple consumers can race on fd 0:
- Two concurrent
do_read_filecalls — guarded bySTDIN_BLOB_READ_IN_FLIGHTatBlob.rs:484. - Blob helper called after a stream is cached (
process.stdinfirst, thenBun.stdin.text()) — guarded byfd_cached_stream, which routes throughreadableStreamTo*so the WHATWG reader lock rejects.
But the reverse of (2) is unguarded: blob helper first, then stream creation. Nothing in
get_stream_with_cache(Blob.rs:1230-1268) checksSTDIN_BLOB_READ_IN_FLIGHTbefore callingReadableStream::from_blob_copy_ref, so aFileReaderis created on fd 0 while aReadFiletask is already mid-flight on the same fd.The code path
const p = Bun.stdin.text(); // (A) process.stdin.on("data", c => ...); // (B)
- (A) enters
get_text→fd_cached_stream(self, this)returnsNone(no stream cached yet —js::stream_get_cachedis empty) → falls through toget_text_clone→do_read_file→STDIN_BLOB_READ_IN_FLIGHT.replace(true)returnsfalse, so the claim succeeds and aReadFiletask is scheduled on fd 0 via theIoRequestLoop. - (B)
process.stdinis built onBun.stdin.stream()(src/js/builtins/ProcessObjectInternals.ts), which reachesget_stream_with_cache.get_cached(this_value)returnsNone(still no cached stream). The function proceeds directly toReadableStream::from_blob_copy_refat line 1255 without ever readingSTDIN_BLOB_READ_IN_FLIGHT, creates aFileReader-backed stream on fd 0, and caches it.
Grep confirms
STDIN_BLOB_READ_IN_FLIGHTis read in exactly one place (Blob.rs:484, insidedo_read_file);get_stream_with_cachenever touches it.Why the existing guards don't catch it
fd_cached_streamonly fires whenjs::stream_get_cached(this_value)isSome— i.e. when a stream has already been materialised. In this ordering the blob helper runs first, so there is nothing cached andget_texttakes thedo_read_filepath.STDIN_BLOB_READ_IN_FLIGHTis checked only bydo_read_file.get_stream_with_cacheis a different entry point and does not consult it.- The two readers use different poll loops:
ReadFileregisters on theIoRequestLoop's epoll fd, while the stream'sFileReaderuses the VM's uSockets loop. They are separate epoll instances, so the second registration does notEEXIST— the kernel just delivers readability to whichever wakes first, and bytes split silently betweenpandprocess.stdin.
Step-by-step proof
Bun.stdin.text()→get_text(Blob.rs:1270).fd_cached_stream→js::stream_get_cached(this)→None(never called.stream()). ReturnsNone.- Falls through to
get_text_clone→do_read_file. is_stdin_fd_store(self)istrue;STDIN_BLOB_READ_IN_FLIGHT.replace(true)returnsfalse(was unset). Guard passes; flag nowtrue.ReadFilescheduled onIoRequestLoop.process.stdin.on("data", …)→ lazily initialisesprocess.stdin, which callsBun.stdin.stream()→get_stream_with_cache(Blob.rs:1230).get_cached(this_value)→None. No check ofSTDIN_BLOB_READ_IN_FLIGHT. Line 1255 creates aFileReaderon fd 0; line 1263 caches it.- Both the
ReadFile(step 4) and theFileReader(step 6) now issueread(2)on fd 0 from independent poll loops. Bytes interleave; neither side errors.
This is exactly the failure mode the PR description names ("bytes split silently"), just for the ordering the tests don't exercise.
Impact and severity
- Not a regression: this ordering behaved identically before the PR; the PR is a strict improvement.
- Less common ordering: users typically wire
process.stdinlisteners before awaitingBun.stdin.text(), so the guarded direction is the one most people hit. - Node parity: the PR description already notes Node has no single-owner stdin lock either; the locked-stream rejection is framed as a quality improvement, not a compat requirement.
- Design cost of closing it: making
get_stream_with_cachethrow synchronously when the flag is set would causeprocess.stdininitialisation to throw, which may be worse UX than splitting bytes. A softer option is to have the freshly-created stream error asynchronously, or just document the gap.
Given all of the above this is nit, not blocking — but per REVIEW.md ("Fix the whole class in the same PR… If a site is intentionally excluded, say so in the PR"), it's worth either closing the gap or explicitly acknowledging it as a known residual in the PR description.
How to fix
At the top of
get_stream_with_cache, mirror thedo_read_fileguard for the stdin case:if is_stdin_fd_store(self) && STDIN_BLOB_READ_IN_FLIGHT.get() { return Err(global_this.throw( jsc::ErrorCode::INVALID_STATE, format_args!("stdin is already being read by another Bun.stdin consumer"), )); }
(or reject the returned stream asynchronously if a synchronous throw from
process.stdininit is undesirable). Alternatively, add a sentence to the PR description naming this ordering as an intentional exclusion. - Two concurrent
-
🟡
src/runtime/webcore/Blob.rs:1324-1329—get_form_data(line 1333, right afterget_bytes) still goes straight toto_form_data→do_read_filewithout checkingfd_cached_stream, soprocess.stdin.on('data')+Bun.stdin.formData()still spawns a freshReadFileon fd 0 and steals bytes from the stream reader.readable_stream_to_form_dataalready exists onJSGlobalObject(used in Body.rs:401) — the same routing applies, just needs the content-type passed as the second arg. Nit sinceBun.stdin.formData()is exotic and the PR title scoped to text/bytes/arrayBuffer/json, but per REVIEW.md "fix the whole class" it's the missed fifth sibling.Extended reasoning...
What's missed
The PR updates four Blob body accessors —
get_text,get_json,get_array_buffer,get_bytes— to checkfd_cached_stream(self, callframe.this())first and route through the cachedReadableStreamwhen one exists.get_form_datasits immediately afterget_bytesin the sameimplblock (Blob.rs:1333-1339) and follows the identical pattern (to_form_data→do_read_file::<ToFormDataWithBytesFn>at Blob.rs:3268-3269), but it is not updated. ItsCallFrameparameter is still the discarded_.Why the flag guard doesn't cover it
The PR adds two guards:
STDIN_BLOB_READ_IN_FLIGHTindo_read_file— this DOES protectformData(), so two concurrentBun.stdin.formData()calls (orformData()+text()) now correctly reject the second withERR_INVALID_STATE.fd_cached_streamrouting in each accessor — this is what handles theprocess.stdincase, andformData()doesn't get it.
process.stdinreads via the cached stream's reader (per the PR description). It never callsdo_read_file, so it never setsSTDIN_BLOB_READ_IN_FLIGHT. WhenBun.stdin.formData()is then called,do_read_filefinds the flagfalse, claims it, and schedules a freshReadFileon fd 0 — exactly the second failure mode from the PR description ("ignores the reader, reads fd 0 anyway; bytes split silently"), unfixed for this one method.Step-by-step trace
- User pipes multipart data to stdin and runs
process.stdin.on('data', c => …). This materialises the cachedReadableStreamon theBun.stdinwrapper and locks it with a reader.STDIN_BLOB_READ_IN_FLIGHTremainsfalse. - User calls
await Bun.stdin.formData(). get_form_data(Blob.rs:1333) — nofd_cached_streamcheck — callsto_form_data.to_form_data(Blob.rs:3268):needs_to_read_file()is true →do_read_file::<ToFormDataWithBytesFn>(global).do_read_file(Blob.rs:484):is_stdin_fd_store(self)is true,STDIN_BLOB_READ_IN_FLIGHT.replace(true)returnsfalse→ guard passes, aReadFiletask is scheduled on fd 0.- Two independent readers now compete for fd 0: the cached stream (feeding
process.stdin) and the newReadFile. Bytes split silently between them; whichever registers its poll second may still surface the rawEEXIST: epoll_ctl. - Even in the common case where
Bun.stdinhas no content-type set, the "Invalid encoding" rejection happens into_form_data_with_bytes(Blob.rs:3000) afterdo_read_filehas already consumed bytes from fd 0 — soprocess.stdinstill sees a truncated stream beforeformData()rejects.
Fix
Mirror the four siblings, passing the content type:
fn get_form_data(&self, global_this: &JSGlobalObject, callframe: &CallFrame) -> JsResult<JSValue> { if let Some(stream) = fd_cached_stream(self, callframe.this()) { let content_type = /* self.get_content_type() as JSValue, or js_undefined() */; return bun_jsc::from_js_host_call(global_this, || { global_this.readable_stream_to_form_data(stream, content_type) }); } ... }
readable_stream_to_form_dataalready exists (JSGlobalObject.rs:1188) and is used the same way inBody.rs:401, so this is not novel plumbing.Severity
Nit. Piping multipart/form-data into stdin while also reading
process.stdinis an exotic combination nobody has filed an issue about, and the PR title explicitly scoped to "text/bytes/arrayBuffer/json". This is a residual gap in pre-existing behaviour, not a regression the PR introduces. Flagging it because REVIEW.md's "Fix the whole class in the same PR — grep for every sibling site sharing the pattern" applies directly:get_form_datais the fifth sibling in the same impl block sharing the identicaldo_read_fileentry, and the fix is a mechanical copy of the four that were done.
|
Both findings are accurate; added an Intentionally not covered section to the PR body:
|
Referencing process.stdin (e.g. process.stdin.isTTY) caches Bun.stdin's ReadableStream without locking it; routing text() through that stream marked it disturbed so a second sequential text() rejected instead of resolving at EOF. Route only when the cached stream is actually locked, and narrow the check to stdin so JSBuildArtifact receivers never reach JSBlob::stream_get_cached. Also make the in-flight flag a process-wide AtomicBool since the IoRequestLoop epoll set is shared across Workers.
There was a problem hiding this comment.
All four prior inline findings are addressed and I found nothing new this pass, but this needs a maintainer's call on reject-vs-coalesce (open sibling #35832) and the documented not-covered gaps before landing.
What was reviewed:
- Claim/release pairing:
swap(true, SeqCst)indo_read_fileis released inNewReadFileHandler::runon both theResultandErrarms (release precedes thematch), and the WindowsReadFileUVpath reaches the samerunvia theReadFileUvHandlerblanket impl. locked_stdin_streamnow gates onis_stdin_fd_store+is_locked_value, so an unlocked cached stream (bareprocess.stdin.isTTY) falls through todo_read_file;Bun.file(0).text()on a fresh wrapper has no cached stream and also falls through.self.dupe()intohandler.contextshares the sameStoreRef, sois_stdin_fd_store(&blob)in the completion handler still matches and the claim is released.
Extended reasoning...
Overview
The PR touches src/runtime/webcore/Blob.rs (+76), ReadableStream.rs (+4), blob/read_file.rs (+3), and adds a 99-line test file. It introduces a process-wide AtomicBool guarding concurrent ReadFile tasks on fd 0, and routes Bun.stdin.{text,json,arrayBuffer,bytes}() through the cached ReadableStream's locked-error path when process.stdin (or a manual reader) already holds it. All four of my earlier inline findings — sequential-after-isTTY regression, over-broad fd match / JSBlob cast, per-thread vs process-wide flag scope, and the missing json test-matrix entry — were addressed in ac43612 and 1c7e2b8, and the worker-termination stuck-claim case was documented as a lateral change.
Security risks
None identified. The change is confined to stdin blob consumption; no auth, crypto, path handling, or untrusted-input parsing is touched. The new state is a single AtomicBool with SeqCst swap/store; no lock-free tricks or shared mutable data beyond that.
Level of scrutiny
Moderate-to-high. This is native concurrency code that adds process-global mutable state and changes user-visible behaviour of a Bun-native API (Bun.stdin.*() now rejects ERR_INVALID_STATE where it previously raced or split bytes). The PR body explicitly leaves the reject-vs-coalesce design choice open against sibling #35832, and the Intentionally not covered section documents three gaps (reverse ordering, formData(), worker-termination) that a maintainer should sign off on rather than a bot.
Other factors
Test coverage is solid for the stated scope: 7 subprocess tests (one per consume method under a locked reader, plus concurrent, sequential-after-isTTY, and the untouched fast path), all test.concurrent, all pipe 1 MiB and assert exact byte counts and the specific error code. The evidence block shows the suite fails on main and passes on the PR under both ASAN-debug and release. I did not approve because (a) the reject-vs-coalesce API decision is explicitly left to a maintainer with a competing open PR, and (b) process-global state guarding a shared kernel resource with documented lifecycle gaps is the kind of change REVIEW.md flags for maintainer agreement.
|
CI status: the diff compiles clean on every lane that ran ( The new test ( |
2 participants
Problem
Three stdin consumers share fd 0:
Bun.stdin.stream()(cached on the JS wrapper),process.stdin(built on that cached stream's reader), andBun.stdin.text()/bytes()/arrayBuffer()/json()(each spawns a freshReadFiletask with its ownio::Poll). Only the first two honour the WHATWG reader lock; the blob helpers ignored it and read fd 0 directly.Two visible failures:
The second
ReadFileissuesEPOLL_CTL_ADDon a fd already in theIoRequestLoopepoll set (eachio::PolltracksWasEverRegisteredindependently), so the kernel returnsEEXIST. The loser has already consumed bytes before it needs to wait; its error path drops that buffer.Fix
do_read_fileclaims a process-wideSTDIN_BLOB_READ_IN_FLIGHTatomic before scheduling a stdinReadFile; a second concurrent caller rejectsERR_INVALID_STATEimmediately.NewReadFileHandler::runreleases the claim on completion so sequential reads keep working.ReadableStreamcached on theBun.stdinwrapper is locked (held byprocess.stdinor a manual.getReader()),get_text/get_json/get_array_buffer/get_bytesroute throughreadableStreamTo{Text,JSON,ArrayBuffer,Bytes}so the held reader produces the sameERR_INVALID_STATE: ReadableStream is lockedas a secondgetReader()would. A cached-but-unlocked stream (e.g. after a bareprocess.stdin.isTTY) falls through todo_read_fileso sequential reads keep resolving at EOF.Node v26 has no single-owner stdin lock either (
process.stdin+fs.promises.readFile('/dev/stdin')splits silently), so the locked-stream rejection is a quality improvement rather than a compat requirement; not surfacing a rawepoll_ctlerrno is a bug fix.Supersedes #35831 (that PR routed through the cached stream but left two concurrent blob reads racing).
Verification
test/js/bun/util/bun-stdin-concurrent-read.test.tspipes 1 MiB into a child and asserts:Bun.stdin.text()EEXIST, other truncatedERR_INVALID_STATEprocess.stdin.on('data')+Bun.stdin.{text,bytes,arrayBuffer}()process.stdinreceives full 1 MiB, blob rejectsERR_INVALID_STATEprocess.stdin.isTTYthen sequentialtext(),text()""""(no regression)text()Existing
Bun.stdinregressions (07500,27849,29787),bun-stdin-slice,process-stdinandblob.test.tsall pass.Intentionally not covered
Bun.stdin.text()started beforeprocess.stdinis first touched:get_stream_with_cachedoes not consult the in-flight flag, so aFileReadercreated after theReadFileis already running still reads fd 0 independently (silent split, noEEXIST; the two readers sit on different epoll instances). Throwing from.stream()here would makeprocess.stdininitialisation itself throw, which is worse UX than the split. Pre-existing behaviour; the common ordering (listener first) is the one that now rejects.Bun.stdin.formData()whileprocess.stdinholds the reader:get_form_datais not routed through the cached stream (it would need the boundary plumbed intoreadable_stream_to_form_data). Two concurrentformData()/text()calls are still caught by thedo_read_fileclaim; only theprocess.stdin+formData()combination still reads fd 0 before rejecting on the missing content-type.[review] gate passed · iteration 2 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 2
evidence per changed file
Bun.stdin.text(): the in-flight claim is released inNewReadFileHandler::run, which runs on the claiming thread's event loop. A Worker terminated while itsReadFileis parked in the io loop never reaches that callback, so the claim stays set and laterBun.stdinblob reads rejectERR_INVALID_STATE. On main the same scenario already rejectsEEXIST(the dead worker'sio::Pollon fd 0 stays registered), so this is a lateral change in error text, not a regression; clearing the claim inDropwould not help because the handler box is leaked on that teardown path.