feat(deletions): Use GroupHistory to determine status change #104044
Conversation
Groups requested for deletion can have any last_seen value and does not inform us as to when was the group's status updated. We need to use GroupHistory to determine how long ago the status was changed for deletion.
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| Group.objects.filter(status__in=statuses_to_delete).values_list( | ||
| "id", "project_id", "last_seen" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Bug: Missing batch limit causes memory issues
The BATCH_LIMIT slice was removed from the query, causing all groups with deletion statuses to be loaded into memory instead of the first 1000. This transforms a batched query into a full table scan that could load thousands or millions of groups, potentially exhausting server memory and causing severe performance degradation.
| group_id=group_id, | ||
| status__in=[GroupHistoryStatus.DELETED], | ||
| date_added__lte=status_change_threshold, | ||
| ).first() |
There was a problem hiding this comment.
Bug: Filtering by nonexistent GroupHistory records
The code filters for GroupHistoryStatus.DELETED records, but no GroupHistory entry with this status appears to be created when groups are marked as PENDING_DELETION or DELETION_IN_PROGRESS. The issue_deleted signal only records analytics events, not GroupHistory. This means the filter will never match any records, preventing any groups from being processed for deletion.
| date_added__lte=status_change_threshold, | ||
| ).first() | ||
| if group_history and group_history.date_added <= status_change_threshold: | ||
| groups_by_project[project_id].append(group_id) |
There was a problem hiding this comment.
Bug: N+1 query problem in deletion loop
The loop executes a separate database query for GroupHistory for each group that passes the last_seen check. If thousands of groups meet the criteria, this creates thousands of individual queries instead of using a bulk query or join, significantly degrading performance and increasing database load.
| for group_id, project_id, last_seen in groups: | ||
| if last_seen >= min_last_seen and last_seen <= max_last_seen: | ||
| groups_by_project[project_id].append(group_id) | ||
| if last_seen >= min_last_seen: |
There was a problem hiding this comment.
Bug: Still filtering by last_seen contradicts PR intent
The code still filters groups by last_seen >= min_last_seen, excluding groups with old last_seen values. This contradicts the PR description stating "Groups requested for deletion can have any last_seen value." A group deleted recently but with very old last_seen would incorrectly be excluded from processing.
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
GroupHistory.DELETED is actually not recorded when groups transition to PENDING deletion. The correct source of truth is AuditLogs. There are two other possible solutions here, add a pending_deletion_at column or record a new GroupHistory value like GroupHistory.PENDING_DELETION, but doing it like this is the least disruptive, but does require an RPC call for AuditLogs from CONTORL
Groups requested for deletion can have any last_seen value and does not inform us as to when was the group's status updated. We need to use GroupHistory to determine how long ago the status was changed for deletion.