diff --git a/lib/cli/exit-handler.js b/lib/cli/exit-handler.js index efb09138aec28..070a6e133076a 100644 --- a/lib/cli/exit-handler.js +++ b/lib/cli/exit-handler.js @@ -43,16 +43,6 @@ 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) { @@ -67,17 +57,6 @@ 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() } @@ -114,11 +93,20 @@ class ExitHandler { } if (!this.#exited) { - log.error('', 'Exit handler never called!') - log.error('', 'This is an error with npm itself. Please report this error at:') - log.error('', ' ') - if (this.#npm.silent) { - output.error('') + // When terminated by a signal, the exit code is 128 + signal number (POSIX convention). + // The upper bound of 165 is because the highest numbered signal on Linux is 37 (SIGRTMAX). + const isLikelySignalExit = exitCode > 128 && exitCode <= 165 + + // When a process receives a signal, it terminates directly and only fires the 'exit' event - no other JavaScript code gets to run, including #handleExit. + if (isLikelySignalExit) { + log.verbose('exit', `Process terminated by signal (exit code ${exitCode})`) + } else { + log.error('', 'Exit handler never called!') + log.error('', 'This is an error with npm itself. Please report this error at:') + log.error('', ' ') + if (this.#npm.silent) { + output.error('') + } } } diff --git a/test/lib/cli/exit-handler.js b/test/lib/cli/exit-handler.js index f8b112beab0a2..0a6274e5837b4 100644 --- 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, log } = require('proc-log') +const { output, time } = 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') @@ -708,135 +708,29 @@ 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 +t.test('signal termination exits', async t => { + t.test('exit code 127 does trigger Exit handler never called!', async (t) => { + const { logs } = await mockExitHandler(t, { + config: { loglevel: 'verbose' }, }) - // 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() - }) + process.emit('exit', 127) - 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.equal(process.exitCode, 127) + t.notMatch(logs.verbose, ['Process terminated by signal (exit code 127)']) + t.match(logs.error, ['Exit handler never called!']) }) - 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' }, + t.test('SIGTERM exit code 143', async (t) => { + const { logs } = await mockExitHandler(t, { + config: { loglevel: 'verbose' }, }) - // 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) + process.emit('exit', 143) - // 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') + t.equal(process.exitCode, 143) + t.match(logs.verbose.filter(l => l.includes('Process terminated by signal')), + ['Process terminated by signal (exit code 143)']) + t.notMatch(logs.error, ['Exit handler never called!']) }) })