fix(proxy): recover Prisma DB reconnect loop when client is disconnected - #32323
Conversation
|
|
Greptile SummaryThis PR fixes a production reconnect deadlock where a Prisma client wedged in the disconnected state caused every reconnect attempt to fail. Both
Confidence Score: 5/5Safe to merge; the change is narrowly scoped to the engine-PID inspection path and every reconnect entry point is covered by the guard. The fix is correct and minimal: a two-line The pre-existing
|
| Filename | Overview |
|---|---|
| litellm/proxy/db/prisma_client.py | Adds is_connected() guard in PrismaWrapper._get_engine_pid() to prevent ClientNotConnectedError from escaping into the reconnect path when the client is disconnected. |
| litellm/proxy/utils.py | Adds is_connected() guard in PrismaClient._get_engine_pid() and switches from self.db._original_prisma to self.writer_db._original_prisma, correctly routing through the writer wrapper even when read-replica routing is active. |
| tests/test_litellm/proxy/conftest.py | Centralises StubClientNotConnectedError, DisconnectedPrisma, and the disconnected_prisma fixture in the shared proxy conftest, replacing the previous copy-paste across individual test files. |
| tests/test_litellm/proxy/db/test_prisma_client.py | Adds is_connected = MagicMock(return_value=True) to three existing tests that simulate connected clients, and adds two new regression tests covering _get_engine_pid() and recreate_prisma_client() when the client is in the disconnected state. |
| tests/test_litellm/proxy/db/test_prisma_planned_engine_restart.py | Adds is_connected = MagicMock(return_value=True) to the _make_wrapper helper to keep existing tests consistent with the new guard. |
| tests/test_litellm/proxy/db/test_prisma_self_heal.py | Fixes mock_prisma_binary to yield the module (enabling new test to configure Prisma.return_value), and adds an end-to-end regression test driving the full attempt_db_reconnect → _run_reconnect_cycle → recreate_prisma_client path from a wedged disconnected state. |
| tests/test_litellm/proxy/utils/prisma_and_spend/test_prisma_client_engine_watcher.py | Adds is_connected = MagicMock(return_value=True) to the PID-extraction test and a new regression test for the disconnected case; the pre-existing test_get_engine_pid_returns_zero_when_engine_attr_missing test now passes via a different code path than its name suggests. |
Reviews (5): Last reviewed commit: "fix(proxy): recover Prisma DB reconnect ..." | Re-trigger Greptile
8d15e6f to
05935a4
Compare
|
@greptileai please review the current head 05935a4. It addresses the P2 (both remaining tests now declare is_connected() -> True) and removes issue references from docstrings |
Greptile SummaryThis PR fixes a persistent reconnect-loop deadlock where Prisma pods could never self-heal after the active client entered the disconnected state. Both
Confidence Score: 4/5Safe to merge — the fix is minimal, targets exactly the broken call sites, and is backed by four regression tests that reproduce the wedged state end-to-end. The production change is a single defensive early-return in two symmetrical No files require special attention;
|
| Filename | Overview |
|---|---|
| litellm/proxy/db/prisma_client.py | Guards _get_engine_pid with an is_connected() pre-check so a disconnected client returns 0 instead of raising ClientNotConnectedError, unblocking the reconnect path. |
| litellm/proxy/utils.py | Mirrors the is_connected() guard from PrismaWrapper and also corrects self.db._original_prisma to self.writer_db._original_prisma, which properly unwraps through RoutingPrismaWrapper in read-replica deployments. |
| tests/test_litellm/proxy/db/test_prisma_client.py | Adds regression test for disconnected client returning 0 from _get_engine_pid, and an end-to-end test that recreate_prisma_client succeeds from the wedged state. Existing mock updated to declare is_connected() → True accurately. |
| tests/test_litellm/proxy/db/test_prisma_planned_engine_restart.py | Adds is_connected = MagicMock(return_value=True) to existing mock to satisfy the new is_connected() pre-check; does not weaken existing assertions. |
| tests/test_litellm/proxy/db/test_prisma_self_heal.py | Fixture updated to yield the mock module (needed for the new test), and adds a full reconnect-path regression test from the wedged disconnected state (attempt_db_reconnect → recreate_prisma_client). |
| tests/test_litellm/proxy/utils/prisma_and_spend/test_prisma_client_engine_watcher.py | Adds regression test for PrismaClient._get_engine_pid returning 0 on a disconnected client; existing mock updated to declare is_connected() → True accurately. |
Reviews (2): Last reviewed commit: "fix(proxy): recover Prisma DB reconnect ..." | Re-trigger Greptile
| mock_kill.assert_not_called() # PID was 0, kill skipped | ||
| mock_new_prisma.connect.assert_awaited_once() | ||
|
|
||
|
|
||
| class _StubClientNotConnectedError(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class _DisconnectedPrisma: | ||
| """Mimics prisma-client-py after disconnect(): ``is_connected()`` is False | ||
| and the ``_engine`` property raises ``ClientNotConnectedError``.""" | ||
|
|
||
| def is_connected(self) -> bool: | ||
| return False | ||
|
|
||
| @property | ||
| def _engine(self) -> None: | ||
| raise _StubClientNotConnectedError( | ||
| "Client is not connected to the query engine, you must call `connect()` " |
There was a problem hiding this comment.
Duplicated test stub across three files
_StubClientNotConnectedError and _DisconnectedPrisma are copy-pasted verbatim into test_prisma_client.py, test_prisma_self_heal.py, and test_prisma_client_engine_watcher.py. Consider extracting them into a shared tests/test_litellm/proxy/db/conftest.py or a _test_helpers.py module so a future change to the stub only needs to happen in one place.
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!
Once the active Prisma client is in the disconnected state, every DB call raises ClientNotConnectedError. The reconnect machinery was supposed to recover from this, but _get_engine_pid() inspected the broken client via prisma's _engine property, which re-raises that same error, so recreate_prisma_client failed before it could build a replacement client and the proxy looped on failed reconnects forever (issue #28322 showed 1486+ consecutive failures over 30 days with zero recoveries) Guard both _get_engine_pid implementations with is_connected() so a disconnected client reads as "no engine" (pid 0) and the recreate path proceeds to construct and connect a fresh client
05935a4 to
8417b96
Compare
|
@greptileai please review the current head 8417b96. It deduplicates the test stub into tests/test_litellm/proxy/conftest.py as a shared disconnected_prisma fixture, addressing the maintainability observation |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@greptileai review head 8417b96 |
Relevant issues
Fixes #28322
Linear ticket
Resolves LIT-4161
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@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).
Screenshots / Proof of Fix
The wedged state from issue #28322 is a Prisma client that is disconnected from its query engine; production pods reach it through rare shutdown/cancellation races after days of uptime (the issue logs show 1486+ consecutive reconnect failures over 30 days with zero recoveries). To reproduce it on demand against a live proxy, I used a custom callback as a fault injector that calls the real
disconnect()on the active client when a magic message arrives, which puts the proxy into byte-for-byte the same state (every DB call raisesClientNotConnectedError)Fault injector loaded via
litellm_settings.callbacks:Live proxy on
localhost:4161with a real Postgres container and real OpenAI calls; watchdog tuned for a fast demo loop (PRISMA_HEALTH_WATCHDOG_INTERVAL_SECONDS=5,PRISMA_RECONNECT_COOLDOWN_SECONDS=2)Before (unfixed)
Proxy log: the reconnect loop fails forever with the exact error signature from the issue, and never recovers (25 consecutive failures and climbing when I stopped the run;
reconnect succeededcount stays 0)After (this fix)
Same trigger, same environment. The engine watcher detects the death from the disconnect and the very first reconnect attempt recovers:
I also verified on a live proxy that the two adjacent scenarios still self-heal with this change: killing the query engine with the DB up (recovers in one cycle) and killing it during a
docker pauseDB outage (recovers on the first cycle afterdocker unpause)Independent e2e verification (Devin)
A separate Devin session independently reproduced and verified the fix end to end on its own environment (fresh clone, real Postgres 16, same fault injector, same config for both runs; only the two guarded files differ). On the fixed branch the wedge trigger leads to
Prisma DB reconnect succeededwithin one watchdog cycle (~10 seconds) with zeroreconnect failedlines, andGET /key/infowith a virtual key returns HTTP 200 afterwards. On the unfixed merge-base (5b93ba0ada) the same trigger loopsPrisma DB reconnect failed (N consecutive) ... Client is not connected to the query engineclimbing to 27 on camera with zero recoveries,GET /key/inforeturns HTTP 500 and the user-facing chat completion returns HTTP 503no_db_connection. Screen recording of both runs:(video attached below)
Type
🐛 Bug Fix
Changes
Once the active Prisma client is in the disconnected state, every DB call raises
prisma.errors.ClientNotConnectedError. The reconnect machinery is the only way out of that state, but bothPrismaWrapper._get_engine_pid(litellm/proxy/db/prisma_client.py) andPrismaClient._get_engine_pid(litellm/proxy/utils.py) inspected the broken client through prisma's_engineproperty, which re-raises that sameClientNotConnectedError, and they only caught(AttributeError, TypeError). Sorecreate_prisma_clientfailed at the very first line, before it could construct a replacement client, and every subsequent attempt (health watchdog, auth-triggered reconnect, IAM token refresh) died the same way; the pod never recovered without a restartThe fix guards both implementations with
is_connected()so a disconnected client reads as "no engine subprocess" (pid 0); the recreate path then skips the kill step and proceeds to construct and connect a fresh client, which is exactly what recovery requires. All reconnect entry points funnel through these two call sites, so the guard covers the watchdog, the auth path, the IAM refresh path and the read replica recreate for free. The guard is safe against a torn read because prisma's_engineproperty only raises when the internal engine is None, which is exactly whenis_connected()returns False, and there is no await between the check and the accessProvenance: the narrow
except (AttributeError, TypeError)dates back to the engine zombie-recovery work (#21899 and commit 1f04fa2). The reconnect path that most easily produced a disconnected client in production (adisconnect()thenconnect()sequence, where a failed connect left the client permanently disconnected) was later replaced by the kill-then-construct flow in commit fbcdacc, which is why recent builds rarely enter the state; the recovery gap itself stayed latent until now. Reports on issue #28322 span builds that still had the old triggerRegression tests (all four fail on the unfixed code and pass with the fix):
test_get_engine_pid_returns_zero_for_disconnected_clientandtest_recreate_prisma_client_recovers_from_disconnected_clientintests/test_litellm/proxy/db/test_prisma_client.pytest_heavy_reconnect_recovers_from_disconnected_prisma_clientintests/test_litellm/proxy/db/test_prisma_self_heal.py, which drives the full real path (attempt_db_reconnect->_run_reconnect_cycle->recreate_prisma_client) from the wedged statetest_get_engine_pid_returns_zero_when_client_disconnectedintests/test_litellm/proxy/utils/prisma_and_spend/test_prisma_client_engine_watcher.pyTwo existing tests were updated to declare
is_connected() -> Trueon their mocked clients, matching the state they were already simulating