Uh oh!
There was an error while loading. Please reload this page.
fix(routing): honour routing.retries on the streaming chat path - #807
Merged
Conversation
`/v1/chat/completions` dispatches streaming and non-streaming requests through two separately-written loops in `chat.rs`. The non-streaming one wraps each target in `for attempt_idx in 0..=retries` with exponential backoff; the streaming one never read `routing.retries` at all — it walked `attempt_models` once and, on a retryable failure, went straight to the next target. With a single target it just failed the request. `/v1/messages` and `/v1/responses` don't have this gap: both run streaming and non-streaming through one loop that already honours `retries`. This restores the same semantics for the streaming chat path: same-target retry with backoff first, fail-over only once `retries` is exhausted. Per-attempt telemetry now classifies the extra attempts as `retry` (the records operators expect between `initial` and `fallback`), and the attempt-failed warning carries `target_attempt` like the non-streaming one. Target-invariant setup (`model_arc` / `pk_arc` / `BridgeContext` / stream budget) moves above the retry loop; the per-attempt reservation, telemetry record, and dispatch stay inside it, matching the non-streaming loop's structure. Tests: two unit tests pinning the attempt sequence (multi-target `initial → retry → fallback`, and single-target retries) plus a streaming case in the retry-backoff E2E asserting the same upstream hit count and backoff floor as the non-streaming one. All three fail before this change. Fixesapi7/AISIX-Cloud#1119Fixesapi7/AISIX-Cloud#1122
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughStreaming chat routing now honors ChangesStreaming routing retries
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StreamingDispatch
participant UpstreamTarget
participant RoutingTelemetry
Client->>StreamingDispatch: stream chat completion
StreamingDispatch->>UpstreamTarget: initial attempt
UpstreamTarget-->>StreamingDispatch: retryable failure
StreamingDispatch->>RoutingTelemetry: record failed attempt
StreamingDispatch->>UpstreamTarget: retry same target
UpstreamTarget-->>StreamingDispatch: failure
StreamingDispatch->>UpstreamTarget: fallback target attempt
UpstreamTarget-->>StreamingDispatch: stream response or failure
StreamingDispatch-->>Client: SSE completion or error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
jarvis9443
commented
Jul 23, 2026
ContributorAuthor
@coderabbitai review |
✅ Action performedReview finished.
|
jarvis9443
commented
Jul 23, 2026
ContributorAuthor
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 42 minutes. |
jarvis9443
commented
Jul 23, 2026
ContributorAuthor
@coderabbitai full review |
✅ Action performedFull review finished. |
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
/v1/chat/completionsdispatches streaming and non-streaming requests through two separately-written loops incrates/aisix-proxy/src/chat.rs.The non-streaming loop wraps each target in
for attempt_idx in 0..=retrieswith exponential backoff. The streaming loop never readrouting.retriesat all — it walkedattempt_modelsonce and, on a retryable failure, went straight to the next target:With a single target there is nothing to fall over to, so one transient upstream error failed the request outright —
retrieswas the only thing that could have saved it./v1/messagesand/v1/responsesdon't have this gap: both run streaming and non-streaming through one loop (dispatch_to_targetbranches internally) that already honoursretries. The streaming chat path was the only one that had drifted.Change
Nest the same-target retry loop inside the streaming target walk, mirroring the non-streaming loop:
routing.retries(default0, so behaviour is unchanged for anyone not configuring it);retry_backoff, exponential + jitter) and re-hit the same target untilretriesis exhausted, then fall over;model_arc/pk_arc/BridgeContext/ stream budget) hoists above the retry loop; the per-attempt rate-limit reservation, telemetry record, and dispatch stay inside it.Behaviour change
Per-attempt telemetry now emits a
retry-kind record for each extra same-target attempt, so the Logs view showsinitial → retry #1 → fallbackinstead ofinitial → fallback. The attempt-failed warning gainstarget_attempt, matching the non-streaming log line.Only requests on a routing model with
retries > 0are affected.retriesis read from the routing block, so a direct (non-group) model is still a single attempt.Tests
streaming_routing_honors_same_target_retries— multi-target group,retries=1, always-502 primary: asserts the primary is hit twice and the attempt sequence isinitial → retry → fallback.streaming_routing_retries_single_target_before_failing— single target,retries=2: asserts three attempts all on that target before the request fails.retry-backoff-e2e.test.tsgains astream: truecase asserting the same upstream hit count (3) and backoff floor as the existing non-streaming case.All three fail before this change and pass after. Full
aisix-proxysuite: 662 passed.Fixes api7/AISIX-Cloud#1119
Fixes api7/AISIX-Cloud#1122
Summary by CodeRabbit
New Features
Bug Fixes