fix(proxy): prevent Prisma engine orphan leak on shutdown disconnect failure (#26619) - #28755
fix(proxy): prevent Prisma engine orphan leak on shutdown disconnect failure (#26619)#28755pike00 wants to merge 1 commit into
Conversation
Congrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
Greptile SummaryThree targeted patches that prevent Prisma engine subprocess orphans on proxy worker shutdown: the uvicorn lifespan now catches
Confidence Score: 5/5Safe to merge — changes are confined to shutdown paths and startup URL construction, with no impact on the hot request path. All three patches are scoped to shutdown/startup code that runs outside the hot request path. The previously raised concerns about the dead @backoff decorator and keepalive defaults overwriting existing URL values have both been fully addressed in this revision: the decorator is removed, and existing_url parsing ensures defaults are skipped for keys already present in the URL. Test coverage is concrete and well-targeted. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/utils.py | Removes @backoff decorator (now dead code) and swallows disconnect() exceptions instead of re-raising; well-commented with issue reference |
| litellm/proxy/proxy_server.py | Wraps proxy_shutdown_event() in try/except inside lifespan so a disconnect failure cannot orphan the Prisma engine subprocess |
| litellm/proxy/proxy_cli.py | Adds libpq keepalive defaults; correctly skips defaults that are already present in the existing DATABASE_URL/DIRECT_URL query string |
| tests/proxy_unit_tests/test_aproxy_startup.py | New regression test drives the full lifespan with a raising proxy_shutdown_event and pins that the exception is caught and logged |
| tests/test_litellm/proxy/test_proxy_server.py | New regression test verifies PrismaClient.disconnect() swallows errors and schedules failure_handler via create_task |
| tests/test_litellm/proxy/test_proxy_cli.py | Existing defaults test updated (strict equality now includes keepalive keys); three new tests cover default emission, extra_params override, and existing-URL preservation |
Reviews (4): Last reviewed commit: "fix(proxy): prevent Prisma engine orphan..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThree surgical patches address Prisma engine orphan leaks under worker churn (#26619): the uvicorn lifespan now wraps
Confidence Score: 4/5Safe to merge — the core fixes correctly break the exception-propagation chain that caused orphan Prisma engine subprocesses, and all three patches are backed by targeted regression tests. The proxy_server and utils changes are minimal and well-guarded. The keepalive injection in proxy_cli correctly wires up the defaults, but the merge order in append_query_params means any keepalive settings already present in a DATABASE_URL query string are silently overwritten by the new defaults rather than respected — operators relying on URL-embedded params would need to move those settings to database_extra_connection_params without being told to do so. litellm/proxy/proxy_cli.py — the keepalive default merge order relative to DATABASE_URL-embedded params deserves a second look.
|
| Filename | Overview |
|---|---|
| litellm/proxy/proxy_server.py | Wraps proxy_shutdown_event() in a try/except inside the uvicorn lifespan so a failed shutdown does not abort lifespan exit and orphan the Prisma engine subprocess. |
| litellm/proxy/utils.py | Removes raise e from PrismaClient.disconnect() so a disconnect failure is logged and reported via failure_handler without propagating up to abort the lifespan. |
| litellm/proxy/proxy_cli.py | Adds libpq keepalive defaults to _build_db_connection_url_params; these win over any keepalive settings already embedded in DATABASE_URL because append_query_params calls parsed_query.update(params). |
| tests/proxy_unit_tests/test_aproxy_startup.py | Adds regression test that drives the full lifespan with proxy_shutdown_event monkeypatched to raise, verifying the exception does not propagate. |
| tests/test_litellm/proxy/test_proxy_server.py | Adds test_prisma_disconnect_does_not_reraise_on_failure using __new__ to bypass the prisma binary import; correctly pins swallow-and-log behavior and verifies failure_handler is scheduled. |
| tests/test_litellm/proxy/test_proxy_cli.py | Updates the existing defaults equality test to include keepalive keys; adds two new tests for keepalive presence and override-via-extras. Test modification is appropriate — it reflects the new expected behavior. |
Comments Outside Diff (2)
-
litellm/proxy/proxy_cli.py, line 66-72 (link)Keepalive defaults silently override DATABASE_URL keepalive params
In
append_query_params,parsed_query.update(params)causes the newly-added defaults to overwrite keepalive values already embedded in the DATABASE_URL query string. An operator who set?keepalives_idle=30directly inDATABASE_URLwould see it silently replaced with60. The documented override path requires moving the setting intodatabase_extra_connection_paramsin the config YAML — which operators relying on URL-embedded params may not know to do. Reversing the merge so DATABASE_URL params win over the defaults (while still lettingextra_paramstake highest precedence) would make the precedence order consistent with the "operators can opt out" guarantee. -
litellm/proxy/proxy_server.py, line 978-980 (link)Redundant
{e}in.exception()callverbose_proxy_logger.exception(...)already appends the exception message and traceback automatically. The{e}in the f-string duplicates the message in the log output. Consider using a plain string to avoid the duplication.
Reviews (1): Last reviewed commit: "fix(proxy): prevent Prisma engine orphan..." | Re-trigger Greptile
a82d129 to
ea24531
Compare
|
@greptileai re-review please — addressed both P1s:
|
ea24531 to
43c25f4
Compare
|
Also dropped the redundant |
…failure (BerriAI#26619) When a worker recycles (Gunicorn --max_requests_before_restart, uvicorn worker crash, deploy churn) and the Prisma disconnect fails during lifespan shutdown, the exception propagates up the uvicorn lifespan, uvicorn skips the Python-level atexit/SIGTERM cleanup, and the Prisma query-engine subprocess is SIGKILL'd without closing its Postgres connections. Without TCP keepalives on DATABASE_URL, those dead sessions sit on the DB side for the OS keepalive default (~7200s) and accumulate toward max_connections. Three surgical changes, all scoped to the bug in BerriAI#26619: 1. proxy_server.py: wrap the await proxy_shutdown_event() call inside proxy_startup_event in try/except. A failed shutdown event must not prevent normal lifespan exit; subprocess termination depends on reaching the atexit/SIGTERM cleanup cleanly. 2. utils.py: stop re-raising from PrismaClient.disconnect(). The existing logging + failure_handler task already records the failure; re-raising serves no caller, and the only caller (the lifespan) is actively harmed by it. Also remove the @backoff.on_exception decorator that wrapped disconnect() — with the exception now swallowed inside the function body, the decorator never sees a raise and the retry contract it advertises is permanently inert. 3. proxy_cli.py: emit libpq TCP keepalive defaults (keepalives=1, keepalives_idle=60, keepalives_interval=10, keepalives_count=3) from _build_db_connection_url_params so the kernel reaps dead Postgres sessions in ~90s instead of ~7200s, even when an engine subprocess is killed before its session closes cleanly. Defaults are skipped for any keepalive key already present in the existing DATABASE_URL (new existing_url argument), so operators who have set explicit values (e.g. keepalives=0) in their DATABASE_URL keep them. extra_params still wins last for explicit per-config overrides. Tests: - test_proxy_lifespan_swallows_shutdown_event_exception drives the full lifespan with proxy_shutdown_event monkeypatched to raise; pins that RuntimeError does not propagate out of the context manager exit and that the failure is logged. - test_proxy_lifespan_runs_shutdown_event_on_clean_exit covers the happy path through the new try/except (lifespan exits cleanly when shutdown does not raise). - test_prisma_disconnect_does_not_reraise_on_failure constructs a stub PrismaClient with self.db.disconnect raising; pins that the call returns normally and failure_handler is scheduled with call_type=disconnect. - test_build_db_connection_url_params_includes_keepalives and test_build_db_connection_url_params_keepalives_overridable_via_extras pin the new defaults and the extras-wins override path. - test_build_db_connection_url_params_respects_existing_url_keepalives pins that operator-set keepalive values in DATABASE_URL are preserved, while keepalive keys NOT in the URL still get defaults. The existing test_build_db_connection_url_params_defaults equality assertion is updated to include the new keepalive keys.
43c25f4 to
5c1b9c3
Compare
|
A note on the This appears to be a codecov false negative driven by
Local
The codecov Happy to retry codecov, push a no-op commit to refresh, or scope the carryforward in |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Relevant issues
Fixes #26619
Problem
When a proxy worker recycles — Gunicorn
--max_requests_before_restart, uvicorn worker crash, deploy churn — the Prismadisconnect()is awaited inside the uvicorn lifespan. If it fails, the exception propagates up the lifespan, uvicorn skips the Python-levelatexit/SIGTERM cleanup, and the Prisma query-engine subprocess is SIGKILL'd without closing its Postgres connections. Without TCP keepalives onDATABASE_URL, those dead sessions sit on the DB side for the OS keepalive default (~7200s) and accumulate towardmax_connectionsuntil the proxy can no longer write to the DB.I hit this in a self-hosted deployment twice in 7 days (47 orphan Prisma engine processes spawned in a single worker-recycle burst, ~188 leaked TCP sessions,
FATAL: sorry, too many clients already). The traced root cause in the upstream issue matched exactly.Approach
Three surgical patches scoped to #26619 specifically — not the broader "rip out Prisma" effort in #28366 / #28404, which are the right long-term fix but months out.
litellm/proxy/proxy_server.py— wrap theawait proxy_shutdown_event()call insideproxy_startup_event(the uvicorn lifespan) intry/except. A failed shutdown event must not prevent normal lifespan exit; subprocess termination depends on reachingatexit/SIGTERM cleanly.litellm/proxy/utils.py— stop re-raising fromPrismaClient.disconnect(). The existing logging +failure_handlertask already records the failure;disconnect()is called only from shutdown paths, no caller benefits from the re-raise, and the lifespan is actively harmed by it.litellm/proxy/proxy_cli.py— emit libpq TCP keepalive defaults (keepalives=1,keepalives_idle=60,keepalives_interval=10,keepalives_count=3) from_build_db_connection_url_params. Kernel reaps dead Postgres sessions in ~90s instead of ~7200s, so even when an engine subprocess is killed before its session closes cleanly, the leak self-bounds. Operators who don't want the defaults can override viaextra_paramssinceextra_params.updateruns last.This is symmetric: the library no longer creates orphans (Patches 1+2), and the deployment no longer holds dead connections for hours when an orphan does slip through (Patch 3). Operators get protection without changes to their Compose/Helm files.
I verified the libpq keepalive params are accepted by Prisma's PostgreSQL connector in practice — I've been running them via
DATABASE_URLappendage in my homelab deployment as a workaround for the last day, with no driver-level rejection. They flow through as query-string params to the underlying Rust postgres driver.Tests
Added in
tests/test_litellm/andtests/proxy_unit_tests/:test_proxy_lifespan_swallows_shutdown_event_exception(tests/proxy_unit_tests/test_aproxy_startup.py, alongside the existing related test) — drives the full lifespan viaproxy_startup_event(app=None)withproxy_shutdown_eventmonkeypatched to raiseRuntimeError. Pins that the exception does not propagate out of the__aexit__and that the failure is logged. Verified the test fails without Patch 1.test_prisma_disconnect_does_not_reraise_on_failure(tests/test_litellm/proxy/test_proxy_server.py) — constructs a stubPrismaClient(via__new__to bypass the prisma-binary import) withself.db.disconnectraising. Pins thatawait client.disconnect()returns normally and thatfailure_handleris scheduled withcall_type="disconnect".test_build_db_connection_url_params_includes_keepalivesandtest_build_db_connection_url_params_keepalives_overridable_via_extras(tests/test_litellm/proxy/test_proxy_cli.py) — pin the new keepalive defaults and theextras-wins override path.The existing
test_build_db_connection_url_params_defaultsequality assertion was updated to include the new keepalive keys (the only existing test broken by Patch 3).Local run:
Pre-Submission checklist
tests/test_litellm/directory (test_prisma_disconnect_does_not_reraise_on_failure,test_build_db_connection_url_params_includes_keepalives,test_build_db_connection_url_params_keepalives_overridable_via_extras)make test-unitfor the touched files (223 passed intest_litellm/proxy/test_proxy_server.py + test_litellm/proxy/test_proxy_cli.py + proxy_unit_tests/test_aproxy_startup.py)@greptileaiand received a Confidence Score of at least 4/5 — will request immediately after openingType
🐛 Bug Fix
Changes
litellm/proxy/proxy_server.py— wrapawait proxy_shutdown_event()intry/exceptlitellm/proxy/utils.py—PrismaClient.disconnect()no longer re-raiseslitellm/proxy/proxy_cli.py— libpq keepalive defaults in_build_db_connection_url_paramstests/proxy_unit_tests/test_aproxy_startup.py— lifespan-shutdown regression testtests/test_litellm/proxy/test_proxy_server.py—PrismaClient.disconnectno-reraise regression testtests/test_litellm/proxy/test_proxy_cli.py— keepalive defaults + override tests