Skip to content

perf(notifications): batch the daily SLA run's notifications through one manager - #16324

Merged
Maffooch merged 6 commits into
devfrom
perf/sla-notify-batched-fanout
Oct 15, 2026
Merged

Maffooch merged 6 commits into
devfrom
perf/sla-notify-batched-fanout

Conversation

@Maffooch

@Maffooch Maffooch commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Description

Stacked on #16315, and both target dev. This branch contains #16315's commit (b1ea96d3e9) followed by the batching commits, so the diff against dev shows both. To review only the batching, compare b1ea96d3e9...8681953546, or step through the last four commits.

Merge order: #16315 first, then this PR, through the merge queue (they can share a queue group). Once #16315 is squash-merged, this branch already holds the same changes, so this PR's squash carries only the batching. Please don't queue this PR ahead of #16315: if it went first, its squash would include all of #16315 and leave #16315 empty. Auto-merge on this PR stays off until #16315 is in the queue.

After #16315 the daily SLA run scans 3M findings in about 35 s, but it then spends about 41 minutes sending notifications, at roughly 7 queries per notified finding. Most of that is repeated work. The module-level create_notification() builds a new NotificationManager for every call, so each notified finding:

  1. re-reads the system Notifications row in the new manager's __init__,
  2. re-resolves the same product's recipients (_get_user_to_send_notifications_to()), and
  3. writes its alerts in an INSERT of its own.

The fan-out batching in NotificationManager (AlertBuffer, the render cache, the shared mail session) only covers one notification at a time, so none of it carries over between findings.

This adds notification_batch(). Every create_notification() call made inside it goes through one manager. The SLA run opens one batch for all of its notifications. No other caller changes.

Inside a batch:

  • The system notifications row is read once, when the manager is built.
  • Each product's recipients are resolved once (_recipients_for_current_scope), and only the current product's are kept. A subclass that overrides _get_user_to_send_notifications_to is still the one that resolves them.
  • Alerts share bulk INSERTs of ALERT_BULK_CREATE_BATCH_SIZE.
  • Each notification still renders its own messages. The render cache is reset per notification, so one finding's rendered text is never reused for another.

Robustness, carried over from #16315:

  • Alerts are written only between notifications (flush_alerts()) and when the batch ends, never inside a notification's savepoint. A notification that fails and rolls back cannot erase alerts already written for other findings.
  • In a batch, an alert INSERT that fails is retried row by row. One bad row costs only its own alert, and one notification whose every row is bad costs only that notification's alerts, however many recipients it has.
  • An outage stays bounded for the whole run, not per flush. The batch stops attempting alert writes after MAX_CONSECUTIVE_ALERT_FAILURES (5) consecutive single-row failures that are either connection-level (OperationalError, InterfaceError) or span more than one notification. So an outage costs the same few attempts however many recipients a notification has, while one notification with bad data still costs only its own alerts. One traceback is logged per flush.
  • Everything not written is reported in one fallback alert when the batch ends, not one per row or per flush. Writing that report is guarded, so if it fails too it is logged and never escapes the run. If the database recovers mid-run, the alerts after that point are still not written; the fallback alert says how many.
  • A notification that fails partway keeps the alerts it had already built for earlier recipients. Before, its savepoint rolled them back. Delivery is now partial instead of all or nothing for that notification.
  • A NOTIFICATION_MANAGER without batch() (any custom manager written before this change), or one that cannot be built, makes notification_batch() fall back to sending each notification on its own, as before.
  • A FINDING_SLA_NOTIFY_QUERYSET_METHOD that raises or is misnamed is logged, and the run reads its findings without it. A hooked query that cannot be read on its first chunk (a bad select_related or prefetch_related, or something that is not a query) is read again without the hook. A hook whose query is valid but matches nothing is respected. A failure on a later chunk goes to fix(notifications): keep the daily SLA run going when one finding fails #16315's read guard, as for the plain query.
  • Outside a batch, _flush_alerts behaves exactly as before. That includes the existing database-down case covered by test_a_failing_alert_batch_falls_back_to_one_failure_alert_per_superuser, which passes unchanged.

It also adds FINDING_SLA_NOTIFY_QUERYSET_METHOD, the companion to the existing FINDING_SLA_PERIOD_METHOD. When set, it receives the run's findings queryset and returns it with whatever the period method reads, so that data arrives with each chunk. DefectDojo Pro's period method reads each finding's companion row for risk-based and KEV-capped SLA configurations. That cost one query per notified finding, and it can now be loaded with the chunk. When the setting is unset (the open-source default), the run is unchanged.

Two changes outside the batch code that the batch depends on:

  • Notifications.merge_notifications_list now merges into a copy of its first entry instead of returning that entry. Its callers set .user on the result, and for a superuser with no notification settings the first entry is the manager's system notifications row. With one manager per run, that redirected every later notification's system-level copy (the user-less alert, mail_notifications_to, the system Slack channel) to that superuser. The same applies on the named-recipients path, which merges [system_notifications, recipient] and sets .user.
  • A Slack user id resolved during the run is kept on the user object the batch reuses, so it is looked up once per run rather than once per notification. _send_single_notification_to_user also copies user.applicable_notifications before appending, so the cached list does not grow.

Test results

Twenty-one new tests in unittests/test_notifications_sla.py: fourteen in TestSlaNotificationBatching and seven in TestSlaNotifyQuerysetHook. They use the real create_notification, with nothing mocked unless a failure is being forced:

  • Query count is the same for 1 and 5 notified findings. Before this change it was 15 and 31.
  • Every finding gets its own alert for every recipient.
  • Recipients are still resolved per product: a reader of one product gets only that product's alerts.
  • One bad alert row does not lose the rest of its INSERT.
  • A notification failing after its alert was built does not roll back alerts written for the others.
  • A database outage costs a bounded number of writes: one batch INSERT, 5 single-row retries and one fallback.
  • With small flushes, an outage costs the same number of write attempts for 6 findings as for 12, and is reported once.
  • A notification whose every row fails, with 9 recipients, does not cost the other notifications their alerts.
  • The system-level notification reaches the system destination for every notification, with superusers who have no notification settings of their own.
  • A NOTIFICATION_MANAGER with only create_notification still sends every notification.
  • A Slack user id is looked up once per run.
  • The run reads its findings through FINDING_SLA_NOTIFY_QUERYSET_METHOD when it is set, and is unchanged without it.
  • A hook that raises, a misnamed hook, a hook whose query cannot compile, and a hook whose prefetch fails each fall back to the plain query. A hook that matches nothing is respected.
  • An outage costs the same number of write attempts (one batch INSERT, 5 retries, one report) whether a notification has 6 recipients or 18.
  • A failure writing the end-of-batch report does not escape the run.
  • Only the current product's recipients stay cached.

Every test added for a review finding failed before its fix and passes now: seven from the first review (failing at baff961354) and five from the second (failing at 3133d0c99e).

python3 manage.py test unittests.test_notifications_sla unittests.test_notifications unittests.test_notifications_fanout unittests.test_apiv2_notifications: 118 tests, OK (24 skipped). Almost all of the skips are two classes in test_notifications.py, TestNotificationTriggers and TestNotificationWebhooks, that dev already skips pending a re-baseline; this PR does not touch them. An earlier version of this description wrongly put the skips down to the v2/v3 location gates.

ruff check (0.16.10, per requirements-lint.txt) is clean.

I also ran DefectDojo Pro's notification, SLA-configuration, sandbox (SilentNotificationManager), i18n, rules-engine and email tests with this helper.py and models.py (Pro subclasses NotificationManager): 1,354 passed, 1 skipped.

Same 3M benchmark as on #16315: the real async_sla_compute_and_notify_task, 3,010,875 open non-Info findings, 45,356 inside the notify window, a 3 GiB memory cap and real notifications.

#16315 this PR
Queries 368,664 5,533
Run time 2,522 s 601 s
Scan and evaluation ~35 s ~37 s
Peak memory 719 MB 750 MB
sla_breach / sla_breach_combined notifications 45,356 / 7,263 45,356 / 7,263
Alert rows written 105,238 105,238

After both rounds of review fixes, I re-ran it on 2026-10-14, when 45,401 findings fell inside the window: 5,528 queries and 738 MB peak, with 45,401 sla_breach and 7,262 combined notifications. Alerts were 105,326 rows, 52,663 of them system-level (exactly one per notification). Run times vary with load on the machine, so compare the query counts and memory, not the seconds.

Mail, Slack, Teams and webhook delivery were off in this benchmark. A batch shares one SMTP session across the run, but I have not measured delivery channels.

Review

A first independent review pass requested changes for six findings, all fixed in 3133d0c99e:

  1. The system row redirected to a superuser.
  2. One notification's rows tripping the outage bound.
  3. A manager without batch() aborting the run.
  4. The repeated Slack lookup.
  5. Hook failure modes.
  6. The outage bound being per flush.

The pass also confirmed that the per-product recipient cache key, the render-cache reset, the ContextVar cleanup and mail session rotation hold.

A second independent pass confirmed those six fixes (the first one by restoring the old merge, which made its test fail). It approved with nits and raised five more, all fixed in 8681953546:

  1. An outage costing one attempt per recipient of the first notification.
  2. The end-of-batch report able to raise out of the run.
  3. A failing prefetch_related in the hook reading nothing.
  4. A valid but empty hook query being ignored.
  5. The recipient cache keeping every product.

Documentation

No user-facing change, so no docs update.

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 and others added 2 commits October 13, 2026 10:23
sla_compute_and_notify() stopped at the first finding that raised, and
notifications are only created after the loop, so one bad finding meant no
SLA notification went out at all. It also loaded every open finding and
fetched its test, engagement and product one query at a time.

- Evaluate each finding in its own try and savepoint; guard each
  notification, JIRA comment and combined notification separately; still
  send what was collected if reading further findings fails.
- Only fetch findings whose SLA ends inside the notify window, stream them
  in chunks, and prefetch their related rows per chunk.
- Send each JIRA SLA comment through that finding's own JIRA instance, and
  stop a product with no JIRA project from raising UnboundLocalError.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…one manager

The SLA run sent each notification through a new NotificationManager, so
every notified finding re-read the system notifications row, re-resolved
its product's recipients and wrote its alerts in an INSERT of its own:
about 7 queries per finding, 368k for 45k findings.

notification_batch() sends every create_notification() made inside it
through one manager. The SLA run opens one batch: the system row is read
once, each product's recipients once, and alerts share bulk INSERTs, while
each notification still renders its own messages.

Alerts are written only between notifications, outside their savepoints,
so one notification's rollback cannot erase another's. In a batch a failed
INSERT is retried row by row, and after 5 consecutive failures the rest is
reported in one fallback alert. Outside a batch nothing changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Maffooch Maffooch added this to the 3.4.200 milestone Oct 13, 2026
@Maffooch Maffooch added the performance performance improvement or bug report label Oct 13, 2026
Maffooch and others added 3 commits October 13, 2026 17:24
…h the run

A FINDING_SLA_PERIOD_METHOD can read more of each finding than the SLA run
loads. DefectDojo Pro's reads each finding's companion row for risk-based
and KEV-capped configurations, which cost one query per notified finding.

FINDING_SLA_NOTIFY_QUERYSET_METHOD, when set, receives the run's findings
queryset and returns it with what the period method needs, so that arrives
with each chunk. Unset, the run is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes from a review of the batched SLA run:

- merge_notifications_list returned its first entry itself, and callers set
  .user on the result. For a superuser with no settings that entry is the
  manager's system notifications row, so in a batch every later
  notification's system-level copy went to that superuser. It now merges
  into a copy.
- A notification whose every alert row fails (5 or more recipients) tripped
  the outage bound and cost the rest of the batch. The bound now counts a
  streak only when rows of more than one notification fail.
- The bound held per flush, not per run. Failure state now lives on the
  batch: once tripped, later flushes are not attempted, and everything not
  written is reported in one fallback alert when the batch ends.
- notification_batch() falls back to unbatched sending when the configured
  NOTIFICATION_MANAGER has no batch() or cannot be built.
- A Slack user id resolved during the run is kept on the cached user, so it
  is looked up once rather than once per notification.
- A FINDING_SLA_NOTIFY_QUERYSET_METHOD that raises, is misnamed, or yields
  a query that cannot compile is logged and the run reads its findings
  without it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gh a broken hook

Second-round review fixes for the batched SLA run:

- An outage is detected after 5 consecutive connection-level failures
  (OperationalError, InterfaceError) whatever notification they belong to,
  so it no longer costs one attempt per recipient of the first failing
  notification. Data errors keep the per-notification rule. One traceback
  is logged per flush.
- The end-of-batch report of unwritten alerts is guarded, so a failure to
  write it cannot escape the run or hide an earlier exception.
- A FINDING_SLA_NOTIFY_QUERYSET_METHOD whose query cannot be read on the
  first chunk (a bad prefetch_related, a non-query) falls back to the plain
  query, instead of reading nothing. The compile-time check is gone, so a
  hook whose query is valid but empty is respected rather than ignored.
- Only the current product's recipients are cached for the batch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Maffooch
Maffooch changed the base branch from fix/sla-notify-resilient-loop to dev October 14, 2026 22:41
@Maffooch
Maffooch marked this pull request as ready for review October 14, 2026 22:41
@Maffooch
Maffooch requested a review from blakeaowens as a code owner October 14, 2026 22:41
@Maffooch
Maffooch requested a review from devGregA October 14, 2026 22:41
@Maffooch
Maffooch enabled auto-merge October 14, 2026 22:43
#16315 was squash-merged as 87354a8 with content identical to this
branch's b1ea96d, so the two files that conflicted keep this branch's
version (#16315's content plus the batching).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Maffooch
Maffooch added this pull request to the merge queue Oct 14, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 14, 2026
@Maffooch
Maffooch added this pull request to the merge queue Oct 15, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 15, 2026
@Maffooch
Maffooch added this pull request to the merge queue Oct 15, 2026
Merged via the queue into dev with commit 27f8eb8 Oct 15, 2026
49 checks passed
@Maffooch
Maffooch deleted the perf/sla-notify-batched-fanout branch October 15, 2026 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance performance improvement or bug report unittests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants