Conversation
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Next review available in: 33 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 (4)
WalkthroughRecursive ChangesELOOP skip and EACCES test coverage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/js/node/fs/fs.test.ts`:
- Around line 1525-1569: The recursive symlink-cycle regression test in the fs
recursive walker coverage should also exercise the async fs.opendir path, since
the current test only covers readdirSync, promises.readdir, and opendirSync.
Extend the existing test case by adding an async fs.promises.opendir/
fs.opendir-based walk using the same root and expected dirent names, so the
recursive walker behavior is verified across all affected entry points.
🪄 Autofix (Beta)
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: 44adf3ff-a6d8-4d34-b85d-085e1e32b19e
📒 Files selected for processing (4)
src/runtime/node/node_fs.rstest/js/node/fs/fs.test.tstest/js/node/fs/readdirSync-recursive-error-leak-fixture.jstest/js/node/fs/readdirSync-recursive-error-leak.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, well-justified errno-set addition with thorough test coverage; the one finding is a comment-wording nit.
Extended reasoning...
Overview
The runtime change is two tokens: | E::ELOOP added to the tolerated-errno match arm in both readdir_with_entries_recursive_async and readdir_with_entries_recursive_sync. The walker uses openat(O_DIRECTORY | O_RDONLY) as its "is this traversable" probe and already skips ENOENT/ENOTDIR/EPERM; a symlink cycle yields ELOOP from the same probe and should be treated identically, since a real directory can never produce ELOOP. This brings all five recursive listing entry points (readdirSync paths/dirents, promises.readdir paths/dirents, recursive opendirSync) in line with Node, which the PR verifies against v26.3.0.
The test changes are larger (~130 lines) but mechanical in nature: a new comprehensive test exercising all five variants over a fixture with a two-link cycle, a self-cycle, a dangling symlink, and a resolvable symlink-to-directory (proving traversal still works); plus migration of three existing error-path tests that previously relied on self-referential symlinks producing ELOOP — exactly the behavior this PR removes. Those tests now trigger the same error path via an unreadable directory (EACCES), with a setuid(65534) drop when running as root since root bypasses DAC. Each migrated test still asserts its original invariant (buffer-entry cleanup, multi-subtask promise settlement, Dirent.path leak bound), and each is wrapped in try/finally to restore permissions before tempDir disposal.
Security risks
None. This widens a skip set on a per-entry directory-open probe inside recursive readdir — no new inputs are trusted, no paths are followed that weren't before, and EACCES still propagates. The setuid calls are test-only, in spawned child processes, gated on already being root.
Level of scrutiny
Low-to-moderate. The native change is a one-token addition to an existing pattern with a clear correctness argument (real directories cannot yield ELOOP from openat(O_DIRECTORY)) and demonstrated Node parity. The test migrations warrant a closer read because they change how three regression tests provoke failure, but I traced each: the dropPrivileges guard is a no-op when not root (so non-root CI just sees EACCES on the 0o000 dir directly), and when root it drops to 65534 after makeUnreadableDir chmods the tempDir to 0755 so the unprivileged child can still traverse to it. tempDir creates directly under os.tmpdir(), so there's no restrictive intermediate ancestor on Linux; on macOS tests don't run as root so the drop never fires. Cleanup restores modes in finally before using disposal runs rmSync.
Other factors
The bug hunter found one nit: the rewritten comment describes the EPERM slot as "unreadable", but unreadable directories yield EACCES (which is deliberately not skipped, and which the PR's own migrated tests depend on propagating). It's a comment-accuracy issue only — the code and tests are internally consistent — so it doesn't block approval. The PR also removes the stale "This is different than what Node does" comment, which is appropriate given the change explicitly aligns with Node. The new test includes a non-vacuous sanity check (readdirSync(join(root, 'lpA')) must throw ELOOP) proving the fixture actually contains a cycle. Full test-suite pass reported for fs.test.ts, the leak test, and dir.test.ts/glob.test.ts; cargo check clean on all three tier-1 targets.
|
All three pieces of feedback are addressed in 0c42e77. On the duplicate flag (#31418)Real overlap, and #31418 is the larger change. I re-checked its extra claim against node v26.3.0 and it still holds, so that work is still needed and is not in this PR:
This PR is only the
Happy either way: close this if #31418 gets rebased, or land this and #31418's remaining delta shrinks to the Review nits
CIThe four red lanes (including That reproduces on this branch's base commit, and this PR touches nothing outside |
|
Tip For best results, initiate chat on the files or code changes.
This all looks good to me. I'll take another pass once the build is green. ✏️ Learnings added
|
…with ELOOP The recursive readdir walker opens every symlink and directory entry with openat(O_DIRECTORY), and tolerates ENOENT, ENOTDIR and EPERM from that open by skipping the entry. ELOOP was not in that set, so a single symlink cycle anywhere in the tree aborted the whole listing and returned nothing. Node's recursive walkers ignore per-entry stat failures, so add ELOOP to the skipped set in both the sync and the async walker. This also covers fs.promises.readdir and recursive fs.opendir, which both go through readdir. The two existing regression tests and the Dirent.path leak fixture used a self-referential symlink to force a failing subdirectory open. They now use an unreadable directory (EACCES), dropping to an unprivileged uid first because root bypasses the permission check.
…ergence fs.promises.opendir(recursive) shares the walker but was not exercised. Move the symlink-to-a-real-directory guard into its own test: node does not descend into symlinked directories for promises.readdir+withFileTypes or for opendir, so the cycle fixture would otherwise have asserted a divergence unrelated to ELOOP. Also name the excluded errno in the walker comment: an unreadable directory is EACCES, which propagates, not EPERM, which is skipped.
0c42e77 to
b4c14f7
Compare
|
Rebased onto `48ff9eb` ( For the record, the old base |
|
CI status on The one red job is
That lane is sharded, and the sibling shard passed, which is the one that happens to carry this PR's tests. So macOS coverage is real: The 2 expired jobs are I am deliberately not pushing an empty retrigger commit. The diff is already proven green per-file on linux (including Summary of where this stands: the diff is two tokens of runtime change ( |
…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 -->
…0668) ### Problem - `fs.promises.readdir(dir, { recursive: true })` and the callback form never settle on a tree with two directory symlink loops and one entry that fails to open (`ln -s . loop1; ln -s . loop2; ln -s bad bad`). Every pool thread spins and the caller cannot catch anything. `readdirSync` rejects the same tree with `ELOOP` in milliseconds. Both async forms share `AsyncReaddirRecursiveTask` (`node_fs_binding.rs:204`). - Cause: a failing subtask records its error in `pending_err` and releases its reference (`node_fs.rs:2404`), but every other subtask keeps calling `enqueue` (`node_fs.rs:2294`), which schedules one new subtask per directory found. Nothing reads `pending_err` until `subtask_count` reaches zero, so the promise waits for the whole frontier. The kernel follows 40 symlinks before `ELOOP`, so two loops make that frontier 2^41 directories. ### Fix - `AsyncReaddirRecursiveTask` gets a `has_error` flag, set right after the first error is recorded. `enqueue` then schedules nothing, and a subtask that starts after the flag is set releases its reference without opening its directory. The scan settles once the few subtasks already in flight return. - The rejection is the same error as before (the first one recorded). On success nothing changes: the flag is never set, so the only new cost is one relaxed atomic load per directory. - The `subtask_count` decrements are now `AcqRel`, so the last subtask sees the other subtasks' `pending_err` and queued results. The cp task already uses this ordering. - Verified: `test/js/node/fs/fs.test.ts` (`readdir({recursive: true}) settles with the first error while symlink loops are still being walked`, promise and callback form; on the stock binary the test times out with the child still spinning, the fixed one settles in 0.6 s under ASAN). Also ran all of `fs.test.ts`, `readdirSync-recursive-error-leak.test.ts`, `dir.test.ts`, `fs-path-length.test.ts` and node's `test-fs-readdir*.js`. ### Background - The async recursive readdir is one `Job` whose off-thread part is shared by pool subtasks. The root directory runs in `run`. Each directory or symlink entry becomes a `ReaddirSubtask` on the work pool. `subtask_count` is a refcount: it starts at 1 for the root, `enqueue` adds one, and every subtask subtracts one when it is done. The subtask that brings it to zero calls `finish_concurrently`, which joins the results or keeps the error and hands the job back to the JS thread. - `readdir_skips_subdir` lists the errors that skip one entry (`ENOENT`, `ENOTDIR`, `EPERM`). Any other error, `ELOOP` included, fails the whole listing, as in the sync walker. - This change does not bound a tree that has only the two loops and no entry that fails early. On a stock kernel the first `ELOOP` appears at depth 41, so that tree is a 2^41 directory walk for `readdirSync`, and for node's sync and async walkers too (node v26 did not finish it here either). Only cycle detection (the dev/ino of the ancestors) would bound it. That is a semantics change, and it interacts with #33432 and #31418, which make `ELOOP` a skipped error. It is not part of this PR. <details><summary>Notes</summary> Repro on the released build (Linux 6.17, overlayfs): ``` mkdir -p /tmp/t/a/b && cd /tmp/t && ln -s bad bad && ln -s . s1 && ln -s . s2 timeout -s KILL 20 bun -e 'require("fs").promises.readdir(".",{recursive:true}).then(a=>console.log("ok",a.length),e=>console.log("rej",e.code))' # never prints, rc=137. Same with fs.readdir(".", {recursive:true}, cb). bun -e 'try{require("fs").readdirSync(".",{recursive:true})}catch(e){console.log(e.code)}' # ELOOP in 9 ms ``` With this change both async forms print `rej ELOOP` in about 10 ms (0.5 s for the debug build, most of it startup). The bare two-loop tree without `bad`: `readdirSync` did not finish in 30 s on this machine. The fuzz report that saw it finish in 11 s ran on a box where the kernel returns `ELOOP` after 8 to 11 components, so the sync walker got its first error early there. The async walker was the only one that did not stop at that error. `USE_SYSTEM_BUN=1 bun test` on the new test: the test hits the 5 s runner timeout and the runner kills the spinning child ("killed 1 dangling process"). `bun bd test` with this change: pass in 583 ms. </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 -->
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-06, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
Every recursive
node:fslisting API throwsELOOPand returns nothing if the tree contains a symlink cycle anywhere:readdirSync(root, { recursive: true })(paths andwithFileTypes),fs.promises.readdir({ recursive: true }), and recursiveopendirSync(). A singleln -s . xor a broken package link innode_modulesis enough to break the whole listing.Repro
Cause
The recursive walker opens every symlink and directory entry with
openat(O_DIRECTORY | O_RDONLY)and treats that open as the "is this a traversable directory" probe. Its tolerated-errno filter only listedENOENT | ENOTDIR | EPERM, so a dangling symlink (ENOENT) and a symlink to a file (ENOTDIR) were skipped, but a symlink cycle (ELOOP) propagated as the error for the entire call. Node's recursive walkers ignore per-entry stat failures and return the complete listing.Fix
Add
ELOOPto the skipped set in both copies of the walker,readdir_with_entries_recursive_asyncandreaddir_with_entries_recursive_syncinsrc/runtime/node/node_fs.rs. A real directory can never produceELOOP, so this only affects unresolvable symlink entries.fs.promises.readdirand recursivefs.opendirboth route throughreaddir, so all five variants are covered by the one change.EACCESstill propagates, matching Node (which throws when it tries to read a directory it lacks permission for).All five variants now return the same entries as Node, in the same count, and a symlink to a real directory is still walked into.
Tests
test/js/node/fs/fs.test.tsgainsrecursive readdir skips symlink cycles instead of failing with ELOOP. The fixture has a two-link cycle, a self-referential symlink and a dangling symlink, and the test asserts the full listing for all six recursive entry points:readdirSync(paths and dirents),promises.readdir(paths and dirents),opendirSyncandpromises.opendir. Every listing matches node v26.3.0 entry for entry. A non-vacuous guard first asserts the fixture's cycle really does open withELOOP.A second test,
recursive readdir still descends into a symlink to a real directory, is the negative contract: widening the skipped-errno set must not skip entries the walker can open.Three existing tests used a self-referential symlink purely as a way to make a subdirectory open fail, which is exactly what this change stops doing. They now use an unreadable directory (
EACCES), dropping to an unprivileged uid first since root bypasses the permission check, and keep asserting the invariants they were written for:readdirSync({encoding: 'buffer', recursive: true}) frees entries safely when a subdir fails to openpromises.readdir({recursive: true}) settles when multiple subtasks failreaddirSync-recursive-error-leak-fixture.js(Dirent.path leak on the sync error path)Verification
cargo check -p bun_runtimeis clean forx86_64-unknown-linux-gnu,x86_64-pc-windows-msvcandaarch64-apple-darwin.