Dispatch create-path notifications async to fix slow POST latency#14731
Open
Dispatch create-path notifications async to fix slow POST latency#14731
Conversation
POST /api/v2/engagements/ takes ~5s on large tenants because create_notification runs recipient enumeration and per-user Alert writes on the request thread. Move the outer create_notification to a Celery worker for the five create-path events (engagement_added, product_added, product_type_added, finding_added, test_added) by adding async_create_notification (accepts ids, re-fetches, delegates) and dispatching via dojo_dispatch_task. This extends the existing per-user async pattern (Slack/email/MSTeams/webhooks) up one level so the recipient query and Alert fan-out no longer block the response. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Reformat async_create_notification importer-guard docstring to D213 style - Skip post_save dispatch when raw=True (loaddata) so the k8s initializer's fixture install path doesn't require an available Celery broker. Without this guard the unconditional async dispatch tries to enqueue during product_type.json load and fails with kombu OperationalError. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
async_create_notificationCelery task indojo/notifications/helper.py; it accepts ids, re-fetches withselect_related, and delegates tocreate_notificationdojo_dispatch_taskinstead of running synchronously:engagement_added,product_added,product_type_added,finding_added(API),test_added(importer)POST /api/v2/engagements/(and the four sibling endpoints) dropping from ~5s to sub-second on tenants with large authorized-user sets — the recipient enumeration query and per-userAlertsinserts now run in the workerWhy ids, not model instances
Existing
send_*_notificationtasks pass Django models through**kwargs, which pickles fine in eager mode but is brittle across real Celery brokers. The new task takes*_idkwargs and re-fetches, matching theuser_idpattern used by the other@app.taskhelpers at the bottom ofhelper.py.Test plan
unittests.test_notifications— full module passes (38 tests). Updated threeTestNotificationTriggersmethods with@override_settings(CELERY_TASK_ALWAYS_EAGER=True), threeTestNotificationTriggersApifinding tests to patchdojo.api_v2.serializers.dojo_dispatch_taskand assertfinding_id=<id>, and threeTestNotificationWebhooksassertions fromcm.output[1]→cm.output[-1](the former index was a fixture-load side effect of the old sync path, see below).TestAsyncNotificationDispatch— verifies dispatch call shape for engagement/product/product_type signals; static guard on the importer dispatch block.TestAsyncNotificationTaskBody— verifies task rehydrates instances, returns silently on missing rows, and runs inline underblock_execution=True.unittests.test_importers_importer+unittests.test_apiv2_notifications— pass (40 tests).test_import_no_push_to_jira— still passes; webhook round-trip fortest_addedis unchanged underblock_execution=True.unittests/suite — 4,051 tests. All previously-failing cases caused by this PR now pass. Updated perf-test expected values inunittests/test_importers_performance.py(+1async_create_notificationdispatch pertest_added; adjusted query counts from actual captured values) andUsersTest.deleted_objectsinunittests/test_rest_framework.py(25 → 13, see below).TestDojoImporterPerformanceSmall.test_import_reimport_reimport_performance_*errors remain (TypeError:_import_reimport_performance() missing 2 required positional arguments: expected_num_queries4 and expected_num_async_tasks4). These are broken onbugfixHEAD before this PR — sibling classTestDojoImporterPerformanceSmallLocationsuses the correct 8-arg call and passes. Not fixed here to keep the diff scoped.Note on
deleted_objects = 25 → 13and webhookcm.output[1] → cm.output[-1]Fixtures load
Product_Type/Product/Engagementrows before the singleNotifications(pk=1)row. On the old sync path, each of thosepost_savesignals synchronously invokedcreate_notification→NotificationManager()→_get_notifications_object()→get_or_create(user=None, product=None, template=False), which created a duplicate system notification row during fixture load becausepk=1hadn't inserted yet. Subsequent manager instantiations triggered a "Multiple system notifications objects found" WARNING that preceded the INFO the webhook tests were actually asserting on — hence the incidentally-correctcm.output[1]index. Similarly, theUsersTestdelete-preview count reflected admin's ownership of that phantom duplicate row. With the dispatch now async (no user context during fixture load → sent to broker → not executed), the phantom row is never created, and the tests' expectations needed to shift to the real values.Out of scope
bulk_createforAlertsrows insideAlertNotificationManger.send_alert_notification— orthogonal worker-side optimization; request latency is fixed as-is.send_*_notificationtasks to use ids instead of model instances — they work today and widening that surface adds risk.recipients=-based calls — not in the hot path.🤖 Generated with Claude Code