[BUG] exit codes not being set properly in node v20

[ericcornelissen]

Is there an existing issue for this?

• I have searched the existing issues

This issue exists in the latest npm version

• I am using the latest npm

Current Behavior

Running npm audit (seemingly) always exists with a 0 exit code, even if a vulnerability is found (with a severity higher than configured by the audit-level).

In contrast, npm@8.15.0 (node@18.7.0) exits with a 1 if a vulnerability is found (with a severity higher than configured by the audit-level).

Expected Behavior

Per the Exit Code section:

[...]

If vulnerabilities were found the exit code will depend on the audit-level config.

Steps To Reproduce

1. Clone https://github.com/ericcornelissen/shescape/tree/dbaa0fd36af4fd0439af87548ce710468f25cb18
2. Run npm audit
3. Observe a warning for (at least) the high severity vulnerability GHSA-9c47-m6qq-7p4h
4. Run echo $?
5. Observe a 0 being printed

Environment

• npm: 9.6.5
• Node.js: v20.0.0
• OS Name: Ubuntu 22.04.2 LTS
• System Model Name: custom
• npm config:

; "user" config from ~/.npmrc

update-notifier = false 

; "project" config from ~/workspace/shescape/.npmrc

lockfile-version = "3" 
save-exact = true 
save-prefix = "" 

; node bin location = ~/.nvm/versions/node/v20.0.0/bin/node
; node version = v20.0.0
; npm local prefix = ~/Documents/workspace/shescape
; npm version = 9.6.5
; cwd = ~/Documents/workspace/shescape
; HOME = ~
; Run `npm config ls -l` to show all defaults.

[darthmaim]

The same happens when running workspace scripts: npm run build -w test
If the script fails, npm still returns with exit code 0.

I have made a reproduction repository: https://github.com/darthmaim/node-exit-code-test
I also added a workflow to test multiple node/npm versions: https://github.com/darthmaim/node-exit-code-test/actions/runs/4837883808

It only fails on node 20

This probably breaks all CI jobs when using worskpaces and node 20, so maybe this should be higher priority?

Hi - thanks for looking into this.

We're using workspaces, npm 9.6.6 and node 20.1.0 and this is currently breaking our CI jobs

[srapilly]

This should be related to this changes on node side nodejs/node#48027

process.exit will be called with undefined here:

cli/lib/utils/exit-handler.js

Lines 220 to 223 in 829503b

	let flushed = 0
	const flush = [process.stderr, process.stdout]
	const exit = () => ++flushed === flush.length && process.exit(exitCode)
	flush.forEach((f) => f.write('', exit))

and with node 20, this will override the process.exitCode set earlier

cli/lib/commands/run-script.js

Lines 200 to 222 in 09b58e4

	const pkg = await rpj(`${workspacePath}/package.json`)
	const runResult = await this.run(args, {
	path: workspacePath,
	pkg,
	}).catch(err => {
	log.error(`Lifecycle script \`${args[0]}\` failed with error:`)
	log.error(err)
	log.error(` in workspace: ${pkg._id || pkg.name}`)
	log.error(` at location: ${workspacePath}`)
	const scriptMissing = err.message.startsWith('Missing script')
	// avoids exiting with error code in case there's scripts missing
	// in some workspaces since other scripts might have succeeded
	if (!scriptMissing) {
	process.exitCode = 1
	}
	return scriptMissing
	})
	res.push(runResult)
	}

[wraithgar]

@MichaelBitard the previous comment shows the root cause here. v20 subtly changed how exitCode setting works.

The process.exit in the exit handler needs to be smarter about either not passing anything and setting process.exitCode if it needs to, or casting that to 0 if it's not already set.

[MichaelBitard]

Ok thanks, from what I see err is always undefined in our cases (running a script that fails in a workspace), but process.exitCode is set to a non 0 value.

I should rely on it.

If I change line 134:

cli/lib/utils/exit-handler.js

Lines 130 to 140 in 829503b

	log.notice('', npm.updateNotification)
	log.level = level
	}
	let exitCode
	let noLogMessage
	let jsonError
	if (err) {
	exitCode = 1
	// if we got a command that just shells out to something else, then it

to let exitCode = process.exitCode then I get the correct behavior.

This seems ok to you?

[ljharb]

process.exitCode || 1 would ensure it’s always nonzero, which seems like what’s desired.

[MichaelBitard]

I think you mean process.exitCode || 0 because it should exit gracefully on most occasions no? or is exitHandler always called on error?

[ljharb]

oh, if it’s ok to sometimes have zero then your original suggestion is what’s wanted, i was just matching the current semantics,

[MichaelBitard]

Yes I think that's it, if everything works well, then exitHandler is called and process.exitCode is undefined, so we don't want to force it to 1 here