From b1733d3c83d8241542ae3ea2f7fa638fe4667df1 Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:27:42 +0700 Subject: [PATCH] fix(plugins): make SIGKILL escalation idempotent per child (#13092) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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: #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. --- .../fixes/plugin-sigkill-listener-leak.md | 1 + src/lib/plugins/loader.ts | 46 +++++--- ...lugins-sigkill-listener-leak-12819.test.ts | 100 ++++++++++++++++++ 3 files changed, 135 insertions(+), 12 deletions(-) create mode 100644 changelog.d/fixes/plugin-sigkill-listener-leak.md create mode 100644 tests/unit/plugins-sigkill-listener-leak-12819.test.ts diff --git a/changelog.d/fixes/plugin-sigkill-listener-leak.md b/changelog.d/fixes/plugin-sigkill-listener-leak.md new file mode 100644 index 0000000000..a4baa47066 --- /dev/null +++ b/changelog.d/fixes/plugin-sigkill-listener-leak.md @@ -0,0 +1 @@ +- fix(plugins): stop leaking an exit listener per plugin hook timeout, which triggered MaxListenersExceededWarning on plugins that ignore SIGTERM diff --git a/src/lib/plugins/loader.ts b/src/lib/plugins/loader.ts index d4bcd2739d..71e9223b00 100644 --- a/src/lib/plugins/loader.ts +++ b/src/lib/plugins/loader.ts @@ -9,6 +9,7 @@ */ import { spawn } from "child_process"; +import type { ChildProcess } from "child_process"; import { writeFile, readFile } from "fs/promises"; import { rmSync } from "fs"; import { join } from "path"; @@ -105,6 +106,37 @@ function forwardChildOutput( * against process exit — under `node --test --test-force-exit` the runner exits * before the promise settles, leaking one temp .mjs per plugin load. */ +/** Children already escalating to SIGKILL. Prevents re-arming a second timer + listener + * for a child that is already being killed. */ +const escalating = new WeakSet(); + +/** + * SIGTERM has already been sent; escalate to SIGKILL if the child ignores it. + * + * Must be idempotent per child. Every hook timeout hits this path, and a plugin that + * traps SIGTERM keeps taking calls, so re-arming would add one exit listener plus one + * killTimer closure per timeout — Node starts printing MaxListenersExceededWarning at 11. + * One pending kill per child is also all that is useful: SIGKILL cannot be ignored, so a + * second timer would only re-signal a corpse. (#12819) + */ +function escalateToSigkill(child: ChildProcess): void { + if (escalating.has(child)) return; + escalating.add(child); + + const onExit = () => { + clearTimeout(killTimer); + escalating.delete(child); + }; + const killTimer = setTimeout(() => { + child.removeListener("exit", onExit); + escalating.delete(child); + try { + child.kill("SIGKILL"); + } catch {} + }, SIGKILL_GRACE_MS); + child.once("exit", onExit); +} + function removeHostScript(path: string): void { try { rmSync(path, { force: true }); @@ -293,12 +325,7 @@ export async function loadPlugin( } child.kill("SIGTERM"); // Escalate to SIGKILL if plugin ignores SIGTERM - const killTimer = setTimeout(() => { - try { - child.kill("SIGKILL"); - } catch {} - }, SIGKILL_GRACE_MS); - child.once("exit", () => clearTimeout(killTimer)); + escalateToSigkill(child); reject(new Error(`Plugin hook '${hook}' timed out after ${timeout}ms`)); }, timeout); @@ -399,12 +426,7 @@ export async function loadPlugin( const cleanup = () => { child.kill("SIGTERM"); // Escalate to SIGKILL after grace period - const killTimer = setTimeout(() => { - try { - child.kill("SIGKILL"); - } catch {} - }, SIGKILL_GRACE_MS); - child.once("exit", () => clearTimeout(killTimer)); + escalateToSigkill(child); removeHostScript(hostScriptPath); log.info("loader.cleanup", { name: manifest.name }); }; diff --git a/tests/unit/plugins-sigkill-listener-leak-12819.test.ts b/tests/unit/plugins-sigkill-listener-leak-12819.test.ts new file mode 100644 index 0000000000..df6a39638a --- /dev/null +++ b/tests/unit/plugins-sigkill-listener-leak-12819.test.ts @@ -0,0 +1,100 @@ +// Regression test for #12819 — loadPlugin() leaked one "exit" listener per hook timeout. +// +// Root cause: on the SIGTERM→SIGKILL escalation path the loader attached a fresh +// `child.once("exit", () => clearTimeout(killTimer))`. `once` only detaches when exit +// actually FIRES, so a plugin that ignores SIGTERM leaves the listener (and its killTimer +// closure) attached on every hook timeout. Node then prints MaxListenersExceededWarning +// once 11 accumulate. +// +// The plugin below traps SIGTERM and keeps running, which is exactly the condition the +// bug needs. We drive several hook timeouts and assert the listener count stays bounded. +import { test, describe, after } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, writeFileSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +const { loadPlugin } = await import("../../src/lib/plugins/loader.ts"); + +const dirs: string[] = []; +after(() => { + for (const d of dirs) rmSync(d, { recursive: true, force: true }); +}); + +/** A plugin that ignores SIGTERM and never answers a hook, forcing the escalation path. */ +function writeStubbornPlugin(): string { + const dir = mkdtempSync(join(tmpdir(), "omniroute-plugin-12819-")); + dirs.push(dir); + const entry = join(dir, "index.mjs"); + writeFileSync( + entry, + [ + // Trap SIGTERM so the loader has to escalate to SIGKILL. + 'process.on("SIGTERM", () => {});', + "export default {", + " // Never resolves → every call hits the hook timeout.", + " onRequest: () => new Promise(() => {}),", + "};", + "", + ].join("\n") + ); + return entry; +} + +describe("plugin loader SIGKILL escalation (#12819)", () => { + test("does not accumulate an exit listener per hook timeout", async () => { + const entryPoint = writeStubbornPlugin(); + const loaded = await loadPlugin( + entryPoint, + { + name: "sigkill-listener-leak", + version: "1.0.0", + license: "MIT", + main: "index.mjs", + source: "local", + tags: [], + requires: { permissions: [] }, + hooks: { onRequest: true, onResponse: false, onError: false }, + skills: [], + enabledByDefault: false, + configSchema: {}, + } as never, + { hookTimeoutMs: 120 } + ); + + const onRequest = ( + loaded.plugin as unknown as { + onRequest?: (ctx: unknown) => Promise; + } + ).onRequest; + assert.ok(onRequest, "onRequest hook should be registered"); + + // `child` is private to the loader, so observe the leak the way a user does: Node + // itself emits MaxListenersExceededWarning once an emitter passes 10 listeners. + const warnings: string[] = []; + const onWarning = (w: Error) => { + if (w.name === "MaxListenersExceededWarning") warnings.push(w.message); + }; + process.on("warning", onWarning); + + try { + // 12 timeouts: comfortably past Node's default limit of 10, so the pre-fix code + // trips the warning while the fixed code stays flat. + for (let i = 0; i < 12; i++) { + await onRequest({ body: {} }).catch(() => undefined); + } + // Warnings are delivered on the next tick; let them land before asserting. + await new Promise((r) => setTimeout(r, 50)); + } finally { + process.removeListener("warning", onWarning); + } + + assert.deepEqual( + warnings, + [], + `hook timeouts must not accumulate exit listeners (#12819): ${warnings[0] ?? ""}` + ); + + loaded.cleanup?.(); + }); +});