fix(combo): make failoverBeforeRetry actually skip the same-model retry (#10217)

* fix(deps): bump nanoid, dompurify for 2 new Dependabot alerts (#189, #190)

Bumps: nanoid ^3.3.17 (was transitive, now overridden), dompurify ^3.4.13
(with monaco-editor scoped override). Closes Dependabot #189, #190.

Remaining #182-#188 (js-yaml + mermaid) already closed by #9651 merge —
awaiting Dependabot re-scan.

npm audit → 0 vulnerabilities.

* fix(repo): harden .gitignore to also ignore a _tasks symlink (/_tasks)

_tasks is a SEPARATE nested git repo (gitignored). The pattern _tasks/ (trailing
slash) ignores only a directory, not a SYMLINK named _tasks. A self-referential
_tasks symlink can slip in via git add -A and, once pulled, checkout materializes
it over the real _tasks repo (destroying plans/specs/hands-off). Anchored /_tasks
ignores the symlink too, preventing re-capture.

* fix(combo): make failoverBeforeRetry actually skip the same-model retry

Both same-target retry loops (priority/auto and round-robin) checked
isTransient/maxRetries/providerExhausted but never consulted
config.failoverBeforeRetry, so a rate-limited model still got
maxRetries+1 back-to-back attempts on itself before falling back to a
sibling — the config option (#2417) was only ever wired into
skipUpstreamRetry, a separate lower-level mechanism. Now the same-model
retry is skipped when failoverBeforeRetry is set AND a sibling target
is actually available; with no sibling left, it still retries same-model
since skipping would just burn the last attempt for nothing.

---------

Co-authored-by: diegosouzapw <diegosouzapw@users.noreply.github.com>
Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@outlook.com>
Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

open-sse/services/combo.ts

@@ -1908,7 +1908,23 @@ export async function handleComboChat({
             !isTokenLimitBreach &&
             !scopedFailure &&
             [408, 429, 500, 502, 503, 504].includes(result.status);
-          if (retry < maxRetries && isTransient && !providerExhausted) {
+          // failoverBeforeRetry means what it says: prefer the next sibling
+          // target over hammering this one again. Without this check, a
+          // transient error always re-hit the SAME model up to maxRetries
+          // times regardless of the setting — config.failoverBeforeRetry was
+          // threaded through to skipUpstreamRetry (a different, lower-level
+          // retry mechanism) but never consulted here, so a rate-limited
+          // model got maxRetries+1 back-to-back attempts on itself before
+          // this loop's own fallback-to-next-target ever ran (#2417). Only
+          // skip the same-model retry when `nextTarget` (computed above)
+          // actually gives us somewhere to fail over to — with no sibling
+          // left, skipping just burns the last attempt for nothing.
+          if (
+            retry < maxRetries &&
+            isTransient &&
+            !providerExhausted &&
+            (!config.failoverBeforeRetry || !nextTarget)
+          ) {
             if (
               !protectedPriorityTarget &&
               provider &&
@@ -3142,7 +3158,18 @@ async function handleRoundRobinCombo({
           !isTokenLimitBreach &&
           !scopedFailure &&
           [408, 429, 500, 502, 503, 504].includes(result.status);
-        if (retry < maxRetries && isTransient && !providerExhausted) {
+        // See the same guard's comment in the "auto" strategy loop above —
+        // failoverBeforeRetry must prevent this same-model retry too, not
+        // just the lower-level skipUpstreamRetry mechanism. Only skip when
+        // `offset + 1 < modelCount` means a sibling target is actually left
+        // in this rotation; with none left, skipping just wastes the attempt.
+        const hasNextRrTarget = offset + 1 < modelCount;
+        if (
+          retry < maxRetries &&
+          isTransient &&
+          !providerExhausted &&
+          (!config.failoverBeforeRetry || !hasNextRrTarget)
+        ) {
           continue;
         }
 

tests/unit/combo-routing-engine.test.ts

@@ -2956,6 +2956,69 @@ test("handleComboChat round-robin retries a transient failure on the same model
   assert.deepEqual(calls, ["model-a", "model-a"]);
 });
 
+test("handleComboChat round-robin: failoverBeforeRetry skips the same-model retry and goes straight to the sibling", async () => {
+  // #2417's whole point: failoverBeforeRetry should prefer a sibling model
+  // over hammering a rate-limited one again. Same shape as the test above
+  // (maxRetries: 1, a transient 429 on the first call) but with a second
+  // model available and failoverBeforeRetry set — calls must show a single
+  // model-a attempt followed directly by model-b, never a same-model retry.
+  const calls = [];
+
+  const result = await handleComboChat({
+    body: {},
+    combo: {
+      name: "rr-failover-before-retry",
+      strategy: "round-robin",
+      models: ["model-a", "model-b"],
+      config: {
+        maxRetries: 1,
+        retryDelayMs: 1,
+        failoverBeforeRetry: true,
+        concurrencyPerModel: 1,
+        queueTimeoutMs: 5,
+      },
+    },
+    handleSingleModel: async (_body, modelStr) => {
+      calls.push(modelStr);
+      if (modelStr === "model-a") return errorResponse(429, "rate limited");
+      return okResponse();
+    },
+    isModelAvailable: async () => true,
+    log: createLog(),
+    settings: null,
+    relayOptions: null,
+    allCombos: null,
+  });
+
+  assert.equal(result.ok, true);
+  assert.deepEqual(calls, ["model-a", "model-b"]);
+});
+
+test("handleComboChat priority strategy: failoverBeforeRetry skips the same-model retry and goes straight to the sibling", async () => {
+  const calls = [];
+
+  const result = await handleComboChat({
+    body: {},
+    combo: {
+      name: "priority-failover-before-retry",
+      models: ["model-a", "model-b"],
+      config: { maxRetries: 1, retryDelayMs: 1, failoverBeforeRetry: true },
+    },
+    handleSingleModel: async (_body, modelStr) => {
+      calls.push(modelStr);
+      if (modelStr === "model-a") return errorResponse(429, "rate limited");
+      return okResponse();
+    },
+    isModelAvailable: async () => true,
+    log: createLog(),
+    settings: null,
+    allCombos: null,
+  });
+
+  assert.equal(result.ok, true);
+  assert.deepEqual(calls, ["model-a", "model-b"]);
+});
+
 test("handleComboChat round-robin recovers from 400s when a later model succeeds", async () => {
   const calls: any[] = [];