fix(proxy): honour allow_requests_on_db_unavailable in readiness probe - #34935
fix(proxy): honour allow_requests_on_db_unavailable in readiness probe#34935Zerohertz wants to merge 3 commits into
allow_requests_on_db_unavailable in readiness probe#34935Conversation
/health/readiness returned 503 whenever a configured Prisma DB was unreachable, with no regard for general_settings.allow_requests_on_db_unavailable. Under the shipped Helm probe defaults that marks every replica NotReady about 30 seconds into a DB outage, so Kubernetes empties the Service and the request-layer fallback the flag enables never gets a request to run on The flag used to gate this. _db_health_readiness_check called handle_db_exception, which re-raised only when the flag was off, and health_readiness turned that into a 503. BerriAI#26134 dropped that call to fix a 503 loop and BerriAI#27003 re-added an unconditional 503, which left the flag with no say over readiness Readiness now keeps the 200 and reports the real db status in the body when the flag is set. It also bounds the DB lookup on that path, because health_check retries under backoff over a query_raw that has no timeout of its own, so an unreachable DB can outlast the kubelet's timeoutSeconds and fail the probe on time whatever status code we would have produced. Giving up early costs no recovery since _db_health_watchdog_loop already owns reconnection. With the flag unset the behaviour is unchanged Resolves BerriAI#34929
Greptile SummaryThis PR makes database readiness behavior follow the existing fail-open setting
Confidence Score: 4/5The timeout configuration needs validation before merging because malformed or infinite values can recreate the readiness outage this change is intended to prevent The fail-open behavior and shared status handling are covered, but the newly exposed timeout setting can produce an HTTP 500 or disable the database bound on the affected readiness path Files Needing Attention: litellm/proxy/health_endpoints/_health_endpoints.py
|
| Filename | Overview |
|---|---|
| litellm/proxy/health_endpoints/_health_endpoints.py | Adds flag-aware, timeout-bounded database readiness handling, but invalid or non-finite timeout configuration can break the probe |
| tests/test_litellm/proxy/health_endpoints/test_health_endpoints.py | Adds focused regression tests for fail-open status handling, detailed responses, and conditional timeout behavior |
Reviews (1): Last reviewed commit: "fix(proxy): honour allow_requests_on_db_..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…d input LITELLM_READINESS_DB_PROBE_TIMEOUT_SECONDS was read with a bare float() on every probe. A malformed value raised ValueError straight out of health_readiness, and the public path has no handler, so a typo turned an unauthenticated readiness probe into a 500 and evicted the pod. That is the outage allow_requests_on_db_unavailable exists to prevent, triggered by a config mistake inf was worse than useless: max(0.1, inf) is inf, so wait_for never fired and a hung DB call went back to outlasting the kubelet timeout while readiness reported connected Unparseable and non-finite values now log a warning and fall back to the 2s default. A valid override still applies, clamped to a 0.1s floor
The readiness bound was introduced with its own LITELLM_READINESS_DB_PROBE_TIMEOUT_SECONDS, which failed the documentation CI: test_env_keys.py requires every os.getenv key in litellm/ to appear in config_settings.md, and that file lives in the separate BerriAI/litellm-docs repo, so a new key cannot land here without a cross-repo change PRISMA_HEALTH_WATCHDOG_PROBE_TIMEOUT_SECONDS already exists and is already documented. Both knobs answer the same question, how long to wait on a DB liveness check, so the readiness path now reads it through a new PrismaClient.db_health_probe_timeout_seconds property instead of adding a second setting. An operator who tightens one gets both This also drops the env parsing that BerriAI#34935's review flagged, since the value is now parsed once at client construction rather than per probe
Solaris-star
left a comment
There was a problem hiding this comment.
Reviewed this in depth — it's the more complete version of the readiness/HA-flag fix (I had attempted a narrower variant that only consulted the flag, so this supersedes that work). A few observations:
The timeout bound is the part that actually matters, and it's correct
Consulting allow_requests_on_db_unavailable alone isn't sufficient: PrismaClient.health_check retries under backoff over a query_raw with no timeout of its own, so an unreachable DB can outlast the kubelet's timeoutSeconds and fail the probe on time regardless of the status code we'd return. Bounding the lookup with asyncio.wait_for and reporting disconnected on timeout is what keeps the pod genuinely in rotation. Good catch.
The timeout asymmetry is intentional and right
The bound is only applied when the fallback flag is set; when it's off, _readiness_db_status awaits the check unbounded. That's correct — when fallback is disabled we want 503, and a probe that hangs until kubelet times out still yields NotReady, which is the desired outcome. Only the "must return 200 fast" path needs the budget.
Reusing the watchdog's probe budget is a nice touch
Exposing db_health_probe_timeout_seconds and sharing it between the watchdog and the readiness probe means an operator tightening one tightens both — they answer the same question (how long to wait on a DB liveness check). The docstring spells out exactly this rationale.
Tests
The coverage is solid: stays-ready-when-unreachable, the parametrized detailed-payload path (200/503), and the slow-client/probe-timeout test that proves the bound actually fires. The _slow_prisma_client(delay=0.3, timeout=0.05) case is a clean way to exercise the timeout without real network flakiness.
One minor, non-blocking thought: on the timeout path asyncio.wait_for cancels the in-flight health_check mid-backoff. In the test that's a mock so it's moot; in production asyncpg/prisma generally handle cancellation cleanly, but it's the one spot I'd keep an eye on under real DB-outage conditions.
TLDR
Problem this solves:
allow_requests_on_db_unavailablehad no effect on readinessHow it solves it:
Relevant issues
Fixes #34934
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Before is captured at
daf22ec8712e406f502da4268efcb1743d5564ff(the branch point), after at771c09f6c04b48f0168abf17d9899e70eed974a0. Every call below hits a real OpenAI model and costs real money; nothing is mockedproof_config.yaml:curl -s -w '\nhttp=%{http_code} time=%{time_total}\n' http://localhost:4000/health/readiness503even though the flag is on:curl -s -w '\nhttp=%{http_code} time=%{time_total}\n' http://localhost:4000/health/readinesstimeoutSeconds: 5probe:200with the real db status in the body, so the pod stays in the Service:curl -s -w '\nhttp=%{http_code} time=%{time_total}\n' http://localhost:4000/health/readinessconnectedagain:allow_requests_on_db_unavailable: falseinproof_config.yaml, restart the proxy, stop the database, and check that readiness goes back to503:docker stop litellm-db curl -s -w '\nhttp=%{http_code}\n' http://localhost:4000/health/readinessType
🐛 Bug Fix
Changes
/health/readinessreturned503whenever a configured Prisma DB was unreachable and never looked atgeneral_settings.allow_requests_on_db_unavailable. Under the probe defaults the Helm chart ships (readinessProbe.path: /health/readiness,periodSeconds: 10,failureThreshold: 3) a database outage marks every replica NotReady inside about thirty seconds, Kubernetes drops all of them from the Service endpoints, and the request-layer fallback that the flag exists to provide never gets a request to run on. A recoverable blip becomes a total outageThe flag used to gate this.
_db_health_readiness_checkcalledPrismaDBExceptionHandler.handle_db_exception, which re-raised only when the flag was off, andhealth_readinessturned that into anHTTPException(503). #26134 dropped the call to fix a 503 loop, then #27003 re-added an unconditional 503, which left the flag with no say over readiness_apply_db_readiness_statusnow makes that call in one place and both readiness paths share it, so the low-detail probe and the detailed payload reachable throughallow_public_health_readiness_detailscannot disagree. With the flag set an unreachable DB keeps the 200; thedbfield still carries the truth either wayStatus code alone is not enough.
PrismaClient.health_checkretries underbackoffover aquery_rawthat has no timeout of its own, so an unreachable DB can outlast the kubelet'stimeoutSecondsand fail the probe on time whatever we would have returned._readiness_db_statustherefore bounds the lookup withasyncio.wait_forwhen the flag is set. Giving up early costs no recovery, since_db_health_watchdog_loopalready owns reconnection and runs on its own schedule. The bound deliberately does not apply when the flag is unset, so a slow but reachable DB on the default path behaves exactly as it does todayThe budget for that bound is the watchdog's own
PRISMA_HEALTH_WATCHDOG_PROBE_TIMEOUT_SECONDS, surfaced as aPrismaClient.db_health_probe_timeout_secondsproperty rather than a new setting. Both are answering the same question, how long to wait on a DB liveness check, and an operator who tightens one wants the other tightened too. Note that the 5 second default is longer than some probe configurations allow, so a deployment runningreadinessProbe.timeoutSecondsbelow that should lower this knobFour tests cover this. Three fail if the source change is reverted, and the fourth fails if the bound is applied unconditionally, so the "unchanged by default" half of the contract is pinned too. They assert on the returned status and body rather than on elapsed time, so there is nothing timing-sensitive to flake in CI
One thing is deliberately left out of scope. #26237 asks that a worker which never successfully loaded its router fail readiness, and it should keep failing even with this flag set, since "router state was never populated" is a different condition from "the DB is momentarily unreachable". That needs first-successful-load tracking in the startup path, so it belongs in its own PR
Final Attestation