fix(tags): order add-path tag-count updates so concurrent imports cannot deadlock - #15652
Open
Maffooch wants to merge 1 commit into
Open
fix(tags): order add-path tag-count updates so concurrent imports cannot deadlock#15652Maffooch wants to merge 1 commit into
Maffooch wants to merge 1 commit into
Conversation
…not deadlock
bulk_add_tags_to_instances issued its per-tag count UPDATE -- which locks that
tag's row in the tagulous tag table (dojo_tagulous_<model>_tags) and, inside an
import, holds it until the surrounding transaction commits -- in the order the
tags were supplied by the caller. Two imports running concurrently and applying
an overlapping set of tags could take those tag-row locks in opposite orders,
so Postgres aborted one with a deadlock (SQLSTATE 40P01) surfaced to the client
as a failed import-scan:
deadlock detected ... while updating tuple in relation
"dojo_tagulous_finding_tags"
This is the add-path twin of the bulk_remove_all_tags deadlock fixed in #15486.
The fix mirrors it: resolve every tag up front, then apply the tags -- and so
issue the count UPDATEs that take the row locks -- in ascending tag-id order.
A shared, deterministic lock sequence for every caller means the lock cycle can
no longer form. The batch body (existing-relationship lookup, through-row
bulk_create, count increment, prefetch-cache invalidation) is unchanged, so the
return value and resulting tag counts are identical.
Test: bulk_add_tags_to_instances is given three pre-created tags in an order
unrelated to their ids; the count UPDATEs must be issued in ascending tag-id
order. The test fails on the old supplied-order code and passes once the updates
are ordered.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014RiNAYZVJ33ME55F2JWPQX
Maffooch
force-pushed
the
claude/beautiful-archimedes-9txnhq
branch
from
August 13, 2026 03:40
a071cec to
043feaf
Compare
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A concurrent
POST /api/v2/import-scan/(import tags enabled) failed with a Postgres deadlock surfaced to the client:dojo_tagulous_finding_tagsis the tagulous tag table (it holdsslug,count,protected), so the statement that deadlocked is the per-tagcount = count + NUPDATE, which takes a row lock on the tag being applied.bulk_add_tags_to_instancesissued that count UPDATE once per tag, in the order the tags were supplied by the caller. During an import these UPDATEs run inside the import's transaction, so each tag-row lock is held until the import commits. Two imports running concurrently and applying an overlapping set of tags could therefore take the same tag-row locks in opposite orders, forming a lock cycle that Postgres breaks by aborting one side (SQLSTATE40P01). Because both imports pass the same tag set, this shows up under normal CI-driven, parallel imports to the same product/engagement.This is the add-path twin of the
bulk_remove_all_tagsdeadlock fixed in #15486, which made the remove-path count decrements run in a deterministic tag-id order for exactly the same reason. The add path was never given the same ordering.Fix
bulk_add_tags_to_instancesnow resolves every tag up front and then applies them — and so issues the count UPDATEs that take the row locks — in ascending tag-id order. A shared, deterministic lock sequence for every caller means the cycle can no longer form. The per-batch body (existing-relationship lookup, through-rowbulk_create, count increment, prefetch-cache invalidation) is unchanged, so return value and resulting tag counts are identical; only the order in which tags are processed changes.Scope note:
bulk_add_tag_mapping(the parser-tag path) applies its count deltas in a singleUPDATE … CASE WHENstatement rather than one UPDATE per tag, so it does not exhibit the multi-statement, caller-order lock acquisition that produces this cycle; it is intentionally left unchanged to keep this fix focused on the traced path.Pro impact
DefectDojo Pro does not override
bulk_add_tags_to_instances; it only calls it (importer batch tagging, rules, connectors). The ordering fix therefore applies to Pro's import/rules/connector tag paths transparently, with no Pro-side change required.Test results
Added
BulkAddTagsToInstancesLockOrderTestinunittests/test_tag_utils_bulk.py, mirroring the existingBulkRemoveAllTagsLockOrderTestadded by #15486. Three tags are created in an order deliberately unrelated to their ids, then supplied tobulk_add_tags_to_instancesin a non-ascending order; the test captures the tag rows locked by the count UPDATEs and asserts they are issued in ascending tag-id order. It fails on the old supplied-order code and passes once the updates are ordered.Note: the full suite requires a Postgres-backed stack that could not be stood up in the ephemeral environment used to author this change; the new test is deterministic (it asserts lock/update ordering, not a live race) and validation is being carried through CI.
Documentation
No user-facing behavior changes; no documentation update required.
Checklist
bugfixbranch.Generated by Claude Code