fix(proxy): treat all PrismaError subclasses as db connection errors - #21773
fix(proxy): treat all PrismaError subclasses as db connection errors#21773ishaan-jaff wants to merge 2 commits into
Conversation
… pubsub fixture Bedrock KB tests were hitting the anthropic API (via berrie proxy) and getting 401s. Fixed by mocking the AsyncHTTPHandler.post call in the 4 failing tests. GCS pubsub v1 test was failing because SpendLogsMetadata added new fields (user_api_key, status, error_information, etc.) that weren't in the expected spend_logs_payload.json fixture.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryReverts the keyword-based narrowing of
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/db/exception_handler.py | Broadens is_database_connection_error to treat ALL PrismaError subclasses as DB connection errors. Reverts regression from #21706 but introduces over-classification — existing unit tests not updated and non-connection callers will misclassify errors. |
| tests/logging_callback_tests/gcs_pub_sub_body/spend_logs_payload.json | Test fixture updated to include additional metadata fields in the spend logs payload JSON. No functional concerns. |
| tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py | Replaces live Anthropic HTTP calls with mocked responses in bedrock knowledge base tests. Good improvement — tests no longer depend on external API availability. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Exception raised] --> B{isinstance PrismaError?}
B -->|Yes - NEW behavior| C[Return True: DB connection error]
B -->|No| D{isinstance DB_CONNECTION_ERROR_TYPES?}
D -->|Yes| C
D -->|No| E{isinstance ClientNotConnected / HTTPClientClosed?}
E -->|Yes| C
E -->|No| F{ProxyException with no_db_connection?}
F -->|Yes| C
F -->|No| G[Return False: not a DB error]
C --> H{Call site context}
H -->|handle_db_exception| I[allow_requests_on_db_unavailable check]
H -->|delete_access_group| J["Returns 503 ⚠️ even for RecordNotFoundError"]
H -->|DB health watchdog| K["Triggers reconnect ⚠️ even for non-connection errors"]
H -->|auth_checks| L["Triggers reconnect ⚠️ even for data errors"]
Last reviewed commit: 54e9d1f
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if isinstance(e, prisma.errors.PrismaError): | ||
| error_message = str(e).lower() | ||
| # Treat generic PrismaError as connection error only when its text | ||
| # clearly indicates transport/connectivity failure. | ||
| connection_keywords = ( | ||
| "can't reach database server", | ||
| "cannot reach database server", | ||
| "can't connect", | ||
| "cannot connect", | ||
| "connection error", | ||
| "connection closed", | ||
| "timed out", | ||
| "timeout", | ||
| "connection refused", | ||
| "network is unreachable", | ||
| "no route to host", | ||
| "broken pipe", | ||
| ) | ||
| if any(keyword in error_message for keyword in connection_keywords): | ||
| return True | ||
| return True |
There was a problem hiding this comment.
Existing unit test will fail
The test test_is_database_connection_error_non_connection_prisma_errors in tests/test_litellm/proxy/db/test_exception_handler.py:75-76 asserts that PrismaError(), DataError, UniqueViolationError, ForeignKeyViolationError, MissingRequiredValueError, RawQueryError, TableNotFoundError, and RecordNotFoundError all return False from is_database_connection_error. With this change, all of them will return True, causing 8 test failures.
This test file was not updated as part of this PR. It needs to be updated to match the new intended behavior (either by asserting True for those cases, or removing those test cases).
Context Used: Rule from dashboard - What: Ensure that any PR claiming to fix an issue includes evidence that the issue is resolved, such... (source)
| if isinstance(e, prisma.errors.PrismaError): | ||
| error_message = str(e).lower() | ||
| # Treat generic PrismaError as connection error only when its text | ||
| # clearly indicates transport/connectivity failure. | ||
| connection_keywords = ( | ||
| "can't reach database server", | ||
| "cannot reach database server", | ||
| "can't connect", | ||
| "cannot connect", | ||
| "connection error", | ||
| "connection closed", | ||
| "timed out", | ||
| "timeout", | ||
| "connection refused", | ||
| "network is unreachable", | ||
| "no route to host", | ||
| "broken pipe", | ||
| ) | ||
| if any(keyword in error_message for keyword in connection_keywords): | ||
| return True | ||
| return True |
There was a problem hiding this comment.
Non-connection errors misclassified as connection errors
This broadening causes unintended side effects in call sites that use is_database_connection_error for purposes other than allow_requests_on_db_unavailable:
-
access_group_endpoints.py:333: ARecordNotFoundErrorduringdelete_access_groupwill now matchis_database_connection_errorand return 503 Service Unavailable instead of falling through to the P2025/not-found check on line 338, which would correctly return 404. -
proxy/utils.py:4113: The DB health watchdog usesis_database_connection_errorto decide whether to trigger a reconnect. With this change, anyPrismaError(e.g. a query syntax error or unique violation from theSELECT 1probe — unlikely but possible in edge cases) would trigger unnecessary reconnection attempts. -
auth/auth_checks.py:2003: Same pattern — aUniqueViolationErrororDataErrorduring key lookup would trigger a DB reconnect attempt when no connectivity issue actually exists.
Consider either:
- Keeping the broad match only for the
handle_db_exception/allow_requests_on_db_unavailablepath (separate method), or - At minimum, reordering the check in
access_group_endpoints.pyso the P2025 check runs beforeis_database_connection_error.
|
Superseded by #21778 — wrong approach (broadened production code instead of fixing the tests) |
1 participant
Relevant issues
Regression introduced in #21706 (commit e012971, "fix(proxy): narrow prisma db connection error classification").
Pre-Submission checklist
tests/litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unitType
🐛 Bug Fix
Changes
is_database_connection_errorwas narrowed in #21706 to only matchPrismaErrorwhen its message contained specific connectivity keywords ("connection refused", "timed out", etc.). Butallow_requests_on_db_unavailable=Trueis supposed to let requests through on ANY prisma DB error — not just network-level ones.Reverts the keyword guard so that any
prisma.errors.PrismaErrorsubclass is treated as a DB error, restoring the behavior before the regression.Tests fixed by this change:
test_delete_access_group_503_on_db_connection_error— delete endpoint was returning 500 instead of 503 whenPrismaErrorwas raised in a transactiontest_handle_authentication_error_db_unavailable[prisma_error0]— barePrismaError()test_handle_authentication_error_db_unavailable[prisma_error1]—DataErrortest_handle_authentication_error_db_unavailable[prisma_error2]—UniqueViolationErrortest_handle_authentication_error_db_unavailable[prisma_error3]—ForeignKeyViolationErrortest_handle_authentication_error_db_unavailable[prisma_error4]—MissingRequiredValueErrortest_handle_authentication_error_db_unavailable[prisma_error5]—RawQueryErrortest_handle_authentication_error_db_unavailable[prisma_error6]—TableNotFoundErrortest_handle_authentication_error_db_unavailable[prisma_error7]—RecordNotFoundError