diff --git a/posthog/rate_limit.py b/posthog/rate_limit.py index c79920f4a88a..0dc5a9c408a1 100644 --- a/posthog/rate_limit.py +++ b/posthog/rate_limit.py @@ -43,6 +43,12 @@ labelnames=[LABEL_TEAM_ID, LABEL_PATH, LABEL_ROUTE], ) +RATE_LIMIT_FAIL_OPEN_COUNTER = Counter( + "rate_limit_fail_open_total", + "Requests allowed through without rate-limiting because checking the limit raised an exception.", + labelnames=["scope", "exception"], +) + @lru_cache(maxsize=1) def get_team_allow_list(_ttl: int) -> list[str]: @@ -67,7 +73,15 @@ def is_rate_limit_enabled(_ttl: int) -> bool: The setting will change way less frequently than it will be called _ttl is passed an infrequently changing value to ensure the cache is invalidated after some delay """ - return get_instance_setting("RATE_LIMIT_ENABLED") + try: + return get_instance_setting("RATE_LIMIT_ENABLED") + except Exception as e: + # This used to propagate straight through allow_request (called outside any + # try/except there), turning a transient DB blip into a 500 on every throttled + # request instead of the fail-open behavior the rest of this file uses. + capture_exception(e) + RATE_LIMIT_FAIL_OPEN_COUNTER.labels(scope="is_rate_limit_enabled", exception=type(e).__name__).inc() + return False path_by_env_pattern = re.compile(r"^/api/environments/(\d+)/") @@ -236,6 +250,7 @@ def _allow_request_internal(self, request, view, *, personal_api_key_only: bool) return False except Exception as e: capture_exception(e) + RATE_LIMIT_FAIL_OPEN_COUNTER.labels(scope=self.scope, exception=type(e).__name__).inc() return True def allow_request(self, request, view): @@ -1244,6 +1259,7 @@ def allow_request(self, request, view): return True except Exception as e: capture_exception(e) + RATE_LIMIT_FAIL_OPEN_COUNTER.labels(scope=self.scope, exception=type(e).__name__).inc() return True diff --git a/posthog/test/test_rate_limit.py b/posthog/test/test_rate_limit.py index 9b32e0d04acf..2b1fe9950d08 100644 --- a/posthog/test/test_rate_limit.py +++ b/posthog/test/test_rate_limit.py @@ -37,6 +37,25 @@ ) +class TestIsRateLimitEnabled(SimpleTestCase): + def setUp(self) -> None: + rate_limit.is_rate_limit_enabled.cache_clear() + + def tearDown(self) -> None: + rate_limit.is_rate_limit_enabled.cache_clear() + + @patch("posthog.rate_limit.capture_exception") + @patch("posthog.rate_limit.get_instance_setting", side_effect=Exception("cache lookup failed for function 481")) + def test_fails_open_when_the_instance_setting_lookup_raises( + self, _get_instance_setting: Mock, capture_exception_mock: Mock + ) -> None: + # Regression guard: this call happens outside the try/except in allow_request, + # so an unhandled exception here used to 500 every throttled request instead + # of falling back to "rate limiting disabled". + self.assertFalse(rate_limit.is_rate_limit_enabled(0)) + capture_exception_mock.assert_called_once() + + class TestUserAPI(APIBaseTest): def setUp(self): super().setUp()