Skip to content

fix(workflows): Handle duplicated AlertRuleWorkflow entries for a Workflow more gracefully #113138

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.

Sign up for GitHub

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

Merged
kcons merged 1 commit into from
Apr 16, 2026
Merged

fix(workflows): Handle duplicated AlertRuleWorkflow entries for a Workflow more gracefully #113138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/sentry/rules/history/backends/postgres.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,17 @@
return results


def get_rule_workflow_ids(target: Workflow | Rule) -> IdPair:
def get_rule_workflow_ids(target: Workflow | Rule, project_id: int | None = None) -> IdPair:
if isinstance(target, Workflow):
workflow_id = target.id
try:
alert_rule_workflow = AlertRuleWorkflow.objects.get(workflow=target)
qs = AlertRuleWorkflow.objects.filter(workflow=target)
if project_id is not None:
# rule_id is not a Django FK, so we use an __in subquery to
# scope to the project. Django evaluates this as a single SQL
# statement (subquery), not a separate round-trip.
qs = qs.filter(rule_id__in=Rule.objects.filter(project_id=project_id).values("id"))
alert_rule_workflow = qs.get()
Copy link
Copy Markdown
Contributor

@cursor cursor bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MultipleObjectsReturned not caught despite graceful-handling intent

Medium Severity

The qs.get() call can still raise MultipleObjectsReturned if more than one AlertRuleWorkflow matches after filtering — for example, if two distinct rules in the same project both point to the same workflow. The try/except only catches AlertRuleWorkflow.DoesNotExist, so a MultipleObjectsReturned exception would propagate as an unhandled 500 error. Since the PR's stated goal is to "avoid crashing," catching (or otherwise handling) MultipleObjectsReturned — e.g., by using qs.first() or adding it to the except clause — would make the fix more robust.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 926e629. Configure here.

Copy link
Copy Markdown
Contributor

@saponifi3d saponifi3d Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's true, but shouldn't convention avoid this and we'd want to be notified about this as a different error?

Copy link
Copy Markdown
Member Author

@kcons kcons Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep.

rule_id = alert_rule_workflow.rule_id
except AlertRuleWorkflow.DoesNotExist:
rule_id = None
Comment on lines 67 to 69
Copy link
Copy Markdown
Contributor

@sentry sentry bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The call to AlertRuleWorkflow.objects.get() does not handle the MultipleObjectsReturned exception, which can occur if duplicate entries exist for a project, leading to a crash.
Severity: HIGH

Suggested Fix

Modify the except block to catch both AlertRuleWorkflow.DoesNotExist and AlertRuleWorkflow.MultipleObjectsReturned exceptions. This pattern is common elsewhere in the codebase for handling .get() calls.

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/rules/history/backends/postgres.py#L67-L69

Potential issue: The `get_rule_workflow` function calls
`AlertRuleWorkflow.objects.get()` to retrieve a workflow. While the query is filtered by
`project_id`, it is still possible for multiple `AlertRuleWorkflow` entries to exist for
different rules within the same project. The code only handles the `DoesNotExist`
exception. If the query returns multiple objects, it will raise an unhandled
`MultipleObjectsReturned` exception, causing a crash in the
`ProjectRuleStatsIndexEndpoint` and `ProjectRuleGroupHistoryIndexEndpoint` API handlers.

Did we get this right? 👍 / 👎 to inform future reviews.

Expand Down Expand Up @@ -186,8 +192,9 @@
end: datetime,
cursor: Cursor | None = None,
per_page: int = 25,
project_id: int | None = None,
) -> CursorResult[RuleGroupHistory]:
workflow_id, rule_id = get_rule_workflow_ids(target)
workflow_id, rule_id = get_rule_workflow_ids(target, project_id=project_id)

Check warning on line 197 in src/sentry/rules/history/backends/postgres.py

View check run for this annotation

@sentry/warden / warden: sentry-backend-bugs

get_rule_workflow_ids raises unhandled MultipleObjectsReturned when duplicate AlertRuleWorkflow entries exist 

The `get_rule_workflow_ids` function (line 66) uses `qs.get()` which only catches `AlertRuleWorkflow.DoesNotExist`. The PR title explicitly states that duplicate `AlertRuleWorkflow` entries exist for workflows. While the new `project_id` filter narrows the queryset, if duplicates exist at the (workflow, project) level, `MultipleObjectsReturned` will be raised and propagate as an unhandled exception. Similarly, line 73 has the same issue when the target is a `Rule` - it doesn't use the `project_id` filter at all and still only catches `DoesNotExist`.

if not workflow_id and isinstance(target, Rule):
logger.warning("No workflow associated with rule", extra={"rule_id": rule_id})
Expand Down Expand Up @@ -257,12 +264,13 @@
target: Rule | Workflow,
start: datetime,
end: datetime,
project_id: int | None = None,
) -> Sequence[TimeSeriesValue]:
start = start.replace(tzinfo=timezone.utc)
end = end.replace(tzinfo=timezone.utc)
existing_data: dict[datetime, TimeSeriesValue] = {}

workflow_id, rule_id = get_rule_workflow_ids(target)
workflow_id, rule_id = get_rule_workflow_ids(target, project_id=project_id)

Check warning on line 273 in src/sentry/rules/history/backends/postgres.py

View check run for this annotation

@sentry/warden / warden: sentry-backend-bugs

[P2N-3SZ] get_rule_workflow_ids raises unhandled MultipleObjectsReturned when duplicate AlertRuleWorkflow entries exist (additional location) 

The `get_rule_workflow_ids` function (line 66) uses `qs.get()` which only catches `AlertRuleWorkflow.DoesNotExist`. The PR title explicitly states that duplicate `AlertRuleWorkflow` entries exist for workflows. While the new `project_id` filter narrows the queryset, if duplicates exist at the (workflow, project) level, `MultipleObjectsReturned` will be raised and propagate as an unhandled exception. Similarly, line 73 has the same issue when the target is a `Rule` - it doesn't use the `project_id` filter at all and still only catches `DoesNotExist`.
if not workflow_id and isinstance(target, Rule):
logger.warning("No workflow associated with rule", extra={"rule_id": target.id})
existing_data = self._fetch_rule_fire_history_hourly_stats(target, start, end)
Expand Down
10 changes: 8 additions & 2 deletions src/sentry/rules/history/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,13 @@ def record(
raise NotImplementedError

def fetch_rule_groups_paginated(
self, target: Rule | Workflow, start: datetime, end: datetime, cursor: Cursor, per_page: int
self,
target: Rule | Workflow,
start: datetime,
end: datetime,
cursor: Cursor,
per_page: int,
project_id: int | None = None,
) -> CursorResult[RuleGroupHistory]:
"""
Fetches groups that triggered a rule or workflow within a given timeframe, ordered by number of
Expand All @@ -59,7 +65,7 @@ def fetch_rule_groups_paginated(
raise NotImplementedError

def fetch_rule_hourly_stats(
self, target: Rule | Workflow, start: datetime, end: datetime
self, target: Rule | Workflow, start: datetime, end: datetime, project_id: int | None = None
) -> Sequence[TimeSeriesValue]:
"""
Fetches counts of how often a rule has fired withing a given datetime range, bucketed by
Expand Down
4 changes: 3 additions & 1 deletion src/sentry/rules/history/endpoints/project_rule_group_history.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ def get(self, request: Request, project: Project, rule: Rule | Workflow) -> Resp
except InvalidParams:
raise ParseError(detail="Invalid start and end dates")

results = fetch_rule_groups_paginated(rule, start, end, cursor, per_page)
results = fetch_rule_groups_paginated(
rule, start, end, cursor, per_page, project_id=project.id
)

response = Response(serialize(results.results, request.user, RuleGroupHistorySerializer()))
self.add_cursor_headers(request, response, results)
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/rules/history/endpoints/project_rule_stats.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,5 +66,5 @@ def get(self, request: Request, project: Project, rule: Rule | Workflow) -> Resp
Note that results are returned in hourly buckets.
"""
start, end = get_date_range_from_params(request.GET)
results = fetch_rule_hourly_stats(rule, start, end)
results = fetch_rule_hourly_stats(rule, start, end, project_id=project.id)
return Response(serialize(results, request.user, TimeSeriesValueSerializer()))
24 changes: 24 additions & 0 deletions tests/sentry/rules/history/endpoints/test_project_rule_group_history.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from sentry.testutils.cases import APITestCase, TestCase
from sentry.testutils.helpers.datetime import before_now, freeze_time
from sentry.testutils.skips import requires_snuba
from sentry.workflow_engine.models import AlertRuleWorkflow

pytestmark = [requires_snuba]

Expand Down Expand Up @@ -95,6 +96,29 @@ def test(self) -> None:
RuleGroupHistorySerializer(),
)

def test_shared_workflow_across_projects(self) -> None:
project_a = self.project
project_b = self.create_project(organization=self.organization)
rule_a = self.create_project_rule(project=project_a)
rule_b = self.create_project_rule(project=project_b)

# Simulate the bug: point both rules at the same Workflow
arw_a = AlertRuleWorkflow.objects.get(rule_id=rule_a.id)
shared_workflow = arw_a.workflow
arw_b = AlertRuleWorkflow.objects.get(rule_id=rule_b.id)
arw_b.workflow = shared_workflow
arw_b.save()

self.login_as(self.user)
# This would crash with MultipleObjectsReturned without the project_id fix
self.get_success_response(
self.organization.slug,
project_a.slug,
rule_a.id,
start=before_now(days=1),
end=before_now(days=0),
)

def test_invalid_dates(self) -> None:
rule = self.create_project_rule()

Expand Down
24 changes: 24 additions & 0 deletions tests/sentry/rules/history/endpoints/test_project_rule_stats.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from sentry.testutils.helpers.datetime import before_now, freeze_time
from sentry.testutils.silo import control_silo_test
from sentry.testutils.skips import requires_snuba
from sentry.workflow_engine.models import AlertRuleWorkflow

pytestmark = [requires_snuba]

Expand Down Expand Up @@ -75,6 +76,29 @@ def test(self) -> None:
{"date": now, "count": 0},
]

def test_shared_workflow_across_projects(self) -> None:
project_a = self.project
project_b = self.create_project(organization=self.organization)
rule_a = self.create_project_rule(project=project_a)
rule_b = self.create_project_rule(project=project_b)

# Simulate the bug: point both rules at the same Workflow
arw_a = AlertRuleWorkflow.objects.get(rule_id=rule_a.id)
shared_workflow = arw_a.workflow
arw_b = AlertRuleWorkflow.objects.get(rule_id=rule_b.id)
arw_b.workflow = shared_workflow
arw_b.save()

self.login_as(self.user)
# This would crash with MultipleObjectsReturned without the project_id fix
self.get_success_response(
self.organization.slug,
project_a.slug,
rule_a.id,
start=before_now(days=1),
end=before_now(days=0),
)

def test_invalid_date_range(self) -> None:
rule = self.create_project_rule(project=self.event.project)
self.login_as(self.user)
Expand Down
Loading