Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a centralized resilience framework, introducing unified settings for request pacing, connection cooldowns, and provider-level circuit breakers while removing redundant target-local breakers. Feedback points out regressions in the fallback logic where authentication and quota errors now result in zero-second cooldowns, which could lead to immediate retries of invalid accounts. Additionally, the reviewer flagged the fragility of using regex matching on error strings for routing decisions and suggested refining numeric input handling in the settings UI to prevent clearing a field from defaulting to zero and triggering validation errors.
| if (status === HTTP_STATUS.UNAUTHORIZED) { | ||
| return { | ||
| shouldFallback: true, | ||
| cooldownMs: COOLDOWN_MS.unauthorized, | ||
| cooldownMs: 0, | ||
| baseCooldownMs: 0, | ||
| reason: RateLimitReason.AUTH_ERROR, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Setting the cooldown to 0 for UNAUTHORIZED errors is a regression. This will cause accounts with invalid or expired credentials to be retried immediately in the next request cycle (unless auto-disable is active), leading to unnecessary upstream pressure and log noise. It is recommended to restore a significant cooldown period for authentication failures.
| if (status === HTTP_STATUS.UNAUTHORIZED) { | |
| return { | |
| shouldFallback: true, | |
| cooldownMs: COOLDOWN_MS.unauthorized, | |
| cooldownMs: 0, | |
| baseCooldownMs: 0, | |
| reason: RateLimitReason.AUTH_ERROR, | |
| }; | |
| } | |
| if (status === HTTP_STATUS.UNAUTHORIZED) { | |
| return { | |
| shouldFallback: true, | |
| cooldownMs: COOLDOWN_MS.unauthorized, | |
| baseCooldownMs: COOLDOWN_MS.unauthorized, | |
| reason: RateLimitReason.AUTH_ERROR, | |
| }; | |
| } |
| if (status === HTTP_STATUS.PAYMENT_REQUIRED || status === HTTP_STATUS.FORBIDDEN) { | ||
| return { | ||
| shouldFallback: true, | ||
| cooldownMs: COOLDOWN_MS.paymentRequired, | ||
| cooldownMs: 0, | ||
| baseCooldownMs: 0, | ||
| reason: RateLimitReason.QUOTA_EXHAUSTED, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Similar to the UNAUTHORIZED case, setting the cooldown to 0 for PAYMENT_REQUIRED or FORBIDDEN errors will result in immediate retries of exhausted or restricted accounts. A non-zero cooldown should be maintained to prevent spinning on accounts that are known to be unusable in the short term.
| if (status === HTTP_STATUS.PAYMENT_REQUIRED || status === HTTP_STATUS.FORBIDDEN) { | |
| return { | |
| shouldFallback: true, | |
| cooldownMs: COOLDOWN_MS.paymentRequired, | |
| cooldownMs: 0, | |
| baseCooldownMs: 0, | |
| reason: RateLimitReason.QUOTA_EXHAUSTED, | |
| }; | |
| } | |
| if (status === HTTP_STATUS.PAYMENT_REQUIRED || status === HTTP_STATUS.FORBIDDEN) { | |
| return { | |
| shouldFallback: true, | |
| cooldownMs: COOLDOWN_MS.paymentRequired, | |
| baseCooldownMs: COOLDOWN_MS.paymentRequired, | |
| reason: RateLimitReason.QUOTA_EXHAUSTED, | |
| }; | |
| } |
| result.headers?.get("content-type") ?? null, | ||
| errorText | ||
| ); | ||
| if (/circuit breaker is open/i.test(errorText)) { |
There was a problem hiding this comment.
Relying on a regex match against a human-readable error string (/circuit breaker is open/i) to control routing logic is fragile. If the error message returned by the provider gate changes in the future, this skip logic will break. Consider using a structured error response or a specific header to signal circuit breaker state to the combo engine.
| type="number" | ||
| min={min} | ||
| value={value} | ||
| onChange={(event) => onChange(Number(event.target.value) || 0)} |
There was a problem hiding this comment.
The || 0 fallback in the onChange handler causes the field to default to 0 whenever the input is cleared. For fields with a minimum value of 1 (like requestsPerMinute), this will cause immediate validation failures upon saving if the user clears the field to type a new value. It would be better to allow the state to hold an empty string or null and handle the conversion/validation during submission or on blur.
rdself
changed the title
There was a problem hiding this comment.
Pull request overview
Rebuilds and consolidates OmniRoute’s resilience controls on top of release/v3.7.0, centering behavior around request queueing, connection cooldowns, a shared provider circuit breaker, and cooldown-aware retry, while restoring 3.7.0 UI/API compatibility.
Changes:
- Introduces unified
resilienceSettings(requestQueue / connectionCooldown / providerBreaker / waitForCooldown) with API + schema support and legacy-compat translation. - Removes combo-local breaker behavior and aligns combo routing to respect the shared provider breaker, with updated monitoring/health payloads.
- Adds/updates unit + integration coverage, including a black-box HTTP resilience e2e test harness.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/combo-routing-engine.test.ts | Updates combo routing expectations to skip provider-breaker-open targets. |
| tests/unit/combo-context-relay.test.ts | Adjusts context-relay behavior to fall through provider breaker open responses. |
| tests/unit/combo-config.test.ts | Updates combo config defaults/legacy key handling assertions. |
| tests/unit/combo-circuit-breaker.test.ts | Removes tests for deprecated combo-local circuit breaker layer. |
| tests/unit/chat-route-coverage.test.ts | Updates chat route coverage for connection cooldown + provider breaker responses. |
| tests/unit/chat-helpers.test.ts | Updates helper tests for new breaker profile keys and proxy fast-fail handling. |
| tests/unit/chat-combo-live-test.test.ts | Adjusts live test expectations for connection cooldown vs breaker state. |
| tests/unit/autocombo-unification.test.ts | Updates intelligent health extraction contract (providerBreakers). |
| tests/unit/account-fallback-service.test.ts | Updates provider profile assertions for new/legacy field mapping. |
| tests/integration/resilience-http-e2e.test.ts | Adds black-box HTTP e2e coverage for resilience layers. |
| src/types/settings.ts | Adds resilienceSettings to Settings type; trims legacy combo defaults fields. |
| src/sse/services/cooldownAwareRetry.ts | Rewires cooldown-aware retry settings to waitForCooldown. |
| src/sse/services/auth.ts | Refactors fallback/cooldown handling and connection persistence updates. |
| src/sse/handlers/chatHelpers.ts | Updates pipeline gate breaker config + removes breaker-execute wrapping from core call. |
| src/sse/handlers/chat.ts | Aligns chat flow with provider breaker settings + new cooldown-aware retry shape. |
| src/shared/validation/schemas.ts | Adds new resilience schemas, renames legacy schemas, and enforces strict update payloads. |
| src/shared/utils/circuitBreaker.ts | Exposes retryAfterMs in breaker status snapshots. |
| src/lib/resilience/settings.ts | Introduces normalized resilience settings + legacy compat mapping/merge helpers. |
| src/lib/monitoring/observability.ts | Adds providerBreakers to health payload and enriches breaker fields. |
| src/lib/dataPaths.js | Removes stale tracked artifact that caused dev import resolution issues. |
| src/lib/combos/intelligentRouting.ts | Reads breaker state from providerBreakers instead of circuitBreakers. |
| src/app/api/settings/combo-defaults/route.ts | Sanitizes combo default/override configs to drop legacy resilience keys. |
| src/app/api/resilience/route.ts | Rebuilds resilience API around new settings model with legacy patch compatibility. |
| src/app/api/resilience/reset/route.ts | Minor cleanup in reset endpoint. |
| src/app/api/policies/route.ts | Removes breaker state from policies API response (locked identifiers only). |
| src/app/(dashboard)/dashboard/settings/components/ResilienceTab.tsx | Rebuilds UI to edit the four resilience layers (config-only). |
| src/app/(dashboard)/dashboard/settings/components/PoliciesPanel.tsx | Updates UI to show only locked identifiers (no breaker list). |
| src/app/(dashboard)/dashboard/settings/components/ComboDefaultsTab.tsx | Sanitizes legacy combo resilience keys from defaults/overrides. |
| src/app/(dashboard)/dashboard/providers/components/ModelAvailabilityPanel.tsx | Minor UI cleanup (comment removal). |
| src/app/(dashboard)/dashboard/providers/components/ModelAvailabilityBadge.tsx | Minor UI cleanup (comment removal). |
| src/app/(dashboard)/dashboard/health/page.tsx | Displays breaker retryAfterMs in UI. |
| src/app/(dashboard)/dashboard/combos/page.tsx | Sanitizes legacy combo resilience keys and removes legacy fields from templates/UI. |
| open-sse/services/rateLimitManager.ts | Reworks queueing defaults to derive from requestQueue settings + runtime apply hook. |
| open-sse/services/comboConfig.ts | Sanitizes legacy combo resilience keys during config resolution. |
| open-sse/services/combo.ts | Removes combo-local breaker logic; skips targets on provider breaker open responses. |
| open-sse/services/accountFallback.ts | Rebuilds provider profile resolution off resilience settings and refactors fallback cooldown logic. |
| open-sse/services/AGENTS.md | Updates docs to reflect provider-global breaker model. |
| README.md | Updates resilience feature documentation to match new architecture. |
| Object.entries(config).filter(([, value]) => value !== undefined && value !== null) | ||
| ).reduce( | ||
| (sanitized, [key, value]) => { | ||
| if (!LEGACY_COMBO_RESILIENCE_KEYS.has(key)) { | ||
| sanitized[key] = value; | ||
| } | ||
| return sanitized; | ||
| }, | ||
| {} as Record<string, any> |
There was a problem hiding this comment.
sanitizeComboRuntimeConfig() calls .reduce() on the object returned by Object.fromEntries(...). That will throw at runtime ("reduce is not a function") and break GET/PATCH /api/settings/combo-defaults. Refactor this to reduce over the filtered entries array (or just filter out legacy keys and use Object.fromEntries once).
| Object.entries(config).filter(([, value]) => value !== undefined && value !== null) | |
| ).reduce( | |
| (sanitized, [key, value]) => { | |
| if (!LEGACY_COMBO_RESILIENCE_KEYS.has(key)) { | |
| sanitized[key] = value; | |
| } | |
| return sanitized; | |
| }, | |
| {} as Record<string, any> | |
| Object.entries(config).filter( | |
| ([key, value]) => | |
| value !== undefined && | |
| value !== null && | |
| !LEGACY_COMBO_RESILIENCE_KEYS.has(key) | |
| ) |
| minTime: currentRequestQueueSettings.minTimeBetweenRequestsMs, | ||
| reservoir: currentRequestQueueSettings.requestsPerMinute, | ||
| reservoirRefreshAmount: currentRequestQueueSettings.requestsPerMinute, | ||
| reservoirRefreshInterval: 60 * 1000, |
There was a problem hiding this comment.
applyRequestQueueSettings() updates existing Bottleneck limiters but doesn't apply requestQueue.maxWaitMs (even though buildLimiterDefaults includes maxWait). If maxWait is intended to be runtime-tunable, include it in updateSettings; otherwise consider removing it from the defaults to avoid a misleading setting.
| reservoirRefreshInterval: 60 * 1000, | |
| reservoirRefreshInterval: 60 * 1000, | |
| maxWait: currentRequestQueueSettings.maxWaitMs, |
rdself
changed the title
rdself
changed the title
rdself
changed the title
|
Superseded by #1449. Re-opened as a clean single-commit PR so the final change set is easier to review. |
Summary
This PR refactors OmniRoute's entire circuit-breaker and cooldown stack for
release/v3.7.0.Before this work, resilience behavior was spread across too many overlapping layers: connection cooldowns, provider breakers, model-availability state, combo-specific health state, and dashboard/API-specific views of runtime health. The result was hard to reason about and hard to trust: the same failure could be interpreted differently depending on where it was handled, and different surfaces could drift away from the actual runtime behavior.
The goal of this refactor is to give OmniRoute one clear resilience model with clear responsibilities at each layer, so the runtime, dashboard, MCP, API, and docs all describe the same system.
What the new model is for
The 3.7.0 resilience model is split into four layers. Each layer exists to answer a different operational question.
1.
requestQueueWhat this layer is for
This layer protects a single connection/account from being hit by overlapping local requests when that connection should process work sequentially.
What it means operationally
2.
connectionCooldownWhat this layer is for
This layer decides when one specific connection/account should temporarily step out of rotation after a retryable failure.
What it means operationally
baseCooldownMs, upstream retry hints, and bounded backoff to decide how long that connection should rest3.
providerBreakerWhat this layer is for
This layer decides when a provider as a whole is unhealthy enough that OmniRoute should stop routing traffic into it for a while.
What it means operationally
4.
waitForCooldownWhat this layer is for
This layer decides what OmniRoute should do when every eligible connection is temporarily unavailable only because of cooldowns.
What it means operationally
What combo routing is responsible for
Combo routing is now treated as a routing layer, not as a second resilience system.
That distinction is important.
A combo should answer questions like:
A combo should not maintain its own parallel understanding of provider health, cooldown state, or breaker state.
What this means operationally
In other words: combo decides where to try next; the resilience system decides whether that target is currently eligible.
Runtime behavior after the refactor
The runtime now follows one consistent flow:
That gives each decision a single home instead of spreading the same responsibility across multiple overlapping subsystems.
Account and fallback handling
This refactor also makes the fallback path easier to reason about.
Dashboard, API, and MCP behavior
The non-runtime surfaces now describe the same resilience model instead of inventing their own parallel layers.
Dashboard
API
/api/resilienceexposes the same settings shape used by the runtime and dashboardconnectionCooldownis configured with:baseCooldownMsuseUpstreamRetryHintsmaxBackoffStepsMCP
Documentation
The docs now describe the same 3.7.0 resilience model used by the implementation:
docs/API_REFERENCE.mddocs/ARCHITECTURE.mddocs/USER_GUIDE.mddocs/openapi.yamlValidation
Type and test coverage
npm run typecheck:corenode --import tsx/esm --test tests/integration/integration-wiring.test.tsnode --import tsx/esm --test tests/unit/autocombo-unification.test.tsgit pushpre-push hook:npm run test:unit(3240passing tests)Playwright coverage
Playwright coverage was run in dev-server mode and verifies the behavior of the new model across UI surfaces:
Commands used:
OMNIROUTE_PLAYWRIGHT_SERVER_MODE=dev npx playwright test tests/e2e/resilience-plan-alignment.spec.tsOMNIROUTE_PLAYWRIGHT_SERVER_MODE=dev npx playwright test tests/e2e/combo-unification.spec.tsNote
Playwright was executed in dev-server mode because the current branch still has unrelated Next build/prerender failures in start/build mode (
/_global-error,/callback). The behavior covered above was validated successfully in dev mode.