Conversation
The callback and promises forms of readdir({ recursive: true }) stored the
raw openat()/getdents() error from the walk, so a missing or unreadable
directory surfaced with syscall "open" while readdirSync and Node report
"scandir". Stamp the syscall where the async task records its pending
error, matching what NodeFS::readdir does for the sync walk.
|
Warning Review limit reached
Next review available in: 9 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 (2)
Comment |
|
Status: ready for review (diff is green; the remaining CI red is unrelated). Reproduced on bun 1.4.0 with the snippet in the description: callback and promises Tests: CI: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line swap to with_path_and_syscall(.., sys::Tag::scandir) brings the async recursive readdir error path in line with the sync path and Node.
What was reviewed:
- Confirmed
NodeFS::readdir(sync) already forcessyscall: scandirat node_fs.rs:6320; this change applies the same normalization at the only async-recursive error store site. - Verified
with_path_and_syscallpreserveserrnoand copies the path exactly like the previouswith_pathcall, so only the syscall tag changes. - Test covers ENOENT/ENOTDIR/EACCES across all three result kinds and all three API forms; EACCES case correctly gated off Windows/root and restores chmod in
finally.
Extended reasoning...
Overview
The PR changes one line in AsyncReaddirRecursiveTask::perform_work (src/runtime/node/node_fs.rs:2450), replacing err.with_path(err_path) with err.with_path_and_syscall(err_path, sys::Tag::scandir). It adds a 90-line describe block to test/js/node/fs/fs.test.ts covering 9 cases (3 result kinds × 3 failure scenarios) across sync/callback/promises readdir.
Security risks
None. This only relabels the syscall field of an already-surfaced error object. No new inputs are parsed, no allocations change, no control flow is altered. with_path_and_syscall (src/sys/Error.rs:231) is the established helper already used by truncate/utimes/lutimes in the same file and by path_watcher; it preserves errno and clones the same path bytes as the old with_path call.
Level of scrutiny
Low. This is a targeted Node.js error-shape compat fix. The sync readdir path at node_fs.rs:6319-6325 already wraps every failure as scandir; the async recursive task was the one place skipping that normalization. The PR description traces the root cause precisely (openat tag from readdir_with_entries_recursive_async leaking through) and cites Node's uv_fs_scandir as the reference. The change is at the right layer — the single choke point where pending_err is set — so all three ReaddirTag variants and all failure modes (root open, mid-walk iterator errors) are covered by the one edit.
Other factors
Test quality is solid: uses tempDir, it.concurrent, asserts full {code, syscall, path, message} shape (not just syscall), correctly skips the EACCES case when running as root or on Windows with a stated reason, and restores mode bits in try/finally before the tempDir cleanup. The PR notes the pre-existing sync-walker err.path discrepancy for the EACCES subdir case and deliberately scopes it out (asserted via toMatchObject on code/syscall only), which is the right call for a focused fix. The comment added at the change site is one line and points at the sync equivalent — appropriate per repo conventions.
|
Updated 4:44 AM PT - Aug 13th, 2026
❌ @robobun, your commit 3060cac has 1 failures in 🧪 To try this PR locally: bunx bun-pr 37988That installs a local version of the PR into your bun-37988 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line fix that tags the async recursive readdir error with scandir, mirroring what the sync path at node_fs.rs:6318 already does. The earlier inline nit about the two-line test comment (1777-1778) still stands but is non-blocking.
What was reviewed
- Confirmed
with_path_and_syscallis the established helper for this (same file: truncate/utime/lutime; alsopath_watcher.rs) and preserves errno/path while overriding only the syscall tag. - Checked that this is the single error sink for all three async result variants (
Files/WithFileTypes/Buffers— the macro dispatches all three through this line). - Test structure: hermetic (
tempDir), covers ENOENT/ENOTDIR/EACCES × sync/callback/promises × 3 result kinds; EACCES correctly gated on!isWindows && uid !== 0with chmod restored infinally.
Extended reasoning...
Overview
The PR changes one line in src/runtime/node/node_fs.rs (AsyncReaddirRecursiveTask::perform_work), swapping err.with_path(err_path) for err.with_path_and_syscall(err_path, sys::Tag::scandir). This normalizes the JS-visible err.syscall to "scandir" for async recursive readdir failures, matching both the sync path in the same file (line 6318 already builds sys::Error { syscall: sys::Tag::scandir, ... }) and Node.js, which reports every readdir failure through uv_fs_scandir. A ~90-line test block in test/js/node/fs/fs.test.ts exercises 9 combinations (3 result kinds × 3 failure modes) across sync, callback, and promises forms.
Security risks
None. This adjusts only the syscall label on an error object that was already being surfaced with the same errno and path. No new inputs are parsed, no new syscalls are made, no permissions logic is touched.
Level of scrutiny
Low. The Rust change is a one-token swap to a helper that is already used in three other places in this file for the identical purpose (retagging an error before it reaches JS). I verified with_path_and_syscall in src/sys/Error.rs:231 — it copies errno, sets syscall, and clones the path, so no field is silently dropped relative to the previous with_path call. The change site sits inside a macro expanded for all three ReaddirTag variants, so the fix covers every result kind uniformly — no sibling site is missed.
Other factors
The comment-cop bot's earlier concern about a paragraph-length comment at the Rust call site was addressed in ebdc481 (comment removed; the sync twin carries no comment either). My previous inline nit about the test-file comment narrating the pre-fix behavior remains open, but I labeled it a nit at the time and it has zero functional impact — the tests themselves are correct, hermetic (each case gets its own tempDir), assert exact code/syscall/path/message, and restore the locked directory's mode in a finally so cleanup survives assertion failures. The PR description confirms the tests fail on 1.4.0 and pass with the change, satisfying the fails-for-the-right-reason bar.
There was a problem hiding this comment.
LGTM — the comment nit from the last pass was addressed in 3060cac, and no issues remain.
What was reviewed:
- The one-line Rust change swaps
with_pathforwith_path_and_syscall(…, sys::Tag::scandir), mirroring what the sync path already does at node_fs.rs:6319 and what Node reports for every readdir failure. - Confirmed
with_path_and_syscall(src/sys/Error.rs:231) preserves errno and only overrides the syscall tag and path, so no error information is lost. - Checked the test: hermetic
tempDirfixtures,it.concurrent, chmod restored infinally, EACCES case correctly skipped on Windows/root, and the sync form's divergenterr.pathis intentionally not asserted (noted in the PR as a separate pre-existing issue).
Extended reasoning...
Overview
One functional line in src/runtime/node/node_fs.rs: AsyncReaddirRecursiveTask::perform_work now stores err.with_path_and_syscall(err_path, sys::Tag::scandir) instead of err.with_path(err_path). This normalizes the JS-visible err.syscall for async recursive readdir failures to "scandir", matching readdirSync (which already wraps into sys::Tag::scandir at node_fs.rs:6319) and Node.js (which only ever reports uv_fs_scandir). The accompanying test in fs.test.ts adds a 3×3 matrix (result kind × failure mode) asserting {code, syscall, path, message} for the sync, callback, and promises forms.
Security risks
None. The change only relabels the syscall tag on an error object that was already being surfaced to JS with the same errno and path. No new inputs are parsed, no permissions are altered, no resources are opened.
Level of scrutiny
Low-to-medium. This is a targeted Node-compat fix on error metadata — no control-flow, allocation, or lifetime changes. I verified with_path_and_syscall in src/sys/Error.rs keeps errno and only sets syscall + path, and confirmed the sync twin already applies the same tag. The single store in perform_work is the only place the async recursive walk shapes its error, so the fix covers the whole class (root ENOENT/ENOTDIR and mid-walk failures) in one line.
Other factors
Both prior review threads are resolved: comment-cop's flag on the Rust-side comment was fixed in ebdc481, and my nit on the test's change-narration comment was fixed in 3060cac (the diff now shows only the one-line contract comment). The tests follow harness conventions — tempDir with using, it.concurrent, try/finally around chmod, and it.skipIf(isWindows || getuid()===0) for the mode-000 case (a pattern already used elsewhere in the suite). The PR description documents that expected values were taken from Node v26.3.0 and that the tests fail on bun 1.4.0.
…opened (#40000) ### Problem - `fs.readdirSync(root, { recursive: true })` throws `ENOENT: no such file or directory, scandir '<root>'` when another process removes a subdirectory during the call. `root` still exists. `fs.promises.readdir` rejects too. - The walk skips a subdirectory whose open fails with ENOENT, ENOTDIR or EPERM. A removal that lands after the open surfaces as ENOENT from `getdents64` instead. Both walkers in `src/runtime/node/node_fs.rs` returned that error. ### Fix - One predicate, `readdir_skips_subdir`, now covers the open and the read of a subdirectory in both walkers. A tolerated read error ends that subdirectory. Root errors and other errnos are returned as before. - Matches node for this race. libc's `readdir(3)` reports this ENOENT as the end of the directory, so node lists the subdirectory as empty and continues. bun reads with the raw syscall and saw the errno. - The open arm is unchanged. Only timing decides which syscall sees the removal, so the read arm uses the same set. - Verified: the new test in `test/js/node/fs/fs.test.ts`. Its four walk variants fail on bun 1.4.0. Other suites: see the notes. ### Background - Each directory below the root is opened with `openat(root_fd, relative)` and read with `getdents64`, in one loop (sync) or in one subtask per directory (async). Any subtask error rejects the promise. - On Linux, `getdents64` on a directory removed after its open fails with ENOENT (`IS_DEADDIR` in fs/readdir.c). glibc, and the FreeBSD arm of bun's iterator, turn this into end of directory. The Linux arm does not. - The test preloads a shim on libc's `syscall()`, bun's route to `getdents64`. It really removes two marked directories during the walk, and the test checks that both are gone. glibc only. <details><summary>Notes</summary> - Origin: found while #39710 was verified. Recursive rm has the same gap on its own walk and is handled in that thread. The original observation: a root with 128 subdirectories, a second process unlinking their files and removing them, and `readdirSync(root, { recursive: true })` in a loop in the main process. It fired about once in a few hundred iterations. - Repro without the race: the shim from the test, run against bun 1.4.0. Sync: `ENOENT scandir 'root-sync'`. Async: `ENOENT getdents64 'root-async/vanish-mid-read'`. - Raw syscall against libc, same machine (kernel 6.17, glibc 2.41), directory opened and then removed: `getdents64` returns -1 with ENOENT on overlayfs and on tmpfs. glibc's `readdir(3)` on the same state returns NULL with errno 0, that is, end of directory. The kernel side is generic: `vfs_rmdir` sets `S_DEAD` on the inode, and `iterate_dir` returns ENOENT for it before any filesystem code runs. So the errno does not depend on the filesystem, only on whether the reader uses libc or the raw syscall. A probe through Python or any other libc reader shows an empty listing for this reason, not an error. - Node, observed: `fs.opendirSync(dir)`, then `rmdirSync(dir)`, then `dir.readSync()` returns `null` (end of directory) on node v26.3.0. bun returns `ENOENT scandir` there before and after this change. That `opendir` path and a root removed after its open are the same libc difference, on a different consumer of the iterator. This change leaves them alone: it would mean a change to the Linux arm of the iterator, which every directory walker in bun shares (readdir, opendir, cp, rm, glob, the shell builtins). The rm side of that is in flight in #39710. That alignment is a separate change if wanted. - Node with the subdirectory removed before node opens it (a shim on `scandir(3)`): `readdirSync` and `fs.promises.readdir` both throw `ENOENT scandir 'root/vanish-before-read'`. bun skips the subdirectory there, and did so before this change. This change does not widen that difference, it only makes the read step behave like the open step. - The shim removes one directory before its first read (the read fails at once) and one after its first read handed out an entry (the end-of-directory read fails). The `withFileTypes` variants cover the `dirent_path_prev` release on the new `break` path. They pass under the ASAN debug build. - Why the message named the root: `NodeFS::readdir` rebuilds every error of the sync form as `scandir` on the root argument. That is unchanged here and is what #38017 changes. - Unchanged: EACCES, EINVAL, ELOOP, EIO and every other errno still fail the call, from the open and from the read. The root cases in the new test pass before and after this change on purpose. They pin that only subdirectories are skipped. - Open PRs on the same lines, all independent of this one: #38017 (name the failing subdirectory), #37988 (syscall name of the async forms), #33432 (add ELOOP to the skipped set, which becomes a one-line change to the predicate). The test asserts `code` and `path` only, so #37988 and #38017 do not change its expectations. - The walkers are shared by all platforms, so the predicate applies on macOS and Windows too. What those kernels report for a removed open directory was not checked here. The test runs on Linux glibc only. - Suites run with the debug build: all of `test/js/node/fs/fs.test.ts`, `readdirSync-recursive-error-leak.test.ts`, `dir.test.ts`, `fs-path-length.test.ts`, node's `test-fs-readdir*.js`, regression tests 17793, 24007, 28159, 29585. `test/regression/issue/10139.test.ts` (a `bun build` of a 128 MB asset with a 5 s budget) times out in this container with the debug build. It does not use recursive readdir. </details> <!-- robobun:evidence:begin --> --- **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/node/fs/fs.test.ts <!-- robobun:evidence:end -->
This branch has not been deployed
1 participant
Problem
fs.readdir(path, { recursive: true }, cb)andfs.promises.readdir(path, { recursive: true })reporterr.syscall === "open"(messageENOENT: no such file or directory, open 'nope') when the root cannot be opened.readdirSyncwith the same arguments, and Node for all three forms, reportscandir.ENOTDIR) and for a subdirectory that fails mid-walk (EACCES): Node saysscandir '<subdir>', Bun saysopen '<subdir>'.scandirerror (NodeFS::readdir, src/runtime/node/node_fs.rs:6316), but the async recursive task stores the walk's error as-is.AsyncReaddirRecursiveTask::perform_work(node_fs.rs:2447) only re-attaches the path, so theopenattag fromreaddir_with_entries_recursive_async(node_fs.rs:6503/6506; agetdents64/NtQueryDirectoryFiletag from the iterator on the other arms) reaches JS.Fix
perform_workrecords the pending error withwith_path_and_syscall(err_path, sys::Tag::scandir)instead ofwith_path(err_path). Every error the async walk can produce goes through this one line, so the JS-visible syscall isscandirno matter which syscall failed, which is what the sync path already does and what Node does (uv_fs_scandiris the only syscall Node's readdir reports, for the root and for every subdirectory of a recursive listing).test/js/node/fs/fs.test.ts(readdir({recursive: true}) reports failures as scandir): 9 cases (paths / withFileTypes / buffer results, each for a missing root, a file as root, and an unreadable subdirectory), assertingcode,syscall,pathandmessageof the sync, callback and promises forms. The unreadable-subdirectory case is skipped as root (the gate and root CI lanes still run the other 6) and was run here as an unprivileged user: all 9 fail on bun 1.4.0, all 9 pass with this change.EACCESsubdirectory case as an unprivileged user).fs.test.ts,test/js/node/test/parallel/test-fs-readdir*.js, andreaddirSync-recursive-error-leak.test.tsagainst the debug build.Background
readdir(recursive or not) on top of libuv'suv_fs_scandir, so its errors always carrysyscall: "scandir"; a recursive listing is a series of scandir calls, one per directory, and the error names the directory whose scandir failed.NodeFS::readdir, which converts any failure into ascandirerror before it reaches JS. The async recursive variant is a separate thread-pool task (AsyncReaddirRecursiveTask): each directory is a subtask that callsreaddir_with_entries_recursive_async, and the first subtask to fail stores its error inpending_err, whichthenturns into the rejection. That store is the only place the async variant shapes its error, and it was skipping the syscall normalization.bun_sys::Error::with_path_and_syscallis the existing helper for "keep the errno, set the path and the JS-visible syscall name"; the sync truncate/utimes paths use it the same way.Repro (bun 1.4.0 vs node v26.3.0)
bun 1.4.0:
node v26.3.0 prints
scandir/..., scandir 'nope'on all three lines. With this change bun does too.Mid-walk failure as an unprivileged user (
root/lockedis mode 000), node:bun 1.4.0 printed the same object with
openin place ofscandir; with this change it matches. (readdirSyncreports the root instead oflockedinerr.pathon both versions; that is a separate sync-walker issue and is not touched here.)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/node/fs/fs.test.ts