From a2b4d7ca1f0b2be33d97bd82ba65e0f54f0da073 Mon Sep 17 00:00:00 2001 From: Armen Zambrano G <44410+armenzg@users.noreply.github.com> Date: Fri, 18 Jul 2025 14:54:23 -0400 Subject: [PATCH] feat(deletions): Prevent DB call from deleting issue When deleting an issue, a failure to query the DB would prevent scheduling the group deletion task. From the customer's perspective, it would look that the group has been deleted, however, all relationships would still be left behind. The biggest problem would be that the group hashes are left behind, thus, any new events matching those group hashes would not create a new issue. This is a better fix than the one proposed in #93541. Fixes #93509. Fixes [ID-716](https://linear.app/getsentry/issue/ID-716/) --- src/sentry/api/helpers/group_index/delete.py | 6 ++--- .../endpoints/organization_group_index.py | 9 ++++++- .../issues/endpoints/project_group_index.py | 8 +++++- .../test_organization_group_index.py | 25 ++++++++++++++++- .../api/endpoints/test_project_group_index.py | 27 ++++++++++--------- 5 files changed, 57 insertions(+), 18 deletions(-) diff --git a/src/sentry/api/helpers/group_index/delete.py b/src/sentry/api/helpers/group_index/delete.py index bcc75cf2ea2b10..e436617b97727f 100644 --- a/src/sentry/api/helpers/group_index/delete.py +++ b/src/sentry/api/helpers/group_index/delete.py @@ -73,6 +73,9 @@ def delete_group_list( }, ) + # Tell seer to delete grouping records for these groups + may_schedule_task_to_delete_hashes_from_seer(error_ids) + Group.objects.filter(id__in=group_ids).exclude( status__in=[GroupStatus.PENDING_DELETION, GroupStatus.DELETION_IN_PROGRESS] ).update(status=GroupStatus.PENDING_DELETION, substatus=None) @@ -84,9 +87,6 @@ def delete_group_list( # fails, we will still have a record of who requested the deletion. create_audit_entries(request, project, group_list, delete_type, transaction_id) - # Tell seer to delete grouping records for these groups - may_schedule_task_to_delete_hashes_from_seer(error_ids) - # Removing GroupHash rows prevents new events from associating to the groups # we just deleted. GroupHash.objects.filter(project_id=project.id, group__id__in=group_ids).delete() diff --git a/src/sentry/issues/endpoints/organization_group_index.py b/src/sentry/issues/endpoints/organization_group_index.py index 3465213e794c33..283e93829c1d5d 100644 --- a/src/sentry/issues/endpoints/organization_group_index.py +++ b/src/sentry/issues/endpoints/organization_group_index.py @@ -1,4 +1,5 @@ import functools +import logging from collections.abc import Mapping, Sequence from datetime import datetime, timedelta from typing import Any @@ -67,6 +68,8 @@ ERR_INVALID_STATS_PERIOD = "Invalid stats_period. Valid choices are '', '24h', '14d' and 'auto'" allowed_inbox_search_terms = frozenset(["date", "status", "for_review", "assigned_or_suggested"]) +logger = logging.getLogger(__name__) + def inbox_search( projects: Sequence[Project], @@ -501,4 +504,8 @@ def delete(self, request: Request, organization: Organization) -> Response: self.get_environments(request, organization), ) - return delete_groups(request, projects, organization.id, search_fn) + try: + return delete_groups(request, projects, organization.id, search_fn) + except Exception: + logger.exception("Error deleting groups") + return Response({"detail": "Error deleting groups"}, status=500) diff --git a/src/sentry/issues/endpoints/project_group_index.py b/src/sentry/issues/endpoints/project_group_index.py index 366b48035cf055..4d7fd459fd2621 100644 --- a/src/sentry/issues/endpoints/project_group_index.py +++ b/src/sentry/issues/endpoints/project_group_index.py @@ -1,4 +1,5 @@ import functools +import logging from rest_framework.request import Request from rest_framework.response import Response @@ -30,6 +31,7 @@ ERR_INVALID_STATS_PERIOD = "Invalid stats_period. Valid choices are '', '24h', and '14d'" ERR_HASHES_AND_OTHER_QUERY = "Cannot use 'hashes' with 'query'" +logger = logging.getLogger(__name__) @region_silo_endpoint @@ -299,4 +301,8 @@ def delete(self, request: Request, project: Project) -> Response: :auth: required """ search_fn = functools.partial(prep_search, request, project) - return delete_groups(request, [project], project.organization_id, search_fn) + try: + return delete_groups(request, [project], project.organization_id, search_fn) + except Exception: + logger.exception("Error deleting groups") + return Response({"detail": "Error deleting groups"}, status=500) diff --git a/tests/sentry/issues/endpoints/test_organization_group_index.py b/tests/sentry/issues/endpoints/test_organization_group_index.py index 51f6d78bd727dc..15fcf96f5bfc1a 100644 --- a/tests/sentry/issues/endpoints/test_organization_group_index.py +++ b/tests/sentry/issues/endpoints/test_organization_group_index.py @@ -7,7 +7,7 @@ from unittest.mock import MagicMock, Mock, call, patch from uuid import uuid4 -from django.db import OperationalError +from django.db.utils import OperationalError from django.urls import reverse from django.utils import timezone @@ -4455,3 +4455,26 @@ def test_bulk_delete_performance_issues(self) -> None: assert response.status_code == 204 self.assert_deleted_groups(groups) + + @patch("sentry.api.helpers.group_index.delete.may_schedule_task_to_delete_hashes_from_seer") + def test_do_not_mark_as_pending_deletion_if_seer_fails(self, mock_seer_delete: Mock): + """ + Test that the issue is not marked as pending deletion if the seer call fails. + """ + # When trying to gather the hashes, the query could be cancelled by the user + mock_seer_delete.side_effect = OperationalError( + "QueryCanceled('canceling statement due to user request\n')" + ) + event = self.store_event(data={}, project_id=self.project.id) + group1 = Group.objects.get(id=event.group_id) + assert GroupHash.objects.filter(group=group1).exists() + + self.login_as(user=self.user) + with self.tasks(): + response = self.get_response(qs_params={"id": [group1.id]}) + assert response.status_code == 500 + assert response.data["detail"] == "Error deleting groups" + + # The group has not been marked as pending deletion + assert Group.objects.get(id=group1.id).status == group1.status + assert GroupHash.objects.filter(group=group1).exists() diff --git a/tests/snuba/api/endpoints/test_project_group_index.py b/tests/snuba/api/endpoints/test_project_group_index.py index 039fee5f7711f3..cddfc3f91005de 100644 --- a/tests/snuba/api/endpoints/test_project_group_index.py +++ b/tests/snuba/api/endpoints/test_project_group_index.py @@ -9,6 +9,7 @@ from uuid import uuid4 from django.conf import settings +from django.db.utils import OperationalError from django.utils import timezone from sentry.integrations.models.external_issue import ExternalIssue @@ -1637,23 +1638,25 @@ def test_bulk_delete_performance_issues(self): self.assert_groups_are_gone(groups) @patch("sentry.api.helpers.group_index.delete.may_schedule_task_to_delete_hashes_from_seer") - @patch("sentry.utils.audit.log_service.record_audit_log") - def test_audit_log_even_if_exception_raised( - self, mock_record_audit_log: Mock, mock_seer_delete: Mock - ): + def test_do_not_mark_as_pending_deletion_if_seer_fails(self, mock_seer_delete: Mock): """ - Test that audit log is created even if an exception is raised after the audit log is created. + Test that the issue is not marked as pending deletion if the seer call fails. """ - # Calling seer happens after creating the audit log entry - mock_seer_delete.side_effect = Exception("Seer error!") - group1 = self.create_group() + # When trying to gather the hashes, the query could be cancelled by the user + mock_seer_delete.side_effect = OperationalError( + "QueryCanceled('canceling statement due to user request\n')" + ) + event = self.store_event(data={}, project_id=self.project.id) + group1 = Group.objects.get(id=event.group_id) + assert GroupHash.objects.filter(group=group1).exists() + self.login_as(user=self.user) url = f"{self.path}?id={group1.id}" with self.tasks(): response = self.client.delete(url, format="json") assert response.status_code == 500 + assert response.data["detail"] == "Error deleting groups" - self.assert_audit_log_entry([group1], mock_record_audit_log) - - # They have been marked as pending deletion but the exception prevented their complete deletion - assert Group.objects.get(id=group1.id).status == GroupStatus.PENDING_DELETION + # The group has not been marked as pending deletion + assert Group.objects.get(id=group1.id).status == group1.status + assert GroupHash.objects.filter(group=group1).exists()