feat(ratelimit): scoped request, token, and concurrency limits (user paths, providers, models)#482
Merged
Merged
Conversation
Add a rate limiting feature that caps how fast a user_path subtree can
consume the gateway, complementing budgets (spend control) with traffic
control:
- Rules cap max_requests and/or max_tokens per minute/hour/day/custom
window, plus a "concurrent" period for in-flight request caps. One
shared counter per rule covers its whole subtree; per-key limits fall
out of binding each managed key to its own user_path.
- Enforcement runs before the budget check on every model endpoint
(chat, responses, messages, embeddings, audio, passthrough, realtime
sessions, batch submission). Requests use an in-memory sliding-window
counter; tokens are post-accounted from usage entries via a usage
logger tap (cache hits excluded, requires USAGE_ENABLED); realtime
sessions hold their concurrency slot for the session lifetime.
- Breaches return 429 with an OpenAI-shaped error (code
rate_limit_exceeded) and an accurate Retry-After; successes carry
x-ratelimit-{limit,remaining,reset}-{requests,tokens} headers from
the most-constrained matching rule.
- Managed via a new dashboard Rate Limits page, /admin/rate-limits
CRUD + reset endpoints, a rate_limits: YAML block, and
SET_RATE_LIMIT_<PATH> env vars (rpm/tpm/rph/tph/rpd/tpd/concurrent).
Gated by RATE_LIMITS_ENABLED (default true; no-op without rules).
- Counters are per instance and reset on restart (documented); rule
definitions persist in a new rate_limits store on all three backends.
- The SET_BUDGET_*/SET_RATE_LIMIT_* env appliers share a new generic
per-user-path env merge helper instead of duplicating the loop.
Spec with gateway research (LiteLLM, Bifrost, Portkey, Kong,
Cloudflare, Helicone) in docs/dev/2026-07-05_rate-limiting-spec.md;
user docs in docs/features/rate-limits.mdx.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis PR adds configurable rate limiting across config, storage, request admission, admin management, dashboard UI, and documentation. It also adds end-to-end coverage for request, token, provider, concurrency, and admin rule flows. ChangesRate Limiting
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/app/app.go (1)
443-460: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWire the guardrail executor through the tapped usage logger.
internal/app/app.go:575-584still passesusageResult.Logger, so guardrail-triggered completions skipratelimit.NewUsageTapand never update the token windows. PassserverUsageLoggerhere too.🤖 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/app/app.go` around lines 443 - 460, The guardrail executor is still using the raw usage logger, so completions bypass the rate-limit usage tap and token windows are not updated. Update the guardrail wiring in the app setup to pass the existing serverUsageLogger instead of usageResult.Logger, keeping the same tapped logger used for server requests. Use the surrounding server.Config/server usage logger setup to locate the change.internal/server/budget_support_test.go (1)
69-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a non-nil rate limiter in
batchAdmissionEnforcer.Both tests pass
nilfor the new rate-limiter argument, so only the pre-existing budget-check behavior is verified. Consider adding a case with a real/fakeRateLimiterto confirmbatchAdmissionEnforceractually enforces rate limits for batch submission (e.g., that an exceeded limit surfaces as an error before/along with the budget check).Also applies to: 94-117
🤖 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/server/budget_support_test.go` around lines 69 - 92, Add test coverage for the non-nil rate-limiter path in batchAdmissionEnforcer, since the current batchAdmissionEnforcer and TestBatchBudgetEnforcerUsesResolvedWorkflow only exercise the nil limiter case. Introduce a fake or stub RateLimiter in the budget_support tests and verify that batchAdmissionEnforcer enforces batch submission limits by returning an error when the limiter rejects the request, while still keeping the existing budget-check assertions around countingBudgetChecker and ResolvedWorkflowPolicy.Source: Coding guidelines
🤖 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 `@config/ratelimit.go`:
- Around line 133-152: The period-to-seconds values in rateLimitEnvName are
duplicated and should be shared with rateLimitConfigPeriodSeconds to avoid
keeping two switch statements in sync. Extract common constants for 60, 3600,
and 86400, then update both rateLimitEnvName and rateLimitConfigPeriodSeconds to
use those shared values while preserving the existing rpm/tpm/rph/tph/rpd/tpd
and concurrent/concurrency mappings.
In `@internal/admin/dashboard/static/js/modules/rate-limits.js`:
- Around line 254-288: The rate limit payload builder accepts a blank custom
period seconds value because rateLimitFormPayload() coerces an empty string to
0, which can accidentally submit a concurrent rule. Update
rateLimitFormPayload() to explicitly reject empty or missing period_seconds
before calling Number(), and only allow 0 when the period is truly concurrent;
keep the existing validation and payload construction keyed off rateLimitForm,
period, and limit_key.
In `@internal/admin/dashboard/static/js/modules/rate-limits.test.cjs`:
- Around line 122-140: The `fetchRateLimits handles 503 as feature unavailable`
test has a redundant and misleading `global.fetch` override because
`createRateLimitsModule(...)` already injects `fetch` into the vm context used
by `fetchRateLimits()`. Remove the `global.fetch` setup/cleanup and update the
comment so the test clearly relies on the injected `fetch` from
`loadRateLimitsModuleFactory`/`createRateLimitsModule`, keeping the test focused
on the module’s own fetch handling.
In `@internal/ratelimit/factory.go`:
- Around line 106-137: Config-seeded rate limit rules bypass the same
normalization and validation used by admin-created rules. Update
seedConfiguredRules to build each Rule through NormalizeRule after
NormalizeUserPath, so CreatedAt/UpdatedAt are populated and cross-field checks
like period/max_tokens/max_requests are enforced consistently with
UpsertRateLimit and other rule creation paths.
In `@internal/ratelimit/limiter.go`:
- Around line 47-53: The current `windowCounter.resetAfter` only computes the
fixed bucket rollover, so `ExceededError.RetryAfter` can be too short for the
sliding-window estimate. Add a helper in `limiter.go` that calculates the
earliest recovery time when the weighted current/previous windows drop back
below the limit, and use it when setting `RetryAfter` in the request/token
exceed paths. Update the `windowCounter`/limiter logic to base recovery on the
sliding estimate rather than the bucket boundary, and add tests covering the
rate-limit behavior change for the affected request and token cases.
In `@internal/ratelimit/service.go`:
- Around line 76-92: The `DeleteRule` path is duplicating the same
`period_seconds` validation and error text that also exists in the store
implementations, so the logic can drift. Extract a shared helper like
`validatePeriodSeconds(int64) error` in the ratelimit store layer near
`normalizeRulesForUpsert`, then update `Service.DeleteRule` and the
store-specific `DeleteRule` methods (including `store_sqlite.go` and the other
backend implementations) to call it instead of inlining the check. Keep the
existing error message centralized in that helper so all callers stay
consistent.
In `@internal/ratelimit/store_mongodb.go`:
- Around line 72-85: The MongoDB upsert path in upsertNormalizedRules should be
source-aware so seeded config does not overwrite manual edits that appear after
configRulesWithoutManualCollisions runs. Update the write logic in
store_mongodb.go to either add a source-precedence condition to the
UpdateOneModel filter/update or treat manual-collision duplicates as benign
skips, and make the same change in the related bulk upsert path referenced by
the other affected block.
In `@internal/server/ratelimit_support.go`:
- Around line 58-73: The fallback in rateLimitCheckError is mapping
non-ExceededError failures from Acquire to a provider 503, but those cases are
malformed user_path path-normalization errors and should be treated as invalid
requests. Update rateLimitCheckError to detect this specific failure path and
return an invalid-request/400 style core error instead of
rate_limit_check_failed, while keeping the existing exceeded branch and
rateLimitBreachHeaders handling intact.
In `@tests/e2e/ratelimit_test.go`:
- Around line 1-194: Add an e2e test that covers concurrency-based rate limiting
using the existing rate limit test helpers and the ratelimit service setup.
Create a rule with a concurrency scope/MaxConcurrent limit and verify
overlapping in-flight requests: the first request should be admitted and held
open, the second should receive the rate-limit response and headers, and the
blocked request must not reach the upstream mockServer. Use the existing
TestRateLimitRequestEnforcement_E2E and TestRateLimitTokenEnforcement_E2E
patterns plus symbols like setupRateLimitService, setupE2EServer, and
rateLimitInt64 to locate the right test area.
---
Outside diff comments:
In `@internal/app/app.go`:
- Around line 443-460: The guardrail executor is still using the raw usage
logger, so completions bypass the rate-limit usage tap and token windows are not
updated. Update the guardrail wiring in the app setup to pass the existing
serverUsageLogger instead of usageResult.Logger, keeping the same tapped logger
used for server requests. Use the surrounding server.Config/server usage logger
setup to locate the change.
In `@internal/server/budget_support_test.go`:
- Around line 69-92: Add test coverage for the non-nil rate-limiter path in
batchAdmissionEnforcer, since the current batchAdmissionEnforcer and
TestBatchBudgetEnforcerUsesResolvedWorkflow only exercise the nil limiter case.
Introduce a fake or stub RateLimiter in the budget_support tests and verify that
batchAdmissionEnforcer enforces batch submission limits by returning an error
when the limiter rejects the request, while still keeping the existing
budget-check assertions around countingBudgetChecker and ResolvedWorkflowPolicy.
🪄 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: 76458e31-9dce-4ca5-9aff-e59d34ec8c0a
📒 Files selected for processing (53)
.env.templateCLAUDE.mdconfig/budget.goconfig/config.example.yamlconfig/config.goconfig/config_test.goconfig/ratelimit.goconfig/ratelimit_test.goconfig/user_path_env.godocs/dev/2026-07-05_rate-limiting-spec.mddocs/docs.jsondocs/features/rate-limits.mdxinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/rate-limits.jsinternal/admin/dashboard/static/js/modules/rate-limits.test.cjsinternal/admin/dashboard/templates/index.htmlinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/page-rate-limits.htmlinternal/admin/dashboard/templates/sidebar.htmlinternal/admin/errors.gointernal/admin/handler.gointernal/admin/handler_budgets.gointernal/admin/handler_ratelimits.gointernal/admin/handler_ratelimits_test.gointernal/admin/routes.gointernal/admin/routes_test.gointernal/app/app.gointernal/ratelimit/factory.gointernal/ratelimit/limiter.gointernal/ratelimit/service.gointernal/ratelimit/service_test.gointernal/ratelimit/store.gointernal/ratelimit/store_mongodb.gointernal/ratelimit/store_postgresql.gointernal/ratelimit/store_sqlite.gointernal/ratelimit/store_sqlite_test.gointernal/ratelimit/types.gointernal/ratelimit/types_test.gointernal/ratelimit/usage_tap.gointernal/ratelimit/usage_tap_test.gointernal/server/audio_service.gointernal/server/budget_support_test.gointernal/server/handlers.gointernal/server/http.gointernal/server/messages_handler.gointernal/server/native_batch_service.gointernal/server/passthrough_service.gointernal/server/ratelimit_support.gointernal/server/ratelimit_support_test.gointernal/server/realtime_service.gointernal/server/translated_inference_service.gotests/e2e/ratelimit_test.gotests/e2e/setup_test.go
Comment on lines
+106
to
+137
| func seedConfiguredRules(ctx context.Context, service *Service, cfg config.RateLimitsConfig) error { | ||
| if service == nil { | ||
| return nil | ||
| } | ||
| rules := make([]Rule, 0) | ||
| for _, entry := range cfg.UserPaths { | ||
| userPath, err := NormalizeUserPath(entry.Path) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid rate limit user path %q: %w", entry.Path, err) | ||
| } | ||
| for limitIdx, limit := range entry.Limits { | ||
| var seconds int64 | ||
| if limit.PeriodSeconds != nil { | ||
| seconds = *limit.PeriodSeconds | ||
| } else { | ||
| parsed, ok := PeriodSecondsFromName(limit.Period) | ||
| if !ok { | ||
| return fmt.Errorf("invalid rate limit period for user path %q limit %d: %q", userPath, limitIdx, limit.Period) | ||
| } | ||
| seconds = parsed | ||
| } | ||
| rules = append(rules, Rule{ | ||
| UserPath: userPath, | ||
| PeriodSeconds: seconds, | ||
| MaxRequests: limit.MaxRequests, | ||
| MaxTokens: limit.MaxTokens, | ||
| Source: SourceConfig, | ||
| }) | ||
| } | ||
| } | ||
| return service.ReplaceConfigRules(ctx, rules) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Config-seeded rules skip NormalizeRule, unlike manual admin rules.
seedConfiguredRules only calls NormalizeUserPath and builds Rule{...} directly, leaving CreatedAt/UpdatedAt as zero-value time.Time and skipping the cross-field validation NormalizeRule performs (e.g. rejecting max_tokens on a concurrent period, or requiring at least one of max_requests/max_tokens). The admin API path (UpsertRateLimit in internal/admin/handler_ratelimits.go) calls ratelimit.NormalizeRule for the same purpose, so YAML/env-seeded rules and manually-created rules take different, inconsistent validation/normalization paths.
🔧 Proposed fix
for limitIdx, limit := range entry.Limits {
var seconds int64
if limit.PeriodSeconds != nil {
seconds = *limit.PeriodSeconds
} else {
parsed, ok := PeriodSecondsFromName(limit.Period)
if !ok {
return fmt.Errorf("invalid rate limit period for user path %q limit %d: %q", userPath, limitIdx, limit.Period)
}
seconds = parsed
}
- rules = append(rules, Rule{
- UserPath: userPath,
- PeriodSeconds: seconds,
- MaxRequests: limit.MaxRequests,
- MaxTokens: limit.MaxTokens,
- Source: SourceConfig,
- })
+ rule, err := NormalizeRule(Rule{
+ UserPath: userPath,
+ PeriodSeconds: seconds,
+ MaxRequests: limit.MaxRequests,
+ MaxTokens: limit.MaxTokens,
+ Source: SourceConfig,
+ })
+ if err != nil {
+ return fmt.Errorf("invalid rate limit rule for user path %q limit %d: %w", userPath, limitIdx, err)
+ }
+ rules = append(rules, rule)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func seedConfiguredRules(ctx context.Context, service *Service, cfg config.RateLimitsConfig) error { | |
| if service == nil { | |
| return nil | |
| } | |
| rules := make([]Rule, 0) | |
| for _, entry := range cfg.UserPaths { | |
| userPath, err := NormalizeUserPath(entry.Path) | |
| if err != nil { | |
| return fmt.Errorf("invalid rate limit user path %q: %w", entry.Path, err) | |
| } | |
| for limitIdx, limit := range entry.Limits { | |
| var seconds int64 | |
| if limit.PeriodSeconds != nil { | |
| seconds = *limit.PeriodSeconds | |
| } else { | |
| parsed, ok := PeriodSecondsFromName(limit.Period) | |
| if !ok { | |
| return fmt.Errorf("invalid rate limit period for user path %q limit %d: %q", userPath, limitIdx, limit.Period) | |
| } | |
| seconds = parsed | |
| } | |
| rules = append(rules, Rule{ | |
| UserPath: userPath, | |
| PeriodSeconds: seconds, | |
| MaxRequests: limit.MaxRequests, | |
| MaxTokens: limit.MaxTokens, | |
| Source: SourceConfig, | |
| }) | |
| } | |
| } | |
| return service.ReplaceConfigRules(ctx, rules) | |
| } | |
| func seedConfiguredRules(ctx context.Context, service *Service, cfg config.RateLimitsConfig) error { | |
| if service == nil { | |
| return nil | |
| } | |
| rules := make([]Rule, 0) | |
| for _, entry := range cfg.UserPaths { | |
| userPath, err := NormalizeUserPath(entry.Path) | |
| if err != nil { | |
| return fmt.Errorf("invalid rate limit user path %q: %w", entry.Path, err) | |
| } | |
| for limitIdx, limit := range entry.Limits { | |
| var seconds int64 | |
| if limit.PeriodSeconds != nil { | |
| seconds = *limit.PeriodSeconds | |
| } else { | |
| parsed, ok := PeriodSecondsFromName(limit.Period) | |
| if !ok { | |
| return fmt.Errorf("invalid rate limit period for user path %q limit %d: %q", userPath, limitIdx, limit.Period) | |
| } | |
| seconds = parsed | |
| } | |
| rule, err := NormalizeRule(Rule{ | |
| UserPath: userPath, | |
| PeriodSeconds: seconds, | |
| MaxRequests: limit.MaxRequests, | |
| MaxTokens: limit.MaxTokens, | |
| Source: SourceConfig, | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("invalid rate limit rule for user path %q limit %d: %w", userPath, limitIdx, err) | |
| } | |
| rules = append(rules, rule) | |
| } | |
| } | |
| return service.ReplaceConfigRules(ctx, rules) | |
| } |
🤖 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/ratelimit/factory.go` around lines 106 - 137, Config-seeded rate
limit rules bypass the same normalization and validation used by admin-created
rules. Update seedConfiguredRules to build each Rule through NormalizeRule after
NormalizeUserPath, so CreatedAt/UpdatedAt are populated and cross-field checks
like period/max_tokens/max_requests are enforced consistently with
UpsertRateLimit and other rule creation paths.
Comment on lines
+76
to
+92
| func (s *Service) DeleteRule(ctx context.Context, userPath string, periodSeconds int64) error { | ||
| if s == nil || s.store == nil { | ||
| return ErrUnavailable | ||
| } | ||
| userPath, err := NormalizeUserPath(userPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if periodSeconds < 0 { | ||
| return fmt.Errorf("period_seconds must be 0 (concurrent) or greater") | ||
| } | ||
| if err := s.store.DeleteRule(ctx, userPath, periodSeconds); err != nil { | ||
| return err | ||
| } | ||
| s.limiter.reset(ruleKey{userPath: userPath, periodSeconds: periodSeconds}) | ||
| return s.Refresh(ctx) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate period_seconds validation across layers.
The period_seconds must be 0 (concurrent) or greater check (and message) here is duplicated verbatim in internal/ratelimit/store_sqlite.go's DeleteRule (lines 93-95), and likely also in the PostgreSQL/MongoDB store implementations following the same pattern. Extracting a shared validatePeriodSeconds(int64) error helper (e.g., in store.go alongside normalizeRulesForUpsert) would prevent the message/logic from drifting if one call site is updated without the others.
♻️ Proposed extraction
+func validatePeriodSeconds(periodSeconds int64) error {
+ if periodSeconds < 0 {
+ return fmt.Errorf("period_seconds must be 0 (concurrent) or greater")
+ }
+ return nil
+}
+
func (s *Service) DeleteRule(ctx context.Context, userPath string, periodSeconds int64) error {
if s == nil || s.store == nil {
return ErrUnavailable
}
userPath, err := NormalizeUserPath(userPath)
if err != nil {
return err
}
- if periodSeconds < 0 {
- return fmt.Errorf("period_seconds must be 0 (concurrent) or greater")
- }
+ if err := validatePeriodSeconds(periodSeconds); err != nil {
+ return err
+ }
if err := s.store.DeleteRule(ctx, userPath, periodSeconds); err != nil {
return err
}🤖 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/ratelimit/service.go` around lines 76 - 92, The `DeleteRule` path is
duplicating the same `period_seconds` validation and error text that also exists
in the store implementations, so the logic can drift. Extract a shared helper
like `validatePeriodSeconds(int64) error` in the ratelimit store layer near
`normalizeRulesForUpsert`, then update `Service.DeleteRule` and the
store-specific `DeleteRule` methods (including `store_sqlite.go` and the other
backend implementations) to call it instead of inlining the check. Keep the
existing error message centralized in that helper so all callers stay
consistent.
Comment on lines
+58
to
+73
| func rateLimitCheckError(err error) error { | ||
| var exceeded *ratelimit.ExceededError | ||
| if errors.As(err, &exceeded) { | ||
| message := exceeded.Error() | ||
| if message == "" { | ||
| message = "rate limit exceeded" | ||
| } | ||
| gatewayErr := core.NewRateLimitError("ratelimit", message).WithCode("rate_limit_exceeded") | ||
| return &gatewayErrorWithResponseHeaders{ | ||
| GatewayError: gatewayErr, | ||
| headers: rateLimitBreachHeaders(exceeded), | ||
| } | ||
| } | ||
| return core.NewProviderError("ratelimit", http.StatusServiceUnavailable, "rate limit check failed", err). | ||
| WithCode("rate_limit_check_failed") | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect NormalizeUserPath and Service.matchingRules/admit to see what error types Acquire can surface.
fd -e go . internal/ratelimit | xargs rg -n "func NormalizeUserPath|func \(s \*Service\) matchingRules|func \(l \*.*\) admit" -A 15Repository: ENTERPILOT/GoModel
Length of output: 2666
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the rate-limit service flow and the core path normalization error taxonomy.
sed -n '1,220p' internal/ratelimit/service.go
printf '\n----\n'
sed -n '1,220p' internal/ratelimit/types.go
printf '\n----\n'
fd -e go . internal/core | xargs rg -n "func NormalizeUserPath|type .*Invalid.*Path|Invalid.*Path|NormalizeUserPath" -A 20Repository: ENTERPILOT/GoModel
Length of output: 18898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the gateway-side handling of Acquire errors and any tests that encode the intended status mapping.
fd -e go . internal | xargs rg -n "rateLimitCheckError|rate_limit_check_failed|rate_limit_exceeded|Acquire\(" -A 20Repository: ENTERPILOT/GoModel
Length of output: 37499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for tests or callers that distinguish invalid user paths from backend rate-limit failures.
fd -e go . . | xargs rg -n "InvalidRequest|ServiceUnavailable|rate_limit_check_failed|NormalizeUserPath\(" -A 10Repository: ENTERPILOT/GoModel
Length of output: 50375
Map invalid user_path errors to 400 (internal/server/ratelimit_support.go:58-73)
Acquire only returns a non-ExceededError on path-normalization failures, so this fallback turns malformed user_path values into 503 rate_limit_check_failed. That should be an invalid-request response instead.
🤖 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/server/ratelimit_support.go` around lines 58 - 73, The fallback in
rateLimitCheckError is mapping non-ExceededError failures from Acquire to a
provider 503, but those cases are malformed user_path path-normalization errors
and should be treated as invalid requests. Update rateLimitCheckError to detect
this specific failure path and return an invalid-request/400 style core error
instead of rate_limit_check_failed, while keeping the existing exceeded branch
and rateLimitBreachHeaders handling intact.
- Retry-After now reports the earliest second the sliding-window estimate would actually admit a retry, instead of the next bucket boundary, which under-promised right after a rollover while the previous window still weighed in (docs updated to match). - The Mongo store's upsert is source-aware like the SQL stores: a config-sourced write only updates config rows, and the duplicate-key insert produced by a shadowing manual row is treated as the intended precedence, not a failure. - Guardrail LLM calls run through the tapped usage logger so their tokens count toward rate limit token windows. - The dashboard form rejects blank custom period seconds instead of coercing them to 0 and silently submitting a concurrent rule. - Deduped the period_seconds validation shared by DeleteRule and the stores, and the period constants shared by the env-name and YAML period parsers. - Added an e2e concurrency test (held in-flight request blocks the next one, slot frees on completion), a batch-enforcer test with a live limiter, exact Retry-After recovery tests, and unit coverage for the Mongo duplicate-key classification; removed a redundant global.fetch override from the dashboard module test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rule identity becomes (scope, subject, period). user_path rules keep their admission-control behavior (429 + Retry-After + x-ratelimit-* headers). Provider and model rules act as routing constraints: virtual-model load balancing consults a rate-limit-aware catalog and failover sweeps consult a RouteGate, so saturated targets are skipped like stale-inventory providers and clients only see 429 when no viable target remains. Token windows are charged to the executed provider/model from usage entries, keeping accounting correct under aliasing and failover. Config gains rate_limits.providers / rate_limits.models blocks and SET_PROVIDER_RATE_LIMIT_<NAME> env vars (suffix underscores map to hyphens like provider-instance env vars; model rules are YAML/admin-only). The dashboard form gets a scope selector and rows show scope chips. Pre-scope rate_limits tables/documents migrate in place at store init on SQLite, PostgreSQL, and MongoDB. Also fixes the rate-limit modal silently refusing to save concurrent rules: native form validation blocked submission on the hidden period-seconds input (min=1 vs value 0); the form is now novalidate and scope/period switches clear stale hidden fields. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SantiagoDePolonia
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
internal/ratelimit/service.go (1)
192-210: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
RecordTokenssilently drops token accounting on normalization failure.If
normalizeSubjectserrors (malformedUserPath), the function returns without recording tokens and without any log, which can silently hide accounting bugs and let token limits under-count for that subject.🔍 Suggested fix
subjects, err := normalizeSubjects(subjects) if err != nil { + slog.Debug("ratelimit: dropping token record due to invalid subjects", "error", err) return }🤖 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/ratelimit/service.go` around lines 192 - 210, RecordTokens is swallowing normalizeSubjects errors and silently skipping accounting, which can hide malformed UserPath issues. Update Service.RecordTokens to handle the normalizeSubjects(subjects) error explicitly by logging or otherwise surfacing it before returning, using the RecordTokens and normalizeSubjects paths to locate the fix, while keeping the existing early-return behavior for invalid inputs.internal/admin/dashboard/static/js/modules/rate-limits.js (1)
326-356: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBlank
user_pathsubject silently becomes root scope (/) with no validation error.Provider/model scopes reject a blank subject (Line 330-332), but user_path scope has no equivalent guard:
subject || '/'(Line 354) silently converts a cleared/whitespace-only path into a root-level rule that "limits the whole subtree" (per the template help text). Clearing the field while editing an existing narrower rule (e.g./team/alpha) and saving would silently broaden it to affect all traffic, without any warning.🐛 Proposed fix
const isConcurrent = String(form.period || '') === 'concurrent'; + if (scope === 'user_path' && !subject) { + return { error: 'User path is required.' }; + }🤖 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/admin/dashboard/static/js/modules/rate-limits.js` around lines 326 - 356, The rate limit payload builder in rateLimitFormPayload() silently converts an empty user_path subject into “/” via subject || '/', which can broaden an existing path rule without validation. Add an explicit validation for user_path that rejects blank or whitespace-only subjects before building the payload, and only allow a normalized root subject when the user intentionally sets it. Keep the existing scope/subject checks in rateLimitFormPayload() and use the same error-reporting pattern as the other required-field validation paths.internal/ratelimit/store_mongodb.go (1)
90-129: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep duplicate-key retries out of the manual write path
Config seeds can keep the benign duplicate-key skip, but manual upserts can race on a new key and currently return success after one writer’s values are dropped. Retry that conflict as a normal update, or surface it instead of swallowing every duplicate-key BulkWrite error.
🤖 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/ratelimit/store_mongodb.go` around lines 90 - 129, The duplicate-key handling in MongoDBStore.upsertNormalizedRules is too broad and masks manual write races. Keep the benign skip only for config-seeded writes (SourceConfig filter path), and for manual upserts either retry the conflicting BulkWrite as a normal update or return the error instead of treating every duplicate-key error as success. Use upsertNormalizedRules, isOnlyDuplicateKeyErrors, and the SourceConfig branch to localize the fix.internal/server/translated_inference_service.go (1)
99-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated rate-limit + budget enforcement pattern.
The
enforceRateLimit→defer release()→enforceBudgetsequence is duplicated verbatim acrossdispatchChatCompletion,dispatchResponses, andEmbeddingsin this file (and again inpassthrough_service.go). Any future change to enforcement ordering (e.g. adding a guardrail check between rate limit and budget) needs to be applied consistently in 4 places.♻️ Suggested consolidation
// in ratelimit_support.go func enforceRateLimitAndBudget(c *echo.Context, limiter RateLimiter, checker BudgetChecker, route rateLimitRoute) (func(), error) { release, err := enforceRateLimit(c, limiter, route) if err != nil { return func() {}, err } if err := enforceBudget(c, checker); err != nil { release() return func() {}, err } return release, nil }- release, err := enforceRateLimit(c, s.rateLimiter, rateLimitRouteFromWorkflow(workflow)) - if err != nil { - return handleError(c, err) - } - defer release() - if err := enforceBudget(c, s.budgetChecker); err != nil { - return handleError(c, err) - } + release, err := enforceRateLimitAndBudget(c, s.rateLimiter, s.budgetChecker, rateLimitRouteFromWorkflow(workflow)) + if err != nil { + return handleError(c, err) + } + defer release()Also applies to: 268-272, 458-462
🤖 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/server/translated_inference_service.go` around lines 99 - 103, The rate-limit and budget enforcement sequence is duplicated in dispatchChatCompletion, dispatchResponses, and Embeddings, so consolidate it into a shared helper (for example, in ratelimit_support.go) and use that helper from these dispatch paths. Keep the existing ordering and cleanup behavior by having the helper wrap enforceRateLimit, defer release handling, and enforceBudget in one place, then update the repeated call sites in translated_inference_service.go (and the matching passthrough_service.go usage) to use the new shared function.internal/ratelimit/limiter.go (1)
123-162: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn the longest blocking
RetryAfteracross all exceeded rules.
admitexits on the first exceeded rule, so overlapping exhausted windows can understateRetry-After(for example, minute + day limits where the minute rule is checked first). Continue evaluating matching rules, keep the exceeded error with the longest wait, and only skip the commit phase after all checks finish. As per coding guidelines, add or update tests for behavior changes.🤖 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/ratelimit/limiter.go` around lines 123 - 162, The admit path in limiter.go currently returns on the first exceeded rule, which can understate Retry-After when multiple matching rules are exhausted. Update the admit logic to keep evaluating all applicable rules in the loop, track the exceeded result with the longest RetryAfter, and defer returning until after all checks complete; use the existing rule/key/counter flow in admit, keyForRule, and ExceededError. Ensure the commit phase is skipped only after evaluation finishes and preserve the most specific exceeded scope/details from the chosen error. Add or update tests covering overlapping rules where a longer RetryAfter should win over an earlier shorter one.Source: Coding guidelines
🤖 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/admin/handler_ratelimits_test.go`:
- Around line 251-328: Add a mixed-case model subject case to
TestRateLimitEndpointsProviderAndModelScopes so it verifies model subject
normalization/matching, not just provider normalization. Update the model upsert
path in UpsertRateLimit/DeleteRateLimit coverage to use a mixed-case subject
such as OpenAI/GPT-4o, then assert the stored response is normalized and that
Acquire in the service still matches a lowercase live request through
ratelimit.Subjects.
In `@internal/admin/handler_ratelimits.go`:
- Around line 266-293: The rateLimitRequestKey validation currently only rejects
a user_path when subject is missing, but it silently ignores user_path when both
subject and a non-user_path scope are provided. Update rateLimitRequestKey to
explicitly validate the rawSubject/rawUserPath combination before
NormalizeSubject: if scope is not ratelimit.ScopeUserPath and rawUserPath is
set, return an error instead of discarding it. Keep the existing scope,
NormalizeScope, NormalizeSubject, and rateLimitRequestPeriodSeconds flow intact,
and make the new error path consistent with the existing limit_key and subject
validation.
In `@internal/gateway/failover_test.go`:
- Around line 84-119: Add a matching test for tryFailoverStream to cover the
same routeGate skip behavior exercised by
TestTryFailoverResponseSkipsRateLimitedTargets. Reuse the existing
failoverTestFixture, blockingRouteGate, and stubFailoverResolver setup to make
one candidate rate-limited and verify tryFailoverStream skips it and proceeds to
the next available selector. Ensure the assertions check the attempted model
list and the successful failover result so the stream path stays aligned with
tryFailoverResponse.
In `@internal/ratelimit/store_postgresql.go`:
- Around line 42-95: Wrap migratePostgreSQLPreScopeTable in a single PostgreSQL
transaction so the RENAME, CREATE, INSERT...SELECT, and DROP steps are atomic
and cannot leave rate_limits partially migrated. Use the same
pool.Begin/tx.Commit pattern already used by ReplaceConfigRules, and run each
statement through the transaction instead of pool.Exec. Also consider taking an
advisory lock around the migration path to prevent multiple replicas from racing
while NewPostgreSQLStore probes and initializes the schema.
In `@internal/ratelimit/store_sqlite_test.go`:
- Around line 182-186: The in-memory SQLite test database is connection-local,
so the test setup can accidentally use a different empty database on later
queries. Update the SQLite test setup around sql.Open in store_sqlite_test.go
and the newSQLiteTestStore helper to call db.SetMaxOpenConns(1) after opening
the database, so all operations stay pinned to the same connection.
In `@internal/ratelimit/store_sqlite.go`:
- Around line 73-95: The rate_limits schema rebuild in StoreSQLite migration is
executed statement-by-statement without atomicity, so a failure in the middle
can leave a partially migrated database. Update the migration loop in
store_sqlite.go to run the rename/create/copy/drop sequence inside a single
transaction, and only commit after every statement succeeds; if any stmt
execution fails, rollback and return the wrapped error from the migration path.
In `@internal/ratelimit/types.go`:
- Around line 213-221: The model subject normalization in the ScopeModel branch
of the subject normalization logic should canonicalize case to match
modelSubjectMatches. Update the trimming/validation path so the returned subject
is lowercased after whitespace is removed and before storage, while keeping the
existing empty and slash checks intact. Use the existing normalize subject
handling around ScopeModel and modelSubjectMatches to ensure persisted model
rules are stored consistently and avoid duplicate entries with different casing.
---
Outside diff comments:
In `@internal/admin/dashboard/static/js/modules/rate-limits.js`:
- Around line 326-356: The rate limit payload builder in rateLimitFormPayload()
silently converts an empty user_path subject into “/” via subject || '/', which
can broaden an existing path rule without validation. Add an explicit validation
for user_path that rejects blank or whitespace-only subjects before building the
payload, and only allow a normalized root subject when the user intentionally
sets it. Keep the existing scope/subject checks in rateLimitFormPayload() and
use the same error-reporting pattern as the other required-field validation
paths.
In `@internal/ratelimit/limiter.go`:
- Around line 123-162: The admit path in limiter.go currently returns on the
first exceeded rule, which can understate Retry-After when multiple matching
rules are exhausted. Update the admit logic to keep evaluating all applicable
rules in the loop, track the exceeded result with the longest RetryAfter, and
defer returning until after all checks complete; use the existing
rule/key/counter flow in admit, keyForRule, and ExceededError. Ensure the commit
phase is skipped only after evaluation finishes and preserve the most specific
exceeded scope/details from the chosen error. Add or update tests covering
overlapping rules where a longer RetryAfter should win over an earlier shorter
one.
In `@internal/ratelimit/service.go`:
- Around line 192-210: RecordTokens is swallowing normalizeSubjects errors and
silently skipping accounting, which can hide malformed UserPath issues. Update
Service.RecordTokens to handle the normalizeSubjects(subjects) error explicitly
by logging or otherwise surfacing it before returning, using the RecordTokens
and normalizeSubjects paths to locate the fix, while keeping the existing
early-return behavior for invalid inputs.
In `@internal/ratelimit/store_mongodb.go`:
- Around line 90-129: The duplicate-key handling in
MongoDBStore.upsertNormalizedRules is too broad and masks manual write races.
Keep the benign skip only for config-seeded writes (SourceConfig filter path),
and for manual upserts either retry the conflicting BulkWrite as a normal update
or return the error instead of treating every duplicate-key error as success.
Use upsertNormalizedRules, isOnlyDuplicateKeyErrors, and the SourceConfig branch
to localize the fix.
In `@internal/server/translated_inference_service.go`:
- Around line 99-103: The rate-limit and budget enforcement sequence is
duplicated in dispatchChatCompletion, dispatchResponses, and Embeddings, so
consolidate it into a shared helper (for example, in ratelimit_support.go) and
use that helper from these dispatch paths. Keep the existing ordering and
cleanup behavior by having the helper wrap enforceRateLimit, defer release
handling, and enforceBudget in one place, then update the repeated call sites in
translated_inference_service.go (and the matching passthrough_service.go usage)
to use the new shared function.
🪄 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: 890e55d2-27e7-469c-b04d-79da04ee6f6d
📒 Files selected for processing (42)
.env.templateCLAUDE.mdconfig/config.example.yamlconfig/config_test.goconfig/ratelimit.goconfig/ratelimit_test.goconfig/user_path_env.godocs/dev/2026-07-05_rate-limiting-spec.mddocs/features/rate-limits.mdxinternal/admin/dashboard/static/js/modules/rate-limits.jsinternal/admin/dashboard/static/js/modules/rate-limits.test.cjsinternal/admin/dashboard/templates/page-rate-limits.htmlinternal/admin/handler_ratelimits.gointernal/admin/handler_ratelimits_test.gointernal/app/app.gointernal/app/ratelimit_catalog.gointernal/gateway/failover.gointernal/gateway/failover_test.gointernal/gateway/inference_orchestrator.gointernal/ratelimit/factory.gointernal/ratelimit/limiter.gointernal/ratelimit/service.gointernal/ratelimit/service_test.gointernal/ratelimit/store.gointernal/ratelimit/store_mongodb.gointernal/ratelimit/store_mongodb_test.gointernal/ratelimit/store_postgresql.gointernal/ratelimit/store_sqlite.gointernal/ratelimit/store_sqlite_test.gointernal/ratelimit/types.gointernal/ratelimit/types_test.gointernal/ratelimit/usage_tap.gointernal/ratelimit/usage_tap_test.gointernal/server/audio_service.gointernal/server/budget_support_test.gointernal/server/messages_handler.gointernal/server/passthrough_service.gointernal/server/ratelimit_support.gointernal/server/ratelimit_support_test.gointernal/server/realtime_service.gointernal/server/translated_inference_service.gotests/e2e/ratelimit_test.go
- Run the pre-scope schema migrations in one transaction on SQLite and PostgreSQL: a crash mid-rebuild could leave the table renamed away, and the next startup would create a fresh empty rate_limits and orphan every rule. The table DDL is now a single shared constant per store. - Lowercase model rule subjects at normalization so storage matches the case-insensitive matching (no duplicate OpenAI/GPT-4o vs openai/gpt-4o rules covering the same requests). - Report the exceeded rule with the longest recovery when several windows are exhausted at once, keeping Retry-After honest in one round trip; the breach check is now shared between admission and the read-only route probe. - Restrict Mongo's benign duplicate-key skip to config-sourced writes and retry manual insert races as plain updates (last write wins, matching the SQL stores). - Reject conflicting subject + user_path on provider/model admin requests instead of silently dropping the user_path. - Pin in-memory SQLite test databases to one connection, cover the stream failover route-gate skip, and extract the shared rate-limit + budget admission sequence used by five dispatch sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
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/ratelimit/store_mongodb.go`:
- Around line 130-138: The retry-failure path in the rate limit MongoDB upsert
flow is returning the original duplicate-key error instead of the retry’s actual
failure. In the logic around s.rules.BulkWrite, capture the retry error from the
second BulkWrite attempt and, when it is not an allowed duplicate-key-only case,
wrap and return that retryErr rather than err so the real cause is preserved.
Keep the duplicate-key fast path unchanged, but make the fall-through report the
final failure from the retry.
🪄 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: 211ed07a-1401-4778-bc8d-72d3e9f41a1e
📒 Files selected for processing (17)
internal/admin/dashboard/static/js/modules/rate-limits.jsinternal/admin/handler_ratelimits.gointernal/admin/handler_ratelimits_test.gointernal/gateway/failover_test.gointernal/ratelimit/limiter.gointernal/ratelimit/service_test.gointernal/ratelimit/store_mongodb.gointernal/ratelimit/store_mongodb_test.gointernal/ratelimit/store_postgresql.gointernal/ratelimit/store_sqlite.gointernal/ratelimit/store_sqlite_test.gointernal/ratelimit/types.gointernal/ratelimit/types_test.gointernal/server/messages_handler.gointernal/server/passthrough_service.gointernal/server/ratelimit_support.gointernal/server/translated_inference_service.go
When the retry after a manual insert race fails for a different reason, wrap that failure instead of the original duplicate-key error, so the real cause is diagnosable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every model row and provider group header on the Models page gets a gauge button that opens an effective-limits inspector: the model's own rules, the provider rules it shares, and the global root user-path rules, each with live usage and add/edit actions. The editor opened from the inspector returns to it on close. The rate-limit editor's scope, subject, and period are now editable while editing. Changing any of them moves the rule: the new key is created first and the old rule deleted after, so a failed delete can never lose the rule, and an edit that merely respells the same identity (case, slashes) is detected via a client-side mirror of the server normalization and treated as an in-place update. A hint in the form explains that moving a rule restarts its live counters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The rate-limit inspector rows now double as usage meters: the row background fills left-to-right with the share of the most constrained cap used, ramping green to warning at 75% and danger at 100%, with the exact percentage in the row tooltip. The Models page gauge buttons carry the limit state: fully painted (with the active dot) when the model or provider has its own rules, half painted when only provider or global root rules throttle it, plain when nothing applies. Tooltips spell the state out, and the models page now loads the rule list so the indicators are correct on first render. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| requestID := requestIDFromContextOrHeader(c.Request()) | ||
|
|
||
| if err := enforceBudget(c, s.budgetChecker); err != nil { | ||
| release, err := enforceAdmission(c, s.rateLimiter, s.budgetChecker, rateLimitRouteFromWorkflow(workflow)) |
There was a problem hiding this comment.
enforceAdmission rejects provider/model-scoped breaches before the orchestrator runs, so a direct request whose primary route is saturated returns 429 instead of entering ExecuteChatCompletion and letting RouteGate skip to configured failover targets. The failover gate in internal/gateway/failover.go only runs after a primary dispatch error, but this line prevents that dispatch entirely.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/ratelimit/store_mongodb.go (1)
90-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftLow test coverage on concurrency-sensitive upsert/retry logic.
This function contains the config-vs-manual precedence filter and the duplicate-key race retry — both areas that were just bug-fixed per prior review rounds. Codecov reports only 9.43% patch coverage for this file. Given the subtlety of the retry/collision semantics (and that a test file already exists for this package), targeted table-driven tests covering: (1) manual write racing a concurrent insert, (2) config seed colliding with an existing manual row, and (3) retry failing for a reason other than duplicate-key, would meaningfully de-risk this path.
🤖 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/ratelimit/store_mongodb.go` around lines 90 - 141, The concurrency-sensitive upsert path in MongoDBStore.upsertNormalizedRules lacks targeted coverage for the config/manual precedence filter and duplicate-key retry handling. Add table-driven tests in the existing package test file that exercise: a manual rule upsert racing a concurrent insert and succeeding after the retry path, a config-sourced rule colliding with an existing manual row and being skipped by duplicateKeyErrorsOnConfigRulesOnly, and a non-duplicate error on the retry path returning a wrapped failure from upsertNormalizedRules. Use the unique symbols upsertNormalizedRules, duplicateKeyErrorsOnConfigRulesOnly, and isOnlyDuplicateKeyErrors to keep the tests aligned with the intended semantics.internal/admin/dashboard/static/js/dashboard.js (1)
449-466: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude Models-page rate-limit dialogs in
overlayDialogOpen()
page-models.htmlrenders the rate-limit inspector/editor, but this guard only treatsrateLimitFormOpenas modal state onthis.page === "rate-limits"and never checksrateLimitInspectorOpen. When the editor is opened from Models,body.dashboard-modal-openstays off and the page can keep scrolling behind the dialog.🤖 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/admin/dashboard/static/js/dashboard.js` around lines 449 - 466, `overlayDialogOpen()` is missing the Models-page rate-limit modal state, so add the Models-specific rate-limit inspector/editor flag(s) to this guard alongside `rateLimitFormOpen`. Update the `Dashboard` state check so it treats the rate-limit dialog as open when `this.page === "models"` and `rateLimitInspectorOpen` is active, matching the existing `rateLimitFormOpen` handling used for `this.page === "rate-limits"`. This will ensure the modal-open class stays applied while the Models-page rate-limit dialog is visible.
🤖 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/admin/dashboard/static/js/modules/rate-limits.js`:
- Around line 705-725: Both rateLimitGaugeClassForModel and
rateLimitGaugeClassForProvider recompute the same rule-matching checks against
this.rateLimits on every call, which is repeated multiple times per row.
Refactor these helpers to avoid re-scanning the full rules list for each
class/label usage by computing the active/inherited state once per row or by
introducing shared cached helper logic using rateLimitInspectorModelID,
rateLimitRuleMatchesModel, rateLimitRuleMatchesProvider, and
hasGlobalRateLimits.
In `@internal/admin/dashboard/static/js/modules/rate-limits.test.cjs`:
- Around line 277-281: Replace the JSON.stringify-based equality checks in
rate-limits.test.cjs with assert.deepEqual so the assertions compare the request
body objects directly and produce better failures. Update the specific body
assertion near the calls[1].body check, and apply the same refactor to the other
matching assertions mentioned in the review, keeping the existing expected
object shapes and using the same test helpers around the affected rate limit
request cases.
In `@internal/admin/dashboard/templates/rate-limit-editor.html`:
- Around line 15-22: The dialog in rateLimitFormOpen currently uses a static
aria-label on the rate-limit editor, while the visible title in the
editor-header h3 changes between edit and create modes. Update the dialog markup
to use aria-labelledby tied to the h3 in the editor header (give that heading a
stable id) so assistive tech announces the same dynamic title as the UI; keep
the submit flow and rateLimitFormPayload behavior unchanged.
---
Outside diff comments:
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 449-466: `overlayDialogOpen()` is missing the Models-page
rate-limit modal state, so add the Models-specific rate-limit inspector/editor
flag(s) to this guard alongside `rateLimitFormOpen`. Update the `Dashboard`
state check so it treats the rate-limit dialog as open when `this.page ===
"models"` and `rateLimitInspectorOpen` is active, matching the existing
`rateLimitFormOpen` handling used for `this.page === "rate-limits"`. This will
ensure the modal-open class stays applied while the Models-page rate-limit
dialog is visible.
In `@internal/ratelimit/store_mongodb.go`:
- Around line 90-141: The concurrency-sensitive upsert path in
MongoDBStore.upsertNormalizedRules lacks targeted coverage for the config/manual
precedence filter and duplicate-key retry handling. Add table-driven tests in
the existing package test file that exercise: a manual rule upsert racing a
concurrent insert and succeeding after the retry path, a config-sourced rule
colliding with an existing manual row and being skipped by
duplicateKeyErrorsOnConfigRulesOnly, and a non-duplicate error on the retry path
returning a wrapped failure from upsertNormalizedRules. Use the unique symbols
upsertNormalizedRules, duplicateKeyErrorsOnConfigRulesOnly, and
isOnlyDuplicateKeyErrors to keep the tests aligned with the intended semantics.
🪄 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: 1f356617-9033-4f56-a59c-c3aa454b00b2
📒 Files selected for processing (12)
docs/features/rate-limits.mdxinternal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/rate-limits.jsinternal/admin/dashboard/static/js/modules/rate-limits.test.cjsinternal/admin/dashboard/templates/gauge-icon.htmlinternal/admin/dashboard/templates/model-table-body.htmlinternal/admin/dashboard/templates/page-models.htmlinternal/admin/dashboard/templates/page-rate-limits.htmlinternal/admin/dashboard/templates/rate-limit-editor.htmlinternal/admin/dashboard/templates/rate-limit-inspector.htmlinternal/ratelimit/store_mongodb.go
Comment on lines
+277
to
+281
| assert.equal(JSON.stringify(calls[1].body), JSON.stringify({ | ||
| scope: 'user_path', | ||
| subject: '/team', | ||
| limit_key: { period_seconds: 60 } | ||
| })); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Prefer assert.deepEqual over JSON.stringify comparisons.
Using JSON.stringify for object/array equality works here but produces less readable diffs on failure and is incidentally order-sensitive.
♻️ Example refactor
- assert.equal(JSON.stringify(sections.map((section) => section.key)), JSON.stringify(['model', 'provider', 'global']));
+ assert.deepEqual(sections.map((section) => section.key), ['model', 'provider', 'global']);Also applies to: 342-354
🤖 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/admin/dashboard/static/js/modules/rate-limits.test.cjs` around lines
277 - 281, Replace the JSON.stringify-based equality checks in
rate-limits.test.cjs with assert.deepEqual so the assertions compare the request
body objects directly and produce better failures. Update the specific body
assertion near the calls[1].body check, and apply the same refactor to the other
matching assertions mentioned in the review, keeping the existing expected
object shapes and using the same test helpers around the affected rate limit
request cases.
…lover Admission previously returned 429 for provider/model-scoped breaches before dispatch ever ran, so the failover sweep (and its RouteGate) never got the chance to serve the request from another target — breaking the advertised "429 only when no viable target remains" behavior for direct routes with failover rules. The primary provider still must not be called (it would happily serve the request and defeat the gateway's limit), so the breach becomes a synthetic primary failure instead: when failover targets exist, admission re-admits the request against its consumer limits only, stamps the stored 429 into the context, and dispatch skips the primary call and enters the failover sweep seeded with that error — for both response and stream paths. The sweep still skips rate-saturated candidates, and the original 429 with its Retry-After surfaces unchanged when nothing can take the request. Consumer (user-path) breaches never defer — switching targets cannot relieve them — and embeddings keep rejecting outright since their failover is disabled. The exceeded rule now travels in the gateway error's unwrap chain (never serialized) so admission can inspect its scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ound - Memoize the Models-page gauge indicator state per rules-list generation: Alpine evaluates the class and label bindings several times per row on every render, each previously rescanning the full rules list. - Count the Models-page rate-limit editor and inspector in overlayDialogOpen() so the modal-open treatment applies there too. - Label the rate-limit editor dialog via aria-labelledby on its heading so assistive tech announces the Edit/Create title the UI shows. - Extract the Mongo bulk-upsert error decision into classifyBulkWriteError and pin its contract (success, config-shadowing skip, manual-race retry, real failure) with a table test. Skipped: replacing the JSON.stringify assertions with assert.deepEqual — the tests use node:assert/strict where deepEqual is prototype-sensitive, and vm.runInContext objects carry foreign prototypes, so the suggested change fails exactly the way these tests originally did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds rate limiting with scoped rules: every rule targets a
user_pathsubtree (consumer control), a provider instance (upstream protection), or a model — capping requests, tokens, and in-flight concurrency per sliding window. Complements budgets (spend control) with traffic control. Spec with the underlying gateway research (LiteLLM, Bifrost, Portkey, Kong, Cloudflare, Helicone) lives indocs/dev/2026-07-05_rate-limiting-spec.md; user docs indocs/features/rate-limits.mdx.User-visible impact
max_requestsand/ormax_tokensperminute/hour/day/custom window, plus aconcurrentperiod for in-flight caps. A rule owns one shared counter for its whole subject: a/teamrule covers the subtree (per-key limits = bind each managed key to its ownuser_path), a provider rule covers every consumer and model routed to that provider.user_pathover its limit gets 429 with an OpenAI-shaped error (code: rate_limit_exceeded), an accurate sliding-windowRetry-After, andx-ratelimit-{limit,remaining,reset}-{requests,tokens}headers from the most-constrained matching rule — OpenAI SDK backoff works out of the box. A saturated provider or model is treated like an unavailable target instead: virtual-model load balancing and failover route around it while capacity exists elsewhere, and only direct requests with no viable alternative get 429.openai/gpt-4opins one provider's model; baregpt-4ocaps the model across every provider. Matching is case-insensitive./admin/rate-limits(GET/PUT/DELETE +reset-one/reset; requests takescope+subject, withuser_pathas shorthand), arate_limits:YAML block (user_paths/providers/models), and env vars:SET_RATE_LIMIT_<PATH>="rpm=100,tpm=50000,rpd=10000,concurrent=10"andSET_PROVIDER_RATE_LIMIT_<NAME>="rpm=500"(suffix underscores become hyphens like provider-instance env vars; model rules are YAML/admin-only). Config-sourced rules are read-only in the dashboard; manual edits win over config seeds (budget lifecycle).RATE_LIMITS_ENABLEDdefaults totrueand is a no-op until rules exist, so existing deployments see zero behavior change. Pre-scoperate_limitstables/documents from earlier builds of this branch migrate in place at startup on SQLite/Postgres/Mongo.Design notes / limitations (documented)
USAGE_ENABLED=true(startup warns otherwise) and one request can overshoot a token window — the same tradeoff Kong and pre-v3 LiteLLM make.Catalog.ModelAvailabledecorator for virtual-model target selection (same skip semantics as stale provider inventory) and aRouteGatein the failover sweep. Failed failover attempts are not request-counted (known undercount; spec future work).rate_limitsstore on SQLite/Postgres/Mongo. Redis-backed counters, per-(path×model) matrix rules, and token pre-reservation are scoped as future work in the spec.SET_BUDGET_*/SET_RATE_LIMIT_*/SET_PROVIDER_RATE_LIMIT_*env appliers share a generic keyed merge helper.min=1vs value0); the form is nownovalidateand scope/period switches clear stale hidden fields.Testing
RouteAvailableprobe, failover skip viaRouteGate, stores (round-trip, config-replace, manual-wins-over-config, pre-scope schema migration), config parsing/validation (provider env, underscore→hyphen suffixes), server enforcement (429 shape, headers, release-on-completion), admin CRUD/reset for all scopes.tests/e2e/ratelimit_test.go): RPM blocking with header assertions, token-limit blocking through the real usage pipeline, concurrency slot hold/release, provider-scope counter shared across consumer paths, admin endpoints; blocked requests verified to never reach the upstream provider./admin/rate-limits.🤖 Generated with Claude Code
Summary by CodeRabbit
user_path,provider, andmodelvia YAML and environment variables, including request/token caps (token limits require usage tracking) and in-flight concurrency limits.Retry-Afterandx-ratelimit-*headers.