Conversation
fetch(url, { body: formData }) where the FormData holds a Bun.file(path)
entry read the entire file into memory while building the multipart body
(Blob::from_dom_form_data does a synchronous read_file + push_cloned per
file-backed part, then flattens the joiner into one contiguous buffer).
Peak RSS was roughly 2x the combined file size, so a 500 MB upload
peaked at 570-1040 MB, while the same file as body: Bun.file(path)
streams flat via sendfile.
This adds a streaming path used only by fetch's body extraction:
- MultipartSegments::from_dom_form_data walks the FormData and emits the
same boundary/header bytes as the buffered serializer, but records
file-backed parts as Segment::File { store, offset, size } instead of
reading them. Total body size is computed from stat so we can still
send an explicit Content-Length.
- MultipartFormLoader is a new ReadableStream source that iterates the
segments, serving Bytes segments from memory and reading File segments
with pread in 256 KB chunks.
- HTTPRequestBody gains a MultipartFormStream variant carrying the
stream plus its content-type and precomputed length. fetch injects the
Content-Length header for it so the HTTP client uses Content-Length
rather than chunked transfer-encoding.
needs_streaming_multipart gates the new path: it is taken only when the
FormData contains at least one file-backed entry and no S3 or unsized
(pipe/FIFO/stat-failed) entry. Missing files, S3 parts, and plain
in-memory FormData still flow through the existing buffered serializer,
so new Response(formData), new Request({body: formData}), and the
ENOENT error behaviour are unchanged.
|
Warning Review limit reached
Next review available in: 3 seconds 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 (18)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 11:23 PM PT - Jul 25th, 2026
❌ @robobun, your commit 6fe893d has some failures in 🧪 To try this PR locally: bunx bun-pr 35792That installs a local version of the PR into your bun-35792 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…uncated file - Extract write_multipart_entry<S: MultipartSink> so FormDataContext and MultipartSegmentBuilder drive the same per-entry wire layout. StringJoiner and the segment Vec<u8> accumulator each implement MultipartSink; only the blob body dispatch remains serializer-specific. - Drop MultipartFormLoader.total_size (never read). - on_pull now errors the stream when pread returns 0 with bytes remaining (file shrank after Content-Length was sent) instead of finishing short.
…restart rebuild failure drain_queued_writes guarded only the two h1 client arms; h2 and h3 reached st.ended = ended with no check. Now stream_body_by_http_id (h2 ClientSession, h3 ClientSession/ClientContext) takes the write generation and drops the message when the buffer's current generation is newer, mirroring the h1 arms. The redirect-restart handler now matches on multipart_form_stream's result and aborts the task on Err/None instead of leaving the redirected request at its body stage with no sink and a pending JS exception.
…eplay - take_and_detach_sink: release the ResumableSink ref (init_exact_refs starts at 2; detach_js alone never decrements) and, when the sink had not reached write_end_request yet, also release the tasklet ref that start_request_stream took for it so a mid-pump detach does not pin the tasklet. - detach_stale_request_sink: guard on request_stream_restart_pending so a callback-coalesced onProgressUpdate that already installed the fresh sink is not detached afterwards. - restart block: reset the stream buffer again on the JS thread before clearing request_stream_restart_pending, so a write_request_data that passed its atomic check before the HTTP-thread reset cannot leave a stale chunk for the new sink to append to.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/webcore/fetch/FetchTasklet.rs:933-936— Theself.ref_()taken instart_request_stream()(line 693, "lets only unref when sink is done") is only released viaon_end→write_end_request→FetchTasklet::deref, butdetach_js()never invokeson_end— so both new detach sites (here at ~933 anddetach_stale_request_sinkat ~2273) orphan that ref for the previous generation, thenstart_request_stream()runs again for the replay and takes another. Net: oneFetchTaskletref (response buffer, headers,url_proxy_buffer, Strong handles) leaked per 307/308 hop. This is distinct from the already-flagged missingResumableFetchSink::deref_(sink)— that leaks the sink allocation; this leaks the tasklet itself, and fixing that alone does not release this ref. Add a matchingFetchTasklet::derefwhensink.take()returnsSomeat both sites (or usesink.cancel(UNDEFINED)like theis_donecleanup at line 917 does, whose comment says exactly "write_end_requestdrops that ref").Extended reasoning...
What the bug is
start_request_stream()at FetchTasklet.rs:693 takes a +1 on theFetchTasklet:self.ref_(); // lets only unref when sink is done let sink = ResumableSink::init_exact_refs(&global_this, stream, std::ptr::from_mut(self), 2); self.sink = Some(sink);
That +1 has exactly one release path: the sink's pump reaches
js_end→on_end→write_end_request, and every path throughwrite_end_requestends withFetchTasklet::deref(this_ptr)(including the newrequest_stream_restart_pendingearly-return this PR added).Why the new detach sites orphan it
detach_js()(ResumableSink.rs:439) downgradesjs_thisto weak and clears the cached slots. Its own doc comment: "UnlikeSelf::cancelthis does NOT run any JS callbacks or invokeon_end." After it runs,is_detached()(line 427:!self.js_this.is_strong() || status == Done) istrue, so any laterjs_write/js_endfrom the pump early-returns at line 303/345 without callingon_end.write_end_requestis therefore never reached for that sink, and the +1 from line 693 is orphaned.Both new sites do exactly
sink.take()+detach_js()and nothing else for the tasklet ref:on_progress_updaterestart handler, ~933-936:if let Some(sink) = self.sink.take() { unsafe { (*sink).detach_js() }; }
detach_stale_request_sink, ~2273-2276:The trailingif let Some(sink) = this_ref.sink.take() { unsafe { (*sink).detach_js() }; } FetchTasklet::deref(this); // ← this balances on_request_stream_restart's ref_(), NOT start_request_stream's
derefhere pairs with thethis_ref.ref_()taken inon_request_stream_restartfor the enqueued concurrent task (its SAFETY comment: "we hold the ref taken above"), so it does not help.
Step-by-step proof
const fd = new FormData(); fd.append("f", Bun.file("/tmp/big.bin")); await fetch(url307, { method: "POST", body: fd }); // /a → 307 → /b
- Initial request reaches the body stage →
start_request_stream():self.ref_()(tasklet refcount +1, call it R₀),init_exact_refs(.., 2)allocates sink₀,self.sink = Some(sink₀). Pump starts. - Server sends 307 before the body finishes (common — servers often redirect without reading the body). HTTP thread:
prepare_stream_body_for_redirect→buf.report_restart()→on_request_stream_restart: setsrequest_stream_restart_pending = true, doesself.ref_()(call it R_task), enqueuesdetach_stale_request_sink. - JS thread runs
detach_stale_request_sink:self.sink.take()→Some(sink₀);(*sink₀).detach_js()— sink₀'sjs_thisis now weak,is_detached() == true. ThenFetchTasklet::deref(this)releases R_task. R₀ is still held. - The pump's pending
reader.read()for sink₀ resolves;sink₀.write(...)→js_write→is_detached()→ early return. Eventuallysink₀.end()→js_end→ line 345if this.is_detached() { return; }→ early return.on_endnever fires;write_end_requestnever runs for gen 0; R₀ is orphaned. - Redirected request reaches its body stage;
on_progress_updateenters the restart block at 927 (or if it ran before step 3, it does thesink.take()/detach_js()itself with the same result).start_request_stream()runs again →self.ref_()(R₁). - The final generation's R₁ is eventually released — either via the pump reaching
write_end_request, or via theis_donecleanup at line 917-919 (sink.cancel(UNDEFINED)→on_end→write_end_request→ deref).clear_sink()inclear_datareleases the final sink's allocation ref but does not (and need not) touch the tasklet refcount. R₀ is never released.
Net: one
FetchTaskletref leaked per 307/308 hop. The tasklet never reachesref_count == 0, so itsresponse_buffer,headers,url_proxy_buffer: Box<[u8]>, and theStronghandles it holds (promise, abort reason, check_server_identity) all leak.Why the PR's redirect test doesn't catch it
The redirect test's server does
for await (const _ of req.body ?? []) {}— it drains the full 1 KB body before sending 307, so the client's pump reachesjs_endbefore the restart signal,on_endfires,write_end_requesthits the newrequest_stream_restart_pendingguard and derefs R₀. The leak requires the 307 to arrive before the client's pump has calledjs_end, which is the common case for an early redirect on a non-trivial upload.Distinct from the already-flagged sink-allocation leak
The existing 🔴 comment on line 936 ("omit
ResumableFetchSink::deref_(sink)") is about the sink's own intrusive refcount (frominit_exact_refs(.., 2)) — withoutderef_(sink), theResumableSinkheap allocation stays at refcount ≥ 1 after GC finalizes the wrapper. Fixing that (addingResumableFetchSink::deref_(sink)) drops the sink; it does not touch the FetchTasklet'sref_count, so R₀ is still orphaned. Both fixes are needed at both sites.Fix
At each
if let Some(sink) = ….sink.take()in the two new sites, release thestart_request_streamref alongside the sink cleanup — either:- Add an explicit
FetchTasklet::deref(this_ptr)inside theSome(sink)arm (paired with the existing sinkderef_fix), or - Replace
detach_js()with(*sink).cancel(JSValue::UNDEFINED), matching the pre-existingis_donecleanup at line 917-919 whose comment already documents the intent: "Cancel the sink so the JS side releases the reader andwrite_end_requestdrops that ref".cancel()fireson_end→write_end_request, which now hits therequest_stream_restart_pendingguard, doesFetchTasklet::deref(this_ptr), and returns without touching the new request's state.
Per REVIEW.md → Native code: memory safety → "Reference counts provably balanced on every terminal path — success, error, cancellation, finalize."
…am; clear restart_callback - h2/h3 drain_send_body still called body.detach() directly on local_closed/request_body_done, bypassing the restart guard. A 307/308 that arrives after END_STREAM over h2/h3 then finds stream.buffer=None and drain_send_body early-returns without ever emitting DATA. Now gated on !client.flags.streaming_body_can_restart to mirror request_stream_detach. - The restart block cancels the previous ReadableStream before deinit so MultipartFormLoader::on_cancel closes its fd immediately instead of waiting for GC. - clear_drain_callback now also clears restart_callback so clear_sink leaves no dangling context pointer on the buffer.
The /b handler now hashes the received body and returns the request Content-Type; the client rebuilds the expected multipart bytes from the boundary and compares hashes, so a stale-chunk or duplicated-segment bug in the replay path would be caught instead of passing on byte count alone.
There was a problem hiding this comment.
All prior findings are addressed (95c909b adds the if is_done { deref } on the restart-rebuild abort arm; 6fe893d has the redirect test verify replayed body bytes via rolling hash), and this pass found nothing new. Deferring to a human reviewer given the scope — ~1200 lines across the h1/h2/h3 client, a new cross-thread restart/generation protocol between the HTTP thread and FetchTasklet, and new refcount balancing in take_and_detach_sink/on_progress_update.
What was reviewed: the prepare_stream_body_for_redirect / generation / request_stream_restart_pending machinery for stale-write races and refcount balance on every on_progress_update exit; the h2/h3 body.detach() gating vs. redirect replay; MultipartFormLoader fd lifecycle on cancel/restart; and that the buffered serializer's wire layout is byte-identical to the new streaming one via the shared write_multipart_entry.
Extended reasoning...
Overview
This PR makes fetch(url, { body: formData }) stream file-backed Bun.file() FormData parts from disk in 256 KB chunks instead of buffering the entire file into memory. It touches 18 files (~1224 insertions / 259 deletions):
src/runtime/webcore/Blob.rs(+507/-180): refactors the multipart serializer into a sharedwrite_multipart_entry<S: MultipartSink>used by both the buffered (StringJoiner) and new streaming (MultipartSegmentBuilder) sinks; addsMultipartSegments::from_dom_form_dataandneeds_streaming_multipart.src/runtime/webcore/MultipartFormLoader.rs(new, 198 lines): a newReadableStreamsource that iteratesSegment::{Bytes,File}andpreads file segments.src/runtime/webcore/fetch/FetchTasklet.rs(+289/-30): newHTTPRequestBody::MultipartFormStreamvariant;take_and_detach_sinkhelper; a restart block inon_progress_updatethat rebuilds the loader/sink on 307/308;on_request_stream_restart/detach_stale_request_sinkcallbacks;request_stream_restart_pendingatomic andrequest_stream_generationstamping onschedule_request_write.src/http/{lib,HTTPThread,ThreadSafeStreamBuffer,HTTPRequestBody}.rsand h2/h3 client: newFlags::{streaming_body_can_restart,pending_request_stream_restart},HTTPClientResult::restart_request_stream,prepare_stream_body_for_redirect,WriteMessage::generation+is_stale_generationfiltering,restart_callbackon the stream buffer, and gating ofbody.detach()at h1/h2/h3 body-completion sites.- Two new tests (RSS-bounded streaming upload with wire-byte hash verification; 301/302/303/307/308 redirect replay with hash verification).
Security risks
None identified. The new file-open path uses bun_sys (O_RDONLY|O_NOCTTY, dup for fd-backed stores) mirroring the existing needs_to_read_file path in fetch.rs. No new user-controlled path parsing; the FormData entries are already trusted at this layer. The generation counter is a defensive fence against stale cross-thread writes, not a security boundary.
Level of scrutiny
High. This is production-critical HTTP client code with new cross-thread state (an atomic restart_pending flag written from the HTTP thread and read on the JS thread; a generation counter used to discard stale WriteMessages; a restart_callback fired from do_redirect). The on_progress_update restart block adds a new early-return path that must maintain the same mutex-unlock / if is_done { deref } invariants as its siblings — and the review history shows exactly those invariants were missed and patched in three separate follow-up commits (79323e1 mutex unlock, 95c909b deref-on-is_done, plus the take_and_detach_sink refcount balancing in 64654c8). The h2/h3 body.detach() gating was also missed initially and patched in 7c9a1d7. A change that accumulates this many concurrency/refcount fixes during review warrants a maintainer's eyes on the final shape.
Other factors
- All eight rounds of prior findings (from both the bug-hunting system and my inline comments) are now addressed and marked resolved; the current pass found nothing new.
- The tests are solid: both now verify wire bytes via a rolling hash against an independently rebuilt multipart body, the RSS test disables ASAN quarantine for the fixture process and asserts
peakDeltaMB < fileSizeMB, and the redirect test covers the full 301/302/303/307/308 matrix. - The buffered serializer refactor (
FormDataContext::on_entry→write_multipart_entry) is behaviour-preserving by construction (both sinks call the same framing helper), and existingFormData-multipart-serialization/FormData & Bun.file (roundtrip)tests are stated to pass unchanged. - The gating (
needs_streaming_multipart+prefer_bufferedfor S3/compress) keeps the buffered path for every case that previously depended on it.
Given the size, the number of subsystems touched, and the concurrency subtlety demonstrated by the review iteration, this should be signed off by a maintainer familiar with the HTTP client / FetchTasklet lifecycle rather than auto-approved.
|
Diff is green locally on linux-x64 (debug ASAN + release) and The mechgate also bounces on a PCH-staleness build failure when toggling between with-fix and without-fix: this PR adds a new codegen'd class ( Fail-before / pass-after is reproducible with the stock installed bun: Ready for a maintainer to review/merge. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-26, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
What
fetch(url, { body: formData })where the FormData holds aBun.file(path)entry read the entire file into memory while building the multipart body.Blob::from_dom_form_datadid a synchronousread_file+push_clonedper file-backed part, then flattened the joiner into one contiguous buffer. Peak RSS was roughly 2x the combined file size, so a 500 MB upload peaked at 570-1040 MB, while the same file asbody: Bun.file(path)streams flat via sendfile.Repro
Fix
Adds a streaming path used by fetch's body extraction:
MultipartSegments::from_dom_form_datawalks the FormData and emits the same boundary/header bytes as the buffered serializer (both now call the sharedwrite_multipart_entry<S: MultipartSink>/for_each_form_data_entry/make_multipart_boundaryhelpers), but records file-backed parts asSegment::File { store, offset, size }instead of reading them. Total body size is computed from stat so the request is sent with an explicitContent-Length.MultipartFormLoaderis a newReadableStreamsource that iterates the segments, servingBytessegments from memory and readingFilesegments withpreadin 256 KB chunks.HTTPRequestBodygains aMultipartFormStreamvariant carrying the segment template, the stream, the content-type and the precomputed length.fetchinjectsContent-Lengthfor it so the HTTP client avoids chunked transfer-encoding.Gating
needs_streaming_multiparttakes the new path only when the FormData contains at least one file-backed entry and no S3 entry.HTTPRequestBody::from_jsadditionally keeps the buffered path whenprefer_bufferedis set, whichfetchdoes for S3 destinations and an explicitcompressoption. Missing files, pipes/FIFOs, and plain in-memory FormData still flow through the buffered serializer, sonew Response(formData),new Request({body: formData}), the S3 PUT path, request-body compression, and theENOENTerror behaviour are unchanged.Redirect replay
A FormData body has a non-null Fetch "source", so 307/308 redirects must re-send it rather than failing with
RequestBodyNotReusable. The HTTP client now supports replaying a restartable streaming body:Flags.streaming_body_can_restartletshandle_response_metadatafollow non-303 redirects for this body and makesrequest_stream_detachkeep the buffer ref so the redirect path still has it.do_redirect/do_redirect_multiplexedcallprepare_stream_body_for_redirect, which for 307/308 extracts theStream, fires the buffer'srestart_callback, bumps its atomicgeneration, resets it, and passes the stream tostart()for the follow-up request. For 301/302/303 it just drops the streaming flag as before.WriteMessageis stamped with the generation it was scheduled for anddrain_queued_writesdiscards stale ones, so an in-flightEndfrom the previous sink cannot mark the replayed request done.FetchTaskletregisterson_request_stream_restartas the restart callback; it setsrequest_stream_restart_pending(which makes stalewrite_request_data/write_end_request/resume_request_data_streamcalls no-op) and enqueues a JS-sidedetach_stale_request_sink. When the redirected request reaches its body stage,HTTPClientResult.restart_request_streamdriveson_progress_updateto rebuild a fresh loader/stream from the saved segment template and start a new sink writing at the new generation.Verification
48 MB file, client-side peak RSS delta (ASAN quarantine disabled for the fixture process):
Bun.file()(sendfile)Bun.file().stream()FormData{Bun.file()}The new tests rebuild the expected multipart bytes independently and compare a rolling hash of the received body, and exercise 301/302/303/307/308 redirects with a FormData+
Bun.file()body. ExistingFormData & Bun.file (roundtrip),FormData-multipart-serialization,FormData-file-error-leak, andfetch-redirecttests pass unchanged.[review] gate passed · iteration 2 · 18 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 7 passed · 2 rejected · iteration 2
evidence per changed file