fix(tags): order add-path tag-count updates so concurrent imports cannot deadlock

dojo/tags/utils.py

@@ -85,11 +85,11 @@ def bulk_add_tags_to_instances(tag_or_tags, instances, tag_field_name: str = "ta
             elif field.remote_field.model == tag_model:
                 target_field_name = field_name
 
+    # Query 1: ensure every tag exists, resolving each to its row up front.
+    resolved_tags = []
     for single_tag_name in tag_names:
         if not single_tag_name:
             continue
-
-        # Query 1: ensure the tag exists once per tag
         if tag_field.tag_options.case_sensitive:
             tag, _ = tag_model.objects.get_or_create(
                 name=single_tag_name,
@@ -100,7 +100,17 @@ def bulk_add_tags_to_instances(tag_or_tags, instances, tag_field_name: str = "ta
                 name__iexact=single_tag_name,
                 defaults={"name": single_tag_name, "protected": False},
             )
-
+        resolved_tags.append(tag)
+
+    # Apply the tags in ascending tag-id order. Each tag's batch loop issues a count
+    # UPDATE that locks that tag's row (dojo_tagulous_<model>_tags); inside an import
+    # the surrounding transaction holds that lock until commit. Two concurrent callers
+    # adding an overlapping set of tags would otherwise lock those rows in the order
+    # the tags were supplied and could take them in opposite orders, deadlocking
+    # (Postgres 40P01). Sorting by tag id makes the lock sequence identical for every
+    # caller so the cycle cannot form -- the same reasoning as bulk_remove_all_tags
+    # (see #15486), applied to the add path.
+    for tag in sorted(resolved_tags, key=lambda resolved_tag: resolved_tag.pk):
         # Process in batches to manage memory
         for i in range(0, len(instances), batch_size):
             batch_instances = instances[i:i + batch_size]

unittests/test_tag_utils_bulk.py

@@ -549,3 +549,84 @@ def record_filter(*args, **kwargs):
                 f"cannot deadlock; got {locked_order} "
                 f"(tag ids: {sorted(tag_ids_by_name.items(), key=itemgetter(1))})",
         )
+
+
+class BulkAddTagsToInstancesLockOrderTest(TestCase):
+
+    # Regression (import deadlock): bulk_add_tags_to_instances issued the per-tag count
+    # UPDATE -- which locks that tag's row in dojo_tagulous_finding_tags and, inside an
+    # import, holds it until the surrounding transaction commits -- in caller-supplied
+    # tag order. Two concurrent imports adding an overlapping set of tags could take
+    # those row locks in opposite orders and deadlock (Postgres 40P01). This is the
+    # add-path twin of the bulk_remove_all_tags deadlock fixed in #15486; the count
+    # UPDATEs must be issued in a deterministic ascending tag-id order.
+
+    def setUp(self):
+        self.tag_model = Finding.tags.tag_model
+        self.reporter = User.objects.create_user(username="add-lock-order-user")
+        product_type = Product_Type.objects.create(name="PT-Add-Lock-Order")
+        product = Product.objects.create(
+            name="Add Lock Order Product", description="test", prod_type=product_type,
+        )
+        engagement = Engagement.objects.create(
+            name="E-Add-Lock-Order", product=product,
+            target_start=timezone.now(), target_end=timezone.now(),
+        )
+        test_type = Test_Type.objects.create(name="Add Lock Order Test Type")
+        self.test = Test.objects.create(
+            title="T-Add-Lock-Order", engagement=engagement, test_type=test_type,
+            target_start=timezone.now(), target_end=timezone.now(),
+        )
+
+    def _make_finding(self, title):
+        return Finding.objects.create(
+            title=title, severity="Low", test=self.test, reporter=self.reporter,
+        )
+
+    def test_tag_count_increments_are_issued_in_ascending_tag_id_order(self):
+        """
+        The count UPDATEs must be ordered, because their order is the lock order.
+
+        The three tags are created first, in an order deliberately unrelated to the
+        order they are later supplied to bulk_add_tags_to_instances, so that "supplied
+        order" and "ascending id" cannot coincide by luck.
+        """
+        # Create the tags first so their ids are fixed (zeta < alpha < mid by id).
+        seed = self._make_finding("F-seed")
+        for name in ("zeta-tag", "alpha-tag", "mid-tag"):
+            seed.tags.add(name)
+
+        tag_ids_by_name = dict(
+            self.tag_model.objects.filter(
+                name__in=["zeta-tag", "alpha-tag", "mid-tag"],
+            ).values_list("name", "pk"),
+        )
+        self.assertEqual(len(tag_ids_by_name), 3, "expected the three tags to exist")
+
+        # Fresh findings carrying none of these tags, so every tag produces a new
+        # relationship and therefore a count UPDATE.
+        findings = [self._make_finding(f"F-add-{i}") for i in range(3)]
+
+        locked_order = []
+        original_filter = self.tag_model.objects.filter
+
+        def record_filter(*args, **kwargs):
+            if "pk" in kwargs:
+                locked_order.append(kwargs["pk"])
+            return original_filter(*args, **kwargs)
+
+        # Supplied in an order that is NOT ascending id: alpha, then zeta, then mid.
+        supplied = ["alpha-tag", "zeta-tag", "mid-tag"]
+        with patch.object(self.tag_model.objects, "filter", side_effect=record_filter):
+            bulk_add_tags_to_instances(tag_or_tags=supplied, instances=findings)
+
+        self.assertEqual(
+            len(locked_order), 3,
+            msg=f"expected one count UPDATE per tag, got {locked_order}",
+        )
+        self.assertEqual(
+            locked_order, sorted(locked_order),
+            msg="tag rows must be locked in ascending id order so concurrent imports "
+                f"cannot deadlock; got {locked_order} "
+                f"(tag ids: {sorted(tag_ids_by_name.items(), key=itemgetter(1))})",
+        )