Merge pull request #325 from spoo-me/fix/web-risk-budget-fail-closed

fix(safety): fail the Web Risk budget closed

Author: Zingzy

infrastructure/cache/web_risk_budget.py

@@ -10,7 +10,10 @@
 Global, not per caller. Per-caller rate limits bound one IP; this bounds
 the bill and the analyzer's headroom.
 
-Without Redis the cap is unenforced, matching every other cache here.
+Unlike the caches here, it fails closed. It shares its Redis with the
+expander's result cache, so an outage removes the cache that absorbs
+lookups and the cap that bounds them at the same moment. Unable to count
+means unable to spend.
 """
 
 from __future__ import annotations
@@ -39,18 +42,18 @@ def __init__(
         self._prefix = prefix
 
     async def take(self) -> bool:
-        """Claim one lookup. False once the day's cap is spent."""
+        """Claim one lookup. False once the day's cap is spent, and false
+        whenever the count is unavailable."""
         if self._redis is None:
-            return True
+            return False
         key = f"{self._prefix}:{datetime.now(timezone.utc):%Y-%m-%d}"
         try:
             used = await self._redis.incr(key)
             if used == 1:
                 await self._redis.expire(key, _KEY_TTL_SECONDS)
         except Exception as exc:
-            # A broken counter must not take the feature down with it.
             log.warning("web_risk_budget_error", error=str(exc))
-            return True
+            return False
         if used == self._limit + 1:
             log.warning("web_risk_budget_exhausted", limit=self._limit)
         return used <= self._limit

tests/unit/infrastructure/test_web_risk_budget.py

@@ -44,12 +44,16 @@ async def test_the_daily_key_expires_so_it_self_clears():
 
 
 @pytest.mark.asyncio
-async def test_without_redis_the_cap_is_unenforced():
-    assert await WebRiskBudget(None, limit=0).take() is True
+async def test_without_redis_nothing_is_spent():
+    assert await WebRiskBudget(None, limit=1_000).take() is False
 
 
 @pytest.mark.asyncio
-async def test_a_broken_counter_does_not_take_the_feature_down():
+async def test_a_broken_counter_refuses_rather_than_waving_calls_through():
+    """The counter shares its Redis with the expander's result cache, so an
+    outage drops the cache and the cap together. Unable to count means
+    unable to spend, or the quota goes exactly when it is least affordable.
+    """
     redis = AsyncMock()
     redis.incr = AsyncMock(side_effect=RuntimeError("redis down"))
-    assert await WebRiskBudget(redis, limit=1).take() is True
+    assert await WebRiskBudget(redis, limit=1_000).take() is False