From 4db9b458390921afc660a2734820f03d23f0e690 Mon Sep 17 00:00:00 2001 From: Yuval Mandelboum Date: Tue, 11 Nov 2025 14:31:38 -0800 Subject: [PATCH 1/3] fix(deletions): Fix scheduled project deletion timeout failure We are seeing deletions failing on grouphash deletions on `Project.delete()`. This suggests `delete_project_group_hashses` is not deleting all hashes. Refactored this deletion to catch orphan grouphashses that might be left causing this timeout. Utilizes `delete_group_hashes` by adding a mode for deleting all group hashes of a project. --- src/sentry/deletions/defaults/group.py | 55 ++++++++++++++++++-------- tests/sentry/deletions/test_project.py | 51 ++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 17 deletions(-) diff --git a/src/sentry/deletions/defaults/group.py b/src/sentry/deletions/defaults/group.py index 5dc64f80b98cce..0dea2bb66cd5d7 100644 --- a/src/sentry/deletions/defaults/group.py +++ b/src/sentry/deletions/defaults/group.py @@ -32,7 +32,6 @@ from sentry.snuba.dataset import Dataset from sentry.tasks.delete_seer_grouping_records import may_schedule_task_to_delete_hashes_from_seer from sentry.utils import metrics -from sentry.utils.query import RangeQuerySetWrapper from ..base import BaseDeletionTask, BaseRelation, ModelDeletionTask, ModelRelation from ..manager import DeletionTaskManager @@ -247,18 +246,22 @@ def mark_deletion_in_progress(self, instance_list: Sequence[Group]) -> None: def delete_project_group_hashes(project_id: int) -> None: - groups = [] - for group in RangeQuerySetWrapper( - Group.objects.filter(project_id=project_id), step=GROUP_CHUNK_SIZE - ): - groups.append(group) + """ + Delete all GroupHash records for a project, including orphaned records. - error_groups, issue_platform_groups = separate_by_group_category(groups) - error_group_ids = [group.id for group in error_groups] - delete_group_hashes(project_id, error_group_ids, seer_deletion=True) + This is called during project deletion to clean up GroupHash records that may not have been + deleted during Group deletion. This includes: + - Orphaned GroupHash records with group_id=None + - GroupHash records that were created but never assigned to a Group + - Secondary grouping hashes that are not associated with Groups - issue_platform_group_ids = [group.id for group in issue_platform_groups] - delete_group_hashes(project_id, issue_platform_group_ids) + Note: This function is called from Project.delete() after Groups have already been deleted + by the deletion task system, so we cannot rely on querying Groups. Instead, we directly + query GroupHash records by project_id. + """ + # Delete all GroupHash records for the project by calling delete_group_hashes with group_ids=None + # This will efficiently handle all records in batches, including orphans + delete_group_hashes(project_id, group_ids=None, seer_deletion=False) def update_group_hash_metadata_in_batches(hash_ids: Sequence[int]) -> None: @@ -312,9 +315,18 @@ def update_group_hash_metadata_in_batches(hash_ids: Sequence[int]) -> None: def delete_group_hashes( project_id: int, - group_ids: Sequence[int], + group_ids: Sequence[int] | None = None, seer_deletion: bool = False, ) -> None: + """ + Delete GroupHash records for a project. + + Args: + project_id: The project to delete hashes for + group_ids: Specific group IDs to delete hashes for. If None or empty, deletes ALL + GroupHash records for the project (including orphans with group_id=None) + seer_deletion: Whether to notify Seer about the deletion + """ # Validate batch size to ensure it's at least 1 to avoid ValueError in range() hashes_batch_size = max(1, options.get("deletions.group-hashes-batch-size")) @@ -322,9 +334,18 @@ def delete_group_hashes( # The loop will naturally terminate when no more hashes are found. iterations = 0 while iterations < GROUP_HASH_ITERATIONS: - qs = GroupHash.objects.filter(project_id=project_id, group_id__in=group_ids).values_list( - "id", "hash" - )[:hashes_batch_size] + # Build query based on whether we're deleting specific groups or all hashes + if group_ids: + # Delete only hashes for specific groups + qs = GroupHash.objects.filter( + project_id=project_id, group_id__in=group_ids + ).values_list("id", "hash")[:hashes_batch_size] + else: + # Delete ALL hashes for project (including orphans with group_id=None) + qs = GroupHash.objects.filter(project_id=project_id).values_list("id", "hash")[ + :hashes_batch_size + ] + hashes_chunk = list(qs) if not hashes_chunk: break @@ -354,8 +375,8 @@ def delete_group_hashes( if iterations == GROUP_HASH_ITERATIONS: metrics.incr("deletions.group_hashes.max_iterations_reached", sample_rate=1.0) logger.warning( - "Group hashes batch deletion reached the maximum number of iterations. " - "Investigate if we need to change the GROUP_HASH_ITERATIONS value." + "delete_group_hashes.max_iterations_reached", + extra={"project_id": project_id, "has_group_ids": bool(group_ids)}, ) diff --git a/tests/sentry/deletions/test_project.py b/tests/sentry/deletions/test_project.py index 58871ea0ccef9a..714600e727f787 100644 --- a/tests/sentry/deletions/test_project.py +++ b/tests/sentry/deletions/test_project.py @@ -13,6 +13,7 @@ from sentry.models.files.file import File from sentry.models.group import Group from sentry.models.groupassignee import GroupAssignee +from sentry.models.grouphash import GroupHash from sentry.models.groupmeta import GroupMeta from sentry.models.groupopenperiod import GroupOpenPeriod from sentry.models.groupopenperiodactivity import GroupOpenPeriodActivity, OpenPeriodActivityType @@ -233,6 +234,56 @@ def test_delete_error_events(self) -> None: ) assert len(events) == 0 + def test_delete_orphaned_grouphashes(self) -> None: + """Test that orphaned GroupHash records (with group_id=None) are deleted during project deletion.""" + project = self.create_project(name="test") + + # Create an event to get a group with associated GroupHash + event = self.store_event( + data={ + "timestamp": before_now(minutes=1).isoformat(), + "message": "test event", + }, + project_id=project.id, + ) + assert event.group is not None + group = event.group + + # Get the GroupHash created for this group + group_hash = GroupHash.objects.get(project=project, group=group) + + # Create additional orphaned GroupHash records (group_id=None) + # These simulate GroupHash records that were created but never assigned to a Group, + # or secondary grouping hashes + orphan_hash_1 = GroupHash.objects.create( + project=project, + hash="a" * 32, + group=None, # Orphaned - no group assigned + ) + orphan_hash_2 = GroupHash.objects.create( + project=project, + hash="b" * 32, + group=None, # Orphaned - no group assigned + ) + + # Verify all GroupHash records exist before deletion + assert GroupHash.objects.filter(project=project).count() == 3 + assert GroupHash.objects.filter(project=project, group__isnull=True).count() == 2 + + # Schedule and run project deletion + self.ScheduledDeletion.schedule(instance=project, days=0) + with self.tasks(): + run_scheduled_deletions() + + # Verify project is deleted + assert not Project.objects.filter(id=project.id).exists() + + # Verify ALL GroupHash records are deleted, including orphans + assert not GroupHash.objects.filter(id=group_hash.id).exists() + assert not GroupHash.objects.filter(id=orphan_hash_1.id).exists() + assert not GroupHash.objects.filter(id=orphan_hash_2.id).exists() + assert GroupHash.objects.filter(project_id=project.id).count() == 0 + @mock.patch("sentry.quotas.backend.remove_seat") def test_delete_with_uptime_monitors(self, mock_remove_seat: mock.MagicMock) -> None: project = self.create_project(name="test") From 72527e0d9c9da22c722257b0d2b72638fa482cb6 Mon Sep 17 00:00:00 2001 From: Yuval Mandelboum Date: Tue, 11 Nov 2025 15:31:33 -0800 Subject: [PATCH 2/3] refactor `delete_project_group_hashes` to be main deletion function increasing clarity --- src/sentry/deletions/defaults/group.py | 80 ++++++++++---------------- 1 file changed, 29 insertions(+), 51 deletions(-) diff --git a/src/sentry/deletions/defaults/group.py b/src/sentry/deletions/defaults/group.py index 0dea2bb66cd5d7..26138651d82c1d 100644 --- a/src/sentry/deletions/defaults/group.py +++ b/src/sentry/deletions/defaults/group.py @@ -214,8 +214,12 @@ def _delete_children(self, instance_list: Sequence[Group]) -> None: error_group_ids = [group.id for group in error_groups] issue_platform_group_ids = [group.id for group in issue_platform_groups] - delete_group_hashes(instance_list[0].project_id, error_group_ids, seer_deletion=True) - delete_group_hashes(instance_list[0].project_id, issue_platform_group_ids) + delete_project_group_hashes( + instance_list[0].project_id, group_ids_filter=error_group_ids, seer_deletion=True + ) + delete_project_group_hashes( + instance_list[0].project_id, group_ids_filter=issue_platform_group_ids + ) # If this isn't a retention cleanup also remove event data. if not os.environ.get("_SENTRY_CLEANUP"): @@ -245,25 +249,6 @@ def mark_deletion_in_progress(self, instance_list: Sequence[Group]) -> None: ).update(status=GroupStatus.DELETION_IN_PROGRESS, substatus=None) -def delete_project_group_hashes(project_id: int) -> None: - """ - Delete all GroupHash records for a project, including orphaned records. - - This is called during project deletion to clean up GroupHash records that may not have been - deleted during Group deletion. This includes: - - Orphaned GroupHash records with group_id=None - - GroupHash records that were created but never assigned to a Group - - Secondary grouping hashes that are not associated with Groups - - Note: This function is called from Project.delete() after Groups have already been deleted - by the deletion task system, so we cannot rely on querying Groups. Instead, we directly - query GroupHash records by project_id. - """ - # Delete all GroupHash records for the project by calling delete_group_hashes with group_ids=None - # This will efficiently handle all records in batches, including orphans - delete_group_hashes(project_id, group_ids=None, seer_deletion=False) - - def update_group_hash_metadata_in_batches(hash_ids: Sequence[int]) -> None: """ Update seer_matched_grouphash to None for GroupHashMetadata rows @@ -313,59 +298,52 @@ def update_group_hash_metadata_in_batches(hash_ids: Sequence[int]) -> None: metrics.incr("deletions.group_hash_metadata.max_iterations_reached", sample_rate=1.0) -def delete_group_hashes( +def delete_project_group_hashes( project_id: int, - group_ids: Sequence[int] | None = None, + group_ids_filter: Sequence[int] | None = None, seer_deletion: bool = False, ) -> None: """ Delete GroupHash records for a project. + This is the main function for deleting GroupHash records. It can delete all hashes for a project + (used during project deletion to clean up orphaned records) or delete hashes for specific groups + (used during group deletion). + Args: project_id: The project to delete hashes for - group_ids: Specific group IDs to delete hashes for. If None or empty, deletes ALL - GroupHash records for the project (including orphans with group_id=None) + group_ids_filter: Optional filter for specific group IDs. + - If None: deletes ALL GroupHash records for the project (including orphans) + - If empty: returns immediately (no-op for safety) + - If non-empty: deletes only hashes for those specific groups seer_deletion: Whether to notify Seer about the deletion """ - # Validate batch size to ensure it's at least 1 to avoid ValueError in range() + # Safety: empty filter means nothing to delete + if group_ids_filter is not None and not group_ids_filter: + return + hashes_batch_size = max(1, options.get("deletions.group-hashes-batch-size")) - # Set a reasonable upper bound on iterations to prevent infinite loops. - # The loop will naturally terminate when no more hashes are found. iterations = 0 while iterations < GROUP_HASH_ITERATIONS: - # Build query based on whether we're deleting specific groups or all hashes - if group_ids: - # Delete only hashes for specific groups - qs = GroupHash.objects.filter( - project_id=project_id, group_id__in=group_ids - ).values_list("id", "hash")[:hashes_batch_size] - else: - # Delete ALL hashes for project (including orphans with group_id=None) - qs = GroupHash.objects.filter(project_id=project_id).values_list("id", "hash")[ - :hashes_batch_size - ] - - hashes_chunk = list(qs) + # Base query: all hashes for this project + qs = GroupHash.objects.filter(project_id=project_id) + + # Apply group filter if provided + if group_ids_filter is not None: + qs = qs.filter(group_id__in=group_ids_filter) + + hashes_chunk = list(qs.values_list("id", "hash")[:hashes_batch_size]) if not hashes_chunk: break try: if seer_deletion: - # Tell seer to delete grouping records for these groups - # It's low priority to delete the hashes from seer, so we don't want - # any network errors to block the deletion of the groups hash_values = [gh[1] for gh in hashes_chunk] may_schedule_task_to_delete_hashes_from_seer(project_id, hash_values) except Exception: logger.warning("Error scheduling task to delete hashes from seer") finally: hash_ids = [gh[0] for gh in hashes_chunk] - # GroupHashMetadata rows can reference GroupHash rows via seer_matched_grouphash_id. - # Before deleting these GroupHash rows, we need to either: - # 1. Update seer_matched_grouphash to None first (to avoid foreign key constraint errors), OR - # 2. Delete the GroupHashMetadata rows entirely (they'll be deleted anyway) - # If we update the columns first, the deletion of the grouphash metadata rows will have less work to do, - # thus, improving the performance of the deletion. update_group_hash_metadata_in_batches(hash_ids) GroupHashMetadata.objects.filter(grouphash_id__in=hash_ids).delete() GroupHash.objects.filter(id__in=hash_ids).delete() @@ -376,7 +354,7 @@ def delete_group_hashes( metrics.incr("deletions.group_hashes.max_iterations_reached", sample_rate=1.0) logger.warning( "delete_group_hashes.max_iterations_reached", - extra={"project_id": project_id, "has_group_ids": bool(group_ids)}, + extra={"project_id": project_id, "filtered": group_ids_filter is not None}, ) From ae0b47d79516e5060418731fa0f1bb111357ea40 Mon Sep 17 00:00:00 2001 From: Yuval Mandelboum Date: Wed, 12 Nov 2025 14:52:28 -0800 Subject: [PATCH 3/3] test refactor --- src/sentry/deletions/defaults/group.py | 2 +- tests/sentry/deletions/test_group.py | 67 +++++++++++++++++++++++++- tests/sentry/deletions/test_project.py | 51 -------------------- 3 files changed, 67 insertions(+), 53 deletions(-) diff --git a/src/sentry/deletions/defaults/group.py b/src/sentry/deletions/defaults/group.py index 63298f50d4f1be..0d21f1fd1e9bc7 100644 --- a/src/sentry/deletions/defaults/group.py +++ b/src/sentry/deletions/defaults/group.py @@ -332,7 +332,7 @@ def delete_project_group_hashes( seer_deletion: Whether to notify Seer about the deletion """ # Safety: empty filter means nothing to delete - if group_ids_filter is not None and not group_ids_filter: + if group_ids_filter is not None and len(group_ids_filter) == 0: return hashes_batch_size = max(1, options.get("deletions.group-hashes-batch-size")) diff --git a/tests/sentry/deletions/test_group.py b/tests/sentry/deletions/test_group.py index 530fcacf0ae0f7..5cf32fb398a4da 100644 --- a/tests/sentry/deletions/test_group.py +++ b/tests/sentry/deletions/test_group.py @@ -9,7 +9,10 @@ from snuba_sdk import Column, Condition, DeleteQuery, Entity, Function, Op, Query, Request from sentry import deletions, nodestore -from sentry.deletions.defaults.group import update_group_hash_metadata_in_batches +from sentry.deletions.defaults.group import ( + delete_project_group_hashes, + update_group_hash_metadata_in_batches, +) from sentry.deletions.tasks.groups import delete_groups_for_project from sentry.issues.grouptype import FeedbackGroup, GroupCategory from sentry.issues.issue_occurrence import IssueOccurrence @@ -389,6 +392,68 @@ def test_update_group_hash_metadata_in_batches(self) -> None: else: raise AssertionError("GroupHashMetadata is None for grouphash id=%s" % grouphash.id) + def test_delete_project_group_hashes_specific_groups(self) -> None: + """Test deleting grouphashes for specific group IDs (including metadata) and empty list safety.""" + self.project.update_option("sentry:similarity_backfill_completed", int(time())) + + event_1 = self.store_event( + data={"platform": "python", "stacktrace": {"frames": [{"filename": "error_1.py"}]}}, + project_id=self.project.id, + ) + event_2 = self.store_event( + data={"platform": "python", "stacktrace": {"frames": [{"filename": "error_2.py"}]}}, + project_id=self.project.id, + ) + + grouphash_1 = GroupHash.objects.get(group=event_1.group) + grouphash_2 = GroupHash.objects.get(group=event_2.group) + assert grouphash_1.metadata is not None + assert grouphash_2.metadata is not None + metadata_1_id = grouphash_1.metadata.id + metadata_2_id = grouphash_2.metadata.id + + assert GroupHash.objects.filter(project=self.project).count() == 2 + + delete_project_group_hashes( + project_id=self.project.id, + group_ids_filter=[event_1.group.id], + ) + + assert not GroupHash.objects.filter(id=grouphash_1.id).exists() + assert not GroupHashMetadata.objects.filter(id=metadata_1_id).exists() + assert GroupHash.objects.filter(id=grouphash_2.id).exists() + assert GroupHashMetadata.objects.filter(id=metadata_2_id).exists() + + # Empty list should be a no-op + delete_project_group_hashes(project_id=self.project.id, group_ids_filter=[]) + assert GroupHash.objects.filter(id=grouphash_2.id).exists() + + def test_delete_project_group_hashes_all_including_orphans(self) -> None: + """Test deleting all grouphashes including orphans when group_ids_filter=None.""" + self.project.update_option("sentry:similarity_backfill_completed", int(time())) + + event = self.store_event( + data={"platform": "python", "stacktrace": {"frames": [{"filename": "error.py"}]}}, + project_id=self.project.id, + ) + grouphash = GroupHash.objects.get(group=event.group) + assert grouphash.metadata is not None + metadata_id = grouphash.metadata.id + + orphan_1 = GroupHash.objects.create(project=self.project, hash="a" * 32, group=None) + orphan_2 = GroupHash.objects.create(project=self.project, hash="b" * 32, group=None) + + assert GroupHash.objects.filter(project=self.project).count() == 3 + assert GroupHash.objects.filter(project=self.project, group__isnull=True).count() == 2 + + delete_project_group_hashes(project_id=self.project.id, group_ids_filter=None) + + assert not GroupHash.objects.filter(id=grouphash.id).exists() + assert not GroupHash.objects.filter(id=orphan_1.id).exists() + assert not GroupHash.objects.filter(id=orphan_2.id).exists() + assert not GroupHashMetadata.objects.filter(id=metadata_id).exists() + assert GroupHash.objects.filter(project=self.project).count() == 0 + class DeleteIssuePlatformTest(TestCase, SnubaTestCase, OccurrenceTestMixin): referrer = Referrer.TESTING_TEST.value diff --git a/tests/sentry/deletions/test_project.py b/tests/sentry/deletions/test_project.py index 714600e727f787..58871ea0ccef9a 100644 --- a/tests/sentry/deletions/test_project.py +++ b/tests/sentry/deletions/test_project.py @@ -13,7 +13,6 @@ from sentry.models.files.file import File from sentry.models.group import Group from sentry.models.groupassignee import GroupAssignee -from sentry.models.grouphash import GroupHash from sentry.models.groupmeta import GroupMeta from sentry.models.groupopenperiod import GroupOpenPeriod from sentry.models.groupopenperiodactivity import GroupOpenPeriodActivity, OpenPeriodActivityType @@ -234,56 +233,6 @@ def test_delete_error_events(self) -> None: ) assert len(events) == 0 - def test_delete_orphaned_grouphashes(self) -> None: - """Test that orphaned GroupHash records (with group_id=None) are deleted during project deletion.""" - project = self.create_project(name="test") - - # Create an event to get a group with associated GroupHash - event = self.store_event( - data={ - "timestamp": before_now(minutes=1).isoformat(), - "message": "test event", - }, - project_id=project.id, - ) - assert event.group is not None - group = event.group - - # Get the GroupHash created for this group - group_hash = GroupHash.objects.get(project=project, group=group) - - # Create additional orphaned GroupHash records (group_id=None) - # These simulate GroupHash records that were created but never assigned to a Group, - # or secondary grouping hashes - orphan_hash_1 = GroupHash.objects.create( - project=project, - hash="a" * 32, - group=None, # Orphaned - no group assigned - ) - orphan_hash_2 = GroupHash.objects.create( - project=project, - hash="b" * 32, - group=None, # Orphaned - no group assigned - ) - - # Verify all GroupHash records exist before deletion - assert GroupHash.objects.filter(project=project).count() == 3 - assert GroupHash.objects.filter(project=project, group__isnull=True).count() == 2 - - # Schedule and run project deletion - self.ScheduledDeletion.schedule(instance=project, days=0) - with self.tasks(): - run_scheduled_deletions() - - # Verify project is deleted - assert not Project.objects.filter(id=project.id).exists() - - # Verify ALL GroupHash records are deleted, including orphans - assert not GroupHash.objects.filter(id=group_hash.id).exists() - assert not GroupHash.objects.filter(id=orphan_hash_1.id).exists() - assert not GroupHash.objects.filter(id=orphan_hash_2.id).exists() - assert GroupHash.objects.filter(project_id=project.id).count() == 0 - @mock.patch("sentry.quotas.backend.remove_seat") def test_delete_with_uptime_monitors(self, mock_remove_seat: mock.MagicMock) -> None: project = self.create_project(name="test")