Просмотр исходного кода

fix(projectconfig): Invalidation task deals with deleted project (#35668)

The invalidation task does get scheduled for every trigger and this
means it can sometimes run for a project-scoped trigger when the
project has also already been deleted.  This happens rather a lot
really, see SENTRY-T1R.

So we fix the database query to gracefully handle the deleted project
and skip deleted projects.  This may still leave some race conditions
around as the computation itself also does a number of database
queries which could fail as well if a project is being deleted
concurrently.  We'll have to wait and see what kind of errors surface
there.

Additionally this removes the `trigger` parameter from the
build_project_config task as this is essentially unused.

Fixes SENTRY-T1R
Floris Bruynooghe 2 лет назад
Родитель
Сommit
ff5d57434d

+ 1 - 4
src/sentry/api/endpoints/relay/project_configs.py

@@ -138,10 +138,7 @@ class RelayProjectConfigsEndpoint(Endpoint):
         if cached_config:
         if cached_config:
             return cached_config
             return cached_config
 
 
-        schedule_build_project_config(
-            public_key=public_key,
-            trigger="project_config.post_v3",
-        )
+        schedule_build_project_config(public_key=public_key)
         return None
         return None
 
 
     def _post_by_key(self, request: Request, full_config_requested):
     def _post_by_key(self, request: Request, full_config_requested):

+ 6 - 8
src/sentry/tasks/relay.py

@@ -176,7 +176,7 @@ def schedule_update_config_cache(
     soft_time_limit=25 * 60,  # 25mins
     soft_time_limit=25 * 60,  # 25mins
     time_limit=25 * 60 + 5,
     time_limit=25 * 60 + 5,
 )
 )
-def build_project_config(public_key=None, trigger=None, **kwargs):
+def build_project_config(public_key=None, **kwargs):
     """Build a project config and put it in the Redis cache.
     """Build a project config and put it in the Redis cache.
 
 
     This task is used to compute missing project configs, it is aggressively
     This task is used to compute missing project configs, it is aggressively
@@ -192,7 +192,6 @@ def build_project_config(public_key=None, trigger=None, **kwargs):
         validate_args(public_key=public_key)
         validate_args(public_key=public_key)
 
 
         sentry_sdk.set_tag("public_key", public_key)
         sentry_sdk.set_tag("public_key", public_key)
-        sentry_sdk.set_tag("update_reason", trigger)
         sentry_sdk.set_context("kwargs", kwargs)
         sentry_sdk.set_context("kwargs", kwargs)
 
 
         from sentry.models import ProjectKey
         from sentry.models import ProjectKey
@@ -217,7 +216,7 @@ def build_project_config(public_key=None, trigger=None, **kwargs):
         )
         )
 
 
 
 
-def schedule_build_project_config(public_key=None, trigger="build"):
+def schedule_build_project_config(public_key=None):
     """Schedule the `build_project_config` with debouncing applied.
     """Schedule the `build_project_config` with debouncing applied.
 
 
     See documentation of `build_project_config` for documentation of parameters.
     See documentation of `build_project_config` for documentation of parameters.
@@ -230,16 +229,16 @@ def schedule_build_project_config(public_key=None, trigger="build"):
     ):
     ):
         metrics.incr(
         metrics.incr(
             "relay.projectconfig_cache.skipped",
             "relay.projectconfig_cache.skipped",
-            tags={"reason": "debounce", "update_reason": trigger},
+            tags={"reason": "debounce"},
         )
         )
         # If this task is already in the queue, do not schedule another task.
         # If this task is already in the queue, do not schedule another task.
         return
         return
 
 
     metrics.incr(
     metrics.incr(
         "relay.projectconfig_cache.scheduled",
         "relay.projectconfig_cache.scheduled",
-        tags={"update_reason": trigger, "task": "build"},
+        tags={"task": "build"},
     )
     )
-    build_project_config.delay(public_key=public_key, trigger=trigger)
+    build_project_config.delay(public_key=public_key)
 
 
     # Checking if the project is debounced and debouncing it are two separate
     # Checking if the project is debounced and debouncing it are two separate
     # actions that aren't atomic. If the process marks a project as debounced
     # actions that aren't atomic. If the process marks a project as debounced
@@ -284,8 +283,7 @@ def compute_configs(organization_id=None, project_id=None, public_key=None):
         for key in ProjectKey.objects.filter(project__in=projects):
         for key in ProjectKey.objects.filter(project__in=projects):
             configs[key.public_key] = None
             configs[key.public_key] = None
     elif project_id:
     elif project_id:
-        projects = [Project.objects.get(id=project_id)]
-        for key in ProjectKey.objects.filter(project__in=projects):
+        for key in ProjectKey.objects.filter(project_id=project_id):
             configs[key.public_key] = compute_projectkey_config(key)
             configs[key.public_key] = compute_projectkey_config(key)
     elif public_key:
     elif public_key:
         try:
         try:

+ 26 - 9
tests/sentry/tasks/test_relay.py

@@ -7,6 +7,7 @@ from sentry.relay.projectconfig_cache.redis import RedisProjectConfigCache
 from sentry.relay.projectconfig_debounce_cache.redis import RedisProjectConfigDebounceCache
 from sentry.relay.projectconfig_debounce_cache.redis import RedisProjectConfigDebounceCache
 from sentry.tasks.relay import (
 from sentry.tasks.relay import (
     build_project_config,
     build_project_config,
+    invalidate_project_config,
     schedule_build_project_config,
     schedule_build_project_config,
     schedule_invalidate_project_config,
     schedule_invalidate_project_config,
 )
 )
@@ -84,16 +85,10 @@ def test_debounce(
 
 
     monkeypatch.setattr("sentry.tasks.relay.build_project_config.apply_async", apply_async)
     monkeypatch.setattr("sentry.tasks.relay.build_project_config.apply_async", apply_async)
 
 
-    schedule_build_project_config(
-        public_key=default_projectkey.public_key, trigger="first_schedule"
-    )
-    schedule_build_project_config(
-        public_key=default_projectkey.public_key, trigger="second_schedule"
-    )
+    schedule_build_project_config(public_key=default_projectkey.public_key)
+    schedule_build_project_config(public_key=default_projectkey.public_key)
 
 
-    assert tasks == [
-        {"public_key": default_projectkey.public_key, "trigger": "first_schedule"},
-    ]
+    assert tasks == [{"public_key": default_projectkey.public_key}]
 
 
 
 
 @pytest.mark.django_db
 @pytest.mark.django_db
@@ -166,6 +161,28 @@ def test_project_get_option_does_not_reload(default_project, task_runner, monkey
     assert not build_project_config.called
     assert not build_project_config.called
 
 
 
 
+@pytest.mark.django_db
+def test_invalidation_project_deleted(default_project, task_runner, redis_cache):
+    # Ensure we have a ProjectKey
+    project_key = next(_cache_keys_for_project(default_project))
+    assert project_key
+
+    # Ensure we have a config in the cache.
+    build_project_config(public_key=project_key)
+    assert redis_cache.get(project_key)["disabled"] is False
+
+    project_id = default_project.id
+
+    # Delete the project normally, this will delete it from the cache
+    with task_runner():
+        default_project.delete()
+    assert redis_cache.get(project_key) is None
+
+    # Duplicate invoke the invalidation task, this needs to be fine with the missing project.
+    invalidate_project_config(project_id=project_id, trigger="testing-double-delete")
+    assert redis_cache.get(project_key) is None
+
+
 @pytest.mark.django_db
 @pytest.mark.django_db
 def test_projectkeys(default_project, task_runner, redis_cache):
 def test_projectkeys(default_project, task_runner, redis_cache):
     # When a projectkey is deleted the invalidation task should be triggered and the project
     # When a projectkey is deleted the invalidation task should be triggered and the project