fix(combo): always clear the loop-safety timer, not just on the happy path (#11804)

dispatchWithCooldownRetry arms a loop-safety timer (setTimeout, 10 minutes by
default) on every setTry iteration, so a combo that never produces a terminal
response still answers with a 504 instead of hanging. The only clearTimeout in
the whole file sat inside the `if (anySuccess)` branch — the comment said so
verbatim: "clear the safety timer on the happy path".

Every error exit therefore returned the response to the client while leaving a
600s timer pending, its closure retaining orderedTargets and the exhausted
provider/connection sets: all_targets_skipped, all_accounts_inactive, the
aggregated-status return, the final fallback, and the global-timeout branch.
The timer is also re-armed per setTry iteration with no clear in between.

Field evidence from the issue: two requests that failed quality validation
returned 502 to the client immediately, and "Combo loop safety timeout ...
force-terminating" was logged for both exactly 600 seconds later — the leaked
timers firing long after the requests were gone.

Fixed structurally rather than by sprinkling clearTimeout across the five
return sites: the handle is hoisted to function scope and released in a
finally, so a future `return` added to this function cannot silently
reintroduce the leak. The 504 backstop itself is unchanged.

Note the timer already called .unref(), so it never held the event loop open —
this is a memory-retention leak, not a hang.

open-sse/services/combo.ts

@@ -1111,8 +1111,19 @@ async function handleComboChatInner({
     let lastError: string | null = null;
     let earliestRetryAfter: ComboRetryAfter | null = null;
     let lastStatus: number | null = null;
+    // #11804: the loop-safety timer is armed per setTry iteration but must be
+    // cleared on EVERY exit path, not just the happy one. Hoisted to function
+    // scope so the `finally` at the end of this function always reaches it —
+    // otherwise each error path (all_targets_skipped / all_accounts_inactive /
+    // aggregated status / final fallback / global timeout) returned to the client
+    // leaving a 10-minute timer pending, whose closure retains orderedTargets and
+    // the exhausted provider/connection sets. Field evidence on the issue: the
+    // client got a 502 immediately, and "Combo loop safety timeout ...
+    // force-terminating" was logged exactly 600s later.
+    let activeLoopSafetyTimer: ReturnType<typeof setTimeout> | null = null;
 
-    for (let setTry = 0; setTry <= maxSetRetries; setTry++) {
+    try {
+      for (let setTry = 0; setTry <= maxSetRetries; setTry++) {
       // #1731: Per-set-iteration set of providers whose quota is fully exhausted.
       // Reset each retry so providers excluded in a previous attempt get another chance.
       const exhaustedProviders = new Set<string>();
@@ -1198,6 +1209,7 @@ async function handleComboChatInner({
           );
         }, loopSafetyMs);
         loopSafetyTimer.unref?.();
+        activeLoopSafetyTimer = loopSafetyTimer;
       });
       const runningTasks = new Set<Promise<void>>();
       let anySuccess = false;
@@ -2870,11 +2882,20 @@ async function handleComboChatInner({
     // Surface the recovery hint with a generic retry recommendation so the client at least
     // gets a non-opaque message instead of "Combo routing completed without an upstream response".
     recordComboFailure(effectiveSessionId, combo.name);
-    return errorResponseWithComboDiagnostics(
-      503,
-      "Combo routing completed without an upstream response",
-      buildNoUpstreamResponseDiagnostics(orderedTargets.length)
-    );
+      return errorResponseWithComboDiagnostics(
+        503,
+        "Combo routing completed without an upstream response",
+        buildNoUpstreamResponseDiagnostics(orderedTargets.length)
+      );
+    } finally {
+      // #11804: always release the loop-safety timer. Covering every exit path by
+      // construction here means a future `return` added to this function cannot
+      // silently reintroduce the leak.
+      if (activeLoopSafetyTimer) {
+        clearTimeout(activeLoopSafetyTimer);
+        activeLoopSafetyTimer = null;
+      }
+    }
   };
 
   // FASE 2.1: acquire the per-connection concurrency slot for the selected

tests/unit/combo-loop-safety-timer-leak-11804.test.ts

@@ -0,0 +1,74 @@
+/**
+ * #11804 — the combo loop-safety timer must be cleared on EVERY exit path.
+ *
+ * `dispatchWithCooldownRetry` (open-sse/services/combo.ts) arms a
+ * `setTimeout(..., loopSafetyMs)` — 10 minutes by default — once per `setTry`
+ * iteration, so a combo that never produces a terminal response still answers
+ * the client with a 504 instead of hanging forever.
+ *
+ * Before this fix the only `clearTimeout` lived inside the `if (anySuccess)`
+ * branch (the code comment said so verbatim: "clear the safety timer on the
+ * happy path"). Every error exit — all_targets_skipped, all_accounts_inactive,
+ * the aggregated-status return, the final fallback, the global-timeout branch —
+ * returned the response to the client and left a 600s timer pending, its
+ * closure retaining `orderedTargets` and the exhausted provider/connection
+ * sets. Field evidence on the issue: a client received 502 immediately and the
+ * "Combo loop safety timeout ... force-terminating" line was logged exactly
+ * 600s later, long after the request was gone.
+ *
+ * This is a source-level guard rather than a runtime one: driving a real combo
+ * through each terminal branch needs the full provider/credential/DB stack, and
+ * the invariant we actually care about ("no exit path may skip the clear") is a
+ * structural property of the function. The guard fails if someone reintroduces
+ * a happy-path-only clear or drops the finally.
+ */
+import test from "node:test";
+import assert from "node:assert/strict";
+import { readFileSync } from "node:fs";
+import { fileURLToPath } from "node:url";
+import { dirname, resolve } from "node:path";
+
+const here = dirname(fileURLToPath(import.meta.url));
+const comboSrc = readFileSync(resolve(here, "../../open-sse/services/combo.ts"), "utf8");
+
+test("#11804: the loop-safety timer is released in a finally, not only on success", () => {
+  assert.match(
+    comboSrc,
+    /finally\s*\{[^}]*clearTimeout\(activeLoopSafetyTimer\)/s,
+    "dispatchWithCooldownRetry must clear the loop-safety timer in a finally block so every " +
+      "exit path (including future ones) releases it"
+  );
+});
+
+test("#11804: the timer handle is reachable from the function-scope cleanup", () => {
+  // The timer is created inside the `for (setTry...)` loop; the cleanup lives at
+  // function scope. If the handle is not published to that outer binding, the
+  // finally silently clears nothing.
+  assert.match(
+    comboSrc,
+    /activeLoopSafetyTimer = loopSafetyTimer/,
+    "the per-iteration timer must be published to the function-scope handle"
+  );
+  const declIdx = comboSrc.indexOf("let activeLoopSafetyTimer");
+  const loopIdx = comboSrc.indexOf("for (let setTry = 0");
+  assert.ok(declIdx > 0, "function-scope timer handle must be declared");
+  assert.ok(
+    declIdx < loopIdx,
+    "the handle must be declared OUTSIDE the setTry loop, otherwise each iteration " +
+      "gets a fresh binding and the previous iteration's timer leaks"
+  );
+});
+
+test("#11804: the safety timeout itself is preserved (fix must not disarm the 504)", () => {
+  // Guard against 'fixing' the leak by simply never arming the timer.
+  assert.match(
+    comboSrc,
+    /loopSafetyTimer = setTimeout\(/,
+    "the loop-safety timer must still be armed — the 504 backstop is the reason it exists"
+  );
+  assert.match(
+    comboSrc,
+    /Combo loop safety timeout/,
+    "the force-termination path must still exist"
+  );
+});