Skip to content

Conversation

@mifu67
Copy link
Contributor

@mifu67 mifu67 commented Nov 19, 2025

Re-up of #103131. The change to allow null detectors in DetectorGroup introduced a typing error.

@mifu67 mifu67 requested a review from a team November 19, 2025 19:18
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 19, 2025
@mifu67 mifu67 force-pushed the mifu67/aci/op-inc-serializer branch from e859a40 to c2d77f1 Compare November 19, 2025 19:25
@mifu67 mifu67 marked this pull request as ready for review November 19, 2025 19:27
@mifu67 mifu67 requested a review from a team as a code owner November 19, 2025 19:27
Comment on lines +128 to +130
for group in group_to_open_periods:
for op in group_to_open_periods[group]:
open_periods_to_detectors[op] = groups_to_detectors[group]
Copy link

Choose a reason for hiding this comment

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

Bug: KeyError occurs in groups_to_detectors[group] if group lacks a DetectorGroup or has a null detector.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The get_open_periods_to_detectors() function at line 130 attempts to access groups_to_detectors[group]. A KeyError will be raised if group is not present in groups_to_detectors. This occurs when a Group either has no associated DetectorGroup or its DetectorGroup has a detector field set to None. This condition is possible in production due to the 0099_backfill_metric_issue_detectorgroup.py migration explicitly creating DetectorGroup entries with detector_id=None.

💡 Suggested Fix

Ensure groups_to_detectors is populated for all relevant group objects, or handle the KeyError by providing a default value or skipping groups without a valid detector.

🤖 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/serializers/workflow_engine_incident.py#L128-L130

Potential issue: The `get_open_periods_to_detectors()` function at line 130 attempts to
access `groups_to_detectors[group]`. A `KeyError` will be raised if `group` is not
present in `groups_to_detectors`. This occurs when a `Group` either has no associated
`DetectorGroup` or its `DetectorGroup` has a `detector` field set to `None`. This
condition is possible in production due to the
`0099_backfill_metric_issue_detectorgroup.py` migration explicitly creating
`DetectorGroup` entries with `detector_id=None`.

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

open_periods_to_detectors = {}
for group in group_to_open_periods:
for op in group_to_open_periods[group]:
open_periods_to_detectors[op] = groups_to_detectors[group]
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: KeyError when DetectorGroup has null detector

The code attempts to access groups_to_detectors[group] for all groups, but this dictionary only contains entries where dg.detector is not None (line 124). When a DetectorGroup has a null detector, the group won't be in the dictionary, causing a KeyError at line 130. This breaks the serializer for any open period whose group has a DetectorGroup with a null detector.

Fix in Cursor Fix in Web

if date_ended:
return IncidentStatus.CLOSED.value

return self.priority_to_incident_status[priority]
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Missing LOW priority mapping causes KeyError

The priority_to_incident_status mapping only includes HIGH and MEDIUM priority levels, but PriorityLevel.LOW (value 25) is a valid priority. When get_incident_status is called with a LOW priority group, line 50 raises a KeyError because LOW is missing from the mapping. This breaks serialization for any open period with a low-priority group.

Fix in Cursor Fix in Web

@codecov
Copy link

codecov bot commented Nov 19, 2025

Codecov Report

❌ Patch coverage is 16.66667% with 50 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
.../endpoints/serializers/workflow_engine_incident.py 16.66% 50 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #103674      +/-   ##
===========================================
- Coverage   80.59%    80.53%   -0.06%     
===========================================
  Files        9266      9267       +1     
  Lines      395577    395909     +332     
  Branches    25217     25217              
===========================================
+ Hits       318807    318849      +42     
- Misses      76341     76631     +290     
  Partials      429       429              

@mifu67 mifu67 merged commit 1405273 into master Nov 19, 2025
67 checks passed
@mifu67 mifu67 deleted the mifu67/aci/op-inc-serializer branch November 19, 2025 21:42
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.

3 participants