From 8def9dd13ee53d25ec68827d464318e17324e778 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 27 Aug 2026 20:46:21 +0000 Subject: [PATCH] node:fs: stop the async recursive readdir walk at the first error fs.readdir and fs.promises.readdir with recursive: true run one pool subtask per directory. A failing subtask recorded its error and released its reference, but every other subtask kept scheduling subtasks for the directories it found, and the promise settled only when the whole frontier had drained. A tree with two directory symlink loops has a 2^41 directory frontier before the kernel reports ELOOP, so the promise never settled and every pool thread spun. Set a flag with the first error. enqueue then schedules nothing, and a subtask that starts after the flag is set releases its reference without opening its directory. The subtask_count decrements are AcqRel so the last subtask sees the other subtasks' pending_err and queued results. --- src/runtime/node/node_fs.rs | 29 +++++++++++++++++++--- test/js/node/fs/fs.test.ts | 49 +++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index 66c12b72fed7..466f65ab71b5 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -2132,6 +2132,13 @@ mod _async_tasks { pub(crate) subtask_count: AtomicUsize, + /// Set once `pending_err` is. From then on `enqueue` schedules nothing + /// and a subtask that starts skips its directory, so the scan settles as + /// soon as the subtasks already in flight return instead of draining the + /// whole frontier. Two directory symlinks back to the same tree make + /// that frontier 2^41 paths deep before the kernel reports ELOOP. + pub(crate) has_error: AtomicBool, + /// The final result list pub(crate) result_list: ResultListEntryValue, @@ -2292,6 +2299,9 @@ mod _async_tasks { impl AsyncReaddirRecursiveTask { pub(crate) fn enqueue(&mut self, basename: &ZStr) { + if self.has_error.load(Ordering::Relaxed) { + return; + } // The subtask runs on another thread after the caller's `name_to_copy_z` // (which points into a per-iteration buffer) has been overwritten, so we // must heap-own the bytes here. Freed in ReaddirSubtask::call's cleanup. @@ -2351,6 +2361,7 @@ mod _async_tasks { done: None, has_result: AtomicBool::new(false), subtask_count: AtomicUsize::new(1), + has_error: AtomicBool::new(false), root_path, result_list, result_list_count: AtomicUsize::new(0), @@ -2370,6 +2381,10 @@ mod _async_tasks { buf: &mut PathBuffer, is_root: bool, ) { + if self.has_error.load(Ordering::Relaxed) { + self.on_subtask_done(); + return; + } macro_rules! impl_tag { ($T:ty, $variant:ident) => {{ // A bare `Vec::new()` here @@ -2401,9 +2416,8 @@ mod _async_tasks { self.pending_err = Some(err.with_path(err_path)); } } - if self.subtask_count.fetch_sub(1, Ordering::Relaxed) == 1 { - self.finish_concurrently(); - } + self.has_error.store(true, Ordering::Relaxed); + self.on_subtask_done(); } Ok(()) => { self.write_results::<$T>(&mut entries); @@ -2439,7 +2453,14 @@ mod _async_tasks { }; } - if self.subtask_count.fetch_sub(1, Ordering::Relaxed) == 1 { + self.on_subtask_done(); + } + + /// Drops this subtask's `subtask_count` reference. The last one finishes + /// the scan; `AcqRel` publishes every subtask's `pending_err` and queued + /// results to it. + fn on_subtask_done(&mut self) { + if self.subtask_count.fetch_sub(1, Ordering::AcqRel) == 1 { self.finish_concurrently(); } } diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 4a95daebc05b..a515c73223d7 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -1834,6 +1834,55 @@ it.skipIf(isWindows)("promises.readdir({recursive: true}) settles when multiple }); }); +// After the first subtask failed, the walker kept scheduling a subtask for every +// directory it found and only settled once that frontier drained. Two symlinks +// back to the root make the frontier 2^41 directories (the kernel follows 40 +// symlinks before ELOOP), so the promise and the callback never settled. +it.skipIf(isWindows)( + "readdir({recursive: true}) settles with the first error while symlink loops are still being walked", + async () => { + using dir = tempDir("readdir-recursive-loop", { + "a/b/keep.txt": "x", + }); + const root = String(dir); + // Opening this one with O_DIRECTORY fails with ELOOP at once. + fs.symlinkSync(join(root, "bad"), join(root, "bad")); + fs.symlinkSync(".", join(root, "loop1")); + fs.symlinkSync(".", join(root, "loop2")); + + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const fs = require("fs"); + const root = ${JSON.stringify(root)}; + const viaPromise = await fs.promises.readdir(root, { recursive: true }).then( + r => "resolved " + r.length, + e => "rejected " + e.code, + ); + console.log("promise", viaPromise); + const { promise, resolve } = Promise.withResolvers(); + fs.readdir(root, { recursive: true }, (e, r) => resolve(e ? "rejected " + e.code : "resolved " + r.length)); + console.log("callback", await promise); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "inherit", + timeout: 10_000, + }); + + const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); + + expect({ stdout: stdout.trim(), exitCode, signalCode: proc.signalCode }).toEqual({ + stdout: "promise rejected ELOOP\ncallback rejected ELOOP", + exitCode: 0, + signalCode: null, + }); + }, +); + describe("readSync", () => { it("rejects the read when the length argument detaches the destination buffer during coercion", () => { const fd = openSync(import.meta.dir + "/readFileSync.txt", "r");