feat(notifications): add EDA-based idempotent processing and DLQ for chapter/event updates#4454
Open
Shofikul-Isl4m wants to merge 24 commits intoOWASP:mainfrom
Open
feat(notifications): add EDA-based idempotent processing and DLQ for chapter/event updates#4454Shofikul-Isl4m wants to merge 24 commits intoOWASP:mainfrom
Shofikul-Isl4m wants to merge 24 commits intoOWASP:mainfrom
Conversation
- Add deadline reminder emails with days_remaining (7, 3, 1 days) - Implement meaningful field change detection for entity updates - Add message-based idempotency to prevent duplicate notifications - Replace automatic DLQ processing with manual management commands - Add signal handlers for chapter and event change detection - Update tests for new functionality
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds entity subscription and notification features: new Subscription and Notification models with migrations, signal handlers that enqueue Redis-stream notifications for snapshots/chapters/events, a Redis-backed worker with retry/DLQ logic, management commands (worker, deadline checker, DLQ manager), admin registrations, settings, cron entry, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
15 issues found across 26 files
Confidence score: 2/5
- There is clear reliability risk in
backend/apps/owasp/management/commands/owasp_run_notification_worker.py: broad exception swallowing can acknowledge failed events, causing permanent event loss (high severity/high confidence). - Multiple concrete duplicate/loss paths exist across notification flow (
owasp_run_notification_worker.py,owasp_notification_dlq.py, andmigrations/0073_notification_subscription.py): non-atomic idempotency, missing DB uniqueness, and DLQ retry behavior can resend emails or drop retry records. post_saveemission before commit inbackend/apps/owasp/signals/snapshot.pyandbackend/apps/owasp/signals/event.pycan publish phantom notifications for rolled-back transactions, creating user-visible inconsistency.- Pay close attention to
backend/apps/owasp/management/commands/owasp_run_notification_worker.py,backend/apps/owasp/management/commands/owasp_notification_dlq.py,backend/apps/owasp/signals/snapshot.py,backend/apps/owasp/signals/event.py,backend/apps/owasp/migrations/0073_notification_subscription.py,backend/settings/base.py- notification delivery, dedupe, and transaction-ordering issues are the primary merge risks.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/owasp/signals/snapshot.py">
<violation number="1" location="backend/apps/owasp/signals/snapshot.py:27">
P1: `post_save` triggers external notification before transaction commit, which can publish phantom events when the surrounding DB transaction rolls back.</violation>
</file>
<file name="backend/tests/apps/owasp/utils/notifications_test.py">
<violation number="1" location="backend/tests/apps/owasp/utils/notifications_test.py:120">
P2: Deadline reminder publisher test misses `days_remaining` assertion, so regressions in required reminder payload serialization can pass undetected.</violation>
</file>
<file name="backend/apps/owasp/management/commands/owasp_run_notification_worker.py">
<violation number="1" location="backend/apps/owasp/management/commands/owasp_run_notification_worker.py:67">
P1: Broad exception swallowing in entity handler causes failed events to be acknowledged and dropped from the stream.</violation>
<violation number="2" location="backend/apps/owasp/management/commands/owasp_run_notification_worker.py:68">
P2: Main-loop processing failures are only logged, leaving stream entries pending (unacked and not DLQ’d), which can stall messages until a later recovery/restart.</violation>
<violation number="3" location="backend/apps/owasp/management/commands/owasp_run_notification_worker.py:168">
P1: Idempotency is non-atomic: email is sent before dedup state is persisted, allowing duplicate sends on crash/partial failure or replay.</violation>
</file>
<file name="backend/tests/apps/owasp/signals/event_test.py">
<violation number="1" location="backend/tests/apps/owasp/signals/event_test.py:19">
P2: Test name/docstring claim updated-notification publish coverage, but assertions validate the no-change/no-publish path instead.</violation>
</file>
<file name="backend/apps/owasp/migrations/0073_notification_subscription.py">
<violation number="1" location="backend/apps/owasp/migrations/0073_notification_subscription.py:40">
P1: `Notification` lacks a database-level idempotency/uniqueness constraint, so current read-then-insert dedupe is race-prone and can create duplicates under concurrent processing.</violation>
<violation number="2" location="backend/apps/owasp/migrations/0073_notification_subscription.py:54">
P2: `Subscription.object_id` is too narrow (`PositiveIntegerField`) for bigint primary keys and can overflow for polymorphic targets.</violation>
</file>
<file name="backend/apps/owasp/management/commands/owasp_notification_dlq.py">
<violation number="1" location="backend/apps/owasp/management/commands/owasp_notification_dlq.py:113">
P1: DLQ retry bypasses the Notification idempotency guard, so repeated/manual retries can send duplicate emails.</violation>
<violation number="2" location="backend/apps/owasp/management/commands/owasp_notification_dlq.py:120">
P1: Retry error handling can lose DLQ messages because it deletes the old entry before successfully adding the updated one.</violation>
</file>
<file name="backend/tests/apps/owasp/signals/chapter_test.py">
<violation number="1" location="backend/tests/apps/owasp/signals/chapter_test.py:19">
P2: The test named as verifying updated notifications actually tests the no-change branch, leaving meaningful-change update notification behavior uncovered.</violation>
</file>
<file name="backend/Makefile">
<violation number="1" location="backend/Makefile:160">
P2: New notification worker target runs through an interactive exec helper that suppresses stderr, which can hide worker failures and break non-interactive execution.</violation>
</file>
<file name="backend/settings/base.py">
<violation number="1" location="backend/settings/base.py:234">
P1: Global `Base` settings default email backend to console output, so missing `EMAIL_BACKEND` in staging/production silently disables real email delivery.</violation>
</file>
<file name="backend/apps/owasp/signals/event.py">
<violation number="1" location="backend/apps/owasp/signals/event.py:36">
P1: Event notifications are emitted in `post_save` before transaction commit, so rolled-back DB writes can still produce Redis events.</violation>
<violation number="2" location="backend/apps/owasp/signals/event.py:46">
P2: Changed-field serialization uses truthiness and incorrectly converts empty-string values to `None`, causing inaccurate update diff payloads.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
backend/apps/owasp/management/commands/owasp_run_notification_worker.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_run_notification_worker.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_notification_dlq.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/migrations/0073_notification_subscription.py
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
backend/settings/base.py (1)
232-235: Remove redundantenviron_nameparameter.When the setting name matches the
environ_namevalue, the parameter is redundant.values.Value()will automatically use the setting name (EMAIL_BACKEND) as the environment variable key (withDJANGO_prefix applied).♻️ Suggested fix
- EMAIL_BACKEND = values.Value( - environ_name="EMAIL_BACKEND", default="django.core.mail.backends.console.EmailBackend" - ) + EMAIL_BACKEND = values.Value(default="django.core.mail.backends.console.EmailBackend")Based on learnings: "when a setting name matches the environ_name value, the environ_name parameter is redundant."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/settings/base.py` around lines 232 - 235, The EMAIL_BACKEND setting currently passes an explicit environ_name to values.Value redundantly; update the EMAIL_BACKEND declaration to call values.Value(...) without the environ_name argument so it relies on the default behavior (values.Value uses the setting name as the env key), keeping the same default "django.core.mail.backends.console.EmailBackend" and the values.Value call otherwise unchanged.backend/apps/owasp/apps.py (1)
11-15: Consider addingnoqa: F401to all signal imports for consistency.All three signal modules are imported purely for side effects (registering
@receiverdecorators). Currently onlysnapshothas the suppression comment.♻️ Suggested fix
def ready(self): """Import signals when app is ready.""" - import apps.owasp.signals.chapter - import apps.owasp.signals.event + import apps.owasp.signals.chapter # noqa: F401 + import apps.owasp.signals.event # noqa: F401 import apps.owasp.signals.snapshot # noqa: F401🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/apps.py` around lines 11 - 15, The ready() method imports signal modules for side effects but only apps.owasp.signals.snapshot has the noqa suppression; add "# noqa: F401" to the other two imports (apps.owasp.signals.chapter and apps.owasp.signals.event) so all three side-effect-only imports in ready() are consistently ignored by the unused-import linter.backend/tests/apps/owasp/signals/event_test.py (1)
18-40: Misleading docstring - test verifies no notification when no fields change.The docstring says "Test that updating an event publishes an 'updated' notification" but the test actually verifies the opposite: when no meaningful fields change, no notification is published. Consider:
- Fixing the docstring to match the actual behavior being tested
- Adding a separate test for when fields DO change and notification IS published
♻️ Suggested fix and additional test
`@patch`("apps.owasp.signals.event.publish_event_notification") - def test_event_updated_publishes_updated_notification(self, mock_publish): - """Test that updating an event publishes an 'updated' notification.""" + def test_event_updated_no_changes_does_not_publish(self, mock_publish): + """Test that updating an event without meaningful changes does not publish.""" event = MagicMock() - # Set up previous values that match current values - no changes, no notification + # Set up previous values that match current values event._previous_values = {Add a test for actual changes:
`@patch`("apps.owasp.signals.event.publish_event_notification") def test_event_updated_with_changes_publishes_notification(self, mock_publish): """Test that updating an event with meaningful changes publishes notification.""" event = MagicMock() event._previous_values = {"name": "Old Name", ...} event.name = "New Name" # ... other fields same as previous event_post_save(sender=None, instance=event, created=False) mock_publish.assert_called_once() assert mock_publish.call_args[0][1] == "updated"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/signals/event_test.py` around lines 18 - 40, Update the misleading docstring in test_event_updated_publishes_updated_notification to state it verifies that no notification is published when no meaningful fields change, and add a new test named test_event_updated_with_changes_publishes_notification that uses MagicMock with event._previous_values different from current values, calls event_post_save(sender=None, instance=event, created=False), and asserts the patched publish_event_notification (mock_publish) is called once and that its call args include the "updated" notification; reference event_post_save, publish_event_notification, mock_publish, and event._previous_values to locate the relevant code.backend/tests/apps/owasp/utils/notifications_test.py (1)
112-124: Consider testingdays_remainingin deadline reminder notification.The test verifies the notification type but doesn't pass or assert
days_remaining, which is mentioned in the PR objectives as part of the deadline reminder payload. The actual command inowasp_check_event_deadlines.pycallspublish_event_notification(event, "deadline_reminder", days_remaining=days).♻️ Suggested enhancement
`@patch`("apps.owasp.utils.notifications.get_redis_connection") def test_publishes_deadline_reminder_notification(self, mock_redis): """Test that event deadline reminder notification is published.""" mock_conn = MagicMock() mock_redis.return_value = mock_conn event = MagicMock() event.id = 10 - publish_event_notification(event, "deadline_reminder") + publish_event_notification(event, "deadline_reminder", days_remaining=3) call_args = mock_conn.xadd.call_args assert call_args[0][1]["type"] == "event_deadline_reminder" + assert call_args[0][1]["days_remaining"] == "3"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/utils/notifications_test.py` around lines 112 - 124, The test test_publishes_deadline_reminder_notification does not verify the days_remaining field; update the test to call publish_event_notification(event, "deadline_reminder", days_remaining=expected_days) and assert that mock_conn.xadd was called with a payload containing "days_remaining" equal to the expected value (in addition to the existing type check) so the deadline reminder payload matches the usage in owasp_check_event_deadlines.py and publish_event_notification.backend/apps/owasp/signals/chapter.py (1)
37-40: Empty string handling may cause inconsistent display.The condition
if old_value else Nonetreats empty strings as falsey, converting them toNone. This could cause display inconsistencies where an empty field shows asNone → "new value"instead of"" → "new value".This is a minor edge case and may be intentional for cleaner display.
♻️ Alternative for explicit None handling
changed_fields[field] = { - "old": str(old_value) if old_value else None, - "new": str(new_value) if new_value else None, + "old": str(old_value) if old_value is not None else None, + "new": str(new_value) if new_value is not None else None, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/signals/chapter.py` around lines 37 - 40, The current conversion uses truthiness and turns empty strings into None causing inconsistent display; update the assignment to only treat actual None as None by changing the conditions to explicit None checks (e.g., use "old": str(old_value) if old_value is not None else None and "new": str(new_value) if new_value is not None else None) in the changed_fields[field] construction so empty strings remain "" while real None values stay None.backend/apps/owasp/management/commands/owasp_run_notification_worker.py (1)
287-334: Fix the 1-day reminder copy.
days_infoalways uses"days", so the 1-day reminder renders"1 days left"in both subject and body.💬 Proposed copy fix
if days_bytes: days = days_bytes.decode() - days_info = f" ({days} days left)" + unit = "day" if days == "1" else "days" + days_info = f" ({days} {unit} left)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/management/commands/owasp_run_notification_worker.py` around lines 287 - 334, The reminder text always shows "days" because days_info is built from the decoded string without pluralization; change the days handling (where days_bytes is decoded into days and days_info is set) to parse the days value as an integer and choose the unit ("day" vs "days") accordingly (e.g., when days_int == 1 use "day") and then set days_info = f" ({days_int} {unit} left)"; this will ensure the subject/body templates referencing days_info (see days_bytes, days, days_info and the "event_deadline_reminder" entry in titles/entity_messages) render "1 day left" correctly.backend/tests/apps/owasp/signals/chapter_test.py (1)
18-38: Add the actual updated-notification path to this suite.This test only exercises the no-op branch, so a regression in meaningful-field detection would still pass. Either rename it to match the current assertion or add a case where one tracked field changes and assert the
"updated"publish call.🧪 Suggested positive-path test
+ `@patch`("apps.owasp.signals.chapter.publish_chapter_notification") + def test_chapter_updated_with_meaningful_change_publishes_updated_notification( + self, mock_publish + ): + chapter = MagicMock() + chapter._previous_values = { + "name": "Old Chapter", + "country": "Test Country", + "region": "Test Region", + "suggested_location": "Test Location", + "description": "Test description", + } + chapter.name = "New Chapter" + chapter.country = "Test Country" + chapter.region = "Test Region" + chapter.suggested_location = "Test Location" + chapter.description = "Test description" + + chapter_post_save(sender=None, instance=chapter, created=False) + + mock_publish.assert_called_once_with( + chapter, + "updated", + {"name": {"old": "Old Chapter", "new": "New Chapter"}}, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/signals/chapter_test.py` around lines 18 - 38, The test only covers the no-op branch; add a positive-path case in the same test file that mutates one tracked field and asserts publish_chapter_notification is called with an "updated" notification: call chapter_post_save(sender=None, instance=chapter, created=False) after changing one property (e.g., change chapter.name or chapter.description from its _previous_values), and use the patched mock_publish to assert it was invoked (e.g., mock_publish.assert_called_once_with(...) or check call args for an "updated" action). Keep the existing no-change assertion or split into two tests (rename the current to reflect "no changes") and add a new test method like test_chapter_updated_publishes_updated_notification_on_field_change referencing chapter_post_save and publish_chapter_notification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/owasp/management/commands/owasp_notification_dlq.py`:
- Around line 103-123: The DLQ retry branch currently sends the email via
send_mail and deletes the stream entry with redis_conn.xdel but never persists a
Notification record or uses the worker's idempotency path; modify this branch
(the block using self._get_value, send_mail, redis_conn.xdel, and
self.stdout.write) to route the delivery through the same
persistence/idempotency routine the live worker uses (i.e., call the
service/function that creates Notification rows and enforces deduplication
instead of directly sending mail), then only delete the DLQ entry and increment
success_count after that routine returns success so retried messages are
recorded in-app and deduped.
In `@backend/apps/owasp/management/commands/owasp_run_notification_worker.py`:
- Around line 156-183: The send_notification function is racy because it checks
Notification.objects.filter(...).exists() then does send_mail() then
Notification.objects.create(), which allows duplicate mails when concurrent
workers run; fix by creating or claiming an idempotency record transactionally
before the side effect: add a unique constraint (e.g., on
recipient/type/related_link/message or an idempotency_key) and in
send_notification use a DB transaction/atomic block to insert a pending
Notification (or upsert/get_or_create) to reserve the notification, only proceed
to call send_mail() if the insert succeeded (i.e., you were the creator), and
then update that Notification to sent/complete; alternatively, implement a
separate Idempotency model and claim it (create unique row) inside an atomic
block before calling send_mail(), ensuring only the holder sends the email.
- Around line 62-68: The main loop currently logs exceptions from
process_message() but leaves the message in the PEL until restart; change the
except block so failed messages are retried or dead-lettered instead of being
stranded: on exception, read a retry_count field from the message (or add one),
increment it, and if retry_count < MAX_RETRIES requeue the message back onto the
stream (XADD) with the updated retry_count and then XACK the original
message_id; if retry_count >= MAX_RETRIES XADD the message to a DLQ stream
(e.g., stream_key + ":dlq") and XACK the original; keep logging the failure and
include message_id and retry_count; reference process_message,
recover_pending_messages, stream_key, group_name, message_id, and
introduce/configure MAX_RETRY_ATTEMPTS.
- Around line 392-395: The generic "except Exception:" block in the worker
swallows failures from _handle_entity_notification() so the message gets XACKed
as if processed; remove or change that catch so failures are re-raised (or
re-thrown) after logging instead of returning silently. Keep the specific
model_class.DoesNotExist handling, but for the generic exception either remove
the except entirely or call logger.exception(...) and then raise to let the
outer worker logic treat it (retry/DLQ) — reference the logger.exception call,
the _handle_entity_notification invocation, and the
notification_type/model_class symbols to locate the code to update.
- Around line 417-427: The DLQ entry created in the exception branch for
recovery failures only records message_id and error, which prevents the DLQ
retry command from reconstructing and resending the notification; update the
redis_conn.xadd call in the except Exception as exc block (the one that logs via
logger.exception and uses self.DLQ_STREAM_KEY, stream_key, group_name,
message_id) to include the original notification payload fields (e.g.,
user_email, title, message, notification_type) along with
type="recovery_failed", message_id and error so the owasp_notification_dlq retry
command can read and resend the message.
In `@backend/apps/owasp/utils/notifications.py`:
- Around line 20-33: The current publish block around
get_redis_connection/redis_conn.xadd(STREAM_KEY, message) catches all exceptions
and only logs them, which drops notifications; change this by persisting failed
messages to a durable outbox/retry store or re-raising for caller-managed retry
so failures aren't lost: implement (or call) a durable fallback path when xadd
fails (e.g., write the same message object to your DB outbox table or enqueue to
a persistent task queue) and/or re-raise the exception instead of swallowing it;
update the snapshot publish code (the try/except surrounding
get_redis_connection, STREAM_KEY, xadd and logger.exception) and apply the same
durable-fallback pattern to the other publish helpers referenced (lines ~48-65
and ~84-103) so all publish failures are persisted or propagated for retry.
---
Nitpick comments:
In `@backend/apps/owasp/apps.py`:
- Around line 11-15: The ready() method imports signal modules for side effects
but only apps.owasp.signals.snapshot has the noqa suppression; add "# noqa:
F401" to the other two imports (apps.owasp.signals.chapter and
apps.owasp.signals.event) so all three side-effect-only imports in ready() are
consistently ignored by the unused-import linter.
In `@backend/apps/owasp/management/commands/owasp_run_notification_worker.py`:
- Around line 287-334: The reminder text always shows "days" because days_info
is built from the decoded string without pluralization; change the days handling
(where days_bytes is decoded into days and days_info is set) to parse the days
value as an integer and choose the unit ("day" vs "days") accordingly (e.g.,
when days_int == 1 use "day") and then set days_info = f" ({days_int} {unit}
left)"; this will ensure the subject/body templates referencing days_info (see
days_bytes, days, days_info and the "event_deadline_reminder" entry in
titles/entity_messages) render "1 day left" correctly.
In `@backend/apps/owasp/signals/chapter.py`:
- Around line 37-40: The current conversion uses truthiness and turns empty
strings into None causing inconsistent display; update the assignment to only
treat actual None as None by changing the conditions to explicit None checks
(e.g., use "old": str(old_value) if old_value is not None else None and "new":
str(new_value) if new_value is not None else None) in the changed_fields[field]
construction so empty strings remain "" while real None values stay None.
In `@backend/settings/base.py`:
- Around line 232-235: The EMAIL_BACKEND setting currently passes an explicit
environ_name to values.Value redundantly; update the EMAIL_BACKEND declaration
to call values.Value(...) without the environ_name argument so it relies on the
default behavior (values.Value uses the setting name as the env key), keeping
the same default "django.core.mail.backends.console.EmailBackend" and the
values.Value call otherwise unchanged.
In `@backend/tests/apps/owasp/signals/chapter_test.py`:
- Around line 18-38: The test only covers the no-op branch; add a positive-path
case in the same test file that mutates one tracked field and asserts
publish_chapter_notification is called with an "updated" notification: call
chapter_post_save(sender=None, instance=chapter, created=False) after changing
one property (e.g., change chapter.name or chapter.description from its
_previous_values), and use the patched mock_publish to assert it was invoked
(e.g., mock_publish.assert_called_once_with(...) or check call args for an
"updated" action). Keep the existing no-change assertion or split into two tests
(rename the current to reflect "no changes") and add a new test method like
test_chapter_updated_publishes_updated_notification_on_field_change referencing
chapter_post_save and publish_chapter_notification.
In `@backend/tests/apps/owasp/signals/event_test.py`:
- Around line 18-40: Update the misleading docstring in
test_event_updated_publishes_updated_notification to state it verifies that no
notification is published when no meaningful fields change, and add a new test
named test_event_updated_with_changes_publishes_notification that uses MagicMock
with event._previous_values different from current values, calls
event_post_save(sender=None, instance=event, created=False), and asserts the
patched publish_event_notification (mock_publish) is called once and that its
call args include the "updated" notification; reference event_post_save,
publish_event_notification, mock_publish, and event._previous_values to locate
the relevant code.
In `@backend/tests/apps/owasp/utils/notifications_test.py`:
- Around line 112-124: The test test_publishes_deadline_reminder_notification
does not verify the days_remaining field; update the test to call
publish_event_notification(event, "deadline_reminder",
days_remaining=expected_days) and assert that mock_conn.xadd was called with a
payload containing "days_remaining" equal to the expected value (in addition to
the existing type check) so the deadline reminder payload matches the usage in
owasp_check_event_deadlines.py and publish_event_notification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85e9f11a-aeb9-4be1-8b50-527887c5a95e
📒 Files selected for processing (26)
backend/Makefilebackend/apps/owasp/Makefilebackend/apps/owasp/admin/notification.pybackend/apps/owasp/apps.pybackend/apps/owasp/management/commands/owasp_check_event_deadlines.pybackend/apps/owasp/management/commands/owasp_notification_dlq.pybackend/apps/owasp/management/commands/owasp_run_notification_worker.pybackend/apps/owasp/migrations/0073_notification_subscription.pybackend/apps/owasp/migrations/0074_remove_subscription_owasp_subsc_user_id_33ae6d_idx_and_more.pybackend/apps/owasp/migrations/0075_alter_subscription_object_id.pybackend/apps/owasp/models/__init__.pybackend/apps/owasp/models/notification.pybackend/apps/owasp/signals/__init__.pybackend/apps/owasp/signals/chapter.pybackend/apps/owasp/signals/event.pybackend/apps/owasp/signals/snapshot.pybackend/apps/owasp/utils/notifications.pybackend/settings/base.pybackend/tests/apps/owasp/management/commands/owasp_check_event_deadlines_test.pybackend/tests/apps/owasp/management/commands/owasp_run_notification_worker_test.pybackend/tests/apps/owasp/signals/__init__.pybackend/tests/apps/owasp/signals/chapter_test.pybackend/tests/apps/owasp/signals/event_test.pybackend/tests/apps/owasp/utils/notifications_test.pycron/productioncspell/custom-dict.txt
backend/apps/owasp/management/commands/owasp_run_notification_worker.py
Outdated
Show resolved
Hide resolved
- Main loop now sends failed messages directly to DLQ with full original data instead of only logging and leaving them stranded in PEL - Recovery DLQ entries now include original stream payload for observability and future retries
- Re-raise DoesNotExist and generic Exception in _handle_entity_notification so failures propagate to the main loop DLQ routing instead of being silently swallowed and ACKed
- Extracted send_notification business logic out of the worker class and moved it to utils/notifications.py to serve as a shared utility - Updated DLQ retry command to use the shared send_notification utility so that manually retried messages go through the exact same deduplication checks and Notification model persistence as the worker - Updated worker and related unit tests to use the new shared utilit sonarcloud security warning for hardcoded http:// URLs in event test
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/apps/owasp/utils/notifications.py (1)
108-135:⚠️ Potential issue | 🟠 MajorRace condition in idempotency check remains unaddressed.
The
exists()→send_mail()→create()sequence is not atomic. With multiple concurrent workers, two can both pass theexists()check before either commits theNotificationrow, resulting in duplicate emails. Additionally, ifsend_mail()succeeds butcreate()fails (DB error), retries will resend the email.The Notification model (context snippet 1) has no
unique_togetherconstraint on(recipient, type, related_link, message), so the DB cannot enforce uniqueness as a fallback.Consider using
get_or_createor a transactional "claim" pattern:Proposed atomic idempotency fix
def send_notification(*, user, title, message, notification_type, related_link): """Send notification to user and persist to DB.""" - if Notification.objects.filter( - recipient_id=user.id, - type=notification_type, - related_link=related_link, - message=message, - ).exists(): + from django.db import IntegrityError, transaction + + # Attempt to claim the notification slot first (idempotency key) + try: + with transaction.atomic(): + notification, created = Notification.objects.get_or_create( + recipient_id=user.id, + type=notification_type, + related_link=related_link, + message=message, + defaults={"title": title}, + ) + except IntegrityError: + # Another worker won the race + logger.info("Already notified %s for %s, skipping", user.email, notification_type) + return + + if not created: logger.info("Already notified %s for %s, skipping", user.email, notification_type) return full_message = f"{message}\n\nView: {related_link}" if related_link else message send_mail( subject=title, message=full_message, from_email="noreply@owasp.org", recipient_list=[user.email], fail_silently=False, ) logger.info("Sent %s email to %s", notification_type, user.email) - - Notification.objects.create( - recipient=user, - type=notification_type, - title=title, - message=message, - related_link=related_link, - )Note: This fix also requires adding a unique constraint on the Notification model for
(recipient, type, related_link, message)to makeget_or_createrace-safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/utils/notifications.py` around lines 108 - 135, The current exists() → send_mail() → create() flow in send_notification is racy; change send_notification to perform an atomic get_or_create on the Notification model (using Notification.objects.get_or_create(recipient=user, type=notification_type, related_link=related_link, message=message, defaults={'title': title})) and only call send_mail if get_or_create returned created=True; additionally add a DB-level unique constraint on (recipient, type, related_link, message) in the Notification model (and create a migration) so get_or_create is race-safe across concurrent workers; ensure you reference the send_notification function and the Notification model when making these changes and run migrations.
🧹 Nitpick comments (3)
backend/apps/owasp/management/commands/owasp_run_notification_worker.py (1)
127-173: Unreachablereturn Falseat line 173.The function always returns from within the
whileloop: eitherreturn Trueon success (line 171) orreturn Falseafter exceedingMAX_RETRIES(line 163). Line 173 is dead code.Remove unreachable code
logger.info( "Email to %s succeeded after %d retries", user.email, retry_count, ) return True - - return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/management/commands/owasp_run_notification_worker.py` around lines 127 - 173, The final "return False" is unreachable in send_notification_with_retry because all control paths inside the while loop already return; remove the trailing "return False" at the end of the function so there is no dead code; keep the existing logic that returns True on success and returns False when retry_count exceeds self.MAX_RETRIES (logger.exception path), and leave constants MAX_RETRIES, BASE_DELAY, and DELAY_MULTIPLIER unchanged.backend/tests/apps/owasp/management/commands/owasp_run_notification_worker_test.py (2)
56-74: Unusedcommandfixture in idempotency test.Same as above —
commandis passed but not used.Remove unused fixture
`@mock.patch`("apps.owasp.utils.notifications.send_mail") `@mock.patch`("apps.owasp.utils.notifications.Notification") def test_send_notification_idempotency( - self, mock_notification, mock_send_mail, command, mock_user, mock_snapshot + self, mock_notification, mock_send_mail, mock_user, mock_snapshot ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/management/commands/owasp_run_notification_worker_test.py` around lines 56 - 74, The test function test_send_notification_idempotency declares an unused fixture parameter named command; remove the unused parameter from the test signature so it reads def test_send_notification_idempotency(self, mock_notification, mock_send_mail, mock_user, mock_snapshot): and run the tests to ensure no other references to command exist—this cleans up the unused fixture without changing behavior of send_notification assertions.
30-54: Unusedcommandfixture in test method.The
commandfixture is passed totest_send_notification_successbut never used — this test exercises thesend_notificationutility function directly, not theCommandclass.Remove unused fixture
`@mock.patch`("apps.owasp.utils.notifications.send_mail") `@mock.patch`("apps.owasp.utils.notifications.Notification") def test_send_notification_success( - self, mock_notification, mock_send_mail, command, mock_user, mock_snapshot + self, mock_notification, mock_send_mail, mock_user, mock_snapshot ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/apps/owasp/management/commands/owasp_run_notification_worker_test.py` around lines 30 - 54, Remove the unused `command` fixture from the test method signature in test_send_notification_success; update the parameter list to (self, mock_notification, mock_send_mail, mock_user, mock_snapshot) so it matches the `@mock.patch` decoration order, leaving the test body unchanged (it calls send_notification and asserts on mock_send_mail and mock_notification.objects.create).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/apps/owasp/utils/notifications.py`:
- Around line 108-135: The current exists() → send_mail() → create() flow in
send_notification is racy; change send_notification to perform an atomic
get_or_create on the Notification model (using
Notification.objects.get_or_create(recipient=user, type=notification_type,
related_link=related_link, message=message, defaults={'title': title})) and only
call send_mail if get_or_create returned created=True; additionally add a
DB-level unique constraint on (recipient, type, related_link, message) in the
Notification model (and create a migration) so get_or_create is race-safe across
concurrent workers; ensure you reference the send_notification function and the
Notification model when making these changes and run migrations.
---
Nitpick comments:
In `@backend/apps/owasp/management/commands/owasp_run_notification_worker.py`:
- Around line 127-173: The final "return False" is unreachable in
send_notification_with_retry because all control paths inside the while loop
already return; remove the trailing "return False" at the end of the function so
there is no dead code; keep the existing logic that returns True on success and
returns False when retry_count exceeds self.MAX_RETRIES (logger.exception path),
and leave constants MAX_RETRIES, BASE_DELAY, and DELAY_MULTIPLIER unchanged.
In
`@backend/tests/apps/owasp/management/commands/owasp_run_notification_worker_test.py`:
- Around line 56-74: The test function test_send_notification_idempotency
declares an unused fixture parameter named command; remove the unused parameter
from the test signature so it reads def test_send_notification_idempotency(self,
mock_notification, mock_send_mail, mock_user, mock_snapshot): and run the tests
to ensure no other references to command exist—this cleans up the unused fixture
without changing behavior of send_notification assertions.
- Around line 30-54: Remove the unused `command` fixture from the test method
signature in test_send_notification_success; update the parameter list to (self,
mock_notification, mock_send_mail, mock_user, mock_snapshot) so it matches the
`@mock.patch` decoration order, leaving the test body unchanged (it calls
send_notification and asserts on mock_send_mail and
mock_notification.objects.create).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1d647b5-a5c2-472c-a7a0-df3767088f09
📒 Files selected for processing (5)
backend/apps/owasp/management/commands/owasp_notification_dlq.pybackend/apps/owasp/management/commands/owasp_run_notification_worker.pybackend/apps/owasp/utils/notifications.pybackend/tests/apps/owasp/management/commands/owasp_run_notification_worker_test.pybackend/tests/apps/owasp/signals/event_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/apps/owasp/signals/event_test.py
- backend/apps/owasp/management/commands/owasp_notification_dlq.py
Contributor
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/owasp/management/commands/owasp_notification_dlq.py">
<violation number="1" location="backend/apps/owasp/management/commands/owasp_notification_dlq.py:131">
P2: `User.DoesNotExist` retry failures bypass `dlq_retries` increment, causing inconsistent DLQ retry accounting.</violation>
</file>
<file name="backend/apps/owasp/utils/notifications.py">
<violation number="1" location="backend/apps/owasp/utils/notifications.py:110">
P1: Idempotency check is non-atomic and persisted after sending email, allowing duplicate emails during concurrency or DB-write-after-send failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Wrapped all post_save publishers (snapshot, chapter, event) in transaction.on_commit() to prevent phantom Redis messages during rollbacks - Fixed falsy dictionary parsing in signals so empty strings aren't converted to None - Updated 0073 migration to use PositiveBigIntegerField for GenericForeignKey object_id to prevent overflow with large integer keys - Added missing days_remaining payload assertion in deadline reminder test - Fixed chapter and event signal tests to properly cover the meaningful-update publish path instead of just the no-change skip path - Added transaction.on_commit mock decorators to test classes to pass Pytest strict database-access rules
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/owasp/signals/event.py`:
- Around line 36-55: The lambdas passed to transaction.on_commit capture
instance and changed_fields by reference which can be mutated before commit;
change them to bind current values as default args so the snapshot is
captured—e.g., for the created branch bind instance (lambda instance=instance:
publish_event_notification(instance, "created")) and for the updated branch bind
both instance and changed_fields (lambda instance=instance,
changed_fields=changed_fields: publish_event_notification(instance, "updated",
changed_fields=changed_fields)); update the usages around transaction.on_commit
and publish_event_notification in the event handling code that inspects
MEANINGFUL_FIELDS.
In `@backend/tests/apps/owasp/utils/notifications_test.py`:
- Around line 112-124: The test passes a string for days_remaining but
publish_event_notification declares days_remaining: int | None; update the
test_publishes_deadline_reminder_notification to pass an integer (5) instead of
the string "5" so the argument type matches the function signature and adjust
the assertion accordingly (still check mock_conn.xadd.call_args for
["days_remaining"] value).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d48a3890-3708-4138-8dcb-ee053eb629ab
📒 Files selected for processing (8)
backend/apps/owasp/migrations/0073_notification_subscription.pybackend/apps/owasp/signals/chapter.pybackend/apps/owasp/signals/event.pybackend/apps/owasp/signals/snapshot.pybackend/tests/apps/owasp/management/commands/owasp_run_notification_worker_test.pybackend/tests/apps/owasp/signals/chapter_test.pybackend/tests/apps/owasp/signals/event_test.pybackend/tests/apps/owasp/utils/notifications_test.py
✅ Files skipped from review due to trivial changes (1)
- backend/tests/apps/owasp/management/commands/owasp_run_notification_worker_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/owasp/signals/chapter.py
- backend/tests/apps/owasp/signals/chapter_test.py
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/apps/owasp/signals/event_test.py">
<violation number="1" location="backend/tests/apps/owasp/signals/event_test.py:13">
P2: Tests patch `transaction.on_commit` but never assert it was used, so regressions that publish notifications outside commit scheduling may go undetected.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Patched lambda late-binding bug in all signals by using default argument binding (inst=instance) to capture immutable snapshots - Hardened DLQ retry logic to explicitly delete messages for missing users, preventing infinite 'Zombie' retry loops in Redis - Added explicit mock_on_commit assertions to chapter and event tests to ensure notifications are always scheduled within transactions - Standardized deadline reminder tests to pass integer days_remaining while verifying stringified Redis payloads
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/owasp/management/commands/owasp_notification_dlq.py">
<violation number="1" location="backend/apps/owasp/management/commands/owasp_notification_dlq.py:131">
P2: Failed DLQ retries are deleted on `User.DoesNotExist`, causing failed notifications to be dropped instead of retained for manual recovery.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
backend/apps/owasp/management/commands/owasp_notification_dlq.py
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/apps/owasp/signals/event.py (1)
20-30: Consider setting_previous_valueswhen the DB record is not found.When
instance.pkis truthy butdb_instanceisNone(e.g., concurrent deletion or manually set pk for a non-existent record),_previous_valuesis not set. The post_save handler'sgetattr(..., {})default handles this gracefully, but explicitly setting an empty dict in this edge case would make the intent clearer.Optional: Make the edge case explicit
`@receiver`(pre_save, sender=Event) def event_pre_save(sender, instance, **kwargs): # noqa: ARG001 """Store the previous values before saving.""" if instance.pk: db_instance = Event.objects.filter(pk=instance.pk).values(*MEANINGFUL_FIELDS).first() if db_instance: instance._previous_values = { # noqa: SLF001 field: db_instance.get(field) for field in MEANINGFUL_FIELDS } + else: + instance._previous_values = {} # noqa: SLF001 else: instance._previous_values = {} # noqa: SLF001🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/owasp/signals/event.py` around lines 20 - 30, event_pre_save may leave instance._previous_values unset when instance.pk is truthy but db_instance is None; ensure we explicitly set instance._previous_values = {} in that branch so the post_save handler can rely on it. Locate the pre_save receiver function event_pre_save and, after fetching db_instance via Event.objects.filter(...).values(*MEANINGFUL_FIELDS).first(), set instance._previous_values = {} when db_instance is falsy (otherwise keep the dict built from MEANINGFUL_FIELDS); this makes the edge case explicit and consistent with the non-pk branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/owasp/management/commands/owasp_notification_dlq.py`:
- Around line 106-150: The loop currently only checks for
user_email/title/message and silently skips other DLQ shapes; fix by reading the
record type (use self._get_value(data, b"type")) and add an explicit branch for
b"processing_failed" that replays the entry through the same worker path used by
owasp_run_notification_worker (call the worker's message handler or re-publish
to the main notification stream), or if unsupported, log a clear error and
increment dlq_retries before deleting the DLQ entry; modify the block around
self._get_value, DLQ_STREAM_KEY, redis_conn.xdel, send_notification and
send_mail to handle the new branch and ensure success_count/error_count and
message deletion are updated appropriately.
- Around line 27-35: Make the CLI flags mutually exclusive by updating the
command's argument setup: in the add_arguments (or where parser is defined)
replace the two separate parser.add_argument calls for "--id" and "--all" with a
mutually exclusive group (e.g., group = parser.add_mutually_exclusive_group())
and add the "--id" and "--all" arguments to that group, or alternatively keep
the args but add an explicit check in handle() that raises a CommandError if
both options are provided; reference the parser.add_argument calls for "--id"
and "--all" to locate the changes.
- Around line 48-55: Replace non-idiomatic exits and silent returns with
Django's CommandError: in the action validation block where you currently call
sys.exit(1) for the "retry" branch, raise CommandError("Error: --id or --all is
required for retry"); similarly change the early returns in retry_dlq (method
retry_dlq) to raise CommandError with the same descriptive message instead of
returning, and do the same for remove_dlq (method remove_dlq) and the "remove"
action validation (raise CommandError("Error: --id or --all is required for
remove") instead of printing and returning); this ensures the management command
exits with code 1 and matches the rest of the codebase.
---
Nitpick comments:
In `@backend/apps/owasp/signals/event.py`:
- Around line 20-30: event_pre_save may leave instance._previous_values unset
when instance.pk is truthy but db_instance is None; ensure we explicitly set
instance._previous_values = {} in that branch so the post_save handler can rely
on it. Locate the pre_save receiver function event_pre_save and, after fetching
db_instance via Event.objects.filter(...).values(*MEANINGFUL_FIELDS).first(),
set instance._previous_values = {} when db_instance is falsy (otherwise keep the
dict built from MEANINGFUL_FIELDS); this makes the edge case explicit and
consistent with the non-pk branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66f9fdc2-44f5-4915-b1ac-f472ea52c9ab
📒 Files selected for processing (7)
backend/apps/owasp/management/commands/owasp_notification_dlq.pybackend/apps/owasp/signals/chapter.pybackend/apps/owasp/signals/event.pybackend/apps/owasp/signals/snapshot.pybackend/tests/apps/owasp/signals/chapter_test.pybackend/tests/apps/owasp/signals/event_test.pybackend/tests/apps/owasp/utils/notifications_test.py
✅ Files skipped from review due to trivial changes (3)
- backend/tests/apps/owasp/utils/notifications_test.py
- backend/tests/apps/owasp/signals/chapter_test.py
- backend/tests/apps/owasp/signals/event_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/owasp/signals/snapshot.py
- backend/apps/owasp/signals/chapter.py
backend/apps/owasp/management/commands/owasp_notification_dlq.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_notification_dlq.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_notification_dlq.py
Outdated
Show resolved
Hide resolved
- Standardized DLQ command to use Django's CommandError for idiomatic error reporting and correct terminal exit codes - Patched DLQ command arguments with mutually exclusive groups to prevent accidental data loss from overlapping --id and --all flags - Improved DLQ logging to distinguish between retriable email failures and system-level worker failures requiring manual admin review - Implemented early-binding snapshots (inst=instance) for all signal lambdas to fix the Python late-binding closure bug in transactions - Strengthened test suite with mandatory on_commit mock assertions and resolved payload type mismatches in deadline reminder tests
- Fixed TRY003 and EM101 errors in owasp_notification_dlq.py by assigning exception string literals to variables before raising CommandError - Ensured compliance with project linting rules for better error traceability and cleaner exception handling
|
Contributor
Author
|
@arkid15r this is ready for review |
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
1 participant
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.
Proposed change
This PR depends on #3701 please rebase before reviewing/merging.
Resolves #3900
Implements a robust Event-Driven Architecture (EDA) notification system with idempotent processing and manual DLQ management.
This implementation uses an event-driven approach:
Entity changes (create/update) → emit events to Redis Stream
Worker consumes events asynchronously → processes notifications
On failure → exponential backoff retry (5 attempts)
Still failing → stored in DLQ for manual handling
Idempotency ensures no duplicate notifications
Changes
owasp_notification_dlqmanagement command:list- Show all failed notificationsretry- Retry failed notificationsremove- Remove failed notifications from DLQChecklist
make check-testlocally: all warnings addressed, tests passed