fix(combo): pre-skip targets with persisted connection cooldown and re-check on retry by sprintberlin · Pull Request #11360 · diegosouzapw/OmniRoute

sprintberlin · 2026-08-24T06:56:22Z

Summary

Pre-skips combo targets whose connections already carry a persisted cooldown or blocking status in SQLite, and performs a fresh DB read before transient retries so that newly persisted cooldowns from sibling requests are respected immediately.

Root Cause

handleComboChat checked circuit breakers, global provider cooldowns, and semaphores before dispatch, but did not check the persisted connection rate_limited_until or unavailable status. The cooldown was only detected during credential lookup after an attempt had already begun. Furthermore, during transient retries, the 5-second connection cache could serve stale "active" data even after a 429 had updated the DB.

Implementation

Added getPersistedConnectionCooldownSkipReason and resolvePersistedConnectionCooldownSkipReason to open-sse/services/combo/comboPredicates.ts.
Integrated pre-dispatch persisted cooldown checks in open-sse/services/combo.ts.
Added a fresh (uncached) read before retry attempts (retry > 0) to immediately recognize cooldowns written mid-flight by sibling requests.
Preserved allowRateLimitedConnection bypass.

Tests & Verification

node --import tsx/esm --test tests/unit/repro-combo-persisted-cooldown-preskip.test.ts (13/13 pass)
git diff --check (pass)

…e-check on retry

…#11355) Merged via consolidated batch validation (fix applied for a cross-PR interaction with #11360, both boarded in the same batch — see combo.ts reconciliation commit). Startup crash recovery cleared every non-terminal transient cooldown unconditionally, erasing legitimate multi-day weekly quota cooldowns on restart. Now only clears expired/unparseable ones. Own repro tests pass.

…turn shape The retry-loop recheck returned a non-conforming {ok:false, reason} object that breaks typecheck against the established {ok, response?} contract used everywhere else in this function. Aligns with the pre-dispatch skip pattern (return null after fallbackCount++), matching the PR's own intent: skip this target and move to the next, not error the whole attempt. This is a live fix — the broken shape reached origin/release/v3.8.50 via #11360's own squash-merge and was breaking typecheck:core until now.

…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).

…diegosouzapw#11355) Merged via consolidated batch validation (fix applied for a cross-PR interaction with diegosouzapw#11360, both boarded in the same batch — see combo.ts reconciliation commit). Startup crash recovery cleared every non-terminal transient cooldown unconditionally, erasing legitimate multi-day weekly quota cooldowns on restart. Now only clears expired/unparseable ones. Own repro tests pass.

…e-check on retry (diegosouzapw#11360) Merged via consolidated batch validation, with one fix applied during batch validation: the retry-loop persisted-cooldown recheck returned a non-conforming {ok:false, reason} shape that failed typecheck against the established {ok, response?} contract — aligned it with the pre-dispatch skip pattern (return null after fallbackCount++), matching this PR's own intent (skip the target, don't error the whole attempt). Pre-skips combo targets with a persisted connection cooldown and re-checks fresh before transient retries. Own regression suite (13/13, including the fixed retry-recheck path) passes.

…n recheck return shape The retry-loop recheck returned a non-conforming {ok:false, reason} object that breaks typecheck against the established {ok, response?} contract used everywhere else in this function. Aligns with the pre-dispatch skip pattern (return null after fallbackCount++), matching the PR's own intent: skip this target and move to the next, not error the whole attempt. This is a live fix — the broken shape reached origin/release/v3.8.50 via diegosouzapw#11360's own squash-merge and was breaking typecheck:core until now.

…cannot dark a pool (diegosouzapw#12168) (diegosouzapw#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 diegosouzapw#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 diegosouzapw#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).

fix(combo): pre-skip targets with persisted connection cooldown and r…

dc348c4

…e-check on retry

sprintberlin requested a review from diegosouzapw as a code owner August 24, 2026 06:56

diegosouzapw merged commit 378eff0 into diegosouzapw:release/v3.8.50 Aug 24, 2026
3 checks passed

This was referenced Sep 1, 2026

[BUG] Service temporarily unavailable: all targets were skipped by pre-dispatch filters #12168

Closed

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

Merged

Navigation Menu

Uh oh!

fix(combo): pre-skip targets with persisted connection cooldown and re-check on retry - #11360

fix(combo): pre-skip targets with persisted connection cooldown and re-check on retry#11360
diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.50from
sprintberlin:fix/combo-persisted-cooldown-preskip

sprintberlin commented Aug 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

sprintberlin commented Aug 24, 2026

Summary

Root Cause

Implementation

Tests & Verification

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants