node:fs: share the recursive readdir scan between pool threads by pointer, not &mut self by robobun · Pull Request #37861 · oven-sh/bun

robobun · 2026-08-12T15:10:54Z

Problem

No user-visible breakage is known: readdir(.., { recursive: true }) on the promise and callback paths returns the right entries. What is wrong is the shape of the shared state under Rust's aliasing rules; Miri rejects it (reduction in the collapsed original).
One recursive scan is a single heap object shared by every pool thread working on it, and each thread reached it through &mut. A &mut claims the whole object, so a directory with two subdirectories is enough: the second subtask's count bump invalidates the reference the first subtask was handed. There is no data race, since the bytes written are atomics, a lock-free queue and a locked error.
The thread that finished a scan handed the object to the JS thread from inside a method still holding &mut self (for the root, plus an argument pointing into the object), so the JS thread freed memory those references still protected. This happened on every scan.
Same class as blob: hand the read handler's pointer to on_read_bytes instead of &mut self #37681 / blob(windows): free CopyFileWindows and ReadFileUV through the task pointer, not under &mut self #37705 / Hand intrusive work-pool tasks to the pool through the object's pointer, not a reference #37768 / io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787 / Pass the job to JobContext::run and FileOpener::get_fd by pointer, not &mut #37820. Pass the job to JobContext::run and FileOpener::get_fd by pointer, not &mut #37820 converts the frame above this one; the two overlap by two lines and are otherwise independent.

Fix

While a thread holds one of the scan's counts it reaches the scan through a shared reference only; the fields written during the fan-out become interior mutable (the error behind a lock, the root fd in a Cell). Property to check: no thread holds &mut to the scan while another thread can touch it. Same shape as the cp fan-out earlier in the same file.
The two frames that drop a count hold the scan as a raw pointer only. The last thread out does the exclusive work (close the root fd, join or discard the lists) in a scoped call that returns the completion token, and hands the object over once that call has returned. Property to check: after a thread's decrement lands it holds no reference into the object, so another thread freeing it is fine.
The decrement becomes AcqRel, so the finishing thread reads the queue and the recorded error after every other thread's writes; with Relaxed that was formally unordered. The separate "already finished" flag is deleted because the count reaches zero exactly once.
Behaviour is meant to be unchanged (same fd lifetime, same first-error-wins and path fallback, same join). Verification: a new source lint fails on main and passes here; the Miri result is from a standalone reduction, not the real code; the real code passed the existing readdir tests, Node's readdir tests and two new encoding: "buffer" tests (which also pass before the fix) on a debug ASAN build; cargo check for host and Windows, clippy and rustfmt are clean.

Background

The recursive async readdir is a fan-out: the job's own pool callback walks the root and schedules one pool subtask per subdirectory. A count covers the root walk plus every subtask; the thread that decrements it to zero finishes the scan and hands the object to the JS thread, which reads and frees it.
In Rust, &mut T claims exclusive access to all of T while the reference is live. A write through any other path, even to an atomic field with no data race, makes further use of that reference undefined behaviour. Miri's Stacked Borrows and Tree Borrows are the checkers; bun run rust:miri uses Tree Borrows.
A reference passed as a function argument is also "protected" until that call returns: freeing the object while the frame is on the stack is undefined behaviour even if the reference is never used again. So a frame that may trigger the free can hold only a raw pointer.
bun_ptr::ParentRef is bun's back-pointer from a subtask to the object that owns it, kept alive by that object's count. It derefs to &T; assume_mut() forms a &mut from it and as_mut_ptr() yields the raw pointer. bun_threading::Guarded<T> is a mutex that owns the value it guards.
test/internal/source-lints/ holds bun tests that regex-scan the Rust tree for banned shapes; the new one bans assume_mut() inside pool-task types and requires the count-dropping entry points of both fan-outs to take a *mut.

Original description

What

readdir(.., { recursive: true }) on the promise/callback path is one AsyncReaddirRecursiveTask (src/runtime/node/node_fs.rs, mod _async_tasks) shared by every pool thread working on the scan: the job's own pool callback walks the root, enqueue schedules a ReaddirSubtask per subdirectory, subtask_count counts them, and whichever thread's decrement reaches zero joins the result lists and finishes the Completion, after which the JS thread reads and frees the whole Job (Job::complete, src/jsc/job.rs) through the job's own pointer.

Every pool-side method on it took &mut self: perform_work, enqueue, write_results, finish_concurrently, and the walker readdir_with_entries_recursive_async(.., &mut AsyncReaddirRecursiveTask, ..). ReaddirSubtask::run_owned got its &mut with readdir_task.assume_mut(). So:

The root walk, every running subtask and the finishing thread each held an exclusive reference to the same object at the same time. The bytes they actually touch are atomics, the lock-free queue and the mutex-guarded error, so there is no data race, but each &mut claims the whole object: the second enqueue in a directory is a fresh &mut reborrow whose fetch_add is a foreign write to the pointer the first subtask was handed, which the model then refuses to let the subtask reborrow. A directory with two subdirectories is enough.
The thread that drops the last count posted the job from inside finish_concurrently(&mut self), called from write_results(&mut self) / perform_work(&mut self) (and, for the root, with basename: &ZStr pointing into root_path), so the JS thread frees memory those arguments still protect. That happens on every scan, on whichever thread finishes it; the other threads' frames are exposed to the same free racily, as soon as their own decrement has landed.

No crash is known: this is the contract shape the aliasing models reject, the same class as #37681 / #37705 / #37768 / #37787 / #37820. #37820 converts the carrier frame above this (JobContext::run) and still calls into perform_work by reference; this PR is everything below it, and the two are independent apart from the two lines in run. Found by the work on #37820.

Standalone reduction under Miri (both shapes rejected under Tree Borrows and Stacked Borrows, the pointer shape accepted under both)

Scan stands for the task, Token for the Completion (finishing it frees the job on another thread, as Job::complete does), the spawned threads for the subtasks; channels pin down one interleaving the real code produces routinely.

struct Job { off: Scan }
struct Scan { subtask_count: AtomicUsize, result_list_count: AtomicUsize, done: Option<Token> }
#[derive(Clone, Copy)] struct Token(*mut Job);
unsafe impl Send for Token {}
impl Token {
    fn finish(self) {   // Completion::finish + the JS thread's Job::complete
        thread::spawn(move || { let t = self; drop(unsafe { Box::from_raw(t.0) }) }).join().unwrap();
    }
}
#[derive(Clone, Copy)] struct SendPtr(*mut Scan);
unsafe impl Send for SendPtr {}
fn new_job() -> *mut Job {
    Box::into_raw(Box::new(Job { off: Scan { subtask_count: AtomicUsize::new(1), result_list_count: AtomicUsize::new(0), done: None } }))
}

// ---- as it was ----
impl Scan {
    fn enqueue(&mut self, go: mpsc::Receiver<()>) -> thread::JoinHandle<()> {
        self.subtask_count.fetch_add(1, Ordering::Relaxed);
        let p = SendPtr(std::ptr::from_mut(self));
        thread::spawn(move || { let p = p; go.recv().unwrap(); unsafe { &mut *p.0 }.perform_work() })  // ParentRef::assume_mut
    }
    fn perform_work(&mut self) { self.write_results(); }
    fn write_results(&mut self) {
        self.result_list_count.fetch_add(1, Ordering::Relaxed);
        if self.subtask_count.fetch_sub(1, Ordering::AcqRel) == 1 { self.finish_concurrently(); }
    }
    fn finish_concurrently(&mut self) { self.done.take().unwrap().finish(); }
}

// (1) a directory with two subdirectories
fn mut_overlap() {
    let job = new_job();
    let scan = unsafe { &mut (*job).off };
    scan.done = Some(Token(job));
    let (go_a, wait_a) = mpsc::channel();
    let (go_b, wait_b) = mpsc::channel();
    let a = scan.enqueue(wait_a);
    let b = scan.enqueue(wait_b);
    go_a.send(()).unwrap(); a.join().unwrap();
    go_b.send(()).unwrap(); b.join().unwrap();
    scan.write_results();
}
//   error: Undefined Behavior: reborrow through <767> is forbidden            (the `&mut *p.0` in the first subtask)
//   help: the accessed tag <767> has state Disabled which forbids this reborrow
//   help: <767> was created here: `SendPtr(std::ptr::from_mut(self))`        (first enqueue)
//   help: later transitioned to Disabled due to a foreign write access: `self.subtask_count.fetch_add(..)`   (second enqueue)

// (2) a directory with no subdirectories at all
fn mut_free() {
    let job = new_job();
    unsafe { (*job).off.done = Some(Token(job)); (*job).off.perform_work(); }
}
//   error: Undefined Behavior: reborrow through <403> is forbidden            (Box::from_raw on the freeing thread)
//   help: the accessed tag <403> is foreign to the protected tag <429> (i.e., it is not a child)
//   help: protected tags must never be Disabled
//   help: the protected tag <429> was created here: `fn finish_concurrently(&mut self)`

// ---- as it is now ----
impl Scan {
    fn enqueue(this: *mut Self, go: mpsc::Receiver<()>) -> thread::JoinHandle<()> {
        unsafe { (*this).subtask_count.fetch_add(1, Ordering::Relaxed) };
        let p = SendPtr(this);
        thread::spawn(move || { let p = p; go.recv().unwrap(); unsafe { Scan::perform_work(p.0, None) }; })
    }
    unsafe fn perform_work(this: *mut Self, subdirs: Option<Vec<mpsc::Receiver<()>>>) -> Vec<thread::JoinHandle<()>> {
        let spawned = {
            let scan = unsafe { &*this };                                   // the walk: shared, scoped
            let spawned: Vec<_> = subdirs.into_iter().flatten().map(|go| Scan::enqueue(this, go)).collect();
            scan.result_list_count.fetch_add(1, Ordering::Relaxed);
            spawned
        };
        unsafe { Scan::on_subtask_done(this) };
        spawned
    }
    unsafe fn on_subtask_done(this: *mut Self) {
        if unsafe { (*this).subtask_count.fetch_sub(1, Ordering::AcqRel) } != 1 { return; }
        let done = unsafe { (*this).done.take() }.unwrap();
        done.finish();
    }
}
// The same two scenarios (the root dropping its count first, so a subtask finishes and frees the
// job while the other subtask's pointer copy still exists; and the root-only scan) both run clean.

Under Stacked Borrows (Miri's default) the same two cases fail with "trying to retag from <786> for Unique permission ... but that tag does not exist in the borrow stack" and "not granting access to tag <418> because that would remove [Unique for <446>] which is strongly protected".

Fix

Same shape as NewAsyncCpTask a few hundred lines up (cp_async(.., this: *mut Self) / CpSingleTask::run_owned / on_subtask_done(this: *mut Self)):

The shared phase goes through ReaddirScanRef (ParentRef<AsyncReaddirRecursiveTask, Mut>, the type the subtask already stored) and &self. enqueue(scan, ..) and the walker take the handle, which also removes the two detach_lifetime round-trips the &mut signatures had forced. What is written during the fan-out is now interior mutable: pending_err is a bun_threading::Guarded<Option<Error>> (as in the shell's rm), root_fd a Cell (written by the root walk before anything is enqueued and by the finishing thread, read shared in between; same as cp's result). has_result is deleted: the count reaching zero already happens exactly once, and done.take().expect(..) keeps the check.
The frames that drop a count hold the scan by pointer only: perform_work(this: *mut Self, subdir: Option<&ZStr>) (the root's basename is now borrowed inside scan_directory, which has returned by the time the count is dropped; the subtask's is its own allocation) and on_subtask_done(this: *mut Self). The last one out does the exclusive work in finish_scan(&mut self), which returns the Completion instead of finishing it, so the hand-over happens from the pointer frame with no borrow live. scan_directory::<T> replaces the impl_tag! macro (whose $variant argument was unused); push_results / record_error are the &self halves of the old write_results and error branch.
The decrement is AcqRel, as in on_subtask_done for cp: the finishing thread's pop_batch and its read of pending_err (outside the lock) are then ordered after every other thread's pushes and error record; with Relaxed they were formally unordered.

Behaviour is meant to be unchanged: same fd lifetime, same first-error-wins with the same path fallback, same join, same then.

Tests

test/internal/source-lints/self-receiver-fan-out.test.ts: inside the impl blocks of every owned_task! type (five in the tree), assume_mut() is banned; and the count-dropping entry points of the two fan-outs (NewAsyncCpTask::{cp_async, on_subtask_done}, AsyncReaddirRecursiveTask::{perform_work, on_subtask_done}) must take the object as *mut with no receiver or &Self parameter, with a check that the listed functions still exist so a rename cannot make it vacuous. With src/ at main it reports node_fs.rs:2332 (the subtask's assume_mut()) and node_fs.rs:2410: fn perform_work(&mut self, ..); with this branch the whole test/internal/source-lints/ directory passes (88 tests).
test/js/node/fs/fs.test.ts: the encoding: "buffer" variant of the recursive promise walk was not covered (the Node comparison tests cover strings and Dirents), and it is now the third instantiation of the same body, so two tests are added next to the existing buffer/ELOOP ones: a join across nested subdirectories including an empty one, and a rejection after subtasks have already queued entries (exercising the discard). They pass before and after; the behavioural coverage of the conversion is the existing fs.test.ts -t readdir set (the Node comparisons, the x100 concurrent scans, the 64-failing-subtasks test, the MAX_PATH_BYTES test).

Verification

Debug (ASAN) build: fs.test.ts -t readdir (30 pass), test/js/node/test/parallel/test-fs-readdir-{recursive,types,types-symlinks}.js, test/internal/source-lints/ (88 pass), and the stash round-trip above. cargo check -p bun_runtime for the host and x86_64-pc-windows-msvc, cargo clippy -p bun_runtime and rustfmt --check are clean.

…t &mut AsyncReaddirRecursiveTask is one heap object shared by every pool thread working on a readdir({ recursive: true }) scan, kept alive by subtask_count, and handed to the JS thread (which frees it) by the thread that drops the last count. Its pool-side methods took &mut self: the root walk, every subtask (ParentRef::assume_mut) and the finishing thread each held an exclusive reference to the same object at once, and the finishing thread posted the job, letting the JS thread free it, from under its own &mut self frames. Both aliasing models reject that (see the lint's header). The shared phase now goes through a ReaddirScanRef (ParentRef) and &self, with the state written during the fan-out interior mutable: pending_err is a Guarded, root_fd a Cell; has_result is gone, as the count reaching zero already happens exactly once. The frames that drop a count, perform_work and on_subtask_done, take the scan by pointer; the last one out does the exclusive work in finish_scan(&mut self), which returns the completion so that the hand-over happens with no borrow live. The decrement is AcqRel, as in NewAsyncCpTask::on_subtask_done, so the finishing thread's join and its read of pending_err are ordered after the other threads' pushes. The walker takes the handle instead of &mut, which also removes the detach_lifetime round-trips. test/internal/source-lints/self-receiver-fan-out.test.ts bans assume_mut() inside the impl blocks of owned_task! types and a self receiver or &Self parameter on the count-dropping entry points of the two fan-outs; with the previous node_fs.rs it reports the subtask's assume_mut() and perform_work(&mut self, ..). fs.test.ts gains the buffer-encoding variant of the recursive promise walk (a join across subdirectories, and a discard after a subtask fails), which the Node comparison tests did not reach.

coderabbitai · 2026-08-12T15:11:03Z

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 83a562db-e247-43f3-958b-33086a9b3b3e

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and e293675.

📒 Files selected for processing (3)

src/runtime/node/node_fs.rs
test/internal/source-lints/self-receiver-fan-out.test.ts
test/js/node/fs/fs.test.ts

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-08-12T15:11:06Z

^{Updated 2:05 PM PT - Aug 12th, 2026}

✅ @robobun, your commit e293675b4114c1d286f157bfb6aa6cb9f68f8cd1 passed in Build #93316! 🎉

🧪 To try this PR locally:

bunx bun-pr 37861

That installs a local version of the PR into your bun-37861 executable, so you can run:

bun-37861 --bun

robobun · 2026-08-12T15:13:00Z

Status: ready for review.

Reproduced as a model violation rather than a crash: the standalone Miri reduction in the description (Tree Borrows and Stacked Borrows) rejects the two shapes the scan had, a directory with two subdirectories and a scan finishing from under its own &mut self frames, and accepts the pointer shape this branch uses. In-tree, test/internal/source-lints/self-receiver-fan-out.test.ts reports node_fs.rs:2332 and node_fs.rs:2410 against main's src/ and passes with this branch; fs.test.ts -t readdir passes on the debug (ASAN) build before and after, as the behaviour is unchanged.

claude

I reviewed this PR and didn't find any bugs. Because it restructures the concurrent unsafe ownership model of AsyncReaddirRecursiveTask (raw-pointer receivers, refcount ordering, interior-mutable field conversions), a human look would still be worthwhile.

What was reviewed:

ParentRef is Copy and Deref-only, so passing scan by value into enqueue/the walker while basename borrows scan.root_path holds only shared borrows — no aliasing conflict.
root_fd: Cell<FD> write ordering: set by the root before any enqueue, read shared by subtasks, replaced in finish_scan after the last count — no overlap with a shared read.
has_result removal is safe: fetch_sub == 1 fires exactly once and done.take().expect(..) still guards double-finish.
FdExt is already in scope at file top (line 21), so dropping the local use before root_fd.close() is fine; AtomicBool is still used by NewAsyncCpTask.

Extended reasoning...

Overview

This PR converts AsyncReaddirRecursiveTask (the pool-side state for fs.promises.readdir({recursive: true})) from &mut self receivers to a *mut Self / &self split, mirroring the existing NewAsyncCpTask shape a few hundred lines up. The shared fan-out phase now goes through ReaddirScanRef (a Copy ParentRef<_, Mut>) with interior-mutable fields (Guarded<Option<Error>>, Cell<FD>, atomics, the lock-free queue); the count-dropping frames (perform_work, on_subtask_done) hold only *mut Self; and finish_scan(&mut self) returns the Completion rather than finishing it under a live borrow. The impl_tag! macro is replaced by a generic scan_directory::<T>; write_results is split into push_results/record_error; the join loop moves into join_result_lists. The decrement ordering is upgraded from Relaxed to AcqRel. It also adds a 279-line source lint enforcing this shape across all owned_task! types, and two encoding: 'buffer' recursive-readdir tests.

Security risks

None. This is an internal aliasing-model refactor of an existing concurrent task; no new inputs, no parsing, no auth/crypto surface.

Level of scrutiny

High. Per REVIEW.md, native memory safety is the most-blocked category, and this touches thread affinity, reference-count balancing on every terminal path, raw-pointer receivers with hand-written SAFETY contracts, and memory-ordering changes. fs.readdir({recursive: true}) is a production hot path. The reasoning in the PR description is very thorough (Miri reductions under both aliasing models, explicit mapping to the sibling NewAsyncCpTask pattern), and I checked that ParentRef is Copy+Deref-only, Guarded::get_mut exists for the &mut self sites in then/finish_scan, the file-level FdExt import covers the removed local one, and AtomicBool is still used by NewAsyncCpTask so no dead import. But a change of this shape — deleting an atomic guard (has_result), changing decrement ordering, and rewriting every pool-side entry point — warrants a human maintainer's read, not auto-approval.

Other factors

The PR acknowledges that JobContext::run(this: &mut Self, ...) still holds a protected &mut across perform_work and defers that to #37820; the two are stated to be independent. The source lint has anti-vacuity checks (verifies the named entry points still exist, verifies a non-empty scan set). The new fs tests are stated to pass before and after (they cover the previously-untested buffer variant, not the aliasing fix itself). No prior human or bot reviews with substantive feedback; CI is still building.

github-actions Bot added the claude label Aug 12, 2026

claude Bot reviewed Aug 12, 2026

View reviewed changes

robobun mentioned this pull request Aug 12, 2026

shell rm: drop a DirTask's own count from a pointer-only frame #37907

Open

Navigation Menu

Uh oh!

node:fs: share the recursive readdir scan between pool threads by pointer, not &mut self - #37861

node:fs: share the recursive readdir scan between pool threads by pointer, not &mut self#37861
robobun wants to merge 1 commit into
mainfrom
farm/5e94f33d/readdir-scan-by-pointer

robobun commented Aug 12, 2026 •

edited by alii

Loading

Uh oh!

coderabbitai Bot commented Aug 12, 2026

Review limit reached

Uh oh!

robobun commented Aug 12, 2026 •

edited

Loading

Uh oh!

robobun commented Aug 12, 2026

Uh oh!

claude Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

robobun commented Aug 12, 2026 • edited by alii Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Background

What

Fix

Tests

Verification

Uh oh!

coderabbitai Bot commented Aug 12, 2026

Review limit reached

Uh oh!

robobun commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Aug 12, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Aug 12, 2026 •

edited by alii

Loading

robobun commented Aug 12, 2026 •

edited

Loading