[hartmark]

Summary

I'm mostly using gemini provider and they have two models. Gemma-4 31B and Gemma-4 24B, they have a 10 RPM limit so when I get 429 on one model it should go over to the next.

I have digged through the code and with help of AI identified that it seems that combos are handling fail over to another model on the same provider.

This PR should address this. All tests and added tests are in place.

[gemini-code-assist]

Code Review

This pull request introduces advanced failover and retry configurations for model combos, including a failoverBeforeRetry option to bypass intra-URL retries and a maxSetRetries feature for retrying the entire target set. The implementation involves updates to the base executor, combo services, Zod schemas, and the dashboard UI, supported by a new system-level failover test suite. Feedback identifies a critical issue with a .ts extension in a dynamic import that may break production builds, a logic error where request metrics are reset during set-level retries, and a potential listener leak in the delay logic used for retries.

[gemini-code-assist]

The dynamic import() call includes a .ts extension. In Next.js and standard Node.js ESM resolution, extensions should generally be omitted (or use .js if required by specific configurations). Including .ts can cause module resolution failures in production builds where the source files are compiled.

Suggested change

	await import("@omniroute/open-sse/services/accountFallback.ts");
	await import("@omniroute/open-sse/services/accountFallback");

[gemini-code-assist]

The variables startTime, fallbackCount, and recordedAttempts are reset at the beginning of each set retry (inside the setTry loop). This causes metrics like total latency and cumulative fallback count to only reflect the final set attempt, which is misleading for requests that required multiple set-level retries.

Additionally, the block at line 2284 that records a failure when recordedAttempts === 0 will fire for every set attempt that finds no available models, potentially cluttering logs and metrics with redundant failure entries.

Consider moving these variables and the final failure recording logic outside the setTry loop to track the entire request lifecycle correctly.

[gemini-code-assist]

The Promise wrapper used for the delay does not remove the abort event listener if the timer completes normally. While request-scoped signals are eventually garbage collected, this pattern can lead to a build-up of listeners in high-concurrency scenarios.

Consider using a cleanup mechanism to remove the listener when the timer fires, or use AbortSignal.timeout or setTimeout from node:timers/promises if supported in your environment.

[kilo-code-bot]

WARNING: skipUpstreamRetry: isCombo disables executor-level retries for ALL combo requests, not just those with failoverBeforeRetry: true.

This means combos without failoverBeforeRetry will also skip the executor's intra-URL retry logic (BaseExecutor.RETRY_CONFIG). For example, if an upstream provider returns a transient 500, the executor won't retry that URL even if the combo config would normally allow retries.

If the combo layer is intended to fully own retry behavior, this is fine. But if non-fobr combos should still benefit from executor-level retries as a safety net, this should be:

Suggested change

	skipUpstreamRetry: isCombo,
	skipUpstreamRetry: isCombo && skipUpstreamRetry,

This way, executor retries are only skipped when both conditions are met: it's a combo request AND skipUpstreamRetry was explicitly passed as true (i.e., failoverBeforeRetry is enabled).

[kilo-code-bot]

Code Review Summary

Status: 1 Warning Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	0
WARNING	1
SUGGESTION	0

Issue Details (click to expand)

WARNING

File	Line	Issue
open-sse/handlers/chatCore.ts	3666	skipUpstreamRetry: isCombo disables executor retries for ALL combos, not just those with failoverBeforeRetry: true. Non-fobr combos lose their executor-level retry safety net.

Other Observations (not in diff)

Issues found in unchanged code that cannot receive inline comments:

File	Line	Issue
tests/e2e/system-failover.test.ts	634-649	Test "single provider, modelA 500 with fobr" — both models use the same TOKEN_A, so the mock server doesn't distinguish between them. After modelA's queued 429 is consumed, modelB hits the default 200. The test name says "modelA retry" but the assertion hits: 2 is ambiguous — it could be 1 modelA + 1 modelB. Consider using distinct tokens per model for clarity.
tests/e2e/system-failover.test.ts	586-601	Test "single provider, modelA 500" — same issue: both models share TOKEN_A. The combo routes modelA first (gets 500), then tries modelB (same token, gets default 200). The test asserts result.json.model === "sys-a/test-model" which passes because the mock always returns that model name. This means the test can't actually verify which combo target succeeded.
tests/e2e/system-failover.test.ts	610-625	Test "single provider, modelA 503 with fobr" — same shared token issue. The assertion hits: 2 could mean either 2 retries on modelA, or 1 hit on modelA + 1 on modelB. Using distinct tokens would make the intent verifiable.

Files Reviewed (17 files)

• open-sse/executors/base.ts — skipUpstreamRetry parameter added correctly
• open-sse/handlers/chatCore.ts — 1 warning (skipUpstreamRetry: isCombo)
• open-sse/services/accountFallback.ts — clearAllModelLockouts() helper added
• open-sse/services/combo.ts — major refactor: set-level retry loop, restructured error handling
• open-sse/services/comboConfig.ts — defaults for new fields
• package.json — test:system script added
• src/app/(dashboard)/dashboard/combos/page.tsx — UI for new config fields
• src/app/api/resilience/reset/route.ts — calls clearAllModelLockouts()
• src/i18n/messages/en.json — i18n strings for new fields
• src/shared/validation/schemas.ts — Zod schemas for new fields
• src/sse/handlers/chat.ts — failoverBeforeRouting propagated through handler chain
• src/sse/handlers/chatHelpers.ts — skipUpstreamRetry passthrough
• src/sse/services/auth.ts — lockout reason extended to status >= 500
• tests/e2e/combos-flow.spec.ts — UI test for new fields
• tests/e2e/helpers/mockUpstreamServer.ts — new test helper (175 lines)
• tests/e2e/system-failover.test.ts — new e2e test file (699 lines)
• tests/unit/combo-config.test.ts — unit tests for new schema fields

---

_{Reviewed by qwen3.6-plus · 759,384 tokens}

[diegosouzapw]

LGTM — falloverBeforeRetry correctly solves per-model quota skipping within a combo without touching the provider-level circuit breaker. Tests in place.

[diegosouzapw]

🎉 Thank you @hartmark for the falloverBeforeRetry feature! This cleanly solves the per-model Gemini RPM quota issue by having combos skip to the next target instead of retrying the same rate-limited model. The feature has been integrated into the v3.8.0 release and will be available in the next build. 🚀

[diegosouzapw]

Thank you @hartmark for this combo routing improvement! The falloverBeforeRetry feature is now part of OmniRoute v3.8.0. This elegantly solves the tail-latency problem where a failing endpoint would exhaust all per-model retries before moving to the next target — now combo routing falls over immediately. The setTry outer loop structure was a clean approach to coordinate this.

[hartmark]

Thank you @hartmark for this combo routing improvement! The falloverBeforeRetry feature is now part of OmniRoute v3.8.0. This elegantly solves the tail-latency problem where a failing endpoint would exhaust all per-model retries before moving to the next target — now combo routing falls over immediately. The setTry outer loop structure was a clean approach to coordinate this.

Thanks mate. I have some batchjob fixes coming up in the next few days. I've noticed some issues