From 644a47f285aac534ae1d9559dacb1d6d6af27743 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 17 Oct 2026 12:20:51 +0300 Subject: [PATCH 1/4] fix!: validate Retry, AsyncRetry and RetryBudget config at construction --- docs/resilience.md | 12 +++--- src/httpware/middleware/resilience/budget.py | 15 +++++++ src/httpware/middleware/resilience/retry.py | 15 ++++++- tests/test_budget.py | 30 +++++++++++++ tests/test_retry.py | 10 +++++ tests/test_retry_policy.py | 44 ++++++++++++++++++++ tests/test_retry_sync.py | 10 +++++ 7 files changed, 129 insertions(+), 7 deletions(-) diff --git a/docs/resilience.md b/docs/resilience.md index a42edf2..802a9e0 100644 --- a/docs/resilience.md +++ b/docs/resilience.md @@ -24,8 +24,8 @@ from httpware.middleware.resilience import AsyncRetry | Parameter | Default | Effect | |---|---|---| | `max_attempts` | `3` | Total tries, including the first. `1` disables retries; `<1` raises `ValueError`. | -| `base_delay` | `0.1` (s) | Floor for the full-jitter exponential backoff. | -| `max_delay` | `5.0` (s) | Ceiling for backoff. | +| `base_delay` | `0.1` (s) | Floor for the full-jitter exponential backoff. Must be `>= 0`; a negative or NaN value raises `ValueError`. | +| `max_delay` | `5.0` (s) | Ceiling for backoff and the longest `Retry-After` honored. Must be finite and `>= 0`; a negative, NaN or infinite value raises `ValueError`. | | `retry_status_codes` | `frozenset({408, 429, 502, 503, 504})` | Status codes considered retryable. | | `retry_methods` | `frozenset({"GET", "HEAD", "OPTIONS", "PUT", "DELETE"})` | Idempotent methods only. To retry POST, pass a frozenset that includes `"POST"`. | | `respect_retry_after` | `True` | When a retryable response has a `Retry-After` header, wait that long instead of the jittered backoff. If the value is over `max_delay`, `AsyncRetry` gives up and re-raises the `StatusError` with a PEP 678 note: `httpware: Retry-After (Ns) exceeded max_delay (Ms); giving up`. To avoid this, set `respect_retry_after=False` or raise `max_delay`. | @@ -70,9 +70,11 @@ A token bucket in the style of Finagle's retry budget. Each request deposits a t | Parameter | Default | Effect | |---|---|---| -| `ttl` | `10.0` (s) | Sliding window over which deposits and withdrawals count. | -| `min_retries_per_sec` | `10.0` | Floor: at least this many retries per second are allowed, whatever the deposit rate. | -| `percent_can_retry` | `0.2` | Fraction of recent deposits that can convert to retries (above the floor). | +| `ttl` | `10.0` (s) | Sliding window over which deposits and withdrawals count. Must be finite and `> 0`. | +| `min_retries_per_sec` | `10.0` | Floor: at least this many retries per second are allowed, whatever the deposit rate. Must be finite and `>= 0`. | +| `percent_can_retry` | `0.2` | Fraction of recent deposits that can convert to retries (above the floor). Must be finite and `>= 0`; values above `1.0` allow more retries than requests. | + +An out-of-range value raises `ValueError` when the budget is constructed. ### The token-bucket formula diff --git a/src/httpware/middleware/resilience/budget.py b/src/httpware/middleware/resilience/budget.py index 131325a..278400f 100644 --- a/src/httpware/middleware/resilience/budget.py +++ b/src/httpware/middleware/resilience/budget.py @@ -17,6 +17,20 @@ from collections.abc import Callable +_TTL_INVALID = "ttl must be a finite number > 0" +_MIN_RETRIES_PER_SEC_INVALID = "min_retries_per_sec must be a finite number >= 0" +_PERCENT_CAN_RETRY_INVALID = "percent_can_retry must be a finite number >= 0" + + +def _validate_budget_config(*, ttl: float, min_retries_per_sec: float, percent_can_retry: float) -> None: + if not (ttl > 0 and math.isfinite(ttl)): + raise ValueError(_TTL_INVALID) + if not (min_retries_per_sec >= 0 and math.isfinite(min_retries_per_sec)): + raise ValueError(_MIN_RETRIES_PER_SEC_INVALID) + if not (percent_can_retry >= 0 and math.isfinite(percent_can_retry)): + raise ValueError(_PERCENT_CAN_RETRY_INVALID) + + class RetryBudget: """Token-bucket budget bounding retry rate to prevent retry storms. @@ -33,6 +47,7 @@ def __init__( percent_can_retry: float = 0.2, _now: Callable[[], float] = time.monotonic, ) -> None: + _validate_budget_config(ttl=ttl, min_retries_per_sec=min_retries_per_sec, percent_can_retry=percent_can_retry) self._ttl = ttl self._min_retries_per_sec = min_retries_per_sec self._percent_can_retry = percent_can_retry diff --git a/src/httpware/middleware/resilience/retry.py b/src/httpware/middleware/resilience/retry.py index 4e267f4..edda2fc 100644 --- a/src/httpware/middleware/resilience/retry.py +++ b/src/httpware/middleware/resilience/retry.py @@ -17,6 +17,7 @@ import datetime as dt import email.utils import logging +import math import time from collections.abc import Awaitable, Callable from http import HTTPStatus @@ -56,6 +57,8 @@ _RETRYABLE_EXCEPTIONS = (StatusError, NetworkError, TimeoutError) _MAX_ATTEMPTS_INVALID = "max_attempts must be >= 1" +_BASE_DELAY_INVALID = "base_delay must be >= 0" +_MAX_DELAY_INVALID = "max_delay must be a finite number >= 0" _STREAMING_BODY_REFUSAL_NOTE = ( "httpware: not retrying because the request body is a stream that cannot replay across attempts" ) @@ -66,6 +69,15 @@ _LOGGER = logging.getLogger("httpware.retry") +def _validate_retry_config(*, max_attempts: int, base_delay: float, max_delay: float) -> None: + if max_attempts < 1: + raise ValueError(_MAX_ATTEMPTS_INVALID) + if not base_delay >= 0: + raise ValueError(_BASE_DELAY_INVALID) + if not (max_delay >= 0 and math.isfinite(max_delay)): + raise ValueError(_MAX_DELAY_INVALID) + + def _parse_retry_after(value: str) -> float | None: """Parse a Retry-After header value. Returns None on malformed input.""" try: @@ -105,8 +117,7 @@ def __init__( # noqa: PLR0913 — retry policy has many orthogonal knobs; a dat respect_retry_after: bool, budget: RetryBudget | None, ) -> None: - if max_attempts < 1: - raise ValueError(_MAX_ATTEMPTS_INVALID) + _validate_retry_config(max_attempts=max_attempts, base_delay=base_delay, max_delay=max_delay) self.max_attempts = max_attempts self.base_delay = base_delay self.max_delay = max_delay diff --git a/tests/test_budget.py b/tests/test_budget.py index afdb577..2577589 100644 --- a/tests/test_budget.py +++ b/tests/test_budget.py @@ -4,8 +4,11 @@ so they cannot be perturbed by other tests sharing the same module. """ +import math import time +import pytest + from httpware.middleware.resilience.budget import RetryBudget @@ -106,3 +109,30 @@ def test_default_now_is_time_monotonic() -> None: """When _now is not passed, the budget uses time.monotonic by default.""" budget = RetryBudget() assert budget._now is time.monotonic # noqa: SLF001 + + +@pytest.mark.parametrize("ttl", [0.0, -1.0, math.nan, math.inf]) +def test_invalid_ttl_rejected(ttl: float) -> None: + with pytest.raises(ValueError, match="ttl must be a finite number > 0"): + RetryBudget(ttl=ttl) + + +@pytest.mark.parametrize("min_retries_per_sec", [-1.0, math.nan, math.inf]) +def test_invalid_min_retries_per_sec_rejected(min_retries_per_sec: float) -> None: + with pytest.raises(ValueError, match="min_retries_per_sec must be a finite number >= 0"): + RetryBudget(min_retries_per_sec=min_retries_per_sec) + + +@pytest.mark.parametrize("percent_can_retry", [-0.1, math.nan, math.inf]) +def test_invalid_percent_can_retry_rejected(percent_can_retry: float) -> None: + with pytest.raises(ValueError, match="percent_can_retry must be a finite number >= 0"): + RetryBudget(percent_can_retry=percent_can_retry) + + +def test_boundary_budget_values_accepted() -> None: + clock = _Clock() + budget = RetryBudget(ttl=0.001, min_retries_per_sec=0.0, percent_can_retry=2.0, _now=clock.now) + budget.deposit() + assert budget.try_withdraw() is True + assert budget.try_withdraw() is True + assert budget.try_withdraw() is False diff --git a/tests/test_retry.py b/tests/test_retry.py index af492d3..b98d5b8 100644 --- a/tests/test_retry.py +++ b/tests/test_retry.py @@ -143,6 +143,16 @@ def test_max_attempts_zero_rejected() -> None: AsyncRetry(max_attempts=0) +def test_negative_base_delay_rejected() -> None: + with pytest.raises(ValueError, match="base_delay must be >= 0"): + AsyncRetry(base_delay=-1) + + +def test_negative_max_delay_rejected() -> None: + with pytest.raises(ValueError, match="max_delay must be a finite number >= 0"): + AsyncRetry(max_delay=-1) + + async def test_budget_exhausted_raises_retry_budget_exhausted_error() -> None: # NOTE: lives here for coverage of the AsyncRetry loop's budget-exhaustion branch. # Task 11 adds the broader budget-gate + sharing tests (carry-through behavior, diff --git a/tests/test_retry_policy.py b/tests/test_retry_policy.py index 2b15129..fecc8dc 100644 --- a/tests/test_retry_policy.py +++ b/tests/test_retry_policy.py @@ -7,6 +7,7 @@ """ import datetime as dt +import math import httpx2 import pytest @@ -206,3 +207,46 @@ def test_budget_refusal_raises_with_cause_and_fields() -> None: def test_invalid_max_attempts_rejected() -> None: with pytest.raises(ValueError, match="max_attempts must be >= 1"): _policy(max_attempts=0) + + +@pytest.mark.parametrize("base_delay", [-0.1, -1, math.nan]) +def test_invalid_base_delay_rejected(base_delay: float) -> None: + with pytest.raises(ValueError, match="base_delay must be >= 0"): + _RetryPolicy( + max_attempts=3, + base_delay=base_delay, + max_delay=_MAX_DELAY, + retry_status_codes=DEFAULT_RETRY_STATUS_CODES, + retry_methods=DEFAULT_IDEMPOTENT_METHODS, + respect_retry_after=True, + budget=None, + ) + + +@pytest.mark.parametrize("max_delay", [-0.1, -1, math.nan, math.inf]) +def test_invalid_max_delay_rejected(max_delay: float) -> None: + with pytest.raises(ValueError, match="max_delay must be a finite number >= 0"): + _RetryPolicy( + max_attempts=3, + base_delay=_BASE_DELAY, + max_delay=max_delay, + retry_status_codes=DEFAULT_RETRY_STATUS_CODES, + retry_methods=DEFAULT_IDEMPOTENT_METHODS, + respect_retry_after=True, + budget=None, + ) + + +@pytest.mark.parametrize(("base_delay", "max_delay"), [(0.0, 0.0), (0.0, _MAX_DELAY), (math.inf, _MAX_DELAY)]) +def test_boundary_delays_accepted(base_delay: float, max_delay: float) -> None: + policy = _RetryPolicy( + max_attempts=3, + base_delay=base_delay, + max_delay=max_delay, + retry_status_codes=DEFAULT_RETRY_STATUS_CODES, + retry_methods=DEFAULT_IDEMPOTENT_METHODS, + respect_retry_after=True, + budget=None, + ) + request = _request("PUT") + assert 0.0 <= policy.decide(attempt=0, request=request, exc=_status_exc(503, request)) <= max_delay diff --git a/tests/test_retry_sync.py b/tests/test_retry_sync.py index e423650..9b13633 100644 --- a/tests/test_retry_sync.py +++ b/tests/test_retry_sync.py @@ -147,6 +147,16 @@ def test_max_attempts_zero_rejected() -> None: Retry(max_attempts=0) +def test_negative_base_delay_rejected() -> None: + with pytest.raises(ValueError, match="base_delay must be >= 0"): + Retry(base_delay=-1) + + +def test_negative_max_delay_rejected() -> None: + with pytest.raises(ValueError, match="max_delay must be a finite number >= 0"): + Retry(max_delay=-1) + + def test_streamed_body_request_is_refused() -> None: sleeper = _SleepRecorder() handler = _ResponseSequence([HTTPStatus.SERVICE_UNAVAILABLE]) From d89824de9dd559d5bdbcfee6456af9fb160c345a Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 17 Oct 2026 12:21:31 +0300 Subject: [PATCH 2/4] fix: match retry_methods case-insensitively --- docs/resilience.md | 2 +- src/httpware/middleware/resilience/retry.py | 4 ++-- tests/test_retry.py | 12 ++++++++++++ tests/test_retry_sync.py | 12 ++++++++++++ 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/docs/resilience.md b/docs/resilience.md index 802a9e0..f0d878f 100644 --- a/docs/resilience.md +++ b/docs/resilience.md @@ -27,7 +27,7 @@ from httpware.middleware.resilience import AsyncRetry | `base_delay` | `0.1` (s) | Floor for the full-jitter exponential backoff. Must be `>= 0`; a negative or NaN value raises `ValueError`. | | `max_delay` | `5.0` (s) | Ceiling for backoff and the longest `Retry-After` honored. Must be finite and `>= 0`; a negative, NaN or infinite value raises `ValueError`. | | `retry_status_codes` | `frozenset({408, 429, 502, 503, 504})` | Status codes considered retryable. | -| `retry_methods` | `frozenset({"GET", "HEAD", "OPTIONS", "PUT", "DELETE"})` | Idempotent methods only. To retry POST, pass a frozenset that includes `"POST"`. | +| `retry_methods` | `frozenset({"GET", "HEAD", "OPTIONS", "PUT", "DELETE"})` | Idempotent methods only. To retry POST, pass a frozenset that includes `"POST"`. Matching ignores case, so `"post"` works too. | | `respect_retry_after` | `True` | When a retryable response has a `Retry-After` header, wait that long instead of the jittered backoff. If the value is over `max_delay`, `AsyncRetry` gives up and re-raises the `StatusError` with a PEP 678 note: `httpware: Retry-After (Ns) exceeded max_delay (Ms); giving up`. To avoid this, set `respect_retry_after=False` or raise `max_delay`. | | `budget` | `RetryBudget()` | The token bucket. Pass one shared `RetryBudget` to several clients to give them a joint budget. | diff --git a/src/httpware/middleware/resilience/retry.py b/src/httpware/middleware/resilience/retry.py index edda2fc..77f132a 100644 --- a/src/httpware/middleware/resilience/retry.py +++ b/src/httpware/middleware/resilience/retry.py @@ -122,7 +122,7 @@ def __init__( # noqa: PLR0913 — retry policy has many orthogonal knobs; a dat self.base_delay = base_delay self.max_delay = max_delay self.retry_status_codes = retry_status_codes - self.retry_methods = retry_methods + self.retry_methods = frozenset(method.upper() for method in retry_methods) self.respect_retry_after = respect_retry_after self.budget = budget if budget is not None else RetryBudget() @@ -139,7 +139,7 @@ def decide( # noqa: C901 — complexity budget: classification + streaming-body exception. `exc` is the currently-handled exception (one of _RETRYABLE_EXCEPTIONS); see the class docstring for chaining semantics. """ - method_eligible = request.method.upper() in self.retry_methods + method_eligible = request.method in self.retry_methods if isinstance(exc, StatusError): retryable_status = exc.response.status_code in self.retry_status_codes if not method_eligible or not retryable_status: diff --git a/tests/test_retry.py b/tests/test_retry.py index b98d5b8..2d380dc 100644 --- a/tests/test_retry.py +++ b/tests/test_retry.py @@ -128,6 +128,18 @@ async def test_retries_post_when_method_explicitly_included() -> None: assert handler.calls == 2 # noqa: PLR2004 — "2" is intentional literal in test assertion +async def test_retry_methods_match_case_insensitively() -> None: + sleeper = _SleepRecorder() + handler = _ResponseSequence([HTTPStatus.SERVICE_UNAVAILABLE, HTTPStatus.OK]) + client = _client( + handler, + retry=AsyncRetry(_sleep=sleeper, retry_methods=frozenset({"post"}), base_delay=0.01, max_delay=0.02), + ) + response = await client.post("https://example.test/x", json={"x": 1}) + assert response.status_code == HTTPStatus.OK + assert handler.calls == 2 # noqa: PLR2004 — initial attempt + 1 retry + + async def test_max_attempts_one_means_no_retries() -> None: sleeper = _SleepRecorder() handler = _ResponseSequence([HTTPStatus.SERVICE_UNAVAILABLE]) diff --git a/tests/test_retry_sync.py b/tests/test_retry_sync.py index 9b13633..ee9e35a 100644 --- a/tests/test_retry_sync.py +++ b/tests/test_retry_sync.py @@ -457,6 +457,18 @@ def test_retries_post_when_method_explicitly_included() -> None: assert len(sleeper.calls) == 1 +def test_retry_methods_match_case_insensitively() -> None: + sleeper = _SleepRecorder() + handler = _ResponseSequence([HTTPStatus.SERVICE_UNAVAILABLE, HTTPStatus.OK]) + client = _client( + handler, + retry=Retry(_sleep=sleeper, retry_methods=frozenset({"post"}), base_delay=0.01, max_delay=0.02), + ) + response = client.post("https://example.test/x", json={"k": "v"}) + assert response.status_code == HTTPStatus.OK + assert handler.calls == 2 # noqa: PLR2004 — initial attempt + 1 retry + + def test_default_budget_is_fresh_per_instance() -> None: r1 = Retry() r2 = Retry() From fa4258354d558d61c9ba65202a027df319256f47 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 17 Oct 2026 12:22:18 +0300 Subject: [PATCH 3/4] fix: emit retry.giving_up when Retry-After exceeds max_delay --- docs/observability.md | 2 ++ docs/resilience.md | 2 +- src/httpware/middleware/resilience/retry.py | 17 ++++++++++++++ tests/test_retry.py | 22 ++++++++++++++++++ tests/test_retry_sync.py | 25 ++++++++++++++++++++- 5 files changed, 66 insertions(+), 2 deletions(-) diff --git a/docs/observability.md b/docs/observability.md index 1ce0cae..3920f62 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -13,6 +13,8 @@ The logger and event names below are a stable public contract: Each log record has an `event` field holding the event name, such as `event="circuit.opened"`, which you can filter on in your log aggregator. `circuit.rejected` is emitted at most once per second per circuit, so an open circuit under heavy traffic does not flood your logs. The first rejection of each outage is always emitted, `suppressed` counts the rejections left out since the previous one, and `circuit.closed` reports the outage's total in `rejected`. Events from `AsyncKeyedCircuitBreaker` and `KeyedCircuitBreaker` also carry `circuit_key`, the circuit they belong to. [Resilience](resilience.md) describes when each event fires. +`retry.giving_up` fires when a retryable failure is re-raised without another attempt, and its `reason` attribute says why. `max_attempts` means every attempt failed. `retry_after_exceeds_max_delay` means the server's `Retry-After` asked for a longer wait than `max_delay` allows; these events also carry `retry_after` and `max_delay` in seconds. Both kinds carry `attempts`, `method`, `url`, `last_status` and `last_exception_type`. + Events that carry a `url` attribute get it redacted first: httpware strips `user:pass@` userinfo and replaces every query-parameter value with `REDACTED`, keeping the keys. A URL logged as `https://api.test/items?page=REDACTED&sort=REDACTED` came from `https://api.test/items?page=2&sort=name`. ```python diff --git a/docs/resilience.md b/docs/resilience.md index f0d878f..67f5ebb 100644 --- a/docs/resilience.md +++ b/docs/resilience.md @@ -28,7 +28,7 @@ from httpware.middleware.resilience import AsyncRetry | `max_delay` | `5.0` (s) | Ceiling for backoff and the longest `Retry-After` honored. Must be finite and `>= 0`; a negative, NaN or infinite value raises `ValueError`. | | `retry_status_codes` | `frozenset({408, 429, 502, 503, 504})` | Status codes considered retryable. | | `retry_methods` | `frozenset({"GET", "HEAD", "OPTIONS", "PUT", "DELETE"})` | Idempotent methods only. To retry POST, pass a frozenset that includes `"POST"`. Matching ignores case, so `"post"` works too. | -| `respect_retry_after` | `True` | When a retryable response has a `Retry-After` header, wait that long instead of the jittered backoff. If the value is over `max_delay`, `AsyncRetry` gives up and re-raises the `StatusError` with a PEP 678 note: `httpware: Retry-After (Ns) exceeded max_delay (Ms); giving up`. To avoid this, set `respect_retry_after=False` or raise `max_delay`. | +| `respect_retry_after` | `True` | When a retryable response has a `Retry-After` header, wait that long instead of the jittered backoff. If the value is over `max_delay`, `AsyncRetry` gives up, emits `retry.giving_up` with `reason="retry_after_exceeds_max_delay"`, and re-raises the `StatusError` with a PEP 678 note: `httpware: Retry-After (Ns) exceeded max_delay (Ms); giving up`. To avoid this, set `respect_retry_after=False` or raise `max_delay`. | | `budget` | `RetryBudget()` | The token bucket. Pass one shared `RetryBudget` to several clients to give them a joint budget. | To bound the total time across all attempts, put [`AsyncTimeout`](#asynctimeout) first in the chain. To bound a single request, set `httpx2.Timeout` on the client or pass `timeout=` per request. diff --git a/src/httpware/middleware/resilience/retry.py b/src/httpware/middleware/resilience/retry.py index 77f132a..dd959c8 100644 --- a/src/httpware/middleware/resilience/retry.py +++ b/src/httpware/middleware/resilience/retry.py @@ -179,6 +179,7 @@ def decide( # noqa: C901 — complexity budget: classification + streaming-body "url": str(request.url), "last_status": last_response.status_code if last_response is not None else None, "last_exception_type": type(exc).__qualname__, + "reason": "max_attempts", }, ) raise exc @@ -196,6 +197,22 @@ def decide( # noqa: C901 — complexity budget: classification + streaming-body max_delay=self.max_delay, ), ) + _emit_event( + _LOGGER, + "retry.giving_up", + level=logging.WARNING, + message=f"retry gave up after {attempt + 1} attempts: Retry-After exceeded max_delay", + attributes={ + "attempts": attempt + 1, + "method": request.method, + "url": str(request.url), + "last_status": last_response.status_code if last_response is not None else None, + "last_exception_type": type(exc).__qualname__, + "reason": "retry_after_exceeds_max_delay", + "retry_after": retry_after, + "max_delay": self.max_delay, + }, + ) raise exc if not self.budget.try_withdraw(): diff --git a/tests/test_retry.py b/tests/test_retry.py index 2d380dc..a0df97b 100644 --- a/tests/test_retry.py +++ b/tests/test_retry.py @@ -647,6 +647,28 @@ async def test_retry_giving_up_emits_observability_event(caplog: pytest.LogCaptu assert record.method == "GET" # ty: ignore[unresolved-attribute] assert record.last_status == HTTPStatus.SERVICE_UNAVAILABLE # ty: ignore[unresolved-attribute] assert record.last_exception_type == "ServiceUnavailableError" # ty: ignore[unresolved-attribute] + assert record.reason == "max_attempts" # ty: ignore[unresolved-attribute] + + +async def test_retry_after_exceeding_max_delay_emits_giving_up_event(caplog: pytest.LogCaptureFixture) -> None: + sleeper = _SleepRecorder() + handler = _ResponseSequenceWithHeaders([(HTTPStatus.SERVICE_UNAVAILABLE, {"Retry-After": "9999"})]) + client = _client(handler, retry=AsyncRetry(_sleep=sleeper, base_delay=0.01, max_delay=2.5)) + + with caplog.at_level(logging.WARNING, logger="httpware.retry"), pytest.raises(ServiceUnavailableError): + await client.get("https://example.test/x") + + records = [r for r in caplog.records if r.name == "httpware.retry" and r.event == "retry.giving_up"] # ty: ignore[unresolved-attribute] + assert len(records) == 1 + record = records[0] + assert record.levelno == logging.WARNING + assert record.reason == "retry_after_exceeds_max_delay" # ty: ignore[unresolved-attribute] + assert record.attempts == 1 # ty: ignore[unresolved-attribute] + assert record.method == "GET" # ty: ignore[unresolved-attribute] + assert record.last_status == HTTPStatus.SERVICE_UNAVAILABLE # ty: ignore[unresolved-attribute] + assert record.last_exception_type == "ServiceUnavailableError" # ty: ignore[unresolved-attribute] + assert record.retry_after == 9999.0 # noqa: PLR2004 — matches the header above # ty: ignore[unresolved-attribute] + assert record.max_delay == 2.5 # noqa: PLR2004 — matches max_delay above # ty: ignore[unresolved-attribute] async def test_retry_budget_refused_emits_observability_event(caplog: pytest.LogCaptureFixture) -> None: diff --git a/tests/test_retry_sync.py b/tests/test_retry_sync.py index ee9e35a..467e98b 100644 --- a/tests/test_retry_sync.py +++ b/tests/test_retry_sync.py @@ -488,7 +488,30 @@ def test_emits_giving_up_log_event(caplog: pytest.LogCaptureFixture) -> None: client = _client(handler, retry=Retry(_sleep=sleeper, base_delay=0.01, max_attempts=2)) with caplog.at_level(logging.WARNING, logger="httpware.retry"), pytest.raises(ServiceUnavailableError): client.get("https://example.test/x") - assert any("retry gave up" in r.getMessage() for r in caplog.records) + giving_up = [r for r in caplog.records if r.name == "httpware.retry" and r.event == "retry.giving_up"] # ty: ignore[unresolved-attribute] + assert len(giving_up) == 1 + assert giving_up[0].reason == "max_attempts" # ty: ignore[unresolved-attribute] + + +def test_retry_after_exceeding_max_delay_emits_giving_up_event(caplog: pytest.LogCaptureFixture) -> None: + sleeper = _SleepRecorder() + handler = _ResponseSequenceWithHeaders([(HTTPStatus.SERVICE_UNAVAILABLE, {"Retry-After": "9999"})]) + client = _client(handler, retry=Retry(_sleep=sleeper, base_delay=0.01, max_delay=2.5)) + + with caplog.at_level(logging.WARNING, logger="httpware.retry"), pytest.raises(ServiceUnavailableError): + client.get("https://example.test/x") + + records = [r for r in caplog.records if r.name == "httpware.retry" and r.event == "retry.giving_up"] # ty: ignore[unresolved-attribute] + assert len(records) == 1 + record = records[0] + assert record.levelno == logging.WARNING + assert record.reason == "retry_after_exceeds_max_delay" # ty: ignore[unresolved-attribute] + assert record.attempts == 1 # ty: ignore[unresolved-attribute] + assert record.method == "GET" # ty: ignore[unresolved-attribute] + assert record.last_status == HTTPStatus.SERVICE_UNAVAILABLE # ty: ignore[unresolved-attribute] + assert record.last_exception_type == "ServiceUnavailableError" # ty: ignore[unresolved-attribute] + assert record.retry_after == 9999.0 # noqa: PLR2004 — matches the header above # ty: ignore[unresolved-attribute] + assert record.max_delay == 2.5 # noqa: PLR2004 — matches max_delay above # ty: ignore[unresolved-attribute] def test_emits_budget_refused_log_event(caplog: pytest.LogCaptureFixture) -> None: From ddfdb7518302c3c60011eb6b678089bb6dd3440a Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 17 Oct 2026 12:22:58 +0300 Subject: [PATCH 4/4] fix: treat a Retry-After integer too large for a float as an infinite wait --- docs/observability.md | 2 +- docs/resilience.md | 2 +- src/httpware/middleware/resilience/retry.py | 14 ++++++++++---- tests/test_retry.py | 12 ++++++------ tests/test_retry_policy.py | 19 +++++++++++++++++++ tests/test_retry_sync.py | 12 ++++++------ 6 files changed, 43 insertions(+), 18 deletions(-) diff --git a/docs/observability.md b/docs/observability.md index 3920f62..2b55554 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -13,7 +13,7 @@ The logger and event names below are a stable public contract: Each log record has an `event` field holding the event name, such as `event="circuit.opened"`, which you can filter on in your log aggregator. `circuit.rejected` is emitted at most once per second per circuit, so an open circuit under heavy traffic does not flood your logs. The first rejection of each outage is always emitted, `suppressed` counts the rejections left out since the previous one, and `circuit.closed` reports the outage's total in `rejected`. Events from `AsyncKeyedCircuitBreaker` and `KeyedCircuitBreaker` also carry `circuit_key`, the circuit they belong to. [Resilience](resilience.md) describes when each event fires. -`retry.giving_up` fires when a retryable failure is re-raised without another attempt, and its `reason` attribute says why. `max_attempts` means every attempt failed. `retry_after_exceeds_max_delay` means the server's `Retry-After` asked for a longer wait than `max_delay` allows; these events also carry `retry_after` and `max_delay` in seconds. Both kinds carry `attempts`, `method`, `url`, `last_status` and `last_exception_type`. +`retry.giving_up` fires when a retryable failure is re-raised without another attempt, and its `reason` attribute says why. `max_attempts` means every attempt failed. `retry_after_exceeds_max_delay` means the server's `Retry-After` asked for a longer wait than `max_delay` allows; these events also carry `retry_after` and `max_delay` in seconds, and `retry_after` is `inf` when the header held an integer too large to fit in a float. Both kinds carry `attempts`, `method`, `url`, `last_status` and `last_exception_type`. Events that carry a `url` attribute get it redacted first: httpware strips `user:pass@` userinfo and replaces every query-parameter value with `REDACTED`, keeping the keys. A URL logged as `https://api.test/items?page=REDACTED&sort=REDACTED` came from `https://api.test/items?page=2&sort=name`. diff --git a/docs/resilience.md b/docs/resilience.md index 67f5ebb..2cd9258 100644 --- a/docs/resilience.md +++ b/docs/resilience.md @@ -40,7 +40,7 @@ To bound the total time across all attempts, put [`AsyncTimeout`](#asynctimeout) - integer seconds: `Retry-After: 30` waits 30 seconds. - an HTTP date (RFC 5322): `Retry-After: Wed, 21 Oct 2026 07:28:00 GMT` waits until that time, or not at all if it has passed. -Either form over `max_delay` makes `AsyncRetry` give up as described above. Negative integers count as 0. Malformed values are ignored and the jittered backoff is used instead. +Either form over `max_delay` makes `AsyncRetry` give up as described above. An integer too large to fit in a float counts as an infinite wait, so it always exceeds `max_delay`. Negative integers count as 0. Malformed values are ignored and the jittered backoff is used instead. ### Streaming-body refusal diff --git a/src/httpware/middleware/resilience/retry.py b/src/httpware/middleware/resilience/retry.py index dd959c8..f3b09ba 100644 --- a/src/httpware/middleware/resilience/retry.py +++ b/src/httpware/middleware/resilience/retry.py @@ -79,11 +79,17 @@ def _validate_retry_config(*, max_attempts: int, base_delay: float, max_delay: f def _parse_retry_after(value: str) -> float | None: - """Parse a Retry-After header value. Returns None on malformed input.""" + """Parse a Retry-After header value. Returns None on malformed input, inf for integers too large for a float.""" try: - return max(0.0, float(int(value))) # clamp: negative integers are malformed servers - except (ValueError, OverflowError): - pass + seconds = int(value) + except ValueError: + if value.strip().isdigit() and value.isascii(): + return math.inf # past int()'s digit limit + else: + try: + return max(0.0, float(seconds)) # clamp: negative integers are malformed servers + except OverflowError: + return math.inf if seconds > 0 else 0.0 try: parsed = email.utils.parsedate_to_datetime(value) except (TypeError, ValueError, OverflowError): diff --git a/tests/test_retry.py b/tests/test_retry.py index a0df97b..29095b8 100644 --- a/tests/test_retry.py +++ b/tests/test_retry.py @@ -779,8 +779,7 @@ async def test_retry_after_exceeds_max_delay_does_not_consume_budget_token() -> assert len(budget._withdrawn) == 0 # noqa: SLF001 — implementation-detail access for invariant -async def test_retry_after_huge_digit_string_does_not_crash() -> None: - """A very large Retry-After integer string must fall back to jitter, not crash with OverflowError.""" +async def test_retry_after_huge_digit_string_gives_up() -> None: sleeper = _SleepRecorder() handler = _ResponseSequenceWithHeaders( [ @@ -789,10 +788,11 @@ async def test_retry_after_huge_digit_string_does_not_crash() -> None: ] ) client = _client(handler, retry=AsyncRetry(_sleep=sleeper, base_delay=0.01, max_delay=0.05)) - response = await client.get("https://example.test/x") - assert response.status_code == HTTPStatus.OK - assert len(sleeper.calls) == 1 - assert 0.0 <= sleeper.calls[0] <= 0.05 # noqa: PLR2004 — 0.05 matches max_delay literal above + with pytest.raises(ServiceUnavailableError) as exc_info: + await client.get("https://example.test/x") + assert any("exceeded max_delay" in n for n in getattr(exc_info.value, "__notes__", [])) + assert sleeper.calls == [] + assert handler.calls == 1 async def test_method_ineligible_with_streaming_body_does_not_attach_streaming_note() -> None: diff --git a/tests/test_retry_policy.py b/tests/test_retry_policy.py index fecc8dc..cb0bc24 100644 --- a/tests/test_retry_policy.py +++ b/tests/test_retry_policy.py @@ -167,6 +167,25 @@ def test_respect_retry_after_false_ignores_header() -> None: assert delay <= _BASE_DELAY # jitter, not the 2.0 header value +@pytest.mark.parametrize( + "header", + ["9" * 309, "9" * 400, "9" * 5000, " " + "9" * 5000 + " "], + ids=["309-digits", "400-digits", "5000-digits", "5000-digits-padded"], +) +def test_retry_after_too_large_for_a_float_gives_up(header: str) -> None: + request = _request("PUT") + exc = _status_exc(503, request, retry_after=header) + with pytest.raises(ServiceUnavailableError) as ei: + _policy().decide(attempt=0, request=request, exc=exc) + assert any("Retry-After (infs) exceeded max_delay" in note for note in _notes(ei.value)) + + +def test_retry_after_hugely_negative_counts_as_zero() -> None: + request = _request("PUT") + exc = _status_exc(503, request, retry_after="-" + "9" * 400) + assert _policy().decide(attempt=0, request=request, exc=exc) == 0.0 + + @pytest.mark.parametrize("zone", ["-0000", "", "XYZ"]) def test_retry_after_date_without_a_usable_zone_is_read_as_gmt(zone: str) -> None: future = dt.datetime.now(dt.UTC) + dt.timedelta(seconds=_DATE_AHEAD_SECONDS) diff --git a/tests/test_retry_sync.py b/tests/test_retry_sync.py index 467e98b..f5d0a2d 100644 --- a/tests/test_retry_sync.py +++ b/tests/test_retry_sync.py @@ -628,8 +628,7 @@ def test_retry_after_exceeds_max_delay_does_not_consume_budget_token_sync() -> N assert len(budget._withdrawn) == 0 # noqa: SLF001 — implementation-detail access for invariant -def test_retry_after_huge_digit_string_does_not_crash_sync() -> None: - """A very large Retry-After integer string must fall back to jitter, not crash (sync).""" +def test_retry_after_huge_digit_string_gives_up_sync() -> None: sleeper = _SleepRecorder() handler = _ResponseSequenceWithHeaders( [ @@ -638,10 +637,11 @@ def test_retry_after_huge_digit_string_does_not_crash_sync() -> None: ] ) client = _client(handler, retry=Retry(_sleep=sleeper, base_delay=0.01, max_delay=0.05)) - response = client.get("https://example.test/x") - assert response.status_code == HTTPStatus.OK - assert len(sleeper.calls) == 1 - assert 0.0 <= sleeper.calls[0] <= 0.05 # noqa: PLR2004 — 0.05 matches max_delay literal above + with pytest.raises(ServiceUnavailableError) as exc_info: + client.get("https://example.test/x") + assert any("exceeded max_delay" in n for n in getattr(exc_info.value, "__notes__", [])) + assert sleeper.calls == [] + assert handler.calls == 1 def test_method_ineligible_with_streaming_body_does_not_attach_streaming_note() -> None: