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 f2e88b382b9213..44afe5b7d051e2 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,26 +1638,28 @@ 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() def test_new_event_for_pending_deletion_group_creates_new_group(self): """Test that after deleting a group, new events with the same fingerprint create a new group."""