perf(tags): centralize tag inheritance + replace signal disconnect with batch context manager (Phase B)#14827
Draft
valentijnscholten wants to merge 2 commits intoDefectDojo:devfrom
Draft
Conversation
valentijnscholten
changed the title
valentijnscholten
force-pushed
the
perf/tag-inheritance-phase-b
branch
from
707cbf9 to
e04bf83
Compare
Maffooch
approved these changes
May 7, 2026
blakeaowens
approved these changes
May 7, 2026
…reate Replaces the per-row `.save()` loop in `propagate_tags_on_product_sync` with bulk SQL through the existing tag-utils helpers. For every child model (Engagement/Test/Finding/Endpoint/Location), reads current inherited_tags in one query, computes the per-child diff against the Product's tags, and applies adds/removes via `bulk_add_tag_mapping` and the new `bulk_remove_tags_from_instances` helper. Both `tags` and `inherited_tags` fields are kept in sync. Also gates the per-child `inherit_tags_on_instance` post_save handler on `created=True`. The previous behavior fired on every save (create OR update), repeatedly re-applying inherited tags to children whose tag state had not changed. Sticky enforcement on user-driven tag edits is unchanged (still handled by `make_inherited_tags_sticky` on m2m_changed). Pinned query-count baselines from PR DefectDojo#14811 drop accordingly: product_tag_add -> 100 findings : 4758 -> 91 (~52x fewer queries) product_tag_remove -> 100 findings : 4540 -> 53 (~85x fewer queries) Sticky and child-creation paths are unchanged in this PR. Phase B targets those (centralized inheritance module + drop the duplicate `inherited_tags` TagField).
…l batch context
Adds dojo/tag_inheritance.py with a thread-local batch() context manager
and is_in_batch() predicate. While the calling thread is inside a batch,
make_inherited_tags_sticky (m2m_changed) early-returns; the calling code
takes responsibility for applying inheritance in bulk.
Replaces the previous pattern in dojo/importers/location_manager.py:556:
disconnected = signals.m2m_changed.disconnect(
make_inherited_tags_sticky, sender=Location.tags.through,
)
try:
...
finally:
if disconnected:
signals.m2m_changed.connect(...)
Signal.disconnect mutates Django's process-global receiver list and is
not thread-safe. While disconnected, every thread/greenlet in the same
process loses sticky enforcement. Safe only under Celery --pool=prefork
and single-threaded gunicorn; broken under --pool=threads|gevent|eventlet,
gunicorn --threads >1, or ASGI threadpools. Also fragile on hard process
exit mid-import (handler stays disconnected for the rest of the process
lifetime).
The new batch context lives in threading.local(): each thread has its
own depth counter, the signal handler stays globally connected, and the
suppression decision is per-thread. No global mutation, no reconnect
hazard.
This is Phase B Stage 1. Subsequent stages will wrap the broader importer
orchestration in batch(), replace the duplicate inherited_tags TagField
with a JSON column, and drop _manage_inherited_tags / per-model
inherit_tags().
Pinned perf-test note: V3 zap_scan_import baseline rises 1243 -> 1263
(~1.6%). The previous process-global disconnect was narrower in scope
(Location.tags.through only); the batch context covers all child-tag
through-tables. Net trade is positive given the threading bug fix; full
Phase B reductions arrive in later stages.
valentijnscholten
force-pushed
the
perf/tag-inheritance-phase-b
branch
from
e04bf83 to
ec02e2e
Compare
Contributor
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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
3 participants
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.
Important
Depends on:
Both must merge first; this branch is stacked on top.
This PR lands the thread-safety fix that could be caused by the current signal disconnect code. It also batches tag updates triggered by a change in tags on a Product. It uses a context manager to disable inheritance during batching, and applies the inherited tags efficiently afterwards in one batch.
Stage 1 (this commit) — thread-local batch context
Adds
dojo/tag_inheritance.pywith:batch()context manager (thread-local depth counter)is_in_batch()predicateWhile inside a batch,
make_inherited_tags_sticky(m2m_changed) early-returns. Calling code is responsible for applying inheritance in bulk.Replaces the previous pattern in
dojo/importers/location_manager.py:556:Signal.disconnectmutates Django's process-global receiver list. While disconnected, every thread/greenlet in the same process loses sticky enforcement. Safe only under Celery--pool=prefork+ single-threaded gunicorn; broken under--pool=threads|gevent|eventlet, gunicorn--threads >1, or ASGI threadpools. Also fragile on hard process exit mid-import (handler stays disconnected for the rest of the process lifetime).The new context lives in
threading.local(): each thread has its own depth counter, the signal handler stays globally connected, and the suppression decision is per-thread. No global mutation, no reconnect hazard.Verification
Stage 1:
unittests.test_tag_inheritance— 36 tests pass (5 skipped for v3 feature flag).unittests.test_tag_utils_bulk— 29 tests pass.unittests.test_tag_inheritance_perf— 20 tests pass.Pinned perf-test note: V3
zap_scan_importbaseline rises 1243 → 1263 (~1.6%). The previous process-global disconnect was narrower in scope (Location.tags.throughonly); the batch context covers all child-tag through-tables. Net trade is positive given the thread-safety fix; full Phase B reductions arrive in Stages 3-5.