Feat/carbon dev #2
Conversation
WalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @CoderSerio. * #2 (comment) The following files were modified: * `benchmark/bench.ts` * `benchmark/rm.ts` * `src/readdir.rs` * `src/rm.rs`
![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
🧹 Nitpick comments (12)
package.json (1)
73-74: Mitata devDependency aligns with the new benchmarking setupAdding
mitataas a devDependency fits the benchmark tooling changes; no issues here. If you’ve fully migrated away fromtinybench, you can optionally drop it later to keep devDeps lean.src/lib.rs (1)
14-18: rm module export wiring is consistent and straightforwardRe-exporting
rmviapub mod rm;andpub use rm::*;mirrors the existingreaddirpattern and cleanly exposes the new API. If the crate grows more modules, consider tightening re-exports (e.g., explicit symbols) to avoid future name collisions, but it’s fine as-is.benchmark/bench.ts (1)
8-24: Benchmark file filtering is solid; message could be slightly friendlierThe case-insensitive
.tsfiltering and early exit behavior look good. Minor nit: when no CLI filter is provided and no benchmarks are found, the log will sayfilter "undefined"—if you care about UX, you could special-case the “no filter” scenario for a cleaner message, but it’s not blocking.README.md (1)
41-86: API docs match the new options; consider reflecting Buffer path supportThe added
concurrencyflag forreaddirand the expandedrmoptions/return type nicely document the new behavior and differences from Node’s core APIs. Since the implementation reportedly acceptsstring/Bufferpaths via N-API, you might optionally update thepathsignature here (e.g.,string | Buffer) so the README matches the actual supported types.benchmark/rm.ts (1)
1-97: Benchmark design is solid and highlights concurrency benefitsThe benchmark setup (warmup runs, per-implementation fresh directories, and averaged
hrtimemeasurements) is well thought out and should give stable comparative numbers between Node and hyper-fs, including theconcurrency: 4case. One minor consideration: a depth of 100 for the nested structure may run into path-length quirks on some Windows environments; if you plan to run these benches there, you might reduce depth and compensate with more branching instead.__test__/rm.spec.ts (1)
1-217: rm/rmSync test coverage is excellent; only minor polish possibleThis suite does a great job exercising both sync/async paths across the main behaviors (files, empty/non-empty dirs, recursive flag, force handling, and concurrency on nested trees), which should make regressions very obvious. As minor polish, you could (a) assert on
error.codein addition to message regex when mimicking Node error codes, and (b) optionally add cleanup for the temp directories if you anticipate running the suite very frequently—but neither is required for correctness.index.d.ts (2)
11-29: Readdir JSDoc does not match the actual TypeScript API surfaceThe JSDoc here still describes the Node-style callback API and
encodingoption, but the exportedreaddir/readdirSyncfunctions are Promise/return-value based and theReaddirOptionsinterface addsskipHiddenandconcurrency(and omitsencoding). This can confuse consumers reading the d.ts.Consider updating the JSDoc to:
- Describe the Promise-returning overload (
Promise<string[] | Dirent[]>) instead of a callback.- Document
skipHiddenandconcurrencyand remove/clarify unsupported fields likeencoding.
36-58: Tightenrmtyping and align rm JSDoc with implemented optionsTwo minor API-surface issues here:
rmis declared asPromise<unknown>even though the Rust task resolves to(). Typing this asPromise<void>would better reflect the actual behavior and improve TS ergonomics.RmOptionsincludesconcurrency, but the JSDoc comment only listsforce,maxRetries,recursive, andretryDelay, and still refers to a Node-style callback signature.If you want the d.ts to be self-explanatory, consider:
- Changing
rmtoPromise<void>.- Updating the JSDoc to remove the callback parameter and include
concurrency(and clearly note thatmaxRetries/retryDelayare currently accepted but not implemented, if that’s the case).src/readdir.rs (2)
8-22: Rust-side JSDoc is still describing the old callback-style Node APIThis comment block documents
fs.readdir(path, options, callback)with anencodingoption, but the exposed N-API bindings (readdir/readdirSync) are Promise/return-value based and support additional options (skip_hidden,concurrency) viaReaddirOptions.If you’re using this as the canonical reference for the JS binding, it’d be clearer to update it to:
- Describe the Promise-returning API shape.
- Reflect
recursive,withFileTypes, and any non-standard flags likeskipHidden/concurrency.
48-52: Minor note on ENOENT message wordingThe ENOENT string uses
"readdir '{}'"whereas Node’s own error messages typically say"scandir '{}'"forfs.readdir. If you want to be byte-for-byte compatible with Node (e.g., for snapshot tests), consider mirroring Node’s wording; otherwise this is fine as-is.src/rm.rs (2)
33-72: Concurrency flag only toggles parallelism andmax_retries/retry_delayare currently unusedA few behavioral details worth making explicit:
In
remove_recursive,concurrencyis only used as a boolean toggle:let concurrency = opts.concurrency.unwrap_or(0); if concurrency > 1 { // parallel via par_iter } else { // serial loop }The numeric value doesn’t control the rayon thread pool size; it just decides whether to use
par_iterat all. Insrc/readdir.rs,concurrencydoes directly map to the rayon pool size viaParallelism::RayonNewPool(n), so there’s a subtle inconsistency across APIs.
max_retriesandretry_delayare accepted inRmOptionsbut never applied in the removal logic (e.g., on transientEBUSY/EPERM/EMFILE-like failures).None of this is strictly wrong, but it may surprise users expecting:
- A consistent meaning for
concurrencyacrossreaddirandrm.- Node-like retry behavior when specifying
maxRetries/retryDelay.At minimum, consider:
- Documenting that (for now)
concurrency > 1just enables parallel traversal using the global rayon pool, without controlling its size.- Either wiring
max_retries/retry_delayinto a simple retry loop for known transient errors, or documenting that they’re currently ignored.
56-67: Error mapping for non-recursive directory removal could rely onErrorKind::DirectoryNotEmptyinstead of string-matchingIn the non-recursive branch you special-case ENOTEMPTY like this:
fs::remove_dir(path).map_err(|e| { if e.kind() == std::io::ErrorKind::AlreadyExists || e.to_string().contains("not empty") { Error::from_reason(format!( "ENOTEMPTY: directory not empty, rm '{}'", path.to_string_lossy() )) } else { Error::from_reason(e.to_string()) } })?;Relying on
e.to_string().contains("not empty")is fragile (locale-dependent and OS-string-format dependent). UsingErrorKind::DirectoryNotEmptywould be more robust and self-documenting:- if e.kind() == std::io::ErrorKind::AlreadyExists || e.to_string().contains("not empty") { + if matches!(e.kind(), std::io::ErrorKind::DirectoryNotEmpty | std::io::ErrorKind::AlreadyExists) {This keeps the intended ENOTEMPTY mapping but avoids depending on the textual message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
Cargo.toml(1 hunks)README.md(2 hunks)__test__/rm.spec.ts(1 hunks)benchmark/bench.ts(1 hunks)benchmark/readdir.ts(1 hunks)benchmark/rm.ts(1 hunks)index.d.ts(2 hunks)index.js(1 hunks)package.json(1 hunks)src/lib.rs(1 hunks)src/readdir.rs(2 hunks)src/rm.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/lib.rs (2)
src/rm.rs (1)
rm(123-125)src/readdir.rs (1)
readdir(185-187)
__test__/rm.spec.ts (1)
src/rm.rs (1)
rm(123-125)
index.d.ts (1)
src/rm.rs (1)
rm(123-125)
src/rm.rs (2)
index.d.ts (1)
RmOptions(50-56)src/readdir.rs (2)
compute(176-178)resolve(179-181)
🔇 Additional comments (3)
Cargo.toml (1)
12-18: Rayon/remove_dir_all additions look appropriate for parallel rmUsing
rayonfor configurable concurrency andremove_dir_allfor recursive deletion fits the rm implementation goals; nothing blocking from this manifest change alone. Just ensure your MSRV/target platforms are compatible with these crate versions as part of your normal release checks.index.js (1)
574-578: rm/rmSync exports correctly mirror native bindingHooking
module.exports.rm/rmSyncdirectly tonativeBinding.rm/rmSyncis consistent with the existingreaddirexports and keeps the JS surface thin. No issues here.benchmark/readdir.ts (1)
1-42: Benchmark structure and mitata usage look solidThe switch to
mitatawith grouped benchmarks,node_modules→cwdfallback, and the extra diagnostic logging are all reasonable. The Node.js vs Hyper-FS comparisons use matching options (withFileTypes,recursive) and the finalawait run({ colors: true })is idiomatic for mitata.No functional issues from the review perspective.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
src/rm.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/rm.rs (2)
index.d.ts (1)
RmOptions(50-56)src/readdir.rs (2)
compute(176-178)resolve(179-181)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: stable - x86_64-pc-windows-msvc - node@22
🔇 Additional comments (3)
src/rm.rs (3)
75-99: Previous critical issue with empty-string paths has been resolved.The past review flagged that empty strings were being mapped to
".", which could recursively delete the CWD. The current implementation at line 76 usesPath::new(&path_str)directly without any special-case mapping. Whenpath_stris empty, the subsequentpath.exists()check (or better,symlink_metadata()after the above fix) will return false/error, triggering either silent success withforce: trueor anENOENTerror, which is safe and appropriate behavior.
101-129: LGTM! Async task implementation follows napi-rs patterns correctly.The
RmTaskstruct andTasktrait implementation properly delegate to the coreremovefunction, and the publicrmandrm_syncexports correctly expose async and sync variants to JavaScript.
40-54: The parallel deletion implementation is correct; no issues with error handling semantics. Both the parallel path usingtry_for_eachand the sequential path return immediately on the first error, resulting in identical behavior. Rayon'stry_for_eachsynchronously waits for spawned work before propagating errors, and the parent directory removal is only attempted if all child deletions succeed—matching standard recursive deletion semantics.Likely an incorrect or invalid review comment.
| #[napi(object)] | ||
| #[derive(Clone)] | ||
| pub struct RmOptions { | ||
| pub force: Option<bool>, | ||
| pub max_retries: Option<u32>, | ||
| pub recursive: Option<bool>, | ||
| pub retry_delay: Option<u32>, | ||
| pub concurrency: Option<u32>, | ||
| } |
There was a problem hiding this comment.
Implement retry logic or remove unused max_retries and retry_delay fields.
The RmOptions struct declares max_retries and retry_delay fields that are never used in the implementation. Users can pass these options, but they will be silently ignored, creating an API surface inconsistency with the promised functionality.
Either implement the retry logic (wrapping operations in a retry loop with exponential backoff based on these options) or remove these fields from RmOptions to avoid misleading users.
Implementation guidance
If implementing retry logic, wrap the operations in remove_recursive with a retry loop that:
- Catches transient filesystem errors (e.g.,
PermissionDenied,ResourceBusy) - Retries up to
max_retriestimes withretry_delaymilliseconds between attempts - Only retries on errors that are likely transient, not permanent failures like
ENOENT
| fs::remove_dir(path).map_err(|e| { | ||
| if e.kind() == std::io::ErrorKind::AlreadyExists || e.to_string().contains("not empty") { | ||
| Error::from_reason(format!( | ||
| "ENOTEMPTY: directory not empty, rm '{}'", | ||
| path.to_string_lossy() | ||
| )) | ||
| } else { | ||
| Error::from_reason(e.to_string()) | ||
| } | ||
| })?; |
There was a problem hiding this comment.
Fix incorrect error kind check for directory-not-empty errors.
Line 59 checks for std::io::ErrorKind::AlreadyExists, which is for "file already exists" errors, not "directory not empty" errors. Rust provides std::io::ErrorKind::DirectoryNotEmpty (stable since 1.17.0) for this purpose. The fallback string check .contains("not empty") is fragile and platform-dependent.
Apply this diff to use the correct error kind:
} else {
fs::remove_dir(path).map_err(|e| {
- if e.kind() == std::io::ErrorKind::AlreadyExists || e.to_string().contains("not empty") {
+ if e.kind() == std::io::ErrorKind::DirectoryNotEmpty {
Error::from_reason(format!(
"ENOTEMPTY: directory not empty, rm '{}'",
path.to_string_lossy()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fs::remove_dir(path).map_err(|e| { | |
| if e.kind() == std::io::ErrorKind::AlreadyExists || e.to_string().contains("not empty") { | |
| Error::from_reason(format!( | |
| "ENOTEMPTY: directory not empty, rm '{}'", | |
| path.to_string_lossy() | |
| )) | |
| } else { | |
| Error::from_reason(e.to_string()) | |
| } | |
| })?; | |
| fs::remove_dir(path).map_err(|e| { | |
| if e.kind() == std::io::ErrorKind::DirectoryNotEmpty { | |
| Error::from_reason(format!( | |
| "ENOTEMPTY: directory not empty, rm '{}'", | |
| path.to_string_lossy() | |
| )) | |
| } else { | |
| Error::from_reason(e.to_string()) | |
| } | |
| })?; |
🤖 Prompt for AI Agents
In src/rm.rs around lines 58 to 67, the error mapping currently checks for
std::io::ErrorKind::AlreadyExists and falls back to matching the error string
for "not empty" which is incorrect and fragile; change the condition to check
for std::io::ErrorKind::DirectoryNotEmpty (stable since 1.17) and remove the
string-based `.contains("not empty")` fallback so the code maps
directory-not-empty errors using the proper ErrorKind and otherwise returns the
original error string.
| if !path.exists() { | ||
| if force { | ||
| // If force is true, silently succeed when path doesn't exist | ||
| return Ok(()); | ||
| } | ||
| return Err(Error::from_reason(format!( | ||
| "ENOENT: no such file or directory, rm '{}'", | ||
| path.to_string_lossy() | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Use try_exists() or symlink_metadata() to properly handle broken symlinks.
Line 87 uses path.exists(), which follows symlinks and returns false for broken symlinks (symlinks pointing to non-existent targets). This means a broken symlink would be treated as non-existent, requiring force: true to delete or returning ENOENT otherwise, even though the symlink itself exists and should be removable.
Apply this diff to handle broken symlinks correctly:
fn remove(path_str: String, options: Option<RmOptions>) -> Result<()> {
let path = Path::new(&path_str);
let opts = options.unwrap_or(RmOptions {
force: Some(false),
recursive: Some(false),
max_retries: None,
retry_delay: None,
concurrency: None,
});
let force = opts.force.unwrap_or(false);
- if !path.exists() {
+ // Use symlink_metadata to detect broken symlinks (which exist but point to non-existent targets)
+ if fs::symlink_metadata(path).is_err() {
if force {
// If force is true, silently succeed when path doesn't exist
return Ok(());This ensures that broken symlinks are treated as existing paths and can be deleted without requiring force: true.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !path.exists() { | |
| if force { | |
| // If force is true, silently succeed when path doesn't exist | |
| return Ok(()); | |
| } | |
| return Err(Error::from_reason(format!( | |
| "ENOENT: no such file or directory, rm '{}'", | |
| path.to_string_lossy() | |
| ))); | |
| } | |
| // Use symlink_metadata to detect broken symlinks (which exist but point to non-existent targets) | |
| if fs::symlink_metadata(path).is_err() { | |
| if force { | |
| // If force is true, silently succeed when path doesn't exist | |
| return Ok(()); | |
| } | |
| return Err(Error::from_reason(format!( | |
| "ENOENT: no such file or directory, rm '{}'", | |
| path.to_string_lossy() | |
| ))); | |
| } |
🤖 Prompt for AI Agents
In src/rm.rs around lines 87 to 96, the code uses path.exists() which follows
symlinks and treats broken symlinks as non-existent; change the check to detect
the presence of the path itself (including broken symlinks) by using
path.symlink_metadata().is_ok() (or Path::try_exists() if preferred) so broken
symlinks are considered existing and can be removed without requiring force;
update the conditional to use that check and keep the existing force/ENOENT
logic intact.
|
The potential issues reported above are tracked and scheduled for resolution before the 1.0 release. |
Description
This PR implements high-performance
rmandrmSyncAPIs, aiming to be a drop-in replacement for Node.jsfs.rm.The key highlight is the introduction of Parallel Deletion powered by Rust's
rayon. Unlike Node.js, which deletes files sequentially, this implementation can process directory entries in parallel when theconcurrencyoption is enabled. This results in significant performance gains, especially for large, nested directory structures (e.g.,node_modules).Changes
Core Implementation:
rm(async task) andrmSyncusing Rust'sstd::fs.fs.rm, supportingforce(ignore non-existent) andrecursiveoptions.ENOENT,ENOTEMPTY,EISDIR).Parallelism Support:
concurrencyoption inRmOptions.rayonto perform parallel recursive deletion.concurrency > 1, the directory entries are collected and processed in parallel threads, maximizing IOPS utilization on modern SSDs.API Compatibility:
pathargument handling (supports string/Buffer behavior via NAPI).recursive,force,concurrency, etc.).Benchmarks
Benchmarks demonstrate that the Rust multi-threaded implementation significantly outperforms Node.js native
fs.rm, particularly in flat directory structures where syscall latency can be hidden by parallelism.Environment:
clk: ~3.98 GHz
cpu: Apple M3
runtime: node 24.0.2 (arm64-darwin)
Checklist
cargo check,npm run lint)concurrencyinRmOptionsSummary by CodeRabbit
New Features
Documentation
Tests
Benchmarks
Chores
✏️ Tip: You can customize this high-level summary in your review settings.