fix(proxy): dispatch Model Groups across passthrough endpoints (#471) - #472
Merged
Conversation
The Anthropic Messages handler resolved the requested Model and called resolve_provider_key on it directly. For a routing model (Model Group) that has no provider_key_id, so any /v1/messages request targeting a group failed with 400 "has no provider_key_id (routing models can't be dispatched directly)" — while the same group worked on /v1/chat/completions. Extract the routing-target resolution chat.rs already had (pick_targets + health filter) into routing::resolve_attempt_models and reuse it in messages.rs. The Messages handler now walks routing.targets, dispatching each target through the right path by its provider — Anthropic passthrough or cross-provider translation — so a group mixing Anthropic and OpenAI providers serves correctly over /v1/messages. Streaming attempts the first target only (matching chat completions); non-streaming fails over across targets on retryable upstream failures. Adds DP E2E coverage for a mixed-provider group over /v1/messages (Anthropic target, OpenAI target, and cross-protocol failover); all three fail before the change and pass after.
|
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 (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR extends the routing and failover system to support the ChangesModel Group Routing and Failover for /v1/Messages
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aisix-proxy/src/messages.rs (1)
548-580:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep streaming
UsageEvent.model_idaligned with non-streaming.These callbacks capture the concrete target id, while the non-streaming and error paths emit the requested model's id from
messages(). For a model group, the same alias will land under differentmodel_ids depending onstream, which makes/v1/messagesusage attribution inconsistent. Thread the outer virtual model id into these callbacks and keep onlyprovider_key_id/upstream_modeltarget-specific.Suggested fix
async fn dispatch( state: &ProxyState, auth: &AuthenticatedKey, body: &mut Value, request_id: &str, started: Instant, ) -> Result<DispatchOutcome, ProxyError> { @@ if is_stream { return dispatch_to_target( state, &snapshot, body, &attempt_models[0], + &model_entry.id, &model_name, request_id, started, &auth.entry.id, auth.key().team_id.clone(), @@ async fn dispatch_to_target( state: &ProxyState, snapshot: &aisix_core::AisixSnapshot, body: &Value, target: &crate::routing::AttemptModel, + virtual_model_id: &str, model_name: &str, request_id: &str, started: Instant, api_key_id: &str, team_id: Option<String>, @@ - let model_id_c = model_id.to_string(); + let model_id_c = virtual_model_id.to_string(); @@ - let model_id_for_telem = model_id.to_string(); + let model_id_for_telem = virtual_model_id.to_string();Also applies to: 800-828
🤖 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/messages.rs` around lines 548 - 580, The streaming callback currently captures the concrete target model id (model_id_c) which makes UsageEvent.model_id diverge from the non-streaming/messages() path; update the closure passed to build_anthropic_passthrough_stream so it captures and uses the outer virtual model id (e.g., create virtual_model_id_c from the outer requested model id) and pass that virtual_model_id_c into emit_anthropic_usage_event instead of model_id_c, while still leaving provider_key_id_c and upstream_model_c as the target-specific values; apply the same change to the other streaming callbacks in the same file (the block around the 800-828 range) so streaming and non-streaming usage attribution use the same model_id.
🤖 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-proxy/src/messages.rs`:
- Around line 343-373: The health tracking is using the caller-facing model_name
instead of the concrete backend target; update the dispatch calls
(cross_provider_dispatch and anthropic_passthrough_dispatch) to pass
model.display_name (or a dedicated resolved_target_name parameter) in place of
model_name for health/health-update purposes, while keeping model_name unchanged
for response rewriting/telemetry; adjust the parameter list and any downstream
health-update usage to read the new resolved name (e.g., resolved_target_name or
model.display_name) so health is recorded against the actual backend target that
served the request.
In `@crates/aisix-proxy/src/routing.rs`:
- Around line 186-222: The code decides "cooldown beats unhealthy" using
runtime_status.status_with_stale but then re-reads runtime_status inside the
closure passed to .filter, which can race and yield an empty selection; fix by
reading runtime status once and reusing those results when building the filtered
list: before the if unhealthy_count < attempts.len() &&
!cooldown_only.is_empty() compute a local map (or set) of attempt.id -> status
(using runtime_status.status_with_stale or
runtime_status.should_skip_for_routing once per attempt) or simply filter from
the already-collected cooldown_only/healthy vectors, then use that precomputed
status map (refer to runtime_status.status_with_stale,
runtime_status.should_skip_for_routing, attempts, unhealthy_count,
cooldown_only) to build the filtered Vec<AttemptModel> so no further
runtime_status queries occur inside the filter closure.
In `@tests/e2e/src/cases/messages-model-group-e2e.test.ts`:
- Around line 292-306: The test currently assumes exact request-count deltas for
OpenAI and Anthropic in the mg-failover scenario which can flake; update the
assertion logic in tests/e2e/src/cases/messages-model-group-e2e.test.ts (around
waitUntilGroupRoutable, callMessages and the openaiDown/anthropicGood baselines)
to be more robust by either retrying callMessages until the expected response is
observed or relaxing the checks from strict equality to a minimum (e.g.,
expect(... - baseline).toBeGreaterThanOrEqual(1)) so transient extra probes
won't fail the test.
---
Outside diff comments:
In `@crates/aisix-proxy/src/messages.rs`:
- Around line 548-580: The streaming callback currently captures the concrete
target model id (model_id_c) which makes UsageEvent.model_id diverge from the
non-streaming/messages() path; update the closure passed to
build_anthropic_passthrough_stream so it captures and uses the outer virtual
model id (e.g., create virtual_model_id_c from the outer requested model id) and
pass that virtual_model_id_c into emit_anthropic_usage_event instead of
model_id_c, while still leaving provider_key_id_c and upstream_model_c as the
target-specific values; apply the same change to the other streaming callbacks
in the same file (the block around the 800-828 range) so streaming and
non-streaming usage attribution use the same model_id.
🪄 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: 64787718-0ba8-40c1-8666-7c03556d82cb
📒 Files selected for processing (5)
crates/aisix-proxy/src/chat.rscrates/aisix-proxy/src/messages.rscrates/aisix-proxy/src/routing.rsdocs/configuration/routing-and-failover.mdtests/e2e/src/cases/messages-model-group-e2e.test.ts
…re-read Address review on #472: - /v1/messages dispatch recorded display-name-keyed health against the caller-facing model name. For a routing group that is the alias, not the target that served the request, so the served target's health was never updated. Record against the resolved target's display_name (the id-keyed runtime_status was already correct), matching chat.rs. - filter_attempt_models re-read runtime_status inside the cooldown branch's filter closure after classifying from an earlier read. A state flip between the two reads could yield an empty Selected list, which streaming callers turn into a panic by indexing attempt_models[0]. Reuse the single classification read — with no healthy candidates, the non-unhealthy set is exactly cooldown_only.
…471) The same routing gap fixed for /v1/messages also affected the other two provider-restricted passthrough endpoints. A routing model has no `provider`, so the OpenAI-only / Anthropic-only gate in these handlers rejected any Model Group with a 400 before walking `routing.targets`: - /v1/responses → "model … is not an OpenAI provider" - /v1/messages/count_tokens → "model … is not an Anthropic provider" Make both routing-aware via the shared routing::resolve_attempt_models: resolve the attempt list, then attempt the targets whose provider matches the endpoint, in order. Non-streaming responses fail over across matching targets on retryable upstream failures; streaming /v1/responses attempts the first matching target only. A group with no matching-provider target still returns the endpoint's 400 (e.g. a count_tokens group with no Anthropic target). E2E: extend the model-group suite with an Anthropic group over /v1/messages/count_tokens and an OpenAI group over /v1/responses; both fail before this change (400) and pass after.
jarvis9443
changed the title
This was referenced Jun 2, 2026
Open
This was referenced Jun 9, 2026
This was referenced Jul 1, 2026
This was referenced Jul 10, 2026
Merged
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.
Problem
A Model Group (routing model) accessed through the Anthropic / OpenAI passthrough endpoints failed instead of dispatching:
/v1/messages→400 model "<MG>" has no provider_key_id (routing models can't be dispatched directly)/v1/responses→400 model "<MG>" is not an OpenAI provider/v1/messages/count_tokens→400 model "<MG>" is not an Anthropic providerAll three resolved the requested Model and dispatched it directly (or applied a provider gate) without walking
routing.targets. A routing model has noprovider/provider_key_id, so it never reached target resolution. The same group worked on/v1/chat/completions, which already walks targets. Reported by a customer whose group mixed an Anthropic and an OpenAI provider.Fix
chat.rsalready had (pick_targets+ health/cooldown filter) intorouting::resolve_attempt_models, shared by all handlers./v1/messages: resolve the attempt list and dispatch each target by its provider — Anthropic passthrough or cross-provider translation. A group mixing Anthropic and OpenAI targets now serves correctly./v1/responses(OpenAI-only) and/v1/messages/count_tokens(Anthropic-only): resolve the attempt list, then attempt the targets whose provider matches the endpoint, in order. A group with no matching-provider target still returns the endpoint's 400.retry_on_429honored); streaming attempts the first matching target only (matching chat completions).display_name(the served target), not the group alias — the id-keyed runtime status was already correct.Behavior change
/v1/messages,/v1/responses, and/v1/messages/count_tokensinstead of returning 400. Direct (non-routing) models are unchanged — their dispatch paths and error messages are preserved.Tests
messages-model-group-e2e.test.ts): mixed-provider group over/v1/messages(Anthropic target, OpenAI target via cross-provider, cross-protocol failover), an Anthropic group over/v1/messages/count_tokens, and an OpenAI group over/v1/responses. Each case fails before the change (400) and passes after.aisix-proxyunit tests (356) and the full DP e2e suite (60 files / 121 tests) pass.Docs
routing-and-failover.md: routing aliases work across all four passthrough endpoints, including mixed-provider groups, the provider-restricted behavior of/v1/responses+/v1/messages/count_tokens, and the streaming-first-target caveat.Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Documentation
Tests