-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(snuba_rpc): Fix error handling for non-protofbuf responses for Snuba RPC #107995
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
base: master
Are you sure you want to change the base?
Conversation
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| except (UnicodeDecodeError, AttributeError): | ||
| body = "<non-text response body>" | ||
| raise SnubaRPCError(f"Snuba RPC returned HTTP {http_resp.status}: {body}") | ||
| return error |
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.
Non-protobuf 404/429 responses bypass status-specific error handling
Medium Severity
_parse_error raises SnubaRPCError directly when protobuf decoding fails, which bypasses the caller's status-specific handling for 404 (NotFound) and 429 (SnubaRPCRateLimitExceeded). If an infrastructure component (e.g., load balancer) returns a non-protobuf 429 response, callers that catch SnubaRPCRateLimitExceeded (like in api/utils.py to return Throttled) will instead see a generic SnubaRPCError, resulting in a misleading "Internal error" message rather than a proper rate-limit response.
Additional Locations (1)
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.
This is not a regression since if we failed parsing we bypassed this status-specific handling anyway. Basically if the response is not a protobuf one then we give on being status specific and return a best effort
…uba RPC When the Snuba RPC fails on infra errors and not responses from Snuba (e.g 503 errors) it fails on decode expecting a protobuf response. Added some protection so these are handled correctly, replacing decode errors with what actually happened.
be9a42b to
e24b846
Compare


When the Snuba RPC fails on infra errors and not responses from Snuba (e.g 503 errors) it fails on decode expecting a protobuf response. Added some protection so these are handled correctly, replacing decode errors with what actually happened by attempting to parse the data as utf-8.
noticed it because of this issue