From c4772f61eb7b670bb803eeb68e647a6e4da21fa8 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:28:16 +0000 Subject: [PATCH 01/22] fetch: keep a body stream nothing reads paused, and cancel it once it is collected A response body stream that nothing reads (res.body touched and left alone, or its reader released) made the fetch switch to BufferAll on the next chunk: the rest of the body was read into the ByteStream with no bound. The flip existed because the tasklet rooted the stream (a Strong on it, and a counted ref that also rooted the source wrapper), so an abandoned stream could only go away once the body ended. The tasklet now holds only the counted ref on the native source, and that ref no longer roots the wrapper (SourceContext::NATIVE_REF_ROOTS_WRAPPER, off for ByteStream). The body is delivered through the source. When the wrapper is collected with no native consumer attached, ByteStream::finalize_detach tells the producer, and the tasklet cancels the fetch from a queued task, the same path as reader.cancel(). While the stream is alive but unread, the transport stays paused. The event loop ref follows the consumers: a pull that goes pending or a sink that is wired refs it, a delivered chunk that nothing waits for unrefs it, and creating the stream no longer refs it. poll_ref becomes a JsCell because the drain signal re-enters the tasklet through a shared ref. --- src/runtime/webcore/ByteStream.rs | 26 +- src/runtime/webcore/ReadableStream.rs | 15 +- src/runtime/webcore/fetch/FetchTasklet.rs | 306 +++++++++++------- src/runtime/webcore/streams.rs | 26 ++ test/js/web/fetch/fetch-backpressure.test.ts | 217 +++++++++++++ .../fetch/fetch-stream-cancel-leak.test.ts | 55 +++- 6 files changed, 532 insertions(+), 113 deletions(-) diff --git a/src/runtime/webcore/ByteStream.rs b/src/runtime/webcore/ByteStream.rs index 06d45a4fa307..d56edd998bab 100644 --- a/src/runtime/webcore/ByteStream.rs +++ b/src/runtime/webcore/ByteStream.rs @@ -69,6 +69,9 @@ impl readable_stream::SourceContext for ByteStream { const NAME: &'static str = "Bytes"; // setRefUnrefFn = null const SUPPORTS_REF: bool = false; + // The only native ref holder is the producer (`FetchTasklet`), which never + // touches the wrapper; see `SourceContext::NATIVE_REF_ROOTS_WRAPPER`. + const NATIVE_REF_ROOTS_WRAPPER: bool = false; crate::source_context_codegen!(js_BytesInternalReadableStreamSource); // R-2: trait sigs are fixed at `&mut self` (shared with the other @@ -86,6 +89,10 @@ impl readable_stream::SourceContext for ByteStream { fn deinit_fn(&mut self) { Self::finalize(self) } + fn finalize_detach(&mut self) -> bool { + Self::on_wrapper_finalized(self); + false + } fn drain_internal_buffer(&mut self) -> Vec { Self::drain(self) } @@ -256,8 +263,9 @@ impl ByteStream { Vec::::move_from_list(list) } - /// Called by native fast-paths after wiring `self.sink`. Restores - /// producer-side backpressure if it was already dropped (BufferAll). + /// A consumer now waits on this stream: a native sink was just wired to + /// it, or a pull found the buffer empty and went pending. The producer + /// keeps the process alive for it (`FetchTasklet::on_consumer_attached`). pub fn signal_consumer_attached(&self) { self.parent_const().producer.get().start(); } @@ -622,6 +630,7 @@ impl ByteStream { // Raw borrow of a JS-owned buffer; rooted by `set_value`. self.pending_buffer.set(std::ptr::from_mut::<[u8]>(buffer)); self.set_value(view); + self.signal_consumer_attached(); // R-2: `JsCell::as_ptr` yields the stable `*mut Pending` that the // returned `streams::Result::Pending` raw-backref needs. @@ -664,6 +673,19 @@ impl ByteStream { self.buffer.get().capacity() } + /// The JS wrapper was collected (`NewSource::finalize`): no stream, reader, + /// or node `Readable` can pull from this source again. A native consumer (a + /// `sink`, or a `buffer_action` that waits for the whole body) does not hold + /// the wrapper and still takes the bytes; with neither attached, the + /// producer is delivering into memory nobody can read. Runs inside a GC + /// sweep, so the producer may only schedule its teardown. + fn on_wrapper_finalized(&self) { + if self.sink.get().is_some() || self.buffer_action.get().is_some() { + return; + } + self.parent_const().producer.get().consumer_collected(); + } + /// NOTE: not `impl Drop` — `ByteStream` is the `context` payload of a `.classes.ts` /// `ReadableStreamSource`; teardown is driven by the GC finalizer via `Source::finalize`, /// which calls this. Per §JSC, `.classes.ts` payloads use `finalize`, not `deinit`/`Drop`. diff --git a/src/runtime/webcore/ReadableStream.rs b/src/runtime/webcore/ReadableStream.rs index 576c986be614..879ef3cd7b2c 100644 --- a/src/runtime/webcore/ReadableStream.rs +++ b/src/runtime/webcore/ReadableStream.rs @@ -750,6 +750,13 @@ pub trait SourceContext: Sized { const NAME: &'static str; /// `setRefUnrefFn != null` const SUPPORTS_REF: bool = false; + /// Whether a native ref ([`NewSource::increment_count`]) also roots the JS + /// wrapper. `FileReader` needs that: a read completes off the event loop and + /// then calls the wrapper's `onClose`. A producer that only writes into the + /// source's native memory (`FetchTasklet` into a `ByteStream`) opts out: the + /// wrapper holds the stream's consumer graph, so rooting it would keep a + /// stream nobody can reach alive for as long as the producer runs. + const NATIVE_REF_ROOTS_WRAPPER: bool = true; // ─── codegen accessors (`.classes.ts` → `generated_classes.rs`) ─────────── // Each context binds its per-type codegen module via `source_context_codegen!`. @@ -849,7 +856,8 @@ pub struct NewSource { /// Back-reference to the owning `JS{Blob,Bytes,File}InternalReadableStreamSource` /// wrapper. Starts `Weak` (set in [`Self::to_readable_stream`]), is /// [`JsRef::upgrade`]d to `Strong` in [`Self::increment_count`] while a - /// native I/O ref is held (FileReader `waiting_for_on_reader_done`), and + /// native I/O ref is held (FileReader `waiting_for_on_reader_done`; contexts + /// with [`SourceContext::NATIVE_REF_ROOTS_WRAPPER`] unset skip this), and /// [`JsRef::downgrade`]d back to `Weak` in [`Self::decrement_count`] when /// only the wrapper's own ref remains. [`Self::finalize`] flips it to /// `Finalized` so [`Self::on_js_close`] reads `None` instead of a @@ -1140,7 +1148,10 @@ impl NewSource { pub fn increment_count(&mut self) { self.ref_count += 1; - // A ref beyond the JS wrapper's own is held (in practice a FileReader + if !C::NATIVE_REF_ROOTS_WRAPPER { + return; + } + // A ref beyond the JS wrapper's own is held (a FileReader // `waiting_for_on_reader_done` I/O ref). Root the wrapper so // `on_js_close`, reached from `on_reader_done` off the event loop with // no JS frame on the stack, never reads a dead-but-unswept cell. diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 3e63b29255bd..5f6662f3f9e9 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -7,6 +7,7 @@ use bun_cares_sys::c_ares_draft as c_ares; use bun_core::{MutableString, OwnedString, String as BunString, ZigStringSlice}; use bun_event_loop::{ ConcurrentTask::{AutoDeinit, ConcurrentTask}, + ManagedTask::ManagedTask, Task, Taskable, }; use bun_http as http; @@ -18,7 +19,7 @@ use bun_http::{ use bun_io::KeepAlive; use bun_jsc::debugger::AsyncTaskTracker; use bun_jsc::{ - self as jsc, GlobalRef, JSGlobalObject, JSValue, JsResult, StringJsc, StrongOptional, + self as jsc, GlobalRef, JSGlobalObject, JSValue, JsCell, JsResult, StringJsc, StrongOptional, }; use bun_sys::FdExt; use bun_threading::Mutex; @@ -113,16 +114,21 @@ pub struct FetchTasklet { /// native response ref if we still need it when JS is discarted // Response is intrusively refcounted; modeled as a raw ptr. pub(crate) native_response: Option<*mut Response>, - /// stream strong ref if any is available - pub(crate) readable_stream_ref: ReadableStreamStrong, - /// A counted ref on that stream's ByteStream source for as long as this tasklet is its - /// `producer`, so unhooking goes through native memory we keep alive rather than through - /// the JS wrappers, which the VM's last sweep destroys in no particular order. + /// A counted ref on the response body stream's ByteStream source for as long as this + /// tasklet is its `producer`. The body is delivered into, and unhooked through, this + /// native memory, never through the JS wrappers, which GC (and the VM's last sweep) + /// destroys in no particular order. This tasklet holds nothing that roots the stream: + /// only its consumers (the Response wrapper, a reader, a pipe, a pending pull) keep it + /// alive, and once they are all gone the source reports the collected wrapper through + /// `on_response_stream_collected` (`SourceContext::NATIVE_REF_ROOTS_WRAPPER`). pub(crate) response_stream_source: Option>, pub(crate) request_headers: Headers, pub(crate) promise: jsc::JSPromiseStrong, pub(crate) concurrent_task: ConcurrentTask, - pub poll_ref: KeepAlive, + /// Keeps the event loop alive while a body consumer waits for bytes. `JsCell`: the + /// ByteStream's drain signal re-enters `on_stream_drained` through a shared ref while a + /// delivery on this tasklet is on the stack. + pub poll_ref: JsCell, /// For Http Client requests /// when Content-Length is provided this represents the whole size of the request /// If chunked encoded this will represent the total received size (ignoring the chunk headers) @@ -154,8 +160,7 @@ pub struct FetchTasklet { /// Set by `on_start_buffering_callback` (JS thread) and read by /// `callback()` (HTTP thread, under `mutex`): the body is being /// accumulated in `scheduled_response_buffer` for a buffered consumer. - /// Distinguishes that path from `drop_backpressure_if_unobserved`, which - /// also sets `BufferAll` but still delivers per chunk. + /// Cleared again when a stream attaches and drains it per chunk. pub(crate) is_buffering_body: AtomicBool, pub(crate) is_waiting_abort: bool, pub(crate) is_waiting_request_stream_start: bool, @@ -492,7 +497,6 @@ impl FetchTasklet { } self.clear_stream_handlers(); - self.readable_stream_ref.deinit(); self.scheduled_response_buffer = MutableString::default(); // Always detach request_body regardless of type. @@ -776,14 +780,13 @@ impl FetchTasklet { let mut err = scopeguard::guard(self.on_reject(), |mut e| e.reset()); let mut js_err = JSValue::ZERO; // if we are streaming update with error - if let Some(readable) = self.readable_stream_ref.get() { - if let Some(bytes) = readable.ptr.bytes() { - js_err = err.to_js(&global_this); - js_err.ensure_still_alive(); - bytes.on_data(StreamResult::Err(StreamError::JSValue( - bun_jsc::strong::Optional::create(js_err, &global_this), - ))); - } + if let Some(source) = self.take_response_stream_source() { + js_err = err.to_js(&global_this); + js_err.ensure_still_alive(); + Self::response_bytes(source).on_data(StreamResult::Err(StreamError::JSValue( + bun_jsc::strong::Optional::create(js_err, &global_this), + ))); + Self::release_response_stream_source(source); } // A failure result is terminal (`to_result` forces `has_more = // false` once `fail` is set), so everything pending must settle @@ -812,26 +815,32 @@ impl FetchTasklet { return Ok(()); } - if let Some(readable) = self.readable_stream_ref.get() { - bun_output::scoped_log!(FetchTasklet, "onBodyReceived readable_stream_ref"); - if let Some(bytes) = readable.ptr.bytes() { + // body can be marked as used but we still need to pipe the data + if !self.result.has_more { + // Unhook before the final delivery, so it cannot signal a producer that is done; + // release after it, so the bytes land in memory we still pin. + if let Some(source) = self.take_response_stream_source() { + bun_output::scoped_log!(FetchTasklet, "onBodyReceived response_stream_source done"); + let bytes = Self::response_bytes(source); bytes.size_hint.set(self.get_size_hint()); - // body can be marked as used but we still need to pipe the data - if self.result.has_more { - let chunk = self.scheduled_response_buffer.list.as_slice(); - bytes.on_data(Self::temporary_chunk(chunk, false)); - self.drop_backpressure_if_unobserved(&readable, &bytes); - } else { - self.clear_stream_handlers(); - let prev = core::mem::take(&mut self.readable_stream_ref); - buffer_reset.set(false); + buffer_reset.set(false); - let chunk = self.scheduled_response_buffer.list.as_slice(); - bytes.on_data(Self::temporary_chunk(chunk, true)); - drop(prev); - } + let chunk = self.scheduled_response_buffer.list.as_slice(); + bytes.on_data(Self::temporary_chunk(chunk, true)); + Self::release_response_stream_source(source); return Ok(()); } + } else if let Some(source) = self.response_stream_source { + bun_output::scoped_log!(FetchTasklet, "onBodyReceived response_stream_source"); + let bytes = Self::response_bytes(source); + bytes.size_hint.set(self.get_size_hint()); + let chunk = self.scheduled_response_buffer.list.as_slice(); + bytes.on_data(Self::temporary_chunk(chunk, false)); + // Delivering can cancel the stream, which unhooks us and drops our ref on `bytes`. + if self.response_stream_source.is_some() { + self.update_keep_alive_for_body_consumer(&bytes); + } + return Ok(()); } if let Some(response) = self.current_response_mut() { @@ -848,7 +857,7 @@ impl FetchTasklet { if self.result.has_more { bytes.on_data(Self::temporary_chunk(chunk, false)); - self.drop_backpressure_if_unobserved(&readable, &bytes); + self.update_keep_alive_for_body_consumer(&bytes); } else { readable.value.ensure_still_alive(); response.detach_readable_stream(&global_this); @@ -949,8 +958,8 @@ impl FetchTasklet { // it again — cancel the sink so the JS side releases the reader; // the pump-promise settlement drops the `startRequestStream` ref. this.cancel_request_body_sink(JSValue::UNDEFINED); - let mut poll_ref = core::mem::take(&mut this.poll_ref); - poll_ref.unref(bun_io::js_vm_ctx()); + this.poll_ref + .with_mut(|poll_ref| poll_ref.unref(bun_io::js_vm_ctx())); // SAFETY: `this` is the live heap tasklet; we hold a ref. FetchTasklet::deref(std::ptr::from_mut(this)); } @@ -1598,15 +1607,15 @@ impl FetchTasklet { fn on_readable_stream_available( ctx: NonNull, - global_this: &JSGlobalObject, + _global_this: &JSGlobalObject, readable: ReadableStream, ) { let this = Self::from_ctx(ctx); this.clear_stream_handlers(); - this.readable_stream_ref = ReadableStreamStrong::init(readable, global_this); if let crate::webcore::readable_stream::Source::Bytes(bytes) = readable.ptr { - // SAFETY: the stream (held Strong above) owns a live ByteStream embedded in - // its Source; JS thread. + // SAFETY: the caller holds the stream, which owns the live ByteStream embedded + // in its Source; the ref taken here keeps the Source alive past the stream. + // JS thread. unsafe { let source = crate::webcore::readable_stream::NewSource::from_context_ptr(bytes); (*source).increment_count(); @@ -1624,9 +1633,10 @@ impl FetchTasklet { return DrainResult::Aborted; } - // A body consumer is attaching; keep the process alive until the - // body finishes (undone in `on_progress_update` when `is_done`). - this.poll_ref.ref_(bun_io::js_vm_ctx()); + // Creating the stream (`res.body`) does not by itself keep the process alive: + // whatever reads it refs the loop when it starts to wait (`on_consumer_attached`, + // `on_stream_drained`), synchronously, before the loop can run dry. A stream that + // is created and then never read holds nothing. // The bytes already in `scheduled_response_buffer` are handed to the // new stream below. That is the drain `Paused` was waiting for, so @@ -1679,17 +1689,43 @@ impl FetchTasklet { } } - /// Unhook this tasklet as the response ByteStream's producer before releasing - /// `readable_stream_ref` — the stream can outlive us in JS — and drop the ref that - /// kept the source's memory ours to write to. Touches no JS cell. + /// The ByteStream behind `source`. Valid while a counted ref pins the source: ours + /// (`response_stream_source`, or the pointer `take_response_stream_source` handed out + /// and `release_response_stream_source` has not consumed yet), or the JS wrapper's. + fn response_bytes( + source: NonNull, + ) -> bun_ptr::BackRef { + // SAFETY: see above; `context` is a field of the live source. Every ByteStream + // method takes `&self` (R-2), so a shared borrow is all a delivery needs. + bun_ptr::BackRef::new(unsafe { &(*source.as_ptr()).context }) + } + + /// Stop being the response ByteStream's producer. Returns the source, still pinned by + /// our counted ref, so a terminal chunk can be delivered into it first; the caller then + /// passes it to `release_response_stream_source`. Touches no JS cell. + fn take_response_stream_source( + &mut self, + ) -> Option> { + let source = self.response_stream_source.take()?; + // SAFETY: pinned by the counted ref taken in `on_readable_stream_available`, which + // only `release_response_stream_source` drops. + unsafe { (*source.as_ptr()).producer.set(SourceHandle::None) }; + Some(source) + } + + /// Drop the counted ref from `on_readable_stream_available`. When the stream's JS + /// wrapper is already gone this frees the source, so nothing may touch it afterwards. + fn release_response_stream_source(source: NonNull) { + // SAFETY: the ref `take_response_stream_source` handed out; `source` is not used + // after this. + unsafe { crate::webcore::byte_stream::Source::decrement_count(source.as_ptr()) }; + } + + /// Unhook from the response ByteStream (the stream can outlive us in JS) and release it. + /// Touches no JS cell. fn clear_stream_handlers(&mut self) { - if let Some(source) = self.response_stream_source.take() { - // SAFETY: counted ref taken in `on_readable_stream_available`; live until - // the `decrement_count` below, which may free it. - unsafe { - (*source.as_ptr()).producer.set(SourceHandle::None); - crate::webcore::byte_stream::Source::decrement_count(source.as_ptr()); - } + if let Some(source) = self.take_response_stream_source() { + Self::release_response_stream_source(source); } } @@ -1703,7 +1739,34 @@ impl FetchTasklet { self.ignore_remaining_response_body(); } + /// The source's JS wrapper was collected, and with it every reader, pipe, and + /// node `Readable` that could still pull from it (`ByteStream::on_wrapper_finalized`). + /// Nobody can read the rest of this body: cancel it like `reader.cancel()` would. + /// This runs inside a GC sweep, where the tasklet must not be torn down (a delivery + /// can be on the stack, and the sweep must not run script), so the cancel is queued + /// behind the current turn. `this` is the source's `producer` pointer, live because + /// `clear_stream_handlers` clears that handle before a tasklet is freed. + pub(crate) fn on_response_stream_collected(this: NonNull) { + // SAFETY: see above. Shared access only (a delivery on this tasklet can be on the + // stack); the hop keeps `this` itself for the exclusive access it needs later. + let tasklet = unsafe { this.as_ref() }; + let vm = tasklet.global_this.bun_vm(); + if vm.is_shutting_down() { + // Teardown aborts every fetch still in flight (`stop_for_vm_teardown`). + return; + } + tasklet.ref_(); + let hop = bun_core::heap::into_raw(Box::new(CollectedResponseStreamHop(this))); + vm.as_mut() + .enqueue_task(ManagedTask::new_owned(hop, CollectedResponseStreamHop::run)); + } + + /// The ByteStream handed bytes to a consumer. Re-arm the transport, and keep the + /// process alive for whatever that consumer asks for next; the next delivery + /// (`update_keep_alive_for_body_consumer`) settles whether anything is waiting. pub(crate) fn on_stream_drained(&self) { + self.poll_ref + .with_mut(|poll_ref| poll_ref.ref_(bun_io::js_vm_ctx())); if self .signal_store .try_transition_receive_mode(BodyReceiveMode::Paused, BodyReceiveMode::AutoPause) @@ -1712,19 +1775,40 @@ impl FetchTasklet { } } - /// A native sink attached after `drop_backpressure_if_unobserved` had - /// already flipped to `BufferAll`. Move to `Paused`: the socket is still - /// reading, so the next `maybe_pause_receive` stops it; the sink's drain - /// then resumes via `on_stream_drained`. + /// A consumer is waiting on the ByteStream: a pull went pending, or a native sink + /// was wired to it. Keep the process alive until the bytes it waits for arrive. pub(crate) fn on_consumer_attached(&self) { - let _ = self - .signal_store - .try_transition_receive_mode(BodyReceiveMode::BufferAll, BodyReceiveMode::Paused); + self.poll_ref + .with_mut(|poll_ref| poll_ref.ref_(bun_io::js_vm_ctx())); + } + + /// A chunk was just delivered to the ByteStream and more are coming. Something that + /// waits for the next one keeps the process alive: a pull the chunk did not satisfy, a + /// buffered consumer (`res.body.text()`), or a native sink. With none of those the + /// transport stays paused (`callback` flipped it as it delivered this chunk), and the + /// paused body must not keep an otherwise idle process alive: that is the state after a + /// reader is released, or after `res.body` is touched but never read. A consumer that + /// comes back pulls, and the pull refs the loop again (`on_consumer_attached`, + /// `on_stream_drained`); a consumer that is collected cancels the body instead + /// (`on_response_stream_collected`). A consumer that keeps reading pulls again in the + /// microtask that takes this chunk, before the loop can observe the unref. + fn update_keep_alive_for_body_consumer(&self, bytes: &crate::webcore::ByteStream) { + let waiting = bytes.sink.get().is_some() + || bytes.buffer_action.get().is_some() + || bytes.pending.get().state == crate::webcore::streams::PendingState::Pending; + self.poll_ref.with_mut(|poll_ref| { + if waiting { + poll_ref.ref_(bun_io::js_vm_ctx()); + } else { + poll_ref.unref(bun_io::js_vm_ctx()); + } + }); } fn on_start_buffering_callback(ctx: NonNull) { let this = Self::from_ctx(ctx); - this.poll_ref.ref_(bun_io::js_vm_ctx()); + this.poll_ref + .with_mut(|poll_ref| poll_ref.ref_(bun_io::js_vm_ctx())); this.is_buffering_body.store(true, Ordering::Release); if this .signal_store @@ -1740,31 +1824,6 @@ impl FetchTasklet { } } - /// After a body chunk has been delivered to the ByteStream with - /// `has_more == true`, flip to `BufferAll` if the stream has no lock, - /// no pipe, and no buffer action. An unlocked stream has no reader, so - /// there is nothing to apply backpressure against; let the body - /// complete so this tasklet (and the source it roots) can be freed. - fn drop_backpressure_if_unobserved( - &self, - readable: &ReadableStream, - bytes: &crate::webcore::ByteStream, - ) { - if self.signal_store.body_receive_mode() == BodyReceiveMode::BufferAll { - return; - } - if bytes.sink.get().is_some() - || bytes.buffer_action.get().is_some() - || bytes.pending.get().state == crate::webcore::streams::PendingState::Pending - || readable.is_locked(&self.global_this) - { - return; - } - self.signal_store - .set_receive_mode_terminal(BodyReceiveMode::BufferAll); - self.schedule_receive_resume(); - } - fn to_body_value(&mut self) -> BodyValue { if let Some(err) = self.get_abort_error() { return BodyValue::Error(err); @@ -1858,13 +1917,13 @@ impl FetchTasklet { } } // we should not keep the process alive if we are ignoring the body - self.poll_ref.unref(bun_io::js_vm_ctx()); + self.poll_ref + .with_mut(|poll_ref| poll_ref.unref(bun_io::js_vm_ctx())); // Also fine from `on_response_finalize` (a JSC Weak finalizer inside - // `WeakBlock::sweep`): unhooking touches no JS cell. The - // request-body sink is left for `clear_sink()` in `deinit()` (an event-loop - // task, outside sweep) to detach. + // `WeakBlock::sweep`): that path only gets here with no stream source held, so + // this touches no JS cell and frees nothing. The request-body sink is left for + // `clear_sink()` in `deinit()` (an event-loop task, outside sweep) to detach. self.clear_stream_handlers(); - self.readable_stream_ref.deinit(); self.response.clear(); if let Some(response) = self.native_response.take() { @@ -1877,11 +1936,12 @@ impl FetchTasklet { bun_output::scoped_log!(FetchTasklet, "onResolve"); let response = bun_core::heap::into_raw(Box::new(self.to_response())); // The fetch() promise is about to resolve; from here the paused - // transport should not by itself keep the event loop alive. The body - // consumer hooks (`on_start_streaming_http_response_body_callback`, - // `on_start_buffering_callback`) re-ref if the caller reads the body. + // transport should not by itself keep the event loop alive. A body + // consumer re-refs when it starts to wait for bytes (`on_consumer_attached`, + // `on_stream_drained`, `on_start_buffering_callback`). if self.is_waiting_body { - self.poll_ref.unref(bun_io::js_vm_ctx()); + self.poll_ref + .with_mut(|poll_ref| poll_ref.unref(bun_io::js_vm_ctx())); } // SAFETY: response is a freshly allocated Response; makeMaybePooled takes ownership semantics on the JS side let global_this = self.global_this; @@ -1925,12 +1985,11 @@ impl FetchTasklet { scheduled_response_buffer: MutableString::default(), response: jsc::Weak::default(), native_response: None, - readable_stream_ref: ReadableStreamStrong::default(), response_stream_source: None, request_headers: fetch_options.headers, promise, concurrent_task: ConcurrentTask::default(), - poll_ref: KeepAlive::default(), + poll_ref: JsCell::new(KeepAlive::default()), body_size: http::BodySize::Unknown, url_proxy_buffer: fetch_options.url_proxy_buffer, signal: fetch_options.signal, @@ -2378,7 +2437,9 @@ impl FetchTasklet { let node_ref = Self::from_raw_mut(node); let mut batch = bun_threading::thread_pool::Batch::default(); node_ref.http.as_mut().unwrap().schedule(&mut batch); - node_ref.poll_ref.ref_(bun_io::js_vm_ctx()); + node_ref + .poll_ref + .with_mut(|poll_ref| poll_ref.ref_(bun_io::js_vm_ctx())); // increment ref so we can keep it alive until the http client is done node_ref.ref_(); @@ -2488,9 +2549,8 @@ impl FetchTasklet { // Grow to Content-Length once so the per-packet append below // doesn't leave the ~2x doubling over-capacity that the // ArrayBuffer would adopt. Gated on `is_buffering_body` - // (set by `on_start_buffering_callback`), not the raw - // `BufferAll` mode: `drop_backpressure_if_unobserved` also - // sets `BufferAll` while still draining per chunk. + // (set by `on_start_buffering_callback`, cleared once a + // stream attaches and drains the buffer per chunk). if task_ref.is_buffering_body.load(Ordering::Acquire) { if let http::BodySize::ContentLength(n) = task_ref.body_size { if n > scheduled.list.capacity() { @@ -2639,12 +2699,13 @@ impl FetchTasklet { // 2b. if we have a promise, we should keep loading the body. // 3. We never started buffering, in which case we should ignore the body. // - // Note: We cannot call .get() on the ReadableStreamRef. This is called inside a finalizer. - if !matches!(body, BodyValue::Locked(_)) || this.readable_stream_ref.has() { - // Scenario 1 or 3. A paused transport in Scenario 1 is - // unstuck by `drop_backpressure_if_unobserved` once the next - // already-scheduled chunk reaches `on_body_received` and - // finds the stream unlocked. + // This is called inside a finalizer: decide from native state only. + if !matches!(body, BodyValue::Locked(_)) || this.response_stream_source.is_some() { + // Scenario 1: the stream outlives its Response for as long as something + // consumes it (`res.body` handed to a pipe, a reader, `Readable.fromWeb`). + // While nothing does, the transport sits paused without holding the loop + // (`update_keep_alive_for_body_consumer`); once the stream's wrapper is + // collected too, `on_response_stream_collected` cancels the body. return; } @@ -2759,3 +2820,32 @@ impl bun_event_loop::Taskable for FetchTaskletPromiseSettle { drop(unsafe { bun_core::heap::take(this) }); } } + +/// The hop `on_response_stream_collected` queues (a `ManagedTask::new_owned` context). +/// Owns one ref on the tasklet, released when the hop is dropped: after it ran, or +/// unrun when its VM tears down the queue. +struct CollectedResponseStreamHop(NonNull); + +impl CollectedResponseStreamHop { + fn run(this: *mut Self) -> ElJsResult<()> { + // SAFETY: the box `on_response_stream_collected` queued; reclaimed exactly once + // here (the task's `cleanup` only runs when it is released unrun). + let hop = unsafe { bun_core::heap::take(this) }; + // SAFETY: the ref `hop` owns keeps the tasklet live; JS thread, between tasks, so + // no other borrow of it is live. + let tasklet = unsafe { &mut *hop.0.as_ptr() }; + // Still producing into that stream? The body can have ended, or been cancelled, + // since the sweep that queued this hop. + if tasklet.response_stream_source.is_some() { + tasklet.on_stream_cancelled(); + } + drop(hop); + Ok(()) + } +} + +impl Drop for CollectedResponseStreamHop { + fn drop(&mut self) { + FetchTasklet::deref(self.0.as_ptr()); + } +} diff --git a/src/runtime/webcore/streams.rs b/src/runtime/webcore/streams.rs index 8e43c642b2d2..88a3aa4845a3 100644 --- a/src/runtime/webcore/streams.rs +++ b/src/runtime/webcore/streams.rs @@ -1010,6 +1010,32 @@ impl SourceHandle { | SourceHandle::TestingCancelOnDrain(_) => {} } } + + /// The source's JS wrapper was collected with no native consumer attached: + /// nothing can read what the producer delivers from now on. Called from a + /// GC sweep, so an arm may only schedule work, and none may run JS. The + /// fetch arm hands over the pointer itself: the work it queues needs the + /// write-capable pointer the handle carries, not a borrow taken here. + pub fn consumer_collected(&mut self) { + match *self { + SourceHandle::FetchResponseBody(p) => { + crate::webcore::fetch::fetch_tasklet::FetchTasklet::on_response_stream_collected( + p.into(), + ); + } + // Remaining variants do not act on this signal. + SourceHandle::None + | SourceHandle::JSController(_) + | SourceHandle::ServerRequestBody(_) + | SourceHandle::ByteStream(_) + | SourceHandle::FileReader(_) + | SourceHandle::Subprocess(_) + | SourceHandle::ShellWritable(_) + | SourceHandle::S3DownloadBody(_) + | SourceHandle::HTMLRewriter(_) + | SourceHandle::TestingCancelOnDrain(_) => {} + } + } } // ────────────────────────────────────────────────────────────────────────── diff --git a/test/js/web/fetch/fetch-backpressure.test.ts b/test/js/web/fetch/fetch-backpressure.test.ts index b9dab7131b6f..d021b0a27edc 100644 --- a/test/js/web/fetch/fetch-backpressure.test.ts +++ b/test/js/web/fetch/fetch-backpressure.test.ts @@ -487,3 +487,220 @@ describe.concurrent("fetch() receive backpressure — streaming consumer shapes" } }); }); + +// A body stream that exists but that nothing reads: `res.body` was touched, or its reader +// was released. Before, the first chunk delivered to such a stream switched the fetch over +// to buffering the rest of the body in memory, with no bound, and kept the process alive +// while it did. The transport has to stay paused instead (the bytes have nowhere to go), +// the stream has to stay readable, and the paused body must not hold an idle process. + +const BIG = 16384; // 1 GiB, as in the "server stops writing" tests above +const BODY = BIG * CHUNK; + +// Writes the body as fast as the socket takes it; `sent()` is how far it got. A paused +// client stops it at the socket buffers (up to a few hundred MiB on some hosts), a client +// that drains lets it write all of BODY. +async function serveUntilBlocked() { + let sent = 0; + let closed = 0; + const payload = Buffer.alloc(CHUNK, 65); + const srv = createServer((_req, res) => { + res.on("error", () => {}); + res.on("close", () => closed++); + res.flushHeaders(); + let i = 0; + const push = () => { + while (i < BIG && !res.destroyed) { + i++; + sent += CHUNK; + if (!res.write(payload)) return void res.once("drain", push); + } + if (!res.destroyed) res.end(); + }; + push(); + }); + srv.listen(0); + await once(srv, "listening"); + const { port } = srv.address() as import("node:net").AddressInfo; + return { + url: `http://127.0.0.1:${port}/`, + sent: () => sent, + // Resolves with `sent()` once it has not moved for 100ms. + async settled() { + let last = -1; + for (let stable = 0; stable < 5; ) { + await Bun.sleep(20); + stable = sent === last ? stable + 1 : 0; + last = sent; + } + return sent; + }, + async untilClosed() { + while (closed === 0) await Bun.sleep(5); + }, + [Symbol.asyncDispose]: () => { + srv.closeAllConnections(); + return new Promise(r => srv.close(() => r(undefined))); + }, + }; +} + +// Sends the headers and one chunk; the test writes the rest through `response`. +async function serveByHand() { + let respond!: (res: import("node:http").ServerResponse) => void; + const response = new Promise(resolve => (respond = resolve)); + const srv = createServer((_req, res) => { + res.on("error", () => {}); + res.flushHeaders(); + res.write(Buffer.alloc(CHUNK, 65)); + respond(res); + }); + srv.listen(0); + await once(srv, "listening"); + const { port } = srv.address() as import("node:net").AddressInfo; + return { + url: `http://127.0.0.1:${port}/`, + response, + [Symbol.asyncDispose]: () => { + srv.closeAllConnections(); + return new Promise(r => srv.close(() => r(undefined))); + }, + }; +} + +// Sequential on purpose: each test watches one server's write progress, and a sibling +// test draining a body on the same HTTP thread would blur that signal. +describe("fetch() receive backpressure — body stream nothing is reading", () => { + const shapes: [string, (res: Response) => Promise>][] = [ + ["res.body read by nothing", async res => res.body!], + [ + "one read(), then releaseLock()", + async res => { + const reader = res.body!.getReader(); + await reader.read(); + reader.releaseLock(); + return res.body!; + }, + ], + ]; + + for (const [name, shape] of shapes) { + test(`${name}: the server blocks, and a later reader resumes the body`, async () => { + await using server = await serveUntilBlocked(); + const body = await shape(await fetch(server.url)); + + expect(await server.settled()).toBeLessThan(BODY); + + const reader = body.getReader(); + let got = 0; + while (got < 16 * CHUNK) got += (await reader.read()).value!.byteLength; + + await reader.cancel(); + await server.untilClosed(); + }, 60_000); + } +}); + +describe.concurrent("fetch() receive backpressure — a body nothing waits for does not hold the process", () => { + // The client keeps the unread body reachable and has nothing else to do: it has to exit + // on its own. Before, it stayed alive draining the body into memory, or, when the pause + // won the race at start, stayed alive holding the paused body. + const idleClients = [ + ["the Response, body touched", /* js */ `globalThis.keep = res; void res.body;`], + [ + "the stream, reader released", + /* js */ `const reader = res.body.getReader(); await reader.read(); reader.releaseLock(); globalThis.keep = res.body;`, + ], + ["an idle reader", /* js */ `const reader = res.body.getReader(); await reader.read(); globalThis.keep = reader;`], + ] as const; + + for (const [holding, script] of idleClients) { + test(`a process holding ${holding} exits on its own`, async () => { + await using server = await serveUntilBlocked(); + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", `const res = await fetch(${JSON.stringify(server.url)}); ${script}`], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const exited = proc.exited.then(exitCode => ({ exitCode })); + const deadline = performance.now() + 10_000; + let outcome: { exitCode: number } | { stillAlive: true; serverSentMiB: number } | undefined; + while (outcome === undefined) { + outcome = await Promise.race([exited, Bun.sleep(20).then(() => undefined)]); + // Took the whole body: it is draining. Took little and is still around: it is + // holding the paused body. Neither exited. + if (outcome === undefined && (server.sent() >= BODY || performance.now() >= deadline)) { + outcome = { stillAlive: true, serverSentMiB: server.sent() >> 20 }; + proc.kill(); + } + } + expect({ outcome, stderr: await proc.stderr.text() }).toEqual({ outcome: { exitCode: 0 }, stderr: "" }); + }, 20_000); + } + + // The other half of the rule: a consumer that does wait for bytes keeps the process + // alive, including one that picks the body up again after a release (the chunk that + // arrives while the body is released is what lets go of the loop). + const waitingClients = [ + [ + "read() pending on a reader", + /* js */ ` + const reader = res.body.getReader(); + let total = (await reader.read()).value.byteLength; + console.log("waiting"); + for (let r; !(r = await reader.read()).done; ) total += r.value.byteLength; + console.log("total " + total); + `, + 2, + ], + [ + "read() pending on a reader acquired after a release", + /* js */ ` + let reader = res.body.getReader(); + let total = (await reader.read()).value.byteLength; + reader.releaseLock(); + console.log("released"); + // Lets the chunk the test writes on "released" arrive while nothing reads the body. + // A slow machine only narrows what this exercises; the assertions do not depend on it. + await Bun.sleep(50); + reader = res.body.getReader(); + const next = reader.read(); + console.log("waiting"); + for (let r = await next; !r.done; r = await reader.read()) total += r.value.byteLength; + console.log("total " + total); + `, + 3, + ], + ] as const; + + for (const [name, script, chunks] of waitingClients) { + test(`a process with a ${name} stays alive until the body ends`, async () => { + await using server = await serveByHand(); + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", `const res = await fetch(${JSON.stringify(server.url)}); ${script}`], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const upstream = await server.response; + const lines: string[] = []; + let pending = ""; + for await (const text of proc.stdout.pipeThrough(new TextDecoderStream())) { + pending += text; + for (let nl; (nl = pending.indexOf("\n")) !== -1; ) { + const line = pending.slice(0, nl); + pending = pending.slice(nl + 1); + lines.push(line); + if (line === "released") upstream.write(Buffer.alloc(CHUNK, 66)); + if (line === "waiting") upstream.end(Buffer.alloc(CHUNK, 67)); + } + } + expect({ lines: lines.slice(-2), stderr: await proc.stderr.text(), exitCode: await proc.exited }).toEqual({ + lines: ["waiting", `total ${chunks * CHUNK}`], + stderr: "", + exitCode: 0, + }); + }); + } +}); diff --git a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts index 9d84531c193a..a9d1e54bbbb4 100644 --- a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts +++ b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts @@ -1,5 +1,5 @@ import { heapStats } from "bun:jsc"; -import { expect, test } from "bun:test"; +import { describe, expect, test } from "bun:test"; import { bunEnv, bunExe } from "harness"; // Test that ReadableStream objects from cancelled fetch responses are properly GC'd. @@ -201,3 +201,56 @@ test("response.body.cancel() on a never-read body aborts the underlying fetch", exitCode: 0, }); }); + +// Nothing holds the body any more, so it has to go away: the stream is collected and the +// fetch behind it is aborted, in whatever state it was dropped. Before, the fetch rooted its +// own stream until the body ended, so against a body that does not end, the stream, the +// connection, and whatever the fetch went on buffering all lived until the process exited. +describe("an abandoned fetch body stream is collected and its fetch is aborted", () => { + const shapes: [string, (res: Response) => Promise][] = [ + ["res.body touched", async res => res.body], + [ + "one read(), then releaseLock()", + async res => { + const reader = res.body!.getReader(); + await reader.read(); + reader.releaseLock(); + }, + ], + ["dropped while a reader holds the lock", res => res.body!.getReader().read()], + ]; + + for (const [name, shape] of shapes) { + test(name, async () => { + let aborted = 0; + using server = Bun.serve({ + port: 0, + fetch(req) { + req.signal.addEventListener("abort", () => aborted++); + return new Response( + new ReadableStream({ + async pull(controller) { + await Bun.sleep(1); + controller.enqueue(new Uint8Array(4096)); + }, + }), + ); + }, + }); + const N = 20; + // Its own frame, so that nothing on this one still refers to a response afterwards. + async function abandonOne() { + await shape(await fetch(server.url)); + } + for (let i = 0; i < N; i++) await abandonOne(); + + const deadline = performance.now() + 3000; + while (aborted < N && performance.now() < deadline) { + Bun.gc(true); + await Bun.sleep(10); + } + // A couple can survive a collection through stale stack slots; the rest must go. + expect(N - aborted).toBeLessThanOrEqual(2); + }); + } +}); From 09ca80ef69c694a0caf130ff86556aae9625102b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 18 Aug 2026 23:15:12 +0000 Subject: [PATCH 02/22] fetch: root the source wrapper while a native sink is wired, pin the source across a delivery wire_native_sink stores the sink's cell in the source wrapper's sinkOwner slot, so the wrapper is what keeps a natively wired sink (HTMLRewriter) alive. With the producer ref no longer rooting the wrapper, a transform whose JS handles were dropped was collected while the fetch still delivered into it (use after free in RewriterPipe::write). NATIVE_REF_ROOTS_WRAPPER becomes a context method: ByteStream answers true while a sink is wired, and wire_native_sink re-evaluates it when it wires one. decrement_count still drops the root once only the wrapper's ref remains. A non-terminal delivery now holds its own ref on the source. The sink's handlers can cancel the body from inside on_data, which releases the producer ref, and a collection in the same script then freed the source under on_data; before, the stream handle that on_body_received kept on its stack happened to root it. on_response_stream_collected no longer forms a reference to the tasklet. The tests drain stderr concurrently and scale the collection deadline for slow builds; a new one drops every handle to a transform over a fetch body and collects while the body arrives. --- src/runtime/webcore/ByteStream.rs | 8 ++- src/runtime/webcore/ReadableStream.rs | 58 ++++++++++++++----- src/runtime/webcore/fetch/FetchTasklet.rs | 42 +++++++++----- test/js/web/fetch/fetch-backpressure.test.ts | 6 +- .../fetch/fetch-stream-cancel-leak.test.ts | 5 +- test/js/workerd/html-rewriter.test.js | 54 +++++++++++++++++ 6 files changed, 136 insertions(+), 37 deletions(-) diff --git a/src/runtime/webcore/ByteStream.rs b/src/runtime/webcore/ByteStream.rs index d56edd998bab..ea218aba0477 100644 --- a/src/runtime/webcore/ByteStream.rs +++ b/src/runtime/webcore/ByteStream.rs @@ -69,9 +69,6 @@ impl readable_stream::SourceContext for ByteStream { const NAME: &'static str = "Bytes"; // setRefUnrefFn = null const SUPPORTS_REF: bool = false; - // The only native ref holder is the producer (`FetchTasklet`), which never - // touches the wrapper; see `SourceContext::NATIVE_REF_ROOTS_WRAPPER`. - const NATIVE_REF_ROOTS_WRAPPER: bool = false; crate::source_context_codegen!(js_BytesInternalReadableStreamSource); // R-2: trait sigs are fixed at `&mut self` (shared with the other @@ -93,6 +90,11 @@ impl readable_stream::SourceContext for ByteStream { Self::on_wrapper_finalized(self); false } + /// The producer's ref (`FetchTasklet`) pins the wrapper only while the + /// wrapper roots a natively wired sink; see the trait method. + fn native_ref_roots_wrapper(&self) -> bool { + self.sink.get().is_some() + } fn drain_internal_buffer(&mut self) -> Vec { Self::drain(self) } diff --git a/src/runtime/webcore/ReadableStream.rs b/src/runtime/webcore/ReadableStream.rs index 879ef3cd7b2c..2905ffc5c04a 100644 --- a/src/runtime/webcore/ReadableStream.rs +++ b/src/runtime/webcore/ReadableStream.rs @@ -323,6 +323,18 @@ impl ReadableStream { byte_stream.parent_const().set_sink_owner(owner_cell); byte_stream.sink.set(sink); byte_stream.sink_paused.set(false); + // The `sinkOwner` slot set above is what roots `owner_cell` from now on, and + // this wrapper is what holds the slot. A producer that holds the source + // (`FetchTasklet`) keeps delivering to `sink` whether or not JS still reaches + // the wrapper, so the wrapper has to stay alive as long as that ref is held. + // Done before the first write below, which can run script and so collect. + // SAFETY: `Source::Bytes` is the wrapper's whole `m_ctx` allocation, live while + // the stream is; `byte_stream` is a back-reference, so no borrow of the context + // is live across this call. + unsafe { + let source = NewSource::from_context_ptr(NonNull::from(byte_stream).as_ptr()); + (*source).root_wrapper_if_needed(); + } self.lock_native(global); byte_stream.signal_consumer_attached(); @@ -750,14 +762,6 @@ pub trait SourceContext: Sized { const NAME: &'static str; /// `setRefUnrefFn != null` const SUPPORTS_REF: bool = false; - /// Whether a native ref ([`NewSource::increment_count`]) also roots the JS - /// wrapper. `FileReader` needs that: a read completes off the event loop and - /// then calls the wrapper's `onClose`. A producer that only writes into the - /// source's native memory (`FetchTasklet` into a `ByteStream`) opts out: the - /// wrapper holds the stream's consumer graph, so rooting it would keep a - /// stream nobody can reach alive for as long as the producer runs. - const NATIVE_REF_ROOTS_WRAPPER: bool = true; - // ─── codegen accessors (`.classes.ts` → `generated_classes.rs`) ─────────── // Each context binds its per-type codegen module via `source_context_codegen!`. /// `js_${NAME}InternalReadableStreamSource::to_js` — `ptr` is the @@ -795,6 +799,19 @@ pub trait SourceContext: Sized { /// `setRefUnrefFn` — default no-op. fn set_ref_unref(&mut self, _enable: bool) {} + /// Whether the JS wrapper must stay alive while a native ref beyond the + /// wrapper's own is held ([`NewSource::root_wrapper_if_needed`]). The + /// default holds for `FileReader`: its read completes off the event loop + /// and then calls the wrapper's `onClose`. `ByteStream` answers from its + /// state: the wrapper roots a natively wired sink's cell (`sinkOwner`), so + /// it has to live while a producer can still deliver to that sink, but + /// with no sink it is what the stream's consumers hold, and rooting it + /// would keep a stream nobody can reach alive for as long as the producer + /// (`FetchTasklet`) runs. + fn native_ref_roots_wrapper(&self) -> bool { + true + } + /// `drainInternalBuffer` — default returns empty. fn drain_internal_buffer(&mut self) -> Vec { Vec::::default() @@ -855,9 +872,8 @@ pub struct NewSource { pub global_this: Option>, /// Back-reference to the owning `JS{Blob,Bytes,File}InternalReadableStreamSource` /// wrapper. Starts `Weak` (set in [`Self::to_readable_stream`]), is - /// [`JsRef::upgrade`]d to `Strong` in [`Self::increment_count`] while a - /// native I/O ref is held (FileReader `waiting_for_on_reader_done`; contexts - /// with [`SourceContext::NATIVE_REF_ROOTS_WRAPPER`] unset skip this), and + /// [`JsRef::upgrade`]d to `Strong` by [`Self::root_wrapper_if_needed`] while a + /// native ref is held and the context needs the wrapper for it, and /// [`JsRef::downgrade`]d back to `Weak` in [`Self::decrement_count`] when /// only the wrapper's own ref remains. [`Self::finalize`] flips it to /// `Finalized` so [`Self::on_js_close`] reads `None` instead of a @@ -1148,13 +1164,23 @@ impl NewSource { pub fn increment_count(&mut self) { self.ref_count += 1; - if !C::NATIVE_REF_ROOTS_WRAPPER { + self.root_wrapper_if_needed(); + } + + /// Root the wrapper while a ref beyond the wrapper's own is held and the + /// context needs the wrapper alive for it + /// ([`SourceContext::native_ref_roots_wrapper`]): a FileReader + /// `waiting_for_on_reader_done` I/O ref, whose `on_js_close` is reached from + /// `on_reader_done` off the event loop with no JS frame on the stack and + /// must never read a dead-but-unswept cell; or a ByteStream that a producer + /// holds while a native sink is wired to it. Re-run whenever one of the two + /// inputs changes: a ref is taken ([`Self::increment_count`]) or a sink is + /// wired ([`ReadableStream::wire_native_sink`]). [`Self::decrement_count`] + /// drops the root once only the wrapper's own ref remains. + pub fn root_wrapper_if_needed(&mut self) { + if self.ref_count <= 1 || !self.context.native_ref_roots_wrapper() { return; } - // A ref beyond the JS wrapper's own is held (a FileReader - // `waiting_for_on_reader_done` I/O ref). Root the wrapper so - // `on_js_close`, reached from `on_reader_done` off the event loop with - // no JS frame on the stack, never reads a dead-but-unswept cell. if let Some(global) = self.global_this.as_deref() { if self.this_jsvalue.is_not_empty() { self.this_jsvalue.upgrade(global); diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 5f6662f3f9e9..fdc3094fd6ae 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -117,10 +117,11 @@ pub struct FetchTasklet { /// A counted ref on the response body stream's ByteStream source for as long as this /// tasklet is its `producer`. The body is delivered into, and unhooked through, this /// native memory, never through the JS wrappers, which GC (and the VM's last sweep) - /// destroys in no particular order. This tasklet holds nothing that roots the stream: - /// only its consumers (the Response wrapper, a reader, a pipe, a pending pull) keep it - /// alive, and once they are all gone the source reports the collected wrapper through - /// `on_response_stream_collected` (`SourceContext::NATIVE_REF_ROOTS_WRAPPER`). + /// destroys in no particular order. The ref roots the stream only while a native sink is + /// wired to the source (`NewSource::root_wrapper_if_needed`: the source's wrapper is what + /// roots that sink). Otherwise only the stream's consumers (the Response wrapper, a + /// reader, a pipe, a pending pull) keep it alive, and once they are all gone the source + /// reports the collected wrapper through `on_response_stream_collected`. pub(crate) response_stream_source: Option>, pub(crate) request_headers: Headers, pub(crate) promise: jsc::JSPromiseStrong, @@ -832,14 +833,22 @@ impl FetchTasklet { } } else if let Some(source) = self.response_stream_source { bun_output::scoped_log!(FetchTasklet, "onBodyReceived response_stream_source"); + // The delivery holds its own ref on the source. Delivering can run script (a + // native sink's handlers), which can cancel the body, and the cancel comes back + // through `on_stream_cancelled` and drops the producer ref while `on_data` is + // still on the stack; a collection in that script then destroys the wrapper and + // with it the source, under `on_data`. Released below, once nothing here uses it. + // SAFETY: live through our producer ref; no borrow of the source exists yet. + unsafe { (*source.as_ptr()).increment_count() }; let bytes = Self::response_bytes(source); bytes.size_hint.set(self.get_size_hint()); let chunk = self.scheduled_response_buffer.list.as_slice(); bytes.on_data(Self::temporary_chunk(chunk, false)); - // Delivering can cancel the stream, which unhooks us and drops our ref on `bytes`. + // Still the producer? (See above: the delivery can have cancelled the body.) if self.response_stream_source.is_some() { self.update_keep_alive_for_body_consumer(&bytes); } + Self::release_response_stream_source(source); return Ok(()); } @@ -1713,11 +1722,13 @@ impl FetchTasklet { Some(source) } - /// Drop the counted ref from `on_readable_stream_available`. When the stream's JS - /// wrapper is already gone this frees the source, so nothing may touch it afterwards. + /// Drop one of our counted refs on the source: the producer ref from + /// `on_readable_stream_available` (handed out by `take_response_stream_source`), or the + /// ref a delivery takes for its own duration. This can be the last ref (the stream's JS + /// wrapper can be gone by now) and then frees the source, so nothing may touch it + /// afterwards. fn release_response_stream_source(source: NonNull) { - // SAFETY: the ref `take_response_stream_source` handed out; `source` is not used - // after this. + // SAFETY: balances a ref this tasklet took; `source` is not used after this. unsafe { crate::webcore::byte_stream::Source::decrement_count(source.as_ptr()) }; } @@ -1747,15 +1758,18 @@ impl FetchTasklet { /// behind the current turn. `this` is the source's `producer` pointer, live because /// `clear_stream_handlers` clears that handle before a tasklet is freed. pub(crate) fn on_response_stream_collected(this: NonNull) { - // SAFETY: see above. Shared access only (a delivery on this tasklet can be on the - // stack); the hop keeps `this` itself for the exclusive access it needs later. - let tasklet = unsafe { this.as_ref() }; - let vm = tasklet.global_this.bun_vm(); + // SAFETY: see above. A delivery holding `&mut` to this tasklet can be on the stack, + // so no reference is formed here: one `Copy` field is read and the atomic count is + // bumped through the pointer. The hop keeps the pointer for the exclusive access it + // needs once it runs. + let global_this: GlobalRef = unsafe { (*this.as_ptr()).global_this }; + let vm = global_this.bun_vm(); if vm.is_shutting_down() { // Teardown aborts every fetch still in flight (`stop_for_vm_teardown`). return; } - tasklet.ref_(); + // SAFETY: as above; `ref_` only touches the interior-mutable counter. + unsafe { bun_ptr::ThreadSafeRefCount::::ref_(this.as_ptr()) }; let hop = bun_core::heap::into_raw(Box::new(CollectedResponseStreamHop(this))); vm.as_mut() .enqueue_task(ManagedTask::new_owned(hop, CollectedResponseStreamHop::run)); diff --git a/test/js/web/fetch/fetch-backpressure.test.ts b/test/js/web/fetch/fetch-backpressure.test.ts index d021b0a27edc..0909a1a30896 100644 --- a/test/js/web/fetch/fetch-backpressure.test.ts +++ b/test/js/web/fetch/fetch-backpressure.test.ts @@ -623,6 +623,7 @@ describe.concurrent("fetch() receive backpressure — a body nothing waits for d stdout: "pipe", stderr: "pipe", }); + const stderr = proc.stderr.text(); const exited = proc.exited.then(exitCode => ({ exitCode })); const deadline = performance.now() + 10_000; let outcome: { exitCode: number } | { stillAlive: true; serverSentMiB: number } | undefined; @@ -635,7 +636,7 @@ describe.concurrent("fetch() receive backpressure — a body nothing waits for d proc.kill(); } } - expect({ outcome, stderr: await proc.stderr.text() }).toEqual({ outcome: { exitCode: 0 }, stderr: "" }); + expect({ outcome, stderr: await stderr }).toEqual({ outcome: { exitCode: 0 }, stderr: "" }); }, 20_000); } @@ -683,6 +684,7 @@ describe.concurrent("fetch() receive backpressure — a body nothing waits for d stdout: "pipe", stderr: "pipe", }); + const stderr = proc.stderr.text(); const upstream = await server.response; const lines: string[] = []; let pending = ""; @@ -696,7 +698,7 @@ describe.concurrent("fetch() receive backpressure — a body nothing waits for d if (line === "waiting") upstream.end(Buffer.alloc(CHUNK, 67)); } } - expect({ lines: lines.slice(-2), stderr: await proc.stderr.text(), exitCode: await proc.exited }).toEqual({ + expect({ lines: lines.slice(-2), stderr: await stderr, exitCode: await proc.exited }).toEqual({ lines: ["waiting", `total ${chunks * CHUNK}`], stderr: "", exitCode: 0, diff --git a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts index a9d1e54bbbb4..c8eb5b02dec9 100644 --- a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts +++ b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts @@ -1,6 +1,6 @@ import { heapStats } from "bun:jsc"; import { describe, expect, test } from "bun:test"; -import { bunEnv, bunExe } from "harness"; +import { bunEnv, bunExe, isASAN, isDebug } from "harness"; // Test that ReadableStream objects from cancelled fetch responses are properly GC'd. // @@ -244,7 +244,8 @@ describe("an abandoned fetch body stream is collected and its fetch is aborted", } for (let i = 0; i < N; i++) await abandonOne(); - const deadline = performance.now() + 3000; + // Bounds the failing case only; the fixed build is done in well under a second. + const deadline = performance.now() + (isASAN || isDebug ? 15_000 : 3000); while (aborted < N && performance.now() < deadline) { Bun.gc(true); await Bun.sleep(10); diff --git a/test/js/workerd/html-rewriter.test.js b/test/js/workerd/html-rewriter.test.js index bd8164715e3c..dfc0333b5922 100644 --- a/test/js/workerd/html-rewriter.test.js +++ b/test/js/workerd/html-rewriter.test.js @@ -3101,4 +3101,58 @@ describe("GC pressure mid-rewrite", () => { }); expect(exitCode).toBe(0); }); + + // The transform's input is a fetch() body wired straight into the pipe. Once the caller + // keeps only the promise for the output, the pipe is reachable from nothing in JS; what + // keeps it alive is the input stream's source, which the fetch holds while it delivers. + // The body arrives across many turns, with collections in between: every chunk must + // still reach the handlers. + it("a transform over a fetch body survives GC once only its output promise is held", async () => { + const fixture = /* js */ ` + const CHUNKS = 20; + let release; + const gate = new Promise(resolve => (release = resolve)); + const server = Bun.serve({ + port: 0, + fetch: () => + new Response( + new ReadableStream({ + async start(controller) { + controller.enqueue(""); + await gate; + for (let i = 0; i < CHUNKS; i++) { + controller.enqueue("

" + Buffer.alloc(1000, "x").toString() + "

"); + await Bun.sleep(1); + } + controller.close(); + }, + }), + { headers: { "content-type": "text/html" } }, + ), + }); + async function start() { + const res = await fetch(server.url); + return new HTMLRewriter().on("p", { element(el) { el.setAttribute("seen", ""); } }).transform(res).text(); + } + const text = start(); + for (let i = 0; i < 5; i++) { Bun.gc(true); await Bun.sleep(1); } + release(); + for (let i = 0; i < 30; i++) { Bun.gc(true); await Bun.sleep(2); } + const html = await text; + console.log(JSON.stringify({ seen: html.split('

').length - 1, ended: html.endsWith("

") })); + server.stop(true); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ + stdout: JSON.stringify({ seen: 20, ended: true }), + stderr: "", + exitCode: 0, + }); + }); }); From 4e32355b33266054564e2628eee1a0b0d7a1c549 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 18 Aug 2026 23:22:51 +0000 Subject: [PATCH 03/22] fetch: shorten the lifecycle comments --- src/runtime/webcore/ByteStream.rs | 17 +-- src/runtime/webcore/ReadableStream.rs | 40 +++---- src/runtime/webcore/fetch/FetchTasklet.rs | 129 ++++++++-------------- src/runtime/webcore/streams.rs | 8 +- 4 files changed, 69 insertions(+), 125 deletions(-) diff --git a/src/runtime/webcore/ByteStream.rs b/src/runtime/webcore/ByteStream.rs index ea218aba0477..c2b74a4d0b4e 100644 --- a/src/runtime/webcore/ByteStream.rs +++ b/src/runtime/webcore/ByteStream.rs @@ -90,8 +90,7 @@ impl readable_stream::SourceContext for ByteStream { Self::on_wrapper_finalized(self); false } - /// The producer's ref (`FetchTasklet`) pins the wrapper only while the - /// wrapper roots a natively wired sink; see the trait method. + /// The wrapper roots a natively wired sink (`sinkOwner`), nothing else. fn native_ref_roots_wrapper(&self) -> bool { self.sink.get().is_some() } @@ -265,9 +264,8 @@ impl ByteStream { Vec::::move_from_list(list) } - /// A consumer now waits on this stream: a native sink was just wired to - /// it, or a pull found the buffer empty and went pending. The producer - /// keeps the process alive for it (`FetchTasklet::on_consumer_attached`). + /// A native sink was wired, or a pull went pending: a consumer now waits + /// for bytes (`FetchTasklet::on_consumer_attached` refs the loop for it). pub fn signal_consumer_attached(&self) { self.parent_const().producer.get().start(); } @@ -675,12 +673,9 @@ impl ByteStream { self.buffer.get().capacity() } - /// The JS wrapper was collected (`NewSource::finalize`): no stream, reader, - /// or node `Readable` can pull from this source again. A native consumer (a - /// `sink`, or a `buffer_action` that waits for the whole body) does not hold - /// the wrapper and still takes the bytes; with neither attached, the - /// producer is delivering into memory nobody can read. Runs inside a GC - /// sweep, so the producer may only schedule its teardown. + /// `NewSource::finalize`: nothing JS-side can pull from this source again. + /// A `sink` or a `buffer_action` does not hold the wrapper and still takes + /// the bytes; without one, tell the producer. Runs inside a GC sweep. fn on_wrapper_finalized(&self) { if self.sink.get().is_some() || self.buffer_action.get().is_some() { return; diff --git a/src/runtime/webcore/ReadableStream.rs b/src/runtime/webcore/ReadableStream.rs index 2905ffc5c04a..c9802f76390b 100644 --- a/src/runtime/webcore/ReadableStream.rs +++ b/src/runtime/webcore/ReadableStream.rs @@ -323,14 +323,11 @@ impl ReadableStream { byte_stream.parent_const().set_sink_owner(owner_cell); byte_stream.sink.set(sink); byte_stream.sink_paused.set(false); - // The `sinkOwner` slot set above is what roots `owner_cell` from now on, and - // this wrapper is what holds the slot. A producer that holds the source - // (`FetchTasklet`) keeps delivering to `sink` whether or not JS still reaches - // the wrapper, so the wrapper has to stay alive as long as that ref is held. - // Done before the first write below, which can run script and so collect. + // The wrapper's `sinkOwner` slot is now what roots `owner_cell`, so a producer + // holding the source must keep the wrapper alive too. Before the first write, + // which can run script. // SAFETY: `Source::Bytes` is the wrapper's whole `m_ctx` allocation, live while - // the stream is; `byte_stream` is a back-reference, so no borrow of the context - // is live across this call. + // the stream is; `byte_stream` is a back-reference, so no borrow is live here. unsafe { let source = NewSource::from_context_ptr(NonNull::from(byte_stream).as_ptr()); (*source).root_wrapper_if_needed(); @@ -799,15 +796,11 @@ pub trait SourceContext: Sized { /// `setRefUnrefFn` — default no-op. fn set_ref_unref(&mut self, _enable: bool) {} - /// Whether the JS wrapper must stay alive while a native ref beyond the - /// wrapper's own is held ([`NewSource::root_wrapper_if_needed`]). The - /// default holds for `FileReader`: its read completes off the event loop - /// and then calls the wrapper's `onClose`. `ByteStream` answers from its - /// state: the wrapper roots a natively wired sink's cell (`sinkOwner`), so - /// it has to live while a producer can still deliver to that sink, but - /// with no sink it is what the stream's consumers hold, and rooting it - /// would keep a stream nobody can reach alive for as long as the producer - /// (`FetchTasklet`) runs. + /// Whether a native ref on the source must also keep the JS wrapper alive + /// ([`NewSource::root_wrapper_if_needed`]). `FileReader`: always, its read + /// completion calls the wrapper's `onClose`. `ByteStream`: only while the + /// wrapper roots a wired sink; otherwise the producer's ref would pin a + /// stream nobody can reach for as long as the body lasts. fn native_ref_roots_wrapper(&self) -> bool { true } @@ -1167,16 +1160,11 @@ impl NewSource { self.root_wrapper_if_needed(); } - /// Root the wrapper while a ref beyond the wrapper's own is held and the - /// context needs the wrapper alive for it - /// ([`SourceContext::native_ref_roots_wrapper`]): a FileReader - /// `waiting_for_on_reader_done` I/O ref, whose `on_js_close` is reached from - /// `on_reader_done` off the event loop with no JS frame on the stack and - /// must never read a dead-but-unswept cell; or a ByteStream that a producer - /// holds while a native sink is wired to it. Re-run whenever one of the two - /// inputs changes: a ref is taken ([`Self::increment_count`]) or a sink is - /// wired ([`ReadableStream::wire_native_sink`]). [`Self::decrement_count`] - /// drops the root once only the wrapper's own ref remains. + /// Root the wrapper while a ref beyond its own is held and the context wants + /// that ([`SourceContext::native_ref_roots_wrapper`]). Called when either + /// input changes: [`Self::increment_count`], and + /// [`ReadableStream::wire_native_sink`]. [`Self::decrement_count`] drops the + /// root once only the wrapper's own ref remains. pub fn root_wrapper_if_needed(&mut self) { if self.ref_count <= 1 || !self.context.native_ref_roots_wrapper() { return; diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index fdc3094fd6ae..131f9e94b977 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -114,21 +114,17 @@ pub struct FetchTasklet { /// native response ref if we still need it when JS is discarted // Response is intrusively refcounted; modeled as a raw ptr. pub(crate) native_response: Option<*mut Response>, - /// A counted ref on the response body stream's ByteStream source for as long as this - /// tasklet is its `producer`. The body is delivered into, and unhooked through, this - /// native memory, never through the JS wrappers, which GC (and the VM's last sweep) - /// destroys in no particular order. The ref roots the stream only while a native sink is - /// wired to the source (`NewSource::root_wrapper_if_needed`: the source's wrapper is what - /// roots that sink). Otherwise only the stream's consumers (the Response wrapper, a - /// reader, a pipe, a pending pull) keep it alive, and once they are all gone the source - /// reports the collected wrapper through `on_response_stream_collected`. + /// A counted ref on the response body's ByteStream source while this tasklet is its + /// `producer`: the body is delivered and unhooked through native memory, not through JS + /// wrappers that GC destroys in any order. It does not keep the stream reachable + /// (`SourceContext::native_ref_roots_wrapper`); once the stream's consumers are gone, the + /// source reports that through `on_response_stream_collected`. pub(crate) response_stream_source: Option>, pub(crate) request_headers: Headers, pub(crate) promise: jsc::JSPromiseStrong, pub(crate) concurrent_task: ConcurrentTask, - /// Keeps the event loop alive while a body consumer waits for bytes. `JsCell`: the - /// ByteStream's drain signal re-enters `on_stream_drained` through a shared ref while a - /// delivery on this tasklet is on the stack. + /// Held while a body consumer waits for bytes. `JsCell`: the ByteStream's drain signal + /// reaches `on_stream_drained` through a shared ref during a delivery. pub poll_ref: JsCell, /// For Http Client requests /// when Content-Length is provided this represents the whole size of the request @@ -833,18 +829,15 @@ impl FetchTasklet { } } else if let Some(source) = self.response_stream_source { bun_output::scoped_log!(FetchTasklet, "onBodyReceived response_stream_source"); - // The delivery holds its own ref on the source. Delivering can run script (a - // native sink's handlers), which can cancel the body, and the cancel comes back - // through `on_stream_cancelled` and drops the producer ref while `on_data` is - // still on the stack; a collection in that script then destroys the wrapper and - // with it the source, under `on_data`. Released below, once nothing here uses it. - // SAFETY: live through our producer ref; no borrow of the source exists yet. + // The delivery holds its own ref: a sink's handlers run inside `on_data` and can + // cancel the body, which drops the producer ref (`on_stream_cancelled`), and a GC + // in there would then free the source under `on_data`. + // SAFETY: live through the producer ref; no borrow of the source exists yet. unsafe { (*source.as_ptr()).increment_count() }; let bytes = Self::response_bytes(source); bytes.size_hint.set(self.get_size_hint()); let chunk = self.scheduled_response_buffer.list.as_slice(); bytes.on_data(Self::temporary_chunk(chunk, false)); - // Still the producer? (See above: the delivery can have cancelled the body.) if self.response_stream_source.is_some() { self.update_keep_alive_for_body_consumer(&bytes); } @@ -1642,10 +1635,8 @@ impl FetchTasklet { return DrainResult::Aborted; } - // Creating the stream (`res.body`) does not by itself keep the process alive: - // whatever reads it refs the loop when it starts to wait (`on_consumer_attached`, - // `on_stream_drained`), synchronously, before the loop can run dry. A stream that - // is created and then never read holds nothing. + // No loop ref for creating the stream: a consumer takes one when it starts to wait + // (`on_consumer_attached`, `on_stream_drained`), before the loop can run dry. // The bytes already in `scheduled_response_buffer` are handed to the // new stream below. That is the drain `Paused` was waiting for, so @@ -1698,41 +1689,33 @@ impl FetchTasklet { } } - /// The ByteStream behind `source`. Valid while a counted ref pins the source: ours - /// (`response_stream_source`, or the pointer `take_response_stream_source` handed out - /// and `release_response_stream_source` has not consumed yet), or the JS wrapper's. + /// The ByteStream behind a source one of our refs still pins. fn response_bytes( source: NonNull, ) -> bun_ptr::BackRef { - // SAFETY: see above; `context` is a field of the live source. Every ByteStream - // method takes `&self` (R-2), so a shared borrow is all a delivery needs. + // SAFETY: the source is live (caller's ref); ByteStream is `&self`-only (R-2). bun_ptr::BackRef::new(unsafe { &(*source.as_ptr()).context }) } - /// Stop being the response ByteStream's producer. Returns the source, still pinned by - /// our counted ref, so a terminal chunk can be delivered into it first; the caller then - /// passes it to `release_response_stream_source`. Touches no JS cell. + /// Stop being the producer. The source stays pinned by our ref, so the caller can still + /// deliver a terminal chunk before `release_response_stream_source`. Touches no JS cell. fn take_response_stream_source( &mut self, ) -> Option> { let source = self.response_stream_source.take()?; - // SAFETY: pinned by the counted ref taken in `on_readable_stream_available`, which - // only `release_response_stream_source` drops. + // SAFETY: still pinned by the ref from `on_readable_stream_available`. unsafe { (*source.as_ptr()).producer.set(SourceHandle::None) }; Some(source) } - /// Drop one of our counted refs on the source: the producer ref from - /// `on_readable_stream_available` (handed out by `take_response_stream_source`), or the - /// ref a delivery takes for its own duration. This can be the last ref (the stream's JS - /// wrapper can be gone by now) and then frees the source, so nothing may touch it - /// afterwards. + /// Drop one of our refs (the producer's, or a delivery's). Can free the source: the JS + /// wrapper may be gone already. fn release_response_stream_source(source: NonNull) { // SAFETY: balances a ref this tasklet took; `source` is not used after this. unsafe { crate::webcore::byte_stream::Source::decrement_count(source.as_ptr()) }; } - /// Unhook from the response ByteStream (the stream can outlive us in JS) and release it. + /// Unhook from the response ByteStream (the stream can outlive us) and release it. /// Touches no JS cell. fn clear_stream_handlers(&mut self) { if let Some(source) = self.take_response_stream_source() { @@ -1750,34 +1733,28 @@ impl FetchTasklet { self.ignore_remaining_response_body(); } - /// The source's JS wrapper was collected, and with it every reader, pipe, and - /// node `Readable` that could still pull from it (`ByteStream::on_wrapper_finalized`). - /// Nobody can read the rest of this body: cancel it like `reader.cancel()` would. - /// This runs inside a GC sweep, where the tasklet must not be torn down (a delivery - /// can be on the stack, and the sweep must not run script), so the cancel is queued - /// behind the current turn. `this` is the source's `producer` pointer, live because - /// `clear_stream_handlers` clears that handle before a tasklet is freed. + /// `ByteStream::on_wrapper_finalized`: nothing can read the rest of this body, so cancel + /// it as `reader.cancel()` would. Runs inside a GC sweep, possibly under a delivery on + /// this tasklet, so the cancel is queued. `this` is the source's `producer` pointer, + /// live until `clear_stream_handlers`. pub(crate) fn on_response_stream_collected(this: NonNull) { - // SAFETY: see above. A delivery holding `&mut` to this tasklet can be on the stack, - // so no reference is formed here: one `Copy` field is read and the atomic count is - // bumped through the pointer. The hop keeps the pointer for the exclusive access it - // needs once it runs. + // SAFETY: `this` is live (see above). A `&mut` to it can be on the stack, so no + // reference is formed: a `Copy` field is read and the atomic count is bumped. let global_this: GlobalRef = unsafe { (*this.as_ptr()).global_this }; let vm = global_this.bun_vm(); if vm.is_shutting_down() { // Teardown aborts every fetch still in flight (`stop_for_vm_teardown`). return; } - // SAFETY: as above; `ref_` only touches the interior-mutable counter. + // SAFETY: as above. unsafe { bun_ptr::ThreadSafeRefCount::::ref_(this.as_ptr()) }; let hop = bun_core::heap::into_raw(Box::new(CollectedResponseStreamHop(this))); vm.as_mut() .enqueue_task(ManagedTask::new_owned(hop, CollectedResponseStreamHop::run)); } - /// The ByteStream handed bytes to a consumer. Re-arm the transport, and keep the - /// process alive for whatever that consumer asks for next; the next delivery - /// (`update_keep_alive_for_body_consumer`) settles whether anything is waiting. + /// The ByteStream handed bytes to a consumer: re-arm the transport, and hold the loop + /// until the next delivery settles whether the consumer waits for more. pub(crate) fn on_stream_drained(&self) { self.poll_ref .with_mut(|poll_ref| poll_ref.ref_(bun_io::js_vm_ctx())); @@ -1789,23 +1766,17 @@ impl FetchTasklet { } } - /// A consumer is waiting on the ByteStream: a pull went pending, or a native sink - /// was wired to it. Keep the process alive until the bytes it waits for arrive. + /// A pull went pending, or a native sink was wired: something waits for bytes. pub(crate) fn on_consumer_attached(&self) { self.poll_ref .with_mut(|poll_ref| poll_ref.ref_(bun_io::js_vm_ctx())); } - /// A chunk was just delivered to the ByteStream and more are coming. Something that - /// waits for the next one keeps the process alive: a pull the chunk did not satisfy, a - /// buffered consumer (`res.body.text()`), or a native sink. With none of those the - /// transport stays paused (`callback` flipped it as it delivered this chunk), and the - /// paused body must not keep an otherwise idle process alive: that is the state after a - /// reader is released, or after `res.body` is touched but never read. A consumer that - /// comes back pulls, and the pull refs the loop again (`on_consumer_attached`, - /// `on_stream_drained`); a consumer that is collected cancels the body instead - /// (`on_response_stream_collected`). A consumer that keeps reading pulls again in the - /// microtask that takes this chunk, before the loop can observe the unref. + /// After a non-terminal delivery: the loop is held only while something waits for the + /// next chunk. Otherwise the transport stays paused (a released reader, an untouched + /// `res.body`) and must not hold an idle process; a consumer that comes back pulls, + /// which refs again. A consumer that keeps reading pulls again in the microtask that + /// takes this chunk, before the loop can see the unref. fn update_keep_alive_for_body_consumer(&self, bytes: &crate::webcore::ByteStream) { let waiting = bytes.sink.get().is_some() || bytes.buffer_action.get().is_some() @@ -1934,9 +1905,8 @@ impl FetchTasklet { self.poll_ref .with_mut(|poll_ref| poll_ref.unref(bun_io::js_vm_ctx())); // Also fine from `on_response_finalize` (a JSC Weak finalizer inside - // `WeakBlock::sweep`): that path only gets here with no stream source held, so - // this touches no JS cell and frees nothing. The request-body sink is left for - // `clear_sink()` in `deinit()` (an event-loop task, outside sweep) to detach. + // `WeakBlock::sweep`), which only gets here with no source held. The request-body + // sink is left for `clear_sink()` in `deinit()` (an event-loop task, outside sweep). self.clear_stream_handlers(); self.response.clear(); @@ -2713,13 +2683,10 @@ impl FetchTasklet { // 2b. if we have a promise, we should keep loading the body. // 3. We never started buffering, in which case we should ignore the body. // - // This is called inside a finalizer: decide from native state only. + // Inside a finalizer: decide from native state only. if !matches!(body, BodyValue::Locked(_)) || this.response_stream_source.is_some() { - // Scenario 1: the stream outlives its Response for as long as something - // consumes it (`res.body` handed to a pipe, a reader, `Readable.fromWeb`). - // While nothing does, the transport sits paused without holding the loop - // (`update_keep_alive_for_body_consumer`); once the stream's wrapper is - // collected too, `on_response_stream_collected` cancels the body. + // Scenario 1: the stream outlives its Response while something consumes it; + // once it is collected too, `on_response_stream_collected` cancels the body. return; } @@ -2835,21 +2802,17 @@ impl bun_event_loop::Taskable for FetchTaskletPromiseSettle { } } -/// The hop `on_response_stream_collected` queues (a `ManagedTask::new_owned` context). -/// Owns one ref on the tasklet, released when the hop is dropped: after it ran, or -/// unrun when its VM tears down the queue. +/// `ManagedTask::new_owned` context queued by `on_response_stream_collected`. Owns one ref +/// on the tasklet, released on drop: after running, or unrun at VM teardown. struct CollectedResponseStreamHop(NonNull); impl CollectedResponseStreamHop { fn run(this: *mut Self) -> ElJsResult<()> { - // SAFETY: the box `on_response_stream_collected` queued; reclaimed exactly once - // here (the task's `cleanup` only runs when it is released unrun). + // SAFETY: the queued box; `cleanup` only runs for a task released unrun. let hop = unsafe { bun_core::heap::take(this) }; - // SAFETY: the ref `hop` owns keeps the tasklet live; JS thread, between tasks, so - // no other borrow of it is live. + // SAFETY: the ref `hop` owns keeps the tasklet live; between tasks, no other borrow. let tasklet = unsafe { &mut *hop.0.as_ptr() }; - // Still producing into that stream? The body can have ended, or been cancelled, - // since the sweep that queued this hop. + // The body can have ended or been cancelled since the sweep that queued this. if tasklet.response_stream_source.is_some() { tasklet.on_stream_cancelled(); } diff --git a/src/runtime/webcore/streams.rs b/src/runtime/webcore/streams.rs index 88a3aa4845a3..7430e8ebfe69 100644 --- a/src/runtime/webcore/streams.rs +++ b/src/runtime/webcore/streams.rs @@ -1011,11 +1011,9 @@ impl SourceHandle { } } - /// The source's JS wrapper was collected with no native consumer attached: - /// nothing can read what the producer delivers from now on. Called from a - /// GC sweep, so an arm may only schedule work, and none may run JS. The - /// fetch arm hands over the pointer itself: the work it queues needs the - /// write-capable pointer the handle carries, not a borrow taken here. + /// The source's JS wrapper was collected and no native consumer is attached: + /// nothing can read what the producer delivers from now on. Called from a GC + /// sweep: arms may schedule work, not run JS. pub fn consumer_collected(&mut self) { match *self { SourceHandle::FetchResponseBody(p) => { From 54487b25bc6bff178cf04bad25759396542372ee Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 18 Aug 2026 23:41:36 +0000 Subject: [PATCH 04/22] fetch: cancel a collected body stream from the sweep itself consumer_collected calls on_stream_cancelled directly, the way the close arm does for reader.cancel(). Everything it does is native state, which on_response_finalize already runs from a Weak finalizer, so the queued task, its ref on the tasklet, and the shutdown check go away. --- src/runtime/webcore/fetch/FetchTasklet.rs | 64 ++++------------------- src/runtime/webcore/streams.rs | 9 ++-- 2 files changed, 14 insertions(+), 59 deletions(-) diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 131f9e94b977..42b465ef88d1 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -7,7 +7,6 @@ use bun_cares_sys::c_ares_draft as c_ares; use bun_core::{MutableString, OwnedString, String as BunString, ZigStringSlice}; use bun_event_loop::{ ConcurrentTask::{AutoDeinit, ConcurrentTask}, - ManagedTask::ManagedTask, Task, Taskable, }; use bun_http as http; @@ -118,7 +117,7 @@ pub struct FetchTasklet { /// `producer`: the body is delivered and unhooked through native memory, not through JS /// wrappers that GC destroys in any order. It does not keep the stream reachable /// (`SourceContext::native_ref_roots_wrapper`); once the stream's consumers are gone, the - /// source reports that through `on_response_stream_collected`. + /// source cancels us through `SourceHandle::consumer_collected`. pub(crate) response_stream_source: Option>, pub(crate) request_headers: Headers, pub(crate) promise: jsc::JSPromiseStrong, @@ -1723,36 +1722,19 @@ impl FetchTasklet { } } + /// `reader.cancel()` / `body.cancel()`, and the stream's wrapper being collected + /// (`SourceHandle::consumer_collected`). The latter runs inside a GC sweep: everything + /// here is native state, as in `on_response_finalize`. pub(crate) fn on_stream_cancelled(&mut self) { if self.signal_store.body_receive_mode() == BodyReceiveMode::Ignore { return; } - // reader.cancel() / body.cancel() aborts the fetch so the server sees the - // close (Node/Deno/browsers abort unconditionally). abort_task() is idempotent. + // Abort so the server sees the close (Node/Deno/browsers abort unconditionally). + // abort_task() is idempotent. self.abort_task(); self.ignore_remaining_response_body(); } - /// `ByteStream::on_wrapper_finalized`: nothing can read the rest of this body, so cancel - /// it as `reader.cancel()` would. Runs inside a GC sweep, possibly under a delivery on - /// this tasklet, so the cancel is queued. `this` is the source's `producer` pointer, - /// live until `clear_stream_handlers`. - pub(crate) fn on_response_stream_collected(this: NonNull) { - // SAFETY: `this` is live (see above). A `&mut` to it can be on the stack, so no - // reference is formed: a `Copy` field is read and the atomic count is bumped. - let global_this: GlobalRef = unsafe { (*this.as_ptr()).global_this }; - let vm = global_this.bun_vm(); - if vm.is_shutting_down() { - // Teardown aborts every fetch still in flight (`stop_for_vm_teardown`). - return; - } - // SAFETY: as above. - unsafe { bun_ptr::ThreadSafeRefCount::::ref_(this.as_ptr()) }; - let hop = bun_core::heap::into_raw(Box::new(CollectedResponseStreamHop(this))); - vm.as_mut() - .enqueue_task(ManagedTask::new_owned(hop, CollectedResponseStreamHop::run)); - } - /// The ByteStream handed bytes to a consumer: re-arm the transport, and hold the loop /// until the next delivery settles whether the consumer waits for more. pub(crate) fn on_stream_drained(&self) { @@ -1904,9 +1886,10 @@ impl FetchTasklet { // we should not keep the process alive if we are ignoring the body self.poll_ref .with_mut(|poll_ref| poll_ref.unref(bun_io::js_vm_ctx())); - // Also fine from `on_response_finalize` (a JSC Weak finalizer inside - // `WeakBlock::sweep`), which only gets here with no source held. The request-body - // sink is left for `clear_sink()` in `deinit()` (an event-loop task, outside sweep). + // Also reached from GC sweeps (`on_response_finalize`, `consumer_collected`): this + // touches no JS cell. Inside the source wrapper's own finalize, the release below + // leaves the wrapper's ref to free the source. The request-body sink is left for + // `clear_sink()` in `deinit()` (an event-loop task, outside sweep). self.clear_stream_handlers(); self.response.clear(); @@ -2686,7 +2669,7 @@ impl FetchTasklet { // Inside a finalizer: decide from native state only. if !matches!(body, BodyValue::Locked(_)) || this.response_stream_source.is_some() { // Scenario 1: the stream outlives its Response while something consumes it; - // once it is collected too, `on_response_stream_collected` cancels the body. + // its own collection cancels the body (`SourceHandle::consumer_collected`). return; } @@ -2801,28 +2784,3 @@ impl bun_event_loop::Taskable for FetchTaskletPromiseSettle { drop(unsafe { bun_core::heap::take(this) }); } } - -/// `ManagedTask::new_owned` context queued by `on_response_stream_collected`. Owns one ref -/// on the tasklet, released on drop: after running, or unrun at VM teardown. -struct CollectedResponseStreamHop(NonNull); - -impl CollectedResponseStreamHop { - fn run(this: *mut Self) -> ElJsResult<()> { - // SAFETY: the queued box; `cleanup` only runs for a task released unrun. - let hop = unsafe { bun_core::heap::take(this) }; - // SAFETY: the ref `hop` owns keeps the tasklet live; between tasks, no other borrow. - let tasklet = unsafe { &mut *hop.0.as_ptr() }; - // The body can have ended or been cancelled since the sweep that queued this. - if tasklet.response_stream_source.is_some() { - tasklet.on_stream_cancelled(); - } - drop(hop); - Ok(()) - } -} - -impl Drop for CollectedResponseStreamHop { - fn drop(&mut self) { - FetchTasklet::deref(self.0.as_ptr()); - } -} diff --git a/src/runtime/webcore/streams.rs b/src/runtime/webcore/streams.rs index 7430e8ebfe69..c0cf396330e1 100644 --- a/src/runtime/webcore/streams.rs +++ b/src/runtime/webcore/streams.rs @@ -1013,14 +1013,11 @@ impl SourceHandle { /// The source's JS wrapper was collected and no native consumer is attached: /// nothing can read what the producer delivers from now on. Called from a GC - /// sweep: arms may schedule work, not run JS. + /// sweep: arms must not run JS. pub fn consumer_collected(&mut self) { match *self { - SourceHandle::FetchResponseBody(p) => { - crate::webcore::fetch::fetch_tasklet::FetchTasklet::on_response_stream_collected( - p.into(), - ); - } + // SAFETY: live backref; cleared before the pointee is freed. + SourceHandle::FetchResponseBody(mut p) => unsafe { p.get_mut() }.on_stream_cancelled(), // Remaining variants do not act on this signal. SourceHandle::None | SourceHandle::JSController(_) From 2d80b571922487df1ada217bd09c39045e2a819b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 19 Aug 2026 01:58:19 +0000 Subject: [PATCH 05/22] streams: cancel a mid-body producer when its native sink goes away A ByteStream with a native sink is locked to that sink. When the sink stopped taking bytes (its write returned Done or Err, or the response it fed was torn down), the stream only dropped the sink. The producer was left paused on its connection, and the fetch's ref kept the stream's wrapper rooted, so nothing ever collected it. ByteStream::detach_finished_sink drops the sink and, if the body is not over, closes the producer the same way cancel_from_sink does for a sink that closes itself. The Done/Err arms of on_data and resume, the inline Done/Err arms of wire_native_sink and the S3 upload, and the server's response teardown go through it. RequestContext::on_abort now tears the native pipe down itself: the ref the pipe took was only released by end_chunk, which a body the client left idle or backpressured never reached, so the context, the stream, and the upstream connection lived until the process exited. Tests: a Bun.serve proxy returning the upstream Response (the native pipe) now aborts the upstream when its client leaves, while the client is reading and while it is not reading at all. Both fail on main. --- src/runtime/server/RequestContext.rs | 25 ++++- src/runtime/webcore/ByteStream.rs | 37 +++++-- src/runtime/webcore/ReadableStream.rs | 11 +- src/runtime/webcore/s3/client.rs | 2 +- .../fetch/fetch-stream-cancel-leak.test.ts | 104 ++++++++++++++++++ 5 files changed, 158 insertions(+), 21 deletions(-) diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index 9177e79a8617..f4aecad0b623 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -400,14 +400,17 @@ mod shim { .as_ref() .is_some_and(|s| matches!(s.data, crate::webcore::blob::store::Data::File(_))) } + /// The response is going away (ended, or the client left). A body still + /// mid-stream is cancelled with it: it stayed locked to this response, so + /// a proxied fetch would otherwise sit paused on its connection for good. #[inline] pub(super) fn byte_stream_unpipe(s: NonNull) { - // The lone caller has just `take()`n the pointer out of - // `self.byte_stream`; the allocation is kept alive by - // `response_body_readable_stream_ref` (BackRef invariant: pointee - // outlives this temporary). R-2: `unpipe_without_deref` takes `&self` - // (interior-mutable `JsCell`), so shared deref is sufficient. - bun_ptr::BackRef::from(s).unpipe_without_deref() + // The caller has just `take()`n the pointer out of `self.byte_stream` + // and still holds `response_body_readable_stream_ref`, which keeps the + // allocation alive (BackRef invariant: pointee outlives this + // temporary). R-2: `detach_finished_sink` takes `&self`, so shared + // deref is sufficient. + bun_ptr::BackRef::from(s).detach_finished_sink() } } use crate::server::DevErrorPage; @@ -1396,6 +1399,16 @@ where return; } + // A body piped natively into this response (`Source::Bytes` in + // `do_render_with_body`) has nobody left to take it: cancel it, and + // release the ref the pipe took. `end_chunk`, which releases that ref + // otherwise, is never reached once the sink is gone, and a body the + // client left idle or backpressured would not have reached it anyway. + if let Some(stream) = this.byte_stream.take() { + shim::byte_stream_unpipe(stream); + this.deref(); + } + // if we can, free the request now. if this.is_dead_request() { this.finalize_without_deinit(); diff --git a/src/runtime/webcore/ByteStream.rs b/src/runtime/webcore/ByteStream.rs index c2b74a4d0b4e..eda6f7061173 100644 --- a/src/runtime/webcore/ByteStream.rs +++ b/src/runtime/webcore/ByteStream.rs @@ -186,6 +186,29 @@ impl ByteStream { self.sink_paused.set(false); } + /// The sink takes nothing more: it answered a write with `Done` or `Err`, or it is + /// being torn down. The stream stays locked to it, so the rest of the body has no + /// taker either. A producer still mid-body is closed the way [`Self::cancel_from_sink`] + /// closes it for a sink that closes itself; left alone it would sit paused on its + /// connection for good. + pub(crate) fn detach_finished_sink(&self) { + if self.has_received_last_chunk.get() { + self.unpipe_without_deref(); + } else { + self.cancel_from_sink(None); + } + } + + /// [`Self::detach_finished_sink`] for a sink this stream ends itself. `end` only + /// detaches the sink from the stream (`FileSink::end_from_stream`, + /// `FetchRequestBodySink::end_from_stream`): the producer is still attached after + /// it and still holds the source, so it is closed last. + fn end_finished_sink(&self, sink: SinkHandle, err: Option) { + self.unpipe_without_deref(); + sink.end(err); + self.detach_finished_sink(); + } + /// Sink's drain ack: unpause, push buffered bytes, end if last chunk already arrived. pub fn resume(&self) { if !self.sink_paused.get() { @@ -212,13 +235,11 @@ impl ByteStream { return; } streams::Writable::Err(e) => { - self.sink.set(SinkHandle::None); - sink.end(Some(streams::StreamError::Error(e))); + self.end_finished_sink(sink, Some(streams::StreamError::Error(e))); return; } streams::Writable::Done => { - self.sink.set(SinkHandle::None); - sink.end(None); + self.end_finished_sink(sink, None); return; } _ => {} @@ -311,15 +332,11 @@ impl ByteStream { self.sink_paused.set(true); } streams::Writable::Err(e) => { - self.sink.set(SinkHandle::None); - self.sink_paused.set(false); - sink.end(Some(streams::StreamError::Error(e))); + self.end_finished_sink(sink, Some(streams::StreamError::Error(e))); return; } streams::Writable::Done => { - self.sink.set(SinkHandle::None); - self.sink_paused.set(false); - sink.end(None); + self.end_finished_sink(sink, None); return; } _ => { diff --git a/src/runtime/webcore/ReadableStream.rs b/src/runtime/webcore/ReadableStream.rs index c9802f76390b..0b004a26004d 100644 --- a/src/runtime/webcore/ReadableStream.rs +++ b/src/runtime/webcore/ReadableStream.rs @@ -26,8 +26,9 @@ pub enum NativeWireResult { /// Not a native source (no `Bytes`/`File` backing, or it already has a /// sink). Fall through to the JS-pump path. NotNative, - /// The source was already done or errored. The sink was not left - /// installed; the caller runs its own end-of-stream handling. + /// The source was already done or errored, or the sink took no more than + /// the bytes buffered so far (the source is cancelled then). The sink was + /// not left installed; the caller runs its own end-of-stream handling. EndedInline(Option), } @@ -351,7 +352,7 @@ impl ReadableStream { match sink.write(&chunk) { Writable::Backpressure(_) => byte_stream.sink_paused.set(true), Writable::Done | Writable::Err(_) => { - byte_stream.sink.set(SinkHandle::None); + byte_stream.detach_finished_sink(); return NativeWireResult::EndedInline(None); } _ => {} @@ -1164,7 +1165,9 @@ impl NewSource { /// that ([`SourceContext::native_ref_roots_wrapper`]). Called when either /// input changes: [`Self::increment_count`], and /// [`ReadableStream::wire_native_sink`]. [`Self::decrement_count`] drops the - /// root once only the wrapper's own ref remains. + /// root once only the wrapper's own ref remains; a sink that goes away + /// mid-body closes the producer (`ByteStream::detach_finished_sink`, + /// `ByteStream::cancel_from_sink`), and the producer's release is that drop. pub fn root_wrapper_if_needed(&mut self) { if self.ref_count <= 1 || !self.context.native_ref_roots_wrapper() { return; diff --git a/src/runtime/webcore/s3/client.rs b/src/runtime/webcore/s3/client.rs index 68d5744d521d..4e46b3067c3a 100644 --- a/src/runtime/webcore/s3/client.rs +++ b/src/runtime/webcore/s3/client.rs @@ -1063,7 +1063,7 @@ pub(crate) fn upload_stream( } crate::webcore::streams::Writable::Done | crate::webcore::streams::Writable::Err(_) => { - byte_stream.sink.set(crate::webcore::SinkHandle::None); + byte_stream.detach_finished_sink(); sink.source.clear(); if !sink.ended { let _ = sink.end(None); diff --git a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts index c8eb5b02dec9..a30cf85b7cd1 100644 --- a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts +++ b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts @@ -255,3 +255,107 @@ describe("an abandoned fetch body stream is collected and its fetch is aborted", }); } }); + +// A fetch body handed to something that takes it natively, here a `Bun.serve` response that +// proxies it. When that response's own client goes away, the fetch behind the body has to be +// aborted with it: the body is locked to the response, so nothing else can read the rest. +// Before, the response only let go of the body, and the fetch sat paused on its connection +// (one upstream connection leaked per proxy client that went away). +describe("a proxied fetch body is aborted once the response's client is gone", () => { + const CHUNK = 64 * 1024; + // More than loopback socket buffers absorb, so a client that reads nothing backpressures + // the whole chain. Finite, so a build that keeps draining stays bounded. + const CHUNKS = 512; + + const shapes: [string, (upstream: Response) => Response][] = [ + // Goes through the JS pump; passes before and after, it is here so both spellings stay covered. + ["new Response(upstream.body)", upstream => new Response(upstream.body)], + // The native pipe. + ["the upstream Response itself", upstream => upstream], + ]; + + function servers(proxyResponse: (upstream: Response) => Response) { + const state = { pulls: 0, upstreamAborted: false, clientGone: Promise.withResolvers() }; + const upstream = Bun.serve({ + port: 0, + fetch(req) { + req.signal.addEventListener("abort", () => (state.upstreamAborted = true)); + return new Response( + new ReadableStream({ + pull(controller) { + if (++state.pulls <= CHUNKS) { + controller.enqueue(new Uint8Array(CHUNK)); + return; + } + // Out of data, but the body is not over: the upstream is still mid-body. + return new Promise(() => {}); + }, + }), + ); + }, + }); + const proxy = Bun.serve({ + port: 0, + async fetch(req) { + req.signal.addEventListener("abort", () => state.clientGone.resolve()); + return proxyResponse(await fetch(upstream.url)); + }, + }); + return { + state, + proxy, + [Symbol.dispose]() { + proxy.stop(true); + upstream.stop(true); + }, + }; + } + + async function expectUpstreamAborted(state: { upstreamAborted: boolean }) { + // Bounds the failing case only; the fixed build aborts the upstream as the client goes. + const deadline = performance.now() + 3000; + while (!state.upstreamAborted && performance.now() < deadline) await Bun.sleep(10); + expect(state.upstreamAborted).toBe(true); + } + + for (const [name, proxyResponse] of shapes) { + test(`${name}, client leaves while reading`, async () => { + using chain = servers(proxyResponse); + const { state } = chain; + + const client = new AbortController(); + const res = await fetch(chain.proxy.url, { signal: client.signal }); + const { value } = await res.body!.getReader().read(); + expect(value!.byteLength).toBeGreaterThan(0); + client.abort(); + await state.clientGone.promise; + + await expectUpstreamAborted(state); + }); + + test(`${name}, client leaves without reading`, async () => { + using chain = servers(proxyResponse); + const { state } = chain; + + const client = new AbortController(); + const res = await fetch(chain.proxy.url, { signal: client.signal }); + expect(res.status).toBe(200); + // Nothing reads: wait for the upstream to stop being pulled, which is the chain + // backpressured end to end (or, on a build that drains, out of data). + let last = -1; + for (let stable = 0; stable < 5; ) { + await Bun.sleep(20); + if (state.pulls === last) { + stable++; + } else { + stable = 0; + last = state.pulls; + } + } + client.abort(); + await state.clientGone.promise; + + await expectUpstreamAborted(state); + }); + } +}); From 4bcc3ecadc9ce3bc412f9269037d6d1e246f6e64 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 19 Aug 2026 02:03:41 +0000 Subject: [PATCH 06/22] streams: shorten the sink teardown comments --- src/runtime/server/RequestContext.rs | 20 +++++++------------- src/runtime/webcore/ByteStream.rs | 13 +++++-------- src/runtime/webcore/ReadableStream.rs | 5 ++--- 3 files changed, 14 insertions(+), 24 deletions(-) diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index f4aecad0b623..f76a29efbbe4 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -400,16 +400,12 @@ mod shim { .as_ref() .is_some_and(|s| matches!(s.data, crate::webcore::blob::store::Data::File(_))) } - /// The response is going away (ended, or the client left). A body still - /// mid-stream is cancelled with it: it stayed locked to this response, so - /// a proxied fetch would otherwise sit paused on its connection for good. + /// The response is done with its natively piped body. A body still + /// mid-stream is cancelled: it stayed locked to this response. #[inline] pub(super) fn byte_stream_unpipe(s: NonNull) { - // The caller has just `take()`n the pointer out of `self.byte_stream` - // and still holds `response_body_readable_stream_ref`, which keeps the - // allocation alive (BackRef invariant: pointee outlives this - // temporary). R-2: `detach_finished_sink` takes `&self`, so shared - // deref is sufficient. + // The caller has just `take()`n `self.byte_stream` and still holds + // `response_body_readable_stream_ref`, which keeps the pointee alive. bun_ptr::BackRef::from(s).detach_finished_sink() } } @@ -1399,11 +1395,9 @@ where return; } - // A body piped natively into this response (`Source::Bytes` in - // `do_render_with_body`) has nobody left to take it: cancel it, and - // release the ref the pipe took. `end_chunk`, which releases that ref - // otherwise, is never reached once the sink is gone, and a body the - // client left idle or backpressured would not have reached it anyway. + // A natively piped body has nobody left to take it. The deref balances + // the ref `do_render_with_body` took for the pipe: `end_chunk`, which + // releases it otherwise, cannot run once the sink is gone. if let Some(stream) = this.byte_stream.take() { shim::byte_stream_unpipe(stream); this.deref(); diff --git a/src/runtime/webcore/ByteStream.rs b/src/runtime/webcore/ByteStream.rs index eda6f7061173..dfa220b01d2f 100644 --- a/src/runtime/webcore/ByteStream.rs +++ b/src/runtime/webcore/ByteStream.rs @@ -186,11 +186,9 @@ impl ByteStream { self.sink_paused.set(false); } - /// The sink takes nothing more: it answered a write with `Done` or `Err`, or it is - /// being torn down. The stream stays locked to it, so the rest of the body has no - /// taker either. A producer still mid-body is closed the way [`Self::cancel_from_sink`] - /// closes it for a sink that closes itself; left alone it would sit paused on its - /// connection for good. + /// The sink takes nothing more (its write answered `Done`/`Err`, or it is being torn + /// down). The stream stays locked to it, so a producer still mid-body is closed, as + /// [`Self::cancel_from_sink`] does for a sink that closes itself. pub(crate) fn detach_finished_sink(&self) { if self.has_received_last_chunk.get() { self.unpipe_without_deref(); @@ -200,9 +198,8 @@ impl ByteStream { } /// [`Self::detach_finished_sink`] for a sink this stream ends itself. `end` only - /// detaches the sink from the stream (`FileSink::end_from_stream`, - /// `FetchRequestBodySink::end_from_stream`): the producer is still attached after - /// it and still holds the source, so it is closed last. + /// detaches the sink (see `FileSink::end_from_stream`), so the producer, which still + /// holds the source, is closed after it. fn end_finished_sink(&self, sink: SinkHandle, err: Option) { self.unpipe_without_deref(); sink.end(err); diff --git a/src/runtime/webcore/ReadableStream.rs b/src/runtime/webcore/ReadableStream.rs index 0b004a26004d..dfcff34bf2a6 100644 --- a/src/runtime/webcore/ReadableStream.rs +++ b/src/runtime/webcore/ReadableStream.rs @@ -1165,9 +1165,8 @@ impl NewSource { /// that ([`SourceContext::native_ref_roots_wrapper`]). Called when either /// input changes: [`Self::increment_count`], and /// [`ReadableStream::wire_native_sink`]. [`Self::decrement_count`] drops the - /// root once only the wrapper's own ref remains; a sink that goes away - /// mid-body closes the producer (`ByteStream::detach_finished_sink`, - /// `ByteStream::cancel_from_sink`), and the producer's release is that drop. + /// root once only the wrapper's own ref remains (a sink that goes away + /// mid-body closes the producer, `ByteStream::detach_finished_sink`). pub fn root_wrapper_if_needed(&mut self) { if self.ref_count <= 1 || !self.context.native_ref_roots_wrapper() { return; From 98475605968ee22c88a28a148842fdcaf6a7293a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 19 Aug 2026 03:31:56 +0000 Subject: [PATCH 07/22] fetch, s3: cancel a body stream through a shared ref A sink that is done with the body closes the producer from inside the producer's own delivery (ByteStream::detach_finished_sink under on_data), so SourceHandle::close must not form a second &mut to the producer there. on_stream_cancelled and what it touches take &self now: the tasklet's response_stream_source and native_response become Cells, the S3 download wrapper's stream ref and task pointer too. Weak::clear and AsyncHTTP::enable_response_body_streaming only write through pointers they hold, so they take &self as well. --- src/http/AsyncHTTP.rs | 4 +- src/jsc/Weak.rs | 4 +- src/runtime/webcore/fetch/FetchTasklet.rs | 54 ++++++++++++----------- src/runtime/webcore/s3/client.rs | 25 ++++++----- src/runtime/webcore/streams.rs | 12 ++--- 5 files changed, 54 insertions(+), 45 deletions(-) diff --git a/src/http/AsyncHTTP.rs b/src/http/AsyncHTTP.rs index 970c61bbe591..577fb8ece599 100644 --- a/src/http/AsyncHTTP.rs +++ b/src/http/AsyncHTTP.rs @@ -280,7 +280,9 @@ impl<'a> AsyncHTTP<'a> { &MAX_SIMULTANEOUS_REQUESTS } - pub fn enable_response_body_streaming(&mut self) { + /// A store into the caller's signal `Store`, not into `self`; shared so the + /// JS side can flip it from a callback that only has a shared tasklet. + pub fn enable_response_body_streaming(&self) { self.signals.store( crate::signals::Field::ResponseBodyStreaming, true, diff --git a/src/jsc/Weak.rs b/src/jsc/Weak.rs index 7241e3742d62..fa01cc157a81 100644 --- a/src/jsc/Weak.rs +++ b/src/jsc/Weak.rs @@ -137,7 +137,9 @@ impl Weak { Some(result) } - pub fn clear(&mut self) { + /// Clears the C++ slot; `self` keeps the (now empty) handle, so this needs + /// no more than a shared ref, like [`Self::get`]. + pub fn clear(&self) { let Some(r#ref) = self.r#ref else { return; }; diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 42b465ef88d1..1820f5564c10 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -1,3 +1,4 @@ +use core::cell::Cell; use core::ffi::c_void; use core::ptr::NonNull; use core::sync::atomic::{AtomicBool, Ordering}; @@ -111,14 +112,17 @@ pub struct FetchTasklet { /// response weak ref we need this to track the response JS lifetime pub(crate) response: jsc::Weak, /// native response ref if we still need it when JS is discarted - // Response is intrusively refcounted; modeled as a raw ptr. - pub(crate) native_response: Option<*mut Response>, + // Response is intrusively refcounted; modeled as a raw ptr. `Cell`, like + // `response_stream_source`: `on_stream_cancelled` releases it from inside a delivery. + pub(crate) native_response: Cell>, /// A counted ref on the response body's ByteStream source while this tasklet is its /// `producer`: the body is delivered and unhooked through native memory, not through JS /// wrappers that GC destroys in any order. It does not keep the stream reachable /// (`SourceContext::native_ref_roots_wrapper`); once the stream's consumers are gone, the - /// source cancels us through `SourceHandle::consumer_collected`. - pub(crate) response_stream_source: Option>, + /// source cancels us through `SourceHandle::consumer_collected`. `Cell`: the stream's + /// sink can cancel us (`on_stream_cancelled`, a shared ref) from inside `on_data`, while + /// the delivery that called it is on the stack. + pub(crate) response_stream_source: Cell>>, pub(crate) request_headers: Headers, pub(crate) promise: jsc::JSPromiseStrong, pub(crate) concurrent_task: ConcurrentTask, @@ -585,7 +589,7 @@ impl FetchTasklet { fn get_current_response(&self) -> Option<*mut Response> { // we need a body to resolve the promise when buffering - if let Some(response) = self.native_response { + if let Some(response) = self.native_response.get() { return Some(response); } @@ -826,7 +830,7 @@ impl FetchTasklet { Self::release_response_stream_source(source); return Ok(()); } - } else if let Some(source) = self.response_stream_source { + } else if let Some(source) = self.response_stream_source.get() { bun_output::scoped_log!(FetchTasklet, "onBodyReceived response_stream_source"); // The delivery holds its own ref: a sink's handlers run inside `on_data` and can // cancel the body, which drops the producer ref (`on_stream_cancelled`), and a GC @@ -837,7 +841,7 @@ impl FetchTasklet { bytes.size_hint.set(self.get_size_hint()); let chunk = self.scheduled_response_buffer.list.as_slice(); bytes.on_data(Self::temporary_chunk(chunk, false)); - if self.response_stream_source.is_some() { + if self.response_stream_source.get().is_some() { self.update_keep_alive_for_body_consumer(&bytes); } Self::release_response_stream_source(source); @@ -1620,7 +1624,7 @@ impl FetchTasklet { unsafe { let source = crate::webcore::readable_stream::NewSource::from_context_ptr(bytes); (*source).increment_count(); - this.response_stream_source = NonNull::new(source); + this.response_stream_source.set(NonNull::new(source)); } } // A ByteStream now drains scheduled_response_buffer per chunk; undo any @@ -1698,9 +1702,7 @@ impl FetchTasklet { /// Stop being the producer. The source stays pinned by our ref, so the caller can still /// deliver a terminal chunk before `release_response_stream_source`. Touches no JS cell. - fn take_response_stream_source( - &mut self, - ) -> Option> { + fn take_response_stream_source(&self) -> Option> { let source = self.response_stream_source.take()?; // SAFETY: still pinned by the ref from `on_readable_stream_available`. unsafe { (*source.as_ptr()).producer.set(SourceHandle::None) }; @@ -1716,16 +1718,17 @@ impl FetchTasklet { /// Unhook from the response ByteStream (the stream can outlive us) and release it. /// Touches no JS cell. - fn clear_stream_handlers(&mut self) { + fn clear_stream_handlers(&self) { if let Some(source) = self.take_response_stream_source() { Self::release_response_stream_source(source); } } - /// `reader.cancel()` / `body.cancel()`, and the stream's wrapper being collected - /// (`SourceHandle::consumer_collected`). The latter runs inside a GC sweep: everything - /// here is native state, as in `on_response_finalize`. - pub(crate) fn on_stream_cancelled(&mut self) { + /// `reader.cancel()` / `body.cancel()`, a sink that is done with the body, and the + /// stream's wrapper being collected (`SourceHandle::consumer_collected`). The sink case + /// runs inside one of our own deliveries and the last one inside a GC sweep, hence the + /// shared ref: everything here is native state, as in `on_response_finalize`. + pub(crate) fn on_stream_cancelled(&self) { if self.signal_store.body_receive_mode() == BodyReceiveMode::Ignore { return; } @@ -1863,7 +1866,7 @@ impl FetchTasklet { ) } - fn ignore_remaining_response_body(&mut self) { + fn ignore_remaining_response_body(&self) { bun_output::scoped_log!(FetchTasklet, "ignoreRemainingResponseBody"); // enabling streaming will make the http thread to drain into the main thread (aka stop buffering) // without a stream ref, response body or response instance alive it will just ignore the result @@ -1878,7 +1881,7 @@ impl FetchTasklet { { self.schedule_receive_resume(); } - if let Some(http_) = self.http.as_mut() { + if let Some(http_) = self.http.as_deref() { if !aborted { http_.enable_response_body_streaming(); } @@ -1924,7 +1927,7 @@ impl FetchTasklet { // Response is intrusively refcounted; bump for native_response. // SAFETY: `response` is the live heap allocation owned by JSC after // `make_maybe_pooled`; `ref_` bumps the intrusive refcount. - self.native_response = Some(Response::ref_(response)); + self.native_response.set(Some(Response::ref_(response))); // Response-owned listener so abort still errors the body after this tasklet detaches its own. if let Some(signal) = self.abort_signal() { // SAFETY: `response` is the live heap allocation owned by JSC. @@ -1951,8 +1954,8 @@ impl FetchTasklet { request_body_streaming_buffer: None, scheduled_response_buffer: MutableString::default(), response: jsc::Weak::default(), - native_response: None, - response_stream_source: None, + native_response: Cell::new(None), + response_stream_source: Cell::new(None), request_headers: fetch_options.headers, promise, concurrent_task: ConcurrentTask::default(), @@ -2338,7 +2341,7 @@ impl FetchTasklet { FetchTasklet::deref(this_ptr); } - fn abort_task(&mut self) { + fn abort_task(&self) { // Idempotent: reader.cancel() and an AbortSignal can both reach here for // the same fetch. Only the first abort enqueues a shutdown; a second // would append a redundant ShutdownMessage for an already-closing socket. @@ -2347,7 +2350,7 @@ impl FetchTasklet { } self.tracker.did_cancel(&self.global_this); - if let Some(http_) = self.http.as_mut() { + if let Some(http_) = self.http.as_deref() { http::http_thread().schedule_shutdown(http_); } } @@ -2655,7 +2658,7 @@ impl FetchTasklet { pub(crate) fn on_response_finalize(&mut self) { bun_output::scoped_log!(FetchTasklet, "onResponseFinalize"); let this = self; - if let Some(response) = this.native_response { + if let Some(response) = this.native_response.get() { // SAFETY: native_response is intrusively-ref'd by FetchTasklet; alive until unref. let body = unsafe { (*response).get_body_value() }; // Three scenarios: @@ -2667,7 +2670,8 @@ impl FetchTasklet { // 3. We never started buffering, in which case we should ignore the body. // // Inside a finalizer: decide from native state only. - if !matches!(body, BodyValue::Locked(_)) || this.response_stream_source.is_some() { + if !matches!(body, BodyValue::Locked(_)) || this.response_stream_source.get().is_some() + { // Scenario 1: the stream outlives its Response while something consumes it; // its own collection cancels the body (`SourceHandle::consumer_collected`). return; diff --git a/src/runtime/webcore/s3/client.rs b/src/runtime/webcore/s3/client.rs index 4e46b3067c3a..1505710dead5 100644 --- a/src/runtime/webcore/s3/client.rs +++ b/src/runtime/webcore/s3/client.rs @@ -1325,14 +1325,16 @@ fn download_stream( task_ptr } +/// The two fields `on_stream_cancelled` clears are cells: the stream's sink can cancel +/// the download from inside `on_data`, while `callback` is delivering. pub struct S3DownloadStreamWrapper { - pub readable_stream_ref: ReadableStreamStrong, + pub readable_stream_ref: JsCell, pub path: Box<[u8]>, pub global: GlobalRef, // JSC_BORROW /// Non-owning. The task frees itself on the main thread once `has_more == false`, /// which first drops this wrapper (clearing the stream's producer handle), so this /// pointer is never observed dangling from `on_stream_cancelled`. - pub task: *mut S3HttpDownloadStreamingTask, + pub task: Cell<*mut S3HttpDownloadStreamingTask>, } impl S3DownloadStreamWrapper { @@ -1355,7 +1357,7 @@ impl S3DownloadStreamWrapper { } }); - if let Some(readable) = self_.readable_stream_ref.get() { + if let Some(readable) = self_.readable_stream_ref.get().get() { // BACKREF: see `Source::bytes()` — payload live while the // readable stream is rooted. R-2: `&` — `on_data` re-enters JS. if let Some(bytes) = readable.ptr.bytes() { @@ -1387,18 +1389,17 @@ impl S3DownloadStreamWrapper { } } - pub(crate) fn on_stream_cancelled(&mut self) { - let self_ = self; + pub(crate) fn on_stream_cancelled(&self) { // Release the Strong ref so the ReadableStream can be GC'd. // The download may still be in progress, but the callback will // see readable_stream_ref.get() return null and skip data delivery. // When the download finishes (has_more == false), deinit() will // clean up the remaining resources. - self_.readable_stream_ref.deinit(); + self.readable_stream_ref.with_mut(|s| s.deinit()); // Abort the in-flight HTTP request so the HTTP thread delivers a final // callback with `has_more == false`, which frees the task and this wrapper. // Without this, a server that never sends the terminal chunk would leak both. - let task = core::mem::replace(&mut self_.task, core::ptr::null_mut()); + let task = self.task.replace(core::ptr::null_mut()); if !task.is_null() { // SAFETY: task is live until its own `on_response` frees it on this thread, // which has not happened yet (it would have dropped this wrapper first). @@ -1432,7 +1433,7 @@ impl Drop for S3DownloadStreamWrapper { fn drop(&mut self) { // Clear the ByteStream's producer handle before `readable_stream_ref` // drops so the stream never calls back into a freed wrapper. - if let Some(readable) = self.readable_stream_ref.get() { + if let Some(readable) = self.readable_stream_ref.get().get() { if let Some(bytes) = readable.ptr.bytes() { bytes .parent_const() @@ -1473,16 +1474,16 @@ pub(crate) fn readable_stream( let readable_value = reader_mut.to_readable_stream(global_this)?; let wrapper = S3DownloadStreamWrapper::new(S3DownloadStreamWrapper { - readable_stream_ref: ReadableStreamStrong::init( + readable_stream_ref: JsCell::new(ReadableStreamStrong::init( ReadableStream { ptr: ReadableStreamPtr::Bytes(&raw mut reader_mut.context), value: readable_value, }, global_this, - ), + )), path: Box::<[u8]>::from(path), global: global_static, - task: core::ptr::null_mut(), + task: Cell::new(core::ptr::null_mut()), }); reader_mut @@ -1508,7 +1509,7 @@ pub(crate) fn readable_stream( // SAFETY: on the success path `download_stream` only schedules work onto the HTTP // thread; the wrapper is freed via `opaque_callback` on this (main) thread, which // cannot run until we return to the event loop, so `wrapper` is still live here. - unsafe { (*wrapper).task = task }; + unsafe { (*wrapper).task.set(task) }; } Ok(readable_value) } diff --git a/src/runtime/webcore/streams.rs b/src/runtime/webcore/streams.rs index c0cf396330e1..277c4782f5f4 100644 --- a/src/runtime/webcore/streams.rs +++ b/src/runtime/webcore/streams.rs @@ -957,10 +957,11 @@ impl SourceHandle { SourceHandle::Subprocess(p) => p.on_close(err), // SAFETY: live backref; cleared before the pointee is freed. SourceHandle::ShellWritable(mut p) => unsafe { p.get_mut() }.on_close(err), - // SAFETY: live backref; cleared before the pointee is freed. - SourceHandle::FetchResponseBody(mut p) => unsafe { p.get_mut() }.on_stream_cancelled(), - // SAFETY: live backref; cleared before the pointee is freed. - SourceHandle::S3DownloadBody(mut p) => unsafe { p.get_mut() }.on_stream_cancelled(), + // Shared refs: both can be reached from inside the producer's own delivery + // (`ByteStream::detach_finished_sink` under `on_data`), so neither may form a + // second `&mut` to it. + SourceHandle::FetchResponseBody(p) => p.on_stream_cancelled(), + SourceHandle::S3DownloadBody(p) => p.on_stream_cancelled(), SourceHandle::ServerRequestBody(_) => {} SourceHandle::HTMLRewriter(p) => p.on_close(err), SourceHandle::TestingCancelOnDrain(_) => {} @@ -1016,8 +1017,7 @@ impl SourceHandle { /// sweep: arms must not run JS. pub fn consumer_collected(&mut self) { match *self { - // SAFETY: live backref; cleared before the pointee is freed. - SourceHandle::FetchResponseBody(mut p) => unsafe { p.get_mut() }.on_stream_cancelled(), + SourceHandle::FetchResponseBody(p) => p.on_stream_cancelled(), // Remaining variants do not act on this signal. SourceHandle::None | SourceHandle::JSController(_) From 004a60fa863c8f8501cc28b67571e18bad903d69 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 04:43:21 +0000 Subject: [PATCH 08/22] fetch: park an unread body stream instead of buffering it, and let GC end it An unobserved response body stream (res.body touched but not read, or a released reader) used to flip the transport to BufferAll, reading the whole body into memory. Keep receiving only until the stream holds 256 KiB nobody has taken, then leave the transport paused, release the event-loop ref, and stop rooting the stream's JS wrapper. A reader, buffered read or native sink that shows up later unparks it. If the parked stream is collected instead, the source tells the tasklet, which aborts the request. Bodies under the mark still complete unread, so their keep-alive connection is reused as before. Also cancel the upstream fetch when a Bun.serve response that was piping it natively loses its client. --- src/http/AsyncHTTP.rs | 3 +- src/jsc/Weak.rs | 3 +- src/runtime/server/RequestContext.rs | 19 +- src/runtime/webcore/ByteStream.rs | 25 +- src/runtime/webcore/ReadableStream.rs | 36 ++ src/runtime/webcore/fetch/FetchTasklet.rs | 335 +++++++++++------- src/runtime/webcore/streams.rs | 19 + test/js/web/fetch/fetch-backpressure.test.ts | 245 ++++++++++++- .../fetch/fetch-stream-cancel-leak.test.ts | 164 ++++++++- test/js/workerd/html-rewriter.test.js | 54 +++ 10 files changed, 765 insertions(+), 138 deletions(-) diff --git a/src/http/AsyncHTTP.rs b/src/http/AsyncHTTP.rs index 970c61bbe591..387b118b5904 100644 --- a/src/http/AsyncHTTP.rs +++ b/src/http/AsyncHTTP.rs @@ -280,7 +280,8 @@ impl<'a> AsyncHTTP<'a> { &MAX_SIMULTANEOUS_REQUESTS } - pub fn enable_response_body_streaming(&mut self) { + /// A store into the shared signal `Store`, not into `self`. + pub fn enable_response_body_streaming(&self) { self.signals.store( crate::signals::Field::ResponseBodyStreaming, true, diff --git a/src/jsc/Weak.rs b/src/jsc/Weak.rs index 7241e3742d62..33a84ec916e9 100644 --- a/src/jsc/Weak.rs +++ b/src/jsc/Weak.rs @@ -137,7 +137,8 @@ impl Weak { Some(result) } - pub fn clear(&mut self) { + /// Clears the C++ slot; `self` keeps the (now empty) handle, so a shared ref suffices. + pub fn clear(&self) { let Some(r#ref) = self.r#ref else { return; }; diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index 9177e79a8617..6797da4c5cd4 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -400,14 +400,13 @@ mod shim { .as_ref() .is_some_and(|s| matches!(s.data, crate::webcore::blob::store::Data::File(_))) } + /// The response is done with its natively piped body. A body still mid-stream is + /// cancelled: it stayed locked to this response, so nothing else can read it. #[inline] pub(super) fn byte_stream_unpipe(s: NonNull) { - // The lone caller has just `take()`n the pointer out of - // `self.byte_stream`; the allocation is kept alive by - // `response_body_readable_stream_ref` (BackRef invariant: pointee - // outlives this temporary). R-2: `unpipe_without_deref` takes `&self` - // (interior-mutable `JsCell`), so shared deref is sufficient. - bun_ptr::BackRef::from(s).unpipe_without_deref() + // The caller has just `take()`n `self.byte_stream` and still holds + // `response_body_readable_stream_ref`, which keeps the pointee alive. + bun_ptr::BackRef::from(s).detach_finished_sink() } } use crate::server::DevErrorPage; @@ -1396,6 +1395,14 @@ where return; } + // A natively piped body has nobody left to take it. The deref balances the ref + // `do_render_with_body` took for the pipe: `end_chunk`, which releases it otherwise, + // cannot run once the response is gone. + if let Some(stream) = this.byte_stream.take() { + shim::byte_stream_unpipe(stream); + this.deref(); + } + // if we can, free the request now. if this.is_dead_request() { this.finalize_without_deinit(); diff --git a/src/runtime/webcore/ByteStream.rs b/src/runtime/webcore/ByteStream.rs index cab5520dae9f..42408021646c 100644 --- a/src/runtime/webcore/ByteStream.rs +++ b/src/runtime/webcore/ByteStream.rs @@ -86,6 +86,9 @@ impl readable_stream::SourceContext for ByteStream { fn deinit_fn(&mut self) { Self::finalize(self) } + fn wrapper_finalized(&mut self) { + self.parent_const().producer.get().consumer_collected(); + } fn drain_internal_buffer(&mut self) -> Vec { Self::drain(self) } @@ -178,6 +181,22 @@ impl ByteStream { self.sink_paused.set(false); } + /// The sink is gone before the stream ended (its peer went away). The stream stays + /// locked to it, so nobody else can read the rest: close the producer too. + pub(crate) fn detach_finished_sink(&self) { + if self.has_received_last_chunk.get() { + self.unpipe_without_deref(); + } else { + self.cancel_from_sink(None); + } + } + + /// Bytes delivered that no consumer has taken yet. + #[inline] + pub fn buffered_len(&self) -> usize { + self.buffer.get().len() - self.offset.get() + } + /// Sink's drain ack: unpause, push buffered bytes, end if last chunk already arrived. pub fn resume(&self) { if !self.sink_paused.get() { @@ -265,8 +284,8 @@ impl ByteStream { Vec::::move_from_list(list) } - /// Called by native fast-paths after wiring `self.sink`. Restores - /// producer-side backpressure if it was already dropped (BufferAll). + /// Called by native fast-paths after wiring `self.sink`: a consumer now + /// waits for bytes, so a parked producer resumes. pub fn signal_consumer_attached(&self) { self.parent_const().producer.get().start(); } @@ -574,6 +593,8 @@ impl ByteStream { bun_jsc::mark_binding!(); debug_assert!(!buffer.is_empty()); debug_assert!(self.buffer_action.get().is_none()); + // A reader is back on a stream the producer may have parked unread. + self.signal_consumer_attached(); if !self.buffer.get().is_empty() { debug_assert!(self.value().is_empty()); // == .zero diff --git a/src/runtime/webcore/ReadableStream.rs b/src/runtime/webcore/ReadableStream.rs index 01cb35763bc9..97d29d716a3b 100644 --- a/src/runtime/webcore/ReadableStream.rs +++ b/src/runtime/webcore/ReadableStream.rs @@ -781,6 +781,10 @@ pub trait SourceContext: Sized { false } + /// The JS wrapper was collected while native refs remain. Runs inside a GC + /// sweep: no JS. `ByteStream` tells a parked producer nobody can read it now. + fn wrapper_finalized(&mut self) {} + /// `setRefUnrefFn` — default no-op. fn set_ref_unref(&mut self, _enable: bool) {} @@ -851,6 +855,10 @@ pub struct NewSource { /// `Finalized` so [`Self::on_js_close`] reads `None` instead of a /// dead-but-unswept cell. pub this_jsvalue: jsc::JsRef, + /// The producer holding a native ref has parked ([`Self::unroot_wrapper`]): + /// its ref keeps this allocation, not the wrapper, so an unread stream can + /// be collected. Cleared by [`Self::root_wrapper`]. + pub wrapper_unrooted: bool, /// R-2: written by context methods (`ByteStream::to_any_blob`, /// `ByteBlobLoader::to_any_blob`) through their parent accessor, so /// interior-mutable. @@ -869,6 +877,7 @@ impl Default for NewSource { producer: Cell::new(streams::SourceHandle::None), global_this: None, this_jsvalue: jsc::JsRef::empty(), + wrapper_unrooted: false, is_closed: Cell::new(false), } } @@ -1140,6 +1149,12 @@ impl NewSource { // `waiting_for_on_reader_done` I/O ref). Root the wrapper so // `on_js_close`, reached from `on_reader_done` off the event loop with // no JS frame on the stack, never reads a dead-but-unswept cell. + if !self.wrapper_unrooted { + self.upgrade_wrapper(); + } + } + + fn upgrade_wrapper(&mut self) { if let Some(global) = self.global_this.as_deref() { if self.this_jsvalue.is_not_empty() { self.this_jsvalue.upgrade(global); @@ -1147,6 +1162,22 @@ impl NewSource { } } + /// The producer keeps its native ref but stops rooting the wrapper: nothing + /// is reading, so the stream should be collectable. [`SourceContext::wrapper_finalized`] + /// tells the producer if that happens. + pub fn unroot_wrapper(&mut self) { + self.wrapper_unrooted = true; + self.this_jsvalue.downgrade(); + } + + /// Undo [`Self::unroot_wrapper`]: a consumer is reading again. + pub fn root_wrapper(&mut self) { + self.wrapper_unrooted = false; + if self.ref_count > 1 { + self.upgrade_wrapper(); + } + } + /// Release one reference. If the count hits zero, runs context teardown and /// **frees the allocation**. /// @@ -1440,6 +1471,11 @@ impl NewSource { let this = Box::into_raw(self); // SAFETY: `this` is live — just unwrapped from `Box`. unsafe { (*this).this_jsvalue.finalize() }; + // SAFETY: `this` is live; the JS-wrapper +1 (released last) keeps ref_count > 0 + // across whatever ref the producer drops in response. + if unsafe { (*this).ref_count } > 1 { + unsafe { (*this).context.wrapper_finalized() }; + } // SAFETY: `this` is live; the JS-wrapper ref below still pins the count. if unsafe { (*this).context.finalize_detach() } { // SAFETY: `this` is live; the JS-wrapper +1 (released below) keeps ref_count > 0. diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 3e63b29255bd..fed37b7f5f29 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -1,3 +1,4 @@ +use core::cell::Cell; use core::ffi::c_void; use core::ptr::NonNull; use core::sync::atomic::{AtomicBool, Ordering}; @@ -18,7 +19,7 @@ use bun_http::{ use bun_io::KeepAlive; use bun_jsc::debugger::AsyncTaskTracker; use bun_jsc::{ - self as jsc, GlobalRef, JSGlobalObject, JSValue, JsResult, StringJsc, StrongOptional, + self as jsc, GlobalRef, JSGlobalObject, JSValue, JsCell, JsResult, StringJsc, StrongOptional, }; use bun_sys::FdExt; use bun_threading::Mutex; @@ -38,6 +39,11 @@ use crate::webcore::{AbortSignal, DrainResult, FetchHeaders, InternalBlob, Respo // ConcurrentTask callbacks at the tier-3 layer. type ElJsResult = bun_event_loop::JsResult; +/// How much of a body nothing is reading is taken off the socket before the transport is left +/// paused (`FetchTasklet::after_body_chunk_delivered`). Below this an unread body still +/// completes, which frees the connection for reuse; above it memory stays bounded. +const UNOBSERVED_BODY_HIGH_WATER_MARK: usize = 256 * 1024; + use boringssl::c::{X509_free, d2i_X509}; // ConcurrentTask::from() needs `Taskable`; tag is declared in bun_event_loop @@ -111,18 +117,24 @@ pub struct FetchTasklet { /// response weak ref we need this to track the response JS lifetime pub(crate) response: jsc::Weak, /// native response ref if we still need it when JS is discarted - // Response is intrusively refcounted; modeled as a raw ptr. - pub(crate) native_response: Option<*mut Response>, - /// stream strong ref if any is available - pub(crate) readable_stream_ref: ReadableStreamStrong, - /// A counted ref on that stream's ByteStream source for as long as this tasklet is its - /// `producer`, so unhooking goes through native memory we keep alive rather than through - /// the JS wrappers, which the VM's last sweep destroys in no particular order. - pub(crate) response_stream_source: Option>, + // Response is intrusively refcounted; modeled as a raw ptr. `Cell`: released from + // `on_body_stream_collected`, which only has a shared ref. + pub(crate) native_response: Cell>, + /// A counted ref on the response body stream's ByteStream source for as long as this tasklet is its + /// `producer`, so delivery and unhooking go through native memory we keep alive rather than + /// through the JS wrappers, which the VM's last sweep destroys in no particular order. The + /// ref also roots the stream's JS wrapper, except while the body is parked + /// (`body_stream_parked`). + pub(crate) response_stream_source: Cell>>, + /// Nothing reads the body stream and it holds `UNOBSERVED_BODY_HIGH_WATER_MARK` bytes: the + /// transport is left paused, the loop is released, and the stream is left collectable + /// (`on_body_stream_collected`). A consumer that comes back unparks (`on_stream_drained`). + pub(crate) body_stream_parked: Cell, pub(crate) request_headers: Headers, pub(crate) promise: jsc::JSPromiseStrong, pub(crate) concurrent_task: ConcurrentTask, - pub poll_ref: KeepAlive, + /// `JsCell`: the ByteStream's drain signal reaches `on_stream_drained` through a shared ref. + pub poll_ref: JsCell, /// For Http Client requests /// when Content-Length is provided this represents the whole size of the request /// If chunked encoded this will represent the total received size (ignoring the chunk headers) @@ -154,8 +166,6 @@ pub struct FetchTasklet { /// Set by `on_start_buffering_callback` (JS thread) and read by /// `callback()` (HTTP thread, under `mutex`): the body is being /// accumulated in `scheduled_response_buffer` for a buffered consumer. - /// Distinguishes that path from `drop_backpressure_if_unobserved`, which - /// also sets `BufferAll` but still delivers per chunk. pub(crate) is_buffering_body: AtomicBool, pub(crate) is_waiting_abort: bool, pub(crate) is_waiting_request_stream_start: bool, @@ -492,7 +502,6 @@ impl FetchTasklet { } self.clear_stream_handlers(); - self.readable_stream_ref.deinit(); self.scheduled_response_buffer = MutableString::default(); // Always detach request_body regardless of type. @@ -585,7 +594,7 @@ impl FetchTasklet { fn get_current_response(&self) -> Option<*mut Response> { // we need a body to resolve the promise when buffering - if let Some(response) = self.native_response { + if let Some(response) = self.native_response.get() { return Some(response); } @@ -776,14 +785,13 @@ impl FetchTasklet { let mut err = scopeguard::guard(self.on_reject(), |mut e| e.reset()); let mut js_err = JSValue::ZERO; // if we are streaming update with error - if let Some(readable) = self.readable_stream_ref.get() { - if let Some(bytes) = readable.ptr.bytes() { - js_err = err.to_js(&global_this); - js_err.ensure_still_alive(); - bytes.on_data(StreamResult::Err(StreamError::JSValue( - bun_jsc::strong::Optional::create(js_err, &global_this), - ))); - } + if let Some(source) = self.take_response_stream_source() { + js_err = err.to_js(&global_this); + js_err.ensure_still_alive(); + Self::response_bytes(source).on_data(StreamResult::Err(StreamError::JSValue( + bun_jsc::strong::Optional::create(js_err, &global_this), + ))); + Self::release_response_stream_source(source); } // A failure result is terminal (`to_result` forces `has_more = // false` once `fail` is set), so everything pending must settle @@ -812,26 +820,35 @@ impl FetchTasklet { return Ok(()); } - if let Some(readable) = self.readable_stream_ref.get() { - bun_output::scoped_log!(FetchTasklet, "onBodyReceived readable_stream_ref"); - if let Some(bytes) = readable.ptr.bytes() { + // body can be marked as used but we still need to pipe the data + if !self.result.has_more { + // Unhook before the final delivery so it cannot signal a producer that is done; + // release after it so the bytes land in memory we still pin. + if let Some(source) = self.take_response_stream_source() { + bun_output::scoped_log!(FetchTasklet, "onBodyReceived response_stream_source done"); + let bytes = Self::response_bytes(source); bytes.size_hint.set(self.get_size_hint()); - // body can be marked as used but we still need to pipe the data - if self.result.has_more { - let chunk = self.scheduled_response_buffer.list.as_slice(); - bytes.on_data(Self::temporary_chunk(chunk, false)); - self.drop_backpressure_if_unobserved(&readable, &bytes); - } else { - self.clear_stream_handlers(); - let prev = core::mem::take(&mut self.readable_stream_ref); - buffer_reset.set(false); - - let chunk = self.scheduled_response_buffer.list.as_slice(); - bytes.on_data(Self::temporary_chunk(chunk, true)); - drop(prev); - } + buffer_reset.set(false); + let chunk = self.scheduled_response_buffer.list.as_slice(); + bytes.on_data(Self::temporary_chunk(chunk, true)); + Self::release_response_stream_source(source); return Ok(()); } + } else if let Some(source) = self.response_stream_source.get() { + bun_output::scoped_log!(FetchTasklet, "onBodyReceived response_stream_source"); + // Pin across the delivery: while parked the wrapper is not rooted, and a consumer + // inside `on_data` can cancel us (`on_stream_cancelled` drops the producer ref). + // SAFETY: live through the producer ref; no borrow of the source exists yet. + unsafe { (*source.as_ptr()).increment_count() }; + let bytes = Self::response_bytes(source); + bytes.size_hint.set(self.get_size_hint()); + let chunk = self.scheduled_response_buffer.list.as_slice(); + bytes.on_data(Self::temporary_chunk(chunk, false)); + if self.response_stream_source.get().is_some() { + self.after_body_chunk_delivered(&bytes); + } + Self::release_response_stream_source(source); + return Ok(()); } if let Some(response) = self.current_response_mut() { @@ -848,7 +865,6 @@ impl FetchTasklet { if self.result.has_more { bytes.on_data(Self::temporary_chunk(chunk, false)); - self.drop_backpressure_if_unobserved(&readable, &bytes); } else { readable.value.ensure_still_alive(); response.detach_readable_stream(&global_this); @@ -949,8 +965,8 @@ impl FetchTasklet { // it again — cancel the sink so the JS side releases the reader; // the pump-promise settlement drops the `startRequestStream` ref. this.cancel_request_body_sink(JSValue::UNDEFINED); - let mut poll_ref = core::mem::take(&mut this.poll_ref); - poll_ref.unref(bun_io::js_vm_ctx()); + this.poll_ref + .with_mut(|poll_ref| poll_ref.unref(bun_io::js_vm_ctx())); // SAFETY: `this` is the live heap tasklet; we hold a ref. FetchTasklet::deref(std::ptr::from_mut(this)); } @@ -1598,19 +1614,19 @@ impl FetchTasklet { fn on_readable_stream_available( ctx: NonNull, - global_this: &JSGlobalObject, + _global_this: &JSGlobalObject, readable: ReadableStream, ) { let this = Self::from_ctx(ctx); this.clear_stream_handlers(); - this.readable_stream_ref = ReadableStreamStrong::init(readable, global_this); if let crate::webcore::readable_stream::Source::Bytes(bytes) = readable.ptr { - // SAFETY: the stream (held Strong above) owns a live ByteStream embedded in - // its Source; JS thread. + // SAFETY: the caller holds the stream, which owns the live ByteStream embedded in + // its Source; the ref taken here keeps the Source (and, until parked, the stream) + // alive past this call. JS thread. unsafe { let source = crate::webcore::readable_stream::NewSource::from_context_ptr(bytes); (*source).increment_count(); - this.response_stream_source = NonNull::new(source); + this.response_stream_source.set(NonNull::new(source)); } } // A ByteStream now drains scheduled_response_buffer per chunk; undo any @@ -1625,8 +1641,10 @@ impl FetchTasklet { } // A body consumer is attaching; keep the process alive until the - // body finishes (undone in `on_progress_update` when `is_done`). - this.poll_ref.ref_(bun_io::js_vm_ctx()); + // body finishes (undone in `on_progress_update` when `is_done`), or + // until the stream parks unread (`park_body_stream`). + this.poll_ref + .with_mut(|poll_ref| poll_ref.ref_(bun_io::js_vm_ctx())); // The bytes already in `scheduled_response_buffer` are handed to the // new stream below. That is the drain `Paused` was waiting for, so @@ -1679,17 +1697,38 @@ impl FetchTasklet { } } - /// Unhook this tasklet as the response ByteStream's producer before releasing - /// `readable_stream_ref` — the stream can outlive us in JS — and drop the ref that - /// kept the source's memory ours to write to. Touches no JS cell. - fn clear_stream_handlers(&mut self) { - if let Some(source) = self.response_stream_source.take() { - // SAFETY: counted ref taken in `on_readable_stream_available`; live until - // the `decrement_count` below, which may free it. - unsafe { - (*source.as_ptr()).producer.set(SourceHandle::None); - crate::webcore::byte_stream::Source::decrement_count(source.as_ptr()); - } + /// The ByteStream behind a source one of our refs still pins. + fn response_bytes( + source: NonNull, + ) -> bun_ptr::BackRef { + // SAFETY: the source is live (caller's ref); ByteStream is `&self`-only (R-2). + bun_ptr::BackRef::new(unsafe { &(*source.as_ptr()).context }) + } + + /// Stop being the producer. The source stays pinned by our ref, so the caller can still + /// deliver a terminal chunk before `release_response_stream_source`. Touches no JS cell. + fn take_response_stream_source( + &self, + ) -> Option> { + let source = self.response_stream_source.take()?; + self.body_stream_parked.set(false); + // SAFETY: still pinned by the ref from `on_readable_stream_available`. + unsafe { (*source.as_ptr()).producer.set(SourceHandle::None) }; + Some(source) + } + + /// Drop one of our refs (the producer's, or a delivery's pin). Can free the source: the + /// JS wrapper may be gone already. + fn release_response_stream_source(source: NonNull) { + // SAFETY: balances a ref this tasklet took; `source` is not used after this. + unsafe { crate::webcore::byte_stream::Source::decrement_count(source.as_ptr()) }; + } + + /// Unhook from the response ByteStream (the stream can outlive us in JS) and release + /// it. Touches no JS cell. + fn clear_stream_handlers(&self) { + if let Some(source) = self.take_response_stream_source() { + Self::release_response_stream_source(source); } } @@ -1703,7 +1742,25 @@ impl FetchTasklet { self.ignore_remaining_response_body(); } + /// `SourceHandle::consumer_collected`: the parked stream's wrapper was swept, so nothing + /// can read the rest of the body. Inside a GC sweep: native state only, like + /// `on_response_finalize`. The abort comes back through the HTTP callback and the usual + /// teardown runs there. + pub(crate) fn on_body_stream_collected(&self) { + bun_output::scoped_log!(FetchTasklet, "onBodyStreamCollected"); + if self.signal_store.body_receive_mode() == BodyReceiveMode::Ignore { + return; + } + self.abort_transport(); + self.ignore_remaining_response_body(); + } + pub(crate) fn on_stream_drained(&self) { + self.unpark_body_stream(); + self.resume_receive(); + } + + fn resume_receive(&self) { if self .signal_store .try_transition_receive_mode(BodyReceiveMode::Paused, BodyReceiveMode::AutoPause) @@ -1712,19 +1769,67 @@ impl FetchTasklet { } } - /// A native sink attached after `drop_backpressure_if_unobserved` had - /// already flipped to `BufferAll`. Move to `Paused`: the socket is still - /// reading, so the next `maybe_pause_receive` stops it; the sink's drain - /// then resumes via `on_stream_drained`. + /// A native sink was wired to the stream: something waits for bytes again. pub(crate) fn on_consumer_attached(&self) { - let _ = self - .signal_store - .try_transition_receive_mode(BodyReceiveMode::BufferAll, BodyReceiveMode::Paused); + self.unpark_body_stream(); + } + + /// After a non-terminal delivery. A consumer waiting on the stream resumes the transport + /// itself as it drains. Bytes nothing took stay in the stream's buffer: keep receiving until + /// it holds `UNOBSERVED_BODY_HIGH_WATER_MARK` (a small body still completes, which frees the + /// connection), then park. + fn after_body_chunk_delivered(&self, bytes: &crate::webcore::ByteStream) { + bun_output::scoped_log!( + FetchTasklet, + "afterBodyChunkDelivered sink={} action={} pending={} buffered={}", + bytes.sink.get().is_some(), + bytes.buffer_action.get().is_some(), + bytes.pending.get().state == crate::webcore::streams::PendingState::Pending, + bytes.buffered_len() + ); + if bytes.sink.get().is_some() + || bytes.buffer_action.get().is_some() + || bytes.pending.get().state == crate::webcore::streams::PendingState::Pending + { + return; + } + if bytes.buffered_len() < UNOBSERVED_BODY_HIGH_WATER_MARK { + self.resume_receive(); + } else { + self.park_body_stream(); + } + } + + fn park_body_stream(&self) { + if self.body_stream_parked.replace(true) { + return; + } + bun_output::scoped_log!(FetchTasklet, "parkBodyStream"); + self.poll_ref + .with_mut(|poll_ref| poll_ref.unref(bun_io::js_vm_ctx())); + if let Some(source) = self.response_stream_source.get() { + // SAFETY: live through the producer ref; no borrow of the source is held here. + unsafe { (*source.as_ptr()).unroot_wrapper() }; + } + } + + fn unpark_body_stream(&self) { + if !self.body_stream_parked.replace(false) { + return; + } + bun_output::scoped_log!(FetchTasklet, "unparkBodyStream"); + self.poll_ref + .with_mut(|poll_ref| poll_ref.ref_(bun_io::js_vm_ctx())); + if let Some(source) = self.response_stream_source.get() { + // SAFETY: as in `park_body_stream`; reached from a consumer holding the stream. + unsafe { (*source.as_ptr()).root_wrapper() }; + } } fn on_start_buffering_callback(ctx: NonNull) { let this = Self::from_ctx(ctx); - this.poll_ref.ref_(bun_io::js_vm_ctx()); + this.poll_ref + .with_mut(|poll_ref| poll_ref.ref_(bun_io::js_vm_ctx())); this.is_buffering_body.store(true, Ordering::Release); if this .signal_store @@ -1740,31 +1845,6 @@ impl FetchTasklet { } } - /// After a body chunk has been delivered to the ByteStream with - /// `has_more == true`, flip to `BufferAll` if the stream has no lock, - /// no pipe, and no buffer action. An unlocked stream has no reader, so - /// there is nothing to apply backpressure against; let the body - /// complete so this tasklet (and the source it roots) can be freed. - fn drop_backpressure_if_unobserved( - &self, - readable: &ReadableStream, - bytes: &crate::webcore::ByteStream, - ) { - if self.signal_store.body_receive_mode() == BodyReceiveMode::BufferAll { - return; - } - if bytes.sink.get().is_some() - || bytes.buffer_action.get().is_some() - || bytes.pending.get().state == crate::webcore::streams::PendingState::Pending - || readable.is_locked(&self.global_this) - { - return; - } - self.signal_store - .set_receive_mode_terminal(BodyReceiveMode::BufferAll); - self.schedule_receive_resume(); - } - fn to_body_value(&mut self) -> BodyValue { if let Some(err) = self.get_abort_error() { return BodyValue::Error(err); @@ -1837,7 +1917,7 @@ impl FetchTasklet { ) } - fn ignore_remaining_response_body(&mut self) { + fn ignore_remaining_response_body(&self) { bun_output::scoped_log!(FetchTasklet, "ignoreRemainingResponseBody"); // enabling streaming will make the http thread to drain into the main thread (aka stop buffering) // without a stream ref, response body or response instance alive it will just ignore the result @@ -1852,19 +1932,18 @@ impl FetchTasklet { { self.schedule_receive_resume(); } - if let Some(http_) = self.http.as_mut() { + if let Some(http_) = self.http.as_deref() { if !aborted { http_.enable_response_body_streaming(); } } // we should not keep the process alive if we are ignoring the body - self.poll_ref.unref(bun_io::js_vm_ctx()); - // Also fine from `on_response_finalize` (a JSC Weak finalizer inside - // `WeakBlock::sweep`): unhooking touches no JS cell. The - // request-body sink is left for `clear_sink()` in `deinit()` (an event-loop - // task, outside sweep) to detach. + self.poll_ref + .with_mut(|poll_ref| poll_ref.unref(bun_io::js_vm_ctx())); + // Also fine from GC sweeps (`on_response_finalize`, `on_body_stream_collected`): + // unhooking touches no JS cell. The request-body sink is left for `clear_sink()` in + // `deinit()` (an event-loop task, outside sweep) to detach. self.clear_stream_handlers(); - self.readable_stream_ref.deinit(); self.response.clear(); if let Some(response) = self.native_response.take() { @@ -1881,7 +1960,8 @@ impl FetchTasklet { // consumer hooks (`on_start_streaming_http_response_body_callback`, // `on_start_buffering_callback`) re-ref if the caller reads the body. if self.is_waiting_body { - self.poll_ref.unref(bun_io::js_vm_ctx()); + self.poll_ref + .with_mut(|poll_ref| poll_ref.unref(bun_io::js_vm_ctx())); } // SAFETY: response is a freshly allocated Response; makeMaybePooled takes ownership semantics on the JS side let global_this = self.global_this; @@ -1897,7 +1977,7 @@ impl FetchTasklet { // Response is intrusively refcounted; bump for native_response. // SAFETY: `response` is the live heap allocation owned by JSC after // `make_maybe_pooled`; `ref_` bumps the intrusive refcount. - self.native_response = Some(Response::ref_(response)); + self.native_response.set(Some(Response::ref_(response))); // Response-owned listener so abort still errors the body after this tasklet detaches its own. if let Some(signal) = self.abort_signal() { // SAFETY: `response` is the live heap allocation owned by JSC. @@ -1924,13 +2004,13 @@ impl FetchTasklet { request_body_streaming_buffer: None, scheduled_response_buffer: MutableString::default(), response: jsc::Weak::default(), - native_response: None, - readable_stream_ref: ReadableStreamStrong::default(), - response_stream_source: None, + native_response: Cell::new(None), + response_stream_source: Cell::new(None), + body_stream_parked: Cell::new(false), request_headers: fetch_options.headers, promise, concurrent_task: ConcurrentTask::default(), - poll_ref: KeepAlive::default(), + poll_ref: JsCell::new(KeepAlive::default()), body_size: http::BodySize::Unknown, url_proxy_buffer: fetch_options.url_proxy_buffer, signal: fetch_options.signal, @@ -2313,17 +2393,22 @@ impl FetchTasklet { } fn abort_task(&mut self) { - // Idempotent: reader.cancel() and an AbortSignal can both reach here for - // the same fetch. Only the first abort enqueues a shutdown; a second - // would append a redundant ShutdownMessage for an already-closing socket. - if self.signal_store.aborted.swap(true, Ordering::Relaxed) { - return; + if self.abort_transport() { + self.tracker.did_cancel(&self.global_this); } - self.tracker.did_cancel(&self.global_this); + } - if let Some(http_) = self.http.as_mut() { + /// Idempotent: reader.cancel(), an AbortSignal and a collected body stream can all + /// reach here for the same fetch. Only the first abort enqueues a shutdown; a second + /// would append a redundant ShutdownMessage for an already-closing socket. No JS. + fn abort_transport(&self) -> bool { + if self.signal_store.aborted.swap(true, Ordering::Relaxed) { + return false; + } + if let Some(http_) = self.http.as_deref() { http::http_thread().schedule_shutdown(http_); } + true } /// Cancel an in-flight request-body sink: stores the abort reason, aborts @@ -2378,7 +2463,9 @@ impl FetchTasklet { let node_ref = Self::from_raw_mut(node); let mut batch = bun_threading::thread_pool::Batch::default(); node_ref.http.as_mut().unwrap().schedule(&mut batch); - node_ref.poll_ref.ref_(bun_io::js_vm_ctx()); + node_ref + .poll_ref + .with_mut(|poll_ref| poll_ref.ref_(bun_io::js_vm_ctx())); // increment ref so we can keep it alive until the http client is done node_ref.ref_(); @@ -2488,9 +2575,7 @@ impl FetchTasklet { // Grow to Content-Length once so the per-packet append below // doesn't leave the ~2x doubling over-capacity that the // ArrayBuffer would adopt. Gated on `is_buffering_body` - // (set by `on_start_buffering_callback`), not the raw - // `BufferAll` mode: `drop_backpressure_if_unobserved` also - // sets `BufferAll` while still draining per chunk. + // (set by `on_start_buffering_callback`). if task_ref.is_buffering_body.load(Ordering::Acquire) { if let http::BodySize::ContentLength(n) = task_ref.body_size { if n > scheduled.list.capacity() { @@ -2628,7 +2713,7 @@ impl FetchTasklet { pub(crate) fn on_response_finalize(&mut self) { bun_output::scoped_log!(FetchTasklet, "onResponseFinalize"); let this = self; - if let Some(response) = this.native_response { + if let Some(response) = this.native_response.get() { // SAFETY: native_response is intrusively-ref'd by FetchTasklet; alive until unref. let body = unsafe { (*response).get_body_value() }; // Three scenarios: @@ -2639,12 +2724,12 @@ impl FetchTasklet { // 2b. if we have a promise, we should keep loading the body. // 3. We never started buffering, in which case we should ignore the body. // - // Note: We cannot call .get() on the ReadableStreamRef. This is called inside a finalizer. - if !matches!(body, BodyValue::Locked(_)) || this.readable_stream_ref.has() { - // Scenario 1 or 3. A paused transport in Scenario 1 is - // unstuck by `drop_backpressure_if_unobserved` once the next - // already-scheduled chunk reaches `on_body_received` and - // finds the stream unlocked. + // Inside a finalizer: decide from native state only. + if !matches!(body, BodyValue::Locked(_)) || this.response_stream_source.get().is_some() + { + // Scenario 1 or 3. In scenario 1 the stream can outlive its Response; if + // nothing reads it, it parks and its own collection ends the body + // (`on_body_stream_collected`). return; } diff --git a/src/runtime/webcore/streams.rs b/src/runtime/webcore/streams.rs index 6eb8eb9dddfd..11bb90261872 100644 --- a/src/runtime/webcore/streams.rs +++ b/src/runtime/webcore/streams.rs @@ -994,6 +994,25 @@ impl SourceHandle { } } + /// The source's JS wrapper was collected while this producer still held the + /// source: nothing can read what it delivers from now on. Called from a GC + /// sweep: arms must not run JS. + pub fn consumer_collected(self) { + match self { + SourceHandle::FetchResponseBody(p) => p.on_body_stream_collected(), + SourceHandle::None + | SourceHandle::JSController(_) + | SourceHandle::ServerRequestBody(_) + | SourceHandle::ByteStream(_) + | SourceHandle::FileReader(_) + | SourceHandle::Subprocess(_) + | SourceHandle::ShellWritable(_) + | SourceHandle::S3DownloadBody(_) + | SourceHandle::HTMLRewriter(_) + | SourceHandle::TestingCancelOnDrain(_) => {} + } + } + pub fn start(&mut self) { match *self { SourceHandle::FetchResponseBody(p) => p.on_start(), diff --git a/test/js/web/fetch/fetch-backpressure.test.ts b/test/js/web/fetch/fetch-backpressure.test.ts index b9dab7131b6f..e1c46c95580b 100644 --- a/test/js/web/fetch/fetch-backpressure.test.ts +++ b/test/js/web/fetch/fetch-backpressure.test.ts @@ -1,7 +1,8 @@ // Receive-side backpressure: a stalled `res.body.getReader()` must stop the // HTTP thread from buffering the entire response in memory. import { describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, isASAN, isDebug, isWindows, tls } from "harness"; +import { bunEnv, bunExe, isASAN, isDebug, isWindows, tempDir, tls } from "harness"; +import { join } from "node:path"; import { randomBytes } from "node:crypto"; import { once } from "node:events"; import { createServer } from "node:http"; @@ -487,3 +488,245 @@ describe.concurrent("fetch() receive backpressure — streaming consumer shapes" } }); }); + +// A body stream that exists but that nothing reads: `res.body` was touched, or its reader +// was released. Before, the first chunk delivered to such a stream switched the fetch over +// to buffering the rest of the body in memory, with no bound, and kept the process alive +// while it did. The transport has to stay paused instead (the bytes have nowhere to go), +// the stream has to stay readable, and the paused body must not hold an idle process. + +const BIG = 16384; // 1 GiB, as in the "server stops writing" tests above +const BODY = BIG * CHUNK; + +// Writes the body as fast as the socket takes it; `sent()` is how far it got. A paused +// client stops it at the socket buffers (up to a few hundred MiB on some hosts), a client +// that drains lets it write all of BODY. +async function serveUntilBlocked() { + let sent = 0; + let closed = 0; + const payload = Buffer.alloc(CHUNK, 65); + const srv = createServer((_req, res) => { + res.on("error", () => {}); + res.on("close", () => closed++); + res.flushHeaders(); + let i = 0; + const push = () => { + while (i < BIG && !res.destroyed) { + i++; + sent += CHUNK; + if (!res.write(payload)) return void res.once("drain", push); + } + if (!res.destroyed) res.end(); + }; + push(); + }); + srv.listen(0); + await once(srv, "listening"); + const { port } = srv.address() as import("node:net").AddressInfo; + return { + url: `http://127.0.0.1:${port}/`, + sent: () => sent, + // Resolves with `sent()` once it has not moved for 100ms. + async settled() { + let last = -1; + for (let stable = 0; stable < 5; ) { + await Bun.sleep(20); + stable = sent === last ? stable + 1 : 0; + last = sent; + } + return sent; + }, + async untilClosed() { + while (closed === 0) await Bun.sleep(5); + }, + [Symbol.asyncDispose]: () => { + srv.closeAllConnections(); + return new Promise(r => srv.close(() => r(undefined))); + }, + }; +} + +// Sends the headers and one chunk; the test writes the rest through `response`. +async function serveByHand() { + let respond!: (res: import("node:http").ServerResponse) => void; + const response = new Promise(resolve => (respond = resolve)); + const srv = createServer((_req, res) => { + res.on("error", () => {}); + res.flushHeaders(); + res.write(Buffer.alloc(CHUNK, 65)); + respond(res); + }); + srv.listen(0); + await once(srv, "listening"); + const { port } = srv.address() as import("node:net").AddressInfo; + return { + url: `http://127.0.0.1:${port}/`, + response, + [Symbol.asyncDispose]: () => { + srv.closeAllConnections(); + return new Promise(r => srv.close(() => r(undefined))); + }, + }; +} + +// Sequential on purpose: each test watches one server's write progress, and a sibling +// test draining a body on the same HTTP thread would blur that signal. +describe("fetch() receive backpressure — body stream nothing is reading", () => { + const shapes: [string, (res: Response) => Promise>][] = [ + ["res.body read by nothing", async res => res.body!], + [ + "one read(), then releaseLock()", + async res => { + const reader = res.body!.getReader(); + await reader.read(); + reader.releaseLock(); + return res.body!; + }, + ], + ]; + + for (const [name, shape] of shapes) { + test(`${name}: the server blocks, and a later reader resumes the body`, async () => { + await using server = await serveUntilBlocked(); + const body = await shape(await fetch(server.url)); + + expect(await server.settled()).toBeLessThan(BODY); + + const reader = body.getReader(); + let got = 0; + while (got < 16 * CHUNK) got += (await reader.read()).value!.byteLength; + + await reader.cancel(); + await server.untilClosed(); + }, 60_000); + } + + // The other ways back to a parked body: a buffered read and a native sink both have to + // pick the transport up again and see the whole body. + for (const [name, drain] of [ + ["res.text()", (res: Response) => res.text().then(t => t.length)], + ["Bun.write(file, res)", (res: Response, dir: string) => Bun.write(join(dir, "out"), res)], + ] as const) { + test(`res.body read by nothing, then ${name} drains the whole body`, async () => { + await using server = await serve("h1"); + using dir = tempDir("fetch-parked-body", {}); + const res = await fetch(server.url); + void res.body; + // Parked: the client stops taking bytes well before the end (16 MiB body, 256 KiB mark). + let last = -1; + for (let stable = 0; stable < 3; ) { + await Bun.sleep(20); + stable = server.sent() === last ? stable + 1 : 0; + last = server.sent(); + } + expect(server.sent()).toBeLessThan(TOTAL); + expect(await drain(res, String(dir))).toBe(TOTAL); + }); + } +}); + +describe.concurrent("fetch() receive backpressure — a body nothing waits for does not hold the process", () => { + // The client keeps the unread body reachable and has nothing else to do: it has to exit + // on its own. Before, it stayed alive draining the body into memory, or, when the pause + // won the race at start, stayed alive holding the paused body. + const idleClients = [ + ["the Response, body touched", /* js */ `globalThis.keep = res; void res.body;`], + [ + "the stream, reader released", + /* js */ `const reader = res.body.getReader(); await reader.read(); reader.releaseLock(); globalThis.keep = res.body;`, + ], + ["an idle reader", /* js */ `const reader = res.body.getReader(); await reader.read(); globalThis.keep = reader;`], + ] as const; + + for (const [holding, script] of idleClients) { + test(`a process holding ${holding} exits on its own`, async () => { + await using server = await serveUntilBlocked(); + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", `const res = await fetch(${JSON.stringify(server.url)}); ${script}`], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const stderr = proc.stderr.text(); + const exited = proc.exited.then(exitCode => ({ exitCode })); + const deadline = performance.now() + 10_000; + let outcome: { exitCode: number } | { stillAlive: true; serverSentMiB: number } | undefined; + while (outcome === undefined) { + outcome = await Promise.race([exited, Bun.sleep(20).then(() => undefined)]); + // Took the whole body: it is draining. Took little and is still around: it is + // holding the paused body. Neither exited. + if (outcome === undefined && (server.sent() >= BODY || performance.now() >= deadline)) { + outcome = { stillAlive: true, serverSentMiB: server.sent() >> 20 }; + proc.kill(); + } + } + expect({ outcome, stderr: await stderr }).toEqual({ outcome: { exitCode: 0 }, stderr: "" }); + }, 20_000); + } + + // The other half of the rule: a consumer that does wait for bytes keeps the process + // alive, including one that picks the body up again after a release (the chunk that + // arrives while the body is released is what lets go of the loop). + const waitingClients = [ + [ + "read() pending on a reader", + /* js */ ` + const reader = res.body.getReader(); + let total = (await reader.read()).value.byteLength; + console.log("waiting"); + for (let r; !(r = await reader.read()).done; ) total += r.value.byteLength; + console.log("total " + total); + `, + 2, + ], + [ + "read() pending on a reader acquired after a release", + /* js */ ` + let reader = res.body.getReader(); + let total = (await reader.read()).value.byteLength; + reader.releaseLock(); + console.log("released"); + // Lets the chunk the test writes on "released" arrive while nothing reads the body. + // A slow machine only narrows what this exercises; the assertions do not depend on it. + await Bun.sleep(50); + reader = res.body.getReader(); + const next = reader.read(); + console.log("waiting"); + for (let r = await next; !r.done; r = await reader.read()) total += r.value.byteLength; + console.log("total " + total); + `, + 3, + ], + ] as const; + + for (const [name, script, chunks] of waitingClients) { + test(`a process with a ${name} stays alive until the body ends`, async () => { + await using server = await serveByHand(); + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", `const res = await fetch(${JSON.stringify(server.url)}); ${script}`], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const stderr = proc.stderr.text(); + const upstream = await server.response; + const lines: string[] = []; + let pending = ""; + for await (const text of proc.stdout.pipeThrough(new TextDecoderStream())) { + pending += text; + for (let nl; (nl = pending.indexOf("\n")) !== -1; ) { + const line = pending.slice(0, nl); + pending = pending.slice(nl + 1); + lines.push(line); + if (line === "released") upstream.write(Buffer.alloc(CHUNK, 66)); + if (line === "waiting") upstream.end(Buffer.alloc(CHUNK, 67)); + } + } + expect({ lines: lines.slice(-2), stderr: await stderr, exitCode: await proc.exited }).toEqual({ + lines: ["waiting", `total ${chunks * CHUNK}`], + stderr: "", + exitCode: 0, + }); + }); + } +}); diff --git a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts index 9d84531c193a..0d2b861a05e5 100644 --- a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts +++ b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts @@ -1,6 +1,6 @@ import { heapStats } from "bun:jsc"; -import { expect, test } from "bun:test"; -import { bunEnv, bunExe } from "harness"; +import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, isASAN, isDebug } from "harness"; // Test that ReadableStream objects from cancelled fetch responses are properly GC'd. // @@ -201,3 +201,163 @@ test("response.body.cancel() on a never-read body aborts the underlying fetch", exitCode: 0, }); }); + +// Nothing holds the body any more, so it has to go away: once the unread bytes reach the +// client's high-water mark the fetch parks the stream (stops rooting it), the stream is +// collected, and the fetch behind it is aborted, in whatever state it was dropped. Before, the +// fetch rooted its own stream until the body ended and buffered the rest of it, so against a +// body that does not end, the stream, the connection, and the buffered bytes all lived until +// the process exited. +describe("an abandoned fetch body stream is collected and its fetch is aborted", () => { + const shapes: [string, (res: Response) => Promise][] = [ + ["res.body touched", async res => res.body], + [ + "one read(), then releaseLock()", + async res => { + const reader = res.body!.getReader(); + await reader.read(); + reader.releaseLock(); + }, + ], + ["dropped while a reader holds the lock", res => res.body!.getReader().read()], + ]; + + for (const [name, shape] of shapes) { + test(name, async () => { + let aborted = 0; + using server = Bun.serve({ + port: 0, + fetch(req) { + req.signal.addEventListener("abort", () => aborted++); + return new Response( + new ReadableStream({ + async pull(controller) { + await Bun.sleep(1); + controller.enqueue(new Uint8Array(64 * 1024)); + }, + }), + ); + }, + }); + const N = 20; + // Its own frame, so that nothing on this one still refers to a response afterwards. + async function abandonOne() { + await shape(await fetch(server.url)); + } + for (let i = 0; i < N; i++) await abandonOne(); + + // Bounds the failing case only; the fixed build is done in well under a second. + const deadline = performance.now() + (isASAN || isDebug ? 15_000 : 3000); + while (aborted < N && performance.now() < deadline) { + Bun.gc(true); + await Bun.sleep(10); + } + // A couple can survive a collection through stale stack slots; the rest must go. + expect(N - aborted).toBeLessThanOrEqual(2); + }, 30_000); + } +}); + +// A fetch body handed to something that takes it natively, here a `Bun.serve` response that +// proxies it. When that response's own client goes away, the fetch behind the body has to be +// aborted with it: the body is locked to the response, so nothing else can read the rest. +// Before, the response only let go of the body, and the fetch sat paused on its connection +// (one upstream connection leaked per proxy client that went away). +describe("a proxied fetch body is aborted once the response's client is gone", () => { + const CHUNK = 64 * 1024; + // More than loopback socket buffers absorb, so a client that reads nothing backpressures + // the whole chain. Finite, so a build that keeps draining stays bounded. + const CHUNKS = 512; + + const shapes: [string, (upstream: Response) => Response][] = [ + // Goes through the JS pump; passes before and after, it is here so both spellings stay covered. + ["new Response(upstream.body)", upstream => new Response(upstream.body)], + // The native pipe. + ["the upstream Response itself", upstream => upstream], + ]; + + function servers(proxyResponse: (upstream: Response) => Response) { + const state = { pulls: 0, upstreamAborted: false, clientGone: Promise.withResolvers() }; + const upstream = Bun.serve({ + port: 0, + fetch(req) { + req.signal.addEventListener("abort", () => (state.upstreamAborted = true)); + return new Response( + new ReadableStream({ + pull(controller) { + if (++state.pulls <= CHUNKS) { + controller.enqueue(new Uint8Array(CHUNK)); + return; + } + // Out of data, but the body is not over: the upstream is still mid-body. + return new Promise(() => {}); + }, + }), + ); + }, + }); + const proxy = Bun.serve({ + port: 0, + async fetch(req) { + req.signal.addEventListener("abort", () => state.clientGone.resolve()); + return proxyResponse(await fetch(upstream.url)); + }, + }); + return { + state, + proxy, + [Symbol.dispose]() { + proxy.stop(true); + upstream.stop(true); + }, + }; + } + + async function expectUpstreamAborted(state: { upstreamAborted: boolean }) { + // Bounds the failing case only; the fixed build aborts the upstream as the client goes. + const deadline = performance.now() + 3000; + while (!state.upstreamAborted && performance.now() < deadline) await Bun.sleep(10); + expect(state.upstreamAborted).toBe(true); + } + + for (const [name, proxyResponse] of shapes) { + test(`${name}, client leaves while reading`, async () => { + using chain = servers(proxyResponse); + const { state } = chain; + + const client = new AbortController(); + const res = await fetch(chain.proxy.url, { signal: client.signal }); + const { value } = await res.body!.getReader().read(); + expect(value!.byteLength).toBeGreaterThan(0); + client.abort(); + await state.clientGone.promise; + + await expectUpstreamAborted(state); + }); + + test(`${name}, client leaves without reading`, async () => { + using chain = servers(proxyResponse); + const { state } = chain; + + const client = new AbortController(); + const res = await fetch(chain.proxy.url, { signal: client.signal }); + expect(res.status).toBe(200); + // Nothing reads: wait for the upstream to stop being pulled, which is the chain + // backpressured end to end (or, on a build that drains, out of data). + let last = -1; + for (let stable = 0; stable < 5; ) { + await Bun.sleep(20); + if (state.pulls === last) { + stable++; + } else { + stable = 0; + last = state.pulls; + } + } + client.abort(); + await state.clientGone.promise; + + await expectUpstreamAborted(state); + }); + } +}); diff --git a/test/js/workerd/html-rewriter.test.js b/test/js/workerd/html-rewriter.test.js index bd8164715e3c..dfc0333b5922 100644 --- a/test/js/workerd/html-rewriter.test.js +++ b/test/js/workerd/html-rewriter.test.js @@ -3101,4 +3101,58 @@ describe("GC pressure mid-rewrite", () => { }); expect(exitCode).toBe(0); }); + + // The transform's input is a fetch() body wired straight into the pipe. Once the caller + // keeps only the promise for the output, the pipe is reachable from nothing in JS; what + // keeps it alive is the input stream's source, which the fetch holds while it delivers. + // The body arrives across many turns, with collections in between: every chunk must + // still reach the handlers. + it("a transform over a fetch body survives GC once only its output promise is held", async () => { + const fixture = /* js */ ` + const CHUNKS = 20; + let release; + const gate = new Promise(resolve => (release = resolve)); + const server = Bun.serve({ + port: 0, + fetch: () => + new Response( + new ReadableStream({ + async start(controller) { + controller.enqueue(""); + await gate; + for (let i = 0; i < CHUNKS; i++) { + controller.enqueue("

" + Buffer.alloc(1000, "x").toString() + "

"); + await Bun.sleep(1); + } + controller.close(); + }, + }), + { headers: { "content-type": "text/html" } }, + ), + }); + async function start() { + const res = await fetch(server.url); + return new HTMLRewriter().on("p", { element(el) { el.setAttribute("seen", ""); } }).transform(res).text(); + } + const text = start(); + for (let i = 0; i < 5; i++) { Bun.gc(true); await Bun.sleep(1); } + release(); + for (let i = 0; i < 30; i++) { Bun.gc(true); await Bun.sleep(2); } + const html = await text; + console.log(JSON.stringify({ seen: html.split('

').length - 1, ended: html.endsWith("

") })); + server.stop(true); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ + stdout: JSON.stringify({ seen: 20, ended: true }), + stderr: "", + exitCode: 0, + }); + }); }); From 543210de54107c0020ba9473f9dc00e34e15f9da Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 05:13:14 +0000 Subject: [PATCH 09/22] fetch: address review of the parked-body change - RequestContext::end_chunk clears byte_stream up front, so on_abort's pipe-ref release cannot run a second time for a sink the stream already ended. - ByteStream::on_pull no longer unparks the producer before it knows the pull drained the buffer; only a drain or a newly wired sink unparks. - NewSource::wrapper_unrooted is a Cell and is cleared when the producer detaches. - Test: a body that parked and then had one partial read is still collected and aborted; a parked body picked up by res.text() or an HTMLRewriter transform drains fully. --- src/runtime/server/RequestContext.rs | 2 ++ src/runtime/webcore/ByteStream.rs | 2 -- src/runtime/webcore/ReadableStream.rs | 12 ++++++---- src/runtime/webcore/fetch/FetchTasklet.rs | 10 +++++--- test/js/web/fetch/fetch-backpressure.test.ts | 16 +++++++++---- .../fetch/fetch-stream-cancel-leak.test.ts | 24 +++++++++++++++++-- 6 files changed, 49 insertions(+), 17 deletions(-) diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index 6797da4c5cd4..e7eeecc066f5 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -3247,6 +3247,8 @@ where // SAFETY: caller passes the live `*mut RequestContext` stored as the // sink ctx; `_ref` keeps it alive for this call. let this = unsafe { &*this }; + // The stream has already dropped this sink; `_ref` is the pipe's ref. + this.byte_stream.set(None); if this.is_aborted_or_ended() { return; diff --git a/src/runtime/webcore/ByteStream.rs b/src/runtime/webcore/ByteStream.rs index 42408021646c..8c554b780c2c 100644 --- a/src/runtime/webcore/ByteStream.rs +++ b/src/runtime/webcore/ByteStream.rs @@ -593,8 +593,6 @@ impl ByteStream { bun_jsc::mark_binding!(); debug_assert!(!buffer.is_empty()); debug_assert!(self.buffer_action.get().is_none()); - // A reader is back on a stream the producer may have parked unread. - self.signal_consumer_attached(); if !self.buffer.get().is_empty() { debug_assert!(self.value().is_empty()); // == .zero diff --git a/src/runtime/webcore/ReadableStream.rs b/src/runtime/webcore/ReadableStream.rs index 97d29d716a3b..be0d067c136b 100644 --- a/src/runtime/webcore/ReadableStream.rs +++ b/src/runtime/webcore/ReadableStream.rs @@ -858,7 +858,7 @@ pub struct NewSource { /// The producer holding a native ref has parked ([`Self::unroot_wrapper`]): /// its ref keeps this allocation, not the wrapper, so an unread stream can /// be collected. Cleared by [`Self::root_wrapper`]. - pub wrapper_unrooted: bool, + pub wrapper_unrooted: Cell, /// R-2: written by context methods (`ByteStream::to_any_blob`, /// `ByteBlobLoader::to_any_blob`) through their parent accessor, so /// interior-mutable. @@ -877,7 +877,7 @@ impl Default for NewSource { producer: Cell::new(streams::SourceHandle::None), global_this: None, this_jsvalue: jsc::JsRef::empty(), - wrapper_unrooted: false, + wrapper_unrooted: Cell::new(false), is_closed: Cell::new(false), } } @@ -1149,7 +1149,7 @@ impl NewSource { // `waiting_for_on_reader_done` I/O ref). Root the wrapper so // `on_js_close`, reached from `on_reader_done` off the event loop with // no JS frame on the stack, never reads a dead-but-unswept cell. - if !self.wrapper_unrooted { + if !self.wrapper_unrooted.get() { self.upgrade_wrapper(); } } @@ -1165,14 +1165,16 @@ impl NewSource { /// The producer keeps its native ref but stops rooting the wrapper: nothing /// is reading, so the stream should be collectable. [`SourceContext::wrapper_finalized`] /// tells the producer if that happens. + /// Same access pattern as [`Self::increment_count`]: reached through the + /// producer's raw pointer while the context may be borrowed. pub fn unroot_wrapper(&mut self) { - self.wrapper_unrooted = true; + self.wrapper_unrooted.set(true); self.this_jsvalue.downgrade(); } /// Undo [`Self::unroot_wrapper`]: a consumer is reading again. pub fn root_wrapper(&mut self) { - self.wrapper_unrooted = false; + self.wrapper_unrooted.set(false); if self.ref_count > 1 { self.upgrade_wrapper(); } diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index fed37b7f5f29..964654560077 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -128,7 +128,8 @@ pub struct FetchTasklet { pub(crate) response_stream_source: Cell>>, /// Nothing reads the body stream and it holds `UNOBSERVED_BODY_HIGH_WATER_MARK` bytes: the /// transport is left paused, the loop is released, and the stream is left collectable - /// (`on_body_stream_collected`). A consumer that comes back unparks (`on_stream_drained`). + /// (`on_body_stream_collected`). A consumer that drains the stream's buffer or a native + /// sink that attaches unparks it. pub(crate) body_stream_parked: Cell, pub(crate) request_headers: Headers, pub(crate) promise: jsc::JSPromiseStrong, @@ -1713,7 +1714,10 @@ impl FetchTasklet { let source = self.response_stream_source.take()?; self.body_stream_parked.set(false); // SAFETY: still pinned by the ref from `on_readable_stream_available`. - unsafe { (*source.as_ptr()).producer.set(SourceHandle::None) }; + unsafe { + (*source.as_ptr()).producer.set(SourceHandle::None); + (*source.as_ptr()).wrapper_unrooted.set(false); + } Some(source) } @@ -1808,7 +1812,7 @@ impl FetchTasklet { self.poll_ref .with_mut(|poll_ref| poll_ref.unref(bun_io::js_vm_ctx())); if let Some(source) = self.response_stream_source.get() { - // SAFETY: live through the producer ref; no borrow of the source is held here. + // SAFETY: live through the producer ref (same pattern as `increment_count`). unsafe { (*source.as_ptr()).unroot_wrapper() }; } } diff --git a/test/js/web/fetch/fetch-backpressure.test.ts b/test/js/web/fetch/fetch-backpressure.test.ts index e1c46c95580b..24f2dbfe95d1 100644 --- a/test/js/web/fetch/fetch-backpressure.test.ts +++ b/test/js/web/fetch/fetch-backpressure.test.ts @@ -1,8 +1,7 @@ // Receive-side backpressure: a stalled `res.body.getReader()` must stop the // HTTP thread from buffering the entire response in memory. import { describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, isASAN, isDebug, isWindows, tempDir, tls } from "harness"; -import { join } from "node:path"; +import { bunEnv, bunExe, isASAN, isDebug, isWindows, tls } from "harness"; import { randomBytes } from "node:crypto"; import { once } from "node:events"; import { createServer } from "node:http"; @@ -605,11 +604,18 @@ describe("fetch() receive backpressure — body stream nothing is reading", () = // pick the transport up again and see the whole body. for (const [name, drain] of [ ["res.text()", (res: Response) => res.text().then(t => t.length)], - ["Bun.write(file, res)", (res: Response, dir: string) => Bun.write(join(dir, "out"), res)], + [ + "HTMLRewriter.transform(res)", + (res: Response) => + new HTMLRewriter() + .on("x", {}) + .transform(res) + .arrayBuffer() + .then(b => b.byteLength), + ], ] as const) { test(`res.body read by nothing, then ${name} drains the whole body`, async () => { await using server = await serve("h1"); - using dir = tempDir("fetch-parked-body", {}); const res = await fetch(server.url); void res.body; // Parked: the client stops taking bytes well before the end (16 MiB body, 256 KiB mark). @@ -620,7 +626,7 @@ describe("fetch() receive backpressure — body stream nothing is reading", () = last = server.sent(); } expect(server.sent()).toBeLessThan(TOTAL); - expect(await drain(res, String(dir))).toBe(TOTAL); + expect(await drain(res)).toBe(TOTAL); }); } }); diff --git a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts index 0d2b861a05e5..bb353851c55c 100644 --- a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts +++ b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts @@ -209,7 +209,7 @@ test("response.body.cancel() on a never-read body aborts the underlying fetch", // body that does not end, the stream, the connection, and the buffered bytes all lived until // the process exited. describe("an abandoned fetch body stream is collected and its fetch is aborted", () => { - const shapes: [string, (res: Response) => Promise][] = [ + const shapes: [string, (res: Response, parked: () => Promise) => Promise][] = [ ["res.body touched", async res => res.body], [ "one read(), then releaseLock()", @@ -220,11 +220,21 @@ describe("an abandoned fetch body stream is collected and its fetch is aborted", }, ], ["dropped while a reader holds the lock", res => res.body!.getReader().read()], + // A read that takes only part of what the parked stream buffered must leave it parked. + [ + "one read() after it parked", + async (res, parked) => { + void res.body; + await parked(); + await res.body!.getReader().read(); + }, + ], ]; for (const [name, shape] of shapes) { test(name, async () => { let aborted = 0; + let pulls = 0; using server = Bun.serve({ port: 0, fetch(req) { @@ -232,6 +242,7 @@ describe("an abandoned fetch body stream is collected and its fetch is aborted", return new Response( new ReadableStream({ async pull(controller) { + pulls++; await Bun.sleep(1); controller.enqueue(new Uint8Array(64 * 1024)); }, @@ -239,10 +250,19 @@ describe("an abandoned fetch body stream is collected and its fetch is aborted", ); }, }); + // The client stopped taking bytes: the server is no longer pulled. + async function parked() { + let last = -1; + for (let stable = 0; stable < 5; ) { + await Bun.sleep(10); + stable = pulls === last ? stable + 1 : 0; + last = pulls; + } + } const N = 20; // Its own frame, so that nothing on this one still refers to a response afterwards. async function abandonOne() { - await shape(await fetch(server.url)); + await shape(await fetch(server.url), parked); } for (let i = 0; i < N; i++) await abandonOne(); From fa3435b29901ce3d8fdf6e13271b782139f20998 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 05:15:17 +0000 Subject: [PATCH 10/22] [autofix.ci] apply automated fixes --- src/runtime/webcore/fetch/FetchTasklet.rs | 4 +- .../fetch/fetch-stream-cancel-leak.test.ts | 86 ++++++++++--------- 2 files changed, 46 insertions(+), 44 deletions(-) diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 964654560077..4373209de4bc 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -1708,9 +1708,7 @@ impl FetchTasklet { /// Stop being the producer. The source stays pinned by our ref, so the caller can still /// deliver a terminal chunk before `release_response_stream_source`. Touches no JS cell. - fn take_response_stream_source( - &self, - ) -> Option> { + fn take_response_stream_source(&self) -> Option> { let source = self.response_stream_source.take()?; self.body_stream_parked.set(false); // SAFETY: still pinned by the ref from `on_readable_stream_available`. diff --git a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts index bb353851c55c..64d418fb5728 100644 --- a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts +++ b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts @@ -232,49 +232,53 @@ describe("an abandoned fetch body stream is collected and its fetch is aborted", ]; for (const [name, shape] of shapes) { - test(name, async () => { - let aborted = 0; - let pulls = 0; - using server = Bun.serve({ - port: 0, - fetch(req) { - req.signal.addEventListener("abort", () => aborted++); - return new Response( - new ReadableStream({ - async pull(controller) { - pulls++; - await Bun.sleep(1); - controller.enqueue(new Uint8Array(64 * 1024)); - }, - }), - ); - }, - }); - // The client stopped taking bytes: the server is no longer pulled. - async function parked() { - let last = -1; - for (let stable = 0; stable < 5; ) { - await Bun.sleep(10); - stable = pulls === last ? stable + 1 : 0; - last = pulls; + test( + name, + async () => { + let aborted = 0; + let pulls = 0; + using server = Bun.serve({ + port: 0, + fetch(req) { + req.signal.addEventListener("abort", () => aborted++); + return new Response( + new ReadableStream({ + async pull(controller) { + pulls++; + await Bun.sleep(1); + controller.enqueue(new Uint8Array(64 * 1024)); + }, + }), + ); + }, + }); + // The client stopped taking bytes: the server is no longer pulled. + async function parked() { + let last = -1; + for (let stable = 0; stable < 5; ) { + await Bun.sleep(10); + stable = pulls === last ? stable + 1 : 0; + last = pulls; + } } - } - const N = 20; - // Its own frame, so that nothing on this one still refers to a response afterwards. - async function abandonOne() { - await shape(await fetch(server.url), parked); - } - for (let i = 0; i < N; i++) await abandonOne(); + const N = 20; + // Its own frame, so that nothing on this one still refers to a response afterwards. + async function abandonOne() { + await shape(await fetch(server.url), parked); + } + for (let i = 0; i < N; i++) await abandonOne(); - // Bounds the failing case only; the fixed build is done in well under a second. - const deadline = performance.now() + (isASAN || isDebug ? 15_000 : 3000); - while (aborted < N && performance.now() < deadline) { - Bun.gc(true); - await Bun.sleep(10); - } - // A couple can survive a collection through stale stack slots; the rest must go. - expect(N - aborted).toBeLessThanOrEqual(2); - }, 30_000); + // Bounds the failing case only; the fixed build is done in well under a second. + const deadline = performance.now() + (isASAN || isDebug ? 15_000 : 3000); + while (aborted < N && performance.now() < deadline) { + Bun.gc(true); + await Bun.sleep(10); + } + // A couple can survive a collection through stale stack slots; the rest must go. + expect(N - aborted).toBeLessThanOrEqual(2); + }, + 30_000, + ); } }); From 6d2033b2305897c7e0a8f7618ac3a697cd112519 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 05:17:05 +0000 Subject: [PATCH 11/22] test: small unread fetch bodies still complete and reuse their connection No-Verification-Needed: test-only change --- test/js/web/fetch/fetch-backpressure.test.ts | 26 ++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/js/web/fetch/fetch-backpressure.test.ts b/test/js/web/fetch/fetch-backpressure.test.ts index 24f2dbfe95d1..0924c9c7d350 100644 --- a/test/js/web/fetch/fetch-backpressure.test.ts +++ b/test/js/web/fetch/fetch-backpressure.test.ts @@ -600,6 +600,32 @@ describe("fetch() receive backpressure — body stream nothing is reading", () = }, 60_000); } + // The other half of the rule: a small body nobody reads is still taken off the socket, so + // the keep-alive connection goes back to the pool instead of staying pinned under it. + test("small unread bodies complete and their connection is reused", async () => { + let connections = 0; + const srv = createServer((_req, res) => { + res.setHeader("content-length", String(16 * 1024)); + res.end(Buffer.alloc(16 * 1024, 65)); + }).on("connection", () => connections++); + srv.listen(0); + await once(srv, "listening"); + try { + const url = `http://127.0.0.1:${(srv.address() as import("node:net").AddressInfo).port}/`; + const N = 10; + for (let i = 0; i < N; i++) { + const res = await fetch(url); + expect(res.status).toBe(200); + void res.body; + } + // Not exactly 1: a request can start before the previous body's last packet landed. + expect(connections).toBeLessThan(N / 2); + } finally { + srv.closeAllConnections(); + await new Promise(r => srv.close(() => r(undefined))); + } + }); + // The other ways back to a parked body: a buffered read and a native sink both have to // pick the transport up again and see the whole body. for (const [name, drain] of [ From 1e50ba54e9c8a1876b032ed9fc093542a3453505 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 05:24:22 +0000 Subject: [PATCH 12/22] test: make the connection-reuse test exercise the unread-but-under-the-mark path The body now trails the headers in several packets so it reaches the unread stream chunk by chunk; a client that parked on the first unread chunk opens 10 connections here instead of 1. No-Verification-Needed: test-only change --- test/js/web/fetch/fetch-backpressure.test.ts | 22 ++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/test/js/web/fetch/fetch-backpressure.test.ts b/test/js/web/fetch/fetch-backpressure.test.ts index 0924c9c7d350..6ef96f693b02 100644 --- a/test/js/web/fetch/fetch-backpressure.test.ts +++ b/test/js/web/fetch/fetch-backpressure.test.ts @@ -603,10 +603,23 @@ describe("fetch() receive backpressure — body stream nothing is reading", () = // The other half of the rule: a small body nobody reads is still taken off the socket, so // the keep-alive connection goes back to the pool instead of staying pinned under it. test("small unread bodies complete and their connection is reused", async () => { + // The body trails the headers in several packets, so it reaches a stream nothing reads + // chunk by chunk; the client has to keep taking it (it is under the high-water mark) for + // the response to finish and the connection to go back to the pool. A client that parks + // on the first unread chunk needs a new connection for every request here. + const PART = 32 * 1024; + const PARTS = 4; let connections = 0; - const srv = createServer((_req, res) => { - res.setHeader("content-length", String(16 * 1024)); - res.end(Buffer.alloc(16 * 1024, 65)); + let finished = Promise.resolve(); + const srv = createServer(async (_req, res) => { + finished = new Promise(r => res.on("finish", () => r())); + res.setHeader("content-length", String(PART * PARTS)); + res.flushHeaders(); + for (let i = 0; i < PARTS; i++) { + await new Promise(r => setTimeout(r, 2)); + res.write(Buffer.alloc(PART, 65)); + } + res.end(); }).on("connection", () => connections++); srv.listen(0); await once(srv, "listening"); @@ -617,8 +630,9 @@ describe("fetch() receive backpressure — body stream nothing is reading", () = const res = await fetch(url); expect(res.status).toBe(200); void res.body; + await finished; } - // Not exactly 1: a request can start before the previous body's last packet landed. + // Not exactly 1: a request can start before the previous body's last packet was taken. expect(connections).toBeLessThan(N / 2); } finally { srv.closeAllConnections(); From 805fb7c0135261c70db0438b0d717a44d2266cd4 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 05:29:19 +0000 Subject: [PATCH 13/22] test: don't assert server write progress in the parked-body comeback tests Loopback buffers on some CI hosts can absorb the whole 16 MiB body, so how far the server got says nothing about the client there. No-Verification-Needed: test-only change --- test/js/web/fetch/fetch-backpressure.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/js/web/fetch/fetch-backpressure.test.ts b/test/js/web/fetch/fetch-backpressure.test.ts index 6ef96f693b02..e1dfe68a91a8 100644 --- a/test/js/web/fetch/fetch-backpressure.test.ts +++ b/test/js/web/fetch/fetch-backpressure.test.ts @@ -658,14 +658,14 @@ describe("fetch() receive backpressure — body stream nothing is reading", () = await using server = await serve("h1"); const res = await fetch(server.url); void res.body; - // Parked: the client stops taking bytes well before the end (16 MiB body, 256 KiB mark). + // Let the client park (16 MiB body, 256 KiB mark): wait until bytes stop moving. How far + // the server got is not asserted; loopback buffers on some hosts can take the whole body. let last = -1; for (let stable = 0; stable < 3; ) { await Bun.sleep(20); stable = server.sent() === last ? stable + 1 : 0; last = server.sent(); } - expect(server.sent()).toBeLessThan(TOTAL); expect(await drain(res)).toBe(TOTAL); }); } From 4772daa87fb7393a285d7adb347ac54bfa21705e Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 05:48:03 +0000 Subject: [PATCH 14/22] test: report what a still-alive fetch client is holding; loosen GC survivor bound No-Verification-Needed: test-only change --- test/js/web/fetch/fetch-backpressure.test.ts | 24 +++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/test/js/web/fetch/fetch-backpressure.test.ts b/test/js/web/fetch/fetch-backpressure.test.ts index e1dfe68a91a8..564b15c206b5 100644 --- a/test/js/web/fetch/fetch-backpressure.test.ts +++ b/test/js/web/fetch/fetch-backpressure.test.ts @@ -684,11 +684,29 @@ describe.concurrent("fetch() receive backpressure — a body nothing waits for d ["an idle reader", /* js */ `const reader = res.body.getReader(); await reader.read(); globalThis.keep = reader;`], ] as const; + // If the process is still around after 6 s, say what it is holding (an unref'd timer, so it + // cannot itself be the reason), then how much a late reader finds buffered and whether more + // follows. Only ever printed by a failing run. + const diagnose = /* js */ ` + setTimeout(async () => { + const { getEventLoopStats } = require("bun:internal-for-testing"); + const stats = getEventLoopStats(); + const reader = (globalThis.keep instanceof Response ? globalThis.keep.body : globalThis.keep instanceof ReadableStream ? globalThis.keep : null)?.getReader() ?? globalThis.keep; + const reads = []; + for (let i = 0; i < 6; i++) { + const r = await Promise.race([reader.read(), Bun.sleep(300).then(() => null)]); + reads.push(r === null ? "pending" : r.done ? "done" : r.value.byteLength); + if (r === null) break; + } + console.error("still alive: " + JSON.stringify({ stats, reads, after: getEventLoopStats() })); + }, 6000).unref(); + `; + for (const [holding, script] of idleClients) { test(`a process holding ${holding} exits on its own`, async () => { await using server = await serveUntilBlocked(); await using proc = Bun.spawn({ - cmd: [bunExe(), "-e", `const res = await fetch(${JSON.stringify(server.url)}); ${script}`], + cmd: [bunExe(), "-e", `const res = await fetch(${JSON.stringify(server.url)}); ${script} ${diagnose}`], env: bunEnv, stdout: "pipe", stderr: "pipe", @@ -696,13 +714,13 @@ describe.concurrent("fetch() receive backpressure — a body nothing waits for d const stderr = proc.stderr.text(); const exited = proc.exited.then(exitCode => ({ exitCode })); const deadline = performance.now() + 10_000; - let outcome: { exitCode: number } | { stillAlive: true; serverSentMiB: number } | undefined; + let outcome: { exitCode: number } | { stillAlive: true; serverSentKiB: number } | undefined; while (outcome === undefined) { outcome = await Promise.race([exited, Bun.sleep(20).then(() => undefined)]); // Took the whole body: it is draining. Took little and is still around: it is // holding the paused body. Neither exited. if (outcome === undefined && (server.sent() >= BODY || performance.now() >= deadline)) { - outcome = { stillAlive: true, serverSentMiB: server.sent() >> 20 }; + outcome = { stillAlive: true, serverSentKiB: server.sent() >> 10 }; proc.kill(); } } From d2977859a406c45cc9c3bea183239a3dee6d1ae4 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 05:49:25 +0000 Subject: [PATCH 15/22] test: loosen the GC survivor bound for abandoned fetch bodies No-Verification-Needed: test-only change --- test/js/web/fetch/fetch-stream-cancel-leak.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts index 64d418fb5728..7ffbfe2ead31 100644 --- a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts +++ b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts @@ -274,8 +274,9 @@ describe("an abandoned fetch body stream is collected and its fetch is aborted", Bun.gc(true); await Bun.sleep(10); } - // A couple can survive a collection through stale stack slots; the rest must go. - expect(N - aborted).toBeLessThanOrEqual(2); + // A few can survive a collection through stale stack slots (conservative scanning); + // the rest must go. Unfixed, none of them do. + expect(N - aborted).toBeLessThan(N / 4); }, 30_000, ); From 49e520d724fd7682fca6a408dee6624dfe3b083d Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 05:50:52 +0000 Subject: [PATCH 16/22] ReadableStream: satisfy undocumented_unsafe_blocks in NewSource::finalize No-Verification-Needed: lint-only, no behavior change --- src/runtime/webcore/ReadableStream.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/runtime/webcore/ReadableStream.rs b/src/runtime/webcore/ReadableStream.rs index be0d067c136b..b682737c40bf 100644 --- a/src/runtime/webcore/ReadableStream.rs +++ b/src/runtime/webcore/ReadableStream.rs @@ -1475,8 +1475,10 @@ impl NewSource { unsafe { (*this).this_jsvalue.finalize() }; // SAFETY: `this` is live; the JS-wrapper +1 (released last) keeps ref_count > 0 // across whatever ref the producer drops in response. - if unsafe { (*this).ref_count } > 1 { - unsafe { (*this).context.wrapper_finalized() }; + unsafe { + if (*this).ref_count > 1 { + (*this).context.wrapper_finalized(); + } } // SAFETY: `this` is live; the JS-wrapper ref below still pins the count. if unsafe { (*this).context.finalize_detach() } { From b0d35fe63fac24a91c3053f00e6bcbd9edacb947 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 06:38:42 +0000 Subject: [PATCH 17/22] internal-for-testing: report the event loop's liveness inputs in getEventLoopStats Adds tasks, immediateTasks, concurrentTasksEmpty, loopActive and eventLoopAlive next to the existing counters, so a test that finds a process unexpectedly alive can say why. No-Verification-Needed: testing-only binding --- src/js/internal-for-testing.ts | 12 ++++++++++-- src/jsc/event_loop.rs | 29 ++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/js/internal-for-testing.ts b/src/js/internal-for-testing.ts index 0c8ead8a215f..e3d2f973b434 100644 --- a/src/js/internal-for-testing.ts +++ b/src/js/internal-for-testing.ts @@ -671,8 +671,16 @@ export const isMemoryPressureWatcherInstalled: () => boolean = $newCppFunction( 0, ); -export const getEventLoopStats: () => { activeTasks: number; concurrentRef: number; numPolls: number } = - $newRustFunction("event_loop.rs", "getActiveTasks", 0); +export const getEventLoopStats: () => { + activeTasks: number; + tasks: number; + immediateTasks: number; + concurrentTasksEmpty: boolean; + concurrentRef: number; + numPolls: number; + loopActive: boolean; + eventLoopAlive: boolean; +} = $newRustFunction("event_loop.rs", "getActiveTasks", 0); export const hostedGitInfo = { parseUrl: $newRustFunction("hosted_git_info.rs", "TestingAPIs.jsParseUrl", 1), diff --git a/src/jsc/event_loop.rs b/src/jsc/event_loop.rs index b1eb57abaa7d..be2b578fd645 100644 --- a/src/jsc/event_loop.rs +++ b/src/jsc/event_loop.rs @@ -1270,12 +1270,39 @@ pub fn get_active_tasks(global_object: &JSGlobalObject, _frame: &CallFrame) -> J // fields and call &-methods on it for the duration of this host fn. let vm_ref = global_object.bun_vm(); let event_loop = vm_ref.event_loop_shared(); - let result = JSValue::create_empty_object(global_object, 3); + let result = JSValue::create_empty_object(global_object, 8); result.put( global_object, b"activeTasks", JSValue::js_number(vm_ref.active_tasks as f64), ); + result.put( + global_object, + b"tasks", + JSValue::js_number(event_loop.tasks.readable_length() as f64), + ); + result.put( + global_object, + b"immediateTasks", + JSValue::js_number( + (event_loop.immediate_tasks.len() + event_loop.next_immediate_tasks.len()) as f64, + ), + ); + result.put( + global_object, + b"concurrentTasksEmpty", + JSValue::from(event_loop.concurrent_tasks.is_empty()), + ); + result.put( + global_object, + b"loopActive", + JSValue::from(vm_ref.platform_loop_opt().is_some_and(|h| h.is_active())), + ); + result.put( + global_object, + b"eventLoopAlive", + JSValue::from(vm_ref.is_event_loop_alive()), + ); result.put( global_object, b"concurrentRef", From effde484fd14bafa83dcd4dac41c7b980abcf957 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 06:42:05 +0000 Subject: [PATCH 18/22] RequestContext: drop the byte_stream clear end_chunk now does up front No-Verification-Needed: dead-store removal, no behavior change --- src/runtime/server/RequestContext.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index e7eeecc066f5..5cd7650acfd7 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -3256,13 +3256,12 @@ where if let Some(err) = err { // No status committed yet: the upstream producer (e.g. a // suspended `HTMLRewriter` transform whose async handler - // rejected) failed before any bytes were emitted. Detach the - // ByteStream state and hand the error to the server's - // `error()` hook so it can supply the response. + // rejected) failed before any bytes were emitted. Drop the + // stream and hand the error to the server's `error()` hook so it + // can supply the response. if !this.flags.has_written_status() { let global_this = this.server().global_this(); let js_err = err.to_js(global_this); - this.byte_stream.set(None); this.response_body_readable_stream_ref .with_mut(|s| s.deinit()); this.run_error_handler(js_err); From 672ac35283ef2303ac4e793abf903ba8b16e3536 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 06:43:32 +0000 Subject: [PATCH 19/22] test: scale the proxied-upstream abort deadline for debug/ASAN builds No-Verification-Needed: test-only change --- test/js/web/fetch/fetch-stream-cancel-leak.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts index 7ffbfe2ead31..0ebf81dd69b6 100644 --- a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts +++ b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts @@ -340,7 +340,7 @@ describe("a proxied fetch body is aborted once the response's client is gone", ( async function expectUpstreamAborted(state: { upstreamAborted: boolean }) { // Bounds the failing case only; the fixed build aborts the upstream as the client goes. - const deadline = performance.now() + 3000; + const deadline = performance.now() + (isASAN || isDebug ? 15_000 : 3000); while (!state.upstreamAborted && performance.now() < deadline) await Bun.sleep(10); expect(state.upstreamAborted).toBe(true); } From 8f3d32d9af8ee0c20a1a96706b87c116410b780d Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 19 Aug 2026 06:58:44 +0000 Subject: [PATCH 20/22] fetch: hand buffered bytes to a new body stream before un-pausing the transport on_start_streaming flipped Paused->AutoPause and scheduled the resume before it took scheduled_response_buffer under the mutex. A chunk the HTTP thread appended (and paused for) in that window was drained into the new stream here, its progress task then found the buffer empty and returned early, and nothing was left to undo the pause: the body sat with one chunk buffered and the process stayed alive. Drain first, then flip and resume. Reproduced locally as 2/60 hangs of `void res.body` against a flooding server; 0/60 after. --- src/runtime/webcore/fetch/FetchTasklet.rs | 49 +++++++++-------------- 1 file changed, 20 insertions(+), 29 deletions(-) diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 4373209de4bc..2151cb1bec56 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -1647,23 +1647,8 @@ impl FetchTasklet { this.poll_ref .with_mut(|poll_ref| poll_ref.ref_(bun_io::js_vm_ctx())); - // The bytes already in `scheduled_response_buffer` are handed to the - // new stream below. That is the drain `Paused` was waiting for, so - // flip back to `AutoPause` so the resume scheduled here actually - // un-pauses the socket; otherwise a reader that finds the drained - // buffer smaller than the pending view returns `Pending` without - // signalling and the transport stays paused past a server FIN. - this.signal_store - .try_transition_receive_mode(BodyReceiveMode::Paused, BodyReceiveMode::AutoPause); - if let Some(http_) = this.http.as_mut() { http_.enable_response_body_streaming(); - - // If the server sent the headers and the response body in two separate socket writes - // and if the server doesn't close the connection by itself - // and doesn't send any follow-up data - // then we must make sure the HTTP thread flushes. - http::http_thread().schedule_receive_resume(http_.async_http_id); } this.mutex.lock(); @@ -1671,23 +1656,29 @@ impl FetchTasklet { // under the mutex so the HTTP thread cannot observe the stale `true` // in `callback()` between this unlock and `on_readable_stream_available`. this.is_buffering_body.store(false, Ordering::Release); - // explicit unlock at each return - // (no `?` paths between lock and unlock, so a guard is unnecessary). - let size_hint = this.get_size_hint(); + let size_hint = this.get_size_hint() as usize; + // This means we have received part of the body but not the whole thing. + let drained = core::mem::take(&mut this.scheduled_response_buffer.list); + this.mutex.unlock(); - // This means we have received part of the body but not the whole thing - if !this.scheduled_response_buffer.list.is_empty() { - let scheduled_response_buffer = core::mem::take(&mut this.scheduled_response_buffer); - this.mutex.unlock(); + // The bytes taken above are the drain `Paused` was waiting for: flip back to + // `AutoPause` and resume. After the drain, not before: a chunk the HTTP thread + // appends (and pauses for) in between would otherwise be handed to the stream here + // with its task finding the buffer empty, and nothing left to undo that pause. + // Also covers headers and body arriving in separate writes with no follow-up + // data: the HTTP thread must flush what it has. + this.signal_store + .try_transition_receive_mode(BodyReceiveMode::Paused, BodyReceiveMode::AutoPause); + this.schedule_receive_resume(); - return DrainResult::Owned { - list: scheduled_response_buffer.list, - size_hint: size_hint as usize, - }; + if drained.is_empty() { + DrainResult::EstimatedSize(size_hint) + } else { + DrainResult::Owned { + list: drained, + size_hint, + } } - - this.mutex.unlock(); - DrainResult::EstimatedSize(size_hint as usize) } fn get_size_hint(&self) -> BlobSizeType { From c7df6479d8f282510c1be38e2773c5e7fc0af95e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 19 Aug 2026 07:09:17 +0000 Subject: [PATCH 21/22] Leave the proxy abort fix to #36700 The on_abort and end_chunk change in RequestContext, the ByteStream::detach_finished_sink helper that only it used, and the proxied upstream tests are the same fix that #36700 carries on its own. This PR is about parking an unread fetch body. #36700 owns the proxy abort case. --- src/runtime/server/RequestContext.rs | 28 ++--- src/runtime/webcore/ByteStream.rs | 10 -- .../fetch/fetch-stream-cancel-leak.test.ts | 104 ------------------ 3 files changed, 10 insertions(+), 132 deletions(-) diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index 5cd7650acfd7..9177e79a8617 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -400,13 +400,14 @@ mod shim { .as_ref() .is_some_and(|s| matches!(s.data, crate::webcore::blob::store::Data::File(_))) } - /// The response is done with its natively piped body. A body still mid-stream is - /// cancelled: it stayed locked to this response, so nothing else can read it. #[inline] pub(super) fn byte_stream_unpipe(s: NonNull) { - // The caller has just `take()`n `self.byte_stream` and still holds - // `response_body_readable_stream_ref`, which keeps the pointee alive. - bun_ptr::BackRef::from(s).detach_finished_sink() + // The lone caller has just `take()`n the pointer out of + // `self.byte_stream`; the allocation is kept alive by + // `response_body_readable_stream_ref` (BackRef invariant: pointee + // outlives this temporary). R-2: `unpipe_without_deref` takes `&self` + // (interior-mutable `JsCell`), so shared deref is sufficient. + bun_ptr::BackRef::from(s).unpipe_without_deref() } } use crate::server::DevErrorPage; @@ -1395,14 +1396,6 @@ where return; } - // A natively piped body has nobody left to take it. The deref balances the ref - // `do_render_with_body` took for the pipe: `end_chunk`, which releases it otherwise, - // cannot run once the response is gone. - if let Some(stream) = this.byte_stream.take() { - shim::byte_stream_unpipe(stream); - this.deref(); - } - // if we can, free the request now. if this.is_dead_request() { this.finalize_without_deinit(); @@ -3247,8 +3240,6 @@ where // SAFETY: caller passes the live `*mut RequestContext` stored as the // sink ctx; `_ref` keeps it alive for this call. let this = unsafe { &*this }; - // The stream has already dropped this sink; `_ref` is the pipe's ref. - this.byte_stream.set(None); if this.is_aborted_or_ended() { return; @@ -3256,12 +3247,13 @@ where if let Some(err) = err { // No status committed yet: the upstream producer (e.g. a // suspended `HTMLRewriter` transform whose async handler - // rejected) failed before any bytes were emitted. Drop the - // stream and hand the error to the server's `error()` hook so it - // can supply the response. + // rejected) failed before any bytes were emitted. Detach the + // ByteStream state and hand the error to the server's + // `error()` hook so it can supply the response. if !this.flags.has_written_status() { let global_this = this.server().global_this(); let js_err = err.to_js(global_this); + this.byte_stream.set(None); this.response_body_readable_stream_ref .with_mut(|s| s.deinit()); this.run_error_handler(js_err); diff --git a/src/runtime/webcore/ByteStream.rs b/src/runtime/webcore/ByteStream.rs index 8c554b780c2c..ec0cfb8647cd 100644 --- a/src/runtime/webcore/ByteStream.rs +++ b/src/runtime/webcore/ByteStream.rs @@ -181,16 +181,6 @@ impl ByteStream { self.sink_paused.set(false); } - /// The sink is gone before the stream ended (its peer went away). The stream stays - /// locked to it, so nobody else can read the rest: close the producer too. - pub(crate) fn detach_finished_sink(&self) { - if self.has_received_last_chunk.get() { - self.unpipe_without_deref(); - } else { - self.cancel_from_sink(None); - } - } - /// Bytes delivered that no consumer has taken yet. #[inline] pub fn buffered_len(&self) -> usize { diff --git a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts index 0ebf81dd69b6..f2b4b461e235 100644 --- a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts +++ b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts @@ -282,107 +282,3 @@ describe("an abandoned fetch body stream is collected and its fetch is aborted", ); } }); - -// A fetch body handed to something that takes it natively, here a `Bun.serve` response that -// proxies it. When that response's own client goes away, the fetch behind the body has to be -// aborted with it: the body is locked to the response, so nothing else can read the rest. -// Before, the response only let go of the body, and the fetch sat paused on its connection -// (one upstream connection leaked per proxy client that went away). -describe("a proxied fetch body is aborted once the response's client is gone", () => { - const CHUNK = 64 * 1024; - // More than loopback socket buffers absorb, so a client that reads nothing backpressures - // the whole chain. Finite, so a build that keeps draining stays bounded. - const CHUNKS = 512; - - const shapes: [string, (upstream: Response) => Response][] = [ - // Goes through the JS pump; passes before and after, it is here so both spellings stay covered. - ["new Response(upstream.body)", upstream => new Response(upstream.body)], - // The native pipe. - ["the upstream Response itself", upstream => upstream], - ]; - - function servers(proxyResponse: (upstream: Response) => Response) { - const state = { pulls: 0, upstreamAborted: false, clientGone: Promise.withResolvers() }; - const upstream = Bun.serve({ - port: 0, - fetch(req) { - req.signal.addEventListener("abort", () => (state.upstreamAborted = true)); - return new Response( - new ReadableStream({ - pull(controller) { - if (++state.pulls <= CHUNKS) { - controller.enqueue(new Uint8Array(CHUNK)); - return; - } - // Out of data, but the body is not over: the upstream is still mid-body. - return new Promise(() => {}); - }, - }), - ); - }, - }); - const proxy = Bun.serve({ - port: 0, - async fetch(req) { - req.signal.addEventListener("abort", () => state.clientGone.resolve()); - return proxyResponse(await fetch(upstream.url)); - }, - }); - return { - state, - proxy, - [Symbol.dispose]() { - proxy.stop(true); - upstream.stop(true); - }, - }; - } - - async function expectUpstreamAborted(state: { upstreamAborted: boolean }) { - // Bounds the failing case only; the fixed build aborts the upstream as the client goes. - const deadline = performance.now() + (isASAN || isDebug ? 15_000 : 3000); - while (!state.upstreamAborted && performance.now() < deadline) await Bun.sleep(10); - expect(state.upstreamAborted).toBe(true); - } - - for (const [name, proxyResponse] of shapes) { - test(`${name}, client leaves while reading`, async () => { - using chain = servers(proxyResponse); - const { state } = chain; - - const client = new AbortController(); - const res = await fetch(chain.proxy.url, { signal: client.signal }); - const { value } = await res.body!.getReader().read(); - expect(value!.byteLength).toBeGreaterThan(0); - client.abort(); - await state.clientGone.promise; - - await expectUpstreamAborted(state); - }); - - test(`${name}, client leaves without reading`, async () => { - using chain = servers(proxyResponse); - const { state } = chain; - - const client = new AbortController(); - const res = await fetch(chain.proxy.url, { signal: client.signal }); - expect(res.status).toBe(200); - // Nothing reads: wait for the upstream to stop being pulled, which is the chain - // backpressured end to end (or, on a build that drains, out of data). - let last = -1; - for (let stable = 0; stable < 5; ) { - await Bun.sleep(20); - if (state.pulls === last) { - stable++; - } else { - stable = 0; - last = state.pulls; - } - } - client.abort(); - await state.clientGone.promise; - - await expectUpstreamAborted(state); - }); - } -}); From 31d2cc3acd4ddf081726d47cc1ee4540bcaf8719 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 19 Aug 2026 07:31:56 +0000 Subject: [PATCH 22/22] Revert "Leave the proxy abort fix to #36700" This reverts commit c7df6479d8f282510c1be38e2773c5e7fc0af95e. The proxy abort fix stays in this PR: RequestContext::on_abort cancels a natively piped body, end_chunk clears byte_stream up front, ByteStream::detach_finished_sink, and the proxied upstream tests. The tree is the same as 8f3d32d again. --- src/runtime/server/RequestContext.rs | 28 +++-- src/runtime/webcore/ByteStream.rs | 10 ++ .../fetch/fetch-stream-cancel-leak.test.ts | 104 ++++++++++++++++++ 3 files changed, 132 insertions(+), 10 deletions(-) diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index 9177e79a8617..5cd7650acfd7 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -400,14 +400,13 @@ mod shim { .as_ref() .is_some_and(|s| matches!(s.data, crate::webcore::blob::store::Data::File(_))) } + /// The response is done with its natively piped body. A body still mid-stream is + /// cancelled: it stayed locked to this response, so nothing else can read it. #[inline] pub(super) fn byte_stream_unpipe(s: NonNull) { - // The lone caller has just `take()`n the pointer out of - // `self.byte_stream`; the allocation is kept alive by - // `response_body_readable_stream_ref` (BackRef invariant: pointee - // outlives this temporary). R-2: `unpipe_without_deref` takes `&self` - // (interior-mutable `JsCell`), so shared deref is sufficient. - bun_ptr::BackRef::from(s).unpipe_without_deref() + // The caller has just `take()`n `self.byte_stream` and still holds + // `response_body_readable_stream_ref`, which keeps the pointee alive. + bun_ptr::BackRef::from(s).detach_finished_sink() } } use crate::server::DevErrorPage; @@ -1396,6 +1395,14 @@ where return; } + // A natively piped body has nobody left to take it. The deref balances the ref + // `do_render_with_body` took for the pipe: `end_chunk`, which releases it otherwise, + // cannot run once the response is gone. + if let Some(stream) = this.byte_stream.take() { + shim::byte_stream_unpipe(stream); + this.deref(); + } + // if we can, free the request now. if this.is_dead_request() { this.finalize_without_deinit(); @@ -3240,6 +3247,8 @@ where // SAFETY: caller passes the live `*mut RequestContext` stored as the // sink ctx; `_ref` keeps it alive for this call. let this = unsafe { &*this }; + // The stream has already dropped this sink; `_ref` is the pipe's ref. + this.byte_stream.set(None); if this.is_aborted_or_ended() { return; @@ -3247,13 +3256,12 @@ where if let Some(err) = err { // No status committed yet: the upstream producer (e.g. a // suspended `HTMLRewriter` transform whose async handler - // rejected) failed before any bytes were emitted. Detach the - // ByteStream state and hand the error to the server's - // `error()` hook so it can supply the response. + // rejected) failed before any bytes were emitted. Drop the + // stream and hand the error to the server's `error()` hook so it + // can supply the response. if !this.flags.has_written_status() { let global_this = this.server().global_this(); let js_err = err.to_js(global_this); - this.byte_stream.set(None); this.response_body_readable_stream_ref .with_mut(|s| s.deinit()); this.run_error_handler(js_err); diff --git a/src/runtime/webcore/ByteStream.rs b/src/runtime/webcore/ByteStream.rs index ec0cfb8647cd..8c554b780c2c 100644 --- a/src/runtime/webcore/ByteStream.rs +++ b/src/runtime/webcore/ByteStream.rs @@ -181,6 +181,16 @@ impl ByteStream { self.sink_paused.set(false); } + /// The sink is gone before the stream ended (its peer went away). The stream stays + /// locked to it, so nobody else can read the rest: close the producer too. + pub(crate) fn detach_finished_sink(&self) { + if self.has_received_last_chunk.get() { + self.unpipe_without_deref(); + } else { + self.cancel_from_sink(None); + } + } + /// Bytes delivered that no consumer has taken yet. #[inline] pub fn buffered_len(&self) -> usize { diff --git a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts index f2b4b461e235..0ebf81dd69b6 100644 --- a/test/js/web/fetch/fetch-stream-cancel-leak.test.ts +++ b/test/js/web/fetch/fetch-stream-cancel-leak.test.ts @@ -282,3 +282,107 @@ describe("an abandoned fetch body stream is collected and its fetch is aborted", ); } }); + +// A fetch body handed to something that takes it natively, here a `Bun.serve` response that +// proxies it. When that response's own client goes away, the fetch behind the body has to be +// aborted with it: the body is locked to the response, so nothing else can read the rest. +// Before, the response only let go of the body, and the fetch sat paused on its connection +// (one upstream connection leaked per proxy client that went away). +describe("a proxied fetch body is aborted once the response's client is gone", () => { + const CHUNK = 64 * 1024; + // More than loopback socket buffers absorb, so a client that reads nothing backpressures + // the whole chain. Finite, so a build that keeps draining stays bounded. + const CHUNKS = 512; + + const shapes: [string, (upstream: Response) => Response][] = [ + // Goes through the JS pump; passes before and after, it is here so both spellings stay covered. + ["new Response(upstream.body)", upstream => new Response(upstream.body)], + // The native pipe. + ["the upstream Response itself", upstream => upstream], + ]; + + function servers(proxyResponse: (upstream: Response) => Response) { + const state = { pulls: 0, upstreamAborted: false, clientGone: Promise.withResolvers() }; + const upstream = Bun.serve({ + port: 0, + fetch(req) { + req.signal.addEventListener("abort", () => (state.upstreamAborted = true)); + return new Response( + new ReadableStream({ + pull(controller) { + if (++state.pulls <= CHUNKS) { + controller.enqueue(new Uint8Array(CHUNK)); + return; + } + // Out of data, but the body is not over: the upstream is still mid-body. + return new Promise(() => {}); + }, + }), + ); + }, + }); + const proxy = Bun.serve({ + port: 0, + async fetch(req) { + req.signal.addEventListener("abort", () => state.clientGone.resolve()); + return proxyResponse(await fetch(upstream.url)); + }, + }); + return { + state, + proxy, + [Symbol.dispose]() { + proxy.stop(true); + upstream.stop(true); + }, + }; + } + + async function expectUpstreamAborted(state: { upstreamAborted: boolean }) { + // Bounds the failing case only; the fixed build aborts the upstream as the client goes. + const deadline = performance.now() + (isASAN || isDebug ? 15_000 : 3000); + while (!state.upstreamAborted && performance.now() < deadline) await Bun.sleep(10); + expect(state.upstreamAborted).toBe(true); + } + + for (const [name, proxyResponse] of shapes) { + test(`${name}, client leaves while reading`, async () => { + using chain = servers(proxyResponse); + const { state } = chain; + + const client = new AbortController(); + const res = await fetch(chain.proxy.url, { signal: client.signal }); + const { value } = await res.body!.getReader().read(); + expect(value!.byteLength).toBeGreaterThan(0); + client.abort(); + await state.clientGone.promise; + + await expectUpstreamAborted(state); + }); + + test(`${name}, client leaves without reading`, async () => { + using chain = servers(proxyResponse); + const { state } = chain; + + const client = new AbortController(); + const res = await fetch(chain.proxy.url, { signal: client.signal }); + expect(res.status).toBe(200); + // Nothing reads: wait for the upstream to stop being pulled, which is the chain + // backpressured end to end (or, on a build that drains, out of data). + let last = -1; + for (let stable = 0; stable < 5; ) { + await Bun.sleep(20); + if (state.pulls === last) { + stable++; + } else { + stable = 0; + last = state.pulls; + } + } + client.abort(); + await state.clientGone.promise; + + await expectUpstreamAborted(state); + }); + } +});