Uh oh!
There was an error while loading. Please reload this page.
feat(routing): fallback_on_statuses — opt selected upstream statuses into retry/failover - #752
Merged
Merged
Conversation
…into retry/failover Some providers use non-429 4xx codes for transient conditions (model overload, queue full, quota exhaustion). The gateway's default treats any non-429 4xx as the caller's error and relays it, so a model group fails a request its other targets could have served (AISIX-Cloud#1012). Routing models gain an explicit opt-in list: routing: strategy: failover targets: [...] fallback_on_statuses: [408, 409, 422] A listed status becomes retryable in is_retryable() — the single predicate every dispatch loop consults — so it participates in both same-target retries and failover, exactly like retry_on_429 does for 429. Unset means unchanged behavior. 5xx codes are already retryable; listing them is a no-op. The list never affects non-status failures (customer-fixable config/credentials stay terminal). Schema constrains entries to 400-599. Threaded through all six dispatch call sites (chat streaming + non-streaming, messages, responses, count_tokens). Per-attempt telemetry already records each attempt's upstream status and kind (initial/retry/fallback), so a rule-triggered failover is attributable from existing fields. Control-plane exposure (cp-admin schema, validation, projection, dashboard) ships as the paired AISIX-Cloud PR; this changes the DP first so the CP projection lands against a DP that accepts the field (deny_unknown_fields). Ref api7/AISIX-Cloud#1012
📝 WalkthroughWalkthroughAdds optional ChangesConfigurable fallback status routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ProxyDispatch
participant RoutingConfig
participant Upstream
Client->>ProxyDispatch: send request
ProxyDispatch->>RoutingConfig: read fallback_on_statuses
ProxyDispatch->>Upstream: try primary target
Upstream-->>ProxyDispatch: 422 upstream error
ProxyDispatch->>ProxyDispatch: classify status as retryable
ProxyDispatch->>Upstream: try fallback target
Upstream-->>ProxyDispatch: successful response
ProxyDispatch-->>Client: return response
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…ange at the validator Audit follow-ups: the six dispatch sites now borrow the configured slice instead of cloning per request, and a schema-validation test pins that out-of-range entries are rejected while in-range lists pass.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aisix-proxy/src/chat.rs (1)
1101-1114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate routing-config extraction pattern; consider a
Model-level helper.
fallback_statusesis derived with the samevirtual_entry.value.routing.as_ref().map(|r| r.fallback_on_statuses_or_default()).unwrap_or(&[])boilerplate twice in this file (streaming and non-streaming paths), and the identical pattern (for bothretry_on_429and nowfallback_on_statuses) is repeated again incount_tokens.rs(and, per the graph context, inmessages.rs/responses.rs). A small convenience method onModel(e.g.fallback_on_statuses_or_default(&self) -> &[u16], mirroring the existingRoutingmethod) would collapse all these call sites to a single line and remove the risk of the branches drifting.♻️ Proposed helper (illustrative)
implModel{pubfnfallback_on_statuses_or_default(&self) -> &[u16]{self.routing.as_ref().map(|r| r.fallback_on_statuses_or_default()).unwrap_or(&[])}}Then each call site becomes
let fallback_statuses = virtual_entry.value.fallback_on_statuses_or_default();.Also applies to: 1822-1833
🤖 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/chat.rs` around lines 1101 - 1114, Introduce a Model-level helper, such as Model::fallback_on_statuses_or_default, that delegates to routing when present and otherwise returns an empty slice. Replace the repeated routing.as_ref().map(...).unwrap_or(&[]) extraction in both chat.rs paths and the corresponding count_tokens.rs, messages.rs, and responses.rs call sites with this helper, including the related retry configuration pattern where applicable.
🤖 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.
Nitpick comments:
In `@crates/aisix-proxy/src/chat.rs`:
- Around line 1101-1114: Introduce a Model-level helper, such as
Model::fallback_on_statuses_or_default, that delegates to routing when present
and otherwise returns an empty slice. Replace the repeated
routing.as_ref().map(...).unwrap_or(&[]) extraction in both chat.rs paths and
the corresponding count_tokens.rs, messages.rs, and responses.rs call sites with
this helper, including the related retry configuration pattern where applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cfffe33d-ca44-4bce-af17-40c31da81ac5
📒 Files selected for processing (10)
crates/aisix-core/src/models/routing.rscrates/aisix-core/src/models/schema.rscrates/aisix-proxy/src/chat.rscrates/aisix-proxy/src/count_tokens.rscrates/aisix-proxy/src/messages.rscrates/aisix-proxy/src/responses.rscrates/aisix-proxy/src/routing.rsschemas/resources/model.schema.jsonschemas/resources/routing.schema.jsontests/e2e/src/cases/fallback-on-statuses-e2e.test.ts
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Some providers use non-429 4xx codes for transient conditions — model overload, queue full, quota exhaustion. The default classification (
is_retryable) treats every non-429 4xx as the caller's error and relays it, so a model group fails a request that its other targets could have served. (AISIX-Cloud#1012)Change
Routing models gain an explicit, empty-by-default opt-in list:
A listed status becomes retryable in
is_retryable()— the single predicate every dispatch loop consults — so it participates in both same-target retries (retries) and failover (max_fallbacks), exactly likeretry_on_429does for 429 (429 in the list works withoutretry_on_429; the boolean stays for compatibility). Unset = behavior unchanged, bit-for-bit. 5xx are already retryable (listing them is a no-op); the list never affects non-status failures (customer-fixable config/credential errors stay terminal). Schema constrains entries to 400–599.Threaded through all six dispatch sites: chat streaming + non-streaming + cross-target break, messages, responses, count_tokens — the whole routing family in one PR.
Observability: per-attempt telemetry already records each attempt's upstream
status,error_classandattempt_kind(initial/retry/fallback), so a rule-triggered failover is fully attributable from existing fields; no new telemetry needed.Interaction note: this is the current-request knob. The pre-existing per-direct-model
cooldown.trigger_statusesbenches a target for future requests and stays an independent layer.Deliberately not included
error.code: Throttling-style matching as a possible extension): status-code opt-in covers the reported scenarios; body matching adds a parsing surface per provider and can be layered on later if a concrete provider needs it.Baseline: mainstream gateways retry 408/409/429/5xx by default and let users force retries per exception category; none offer per-status opt-in. We keep stricter defaults and add the more precise status-list control instead — providers in scope here signal overload with codes (e.g. 422) that category-based systems can't distinguish from validation errors.
Control-plane pairing
The CP is a closed schema — this field is unreachable by users until the paired AISIX-Cloud PR (cp-admin.yaml + validation + etcd projection + dashboard) lands. DP ships first because it rejects unknown fields (
deny_unknown_fields); the CP PR follows immediately and carries the closing reference for AISIX-Cloud#1012.Tests
retry_on_429; 5xx unaffected; non-status failures never resurrected; existing classification test updated to the new signature with&[](pinning default-unchanged).aisix+ etcd): two identical two-target groups — default group relays the first target's 422 without consulting the backup;fallback_on_statuses: [422]group fails over and succeeds on the backup; a 400 from a configured group (422-only list) still relays. Existingfallback/fallback-edgese2es pass unchanged.Ref api7/AISIX-Cloud#1012 (closing reference rides the CP PR so the issue doesn't close on the DP half)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests