Nora
9c0aa577fb
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.
56 lines
2.4 KiB
Python
56 lines
2.4 KiB
Python
"""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) |