chore(alerts): Remove PUT and POST legacy paths for metric alerts#116017
Merged
Conversation
github-actions
Bot
added
the
Scope: Backend
ceorourke
changed the title
ceorourke
force-pushed
the
ceorourke/rm-put-post-metric-code
branch
from
ef88a4e to
8316025
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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.
Reviewed by Cursor Bugbot for commit 8316025. Configure here.
| return Response( | ||
| serialize(detector, request.user, WorkflowEngineDetectorSerializer()), | ||
| status=status.HTTP_200_OK, | ||
| ) |
Contributor
There was a problem hiding this comment.
PUT 500 without detector link
High Severity
A successful metric alert PUT can now return a 500. The endpoint unconditionally attempts to load a Detector after an update. If dual_update_alert_rule didn't create a corresponding Detector for a legacy rule, this raises an uncaught Detector.DoesNotExist.
Reviewed by Cursor Bugbot for commit 8316025. Configure here.
Member
Author
There was a problem hiding this comment.
The flag is GA'd so if that is actually happening it's already been happening.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+211
to
+212
| detector = Detector.objects.get(alertruledetector__alert_rule_id=alert_rule.id) | ||
| return Response( |
Contributor
There was a problem hiding this comment.
Bug: Updating an old metric alert rule causes an unhandled Detector.DoesNotExist exception because the endpoint assumes a related Detector object always exists after an update.
Severity: HIGH
Suggested Fix
The endpoint code should handle the Detector.DoesNotExist exception, for example by using a try...except block around the Detector.objects.get() call or by using Detector.objects.filter(...).first() and then checking if the result is None before proceeding.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry/incidents/endpoints/organization_alert_rule_index.py#L211-L212
Potential issue: When updating a metric alert rule created before the dual-write system
was implemented, the PUT endpoint will fail with a 500 error. The
`dual_update_alert_rule` function handles the missing `AlertRuleDetector` for these old
rules by returning `None` after the update. However, the endpoint code then
unconditionally attempts to fetch this non-existent `Detector` via
`Detector.objects.get(alertruledetector__alert_rule_id=updated_rule.id)`. This query
raises an unhandled `Detector.DoesNotExist` exception, causing the request to fail for
any user attempting to modify an older metric alert rule.
Did we get this right? 👍 / 👎 to inform future reviews.
wedamija
approved these changes
May 21, 2026
Comment on lines
+211
to
+212
| detector = Detector.objects.get(alertruledetector__alert_rule_id=alert_rule.id) | ||
| return Response( |
Contributor
There was a problem hiding this comment.
Bug: Removing the try/except Detector.DoesNotExist block when fetching a Detector for an alert rule can cause unhandled exceptions and 500 errors if the dual-write failed.
Severity: HIGH
Suggested Fix
Reinstate the try/except Detector.DoesNotExist block around the Detector.objects.get(...) calls in both the POST and PUT endpoints. In the except block, fall back to serializing the AlertRule directly, preserving the previous behavior.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry/incidents/endpoints/organization_alert_rule_index.py#L211-L212
Potential issue: In both the alert rule creation (POST) and update (PUT) endpoints, the
`try/except Detector.DoesNotExist` block has been removed. The code now directly queries
for a `Detector` via `Detector.objects.get(alertruledetector__alert_rule_id=...)`. If
the dual-write mechanism fails to create the corresponding `AlertRuleDetector` link,
this query will raise an unhandled `Detector.DoesNotExist` exception, causing a 500
Internal Server Error and preventing the user from creating or updating the alert rule.
Historical data, including previous data migrations, indicates that the dual-write was
not always reliable, making this a realistic failure scenario, particularly for older
alert rules being updated.
Also affects:
src/sentry/incidents/endpoints/organization_alert_rule_details.py:120
Contributor
Backend Test FailuresFailures on
|
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
2 participants
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.
Remove legacy path code using the old serializer for metric alert PUT and POST endpoints.