Browse Source

fix(issue_alerts): Make sure that we update cached groups after flushing buffers. (#33950)

This fixes a bug with issue alerts where `IssueOccurrencesFilter` will cause an alert to not fire
correctly. This is because the value of `group.times_seen` is cached and ends up being inaccurate.

This happens because we cache groups for 5 minutes in `post_process_group`. We usually try to push
to the cache when updating a group, but when we update `times_seen` using buffers we were using
`create_or_update`, which just updates the database and doesn't fire `post_save` signals. We rely on
`post_save` to properly populate the cache here.

Special casing `Group` here so that we fetch the actual row and call `.update` on it directly. This
fires `post_save` which then updates the cached value.
Dan Fuller 2 years ago
parent
commit
59ba4e6e92
2 changed files with 33 additions and 8 deletions
  1. 16 8
      src/sentry/buffer/base.py
  2. 17 0
      tests/sentry/buffer/redis/tests.py

+ 16 - 8
src/sentry/buffer/base.py

@@ -71,14 +71,22 @@ class Buffer(Service, metaclass=BufferMount):
 
             # HACK(dcramer): this is gross, but we don't have a good hook to compute this property today
             # XXX(dcramer): remove once we can replace 'priority' with something reasonable via Snuba
-            if model is Group and "last_seen" in update_kwargs and "times_seen" in update_kwargs:
-                update_kwargs["score"] = ScoreClause(
-                    group=None,
-                    times_seen=update_kwargs["times_seen"],
-                    last_seen=update_kwargs["last_seen"],
-                )
-
-            _, created = model.objects.create_or_update(values=update_kwargs, **filters)
+            if model is Group:
+                if "last_seen" in update_kwargs and "times_seen" in update_kwargs:
+                    update_kwargs["score"] = ScoreClause(
+                        group=None,
+                        times_seen=update_kwargs["times_seen"],
+                        last_seen=update_kwargs["last_seen"],
+                    )
+                # XXX: create_or_update doesn't fire `post_save` signals, and so this update never
+                # ends up in the cache. This causes issues when handling issue alerts, and likely
+                # elsewhere. Use `update` here since we're already special casing, and we know that
+                # the group will already exist.
+                group = Group.objects.get(**filters)
+                group.update(**update_kwargs)
+                created = False
+            else:
+                _, created = model.objects.create_or_update(values=update_kwargs, **filters)
 
         buffer_incr_complete.send_robust(
             model=model,

+ 17 - 0
tests/sentry/buffer/redis/tests.py

@@ -4,6 +4,7 @@ from unittest import mock
 
 from django.utils import timezone
 from django.utils.encoding import force_text
+from freezegun import freeze_time
 
 from sentry.buffer.redis import RedisBuffer
 from sentry.models import Group, Project
@@ -86,6 +87,22 @@ class RedisBufferTest(TestCase):
         self.buf.process("foo")
         process.assert_called_once_with(Group, columns, filters, extra, signal_only)
 
+    @freeze_time()
+    def test_group_cache_updated(self):
+        # Make sure group is stored in the cache and keep track of times_seen at the time
+        orig_times_seen = Group.objects.get_from_cache(id=self.group.id).times_seen
+        times_seen_incr = 5
+        self.buf.incr(
+            Group,
+            {"times_seen": times_seen_incr},
+            {"pk": self.group.id},
+            {"last_seen": timezone.now()},
+        )
+        with self.tasks(), mock.patch("sentry.buffer", self.buf):
+            self.buf.process_pending()
+        group = Group.objects.get_from_cache(id=self.group.id)
+        assert group.times_seen == orig_times_seen + times_seen_incr
+
     def test_get(self):
         model = mock.Mock()
         model.__name__ = "Mock"