From 285bd7d750536bb9affbf904335f57c753a6d57f Mon Sep 17 00:00:00 2001
From: nlf <quitlahok@gmail.com>
Date: Thu, 2 Mar 2023 14:37:45 -0800
Subject: [PATCH] fix: handle signals more correctly

previously we were assuming that npm received the signal first and was required to forward the signal to child processes, however that seems to no longer be the case. instead npm and the child process both receive the signal at the same time. the previous logic has been modified such that it places a no-op function as the signal handler. this is strictly to prevent the default behavior of exiting node from happening. once all child process have exited, the handlers are all removed and we exit appropriately
---
 lib/run-script-pkg.js  |  4 ++++
 lib/signal-manager.js  | 14 ++++++++------
 test/signal-manager.js | 21 ---------------------
 3 files changed, 12 insertions(+), 27 deletions(-)

diff --git a/lib/run-script-pkg.js b/lib/run-script-pkg.js
index c10d20b..80a4324 100644
--- a/lib/run-script-pkg.js
+++ b/lib/run-script-pkg.js
@@ -94,7 +94,11 @@ const runScriptPkg = async options => {
   return p.catch(er => {
     const { signal } = er
     if (stdio === 'inherit' && signal) {
+      // by the time we reach here, the child has already exited. we send the
+      // signal back to ourselves again so that npm will exit with the same
+      // status as the child
       process.kill(process.pid, signal)
+
       // just in case we don't die, reject after 500ms
       // this also keeps the node process open long enough to actually
       // get the signal, rather than terminating gracefully.
diff --git a/lib/signal-manager.js b/lib/signal-manager.js
index 7e10f85..efc00b4 100644
--- a/lib/signal-manager.js
+++ b/lib/signal-manager.js
@@ -1,17 +1,19 @@
 const runningProcs = new Set()
 let handlersInstalled = false
 
+// NOTE: these signals aren't actually forwarded anywhere. they're trapped and
+// ignored until all child processes have exited. in our next breaking change
+// we should rename this
 const forwardedSignals = [
   'SIGINT',
   'SIGTERM',
 ]
 
-const handleSignal = signal => {
-  for (const proc of runningProcs) {
-    proc.kill(signal)
-  }
-}
-
+// no-op, this is so receiving the signal doesn't cause us to exit immediately
+// instead, we exit after all children have exited when we re-send the signal
+// to ourselves. see the catch handler at the bottom of run-script-pkg.js
+// istanbul ignore next - this function does nothing
+const handleSignal = () => {}
 const setupListeners = () => {
   for (const signal of forwardedSignals) {
     process.on(signal, handleSignal)
diff --git a/test/signal-manager.js b/test/signal-manager.js
index afc0ab3..7a0544f 100644
--- a/test/signal-manager.js
+++ b/test/signal-manager.js
@@ -44,24 +44,3 @@ test('adds only one handler for each signal, removes handlers when children have
 
   t.end()
 })
-
-test('forwards signals to child process', t => {
-  const proc = new EventEmitter()
-  proc.kill = (signal) => {
-    t.equal(signal, signalManager.forwardedSignals[0], 'child receives correct signal')
-    proc.emit('exit', 0)
-    for (const forwarded of signalManager.forwardedSignals) {
-      t.equal(
-        process.listeners(forwarded).includes(signalManager.handleSignal),
-        false, 'listener has been removed')
-    }
-    t.end()
-  }
-
-  signalManager.add(proc)
-  // passing the signal name here is necessary to fake the effects of actually
-  // receiving the signal per nodejs documentation signal handlers receive the
-  // name of the signal as their first parameter
-  // https://nodejs.org/api/process.html#process_signal_events
-  process.emit(signalManager.forwardedSignals[0], signalManager.forwardedSignals[0])
-})