Exit Code is Zero if Subcommand Killed by Signal

[dsanders11]

If a subcommand child process is killed by a signal (e.g. SIGKILL), then commander will exit with exit code zero. I'm seeing this where OOM was killing the subcommand but commander was exiting with zero.

Relevant code looks to be here. The arguments to the close event are (code, signal) and in the case of killed by signal, code will be null, so process.exit gets called with null which leads to exiting with zero. Additionally, should the exitCallback case (on line 1036) be using proc.exitCode instead of process.exitCode? The latter doesn't seem correct.

commander.js/lib/command.js

Lines 1032 to 1038 in 4ef19fa

	if (!exitCallback) {
	proc.on('close', process.exit.bind(process));
	} else {
	proc.on('close', () => {
	exitCallback(new CommanderError(process.exitCode || 0, 'commander.executeSubCommandAsync', '(close)'));
	});
	}

Possible fix for the issue could be to bubble up signals, although I'm not sure how to handle the exitCallback case then, since AFAIK there's no way to get an exit code for a signal from Node.js (other than mapping it yourself).

proc.on('close', (code, signal) => {
  if (signal) {
    process.kill(process.pid, signal);
  } else {
    process.exit.bind(process)(code);
  }
});

Alternatively a fixed exit code of 1 could be used in the case of a signal, which would work for the exitCallback case. However, that's not desirable since it will hide the underlying exit code from the subcommand.

Manually mapping signals to exit codes might be the simplest way to handle this for both cases.

[shadowspawn]

Additionally, should the exitCallback case (on line 1036) be using proc.exitCode instead of process.exitCode?

Yes, I agree.

Edit: actually, should be using the code argument from the close event. (And signal argument, as this issue raises!)

[shadowspawn]

Relevant code looks to be here. The arguments to the close event are (code, signal) and in the case of killed by signal, code will be null, so process.exit gets called with null which leads to exiting with zero.

Ah right, calling process.exit with the actual event arguments is simplistic at best and wrong for the signal case.

Possible fix for the issue could be to bubble up signals,

I am not sure using same signal in parent is an appropriate response in all cases. (Although it would probably reproduce the eventual exit status.)

although I'm not sure how to handle the exitCallback case then, since AFAIK there's no way to get an exit code for a signal from Node.js (other than mapping it yourself).

The only portable way I found to map signal names to signal numbers is kill -l (i.e. shell, not node).

However, my understanding after some research is that the external "exit code" for a signal may be more complicated than what the program can achieve by calling .exit() anyway, with treatment depending on the shell.

For example: "When a command terminates on a fatal signal whose number is N, Bash uses the value 128+N as the exit status."

Alternatively a fixed exit code of 1 could be used in the case of a signal, which would work for the exitCallback case.

I think this would be simpler to implement, and simpler to predict behaviour.

However, that's not desirable since it will hide the underlying exit code from the subcommand.

Morally I am not sure if there is an underlying exit code at this point, and in particular this is explicit in what node makes available. We get passed an exit code or a signal, one or the other.

[shadowspawn]

The related 'exit' event description is a bit more explicit about the arguments:

The 'exit' event is emitted after the child process ends. If the process exited, code is the final exit code of the process, otherwise null. If the process terminated due to receipt of a signal, signal is the string name of the signal, otherwise null. One of the two will always be non-null.

https://nodejs.org/dist/latest-v18.x/docs/api/child_process.html#event-exit

[dsanders11]

Fair points. With this issue I'm concerned about two things, in order of importance:

1. Ensure commander exits with a non-zero exit code if the subcommand process exits due to a signal - otherwise it is silently failing which can cause chained commands (or CI jobs) to continue when they shouldn't
2. An easy way to debug what caused the subcommand to exit

A fixed exit code will address 1, which is good, but does not address 2. You're right that bubbling up the signal to the parent process may not be appropriate in certain corner cases, but it does address 2 in that you get some indication of what caused the subcommand to exit. A fixed exit code wouldn't be as much of a concern if there was a debug log or something (which I don't believe commander has) which could contain the extra information to address 2, but without one, I don't think a fixed exit code can resolve 2.

Given the existing constraints, there may not be a way to address 2, unfortunately.

[shadowspawn]

Thanks @dsanders11

I opened draft #2023. Does not have unit tests yet, and I'll leave it draft for a couple of days in case we have any fresh ideas about signal number.

[shadowspawn]

Released in Commander v12.0.0.