From 089eefb326d49e355ab75422d69df26b3dbb4eed Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Wiktor=20Obr=C4=99bski?= <wiktor.obrebski@gmail.com>
Date: Wed, 3 Jan 2024 21:13:03 +0100
Subject: [PATCH] fix: Revert Signal Handling Regression (#188)

This PR reverts the changes made in commit
`545f3be94d412941537ad0011717933d48cb58cf`, which inadvertently broke
signal forwarding to child processes (PR #142 ).
Contrary to the assumptions by @nlf , `SIGTERM` and similar signals are
not being correctly propagated to child processes. Instead, they are
only received by npm, resulting in incomplete signal handling.

The removal of signal forwarding in #142 means that child processes do
not receive necessary signals for appropriate cleanup and termination.

This issue is evident in workflows involving `npm start` used as a
Docker command for local execution. For instance, using CTRL + C does
not properly terminate the application
and results in a forced kill after a 10-second delay.

This behavior could lead to more significant problems in production
environments, (if `npm` is used to start the app) such as data loss due
to improper database connection closures.

Create a package.json with the following content:
```json
{
  "name": "npm",
  "scripts": {
    "start": "node ./main-test.js"
  }
}
```

Create a main-test.js file:
```typescript
const interval = setInterval(() => console.log('alive!'), 3000);

async function onSignal(signal) {
  console.log(`${signal} received, cleaning up...`);
  clearInterval(interval);
  console.log('Cleaning up done');
}
process.on('SIGINT', onSignal);
process.on('SIGTERM', onSignal);
```

Execute `npm start`. The script should output `alive!` every 3 seconds.
Attempt to terminate it using `kill -SIGTERM [PID]`.
It should log `Cleaning up done` and shut down gracefully,
which it does in older versions of `npm` (e.g., `v8.19.4`) but fails in
newer versions (e.g., `v9.6.7`).

Reverting this change will restore the expected behavior for signal
handling in `npm`

- https://github.com/npm/cli/issues/6547
- https://github.com/npm/cli/issues/6684
- #142
---
 lib/signal-manager.js  | 11 ++++++-----
 test/signal-manager.js | 21 +++++++++++++++++++++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/lib/signal-manager.js b/lib/signal-manager.js
index efc00b4..a099a4a 100644
--- a/lib/signal-manager.js
+++ b/lib/signal-manager.js
@@ -1,9 +1,6 @@
 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',
@@ -12,8 +9,12 @@ const forwardedSignals = [
 // 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 handleSignal = signal => {
+  for (const proc of runningProcs) {
+    proc.kill(signal)
+  }
+}
+
 const setupListeners = () => {
   for (const signal of forwardedSignals) {
     process.on(signal, handleSignal)
diff --git a/test/signal-manager.js b/test/signal-manager.js
index 7a0544f..afc0ab3 100644
--- a/test/signal-manager.js
+++ b/test/signal-manager.js
@@ -44,3 +44,24 @@ 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])
+})