fix(combo): bound the pre-dispatch unavailable skip so a stale label cannot dark a pool (#12168) (#12285)

getPersistedConnectionCooldownSkipReason() returned a skip for ANY connection
whose testStatus was `unavailable`, with no elapsed-cooldown check:

    if (status === "unavailable") return `Skipping ...`;

That is the raw-label anti-pattern AGENTS.md warns about ("check whether code
is reading raw state instead of using getStatus()/canExecute()") — the
resilience layers are meant to recover lazily. The sibling helper directly
above it, getConnectionStatusQuotaCutoffReason(), does require
hasFutureRateLimitUntil() before treating `unavailable` as blocking.

Its stated justification — "Lazy recovery is unaffected: clearAccountError()
resets the status on first success" — does not hold on this path. This gate
runs BEFORE dispatch, so it prevents the very successful request that would
call clearAccountError(). And a row whose rateLimitedUntil is absent cannot be
rescued by the out-of-band recovery job either, because hasElapsedCooldown()
there requires a timestamp to be present.

Net effect reported in #12168: an entire combo pool answering
ALL_TARGETS_SKIPPED with recordedAttempts === 0 — zero upstream attempts, no
path back to healthy.

The original intent (do not burst into a connection AUTH just retired, before
the timestamp lands) is preserved, but bounded: the bare label is honoured only
while lastErrorAt is inside a grace window, mirroring ERROR_LABEL_GRACE_MS in
src/lib/quota/connectionRecovery.ts so the two never disagree about whether a
label is still meaningful. Past the window the request goes through, and one
real attempt either succeeds (clearing the status) or re-arms the cooldown with
a fresh timestamp.

Regression introduced by #11360, shipped in v3.8.50.

Two assertions in repro-combo-persisted-cooldown-preskip.test.ts encoded the
buggy behavior as intended ("skips an unavailable connection whose cooldown
already expired") and are realigned to the corrected contract, plus a case for
the orphan state (unavailable with no timestamps at all).

open-sse/services/combo/comboPredicates.ts

@@ -472,6 +472,30 @@ export function hasFutureRateLimitUntil(value: unknown): boolean {
   return Number.isFinite(time) && time > Date.now();
 }
 
+/**
+ * #12168: mirrors ERROR_LABEL_GRACE_MS in src/lib/quota/connectionRecovery.ts — a
+ * bare status label with no cooldown timestamp is only trusted while the failure
+ * that wrote it is recent. Kept in sync with that constant deliberately: both
+ * answer the same question ("is this label still meaningful?") and they must not
+ * disagree, or a connection the recovery job considers healthy would still be
+ * pre-skipped by combo dispatch.
+ */
+const UNAVAILABLE_LABEL_GRACE_MS = 60 * 1000;
+
+/**
+ * True when a bare `unavailable` label should still be honoured: the recorded
+ * failure is inside the grace window. A missing/unparseable lastErrorAt is
+ * treated as stale (not blocking) — an unbounded skip is exactly the failure
+ * mode #12168 reported, and one extra upstream attempt is far cheaper than a
+ * permanently dark connection pool.
+ */
+export function isWithinUnavailableGrace(lastErrorAt: unknown): boolean {
+  if (lastErrorAt == null || lastErrorAt === "") return false;
+  const time = new Date(String(lastErrorAt)).getTime();
+  if (!Number.isFinite(time)) return false;
+  return Date.now() - time < UNAVAILABLE_LABEL_GRACE_MS;
+}
+
 export function getConnectionStatusQuotaCutoffReason(
   connection: Record<string, unknown> | undefined
 ): string | undefined {
@@ -508,13 +532,28 @@ export function getPersistedConnectionCooldownSkipReason(
   if (QUOTA_BLOCKING_CONNECTION_STATUSES.has(status)) {
     return `Skipping ${target.modelStr} — connection ${target.connectionId} status=${status}`;
   }
-  // `unavailable` with no (or an already-expired) rateLimitedUntil still means AUTH
-  // took this connection out of rotation — markAccountUnavailable() writes the status
-  // before, and sometimes without, a timestamp ("Using zai account …" then a real
-  // upstream 429). Without this branch the pre-skip only fired once the timestamp had
-  // landed, so a burst still dispatched against a connection AUTH had already retired.
-  // Lazy recovery is unaffected: clearAccountError() resets the status on first success.
-  if (status === "unavailable") {
+  // `unavailable` with no rateLimitedUntil still means AUTH took this connection out
+  // of rotation — markAccountUnavailable() writes the status before, and sometimes
+  // without, a timestamp ("Using zai account …" then a real upstream 429). Without
+  // this branch a burst still dispatched against a connection AUTH had already retired.
+  //
+  // #12168: but the skip must be BOUNDED. The original version returned here for any
+  // `unavailable` row, which is the raw-label anti-pattern AGENTS.md warns about — the
+  // resilience layers are supposed to recover lazily. Its stated justification
+  // ("clearAccountError() resets the status on first success") does not hold on this
+  // path: this gate runs BEFORE dispatch, so it prevents the very successful request
+  // that would call clearAccountError(). A connection left with a stale `unavailable`
+  // label and no timestamp could therefore never dispatch again, and the out-of-band
+  // recovery job cannot rescue it either — hasElapsedCooldown() there requires a
+  // rateLimitedUntil to be present. Result: a whole combo pool could report
+  // ALL_TARGETS_SKIPPED with zero upstream attempts, forever.
+  //
+  // Bound it the same way src/lib/quota/connectionRecovery.ts bounds a bare error
+  // label: honour the skip only while the failure is recent (lastErrorAt within the
+  // grace window). Past that, treat the label as stale and let the request through —
+  // one real attempt then either succeeds (clearing the status) or re-arms the
+  // cooldown with a fresh timestamp.
+  if (status === "unavailable" && isWithinUnavailableGrace(connection.lastErrorAt)) {
     return `Skipping ${target.modelStr} — connection ${target.connectionId} status=unavailable`;
   }
   return null;

tests/unit/repro-combo-persisted-cooldown-preskip.test.ts

@@ -64,24 +64,45 @@ describe("combo persisted-cooldown pre-skip", () => {
     );
   });
 
-  it("skips an unavailable connection that has no cooldown timestamp yet", () => {
+  it("skips a RECENT unavailable connection that has no cooldown timestamp yet", () => {
     // AUTH's markAccountUnavailable() writes testStatus before (and sometimes
     // without) rate_limited_until — a burst must not dispatch into that window.
+    // #12168: the skip is now bounded by lastErrorAt, so the failure must be
+    // recent for the bare label to still block.
     const reason = getPersistedConnectionCooldownSkipReason(TARGET, {
       testStatus: "unavailable",
       rateLimitedUntil: null,
+      lastErrorAt: new Date().toISOString(),
     });
     assert.ok(reason);
     assert.match(reason!, /status=unavailable/);
   });
 
-  it("skips an unavailable connection whose cooldown already expired", () => {
+  it("#12168: does NOT skip a stale unavailable label once the grace window passed", () => {
+    // This inverts the pre-#12168 assertion, which locked in the bug: an
+    // `unavailable` row whose failure is old (and whose cooldown, if any, has
+    // expired) was skipped unconditionally. Because this gate runs BEFORE
+    // dispatch, that prevented the very success that would call
+    // clearAccountError() — and the recovery job cannot rescue a row with no
+    // rateLimitedUntil either. A whole pool could stay dark forever.
     const reason = getPersistedConnectionCooldownSkipReason(TARGET, {
       testStatus: "unavailable",
       rateLimitedUntil: new Date(Date.now() - 60_000).toISOString(),
+      lastErrorAt: new Date(Date.now() - 10 * 60_000).toISOString(),
     });
-    assert.ok(reason);
-    assert.match(reason!, /status=unavailable/);
+    assert.equal(reason, null);
+  });
+
+  it("#12168: does NOT skip an unavailable row with no timestamps at all", () => {
+    // The orphan state: testStatus left as `unavailable` while rateLimitedUntil
+    // was nulled. Unbounded skipping here is what produced ALL_TARGETS_SKIPPED
+    // with zero upstream attempts.
+    const reason = getPersistedConnectionCooldownSkipReason(TARGET, {
+      testStatus: "unavailable",
+      rateLimitedUntil: null,
+      lastErrorAt: null,
+    });
+    assert.equal(reason, null);
   });
 
   it("does not skip an expired cooldown on an otherwise healthy connection", () => {