fix(plugins): make SIGKILL escalation idempotent per child - #13092
Merged
diegosouzapw merged 1 commit intoSep 11, 2026
Merged
diegosouzapw merged 1 commit into
diegosouzapw merged 1 commit into
Conversation
loadPlugin() attached a fresh `child.once("exit", () => clearTimeout(killTimer))`
on every SIGTERM->SIGKILL escalation. `once` only detaches when exit actually
fires, so a plugin that traps SIGTERM keeps serving calls and accumulates one
listener plus one killTimer closure per hook timeout. Node prints
MaxListenersExceededWarning once eleven pile up.
Escalation is now guarded by a WeakSet keyed on the child, so a child already
being killed does not re-arm. That is also all that is useful: SIGKILL cannot be
ignored, so a second timer would only re-signal a corpse. Both the timer and the
listener are released on every path, including the one where the child never
exits.
The shared helper replaces two copies of the same block, in the hook-timeout
path and in cleanup().
Fixes diegosouzapw#12819
Contributor
Author
|
Note on the checks here: the workflows are sitting in Locally on Windows 11 / Node 24.19.0: |
This was referenced Sep 9, 2026
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…apw#13092) `once` only detaches when exit actually fires, so a plugin trapping SIGTERM accumulated one listener and one timer per hook timeout. Keying idempotence on the child via a `WeakSet` is right — a second SIGKILL timer would only re-signal a corpse. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches⚠️ base-red inherited: diegosouzapw#12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
loadPlugin()attached a newchild.once("exit", () => clearTimeout(killTimer))on every SIGTERM→SIGKILL escalation.onceonly detaches when exit actually fires, so a plugin that traps SIGTERM keeps serving calls and accumulates one listener plus onekillTimerclosure per hook timeout. Node printsMaxListenersExceededWarningonce eleven pile up. Healthy plugins never reach this path.The change
Escalation is now guarded by a
WeakSetkeyed on the child, so a child already being killed does not re-arm. That is also all that is useful:SIGKILLcannot be ignored, so a second timer would only re-signal a corpse. Both the timer and the listener are released on every path — including the one where the child never exits, whichoncealone does not cover.The shared helper also replaces two copies of the same block (hook-timeout path and
cleanup()).Note on the first attempt
Self-detaching the listener inside
onExitis not sufficient on its own, and the test caught it. The grace period is 3s, far longer than the interval between hook timeouts, so listeners still accumulated faster than they were released:Idempotence per child is what actually fixes it.
Test
tests/unit/plugins-sigkill-listener-leak-12819.test.tsloads a real plugin that traps SIGTERM and never answers a hook, then drives 12 timeouts. It observes the leak the way a user does — by listening for Node's ownMaxListenersExceededWarning— because the child handle is private to the loader; asserting on a handle the test cannot reach would have silently passed either way.Verification
Fixes #12819