JSSink: release native backing in prototype close() - #36785
Conversation
The generated ${name}__doClose (ArrayBufferSink.close(), FileSink.close(),
etc.) nulls m_sinkPtr via detach() and then calls ${name}__close, which
only runs end(None). Because m_sinkPtr is null, the later ~JS${name}
destructor skips ${name}__finalize, so the wrapper's ownership of the
native backing was never released on this path: ArrayBufferSink leaked
its Box and buffer outright, and FileSink/NetworkSink leaked the
wrapper's +1 intrusive ref.
__doClose now fires the destroy callback (so Subprocess can clear its
weak stdin back-pointer before the sink can be freed) and then calls
__finalize, mirroring the destructor order.
FileSink::finalize no longer clears pending/readable_stream: now that it
is reachable synchronously from close(), tearing those down would strand
a backpressured write() promise or an in-flight ReadableStream stdin.
Both are released by deinit() (Box drop) once the keep-alive and
assignToStream refs are gone.
WalkthroughThe change finalizes native ChangesSink lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #34999. That PR is primarily the NetworkSink refcount fix and adds a new The substantive difference: this PR also fires Whichever lands first, the other will need a small rebase in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this reworks native refcount release for all seven JSSink types and re-times FileSink::finalize / on_stdin_destroyed, so it's worth a human look at the ref-balance across every sink.
What was reviewed:
__doClosenow mirrors the destructor (Bun__onSinkDestroyed→__finalize) afterdetach();m_onDestroyisstd::exchanged to 0 so the wrapper's dtor won't fire it again.FileSink::finalizedropping the eagerpending/readable_streamclears — traced that both are still released viadeinit's Box drop oncemust_be_kept_alive_until_eof/ assignToStream refs are gone.on_stdin_destroyednow firing synchronously fromchild.stdin.close()— nullsweak_file_sink_stdin_ptrbefore__finalizecan drop the last ref, and the Subprocess deref there is safe whilechildis still JS-reachable.- New tests cover the leak (LSAN +
liveCount()), use-after-close, and backpressured-promise settlement; the pre-existing EPIPE test drops itsdetect_leaks=0workaround.
Extended reasoning...
Overview
This PR fixes a leak in the generated ${name}__doClose (backing .close() on all seven JSSink wrappers): it detached m_sinkPtr and called __close but never __finalize, so the wrapper's destructor skipped the native release and every close() leaked the backing struct/buffers. The fix adds Bun__onSinkDestroyed(m_onDestroy, ptr) + __finalize(ptr) after __close, mirroring the destructor's teardown order. Because FileSink::finalize is now reachable synchronously from close() (not just GC sweep), it no longer eagerly clears pending (backpressured write promise) or readable_stream (spawn-stdin driver); both defer to deinit()'s Box drop.
Security risks
None. This is memory-lifetime bookkeeping — no user input parsing, auth, or externally-facing surface changes.
Level of scrutiny
High. This is squarely in the repo's "most-blocked category" (native memory safety / refcount balancing / GC interaction). The generated change applies to seven sink types (ArrayBufferSink, FileSink, three HTTP response sinks, NetworkSink, FetchRequestBodySink), each with its own finalize semantics that must now be safe when invoked synchronously from user JS rather than only from lazy sweep. The FileSink::finalize change also shifts when Subprocess::on_stdin_destroyed fires (from GC-time to close()-time via Bun__onSinkDestroyed), which nulls weak_file_sink_stdin_ptr and may deref() the Subprocess — a maintainer should confirm that early HAS_STDIN_DESTRUCTOR_CALLED / cleared weak-ptr doesn't break the process-exit path (on_attached_process_exit is skipped once the weak-ptr is null, which looks intended).
Other factors
The PR references #29883 (same fix in the removed Zig sources), and the test suite already carried a detect_leaks=0 workaround acknowledging the FileSink case, so the leak is well-established. The new tests are targeted (LSAN subprocess for ArrayBufferSink, liveCount() for FileSink, backpressured-promise settlement for the pending-clear removal, use-after-close guard). I checked that m_onDestroy is std::exchanged to 0 so the wrapper's later dtor won't double-fire it, and that js_finalize for the other sink types (ArrayBufferSink, NetworkSink, FetchRequestBodySink, HTTP response sinks) doesn't obviously depend on sweep-only invariants — but the ref-balance for each deserves maintainer eyes given how many callers reach .close().
Update the header comments on FileSink::finalize, ArrayBufferSink::finalize and the generated __finalize thunk to list both call sites (destructor lazy sweep and the prototype close() path) so the governing constraint reads correctly up front.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/codegen/generate-jssink.ts (1)
511-542: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftVerify exception-scope handling in
${name}__doClosebefore finalizing.
${name}__close(lexicalGlobalObject, ptr)runs inside aDECLARE_THROW_SCOPEand can set a pending exception. The new code callsBun__onSinkDestroyed(destroy, ptr)and${name}__finalize(ptr)immediately afterward, without checking or clearing that exception first. Only then doesRETURN_IF_EXCEPTION(scope, {})run.This differs from the established pattern in the same file for
${controller}__closeand${controller}__end(Lines 425-433, 472-480), which explicitly stash the pending exception, clear it withscope.tryClearException(), run the JS-entering call, clear again, and rethrow the original exception. That pattern exists specifically because JSC has assertions against entering the VM again while an exception scope is set.If
Bun__onSinkDestroyedor${name}__finalizenever call back into JS for any of the four sink types (ArrayBufferSink, FileSink, NetworkSink, FetchRequestBodySink), this is safe as written. If any of them do (now or after a future change), running them with a pending exception can trip JSC's exception-scope validator (enabled on the ASAN CI shard). Since this is templated code shared by all sink classes, a single defect here affects every sink.This location was previously flagged by automated review for needing a paragraph-long comment to justify a workaround. Restructuring to mirror
${controller}__close's stash/clear/rethrow removes both the exception-ordering risk and the need for that justification comment.🔧 Proposed fix mirroring the `${controller}__close` pattern
sink->detach(); ${name}__close(lexicalGlobalObject, ptr); - // detach() nulled m_sinkPtr, so ~${className} will not run __finalize for - // this ptr. Release the wrapper's ownership here (unconditionally: even if - // __close set an exception) so the native backing is freed rather than - // leaked. Fire the destroy callback first, same as the destructor does: - // Subprocess holds a weak back-pointer that must be cleared before - // __finalize can drop the last ref on the sink. - if (auto destroy = std::exchange(sink->m_onDestroy, 0)) { - Bun__onSinkDestroyed(destroy, ptr); - } - ${name}__finalize(ptr); - RETURN_IF_EXCEPTION(scope, {}); + // detach() nulled m_sinkPtr, so ~${className} will not run __finalize for + // this ptr; run the destructor's onDestroy-then-finalize sequence here so + // the native backing is freed rather than leaked, even if __close threw. + if (JSC::Exception* pending = scope.exception()) [[unlikely]] { + if (!scope.tryClearException()) { + return {}; + } + if (auto destroy = std::exchange(sink->m_onDestroy, 0)) { + Bun__onSinkDestroyed(destroy, ptr); + } + ${name}__finalize(ptr); + (void)scope.tryClearException(); + scope.throwException(lexicalGlobalObject, pending); + return {}; + } + if (auto destroy = std::exchange(sink->m_onDestroy, 0)) { + Bun__onSinkDestroyed(destroy, ptr); + } + ${name}__finalize(ptr); + RETURN_IF_EXCEPTION(scope, {}); return JSC::JSValue::encode(JSC::jsUndefined());Please confirm with a search whether
Bun__onSinkDestroyedor any${name}__finalizeimplementation ever invokes JS.oven-sh bun Bun__onSinkDestroyed implementation Subprocess weak_file_sink_stdin_ptr🤖 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/codegen/generate-jssink.ts` around lines 511 - 542, Update `${name}__doClose` to mirror the exception handling used by `${controller}__close` and `${controller}__end`: stash any exception from `${name}__close`, clear the scope before invoking `Bun__onSinkDestroyed` and `${name}__finalize`, clear any newly raised exception, then rethrow the original exception. Preserve cleanup ordering and remove the workaround comment once the control flow provides the necessary justification.Source: Path instructions
🤖 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/codegen/generate-jssink.ts`:
- Around line 511-542: Update `${name}__doClose` to mirror the exception
handling used by `${controller}__close` and `${controller}__end`: stash any
exception from `${name}__close`, clear the scope before invoking
`Bun__onSinkDestroyed` and `${name}__finalize`, clear any newly raised
exception, then rethrow the original exception. Preserve cleanup ordering and
remove the workaround comment once the control flow provides the necessary
justification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47bf9379-76b8-4105-87cf-5b92296ed317
📒 Files selected for processing (5)
src/codegen/generate-jssink.tssrc/runtime/webcore/ArrayBufferSink.rssrc/runtime/webcore/FileSink.rstest/js/bun/util/arraybuffersink.test.tstest/js/bun/util/filesink.test.ts
|
On the exception-scope concern: checked whether
The Ran both test files under |
There was a problem hiding this comment.
No issues found, but this reworks native finalization/refcount lifecycle across the C++/Rust FFI boundary for all seven JSSink types — worth a human look.
What was reviewed:
__doCloseteardown order:m_onDestroyfires before__finalizeso Subprocess's weak stdin backpointer is cleared before the sink can free;std::exchangeprevents the destructor from firing it again.FileSink::finalizeno longer clearingpending/readable_stream: traced both the sweep path (deferred todeinitvia Box drop, same net effect) and the synchronous-close path (in-flight IO still holds the keep-alive ref, sorun_pendingcan settle the promise).- Confirmed none of the reachable
__finalize/Bun__onSinkDestroyedpaths re-enter JS between__closeand the trailingRETURN_IF_EXCEPTION, so no exception-scope stash/clear is needed here (unlike the controller path).
Extended reasoning...
Overview
The PR fixes a native memory leak in the generated ${name}__doClose (backing .close() on ArrayBufferSink, FileSink, NetworkSink, FetchRequestBodySink, and the three HTTP response sinks). Previously detach() nulled m_sinkPtr before __close ran, so the wrapper's destructor skipped __finalize and every .close() leaked the native backing. The fix mirrors the destructor's teardown order inline: fire m_onDestroy, then __finalize(ptr), even if __close set an exception. FileSink::finalize is adjusted to stop tearing down pending/readable_stream (now that it can run synchronously from .close() while a backpressured write is still in flight), leaving them for deinit (Box drop). Four new tests cover the LSAN leak, the liveCount() leak, post-close method behavior, and the pending-write-during-close case.
Security risks
None. This is a native lifecycle/refcount fix; no user input parsing, auth, or trust boundaries are touched.
Level of scrutiny
High. This is squarely in the repo's most-blocked review category (native memory safety): it changes when finalize runs relative to close, adjusts which fields are torn down at which lifecycle stage, and interacts with intrusive refcounting, JSC lazy sweep, Subprocess's weak stdin backpointer, and the keep-alive ref taken on backpressure. The change fans out through generated C++ to seven sink types, only two of which have direct test coverage here (the PR argues the HTTP response sinks never reach __doClose, and NetworkSink/FetchRequestBodySink finalize is a task-ref release). The reasoning is careful and the tests are well-targeted, but the invariant surface ("finalize now runs from two contexts with different constraints") is exactly the shape that benefits from a maintainer's eyes.
Other factors
- There is a known overlapping PR (#34999) that touches the same
__doClosecodegen with a different ABI approach; whichever lands first requires the other to rebase. - The
FileSink::finalizechange is a real behavioral shift for the sweep path too: when the keep-alive ref outlives the wrapper,pending/readable_streamnow survive untildeinitinstead of being cleared at sweep time. I traced this and believe it's correct (and arguably fixes a latent bug where sweep could drop a pending promise's Strong beforeon_writesettled it), but it's a non-obvious consequence a maintainer should confirm. - Test coverage is solid: LSAN-gated leak repro, native
liveCount()check, post-close error guard, and a spawn-stdin backpressure test that specifically exercises them_onDestroyordering. The pre-existing EPIPE test dropping itsdetect_leaks=0workaround is a nice proof the leak is actually gone.
|
CI on ebb4bce: Ready for review. |
2 participants
Problem
The generated
${name}__doClose(backingArrayBufferSink.prototype.close(),FileSink.prototype.close(), and the other JSSink wrappers) does this:${name}__closeonly runsend(None)and does not free anything. When the wrapper is later collected,~JS${name}checksif (m_sinkPtr)before calling${name}__finalize, andm_sinkPtris already null, sofinalizeis skipped. Everyclose()leaked the native backing:ArrayBufferSink: the boxed struct and itsVec<u8>buffer.FileSink(includingBun.file(...).writer()andchild.stdin): the wrapper's +1 intrusive ref, so the struct and its IO buffers.NetworkSink/FetchRequestBodySink: the wrapper's task ref.The HTTP response sinks only go through the controller path and are not affected.
filesink.test.tsalready carried a comment and adetect_leaks=0override acknowledging the FileSink case.Fix
${name}__doClosenow mirrors the destructor's teardown order after detaching: firem_onDestroy(so Subprocess clears itsweak_file_sink_stdin_ptrbefore the sink can be freed), then call${name}__finalize(ptr). This runs even if__closeset an exception, since the wrapper has already given up its pointer.Because
FileSink::finalizeis now reachable synchronously fromclose(), it no longer clearspending(which may hold a backpressuredwrite()promise thatrun_pendingstill has to settle) orreadable_stream(which may still be driving a spawn stdin). Both are released bydeinit()via Box drop once the keep-alive and assignToStream refs are gone.js_sink_refstill gets cleared: it roots the wrapper itself.#29883 fixed the same bug in the Zig sources but was closed when those files were removed in the Rust migration.
Verification
arraybuffersink.test.ts: LSAN-gated subprocess test forclose()(fails on main with >16 KiB leaked, clean with the fix) and a guard thatwrite()/flush()/end()afterclose()still throw "already been closed".filesink.test.ts:fileSinkInternals.liveCount()check forclose()(8 leaked on main, 0 with the fix); a guard that a backpressuredwrite()promise still settles whenclose()runs before the drain; and the pre-existing EPIPE test drops itsdetect_leaks=0workaround.no test proof · iteration 0 · 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