From 9c0aa577fb667486e996400de645371eba9c2797 Mon Sep 17 00:00:00 2001 From: Nora Date: Tue, 7 Jul 2026 13:56:49 -0600 Subject: [PATCH] fix: RateLimitMiddleware._buckets is class-level so reload-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 20 pre-existing pytest failures were caused by tests/test_api.py calling importlib.reload(cyclone.api) mid-suite. Reload creates a NEW FastAPI app with a NEW RateLimitMiddleware whose private _buckets dict is independent of the OLD instance. Tests that imported 'from cyclone.api import app' at module load kept referencing the OLD app, so their requests accumulated in the orphaned bucket which the conftest reset never cleared. After ~300 requests, the orphaned bucket tripped the limiter and these tests got spurious 429s: - test_existing_endpoints_require_auth (6) - test_inbox_endpoints (8) - test_inbox_endpoints_sp7 (1) - test_list_endpoint_counts (4) Hoisting _buckets to a class-level dict makes one RateLimitMiddleware._buckets.clear() (in conftest reset) reach every instance — current, stale, post-reload — eliminating the orphaned-bucket leak. Per-instance _lock stays per-instance since it guards mutation of the shared dict. Verified stable: 3 consecutive full-suite runs all pass with 1435 passed, 10 skipped, 0 failed in ~82s each. --- backend/src/cyclone/security.py | 19 ++++++- backend/tests/conftest.py | 35 +++++++----- .../tests/test_rate_limit_shared_buckets.py | 56 +++++++++++++++++++ 3 files changed, 94 insertions(+), 16 deletions(-) create mode 100644 backend/tests/test_rate_limit_shared_buckets.py diff --git a/backend/src/cyclone/security.py b/backend/src/cyclone/security.py index 58f581a..39a1827 100644 --- a/backend/src/cyclone/security.py +++ b/backend/src/cyclone/security.py @@ -196,8 +196,13 @@ class RateLimitMiddleware: On unexpected errors the limiter fails OPEN — better to serve a few extra requests than to 503 every request because of a bug. + + The ``_buckets`` dict is class-level so every instance shares the + same per-IP sliding-window state. See ``__init__`` for why. """ + _buckets: dict[str, "_Bucket"] = {} + EXEMPT_PATHS = ("/api/health", "/healthz", "/readyz") def __init__( @@ -211,7 +216,19 @@ class RateLimitMiddleware: "CYCLONE_RATE_LIMIT_PER_MIN", DEFAULT_RATE_LIMIT_PER_MIN, ) self.window_s = window_s or DEFAULT_RATE_LIMIT_WINDOW_S - self._buckets: dict[str, _Bucket] = {} + # _buckets is a class-level dict so every RateLimitMiddleware + # instance shares the same per-IP sliding-window state. + # This matters because tests that call + # ``importlib.reload(cyclone.api)`` cause Starlette to rebuild + # the middleware stack with a fresh instance whose private + # bucket would otherwise be independent of the old one — and + # tests that imported ``from cyclone.api import app`` at + # module load time keep referencing the old instance, so + # their requests would accumulate in the orphaned bucket and + # trip the limiter after ~300 requests. Sharing the dict + # makes one ``_buckets.clear()`` (in the conftest's reset + # hook) clear every instance at once. + self._buckets: dict[str, _Bucket] = type(self)._buckets self._lock = threading.Lock() async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index daabf70..6824620 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -53,10 +53,15 @@ def _auto_init_db(tmp_path, monkeypatch): _api_mod.app.state.event_bus = EventBus() deps.AUTH_DISABLED = True # The rate-limit middleware keeps a per-IP sliding window in - # ``_buckets``. Without a reset between tests, later tests in a - # full-suite run get ``429 Too Many Requests`` once the testclient - # IP exhausts its 300 req/60s budget. Walk the middleware stack - # and clear the buckets so every test starts with a fresh window. + # ``_buckets`` (now a class-level dict — see + # ``cyclone.security.RateLimitMiddleware._buckets``). Without a + # reset between tests, later tests in a full-suite run get + # ``429 Too Many Requests`` once the testclient IP exhausts its + # 300 req/60s budget. Walk the middleware stack and clear the + # buckets so every test starts with a fresh window. Because the + # buckets dict is class-level, this clears every instance at + # once — important when tests like ``test_cors_extra_origins_via_env`` + # trigger ``importlib.reload(cyclone.api)`` mid-suite. # Trigger the stack build with a cheap health probe (the only # request exempt from the limiter — see RateLimitMiddleware.EXEMPT_PATHS). _reset_rate_limit_buckets(_api_mod.app) @@ -65,6 +70,7 @@ def _auto_init_db(tmp_path, monkeypatch): finally: deps.AUTH_DISABLED = False _api_mod.app.state.event_bus = None + _reset_rate_limit_buckets(_api_mod.app) db._reset_for_tests() @@ -76,23 +82,22 @@ def _reset_rate_limit_buckets(app) -> None: instance), so without a reset between tests the full suite trips the limiter after ~300 requests and later tests get 429s. + As of the SP38 followup the buckets dict is class-level, so this + helper clears every ``RateLimitMiddleware`` instance at once + (important when ``importlib.reload(cyclone.api)`` mid-suite has + created a new instance and tests still hold a stale ``app`` + reference to the old one — see + ``tests/test_rate_limit_shared_buckets.py``). + The middleware stack is only built on the first request, so we prime it with an exempt health probe before walking to the RateLimit layer. If the stack ever stops being a single chain of ``.app`` links, this helper raises AttributeError — better to fail loudly than silently leak state. """ - from fastapi.testclient import TestClient - TestClient(app).get("/api/health") - cur = app.middleware_stack - while cur is not None: - if hasattr(cur, "_buckets"): - cur._buckets.clear() - return - cur = getattr(cur, "app", None) - # No RateLimitMiddleware in the stack — nothing to reset. Should - # not happen in this codebase (security.py registers it at boot) - # but we don't want a missing reset to crash unrelated tests. + from cyclone.security import RateLimitMiddleware + # Class-level dict — one clear reaches every instance. + RateLimitMiddleware._buckets.clear() # --------------------------------------------------------------------------- diff --git a/backend/tests/test_rate_limit_shared_buckets.py b/backend/tests/test_rate_limit_shared_buckets.py new file mode 100644 index 0000000..808fc55 --- /dev/null +++ b/backend/tests/test_rate_limit_shared_buckets.py @@ -0,0 +1,56 @@ +"""Regression test: RateLimitMiddleware must share its bucket across instances. + +When ``importlib.reload(cyclone.api)`` runs (e.g. from +``test_cors_extra_origins_via_env``), Starlette rebuilds the middleware +stack with a NEW ``RateLimitMiddleware`` instance whose ``_buckets`` +dict is fresh. Tests that imported ``app`` at module load time still +reference the OLD app — and the OLD RateLimitMiddleware — so its +private ``_buckets`` dict keeps accumulating requests across the rest +of the suite. After ~300 requests it hits the rate limit and the +remaining tests get spurious 429s. + +Fix: hoist ``_buckets`` to a class-level dict so every +``RateLimitMiddleware`` instance (current + stale) shares the same +sliding-window state. The per-instance ``_lock`` stays per-instance +since it guards mutation of the shared dict. + +This test pins that invariant: two ``RateLimitMiddleware`` instances +share the same underlying bucket dict. +""" +from __future__ import annotations + +from cyclone.security import RateLimitMiddleware + + +class _DummyApp: + """Minimal ASGI app stand-in for the middleware's inner app.""" + + async def __call__(self, scope, receive, send): + pass + + +def test_rate_limit_buckets_are_shared_across_instances(): + """Two RateLimitMiddleware instances must share _buckets.""" + m1 = RateLimitMiddleware(_DummyApp()) + m2 = RateLimitMiddleware(_DummyApp()) + # After the fix: m1._buckets IS m2._buckets (same class-level dict). + # Before the fix: each instance had its own {} dict. + assert m1._buckets is m2._buckets, ( + "RateLimitMiddleware instances do NOT share their bucket dict. " + "After importlib.reload(cyclone.api), the new instance's " + "_buckets is independent of the old one — orphaned buckets " + "accumulate across tests that hold stale `app` references and " + "trip the 300 req/60s limit. Hoist _buckets to a class-level " + "dict so every instance shares state." + ) + + +def test_class_level_buckets_attribute_exists(): + """The class must declare a class-level _buckets dict.""" + # This is the structural pre-condition for the sharing invariant. + assert hasattr(RateLimitMiddleware, "_buckets"), ( + "RateLimitMiddleware must declare a class-level _buckets " + "attribute so every instance shares it. Without this, " + "importlib.reload(cyclone.api) creates an orphaned bucket." + ) + assert isinstance(RateLimitMiddleware._buckets, dict) \ No newline at end of file