feat(ACI): Make OrganizationAlertRuleDetails DELETE method backwards compatible by ceorourke · Pull Request #109845 · getsentry/sentry

ceorourke · 2026-03-03T23:12:39Z

Make the delete an alert rule method backwards compatible by accepting a detector ID and using the OrganizationDetectorDetailsEndpoint delete strategy. This also implements a new base class and convert_args to look up either a detector or an alert rule, and removes some old code using the feature flag from before we needed this to work in the UI. For now, this just makes the other methods fail if a detector is passed in.

…compatible

linear · 2026-03-03T23:12:42Z

ISWF-2145 Make OrganizationAlertRuleDetails DELETE method backwards compatible

src/sentry/workflow_engine/endpoints/organization_detector_details.py

src/sentry/incidents/endpoints/bases.py

cursor · 2026-03-03T23:30:02Z

src/sentry/incidents/endpoints/bases.py

+                except Detector.DoesNotExist:
+                    raise ResourceDoesNotExist
+
+            return args, kwargs


Feature-flagged orgs lose access to unmigrated alert rules

Medium Severity

When workflow-engine-rule-serializers is enabled, convert_args has no fallback to look up an AlertRule directly. If an alert rule exists but hasn't been migrated (no AlertRuleDetector row), the AlertRuleDetector lookup fails, then get_object_id_from_fake_id produces a negative number, and the Detector lookup also fails, resulting in a ResourceDoesNotExist (404). A valid, unmigrated alert rule becomes completely inaccessible through this endpoint.

This seems perfectly ok. Unmigrated rules shouldn't exist, and where they do, we seem to intend to ignore them.

github-actions · 2026-03-03T23:44:51Z

Backend Test Failures

Failures on 43261a5 in this run:

tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py::AlertRuleDetailsGetEndpointTest::test_workflow_engine_serializer

— log

tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py:238: in test_workflow_engine_serializer
    resp = self.get_success_response(self.organization.slug, self.alert_rule.id)
src/sentry/testutils/cases.py:628: in get_success_response
    assert_status_code(response, status.HTTP_200_OK)
src/sentry/testutils/asserts.py:47: in assert_status_code
    assert minimum <= response.status_code < maximum, (
E   AssertionError: (400, b'{"alert_rule":["Passing a detector through this endpoint is not yet supported"]}')
E   assert 400 < 201
E    +  where 400 = <Response status_code=400, "application/json">.status_code

tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py::AlertRuleDetailsPutEndpointTest::test_workflow_engine_serializer

— log

tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py:808: in test_workflow_engine_serializer
    resp = self.get_success_response(
src/sentry/testutils/cases.py:633: in get_success_response
    assert_status_code(response, status.HTTP_200_OK)
src/sentry/testutils/asserts.py:47: in assert_status_code
    assert minimum <= response.status_code < maximum, (
E   AssertionError: (400, b'{"alert_rule":["Passing a detector through this endpoint is not yet supported"]}')
E   assert 400 < 201
E    +  where 400 = <Response status_code=400, "application/json">.status_code

src/sentry/incidents/endpoints/organization_alert_rule_details.py

github-actions · 2026-03-04T00:20:16Z

Backend Test Failures

Failures on 4cda2ac in this run:

tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py::AlertRuleDetailsPutEndpointTest::test_workflow_engine_serializer

— log

tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py:808: in test_workflow_engine_serializer
    resp = self.get_success_response(
src/sentry/testutils/cases.py:633: in get_success_response
    assert_status_code(response, status.HTTP_200_OK)
src/sentry/testutils/asserts.py:47: in assert_status_code
    assert minimum <= response.status_code < maximum, (
E   AssertionError: (400, b'{"alert_rule":["Passing a detector through this endpoint is not yet supported"]}')
E   assert 400 < 201
E    +  where 400 = <Response status_code=400, "application/json">.status_code

tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py::AlertRuleDetailsGetEndpointTest::test_workflow_engine_serializer

— log

tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py:238: in test_workflow_engine_serializer
    resp = self.get_success_response(self.organization.slug, self.alert_rule.id)
src/sentry/testutils/cases.py:628: in get_success_response
    assert_status_code(response, status.HTTP_200_OK)
src/sentry/testutils/asserts.py:47: in assert_status_code
    assert minimum <= response.status_code < maximum, (
E   AssertionError: (400, b'{"alert_rule":["Passing a detector through this endpoint is not yet supported"]}')
E   assert 400 < 201
E    +  where 400 = <Response status_code=400, "application/json">.status_code

saponifi3d

just super nits, lgtm!

src/sentry/incidents/endpoints/organization_alert_rule_details.py

saponifi3d · 2026-03-04T18:33:09Z

src/sentry/incidents/endpoints/bases.py

+
+
+class WorkflowEngineOrganizationAlertRuleEndpoint(OrganizationAlertRuleEndpoint):
+    def convert_args(


Not for this pr / yet, but this code looks fairly similar to the convert args on WorkflowEngineOrganizationRuleEndpoint -- if we have another instance of this, it might be worth refactoring a little to pull the shared logic out.

src/sentry/incidents/endpoints/organization_alert_rule_details.py

kcons · 2026-03-04T18:41:51Z

src/sentry/workflow_engine/endpoints/organization_detector_details.py

+        raise PermissionDenied
+
+    validator = get_detector_validator(request, detector.project, detector.type, instance=detector)
+    validator.delete()


Don't we want to do this async so the DataSources can get cleaned up too?
I realize this is copied code, but.. a concern.

The base class schedules the deletion and marks it as pending https://github.com/getsentry/sentry/blob/master/src/sentry/workflow_engine/endpoints/validators/base/detector.py#L221-L231

I added a data source and data source detector to the test to make sure they're removed

ah! I totally missed that this was the validator, not the detector >_<
carry on.

kcons · 2026-03-04T18:44:21Z

src/sentry/incidents/endpoints/organization_alert_rule_details.py

+
+                return Response(status=status.HTTP_204_NO_CONTENT)
+        except AlreadyDeletedError:
+            return Response(


Interesting, I'd expect us to return a 204 or maybe a 404 if it is already deleted.

src/sentry/incidents/endpoints/bases.py

src/sentry/incidents/endpoints/organization_alert_rule_details.py

…tector

sentry · 2026-03-04T19:06:29Z

src/sentry/incidents/endpoints/bases.py

+        if features.has("organizations:workflow-engine-rule-serializers", organization):
+            try:
+                ard = AlertRuleDetector.objects.get(
+                    alert_rule_id=validated_alert_rule_id,
+                    detector__project__organization=organization,
+                )
+                kwargs["alert_rule"] = ard.detector


Bug: With the workflow-engine-rule-serializers flag on, GET/PUT requests for dual-written alert rules incorrectly resolve to a Detector and fail with a 400 error.
_{Severity: HIGH}

Suggested Fix

Update the fetch_alert_rule and update_alert_rule methods to correctly handle Detector objects that are resolved from an alert_rule_id, similar to how the DELETE method handles them. This will prevent the endpoint from incorrectly returning a 400 error for valid, dual-written alert rules.

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/bases.py#L125-L131 Potential issue: When the `organizations:workflow-engine-rule-serializers` feature flag is enabled, the `convert_args` method resolves alert rule IDs for dual-written rules into `Detector` objects. However, the GET and PUT endpoint methods that receive this object are implemented to reject `Detector` instances and return a 400 error. This causes requests to fetch or update these specific alert rules to fail for any organization with the feature flag active, breaking a core workflow.

that is intentional

sentry · 2026-03-04T19:06:29Z

src/sentry/incidents/endpoints/organization_alert_rule_details.py

+            if isinstance(alert_rule, Detector):
+                project = alert_rule.project
+            else:
+                # check to see if there's a project associated with the alert rule
+                project = alert_rule.projects.get()


Bug: A missing .select_related("project") when fetching Detector objects in convert_args causes an extra database query per request when project access is checked.
_{Severity: MEDIUM}

Suggested Fix

Add .select_related("project") to the queries that fetch Detector instances in the convert_args method. This applies to both the lookup via AlertRuleDetector.objects.get() and the direct lookup via Detector.objects.get() to ensure the project is pre-fetched and avoid the extra query.

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_details.py#L309-L313 Potential issue: In `WorkflowEngineOrganizationAlertRuleEndpoint.convert_args`, when a `Detector` instance is fetched, it is done without pre-fetching the related project via `select_related("project")`. Subsequently, the `_check_project_access` decorator accesses `alert_rule.project`, which triggers an additional, unnecessary database query to fetch the project for each request. This introduces a performance regression for all GET, PUT, and DELETE requests that handle a `Detector` object.

cursor

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.}

cursor · 2026-03-04T19:13:42Z

src/sentry/incidents/endpoints/organization_alert_rule_details.py

+        except AlreadyDeletedError:
+            return Response(
+                "This rule has already been deleted", status=status.HTTP_400_BAD_REQUEST
+            )


Atomic transaction can roll back successful detector deletion

Medium Severity

The Detector path in remove_alert_rule wraps both remove_detector and delete_alert_rule in a single transaction.atomic block. If delete_alert_rule raises any uncaught exception (e.g., an unexpected DB error), the entire transaction rolls back — including the detector deletion that remove_detector already completed. The code's intent is to always delete the detector and optionally clean up the linked AlertRule (as evidenced by catching AlertRuleDetector.DoesNotExist and AlertRule.DoesNotExist to still return 204). Wrapping both in one transaction contradicts that intent, since a failure deleting the AlertRule also undoes the detector deletion, potentially leaving the user unable to delete the detector on retry if the alert rule issue persists.

cursor · 2026-03-04T19:13:42Z

src/sentry/incidents/endpoints/organization_alert_rule_details.py

+        except AlreadyDeletedError:
+            return Response(
+                "This rule has already been deleted", status=status.HTTP_400_BAD_REQUEST
+            )


Unreachable AlreadyDeletedError handler rolls back detector deletion

Low Severity

The except AlreadyDeletedError handler on the Detector path is effectively unreachable. AlreadyDeletedError is only raised by delete_alert_rule when alert_rule.status == AlertRuleStatus.SNAPSHOT.value, but AlertRule.objects.get() uses the AlertRuleManager which excludes SNAPSHOT-status rules. Since Django model instances don't auto-refresh, the in-memory status will never be SNAPSHOT at the point delete_alert_rule checks it. This dead handler adds misleading code and, if somehow reached, would roll back the detector deletion — returning a 400 "already deleted" error even though the detector has not been deleted.

kcons

ok, I think the bots may have a point in one or two of these cases, but I'll let you resolve that.

ceorourke added 3 commits March 3, 2026 12:42

re-trigger

b0d9542

feat(ACI): Make OrganizationAlertRuleDetails DELETE method backwards …

8ba9198

…compatible

use correct deletion method

b61d391

ceorourke requested review from a team as code owners March 3, 2026 23:12

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 3, 2026

sentry bot reviewed Mar 3, 2026

View reviewed changes

src/sentry/workflow_engine/endpoints/organization_detector_details.py Outdated Show resolved Hide resolved

cursor bot reviewed Mar 3, 2026

View reviewed changes

src/sentry/workflow_engine/endpoints/organization_detector_details.py Outdated Show resolved Hide resolved

oops actually return

1380268

vercel bot deployed to Preview March 3, 2026 23:25 View deployment

cursor bot reviewed Mar 3, 2026

View reviewed changes

dual delete: the other way

384f290

vercel bot deployed to Preview March 4, 2026 00:00 View deployment

cursor bot reviewed Mar 4, 2026

View reviewed changes

src/sentry/incidents/endpoints/organization_alert_rule_details.py Outdated Show resolved Hide resolved

Add alreadydeletederror, update tests

019ebab

vercel bot deployed to Preview March 4, 2026 00:27 View deployment

saponifi3d approved these changes Mar 4, 2026

View reviewed changes

rm msg

5725e44

vercel bot deployed to Preview March 4, 2026 18:39 View deployment

kcons requested changes Mar 4, 2026

View reviewed changes

ceorourke enabled auto-merge (squash) March 4, 2026 18:48

ceorourke disabled auto-merge March 4, 2026 18:48

some pr concerns

7ca2784

vercel bot deployed to Preview March 4, 2026 18:55 View deployment

cursor bot reviewed Mar 4, 2026

View reviewed changes

src/sentry/incidents/endpoints/organization_alert_rule_details.py Outdated Show resolved Hide resolved

add to test to make sure we delete the data source and data source de…

0be8cdd

…tector

vercel bot deployed to Preview March 4, 2026 19:02 View deployment

consistently use target

d01f633

vercel bot deployed to Preview March 4, 2026 19:05 View deployment

sentry bot reviewed Mar 4, 2026

View reviewed changes

cursor bot reviewed Mar 4, 2026

View reviewed changes

kcons approved these changes Mar 4, 2026

View reviewed changes



		class WorkflowEngineOrganizationAlertRuleEndpoint(OrganizationAlertRuleEndpoint):
		def convert_args(

Uh oh!

Conversation

ceorourke commented Mar 3, 2026

Uh oh!

linear bot commented Mar 3, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cursor bot Mar 3, 2026

Choose a reason for hiding this comment

Feature-flagged orgs lose access to unmigrated alert rules

Uh oh!

kcons Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backend Test Failures

Uh oh!

Uh oh!

github-actions bot commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backend Test Failures

Uh oh!

saponifi3d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

saponifi3d Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kcons Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

ceorourke Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

kcons Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

kcons Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sentry bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

ceorourke Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

sentry bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor bot Mar 4, 2026

Choose a reason for hiding this comment

Atomic transaction can roll back successful detector deletion

Uh oh!

cursor bot Mar 4, 2026

Choose a reason for hiding this comment

Unreachable AlreadyDeletedError handler rolls back detector deletion

Uh oh!

kcons left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Mar 3, 2026 •

edited

Loading

github-actions bot commented Mar 4, 2026 •

edited

Loading

Unreachable `AlreadyDeletedError` handler rolls back detector deletion