fix(dedupe): stop imports failing when findings are deleted under them

[Maffooch]

Description

Imports, reimports and POST /api/v2/findings/ fail, or silently lose the findings they just wrote, when a finding is deleted while they are still working on it. The usual error is insert or update on table "<child>" violates foreign key constraint ... Key (finding_id)=(N) is not present in table "dojo_finding". It shows up on dojo_finding_found_by, dojo_finding_cwe, dojo_findingvulnerabilityreference, dojo_finding_tags, dojo_endpoint_status, dojo_locationfindingreference, dojo_finding_group and the Pro EnhancedFinding companion. On the background import path the whole run fails after the API has already returned 200, so that run's scan data is lost and the caller never sees an error.

Django declares its foreign keys DEFERRABLE INITIALLY DEFERRED, so a writer inserting a child row takes no lock on the finding that a delete could wait for. The guards added so far each protect one child table, and a row lock on the delete side cannot make a writer wait. Production logs showed two deleters behind the failures:

1. The excess-duplicate delete (async_dupe_delete, every minute). With a low max_dupes it deletes duplicates the moment they are marked, including ones a running import created or matched seconds earlier. A common trigger is a report with several rows for one package (one per CVE) that hash to the same finding: every match saves the finding again, every save re-runs deduplication, and the next cleanup tick deletes it mid-import.
2. Deletes of the finding or its test while the import runs: DELETE /api/v2/findings/{id}/ from a client, or a test, engagement or product delete (the asynchronous one removes the findings first and the test last).

This PR fixes both at the delete and import level rather than per child table.

The excess-duplicate delete skips a test only while an import is running on it

• Every import and reimport, and every deduplication batch, holds a shared PostgreSQL advisory lock on its test while it runs (dojo/importers/import_lock.py). The lock is taken as soon as the test exists and before the first finding is created or matched.
• Tests under import are left out of the run's selection before the per-run cap (DD_DUPE_DELETE_MAX_PER_RUN) applies, so they never use up a run and every other test is reached.
• Each delete chunk then checks again inside its transaction. It first takes the same key with a non-waiting try-lock on each of its tests and skips the duplicates of any test whose lock is held. Only after that does it row-lock findings, and only those of free tests, so the delete never holds a row of a test under import and its import never waits on the delete. The next run after the import ends deletes them, so a test that is reimported every few minutes is still cleaned up between reimports. Duplicates in every other test are deleted exactly as before.
• An import that starts on a test while a chunk is deleting its duplicates waits for that chunk to commit, then finds those rows gone rather than half deleted.
• Product grading is scheduled only for products that actually lost a duplicate.
• A finding saved in the last DD_DUPE_DELETE_GRACE_MINUTES (default 1) also waits for a later run. That covers a create request that is still writing the finding's child rows.
• With duplicate deletion off (the default), no lock is taken and no query is added.

A lock can never outlive its import

A lock left behind would keep the delete off that test for good, so each way one could be left behind is bounded:

• A worker that crashes or is killed takes the lock with its connection.
• Locks are taken one test at a time, so a wait cancelled part way leaves none behind. A release that fails for any reason (the import's transaction aborted, the connection broke, a time limit) marks the connection to be closed. Django and Celery close it at the end of the request or task, which drops the lock, even when CONN_MAX_AGE would keep the connection open indefinitely.
• Behind a connection pooler in transaction mode the release can land on another server connection and find nothing. That is logged as an error naming DD_DUPE_DELETE_SKIP_TESTS_UNDER_IMPORT=False, which turns the lock off.
• If every connection holding a test's lock has been idle for DD_DUPE_DELETE_ABANDONED_IMPORT_LOCK_MINUTES (default 30), no import is running, so the delete treats the lock as abandoned, deletes anyway, and logs a warning with the holding backends. This covers a host that died without closing its connection. A holder whose state the database user cannot see (a backend of another database role) counts as live, even next to an idle one, so the check can only give a test back to the delete. The decision is made per holder in Python (abandoned_tests), not in an SQL aggregate that would skip the unknown state.

No resurrection on matched-finding saves

A reimport saving a finding it matched earlier in the run (CVE back-fill, reactivation, mitigation, component fill) now goes through save_matched_finding(), which saves with force_update. A finding deleted since is skipped and stays deleted, instead of failing the import with Finding.DoesNotExist or being re-created by Django's UPDATE-then-INSERT fallback.

A clear error when the import target is being deleted

process_scan turns a foreign-key violation that names a finding or test which no longer exists into the importer's existing "was deleted while the scan was being processed" style ValidationError, so the caller gets a 400 with a reason instead of a 500. A violation naming a row that still exists is re-raised unchanged. A matched finding that the reimport does not need to save (an unchanged match) and that a client deletes mid-run still fails the reimport, now with this 400.

Expect a small rebase against the open #16337 (async_dupe_delete as a singleton), which touches the same two files.

Test results

New tests in unittests/test_dedupe_delete_commit_race.py commit real rows, so the deferred foreign keys fire as in production, and each one deletes every row it committed afterwards so later modules see a clean database. Deleters run on their own connection, as the beat task does in another worker.

• TestDupeDeleteRacesInFlightWriter: a new duplicate, and a matched duplicate whose test an import is running on, survive while their writer adds a found_by row. Controls: a settled duplicate is deleted, a grace of 0 deletes at once, and a duplicate of a test that was reimported moments ago, with no import running, is deleted. That last control fails on the previous revision of this PR, which held back a whole test for 30 minutes after any import activity. A deduplication batch holds its test's lock while it runs and releases it, and takes none with duplicate deletion off.
• TestImportLockCannotOutliveItsImport: a release in an aborted transaction drops the connection and the lock; a release that finds no lock logs the fix; a lock whose holder sat idle past the threshold no longer holds the delete back; and one within the threshold still does. The first three fail before their fix.
• TestDupeDeleteLeavesImportedTestsAlone: with a per-run cap of one and the test under import holding the lowest id, the run still deletes the other test's duplicate; an import can save its own duplicate, and tag it with a tag the chunk is deleting, while a chunk deletes another test's duplicates, without waiting or being aborted. All three fail on the first revision of the lock, which row-locked the whole chunk before checking the locks and applied the cap first.
• TestAbandonedImportLockDecision: a holder with hidden state beside an idle one keeps its test; only tests whose every holder is idle past the threshold are abandoned.
• TestImportRacesDupeDelete: a real import, reimport, and reimport matching existing duplicates, with the delete firing on another connection right after each deduplication batch and the grace period off, so the lock alone protects them. All three fail without the lock (findings lost, or the dojo_endpoint_status FK violation).
• TestFindingCreateRacesDupeDelete: POST /api/v2/findings/ with the delete firing right after the create's own deduplication (fails on dev with a 500).
• TestReimportStartRacesDupeDeleteSelection: a delete run whose selection predates a reimport does not remove the duplicates that reimport has matched since (fails without the lock).
• TestTaggedImportRacesTestDelete and its Locations-off subclass: imports with parser and import tags, group_by finding groups, endpoint and location flushes, and an asynchronous-delete shape (findings gone, test still present). Each must fail with the clear error rather than a raw FK violation. Deleting a different test that shares the import's tags leaves the tag counts consistent.
• TestReimportRacesMatchedFindingDelete: a finding deleted between two matched saves, and before its reactivation save, is not re-created and does not fail the reimport (fails on dev with Finding.DoesNotExist).

Three test_duplication_loops tests that cover which duplicates are excess pin DD_DUPE_DELETE_GRACE_MINUTES=0, because they save their findings just before the delete.

Run locally in the OSS unit-test container: test_dedupe_delete_commit_race, test_finding_helper and test_duplication_loops pass (96 tests). To check the row cleanup, the race module was run first on a fresh test database, and then the import, reimport, tag, JIRA, API v3 import and sample-data modules ran against that same kept database: 1797 tests, all passing. ruff check is clean.

The race was also reproduced against a running stack: 3000-finding reimports, finding creates and test deletes, with the duplicate delete running every 50 ms. Before the change there were HTTP 500s on create and on sync reimport, and thousands of lost import-history rows.

Documentation

docs/content/triage_findings/finding_deduplication/about_deduplication.md gains a "When duplicates are deleted" section describing when a duplicate waits and the three settings.

Checklist

• Make sure to rebase your PR against the very latest dev.
• Submit all PRs, features and bug fixes alike, against the dev branch.
• Give a meaningful name to your PR, as it may end up being used in the release notes.
• Your code is Ruff compliant (see ruff.toml).
• Your code is python 3.13 compliant.
• If this is a new feature and not a bug fix, you've included the proper documentation in the docs at https://github.com/DefectDojo/django-DefectDojo/tree/dev/docs as part of this PR.
• Model changes must include the necessary migrations in the dojo/db_migrations folder.
• Add applicable tests to the unit tests.
• Add the proper label to categorize your PR.

🤖 Generated with Claude Code

[Maffooch]

Review verdict: approve with nits (merge once the re-run unit tests are green)

Reviewed at 3632ced1fd and re-checked at 3dbe667fb5, which landed during the review. Backend-only change, no UI.

Blocking

Nothing in the code itself.

• CI was red on 3632ced1fd, and 3dbe667fb5 fixes it. Both Rest Framework unit-test shards failed with 3 errors in unittests.test_finding_helper.TestPostProcessFindingsBatchDeadlockRetry: TypeError: cannot unpack non-iterable Mock object. Cause: those tests mock System_Settings with a bare Mock, so delete_duplicates was truthy and the new mark_tests_under_import() call (dojo/finding/helper.py:590) built a query from Mock test ids. I reproduced the 3 errors locally against the 3632ced1fd test file. With 3dbe667fb5 (explicit delete_duplicates=False on the mock) all 25 test_finding_helper tests pass. The unit-test jobs on the new head were still pending when I posted this, so merge once they go green.

What I verified

• The new tests fail without the fix. I ran unittests.test_dedupe_delete_commit_race with the six changed source files bind-mounted back to the merge base (9d80928f17) and the PR's test file in place: 15 failures and 1 error out of 39. At the PR head all 39 pass.
• Wider suite. Every OSS module matching dedup/duplic/reimport/importer/false_positive/dupe/import_history passed at the PR head (jira excluded): Ran 658 tests ... OK (skipped=10). That covers test_importers_performance, so query counts are unchanged.
• Lint. ruff==0.16.10 (the version pinned in requirements-lint.txt) passes on all 8 changed Python files.
• Live behaviour on a running stack (26,489 findings, 985 duplicates, all in one test). I called _async_dupe_delete_impl() inside a transaction that I rolled back afterwards, with max_dupes=0 and every finding and test aged by 2h:
  • grace 30, test marked through mark_tests_under_import: 985 -> 985 duplicates (1 query)
  • grace 30, test not marked: 985 -> 785 (the 200 per-run cap)
  • grace 0, test marked: 985 -> 785
  • after the rollback there were 985 duplicates again.
• Query plan. EXPLAIN ANALYZE of the settled-duplicate selection uses the existing duplicate index on dojo_finding, and the updated filters run as row filters on it. There is no new seq scan on findings. dojo_test is seq-scanned here, but this stack has only 13 tests. At 10M findings the cost of newer_siblings_subq is unchanged by this PR, and the new predicate only narrows that selection further.
• Lock ordering. The importer runs in autocommit (see the comment at base_importer.py:578), so the UPDATE dojo_test in mark_tests_under_import commits right away and holds no lock across the import. The delete chunk takes findings FOR UPDATE, then tests FOR SHARE (dojo/tasks.py:162). If a reimport marks its test while a chunk holds the test, it waits for that chunk to commit. I don't see a long-held lock on the other side that could deadlock with it.
• Import history. A matched finding that save_matched_finding skips is still in unchanged_items when it was appended before the component-fill save (default_reimporter.py:1268). update_import_history already drops deleted ids before its bulk insert (base_importer.py:758), so this cannot raise a new FK violation.

Nits / worth considering (not blocking)

1. A test reimported more often than the grace period never has its excess duplicates deleted. test.updated is refreshed on every reimport, at the start and when the test is saved at the end. A CI pipeline that reimports into the same test every 10-20 minutes will therefore keep that test's duplicates indefinitely at the 30-minute default. The docs section mentions it ("nothing has been imported into its Test"), but the release note should say it plainly, because before this change those duplicates were deleted.
2. The PR description overstates the red-on-dev claim. It says each new test fails on dev. At the merge base, 16 of the 39 tests in the module fail; the controls pass, as do several TestTaggedImportRacesTestDelete variants (import tags, location-reference flush). A one-line correction would do.
3. save_matched_finding only protects matched findings that get saved. A matched finding deleted mid-run that goes down the unchanged path (no save) still reaches the child-row writes. The whole reimport then fails, with the new clear 400 rather than a raw FK error. That is acceptable, but "does not fail the reimport" in the description only holds for the save paths.
4. The skip log in dojo/tasks.py:193 names only one cause. It says "whose test an import started on since they were selected", but the re-check also skips findings that were themselves saved after selection. Something like "that are no longer settled" would cover both.
5. findings_deleted_message (base_importer.py:96) always blames a test, engagement or product delete. With DD_DUPE_DELETE_GRACE_MINUTES=0, the duplicate cleanup can produce the same message. A clause naming duplicate cleanup as a possible cause would cut down support confusion.
6. Rebase on fix(dedupe): run async_dupe_delete as a singleton to stop overlapping runs deadlocking #16337 as the description says. Both PRs touch _async_dupe_delete_impl.

Not verified here

• A concurrent stress run at production scale. I relied on the committed-row tests, plus the 3000-finding stress run described in the PR, which I did not repeat.
• Every downstream consumer of Test.updated. Moving it at reimport start shifts any "last scanned" style read that uses it a little earlier, and a reimport that later fails still moves it.

[Maffooch]

Adversarial review at 2e63fd2: changes requested

I didn't write this change. I tried to break it against a real PostgreSQL 18 test database, using scratch tests that are not part of the PR. What holds up:

• The lock is load-bearing. With DD_DUPE_DELETE_SKIP_TESTS_UNDER_IMPORT=False, 8 of the 44 tests in test_dedupe_delete_commit_race fail: the three TestImportRacesDupeDelete tests, TestReimportStartRacesDupeDeleteSelection, the matched-duplicate and dedupe-batch tests, and two lock-lifetime tests. At head, all 44 pass, and test_duplication_loops plus test_finding_helper pass (47).
• I found no way for the lock to be held forever in the default config. Celery 5.6.3's on_task_postrun force-closes every connection after each task, and CONN_MAX_AGE defaults to 0, so any leak lasts one task or request at most.
• The matched-save path is fine: a match deleted before the component-fill save does not fail the reimport (import history already filters out deleted findings). Docs and settings defaults match the code.
• CI: the unit-test and UI-test jobs were still pending when I checked.

Blockers

1. The delete chunk row-locks the duplicates it is about to skip, which can deadlock the import the lock exists to protect.
_delete_excess_duplicates runs SELECT ... FOR UPDATE on every finding in the chunk (dojo/tasks.py:176). Only after that does it check the import locks (dojo/tasks.py:178). The skipped findings keep their FOR UPDATE until the chunk commits, which is after the whole cascade delete of the other tests' duplicates. Two consequences, both reproduced:

• An import writing its own duplicate blocks behind the chunk. I held test A's import lock on another connection and ran the delete, with one duplicate in A and one in a free test B. An UPDATE of A's duplicate from a third connection during the chunk timed out with canceling statement due to lock timeout, even though the chunk skips that row and never deletes it.
• It deadlocks, and the import loses. Setup: A's import tags its own duplicate inside a transaction, which updates the shared tag's count row; then the FK check on the new through row runs. The chunk is deleting B's duplicate, which carries the same tag. The import waits on the chunk's FOR UPDATE of A's duplicate, and the chunk waits on the import's tag row. Result: deadlock detected ... while locking tuple ... in relation "dojo_finding" ... FOR KEY SHARE, and the victim was the import. The trigger is common: import tags shared across tests plus a low max_dupes.

Suggested fix: take the advisory try-locks first, on the test ids the selection already has, and then FOR UPDATE only the findings of tests that were free. The try-lock never waits, so this order adds no blocking. The import's shared lock and the chunk's exclusive lock still serialize an import that starts mid-chunk.

2. A test under import at the head of the selection stops duplicate deletion for every test.
The selection is .order_by("id")[:DUPE_DELETE_MAX_PER_RUN] (dojo/tasks.py:122-123). Busy tests are only removed afterwards, inside the chunk. When the lowest-id 200 excess duplicates belong to a test whose import is running, every run selects those same 200, skips all of them, and deletes nothing anywhere. Repro, with DUPE_DELETE_MAX_PER_RUN=1: test A holds its import lock and has the lower-id duplicate, test B has a settled excess duplicate, and five delete runs leave B's duplicate in place. On dev, run 1 deletes A's and run 2 deletes B's. Large tests that are reimported back to back hit this, and so does any lock the valve doesn't release. It also makes the docs line "Every other excess duplicate is deleted on the next run, as before" (about_deduplication.md:180) untrue. Suggested fix: exclude busy tests from the selection itself (try-lock or a pg_locks anti-join before the slice), or keep selecting past skipped rows until the cap is filled.

Should fix

3. The abandoned-lock valve does not do what its docstring says when it cannot see a holder.
The docstring says "A holder whose state this database user may not see counts as live" (dojo/importers/import_lock.py:137). But bool_and ignores NULLs (import_lock.py:152), and an unseen holder's state comes back as NULL. So a test with one idle holder past the threshold and one live holder under another role counts as abandoned. I checked this by running the PR's query as a non-superuser role. Role A had an idle holder, role B had an active holder (pg_sleep). The query returned the test with {idle,NULL}. As superuser, it returned nothing. This needs separate DB roles to happen, but it is the one guarantee the valve makes. Fix: bool_and(COALESCE(activity.state = 'idle' AND activity.state_change < ..., false)), plus a test with a hidden holder.

Nits

• dojo/finding/helper.py:555 takes lock_tests_of_findings_under_import before the try. The multi-test unnest statement takes locks one row at a time, and session advisory locks survive a statement error. So a cancel or statement_timeout partway through leaks the locks already taken, with no release and no close_at. The same applies to a non-DatabaseError (for example SoftTimeLimitExceeded) inside unlock_tests_under_import, which only catches DatabaseError (import_lock.py:96). Celery's forced close bounds both today. A web worker with conn_max_age set does not, and the valve only fires if that connection goes idle for 30 minutes, which a busy pooled web connection never does.
• affected_products (dojo/tasks.py:130) still includes the products of skipped findings, so product grading is scheduled for products where nothing was deleted.
• Each chunk now locks its findings twice: the new FOR UPDATE, then lock_findings_for_delete inside bulk_delete_findings. If blocker 1 is fixed as suggested, the outer lock can become the only one.
• The PR body says the delete "never waits on an import". The chunk's FOR UPDATE does wait for any import transaction holding a row lock on a chunk finding.
• Test cleanup: _delete_rows_added_since (unittests/test_dedupe_delete_commit_race.py:161) deletes every row above a high-water mark in every integer-keyed table in the schema. That is correct for one writer. It also deletes rows committed by anything else writing to the same kept test DB, such as a second runner, or a daemon _ConcurrentThread from a timed-out test that commits late. It also deletes rows the framework creates lazily and caches in-process (content types, permissions). I didn't hit either. A one-line docstring warning that the module assumes it is the only writer would help.

Verdict: changes requested, for blockers 1 and 2. Both fall inside the code this PR adds, and both have small fixes.

🤖 Generated with Claude Code

[Maffooch]

Re-review at 55dff3c (reply to my verdict)

I re-ran every break attempt this push touches against the same PostgreSQL 18 test database. The scratch tests are the same ones as before and are not part of the PR. As a control, I ran them against 2e63fd2's dojo/tasks.py and dojo/importers/import_lock.py: 5 of 6 fail there and all 6 pass here, so each one tells the two revisions apart. The PR's own modules also pass at this head: test_dedupe_delete_commit_race, test_duplication_loops and test_finding_helper, 96 tests.

Blocker 1 (skipped rows kept locked, and the tag deadlock): fixed. The chunk now takes the non-blocking import locks first (dojo/tasks.py:181). It then row-locks only findings of free tests that are still settled (dojo/tasks.py:186).

• Locked-row attempt: while the chunk deletes another test's duplicate, a third connection updates the busy test's duplicate. That update now succeeds. At the old head it hit canceling statement due to lock timeout.
• Deadlock attempt: the import holds the shared tag row and writes its own duplicate's tag through row with the FK check forced. It now finishes without waiting on the chunk. The chunk deletes the free duplicate and leaves the busy one. At the old head the same setup gave deadlock detected ... FOR KEY SHARE, and the import was the victim.

Blocker 2 (per-run cap starvation): fixed. Tests whose lock is held are now dropped from the selection before the cap applies (dojo/tasks.py:114). Repro: DUPE_DELETE_MAX_PER_RUN=1, the busy test holds the lower-id duplicate, and the free test holds a settled one. The free test's duplicate is now deleted within five runs. At the old head it survived all five. The docs line about every other duplicate being deleted on the next run now matches the code.

bool_and NULL visibility in the valve: fixed. The valve now pulls holders row by row and decides in Python (dojo/importers/import_lock.py:183-199). A holder whose state is not visible counts as live. I re-ran the role demo with the new holder query, as non-superuser role A:

• Test 42 had one active holder under role B and one idle holder under A. The query returned <NULL> state and age for B's holder and idle for A's.
• Test 43 had only A's idle holder.

Fed those rows with a 2 s threshold, abandoned_tests returned {43}. So the test with a hidden live holder stays protected, and the valve still frees the one that is really abandoned. The previous query treated test 42 as abandoned. Both temporary roles are dropped.

Lock-leak nits: fixed.

• Cancelled part way: lock_tests_under_import now locks one test per statement and releases what it took on any exception (import_lock.py:70-71). Repro: lock [a, b] with b held exclusively elsewhere and statement_timeout=500ms. The call raises OperationalError and a is no longer held on the connection. At the old head a stayed locked.
• Non-DatabaseError during release: unlock_tests_under_import now catches BaseException, sets close_at and re-raises (import_lock.py:102). Repro: a non-database exception raised from the release's cursor propagates, the connection is marked to close, and after close_if_unusable_or_obsolete() the lock is gone. At the old head close_at stayed None.
• The grading nit is fixed too: only products that lost a duplicate are graded (dojo/tasks.py:145).

One remaining nit, not blocking: an asynchronous exception (a Celery soft time limit) that lands after pg_advisory_lock_shared returns but before locked.append (import_lock.py:70) still leaks that one lock. Celery 5.6 closes the connection after every task, so it cannot outlive the task.

Verdict: changed to no blockers, ready to merge once CI is green. When I checked, 19 jobs at this head were still pending, including the unit-test, UI-test and performance jobs.

🤖 Generated with Claude Code