Conversation
A child entry that disappears between readdir() and unlinkat()/openat() (concurrent delete) made the main stack loop in zig_delete_tree return ENOENT for the whole tree. With force:true, rm() treated that as "root path missing" and returned success with the tree still on disk; with force:false it reported a misleading top-level ENOENT. The min_stack variant of the same walk already skipped a child ENOENT and kept going; the main 16-slot stack loop did not. Add the matching ENOENT arms to its dt_delete_file and dt_open_dir child matches. This matches Node's lib/internal/fs/rimraf.js, which callback/promise fs.rm route through: a child unlink that reports ENOENT is treated as "already gone" and the walk continues.
|
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 (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughChangesRecursive deletion race handling
Suggested reviewers: Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Recursive removal now continues when a child disappears during traversal, with regression coverage for sync, promise, and callback APIs. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:21 PM PT - Sep 9th, 2026
⏳ @Jarred-Sumner, your commit 96d069a is still building in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/rm-recursive-child-enoent.test.ts`:
- Around line 1-9: Remove the multi-line narrative comment above the regression
test in rm-recursive-child-enoent.test.ts. Keep the test coverage unchanged; do
not replace it with non-URL commentary, and add an issue URL only if one is
available.
- Around line 102-105: Update the subprocess helper and its callers around the
fixture-result parsing to return both the parsed JSON result and exit code
without asserting exitCode there. In each test, assert the expected removal
behavior from the parsed result first, then assert exitCode equals 0 as the
final assertion.
- Around line 49-50: Extend the recursive-removal fixture around the existing
GHOST and keep-d.txt setup to include a child directory that disappears before
opening, causing the dt_open_dir ENOENT path in zig_delete_tree to execute. Run
this disappearing-directory scenario through the same API and force-option
matrix already used by the test, preserving the existing file-based unlinkat
ENOENT coverage.
🪄 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: 10f1172b-78df-455a-93e3-ffaa65bb7910
📒 Files selected for processing (2)
src/runtime/node/node_fs.rstest/js/node/fs/rm-recursive-child-enoent.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Also covers the stack-full fallback that delegates to the min_stack variant: its open_initial_subpath call propagates ENOENT for the child entry, which is now treated the same as the other two sites. Test: drop narrative header, assert stdout/stderr/exitCode together via toEqual, and add a depth-18 variant that reaches the min_stack fallback.
getdents64 on a directory that is concurrently removed can return ENOENT (observed on tmpfs/overlayfs). Handle it at both iterator sites the same way: treat as end-of-directory and fall through to the parent rmdir, which already tolerates ENOENT. The depth-18 test cases hit the pre-existing min_stack inner-loop ENOENT arm and passed on main unchanged, so drop them.
There was a problem hiding this comment.
LGTM — the child-level ENOENT arms mirror the pre-existing min_stack inner-loop handling, and my earlier feedback (stack-full fallback, depth-18 cases that passed on main) has been addressed in a497009 / 99a61fb.
Reviewed: the five new ENOENT sites in zig_delete_tree / min_stack — each falls through to a parent rmdir that already tolerates ENOENT, so no case leaves an fd open or skips cleanup. Confirmed dt_err(ENOENT) → crate::Error::FileNotFound, so the fallback match at 9857 is correct. Test covers sync/promise/callback × force/noforce via LD_PRELOAD unlinkat; the un-shimmable arms (openat/getdents64 go through rustix raw syscalls) are one-line symmetry with lines 10086/10095 which have shipped since the function was written.
Extended reasoning...
Overview
Adds ENOENT handling at five child-level sites in zig_delete_tree (src/runtime/node/node_fs.rs): the main-loop iterator, dt_open_dir, the stack-full min_stack fallback, dt_delete_file, and the min_stack iterator. Each treats a child that vanished between readdir and the follow-up syscall as already gone and continues the walk. Ships a Linux-only LD_PRELOAD test covering rmSync/promises.rm/callback rm × force/no-force.
Security risks
None. This makes recursive removal more tolerant of a specific race (concurrent unlink of a child), never less strict. No new syscalls, no path handling changes, no user-controlled input reaching new code paths.
Level of scrutiny
Medium. fs.rm({recursive}) is core node:fs surface, but the change is ~15 net lines confined to one function's error-match arms, and each new arm is byte-for-byte symmetry with the pre-existing Err(E::ENOENT) => continue 'dir_it at min_stack's inner loop (lines 10086/10095). I traced control flow after each new break: the iterator arms fall through to the pop-and-rmdir which already has Err(E::ENOENT) => {} at line 9921; the 'handle_entry breaks proceed to the next entry. Verified dt_err(E::ENOENT) maps to crate::Error::FileNotFound (line 9646), so the stack-full fallback's Err(crate::Error::FileNotFound) pattern is the right comparand for what open_initial_subpath propagates.
Other factors
This PR has already been through three rounds of my review: I flagged the missing stack-full-fallback arm (fixed in a497009), the dt_open_dir coverage gap (explained as unshimmable — openat goes through rustix's raw-syscall backend with no libc import), and the depth-18 cases that passed on main (dropped in 99a61fb). All threads resolved. The remaining six depth-1 cases fail on main and pass with the fix. The deliberate divergence from Node's native rmSync (which has the same silent-abort bug) in favor of rimraf semantics is well-argued in the description — Bun uses one walk for all three entry points, and "reports success with the tree still on disk" is unambiguously wrong. Standalone test file placement matches existing precedent (rm-windows-ntstatus.test.ts).
There was a problem hiding this comment.
No remaining concerns from me — the depth-18 cases were dropped in 99a61fb as suggested, and the five new ENOENT arms are each a one-line "entry already gone → continue" that mirrors the pre-existing min_stack inner-loop handling. Deferring for a maintainer to sign off on the Node-compat call (Bun's rmSync now matches rimraf rather than Node's native rmSync) and on sequencing with #35749, which rewrites error plumbing around these same matches.
What was reviewed: traced each new arm against its min_stack counterpart and the pop-site dt_delete_dir (which already tolerates ENOENT at :9921); confirmed the iterator-ENOENT → break falls through to that tolerant rmdir; verified only unlinkat is libc-routed on Linux so the dt_delete_file arm is the only one the LD_PRELOAD test can deterministically hit.
Extended reasoning...
Overview
Adds Err(E::ENOENT) handling at five child-level sites in zig_delete_tree / zig_delete_tree_min_stack_size_with_kind_hint (src/runtime/node/node_fs.rs) so a child that races to ENOENT between readdir and the follow-up syscall is treated as already gone rather than aborting the whole recursive walk. Adds a Linux-only LD_PRELOAD test (test/js/node/fs/rm-recursive-child-enoent.test.ts) that shims unlinkat to deterministically simulate the race across rmSync / promises.rm / callback rm × force on/off.
Review history
Three prior rounds of my review were all addressed: the stack-full fallback now catches FileNotFound (a497009); the depth-18 test dimension that didn't exercise the new arm was dropped (99a61fb); the dt_open_dir / iterator / fallback arms remain without direct test coverage because openat and getdents64 go through rustix's raw-syscall backend on Linux and are not LD_PRELOAD-interceptable — this is documented in the PR description and each arm is a one-line mirror of the pre-existing Err(E::ENOENT) => continue 'dir_it in the min_stack variant's inner loop. I also confirmed the pop-site dt_delete_dir at :9919-9921 already has an Err(E::ENOENT) => {} arm, so the new iterator break falls through to a tolerant parent rmdir as the description claims.
Security risks
None identified. The change only relaxes handling of ENOENT on a child entry during a recursive delete the caller already requested; it cannot delete anything outside the target tree and cannot mask a non-ENOENT error (each new arm matches E::ENOENT / FileNotFound specifically).
Level of scrutiny
Medium-high — recursive fs.rm is a destructive, widely-used operation. That said, the diff is ~20 lines of match-arm additions following an established in-file pattern, and the failure mode being fixed (silent success with the tree still on disk) is strictly worse than the new behavior.
Why defer rather than approve
Two things a maintainer should be aware of at merge time, neither a correctness concern with this diff:
- Node-compat semantics: the PR deliberately makes Bun's
rmSyncmatch Node's rimraf (callback/promise) behavior rather than Node's nativermSync(which has the same silent-abort bug). The argument is sound — Bun uses one walk for all three entry points and shouldn't leave trees on disk after reporting success — but it's a documented divergence from one Node entry point worth a human ack. - #35749 overlap: that PR rewrites the error plumbing around these same match statements. The author flags them as orthogonal; a maintainer merging should decide sequencing.
|
Rebased on main at 96d069a. Build #113626 has two red tests, neither in Ready for review. |
|
This also fixes #42062, a deterministic case of the same bug that needs no race. On a macOS exFAT volume (FSKit), unlinking Reproduced on Linux with an |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This branch has not been deployed
2 participants
What
fs.rm(path, { recursive: true })aborts mid-walk when an operation on a child entry returnsENOENT, which happens when another process deletes the entry betweenreaddirand the follow-upunlinkat/openat/getdents64. Withforce: truethe returnedENOENTis mistaken for "root path missing" and swallowed, sormreturns success with the rest of the tree (including the root) still on disk. Withforce: falseit reports a misleading top-levelENOENT.Repro
Deterministic via an
LD_PRELOADshim that makesunlinkaton one child remove the file and then reportENOENT(simulating a concurrent unlink):force:trueforce:falseENOENT, root still existsrmSyncNode's callback/promise
rm(lib/internal/fs/rimraf.js) treats a childENOENTas "already gone" and keeps walking. Node's nativermSync(the C++binding.rmSyncusingstd::filesystem) has the same silent-abort as Bun did; since Bun uses one walk for all three entry points, we match the rimraf semantics sormSync/promises.rm/ callbackrmagree and don't leave a tree on disk after reporting success.Cause
zig_delete_tree(src/runtime/node/node_fs.rs) had noENOENTarm at any of the child-level sites in its main 16-slot stack loop: the directory iterator,dt_open_diron a directory child,dt_delete_fileon a file child, and the stack-full fallback into themin_stackvariant. Any childENOENThit the catch-allreturn Err(...)and unwound the whole walk. Themin_stackvariant's inner loop already hadErr(E::ENOENT) => continue 'dir_itat itsdt_open_dir/dt_delete_filesites; its iterator had the same gap.Fix
Treat a child-level
ENOENTas "already gone" at every site in both variants, the same way themin_stackinner loop already did: move on to the next entry (or treat it as end-of-directory for the iterator), falling through to the existing parentrmdirwhich already toleratesENOENT.Verification
test/js/node/fs/rm-recursive-child-enoent.test.tscompiles the shim and runsrmSync/promises.rm/ callbackrmwith and withoutforce: 6 cases, all of which fail onmain(root left on disk / spuriousENOENT) and pass with this change.test/js/node/fs/fs.test.ts -t '^rm'and the vendoredtest-fs-rm*.jspass.cargo checkis green on linux/macos/windows.Only the
dt_delete_filearm is directly exercised by the LD_PRELOAD shim:unlinkatgoes through libc (src/sys/lib.rs), butopenatandgetdents64on Linux userustix's raw-syscall backend with no libc import for LD_PRELOAD to intercept. The other arms are the same one-line treatment and mirror themin_stackinner loop's pre-existing handling.Discovered during review of #35749; orthogonal to that change (which rewrites the error plumbing around these same matches).
Fixes #42062 (macOS exFAT: unlinking
namealso removes the AppleDouble._namesibling, so the walk hits ENOENT on the sibling and stops).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/rm-recursive-child-enoent.test.ts