fix: remove separate health app - #27430
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR removes the
Confidence Score: 5/5Safe to merge — the removal is complete and consistent across all Dockerfiles, the Helm chart, the entrypoint, and the test suite. Every artifact tied to the separate-health-app pattern (supervisord config, factory module, Dockerfile installs, Helm conditionals, entrypoint branch) is removed in lock-step. The replacement test class uses only mocks and makes no real network calls. The deliberate breaking change for existing SEPARATE_HEALTH_APP=1 users has already been reviewed and accepted by a senior developer in a prior thread. No files require special attention.
|
| Filename | Overview |
|---|---|
| docker/prod_entrypoint.sh | Removes the SEPARATE_HEALTH_APP=1 branch that exec'd supervisord; also fixes the missing trailing newline. |
| docker/supervisord.conf | Entire file deleted; the supervisord configuration was the only artifact that tied the separate health process to the main process. |
| litellm/proxy/health_endpoints/health_app_factory.py | Entire file deleted — the factory that spun up the second FastAPI app is no longer needed. |
| deploy/charts/litellm-helm/templates/deployment.yaml | Removes separateHealthApp/separateHealthPort env injection and port definition; hardcodes liveness/readiness/startup probe port to "http". |
| deploy/charts/litellm-helm/values.yaml | Removes separateHealthApp and separateHealthPort keys from the default values file. |
| tests/test_litellm/proxy/test_proxy_cli.py | Removes TestHealthAppFactory (tests for the deleted factory) and adds TestRunServerDbSetup which tests prisma DB setup behaviour via mocks — no real network calls. |
| Dockerfile | Removes supervisor from apk install and drops COPY docker/supervisord.conf — clean removal of the separate-health-app scaffolding. |
| docker/Dockerfile.database | Mirrors main Dockerfile: removes supervisor from apk install and drops COPY docker/supervisord.conf. |
| docker/Dockerfile.non_root | Removes supervisor from apk install and removes the multi-stage copy of supervisord.conf from the builder layer. |
| litellm/proxy/proxy_cli.py | Removes the comment block advertising the separate-health-app uvicorn invocation pattern. |
Reviews (2): Last reviewed commit: "fix: remove separate health app" | Re-trigger Greptile
| #!/bin/sh | ||
|
|
||
| if [ "$SEPARATE_HEALTH_APP" = "1" ]; then | ||
| export LITELLM_ARGS="$@" | ||
| export SUPERVISORD_STOPWAITSECS="${SUPERVISORD_STOPWAITSECS:-3600}" | ||
| exec supervisord -c /etc/supervisord.conf | ||
| fi | ||
|
|
||
| if [ "$USE_DDTRACE" = "true" ]; then | ||
| export DD_TRACE_OPENAI_ENABLED="False" | ||
| exec ddtrace-run litellm "$@" | ||
| else | ||
| exec litellm "$@" | ||
| fi No newline at end of file | ||
| fi |
There was a problem hiding this comment.
Breaking change for existing SEPARATE_HEALTH_APP users
Any deployment that currently runs with SEPARATE_HEALTH_APP=1 set as an environment variable was relying on supervisord launching a dedicated health-check process on a separate port. After this change, that env var is silently ignored and the regular single-process litellm server starts instead. Users who chose the separate health app specifically to keep probes alive under main-process load (the primary motivation for the feature) will lose that isolation with no migration path or deprecation notice.
Rule Used: What: avoid backwards-incompatible changes without... (source)
There was a problem hiding this comment.
This flag should never have been supported, it causes k8s deployments to silently fail since health checks wont check the actual health of litellm
d351abd
into
litellm_internal_staging
…rate-health-app fix: remove separate health app
Relevant issues
Linear ticket
Resolves LIT-2908
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Screenshots / Proof of Fix
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes
Remove
SEPARATE_HEALTH_APPand thesupervisordscaffolding that existed solely to support it. This ran the proxy's health endpoints as a second FastAPI app on a second port, in a separate process, supervised alongside the main proxy. That defeats the purpose of a liveness probe.The Kubernetes docs are explicit about what a liveness probe is for:
A probe is supposed to fire when the application itself is wedged — event loop blocked, deadlocked under load, GIL-bound, etc. A probe served from a different process can't observe any of that. It happily returns 200 while the thing it's supposed to be checking is dead, which gives Kubernetes false confidence not to restart a deadlocked proxy.
Colin Breck names exactly this failure mode:
Updated docs: BerriAI/litellm-docs#99