fix(ratelimit): honest 429 when every virtual-model target is saturated#492
Conversation
A fully rate-saturated alias previously fell through the same path as an alias with no live targets: resolution failed and the client got an unavailable-model error instead of 429 with Retry-After, and the alias even disappeared from /v1/models while throttled — because rate-limit capacity was folded into the catalog's ModelAvailable, conflating "throttled" with "dead". Capacity now steers load balancing only, through an explicit TargetCapacity probe on the virtual models service: the balancer prefers targets with live capacity and, when every live target is saturated, falls back to the first declared one so the request reaches admission and receives the honest 429 (or defers to configured failover). Catalog membership, /v1/models listing, and dashboard redirect validity are no longer affected by throttling, and targets the catalog marks unavailable stay excluded even during the fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe virtual-models balancer now filters targets by a rate-limit capacity probe before selecting a target, with fallback to the first target when all are saturated. App initialization wires the probe from the rate-limit service, and the docs and tests reflect the updated saturation behavior. ChangesCapacity-aware routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant App as app.go init
participant VM as virtualmodels.Service
participant Limiter as ratelimit.Service
participant Balancer as balancedResolution
App->>VM: New(providerResult.Registry)
App->>VM: SetTargetCapacity(func(qualifiedModel) bool)
VM->>Limiter: RouteAvailable(provider, qualifiedModel)
Limiter-->>VM: capacity result
VM->>Balancer: balancedResolution(supported targets)
Balancer->>Balancer: targetsWithCapacity(supported)
alt all targets saturated
Balancer->>Balancer: fallback to first target
else capacity available
Balancer->>Balancer: select via strategy
end
Balancer-->>VM: selected target
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/virtualmodels/balancer.go`:
- Around line 43-70: Add a test for Service.balancedResolution that covers
StrategyCost with capacity filtering: build a redirectEntry whose
supportedTargets includes a cheaper saturated target and a costlier target with
live capacity, then verify balancedResolution selects the costlier one after
targetsWithCapacity filtering. Make the test reference balancedResolution,
cheapestTarget, and targetsWithCapacity so the behavior change is locked in
alongside the existing round-robin coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c366fd01-3af5-4882-bd8a-6139a5b00fe5
📒 Files selected for processing (8)
CLAUDE.mddocs/dev/2026-07-05_rate-limiting-spec.mddocs/features/rate-limits.mdxinternal/app/app.gointernal/app/ratelimit_catalog.gointernal/virtualmodels/balancer.gointernal/virtualmodels/balancer_test.gointernal/virtualmodels/service.go
💤 Files with no reviewable changes (1)
- internal/app/ratelimit_catalog.go
| func (s *Service) balancedResolution(entry redirectEntry) (core.ModelSelector, bool) { | ||
| supported := entry.supportedTargets(s.catalog) | ||
| switch len(supported) { | ||
| case 0: | ||
| if len(supported) == 0 { | ||
| return core.ModelSelector{}, false | ||
| case 1: | ||
| // A single live target needs no strategy and must not advance round-robin | ||
| // state, so an alias and a one-target-available redirect behave identically. | ||
| return supported[0].selector, true | ||
| } | ||
| // Prefer targets with live rate-limit capacity. When every live target is | ||
| // saturated, fall back to the first declared one: the request then reaches | ||
| // admission and receives an honest 429 with Retry-After (or defers to | ||
| // failover) instead of the all-targets-down error path. | ||
| pool := s.targetsWithCapacity(supported) | ||
| if len(pool) == 0 { | ||
| pool = supported[:1] | ||
| } | ||
| if len(pool) == 1 { | ||
| // A single viable target needs no strategy and must not advance | ||
| // round-robin state, so an alias and a one-target-available redirect | ||
| // behave identically. | ||
| return pool[0].selector, true | ||
| } | ||
|
|
||
| switch normalizeStrategy(entry.strategy) { | ||
| case StrategyCost: | ||
| return s.cheapestTarget(supported).selector, true | ||
| return s.cheapestTarget(pool).selector, true | ||
| default: // StrategyRoundRobin | ||
| index := weightedIndex(supported, s.balancer.next(entry.vm.Source)) | ||
| return supported[index].selector, true | ||
| index := weightedIndex(pool, s.balancer.next(entry.vm.Source)) | ||
| return pool[index].selector, true | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Recommend a test for the Cost strategy + capacity-filtering interaction.
cheapestTarget now runs over the capacity-filtered pool rather than the full supported list — a real behavior change (a cheaper but saturated target is now skipped in favor of a costlier one with capacity). The new tests only exercise StrategyRoundRobin; there's no test verifying this for StrategyCost.
As per coding guidelines, **/*_test.go: "Add or update tests for behavior changes; tests should cover request translation, response normalization, error handling, default configuration, and provider-specific parameter mapping." Load-balancing strategy selection is a critical path worth the same rigor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/virtualmodels/balancer.go` around lines 43 - 70, Add a test for
Service.balancedResolution that covers StrategyCost with capacity filtering:
build a redirectEntry whose supportedTargets includes a cheaper saturated target
and a costlier target with live capacity, then verify balancedResolution selects
the costlier one after targetsWithCapacity filtering. Make the test reference
balancedResolution, cheapestTarget, and targetsWithCapacity so the behavior
change is locked in alongside the existing round-robin coverage.
Source: Coding guidelines
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
A cheaper but rate-saturated target loses to a costlier one with live capacity — a behavior change of the capacity filter that only had round-robin coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
What
Fixes the one known behavioral gap left from #482: a virtual model whose targets are all rate-saturated fell through the same resolution path as an alias with no live targets — the client got an unavailable-model error instead of
429+Retry-After, and the alias even vanished fromGET /v1/modelswhile throttled.Why it happened
Rate-limit capacity was folded into the catalog's
ModelAvailablevia a decorator, which conflated throttled (come back in 30s) with dead (stale inventory). Everything consuming catalog availability inherited that conflation: target selection, model listing, and dashboard redirect validity.How it's fixed
Capacity now steers load balancing only, through an explicit
TargetCapacityprobe on the virtual models service (wired in app init fromratelimit.Service.RouteAvailable):429withRetry-After— or defers to configured failover rules, per the saturated-primary behavior shipped in feat(ratelimit): scoped request, token, and concurrency limits (user paths, providers, models) #482./v1/modelsand their redirects stay valid in the dashboard.The
rateLimitAwareCatalogdecorator is deleted; the registry is passed to virtual models directly again.Testing
Three new balancer tests pin the contract: capacity-preferred selection, deterministic first-target fallback when all are saturated, and fallback still skipping catalog-unavailable targets. Full unit + e2e suites and lint pass; docs (
rate-limits.mdx, spec, CLAUDE.md) updated to describe the fallback.🤖 Generated with Claude Code
Summary by CodeRabbit
429withRetry-After(instead of unavailable-model errors). Saturation does not remove aliases from/v1/models.