fix(releases): Prevent row-lock contention on last_seen bump

src/sentry/models/releaseenvironment.py

@@ -1,6 +1,7 @@
 from datetime import timedelta
 
 from django.db import models
+from django.db.utils import OperationalError
 from django.utils import timezone
 
 from sentry.backup.scopes import RelocationScope
@@ -63,16 +64,21 @@ def _get_or_create_impl(cls, project, release, environment, datetime, metric_tag
 
         metric_tags["created"] = "true" if created else "false"
 
-        # TODO(dcramer): this would be good to buffer, but until then we minimize
-        # updates to once a minute, and allow Postgres to optimistically skip
-        # it even if we can't
+        bump_key = f"releaseenv_bump:{instance.id}"
         if not created and instance.last_seen < datetime - timedelta(seconds=60):
-            metric_tags["bumped"] = "true"
-            cls.objects.filter(
-                id=instance.id, last_seen__lt=datetime - timedelta(seconds=60)
-            ).update(last_seen=datetime)
-            instance.last_seen = datetime
-            cache.set(cache_key, instance, 3600)
+            if cache.add(bump_key, "1", timeout=60):
+                try:
+                    cls.objects.filter(id=instance.id, last_seen__lt=datetime).update(
+                        last_seen=datetime
+                    )
+                except OperationalError:
+                    metric_tags["bumped"] = "error"
+                    return instance
+                instance.last_seen = datetime
+                cache.set(cache_key, instance, 3600)
+                metric_tags["bumped"] = "true"
+            else:
+                metric_tags["bumped"] = "skipped"
         else:
             metric_tags["bumped"] = "false"
 

src/sentry/models/releaseprojectenvironment.py

@@ -5,6 +5,7 @@
 from typing import TypedDict
 
 from django.db import models
+from django.db.utils import OperationalError
 from django.utils import timezone
 
 from sentry.backup.scopes import RelocationScope
@@ -82,14 +83,24 @@ def _get_or_create_impl(cls, release, project, environment, datetime, metrics_ta
 
         metrics_tags["created"] = "true" if created else "false"
 
-        # Same as releaseenvironment model. Minimizes last_seen updates to once a minute
+        # Same as releaseenvironment model. Minimizes last_seen updates to once a minute.
+        # The cache.add acts as a distributed lock so only one worker attempts the
+        # DB update per row per 60s, avoiding row-lock contention on hot releases.
+        bump_key = f"releaseprojectenv_bump:{instance.id}"
         if not created and instance.last_seen < datetime - timedelta(seconds=60):
-            cls.objects.filter(
-                id=instance.id, last_seen__lt=datetime - timedelta(seconds=60)
-            ).update(last_seen=datetime)
-            instance.last_seen = datetime
-            cache.set(cache_key, instance, 3600)
-            metrics_tags["bumped"] = "true"
+            if cache.add(bump_key, "1", timeout=60):
+                try:
+                    cls.objects.filter(id=instance.id, last_seen__lt=datetime).update(
+                        last_seen=datetime
+                    )
+                except OperationalError:
+                    metrics_tags["bumped"] = "error"
+                    return instance
+                instance.last_seen = datetime
+                cache.set(cache_key, instance, 3600)
+                metrics_tags["bumped"] = "true"
+            else:
+                metrics_tags["bumped"] = "skipped"
         else:
             metrics_tags["bumped"] = "false"
 

tests/sentry/models/test_releaseenvironment.py

@@ -1,11 +1,14 @@
 from datetime import timedelta
+from unittest.mock import patch
 
+from django.db.utils import OperationalError
 from django.utils import timezone
 
 from sentry.models.environment import Environment
 from sentry.models.release import Release
 from sentry.models.releaseenvironment import ReleaseEnvironment
 from sentry.testutils.cases import TestCase
+from sentry.utils.cache import cache
 
 
 class GetOrCreateTest(TestCase):
@@ -52,3 +55,68 @@ def test_simple(self) -> None:
         )
         assert relenv.id == relenv2.id
         assert ReleaseEnvironment.objects.get(id=relenv.id).last_seen == relenv2.last_seen
+
+    def test_bump_skipped_when_cache_lock_held(self) -> None:
+        """Second worker skips the DB update when another worker already holds the bump lock."""
+        project = self.create_project(name="foo")
+        datetime_now = timezone.now()
+
+        release = Release.objects.create(organization_id=project.organization_id, version="abcdef")
+        release.add_project(project)
+        env = Environment.objects.create(organization_id=project.organization_id, name="prod")
+
+        ReleaseEnvironment.get_or_create(
+            project=project, release=release, environment=env, datetime=datetime_now
+        )
+
+        datetime_next = datetime_now + timedelta(days=1)
+
+        re = ReleaseEnvironment.objects.get(
+            organization_id=project.organization_id, release=release, environment=env
+        )
+        bump_key = f"releaseenv_bump:{re.id}"
+        cache.set(bump_key, "1", timeout=60)
+
+        with patch.object(
+            ReleaseEnvironment.objects,
+            "filter",
+            wraps=ReleaseEnvironment.objects.filter,
+        ) as mock_filter:
+            result = ReleaseEnvironment.get_or_create(
+                project=project, release=release, environment=env, datetime=datetime_next
+            )
+            for call in mock_filter.call_args_list:
+                assert "last_seen__lt" not in (call.kwargs or {})
+
+        re.refresh_from_db()
+        assert re.last_seen == datetime_now
+        assert result is not None
+
+    def test_bump_survives_operational_error(self) -> None:
+        """OperationalError on the UPDATE doesn't prevent the instance from being returned."""
+        project = self.create_project(name="foo")
+        datetime_now = timezone.now()
+
+        release = Release.objects.create(organization_id=project.organization_id, version="abcdef")
+        release.add_project(project)
+        env = Environment.objects.create(organization_id=project.organization_id, name="prod")
+
+        ReleaseEnvironment.get_or_create(
+            project=project, release=release, environment=env, datetime=datetime_now
+        )
+
+        datetime_next = datetime_now + timedelta(days=1)
+
+        with patch("sentry.models.releaseenvironment.ReleaseEnvironment.objects") as mock_manager:
+            mock_qs = mock_manager.filter.return_value
+            mock_qs.update.side_effect = OperationalError("canceling statement due to user request")
+
+            result = ReleaseEnvironment.get_or_create(
+                project=project, release=release, environment=env, datetime=datetime_next
+            )
+
+        assert result is not None
+        re = ReleaseEnvironment.objects.get(
+            organization_id=project.organization_id, release=release, environment=env
+        )
+        assert re.last_seen == datetime_now

tests/sentry/models/test_releaseprojectenvironment.py

@@ -1,11 +1,14 @@
 from datetime import timedelta
+from unittest.mock import patch
 
+from django.db.utils import OperationalError
 from django.utils import timezone
 
 from sentry.models.environment import Environment
 from sentry.models.release import Release
 from sentry.models.releaseprojectenvironment import ReleaseProjectEnvironment
 from sentry.testutils.cases import TestCase
+from sentry.utils.cache import cache
 
 
 class GetOrCreateTest(TestCase):
@@ -83,3 +86,68 @@ def test_no_update_too_close(self) -> None:
         )
         assert release_project_env.first_seen == self.datetime_now
         assert release_project_env.last_seen == self.datetime_now
+
+    def test_bump_skipped_when_cache_lock_held(self) -> None:
+        """Second worker skips the DB update when another worker already holds the bump lock."""
+        ReleaseProjectEnvironment.get_or_create(
+            project=self.project,
+            release=self.release,
+            environment=self.environment,
+            datetime=self.datetime_now,
+        )
+
+        datetime_next = self.datetime_now + timedelta(days=1)
+
+        rpe = ReleaseProjectEnvironment.objects.get(
+            project=self.project, release=self.release, environment=self.environment
+        )
+        bump_key = f"releaseprojectenv_bump:{rpe.id}"
+        cache.set(bump_key, "1", timeout=60)
+
+        with patch.object(
+            ReleaseProjectEnvironment.objects,
+            "filter",
+            wraps=ReleaseProjectEnvironment.objects.filter,
+        ) as mock_filter:
+            result = ReleaseProjectEnvironment.get_or_create(
+                project=self.project,
+                release=self.release,
+                environment=self.environment,
+                datetime=datetime_next,
+            )
+            for call in mock_filter.call_args_list:
+                assert "last_seen__lt" not in (call.kwargs or {})
+
+        rpe.refresh_from_db()
+        assert rpe.last_seen == self.datetime_now
+        assert result is not None
+
+    def test_bump_survives_operational_error(self) -> None:
+        """OperationalError on the UPDATE doesn't prevent the instance from being returned."""
+        ReleaseProjectEnvironment.get_or_create(
+            project=self.project,
+            release=self.release,
+            environment=self.environment,
+            datetime=self.datetime_now,
+        )
+
+        datetime_next = self.datetime_now + timedelta(days=1)
+
+        with patch(
+            "sentry.models.releaseprojectenvironment.ReleaseProjectEnvironment.objects"
+        ) as mock_manager:
+            mock_qs = mock_manager.filter.return_value
+            mock_qs.update.side_effect = OperationalError("canceling statement due to user request")
+
+            result = ReleaseProjectEnvironment.get_or_create(
+                project=self.project,
+                release=self.release,
+                environment=self.environment,
+                datetime=datetime_next,
+            )
+
+        assert result is not None
+        rpe = ReleaseProjectEnvironment.objects.get(
+            project=self.project, release=self.release, environment=self.environment
+        )
+        assert rpe.last_seen == self.datetime_now