fix(importers): stop reimport recreating a test deleted mid-scan

[Maffooch]

Description

POST /api/v2/reimport-scan/ fails with a 500 and a foreign key violation on dojo_test.engagement_id when the object it is importing into is deleted while the scan is being processed. Reported repeatedly from production instances.

The importers resolve the test up front and hold it — and through it its engagement — in memory for the whole run, then write the changes collected on it back at the end (update_timestamps, update_test_meta, update_test_tags, then self.test.save() / self.test.engagement.save(), and finally update_test_progress()).

Model.save() on an instance whose row has been deleted in the meantime does not fail. The pk is set, so Django issues an UPDATE; it matches no row, and Model._save_table() falls through to an INSERT. The row that was just deleted is recreated. Deleting a product, engagement or test in the UI while one of its scans is being processed is enough to get there, and the recreated row then goes one of two ways:

• the same delete removed the parent rows too, so the INSERT violates a foreign key — this is the reported failure, insert or update on table "dojo_test" violates foreign key constraint "dojo_test_engagement_id_..._fk_dojo_engagement_id" with the engagement id absent from dojo_engagement;
• only the test itself was deleted, so the INSERT succeeds and silently resurrects a test whose findings are gone.

The second outcome is the quieter half of the same defect and is why suppressing the error would not be enough: the write must not happen at all.

The change

BaseImporter.verify_test_still_exists() confirms the test row is still present and otherwise raises Test.DoesNotExist naming the deleted test. It is called from the two points that write the test back: the new save_test_and_engagement() (used by both DefaultImporter.process_scan and DefaultReImporter.process_scan in place of the inline save pair) and update_test_progress(), which is the only write-back on the post-processing path.

The engagement needs no check of its own: the test row could not still be there if the engagement it points at had been deleted, so verifying the test covers both saves.

An import that loses its test still fails — there is nothing left to report on and no honest success to return — but it now fails with an error that says what happened, and without writing a resurrected row first.

Two approaches were rejected on the way:

• save(force_update=True), which turns the INSERT fallback into a DatabaseError at no query cost, marks the surrounding transaction for rollback (Model.save_base wraps the save in transaction.mark_for_rollback_on_error). That leaves the caller unable to run any further query, which would break failure-path cleanup that has to run after the error.
• QuerySet.update(), which returns a row count instead of falling through to an INSERT, bypasses save() and with it the change tracking on Test.

Test results

New test module unittests/test_importers_deleted_object.py, class TestImportObjectDeletedDuringScan (5 tests):

• update_test_progress() refuses to recreate the test, parameterized over the test row being deleted directly and over the engagement being deleted (which cascades to it);
• save_test_and_engagement() refuses to recreate the test, and neither the test nor the engagement row comes back — the reported scenario;
• two control cases: both write-back points still persist their pending changes when the rows are there.

On unmodified bugfix the parameterized test fails with AssertionError: DoesNotExist not raised and the deleted row present again afterwards, and the engagement variant additionally surfaces the reported constraint by name at the end of the transaction:

django.db.utils.IntegrityError: update or delete on table "dojo_engagement" violates
foreign key constraint "dojo_test_engagement_id_46ff752d_fk_dojo_engagement_id" on table "dojo_test"

Verified against PostgreSQL 16 on Python 3.13:

• unittests.test_importers_deleted_object — 5 tests, OK (2 failures + 3 errors before the change)
• unittests.test_import_reimport, unittests.test_importers_importer, unittests.test_importers_closeold, unittests.test_reimport_prefetch, unittests.test_importers_deleted_object — 192 tests, OK (1 skipped)
• unittests.test_importers_performance — 11 tests, OK
• ruff check . clean

Query counts

The existence check is one primary key lookup at each of the two write-back points, so every import and reimport costs two extra queries against roughly 100–200. All 34 pinned expectations in unittests/test_importers_performance.py moved by exactly +2; no other count changed, which also confirms the two checks are the whole cost. Paying two indexed lookups per scan to stop resurrecting deleted objects is the trade this makes deliberately.

Documentation

No documentation change: the 500 was never documented behaviour, and this adds no setting or API surface.

Downstream / Pro consideration

• Pro overrides neither update_test_progress nor the reimporter's write-back, so the fix reaches the Pro import paths with no Pro-side edit, and save_test_and_engagement() is new API on BaseImporter that nothing overrides.
• The raised Test.DoesNotExist is a plain Python raise before any write, so it leaves the transaction usable. Pro's importer failure handler runs several queries of its own after catching an import error (recording the failed status on its companion row, moving the report, writing a usage record); those still work. This is the property that ruled out force_update above.
• Pro's smart-upload importer keeps its own inline self.test.save() / self.test.engagement.save() pair and is unaffected by this change; switching it to the inherited save_test_and_engagement() would extend the same protection there and is worth a follow-up in that repo.
• The finding-level form of this defect — Finding.save() recreating a finding deleted mid-reimport — is a separate call site. Its close-old-candidate case was fixed in fix(reimport): skip close-old candidates whose finding row was deleted mid-reimport #15408; a report of it during finding creation is not addressed here.

Checklist

• Bugfixes should be submitted against the bugfix branch — based on and targeting bugfix.
• 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 — tests were run on 3.13.
• Add applicable tests to the unit tests.
• Model changes must include the necessary migrations — N/A, no model changes.

Reported internally

• https://defectdojo.slack.com/archives/C0BK62Q76P8/p1785364767408819
• https://defectdojo.slack.com/archives/C0BK62Q76P8/p1785364233740989
• https://defectdojo.slack.com/archives/C0BK62Q76P8/p1785361152523619

---

Generated by Claude Code

[Maffooch]

Merging this needs a paired Pro change — flagging rather than guessing at it.

The two existence checks add two queries to every import and reimport, and Pro pins the same counts independently in dojo-pro/unit_tests/test_importers_performance.py. Those expectations are written against the open-source importer flow, so they will go stale the moment this merges — Pro's suite will fail with N != N-2 in the same shape seen here.

Affected expectations in that file (current values):

test	queries1	queries2	queries3	queries4
test_import_reimport_reimport_performance_pghistory_async	161	143	43	105
test_import_reimport_reimport_performance_pghistory_no_async	181	153	53	105
..._no_async_with_product_grading	191	163	60	114
test_deduplication_performance_pghistory_async	96	95	—	—
test_deduplication_performance_pghistory_no_async	(see file)

The delta should be +2 on every step that runs a full process_scan, which is what happened here — all 34 open-source expectations moved by exactly +2, none by any other amount. I have deliberately not opened a Pro PR with those numbers pre-bumped: I could not run Pro's suite in this environment, and pinned counts belong to a run that actually produced them rather than to arithmetic. Someone with the Pro stack should run unit_tests/test_importers_performance.py against this branch and commit the observed values.

If that Pro PR is raised before this one merges, it needs oss-branch: claude/admiring-ramanujan-pmg6hq in its test configuration so Pro CI validates against this branch, then flipped back to bugfix once this is in.

Nothing else on the Pro side needs a change: Pro overrides neither update_test_progress nor the reimporter write-back, and the new save_test_and_engagement() is inherited unmodified.

---

Generated by Claude Code

[Maffooch]

CI was red on the first commit: 6 failures out of 4212, all of them mine, all the same kind of thing I'd already flagged — and one I should have caught before pushing.

unittests/test_tag_inheritance_perf.py pins the ZAP import and reimport query counts independently of test_importers_performance.py. I checked the latter and updated its 34 expectations, but never looked for a second file pinning the same flow, so those six baselines still carried the pre-change numbers:

AssertionError: 289 != 287   test_baseline_zap_scan_import_v2
AssertionError: 313 != 311   test_baseline_zap_scan_import_v3
AssertionError:  76 != 74    test_baseline_zap_scan_reimport_no_change_v2
AssertionError:  88 != 86    test_baseline_zap_scan_reimport_no_change_v3
AssertionError: 150 != 148   test_baseline_zap_scan_reimport_with_new_findings_v2
AssertionError: 179 != 177   test_baseline_zap_scan_reimport_with_new_findings_v3

Every one is +2, the same two existence checks, which is the corroboration I'd want: two independently-written baselines agree on the cost. Pushed with all six moved. The tag add/remove, finding-creation, endpoint and location baselines in the same class don't run an import and are untouched.

Rather than fix just the reported six, I went back and ran every file in unittests/ that calls assertNumQueries — test_importers_performance, test_tag_inheritance_perf, test_jira_finding_group_perf, test_false_positive_history_logic, test_system_settings, test_metrics_queries — 119 tests, OK (2 skipped). No other pinned count is affected. The full suite is re-running locally as an independent check alongside CI.

No change to the fix itself; this commit is test expectations only.

---

Generated by Claude Code

[Maffooch]

CI is green on 1ad99e45db — the Unit tests workflow (ruff, rest-framework unit tests, integration, k8s, performance, image builds) concluded success, and every check run on the head commit passed.

Closing out the independent check I said was running: the full unittests suite locally, 4230 tests, 1 failure — test_jira_config_product.JIRAConfigProductTest.test_add_jira_instance_unknown_host. That one is environmental, not from this change. It posts a deliberately unresolvable host and expects the resulting connection error to surface as a validation error; the sandbox I ran in routes outbound HTTPS through a proxy that answers instead of failing, so the condition can't be reproduced there. It passed in CI on both commits, including the first one that already carried the importer change, and nothing in this diff touches JIRA configuration.

So: the 6 failures CI caught were real and mine, they're fixed, and the only remaining local failure is an artifact of where I ran it.

---

Generated by Claude Code

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[Maffooch]

Closing this as superseded — not resolving the conflict, because resolving it would only push a duplicate.

#15427 landed on bugfix (commit 7cb7a55) and fixes the same defect by the same route: check the row before the write-back rather than let Model.save() fall through to an INSERT. It reaches the identical conclusion on the implementation choice too, including rejecting save(force_update=True) because the forced update raises inside the un-savepointed atomic block Model.save_base opens and marks the whole transaction for rollback. Every file this PR touches is a file that one already changed, which is where the conflict comes from.

It is also the better version of the fix, in three ways I'd rather have than my own:

• it raises ValidationError, so the caller gets a 400 with an actionable message naming what was deleted, where I raised Test.DoesNotExist and left the response a 500;
• it guards the engagement write independently instead of inferring the engagement's existence from the test's, which is more robust than my argument that an enforced foreign key makes the second check redundant;
• it queries through _base_manager, so the check can't be skewed by a default manager — something I didn't consider.

Its unittests/test_importers_deleted_target.py is a strict superset of my test_importers_deleted_object.py: 6 tests to my 5, covering the test deleted mid-run, the engagement cascade reporting the reason rather than a constraint name, the import path, the closing progress write, the engagement half, and a control case. There is no coverage here that it lacks, so contributing my module would add a second file asserting the same behaviour.

Both perf modules are already updated there at +3 — one lookup per guarded save, against my +2 for two guarded call sites. Their number is the right one for their implementation; the +2 here would be wrong on top of it.

For the record, since these came out of a production error sweep: the three reports that motivated this are the same ones behind #15427, and I've pointed the internal threads at that PR instead of this one so the merged fix is what gets found next time. The one report from that sweep that is not covered by either PR is the finding-level form of this defect — Finding.save() re-creating a finding after its parent test is deleted, seen as a foreign key violation on dojo_finding.test_id. I could not pin the call site to that one (the traceback's middle was elided and deferred foreign key checking moves the error to COMMIT, so the visible frames aren't evidence), so it is still open and written up internally rather than guessed at here.

Nothing about the fix is lost by closing this.

---

Generated by Claude Code