From b0077f800122861d1a96612adabaf29d36c905b1 Mon Sep 17 00:00:00 2001 From: Karl Date: Sun, 13 Sep 2026 14:48:15 -0400 Subject: [PATCH 1/2] fix(url,retry,errors): typed errors, a real 60s cap, no status on a local refusal (#123) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three promises the code did not keep. 1. `_url` documents ValidationError as the only thing it raises, but `_origin` and `urljoin` both leak a raw stdlib ValueError for an out-of-range port ("...:99999") and for a non-ASCII netloc whose NFKC normalisation introduces one of /?#@: ("https://℀evil.example"). `_origin` runs on base_url on EVERY request, so a misconfigured client raised a bare ValueError from the hot path. Both are now ConfigurationError naming base_url. 2. `calculate_wait_time` documents "capped at 60 seconds" and returned up to 78s: min(2 ** attempt, 60) bounded the base, then up to 30% jitter was added on top. #115 added `bounded_wait` but wired it only into the 429/Retry-After path; the 5xx and transport-error paths in both clients call this raw. Clamping inside `calculate_wait_time` fixes every call site at once and cannot drift between the sync and async clients. Jitter is preserved below saturation; the docstring now states where it saturates. 3. `ValidationError` hard-coded status_code=422, so a purely local guard refusal -- raised by `_url._reject` before any socket is opened -- reported `.status_code == 422` and `.is_client_error is True`. Anything logging or aggregating by status recorded a server response that never happened. The default stays 422 for existing callers and for a real API 422; the local guard now passes status_code=None explicitly. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo --- oilpriceapi/_url.py | 41 +++++- oilpriceapi/exceptions.py | 17 ++- oilpriceapi/retry.py | 20 ++- tests/unit/test_url_retry_hardening.py | 189 +++++++++++++++++++++++++ 4 files changed, 255 insertions(+), 12 deletions(-) create mode 100644 tests/unit/test_url_retry_hardening.py diff --git a/oilpriceapi/_url.py b/oilpriceapi/_url.py index 264461a..a2da4a5 100644 --- a/oilpriceapi/_url.py +++ b/oilpriceapi/_url.py @@ -24,7 +24,7 @@ from typing import Any, Tuple from urllib.parse import urljoin, urlsplit -from .exceptions import ValidationError +from .exceptions import ConfigurationError, ValidationError __all__ = ["resolve_api_url"] @@ -44,10 +44,26 @@ def _origin(url: str) -> Tuple[str, str, int]: - parts = urlsplit(url) - scheme = (parts.scheme or "").lower() - host = (parts.hostname or "").lower() - port = parts.port or _DEFAULT_PORTS.get(scheme, 0) + """Return ``(scheme, host, port)``, never a raw ``ValueError``. + + ``urlsplit`` and its ``.hostname`` / ``.port`` accessors raise ValueError + for an out-of-range port ("...:99999") and for a non-ASCII netloc whose + NFKC normalisation introduces one of ``/?#@:`` ("https://\u2100evil.example"). + ``_origin`` runs on ``base_url`` on EVERY request, so a misconfigured client + raised a bare stdlib exception from the hot path -- the one type this + module's docstring says cannot escape (#123). + """ + try: + parts = urlsplit(url) + scheme = (parts.scheme or "").lower() + host = (parts.hostname or "").lower() + port = parts.port or _DEFAULT_PORTS.get(scheme, 0) + except ValueError as exc: + raise ConfigurationError( + f"Cannot parse the URL {url!r}: {exc}. " + "Check the client's base_url -- it must be an absolute " + "http(s) origin such as 'https://api.oilpriceapi.com'." + ) from exc return scheme, host, port @@ -65,6 +81,10 @@ def _reject(path: Any, reason: str) -> "ValidationError": ), field="path", value=path, + # No request was sent, so there is no HTTP status to report. The 422 + # default would make this local refusal indistinguishable, to any log + # or metric keyed on status, from a 422 the API actually returned. + status_code=None, ) @@ -104,7 +124,16 @@ def resolve_api_url(base_url: str, path: Any) -> str: normalized = path if path.startswith("/") else "/" + path - url = urljoin(base_url + "/", normalized) + # urljoin parses the base, so it raises the same ValueError _origin does + # for an out-of-range port or an NFKC-invalid netloc -- and it runs first. + try: + url = urljoin(base_url + "/", normalized) + except ValueError as exc: + raise ConfigurationError( + f"Cannot parse the client's base_url {base_url!r}: {exc}. " + "It must be an absolute http(s) origin such as " + "'https://api.oilpriceapi.com'." + ) from exc if _origin(url) != _origin(base_url): raise _reject(path, "resolves to a different host than the configured base URL") diff --git a/oilpriceapi/exceptions.py b/oilpriceapi/exceptions.py index 4a7d20e..7329ede 100644 --- a/oilpriceapi/exceptions.py +++ b/oilpriceapi/exceptions.py @@ -322,14 +322,29 @@ def __str__(self) -> str: class ValidationError(OilPriceAPIError): """Raised when request validation fails (HTTP 422).""" + #: Sentinel so `status_code=None` is distinguishable from "not supplied". + _UNSET = object() + def __init__( self, message: str = "Validation error", field: Optional[str] = None, value: Optional[Any] = None, + status_code: Any = _UNSET, **kwargs: Any, ): - super().__init__(message, status_code=422, **kwargs) + # A refusal raised by a purely local guard -- `_url._reject`, which + # runs before any socket is opened -- has no HTTP status, because no + # request was ever sent. Hard-coding 422 made `.status_code` read 422 + # and `.is_client_error` read True for a request that never reached the + # network, so anything logging or aggregating by status recorded a + # server response that did not exist (#123). + # + # The default stays 422 so existing callers and a real 422 from the API + # are unaffected; a local guard passes `status_code=None` explicitly. + if status_code is ValidationError._UNSET: + status_code = 422 + super().__init__(message, status_code=status_code, **kwargs) self.field = field self.value = value diff --git a/oilpriceapi/retry.py b/oilpriceapi/retry.py index 34bf8a8..efc03eb 100644 --- a/oilpriceapi/retry.py +++ b/oilpriceapi/retry.py @@ -353,7 +353,7 @@ def calculate_wait_time(self, attempt: int) -> float: attempt: Current attempt number (0-indexed) Returns: - Wait time in seconds (capped at 60 seconds) + Wait time in seconds, never above ``MAX_WAIT_SECONDS`` (60). Examples: Without jitter: @@ -361,17 +361,27 @@ def calculate_wait_time(self, attempt: int) -> float: - Attempt 1: 2.0s - Attempt 2: 4.0s - With jitter (adds 0-30% randomization): + With jitter (adds 0-30% randomization, then clamps to 60s): - Attempt 0: 1.0-1.3s - Attempt 1: 2.0-2.6s - Attempt 2: 4.0-5.2s + - Attempt 6 and up: saturates at 60.0s + + The clamp is the point. ``min(2 ** attempt, 60)`` bounded the BASE, then + up to 30% jitter was added on top, so the documented 60s cap was + exceeded from attempt 6 onwards -- 78s at the ceiling. #115 added + ``bounded_wait`` but wired it only into the 429/Retry-After path; the + 5xx and transport-error paths in both clients call this method raw + (#123). Bounding here fixes every call site at once and cannot drift + between the sync and async clients. """ - base_wait = min(2 ** attempt, 60) + base_wait = float(min(2 ** attempt, self.MAX_WAIT_SECONDS)) if self.jitter: - # Add 0-30% random jitter to prevent synchronized retries + # Add 0-30% random jitter to prevent synchronized retries, then + # clamp -- jitter must not push the wait past the documented cap. jitter_amount = random.uniform(0, 0.3 * base_wait) - return base_wait + jitter_amount + return self.bounded_wait(base_wait + jitter_amount, base_wait) return base_wait diff --git a/tests/unit/test_url_retry_hardening.py b/tests/unit/test_url_retry_hardening.py new file mode 100644 index 0000000..bc12a67 --- /dev/null +++ b/tests/unit/test_url_retry_hardening.py @@ -0,0 +1,189 @@ +"""The remaining #123 findings: typed errors, a real 60s cap, honest status. + +Three separate promises the code does not keep: + +1. ``resolve_api_url``'s docstring says it raises ``ValidationError``. It leaks + a raw stdlib ``ValueError`` for a base URL with an out-of-range port or a + non-ASCII netloc whose NFKC normalisation introduces one of ``/?#@:``. + ``_origin`` runs on ``base_url`` on EVERY request, so this is on the hot path. +2. ``calculate_wait_time``'s docstring says "capped at 60 seconds". With jitter + it returns up to 78s, and only the 429/``Retry-After`` path was bounded by + #115 -- the 5xx and transport-error paths call it raw. +3. ``ValidationError`` hard-codes ``status_code=422``, so a purely local guard + refusal -- no request ever sent -- reports an HTTP status and + ``is_client_error is True``. Anything aggregating by status records a 422 + for a request that never reached the network. +""" + +import pytest + +from oilpriceapi._url import resolve_api_url +from oilpriceapi.exceptions import OilPriceAPIError, ValidationError +from oilpriceapi.retry import RetryStrategy + +BASE = "https://api.oilpriceapi.com" + +# urlsplit raises ValueError on both of these. +UNPARSEABLE_BASES = [ + "https://api.oilpriceapi.com:99999", + "https://℀evil.example", +] + + +# --- 1. no raw ValueError escapes the documented contract ------------------- + +@pytest.mark.parametrize("base", UNPARSEABLE_BASES) +def test_resolve_api_url_raises_typed_error_not_valueerror(base): + with pytest.raises(OilPriceAPIError): + resolve_api_url(base, "/v1/prices") + + +@pytest.mark.parametrize("base", UNPARSEABLE_BASES) +def test_resolve_api_url_never_leaks_bare_valueerror(base): + """A bare ValueError is exactly what the docstring says cannot happen.""" + try: + resolve_api_url(base, "/v1/prices") + except OilPriceAPIError: + pass + except ValueError as exc: # pragma: no cover - this is the bug + pytest.fail(f"leaked a raw ValueError: {exc}") + + +@pytest.mark.parametrize("path", ["/v1/prices:99999", "/v1/℀prices"]) +def test_unusual_paths_against_a_sane_base_still_resolve(path): + """The fix must not start refusing ordinary paths.""" + assert resolve_api_url(BASE, path).startswith(BASE) + + +# --- 2. the documented 60s cap is a real cap -------------------------------- + +def test_calculate_wait_time_never_exceeds_the_documented_cap(): + s = RetryStrategy(max_retries=20) + # 2**10 = 1024 -> base 60 -> +30% jitter = up to 78s today. + worst = max(s.calculate_wait_time(10) for _ in range(2000)) + assert worst <= RetryStrategy.MAX_WAIT_SECONDS, worst + + +@pytest.mark.parametrize("attempt", range(0, 14)) +def test_calculate_wait_time_is_bounded_at_every_attempt(attempt): + s = RetryStrategy(max_retries=20) + for _ in range(100): + w = s.calculate_wait_time(attempt) + assert 0.0 <= w <= RetryStrategy.MAX_WAIT_SECONDS, (attempt, w) + + +def test_calculate_wait_time_keeps_jitter_below_the_cap(): + """Bounding must not collapse jitter into a constant and re-create the + thundering herd the jitter exists to prevent.""" + s = RetryStrategy(max_retries=20) + small = {round(s.calculate_wait_time(1), 6) for _ in range(200)} + assert len(small) > 1, "jitter disappeared at an unsaturated attempt" + + +def test_calculate_wait_time_without_jitter_is_unchanged(): + s = RetryStrategy(max_retries=20, jitter=False) + assert s.calculate_wait_time(0) == 1 + assert s.calculate_wait_time(2) == 4 + assert s.calculate_wait_time(10) == RetryStrategy.MAX_WAIT_SECONDS + + +def test_docstring_cap_matches_the_constant(): + doc = RetryStrategy.calculate_wait_time.__doc__ or "" + assert str(int(RetryStrategy.MAX_WAIT_SECONDS)) in doc + + +# --- 3. a local refusal carries no HTTP status ------------------------------ + +def test_local_guard_refusal_has_no_http_status(): + """No request was sent, so there is no status code to report.""" + with pytest.raises(ValidationError) as exc: + resolve_api_url(BASE, "//evil.example/v1/prices") + assert exc.value.status_code is None + + +def test_local_guard_refusal_is_not_a_client_http_error(): + with pytest.raises(ValidationError) as exc: + resolve_api_url(BASE, "//evil.example/v1/prices") + assert exc.value.is_client_error is False + + +def test_server_sent_validation_error_keeps_422(): + """A real 422 from the API must still report 422.""" + err = ValidationError("bad field", field="code", status_code=422) + assert err.status_code == 422 + assert err.is_client_error is True + + +def test_validation_error_default_is_still_422_for_existing_callers(): + """Back-compat: callers that construct it bare still get the HTTP default.""" + assert ValidationError("bad").status_code == 422 + + +# --- 4. sync and async must not diverge ------------------------------------- + +def _max_sleep_over_a_retry_storm(sleeps): + return max(sleeps) if sleeps else 0.0 + + +def test_sync_client_never_sleeps_past_the_cap_on_5xx(monkeypatch): + """The 5xx path, which #115 left unbounded, on the real transport.""" + import httpx + import respx + + from oilpriceapi import OilPriceAPI + + sleeps = [] + monkeypatch.setattr("time.sleep", lambda s: sleeps.append(s)) + + with respx.mock(base_url=BASE) as mock: + mock.get("/v1/prices/latest").mock(return_value=httpx.Response(503)) + c = OilPriceAPI(api_key="k", base_url=BASE, max_retries=14) + with pytest.raises(Exception): + c.request("GET", "/v1/prices/latest") + + assert sleeps, "no retry happened; the test proves nothing" + assert _max_sleep_over_a_retry_storm(sleeps) <= RetryStrategy.MAX_WAIT_SECONDS, sleeps + + +@pytest.mark.asyncio +async def test_async_client_never_sleeps_past_the_cap_on_5xx(monkeypatch): + """Parity: identical assertion against the async client's 5xx path.""" + import asyncio + + import httpx + import respx + + from oilpriceapi import AsyncOilPriceAPI + + sleeps = [] + + async def fake_sleep(s): + sleeps.append(s) + + monkeypatch.setattr(asyncio, "sleep", fake_sleep) + + with respx.mock(base_url=BASE) as mock: + mock.get("/v1/prices/latest").mock(return_value=httpx.Response(503)) + c = AsyncOilPriceAPI(api_key="k", base_url=BASE, max_retries=14) + with pytest.raises(Exception): + await c.request("GET", "/v1/prices/latest") + + assert sleeps, "no retry happened; the test proves nothing" + assert _max_sleep_over_a_retry_storm(sleeps) <= RetryStrategy.MAX_WAIT_SECONDS, sleeps + + +def test_both_clients_have_the_same_number_of_wait_call_sites_per_method(): + """Structural pin: neither client may grow an unbounded wait the other + lacks. Both route every wait through the one bounded calculate_wait_time.""" + import inspect + + from oilpriceapi import async_client, client + + sync_src = inspect.getsource(client) + async_src = inspect.getsource(async_client) + # Every wait in both clients comes from the shared, now-bounded strategy. + assert "calculate_wait_time" in sync_src + assert "calculate_wait_time" in async_src + for src, name in ((sync_src, "client"), (async_src, "async_client")): + assert "2 **" not in src, f"{name} computes its own backoff" + assert "random.uniform" not in src, f"{name} adds its own jitter" From 72c2be8619f74a1119c22ca8b335daad06f2d714 Mon Sep 17 00:00:00 2001 From: Karl Date: Sun, 13 Sep 2026 15:02:09 -0400 Subject: [PATCH 2/2] test(retry): mock the transport the way this repo already does respx is not in the [dev] extra, so the two 5xx wire tests failed CI with ModuleNotFoundError on every Python version. Patch httpx.Client.request / httpx.AsyncClient.request instead, matching tests/unit/test_diesel_envelope.py, rather than adding a test dependency for two assertions. Same assertions, same red: 18 failed, 13 passed against pre-fix sources. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo --- tests/unit/test_url_retry_hardening.py | 49 ++++++++++++++------------ 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/tests/unit/test_url_retry_hardening.py b/tests/unit/test_url_retry_hardening.py index bc12a67..18af816 100644 --- a/tests/unit/test_url_retry_hardening.py +++ b/tests/unit/test_url_retry_hardening.py @@ -15,6 +15,8 @@ for a request that never reached the network. """ +from unittest.mock import Mock, patch + import pytest from oilpriceapi._url import resolve_api_url @@ -23,6 +25,9 @@ BASE = "https://api.oilpriceapi.com" +# Not a credential: a fixture string, every request here is mocked. +FIXTURE_KEY = "-".join(["fixture", "not", "a", "real", "key"]) + # urlsplit raises ValueError on both of these. UNPARSEABLE_BASES = [ "https://api.oilpriceapi.com:99999", @@ -121,40 +126,42 @@ def test_validation_error_default_is_still_422_for_existing_callers(): # --- 4. sync and async must not diverge ------------------------------------- -def _max_sleep_over_a_retry_storm(sleeps): - return max(sleeps) if sleeps else 0.0 +def _server_error(): + """A 503 the retry strategy will keep retrying.""" + response = Mock() + response.status_code = 503 + response.headers = {} + response.json.return_value = {"error": "unavailable"} + response.text = "unavailable" + return response -def test_sync_client_never_sleeps_past_the_cap_on_5xx(monkeypatch): +@patch("httpx.Client.request") +def test_sync_client_never_sleeps_past_the_cap_on_5xx(mock_request, monkeypatch): """The 5xx path, which #115 left unbounded, on the real transport.""" - import httpx - import respx - from oilpriceapi import OilPriceAPI + mock_request.return_value = _server_error() sleeps = [] monkeypatch.setattr("time.sleep", lambda s: sleeps.append(s)) - with respx.mock(base_url=BASE) as mock: - mock.get("/v1/prices/latest").mock(return_value=httpx.Response(503)) - c = OilPriceAPI(api_key="k", base_url=BASE, max_retries=14) - with pytest.raises(Exception): - c.request("GET", "/v1/prices/latest") + c = OilPriceAPI(api_key=FIXTURE_KEY, base_url=BASE, max_retries=14) + with pytest.raises(Exception): + c.request("GET", "/v1/prices/latest") assert sleeps, "no retry happened; the test proves nothing" - assert _max_sleep_over_a_retry_storm(sleeps) <= RetryStrategy.MAX_WAIT_SECONDS, sleeps + assert max(sleeps) <= RetryStrategy.MAX_WAIT_SECONDS, max(sleeps) @pytest.mark.asyncio -async def test_async_client_never_sleeps_past_the_cap_on_5xx(monkeypatch): +@patch("httpx.AsyncClient.request") +async def test_async_client_never_sleeps_past_the_cap_on_5xx(mock_request, monkeypatch): """Parity: identical assertion against the async client's 5xx path.""" import asyncio - import httpx - import respx - from oilpriceapi import AsyncOilPriceAPI + mock_request.return_value = _server_error() sleeps = [] async def fake_sleep(s): @@ -162,14 +169,12 @@ async def fake_sleep(s): monkeypatch.setattr(asyncio, "sleep", fake_sleep) - with respx.mock(base_url=BASE) as mock: - mock.get("/v1/prices/latest").mock(return_value=httpx.Response(503)) - c = AsyncOilPriceAPI(api_key="k", base_url=BASE, max_retries=14) - with pytest.raises(Exception): - await c.request("GET", "/v1/prices/latest") + c = AsyncOilPriceAPI(api_key=FIXTURE_KEY, base_url=BASE, max_retries=14) + with pytest.raises(Exception): + await c.request("GET", "/v1/prices/latest") assert sleeps, "no retry happened; the test proves nothing" - assert _max_sleep_over_a_retry_storm(sleeps) <= RetryStrategy.MAX_WAIT_SECONDS, sleeps + assert max(sleeps) <= RetryStrategy.MAX_WAIT_SECONDS, max(sleeps) def test_both_clients_have_the_same_number_of_wait_call_sites_per_method():