Conversation
WalkthroughChangesThe change transfers async filesystem and copy task ownership through boxed allocations. Completion paths now use Async task ownership
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced as a source-level contract bug (no crash; the three sites are the ones the lint in test/internal/source-lints/self-receiver-teardown.test.ts reports against CI: Buildkite build 92741 (same Rust code) passed 190/190. The latest build, 93199, has every test lane green (179 of 181 jobs passed, the rest were test-level flakes that passed on retry); it shows as failed only because the two macOS 26 aarch64 test jobs expired waiting for an agent, so it is infrastructure, not this change. Build 93176 before it had one Windows failure in test/bake/deinitialization.test.ts (an exit-time crash in dev-server code this PR does not touch, absent on the re-run); that has been reported for main triage separately. Ready for re-review. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Use an RAII type instead?
|
Yes, that is cleaner. Reworking it so the completion reclaims the Box at the entry point ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks unsafe teardown ordering in NewAsyncCpTask::run_from_js_thread (destroy now runs after resolve/reject via scopeguard, where it previously ran before) and touches the hot task-dispatch path for both platforms, a human look would still be worthwhile.
What was reviewed:
- Drop order in both rewritten bodies —
_dispatch,promise, andtaskall last-use before_deinitfrees*this;tracker.dispatchtakesselfby value soDispatchScopeholds no borrow into the task. JSPromiseStrong::get(&self)returns a JS-heap&mut JSPromise(not a pointer into*this), so holding it across the guard is fine now that the strong root outlives the settle.- The mini-loop thunk's
let _ = run_from_js_thread(p): theIS_SHELLbranch returnsOk(())unconditionally aftercp_on_finish, so nothing is swallowed. c_voidimport removal — only a comment reference remains; the Windows.cast()infers fromuv_fs_t.data.
Extended reasoning...
Overview
Converts UVFSRequest::run_from_js_thread (Windows libuv fs ops) and NewAsyncCpTask::run_from_js_thread (fs.cp on all platforms + shell cp) from &mut self to unsafe fn(this: *mut Self), so the trailing Self::destroy no longer frees an allocation while a protected reference argument is live (Stacked/Tree Borrows UB). The three call sites in dispatch.rs and the Windows __fs_run macro table now pass cast_ptr! through instead of forming &mut. run_from_js_thread_mini is deleted and its body inlined into the AnyTaskWithExtraContext callback. A new source-lint (self-receiver-teardown.test.ts) bans the destroy(ptr::from_mut(self)) shape and its scopeguard::guard deferred form, with a ratcheted allowlist for three files being converted separately.
Security risks
None. This is an aliasing-model correctness fix in already-unsafe teardown code; it does not introduce new unsafe operations, only re-spells the receiver so the free happens through a raw pointer instead of a protected reference. No user-controlled input handling changes.
Level of scrutiny
High — this is memory-safety-critical unsafe Rust on a hot dispatch path, and the cp-task change reorders destroy relative to resolve/reject. I traced the drop order (locals in reverse declaration: _dispatch → … → _deinit), confirmed AsyncTaskTracker::dispatch takes self by value (so DispatchScope<'_> borrows only global_object, not the task), and confirmed JSPromiseStrong::get returns a JS-heap cell reference so the strong root staying alive through settle is the correct direction. The reordering matches UVFSRequest's existing order and the by-value AsyncFSTask::then, and everything happens within one task turn before microtasks drain, so the KeepAlive unref moving after settle is not observable. The pattern matches CompressionStream::run_from_js_thread, DuplexUpgradeContext::run_event, and PostgresSQLConnection::deinit already in the tree.
Other factors
The PR description documents extensive verification (ASAN debug on Linux, 49 cp tests + 36 upstream Node parallel scripts, shell cp under both event-loop arms with the experimental flag on, Windows cargo check, clippy, fmt, and the new lint failing on main at exactly the three fixed sites). The lint test includes positive/negative pattern fixtures and a non-vacuous scan-count guard. No prior reviewer comments to address. Deferring rather than approving because the destroy-after-settle reordering is a real behavioral change in unsafe teardown code that a maintainer should sign off on, even though I found nothing wrong with it.
|
Updated 12:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 5900596 has some failures in 🧪 To try this PR locally: bunx bun-pr 37693That installs a local version of the PR into your bun-37693 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Pushed in f24763c (plus two comment-trim commits, head d5b0628): |
robobun
changed the title
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The self: Box<Self> + Drop rework looks correct and matches the neighboring heap::take dispatch arms, but since it changes ownership/drop semantics on the fs.cp completion path (all platforms) and the Windows libuv fs request path, and reorders destroy relative to promise settlement, a human look would still be worthwhile.
What was reviewed:
KeepAlive::unrefis a no-op whenstatus != Active, so the newDropimpls are safe on an un-ref_'d default and cannot double-unref.release_unrunnow goes throughheap::destroy, which runsDrop, so the keep-alive andThreadSafe<args::Cp>protects are still released on VM teardown.- The two
Err(e) => return promise.reject(...)early returns in the old cp task never calleddestroy; the Box now frees on those paths too. - The mini-loop thunk's
let _ = task.run_from_js_thread()is only reachable forIS_SHELL, where the body returnsOk(())before touching the promise.
Extended reasoning...
Overview
Converts UVFSRequest::run_from_js_thread (Windows libuv-backed open/close/read/write/readv/writev/statfs) and NewAsyncCpTask::run_from_js_thread (fs.cp on all platforms, plus the shell cp builtin) from &mut self methods that free their own receiver via Self::destroy(ptr::from_mut(self)) to self: Box<Self> methods where the caller reclaims the box (heap::take) and Drop handles the keep-alive unref. Touches src/runtime/dispatch.rs (three dispatch arms + the Windows __fs_run table), src/runtime/node/node_fs.rs (~100 lines of native diff: two Drop impls added, two destroy() fns and run_from_js_thread_mini deleted, cp completion body simplified), and adds a 224-line source-lint test with a ratcheted allowlist for three files with the same shape still in flight.
Security risks
None identified. This is an internal ownership/aliasing-model refactor; no user-controlled input handling, parsing, or trust-boundary code changes.
Level of scrutiny
High. This is memory-safety-critical native code on a hot fs completion path. The change is well-motivated (freeing through a protected &mut self receiver is UB under Stacked/Tree Borrows) and the Box<Self> + Drop shape matches the established pattern in neighboring dispatch arms (AsyncModule, StatWatcherTimerUpdate, ValkeyDeferredClose). But it adds Drop impls to two types that previously had none, reorders the cp task's destroy relative to promise.resolve/reject (now after, matching UVFSRequest), and removes the raw-*mut JSPromise + opaque_mut dance in favor of holding the JSPromiseStrong borrow across settlement. Each of these is individually justified in the PR description and I traced them through, but the composition is subtle enough that a maintainer familiar with the JSC promise/keep-alive lifecycle should confirm.
Other factors
- Verified
KeepAlive::unrefearly-returns onInactive/Done(src/io/keep_alive.rs:43), so the newDropcannot underflow the loop refcount if a task is somehow dropped withoutref_()having run, andDropon the shell specialization is gated on!IS_SHELLmatching the olddestroy. release_unrunfor both types now callsbun_core::heap::destroy(=drop(Box::from_raw)) instead of the deletedSelf::destroy;Dropruns, so the keep-alive unref and field drops (includingThreadSafe<args::Cp>'sunprotect()) still happen on VM-teardown release.- The old cp
run_from_js_threadleaked the task on the twoto_js_with_async_stack/fs_to_jsErrearly-return paths (they returned beforeSelf::destroy); the Box now drops on every return. - The PR description's "Fix" section still describes the earlier
this: *mut Self+ scopeguard revision; the actual code is theself: Box<Self>rework from the follow-up comment. Not blocking, but worth syncing before merge. - The comment-cop bot flagged long comments on earlier commits; the two most recent commits (
trim the completion comments,one-line completion comments) appear to have addressed those. - The new source-lint test's regex + allowlist approach mirrors sibling lints in
test/internal/source-lints/; it self-tests its patterns against positive/negative fixtures and guards against a vacuous scan.
…37705 Same lint body as the other two PRs converting sites of this shape (adds the scopeguard deferred form and finalize as a callee), so each PR's copy differs only in which allowlist entry it deletes. This copy drops the ThreadPool.rs entry and keeps node_fs.rs, copy_file.rs and read_file.rs at their current counts.
### 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>
…37705 Same lint body as the other two PRs converting sites of this shape (adds the scopeguard deferred form and finalize as a callee), so each PR's copy differs only in which allowlist entry it deletes. This copy drops the ThreadPool.rs entry and keeps node_fs.rs, copy_file.rs and read_file.rs at their current counts.
…ot &mut self UVFSRequest::run_from_js_thread and NewAsyncCpTask::run_from_js_thread took &mut self and freed the Box containing self before returning (a scope guard over ptr::from_mut(self) in the first, Self::destroy(ptr::from_mut(self)) in the second). A reference argument is protected for the duration of the call, so deallocating through it is undefined behaviour under both aliasing models. Both now take this: *mut Self. The body moves the result out, works through a shared borrow for the rest, and a guard over the raw pointer frees the task after the promise is settled (which also lets the cp task drop its raw JSPromise pointer and covers its early returns). The shell's mini-loop thunk calls the pointer-taking entry point directly, and the dispatch arms pass task.ptr through instead of forming &mut. Adds a source lint banning destroy/deinit (direct or via scopeguard) applied to a pointer spelled from self, with the remaining in-flight conversions allowlisted at their current counts.
run_from_js_thread on UVFSRequest and NewAsyncCpTask takes self: Box<Self>; the dispatch arms and the shell's mini-loop thunk reclaim the leaked box with heap::take and call it, so every return path frees the task when the box drops. The keep-alive unref moves into Drop impls, which replaces destroy() (release_unrun is heap::destroy) and removes the scope guards.
Route the example snippets through the same scanText() the file scan uses and add a fixture pinning comment stripping and line attribution, so the pipeline stays exercised once the allowlist empties. Match the fn-value guard form (scopeguard::guard(ptr, Self::destroy)), finalize as a callee, nested turbofish, reborrowed and parenthesized receivers. Allowlist read_file.rs, which the finalize spelling now reaches.
d5b0628 to
7e23a06
Compare
|
Self-review of the lint turned up three things, fixed in 7e23a06 (branch also rebased onto current main): the example snippets bypassed the comment-stripping step that the real scan depends on (now everything goes through one |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/internal/source-lints/self-receiver-teardown.test.ts`:
- Around line 103-107: Update the DEFERRED regex to match only complete
scopeguard callbacks, requiring the closure and scopeguard::guard() delimiters
after the teardown call so callbacks with additional statements are excluded.
Add the multi-call callback described in the comment to the allowed regression
cases.
- Around line 154-156: Update the allowlist handling around ALLOW and the hits
loop to associate each exception with its documented teardown-site text or
pattern instead of skipping the first N matches. Reject any hit that does not
match an allowlisted site, while retaining the existing count assertion as a
ratchet.
🪄 Autofix
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: 36b80819-05f5-4a78-ad24-7d533a86a2f0
📒 Files selected for processing (3)
src/runtime/dispatch.rssrc/runtime/node/node_fs.rstest/internal/source-lints/self-receiver-teardown.test.ts
…spelling Each entry now names the exact match text it exempts as well as the count, so a different spelling appearing in an allowlisted file is reported even while the entry exists; the triage step is a function with its own test. Also document that a guard closure is matched on its first expression.
|
On the two review points about the lint (both in 7b62972): allowlist entries now carry the exact spelling they exempt as well as the count, so a different spelling in an allowlisted file is reported while the entry exists, and the triage step has its own test. For the guard pattern I went the other way from the suggestion: a guard closure that frees the receiver and then does more is still the bug, so it stays matched (added as a positive example) and the header now says precisely what is matched, the closure's first expression. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks task-box ownership and Drop ordering across dispatch.rs, both event-loop paths, and the Windows-only UVFSRequest completions — with a stated order-of-operations change for the cp task — a maintainer look is still worthwhile.
What was reviewed:
- The new
Dropimpls vs. the removeddestroy(): keep-aliveunrefnow runs after promise settlement for cp (same tick, before microtasks drain) and on every return path including the twoErrearly returns that previously leaked. - All call sites of
run_from_js_threadupdated in step: the twodispatch.rsarms, the Windows__fs_runmacro, the mini-loop thunk, andrelease_unrun→heap::destroy. - The new source lint's regexes and allowlist against the documented spellings; the fixture and triage tests keep it exercised once the allowlist empties.
Extended reasoning...
Overview
The PR converts two &mut self completion methods in src/runtime/node/node_fs.rs (UVFSRequest::run_from_js_thread on Windows, NewAsyncCpTask::run_from_js_thread on all platforms) to self: Box<Self>, moves the keep-alive unref into Drop impls, deletes the destroy() methods and the scopeguard, and updates every entry point (dispatch.rs arms, the Windows for_each_fs_uv_op! macro, the shell mini-loop thunk, Taskable::release_unrun) to heap::take the box first. It also adds a ~320-line source-lint test that bans destroy/deinit/finalize(ptr::from_mut(self)) and its scopeguard forms tree-wide, with a spelling-bound allowlist for the four files still carrying the pattern.
Security risks
None. This is an internal ownership refactor of async fs task teardown; no user-facing input handling, no auth/crypto/permissions, no new external inputs. The lint test spawns git ls-tree on the repo root, which is benign.
Level of scrutiny
High. Memory safety is the repo's most-scrutinized review category, and this change touches exactly the concerns REVIEW.md calls out: pairing acquisition with release at every exit (now via Drop), release ordering (cp now drops after settling the promise instead of before — the PR description explains why this is safe within one task turn), and reference-count balance across success/error/release_unrun paths. The Windows UVFSRequest path and the __fs_run dispatch macro cannot be exercised on Linux CI alone; the author reports native Windows debug-build verification, which a maintainer should confirm.
Other factors
- The two CodeRabbit review points on the lint (guard-closure completeness, allowlist bound to spelling) were addressed in the head commit and are marked resolved.
- The comment-cop bot's flags were addressed (comments trimmed).
- The bug-hunting system found nothing; I checked that
KeepAlive::unrefinDropcannot run beforeref_()(the box is leaked beforeref_()and only reclaimed at dispatch/release_unrun), that the mini-loop thunk'sdebug_assert!(IS_SHELL)matches the only poster of that path, and thatrun_from_js_thread_mini's removal leaves no dangling caller. - Deferring rather than approving because this is a lifecycle/ordering change in the event-loop dispatcher and fs task teardown across two platforms — the class of change the repo review guide reserves for maintainer sign-off.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes ownership and drop ordering for async fs task completions (unsafe reclaim points, new Drop impls that unref the event loop, Windows-only UVFSRequest paths), a human look would still be worthwhile.
What was reviewed:
- The
Box<Self>reclaim shape matches the neighboringStatWatcherTimerUpdate/AsyncModule/ValkeyDeferredClosearms indispatch.rs;release_unrun→heap::destroystill runs the same unref via the newDrop. - The cp task's drop-after-settle ordering now matches
UVFSRequest(sameself.promise.get()pattern at node_fs.rs:957); the removed earlydestroyno longer forces the raw*mut JSPromisecopy, and the twoErr(e)returns that used to leak the task now free it. - The mini-loop thunk's discarded result is safe: the
IS_SHELLbranch ofrun_from_js_threadunconditionally returnsOk(())aftercp_on_finish. - The lint's spelling-bound allowlist and closed-callback matching address the two CodeRabbit points; positive/negative cases and the fixture cover comment-stripping and line attribution.
Extended reasoning...
Overview
Converts two async fs completion methods from &mut self + explicit Self::destroy(ptr::from_mut(self)) to self: Box<Self> with the keep-alive unref in Drop, fixing a Stacked/Tree Borrows protector violation (deallocating while the receiver reference is protected). Touches src/runtime/dispatch.rs (four arms + the Windows __fs_run table now heap::take the box), src/runtime/node/node_fs.rs (UVFSRequest on Windows and NewAsyncCpTask<IS_SHELL> on all platforms; destroy() and run_from_js_thread_mini deleted), and adds test/internal/source-lints/self-receiver-teardown.test.ts (regex ratchet with a spelling-bound allowlist for four in-flight sibling conversions).
Security risks
None. No user-facing input handling, parsing, or auth surface changes; this is an internal ownership refactor of task-completion lifetime.
Level of scrutiny
High. Per REVIEW.md this is the most-blocked category — native memory safety: unsafe box reclaim, new Drop impls that touch the event loop keep-alive, a change to when the cp task frees relative to promise settlement, and Windows-only libuv request paths that only Windows CI exercises. The change is well-argued and matches established shapes in the same file, but it is not mechanical.
Other factors
- The PR is part of a coordinated set (#37551, #37685, #37705) that intentionally share the lint file so whichever lands second conflicts rather than landing a stale allowlist entry; a maintainer should be aware of the merge-order plan.
- All prior review threads (comment-cop paragraph-comment nags, two CodeRabbit points on the lint's allowlist and guard-callback matching) are marked resolved with follow-up commits; the Rust side is unchanged since d5b0628 which the author reports built green (Buildkite #92741).
- I checked that the new cp
run_from_js_threadmirrors the existingUVFSRequestversion (samepromise.get()/resolve/rejectshape), that theDropunref is the exact bodydestroy()had, and that the shell mini-loop path's discardedResultis provablyOkforIS_SHELL. No issues found, but a maintainer sign-off on the drop-ordering change and the Windows path is appropriate.
This branch has not been deployed
2 participants
Problem
node:fswork take&mut selfand free the heap box that holdsselfbefore returning: the Windows libuv-backed promise ops (open/close/read/write/readv/writev/statfs), andfs.cp/fs.promises.cpon every platform, which the shell'scpbuiltin also goes through.deallocating while item is strongly protected(Stacked Borrows) orthe strongly protected tag disallows deallocations(Tree Borrows, whichbun run rust:miriuses), pointing at the&mut selfreceiver.Fix
self: Box<Self>. The caller holding the raw pointer (the dispatch arm, or the shell's mini-loop thunk) reclaims the box and hands it over, the same shape as the neighbouring task arms; the box drops on every return path, so the error returns no longer leak.destroy()did moves intoDropon the two task types anddestroy()is deleted. This is correct because a callee may deallocate a box it owns but never a reference it was lent, and with the unref inDropthere is no way to free the task without it.mainand passes here; the same shape in four other files is allowlisted by exact spelling and count, each with its own conversion PR. There is no runtime repro since no crash is known; the fscpand promises tests pass on Linux debug (ASAN) and Windows debug builds, and the shellcpbuiltin was run by hand on both loops.Background
heap::taketurns that pointer back into aBox,heap::destroytakes and drops it.dereferenceableattribute rustc puts on the compiled function, so this is not only a Miri concern.run_taskinsrc/runtime/dispatch.rsmatches on the tag and casts the pointer. The shellcpbuiltin can instead complete on a separate mini event loop, which is why the cp task has a second entry point.test/internal/source-lints/holds bun tests that regex-scan the tree's own sources for banned shapes, with a ratcheting allowlist so existing sites can be converted one PR at a time.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
Original description
Problem
Two JS-thread completion methods in src/runtime/node/node_fs.rs take
&mut selfand free the Box that containsselfbefore returning:UVFSRequest::run_from_js_thread(&mut self)(Windows: the libuv-backedopen/close/read/write/readv/writev/statfspromises) armsscopeguard::guard(ptr::from_mut(self), |p| Self::destroy(p)), so the free runs in the epilogue while the receiver is still live.NewAsyncCpTask::run_from_js_thread(&mut self)(fs.cp/fs.promises.cpon every platform, and the shell'scpbuiltin throughNewAsyncCpTask<true>) callsSelf::destroy(ptr::from_mut(self))inline on both of its completion paths. The shell's mini-loop path added one more&mutframe (run_from_js_thread_mini), and the two dispatch arms in src/runtime/dispatch.rs formed the&mutwith(*task.ptr.cast::<AsyncCpTask>()).run_from_js_thread().destroyis an unconditionalheap::take. A reference argument is protected for the duration of the call it was passed to, and deallocating protected memory is undefined behaviour under both aliasing models regardless of whether the reference is touched again afterwards: Stacked Borrows reports it asdeallocating while item is strongly protected, Tree Borrows (whatbun run rust:miriuses) asthe strongly protected tag disallows deallocations, pointing at the&mut selfreceiver. Spelling the receiver as a raw pointer at the call site does not change that; the protected argument is the method's own receiver. No crash is known from this.Fix
The completions own their allocation instead of freeing it from behind a reference (RAII, as suggested in review):
run_from_js_threads takeself: Box<Self>. The dispatch arms (and the Windows__fs_runtable) reclaim the box withheap::take(task.ptr)and call it, the same shape as theStatWatcherTimerUpdate/AsyncModule/ValkeyDeferredClosearms next to them; the shell's mini-loop thunk does the same, sorun_from_js_thread_minigoes away (it was only reachable for the shell specialization, whose result is alwaysOk, which the thunk now asserts). The box drops on every return path, including the twoErrreturns from the conversion helpers that previously leaked the task.destroy()did moves intoDropimpls on the two task types, sodestroy()is deleted andTaskable::release_unrunbecomesheap::destroy. Field drops (promise handle, protected arguments) run in the same order as before, after the unref.Box<Self>argument is what the aliasing models (and clippy'sboxed_local, suppressed here the same way as at the other reclaim points in the tree) are designed around: a box may be deallocated by the callee that owns it, a reference may not.The only order-of-operations change is in the cp task, which used to destroy itself before settling the promise (hence its raw
*mut JSPromisecopy and the comments explaining why the promise outliveddestroy); it now drops after settling, likeUVFSRequestalready did and like the by-valueAsyncFSTask::then. Everything still happens inside the same task turn, before microtasks drain.c_voiddrops out of the file's imports because the removed parameter was its last non-Windows use.Tests
test/internal/source-lints/self-receiver-teardown.test.ts bans
destroy(..)/deinit(..)/finalize(..)applied to a pointer spelled fromself(ptr::from_mut(self),from_mut(&mut *self),self as *mut _,&raw mut *self,addr_of_mut!(*self),NonNull::from(self), optionally parenthesized or.cast()ed, turbofish allowed), and the deferred formsscopeguard::guard(<that pointer>, |p| .. destroy(p))andscopeguard::guard(<that pointer>, Self::destroy); the first of those is the shapeUVFSRequesthad and a direct-call pattern cannot see it, the second is what clippy'sredundant_closureturns the first into when the callee is a safe fn. The per-file step is onescanText()(strip full-line comments, match, attribute lines); the ~50 positive and negative snippets go through it, and a small fixture (the three shapesmainhad, behind a prose mention and with a SAFETY comment inside the guard's closure) pins its exact{line, text}output, so the pipeline stays exercised after the allowlist below has emptied. Againstmainit reports exactlyand nothing else beyond the files carrying the same shape, which are allowlisted by exact spelling and count (any other spelling in those files is still reported) so each conversion deletes its entry: src/install/lifecycle_script_runner.rs (5, #37551), src/bundler/ThreadPool.rs (1, #37685), src/runtime/webcore/blob/copy_file.rs (2) and read_file.rs (1, both #37705). Refcount releases and the
heap::take/Box::from_rawprimitives are documented as out of scope (#37672 covers the latter).#37685 and #37705 carry a copy of this lint at the same path with their own file's sites allowlisted and this file's three listed against this PR. The identical path is deliberate: whichever PR lands second gets a conflict on the file instead of landing a stale ratchet entry on
main, and resolves it by keeping one copy and deleting the entries for the conversions that have landed. The copies have been kept in step (this one currently has the fn-value guard form, the fixture and the spelling-bound allowlist on top of the shared regexes).Verification
Debug (ASAN) build on Linux: test/js/node/fs/cp.test.ts and cp-symlink-target.test.ts (49 pass), the 36 upstream
test-fs-cp-async-*/test-fs-cp-promises-*scripts under test/js/node/test/parallel (all pass),fs.promises.cpkeeping the process alive until it settles and the process exiting afterwards (theDropunref), and, since the shellcpbuiltin is off on POSIX withoutBUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1, an ad hoc run with it on: 20 recursive$\cp -R`copies plus the error and-vpaths from JS (the JS event loop arm), and recursive, error,-vand&&-chained copies throughbun exec(the mini event loop thunk).bun test test/internal/source-lints/(18 files, 82 tests) passes, and the new lint fails againstmain's node_fs.rs as shown above. On a Windows debug build of the same head: test/js/node/fs/promises.test.js and cp.test.ts (74 pass), the readv/writev/statfs/promises subset of fs.test.ts (98 pass; the one failure,fs/promises > writeFile, fails identically onmainwhen the file is run with that filter because it relies on a directory an earlier test creates), and test/js/bun/shell/commands/cp.test.ts (30 pass, which on Windows runs thecpbuiltin throughNewAsyncCpTaskon both the JS loop and, in the(exec)variants, the mini loop).cargo check -p bun_runtime --target x86_64-pc-windows-msvcalso passes;cargo clippy -p bun_runtimeandcargo fmt --check` are clean.