s3: free the NetworkSink behind writer() via intrusive refcount - #34999
s3: free the NetworkSink behind writer() via intrusive refcount#34999robobun wants to merge 7 commits into
Conversation
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().
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis change adds a wrapper-detachment callback path, preserves pending ChangesSink lifecycle and leak handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:15 PM PT - Jul 21st, 2026
✅ @robobun, your commit 4c1d8cf025752e4d27942a4fc6a0099e1ecc97fa passed in 🧪 To try this PR locally: bunx bun-pr 34999That installs a local version of the PR into your bun-34999 --bun |
__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.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/runtime/webcore/FileSink.rs`:
- Around line 935-939: Shorten the pending-lifecycle comment near
finalize/doClose to three lines or fewer while preserving the invariant that
pending is not cleared during close, run_pending settles backpressured writes
after draining, and deinit drops the field when the refcount reaches zero.
In `@src/runtime/webcore/streams.rs`:
- Around line 2136-2139: Condense the ownership comment above the raw
NetworkSink pointer to three lines or fewer while preserving that JSNetworkSink
and MultiPartUpload.callback_context own it, both release through their
respective finalization paths, and finalize() frees it via intrusive reference
counting.
In `@test/js/bun/s3/s3-networksink-leak.test.ts`:
- Around line 4-14: Remove the multi-line historical regression explanation from
the test, preserving the test name and assertions unchanged. Retain only an
issue URL comment if one is present or available, and ensure any remaining
comment is no more than three lines.
🪄 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: 39308ea1-695f-4988-a148-7c48384a1fe0
📒 Files selected for processing (5)
src/codegen/generate-jssink.tssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/streams.rstest/js/bun/s3/s3-networksink-leak.test.ts
|
CI on 4d2f779 (build #77266): the diff is green. The only hard failure is |
There was a problem hiding this comment.
The .close() leak I flagged earlier is now fixed via ${name}__finalize(ptr) in __doClose, matching #29883's approach. I traced the refcount balance for all six affected sink types and it holds — but the codegen change fans out to every JSSink's .close() path plus the FileSink pending lifecycle tweak, so this warrants a human look.
What was reviewed:
- NetworkSink rc=2 balance across
.end()/.close()× success/failure — each path reaches exactly twofinalize()→deref()calls;abort()no longer double-derefs on the failure branch ofwrapper_callback. __doClose→__finalizefanout: ArrayBufferSink (destroy— was leaking, now freed once), FileSink (rc-based, keep-alive ref survives), HTTPServerWritable×3 (finalize()is documented idempotent, no rc).- Controller
__close/__endintentionally still skip__finalize—assign_to_streamtakes no per-wrapper +1 for the controller, so nothing to release there. - FileSink not clearing
pending: rc>1 viamust_be_kept_alive_until_eofwhile a write is outstanding, sorun_pendingstill has a live sink;deinit()drops the field at rc=0.
Extended reasoning...
Overview
Fixes an S3 NetworkSink leak by adding an intrusive CellRefCounted refcount with two owners (JS wrapper + MultiPartUpload.callback_context), and closes a related pre-existing leak where ${name}__doClose nulled m_sinkPtr before the destructor could reach __finalize. The codegen fix applies to all six JSSink types. FileSink::finalize stops clearing pending so a backpressured write promise survives .close(). abort() now stops at detach_writable() so the failure path in wrapper_callback doesn't double-deref before its trailing finalize().
Security risks
None. This is internal lifetime management; no user-controlled input reaches new validation or allocation paths.
Level of scrutiny
High. Per REVIEW.md, native memory safety is the most-blocked category. The refcount must be provably balanced on every terminal path, and the __doClose codegen edit fans out to six sink types with three different finalize() disciplines (rc-based free, direct destroy, idempotent no-free). I verified each:
- NetworkSink: rc=2 at construction;
.end()path releases via~JSNetworkSink→__finalize+wrapper_callback→finalize();.close()path releases via__doClose→__finalize+wrapper_callback→finalize().abort()sole caller iswrapper_callback's failure arm, which follows withfinalize()— soabort()dropping its ownderef()is required to avoid a double-release. - FileSink: already rc-based; wrapper's +1 is now released on
.close()where it previously leaked. If a write is pending,end(None)takes the keep-alive +1 so rc stays ≥1 throughfinalize()'sderef(), andrun_pendinglater settles the (now-preserved)pendingpromise. - ArrayBufferSink:
finalize()→destroy()frees directly;end(None)does not free, so the new__finalizecall is the single free (previously leaked). - HTTPServerWritable×3:
finalize()is explicitly designed to be called many times and does not free;end(None)may call it internally, and the second call from__doCloseseesdone=trueand the buffer-pool cleanup is idempotent. - Controller paths (
${controller}__close/__end): intentionally unchanged —assign_to_streamtakes no per-controller +1 (see the comment inFileSink::assign_to_stream), so there is no ref to release.
Other factors
My earlier review found the .close() gap; it was fixed following #29883's exact approach and the leak test now covers both .end() and .close() × success/failure under detect_leaks=1. The PR reports filesink/arraybuffersink/serve suites unchanged vs main. Despite the analysis checking out, the cross-cutting nature (generated C++ affecting six native lifecycle paths) and the FileSink.pending behavioral tweak make this worth a human maintainer's eyes rather than a bot-only approval.
Jarred-Sumner
left a comment
There was a problem hiding this comment.
finalize is typically called during GC finalization. When it is called during GC finalization, we can't allocate JS objects or enter JavaScript. Calling pending.run would potentially enter JavaScript, and this code would crash.
Instead, we should add a different method for cleaning up and move most of FileSink's logic for finalize into it, and then both call it.
Jarred-Sumner
left a comment
There was a problem hiding this comment.
callback_context should not call finalize? It should call deref
…ctly 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.
|
Addressed both in 3451a9f:
|
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.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/runtime/webcore/FileSink.rs`:
- Around line 1205-1210: Add a regression test for FileSink covering the
wrapper_detached path: force a write to remain pending under backpressure,
invoke .close(), and assert the retained write Promise eventually settles.
Ensure the test uses FileSink rather than only S3 NetworkSink and verifies the
pending-write behavior introduced in wrapper_detached.
- Around line 931-938: Condense the ownership comment near JsSinkType::construct
to no more than three lines, while preserving the invariant that
to_js/to_js_with_destructor add a wrapper reference released by finalize,
construct’s initial ref_count belongs to the stored wrapper, and init/create
callers release their initial reference after to_js.
In `@src/runtime/webcore/s3/client.rs`:
- Around line 470-475: Update the callback containing the resolve/reject
settlement calls to capture their result instead of propagating with ?, then
always execute NetworkSink::detach_writable and the unsafe NetworkSink::deref
teardown before returning the captured result. Preserve the existing settlement
behavior while ensuring JsTerminated and other errors cannot bypass callback
cleanup.
🪄 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: 4de1daaf-8091-4cdb-bc63-a669b797dbeb
📒 Files selected for processing (6)
src/codegen/generate-jssink.tssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/Sink.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/streams.rstest/js/bun/s3/s3-networksink-leak.test.ts
…ackpressure 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.
…iter tests under ASAN The writer() tests trip LeakSanitizer on the release-asan lane via a pre-existing NetworkSink leak (fix open in #34999); skip them under ASAN until that lands.
### Problem
The JSSink finalize chain frees the sink while reference arguments to it
are still live. At `3fc747a7da`:
* generated thunk `extern "C" fn ${name}__finalize(this: &mut ${name})`
(src/codegen/generate-jssink.ts), called from `~JS${name}`,
`~JSReadable${name}Controller` and `${name}__doClose`
* `JSSink::js_finalize(this: &mut T)` (src/runtime/webcore/Sink.rs)
* `JsSinkType::finalize(&mut self)` (src/runtime/webcore/Sink.rs), whose
impls do the actual release:
* `ArrayBufferSink` (src/runtime/webcore/ArrayBufferSink.rs):
`Self::finalize(ptr::from_mut(self))` -> `destroy` -> `heap::take`,
unconditionally. The comment on the impl said the C export owned the
free; this call is the free.
* `FileSink` (src/runtime/webcore/FileSink.rs): the inherent
`finalize(&mut self)` ends in `FileSink::deref(ptr::from_mut(self))`,
which runs `deinit` -> `heap::take` whenever the wrapper's +1 was the
last ref, i.e. on an ordinary GC sweep of a sink nothing else holds. The
header comment argued this was fine because the `&mut` carries write
provenance, which is true but is not the problem.
* `FetchRequestBodySink`
(src/runtime/webcore/fetch/FetchRequestBodySink.rs): drops the tasklet
ref taken in `start_request_stream`. The tasklet owns the sink
allocation, so if that ref is the last one, `FetchTasklet::deinit` ->
`clear_data` -> `clear_sink` -> `heap::take(sink)` frees `*self` inside
the call. That is the fallback path for a pump that never settled; it is
reachable at least on worker teardown: phase B of `VirtualMachine`
teardown releases the aborted fetch's other refs on the tasklet, and
phase C then destroys the heap, sweeping the controller with `m_sinkPtr`
still set because `JSSinkController__onClose` does not run the detaching
JS callback once termination is pending.
* `HTTPServerWritable`, `NetworkSink` and `RewriterPipe` do not free
anything here (their allocations are owned by the `RequestContext`, the
S3 wrapper and the pipe's own refcount respectively).
A reference passed as an argument has to stay dereferenceable until the
call returns. Freeing it from inside the call is undefined behaviour
under both aliasing models whether or not the reference is used again
(Stacked Borrows: `deallocating while item is strongly protected`; Tree
Borrows, which `bun run rust:miri` uses, rejects it the same way), and
that protector is the model behind the `dereferenceable` attribute rustc
puts on every `&`/`&mut` argument, so the optimizer may legitimately
move a load through any of the three frames past the free. No crash is
known from this; ASAN only has something to catch if the optimizer
actually takes that liberty, which the unoptimized debug build never
does, so it is not observable as a runtime test. Same family as #37672,
#37681, #37685, #37693, #37705 and #37551; #37705's description leaves
this chain out explicitly because it needs a change to the generated
thunk.
### Fix
The whole chain takes the raw pointer, which is what the C++ side has
anyway (`void* m_sinkPtr`):
* generate-jssink.ts emits `pub unsafe extern "C" fn
${name}__finalize(this: *mut ${name})` forwarding to `js_finalize`; the
ABI is unchanged, so JSSink.cpp is untouched.
* `JSSink::js_finalize(this: *mut T)` forwards to the trait.
* `JsSinkType::finalize` becomes `unsafe fn finalize(this: *mut Self)`,
documented as "the cell is giving up its claim; this may free the sink",
the same shape as `HTTPServerWritable::abort(this: *mut Self)` and the
FileSink PipeWriter callbacks.
* The three freeing impls release through the pointer without forming a
reference to the allocation: `ArrayBufferSink` calls `destroy` directly
(the inherent `finalize` wrapper, whose only caller was the trait impl,
is deleted); `FileSink::finalize(this: *mut FileSink)` keeps the same
body with per-statement `(*this).field` access, like `on_close` in the
same file (the file header no longer claims the `&mut` version was
sound; the rationale lives once, on the trait method);
`FetchRequestBodySink::finalize(this: *mut Self)` takes `task` out
through the pointer and does not touch it after the deref.
* `HTTPServerWritable` and `NetworkSink` reborrow inside their own impl
to call the unchanged inherent `finalize(&mut self)`; that borrow ends
before the impl returns and nothing under it frees, which the SAFETY
comments state. `RewriterPipe`'s impl stays empty.
Every impl performs the same operations in the same order as before; the
only thing that moves is the type the pointer travels as.
`js_controller_detached`, `js_close` and `js_end_with_sink` still take
`&mut`: nothing frees under them (the `controller_detached` contract on
the trait already requires deferring a last-owner free for that reason).
`FileSink::assign_to_stream`'s `FileSinkRef` guard also derefs from a
`&mut self` frame, but its ref is balanced against one it took itself
and every caller (subprocess stdin setup) holds its own ref across the
call, so it can never be the one that frees; left alone. Sites with the
same shape outside this chain
(`S3UploadStreamWrapper::handle_{resolve,reject}_stream`,
`FetchTasklet::write_end_request`) are not sink frames and are reported
separately.
### Tests
test/internal/source-lints/jssink-finalize-raw-ptr.test.ts scans every
`impl ... JsSinkType for ...` block for a `finalize` item and requires
`unsafe fn finalize(<ident>: *mut Self)`, checks the other frames by
signature (trait declaration, `js_finalize`, the codegen template, and
the three inherent methods that perform the free, which `pub` tells
apart from the trait impls in the same files), and checks its own
patterns against positive and negative spellings. With src/ restored to
`main` it reports:
```
src/runtime/api/html_rewriter.rs:1650: impl JsSinkType for RewriterPipe: fn finalize(&mut self) (line 1661)
src/runtime/webcore/ArrayBufferSink.rs:213: impl JsSinkType for ArrayBufferSink: fn finalize(&mut self) (line 221)
src/runtime/webcore/fetch/FetchRequestBodySink.rs:274: impl JsSinkType for FetchRequestBodySink: fn finalize(&mut self) (line 281)
src/runtime/webcore/FileSink.rs:1283: impl JsSinkType for FileSink: fn finalize(&mut self) (line 1294)
src/runtime/webcore/streams.rs:2104: impl JsSinkType for HTTPServerWritable: fn finalize(&mut self) (line 2119)
src/runtime/webcore/streams.rs:2523: impl JsSinkType for NetworkSink: fn finalize(&mut self) (line 2530)
src/runtime/webcore/Sink.rs: JsSinkType::finalize declaration does not take the sink as `*mut`
src/runtime/webcore/Sink.rs: JSSink::js_finalize does not take the sink as `*mut`
src/codegen/generate-jssink.ts: generated `${name}__finalize` thunk does not take the sink as `*mut`
src/runtime/webcore/FileSink.rs: FileSink::finalize does not take the sink as `*mut`
src/runtime/webcore/fetch/FetchRequestBodySink.rs: FetchRequestBodySink::finalize does not take the sink as `*mut`
```
(`ArrayBufferSink::destroy` already took `*mut` on `main`; its entry is
a ratchet.)
The behaviour itself is the existing coverage of each finalize path; see
below.
### Verification
Debug (ASAN) build on Linux: `cargo clippy -p bun_runtime` and `rustfmt
--check` on the touched files are clean; the generated thunks have the
new signature. Passing: test/internal/source-lints/ (all 18 files),
test/js/bun/util/arraybuffersink.test.ts and filesink.test.ts (wrapper
sweep and prototype `.close()` for the two Box/refcount sinks),
test/js/bun/spawn/spawn.test.ts (stdin `FileSink` via
`assign_to_stream`), test/js/web/fetch/body-stream.test.ts,
fetch-abort-stream-body.test.ts and fetch-stream-cancel-leak.test.ts
(`FetchRequestBodySink`),
test/js/bun/http/serve-response-stream-sink-leak,
serve-direct-readable-stream, serve-stream-reject-flush-leak and
serve-async-stream-client-abort (`HTTPServerWritable` controller
teardown), test/js/web/fetch/server-response-stream-leak.test.ts,
test/js/web/streams/streams.test.js,
test/js/workerd/html-rewriter.test.js and html-rewriter-leak.test.ts
(`RewriterPipe`), test/js/bun/s3/s3-stream-error-gc.test.ts and
s3-argument-validation.test.ts. The S3 upload tests that would drive
`NetworkSink` (s3.test.ts, s3-storage-class.test.ts) cannot connect from
this environment and fail identically on the released binary, so that
impl (a one-line forward to the unchanged inherent method) is left to
CI.
Overlap with the sibling lints, each of which documents these sites as
tracked separately: #37685 / #37693 / #37705 add
`self-receiver-teardown.test.ts` with
`src/runtime/webcore/ArrayBufferSink.rs: 1` allowlisted for the
`Self::finalize(ptr::from_mut(self))` line this PR removes, and #37703
adds `self-receiver-release.test.ts` with
`src/runtime/webcore/FileSink.rs: 2` allowlisted for the two derefs
inside the old `FileSink::finalize(&mut self)` (running that lint
against this branch reports FileSink.rs at 0). Whichever side lands
second deletes the entry; nothing else conflicts (#37703's
FetchRequestBodySink.rs hunk is `end_from_stream`, a different
function). #34999 and #35528 edit the body of `FileSink::finalize`
textually but keep the receiver.
---------
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Problem
NetworkSink(the native struct behinds3.file(k).writer()) is heap-allocated inwritable_streamviabun_core::heap::into_rawand never freed. Two owners hold a raw*mut NetworkSink:JSNetworkSinkwrapper'sm_sinkPtr: on GC,~JSNetworkSinkcallsNetworkSink__finalize→NetworkSink::finalize()MultiPartUpload.callback_context: on upload completion,wrapper_callbackcallssink.finalize()finalize()only randetach_writable().finalize_and_destroy()doesheap::takebut has no callers. Everywriter()leaked one ~80-byteNetworkSinkon both the success and failure completion paths:Separately, the plain-sink
.close()prototype method nullsm_sinkPtrviadetach()before the destructor can run, so~JS${name}skips__finalizeand the wrapper's native payload is never released on that path for anyJSSinktype.This is the Rust-side reappearance of #29883, which fixed the same leak in the Zig implementation before that PR was closed by the migration.
Fix
NetworkSinkgains an intrusiveCellRefCountedrefcount.writable_streaminitialises it to 2 (JS wrapper +callback_context).finalize()is now a barederef(); the sink's counted ref on theMultiPartUploadis released indeinit()once the refcount hits zero.wrapper_callbackreleasescallback_context's +1 viaNetworkSink::derefdirectly.abort()stops atdetach_writable()so the failure path does not double-deref. The unusedfinalize_and_destroyis removed.JsSinkTypegains awrapper_detached()hook (default:finalize()).generate-jssink.tsemits a new${name}__wrapperDetachedextern and__doClosecalls it after__close, so.close()releases the wrapper's ref without routing through the GC-sweepfinalize(). This closes the same pre-existing leak forArrayBufferSink.close()andFileSink.close().FileSink::finalizeis unchanged from main;release_wrapper_ref()factors out the shared cleanup, andwrapper_detached()calls only that so a backpressured write promise survives.close().Verification
test/js/bun/s3/s3-networksink-leak.test.tsspawns subprocesses underdetect_leaks=1with 2 and 22 writers against a mock S3 server (200 and 403 responses, finished via both.end()and.close()) and asserts the extra 20 writers do not add leaked bytes. Before: diff = 1600 (= 20 × 80). After: diff = 0, and a symbolized run with the repo suppressions reports no leaks at all.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/filesink.test.ts