fix(url,retry,errors): typed errors, a real 60s cap, no status on a local refusal (#123) (#134)

* fix(url,retry,errors): typed errors, a real 60s cap, no status on a local refusal (#123)

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo

* 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: karlwaldman

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")

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
 

oilpriceapi/retry.py

@@ -396,25 +396,35 @@ 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:
             - Attempt 0: 1.0s
             - 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
 

tests/unit/test_url_retry_hardening.py

@@ -0,0 +1,194 @@
+"""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.
+"""
+
+from unittest.mock import Mock, patch
+
+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"
+
+# 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",
+    "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 _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
+
+
+@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."""
+    from oilpriceapi import OilPriceAPI
+
+    mock_request.return_value = _server_error()
+    sleeps = []
+    monkeypatch.setattr("time.sleep", lambda s: sleeps.append(s))
+
+    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(sleeps) <= RetryStrategy.MAX_WAIT_SECONDS, max(sleeps)
+
+
+@pytest.mark.asyncio
+@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
+
+    from oilpriceapi import AsyncOilPriceAPI
+
+    mock_request.return_value = _server_error()
+    sleeps = []
+
+    async def fake_sleep(s):
+        sleeps.append(s)
+
+    monkeypatch.setattr(asyncio, "sleep", fake_sleep)
+
+    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(sleeps) <= RetryStrategy.MAX_WAIT_SECONDS, max(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"