fix(rpc): fix seer RPC exception handling to default to 500 and preserve rest APIException#112435
Open
fix(rpc): fix seer RPC exception handling to default to 500 and preserve rest APIException#112435
Conversation
Previously, all exceptions from downstream services (Sentry/Snuba) were being caught and re-raised as ValidationError (400), which incorrectly represented issues like rate limits (429) or internal errors (500) as bad request parameters. This change: - Adds a handler to re-raise any REST framework APIException directly, preserving their status codes - Changes the generic exception handler to raise APIException (500) instead of ValidationError (400) - Adds tests to verify that REST framework exceptions are properly re-raised and generic exceptions return 500 This allows Seer to properly differentiate between client errors (4xx) and server errors (5xx) from downstream services. Co-authored-by: Andrew Liu <aliu39@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Test doesn't exercise the new APIException code path
- Mocked in_test_environment() to return False so the test properly exercises the APIException conversion code path instead of re-raising the original exception.
Or push these changes by commenting:
@cursor push bbfaa3b8be
Preview (bbfaa3b8be)
diff --git a/tests/sentry/seer/endpoints/test_seer_rpc.py b/tests/sentry/seer/endpoints/test_seer_rpc.py
--- a/tests/sentry/seer/endpoints/test_seer_rpc.py
+++ b/tests/sentry/seer/endpoints/test_seer_rpc.py
@@ -112,9 +112,12 @@
path = self._get_path("get_organization_slug")
data: dict[str, Any] = {"args": {"org_id": 1}, "meta": {}}
- with patch(
- "sentry.seer.endpoints.seer_rpc.SeerRpcServiceEndpoint._dispatch_to_local_method"
- ) as mock_dispatch:
+ with (
+ patch(
+ "sentry.seer.endpoints.seer_rpc.SeerRpcServiceEndpoint._dispatch_to_local_method"
+ ) as mock_dispatch,
+ patch("sentry.seer.endpoints.seer_rpc.in_test_environment", return_value=False),
+ ):
mock_dispatch.side_effect = RuntimeError("Unexpected internal error")
response = self.client.post(This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 388d7ab. Configure here.
Member
Author
|
Re cursor: not sure how to simulate non-test environment from a test.. code seems straightforward enough to not cover |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Problem
When Seer makes RPC calls to Sentry and encounters downstream errors (like 429 rate limits or 500 errors from Snuba), the current code catches all exceptions and re-raises them as
ValidationError(400). This misrepresents the actual issue, making it appear as if the client sent bad request parameters when the real cause is a downstream service error.Example issue: https://sentry.dev.getsentry.net:7999/issues/7380803020/events/d3e6e1d5ddf44d7b94c86e6ce2435ea6/
Solution
This PR fixes the exception handling in the seer RPC endpoint to raise a generic 500 APIException by default, implying an error happened on sentry server. Also re-raises APIException's (base class for rest framework excs) so developers can bubble their own status codes up to response for Seer.
APIException(which includesThrottled, custom API exceptions, etc.) is now re-raised as-is, preserving its status codeAPIException(500) instead ofValidationError(400) - Unknown errors now correctly return 500 Internal Server Error instead of 400 Bad RequestChanges
SeerRpcServiceEndpoint.post()insrc/sentry/seer/endpoints/seer_rpc.py:except APIException: raisehandler before the generic exception handlerraise ValidationError from etoraise APIException from efor generic exceptionstests/sentry/seer/endpoints/test_seer_rpc.py:test_rest_framework_exceptions_are_reraised- verifies custom API exceptions preserve their status codestest_generic_exceptions_return_500- verifies generic exceptions return 500 instead of 400Testing
Pre-commit checks pass ✅
Slack Thread