fix(gateway): respect RPM limits in low-uptime fallback - #2042
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds exported helper pickNonRateLimitedCandidates; uses it in chat routing (requested-provider reroute and low-uptime fallback) to filter out rate-limited provider/model candidates (with fail-open), and adds tests covering filtering, fail-open, deduping, and empty-input behavior. ChangesRate-Limit Candidate Selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Aligns the gateway’s low-uptime fallback routing with the existing rate-limit fallback routing by avoiding (when possible) providers already at their configured RPM/RPD caps, reducing avoidable immediate 429s after reroute.
Changes:
- Filters low-uptime fallback alternatives via
filterRateLimitedProviders, with fail-open behavior when all alternatives are capped. - Routes scoring/metrics collection using the resulting
uptimeFallbackCandidatesinstead of the fullavailableModelProviders. - Keeps behavior consistent with the pre-existing rate-limit fallback candidate filtering logic in the same file.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const rateLimitedAlternatives = await filterRateLimitedProviders( | ||
| project.organizationId, | ||
| availableModelProviders.map((p) => ({ | ||
| providerId: p.providerId, | ||
| model: baseModelId, | ||
| providerModelName: p.modelName, | ||
| })), |
There was a problem hiding this comment.
filterRateLimitedProviders is called once per entry in availableModelProviders, which likely includes multiple regions per provider (it’s later collapsed via collapseProvidersToBestRegionPerProvider). This can trigger redundant rate-limit peeks (DB + Redis) for the same provider/model, increasing latency in the low-uptime fallback path. Consider de-duplicating the candidates (e.g., by providerId + providerModelName) before calling filterRateLimitedProviders, while still filtering the full availableModelProviders list using the resulting Set.
| const rateLimitedAlternatives = await filterRateLimitedProviders( | |
| project.organizationId, | |
| availableModelProviders.map((p) => ({ | |
| providerId: p.providerId, | |
| model: baseModelId, | |
| providerModelName: p.modelName, | |
| })), | |
| const uniqueRateLimitCandidates = Array.from( | |
| new Map( | |
| availableModelProviders.map((p) => [ | |
| `${p.providerId}:${p.modelName}`, | |
| { | |
| providerId: p.providerId, | |
| model: baseModelId, | |
| providerModelName: p.modelName, | |
| }, | |
| ]), | |
| ).values(), | |
| ); | |
| const rateLimitedAlternatives = await filterRateLimitedProviders( | |
| project.organizationId, | |
| uniqueRateLimitCandidates, |
| // Exclude alternatives that are already at their RPM/RPD cap so the | ||
| // low-uptime fallback doesn't route into a rate-limited provider. | ||
| // Fail-open: if all alternatives are rate-limited, keep them all. | ||
| const rateLimitedAlternatives = await filterRateLimitedProviders( | ||
| project.organizationId, | ||
| availableModelProviders.map((p) => ({ | ||
| providerId: p.providerId, | ||
| model: baseModelId, | ||
| providerModelName: p.modelName, | ||
| })), | ||
| ); | ||
| const nonRateLimitedAlternatives = availableModelProviders.filter( | ||
| (p) => !rateLimitedAlternatives.has(p.providerId), | ||
| ); | ||
| const uptimeFallbackCandidates = | ||
| nonRateLimitedAlternatives.length > 0 | ||
| ? nonRateLimitedAlternatives | ||
| : availableModelProviders; |
There was a problem hiding this comment.
This change introduces new routing behavior (skipping rate-limited alternatives during low-uptime fallback) but there doesn’t appear to be an integration test covering the scenario where the cheapest/healthiest alternative is at its provider RPM/RPD cap and the router should pick the next-best candidate (and also the fail-open case when all alternatives are capped). Please add a regression test (likely alongside existing low-uptime fallback tests in apps/gateway/src/fallback.spec.ts) to prevent this from regressing.
| // Exclude alternatives that are already at their RPM/RPD cap so the | ||
| // low-uptime fallback doesn't route into a rate-limited provider. | ||
| // Fail-open: if all alternatives are rate-limited, keep them all. | ||
| const rateLimitedAlternatives = await filterRateLimitedProviders( | ||
| project.organizationId, | ||
| availableModelProviders.map((p) => ({ | ||
| providerId: p.providerId, | ||
| model: baseModelId, | ||
| providerModelName: p.modelName, | ||
| })), | ||
| ); | ||
| const nonRateLimitedAlternatives = availableModelProviders.filter( | ||
| (p) => !rateLimitedAlternatives.has(p.providerId), | ||
| ); | ||
| const uptimeFallbackCandidates = | ||
| nonRateLimitedAlternatives.length > 0 | ||
| ? nonRateLimitedAlternatives | ||
| : availableModelProviders; |
There was a problem hiding this comment.
This rate-limit-aware candidate filtering duplicates the earlier rate-limit fallback block (same file around the rate-limit-fallback routing). Consider extracting a small helper for “pick fallback candidates with fail-open rate-limit filtering” to avoid the two paths drifting over time (and to centralize any future tweaks to the filtering rules).
The low-uptime fallback path selected an alternative provider purely on uptime and price, ignoring per-provider RPM/RPD caps. Filter out rate-limited alternatives (fail-open if all are capped), matching the rate-limit fallback behavior. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
availableModelProviders comes from the region-expanded list, so multiple variants of the same provider+model triggered redundant peekProviderRateLimit calls (Redis hit per region). Dedupe by providerId+modelName before calling filterRateLimitedProviders. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
b4e6f99 to
8002624
Compare
Both rate-limit and low-uptime fallback paths reused the same dedupe → peek → filter → fail-open dance. Pull it into a single helper next to filterRateLimitedProviders so future tweaks to the filtering rules apply to both paths at once. Add unit tests covering the next-best, fail-open, and region-dedupe cases. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Summary
apps/gateway/src/chat/chat.tsselected an alternative provider based only on uptime and price, ignoring per-provider RPM/RPD caps. A request that was rerouted away from a low-uptime provider could land on another provider that was already at its rate limit, causing an avoidable 429 atcheckProviderRateLimitright after.availableModelProvidersthroughfilterRateLimitedProvidersbefore scoring, with fail-open if every alternative is capped. This mirrors the existing rate-limit fallback path (same file, lines ~2056–2071), so the two fallback paths now share the same RPM-aware candidate selection.Test plan
pnpm build— passed locallypnpm lint— passed locally🤖 Generated with Claude Code
Summary by CodeRabbit