fix: RateLimitMiddleware._buckets is class-level so reload-safe

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.
This commit is contained in:
Nora
2026-07-07 13:56:49 -06:00
parent 83a31b6fea
commit 9c0aa577fb
3 changed files with 94 additions and 16 deletions
+20 -15
View File
@@ -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()
# ---------------------------------------------------------------------------