fix(webhook): clear abort timer in finally to avoid dangling timers on fetch error (#4115)

Integrated into release/v3.8.28 (r8)

src/lib/webhookDispatcher.ts

@@ -45,14 +45,19 @@ async function deliverRaw(
     parseAndValidateWebhookUrl(url);
     const controller = new AbortController();
     const timeoutId = setTimeout(() => controller.abort(), 10_000);
-    const res = await fetch(url, {
-      method: "POST",
-      headers: { "Content-Type": "application/json", "User-Agent": "OmniRoute-Webhook/1.0" },
-      body: JSON.stringify(body),
-      signal: controller.signal,
-    });
-    clearTimeout(timeoutId);
-    return { success: res.ok, status: res.status, latencyMs: Date.now() - start };
+    try {
+      const res = await fetch(url, {
+        method: "POST",
+        headers: { "Content-Type": "application/json", "User-Agent": "OmniRoute-Webhook/1.0" },
+        body: JSON.stringify(body),
+        signal: controller.signal,
+      });
+      return { success: res.ok, status: res.status, latencyMs: Date.now() - start };
+    } finally {
+      // Always clear the abort timer — on a non-timeout fetch error the previous code skipped
+      // clearTimeout, leaving a dangling 10s timer (and AbortController) per failed call.
+      clearTimeout(timeoutId);
+    }
   } catch (error: any) {
     return {
       success: false,
@@ -91,13 +96,19 @@ export async function deliverWebhook(
       const controller = new AbortController();
       const timeoutId = setTimeout(() => controller.abort(), 10_000);
 
-      const res = await fetch(url, {
-        method: "POST",
-        headers,
-        body,
-        signal: controller.signal,
-      });
-      clearTimeout(timeoutId);
+      let res: Response;
+      try {
+        res = await fetch(url, {
+          method: "POST",
+          headers,
+          body,
+          signal: controller.signal,
+        });
+      } finally {
+        // Clear the abort timer on every path — a non-timeout fetch error previously skipped
+        // clearTimeout, leaking a dangling 10s timer + AbortController per failed attempt.
+        clearTimeout(timeoutId);
+      }
 
       if (res.ok || res.status < 500) {
         return { success: res.ok, status: res.status };

tests/unit/webhook-abort-timer-cleanup.test.ts

@@ -0,0 +1,57 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+
+const { deliverWebhook } = await import("../../src/lib/webhookDispatcher.ts");
+
+// Regression for the dangling abort-timer leak: deliverWebhook arms a 10s
+// setTimeout(() => controller.abort()) before each fetch. The pre-fix code only
+// called clearTimeout on the success path, so a non-timeout fetch rejection
+// (ECONNREFUSED, DNS failure, etc.) skipped clearTimeout, leaking a live 10s timer
+// + AbortController per failed delivery. The fix clears the timer in a `finally`.
+test("deliverWebhook clears the abort timer even when fetch rejects", async () => {
+  const realSetTimeout = globalThis.setTimeout;
+  const realClearTimeout = globalThis.clearTimeout;
+  const realFetch = globalThis.fetch;
+
+  const abortTimerIds = new Set<unknown>();
+  const clearedIds = new Set<unknown>();
+
+  // Track the 10s abort timer ids; delegate to the real timer so ids stay valid.
+  globalThis.setTimeout = ((fn: any, delay?: number, ...args: any[]) => {
+    const id = realSetTimeout(fn, delay as any, ...args);
+    if (delay === 10_000) abortTimerIds.add(id);
+    return id;
+  }) as typeof setTimeout;
+  globalThis.clearTimeout = ((id: any) => {
+    clearedIds.add(id);
+    return realClearTimeout(id);
+  }) as typeof clearTimeout;
+  // Non-timeout network failure — the exact path that previously skipped clearTimeout.
+  globalThis.fetch = (async () => {
+    throw new Error("ECONNREFUSED");
+  }) as typeof fetch;
+
+  try {
+    const res = await deliverWebhook(
+      "https://example.com/webhook",
+      { event: "test.event" as any, timestamp: new Date().toISOString(), data: {} },
+      null,
+      0 // maxRetries=0 → single attempt, no exponential-backoff timers
+    );
+
+    assert.equal(res.success, false, "delivery should fail when fetch rejects");
+    assert.ok(abortTimerIds.size >= 1, "an abort timer should have been armed");
+    // The regression guard: every armed abort timer must have been cleared,
+    // even though the fetch rejected.
+    for (const id of abortTimerIds) {
+      assert.ok(
+        clearedIds.has(id),
+        "abort timer must be cleared in finally even when fetch rejects (no dangling 10s timer)"
+      );
+    }
+  } finally {
+    globalThis.setTimeout = realSetTimeout;
+    globalThis.clearTimeout = realClearTimeout;
+    globalThis.fetch = realFetch;
+  }
+});