fix(quota): match model conditions against the requested parent as well as the dispatched target - #957
Merged
Merged
Conversation
…ll as the dispatched target
A conditional rate-limit policy whose model condition names a routing
group (or whose model_name names the group's alias) never fired: the
request gate defers model-property policies to the per-target gate,
where the condition input carried only the dispatched member's identity
— the parent's id was compared nowhere.
The per-target condition input now carries the {dispatched target,
requested parent} pair for the model dimensions. A leaf is raw-true
when either identity satisfies its operator, and negate flips the
combined result — so a group-referencing condition selects exactly the
requests addressed to that group, and a negated one excludes them
instead of (absurdly) matching all of them. Reservation phases and
bucket values are unchanged: group_by [model] still splits per concrete
target, and classic scope:model rows (which already matched parent ids
at the request gate) now agree with the conditional form.
Wired through every group-capable dispatch loop (chat streaming +
non-streaming, /v1/messages, /v1/responses, /v1/messages/count_tokens)
and the ensemble panel/judge reservations; semantic routing shares the
chat loop. On an ensemble, a parent-referencing condition reserves per
sub-call (panel members + judge) — the parent-level per-request cap
remains the entry's own inline rate_limit.
Fixes api7/AISIX-Cloud#1267
📝 WalkthroughWalkthroughModel policies now match dispatched targets and addressed routing parents. Routing-parent identity flows through routed and ensemble quota reservations. Tests cover direct dispatch, routing groups, negation, failover, aliases, target bucketing, and multiple request endpoints. ChangesRouting-parent policy matching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatRequest
participant QuotaReservation
participant PolicyEvaluator
participant TargetModel
Client->>ChatRequest: submit request for routing parent
ChatRequest->>QuotaReservation: reserve routed target with RoutingParent
QuotaReservation->>PolicyEvaluator: evaluate target and parent conditions
PolicyEvaluator->>QuotaReservation: return policy match and target bucket
QuotaReservation->>TargetModel: continue dispatch or reject
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Updates conditional quota matching so routed requests consider both the requested parent model and dispatched target.
Changes:
- Adds parent-aware model condition evaluation across dispatch paths.
- Preserves concrete-target bucket grouping.
- Adds unit, schema, and E2E coverage.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/aisix-core/src/models/policy_conditions.rs |
Adds parent-aware condition evaluation. |
crates/aisix-proxy/src/quota.rs |
Threads parent identity into quota matching. |
crates/aisix-proxy/src/chat.rs |
Wires chat, semantic, and ensemble dispatch. |
crates/aisix-proxy/src/messages.rs |
Wires Messages dispatch. |
crates/aisix-proxy/src/responses.rs |
Wires Responses dispatch. |
crates/aisix-proxy/src/count_tokens.rs |
Wires token-count dispatch. |
crates/aisix-proxy/src/ensemble.rs |
Propagates ensemble parent identity. |
crates/aisix-proxy/src/lib.rs |
Updates quota test callers. |
schemas/resources/rate_limit_policy.schema.json |
Updates generated dimension descriptions. |
tests/e2e/src/cases/group-model-condition-ratelimit-e2e.test.ts |
Adds routing-group quota scenarios. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aisix-core/src/models/policy_conditions.rs (1)
475-524: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCombine routed
Nechecks with AND.
ConditionOperator::Neis valid forModelandModelName. The current OR combination makesmodel ~= <group-id>true for every request routed through that group because the target differs from the group. ApplyNeacross the pair astarget != value && parent != value, and add regression tests for both target and parent matches.🤖 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/policy_conditions.rs` around lines 475 - 524, Update the routed evaluation logic before applying negation so ConditionOperator::Ne requires both the target and optional routing parent to differ from the value, while existing operators retain their OR behavior. Use the visible leaf evaluation flow and add regression coverage for Model/ModelName cases where either the target or parent matches the excluded value.
🤖 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.
Outside diff comments:
In `@crates/aisix-core/src/models/policy_conditions.rs`:
- Around line 475-524: Update the routed evaluation logic before applying
negation so ConditionOperator::Ne requires both the target and optional routing
parent to differ from the value, while existing operators retain their OR
behavior. Use the visible leaf evaluation flow and add regression coverage for
Model/ModelName cases where either the target or parent matches the excluded
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05b8de19-1b0f-4624-88e1-d9a536be35fe
📒 Files selected for processing (10)
crates/aisix-core/src/models/policy_conditions.rscrates/aisix-proxy/src/chat.rscrates/aisix-proxy/src/count_tokens.rscrates/aisix-proxy/src/ensemble.rscrates/aisix-proxy/src/lib.rscrates/aisix-proxy/src/messages.rscrates/aisix-proxy/src/quota.rscrates/aisix-proxy/src/responses.rsschemas/resources/rate_limit_policy.schema.jsontests/e2e/src/cases/group-model-condition-ratelimit-e2e.test.ts
The two identities in the {target, parent} pair are distinct strings,
so combining ~= disjunctively made it vacuously true on every routed
request and broke the a ~= b == !(a == b) equivalence the operator
vocabulary (and the dashboard's operator normalization) relies on.
Positive operators keep the exists-reading; ~= now requires every
identity to differ.
Also extends the e2e to the streaming-chat and /v1/responses loops
per review.
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 `@tests/e2e/src/cases/group-model-condition-ratelimit-e2e.test.ts`:
- Around line 438-465: Restore the combined setup guard in the streaming test at
tests/e2e/src/cases/group-model-condition-ratelimit-e2e.test.ts lines 438-465 by
including seed and otlp alongside etcdReachable and app. Apply the same guard
update to the responses test at lines 467-489, preserving ctx.skip() and the
early return.
🪄 Autofix
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: aa90475e-d213-4670-95fc-5167cedfe96e
📒 Files selected for processing (2)
crates/aisix-core/src/models/policy_conditions.rstests/e2e/src/cases/group-model-condition-ratelimit-e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/aisix-core/src/models/policy_conditions.rs
The mock answers SSE or JSON per fixture, so the streaming case gets its own member + group and asserts the event-stream content type — the reservation now provably rides the streaming loop, not a JSON fallback.
This was referenced Aug 12, 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
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.
Problem
A conditional rate-limit policy whose
modelcondition names a routing group — the dashboard offers every model kind as a condition value — never fires. The request gate defers model-property policies to the per-target gate, and the per-target condition input carried only the dispatched member's identity, so the group's UUID was compared nowhere. The same held formodel_nameconditions naming the group's alias, and for ensemble/semantic parent entries. The policy saves, projects, and silently never matches.Fixes api7/AISIX-Cloud#1267
Change
The per-target condition input now carries the {dispatched target, requested parent} pair for the two model dimensions (
ConditionInput.routing_parent_model/_model_name):==/in/~~/~*) are raw-true when either identity satisfies them;~=is raw-true only when both differ (the two identities are distinct strings, so an ∃ reading of~=would be vacuously true on every routed request), preservinga ~= b≡!(a == b);negateflips the combined result, so!(model in [<group>])excludes requests addressed to the group;group_by: [model]still splits per concrete target. One counting nuance that predates this PR and is unchanged by it: window counts are consumed per reserved attempt, so an attempt that fails upstream after reserving still consumes shared-bucket quota during failover — the docs PR states this explicitly;scope: modelrows already matched parent ids at the request gate; the conditional form now agrees with them instead of contradicting them;Wired through every group-capable dispatch loop — chat (both streaming and non-streaming),
/v1/messages,/v1/responses,/v1/messages/count_tokens— and the ensemble panel/judge reservations via the sharedreserve_model_onlypath; semantic routing shares the chat loop. On an ensemble, a parent-referencing condition reserves per sub-call (panel members + judge); the parent-level per-request cap remains the entry's own inlinerate_limit.This also brings the condition semantics in line with how mainstream gateways key requested-model limits: caller-facing limits are keyed on the model name the client sent (the group alias), while per-deployment limits follow the concrete target — both are now expressible.
Behavior changes
model/model_nameconditions start matching (previously dead config).model ~= X/model_name ~= Xnow requires both the target and the requested parent to differ from X (previously only the target was compared).Tests
~=∀-semantics with the~= ≡ !(==)equivalence pinned across all three value positions, member-through-parent, no parent leak into identity dimensions.group_by [model]buckets stay on the target id.group-model-condition-ratelimit-e2e.test.ts, real binary + etcd + mock upstream), 7 cases: the reported scenario (team ∧ group-id, per-member split, 429 with policy attribution), group-aliasmodel_name, negation exclusion, member-id failover + shared bucket with the direct alias, and/v1/messages+ streaming-chat (real SSE fixture) +/v1/responsessiblings. 6 of 7 fail on main (the member-id regression guard passes, as it should); all 7 pass with the fix. The existing multidim suite — including the case pinning "the group parent never burns a per-target bucket" — is unchanged and green.Generated
schemas/resources/rate_limit_policy.schema.jsonpicks up the new dimension descriptions. The cp-admin.yaml description update and user-facing docs (EN/ZH) ship in the paired CP/docs PRs.🤖 Generated with Claude Code