fix: ensure that exit code for fatal errors is not overwritten

[mdjermanovic]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Currently, when an unhandled exception happens before the main task is completed, the main task overwrites the exit code, so there can be a scenario where eslint exits with zero (i.e., success) although there was an unhandled exception.

Reproduction is this test that is currently failing in CI, though only on Node 21.1.0.

What changes did you make? (Give an overview)

Updated bin/eslint.js to not change the exit code if onFatalError was called.

I also added another test that should be failing without this change in all Node versions (at least it does for me locally on Node 20.7.0).

Is there anything you'd like reviewers to focus on?

[netlify]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	a41b041
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/653b9beb4d08bb0008ccfbba

[nzakas]

This looks okay to me, but I'd like to hold off on merging until we can verify this isn't an issue with Node.js 21. To me, this looks like a timing issue in Node.js, and I'd prefer not add in the additional checks until we can verify that. (We've had this code through several major versions of Node.js without any issues -- it seems unlikely that a change in timing like this is intentional.)

[nzakas]

Can you add some comments around these additions?

[nzakas]

Update: spent some time trying to create a minimal repro and couldn’t do it, so ok with merging this after some comments are added.

[fasttime]

Just a note that using Promise.resolve shouldn't be necessary. In the current implementation, loadFlatConfigFile always returns a Promise, because it's an async function. It would be pretty tricky to make it return synchronously, and that would be only possible for CommonJS modules.

[mdjermanovic]

You're right, and I noticed the same while experimenting with the test, that it works as intended without the Promise too. I decided to keep exporting the Promise to make sure there must be an async step so that the error is thrown before the linting finishes no matter how we rewrite the code in the future.

However, I have now checked more thoroughly what happens while executing this and it seems that multiple awaits do pass before the next tick 🤔. I'll update the test to return a Promise that isn't initially settled.

[fasttime]

LGTM

[mdjermanovic]

I can reproduce eslint exiting with 0 after an uncaught exception in Node 20 this way:

// eslint.config.js

process.nextTick(() => {
    throw new Error("My error");
});

module.exports = [];

Current behavior:

$ node bin/eslint Makefile.js; echo exit code $?

Oops! Something went wrong! :(

ESLint: 8.52.0

Error: My error
    at C:\projects\eslint\eslint.config.js:4:11
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11)
exit code 0

After the fix, i.e. with bin/eslint.js from this PR:

$ node bin/eslint Makefile.js; echo exit code $?

Oops! Something went wrong! :(

ESLint: 8.52.0

Error: My error
    at C:\projects\eslint\eslint.config.js:4:11
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11)
exit code 2

The default behavior of Node.js is to exit with non-zero right away when an uncaught exception happens. This is how it would work if we remove process.on("uncaughtException", onFatalError);:

$ node bin/eslint Makefile.js; echo exit code $?
C:\projects\eslint\eslint.config.js:4
    throw new Error("My error");
    ^

Error: My error
    at C:\projects\eslint\eslint.config.js:4:11
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11)

Node.js v20.7.0
exit code 1

Our error handling suppresses exiting after uncaught exceptions and thus lets all tasks finish, which is I think questionable behavior, but either way I believe it definitely shouldn't exit with 0 at the end. There probably is a bug in Node 21.1.0 that affects the test case that's currently failing on our CI, as the task execution order is defined and therefore shouldn't change (at least not in minor versions), but the Node bug showed that we do have a bug in our error handling.

[mdjermanovic]

I've added comments and updated the new test. Since the code changes are approved, I'm going to merge this now to fix CI, but please let me know if anything should be updated in a follow-up.