From 0498979d485279a53c19f8c5b0f8059b3018f01d Mon Sep 17 00:00:00 2001 From: Przemyslaw Palyska Date: Sat, 2 May 2026 16:24:52 +0100 Subject: [PATCH] =?UTF-8?q?fix(quota=5Fsession):=20=5Fbuild=5Flimiter=20?= =?UTF-8?q?=E2=80=94=20pass=20rates=20list=20as=20single=20arg,=20not=20un?= =?UTF-8?q?packed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pyrate-limiter's Limiter(arg) takes a SINGLE positional arg (Rate or List[Rate]). The previous Limiter(*rates) unpacking silently dropped the daily-budget Rate, leaving only the per-minute rate enforced. Verified by inspecting bucket_factory.get_buckets()[0].rates after fix — both 60/60000ms and 800/86400000ms rates present. Discovered by wojak-wojtek s37 day 6 dispatch when adopting QuotaSession across 4 Finnhub fetchers (60/min + 800/day budget) — daily limit was not firing in production. Adds 3 regression tests in test_quota_session.py that capture Limiter constructor args via a fake module to lock in the single-arg contract. Co-Authored-By: Claude Opus 4.7 (1M context) --- scraperx/quota_session.py | 11 +++++- tests/test_quota_session.py | 74 +++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/scraperx/quota_session.py b/scraperx/quota_session.py index ba9d190..000ced2 100644 --- a/scraperx/quota_session.py +++ b/scraperx/quota_session.py @@ -147,7 +147,14 @@ def _build_session(self) -> Any: ) def _build_limiter(self, rates: list[Any]) -> Any | None: - """Build the pyrate-limiter Limiter, or return None if no rates given.""" + """Build the pyrate-limiter Limiter, or return None if no rates given. + + Note: pyrate-limiter's ``Limiter(...)`` constructor takes a SINGLE + positional arg (Rate or List[Rate]). Passing ``Limiter(*rates)`` + unpacks the list into multiple positional args, which silently drops + every rate after the first — the per-minute rate is enforced but the + daily-budget rate is not. Always pass the list as a single arg. + """ if not rates: return None try: @@ -157,7 +164,7 @@ def _build_limiter(self, rates: list[Any]) -> Any | None: "QuotaSession needs the optional `pyrate-limiter` package. " "Install with `pip install pyrate-limiter>=4`." ) from e - return Limiter(*rates) if len(rates) > 1 else Limiter(rates[0]) + return Limiter(rates) if len(rates) > 1 else Limiter(rates[0]) def _acquire(self) -> None: """Block until both rate buckets have a free token. No-op if no limiter. diff --git a/tests/test_quota_session.py b/tests/test_quota_session.py index b5fcfdd..f5e1a3c 100644 --- a/tests/test_quota_session.py +++ b/tests/test_quota_session.py @@ -143,3 +143,77 @@ def ratelimit(self, *a, **kw): sess = _make_session_with_limiter(monkeypatch, _Boom()) with pytest.raises(RuntimeError, match="ratelimit"): sess._acquire() + + +# --------------------------------------------------------------------------- +# _build_limiter — guard against the rates-unpacking regression +# --------------------------------------------------------------------------- + + +def test_build_limiter_passes_multi_rate_list_as_single_arg(monkeypatch): + """Regression: ``Limiter(*rates)`` silently drops every rate after the first. + + pyrate-limiter's ``Limiter(arg)`` accepts a single positional arg — either + a Rate or a List[Rate]. Unpacking the list into multiple positional args + leaves only the FIRST rate enforced, so callers wiring "60/min AND 800/day" + get the daily budget silently dropped. + + We capture the constructor args of a fake Limiter and assert the list + arrives intact. + """ + captured: dict[str, Any] = {} + + class _FakeLimiter: + def __init__(self, *args, **kwargs): + captured["args"] = args + captured["kwargs"] = kwargs + + # Replace pyrate_limiter.Limiter at the import site + import sys + fake_mod = type(sys)("pyrate_limiter") + fake_mod.Limiter = _FakeLimiter # type: ignore[attr-defined] + monkeypatch.setitem(sys.modules, "pyrate_limiter", fake_mod) + + monkeypatch.setattr(QuotaSession, "_build_session", lambda self: _NullSession()) + + rate_a = object() + rate_b = object() + sess = QuotaSession( + cache_path="/tmp/qs-test.sqlite", + rates=[rate_a, rate_b], + ) + # The wrapper still constructs a session even though we stubbed _build_session; + # what matters is the captured args from _build_limiter. + assert sess is not None + assert captured["args"] == ([rate_a, rate_b],), ( + f"Expected Limiter to receive the rates list as a single positional arg, " + f"got {captured['args']!r}. If this assertion fires with two separate " + f"rate objects, the multi-rate unpacking regression is back." + ) + + +def test_build_limiter_passes_single_rate_directly(monkeypatch): + """Single-rate path stays as ``Limiter(rate)`` (not wrapped in a list).""" + captured: dict[str, Any] = {} + + class _FakeLimiter: + def __init__(self, *args, **kwargs): + captured["args"] = args + + import sys + fake_mod = type(sys)("pyrate_limiter") + fake_mod.Limiter = _FakeLimiter # type: ignore[attr-defined] + monkeypatch.setitem(sys.modules, "pyrate_limiter", fake_mod) + + monkeypatch.setattr(QuotaSession, "_build_session", lambda self: _NullSession()) + + rate_only = object() + QuotaSession(cache_path="/tmp/qs-test.sqlite", rates=[rate_only]) + assert captured["args"] == (rate_only,) + + +def test_build_limiter_returns_none_for_empty_rates(monkeypatch): + """No rates → no Limiter (and pyrate-limiter is never imported).""" + monkeypatch.setattr(QuotaSession, "_build_session", lambda self: _NullSession()) + sess = QuotaSession(cache_path="/tmp/qs-test.sqlite", rates=[]) + assert sess._limiter is None