diff --git a/src/sentry/models/releaseenvironment.py b/src/sentry/models/releaseenvironment.py index acb0ae86be09..e18a2da0a2ce 100644 --- a/src/sentry/models/releaseenvironment.py +++ b/src/sentry/models/releaseenvironment.py @@ -1,5 +1,3 @@ -from datetime import timedelta - from django.db import models from django.utils import timezone @@ -13,6 +11,7 @@ ) from sentry.utils import metrics from sentry.utils.cache import cache +from sentry.utils.last_seen import try_bump_last_seen @cell_silo_model @@ -63,16 +62,15 @@ 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 - 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 not created: + try_bump_last_seen( + model_class=cls, + instance=instance, + datetime=datetime, + bump_key=f"releaseenv_bump:{instance.id}", + cache_key=cache_key, + metrics_tags=metric_tags, + ) else: metric_tags["bumped"] = "false" diff --git a/src/sentry/models/releaseprojectenvironment.py b/src/sentry/models/releaseprojectenvironment.py index eec01d046b28..309ffdd0e5b6 100644 --- a/src/sentry/models/releaseprojectenvironment.py +++ b/src/sentry/models/releaseprojectenvironment.py @@ -1,6 +1,6 @@ from __future__ import annotations -from datetime import datetime, timedelta +from datetime import datetime from enum import Enum from typing import TypedDict @@ -17,6 +17,7 @@ ) from sentry.utils import metrics from sentry.utils.cache import cache +from sentry.utils.last_seen import try_bump_last_seen class ReleaseStages(str, Enum): @@ -82,14 +83,15 @@ 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 - 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 not created: + try_bump_last_seen( + model_class=cls, + instance=instance, + datetime=datetime, + bump_key=f"releaseprojectenv_bump:{instance.id}", + cache_key=cache_key, + metrics_tags=metrics_tags, + ) else: metrics_tags["bumped"] = "false" diff --git a/src/sentry/utils/last_seen.py b/src/sentry/utils/last_seen.py new file mode 100644 index 000000000000..62f29714b51a --- /dev/null +++ b/src/sentry/utils/last_seen.py @@ -0,0 +1,45 @@ +from __future__ import annotations + +from datetime import datetime as dt +from datetime import timedelta +from typing import Any, Protocol + +from django.core.cache import cache +from django.db.utils import OperationalError + + +class HasLastSeen(Protocol): + id: int + last_seen: dt + + +def try_bump_last_seen( + *, + model_class: Any, + instance: HasLastSeen, + datetime: dt, + bump_key: str, + cache_key: str, + metrics_tags: dict[str, str], +) -> bool: + """Throttled last_seen bump — at most once per 60s per row via a cache-based lock.""" + if instance.last_seen >= datetime - timedelta(seconds=60): + metrics_tags["bumped"] = "false" + return False + + if not cache.add(bump_key, "1", timeout=60): + metrics_tags["bumped"] = "skipped" + return False + + try: + model_class.objects.filter(id=instance.id, last_seen__lt=datetime).update( + last_seen=datetime + ) + except OperationalError: + metrics_tags["bumped"] = "error" + return False + + instance.last_seen = datetime + cache.set(cache_key, instance, 3600) + metrics_tags["bumped"] = "true" + return True diff --git a/tests/sentry/models/test_releaseenvironment.py b/tests/sentry/models/test_releaseenvironment.py index d572797573bb..209de37f910d 100644 --- a/tests/sentry/models/test_releaseenvironment.py +++ b/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 diff --git a/tests/sentry/models/test_releaseprojectenvironment.py b/tests/sentry/models/test_releaseprojectenvironment.py index 375b01f04e7b..dd2dca76adef 100644 --- a/tests/sentry/models/test_releaseprojectenvironment.py +++ b/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