Uh oh!
There was an error while loading. Please reload this page.
feat(proxy): wire Hub::dispatch_two_tier with legacy fallback (Phase D cutover) - #305
Conversation
…D cutover) Adds `crate::dispatch::resolve_bridge` — a small helper that tries the two-tier dispatch path (specialized vendor → adapter family, both new in PR #300) first, then falls back to the legacy `Provider`-keyed registry when neither tier matches. The fallback exists because today's on-disk `ProviderKey` payloads carry `provider: ""` + `adapter: None` (the new fields ship empty until cp-api's B3 sub-PR populates them) — the two-tier path therefore returns `None` on every existing key and the legacy registry continues serving traffic unchanged. After B3 ships and cp-api re-projects every `ProviderKey` with the new `provider` + `adapter` fields filled, the two-tier path will start returning bridges and the legacy fallback becomes the residual safety net. Once we're confident the cutover is complete, a follow-up PR can delete the legacy `Hub::register(Provider, _)` registrations and the fallback branch. Changes: - `dispatch.rs`: add `pub(crate) fn resolve_bridge(hub, pk, provider) -> Option<Arc<dyn Bridge>>` - `chat.rs:817`: route the single dispatch site through `resolve_bridge` instead of `state.hub.get(provider)`. The validation check at `chat.rs:500` keeps the legacy-only check — pk isn't in scope there yet, and the two-tier and legacy registries always cover the same Provider set today, so it's not load-bearing. Zero behavior change today (both tiers miss, legacy serves the request). Tests: - `cargo test -p aisix-proxy --lib` — 218 passed - `cargo clippy --workspace --all-targets -- -D warnings` clean - `cargo fmt --all -- --check` clean Refs api7/AISIX-Cloud#302
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR refactors bridge resolution to use a new two-tier lookup strategy. A ChangesBridge Resolution Two-Tier Lookup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
moonming
commented
May 17, 2026
Independent third-party audit per CLAUDE.md §8 Conducted cold (no shared context). Read the PR description, the full diff, the two touched files, and grepped the broader proxy crate for additional dispatch surfaces. Verification: HIGHH1. Scope mismatch — five other dispatch sites still use |
| file:line | role | post-PR routing |
|---|---|---|
chat.rs:500 | validation pre-check | legacy-only (intentional, pk not in scope) |
chat.rs:524–527 | streaming chat dispatch | legacy-only — MISSED |
chat.rs:817–821 | non-streaming chat dispatch + routing fallover | resolve_bridge ✓ |
completions.rs:119–122 | /v1/completions dispatch | legacy-only — MISSED |
embeddings.rs:139–142 | /v1/embeddings dispatch | legacy-only — MISSED |
images.rs:134–137 | /v1/images/generations dispatch | legacy-only — MISSED |
messages.rs:425–428 | cross_provider_dispatch for /v1/messages non-Anthropic upstream | legacy-only — MISSED |
Consequence after issue #302 Phase D ships (cp-api B3 + DP register_specialized / register_family wiring):
- A request to
POST /v1/chat/completionswithstream: false→ two-tier resolution honoured. - A request to
POST /v1/chat/completionswithstream: true→ silently bypasses two-tier; specialized Bridge registrations have no effect for streaming chat. - A request to
POST /v1/embeddings//v1/completions//v1/images/generations//v1/messages(cross-provider) → silently bypasses two-tier.
This contradicts the issue #302 TL;DR contract ("DP 端 Hub 两层 dispatch") and means a specialized DeepSeek/Jina/etc. Bridge added in a future PR would work for non-streaming chat but silently no-op everywhere else — exactly the kind of "works in one place, broken in another" inconsistency Phase D is meant to eliminate.
Suggested fix: route all six dispatch sites through resolve_bridge. Concretely:
// chat.rs:524 (streaming path)letSome(bridge) =
crate::dispatch::resolve_bridge(&state.hub,&pk_entry.value, provider)else{returnErr(with_model(ProxyError::ProviderUnavailable));};// completions.rs:119let bridge = crate::dispatch::resolve_bridge(&state.hub,&pk_entry.value, provider).ok_or(ProxyError::ProviderUnavailable)?;// embeddings.rs:139, images.rs:134 — same shape// messages.rs:425 — same shape, using `pk_entry.value` from the caller frameIf the intent is genuinely "Phase D cutover is chat-completions-non-streaming only and the rest land in a follow-up PR", the PR title/description should say that explicitly and the follow-up should be linked. Right now the PR claims completeness ("no other surfaces dispatch via Hub") that the code does not match.
H2. resolve_bridge itself has no unit test
dispatch.rs adds the function but mod tests is unchanged — the existing 8 tests cover URL helpers and resolve_provider_key, none of them touches resolve_bridge. The PR description leans on "218 lib tests pass", but every one of those tests is built on a Hub constructed via hub.register(Provider, Bridge) — none of them registers anything via register_specialized / register_family, so the specialized-hit and family-hit branches of resolve_bridge are completely uncovered. The only branch the existing suite exercises is the legacy fallback (which behaves identically to the previous code), so the suite cannot fail on a regression in the two-tier path.
This is exactly the "tests pass but the new code path is untested" gap CLAUDE.md §8 calls out.
Suggested fix: add three trivial unit tests in dispatch.rs:
#[cfg(test)]mod resolve_bridge_tests {usesuper::*;use aisix_core::models::Adapter;use aisix_gateway::Bridge;use std::sync::Arc;// A trivial Bridge whose name lets the assertion identify which// registration tier resolved.#[derive(Debug)]structNamedBridge(&'staticstr);#[async_trait::async_trait]implBridgeforNamedBridge{fnname(&self) -> &str{self.0}// ... (use whatever the existing test stubs use in hub.rs)}fnpk_with(provider:&str,adapter:Option<Adapter>) -> ProviderKey{letmut pk:ProviderKey = serde_json::from_str(r#"{"display_name":"x","secret":"k"}"#).unwrap();
pk.provider = provider.to_string();
pk.adapter = adapter;
pk
}#[test]fnspecialized_hit_wins_over_family_and_legacy(){let hub = Hub::new();
hub.register_specialized("deepseek",Arc::new(NamedBridge("specialized")));
hub.register_family(Adapter::Openai,Arc::new(NamedBridge("family")));
hub.register(Provider::Openai,Arc::new(NamedBridge("legacy")));let b = resolve_bridge(&hub,&pk_with("deepseek",Some(Adapter::Openai)),Provider::Openai,).unwrap();assert_eq!(b.name(),"specialized");}#[test]fnfamily_hit_when_no_specialized(){let hub = Hub::new();
hub.register_family(Adapter::Openai,Arc::new(NamedBridge("family")));
hub.register(Provider::Openai,Arc::new(NamedBridge("legacy")));let b = resolve_bridge(&hub,&pk_with("anyvendor",Some(Adapter::Openai)),Provider::Openai,).unwrap();assert_eq!(b.name(),"family");}#[test]fnlegacy_fallback_when_neither_tier_registered(){let hub = Hub::new();
hub.register(Provider::Openai,Arc::new(NamedBridge("legacy")));let b = resolve_bridge(&hub,&pk_with("",None),Provider::Openai,).unwrap();assert_eq!(b.name(),"legacy");}#[test]fnreturns_none_when_nothing_registered(){let hub = Hub::new();let r = resolve_bridge(&hub,&pk_with("",None),Provider::Openai,);assert!(r.is_none());}}(If NamedBridge is too heavy because of the full Bridge trait surface, reuse StubBridge from aisix-gateway/src/hub.rs tests by making it pub(crate) or by duplicating the minimal stub — hub.rs already has the working pattern at lines 188–199.)
MEDIUM
M1. Latent inconsistency at chat.rs:500 (validation) vs chat.rs:821 (dispatch)
chat.rs:500 validates "is this provider known to the gateway?" with legacy-only state.hub.get(provider).is_none(). Once Phase D fully ships and a specialized Bridge for, say, vendor "newcorp" is registered without a corresponding legacy Provider enum variant (the whole point of Phase D — collapse the closed enum), the validation will reject the request with 503 even though the dispatch path at line 821 would have resolved it via dispatch_two_tier.
This is not a present-day bug — today every specialized vendor still maps to one of the six legacy Provider enum variants. But once Phase E/F lands and the enum is collapsed, this validation site becomes a silent false-negative gate ahead of an otherwise-working dispatch path.
The PR description explicitly punts on this ("pk isn't in scope there yet, and the two-tier and legacy registries always cover the same Provider set today"). That's true today but is exactly the kind of latent gap a Phase D PR should at least file as a follow-up.
Suggested fix (one of):
- (Cheapest) drop the line 500 pre-validation entirely — the routing loop at line 821 already returns a proper
BridgeError::Config("no bridge registered for provider")envelope that surfaces the same operator-error class with the same HTTP status mapping. The pre-check is a 5-line shortcut on top of an already-correct fallthrough. - (More principled) resolve
pk_entryfor the only-target case before the validation gate, then useresolve_bridgethere too. Adds ~5 lines. - (Defer) file a tracking issue ("collapse chat.rs:500 once Provider enum closes") and link from this PR + refactor(server): inline DeepSeek/Google bridge factories + delete wrapper crates (Phase A) #302.
Either fix is fine; doing nothing leaves a latent regression for the team that lands Phase E.
M2. dispatch_two_tier → None after specialized was registered could mask a config drift
Today dispatch_two_tier returns None whenever the requested pk.provider is not registered as specialized andpk.adapter is None or not registered as a family. The resolve_bridge wrapper then falls back to hub.get(provider).
Post-Phase D, suppose an operator registers a specialized Bridge for "deepseek" and the DP is reconfigured at runtime to unregister it (or it's evicted by a future eviction policy, or a typo in register_specialized writes "deep-seek" instead of "deepseek"). The resolve_bridge wrapper silently falls back to the legacy Provider::Deepseek bridge — which today is an OpenAI-compat bridge with_name("deepseek"). The request succeeds against the wrong handler.
This is graceful, not silently-wrong-output: the legacy bridge is what serves this vendor today, so the fallback is the correct behaviour pre-cutover. But the comment in dispatch.rs says "Returns None only when both layers miss — i.e. the operator has no bridge wired for this request at all", which under-sells the silent shadowing: a specialized handler going missing falls back to whatever the legacy registry still has, with no log line.
Suggested fix: add a tracing::warn! (or debug!) when dispatch_two_tier returned None but hub.get(provider) returned Some, so operators get a signal during the cutover. Something like:
pub(crate)fnresolve_bridge(hub:&Hub,provider_key:&ProviderKey,provider:Provider,) -> Option<Arc<dynBridge>>{ifletSome(b) = hub.dispatch_two_tier(provider_key){returnSome(b);}let fallback = hub.get(provider)?;if !provider_key.provider.is_empty() || provider_key.adapter.is_some(){
tracing::debug!(
target = "aisix_proxy::dispatch",
pk_provider = %provider_key.provider,
pk_adapter = ?provider_key.adapter,
legacy_provider = ?provider,"two-tier dispatch missed for a PK that carries new-shape \ fields; falling back to legacy Provider-keyed registry");}Some(fallback)}debug! keeps this off the hot logging path in normal operation but gives the operator a knob during cutover. The condition guard means it stays silent today (where every PK has provider: "" and adapter: None) and only fires post-B3.
LOW
L1. PR description's grep is incomplete and should be re-run
The PR description's audit-trail grep:
grep -n "state\.hub\.get\|hub\.get\(.*Provider"
is single-line and misses the standard rustfmt-wrapped form state\n .hub\n .get(provider). Recommend replacing the description's grep with rg -U --multiline "state[\s\n]*\.[\s\n]*hub[\s\n]*\.[\s\n]*get" (or simply rg -U --multiline "\.hub") so the next reviewer can verify scope without rediscovering the multi-line gap.
L2. messages.rs:399 doc-comment will be out-of-date once H1 lands
Module doc-comment at messages.rs:399:
/// 2. hub.get(model.provider) → Bridge for the configured upstream
will drift if H1 is fixed by routing the cross-provider path through resolve_bridge. Update to:
/// 2. resolve_bridge(hub, pk, model.provider) → Bridge (two-tier with
/// legacy fallback; see crate::dispatch::resolve_bridge)
Not a blocker, but matches the new contract.
L3. chat.rs:524 carries a now-misleading comment
The streaming dispatch site has a long pre-PR comment explaining streaming fallback semantics. The comment is correct, but a future maintainer comparing chat.rs:524 (legacy hub.get) and chat.rs:821 (resolve_bridge) will rightly wonder why streaming dispatches differently. If H1's fix lands and routes both through resolve_bridge, the asymmetry disappears. If it doesn't, a one-line comment at line 524 explaining the deliberate divergence ("streaming path still uses legacy hub.get — Phase D cutover pending, see #305") avoids the future maintainer thinking they spotted a copy-paste bug.
Sensitive-info leakage, security, breaking changes
- Sensitive info: none.
resolve_bridgereturnsOption<Arc<dyn Bridge>>; theNonearm inchat.rs:821producesBridgeError::Config("no bridge registered for provider")which is operator-error class and doesn't leak any PK fields. The fallback's existence is invisible to clients. - Security: none. No new auth path, no new input boundary, no header forwarding change.
- Breaking changes: none today (legacy fallback covers every PK). The PR's framing ("zero behavior change today") is accurate for the patched dispatch site. The H1 finding is about future breakage post-Phase-D-cutover, not present-day regressions.
Verdict
Merge gate per CLAUDE.md §8: NOT YET — H1 and H2 must be addressed (or explicitly deferred with a linked follow-up issue) before merge.
H1 is the substantive concern: the PR's claim "no other surfaces dispatch via Hub" is factually wrong; five other dispatch sites still bypass the two-tier path. Either patch them all in this PR (the diff stays small — five 4-line changes) or scope the PR title/description to "chat-completions non-streaming" and file a follow-up for the rest. H2 is a 50-line test addition that's hard to justify deferring given the entire PR's runtime impact is "now invokes a helper" — proving the helper's branches behave correctly is the bare minimum.
M1 and M2 should be either addressed inline or filed as linked follow-up issues — both are latent gaps that bite Phase E/F, not present-day regressions.
LOW findings are housekeeping; merge is not blocked on them but they're worth a small editing pass.
Once H1 + H2 are resolved (and M1 + M2 are addressed or explicitly justified), this PR passes independent audit. The core change — adding a small resolve_bridge helper that wraps dispatch_two_tier with a legacy fallback — is correct and well-documented; the only issue is the scope mismatch between what the description claims and what the diff covers.
Addresses audit HIGH-1 + HIGH-2 + MEDIUM-2 from independent audit on PR #305 (#305 (comment)): **HIGH-1** — initial PR only patched chat.rs:817 (non-streaming chat). Audit's multi-line grep found 5 more dispatch sites that bypass resolve_bridge entirely; post-Phase-D specialized Bridge registrations would silently be ignored for streaming chat, completions, embeddings, messages, and images. All 5 now route through `resolve_bridge`: - chat.rs:524 streaming chat - completions.rs:119 /v1/completions - embeddings.rs:139 /v1/embeddings - images.rs:134 /v1/images/generations - messages.rs:425 /v1/messages (cross-provider Anthropic-shape) chat.rs:500 stays legacy-only intentionally — pk isn't in scope at the pre-validation point, and today's two-tier and legacy registries cover the same Provider set so the gap isn't observable. Documented as MEDIUM-1 latent risk; will revisit when Phase E/F collapses the Provider enum. **HIGH-2** — added 4 unit tests in `dispatch.rs::tests::resolve_bridge_tests` covering all three reachable outcomes: 1. specialized_hit_wins_over_family_and_legacy — pk.provider hits 2. family_hit_when_specialized_misses — pk.adapter falls through to family 3. legacy_fallback_when_both_new_tiers_miss — today's pre-cutover state 4. none_when_nothing_registered — all three layers empty A minimal local StubBridge fixture avoids the cross-crate visibility issue with aisix-gateway's private test stub. **MEDIUM-2** — `resolve_bridge` now emits a `tracing::debug!` when the PK carries new-shape fields (`provider` non-empty or `adapter: Some`) but the two-tier path still missed. Pre-cutover PKs (empty provider + adapter: None) take the silent path. Post-cutover this is the early signal that a specialized Bridge name was misregistered (typo, runtime unregister) or that the adapter map missed an entry. LOWs deferred (PR description grep update, messages.rs:399 doc-comment, chat.rs:524 explanatory comment). Tests: - `cargo test -p aisix-proxy --lib` — 222 passed (218 existing + 4 new) - `cargo clippy --workspace --all-targets -- -D warnings` clean - `cargo fmt --all -- --check` clean Refs api7/AISIX-Cloud#302
moonming
commented
May 17, 2026
HIGH-1 + HIGH-2 + MEDIUM-2 addressed in 2fc8496Per the independent audit: HIGH-1 — wired 5 missed dispatch sitesMulti-line grep verified by audit: 6 total dispatch sites in aisix-proxy, original PR only patched 1. All 5 missed now use `resolve_bridge`:
`chat.rs:500` stays legacy-only intentionally (pk not in scope yet, two-tier and legacy registries cover same Provider set today). Tracked as MEDIUM-1 latent risk for Phase E/F when Provider enum collapses. HIGH-2 — added 4 unit tests for `resolve_bridge``dispatch::tests::resolve_bridge_tests`:
Minimal local StubBridge fixture avoids cross-crate visibility on aisix-gateway's private test stub. MEDIUM-2 — tracing::debug! when fallback fires post-cutover`resolve_bridge` now emits `tracing::debug!` when the PK carries new-shape fields (`provider` non-empty OR `adapter: Some`) but two-tier missed. Pre-cutover PKs stay silent (dominant case). Post-cutover this is the early signal for misregistered Bridges or adapter_map gaps. LOWs deferred
Tests: `cargo test -p aisix-proxy --lib` — 222 passed (218 existing + 4 new); clippy clean; fmt clean. |
Uh oh!
There was an error while loading. Please reload this page.
Summary
Phase D cutover for
aisix-proxydispatch. Addscrate::dispatch::resolve_bridge— a small helper that tries the two-tier dispatch path (specialized vendor → adapter family from PR #300) first, then falls back to the legacyProvider-keyed registry when neither tier matches.The fallback exists because today's on-disk
ProviderKeypayloads carryprovider: ""+adapter: None(the new fields from PR #298/#303 ship empty until cp-api's B3 sub-PR populates them). The two-tier path therefore returnsNoneon every existing key and the legacy registry continues serving traffic unchanged. Zero behavior change today.After B3 ships and cp-api re-projects every
ProviderKeywith the newprovider+adapterfields filled, the two-tier path will start returning bridges and the legacy fallback becomes the residual safety net.Changes
dispatch.rs: addpub(crate) fn resolve_bridge(hub, pk, provider) -> Option<Arc<dyn Bridge>>chat.rs:817: route the single dispatch site throughresolve_bridgeinstead ofstate.hub.get(provider)chat.rs:500: validation check stays legacy-only (pk isn't in scope there yet, and the two-tier and legacy registries always cover the same Provider set today)Single
grep -n "state\.hub\.get\|hub\.get\(.*Provider"acrossaisix-proxy/src/confirms only those two call sites exist; no other surfaces (messages.rs / completions.rs / embeddings.rs / responses.rs / rerank.rs) dispatch via Hub.Test plan
cargo test -p aisix-proxy --lib— 218 passedcargo clippy --workspace --all-targets -- -D warningscleancargo fmt --all -- --checkcleanadapter/providerfields first)Refs api7/AISIX-Cloud#302
Summary by CodeRabbit