fix(health): return 503 when targeted model is unhealthy or DB is disconnected - #27003
Conversation
…or DB is disconnected /health?model=foo and /health?model_id=foo previously returned HTTP 200 even when zero endpoints were healthy, forcing monitoring systems to parse the JSON body to detect failure. /health/readiness similarly returned 200 even when a configured Prisma DB was unreachable, leaving unhealthy pods in rotation. Both endpoints now flip to HTTP 503 in the failure case while keeping the JSON response body identical, so existing parsers continue to work and orchestrators can rely on the HTTP status alone.
Greptile SummaryThis PR makes The only remaining issues are two new test functions that omit the Confidence Score: 4/5Safe to merge; only P2 test-consistency issues remain. All production logic changes look correct. The only findings are P2: two new tests omit an explicit tests/test_litellm/proxy/health_endpoints/test_health_endpoints.py — two tests need
|
| Filename | Overview |
|---|---|
| litellm/proxy/health_endpoints/_health_endpoints.py | Adds _resolve_targeted_model_ids to scope background-cache results to a specific deployment before evaluating healthy_count; adds 503 in _post_process for targeted-unhealthy paths and in health_readiness for DB disconnection. Logic is correct for the stated goals. |
| tests/test_litellm/proxy/health_endpoints/test_health_endpoints.py | 7 new tests covering targeted-503, cache scoping, and readiness DB states. Two new 200-path tests omit model_id=None and one has an inaccurate comment about Response() initialization; tests still pass by coincidence today. |
Reviews (3): Last reviewed commit: "fix(health): validate model_id against s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if db_health_status["status"] != "connected": | ||
| response.status_code = status.HTTP_503_SERVICE_UNAVAILABLE |
There was a problem hiding this comment.
Both /health?model=foo and /health/readiness now return 503 where they previously returned 200. The PR description acknowledges this breaks existing callers. Per the team's rule, backwards-incompatible behavioural changes must be gated behind a user-controlled flag (e.g., litellm.health_strict_status_codes) so that operators can opt in on their own schedule rather than being broken on upgrade. The same applies to the /health 503 change introduced in _post_process.
Rule Used: What: avoid backwards-incompatible changes without... (source)
…503 check When use_background_health_checks is enabled, /health?model=foo returned the full cached aggregate across every model — so an unhealthy foo combined with any other healthy deployment kept healthy_count > 0 and the targeted-503 path never fired. Resolve the targeted model/model_id to a deployment-id set first (mirroring perform_health_check's match-on-model_name-or-litellm_model semantics) and narrow the cache to those IDs before _post_process evaluates healthy_count, so the 503 contract holds for both the live and cache code paths.
Per review: `assert response.status_code != 503` is satisfied by 404, 500, or any other non-503 code, so a regression that returned the wrong non-503 status would slip through. Switch to `== 200` so the assertions verify the actual expected status, not just the absence of one specific failure.
|
@greptileai re review |
…h resolver
A non-admin scoped to ["model-a"] could call /health?model_id=id-b
(where id-b belongs to a deployment outside their scope) and the
background-cache code path would return id-b's cached health entry. The
helper returned {model_id} unconditionally, so the cache filter was
driven by an unvalidated id and the global cache leaked the entry — the
ternary `targeted_ids if not None else allowed_model_ids` skipped any
intersection with the caller's allowed deployments.
Make _resolve_targeted_model_ids walk the supplied model_list for both
the model and model_id branches. Callers pass an already-scoped list
(filtered to allowed model_names for non-admins, full list for admins),
so an out-of-scope model_id resolves to an empty set and the cache
filter drops every entry — matching the live path's existing behavior.
|
@greptileai please re-review The P1 backwards-incompat-without-feature-flag finding is intentionally not addressed — accepted in the PR description. |
610f79d
into
litellm_internal_staging
…-non200-on-failure fix(health): return 503 when targeted model is unhealthy or DB is disconnected
2 participants
Summary
GET /health?model=foo(and?model_id=foo) now returns HTTP 503 instead of 200 when the targeted model has zero healthy endpoints. Body shape is unchanged.healthy_countis evaluated, so an unhealthyfoois no longer masked by other healthy models in the global aggregate.GET /health/readinessnow returns HTTP 503 instead of 200 when a Prisma DB is configured but its connection check fails. Theprisma_client is None("Not connected") path stays 200, since "no DB configured" is a valid deployment.status == 200on/health*regardless of body content, but that's fine — the new behavior is what monitoring systems and orchestrators expect.Resolves LIT-2552
Test plan
uv run pytest tests/test_litellm/proxy/health_endpoints/test_health_endpoints.py— 7 new tests (4 for /health model param incl. the background-cache + targeted case, 3 for /health/readiness DB states); pre-existing tests updated to passmodel=None, model_id=Noneexplicitly when calling the handler directly so the FastAPIQuery(None)sentinel doesn't leak into the new cache filter.uv run mypy litellm/proxy/health_endpoints/_health_endpoints.py— clean.HTTP 200, AFTERHTTP 503, body identical.