fix(ext/node): align process.chdir error shape with Node#33612
fix(ext/node): align process.chdir error shape with Node#33612nathanwhitbot wants to merge 10 commits intodenoland:mainfrom
process.chdir error shape with Node#33612Conversation
When `process.chdir(dir)` fails, wrap the underlying error via
`denoErrorToNodeError` so it carries Node's expected shape: a libuv-style
message ("ENOENT: no such file or directory, chdir '<cwd>' -> '<dest>'"),
`code: 'ENOENT'`, `syscall: 'chdir'`, `path` (current cwd), and `dest`
(requested directory).
Enables `parallel/test-process-chdir-errormessage.js`.
nathanwhitbot
left a comment
There was a problem hiding this comment.
This duplicates #33584 (already open with the same fix). A few concrete differences worth merging back here, or closing this in favor of #33584:
Edge case: deleted cwd. fs.cwd() on the catch path can itself throw if the current working directory has been removed. This is not hypothetical — parallel/test-fs-mkdir.js's tmpdir cleanup hits this exact path on linux-aarch64 (tmpdir's parent is rmdir'd, then a chdir is attempted), and the snapshot bootstrapper throws an uncaught NotFound from the fs.cwd() call.
#33584 has the matching guard:
let fromPath = "";
try {
fromPath = fs.cwd();
} catch {
// Ignore -- chdir() below will surface a chdir-shaped error.
}
try {
fs.chdir(directory);
} catch (err) {
throw denoErrorToNodeError(err as Error, {
syscall: "chdir",
path: fromPath,
dest: directory,
});
}Without it, parallel/test-fs-mkdir.js (and any test that rmdirs its cwd before exit) would fail.
Test plan: please also re-run parallel/test-fs-mkdir.js and parallel/test-process-chdir.js after this change — those are the ones that exercise the cwd-disappearing path.
Recommendation: close this in favor of #33584, or pull the fromPath guard into this PR.
fibibot
left a comment
There was a problem hiding this comment.
This is an exact functional duplicate of #33584 (which I approved a few hours ago and is still open). Both:
- Wrap
fs.chdir(directory)in a try/catch that funnels failures throughdenoErrorToNodeError({ syscall: "chdir", path, dest }). - Enroll
parallel/test-process-chdir-errormessage.jsat the same alphabetical position. - Apply the same
tests/unit_node/process_test.tsupdate (Deno.errors.NotFound/"file"→Error/"ENOENT"+ assertions oncode/syscall/dest).
One small difference worth picking the better of:
- #33584 snapshots
fs.cwd()before the chdir attempt with a defensivetry/catchand falls back to""if the cwd has been deleted. Matches Node's "path= cwd-at-call-time" semantic and survives the unlinked-cwd case (relevant given the just-landed #33493 / #33587 cwd-inherit work). - #33612 (this PR) calls
fs.cwd()inline inside the catch handler, with no fallback. If the user's cwd has been unlinked,fs.cwd()itself throws and the original chdir error is masked by a less-useful "couldn't read cwd" failure.
I'd recommend closing this in favor of #33584 — same fix, more robust.
CI is also still in flight here (56/110 passing, all builds pending) so I'd hold off on APPROVE either way per fibibot's gate.
cc @bartlomieju on the duplicate-PR coordination.
Summary
When
process.chdir(dir)fails, wrap the underlying Deno error viadenoErrorToNodeErrorso it carries Node's expected shape:message(ENOENT: no such file or directory, chdir '<cwd>' -> '<dest>')name: 'Error'code: 'ENOENT'syscall: 'chdir'path(cwd at the time of the failed call)dest(requested directory)Previously the thrown error was deno's
NotFoundwith the raw"No such file or directory (os error 2): chdir 'does-not-exist'"message and none of those properties.Enables
parallel/test-process-chdir-errormessage.js.Test plan
parallel/test-process-chdir-errormessage.jspasses viacargo test --test node_compatparallel/test-process-chdir.jsstill passes