diff --git a/src/backend/core/models.py b/src/backend/core/models.py index a1e303e6e..5b9867e82 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -1954,6 +1954,12 @@ class Reaction(BaseModel): return f"Reaction {self.emoji} on comment {self.comment.id}" +# The notification guard only has to cover the window between the cooldown +# check and the `notified_at` save: a leaked key (e.g. killed worker) must not +# silence a context for the whole cooldown period +MENTION_NOTIFICATION_GUARD_TIMEOUT_SECONDS = 60 + + class Mention(BaseModel): """A mention of a user in a document body or in a comment thread. @@ -2062,7 +2068,7 @@ class Mention(BaseModel): if not cache.add( self.notification_guard_key, str(self.pk), - timeout=settings.MENTION_NOTIFICATION_COOLDOWN_MINUTES * 60, + timeout=MENTION_NOTIFICATION_GUARD_TIMEOUT_SECONDS, ): return False @@ -2095,10 +2101,17 @@ class Mention(BaseModel): "link": f"{domain}/docs/{self.document_id}/#{self.anchor_id}", } - self.document.send_email(subject, [user.email], context, language) + sent = False + try: + self.document.send_email(subject, [user.email], context, language) + self.notified_at = timezone.now() + self.save(update_fields=["notified_at", "updated_at"]) + sent = True + finally: + # Release the context so the next mention can notify + if not sent: + cache.delete(self.notification_guard_key) - self.notified_at = timezone.now() - self.save(update_fields=["notified_at", "updated_at"]) return True diff --git a/src/backend/core/tests/test_models_mentions.py b/src/backend/core/tests/test_models_mentions.py index da6197e5b..ae4de53ee 100644 --- a/src/backend/core/tests/test_models_mentions.py +++ b/src/backend/core/tests/test_models_mentions.py @@ -2,7 +2,10 @@ Unit tests for the Mention model """ +from unittest import mock + from django.core import mail +from django.core.cache import cache import pytest from rest_framework.exceptions import ValidationError @@ -106,3 +109,29 @@ def test_models_mentions_notify_concurrent_duplicate(): assert second.notified_at is None # pylint: disable-next=no-member assert len(mail.outbox) == 1 + + +def test_models_mentions_notify_failure_releases_guard(): + """A failed notification should release the context guard. + + Otherwise the next mention in the same context would be silently dropped + until the guard expires, although nobody was notified. + """ + document = factories.DocumentFactory() + mentioned_user = factories.UserFactory() + first = factories.MentionFactory(document=document, mentioned_user=mentioned_user) + second = factories.MentionFactory(document=document, mentioned_user=mentioned_user) + + with ( + mock.patch.object(models.Document, "send_email", side_effect=RuntimeError), + pytest.raises(RuntimeError), + ): + first.notify() + + first.refresh_from_db() + assert first.notified_at is None + assert cache.get(first.notification_guard_key) is None + + assert second.notify() is True + # pylint: disable-next=no-member + assert len(mail.outbox) == 1