Skip to content

Commit 03d3ee7

Browse files
owlstronautowlstronaut
committed
fix: allows graceful shutdown without misleading messaging
1 parent ed71acb commit 03d3ee7

File tree

2 files changed

+30
-149
lines changed

2 files changed

+30
-149
lines changed

‎lib/cli/exit-handler.js‎

Lines changed: 13 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -43,16 +43,6 @@ class ExitHandler {
4343
registerUncaughtHandlers () {
4444
this.#process.on('uncaughtException', this.#handleExit)
4545
this.#process.on('unhandledRejection', this.#handleExit)
46-
47-
// Handle signals that might bypass normal exit flow
48-
// These signals can cause the process to exit without calling the exit handler
49-
const signalsToHandle = ['SIGTERM', 'SIGINT', 'SIGHUP']
50-
for (const signal of signalsToHandle) {
51-
this.#process.on(signal, () => {
52-
// Call the exit handler to ensure proper cleanup
53-
this.#handleExit(new Error(`Process received ${signal}`))
54-
})
55-
}
5646
}
5747

5848
exit (err) {
@@ -67,17 +57,6 @@ class ExitHandler {
6757
this.#process.off('exit', this.#handleProcesExitAndReset)
6858
this.#process.off('uncaughtException', this.#handleExit)
6959
this.#process.off('unhandledRejection', this.#handleExit)
70-
71-
const signalsToCleanup = ['SIGTERM', 'SIGINT', 'SIGHUP']
72-
for (const signal of signalsToCleanup) {
73-
try {
74-
this.#process.off(signal, this.#handleExit)
75-
} catch (err) {
76-
// Ignore errors during cleanup - this is defensive programming for edge cases
77-
// where the process object might be in an unexpected state during shutdown
78-
}
79-
}
80-
8160
if (this.#loaded) {
8261
this.#npm.unload()
8362
}
@@ -114,11 +93,19 @@ class ExitHandler {
11493
}
11594

11695
if (!this.#exited) {
117-
log.error('', 'Exit handler never called!')
118-
log.error('', 'This is an error with npm itself. Please report this error at:')
119-
log.error('', ' <https://github.com/npm/cli/issues>')
120-
if (this.#npm.silent) {
121-
output.error('')
96+
// When a process is terminated by a signal, its exit code is 128 + signal number.
97+
// The upper bound of 165 is because the highest numbered signal on Linux is 37 (SIGRTMAX).
98+
const isLikelySignalExit = exitCode > 128 && exitCode <= 165
99+
100+
if (isLikelySignalExit) {
101+
log.verbose('exit', `Process terminated by signal (exit code ${exitCode})`)
102+
} else {
103+
log.error('', 'Exit handler never called!')
104+
log.error('', 'This is an error with npm itself. Please report this error at:')
105+
log.error('', ' <https://github.com/npm/cli/issues>')
106+
if (this.#npm.silent) {
107+
output.error('')
108+
}
122109
}
123110
}
124111

‎test/lib/cli/exit-handler.js‎

Lines changed: 17 additions & 123 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const EventEmitter = require('node:events')
44
const os = require('node:os')
55
const t = require('tap')
66
const fsMiniPass = require('fs-minipass')
7-
const { output, time, log } = require('proc-log')
7+
const { output, time } = require('proc-log')
88
const errorMessage = require('../../../lib/utils/error-message.js')
99
const ExecCommand = require('../../../lib/commands/exec.js')
1010
const { load: loadMockNpm } = require('../../fixtures/mock-npm')
@@ -708,135 +708,29 @@ t.test('do no fancy handling for shellouts', async t => {
708708
})
709709
})
710710

711-
t.test('container scenarios that trigger exit handler bug', async t => {
712-
t.test('process.exit() called before exit handler cleanup', async (t) => {
713-
// Simulates when npm process exits directly without going through proper cleanup
714-
715-
let exitHandlerNeverCalledLogged = false
716-
let npmBugReportLogged = false
717-
718-
await mockExitHandler(t, {
719-
config: { loglevel: 'notice' },
720-
})
721-
722-
// Override log.error to capture the specific error messages
723-
const originalLogError = log.error
724-
log.error = (prefix, msg) => {
725-
if (msg === 'Exit handler never called!') {
726-
exitHandlerNeverCalledLogged = true
727-
}
728-
if (msg === 'This is an error with npm itself. Please report this error at:') {
729-
npmBugReportLogged = true
730-
}
731-
return originalLogError(prefix, msg)
732-
}
733-
734-
t.teardown(() => {
735-
log.error = originalLogError
711+
t.test('signal termination exits', async t => {
712+
t.test('exit code 127 does trigger Exit handler never called!', async (t) => {
713+
const { logs } = await mockExitHandler(t, {
714+
config: { loglevel: 'verbose' },
736715
})
737716

738-
// This happens when containers are stopped/killed before npm can clean up properly
739-
process.emit('exit', 1)
740-
741-
// Verify the bug is detected and logged correctly
742-
t.equal(exitHandlerNeverCalledLogged, true, 'should log "Exit handler never called!" error')
743-
t.equal(npmBugReportLogged, true, 'should log npm bug report message')
744-
})
745-
746-
t.test('SIGTERM signal is handled properly', (t) => {
747-
// This test verifies that our fix handles SIGTERM signals
748-
749-
const ExitHandler = tmock(t, '{LIB}/cli/exit-handler.js')
750-
const exitHandler = new ExitHandler({ process })
751-
752-
const initialSigtermCount = process.listeners('SIGTERM').length
753-
const initialSigintCount = process.listeners('SIGINT').length
754-
const initialSighupCount = process.listeners('SIGHUP').length
755-
756-
// Register signal handlers
757-
exitHandler.registerUncaughtHandlers()
758-
759-
const finalSigtermCount = process.listeners('SIGTERM').length
760-
const finalSigintCount = process.listeners('SIGINT').length
761-
const finalSighupCount = process.listeners('SIGHUP').length
762-
763-
// Verify the fix: signal handlers should be registered
764-
t.ok(finalSigtermCount > initialSigtermCount, 'SIGTERM handler should be registered')
765-
t.ok(finalSigintCount > initialSigintCount, 'SIGINT handler should be registered')
766-
t.ok(finalSighupCount > initialSighupCount, 'SIGHUP handler should be registered')
767-
768-
// Clean up listeners to avoid affecting other tests
769-
const sigtermListeners = process.listeners('SIGTERM')
770-
const sigintListeners = process.listeners('SIGINT')
771-
const sighupListeners = process.listeners('SIGHUP')
772-
773-
for (const listener of sigtermListeners) {
774-
process.removeListener('SIGTERM', listener)
775-
}
776-
for (const listener of sigintListeners) {
777-
process.removeListener('SIGINT', listener)
778-
}
779-
for (const listener of sighupListeners) {
780-
process.removeListener('SIGHUP', listener)
781-
}
782-
783-
t.end()
784-
})
717+
process.emit('exit', 127)
785718

786-
t.test('signal handler execution', async (t) => {
787-
const ExitHandler = tmock(t, '{LIB}/cli/exit-handler.js')
788-
const exitHandler = new ExitHandler({ process })
789-
790-
// Register signal handlers
791-
exitHandler.registerUncaughtHandlers()
792-
793-
process.emit('SIGTERM')
794-
process.emit('SIGINT')
795-
process.emit('SIGHUP')
796-
797-
// Clean up listeners
798-
process.removeAllListeners('SIGTERM')
799-
process.removeAllListeners('SIGINT')
800-
process.removeAllListeners('SIGHUP')
801-
802-
t.pass('signal handlers executed successfully')
803-
t.end()
719+
t.equal(process.exitCode, 127)
720+
t.notMatch(logs.verbose, ['Process terminated by signal (exit code 127)'])
721+
t.match(logs.error, ['Exit handler never called!'])
804722
})
805723

806-
t.test('hanging async operation interrupted by signal', async (t) => {
807-
// This test simulates the scenario where npm hangs on a long operation and receives SIGTERM/SIGKILL before it can complete
808-
809-
let exitHandlerNeverCalledLogged = false
810-
811-
const { exitHandler } = await mockExitHandler(t, {
812-
config: { loglevel: 'notice' },
724+
t.test('SIGTERM exit code 143', async (t) => {
725+
const { logs } = await mockExitHandler(t, {
726+
config: { loglevel: 'verbose' },
813727
})
814728

815-
// Override log.error to detect the bug message
816-
const originalLogError = log.error
817-
log.error = (prefix, msg) => {
818-
if (msg === 'Exit handler never called!') {
819-
exitHandlerNeverCalledLogged = true
820-
}
821-
return originalLogError(prefix, msg)
822-
}
823-
824-
t.teardown(() => {
825-
log.error = originalLogError
826-
})
827-
828-
// Track if exit handler was called properly
829-
let exitHandlerCalled = false
830-
exitHandler.exit = () => {
831-
exitHandlerCalled = true
832-
}
833-
834-
// Simulate sending signal to the process without proper cleanup
835-
// This mimics what happens when a container is terminated
836-
process.emit('exit', 1)
729+
process.emit('exit', 143)
837730

838-
// Verify the bug conditions
839-
t.equal(exitHandlerCalled, false, 'exit handler should not be called in this scenario')
840-
t.equal(exitHandlerNeverCalledLogged, true, 'should detect and log the exit handler bug')
731+
t.equal(process.exitCode, 143)
732+
t.match(logs.verbose.filter(l => l.includes('Process terminated by signal')),
733+
['Process terminated by signal (exit code 143)'])
734+
t.notMatch(logs.error, ['Exit handler never called!'])
841735
})
842736
})

0 commit comments

Comments
 (0)