Cache repeated thumbnail failures within configured TTL (#4249)

* Add pytest-pook and move pytest config into pyproject.toml

* Remove unnecessary custom async test handling for image proxy

* Remove unnecessary pook mock match assertions

pytest-pook handles this check automatically

* Cache repeated thumbnail failures within configured TTL

* Fix media info interface to match actual usage

* Fix documentation string

* Fix typo

Co-authored-by: zack <6351754+zackkrida@users.noreply.github.com>

---------

Co-authored-by: zack <6351754+zackkrida@users.noreply.github.com>

api/api/utils/image_proxy/__init__.py

@@ -1,7 +1,10 @@
 import logging
 from dataclasses import dataclass
-from typing import Literal
+from datetime import timedelta
+from functools import wraps
+from typing import Literal, Type
 from urllib.parse import urlparse
+from uuid import UUID
 
 from django.conf import settings
 from django.http import HttpResponse
@@ -11,6 +14,7 @@
 import django_redis
 from aiohttp.client_exceptions import ClientResponseError
 from asgiref.sync import sync_to_async
+from redis.client import Redis
 from redis.exceptions import ConnectionError
 
 from api.utils.aiohttp import get_aiohttp_session
@@ -39,7 +43,7 @@
 @dataclass
 class MediaInfo:
     media_provider: str
-    media_identifier: str
+    media_identifier: UUID
     image_url: str
 
 
@@ -120,9 +124,90 @@ def _tally_client_response_errors(tallies, month: str, domain: str, status: int)
         logger.warning("Redis connect failed, thumbnail HTTP errors not tallied.")
 
 
+# thmbfail == THuMBnail FAILures; this key path will exist for every thumbnail requested, so it needs to be space efficient
+FAILURE_CACHE_KEY_TEMPLATE = "thmbfail:{ident}"
+
+
+def _cache_repeated_failures(_get):
+    """
+    Wrap ``image_proxy.get`` to cache repeated upstream failures
+    and avoid re-requesting images likely to fail.
+
+    Do this by incrementing a counter for each media identifier each time the inner request
+    fails. Before making thumbnail requests, check this counter. If it is above the configured
+    threshold, assume the request will fail again, and eagerly return a failed response without
+    sending the request upstream.
+
+    Additionally, if the request succeeds and the failure count is not 0, decrement the counter
+    to reflect the successful response, accounting for thumbnails that were temporarily flaky,
+    while still allowing them to get temporarily cached as a failure if additional requests fail
+    and push the counter over the threshold.
+    """
+    logger = parent_logger.getChild("_cache_repeated_failures")
+
+    @wraps(_get)
+    async def do_cache(*args, **kwargs):
+        media_info: MediaInfo = args[0]
+        compressed_ident = str(media_info.media_identifier).replace("-", "")
+        redis_key = FAILURE_CACHE_KEY_TEMPLATE.format(ident=compressed_ident)
+        tallies: Redis = django_redis.get_redis_connection("tallies")
+
+        try:
+            cached_failure_count = await sync_to_async(tallies.get)(
+                redis_key,
+            )
+            cached_failure_count = (
+                int(cached_failure_count) if cached_failure_count is not None else 0
+            )
+        except ConnectionError:
+            # Ignore the connection error, treat it like it's never been cached
+            cached_failure_count = 0
+
+        if cached_failure_count > settings.THUMBNAIL_FAILURE_CACHE_TOLERANCE:
+            logger.info(
+                "%s thumbnail is too flaky, using cached failure response.",
+                media_info.media_identifier,
+            )
+            raise UpstreamThumbnailException("Thumbnail unavailable from provider.")
+
+        try:
+            response = await _get(*args, **kwargs)
+            if cached_failure_count > 0:
+                # Decrement the key
+                # Do not delete it, because if it isn't 0, then it has failed before
+                # meaning we should continue to monitor it within the cache window
+                # in case the upstream is flaky and eventually goes over the tolerance
+                try:
+                    await sync_to_async(tallies.decr)(redis_key)
+                    await sync_to_async(tallies.expire)(
+                        redis_key, settings.THUMBNAIL_FAILURE_CACHE_WINDOW_SECONDS
+                    )
+                except ConnectionError:
+                    logger.warning(
+                        "Redis connect failed, thumbnail failure not decremented."
+                    )
+            return response
+        except:
+            try:
+                await sync_to_async(tallies.incr)(redis_key)
+                # Call expire each time the key is incremented
+                # This pushes expiration out each time a new failure is cached
+                await sync_to_async(tallies.expire)(
+                    redis_key, settings.THUMBNAIL_FAILURE_CACHE_WINDOW_SECONDS
+                )
+            except ConnectionError:
+                logger.warning(
+                    "Redis connect failed, thumbnail failure not incremented."
+                )
+            raise
+
+    return do_cache
+
+
 _UPSTREAM_TIMEOUT = aiohttp.ClientTimeout(15)
 
 
+@_cache_repeated_failures
 async def get(
     media_info: MediaInfo,
     request_config: RequestConfig = RequestConfig(),

api/conf/settings/thumbnails.py

@@ -1,3 +1,5 @@
+from datetime import timedelta
+
 from decouple import config
 
 
@@ -15,3 +17,15 @@
 # to cast them in assertions to match the parsed param types)
 THUMBNAIL_WIDTH_PX = config("THUMBNAIL_WIDTH_PX", default="600")
 THUMBNAIL_QUALITY = config("THUMBNAIL_JPG_QUALITY", default="80")
+
+# The length of time to cache repeated thumbnail failures
+THUMBNAIL_FAILURE_CACHE_WINDOW_SECONDS = config(
+    "THUMBNAIL_FAILURE_CACHE_WINDOW_SECONDS",
+    default=int(timedelta(days=2).total_seconds()),
+    cast=int,
+)
+
+# The number of times to try a thumbnail request before caching a failure
+THUMBNAIL_FAILURE_CACHE_TOLERANCE = config(
+    "THUMBNAIL_FAILURE_CACHE_TOLERANCE", default=2, cast=int
+)

api/pyproject.toml

@@ -60,7 +60,26 @@ test = [
   "pook @ git+https://github.com/h2non/pook.git@master",
   "pytest >=7.4.4, <8",
   "pytest-django >=4.8.0, <5",
+  "pytest-pook>=1.0.0",
   "pytest-raises >=0.11, <0.12",
   "pytest-sugar >=0.9.7, <0.10",
   "schemathesis >=3.25.6, <4",
 ]
+
+[tool.pytest.ini_options]
+DJANGO_SETTINGS_MODULE = "conf.settings"
+
+pythonpath = "."
+
+filterwarnings = [
+  # Ignore warnings related to unverified HTTPS requests.
+  # Reason: This warning is suppressed to avoid raising warnings when making HTTP requests
+  # to servers with invalid or self-signed SSL certificates. It allows the tests to proceed
+  # without being interrupted by these warnings.
+  "ignore:Unverified HTTPS request",
+
+  # Ignore warnings about the upcoming change in the default value of 'USE_TZ' setting in Django 5.0.
+  # Reason: The reason this warning is suppressed is actually because we already set USE_TZ to True.
+  # Since no changes are required on our part, we can safely ignore this to declutter the logs.
+  "ignore:The default value of USE_TZ will change from False to True in Django 5.0",
+]