Skip to content

perf(tags): centralize tag inheritance + replace signal disconnect with batch context manager (Phase B)#14827

Merged
mtesauro merged 4 commits intoDefectDojo:devfrom
valentijnscholten:perf/tag-inheritance-phase-b
May 8, 2026
Merged

perf(tags): centralize tag inheritance + replace signal disconnect with batch context manager (Phase B)#14827
mtesauro merged 4 commits intoDefectDojo:devfrom
valentijnscholten:perf/tag-inheritance-phase-b

Conversation

@valentijnscholten
Copy link
Copy Markdown
Member

@valentijnscholten valentijnscholten commented May 6, 2026

This PR lands the thread-safety fix. Instead of diconnectings signas we introduce a context manager that disables tag inheritance.

Stage 1 (this commit) — thread-local batch context

Adds dojo/tag_inheritance.py with:

  • batch_mode() context manager (thread-local depth counter)
  • is_in_batch_mode() predicate

While 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:

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. 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.

@valentijnscholten valentijnscholten changed the title perf(tags): centralize tag inheritance + replace signal disconnect with thread-local batch (Phase B) perf(tags): centralize tag inheritance + replace signal disconnect with batch context manager (Phase B) May 6, 2026
@valentijnscholten valentijnscholten force-pushed the perf/tag-inheritance-phase-b branch from 707cbf9 to e04bf83 Compare May 6, 2026 22:06
@Maffooch Maffooch requested review from Jino-T, blakeaowens and dogboat May 7, 2026 03:58
@Maffooch Maffooch added this to the 2.59.0 milestone 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.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

Conflicts have been resolved. A maintainer will review the pull request shortly.

@valentijnscholten valentijnscholten marked this pull request as ready for review May 8, 2026 15:12
Copy link
Copy Markdown
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

Copy link
Copy Markdown
Contributor

@dogboat dogboat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing that!

@mtesauro mtesauro merged commit 7ac0d9d into DefectDojo:dev May 8, 2026
158 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants