fix(aci): handle fake incident ids in IGOP lookup #103937
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| # try looking up GroupOpenPeriod directly using calculated open_period_id | ||
| fake_id = incident_identifier or incident_id | ||
| if fake_id: | ||
| calculated_open_period_id = get_object_id_from_fake_id(int(fake_id)) |
There was a problem hiding this comment.
Bug: Real incident IDs incorrectly treated as fake IDs
The fallback logic applies get_object_id_from_fake_id() to both incident_id and incident_identifier, but incident_id represents a real database ID from the IncidentGroupOpenPeriod table, not a fake ID. Subtracting 10^9 from a real incident ID produces a negative or incorrect value, preventing legitimate lookups when a real incident_id has no matching IGOP row.
There was a problem hiding this comment.
Since we're sending workflow engine notifications for everyone now, all incidents are either real and fetched via IGOP table, or faked using the fake ID formula.
src/sentry/workflow_engine/endpoints/organization_incident_groupopenperiod_index.py
Outdated
Show resolved
Hide resolved
src/sentry/workflow_engine/endpoints/organization_incident_groupopenperiod_index.py
Outdated
Show resolved
Hide resolved
src/sentry/workflow_engine/endpoints/organization_incident_groupopenperiod_index.py
Show resolved
Hide resolved
src/sentry/workflow_engine/endpoints/serializers/incident_groupopenperiod_serializer.py
Outdated
Show resolved
Hide resolved
tests/sentry/workflow_engine/endpoints/test_organization_incident_groupopenperiod.py
Outdated
Show resolved
Hide resolved
mifu67
left a comment
mifu67
left a comment
There was a problem hiding this comment.
Math looks good! Idk about the cursor comments ¯_(ツ)_/¯ and let's merge the backend changes first.
src/sentry/workflow_engine/endpoints/organization_incident_groupopenperiod_index.py
Outdated
Show resolved
Hide resolved
src/sentry/workflow_engine/endpoints/organization_incident_groupopenperiod_index.py
Outdated
Show resolved
Hide resolved
| gop_queryset = gop_queryset.filter(group_id=group_id) | ||
|
|
||
| if open_period_id: | ||
| gop_queryset = gop_queryset.filter(id=open_period_id) |
There was a problem hiding this comment.
Bug: Conflicting ID filters in fallback query
The fallback logic filters GroupOpenPeriod by id=calculated_open_period_id on line 92, then filters by id=open_period_id on line 100 if provided. Since a row can't have two different IDs, this makes the query impossible to satisfy when open_period_id differs from calculated_open_period_id. The filter on line 100 either contradicts the earlier filter or is redundant.
There was a problem hiding this comment.
ignoring, in theory you shouldn't be passing incident and GOP ids together anyways
…#103938) Should probably be merged before #103937. If an open period comes in without a corresponding entry in the IGOP table, create a fake ID for the incident and pass it to the frontend, where it will be used for redirect logic. Do the same for detector IDs that don't have entries in the AlertRuleDetector table.
When looking up a GroupOpenPeriod via incident id, it's possible that there is no corresponding row in the IncidentGroupOpenPeriod table. We then assume that the incident id that was passed is a fake id, so we can use
get_object_id_from_fake_id()to get the open period id and group.