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

[karlwaldman]

The remaining three findings from #123. The fourth — the scheme-less base_url tautology — is split out into #131 because its failure mode is different in kind.

1. A raw ValueError escapes a contract that says it cannot

_url's module docstring and resolve_api_url's Raises: both promise ValidationError. Two inputs leak a bare stdlib ValueError instead, measured on main at 7982b0b, CPython 3.14.7:

resolve_api_url("https://api.oilpriceapi.com:99999", "/v1/prices")
ValueError: Port out of range 0-65535

resolve_api_url("https://℀evil.example", "/v1/prices")
ValueError: netloc '℀evil.example' contains invalid characters under NFKC normalization

The second is the unicode case an ASCII probe cannot reach: CPython's urlsplit refuses a non-ASCII netloc whose NFKC normalisation introduces one of /?#@:.

This matters more than a tidy-up because _origin runs on base_url on every single request, so the leak is on the hot path, not at construction. urljoin raises the same errors and runs first, so both had to be wrapped — wrapping only _origin, as the issue suggests, left the NFKC case still escaping. Caught here during the fix.

Both now raise ConfigurationError naming base_url.

2. The documented 60-second cap was not a cap

RetryStrategy(max_retries=20).calculate_wait_time(10)   # -> 63.42

min(2 ** attempt, 60) bounds the base, then up to 30% jitter is added on top — so from attempt 6 onward the documented cap is exceeded, up to 78s at the ceiling. Over 2000 draws at attempt 10 I measured a worst case of 77.97s against a MAX_WAIT_SECONDS of 60.

#115 added bounded_wait and wired it only into the 429/Retry-After path. The 5xx and transport-error paths call calculate_wait_time raw — 8 call sites in client.py, 4 in async_client.py.

Clamping inside calculate_wait_time fixes all 12 at once and, more importantly, cannot drift between the two clients the way 12 separate call-site clamps could. Jitter is preserved below saturation (pinned by a test that asserts the values are still spread, so the fix does not re-create the thundering herd the jitter exists to prevent), and the docstring now says where it saturates instead of stating a cap it did not enforce.

3. A local refusal reported an HTTP status for a request never sent

ValidationError.__init__ hard-coded status_code=422. _url._reject fires before any socket is opened, yet:

ValidationError status_code= 422 is_client_error= True

Anything logging or aggregating by status recorded a 422 the API never returned. status_code is now a real parameter defaulting to 422 — so existing callers and a genuine API 422 are unchanged — and the local guard passes status_code=None. is_client_error already returned False for None, so no change was needed there.

Red

Tests written first, against unmodified main:

========================= short test summary info =========================
FAILED test_resolve_api_url_raises_typed_error_not_valueerror[https://api.oilpriceapi.com:99999]
FAILED test_resolve_api_url_raises_typed_error_not_valueerror[https://℀evil.example]
FAILED test_resolve_api_url_never_leaks_bare_valueerror[https://api.oilpriceapi.com:99999]
FAILED test_resolve_api_url_never_leaks_bare_valueerror[https://℀evil.example]
FAILED test_calculate_wait_time_never_exceeds_the_documented_cap
FAILED test_calculate_wait_time_is_bounded_at_every_attempt[6]
FAILED test_calculate_wait_time_is_bounded_at_every_attempt[7]
FAILED test_calculate_wait_time_is_bounded_at_every_attempt[8]
FAILED test_calculate_wait_time_is_bounded_at_every_attempt[9]
FAILED test_calculate_wait_time_is_bounded_at_every_attempt[10]
FAILED test_calculate_wait_time_is_bounded_at_every_attempt[11]
FAILED test_calculate_wait_time_is_bounded_at_every_attempt[12]
FAILED test_calculate_wait_time_is_bounded_at_every_attempt[13]
FAILED test_local_guard_refusal_has_no_http_status
FAILED test_local_guard_refusal_is_not_a_client_http_error
FAILED test_server_sent_validation_error_keeps_422
FAILED test_sync_client_never_sleeps_past_the_cap_on_5xx
FAILED test_async_client_never_sleeps_past_the_cap_on_5xx
======================== 18 failed, 13 passed in 0.29s ========================

Re-proved red-capable after the fix by restoring the pre-fix sources (git checkout origin/main -- oilpriceapi/_url.py oilpriceapi/retry.py oilpriceapi/exceptions.py) — that is the run pasted above.

Note test_calculate_wait_time_is_bounded_at_every_attempt fails at 6 and up and passes at 0–5. That is the bug's actual shape: below saturation the jitter has room under the cap.

Green

tests/unit/test_url_retry_hardening.py ...............................   [100%]
============================== 31 passed in 0.22s ==============================

Sync and async parity

The two 5xx tests are the load-bearing ones, and they are behavioural, not structural — they drive the real transport with respx, force a 503 retry storm at max_retries=14, intercept time.sleep / asyncio.sleep, and assert no sleep exceeds MAX_WAIT_SECONDS. Both assert sleeps is non-empty first, so a test that silently stopped retrying cannot pass by doing nothing. This is exactly the 5xx path #115 left unbounded, checked identically on both clients.

A third test pins structurally that neither client computes its own backoff (2 **) or adds its own jitter (random.uniform) — both must keep routing every wait through the one shared, now-bounded calculate_wait_time.

A note on test mocking

The first push used respx, which is not in this repo's [dev] extra, so CI failed with ModuleNotFoundError on all five Python versions. A follow-up commit switches to patching httpx.Client.request / httpx.AsyncClient.request, which is this repo's existing convention (tests/unit/test_diesel_envelope.py). Same assertions, same red counts — no test dependency added for a handful of assertions.

Full suite

	passed	failed	skipped
before (origin/main)	864	3	63
after	895	3	63

+31 is exactly this PR's new tests. The 3 failures are tests/integration/test_demo_contract.py making live calls and getting HTTP 429 — environmental, identical before and after.

git diff --stat: 3 files changed, 66 insertions(+), 12 deletions(-). No CRLF normalisation of resources/alerts.py or resources/diesel.py.

Left out on purpose

• The scheme-less base_url tautology is fix(config): refuse a scheme-less base_url so the origin guard stays real (#123) #131.
• The diesel.py state_code casing note at the end of [P3][bug] URL guard and retry hardening: raw ValueError escapes, scheme-less base_url no-ops the guard, jitter exceeds the 60s cap #123 is a separate PR — while checking it I found sync and async diverge on more than casing: resources/diesel.py:116-118 has a state_code fallback block that async_resources.py:40-45 does not have at all. That is a bigger finding than the casing note and does not belong in a URL/retry PR.

Version deliberately left at 1.14.0; release decision is separate.

Closes parts 1, 3 and 4 of #123.

🤖 Generated with Claude Code

https://claude.ai/code/session_015ao5paex73xXvuM424Libo

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: dbbef7c7-e84a-4e9e-945d-4d01124bb158

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}