-
-
Notifications
You must be signed in to change notification settings - Fork 11k
fix(proxy): treat all PrismaError subclasses as db connection errors #21773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,25 +43,7 @@ def is_database_connection_error(e: Exception) -> bool: | |
| ): | ||
| return True | ||
| 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 | ||
|
Comment on lines
45
to
+46
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-connection errors misclassified as connection errors This broadening causes unintended side effects in call sites that use
Consider either:
|
||
| if isinstance(e, ProxyException) and e.type == ProxyErrorTypes.no_db_connection: | ||
| return True | ||
| return False | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing unit test will fail
The test
test_is_database_connection_error_non_connection_prisma_errorsintests/test_litellm/proxy/db/test_exception_handler.py:75-76asserts thatPrismaError(),DataError,UniqueViolationError,ForeignKeyViolationError,MissingRequiredValueError,RawQueryError,TableNotFoundError, andRecordNotFoundErrorall returnFalsefromis_database_connection_error. With this change, all of them will returnTrue, 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
Truefor 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)