diff --git a/infrastructure/http_retry.py b/infrastructure/http_retry.py index aa3689a5..e345129d 100644 --- a/infrastructure/http_retry.py +++ b/infrastructure/http_retry.py @@ -12,6 +12,7 @@ propagate; postcodes.io soft-fails to the seed postcode). from __future__ import annotations +import random import time from typing import Callable, Optional, TypeVar @@ -35,12 +36,20 @@ def call_with_retry( backoff_base: float = 1.0, backoff_multiplier: float = 2.0, max_backoff: float = 60.0, + jitter: bool = False, ) -> T: """Retry ``fn`` on transient failures — ``TransientHttpError`` (e.g. a 429) and ``httpx.TransportError`` (read/connect timeouts, connection resets) — backing off exponentially, or by the error's ``retry_after`` when it carries one. Non-transient failures propagate immediately; the last transient error - is re-raised once ``max_retries`` is exhausted.""" + is re-raised once ``max_retries`` is exhausted. + + ``jitter`` applies **full jitter** to the *computed* backoff (a server-advised + ``retry_after`` is honoured exactly): the delay becomes a uniform random draw + in ``[0, computed_backoff]``. This de-synchronises retries across many + concurrent callers so they don't re-collide in lockstep — the "synchronised + requests look like a DDoS" failure mode Google's Solar API best-practices warn + about, and the cause of the 429 storm under 32 concurrent containers.""" last_exc: Optional[Exception] = None for attempt in range(max_retries + 1): try: @@ -52,9 +61,11 @@ def call_with_retry( exc.retry_after if isinstance(exc, TransientHttpError) else None ) if retry_after is not None: - delay = retry_after + delay = min(retry_after, max_backoff) else: - delay = backoff_base * (backoff_multiplier**attempt) - time.sleep(min(delay, max_backoff)) + delay = min(backoff_base * (backoff_multiplier**attempt), max_backoff) + if jitter: + delay = random.uniform(0.0, delay) + time.sleep(delay) assert last_exc is not None raise last_exc diff --git a/tests/infrastructure/test_http_retry.py b/tests/infrastructure/test_http_retry.py index ec1e229f..13250cde 100644 --- a/tests/infrastructure/test_http_retry.py +++ b/tests/infrastructure/test_http_retry.py @@ -109,6 +109,46 @@ def test_non_transient_error_propagates_without_retry(_no_sleep: MagicMock) -> N _no_sleep.assert_not_called() +def test_jitter_full_randomizes_the_computed_backoff(_no_sleep: MagicMock) -> None: + # Arrange — fail once transiently (no server Retry-After), then succeed. + calls = {"n": 0} + + def fn() -> str: + calls["n"] += 1 + if calls["n"] < 2: + raise TransientHttpError("429") + return "ok" + + # Act — full jitter draws the sleep uniformly from [0, computed_backoff]. + with patch("infrastructure.http_retry.random.uniform", return_value=0.4) as runi: + result = call_with_retry(fn, jitter=True) + + # Assert — the first computed backoff is 1.0; jitter draws over [0, 1.0] + # and the drawn value is what we sleep for (de-syncs concurrent retries). + assert result == "ok" + runi.assert_called_once_with(0.0, 1.0) + _no_sleep.assert_called_once_with(0.4) + + +def test_jitter_does_not_apply_to_a_server_retry_after(_no_sleep: MagicMock) -> None: + # Arrange — the server advised an exact Retry-After; jitter must not perturb it. + raised = {"done": False} + + def fn() -> str: + if not raised["done"]: + raised["done"] = True + raise TransientHttpError("429", retry_after=7.0) + return "ok" + + # Act + with patch("infrastructure.http_retry.random.uniform") as runi: + call_with_retry(fn, jitter=True) + + # Assert — Retry-After is honoured verbatim, no random draw. + runi.assert_not_called() + _no_sleep.assert_called_once_with(7.0) + + def test_retry_after_is_capped_by_max_backoff(_no_sleep: MagicMock) -> None: # Arrange raised = {"done": False}