process: improve error message for process.cwd() when directory is deleted

[Ankush1oo8]

This PR improves the error message thrown by process.cwd() when the current working directory is deleted. Instead of throwing a new error, it enhances the original error message, making it clearer that the directory was deleted while the process was still inside it.

Changes Made:

• Modified wrappedCwd() to enhance the existing error message for ENOENT: uv_cwd instead of creating a new Error instance.
• Ensured the error message explicitly states the reason and how to resolve it (using process.chdir() to switch directories).

Before (Old Behavior):

When the working directory was deleted, process.cwd() threw a generic error with little context:

Error: ENOENT: no such file or directory, uv_cwd

After (New Behavior):

Now, the error provides a more meaningful message:

Error: Current working directory does not exist - this can happen if the directory was deleted while the process was still inside it. Original error: ENOENT: no such file or directory, uv_cwd

Relevant Issue:

(If this PR fixes an issue, add the issue number here)
Example: Fixes #XXXX

PR-URL:

#57053

Reviewer Notes:

• Based on feedback from @gurgunday, I’ve updated the PR to modify the existing error message instead of creating a new error.
• Please review and let me know if further refinements are needed!

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[Qard]

Thanks for taking this one on! ❤️

A few things though:

Why would mutating the error be preferable to using error.cause? (For example:

node/lib/timers/promises.js

Line 75 in d384151

return PromiseReject(new AbortError(undefined, { cause: signal.reason }));

) Question for @gurgunday I suppose on why modifying the error was suggested. Doing this would lose the stack information of the original error whereas error.cause would preserve it, which may be helpful.

This also needs a test. I think the change to the native function may make the javascript change unreachable due to no longer having the syscall info. 🤔

And lastly, a more minor detail: a couple lint checks fail as the commit message is too long and the message lines are too long. We limit lines to 80 columns.

[codecov]

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.11%. Comparing base (cc7018e) to head (6599712).
Report is 28 commits behind head on main.

Files with missing lines	Patch %	Lines
...ernal/bootstrap/switches/does_own_process_state.js	58.33%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #57053   +/-   ##
=======================================
  Coverage   89.10%   89.11%           
=======================================
  Files         665      665           
  Lines      193203   193213   +10     
  Branches    37216    37216           
=======================================
+ Hits       172158   172181   +23     
- Misses      13768    13778   +10     
+ Partials     7277     7254   -23     

Files with missing lines	Coverage Δ
src/node_process_methods.cc	85.82% <100.00%> (ø)
...ernal/bootstrap/switches/does_own_process_state.js	93.50% <58.33%> (-3.03%)	⬇️

... and 41 files with indirect coverage changes

[Ankush1oo8]

what should further with this pr

[gurgunday]

Why would mutating the error be preferable to using error.cause?

I agree that error.cause is better!

I believe originally there was a new error thrown without .cause and I had said that it would be better to throw the same error instead, but error.cause would contain even more information so I think it's the best way to do it

[Qard]

what should further with this pr

I think @jasnell has the best advice here--add the error as a THROW_ERR_... macro in node_errors.h and just do everything on the native side. No need for a JS side wrapping error at all if the original error from the native side has sufficient information included.

[Ankush1oo8]

Please check the pr
I have made the necessary changes

[jasnell]

Code change LGTM but this needs a test. The test might be a bit tricky but it could create a temporary directory with a script in it, run that script, delete that directory, then call the api from the script that was in that directory, etc.... anyway, once a test is added I'm happy to approve this.

[Ankush1oo8]

Code change LGTM but this needs a test. The test might be a bit tricky but it could create a temporary directory with a script in it, run that script, delete that directory, then call the api from the script that was in that directory, etc.... anyway, once a test is added I'm happy to approve this.

I am sorry but i am not able to do it.

[Ankush1oo8]

@addaleax can you check now please

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/65369/

[Ankush1oo8]

Now what should I do

[jasnell]

There's a merge conflict that needs to be resolved. Rebase your branch on the latest main and resolve the conflict then we'll be able to move forward.

[Ankush1oo8]

@jasnell I have done the merging of conflicts can you tell if something more to be done

[Ankush1oo8]

I there still some problem?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/65390/

[jasnell]

No idea why that CI run didn't start. Trying again.

[jasnell]

@Ankush1oo8 ... here's a possible test that works locally for me. We'll need to verify that it works on all platforms.

'use strict';

// This test verifies that an appropriate error is thrown when the current
// working directory is deleted and process.cwd() is called.
//
// We do this by spawning a forked process and deleting the tmp cwd
// while it is starting up.

const common = require('../common');

const { fork } = require('node:child_process');
const { rmSync } = require('node:fs');
const { strictEqual, ok } = require('node:assert');
const { Buffer } = require('node:buffer');

if (process.argv[2] === 'child') {
  return;
}

const tmpdir = require('../common/tmpdir');
tmpdir.refresh();
const tmpDir = tmpdir.path;

const proc = fork(__filename, ['child'], {
  cwd: tmpDir,
  silent: true,
});
proc.on('spawn', common.mustCall(() => rmSync(tmpDir, { recursive: true })));

proc.on('exit', common.mustCall((code) => {
  strictEqual(code, 1);
  proc.stderr.toArray().then(common.mustCall((chunks) => {
    const buf = Buffer.concat(chunks);
    ok(/Current working directory does not exist/.test(buf.toString()));
  }));
}));

[Ankush1oo8]

@jasnell Okey

[Ankush1oo8]

How do we that for all platforms

[jasnell]

Just add the test in test/parallel and CI will run it on the various platforms.

[Ankush1oo8]

Okey

[Ankush1oo8]

@jasnell can you check now?

[jasnell]

@Ankush1oo8 ... unfortunately it looks like you used a merge commit to catch this up to main. We don't do merge commits. Can I ask you to remove the merge commit and use rebase to catch it up?

[Ankush1oo8]

@jasnell Shall I delete this PR and again send one

[jasnell]

No need. You ought to be able to reset your local branch to just your commits to drop the merge commit.