Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
253d892
fetch: reserve Content-Length on the buffered-body handoff so arrayBu…
robobun Jul 31, 2026
995e917
test: import node:net at module scope
robobun Jul 31, 2026
4ca3599
tighten comments
robobun Jul 31, 2026
54ab69a
gate the Content-Length reserve on BufferAll mode
robobun Jul 31, 2026
a8b1e0a
gate the Content-Length reserve on on_start_buffering, not BufferAll …
robobun Jul 31, 2026
51103b4
clear is_buffering_body when a ByteStream attaches
robobun Jul 31, 2026
b8c5b61
AsyncHTTP: drop response_buffer field + init/init_sync param
robobun Jul 31, 2026
5302034
InternalState: replace body_out_str with owned decoded_body
robobun Jul 31, 2026
62fb870
s3: S3HttpSimpleTask reads response_buffer, extends in http_callback
robobun Jul 31, 2026
df2cb07
FetchTasklet: drop response_buffer; read result.body as &[u8]
robobun Jul 31, 2026
df3e7fd
http/lib: own decoded_body, deliver &[u8] in progress callback
robobun Jul 31, 2026
2f17b0f
NetworkTask.notify: read result.body slice, accumulate locally
robobun Jul 31, 2026
df0accc
s3/download_stream: result.body is &[u8]; drop response_buffer field
robobun Jul 31, 2026
314db32
RemoteImageDownload: extend response_buffer from result.body; drop in…
robobun Jul 31, 2026
3cf915d
s3/client.rs: drop response_buffer arg from AsyncHTTP::init calls
robobun Jul 31, 2026
0881b6d
http: send_sync takes &mut MutableString; channel appends body
robobun Jul 31, 2026
4bb2ac0
s3 download_stream: accumulate body on every callback
robobun Jul 31, 2026
10b0708
http: lift decoded_body to stack before terminal callback
robobun Jul 31, 2026
67b4da8
send_sync: set response_buffer before boxing, drop unsafe write
robobun Jul 31, 2026
ff882dc
NetworkTask::notify: reset response_buffer on new-attempt metadata
robobun Jul 31, 2026
636c841
FetchTasklet: clear is_buffering_body under mutex on stream attach
robobun Jul 31, 2026
5170259
test(fetch): trim long-redirect loop under debug/ASAN
robobun Jul 31, 2026
2f5f428
abort-signal-leak: scale iterations on debug to fit 5s budget
robobun Jul 31, 2026
c3f621e
fetch-leak fixture: cache the 2MB string for URLSearchParams
robobun Jul 31, 2026
23aaf8e
fetch-tcp-stress: scale iterations on debug/ASAN to fit 30s budget
robobun Jul 31, 2026
8214d20
fetch-leak: cut ITERATIONS to 20 on debug/ASAN for URLSearchParams
robobun Jul 31, 2026
7157194
revert unrelated test-timing tweaks (scope creep; pre-existing on main)
robobun Jul 31, 2026
cf1f11f
FetchTasklet: release scheduled_response_buffer capacity above 512K o…
robobun Jul 31, 2026
c6d6a5a
s3/download_stream: handle_oom on write; name DECODED_BODY_RETAIN_CAP
robobun Jul 31, 2026
48b9d2a
http: carry owned body_owned Vec on terminal callback for zero-copy h…
robobun Jul 31, 2026
7ba280e
s3/simple_request: add SAFETY comment for detach_lifetime (clippy)
robobun Jul 31, 2026
637d54b
FetchTasklet: adopt body_owned before reserving to avoid transient 2x…
robobun Jul 31, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 47 additions & 7 deletions src/runtime/webcore/fetch/FetchTasklet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@

bun_output::declare_scope!(FetchTasklet, visible);

/// Upper bound on the Content-Length-driven `reserve_exact` in `callback()`.
const SCHEDULED_PRERESERVE_MAX: usize = 256 * 1024 * 1024;

use http::signals::BodyReceiveMode;

#[derive(bun_ptr::ThreadSafeRefCounted)]
Expand Down Expand Up @@ -115,6 +118,12 @@
// Custom Hostname
pub(crate) hostname: Option<Box<[u8]>>,
pub(crate) is_waiting_body: bool,
/// 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.
Comment thread
robobun marked this conversation as resolved.
pub(crate) is_buffering_body: AtomicBool,
pub(crate) is_waiting_abort: bool,
pub(crate) is_waiting_request_stream_start: bool,
pub(crate) mutex: Mutex,
Expand Down Expand Up @@ -1703,6 +1712,9 @@
) {
let this = Self::from_ctx(ctx);
this.readable_stream_ref = ReadableStreamStrong::init(readable, global_this);
// A ByteStream now drains scheduled_response_buffer per chunk; undo any
// buffered-consumer reservation request so callback() stops growing it.
Comment thread
robobun marked this conversation as resolved.
this.is_buffering_body.store(false, Ordering::Release);

Check failure on line 1717 in src/runtime/webcore/fetch/FetchTasklet.rs

View check run for this annotation

Claude / Claude Code Review

is_buffering_body clear in on_readable_stream_available races with HTTP-thread callback()

Clearing `is_buffering_body` in `on_readable_stream_available` doesn't close the ValueBufferer race — that store runs on the JS thread *without* the mutex, strictly after `on_start_streaming_http_response_body_callback` has already unlocked (line 1757/1765) and after the JS-thread work at Body.rs:872-910. The HTTP thread (already woken by `schedule_receive_resume()` at line 1746) can win the mutex in `callback()` in that window, see `is_buffering_body == true` with `scheduled.capacity() == 0`, a
Comment thread
claude[bot] marked this conversation as resolved.
}

fn on_start_streaming_http_response_body_callback(ctx: *mut c_void) -> DrainResult {
Expand Down Expand Up @@ -1805,6 +1817,7 @@
fn on_start_buffering_callback(ctx: *mut c_void) {
let this = Self::from_ctx(ctx);
this.poll_ref.ref_(bun_io::js_vm_ctx());
this.is_buffering_body.store(true, Ordering::Release);
if this
.signal_store
.set_receive_mode_terminal(BodyReceiveMode::BufferAll)
Expand Down Expand Up @@ -2032,6 +2045,7 @@
upgraded_connection: fetch_options.upgraded_connection,
hostname: fetch_options.hostname,
is_waiting_body: false,
is_buffering_body: AtomicBool::new(false),
is_waiting_abort: false,
is_waiting_request_stream_start: false,
mutex: Mutex::new(),
Expand Down Expand Up @@ -2595,20 +2609,46 @@
}
} else {
if success {
bun_core::handle_oom(
task_ref
.scheduled_response_buffer
.write(task_ref.response_buffer.list.as_slice()),
);
let scheduled = &mut task_ref.scheduled_response_buffer;
let incoming = &mut task_ref.response_buffer;
if scheduled.list.capacity() == 0 {
// `body_out_str` aliases the field, not the Vec's heap
// pointer (asserted above), so swapping here is invisible
// to the HTTP client.
Comment thread
robobun marked this conversation as resolved.
Outdated
core::mem::swap(scheduled, incoming);
}
// 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.
Comment thread
robobun marked this conversation as resolved.
if task_ref.is_buffering_body.load(Ordering::Acquire) {
if let http::BodySize::ContentLength(n) = task_ref.body_size {
if n > scheduled.list.capacity() {
let additional = n
.min(SCHEDULED_PRERESERVE_MAX)
.saturating_sub(scheduled.list.len());
let _ = scheduled.list.try_reserve_exact(additional);
}
}
}
Comment thread
robobun marked this conversation as resolved.
if !incoming.list.is_empty() {
bun_core::handle_oom(scheduled.write(incoming.list.as_slice()));
}
if task_ref.result.has_more && !task_ref.scheduled_response_buffer.list.is_empty() {
let _ = task_ref.signal_store.try_transition_receive_mode(
BodyReceiveMode::AutoPause,
BodyReceiveMode::Paused,
);
}
}
// reset for reuse
task_ref.response_buffer.reset();
if task_ref.result.has_more {
// reset for reuse
task_ref.response_buffer.reset();
} else {
task_ref.response_buffer = MutableString::default();
}
}

if let Err(has_schedule_callback) = task_ref.has_schedule_callback.compare_exchange(
Expand Down
42 changes: 42 additions & 0 deletions test/js/web/fetch/fetch-buffer-peak-fixture.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

64 changes: 64 additions & 0 deletions test/js/web/fetch/fetch-leak.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test";
import { bunEnv, bunExe, tls as COMMON_CERT, gc, isASAN, isCI, isDebug } from "harness";
import { once } from "node:events";
import { createServer } from "node:http";
import net from "node:net";
import { join } from "node:path";

describe("fetch doesn't leak", () => {
Expand Down Expand Up @@ -962,3 +963,66 @@ test("aborting in-flight streaming fetch() responses does not retain the buffere
expect(stderr).not.toContain("LEAK");
expect(exitCode).toBe(0);
});

test("fetch().arrayBuffer() of a large Content-Length body peaks at ~1x the payload", async () => {
// The per-packet handoff in FetchTasklet::callback appended each socket read
// into scheduled_response_buffer via extend_from_slice, so the Vec grew by
// amortized doubling. For a body just past a doubling step that left the
// allocation the ArrayBuffer adopts at ~2x the payload, and the intermediate
// reallocations spiked ru_maxrss well past 2x. Reserving Content-Length
// exactly up front keeps it to a single allocation that is moved through to
// the ArrayBuffer.
//
// Body size is 128 MiB + 1 MiB so the old doubling growth would have crossed
// the 128 -> 256 step, making the unfixed peak reliably > 2x body.
const bodyBytes = 129 * 1024 * 1024;
const chunk = Buffer.alloc(256 * 1024, "abcdefghij");
const server = net.createServer(socket => {
socket.once("data", () => {
socket.write(`HTTP/1.1 200 OK\r\nContent-Length: ${bodyBytes}\r\nConnection: close\r\n\r\n`);
let sent = 0;
const pump = () => {
while (sent < bodyBytes) {
const n = Math.min(chunk.length, bodyBytes - sent);
sent += n;
if (!socket.write(n === chunk.length ? chunk : chunk.subarray(0, n))) {
socket.once("drain", pump);
return;
}
}
socket.end();
};
pump();
});
socket.on("error", () => {});
});
await once(server.listen(0, "127.0.0.1"), "listening");
const { port } = server.address();

try {
await using proc = Bun.spawn({
cmd: [bunExe(), "--smol", join(import.meta.dir, "fetch-buffer-peak-fixture.ts")],
env: {
...bunEnv,
SERVER: `http://127.0.0.1:${port}/`,
BODY_BYTES: String(bodyBytes),
},
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
console.log(stdout.trim());
expect(stderr).toBe("");
const { bodyMB, rssBeforeMB, rssAfterMB } = JSON.parse(stdout.trim());
expect(bodyMB).toBe(129);
// Unfixed: the doubling reallocations (retained by ASAN quarantine / mimalloc
// page cache) leave RSS at >= 2x body over the pre-fetch resident
// set (release linux ~2.9x, debug+ASAN default quarantine ~2.7x).
// Fixed: a single exact allocation, ~1.0x body.
const delta = rssAfterMB - rssBeforeMB;
expect(delta).toBeLessThan(1.5 * bodyMB);
expect(exitCode).toBe(0);
} finally {
server.close();
}
}, 60000);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Loading