fix(dedupe): re-point chained duplicates before deleting excess ones (backport of #15364) (#15383)

* fix(dedupe): re-point chained duplicates before deleting excess ones

async_dupe_delete could pick an excess duplicate that a SURVIVING finding
still references via duplicate_finding (a chained duplicate A -> B -> C,
which arises from past bugs or high parallel load - the case
fix_loop_duplicates exists for). That self-FK is ON DELETE DO_NOTHING, so
the delete raises IntegrityError, the chunk rolls back, and the periodic
task retries the same finding forever without ever making progress.
Observed in production: the same finding id failing every run, and no
excess duplicates deleted on that instance at all.

Before deleting, inbound references from survivors are now re-pointed at
their chain's root outside the delete set; survivors whose chain
dead-ends or loops inside it are promoted to originals, mirroring how
fix_loop_duplicates handles parentless duplicates.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit a83aca3)

* test: assert the FK invariant the dedupe fix protects

Django defers FK checks to COMMIT, so inside a TestCase transaction the
missing re-point surfaces as a reference to a deleted row rather than the
IntegrityError production sees at commit time. Assert both the dangling
reference and connection.check_constraints() so the test fails for the
right reason without the fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit 04b855a)

---------

Co-authored-by: Greg Anderson <greg.anderson@owasp.org>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: 3 people

dojo/tasks.py

@@ -1,4 +1,5 @@
 import logging
+from collections import defaultdict
 
 import pghistory
 from celery import Task
@@ -41,6 +42,74 @@ def async_dupe_delete(*args, **kwargs):
         _async_dupe_delete_impl()
 
 
+def _repoint_chained_duplicates(ids_to_delete):
+    """
+    Break inbound duplicate_finding references from findings that survive this run.
+
+    Chained duplicates (A -> B -> C, from past bugs or high parallel load - see
+    fix_loop_duplicates) can leave a SURVIVING finding pointing at an excess duplicate
+    that is about to be deleted. The duplicate_finding self-FK is ON DELETE DO_NOTHING,
+    so deleting while such references exist raises IntegrityError, rolls the chunk
+    back, and the task retries forever without ever making progress (observed in
+    production: the same finding id failing every run).
+
+    Survivors are re-pointed at their chain's root outside the delete set; when the
+    chain dead-ends or loops inside the delete set they are promoted to originals,
+    mirroring how fix_loop_duplicates handles parentless duplicates.
+    """
+    delete_set = set(ids_to_delete)
+
+    # Each deleted finding's own parent lets us walk chains that pass through the
+    # delete set: id -> duplicate_finding_id.
+    parent_of_deleted = dict(
+        Finding.objects.filter(id__in=delete_set).values_list("id", "duplicate_finding_id"),
+    )
+
+    def resolve_surviving_root(start_id):
+        seen = set()
+        current = start_id
+        while current in parent_of_deleted:
+            if current in seen:
+                # Defensive: a reference loop entirely inside the delete set.
+                return None
+            seen.add(current)
+            current = parent_of_deleted[current]
+        return current  # A surviving finding id, or None when the chain dead-ends.
+
+    survivors = (
+        Finding.objects
+        .filter(duplicate_finding_id__in=delete_set)
+        .exclude(id__in=delete_set)
+        .values_list("id", "duplicate_finding_id")
+    )
+
+    repoint_groups = defaultdict(list)  # surviving root id -> [survivor ids]
+    promote_ids = []
+
+    for survivor_id, parent_id in survivors:
+        root_id = resolve_surviving_root(parent_id)
+        if root_id is None or root_id == survivor_id:
+            # No surviving root (or the chain circles back to the survivor
+            # itself): promote to original rather than fabricate a self-loop.
+            promote_ids.append(survivor_id)
+        else:
+            repoint_groups[root_id].append(survivor_id)
+
+    if not repoint_groups and not promote_ids:
+        return
+
+    deduplicationLogger.warning(
+        "dupe delete: re-pointing %d chained duplicate reference(s) (promoting %d to original) before deleting %d findings",
+        sum(len(ids) for ids in repoint_groups.values()), len(promote_ids), len(delete_set),
+    )
+
+    for root_id, survivor_ids in repoint_groups.items():
+        Finding.objects.filter(id__in=survivor_ids).update(duplicate_finding_id=root_id)
+
+    if promote_ids:
+        Finding.objects.filter(id__in=promote_ids).update(duplicate_finding=None, duplicate=False)
+
+
 def _async_dupe_delete_impl():
     """Internal implementation of async_dupe_delete within pghistory context."""
     try:
@@ -115,6 +184,8 @@ def _async_dupe_delete_impl():
     logger.info("total number of excess duplicates to delete: %s", len(ids_to_delete))
 
     if ids_to_delete:
+        _repoint_chained_duplicates(ids_to_delete)
+
         # order_desc=True deletes higher ids before lower ids, consistent with how
         # finding_delete handles duplicate clusters (duplicate_cluster.order_by("-id").delete()).
         bulk_delete_findings(Finding.objects.filter(id__in=ids_to_delete), order_desc=True)

unittests/test_duplication_loops.py

@@ -1,6 +1,7 @@
 import logging
 
 from crum import impersonate
+from django.db import connection
 from django.test.utils import override_settings
 
 from dojo.finding.deduplication import set_duplicate
@@ -407,6 +408,66 @@ def test_delete_duplicate_order(self):
         self.finding_a.refresh_from_db()
         self.assertEqual(self.finding_a.duplicate_finding_set().count(), 1)
 
+    def test_delete_duplicate_excess_with_chained_reference(self):
+        """
+        A surviving finding that points at an excess duplicate (a chained duplicate, from past
+        bugs or high parallel load) must not be left dangling. The duplicate_finding self-FK is
+        ON DELETE DO_NOTHING, so without re-pointing the survivor first the delete leaves a
+        reference to a deleted row. Django defers FK checks to COMMIT, so in production that
+        surfaces as an IntegrityError that rolls the chunk back and makes the periodic task
+        retry the same finding forever; inside a TestCase transaction it surfaces as the
+        dangling reference and failed constraint check asserted below.
+        """
+        system_settings = System_Settings.objects.get(no_cache=True)
+        system_settings.delete_duplicates = True
+        system_settings.max_dupes = 1
+        system_settings.save()
+
+        self.finding_b.date = "2024-01-01"
+        self.finding_c.date = "2023-01-01"
+        set_duplicate(self.finding_b, self.finding_a)
+        set_duplicate(self.finding_c, self.finding_a)
+
+        # A fourth finding chained onto the excess duplicate C. set_duplicate would
+        # normalize the chain to the root, so wire the pathological state directly,
+        # the way it exists in the wild.
+        finding_x = copy_model_util(Finding.objects.get(id=4), exclude_fields=["duplicate_finding"])
+        finding_x.title = "X: " + finding_x.title
+        finding_x.hash_code = None
+        finding_x.duplicate = True
+        finding_x.duplicate_finding = self.finding_c
+        finding_x.save()
+
+        try:
+            _async_dupe_delete_impl()
+
+            self.assertFalse(
+                Finding.objects.filter(id=self.finding_c.id).exists(),
+                "The excess duplicate (Finding C) should have been deleted despite the chained reference.",
+            )
+            self.assertFalse(
+                Finding.objects.filter(duplicate_finding_id=self.finding_c.id).exists(),
+                "No finding may still reference the deleted duplicate.",
+            )
+            # The check Postgres would run at COMMIT in production, where a leftover
+            # reference is the IntegrityError that wedges the periodic task.
+            connection.check_constraints()
+            self.assertTrue(
+                Finding.objects.filter(id=self.finding_b.id).exists(),
+                "The newest duplicate (Finding B) should still exist.",
+            )
+
+            finding_x.refresh_from_db()
+            self.assertEqual(
+                finding_x.duplicate_finding_id,
+                self.finding_a.id,
+                "The chained survivor should be re-pointed at the cluster's surviving root.",
+            )
+            self.assertTrue(finding_x.duplicate, "The chained survivor stays a duplicate, now of the root.")
+        finally:
+            if finding_x.id and Finding.objects.filter(id=finding_x.id).exists():
+                finding_x.delete()
+
     def test_delete_duplicate_order_same_date_tiebreak_by_id(self):
         """When duplicate findings share the same date, excess deletes use id as tie-break (oldest id first)."""
         system_settings = System_Settings.objects.get()