Skip to content

Conversation

@yuvmen
Copy link
Member

@yuvmen yuvmen commented Feb 10, 2026

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

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 10, 2026
@yuvmen yuvmen requested a review from a team February 10, 2026 23:56
@yuvmen yuvmen requested a review from a team February 10, 2026 23:56
Copy link
Contributor

@cursor cursor bot left a 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
Copy link
Contributor

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)

Fix in Cursor Fix in Web

Copy link
Member Author

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant