fix(tags): order single-finding delete tag-count decrements so concurrent deletes cannot deadlock

dojo/finding/models.py

@@ -735,6 +735,18 @@ def delete(self, *args, product_grading_option=True, push_to_jira=DELETE_JIRA_SY
         logger.debug("%d finding delete", self.id)
         from dojo.finding import helper as finding_helper  # noqa: PLC0415 -- lazy import, avoids circular dependency
         finding_helper.finding_delete(self, push_to_jira=push_to_jira)
+        # Remove this finding's tags in a deterministic (ascending tag-id) order BEFORE
+        # the cascade. Left to super().delete(), tagulous's per-object clear() decrements
+        # the shared tag-count rows one UPDATE at a time in the manager's own order, so
+        # two concurrent single-finding deletes touching an overlapping tag set can take
+        # those row locks in opposite orders and deadlock (Postgres 40P01, "while updating
+        # tuple ... in relation dojo_tagulous_finding_tags"). bulk_remove_all_tags issues
+        # the same count decrements in ascending tag-id order -- the shared lock order
+        # already used by the bulk cascade delete and the import add path -- so the cycle
+        # cannot form. It also clears the through rows, so the tagulous pre_delete handler
+        # then finds nothing left to decrement (no double counting).
+        from dojo.tags.utils import bulk_remove_all_tags  # noqa: PLC0415 -- lazy import, avoids circular dependency
+        bulk_remove_all_tags(Finding, Finding.objects.filter(pk=self.pk))
         super().delete(*args, **kwargs)
         if product_grading_option:
             from dojo.models import (  # noqa: PLC0415 -- lazy import, avoids circular dependency

unittests/test_tag_utils_bulk.py

@@ -551,6 +551,79 @@ def record_filter(*args, **kwargs):
         )
 
 
+class FindingDeleteTagLockOrderTest(TestCase):
+
+    # Regression: a single-object DELETE /api/v2/findings/{id}/ (Finding.delete())
+    # decremented tag counts through tagulous's per-object clear(), which issues one
+    # count UPDATE per tag in the manager's own order -- NOT the ascending tag-id order
+    # that bulk_remove_all_tags (#15486) and the add path (#15652) use. Because a count
+    # UPDATE takes a row lock, two concurrent single-finding deletes touching an
+    # overlapping tag set could take those tag-row locks in opposite orders and deadlock
+    # (Postgres 40P01, "while updating tuple ... in relation dojo_tagulous_finding_tags").
+    # Routing the delete through bulk_remove_all_tags gives every caller one shared lock
+    # order, so the cycle cannot form.
+
+    def setUp(self):
+        self.reporter = User.objects.create_user(username="finding-del-lock-user")
+        product_type = Product_Type.objects.create(name="PT-Finding-Del-Lock")
+        product = Product.objects.create(
+            name="Finding Del Lock Product", description="test", prod_type=product_type,
+        )
+        engagement = Engagement.objects.create(
+            name="E-Finding-Del-Lock", product=product,
+            target_start=timezone.now(), target_end=timezone.now(),
+        )
+        test_type = Test_Type.objects.create(name="Finding Del Lock Test Type")
+        self.test = Test.objects.create(
+            title="T-Finding-Del-Lock", engagement=engagement, test_type=test_type,
+            target_start=timezone.now(), target_end=timezone.now(),
+        )
+
+    def test_finding_delete_decrements_tag_counts_in_ascending_tag_id_order(self):
+        """
+        The decrement UPDATEs must be ordered, because their order is the lock order.
+
+        Tags are attached in an order unrelated to their ids so that the manager's own
+        iteration order and "ascending id" cannot coincide by luck.
+        """
+        finding = Finding.objects.create(
+            title="Finding Del Lock", severity="Low", test=self.test, reporter=self.reporter,
+        )
+        finding.tags = ["zeta-tag", "alpha-tag", "mid-tag"]
+        finding.save()
+
+        tag_model = Finding.tags.tag_model
+        our_tag_ids = set(
+            tag_model.objects.filter(
+                name__in=["zeta-tag", "alpha-tag", "mid-tag"],
+            ).values_list("pk", flat=True),
+        )
+        self.assertEqual(len(our_tag_ids), 3, "expected the three tags to exist")
+
+        locked_order = []
+        original_filter = tag_model.objects.filter
+
+        def record_filter(*args, **kwargs):
+            # Only the per-tag count UPDATEs (filter(pk=<tag id>).update(...)) take the
+            # row locks that can deadlock; ignore any other tag-model lookups.
+            if kwargs.get("pk") in our_tag_ids:
+                locked_order.append(kwargs["pk"])
+            return original_filter(*args, **kwargs)
+
+        with patch.object(tag_model.objects, "filter", side_effect=record_filter):
+            finding.delete(product_grading_option=False)
+
+        self.assertEqual(
+            len(locked_order), 3,
+            msg=f"expected one decrement per tag, got {locked_order}",
+        )
+        self.assertEqual(
+            locked_order, sorted(locked_order),
+            msg="tag rows must be locked in ascending id order so concurrent single-finding "
+                f"deletes cannot deadlock; got {locked_order} (tag ids: {sorted(our_tag_ids)})",
+        )
+
+
 class BulkAddTagsToInstancesLockOrderTest(TestCase):
 
     # Regression (import deadlock): bulk_add_tags_to_instances issued the per-tag count