fix: handle signal exits gracefully

owlstronaut · owlstronaut · commit 0a97ffdf8b2d · 2025-07-15T09:21:26.000-07:00
diff --git a/lib/cli/exit-handler.js b/lib/cli/exit-handler.js
@@ -43,6 +43,16 @@ class ExitHandler {
   registerUncaughtHandlers () {
     this.#process.on('uncaughtException', this.#handleExit)
     this.#process.on('unhandledRejection', this.#handleExit)
+
+    // Handle signals that might bypass normal exit flow
+    // These signals can cause the process to exit without calling the exit handler
+    const signalsToHandle = ['SIGTERM', 'SIGINT', 'SIGHUP']
+    for (const signal of signalsToHandle) {
+      this.#process.on(signal, () => {
+        // Call the exit handler to ensure proper cleanup
+        this.#handleExit(new Error(`Process received ${signal}`))
+      })
+    }
   }
 
   exit (err) {
@@ -57,6 +67,17 @@ class ExitHandler {
     this.#process.off('exit', this.#handleProcesExitAndReset)
     this.#process.off('uncaughtException', this.#handleExit)
     this.#process.off('unhandledRejection', this.#handleExit)
+
+    const signalsToCleanup = ['SIGTERM', 'SIGINT', 'SIGHUP']
+    for (const signal of signalsToCleanup) {
+      try {
+        this.#process.off(signal, this.#handleExit)
+      } catch (err) {
+        // Ignore errors during cleanup - this is defensive programming for edge cases
+        // where the process object might be in an unexpected state during shutdown
+      }
+    }
+
     if (this.#loaded) {
       this.#npm.unload()
     }
diff --git a/test/lib/cli/exit-handler.js b/test/lib/cli/exit-handler.js
@@ -4,7 +4,7 @@ const EventEmitter = require('node:events')
 const os = require('node:os')
 const t = require('tap')
 const fsMiniPass = require('fs-minipass')
-const { output, time } = require('proc-log')
+const { output, time, log } = require('proc-log')
 const errorMessage = require('../../../lib/utils/error-message.js')
 const ExecCommand = require('../../../lib/commands/exec.js')
 const { load: loadMockNpm } = require('../../fixtures/mock-npm')
@@ -707,3 +707,136 @@ t.test('do no fancy handling for shellouts', async t => {
     })
   })
 })
+
+t.test('container scenarios that trigger exit handler bug', async t => {
+  t.test('process.exit() called before exit handler cleanup', async (t) => {
+    // Simulates when npm process exits directly without going through proper cleanup
+
+    let exitHandlerNeverCalledLogged = false
+    let npmBugReportLogged = false
+
+    await mockExitHandler(t, {
+      config: { loglevel: 'notice' },
+    })
+
+    // Override log.error to capture the specific error messages
+    const originalLogError = log.error
+    log.error = (prefix, msg) => {
+      if (msg === 'Exit handler never called!') {
+        exitHandlerNeverCalledLogged = true
+      }
+      if (msg === 'This is an error with npm itself. Please report this error at:') {
+        npmBugReportLogged = true
+      }
+      return originalLogError(prefix, msg)
+    }
+
+    t.teardown(() => {
+      log.error = originalLogError
+    })
+
+    // This happens when containers are stopped/killed before npm can clean up properly
+    process.emit('exit', 1)
+
+    // Verify the bug is detected and logged correctly
+    t.equal(exitHandlerNeverCalledLogged, true, 'should log "Exit handler never called!" error')
+    t.equal(npmBugReportLogged, true, 'should log npm bug report message')
+  })
+
+  t.test('SIGTERM signal is handled properly', (t) => {
+    // This test verifies that our fix handles SIGTERM signals
+
+    const ExitHandler = tmock(t, '{LIB}/cli/exit-handler.js')
+    const exitHandler = new ExitHandler({ process })
+
+    const initialSigtermCount = process.listeners('SIGTERM').length
+    const initialSigintCount = process.listeners('SIGINT').length
+    const initialSighupCount = process.listeners('SIGHUP').length
+
+    // Register signal handlers
+    exitHandler.registerUncaughtHandlers()
+
+    const finalSigtermCount = process.listeners('SIGTERM').length
+    const finalSigintCount = process.listeners('SIGINT').length
+    const finalSighupCount = process.listeners('SIGHUP').length
+
+    // Verify the fix: signal handlers should be registered
+    t.ok(finalSigtermCount > initialSigtermCount, 'SIGTERM handler should be registered')
+    t.ok(finalSigintCount > initialSigintCount, 'SIGINT handler should be registered')
+    t.ok(finalSighupCount > initialSighupCount, 'SIGHUP handler should be registered')
+
+    // Clean up listeners to avoid affecting other tests
+    const sigtermListeners = process.listeners('SIGTERM')
+    const sigintListeners = process.listeners('SIGINT')
+    const sighupListeners = process.listeners('SIGHUP')
+
+    for (const listener of sigtermListeners) {
+      process.removeListener('SIGTERM', listener)
+    }
+    for (const listener of sigintListeners) {
+      process.removeListener('SIGINT', listener)
+    }
+    for (const listener of sighupListeners) {
+      process.removeListener('SIGHUP', listener)
+    }
+
+    t.end()
+  })
+
+  t.test('signal handler execution', async (t) => {
+    const ExitHandler = tmock(t, '{LIB}/cli/exit-handler.js')
+    const exitHandler = new ExitHandler({ process })
+
+    // Register signal handlers
+    exitHandler.registerUncaughtHandlers()
+
+    process.emit('SIGTERM')
+    process.emit('SIGINT')
+    process.emit('SIGHUP')
+
+    // Clean up listeners
+    process.removeAllListeners('SIGTERM')
+    process.removeAllListeners('SIGINT')
+    process.removeAllListeners('SIGHUP')
+
+    t.pass('signal handlers executed successfully')
+    t.end()
+  })
+
+  t.test('hanging async operation interrupted by signal', async (t) => {
+    // This test simulates the scenario where npm hangs on a long operation and receives SIGTERM/SIGKILL before it can complete
+
+    let exitHandlerNeverCalledLogged = false
+
+    const { exitHandler } = await mockExitHandler(t, {
+      config: { loglevel: 'notice' },
+    })
+
+    // Override log.error to detect the bug message
+    const originalLogError = log.error
+    log.error = (prefix, msg) => {
+      if (msg === 'Exit handler never called!') {
+        exitHandlerNeverCalledLogged = true
+      }
+      return originalLogError(prefix, msg)
+    }
+
+    t.teardown(() => {
+      log.error = originalLogError
+    })
+
+    // Track if exit handler was called properly
+    let exitHandlerCalled = false
+    exitHandler.exit = () => {
+      exitHandlerCalled = true
+    }
+
+    // Simulate sending signal to the process without proper cleanup
+    // This mimics what happens when a container is terminated
+    process.emit('exit', 1)
+
+    // Verify the bug conditions
+    t.equal(exitHandlerCalled, false, 'exit handler should not be called in this scenario')
+    t.equal(exitHandlerNeverCalledLogged, true, 'should detect and log the exit handler bug')
+  })
+})
