From 3a48e0c8aeec6d1771b3ec6a8491c2a4597f4efe Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 00:49:58 +0000 Subject: [PATCH] fix(tags): order single-finding delete tag decrements to avoid deadlock A single-object finding delete (Finding.delete(), used by DELETE /api/v2/findings/{id}/) removed its tags through tagulous's per-object clear(), which issues one tag-count UPDATE per tag in the manager's own order rather than ascending tag-id order. Because each UPDATE takes a row lock, two concurrent single-finding deletes touching an overlapping tag set could acquire those tag-row locks in opposite orders and deadlock (Postgres 40P01, "while updating tuple ... in relation dojo_tagulous_finding_tags"). Route the delete through bulk_remove_all_tags -- the same ascending tag-id ordering already used by the bulk cascade delete (#15486) and the import add path (#15652) -- so every tag-count mutation shares one lock order and the cycle cannot form. Clearing the through rows first also leaves the tagulous pre_delete handler nothing to decrement, so counts are not double-processed. No schema change / no migration. Adds FindingDeleteTagLockOrderTest asserting the decrements are issued in ascending tag-id order. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_015i5bbWnQg2wKmQpvD7DRLz --- dojo/finding/models.py | 12 ++++++ unittests/test_tag_utils_bulk.py | 73 ++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/dojo/finding/models.py b/dojo/finding/models.py index 12287002642..c07f21e600a 100644 --- a/dojo/finding/models.py +++ b/dojo/finding/models.py @@ -725,6 +725,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 diff --git a/unittests/test_tag_utils_bulk.py b/unittests/test_tag_utils_bulk.py index c57913784dc..7bb861f9352 100644 --- a/unittests/test_tag_utils_bulk.py +++ b/unittests/test_tag_utils_bulk.py @@ -549,3 +549,76 @@ def record_filter(*args, **kwargs): f"cannot deadlock; got {locked_order} " f"(tag ids: {sorted(tag_ids_by_name.items(), key=itemgetter(1))})", ) + + +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=).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)})", + )