[Maffooch]

Description

Migration 0305_archive_batch_indexes (new in 3.5.0) builds 11 partial indexes with AddIndexConcurrently in a non-atomic migration. If the process dies partway through, which we observed during a production rollout when the migration container was OOM-killed, the interrupted CREATE INDEX CONCURRENTLY leaves its index in the catalog marked INVALID. Because the migration was never recorded as applied, the next attempt runs it again and fails on the first index that already exists:

django.db.utils.ProgrammingError: relation "dojo_findvulnref_archive_batch" already exists

That repeats on every retry, so the upgrade cannot finish without someone cleaning up by hand.

This PR edits 0305 in place. Each operation is now an AddIndexConcurrently subclass defined in the migration file that checks pg_index for an index of the same name before building:

• a valid index is kept as is (an earlier run built it before a later operation failed),
• an invalid leftover is dropped with DROP INDEX CONCURRENTLY IF EXISTS, then rebuilt,
• a missing index is built as before.

Killing the migration process does not stop its build on the server, which runs on to completion. So before deciding, the operation checks pg_stat_progress_create_index for a build of the same index from another backend and waits for it to finish (polling every 5 seconds, with one log line when the wait starts and one when it ends). Without that wait, the retry would see the in-flight index as INVALID, block inside DROP INDEX CONCURRENTLY until the old build finished, then drop the finished index and build it again, scanning a large table such as dojo_finding twice.

Migration state is inherited from AddIndexConcurrently, so makemigrations --check is unaffected. The reverse path was already idempotent (DROP INDEX CONCURRENTLY IF EXISTS). Editing in place is safe: an install that recorded 0305 as applied never runs it again, and one stuck on it has not recorded it.

One side effect: sqlmigrate dojo 0305 now reads the catalog, so against a database that already has all 11 valid indexes it prints (no-op) for each operation.

Test results

New tests in unittests/test_migration_0305_archive_batch_indexes.py run the 0305 operation for dojo_test against the real catalog. They are SimpleTestCase tests on autocommit connections, because CREATE INDEX CONCURRENTLY cannot run inside a TestCase transaction, and they restore the index afterwards. They cover four states:

• an INVALID leftover (created without superuser rights by a failed CREATE UNIQUE INDEX CONCURRENTLY over duplicate rows): dropped and rebuilt on dojo_test,
• a valid index: kept, same OID,
• no index: built,
• a build still in progress (held open by another transaction's snapshot, released while the operation polls): waited for, and its index is kept with the same OID.

All 4 pass with PYTHONWARNINGS=error. Against the original 0305 they fail with relation "dojo_test_archive_batch" already exists.

Manual checks on PostgreSQL 16, starting from a fresh database migrated to 0304:

• Fresh run: all 11 indexes created and valid.
• Reproduced the bug with the original file: applied 0305, deleted its django_migrations row, marked two indexes INVALID (one with indisready = false as well) and dropped two. Re-running the original migration failed with the error above.
• Ran the fixed migration on that same database: it applied cleanly. The 7 valid indexes kept their OIDs (not rebuilt), the 2 invalid ones were rebuilt, and the 2 missing ones were created. pg_get_indexdef output for all 11 matches a fresh run.
• Re-ran it with every index already valid and the record deleted: it applied cleanly and rebuilt nothing.
• Reverse to 0304 drops all 11, forward again recreates them.
• makemigrations dojo --check --dry-run: no changes detected.
• ruff check --config ruff.toml on both files: clean.

Not tested: a production-sized table, PostgreSQL versions other than 16, and two migration processes started at the same moment.

Documentation

None needed.

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]

Adversarial review: full tier, Opus

Tier: full (a migration change). Reviewed head d6603eef26. Backend-only, so there is no screen to drive, no harness and no screenshot pair.

CI: every check on the head commit is green. That includes ruff, Migration Graph Check, both Rest Framework Unit Tests jobs (8499 tests, OK (skipped=1304); the log shows Applying dojo.0305_archive_batch_indexes... OK on the changed file), the UI test shards, k8s and performance.

Regression test: I ran it locally on PostgreSQL 16 in a throwaway defectdojo-django container with PYTHONWARNINGS=error.

• PR tree: Ran 4 tests ... OK.
• 0305_archive_batch_indexes.py taken back to origin/dev: FAILED (errors=6). The failures are relation "dojo_test_archive_batch" already exists (5) and 'AddIndexConcurrently' object has no attribute '_build_in_progress' (1).
• Restored the file to HEAD afterwards.

The tests exercise the real catalog states rather than mocks.

Break attempts

1. Real killed client, mixed state, one migrate run. On a clone of a fully migrated database I deleted the 0305 django_migrations row. Then I set up three broken indexes:

   • dojo_findvulnref_archive_batch: left INVALID by a failed unique build.
   • dojo_endpstatus_archive_batch: dropped.
   • dojo_finding_archive_batch: a CREATE INDEX CONCURRENTLY started from a psql client, held in waiting for old snapshots by a REPEATABLE READ session. I then kill -9'd the client. The backend kept running, which confirms the PR's premise.

   Then migrate dojo 0305 applied cleanly:

   • The in-flight index finished and was kept, same OID (20964).
   • The INVALID leftover was rebuilt on the right table.
   • The missing index was built.
   • The other 8 kept their original OIDs.
   • All 11 are valid and the migration is recorded.

   Pass.

2. Two migrate dojo 0305 processes started at once on a database with all 11 indexes dropped and the record deleted. Both printed OK and all 11 indexes ended up valid. Django recorded 0305 twice in django_migrations. That comes from Django's migrate having no lock and is outside this PR; the row is harmless. With the base file, one of the two would have failed. Pass.

3. sqlmigrate: I ran it with 10 valid indexes and dojo_test_archive_batch missing. It prints (no-op) for the 10 and the real CREATE INDEX CONCURRENTLY for the missing one, as the description says. --backwards still prints DROP INDEX CONCURRENTLY IF EXISTS. Pass.

4. State and reversibility: makemigrations dojo --check --dry-run reports No changes detected. migrate dojo 0304 then migrate dojo 0305 drops and recreates all 11, and all are valid afterwards. Pass.

5. The tests themselves: read for false coverage.

   • Each test asserts OID and validity from pg_index, not a mock.
   • The in-progress test really holds a build open with another snapshot and releases it while the operation polls.
   • Cleanup restores the index even when a test fails, so a --keepdb database is not left broken.
   • CI runs this file under --parallel. Each worker has its own cloned database, and a concurrent index build only waits on snapshots in its own database, so workers do not block each other.

   Pass.

Static only / not checked

• Production-sized tables: not run. On dojo_finding at millions of rows the wait loop is what saves a second full-table scan. I could only check that logic on small tables.
• A retry under a different database role: not run. pg_stat_progress_create_index hides index_relid from another role unless that role has pg_read_all_stats. A retry under a different role than the killed build would not see the in-flight build. It would fall back to DROP INDEX CONCURRENTLY, which waits for the build to finish, and then rebuild: correct but slower. Deployments run migrations under the same role, so I am not filing this.
• Performance: nothing here runs on the request path. The polling query reads a small system view every 5 seconds.

Findings

• R1 nit this-pr dojo/db_migrations/0305_archive_batch_indexes.py:38: the loop logs Waiting for an earlier build of index ... to finish on every 5-second poll. A wait on a large dojo_finding build can take tens of minutes and fill the migration log with hundreds of identical lines. Log once when the wait starts, and optionally once when it ends with the elapsed time.

Recommendation

changes required: 1 this-pr nit open (R1). Apart from R1, the fix behaves as described in every state I could construct, including a real orphaned server-side build.

[Maffooch]

• R1 fixed in e3ea307: the wait logs one warning when it starts and one when it ends, however many polls it takes. The in-flight test now asserts exactly those two records; against the previous commit it fails with 2 != 10.