Conversation
… the failing path The recursive delete walker returned ENOENT for a child that another process removed first, and for a directory that was removed while the walker still had it open (getdents64 reports ENOENT for it). With force: true the caller read that as a missing root and returned success with the rest of the tree in place. The walker now skips a vanished child, treats a removed directory as finished, and only returns ENOENT for the root. maxRetries and retryDelay were parsed but never used. The removal is now retried on EBUSY, EMFILE, ENFILE, ENOTEMPTY and EPERM, the set Node's rimraf retries, with a delay of retryDelay * attempt. A retry that finds the path gone succeeds. Errors carry the raw errno and the path of the entry that failed instead of a name string that mapped unknown errors to EFAULT and always named the root. The non-recursive unlink path reports its raw errno too. The recursive branch of the native rmdir is unreachable: the JS layer routes recursive rmdir through rm. It is removed. getdents64 is retried on EINTR in both directory iterators.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesRecursive removal reliability
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change improves recursive removal under races and transient errors, with targeted coverage for concurrent deletion, retries, and error paths. No actionable merge-blocking risk is currently established. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:40 AM PT - Sep 6th, 2026
❌ @robobun, your commit 49e909c has 1 failures in 🧪 To try this PR locally: bunx bun-pr 41480That installs a local version of the PR into your bun-41480 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/node/node_fs.rs— nit: the comment "See the matching comment inrmdir" now points at nothing — this PR deleted the recursive branch (and its Windows path-resolution comment) fromrmdir(). Fix: make the comment self-contained (it already explains the Windows rooted-but-driveless case) and drop the cross-reference.Extended reasoning...
The referenced comment block in
rmdir()("On Windows a rooted-but-driveless path …") was removed at lines 7431–7452 of the diff. The remainingrm()comment still tells readers to look there, which will send the next maintainer on a dead-end search. No runtime effect; documentation-only.Verification: nit — The comment at src/runtime/node/node_fs.rs:7454 reads "See the matching comment in
rmdir: pre-resolve the path on Windows so rooted-but-driveless paths …". This PR deletes the referenced comment: the diff hunk@@ -7458,28 +7431,6 @@removes the entireif args.recursive { … }block fromrmdir(), including the "On Windows a rooted-but-driveless path ("/tmp/foo") must resolve…
…T from a stalled walker
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // On Windows a rooted-but-driveless path ("/tmp/foo") must resolve | ||
| // against the cwd drive. The dt_* helpers go through | ||
| // Syscall::*at -> to_nt_path / normalize_path_windows, which do not | ||
| // add the drive and would turn "/tmp/foo" into a nonexistent NT | ||
| // name (ENOENT). Pre-resolve with slice_z so the path carries a | ||
| // drive letter, the same way existsSync/statSync/unlinkSync see it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The walker swallows ENOENT below the root, so this is the | ||
| // root itself. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // zig_delete_tree — recursive delete-tree. An error names the entry that | ||
| // failed (`sys::Error::path`) and carries the raw errno. ENOENT below the | ||
| // root is not an error: another process removed the entry first. ENOENT | ||
| // for the root itself is returned, which `fs.rm` with `force: false` needs. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Implemented on top of `bun_sys` primitives (`openat` + `unlinkat`). The | ||
| // structure: 16-slot stack, treat_as_dir flip-flop, close-then-deleteDir, | ||
| // retry-on-DirNotEmpty. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Addressed in 939e655:
|
| /// Runs `remove` up to `maxRetries + 1` times. A retry happens for the | ||
| /// errors Node's rimraf retries (EBUSY, EMFILE, ENFILE, ENOTEMPTY, EPERM) | ||
| /// after a sleep of `retryDelay * attempt` milliseconds. A retry that finds | ||
| /// the path gone is a success: the first attempt proved the path existed. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The path of the entry the walker is working on: `sub_path`, then the name | ||
| /// of each directory entered below it (the stack items that own a name), | ||
| /// then `name` when given. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Linux reports ENOENT from getdents64 on a directory that was | ||
| // removed while open. Nothing is left to delete in it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Deletes `sub_path` (relative to `self_`) with one open directory at a | ||
| /// time. The walker above switches to this once its stack is full. An error | ||
| /// names `sub_path`, not the deeper entry that failed: this function does | ||
| /// not keep the chain of directory names it descended through. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
2078765 shortens the comments the comment linter flagged in |
| // Syscall::*at does not add the cwd drive to a rooted path | ||
| // ("/tmp/foo"); slice_z does. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // zig_delete_tree — recursive delete-tree. Errors carry the raw errno and the | ||
| // path of the entry that failed. ENOENT is only returned for the root. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Retries `remove` on the errors Node's rimraf retries, `maxRetries` times, | ||
| /// sleeping `retryDelay * attempt` ms. A retry that finds the path gone succeeds. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// One open directory at a time, for trees deeper than the stack above. | ||
| /// Errors name `sub_path`: the chain of names below it is not kept. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
494381d reduces the remaining flagged comments to one line each (the Windows path note in |
Node ignores maxRetries and retryDelay when recursive is not true.
|
Fixed in 64df19b: the non-recursive |
|
Overlap note. Three open PRs removed What this means for this PR:
The body's reference to #33436 is now stale. The |
There was a problem hiding this comment.
The non-recursive unlink now bypasses rm_with_retries (commit 64df19b), so maxRetries/retryDelay are ignored when recursive is false as Node documents — that addresses the earlier blocking concern. I re-reviewed the walker and retry changes and found no further issues; given this rewrites ~300 lines of the delete-tree walker's error propagation across Linux/macOS/Windows branches, a human pass on the cross-platform paths is still worthwhile.
What was reviewed:
rm_with_retriesis now reached only from theargs.recursivebranch; the non-recursive path callssys::unlinkonce and surfaces the raw errno.- ENOENT swallowing is scoped to child entries (unlinkat/openat/getdents/min-stack recursion) — the root's ENOENT still propagates so
forcesemantics stay correct. dt_entry_pathfiltersname_is_borrowedso the root isn't duplicated in the reported path; the poppedtopis passed via thenameargument since it's already off the stack.- EINTR loops in both
getdentsiterators mirror the existing macOSgetdirentries64pattern.
Extended reasoning...
Overview
This PR rewrites error propagation in the recursive fs.rm delete-tree walker (src/runtime/node/node_fs.rs), replacing three string-name→errno mapping tables with direct sys::Maybe<()> propagation carrying the raw errno, syscall tag, and the failing entry's path. It adds ENOENT-tolerance for child entries removed concurrently, a rm_with_retries wrapper implementing Node's maxRetries/retryDelay semantics, EINTR retry loops in both getdents iterators, and a new race/EMFILE test file. The unreachable recursive branch of native rmdir() is deleted (JS routes recursive rmdir through rm).
Since the last review, commit 64df19b reverted the non-recursive rm path from rm_with_retries(args, || sys::unlink(dest)) back to a single sys::unlink(dest) call, addressing the Node-compat regression flagged earlier (Node ignores maxRetries/retryDelay when recursive is not true). Commit 494381d shortened comments. Commit 939e655 wired worker error events to reject the awaited promises and made the two-walker race test tolerate a root-ENOENT loser, addressing both earlier 🟡 test-robustness comments.
Security risks
No new attack surface. The change narrows error handling (raw errno passthrough replaces a lossy table with an EFAULT fallthrough) and adds bounded retries with a caller-controlled sleep on a work-pool thread. Paths reported in errors are constructed from directory-entry names read from getdents — no user-controlled string concatenation into a syscall argument. The rm_with_retries sleep is retry_delay * attempt with u64::from widening before the multiply, so no overflow on the documented value ranges.
Level of scrutiny
High. This is a ~300-line rewrite of a Node-compat filesystem walker with per-OS #[cfg] branches (macOS EPERM disambiguation, Windows path pre-resolution), a change in error-return type rippling through four functions, and new concurrency-tolerant behavior. The PR description notes Windows STATUS_DELETE_PENDING and force+ENOTDIR-ancestor handling are explicitly out of scope. .claude/docs/landing-prs.md calls for auditing every sibling platform backend and citing Node source for magic constants — the retry errno set matches Node's lib/internal/fs/rimraf.js, but a maintainer should confirm the Windows walker path (which this PR touches only via the slice_z pre-resolution comment) still behaves.
Other factors
The new test file spawns worker threads with a SharedArrayBuffer barrier and a ulimit -n 64 subprocess for deterministic EMFILE — the PR notes 8/8 debug-build passes and 29/30 failure rate on the released binary for the race tests, which is reasonable evidence they exercise the fix. The github-actions[bot] inline comments on node_fs.rs (likely formatting/lint) were followed by fix commits. No CHANGES_REQUESTED reviews from human reviewers are outstanding. The exit reason was dry_streak, so the hunt ran to completion. Given the scope and cross-platform surface, deferring rather than approving.
|
CI state at 49e909c: 180 of 181 jobs pass. The one red job is |
…9025) Bun parses fs.rm maxRetries/retryDelay but never retries (oven-sh/bun#41480). removeTree/removeTreeSync retry EBUSY, EPERM, EMFILE, ENFILE and ENOTEMPTY with linear backoff and rethrow the real error when the budget runs out.
This branch has not been deployed
2 participants
Problem
fs.rm(dir, { recursive: true })fails withENOENTwhen another process removes a child betweengetdents64andunlinkat/openat, or removes a directory the walker still has open (Linuxgetdents64then reportsENOENT). Withforce: truethe caller reads thatENOENTas "the root is missing" and returns success with the rest of the tree still on disk (zig_delete_treeinsrc/runtime/node/node_fs.rsand theFileNotFoundcheck inrm()).maxRetriesandretryDelayare parsed but never used.EBUSY,EMFILE,ENFILE,ENOTEMPTYandEPERMfail on the first attempt.dt_errmaps any errno outside its table toUnexpected, whichmap_anyerror_to_errno*turns intoEFAULT. The non-recursive path has a third table with the sameEFAULTfallthrough. The reportedpathis always the root, never the entry that failed.Fix
zig_delete_treereturnssys::Maybe<()>: the raw errno plus the path of the entry that failed (dt_entry_pathjoins the root with the stack of directory names).ENOENTfor a child, andENOENTfromgetdents64on a directory that was removed while open, continue the walk. Only the root can returnENOENT, so theforcecheck inrm()is correct again. The three name tables are gone, the non-recursiveunlinkreports its raw errno, and the unreachable recursive branch of the nativermdiris removed (the JS layer routes recursivermdirthroughrm).rm_with_retrieswraps the recursive walk. It retries the errors Node'srimrafretries,maxRetriestimes, after a sleep ofretryDelay * attemptms. A retry that finds the path gone succeeds, as in Node. The non-recursiveunlinkis not retried: Node ignoresmaxRetrieswhenrecursiveis not true.getdents64iterators (dir_iterator.rs,sys/lib.rs) loop onEINTR. On macOS,dt_delete_filereportsENOENTwhen the entry vanishes betweenunlinkatand thelstatatthat disambiguatesEPERM.test/js/node/fs/rm-recursive-errors.test.ts(7 tests, all fail on main). Alsofs.test.ts,promises.test.js,test-fs-rm.js, andcargo checkfor darwin and windows targets.Background
zig_delete_treeis the recursive delete walker behindfs.rmwithrecursive: true. It keeps a stack of open directory descriptors (16 deep) and iterates each withgetdents64. Past 16 levels it switches tozig_delete_tree_min_stack_size_with_kind_hint, which keeps one descriptor open and restarts from its root after each directory.fs.rmislib/internal/fs/rimraf.js. It ignoresENOENTon a child, retries the whole removal onEBUSY,EMFILE,ENFILE,ENOTEMPTYandEPERM, and reports the path of the child that failed. The sync path (C++RmSync) sleeps between retries the same way this change does.sys::Errorcarrieserrno, the syscall tag and apath.rm()re-tags the walker's error asrmand strips the Windows\\?\prefix from the path.Notes
Related open PRs that each cover one face of this: #35800 and #39710 (ENOENT race), #35749 (errno passthrough), #35927 (failing path), #39003 (maxRetries), #41486 (ENOTDIR instead of EFAULT on the non-recursive path, the JS-side
lstatrethrow, and an unrelated stream path fix). This change covers their Linux and macOS parts in one walker. Not covered: the WindowsSTATUS_DELETE_PENDINGmapping on the directory open from #39710, and thelstatrethrow and stream path fix from #41486. #41486 owns the removal ofmap_rm_errno_narrow. This branch needs a rebase once it merges.The race tests start a worker that deletes the same tree while the main thread walks it. Three tests use a deleter that unlinks from the end of each listing, so the walker sees entries vanish between
getdents64andunlinkat/openat. On the released binary they fail 29 of 30 runs. The fourth test runs two recursive walkers at once: the one that falls behind still holds a directory open when the other removes it, and its nextgetdents64reportsENOENT(IS_DEADDIR, verified with a rawsyscall(SYS_getdents64)on tmpfs and overlayfs). Without the iterator arm it failed 5 of 6 runs. With the fix the file passed 8 of 8 runs under the debug build.The
EMFILEtests run bun underulimit -n 64, open descriptors untilEMFILE, then free two: enough to openrootandroot/a, notroot/a/b. That gives a deterministicEMFILEat a known entry. The retry test frees descriptors from a timer on the main thread while the walk retries on the pool thread.Retries sleep on the calling thread. For the async flavours that is a work-pool thread, blocked for at most
retryDelay * maxRetries * (maxRetries + 1) / 2ms. Node's C++rmSyncsleeps the same way. Node's asyncrimrafusessetTimeoutinstead.The
EINTRarms have no test:getdents64does not returnEINTRon a local filesystem without syscall injection. They are the Linux and BSD twins of the existinggetdirentries64loop on macOS.The deep-tree walker (
zig_delete_tree_min_stack_size_with_kind_hint) names the depth-16 directory in its error, not the deeper entry, because it does not keep the chain of names it descended through.Suites run:
test/js/node/fs/rm-recursive-errors.test.ts(x8),test/js/node/fs/fs.test.ts,test/js/node/fs/promises.test.js,test/js/node/fs/dir.test.ts,test/js/node/test/parallel/test-fs-rm.js,test-fs-rmdir-throws-on-file.js,bun scripts/rust-check-all.ts aarch64-apple-darwin x86_64-pc-windows-msvc.no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/fs/rm-recursive-errors.test.ts