Exit with non-zero code when subprocess terminated by signal (#2023)

lib/command.js

@@ -1040,15 +1040,15 @@ Expecting one of '${allowedValues.join("', '")}'`);
     }
 
     // By default terminate process when spawned process terminates.
-    // Suppressing the exit if exitCallback defined is a bit messy and of limited use, but does allow process to stay running!
     const exitCallback = this._exitCallback;
-    if (!exitCallback) {
-      proc.on('close', process.exit.bind(process));
-    } else {
-      proc.on('close', () => {
-        exitCallback(new CommanderError(process.exitCode || 0, 'commander.executeSubCommandAsync', '(close)'));
-      });
-    }
+    proc.on('close', (code, _signal) => {
+      code = code ?? 1; // code is null if spawned process terminated due to a signal
+      if (!exitCallback) {
+        process.exit(code);
+      } else {
+        exitCallback(new CommanderError(code, 'commander.executeSubCommandAsync', '(close)'));
+      }
+    });
     proc.on('error', (err) => {
       // @ts-ignore
       if (err.code === 'ENOENT') {

tests/command.executableSubcommand.signals.test.js

@@ -1,39 +1,46 @@
 const childProcess = require('child_process');
 const path = require('path');
 
-// Test that a signal sent to the parent process is received by the executable subcommand process (which is listening).
+const pmPath = path.join(__dirname, 'fixtures', 'pm');
 
-// Disabling tests on Windows as:
+// Disabling some tests on Windows as:
 // "Windows does not support sending signals"
 //  https://nodejs.org/api/process.html#process_signal_events
 const describeOrSkipOnWindows = (process.platform === 'win32') ? describe.skip : describe;
 
-// Note: the previous (sinon) test had custom code for SIGUSR1, revisit if required:
-//    As described at https://nodejs.org/api/process.html#process_signal_events
-//    this signal will start a debugger and thus the process might output an
-//    additional error message:
-//      "Failed to open socket on port 5858, waiting 1000 ms before retrying".
+describeOrSkipOnWindows('signals', () => {
+  test.each(['SIGINT', 'SIGHUP', 'SIGTERM', 'SIGUSR1', 'SIGUSR2'])('when program sent %s then executableSubcommand sent signal too', (signal, done) => {
+    // Spawn program. The listen subcommand waits for a signal and writes the name of the signal to stdout.
+    const proc = childProcess.spawn(pmPath, ['listen'], {});
 
-describeOrSkipOnWindows.each([['SIGINT'], ['SIGHUP'], ['SIGTERM'], ['SIGUSR1'], ['SIGUSR2']])(
-  'test signal handling in executableSubcommand', (value) => {
-    // Slightly tricky test, stick with callback and disable lint warning.
-    // eslint-disable-next-line jest/no-done-callback
-    test(`when command killed with ${value} then executableSubcommand receives ${value}`, (done) => {
-      const pmPath = path.join(__dirname, './fixtures/pm');
+    let processOutput = '';
+    proc.stdout.on('data', (data) => {
+      if (processOutput.length === 0) {
+        // Send signal to program.
+        proc.kill(`${signal}`);
+      }
+      processOutput += data.toString();
+    });
+    proc.on('close', (code) => {
+      // Check the child subcommand received the signal too.
+      expect(processOutput).toBe(`Listening for signal...${signal}`);
+      done();
+    });
+  });
 
-      // The child process writes to stdout.
-      const proc = childProcess.spawn(pmPath, ['listen'], {});
+  test('when executable subcommand sent signal then program exit code is non-zero', () => {
+    const { status } = childProcess.spawnSync(pmPath, ['terminate'], {});
+    expect(status).toBeGreaterThan(0);
+  });
 
-      let processOutput = '';
-      proc.stdout.on('data', (data) => {
-        if (processOutput.length === 0) {
-          proc.kill(`${value}`);
-        }
-        processOutput += data.toString();
-      });
-      proc.on('close', (code) => {
-        expect(processOutput).toBe(`Listening for signal...${value}`);
-        done();
-      });
-    });
+  test('when command has exitOverride and executable subcommand sent signal then exit code is non-zero', () => {
+    const { status } = childProcess.spawnSync(pmPath, ['exit-override', 'terminate'], {});
+    expect(status).toBeGreaterThan(0);
+  });
+
+  // Not a signal test, but closely related code so adding here.
+  test('when command has exitOverride and executable subcommand fails then program exit code is subcommand exit code', () => {
+    const { status } = childProcess.spawnSync(pmPath, ['exit-override', 'fail'], {});
+    expect(status).toEqual(42);
   });
+});

tests/fixtures/pm

@@ -1,6 +1,7 @@
 #!/usr/bin/env node
 
-var { program } = require('../../');
+const path = require('node:path');
+const { program } = require('../../');
 
 process.env.FORCE_COLOR = 0; // work-around bug in Jest: https://github.com/jestjs/jest/issues/14391
 
@@ -17,4 +18,14 @@ program
   .command('specifyInstall', 'specify install subcommand', { executableFile: 'pm-install' })
   .command('specifyPublish', 'specify publish subcommand', { executableFile: 'pm-publish' })
   .command('silent', 'silently succeed')
+  .command('fail', 'exit with non-zero status code')
+  .command('terminate', 'terminate due to signal');
+
+program
+  .command('exit-override')
+  .exitOverride((err) => { process.exit(err.exitCode); })
+  .command('fail', 'exit with non-zero status code', { executableFile: path.join(__dirname, 'pm-fail.js') })
+  .command('terminate', 'terminate due to signal', { executableFile: path.join(__dirname, 'pm-terminate.js') });
+
+program
   .parse(process.argv);

tests/fixtures/pm-fail.js

@@ -0,0 +1 @@
+process.exit(42);

tests/fixtures/pm-terminate.js

@@ -0,0 +1 @@
+process.kill(process.pid, 'SIGINT');