node:fs: match Node's recursive readdir symlink and cycle handling - #31418
Jarred-Sumner wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 18 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Updated 8:57 PM PT - May 25th, 2026
✅ @Jarred-Sumner, your commit e1cb5b45e4d15cc42c71ed9f8996175b02e9e393 passed in 🧪 To try this PR locally: bunx bun-pr 31418That installs a local version of the PR into your bun-31418 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues, but this changes user-visible recursive readdir semantics (ELOOP now skipped, promises+withFileTypes uniquely skips symlink descent) across both the JS binding and the Rust walker, so it's worth a human look to confirm the Node-parity claims and the new internal positional arg.
Extended reasoning...
Overview
This PR aligns recursive fs.readdir/readdirSync symlink handling with Node.js. It touches:
src/runtime/node/node_fs.rs: addsno_symlink_descentto theReaddirargs struct (parsed from a third positional boolean), addsE::ELOOPto the silently-skipped open errors in both the sync and async recursive walkers, and gates symlink descent on the new flag in two places per walker.src/js/node/fs.promises.ts: replaces the genericasyncWrapforreaddirwith a wrapper that passestrueas the third arg, so only the promises flavor getsno_symlink_descent(and only whenwithFileTypesis set, since the Rust side ANDs withwith_file_types).- Tests: rewrites the symlink-loop test to assert the loop is listed without throwing, adds a comprehensive fixture covering inside/outside/dangling/self-referential symlinks across sync/callback/promises × with/without
withFileTypes, adds a parent-cycle test, and migrates the error-leak fixture from a symlink loop to a mode-000 directory (with a root-uid skip).
Security risks
No new attack surface is introduced — recursive readdir already followed directory symlinks (including outside the root), and this PR doesn't change that for the existing flavors; it actually restricts descent for the promises+withFileTypes case. Suppressing ELOOP per-entry rather than failing the whole walk matches Node and is a robustness improvement, not a security regression. The new third positional arg is internal-only (the public callback wrapper in src/js/node/fs.ts only forwards (path, options)), and the Rust parser guards on val.is_boolean() so stray extra args are ignored.
Level of scrutiny
This warrants moderate-to-high scrutiny. It's a behavioral change to a core, widely-used filesystem API with intentionally asymmetric semantics across API flavors (only fs.promises.readdir + withFileTypes skips symlink descent — surprising but claimed to match Node v24). The implementation spans the JS/Rust boundary with a new informal positional-arg contract, and the recursive walker has two parallel implementations that must stay in sync. None of this is mechanical.
Other factors
The bug-hunting pass found nothing. Test coverage is thorough and the PR description documents Node-verified expectations. I verified the callback path (src/js/node/fs.ts:424) and Dir iterator don't pass the third arg, so they're unaffected. The leak fixture's new root-uid early-exit means it silently passes under root, which slightly weakens coverage in root-running CI but is reasonable given EACCES can't be triggered there. Given the subtle cross-flavor semantics and core-API surface, a human should confirm the Node-parity matrix before merge.
…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 -->
This branch has not been deployed
2 participants
Aligns recursive
fs.readdir/fs.readdirSyncsymlink handling with Node.js.Verified against Node v24 with fixtures covering directory symlinks (inside and outside the scanned root), dangling symlinks, self-referential symlinks, and parent-directory cycles, across
readdirSync, callbackfs.readdir, andfs.promises.readdir, each with and withoutwithFileTypes:readdirSync, callbackreaddir, andfs.promises.readdirwithoutwithFileTypes; it never throws on cycles or dangling links. Bun previously failed the whole walk withELOOPwhen it encountered a self-referential symlink.fs.promises.readdir(path, { recursive: true, withFileTypes: true })is the one flavor where Node does not descend into symlinked directories; Bun previously did.Changes:
ELOOPlike the other per-entry open failures (entry is listed, walk continues), matching Node.withFileTypespath passes an internal flag so symlinked directories are listed but not descended into, matching Node; all other flavors are unchanged.readdirSyncrecursive error-path test and RSS fixture now trigger their error via an unreadable directory instead of a symlink loop (which no longer errors); the fixture skips its measurement when running as root.ELOOP) and pass here.Local verification: targeted fs suites, the recursive-readdir leak test, the vendored
test-fs-readdir-types-symlinks.js, andtest/cli/install/symlink-path-traversal.test.tsall pass; cross-targetcargo checkpasses on all CI targets.