webcore(Blob): coalesce concurrent fd-backed blob reads onto a single ReadFile - #35832
webcore(Blob): coalesce concurrent fd-backed blob reads onto a single ReadFile#35832robobun wants to merge 13 commits into
Conversation
… ReadFile
Two concurrent Bun.stdin.arrayBuffer() calls (or any .text()/.bytes() on
the same fd-backed Blob) each spawned a fresh ReadFile over the same fd.
On a pipe that meant:
* both readers raced read() on fd 0, so the byte stream was split
between them and the survivor could resolve short with no error, and
* each ReadFile owned its own io::Poll, so both issued
epoll_ctl(EPOLL_CTL_ADD) on fd 0 and the loser rejected with the raw
'EEXIST: file already exists, epoll_ctl'.
Store now carries an in_flight_blob_reader pointer. do_read_file records
the active ReadFile there for fd-backed stores; a second call attaches
its completion to that reader's extra_completions list instead of
scheduling a new one. ReadFile::then clears the marker and fans the
result out (bytes cloned per extra listener, or the error cloned).
Path-backed Bun.file() is untouched (each read opens its own fd), and a
read started after the first resolves still begins a fresh reader.
WalkthroughChangesBlob reads on non-Windows now coalesce matching fd-backed operations, share completion results, and clear completed readers. Blob entry points register and schedule readers accordingly. New concurrent stdin tests cover coalesced, sequential, mixed-method, and sliced reads. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:11 AM PT - Jul 26th, 2026
❌ @robobun, your commit 7534f3b has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35832That installs a local version of the PR into your bun-35832 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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/read_file.rs:447-455— Coalescing is keyed on theStoreinstance, not the fd, so two Blobs wrapping the same fd via different Stores still race — e.g.Promise.all([Bun.stdin.arrayBuffer(), Bun.file(0).arrayBuffer()])(Bun.stdinuses the thread-localstdio_stores::STDINcache whileBun.file(0)usesvm.rare_data().stdin()— distinctStores), or two independentBun.file(fd)calls on a non-stdio fd (each hitsStore::init_file, fresh Store per call). Not a regression — this behavior is unchanged from before — but the comment atBlob.rs:477("For fd-backed stores (Bun.stdin, Bun.file(fd))…") reads as if the whole class is covered; consider scoping it to "same Store instance" and/or noting the cross-Store gap in the PR description as intentionally deferred.Extended reasoning...
What the gap is
in_flight_blob_readerlives onStore, andtry_coalesce_fd_readonly consults the caller's ownstore.in_flight_blob_reader. That's sufficient when every Blob over a given fd shares oneStore, but there are two paths where the same fd is wrapped by distinctStoreinstances:Bun.stdinvsBun.file(0)—Bun.stdinis served from the thread-local cache insrc/runtime/api/BunObject.rs(stdio_stores::STDIN→build_store()→Store::new(...)).Bun.file(0)goes throughfind_or_create_file_from_path(Blob.rs:3784-3803):fd.stdio_tag()matches, so it usesvm.rare_data().stdin()(rare_data.rs:940-956), which lazy-inits a separateStorevia__bun_stdio_blob_store_new. Two independent caches → two distinctStores over fd 0.- Two
Bun.file(fd)calls on a non-stdio fd — theFdarm offind_or_create_file_from_pathfalls through toStore::init_file(path, None)at line 3810, which doesStore::new(Store { ... })— a fresh heapStoreon every call.
In both cases each
Store'sin_flight_blob_readeris null when the other Blob checks it, sotry_coalesce_fd_readreturnsfalsefor both, both proceed toReadFile::create+mark_in_flight, and bothReadFiles issueepoll_ctl(EPOLL_CTL_ADD)on the same fd → the second getsEEXIST, exactly the failure this PR fixes for the same-Store path.Step-by-step for
Bun.stdin+Bun.file(0)Bun.stdin.arrayBuffer()—stdio_stores::stdin()populates the thread-local slot with Store A (fd 0).do_read_file:try_coalesce_fd_read(&A, ...)→A.in_flight_blob_readeris null → returnsfalse. CreatesReadFile #1,mark_in_flight(&A, #1)setsA.in_flight_blob_reader = #1. Schedules.Bun.file(0).arrayBuffer()—find_or_create_file_from_pathseesstdio_tag() == StdIn, callsvm.rare_data().stdin()which populatesRareData::stdin_storewith Store B (also fd 0, different allocation).do_read_file:try_coalesce_fd_read(&B, ...)→B.in_flight_blob_readeris null (never touched) → returnsfalse. CreatesReadFile #2,mark_in_flight(&B, #2). Schedules.- Both readers run on the work pool: each owns its own
io::Polland issuesepoll_ctl(ADD, fd=0); the loser rejects withEEXIST, and if both survive toread(), the byte stream is split.
For a non-stdio fd like
const b = Bun.file(5); const c = Bun.file(5); Promise.all([b.arrayBuffer(), c.arrayBuffer()])the trace is the same except step 2 goes throughStore::init_fileinstead of the RareData cache.Why this doesn't block merge
- Not a regression. Before this PR every concurrent fd-backed read raced; after, same-Store concurrent reads coalesce and cross-Store reads still race. Nothing gets worse.
- The common case is fixed. Repeated
Bun.stdinaccesses share the thread-local Store, and repeatedBun.file(0)accesses share the RareData Store, soPromise.all([Bun.stdin.x(), Bun.stdin.y()])andPromise.all([Bun.file(0).x(), Bun.file(0).y()])are each fixed. Only mixing the two, or wrapping an arbitrary fd twice, still fails. - A per-fd fix is a larger design. Keying by fd number needs a per-VM
fd → *ReadFilemap with fd-recycling hygiene (an fd closed and reopened between reads must not attach to a stale reader). That's reasonable to defer.
What to change
REVIEW.md's "fix the whole class in the same PR … if a site is intentionally excluded, say so in the PR" applies here. Two low-effort options:
- Tighten the comment at
Blob.rs:477to say "For fd-backed stores, a second concurrent blob read on the same Store…" so it doesn't imply arbitraryBun.file(fd)instances coalesce with each other. - Add a sentence to the PR description noting that cross-Store reads on the same fd (
Bun.stdin+Bun.file(0), or two independentBun.file(fd)blobs on a non-stdio fd) are out of scope and still race.
Optionally, since two independent stdin caches is itself a footgun, having
stdio_stores::build_storeretain theRareDatastore (or vice-versa) would collapse theBun.stdin/Bun.file(0)case without needing a per-fd map — but that's a separate cleanup.
…h raw ptr A sliced and an unsliced concurrent read share a Store, so the unsliced caller would attach to a max_length=3 reader and resolve with 3 bytes. Refuse to coalesce when the caller's (offset, max_length) differs from the in-flight reader's, and mark_in_flight now only publishes into an empty slot so the mismatched second reader cannot overwrite the first's marker. try_coalesce_fd_read now projects offset/max_length/extra_completions through the raw pointer rather than materialising &ReadFile, which would alias the work-pool thread's &mut ReadFile under Stacked Borrows.
…ering assumption do_read_file now initialises the handler's JSPromiseStrong before handing the handler pointer to try_coalesce_fd_read or ReadFile::create, so the 'sole owner' SAFETY holds at the write and the coalesced path returns the same promise without a second init. The sliced+unsliced guard test no longer assumes the sliced reader wins the fd race; the invariant it checks is only that the unsliced result is never resolved with the sliced window.
…Store A Store can be reached from multiple JS threads via the process-global ObjectURLRegistry, so a field on Store is not single-thread by construction and a worker could race main's then()/drop and dereference a freed ReadFile. Move the in-flight tracking to a thread_local HashMap<*const Store, *mut ReadFile> keyed on the store pointer; insert, lookup and removal all happen on the creating event loop (WorkTask::then runs where the task was created), so the value is live while present. This also drops the Store field and its 9 construction sites from the diff. The test's run() helper now drains stdout/stderr concurrently with stdin.end() and swallows EPIPE on the end(), so the sliced+unsliced case can exit without reading all of stdin without the helper throwing.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/webcore/blob/read_file.rs:222-223— nit: this doc comment still says "attached viaStore::in_flight_blob_reader", but 1e54f71 removed that field and moved tracking to theIN_FLIGHT_FD_READERSthread-local — a grep confirms this is the only remaining reference toin_flight_blob_readerinsrc/. Point it atIN_FLIGHT_FD_READERS(matching the siblingtry_coalesce_fd_read/mark_in_flightdocs) or drop the second clause, since the thread-local's own doc already explains the mechanism.Extended reasoning...
What the issue is
The doc comment on
ReadFile::extra_completions(read_file.rs:222) reads:/// JS-thread-only; attached via `Store::in_flight_blob_reader`. pub extra_completions: bun_threading::Guarded<Vec<(ReadFileOnReadFileCallback, *mut c_void)>>,
But commit 1e54f71 ("review: track in-flight readers in a per-JS-thread map instead of on Store") — the tip commit of this PR — removed the
in_flight_blob_readerfield fromStoreand replaced it with theIN_FLIGHT_FD_READERSthread-local map (read_file.rs:170-182). The comment now points readers at a mechanism that no longer exists.Step-by-step proof
grep -rn in_flight_blob_reader src/returns exactly one hit: this doc comment at read_file.rs:222. There is no field, method, or constant by that name anywhere else insrc/.- The mechanism the comment is trying to describe is now the
IN_FLIGHT_FD_READERSthread-local at read_file.rs:170-182, whose own doc comment already explains that a second concurrent read "attaches to the active reader". - The two sibling functions that operate on this field —
try_coalesce_fd_read(read_file.rs:426) andmark_in_flight(read_file.rs:463) — were both updated in 1e54f71 to say "SeeIN_FLIGHT_FD_READERS." This one field doc was missed in the same pass. - The PR description ("Fix" section) also still says "
Storenow carries anin_flight_blob_readerpointer", which is likewise stale after 1e54f71 — worth updating in the same edit so nobody goes looking for the field.
Why nothing prevents it
The comment was accurate through 938ed8d and simply wasn't updated when 1e54f71 swapped the storage from an atomic pointer on
Storeto a per-JS-threadHashMap<*const Store, *mut ReadFile>. There is no lint that catches doc-comment references to removed identifiers.Impact
No runtime effect — this is purely a documentation staleness. But per REVIEW.md ("Comments carry only durable non-obvious content") and CLAUDE.md rule 14, a comment that names a removed field is worse than no comment: someone tracing how
extra_completionsgets populated will grep forStore::in_flight_blob_reader, find nothing, and have to re-derive that the actual attachment point istry_coalesce_fd_read→IN_FLIGHT_FD_READERSlookup →extras.lock().push(...).How to fix
Either update the reference to match the siblings:
/// JS-thread-only; attached via `try_coalesce_fd_read`. See `IN_FLIGHT_FD_READERS`.or drop the second clause entirely (
/// JS-thread-only.), since the thread-local's own doc already covers how attachment works and the sibling function docs already point there.
…ake sliced test completion_thunk: restore the 'JsTerminated error is intentionally swallowed' line that was on the local handler_run it replaced. extra_completions: point the doc at try_coalesce_fd_read instead of the removed Store::in_flight_blob_reader field. sliced+unsliced test: use a 128-byte payload so neither reader has to register with epoll/kqueue (on kqueue a second EV_ADD for the same (ident, filter) silently overwrites the first and strands it), and drop the result[0].byteLength === 3 assertion. The coalescing decision happens on the JS thread before either reader is scheduled, so the guard being tested does not depend on payload size. Also wrap proc.stdin.end() in Promise.resolve() since it returns a number when the write fits the socket buffer.
A leading mismatched-window read occupied the store's only map slot, so two subsequent same-window reads both missed the coalesce and raced anyway. Keying on the (store, offset, max_length) tuple gives each distinct window its own in-flight reader; the lookup key already encodes the window, so the separate offset/max_length comparison (and its raw pointer projections) are no longer needed.
There was a problem hiding this comment.
No further findings — 5f4ff6e addresses the last nit (composite (store, offset, max_length) key). Deferring to a human for the unsafe lifetime/aliasing sign-off; this is not a mechanical change.
What was reviewed:
then()'s self-pointer comparison:from_ref(&*box)yields the same heap address inserted bymark_in_flight, andstoreis alwaysSomeat that point, so the map entry is always cleared before drop.- Composite key: with the window in the key,
try_coalescemiss ⇒mark_in_flightalways inserts (no more silent no-op), so every scheduled reader is the one in the map for its key. - Extras fan-out: each branch of
then()(both error arms + success) drainsextrasbefore the primary callback, and clones the buffer/error per listener — no double-free, no leaked handler. thread_local!scoping vs REVIEW.md's "never back per-VM state with thread-locals": each JS VM has its own OS thread, so per-thread == per-VM here; the ObjectURLRegistry cross-thread path just misses the map as intended.
Extended reasoning...
Overview
The PR coalesces concurrent fd-backed Blob reads (e.g. two Bun.stdin.arrayBuffer() calls) onto a single ReadFile instead of racing two readers on the same fd. It adds a per-JS-thread thread_local! HashMap<(*const Store, SizeType, SizeType), *mut ReadFile>, an extra_completions: Guarded<Vec<...>> field on ReadFile, insert/lookup/remove hooks in do_read_file / then(), and four subprocess tests. This has been through five review rounds (slice-window mismatch, cross-thread Store sharing via ObjectURLRegistry, EPIPE/kqueue test flake, stale doc comments, and single-slot map keying) — all resolved, and the bug-hunt pass on 5f4ff6e found nothing.
Security risks
None identified. The change is POSIX-only, gated to fd-backed Stores, and does not touch validation, auth, or network parsing. The raw-pointer map is thread-local and keyed on a heap address the caller already holds a StoreRef to, so there is no attacker-controlled key.
Level of scrutiny
High. This is core Blob I/O with new unsafe blocks whose soundness rests on two non-local invariants: (1) WorkTask::then runs on the creating event loop's thread, so insert/lookup/remove on the thread_local! map are serialized; and (2) the work-pool thread's &mut ReadFile never touches extra_completions, so the JS thread's &*addr_of!((*existing).extra_completions) + mutex lock is data-race-free. Both hold on inspection, and the SAFETY comments state them, but this is exactly the kind of reasoning a maintainer should confirm rather than a bot.
Other factors
- The
then()cleanup comparesm.get(&key) == Some(&self_ptr)before removing, so a reader that was never inserted (path-backed store, or a hypothetical future no-op insert) cannot evict someone else's entry. - With the composite key,
or_insert_withinmark_in_flightis now effectively an unconditional insert (the immediately-precedingtry_coalesce_fd_readon the same JS thread would have returnedtrueif the key existed), so there is no longer a "silently untracked" reader. - Minor: the restored swallowed-error comment on
completion_thunkkept the "intentionally swallowed" line but not the original// TODO: propagate the exception.— not worth another round on its own. - Tests cover the headline case, mixed
.text()/.arrayBuffer(), sequential reads staying independent, and the sliced-vs-unsliced non-coalescing guard; all POSIX-only andtest.concurrent. They look non-flaky after the earlier de-flake rounds, but CI across Linux/macOS is the real proof.
There was a problem hiding this comment.
No new findings on 5f4ff6e — all four earlier rounds (cross-thread Store UAF, EPIPE flake, kqueue EV_ADD hang, composite-key coalescing) are addressed. Deferring to a human for the design/soundness call: a thread_local! map of raw *mut ReadFile with &*addr_of!((*p).extra_completions) projected while a work-pool thread may hold &mut ReadFile is the kind of aliasing/lifetime pattern a maintainer should sign off on.
What was reviewed:
then()cleanup key:from_ref::<ReadFile>(&this)deref-coerces theBoxto the same heap address thatmark_in_flightinserted, and the entry is removed before callbacks so a re-entrant read starts fresh.- Coalesced
handlerownership:completion_thunkheap::takes each extra ctx, so no leak on the coalesce path; error branches cloneSystemErrorper listener. .or_insert_with()inmark_in_flightis now unreachable-as-no-op under the composite key (same-key lookup would have coalesced), so no untracked reader can leave a stale map entry.
Extended reasoning...
Overview
Touches src/runtime/webcore/Blob.rs (two do_read_file call sites), src/runtime/webcore/blob/read_file.rs (new IN_FLIGHT_FD_READERS thread_local map, try_coalesce_fd_read/mark_in_flight, extra_completions fan-out in then()), and adds four subprocess tests to test/js/bun/util/bun-stdin-slice.test.ts. POSIX-only (#[cfg(not(windows))]); Windows ReadFileUV is untouched.
Security risks
None identified. No untrusted-input parsing, no auth/crypto. The map key is a *const Store used only for identity comparison, never dereferenced.
Level of scrutiny
High. This is unsafe Rust in a hot I/O path with cross-thread lifetime reasoning: the JS thread stores/reads a raw *mut ReadFile while the work pool concurrently runs &mut ReadFile; correctness rests on WorkTask::then running on the creating event loop and on the addr_of!-projected &Guarded<_> not being invalidated by the work pool's &mut. That is a maintainer-level judgement, not a mechanical check.
Other factors
The PR went through four review iterations, each surfacing a real defect (per-Store atomic → cross-thread UAF via ObjectURLRegistry; parent-side EPIPE flake; kqueue silent EV_ADD overwrite hang; single-slot map defeating same-window coalescing behind a leading slice). All are now addressed and this run's bug hunt found nothing new. The fix is architecturally a workaround for the underlying "one io::Poll per fd" constraint rather than a change at that layer; whether that scoping is what maintainers want is worth a human look. Not approving because the change is neither simple nor mechanical.
|
CI status at 7534f3b:
The diff is ready for review. |
[skip size check]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/blob/read_file.rs (1)
619-701: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftJsTerminated is swallowed by
completion_thunk, butthen()now fans out to N+1 callbacks per completion.
completion_thunk(lines 138-145) discardsC::run'sJsTerminatedResultwithlet _ = unsafe { C::run(...) }. Before this PR,then()only ever invoked one callback, so a swallowedJsTerminatedhad no further consequence for thatthen()call. Nowthen()loops over every drainedextra_completionsentry and then still calls the primarycb, all potentially routed throughcompletion_thunk. If aJsTerminated(VM termination) occurs while invoking one coalesced listener's callback,then()has no way to observe it and will keep invoking the remainingextra_cbs and the primarycbanyway — continuing to interact with promises/JS state after the VM has signaled termination.This is a new hazard introduced specifically by the fan-out design (single-callback
then()never had a "keep going after this callback" step). Consider havingthen()'s loop check a termination signal (e.g. via theJSGlobalObject/VM) between callback invocations and stop dispatching further callbacks once termination is observed.As per coding guidelines: "After every call that can enter JavaScript or run user code, perform the required exception/error propagation before using the result; validate types before non-throwing accessors and do not clear exceptions."
🤖 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 `@src/runtime/webcore/blob/read_file.rs` around lines 619 - 701, Update ReadFile::then to retain the JSGlobalObject and check the VM termination state after each extra callback before dispatching another callback. Stop fan-out immediately when termination is observed, including before invoking the primary callback, so no further callbacks enter JavaScript after JsTerminated. Use the runtime’s existing termination/error-propagation mechanism rather than clearing or ignoring the signal.Source: Coding guidelines
🤖 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 `@src/runtime/webcore/blob/read_file.rs`:
- Around line 675-692: Update the coalesced-listener result handling around the
extra_cb loop to avoid cloning the full read buffer for every listener.
Introduce or reuse a shared, immutable buffer ownership mechanism compatible
with all ReadFileResultType::Result consumers, and update reclamation paths
currently relying on heap::take to support that shared representation while
preserving the primary callback’s delivery semantics.
In `@test/js/bun/util/bun-stdin-slice.test.ts`:
- Around line 51-160: Add a concurrent test alongside the existing stdin
coalescing cases that starts two identical non-default sliced reads, such as
matching slice(0, N).bytes() calls, and verifies both fulfill with the same
expected length and content or hash. Keep the test focused on confirming that
identical offset/max_length windows coalesce, distinct from the existing
full-window and mismatched-window tests.
---
Outside diff comments:
In `@src/runtime/webcore/blob/read_file.rs`:
- Around line 619-701: Update ReadFile::then to retain the JSGlobalObject and
check the VM termination state after each extra callback before dispatching
another callback. Stop fan-out immediately when termination is observed,
including before invoking the primary callback, so no further callbacks enter
JavaScript after JsTerminated. Use the runtime’s existing
termination/error-propagation mechanism rather than clearing or ignoring the
signal.
🪄 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: 57676592-5cbf-4210-a113-9d3fef121590
📒 Files selected for processing (3)
src/runtime/webcore/Blob.rssrc/runtime/webcore/blob/read_file.rstest/js/bun/util/bun-stdin-slice.test.ts
2 participants
Two concurrent
Bun.stdin.arrayBuffer()calls (or.text()/.bytes()/.json()on the same fd-backed Blob) each spawned a freshReadFileover the shared fd. On a piped stdin that meant:read()on fd 0, so the byte stream was split between them and the survivor could resolve short with no error; andReadFileowned its ownio::Poll, so both issuedepoll_ctl(EPOLL_CTL_ADD)on fd 0 and the loser rejected with the rawEEXIST: file already exists, epoll_ctl.Repro
Fix
do_read_filerecords the activeReadFilein a per-JS-threadIN_FLIGHT_FD_READERSmap keyed on theStorepointer; a second concurrent call with the same(offset, max_length)attaches its completion to that reader'sextra_completionslist instead of scheduling a racing reader.ReadFile::then(which runs on the creating event loop) clears the entry and fans the result out, cloning the buffer or error per extra listener.The map is
thread_local!because aStorecan be reached from multiple JS threads viaObjectURLRegistry; a worker resolving the same blob url simply sees an empty map and falls through to the pre-PR behaviour, so no cross-thread deref of the raw pointer is possible. Coalescing is refused when(offset, max_length)differ (a concurrent sliced + unsliced read keeps its previous loud behaviour rather than silently resolving the unsliced call with the sliced window). Path-backedBun.file()is untouched (each read opens its own fd), and a read started after the first resolves still begins a fresh reader so piped stdin keeps its consume-once semantics.Verification
bun bd test test/js/bun/util/bun-stdin-slice.test.tspasses; the newarrayBuffer + arrayBufferandtext + arrayBuffercases fail on canary with theEEXISTrejection and short reads shown above.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/bun-stdin-slice.test.ts