diff --git a/open-sse/services/combo/comboPredicates.ts b/open-sse/services/combo/comboPredicates.ts index 3ad18343b3..2c6acb099e 100644 --- a/open-sse/services/combo/comboPredicates.ts +++ b/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 | 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; diff --git a/tests/unit/repro-combo-persisted-cooldown-preskip.test.ts b/tests/unit/repro-combo-persisted-cooldown-preskip.test.ts index 412f06f12c..c869a42285 100644 --- a/tests/unit/repro-combo-persisted-cooldown-preskip.test.ts +++ b/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", () => {