Incorrect ENOENT error on Windows with shell=true when child exits with code 1

[mterrel]

Summary

On Windows, when the execa child process exits with exit code 1 and the shell option is set to true, the error thrown is always ENOENT even though the child process was found and executed correctly (albeit with a non-zero exit code).

Using this code as an example:

const execa = require("execa");
execa('node -e "process.exit(1)"', { shell: true })
    .catch(e => console.log(`Exit 1: ${e}\n`));

Expected behavior

The output should be:

Exit 1: Error: Command failed with exit code 1: node -e "process.exit(1)"

Actual behavior

The current output is:

Exit 1: Error: Command failed with ENOENT: node -e "process.exit(1)"
spawn node -e "process.exit(1)" ENOENT

Additional information

The issue appears to be related to verifyENOENT from the cross-spawn library and its incorrect detection of whether a valid file was found to execute.

The following code shows that the issue only occurs when the child's exit code is 1, but not for exit code 2:

const execa = require("execa");

execa('node -e "process.exit(2)"', { shell: true })
    .catch(e => console.log(`Exit 2: ${e}\n`));

execa('node -e "process.exit(1)"', { shell: true })
    .catch(e => console.log(`Exit 1: ${e}\n`));

As an additional test case, I'd recommend the following code because exit is a shell command, not an executable in the path. This code should not throw an ENOENT error.

const execa = require("execa");

execa('exit 1', { shell: true })
    .catch(e => console.log(`Exit 1: ${e}\n`));

[sindresorhus]

Can you try the same with require('child_process).execFile? Just to rule out it being a Node.js bug.

[ehmicky]

The error is indeed in cross-spawn. This issue describes it. I have suggested a solution to it here.

[mterrel]

Can you try the same with require('child_process).execFile? Just to rule out it being a Node.js bug.

The output from require("child_process").execFile('exit 1', { shell: true }, e => console.log(e)) is:

 Error: Command failed: exit 1

    at ChildProcess.exithandler (child_process.js:303:12)
    at ChildProcess.emit (events.js:315:20)
    at ChildProcess.EventEmitter.emit (domain.js:506:15)
    at maybeClose (internal/child_process.js:1021:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:286:5)
    at Process.callbackTrampoline (internal/async_hooks.js:120:14) {
  killed: false,
  code: 1,
  signal: null,
  cmd: 'exit 1'
}

[mterrel]

@ehmicky @sindresorhus Yes, moxystudio/node-cross-spawn#104 definitely is describing the same issue within cross-spawn. But note that depending on how cross-spawn chooses to fix the issue, execa will most likely need to change too, whether to pass through a new option, or more likely, since execa directly calls crossSpawn._enoent.hookChildProcess, execa may just need to implement the similar conditional logic to not call crossSpawn._enoent.hookChildProcess based on an option.

Honestly, I think the ENOENT behavior is incorrect in so many cases, it's not useful. And just to be clear, it's always 100% broken for shell: true and can never work given the current "is there a file on filesystem" approach from the cross-spawn implementation. In my humble opinion, I think it's better in this case to not hook emit and just pass the error back to the caller of execa unmodified so the caller can handle it correctly.

[ehmicky]

Yes, I agree with you about the ENOENT behavior. That's why I suggest in that comment removing it altogether.

If we do so, no changes will be required in execa except from upgrading to the new version of cross-spawn and removing crossSpawn._enoent.hookChildProcess() as you mention.

[sindresorhus]

In my humble opinion, I think it's better in this case to not hook emit and just pass the error back to the caller of execa unmodified so the caller can handle it correctly.

I agree.

[mterrel]

@sindresorhus If you'd like, I'd be happy to submit a PR. Just let me know.

[ehmicky]

I'm realizing we don't need to wait for cross-spawn to fix this problem. Removing the call to crossSpawn._enoent.hookChildProcess() should be enough, correct?
If so, a PR would be great, thanks!

[mterrel]

No problem on creating a PR. So just to double-extra confirm: you just want the call to crossSpawn._enoent.hookChildProcess removed altogether for all cases. This would technically be a breaking change to the API (i.e. major version number for SemVer), correct? It hopefully doesn't affect too many users, but I just wanted to check that it's ok with y'all before I make the change. I'll mark the change appropriately and add some detail about what's breaking about the change and potential workarounds in the PR.

All that sound ok?

[ehmicky]

This sounds good to me, thanks for clarifying the fact that is technically a breaking change (although this could also be considered a bug fix I believe, depending on the way you look at it). What do you think @sindresorhus?

[mterrel]

I'm just thinking that for any user of execa that checks for ENOENT on the thrown error and takes some specific action, removal of hookChildProcess will mean that their action code will no longer be triggered. If I were such a user, I'd definitely consider that breaking and would have wanted a warning about it, preferably via major version number. Just my $0.01.

[ehmicky]

Yes, definitely.
On one hand, users depending on the assumption that ENOENT would be thrown on Windows anytime the exit code is 1 would get a breaking change. On the other hand, users depending on the assumption that ENOENT is thrown by execa only when the executable did not exist would get a bug fix.
Probably worth a major release as you point out.

[ehmicky]

PR at #447 thanks to @mterrel 🎉