fetch: don't resume the body socket while ByteStream has spilled bytes - #36588
Conversation
WalkthroughChangesByteStream now delays producer-drained signaling when incoming data overflows the pending pull view. Fetch tests cover Node stream backpressure and concurrent download memory usage. Fetch backpressure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I checked that the deferred signal_drained() can't deadlock — on_pull at ByteStream.rs:552 fires it once buffer empties, and pending.run() still resolves the pull so the reader re-pulls. Also confirmed MutableString is just { list: Vec<u8> }, so the .list move in callback is sound and the following response_buffer.reset() is a no-op on the taken-empty vec. Deferring to a human given this touches the fetch body backpressure gate.
Extended reasoning...
Checked the spillover path end-to-end: after has_remaining skips signal_drained(), pending.run() resolves the reader with IntoArray, the next on_pull copies from buffer, and the if self.buffer.get().is_empty() { self.signal_drained(); } branch resumes the producer — so no stall. The sink and buffer_action arms of on_data are unchanged. On the FetchTasklet side, MutableString has no allocator/state field beyond .list, so swapping .list directly is equivalent to the whole-struct core::mem::take used at the other drain sites (lines 916, 1748). Not approving because the backpressure gate is a core fetch-streaming invariant and the RSS regression test only runs on release Linux.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/fetch/fetch-backpressure.test.ts`:
- Around line 265-296: Reduce the big payload size in the stalled Writable test
while keeping it comfortably above the relevant buffering thresholds. Preserve
the existing stall, drain, and full-transfer assertions, and use the file’s
existing size constants as precedent when choosing the smaller value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5ec56d8-1dcf-4cee-b167-84a8d35c5939
📒 Files selected for processing (3)
src/runtime/webcore/ByteStream.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/web/fetch/fetch-backpressure.test.ts
There was a problem hiding this comment.
No bugs found this pass; the earlier test-diagnostic nit was addressed in 33bc172. Deferring to a human because the core change is a timing shift in the ByteStream↔FetchTasklet backpressure handshake plus a lifecycle change to response_buffer (which the HTTP client holds via *mut MutableString) — small diff, but hot-path streaming state machine across two threads.
What was checked and ruled out:
- Skipping
signal_drained()on spill can't deadlock:on_pullre-signals oncebufferempties, and if the reader stalls that's the intended pause. mem::take(&mut response_buffer.list)is safe w.r.t. the*mut MutableStringhanded toAsyncHTTP::init— the client holds a pointer to the struct, not the Vec, and.reset()afterward is a harmless no-op on the fresh empty Vec.- Dropping
scheduled_response_buffercapacity in theon_body_receivedscopeguard doesn't regress the non-streaming path — that path setsbuffer_reset = falseso the guard doesn't fire.
Extended reasoning...
Overview
Three files: ByteStream::on_data moves the signal_drained() call into the !has_remaining branch so the producer socket stays paused while the spill tail sits in buffer; FetchTasklet::callback moves (rather than copies) response_buffer.list into scheduled_response_buffer when the latter is empty, and the on_body_received scopeguard now drops the staging allocation instead of retaining capacity; two new tests in fetch-backpressure.test.ts (a Readable.fromWeb stalled-pipe drain test on all lanes, and a release-Linux-only RSS bound for the download-proxy shape).
Security risks
None. This is memory-window / backpressure timing on the fetch receive path. No auth, no parsing of untrusted structure, no new user-facing surface.
Level of scrutiny
High. The diff is small but it changes when the HTTP-thread socket is resumed relative to the JS-thread reader, and it changes the allocation lifecycle of a buffer that the HTTP client references through a raw *mut MutableString. A missed edge in the resume gate would manifest as a hang under specific pull-view sizing, and the RSS regression test only runs on release Linux — every other lane relies on the drain test alone. Someone who owns the fetch backpressure state machine should confirm the has_remaining/is_really_done interaction (last chunk larger than the pull view still sets done before the tail is appended — pre-existing, but worth a glance now that resume ordering changed around it).
Other factors
- I traced
signal_drained()throughon_pull: whenhas_remainingskips the resume, the next pull drainsbufferand callssignal_drained()once it empties, so the producer is resumed exactly when the reader catches up. If the reader never pulls again, the socket stays paused — which is the intended backpressure, not a stall. MutableStringis{ list: Vec<u8> }with#[derive(Default)], somem::takeon.listleaves a valid empty Vec behind for the next recv; the subsequentresponse_buffer.reset()at line 2624 becomes a no-op. The HTTP client's stored*mut MutableStringpoints at the struct, not the Vec allocation, so swapping the inner Vec is sound.- The
on_body_receivedscopeguard change (drop vs.reset()) is guarded bybuffer_reset, which the non-streaming accumulate path (line ~914) already clears — so BufferAll still accumulates without re-allocating each cycle. The comment's claim that the retained capacity is never reused in the streaming path checks out: after the guard runs the nextcallbackalways findsscheduledempty and takes the move branch. - Previous review round: I flagged both new tests for swallowing failure paths; 33bc172 fixed both (pipeline error captured via
.then(()=>null, e=>e), subprocess client resolves oncloseand reports{short: N}). Those threads are resolved and the current diff reflects the fix. - The 225 MB RSS threshold sits between the stated pre-fix (~247–272 MB) and post-fix (~158–187 MB) bands with reasonable margin, and is correctly gated off ASAN/debug/non-Linux. Whether it's stable across the CI Linux fleet is a CI question, not a code-review one.
|
@robobun fix conflicts |
ByteStream.on_data was calling signal_drained() after filling a pending pull view even when the delivered chunk overflowed into the ByteStream buffer. That let the HTTP-thread socket resume with data still queued; the next recv landed with no reader waiting and went straight to ByteStream.buffer, and the buffer grew past one recv and kept that capacity for the rest of the fetch. Gating signal_drained on an empty buffer makes on_pull the sole resume point when there is overflow, so ByteStream.buffer tops out at one recv. bench/fetch/streaming-backpressure.mjs download-proxy, 50 x 64 MB: before: 277 MB peak / 4.13 s after: 226 MB peak / 3.92 s node: 204 MB peak / 5.80 s
33bc172 to
7ad4e38
Compare
|
Rebased onto main. The |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/ByteStream.rs (1)
399-406: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse
chunk.len()for the completion check.to_copy_len <= pending_buffer_lenis always true. When the final chunk exceeds the pending view,IntoArrayAndDonecloses the controller while the remaining bytes stay inself.buffer, truncating the response. Replace the check withchunk.len() <= pending_buffer_len.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/webcore/ByteStream.rs` around lines 399 - 406, Update the is_really_done completion check in ByteStream’s chunk-processing logic to compare chunk.len() with pending_buffer_len instead of to_copy_len, ensuring final chunks larger than the pending view remain buffered rather than truncating the response.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runtime/webcore/ByteStream.rs`:
- Around line 399-406: Update the is_really_done completion check in
ByteStream’s chunk-processing logic to compare chunk.len() with
pending_buffer_len instead of to_copy_len, ensuring final chunks larger than the
pending view remain buffered rather than truncating the response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c3aeadb-d26f-4a9d-93fd-8fe85f9aef97
📒 Files selected for processing (2)
src/runtime/webcore/ByteStream.rstest/js/web/fetch/fetch-backpressure.test.ts
|
Re: the coderabbit finding at |
There was a problem hiding this comment.
No issues found after the fix-up — the earlier test-diagnostic nit is addressed. Deferring to a human for the final call: the native change is one line of flow-control in ByteStream::on_data on the fetch body hot path, and the new release-Linux RSS bound (225 MB vs. ~197 measured here / ~241 on main) is a CI-gating threshold worth a maintainer sanity-check against the fleet.
What was reviewed:
- Traced that
on_pullalready callssignal_drained()oncebufferempties, so withholding the signal on spill cannot deadlock — the new drain test and the pre-existing per-kind stalled-reader tests exercise that resume path. - Checked the
is_really_done && has_remainingedge (last chunk larger than the pull view): the producer is already done there, so skippingsignal_drained()is harmless; pre-existing behavior on that path is unchanged. - The sink and
buffer_actionbranches return before the changed block, so buffered consumers and native-sink piping are unaffected.
Extended reasoning...
Overview
After rebase this PR is a single behavioral change in src/runtime/webcore/ByteStream.rs: on_data now calls signal_drained() only when the incoming chunk fit entirely in the pending pull view; when the tail spilled into buffer, resume is deferred to the next on_pull (which already signals once the buffer empties). Two tests are added to test/js/web/fetch/fetch-backpressure.test.ts: a Readable.fromWeb stalled-Writable stall→drain test (all lanes) and a release-Linux-only RSS bound for the download-proxy shape.
Security risks
None. This is receive-side flow-control timing; no parsing of untrusted input, no auth/crypto, no new user-facing surface.
Level of scrutiny
Medium-high. The diff is tiny, but it sits in the fetch response-body streaming path that every res.body consumer runs through. Flow-control changes here can hang real workloads if a resume edge is missed. I verified the compensating edge exists (on_pull → signal_drained() when buffer empties) and that the sink / buffer_action branches are untouched, and the drain test asserts full-body delivery after a stall. The deadlock risk looks covered, but a maintainer familiar with SourceHandle::FetchResponseBody::on_ready and the H2/H3 producer state machines should confirm nothing else depends on the old unconditional signal.
Other factors
- My earlier nit (swallowed failure paths in both new tests) was addressed in 33bc172: pipeline errors now reach the assertion via
.then(() => null, e => e), and the subprocess client resolves oncloseand reportsshort. - CodeRabbit's payload-size suggestion was withdrawn after robobun explained the 1 GiB body matches the existing stalled-reader tests' loopback-autotuning requirement.
- The 225 MB RSS threshold sits between the measured ~158-197 MB (fixed) and ~247-272 MB (unfixed) bands with reasonable margin, but it's Linux-loopback-tuned and skipped on ASAN/debug — a maintainer should confirm it's comfortable across the Linux release lanes before it gates CI.
- Jarred is already on the thread (requested the rebase) but hasn't reviewed the substance yet.
|
CI build 86496: 195/196 passed. The one red lane is |
JSS3File: BUN__createJSS3File has zero callers (born dead in #36588; only BUN__createJSS3FileUnsafely is wired to Rust). Remove it, constructS3File, the JSS3File__construct extern decl, and the Rust construct/construct_internal chain plus its now-unused PathLikeExt import. InspectorBunFrontendDevServerAgent.cpp: drop orphaned m_globalobject initializer comment and redundant UNUSED_PARAM (parameter is used). RequestContext.rs: drop stale TODO referencing the deleted InlineBlob type. Remove commented-out #include "JSDOMWindow.h" from the 5 remaining sibling files (DOMWrapperWorld, JSDOMWrapper, JSErrorHandler, JSDOMPromise, JSDOMPromiseDeferred). source-lints: add \b anchor so BunFrontendDevServerAgent__notify doesn't match the live InspectorBunFrontendDevServerAgent__notify* wrappers; broaden RequestContext/S3File guards to cover the new deletions.
2 participants
Shrinks the
node:httpproxy memory window (pipeline(Readable.fromWeb((await fetch(upstream)).body), res)) by keeping the upstream socket paused whileByteStreamstill has spilled bytes.Repro
df49a6e1c, after #36570)At
CONCURRENCY=40 MB=32(the shape the RSS test uses): main 241 MB, this PR 197 MB.Cause
ByteStream::on_datacalledsignal_drained()after filling a pending pull view even when the tail of the chunk spilled intobuffer. That resumed the upstream socket with data still queued; the next recv landed with no reader waiting (pipelinewas paused onres.write() == false) and went straight toByteStream.buffer. Instrumented on main at C=50,on_datahit the no-pending-pull append path 1686 times; with this change, zero.FetchTasklet.scheduled_response_buffergrowth from the same pattern was already capped by #36570 (which drops the staging allocation once it passesDECODED_BODY_RETAIN_CAP), so theByteStream.bufferoverflow is what remains.Fix
on_dataonly callssignal_drained()when the whole chunk fit the pull view. When it spilled, the nexton_pullsignals once it drainsbuffer, so the producer stays paused until the reader has caught up.Tests
test/js/web/fetch/fetch-backpressure.test.tsgains aReadable.fromWebstalled-pipe drain test (all lanes) and a release-Linux-only RSS bound for thedownload-proxyshape (fails on main with peak ~241 MB, passes here at ~197 MB). ASAN quarantine dominates the per-conn buffer delta, so the RSS bound is skipped under debug/ASAN; the drain test covers the path there.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch-backpressure.test.ts