Dedupe: row-lock the duplicate_finding self-FK so import dedup and duplicate deletion stop failing at COMMIT

[Maffooch]

[sc-15672]

Description

Two background jobs fail at COMMIT on the dojo_finding.duplicate_finding_id self-FK, one from each side of the same race:

• Dedup post-processing of an import: Key (duplicate_finding_id)=(N) is not present in table "dojo_finding".
• The excess-duplicate delete task (async_dupe_delete): Key (id)=(N) is still referenced from table "dojo_finding".

The self-FK is ON DELETE DO_NOTHING and DEFERRABLE INITIALLY DEFERRED, so nothing is checked until COMMIT. Both sides already guard it with a read-then-write step, but neither step held a lock:

• _drop_links_to_deleted_originals (dojo/finding/deduplication.py) checked that each matched original still exists with a plain SELECT, then bulk-updated the links. A delete that was still uncommitted when the SELECT ran was invisible to it, so the flush wrote its links and its COMMIT failed once the delete committed.
• resolve_inbound_duplicate_references (dojo/finding/helper.py) read which findings still point into a chunk, re-pointed them, then deleted the chunk. A dedup flush that committed a new link into the chunk after that read made the chunk's COMMIT fail.

When either COMMIT fails the whole transaction rolls back: the entire batch's deduplication on the import side, the whole chunk on the delete side.

Fix

Both sides now lock the rows the FK joins, in one statement each, ordered by id.

• Dedup flush: the existence check becomes SELECT id ... WHERE id = ANY(originals + rows being written) ORDER BY id FOR KEY SHARE, inside the flush's existing transaction. FOR KEY SHARE is the lock Postgres itself takes on a referenced row when it checks a foreign key. It conflicts only with DELETE and key updates, so ordinary updates to an original and other flushes linking to it are not blocked. If a concurrent transaction is deleting an original, the check waits for it. Once the delete commits, the row is not returned and its links are dropped the same way the guard already dropped links to originals deleted earlier.
• Bulk delete chunk: a new lock_findings_for_delete(chunk_ids) takes SELECT ... FOR UPDATE ORDER BY id on the chunk right before resolve_inbound_duplicate_references. Any flush that wants to link into the chunk then waits until the chunk commits, and finds the original gone. A flush that locked first commits first, and the resolver's read sees its links.
• The resolver moved from before the child-row cascade to after it, so the chunk's finding rows are still the last rows the chunk locks, as they were when only the final DELETE locked them. The cascade skips every Finding relation, so it never reads or writes a duplicate_finding value.

Options considered and not taken:

• on_delete=SET_NULL: Django emulates it in the Collector, and the raw-SQL chunked delete bypasses the Collector. A database-level ON DELETE SET NULL would not fix the import side either, because the flush's uncommitted link is not visible to the delete. It would also leave duplicate=True rows with no original, and it needs a migration.
• Catching the deferred IntegrityError at COMMIT and retrying: this only works when the flush's atomic block is the outermost transaction, and it reacts after the failure where a lock prevents it.

Deadlock analysis:

• Flush against chunk: each side takes all of its row locks in one ascending-id statement before writing anything. Whichever side locks a shared row first, the other queues behind it and holds nothing the first side needs. The flush locks the rows it is about to write as well as the originals, because a flush can re-point a finding the chunk is deleting (the transitive flatten in set_duplicate). Locking only the originals produced a real deadlock detected in the new tests; the combined lock passes.
• Chunk against chunk: both use ascending-id FOR UPDATE on their own chunk, and chunks commit separately, so there is no cycle, including with order_desc=True.
• M2M and tag rows: the chunk still clears through rows and tag counts first, in the existing ascending tag-id order, and locks finding rows after. The lock order against the import tag path is unchanged.
• Single-finding delete (Finding.delete / finding_delete) is unchanged. The Collector's DELETE now waits for a flush holding FOR KEY SHARE on the finding. If that flush commits a link to it, the delete fails at COMMIT with the same FK error it can hit today, which delete_finding_with_conflict_retry already retries.
• Remaining deadlocks against other writers are still covered by the existing transient-conflict retries in post_process_findings_batch and _bulk_delete_findings_internal.

No migration and no schema change.

Test results

New unittests/test_dedupe_delete_commit_race.py reproduces both interleavings with two real connections and real commits. A TestCase never checks a deferred constraint and TransactionTestCase cannot run in this suite, so the class commits its own rows and removes them afterwards, including the lazily created notification settings row that would otherwise shift later query counts. The interleaving is deterministic: the side that has to go second waits until the other has finished its step or is blocked on a lock (pg_blocking_pids).

• Original deleted between the flush guard and COMMIT: before the fix, Key (duplicate_finding_id)=(11) is not present in table "dojo_finding". After, the link is dropped, the rest of the batch is written, and no links dangle.
• Link written after the chunk resolved its inbound references: before the fix, Key (id)=(6) is still referenced from table "dojo_finding". After, the chunk deletes and the late flush drops its link.
• Controls: a concurrent delete of an unrelated finding and a concurrent link to a surviving original are unaffected.
• Lock-order guard: the flush locks first and the chunk queues without deadlocking. The chunk's retry is disabled in this test so a deadlock fails it.

Local results:

• New module: 2 of 5 failed before the fix, 5/5 pass after.
• Dedup and delete suites (test_dedupe_flush_missing_original, test_bulk_delete_*, test_deduplication_logic, test_duplication_loops, test_finding_helper, test_prepare_duplicates_for_delete, test_importers_deduplication, reimport flush/drain and others): 275 tests OK.
• test_async_delete and test_cascade_delete: 31 OK.
• Full suite in the docker/entrypoint-unit-tests.sh phases: parallel 7836 OK (504 skipped), non-parallel 45 OK, transactional 7 OK (all skipped), performance 11 OK with query counts unchanged.
• The new tests commit real rows, so cleanup was checked on a fresh database: test_importers_performance passes both before and after the new module runs. The only rows left behind are pghistory audit events, and test_auditlog and test_flush_auditlog pass with them present.
• After rebasing on the latest bugfix (which includes Stream uid/hash dedupe candidates to bound reimport memory #16042, touching the same dedup module): the new module plus test_dedupe_candidate_streaming, test_dedupe_flush_missing_original, test_deduplication_logic, test_bulk_delete_duplicate_references, test_duplication_loops, test_prepare_duplicates_for_delete and test_importers_deduplication ran 218 tests, OK (2 skipped). Ruff 0.16.5 passes on the changed files.

Documentation

No documentation change. Deduplication and duplicate-deletion behavior is unchanged apart from these jobs no longer failing.

Checklist

• Make sure to rebase your PR against the very latest dev. (Bug fix: rebased on the latest bugfix.)
• Features/Changes should be submitted against the dev.
• Bugfixes should be submitted against the bugfix 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