Skip to content

fix: allows graceful shutdown without misleading messaging #8523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix: allows graceful shutdown without misleading messaging #8523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 14 additions & 26 deletions lib/cli/exit-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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()
}
Expand Down Expand Up @@ -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('', ' <https://github.com/npm/cli/issues>')
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('', ' <https://github.com/npm/cli/issues>')
if (this.#npm.silent) {
output.error('')
}
}
}

Expand Down
140 changes: 17 additions & 123 deletions test/lib/cli/exit-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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!'])
})
})
Loading