From 51e6126ed196ae6461673a48345fa961766e2a36 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 21 Jul 2026 19:42:28 +0000 Subject: [PATCH 1/7] s3: free the NetworkSink behind writer() via intrusive refcount Every s3.file(k).writer() leaked one NetworkSink. Two owners hold a raw pointer to it (the JSNetworkSink wrapper's m_sinkPtr and the MultiPartUpload.callback_context) and both release paths routed through NetworkSink::finalize(), which only detached the upload task. finalize_and_destroy() existed but had no callers. Give NetworkSink a CellRefCounted refcount (rc=1 from Default, +1 for callback_context in writable_stream). finalize() now also derefs; abort() stops at detach_writable() so wrapper_callback's failure path does not double-deref before its own trailing finalize(). --- src/runtime/webcore/s3/client.rs | 5 ++ src/runtime/webcore/streams.rs | 26 ++++----- test/js/bun/s3/s3-networksink-leak.test.ts | 68 ++++++++++++++++++++++ 3 files changed, 84 insertions(+), 15 deletions(-) create mode 100644 test/js/bun/s3/s3-networksink-leak.test.ts diff --git a/src/runtime/webcore/s3/client.rs b/src/runtime/webcore/s3/client.rs index b63d9a883161..af25951b571f 100644 --- a/src/runtime/webcore/s3/client.rs +++ b/src/runtime/webcore/s3/client.rs @@ -544,6 +544,11 @@ pub(crate) fn writable_stream( ..Default::default() })); + // +1 for `callback_context` below; released by `wrapper_callback`'s trailing + // `sink.finalize()`. The JS wrapper's +1 is the `Default` rc=1, released by + // `NetworkSink__finalize` when the wrapper is swept. + // SAFETY: freshly heap-allocated; exclusive access here. + unsafe { &*response_stream }.ref_(); task.callback_context = response_stream.cast::(); task.on_writable = Some(on_writable_thunk); diff --git a/src/runtime/webcore/streams.rs b/src/runtime/webcore/streams.rs index 5ed896085c55..dbb79cf385f1 100644 --- a/src/runtime/webcore/streams.rs +++ b/src/runtime/webcore/streams.rs @@ -2133,7 +2133,13 @@ pub type H3ResponseSink = HTTPServerWritable; // NetworkSink // ────────────────────────────────────────────────────────────────────────── +// Two owners hold a raw `*mut NetworkSink`: the `JSNetworkSink` wrapper +// (`m_sinkPtr`, released via the generated `__finalize`) and the +// `MultiPartUpload.callback_context` (released in `wrapper_callback`). Both +// routes end in `finalize()`, so the allocation is freed by intrusive refcount. +#[derive(bun_ptr::CellRefCounted)] pub struct NetworkSink { + pub ref_count: core::cell::Cell, // Stored as `BackRef` // (set-once); while `Some` the sink holds a counted ref on the intrusively // ref-counted `MultiPartUpload`, released in `detach_writable`. @@ -2152,6 +2158,7 @@ pub struct NetworkSink { impl Default for NetworkSink { fn default() -> Self { Self { + ref_count: core::cell::Cell::new(1), task: None, signal: Signal::default(), global_this: None, @@ -2241,6 +2248,9 @@ impl NetworkSink { pub fn finalize(&mut self) { self.detach_writable(); + // SAFETY: `&mut self` carries write provenance over the whole + // allocation; this is the last use of `self`. + unsafe { NetworkSink::deref(core::ptr::from_mut::(self)) }; } fn detach_writable(&mut self) { @@ -2305,26 +2315,12 @@ impl NetworkSink { )) } - /// # Safety - /// `this` must be a valid, uniquely-owned heap pointer to `Self` produced - /// by `bun_core::heap::into_raw`; the caller transfers ownership. - // Forwards `this` to `bun_core::heap::take` without dereferencing it here; - // not_unsafe_ptr_arg_deref is a false positive on opaque-token forwarding. - #[allow(clippy::not_unsafe_ptr_arg_deref)] - pub fn finalize_and_destroy(this: *mut Self) { - // SAFETY: this was heap-allocated; reclaim sole ownership before - // touching fields so no `&mut *this` is live alongside the Box. - let mut this = unsafe { bun_core::heap::take(this) }; - this.finalize(); - drop(this); - } - pub fn abort(&mut self) { self.ended = true; self.done = true; self.signal.close(None); self.cancel = true; - self.finalize(); + self.detach_writable(); } pub fn write(&mut self, data: &StreamResult) -> Writable { diff --git a/test/js/bun/s3/s3-networksink-leak.test.ts b/test/js/bun/s3/s3-networksink-leak.test.ts new file mode 100644 index 000000000000..80a7ab361932 --- /dev/null +++ b/test/js/bun/s3/s3-networksink-leak.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, isASAN } from "harness"; + +// The NetworkSink behind s3.file(k).writer() is heap-allocated and has two +// owners: the JS wrapper (m_sinkPtr) and the MultiPartUpload's callback_context. +// Both release paths routed through NetworkSink::finalize(), which only +// detached the upload task and never reclaimed the Box. Every writer() leaked +// one ~80-byte NetworkSink on both the success and failure completion paths. +// +// This test spawns two subprocesses under detect_leaks=1 with N and N+20 +// writers and asserts the extra 20 writers do not add a proportional number of +// leaked bytes. symbolize=0 keeps each run under a second; one-time at-exit +// allocations are constant between the two runs and cancel out in the diff. + +async function runWriters(count: number, fail: boolean) { + const script = ` + using server = Bun.serve({ + port: 0, + async fetch(req) { + await req.arrayBuffer(); + ${ + fail + ? `return new Response( + 'AccessDeniednope', + { status: 403 }, + );` + : `return new Response("", { status: 200, headers: { etag: '"e"' } });` + } + }, + }); + const s3 = new Bun.S3Client({ + accessKeyId: "k", + secretAccessKey: "s", + bucket: "b", + endpoint: \`http://127.0.0.1:\${server.port}\`, + }); + for (let i = 0; i < ${count}; i++) { + const w = s3.file("key-" + i, { retry: 0 }).writer(); + w.write("hello"); + try { await w.end(); } catch {} + } + Bun.gc(true); + console.log("done"); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: { + ...bunEnv, + ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1", "symbolize=0"].filter(Boolean).join(":"), + }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout.trim()).toBe("done"); + return Number(/SUMMARY: AddressSanitizer: (\d+) byte\(s\) leaked/.exec(stderr)?.[1] ?? 0); +} + +describe.skipIf(!isASAN)("S3 writer() NetworkSink is freed", () => { + for (const fail of [true, false]) { + test.concurrent(`when the upload ${fail ? "fails" : "succeeds"}`, async () => { + const small = await runWriters(2, fail); + const large = await runWriters(22, fail); + // Before the fix the diff is exactly 20 * sizeof(NetworkSink) = 1600. + expect(large - small).toBeLessThan(400); + }); + } +}); From abccf8d3d214bbd65a675779726e4648da530b5a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 21 Jul 2026 20:38:31 +0000 Subject: [PATCH 2/7] release the wrapper's ref in __doClose; address review feedback __doClose nulls m_sinkPtr before ~JSSink runs, so the destructor skipped __finalize and the wrapper's intrusive ref on NetworkSink (and ArrayBufferSink / FileSink) was never released on the .close() path. __doClose now calls __finalize(ptr) after __close(ptr). FileSink::finalize no longer clears self.pending: .close() can now reach finalize while a backpressured write promise is still outstanding, and run_pending settles it once the writer drains. deinit() drops the field at rc=0. Also: set ref_count=2 in the NetworkSink initializer (matches the other two-owner allocations in client.rs), strip http_proxy/HTTPS_PROXY from the leak test's env, and move {retry: 0} onto .writer() where it is actually read. --- src/codegen/generate-jssink.ts | 3 ++ src/runtime/webcore/FileSink.rs | 6 +++- src/runtime/webcore/s3/client.rs | 6 +--- test/js/bun/s3/s3-networksink-leak.test.ts | 35 ++++++++++++++-------- 4 files changed, 31 insertions(+), 19 deletions(-) diff --git a/src/codegen/generate-jssink.ts b/src/codegen/generate-jssink.ts index dea2a643525c..815f713155aa 100644 --- a/src/codegen/generate-jssink.ts +++ b/src/codegen/generate-jssink.ts @@ -513,6 +513,9 @@ JSC_DEFINE_HOST_FUNCTION(${name}__doClose, (JSC::JSGlobalObject * lexicalGlobalO sink->detach(); RETURN_IF_EXCEPTION(scope, {}); ${name}__close(lexicalGlobalObject, ptr); + // detach() nulled m_sinkPtr so ~${className} will not reach __finalize; + // release the wrapper's ref here instead. + ${name}__finalize(ptr); return JSC::JSValue::encode(JSC::jsUndefined()); } diff --git a/src/runtime/webcore/FileSink.rs b/src/runtime/webcore/FileSink.rs index 308099ed6ac9..9425c99bc6fc 100644 --- a/src/runtime/webcore/FileSink.rs +++ b/src/runtime/webcore/FileSink.rs @@ -932,8 +932,12 @@ impl FileSink { // `ref_()` there. Callers that allocate via `init`/`create` and then // `to_js()` must `deref()` once to release init's +1 (see // `Blob::get_writer`). + // + // `pending` is NOT cleared here: `.close()` reaches `finalize` via + // `doClose` while a backpressured write may still be awaiting its + // promise; `run_pending` settles it when the writer drains, and + // `deinit()` drops the field once the refcount hits zero. self.readable_stream.set(readable_stream::Strong::default()); - self.pending.set(streams::WritablePending::default()); self.js_sink_ref.with_mut(|r| r.deinit()); // SAFETY: `&mut self` carries write provenance over the whole // allocation; this is the last use of `self` in `finalize`. diff --git a/src/runtime/webcore/s3/client.rs b/src/runtime/webcore/s3/client.rs index af25951b571f..233b2f3f8803 100644 --- a/src/runtime/webcore/s3/client.rs +++ b/src/runtime/webcore/s3/client.rs @@ -538,17 +538,13 @@ pub(crate) fn writable_stream( // compatible (`{ sink: NetworkSink }`) so the cast in `to_sink()` is just a pointer reinterpret. let response_stream: *mut NetworkSink = bun_core::heap::into_raw(NetworkSink::new(NetworkSink { + ref_count: core::cell::Cell::new(2), // +1 for callback_context task: NonNull::new(task_ptr).map(bun_ptr::BackRef::from), global_this: Some(bun_ptr::BackRef::new(global_this)), high_water_mark: part_size as BlobSizeType, ..Default::default() })); - // +1 for `callback_context` below; released by `wrapper_callback`'s trailing - // `sink.finalize()`. The JS wrapper's +1 is the `Default` rc=1, released by - // `NetworkSink__finalize` when the wrapper is swept. - // SAFETY: freshly heap-allocated; exclusive access here. - unsafe { &*response_stream }.ref_(); task.callback_context = response_stream.cast::(); task.on_writable = Some(on_writable_thunk); diff --git a/test/js/bun/s3/s3-networksink-leak.test.ts b/test/js/bun/s3/s3-networksink-leak.test.ts index 80a7ab361932..a72b48584f97 100644 --- a/test/js/bun/s3/s3-networksink-leak.test.ts +++ b/test/js/bun/s3/s3-networksink-leak.test.ts @@ -5,16 +5,17 @@ import { bunEnv, bunExe, isASAN } from "harness"; // owners: the JS wrapper (m_sinkPtr) and the MultiPartUpload's callback_context. // Both release paths routed through NetworkSink::finalize(), which only // detached the upload task and never reclaimed the Box. Every writer() leaked -// one ~80-byte NetworkSink on both the success and failure completion paths. +// one ~80-byte NetworkSink on both the success and failure completion paths, +// whether finished via .end() or .close(). // // This test spawns two subprocesses under detect_leaks=1 with N and N+20 // writers and asserts the extra 20 writers do not add a proportional number of // leaked bytes. symbolize=0 keeps each run under a second; one-time at-exit // allocations are constant between the two runs and cancel out in the diff. -async function runWriters(count: number, fail: boolean) { +async function runWriters(count: number, fail: boolean, finish: "end" | "close") { const script = ` - using server = Bun.serve({ + const server = Bun.serve({ port: 0, async fetch(req) { await req.arrayBuffer(); @@ -28,25 +29,31 @@ async function runWriters(count: number, fail: boolean) { } }, }); + server.unref(); const s3 = new Bun.S3Client({ accessKeyId: "k", secretAccessKey: "s", bucket: "b", endpoint: \`http://127.0.0.1:\${server.port}\`, }); + process.once("beforeExit", () => { Bun.gc(true); console.log("done"); }); for (let i = 0; i < ${count}; i++) { - const w = s3.file("key-" + i, { retry: 0 }).writer(); + const w = s3.file("key-" + i).writer({ retry: 0 }); w.write("hello"); - try { await w.end(); } catch {} + ${finish === "end" ? `try { await w.end(); } catch {}` : `w.close();`} } - Bun.gc(true); - console.log("done"); `; await using proc = Bun.spawn({ cmd: [bunExe(), "-e", script], env: { ...bunEnv, ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1", "symbolize=0"].filter(Boolean).join(":"), + // The S3 client does not honor NO_PROXY for writer(), so an inherited + // proxy would hijack the request to the in-process mock server. + http_proxy: undefined, + HTTP_PROXY: undefined, + https_proxy: undefined, + HTTPS_PROXY: undefined, }, stdout: "pipe", stderr: "pipe", @@ -58,11 +65,13 @@ async function runWriters(count: number, fail: boolean) { describe.skipIf(!isASAN)("S3 writer() NetworkSink is freed", () => { for (const fail of [true, false]) { - test.concurrent(`when the upload ${fail ? "fails" : "succeeds"}`, async () => { - const small = await runWriters(2, fail); - const large = await runWriters(22, fail); - // Before the fix the diff is exactly 20 * sizeof(NetworkSink) = 1600. - expect(large - small).toBeLessThan(400); - }); + for (const finish of ["end", "close"] as const) { + test.concurrent(`via .${finish}() when the upload ${fail ? "fails" : "succeeds"}`, async () => { + const small = await runWriters(2, fail, finish); + const large = await runWriters(22, fail, finish); + // Before the fix the diff is >= 20 * sizeof(NetworkSink) ~= 1600. + expect(large - small).toBeLessThan(400); + }); + } } }); From b7a1ae25bd646526deac1c353702c79cd114f0af Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 21 Jul 2026 20:45:59 +0000 Subject: [PATCH 3/7] shorten code comments to three lines --- src/runtime/webcore/FileSink.rs | 6 ++---- src/runtime/webcore/streams.rs | 7 +++---- test/js/bun/s3/s3-networksink-leak.test.ts | 15 +++------------ 3 files changed, 8 insertions(+), 20 deletions(-) diff --git a/src/runtime/webcore/FileSink.rs b/src/runtime/webcore/FileSink.rs index 9425c99bc6fc..4792069bbacd 100644 --- a/src/runtime/webcore/FileSink.rs +++ b/src/runtime/webcore/FileSink.rs @@ -933,10 +933,8 @@ impl FileSink { // `to_js()` must `deref()` once to release init's +1 (see // `Blob::get_writer`). // - // `pending` is NOT cleared here: `.close()` reaches `finalize` via - // `doClose` while a backpressured write may still be awaiting its - // promise; `run_pending` settles it when the writer drains, and - // `deinit()` drops the field once the refcount hits zero. + // `pending` is left for `run_pending`/`deinit()`: `.close()` reaches + // here via `doClose` while a backpressured write may still be awaited. self.readable_stream.set(readable_stream::Strong::default()); self.js_sink_ref.with_mut(|r| r.deinit()); // SAFETY: `&mut self` carries write provenance over the whole diff --git a/src/runtime/webcore/streams.rs b/src/runtime/webcore/streams.rs index dbb79cf385f1..1b2b66c9de4f 100644 --- a/src/runtime/webcore/streams.rs +++ b/src/runtime/webcore/streams.rs @@ -2133,10 +2133,9 @@ pub type H3ResponseSink = HTTPServerWritable; // NetworkSink // ────────────────────────────────────────────────────────────────────────── -// Two owners hold a raw `*mut NetworkSink`: the `JSNetworkSink` wrapper -// (`m_sinkPtr`, released via the generated `__finalize`) and the -// `MultiPartUpload.callback_context` (released in `wrapper_callback`). Both -// routes end in `finalize()`, so the allocation is freed by intrusive refcount. +// Two intrusive-rc owners: the `JSNetworkSink` wrapper (`m_sinkPtr`, released +// via `__finalize`) and `MultiPartUpload.callback_context` (released in +// `wrapper_callback`); both call `finalize()`, which `deref()`s. #[derive(bun_ptr::CellRefCounted)] pub struct NetworkSink { pub ref_count: core::cell::Cell, diff --git a/test/js/bun/s3/s3-networksink-leak.test.ts b/test/js/bun/s3/s3-networksink-leak.test.ts index a72b48584f97..574131b4fb46 100644 --- a/test/js/bun/s3/s3-networksink-leak.test.ts +++ b/test/js/bun/s3/s3-networksink-leak.test.ts @@ -1,18 +1,9 @@ import { describe, expect, test } from "bun:test"; import { bunEnv, bunExe, isASAN } from "harness"; -// The NetworkSink behind s3.file(k).writer() is heap-allocated and has two -// owners: the JS wrapper (m_sinkPtr) and the MultiPartUpload's callback_context. -// Both release paths routed through NetworkSink::finalize(), which only -// detached the upload task and never reclaimed the Box. Every writer() leaked -// one ~80-byte NetworkSink on both the success and failure completion paths, -// whether finished via .end() or .close(). -// -// This test spawns two subprocesses under detect_leaks=1 with N and N+20 -// writers and asserts the extra 20 writers do not add a proportional number of -// leaked bytes. symbolize=0 keeps each run under a second; one-time at-exit -// allocations are constant between the two runs and cancel out in the diff. - +// LSAN byte-count diff between N and N+20 writers: symbolize=0 keeps each +// subprocess fast, and one-time at-exit allocations cancel out in the diff. +// https://github.com/oven-sh/bun/pull/34999 async function runWriters(count: number, fail: boolean, finish: "end" | "close") { const script = ` const server = Bun.serve({ From 3451a9fb19b6fff7cf05e1d63293004454bc12d5 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 21 Jul 2026 23:20:02 +0000 Subject: [PATCH 4/7] split .close() release from GC finalize; wrapper_callback derefs directly JsSinkType gains wrapper_detached() (called from the new __wrapperDetached extern in __doClose) so the .close() release path is distinct from the GC-sweep finalize(). FileSink::finalize is reverted to main's behaviour (clears pending) and factored through release_wrapper_ref(); wrapper_detached() calls only release_wrapper_ref() so a backpressured write promise survives .close(). NetworkSink::finalize is now a bare deref(); the sink's ref on the MultiPartUpload moves to deinit() (rc=0). wrapper_callback releases the task's +1 on the sink via NetworkSink::deref directly. --- src/codegen/generate-jssink.ts | 15 +++++++++++++-- src/runtime/webcore/FileSink.rs | 32 ++++++++++++++++++++------------ src/runtime/webcore/Sink.rs | 11 +++++++++++ src/runtime/webcore/s3/client.rs | 4 +++- src/runtime/webcore/streams.rs | 17 +++++++++++++---- 5 files changed, 60 insertions(+), 19 deletions(-) diff --git a/src/codegen/generate-jssink.ts b/src/codegen/generate-jssink.ts index 815f713155aa..f8238c533a69 100644 --- a/src/codegen/generate-jssink.ts +++ b/src/codegen/generate-jssink.ts @@ -285,6 +285,7 @@ using namespace JSC; ${classes.map(name => `extern "C" size_t ${name}__memoryCost(void* sinkPtr);`).join("\n")} ${classes.map(name => `extern "C" void ${name}__controllerDetached(void* sinkPtr, JSC::EncodedJSValue controllerValue);`).join("\n")} +${classes.map(name => `extern "C" void ${name}__wrapperDetached(void* sinkPtr);`).join("\n")} `; var templ = head; @@ -514,8 +515,8 @@ JSC_DEFINE_HOST_FUNCTION(${name}__doClose, (JSC::JSGlobalObject * lexicalGlobalO RETURN_IF_EXCEPTION(scope, {}); ${name}__close(lexicalGlobalObject, ptr); // detach() nulled m_sinkPtr so ~${className} will not reach __finalize; - // release the wrapper's ref here instead. - ${name}__finalize(ptr); + // release the wrapper's ref via a non-GC path. + ${name}__wrapperDetached(ptr); return JSC::JSValue::encode(JSC::jsUndefined()); } @@ -1118,6 +1119,16 @@ pub extern "C" fn ${name}__finalize(this: &mut ${name}) { ${JSSinkT}::js_finalize(this) } +`; + + // extern "C" void ${name}__wrapperDetached(void* sinkPtr) — called from + // ${name}__doClose after it nulls m_sinkPtr. C++ caller null-checks `ptr`. + symbols.push(`${name}__wrapperDetached`); + templ += `#[unsafe(no_mangle)] +pub extern "C" fn ${name}__wrapperDetached(this: &mut ${name}) { + ${JSSinkT}::js_wrapper_detached(this) +} + `; // extern "C" void ${name}__controllerDetached(void* sinkPtr, JSC::EncodedJSValue) diff --git a/src/runtime/webcore/FileSink.rs b/src/runtime/webcore/FileSink.rs index 4792069bbacd..e05d6322f524 100644 --- a/src/runtime/webcore/FileSink.rs +++ b/src/runtime/webcore/FileSink.rs @@ -924,21 +924,23 @@ impl FileSink { } } - // Per-wrapper accounting is on `ref_count` directly: each path that - // hands `self` to C++ (`to_js` / `to_js_with_destructor`) takes a +1 - // via `self.ref_()`, and `finalize`'s `deref()` below releases it. - // `JsSinkType::construct` allocates with `ref_count=1` and that +1 - // belongs to the wrapper it's about to be stored in, so no extra - // `ref_()` there. Callers that allocate via `init`/`create` and then - // `to_js()` must `deref()` once to release init's +1 (see - // `Blob::get_writer`). - // - // `pending` is left for `run_pending`/`deinit()`: `.close()` reaches - // here via `doClose` while a backpressured write may still be awaited. + self.pending.set(streams::WritablePending::default()); + self.release_wrapper_ref(); + } + + // Per-wrapper accounting is on `ref_count` directly: each path that + // hands `self` to C++ (`to_js` / `to_js_with_destructor`) takes a +1 + // via `self.ref_()`, and `finalize`'s `deref()` below releases it. + // `JsSinkType::construct` allocates with `ref_count=1` and that +1 + // belongs to the wrapper it's about to be stored in, so no extra + // `ref_()` there. Callers that allocate via `init`/`create` and then + // `to_js()` must `deref()` once to release init's +1 (see + // `Blob::get_writer`). + fn release_wrapper_ref(&mut self) { self.readable_stream.set(readable_stream::Strong::default()); self.js_sink_ref.with_mut(|r| r.deinit()); // SAFETY: `&mut self` carries write provenance over the whole - // allocation; this is the last use of `self` in `finalize`. + // allocation; this is the last use of `self`. unsafe { FileSink::deref(std::ptr::from_mut::(self)) }; } @@ -1200,6 +1202,12 @@ impl crate::webcore::sink::JsSinkType for FileSink { fn finalize(&mut self) { Self::finalize(self) } + fn wrapper_detached(&mut self) { + // `.close()` may run while a backpressured write promise is still + // awaited; leave `pending` for `run_pending` and skip the GC-sweep + // shutdown cleanup. + Self::release_wrapper_ref(self) + } fn construct(this: &mut core::mem::MaybeUninit) { // `Self::construct()` allocates with `ref_count=1`; that +1 belongs to // the C++ `JSFileSink` wrapper `js_construct` is about to create. diff --git a/src/runtime/webcore/Sink.rs b/src/runtime/webcore/Sink.rs index 86c84ac31178..c5de565d0bd0 100644 --- a/src/runtime/webcore/Sink.rs +++ b/src/runtime/webcore/Sink.rs @@ -749,6 +749,11 @@ pub trait JsSinkType: Sized { fn memory_cost(&self) -> usize; fn finalize(&mut self); + /// `.close()` nulled `m_sinkPtr`; release the wrapper's ref. Runs outside + /// GC sweep, so overrides may differ from `finalize`. Default: `finalize`. + fn wrapper_detached(&mut self) { + self.finalize(); + } fn write_bytes(&mut self, data: &streams::Result) -> streams::result::Writable; fn write_utf16(&mut self, data: &streams::Result) -> streams::result::Writable; fn write_latin1(&mut self, data: &streams::Result) -> streams::result::Writable; @@ -1032,6 +1037,12 @@ impl JSSink { this.finalize(); } + /// `${abi_name}__wrapperDetached` body. + #[inline] + pub fn js_wrapper_detached(this: &mut T) { + this.wrapper_detached(); + } + /// `${abi_name}__controllerDetached` body — called from /// `JSReadable*Controller::detach()` (controller `.end()`/`.close()` host /// fns) and from the controller's destructor, i.e. whenever the diff --git a/src/runtime/webcore/s3/client.rs b/src/runtime/webcore/s3/client.rs index 233b2f3f8803..5420a7652bb3 100644 --- a/src/runtime/webcore/s3/client.rs +++ b/src/runtime/webcore/s3/client.rs @@ -467,7 +467,9 @@ pub(crate) fn writable_stream( } } } - sink.finalize(); + // SAFETY: `sink` is the heap-allocated `callback_context`; this releases + // the task's +1 on it and is the last use of `sink`. + unsafe { NetworkSink::deref(core::ptr::from_mut(sink)) }; Ok(()) } diff --git a/src/runtime/webcore/streams.rs b/src/runtime/webcore/streams.rs index 1b2b66c9de4f..f7afbe9517be 100644 --- a/src/runtime/webcore/streams.rs +++ b/src/runtime/webcore/streams.rs @@ -2133,10 +2133,11 @@ pub type H3ResponseSink = HTTPServerWritable; // NetworkSink // ────────────────────────────────────────────────────────────────────────── -// Two intrusive-rc owners: the `JSNetworkSink` wrapper (`m_sinkPtr`, released -// via `__finalize`) and `MultiPartUpload.callback_context` (released in -// `wrapper_callback`); both call `finalize()`, which `deref()`s. +// Two intrusive-rc owners: the `JSNetworkSink` wrapper (`m_sinkPtr`) and +// `MultiPartUpload.callback_context`; each `deref()`s exactly once. `deinit` +// releases the sink's counted ref on the task before freeing. #[derive(bun_ptr::CellRefCounted)] +#[ref_count(destroy = Self::deinit)] pub struct NetworkSink { pub ref_count: core::cell::Cell, // Stored as `BackRef` @@ -2246,12 +2247,20 @@ impl NetworkSink { } pub fn finalize(&mut self) { - self.detach_writable(); // SAFETY: `&mut self` carries write provenance over the whole // allocation; this is the last use of `self`. unsafe { NetworkSink::deref(core::ptr::from_mut::(self)) }; } + /// Runs once when the refcount hits zero. `Drop for MultiPartUpload` does + /// not enter JS, so this is safe when reached from a GC-sweep `finalize`. + unsafe fn deinit(this: *mut Self) { + // SAFETY: caller contract — sole owner of a heap-allocated `Self`. + unsafe { &mut *this }.detach_writable(); + // SAFETY: `this` was allocated via `heap::into_raw` in `writable_stream`. + drop(unsafe { bun_core::heap::take(this) }); + } + fn detach_writable(&mut self) { if let Some(task) = self.task.take() { // task is ref-counted; deref releases our ref From 92969a0b12ac61f8edcd41ebb8c9f1c0ac989303 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 22 Jul 2026 00:02:33 +0000 Subject: [PATCH 5/7] detach the task synchronously in wrapper_callback Moving detach_writable() to deinit() (rc=0) meant the MultiPartUpload's poll_ref stayed ref'd until the JSNetworkSink wrapper was swept, so a writer retained past await w.end() kept the process alive. wrapper_callback now releases the sink's +1 on the task before the sink deref; deinit() still handles the case where the wrapper went away first. New test covers the retained-writer exit path. --- src/runtime/webcore/s3/client.rs | 3 +++ src/runtime/webcore/streams.rs | 2 +- test/js/bun/s3/s3-networksink-leak.test.ts | 26 ++++++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/runtime/webcore/s3/client.rs b/src/runtime/webcore/s3/client.rs index 5420a7652bb3..42e8cd599eee 100644 --- a/src/runtime/webcore/s3/client.rs +++ b/src/runtime/webcore/s3/client.rs @@ -467,6 +467,9 @@ pub(crate) fn writable_stream( } } } + // The upload has terminally completed: release the sink's +1 on the + // task now so `poll_ref` is unref'd synchronously, not on GC. + sink.detach_writable(); // SAFETY: `sink` is the heap-allocated `callback_context`; this releases // the task's +1 on it and is the last use of `sink`. unsafe { NetworkSink::deref(core::ptr::from_mut(sink)) }; diff --git a/src/runtime/webcore/streams.rs b/src/runtime/webcore/streams.rs index f7afbe9517be..9f122f12020a 100644 --- a/src/runtime/webcore/streams.rs +++ b/src/runtime/webcore/streams.rs @@ -2261,7 +2261,7 @@ impl NetworkSink { drop(unsafe { bun_core::heap::take(this) }); } - fn detach_writable(&mut self) { + pub fn detach_writable(&mut self) { if let Some(task) = self.task.take() { // task is ref-counted; deref releases our ref bun_s3::MultiPartUpload::deref_(task.as_ptr()); diff --git a/test/js/bun/s3/s3-networksink-leak.test.ts b/test/js/bun/s3/s3-networksink-leak.test.ts index 574131b4fb46..8c202364e97b 100644 --- a/test/js/bun/s3/s3-networksink-leak.test.ts +++ b/test/js/bun/s3/s3-networksink-leak.test.ts @@ -66,3 +66,29 @@ describe.skipIf(!isASAN)("S3 writer() NetworkSink is freed", () => { } } }); + +test("S3 writer() unrefs the event loop once end() resolves, even if the writer is retained", async () => { + const script = ` + const server = Bun.serve({ + port: 0, + async fetch(req) { await req.arrayBuffer(); return new Response("", { status: 200, headers: { etag: '"e"' } }); }, + }); + server.unref(); + const s3 = new Bun.S3Client({ accessKeyId: "k", secretAccessKey: "s", bucket: "b", endpoint: \`http://127.0.0.1:\${server.port}\` }); + const w = s3.file("k").writer({ retry: 0 }); + w.write("hi"); + await w.end(); + globalThis.keep = w; + process.once("beforeExit", () => console.log("beforeExit")); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: { ...bunEnv, http_proxy: undefined, HTTP_PROXY: undefined, https_proxy: undefined, HTTPS_PROXY: undefined }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("beforeExit"); + expect(exitCode).toBe(0); +}); From 4d2f77982f8783f30201aae068ed17fb747fc34d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 22 Jul 2026 00:15:49 +0000 Subject: [PATCH 6/7] make wrapper_callback teardown unconditional; add FileSink .close() backpressure test wrapper_callback's detach+deref now runs via a scopeguard so a JsTerminated from promise settlement cannot skip it. Added a filesink.test.ts case that fills a socket pair, calls .close(), and asserts the pending write promise still settles. Shortened the release_wrapper_ref comment to three lines. --- src/runtime/webcore/FileSink.rs | 11 +++-------- src/runtime/webcore/s3/client.rs | 15 +++++++++------ test/js/bun/util/filesink.test.ts | 28 ++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/runtime/webcore/FileSink.rs b/src/runtime/webcore/FileSink.rs index e05d6322f524..91412ad4dd5f 100644 --- a/src/runtime/webcore/FileSink.rs +++ b/src/runtime/webcore/FileSink.rs @@ -928,14 +928,9 @@ impl FileSink { self.release_wrapper_ref(); } - // Per-wrapper accounting is on `ref_count` directly: each path that - // hands `self` to C++ (`to_js` / `to_js_with_destructor`) takes a +1 - // via `self.ref_()`, and `finalize`'s `deref()` below releases it. - // `JsSinkType::construct` allocates with `ref_count=1` and that +1 - // belongs to the wrapper it's about to be stored in, so no extra - // `ref_()` there. Callers that allocate via `init`/`create` and then - // `to_js()` must `deref()` once to release init's +1 (see - // `Blob::get_writer`). + // Each C++ wrapper holds one +1 (taken in `to_js`/`to_js_with_destructor`, + // released here). `construct`'s initial rc=1 belongs to its wrapper; callers + // using `init`/`create` then `to_js()` release init's +1 (see Blob::get_writer). fn release_wrapper_ref(&mut self) { self.readable_stream.set(readable_stream::Strong::default()); self.js_sink_ref.with_mut(|r| r.deinit()); diff --git a/src/runtime/webcore/s3/client.rs b/src/runtime/webcore/s3/client.rs index 42e8cd599eee..3cb150f643f3 100644 --- a/src/runtime/webcore/s3/client.rs +++ b/src/runtime/webcore/s3/client.rs @@ -431,6 +431,15 @@ pub(crate) fn writable_stream( ) -> JsResult { // Local callback wrapper fn wrapper_callback(result: S3UploadResult, sink: &mut NetworkSink) -> JsTerminatedResult<()> { + // Release the sink's +1 on the task (so `poll_ref` unrefs synchronously) + // and `callback_context`'s +1 on the sink, even on `JsTerminated` below. + let _teardown = scopeguard::guard(core::ptr::from_mut(sink), |s| { + // SAFETY: `s` is the heap-allocated `callback_context`, still live + // for the duration of this callback; last use here. + unsafe { &mut *s }.detach_writable(); + // SAFETY: releases `callback_context`'s +1. + unsafe { NetworkSink::deref(s) }; + }); // `global_this` is a `BackRef` set at construction; copy it so the // re-borrow does not hold `&sink` across the `&mut sink` calls below. let global = sink @@ -467,12 +476,6 @@ pub(crate) fn writable_stream( } } } - // The upload has terminally completed: release the sink's +1 on the - // task now so `poll_ref` is unref'd synchronously, not on GC. - sink.detach_writable(); - // SAFETY: `sink` is the heap-allocated `callback_context`; this releases - // the task's +1 on it and is the last use of `sink`. - unsafe { NetworkSink::deref(core::ptr::from_mut(sink)) }; Ok(()) } diff --git a/test/js/bun/util/filesink.test.ts b/test/js/bun/util/filesink.test.ts index 4fbfe3902743..c53bd9f4daa6 100644 --- a/test/js/bun/util/filesink.test.ts +++ b/test/js/bun/util/filesink.test.ts @@ -207,6 +207,34 @@ it("write result is not cumulative", async () => { await util.promisify(fs.close)(fd); }); +it.skipIf(!isPosix)("a backpressured write() promise settles after .close()", async () => { + const [readFd, writeFd] = createSocketPair(); + const sink = Bun.file(writeFd).writer(); + const size = 4 * 1024 * 1024; + + const pending = sink.write(Buffer.alloc(size, 0x61)); + expect(pending).toBeInstanceOf(Promise); + + // .close() detaches m_sinkPtr and releases the wrapper's ref via + // wrapper_detached, which must leave `pending` alone so run_pending can + // still settle the promise once the writer drains. + sink.close(); + + let received = 0; + const reader = (async () => { + for await (const part of Bun.file(readFd).stream()) received += part.byteLength; + })(); + + try { + expect(await pending).toBe(size); + } finally { + fs.closeSync(writeFd); + await reader; + fs.closeSync(readFd); + } + expect(received).toBe(size); +}); + // A backpressured write buffers everything `write(2)` would not take, so the // Promise it returns has to resolve with the chunk's own byte count. It used to // resolve with the partial `write(2)` return instead. From 4c1d8cf025752e4d27942a4fc6a0099e1ecc97fa Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 22 Jul 2026 00:33:58 +0000 Subject: [PATCH 7/7] drop stderr-empty assertion in the retained-writer test --- test/js/bun/s3/s3-networksink-leak.test.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/js/bun/s3/s3-networksink-leak.test.ts b/test/js/bun/s3/s3-networksink-leak.test.ts index 8c202364e97b..b10685c7e5af 100644 --- a/test/js/bun/s3/s3-networksink-leak.test.ts +++ b/test/js/bun/s3/s3-networksink-leak.test.ts @@ -87,8 +87,6 @@ test("S3 writer() unrefs the event loop once end() resolves, even if the writer stdout: "pipe", stderr: "pipe", }); - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect(stderr).toBe(""); - expect(stdout.trim()).toBe("beforeExit"); - expect(exitCode).toBe(0); + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: "beforeExit", exitCode: 0 }); });