Skip to content

fix(ext/node): wrap process.chdir errors with path/dest/syscall#33584

Open
nathanwhitbot wants to merge 4 commits intodenoland:mainfrom
nathanwhitbot:fix/node-compat-iter47
Open

fix(ext/node): wrap process.chdir errors with path/dest/syscall#33584
nathanwhitbot wants to merge 4 commits intodenoland:mainfrom
nathanwhitbot:fix/node-compat-iter47

Conversation

@nathanwhitbot
Copy link
Copy Markdown
Contributor

Summary

Node's process.chdir surfaces a uv-shaped Error with:

  • name: 'Error', code: 'ENOENT'
  • message: ENOENT: no such file or directory, chdir '<oldcwd>' -> '<dest>'
  • path: <cwd before the call>, dest: <argument>, syscall: 'chdir'

Deno's polyfill called Deno.cwd().chdir() and rethrew the raw Deno.errors.NotFound, so callers saw name: 'NotFound', no path, no dest, and a non-uv message — anything that does try/catch and checks err.path or err.syscall (a common Node pattern) was broken.

This patch snapshots the cwd before the call and runs failures through denoErrorToNodeError({ syscall: 'chdir', path: fromPath, dest: directory }), matching the C++ binding wrapper used in Node's internal/bootstrap/switches/does_own_process_state.js.

Enables parallel/test-process-chdir-errormessage.js.

Test plan

  • cargo test --test node_compat -- test-process-chdir-errormessage (was failing on main, passes here)
  • cargo build --bin deno clean
  • tools/format.js and tools/lint.js --js clean
  • Sibling enabled tests still pass: test-process-chdir, test-process-config.

Node's `process.chdir` raises a `uv_chdir`-style Error with `code:
'ENOENT'`, `syscall: 'chdir'`, `path` (the cwd before the call), and
`dest` (the target). Deno was passing through Deno's `NotFound` error
verbatim, so consumers saw `name: 'NotFound'`, no `path`, no `dest`,
and a non-uv-shaped message.

Snapshot the cwd before the call and run the result through
`denoErrorToNodeError` with the expected uv context.

Enables `parallel/test-process-chdir-errormessage.js`.
Copy link
Copy Markdown
Contributor Author

@nathanwhitbot nathanwhitbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Faithful shape — denoErrorToNodeErroruvException produces an error
that matches Node's binding.chdir error: code: 'ENOENT', path (cwd
before the call), dest (target), syscall: 'chdir', and the
ENOENT: no such file or directory, chdir '<path>' -> '<dest>' message.

A few small things to consider, none blocking:

  • fs.cwd() failure not wrapped. If the current cwd has been deleted
    (e.g., process.chdir after rmdir($PWD)), the snapshot call itself
    throws and the resulting error skips the wrap. Node's behavior in that
    scenario is also odd (uv_cwd fails first), so probably fine — just
    flagging that this isn't strictly equivalent.
  • Errors that don't carry an extractable os errno in their message
    flow through denoErrorToNodeError and come back unchanged, losing the
    path/dest/syscall annotations. For the common chdir failure modes
    (NotFound → "os error 2", AccessDenied → "os error 13") this works,
    but worth keeping in mind if a non-uv-shaped error ever gets thrown
    from fs.chdir.
  • The test the PR enables (test-process-chdir-errormessage.js) is
    exactly the regression test for this — it asserts on err.path,
    err.dest, and the message format. Good coverage match.

LGTM.

When the current working directory has been deleted (common in tmpdir
cleanup during process exit), fs.cwd() throws NotFound. The new
process.chdir wrapper from this PR was reading cwd unconditionally
before the chdir attempt, propagating that throw as an uncaught
NotFound and breaking test-fs-mkdir.js (which exercises the tmpdir
cleanup path).

Wrap the cwd snapshot in try/catch and fall back to an empty path so
the chdir attempt still runs and any failure is shaped through
denoErrorToNodeError as before.
The snapshot build at libs/core/extensions.rs:139 enforces 7-bit ASCII
on ext:deno_node/* files. The comments I added in 7677cb0 used em
dashes which broke the snapshot build on linux-aarch64.
The wrapper now emits a Node-shaped uv error (Error with code, syscall,
path, dest) instead of Deno.errors.NotFound. Update the existing
'process.chdir failure' test to assert that shape.
@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

test node_compat debug linux-aarch64 failed on test-child-process-stdio-reuse-readable-stdio.js — same stdio reuse-order flake currently also hitting #33588. Not related to this PR's changes (the test was already enabled before). cc @nathanwhit for a rerun once main has settled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants