From 8fdac9df0167facf0c98722711e6baddbade1cb0 Mon Sep 17 00:00:00 2001 From: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Date: Thu, 20 Aug 2026 12:19:20 +0000 Subject: [PATCH] fix(finding): retry deferred-FK race on single-finding delete Deleting a single finding via the API (DELETE /api/v2/findings/{id}/) or the UI delete view runs Django's collector, which clears the finding's Test_Import_Finding_Action children before deleting the finding row. Those FK constraints are DEFERRABLE INITIALLY DEFERRED, so a concurrent import that commits a new child row referencing the finding between the child clear and the transaction COMMIT trips a foreign-key violation (SQLSTATE 23503) at commit time, surfaced to the caller as an Internal Server Error. The async cascade delete already retries this delete-vs-import race (_is_retryable_delete_conflict), but the synchronous single-finding path had none. Add delete_finding_with_conflict_retry, which re-runs the delete on a transient conflict (FK violation 23503, or deadlock/serialization 40P01/40001) with exponential backoff and re-raises anything else or a conflict that survives every attempt. Wire it into both the API viewset and the UI delete view. Tests cover: success first attempt, FK-conflict retried then succeeds, deadlock/serialization retried, unique-violation not retried, retries exhausted re-raises, and growing backoff. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01AFdaF25N1sK1Pfvb2xr65q --- dojo/finding/api/views.py | 2 +- dojo/finding/helper.py | 47 +++++++- dojo/finding/ui/views.py | 4 +- .../test_finding_delete_conflict_retry.py | 110 ++++++++++++++++++ 4 files changed, 160 insertions(+), 3 deletions(-) create mode 100644 unittests/test_finding_delete_conflict_retry.py diff --git a/dojo/finding/api/views.py b/dojo/finding/api/views.py index 33f0c83101..425e75bba9 100644 --- a/dojo/finding/api/views.py +++ b/dojo/finding/api/views.py @@ -183,7 +183,7 @@ def destroy(self, request, *args, **kwargs): push_to_jira = get_request_boolean(request, "push_to_jira") except DRFValidationError as error: raise DRFValidationError({"push_to_jira": error.detail}) from error - instance.delete(push_to_jira=push_to_jira) + finding_helper.delete_finding_with_conflict_retry(instance, push_to_jira=push_to_jira) return Response(status=status.HTTP_204_NO_CONTENT) def get_queryset(self): diff --git a/dojo/finding/helper.py b/dojo/finding/helper.py index 6c1b5f741d..f9116680d4 100644 --- a/dojo/finding/helper.py +++ b/dojo/finding/helper.py @@ -19,7 +19,7 @@ import dojo.risk_acceptance.helper as ra_helper from dojo.celery import app -from dojo.db_utils import is_transient_db_conflict +from dojo.db_utils import is_foreign_key_conflict, is_transient_db_conflict from dojo.endpoint.utils import endpoint_get_or_create, save_endpoints_to_add from dojo.file_uploads.helper import delete_related_files from dojo.finding.cwe import finding_cwe_labels @@ -649,6 +649,51 @@ def finding_delete(instance, *, push_to_jira=DELETE_JIRA_SYNC_UNSET, **kwargs): instance.found_by.clear() +# Seconds before the first retry of a single-finding delete; doubled on each attempt. +SINGLE_DELETE_RETRY_DELAY = 0.5 +SINGLE_DELETE_MAX_CONFLICT_RETRIES = 3 + + +def delete_finding_with_conflict_retry(finding, **kwargs): + """ + Delete one finding, retrying the delete-vs-import race that otherwise returns a 500. + + A single-finding delete runs Django's collector, which clears the finding's + ``Test_Import_Finding_Action`` children before deleting the finding row. Those FK + constraints are ``DEFERRABLE INITIALLY DEFERRED``, so a concurrent import that commits a + new child row referencing this finding between the child clear and the transaction COMMIT + trips a foreign-key violation at commit time (SQLSTATE 23503). The reference is real but + transient: on a re-run the collector clears the newly-created child and the delete + completes. Deadlocks and serialization failures (40P01/40001) against a concurrent import + or the dedup job are retried the same way. + + This mirrors the async cascade delete's ``_is_retryable_delete_conflict`` handling (see + ``dojo.utils.async_delete_task``) for the synchronous single-finding API/UI delete path, + which previously had no such protection and surfaced the race as an Internal Server Error. + Each attempt re-runs ``Finding.delete`` in a fresh transaction (there is no + ``ATOMIC_REQUESTS``), and the failed attempt has already rolled back, so the finding still + exists to be re-deleted. Any non-conflict error, and a conflict that survives every + attempt, is re-raised. ``kwargs`` (e.g. ``push_to_jira``) are forwarded to + ``Finding.delete``. + """ + for attempt in range(SINGLE_DELETE_MAX_CONFLICT_RETRIES + 1): + try: + finding.delete(**kwargs) + except (OperationalError, IntegrityError) as exc: + retryable = is_transient_db_conflict(exc) or is_foreign_key_conflict(exc) + if not retryable or attempt == SINGLE_DELETE_MAX_CONFLICT_RETRIES: + raise + backoff = SINGLE_DELETE_RETRY_DELAY * (2 ** attempt) + logger.warning( + "delete_finding_with_conflict_retry: transient DB conflict deleting finding %s, " + "retry %d/%d in %.1fs: %s", + getattr(finding, "pk", None), attempt + 1, SINGLE_DELETE_MAX_CONFLICT_RETRIES, backoff, exc, + ) + sleep(backoff) + else: + return + + @receiver(post_delete, sender=Finding) def finding_post_delete(sender, instance, **kwargs): # Catch instances in async delete where a single object is deleted more than once diff --git a/dojo/finding/ui/views.py b/dojo/finding/ui/views.py index 4e9428399d..dc21e2c629 100644 --- a/dojo/finding/ui/views.py +++ b/dojo/finding/ui/views.py @@ -1092,7 +1092,9 @@ def get_finding(self, finding_id: int): def process_form(self, request: HttpRequest, finding: Finding, context: dict): if context["form"].is_valid(): product = finding.test.engagement.product - finding.delete(push_to_jira=context["form"].cleaned_data.get("push_to_jira")) + finding_helper.delete_finding_with_conflict_retry( + finding, push_to_jira=context["form"].cleaned_data.get("push_to_jira"), + ) # Update the grade of the product async dojo_dispatch_task(calculate_grade, product.id) # Add a message to the request that the finding was successfully deleted diff --git a/unittests/test_finding_delete_conflict_retry.py b/unittests/test_finding_delete_conflict_retry.py new file mode 100644 index 0000000000..dc1ebd5528 --- /dev/null +++ b/unittests/test_finding_delete_conflict_retry.py @@ -0,0 +1,110 @@ +""" +Unit tests for the synchronous single-finding delete conflict retry. + +A single-finding delete (API ``DELETE /api/v2/findings/{id}/`` and the UI delete view) +runs Django's collector, which clears the finding's ``Test_Import_Finding_Action`` children +before deleting the finding row. Those FK constraints are ``DEFERRABLE INITIALLY DEFERRED``, +so a concurrent import that commits a new child row referencing this finding between the +child clear and the transaction COMMIT trips a foreign-key violation (SQLSTATE 23503) at +commit time -- surfaced to the caller as an Internal Server Error. + +``delete_finding_with_conflict_retry`` re-runs the delete for that transient race, mirroring +the async cascade delete's ``_is_retryable_delete_conflict`` handling. These tests mock +``Finding.delete`` so the retry logic is exercised without touching the database. +""" +from unittest.mock import MagicMock, patch + +from django.db import IntegrityError, OperationalError +from django.test import SimpleTestCase +from psycopg.errors import DeadlockDetected, ForeignKeyViolation, SerializationFailure, UniqueViolation + +from dojo.finding.helper import SINGLE_DELETE_MAX_CONFLICT_RETRIES, delete_finding_with_conflict_retry + + +def _wrapped(exc_class, driver_error_class, message="db error"): + """ + Build the exception shape the cloud reports show. + + Django re-raises the driver error as its own IntegrityError/OperationalError and keeps + the psycopg exception -- the one carrying the SQLSTATE -- as ``__cause__``. + """ + cause = driver_error_class(message) + exc = exc_class(str(cause)) + exc.__cause__ = cause + return exc + + +class TestDeleteFindingWithConflictRetry(SimpleTestCase): + + def _finding(self): + finding = MagicMock(name="finding") + finding.pk = 3109377 + return finding + + def test_success_on_first_attempt_deletes_once(self): + finding = self._finding() + with patch("dojo.finding.helper.sleep") as mock_sleep: + delete_finding_with_conflict_retry(finding, push_to_jira=False) + finding.delete.assert_called_once_with(push_to_jira=False) + mock_sleep.assert_not_called() + + def test_foreign_key_conflict_is_retried_then_succeeds(self): + """ + The reported failure: a concurrent import committed a Test_Import_Finding_Action + row referencing the finding between the child clear and COMMIT, so the delete hit a + deferred FK violation. On a re-run the collector clears the new child and the delete + completes, so the caller must not see a 500. + """ + finding = self._finding() + fk_exc = _wrapped( + IntegrityError, + ForeignKeyViolation, + 'update or delete on table "dojo_finding" violates foreign key constraint ' + '"dojo_test_import_fin_finding_id_28fe8e2d_fk_dojo_find" on table ' + '"dojo_test_import_finding_action"', + ) + finding.delete.side_effect = [fk_exc, None] + with patch("dojo.finding.helper.sleep") as mock_sleep: + delete_finding_with_conflict_retry(finding, push_to_jira=False) + self.assertEqual(finding.delete.call_count, 2) + mock_sleep.assert_called_once() + + def test_transient_conflict_is_retried_then_succeeds(self): + for driver_error_class in (DeadlockDetected, SerializationFailure): + with self.subTest(driver=driver_error_class.__name__): + finding = self._finding() + exc = _wrapped(OperationalError, driver_error_class) + finding.delete.side_effect = [exc, None] + with patch("dojo.finding.helper.sleep"): + delete_finding_with_conflict_retry(finding) + self.assertEqual(finding.delete.call_count, 2) + + def test_non_retryable_integrity_error_is_not_retried(self): + """Control: a unique violation (23505) is a real bug, not the delete-vs-import race.""" + finding = self._finding() + unique_exc = _wrapped(IntegrityError, UniqueViolation, "duplicate key value") + finding.delete.side_effect = unique_exc + with patch("dojo.finding.helper.sleep") as mock_sleep, self.assertRaises(IntegrityError): + delete_finding_with_conflict_retry(finding) + finding.delete.assert_called_once() + mock_sleep.assert_not_called() + + def test_conflict_surfaces_once_retries_are_exhausted(self): + """A conflict that keeps repeating must be reported rather than retried forever.""" + finding = self._finding() + fk_exc = _wrapped(IntegrityError, ForeignKeyViolation, "violates foreign key constraint") + finding.delete.side_effect = fk_exc + with patch("dojo.finding.helper.sleep"), self.assertRaises(IntegrityError): + delete_finding_with_conflict_retry(finding) + self.assertEqual(finding.delete.call_count, SINGLE_DELETE_MAX_CONFLICT_RETRIES + 1) + + def test_retry_backoff_grows_with_each_attempt(self): + finding = self._finding() + fk_exc = _wrapped(IntegrityError, ForeignKeyViolation, "violates foreign key constraint") + finding.delete.side_effect = fk_exc + with patch("dojo.finding.helper.sleep") as mock_sleep, self.assertRaises(IntegrityError): + delete_finding_with_conflict_retry(finding) + delays = [call.args[0] for call in mock_sleep.call_args_list] + self.assertEqual(len(delays), SINGLE_DELETE_MAX_CONFLICT_RETRIES) + self.assertEqual(delays, sorted(delays)) + self.assertLess(delays[0], delays[-1])