Repository navigation
fix(core): clean up repeated shutdown signal listeners - #17625
Conversation
|
a single call like enableShutdownHooks(['sigterm', 'SIGTERM']) (the Set-dedup runs before uppercase/trim normalization) registers two listeners but remembers only one, leaking a listener that the old activeShutdownSignals-based removal loop used to clean up |
|
and also Opus feedback: the promoted receivedSignal flag is never reset anywhere so it latches permanently once a cleanup fires, and activeShutdownSignals is now a redundant, never-cleared duplicate of the Map's keys (dropping it in favor of shutdownCleanupRefs.has() would fix the silent no-op when re-enabling hooks after close()). Also worth noting: the new "repeated registration" test passes on pre-PR code, so it doesn't actually guard the fix. The remaining findings are test-code cleanups. |
Track each registered cleanup handler and share shutdown state across registrations so close removes every process listener and concurrent signals run hooks once. Closes nestjs#17623
706fd6f to
2b44718
Compare
|
Thanks for the feedback. I updated the PR to address each point:
I also rebased onto the latest |
micalevisk
left a comment
micalevisk
left a comment
There was a problem hiding this comment.
Thanks for the follow-up, the source changes look correct to me and all three points from the earlier comments are addressed. I ran the unit spec (31 passing), the integration/hooks e2e after a full build (5 passing), lint/prettier, and a handful of targeted source mutations against the new tests.
One thing is still missing before this can go in: there is no regression test for the headline case of #17623, i.e. the same signal registered by two separate enableShutdownHooks() calls. The "normalize" test covers aliases within one call and "separate calls" covers different signals, but if I delete the .filter(signal => !this.shutdownCleanupRefs.has(signal)) line the whole spec still passes, even though that mutant leaks the first listener on close() (the second call overwrites the Map entry and registers a second listener). See the inline suggestion; I confirmed that test fails on master, fails on the mutant, and passes on this branch.
A couple of smaller test-quality points inline (two tests that can pass vacuously, and the repeated finally cleanup). No behaviour regressions found: re-enabling after close(), all-listener removal on the signal path, and running hooks once across registrations all match what the issue asks for and are not breaking.
Out of scope for this PR, but worth a follow-up issue: close() does not check receivedSignal, so calling it while a signal-triggered shutdown is in flight still runs the hook sequence twice (the adjacent behaviour from #14998), and options passed on a later call for an already-registered signal are silently ignored.
Add a regression for the same signal registered by separate calls and assert listener counts before and after cleanup. Share baseline cleanup, use the handler map consistently, and document registration behavior. Reset the shutdown guard alongside listener cleanup.
|
Thanks for the detailed review, @micalevisk. Addressed the feedback in commit "a454e680562cfe43f4b96bfcb9171cf48d0eeb30":
Validation on Linux with Node 24.19.0:
The existing manifest/lockfile mismatch prevented "npm ci", so validation used "npm install --legacy-peer-deps --no-package-lock --no-audit --no-fund". No dependency files changed. "NO_COLOR" was unset because the logger spec expects ANSI colors |
Promoting the received-signal guard to instance scope stopped concurrent signals from tearing the context down twice, but `close()` never took part in it. A signal delivered while a programmatic `close()` was still awaiting its hooks re-entered the cleanup handler with the guard clear and ran `callDestroyHook`, `callBeforeShutdownHook`, `dispose` and `callShutdownHook` a second time, concurrently. Replace the boolean with the in-flight shutdown promise itself and share it between both entry points, so whichever arrives second awaits the first instead of starting its own teardown. The sequence moves into `runShutdownSequence`, which `close()` and the signal handler now reach through `shutdown()` rather than each keeping their own copy of the six lifecycle calls. Clearing the promise once the cycle settles also unwedges the failure path: previously a throw skipped `unsubscribeFromProcessSignals`, leaving the flag latched and the registry populated, so the reset in `enableShutdownHooks` could never fire and the context ignored every later signal. Co-Authored-By: Claude Opus 5 <[email protected]>
Summary
enableShutdownHooks()Testing
tsc -b packages/core --pretty falseoxlint packages/core/nest-application-context.ts packages/core/test/nest-application-context.spec.tsgit diff --checkSIGTERMtwice: listener count0 -> 1 -> 0SIGTERMandSIGINTseparately: both0 -> 1 -> 0Closes #17623