Repository navigation
fix(proxy): derive auto-router health from its underlying models - #38174
Conversation
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4/5 The implementation is solid. Here's the breakdown: What works wellCorrectness of the core verdict logic ( visible: Final = resolved & visible_ids
if not visible or not visible <= unhealthy_ids:
return NoneThe condition is right: only red a router when all visible deployments for a dependency are unhealthy. If even one is healthy, the router stays green. The fail-open cases (empty alias, unresolvable name when aliased, no router injected) are all handled correctly. Resolution parity with the request path — using Dependency probe expansion — Defensive config parsing — Backwards compatibility — Test coverage — 8 new focused tests: tier-down, all-healthy, missing name, caller-visibility gating, no-op on full list, probe expansion. The parametrized Minor concerns (not blockers)
The Semantic router routes are unresolvable from the params dict alone (they live in a JSON string or a file path). The PR documents this and accepts it as a known limitation — the default and embedding models are still checked. This is the right call given the complexity of parsing an arbitrary JSON blob at health-check time. The implementation is clean, well-tested, and the described before/after behavior matches what the code does. The two minor points above are optimization opportunities, not correctness issues. |
Greptile SummaryThis PR derives auto-router health from the deployments used by its tier, default, classifier, and embedding models.
Confidence Score: 3/5The PR should not merge until disabled dependency probes and unresolved alias targets produce health results consistent with their request-time behavior Targeted checks can probe deployments explicitly excluded from health checks, while aliases with missing targets remain green even though requests through them fail Files Needing Attention: litellm/proxy/health_check.py
|
| Filename | Overview |
|---|---|
| litellm/proxy/health_check.py | Adds dependency probing and router verdict finalization, but reintroduces disabled deployments and mishandles aliases with missing targets |
| litellm/router_utils/auto_router_model_naming.py | Adds typed dependency extraction for strategy-router configurations with documented fail-open limits |
| litellm/proxy/health_check_utils/shared_health_check_manager.py | Propagates Router context through shared and fallback health checks |
| litellm/proxy/health_endpoints/_health_endpoints.py | Supplies the shared Router to live health checks while preserving caller model scoping |
| litellm/proxy/proxy_server.py | Supplies Router context to direct and shared background health-check execution |
| tests/test_litellm/proxy/test_health_check_max_tokens.py | Adds focused strategy-router verdict and dependency-expansion coverage but omits disabled dependencies and unresolved alias targets |
| tests/test_litellm/proxy/test_shared_health_check.py | Updates shared-health-check call assertions for Router propagation |
| tests/test_litellm/router_utils/test_auto_router_model_naming.py | Exercises dependency extraction across supported strategy-router configuration shapes |
Reviews (1): Last reviewed commit: "fix(proxy): derive auto-router health fr..." | Re-trigger Greptile
| dependency_probes: Final = ( | ||
| _dependency_deployments_to_probe(requested, universe, router) if router is not None else () | ||
| ) | ||
| model_list = requested + list(dependency_probes) # mutable-ok: _perform_health_check takes a list |
There was a problem hiding this comment.
Fixed. Eligibility now has one owner, applied to the requested set and the probe pool alike, so a disabled dependency cannot re-enter. Test pins it.
| resolved: Final = router.get_model_list(model_name=model_name) | ||
| if not resolved and model_name not in router.model_group_alias: | ||
| return None | ||
| return frozenset(ident for entry in (resolved or ()) if (ident := _deployment_id(entry))) |
There was a problem hiding this comment.
When an alias target has no deployment, this returns an empty set, causing router health to stay green while requests fail
| resolved: Final = router.get_model_list(model_name=model_name) | |
| if not resolved and model_name not in router.model_group_alias: | |
| return None | |
| return frozenset(ident for entry in (resolved or ()) if (ident := _deployment_id(entry))) | |
| resolved: Final = router.get_model_list(model_name=model_name) | |
| if not resolved: | |
| return None | |
| return frozenset(ident for entry in resolved if (ident := _deployment_id(entry))) |
There was a problem hiding this comment.
Fixed, took your suggestion. An alias resolving to zero deployments is now unresolvable, not unknown. Test covers a broken alias target.
| dependency_probes: Final = ( | ||
| _dependency_deployments_to_probe(requested, universe, router) if router is not None else () | ||
| ) | ||
| model_list = requested + list(dependency_probes) # mutable-ok: _perform_health_check takes a list |
There was a problem hiding this comment.
Reassigning model_list violates the repository's immutable-data convention and obscures the distinction between requested and additionally probed deployments
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed. Narrowing and eligibility are pure helpers now, so requested and probed are separate names and the parameter is never rebound.
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
9ab1438 to
8e87537
Compare
|
Pushed fixes for all five findings. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4/5 The implementation is solid. Here's the full breakdown: What's working well
Two remaining concerns (minor)
Neither is a blocker — the fix is correct and the scenarios that mattered before this PR (green routers silently 500ing, admin UI showing healthy for dead-tier routers, |
8e87537 to
fa4b563
Compare
|
Pushed fixes for the complexity default derivation plus the two cleanups. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4.5 / 5 Here's why: What's strong:
What keeps it from 5/5:
Neither of these is a regression from the current state; both are documented known limitations, not new bugs. The PR is ready. |
fa4b563 to
95f2908
Compare
|
Fixed nested-router propagation. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
4.5 / 5 The nested-router propagation fix is correct and the overall implementation is solid. Here's the breakdown: What's working well
One remaining concern
def _settle_router_verdicts(
markers: Mapping[str, Mapping[str, object]],
router: "Router",
visible_ids: frozenset[str],
probe_failures: frozenset[str],
settled: Mapping[str, str],
rounds_left: int,
) -> Mapping[str, str]:
while rounds_left > 0:
unhealthy_now: Final = probe_failures | frozenset(settled)
fresh = {
marker_id: error
for marker_id, deployment in markers.items()
if marker_id not in settled
if (error := _strategy_router_dependency_error(deployment, router, visible_ids, unhealthy_now))
}
if not fresh:
return settled
settled = MappingProxyType({**settled, **fresh})
rounds_left -= 1
return settledThat's the only gap I'd flag. Everything else — the dependency extraction logic, the targeted-check probe expansion, the cycle termination proof, the test coverage — is in good shape. |
95f2908 to
8b6c918
Compare
|
Fixed transitive probe expansion for nested routers. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Confidence score: 5/5. The transitive probe expansion fixes the remaining targeted-check gap: |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8b6c918. Configure here.
8b6c918 to
216ddd0
Compare
|
Rebased onto current staging, conflicts resolved. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 5/5 The rebase does not introduce any new correctness concerns, and the previously identified issues are addressed:
No blocking issues remain from this review. The current CI status is still pending, so this score is based on the implementation and test coverage rather than a completed CI run. |
|
Confidence score: 4/5 The rebased commit correctly addresses the reported behavior. The implementation:
The added tests cover the important positive, negative, malformed-config, alias, visibility, disabled-dependency, nested-router, cycle, and targeted-probe cases. I’m giving 4/5 rather than 5/5 because this changes core |
216ddd0 to
4004325
Compare
|
@greptile can you reply here with a score and why |
|
bugbot run |
|
Score: 4/5 This is a strong, well-scoped fix. It derives strategy-router health from the models the routing path actually resolves, distinguishes unhealthy dependencies from missing ones, respects disabled or partially judged deployments, propagates failures through nested routers, and bounds cycles. The tests cover tiers, defaults, classifiers, aliases, malformed configs, partial evidence, nested routers, and cycles. It also correctly avoids extra probes during full-list checks while expanding dependencies for targeted router checks. I’m not giving 5/5 because the change substantially alters |
| if router is not None | ||
| else () | ||
| ) | ||
| checked: Final = requested + list(dependency_probes) # mutable-ok: _perform_health_check takes a list |
There was a problem hiding this comment.
Targeted probes skip ID deduplication
Low Severity
requested goes through filter_deployments_by_id, but dependency_probes are taken from the undeduped universe via _deployments_by_id. Duplicate rows that share an id (the alias case that filter already exists for) get probed twice on the targeted /health?model_id= path. Divergent results for the same id can also put it in probe_failures while a sibling probe succeeded, which reds the router.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 4004325. Configure here.
There was a problem hiding this comment.
Confirmed and fixed at the primitive rather than the call site.
_deployments_by_id is the one answer to "give me the deployments for these ids", and it was the only path that did not honour the one-row-per-id rule filter_deployments_by_id exists for. It now calls that function instead of mirroring a weaker version of it:
matched: Final = tuple(d for d in universe if (uid := _deployment_id(d)) and uid in ids)
return tuple(filter_deployments_by_id(model_list=matched))Fixing it there covers both of its callers — the returned probe set and the sweep's frontier — so the fix cannot be reintroduced by a future third caller. Both halves you flagged go with it: the duplicate row is no longer probed twice, and one id can no longer land in probe_failures from one probe while a sibling probe of the same id succeeded.
Test: test_dependency_probes_carry_one_row_per_id builds a universe with dead-1 present twice and asserts the probe set carries it once. It fails on the previous commit.
| last_type_error: TypeError | None = None | ||
| for extra_kwargs in ( | ||
| { | ||
| "router": llm_router, |
There was a problem hiding this comment.
Retry fallback drops skip-disabled filter
Low Severity
router was folded into the first perform_health_check kwargs set, but the next fallback is only instrumentation_context. A TypeError on the new router argument now retries without health_check_skip_disabled_background_models, so opted-out deployments get probed on the background fallback path.
Reviewed by Cursor Bugbot for commit 4004325. Configure here.
There was a problem hiding this comment.
Correct, and it exposed the real defect: the ladder was a hand-maintained power set of kwarg combinations, so every argument added to perform_health_check needs a new rung or it opens exactly this kind of hole. Guarding it with one more rung would have left the next argument to reintroduce the bug.
The ladder is gone. The TypeError already names the argument the callee rejected, so drop that one and keep the rest:
optional: Mapping[str, object] = MappingProxyType(
{
"router": llm_router,
"instrumentation_context": instrumentation_context,
**health_check_filter_kwargs_from_general_settings(general_settings),
}
)
for _ in range(len(optional) + 1):
try:
return await perform_health_check(..., **optional)
except TypeError as e:
rejected = _UNEXPECTED_KWARG.search(str(e))
if rejected is None or rejected["name"] not in optional:
raise
optional = MappingProxyType({k: v for k, v in optional.items() if k != rejected["name"]})A callee that predates router now loses router and nothing else, so health_check_skip_disabled_background_models survives and opted-out deployments stay unprobed. The rejected["name"] not in optional test keeps the previous re-raise behaviour for a TypeError that is not about one of these arguments, so _is_unexpected_keyword_argument_type_error was deleted rather than replaced.
Tests: the three existing rung tests (legacy three-arg stub, instrumentation-only, filter-only) all still pass, since one-at-a-time removal reaches each of those shapes. New test_run_direct_health_check_drops_only_the_rejected_kwarg covers the case you found — a stub accepting instrumentation_context and the filter but not router — and asserts the callee receives (instrumentation_context, True). It fails on the previous commit.
An auto_router deployment is a marker, not something a probe can contact, so
`_run_model_health_check` returns `{}` for it and it lands healthy whatever is
behind it. This derives its verdict from the models it actually resolves.
Rules and owners:
- `strategy_router_dependencies` is the single answer to "what does this router
call": tier, default, classifier and embedding names per router kind, aligned
with what init and the request path actually use.
- `_health_check_eligible` is the single probe-eligibility gate, applied to the
requested set and to the pool a router's dependencies are drawn from alike, so
an opted-out deployment cannot re-enter through a router that depends on it.
- `_resolved_deployment_ids` resolves names through `get_model_list`, the same
composition of alias, routing-group and wildcard channels a request uses.
- A dependency reds its router only when *every* deployment behind the name is
known unhealthy. A replica this run never judged, hidden from the caller or
opted out of health checks, can still serve what the dead one drops, so
partial evidence leaves the verdict green. Absent information never reds.
- Verdicts settle over rounds, because a marker never fails a probe of its own
and a parent whose tier is a red router must inherit that fault. Both sweeps
are bounded loops, so a router cycle terminates green.
- Dependency probes are added only on the targeted `/health?model_id=` path the
dashboard uses per deployment, and are dropped from the response.
Resolves LIT-6073
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4004325 to
086b961
Compare
|
@greptile can you reply here with score & why |
|
bugbot run |
|
Score: 4/5 (high confidence). Why:
I’m not giving 5/5 because this changes |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 086b961. Configure here.


TLDR
Problem this solves:
How it solves it:
User Flow
Before: an admin running a complexity router whose SIMPLE tier points at a broken deployment sees a healthy router, then fields 500s from users
healthy_endpointswith a green badge, while the deployment its SIMPLE tier routes to sits inunhealthy_endpointsThere are no healthy deployments for this modelAfter: the same health call reports the router unhealthy and names the model that broke
unhealthy_endpointswithtier model 'dead-model' has no healthy deploymentRelevant issues
Linear ticket
Resolves LIT-6073
Pre-Submission checklist
uv run pytest tests/test_litellm/<your_test_file>.py -vWhat makes a router report non-green
Every case below is a router reporting unhealthy. Anything not listed leaves it green, and absent information never reds a router.
A tier model has no healthy deployment. Every deployment behind that name failed its own probe in the same run. Tier pools count per member, because pool selection is a blind
random.choicewith no health awareness, so one dead member fails that share of the tier's trafficThe default model has no healthy deployment.
complexity_router_default_model,auto_router_default_modelorquality_router_default_model, whichever the router would actually resolve, since the params field overrides the config oneThe classifier model has no healthy deployment, for
classifier_type: llm. A dead classifier still serves requests by silently falling back, so the routing decision is gone while the traffic looks fineA router it routes to is itself red. Verdicts settle over rounds, so a parent whose tier or default names another strategy router inherits that child's fault instead of reading the child's unprobed marker as healthy. Rounds are bounded by the marker count, so two routers pointing at each other terminate green rather than recursing
A referenced model name resolves to no deployment at all. Resolution goes through every channel the request path uses, so a working alias, routing group or wildcard is not reported as missing, while an alias whose target is gone reds the router because a request through it fails the same way
Fail-open cases, which stay green:
disable_background_health_check. A replica that was never probed can still serve what a dead sibling drops, so a fraction of the evidence never decides the verdictsemantic_keyword_matchingis off, since the router never calls itcomplexity_router_config.default_model, which init overwrites with a tier-derived value, so only thelitellm_paramsspelling counts (a quality router does fall back to its config field, so both count there)auto_router_configJSON string or anauto_router_config_pathfile, so only its default and embedding models are checkedScreenshots / Proof of Fix
Config used for both runs, plus a router whose models all serve as the control.
good-modelis a real billed gateway deployment,dead-modelpoints at a port with nothing on itBefore (43ae350)
Full health check
curl -s -H "Authorization: Bearer $KEY" http://127.0.0.1:4674/healthPer-deployment check, which is what the Admin UI health table issues
for id in ...; do curl -s -w "%{http_code}" ".../health?model_id=$id"; doneA green router cannot serve
curl -s -w "HTTP %{http_code}" -X POST .../v1/chat/completions -d '{"model":"router-tier-down","messages":[{"role":"user","content":"hi"}]}'After (216ddd0)
Full health check
curl -s -H "Authorization: Bearer $KEY" http://127.0.0.1:4673/healthPer-deployment check, which is what the Admin UI health table issues
Each targeted call returns exactly its own deployment,
1 / 0or0 / 1, so the dependencies pulled in to reach the verdict are not reported backThe targeted path agrees with the full list on every case, including
router-nested-parent, whose tier is another router that is itself red. Reaching that verdict needs the child's own models probed, not just the child, so expansion follows routers through routersA green router cannot serve
router-healthystill answers 200 on a real billed call through the gateway, so only routers with a real fault turned redType
🐛 Bug Fix
Caveats (if any)
Final Attestation
Note
Medium Risk
Changes core
/healthsemantics and can add probes on targeted router checks; logic is complex (nested routers, partial evidence, skip-disabled) but heavily tested and fail-open when no router is injected.Overview
Strategy-router (
auto_router/*) deployments no longer show green on/healthwhen their backing models cannot serve. Markers still skip direct LLM probes, but when aRouteris passed in, health runs dependency resolution, optional extra probes, and post-processing that can move a router from healthy to unhealthy with a specific error (e.g.tier model 'dead-group' has no healthy deployment).strategy_router_dependencies()inauto_router_model_naming.pyenumerates tier, default, classifier, and embedding names per router kind (with runtime-conditional fields so dead config keys do not false-red).perform_health_checkrefactors targeting/eligibility into helpers, expands probes only for targeted router checks (not full-list runs), propagates nested router failures in bounded rounds, and strips dependency-only probes from the response.Wiring:
GET /health, shared Redis health checks, and background paths passllm_router._run_direct_health_check_with_instrumentationdrops unknown optional kwargs by name instead of a fixed retry ladder.Reviewed by Cursor Bugbot for commit 086b961. Bugbot is set up for automated code reviews on this repo. Configure here.