ref(grouping): Clarify Seer grouping deletion code (#95152)

This will make my next pull request easier to review.

Ref
[inc-1236](https://sentry.slack.com/archives/C095HFVE80Y/p1752007736483209)

Author: armenzg

src/sentry/seer/similarity/grouping_records.py

@@ -21,6 +21,7 @@
 logger = logging.getLogger(__name__)
 
 POST_BULK_GROUPING_RECORDS_TIMEOUT = 10000
+DELETE_HASH_METRIC = "grouping.similarity.delete_records_by_hash"
 
 
 class CreateGroupingRecordData(TypedDict):
@@ -125,7 +126,7 @@ def delete_project_grouping_records(
         return False
 
 
-def delete_grouping_records_by_hash(project_id: int, hashes: Sequence[str]) -> bool:
+def call_seer_to_delete_these_hashes(project_id: int, hashes: Sequence[str]) -> bool:
     extra = {"project_id": project_id, "hashes": hashes}
     try:
         body = {"project_id": project_id, "hash_list": hashes}
@@ -142,6 +143,11 @@ def delete_grouping_records_by_hash(project_id: int, hashes: Sequence[str]) -> b
             "seer.delete_grouping_records.hashes.timeout",
             extra=extra,
         )
+        metrics.incr(
+            DELETE_HASH_METRIC,
+            sample_rate=options.get("seer.similarity.metrics_sample_rate"),
+            tags={"success": False, "reason": "ReadTimeoutError"},
+        )
         return False
 
     if response.status >= 200 and response.status < 300:
@@ -150,15 +156,15 @@ def delete_grouping_records_by_hash(project_id: int, hashes: Sequence[str]) -> b
             extra=extra,
         )
         metrics.incr(
-            "grouping.similarity.delete_records_by_hash",
+            DELETE_HASH_METRIC,
             sample_rate=options.get("seer.similarity.metrics_sample_rate"),
             tags={"success": True},
         )
         return True
     else:
         logger.error("seer.delete_grouping_records.hashes.failure", extra=extra)
         metrics.incr(
-            "grouping.similarity.delete_records_by_hash",
+            DELETE_HASH_METRIC,
             sample_rate=options.get("seer.similarity.metrics_sample_rate"),
             tags={"success": False},
         )

src/sentry/tasks/delete_seer_grouping_records.py

@@ -6,7 +6,7 @@
 from sentry.models.group import Group
 from sentry.models.grouphash import GroupHash
 from sentry.seer.similarity.grouping_records import (
-    delete_grouping_records_by_hash,
+    call_seer_to_delete_these_hashes,
     delete_project_grouping_records,
 )
 from sentry.seer.similarity.utils import ReferrerOptions, killswitch_enabled
@@ -16,15 +16,13 @@
 from sentry.taskworker.namespaces import seer_tasks
 from sentry.utils.query import RangeQuerySetWrapper
 
-BATCH_SIZE = 1000
-
 logger = logging.getLogger(__name__)
 
 
 @instrumented_task(
     name="sentry.tasks.delete_seer_grouping_records_by_hash",
     queue="delete_seer_grouping_records_by_hash",
-    max_retries=0,
+    max_retries=0,  # XXX: Why do we not retry?
     silo_mode=SiloMode.REGION,
     soft_time_limit=60 * 15,
     time_limit=60 * (15 + 5),
@@ -42,17 +40,17 @@ def delete_seer_grouping_records_by_hash(
 ) -> None:
     """
     Task to delete seer grouping records by hash list.
-    Calls the seer delete by hash endpoint with batches of hashes of size `BATCH_SIZE`.
+    Calls the seer delete by hash endpoint with batches of hashes of size `batch_size`.
     """
     if killswitch_enabled(project_id, ReferrerOptions.DELETION) or options.get(
         "seer.similarity-embeddings-delete-by-hash-killswitch.enabled"
     ):
         return
 
-    batch_size = options.get("embeddings-grouping.seer.delete-record-batch-size")
+    batch_size = options.get("embeddings-grouping.seer.delete-record-batch-size") or 100
     len_hashes = len(hashes)
     end_index = min(last_deleted_index + batch_size, len_hashes)
-    delete_grouping_records_by_hash(project_id, hashes[last_deleted_index:end_index])
+    call_seer_to_delete_these_hashes(project_id, hashes[last_deleted_index:end_index])
     if end_index < len_hashes:
         delete_seer_grouping_records_by_hash.apply_async(args=[project_id, hashes, end_index])
 
@@ -71,15 +69,16 @@ def call_delete_seer_grouping_records_by_hash(
         and not options.get("seer.similarity-embeddings-delete-by-hash-killswitch.enabled")
     ):
         group_hashes = []
+        batch_size = options.get("embeddings-grouping.seer.delete-record-batch-size") or 100
 
         for group_hash in RangeQuerySetWrapper(
             GroupHash.objects.filter(project_id=project.id, group__id__in=group_ids),
-            step=BATCH_SIZE,
+            step=batch_size,
         ):
             group_hashes.append(group_hash.hash)
 
-            # Schedule task when we reach BATCH_SIZE
-            if len(group_hashes) >= BATCH_SIZE:
+            # Schedule task when we reach batch_size
+            if len(group_hashes) >= batch_size:
                 delete_seer_grouping_records_by_hash.apply_async(args=[project.id, group_hashes, 0])
                 group_hashes = []
 

tests/sentry/seer/similarity/test_grouping_records.py

@@ -11,7 +11,7 @@
 from sentry.seer.similarity.grouping_records import (
     POST_BULK_GROUPING_RECORDS_TIMEOUT,
     CreateGroupingRecordsRequest,
-    delete_grouping_records_by_hash,
+    call_seer_to_delete_these_hashes,
     post_bulk_grouping_records,
 )
 from sentry.testutils.pytest.fixtures import django_db_all
@@ -178,7 +178,7 @@ def test_delete_grouping_records_by_hash_success(
     )
 
     project_id, hashes = 1, ["1", "2"]
-    response = delete_grouping_records_by_hash(project_id, hashes)
+    response = call_seer_to_delete_these_hashes(project_id, hashes)
     assert response is True
     mock_logger.info.assert_called_with(
         "seer.delete_grouping_records.hashes.success",
@@ -189,6 +189,7 @@ def test_delete_grouping_records_by_hash_success(
     )
 
 
+@django_db_all
 @mock.patch("sentry.seer.similarity.grouping_records.logger")
 @mock.patch("sentry.seer.similarity.grouping_records.seer_grouping_connection_pool.urlopen")
 def test_delete_grouping_records_by_hash_timeout(
@@ -198,7 +199,7 @@ def test_delete_grouping_records_by_hash_timeout(
         DUMMY_POOL, SEER_HASH_GROUPING_RECORDS_DELETE_URL, "read timed out"
     )
     project_id, hashes = 1, ["1", "2"]
-    response = delete_grouping_records_by_hash(project_id, hashes)
+    response = call_seer_to_delete_these_hashes(project_id, hashes)
     assert response is False
     mock_logger.exception.assert_called_with(
         "seer.delete_grouping_records.hashes.timeout",
@@ -223,7 +224,7 @@ def test_delete_grouping_records_by_hash_failure(
         status=500,
     )
     project_id, hashes = 1, ["1", "2"]
-    response = delete_grouping_records_by_hash(project_id, hashes)
+    response = call_seer_to_delete_these_hashes(project_id, hashes)
     assert response is False
     mock_logger.error.assert_called_with(
         "seer.delete_grouping_records.hashes.failure",

tests/sentry/tasks/test_delete_seer_grouping_records.py

@@ -1,6 +1,7 @@
 from time import time
 from unittest.mock import MagicMock, patch
 
+from sentry import options
 from sentry.models.grouphash import GroupHash
 from sentry.tasks.delete_seer_grouping_records import (
     call_delete_seer_grouping_records_by_hash,
@@ -12,21 +13,23 @@
 
 @django_db_all
 class TestDeleteSeerGroupingRecordsByHash(TestCase):
-    @patch("sentry.tasks.delete_seer_grouping_records.delete_grouping_records_by_hash")
+    @patch("sentry.tasks.delete_seer_grouping_records.call_seer_to_delete_these_hashes")
     @patch(
         "sentry.tasks.delete_seer_grouping_records.delete_seer_grouping_records_by_hash.apply_async"
     )
     def test_delete_seer_grouping_records_by_hash_batches(
         self,
         mock_delete_seer_grouping_records_by_hash_apply_async: MagicMock,
-        mock_delete_grouping_records_by_hash: MagicMock,
+        mock_call_seer_to_delete_these_hashes: MagicMock,
     ) -> None:
         """
-        Test that when delete_seer_grouping_records_by_hash is called with over 20 hashes, it spawns
+        Test that when delete_seer_grouping_records_by_hash is called with more hashes than the batch size, it spawns
         another task with the end index of the previous batch.
         """
-        mock_delete_grouping_records_by_hash.return_value = True
-        project_id, hashes = 1, [str(i) for i in range(101)]
+        batch_size = options.get("embeddings-grouping.seer.delete-record-batch-size") or 100
+        mock_call_seer_to_delete_these_hashes.return_value = True
+        project_id, hashes = 1, [str(i) for i in range(batch_size + 1)]
+        # We call it as a function and will schedule a task for the extra hash
         delete_seer_grouping_records_by_hash(project_id, hashes, 0)
         assert mock_delete_seer_grouping_records_by_hash_apply_async.call_args[1] == {
             "args": [project_id, hashes, 100]
@@ -61,7 +64,7 @@ def test_call_delete_seer_grouping_records_by_hash_simple(
     def test_call_delete_seer_grouping_records_by_hash_chunked(self) -> None:
         """
         Test that call_delete_seer_grouping_records_by_hash chunks large numbers of hashes
-        into separate tasks with a maximum of BATCH_SIZE hashes per task.
+        into separate tasks with a maximum of batch_size hashes per task.
         """
         self.project.update_option("sentry:similarity_backfill_completed", int(time()))
 
@@ -70,7 +73,7 @@ def test_call_delete_seer_grouping_records_by_hash_chunked(self) -> None:
             patch(
                 "sentry.tasks.delete_seer_grouping_records.delete_seer_grouping_records_by_hash.apply_async"
             ) as mock_apply_async,
-            patch("sentry.tasks.delete_seer_grouping_records.BATCH_SIZE", batch_size),
+            self.options({"embeddings-grouping.seer.delete-record-batch-size": batch_size}),
         ):
             # Create 15 group hashes to test chunking (10 + 5 with batch size of 10)
             group_ids, expected_hashes = [], []
@@ -91,12 +94,14 @@ def test_call_delete_seer_grouping_records_by_hash_chunked(self) -> None:
             first_call_args = mock_apply_async.call_args_list[0][1]["args"]
             assert len(first_call_args[1]) == batch_size
             assert first_call_args[0] == self.project.id
+            assert first_call_args[1] == expected_hashes[0:batch_size]
             assert first_call_args[2] == 0
 
             # Verify the second chunk has 5 hashes (remainder)
             second_call_args = mock_apply_async.call_args_list[1][1]["args"]
             assert len(second_call_args[1]) == 5
             assert second_call_args[0] == self.project.id
+            assert second_call_args[1] == expected_hashes[batch_size:]
             assert second_call_args[2] == 0
 
     @patch(