Skip to content

fix: handle signal exits gracefully #8429

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.

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

Merged
merged 1 commit into from
Jul 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 21 additions & 0 deletions lib/cli/exit-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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()
}
Expand Down
135 changes: 134 additions & 1 deletion test/lib/cli/exit-handler.js
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 } = 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')
Expand Down Expand Up @@ -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')
})
})
Loading