feat(ratelimit): scheduled suspension windows for rate limit policies - #876
Conversation
A RateLimitPolicy can now carry `schedules` — recurring wall-clock windows (weekly days or explicit dates, HH:MM range, IANA timezone) during which the policy is suspended: the quota gate skips it and enforcement resumes automatically when the window closes. Windows are a union; cross-midnight windows belong to their start day; the bucket key never changes, so suspension does not reset the surrounding window's counts. Both policy-iteration sites honor suspension (reserve_layers and the routing/ensemble reserve_model_only path). Evaluation converts UTC to the schedule's local wall clock (total conversion — DST-safe), and any malformed field fails toward enforcing. Rows without the field keep enforcing unchanged; cp-api omits the field when empty so schedule-less rows stay parseable by pre-`schedules` strict data planes. A policy that does carry schedules requires data planes on this version (older strict loaders drop the whole row). LiteLLM has no equivalent to compare against: its rate limits are inline key/team/user fields with no standalone policy object and no time-based scheduling.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 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: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesScheduled policy suspension
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant QuotaReservation
participant RateLimitPolicy
participant PolicySchedule
Client->>QuotaReservation: submit request
QuotaReservation->>RateLimitPolicy: evaluate current UTC time
RateLimitPolicy->>PolicySchedule: check suspension schedule
PolicySchedule-->>RateLimitPolicy: matching or non-matching result
RateLimitPolicy-->>QuotaReservation: suspended_at result
QuotaReservation-->>Client: allow request or enforce quota
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (5)
crates/aisix-core/src/models/rate_limit_policy.rs (3)
162-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging once when a schedule field fails to parse.
An unparseable
timezone,start_time, orend_timemakes the window never match. The direction is safe, because the policy keeps enforcing. The operator receives no signal, so a typo such asAsia/Shangailooks identical to a correctly configured schedule that is simply outside its window.The data plane is a hot path, so log at most once per distinct policy rather than per evaluation.
quota.rsalready carries this pattern for the ignoredmax_tokenswarning.This is optional. The current behavior is correct.
🤖 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 `@crates/aisix-core/src/models/rate_limit_policy.rs` around lines 162 - 171, Optionally add once-per-policy warning logging to the matches method for invalid timezone, start_time, or end_time values, following the existing max_tokens warning pattern in quota.rs. Preserve the current false-return behavior and ensure repeated evaluations do not emit duplicate warnings for the same policy.
237-243: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
suspended_atre-parses every schedule string on every call.
matchesparsestimezonethroughchrono_tz::Tz::from_strand both time fields on each invocation.reserve_layerscallssuspended_atfor every policy in the snapshot on every request, so the cost scales with policies × schedules × request rate.The parse itself is cheap, so this is not urgent. Two options reduce it:
- Call
suspended_atonly for policies that already matched their scope. See the related comment oncrates/aisix-proxy/src/quota.rs.- Parse
timezoneand the times once at deserialization into typed fields, and keep theStringfields for serialization.The second option removes the repeated parse entirely and makes the malformed-input case detectable at load time.
🤖 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 `@crates/aisix-core/src/models/rate_limit_policy.rs` around lines 237 - 243, Update the schedule deserialization and matching flow around RateLimitPolicy::suspended_at and the schedule type so timezone and time strings are parsed once into typed fields during deserialization, while retaining the existing String fields for serialization. Make matches reuse those parsed values instead of reparsing on each call, and ensure malformed schedule input is rejected during loading.
391-406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a DST-boundary case to pin the documented DST-safety claim.
matchesdocuments that UTC → local conversion is total and free of DST ambiguity. No test crosses a DST boundary.same_instant_differs_by_timezoneusesAmerica/New_Yorkonly for the weekday offset.Two cases are worth pinning, because a future refactor to local → UTC comparison would break them silently:
- Fall-back: local 01:30 occurs twice. A
[01:00, 02:00)window must match on both UTC instants.- Spring-forward: local 02:30 does not exist. A
[02:00, 03:00)window must match no instant on that date.💚 Proposed test covering both DST transitions
#[test] fn dst_transitions_evaluate_from_the_local_wall_clock() { // US DST 2026: forward Mar 8, back Nov 1 (both at 02:00 local). let p = policy_with_schedules(json!([{ "timezone": "America/New_York", "days_of_week": ["sun"], "start_time": "01:00", "end_time": "02:00" }])); // Fall back: 01:30 local happens twice (EDT -04:00, then EST -05:00). assert!(p.suspended_at(at("2026-11-01T05:30:00Z")), "01:30 EDT"); assert!(p.suspended_at(at("2026-11-01T06:30:00Z")), "01:30 EST"); // Spring forward: 01:30 local still exists; 02:30 local never does, // so the instant that would carry it falls outside the window. assert!(p.suspended_at(at("2026-03-08T06:30:00Z")), "01:30 EST"); assert!(!p.suspended_at(at("2026-03-08T07:30:00Z")), "03:30 EDT"); }🤖 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 `@crates/aisix-core/src/models/rate_limit_policy.rs` around lines 391 - 406, Add a DST transition test alongside same_instant_differs_by_timezone, using the America/New_York schedule and suspended_at: verify a [01:00, 02:00) Sunday window matches both fall-back 01:30 UTC instants, matches spring-forward 01:30, and rejects the corresponding nonexistent 02:30/03:30 instant.tests/e2e/src/cases/ratelimit-e2e.test.ts (1)
277-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport the observed status when a propagation wait times out.
Both suspension transitions assert through
waitConfigPropagation, so noexpectruns on the final state. If a transition never lands, the failure message iswaitConfigPropagation: condition not met within 30000ms. It does not say which status the proxy returned, so CI triage cannot tell a propagation stall from a wrong status.Record the last observed status and assert on it after the wait.
♻️ Proposed change to surface the observed status
+ let observed = -1; + const settled = async (want: number): Promise<boolean> => { + observed = await callStatus(); + return observed === want; + }; + // Enter a suspension window → propagation lands when a call passes // again. Suspended probes reserve nothing, so counts stay intact. await seed.update( "rate_limit_policies", SCHED_POLICY_ID, policyDoc(alwaysOn), ); - await waitConfigPropagation(async () => (await callStatus()) === 200); + await waitConfigPropagation(() => settled(200)).catch(() => { + throw new Error(`suspended policy still gated requests; last status ${observed}`); + }); // Leave the window (schedule no longer matches). The bucket still // holds the burned slot from this minute, so enforcement resumes // as 429 — suspension must not reset quotas. await seed.update( "rate_limit_policies", SCHED_POLICY_ID, policyDoc(neverOn), ); - await waitConfigPropagation(async () => (await callStatus()) === 429); + await waitConfigPropagation(() => settled(429)).catch(() => { + throw new Error( + `policy did not resume enforcement on the burned bucket; last status ${observed}`, + ); + });🤖 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 `@tests/e2e/src/cases/ratelimit-e2e.test.ts` around lines 277 - 287, Update the suspension transition checks around waitConfigPropagation and callStatus to record the last observed HTTP status during polling, then assert that recorded status after each wait. Preserve the existing expected values of 200 and 429 while ensuring timeout failures report the observed status.crates/aisix-proxy/src/quota.rs (1)
180-188: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEvaluate
suspended_atafter the scope match, asreserve_model_onlydoes.The suspension check runs for every policy in the snapshot, before
appliesnarrows the set. Policies belonging to other keys, teams, members, and models therefore pay a timezone parse and schedule scan on every request, then get discarded two lines later.
reserve_model_onlyat lines 329-332 places the same check last, sosuspended_atruns only whenscopeandscope_refalready match. Match that order here.suspended_athas no side effects, so the behavior is identical.♻️ Proposed reordering
for entry in snap.rate_limit_policies.entries() { let policy = &entry.value; - // Inside a scheduled suspension window the policy reserves - // nothing; enforcement resumes automatically when the window - // closes, on the unchanged bucket (AISIX-Cloud#1104). - if policy.suspended_at(now) { - continue; - } let applies = match policy.scope {if !applies { continue; } + // Inside a scheduled suspension window the policy reserves + // nothing; enforcement resumes automatically when the window + // closes, on the unchanged bucket (AISIX-Cloud#1104). + if policy.suspended_at(now) { + continue; + } let rl = policy_to_rate_limit(policy);🤖 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 `@crates/aisix-proxy/src/quota.rs` around lines 180 - 188, Reorder the policy loop so the scope and scope_ref matching performed by applies runs before the suspended_at check. In the snapshot enforcement flow around suspended_at, skip non-matching policies first, then evaluate suspension only for applicable policies, matching the ordering used by reserve_model_only.
🤖 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 `@crates/aisix-core/src/models/rate_limit_policy.rs`:
- Around line 106-136: Update the PolicySchedule documentation to describe
start_time < end_time as a same-day window, start_time > end_time as a
midnight-crossing window owned by the start day, and start_time == end_time as
an empty window that never matches. Remove the inline comment in matches() that
incorrectly claims equal times are rejected by write-path validation.
---
Nitpick comments:
In `@crates/aisix-core/src/models/rate_limit_policy.rs`:
- Around line 162-171: Optionally add once-per-policy warning logging to the
matches method for invalid timezone, start_time, or end_time values, following
the existing max_tokens warning pattern in quota.rs. Preserve the current
false-return behavior and ensure repeated evaluations do not emit duplicate
warnings for the same policy.
- Around line 237-243: Update the schedule deserialization and matching flow
around RateLimitPolicy::suspended_at and the schedule type so timezone and time
strings are parsed once into typed fields during deserialization, while
retaining the existing String fields for serialization. Make matches reuse those
parsed values instead of reparsing on each call, and ensure malformed schedule
input is rejected during loading.
- Around line 391-406: Add a DST transition test alongside
same_instant_differs_by_timezone, using the America/New_York schedule and
suspended_at: verify a [01:00, 02:00) Sunday window matches both fall-back 01:30
UTC instants, matches spring-forward 01:30, and rejects the corresponding
nonexistent 02:30/03:30 instant.
In `@crates/aisix-proxy/src/quota.rs`:
- Around line 180-188: Reorder the policy loop so the scope and scope_ref
matching performed by applies runs before the suspended_at check. In the
snapshot enforcement flow around suspended_at, skip non-matching policies first,
then evaluate suspension only for applicable policies, matching the ordering
used by reserve_model_only.
In `@tests/e2e/src/cases/ratelimit-e2e.test.ts`:
- Around line 277-287: Update the suspension transition checks around
waitConfigPropagation and callStatus to record the last observed HTTP status
during polling, then assert that recorded status after each wait. Preserve the
existing expected values of 200 and 429 while ensuring timeout failures report
the observed status.
🪄 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: CHILL
Plan: Pro
Run ID: 4b42ba04-7356-4198-8630-3bb9b2d5ae3c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcrates/aisix-core/Cargo.tomlcrates/aisix-core/src/models/rate_limit_policy.rscrates/aisix-core/src/models/schema.rscrates/aisix-proxy/src/lib.rscrates/aisix-proxy/src/quota.rsschemas/resources/rate_limit_policy.schema.jsontests/e2e/src/cases/ratelimit-e2e.test.ts
CodeRabbit review: start==end is an empty window, not a write-path rejection on the DP side (only cp-api validates the shape); spell out the three comparisons in the struct docs the schema description inherits.
1 participant
What
Adds an optional
schedulesfield toRateLimitPolicy: recurring wall-clock windows during which the policy is suspended (not enforced). Enforcement resumes automatically when the window closes — no state machine, suspension is a pure predicate over the current time.Ref api7/AISIX-Cloud#1104 (customer need: turn rate limiting off on weekends/holidays and during workday off-peak hours, restoring automatically).
{ "name": "team-quota", "scope": "team", "scope_ref": "…", "window": "minute", "max_requests": 100, "schedules": [ { // workday off-peak: 22:00 → next morning 09:00 "timezone": "Asia/Shanghai", "days_of_week": ["mon","tue","wed","thu","fri"], "start_time": "22:00", "end_time": "09:00" }, { // weekends, all day "timezone": "Asia/Shanghai", "days_of_week": ["sat","sun"], "start_time": "00:00", "end_time": "24:00" }, { // holidays: explicit dates "timezone": "Asia/Shanghai", "dates": ["2026-10-01","2026-10-02"], "start_time": "00:00", "end_time": "24:00" } ] }Semantics
days_of_weekor by explicitdates(holidays); the schema's injectedoneOfenforces exactly one selector.start_time/end_timeareHH:MMwall clock in the entry's IANAtimezone;24:00is a valid end.end ≤ startcrosses midnight and the window belongs to its start day (fri 22:00→09:00covers Fri 22:00 – Sat 09:00; cover Sunday nights by addingsun).reserve_layers(all endpoints) andreserve_model_only(routing/ensemble per-target reservations).Compatibility
schedulesare byte-identical to today and keep enforcing on every DP version (serde default + omit-when-empty on the write side).schedulesrequires DPs on this version: older strict loaders (deny_unknown_fields, pre-feat(config): lenient etcd parsing with tri-state compatibility reporting #872) drop the whole row, i.e. that policy would not be enforced at all on not-yet-upgraded DPs. Called out for the docs; the CP-side PR documents the same.LiteLLM baseline: no equivalent exists (inline key/team/user rpm/tpm fields only — no standalone policy object, no time-based scheduling), so there is no behavior to align with.
Tests
24:00, union, malformed-entry conservatism, old-row deserialization, empty-schedulesserialization omission.tests/e2e, real binary + etcd): pre-schedulesrow enforces → entering a window pauses → leaving it resumes as 429 within the same minute window, proving counter continuity through the toggle.Summary by CodeRabbit
New Features
Tests
Fixes api7/AISIX-Cloud#1104