diff --git a/oilpriceapi/async_client.py b/oilpriceapi/async_client.py index ee93289..d605061 100644 --- a/oilpriceapi/async_client.py +++ b/oilpriceapi/async_client.py @@ -44,7 +44,7 @@ ) from .models import HistoricalPrice, HistoricalResponse, MarketBrief, Price from .resource_validators import format_date -from .retry import RetryStrategy +from .retry import RetryStrategy, mark_ambiguous_write, validated_max_retries class AsyncOilPriceAPI: @@ -56,7 +56,13 @@ class AsyncOilPriceAPI: api_key: API key for authentication base_url: Base URL for API timeout: Request timeout in seconds - max_retries: Maximum request attempts + max_retries: Total request ATTEMPTS, not retries after the first. + Must be at least 1; anything less raises ConfigurationError. + retry_on: Status codes to retry on. An explicit empty list is honoured. + + Retry safety: POST and PATCH are sent exactly once -- a timeout, transport + error or 5xx is ambiguous, so replaying could duplicate the write. Pass + ``idempotent=True`` to ``request()`` to opt back in. Example: >>> async with AsyncOilPriceAPI() as client: @@ -93,8 +99,13 @@ def __init__( # Configuration self.base_url = (base_url or self.DEFAULT_BASE_URL).rstrip("/") self.timeout = timeout or self.DEFAULT_TIMEOUT - self.max_retries = max_retries or self.DEFAULT_MAX_RETRIES - self.retry_on = retry_on or self.DEFAULT_RETRY_CODES + # Explicit None checks, not `or` (#104) -- see OilPriceAPI.__init__. + self.max_retries = ( + self.DEFAULT_MAX_RETRIES if max_retries is None else validated_max_retries(max_retries) + ) + self.retry_on = ( + list(self.DEFAULT_RETRY_CODES) if retry_on is None else list(retry_on) + ) self.max_connections = max_connections self.max_keepalive_connections = max_keepalive_connections self.app_url = app_url @@ -196,9 +207,16 @@ async def request( path: str, params: Optional[Dict[str, Any]] = None, json_data: Optional[Dict[str, Any]] = None, + idempotent: Optional[bool] = None, **kwargs, ) -> Union[Dict[str, Any], List[Any]]: - """Make async HTTP request to API.""" + """Make async HTTP request to API. + + Args: + idempotent: Assert that repeating this request is safe. Without it, + a non-idempotent method (POST, PATCH) is sent exactly once and + never replayed after an ambiguous outcome (#104). + """ await self._ensure_client() assert self._client is not None # set by _ensure_client @@ -239,11 +257,12 @@ async def request( ) # Auto-retry with Retry-After if we have attempts left - if self._retry_strategy.should_retry(attempt, 429, response.headers): - try: - wait_time = min(float(retry_after), 60.0) - except (TypeError, ValueError): - wait_time = self._retry_strategy.calculate_wait_time(attempt) + if self._retry_strategy.should_retry( + attempt, 429, response.headers, method=method, idempotent=idempotent + ): + wait_time = self._retry_strategy.bounded_wait( + retry_after, self._retry_strategy.calculate_wait_time(attempt) + ) logger.info( f"Rate limited. Retrying in {wait_time}s (attempt {attempt + 1}/{self.max_retries})" ) @@ -251,7 +270,11 @@ async def request( continue elif response.status_code >= 500: if self._retry_strategy.should_retry( - attempt, response.status_code, response.headers + attempt, + response.status_code, + response.headers, + method=method, + idempotent=idempotent, ): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( @@ -273,20 +296,26 @@ async def request( api_key=self.api_key, timeout=self.timeout, ) - if self._retry_strategy.should_retry_on_exception(attempt): + if self._retry_strategy.should_retry_on_exception( + attempt, method=method, idempotent=idempotent + ): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, "Request timeout", wait_time, is_async=True ) await asyncio.sleep(wait_time) continue + if not self._retry_strategy.is_replay_safe(method, idempotent): + raise mark_ambiguous_write(last_exception, method) raise last_exception except httpx.RequestError as error: last_exception = error_from_exception( error, api_key=self.api_key, ) - if self._retry_strategy.should_retry_on_exception(attempt): + if self._retry_strategy.should_retry_on_exception( + attempt, method=method, idempotent=idempotent + ): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, @@ -296,6 +325,8 @@ async def request( ) await asyncio.sleep(wait_time) continue + if not self._retry_strategy.is_replay_safe(method, idempotent): + raise mark_ambiguous_write(last_exception, method) raise last_exception if last_exception: diff --git a/oilpriceapi/client.py b/oilpriceapi/client.py index e61b2ad..c9c947b 100644 --- a/oilpriceapi/client.py +++ b/oilpriceapi/client.py @@ -45,7 +45,7 @@ from .resources.subscriptions import SubscriptionsResource from .resources.webhooks import WebhooksResource from .resources.well_production import WellProductionResource -from .retry import RetryStrategy +from .retry import RetryStrategy, mark_ambiguous_write, validated_max_retries class OilPriceAPI: @@ -63,8 +63,20 @@ class OilPriceAPI: api_key: API key for authentication. If not provided, uses OILPRICEAPI_KEY env var. base_url: Base URL for API. Defaults to production. timeout: Request timeout in seconds. Defaults to 30. - max_retries: Maximum request attempts for failed requests. Defaults to 3. + max_retries: Total request ATTEMPTS, not retries after the first. + Defaults to 3. Must be at least 1; pass 1 for a single attempt with + no retries. Anything less, or a non-int, raises ConfigurationError + rather than being silently replaced by the default. retry_on: Status codes to retry on. Defaults to [429, 500, 502, 503, 504]. + An explicit empty list is honoured and disables status-code retries. + + Retry safety: a non-idempotent method (POST, PATCH) is sent exactly ONCE. + A timeout, a transport error or a 5xx is an ambiguous outcome — the server + may have committed the write before the response was lost — so replaying it + could create a duplicate. Idempotent methods (GET, HEAD, OPTIONS, TRACE, + PUT, DELETE) still retry as before, and a 429 still retries any method + because it is an outright refusal. Pass ``idempotent=True`` to ``request()`` + to opt a specific write back into retrying. Example: >>> # Recommended: Use context manager for automatic cleanup @@ -108,8 +120,15 @@ def __init__( # Configuration self.base_url = (base_url or self.DEFAULT_BASE_URL).rstrip("/") self.timeout = timeout or self.DEFAULT_TIMEOUT - self.max_retries = max_retries or self.DEFAULT_MAX_RETRIES - self.retry_on = retry_on or self.DEFAULT_RETRY_CODES + # Explicit None checks, not `or`: an explicit max_retries=0 used to + # become 3 and an explicit retry_on=[] used to become the default status + # list, silently discarding what the caller asked for (#104). + self.max_retries = ( + self.DEFAULT_MAX_RETRIES if max_retries is None else validated_max_retries(max_retries) + ) + self.retry_on = ( + list(self.DEFAULT_RETRY_CODES) if retry_on is None else list(retry_on) + ) # Initialize retry strategy self._retry_strategy = RetryStrategy(max_retries=self.max_retries, retry_on=self.retry_on) @@ -205,6 +224,7 @@ def request( params: Optional[Dict[str, Any]] = None, json_data: Optional[Dict[str, Any]] = None, timeout: Optional[float] = None, + idempotent: Optional[bool] = None, **kwargs, ) -> Dict[str, Any]: """Make HTTP request to API. @@ -218,6 +238,9 @@ def request( params: Query parameters json_data: JSON body data timeout: Request timeout in seconds. If None, uses client's default timeout. + idempotent: Assert that repeating this request is safe. Without it, + POST and PATCH are sent exactly once and never replayed after an + ambiguous outcome (#104). **kwargs: Additional httpx request arguments Returns: @@ -274,18 +297,28 @@ def request( ) # Auto-retry with Retry-After if we have attempts left - if self._retry_strategy.should_retry(attempt, 429, response.headers): - try: - wait_time = min(float(retry_after), 60.0) - except (TypeError, ValueError): - wait_time = self._retry_strategy.calculate_wait_time(attempt) + if self._retry_strategy.should_retry( + attempt, 429, response.headers, method=method, idempotent=idempotent + ): + # Bounded in BOTH directions: a server Retry-After of + # 31612 would park the process for 8.8 hours, and a + # negative one makes time.sleep() raise (#104). + wait_time = self._retry_strategy.bounded_wait( + retry_after, self._retry_strategy.calculate_wait_time(attempt) + ) logger.info( f"Rate limited. Retrying in {wait_time}s (attempt {attempt + 1}/{self.max_retries})" ) time.sleep(wait_time) continue elif response.status_code >= 500: - if self._retry_strategy.should_retry(attempt, response.status_code, response.headers): + if self._retry_strategy.should_retry( + attempt, + response.status_code, + response.headers, + method=method, + idempotent=idempotent, + ): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, @@ -306,13 +339,17 @@ def request( api_key=self.api_key, timeout=effective_timeout, ) - if self._retry_strategy.should_retry_on_exception(attempt): + if self._retry_strategy.should_retry_on_exception( + attempt, method=method, idempotent=idempotent + ): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, "Request timeout", wait_time, is_async=False ) time.sleep(wait_time) continue + if not self._retry_strategy.is_replay_safe(method, idempotent): + raise mark_ambiguous_write(last_exception, method) logger.error(f"Request timed out after {self.max_retries} attempts") raise last_exception except httpx.RequestError as error: @@ -320,7 +357,9 @@ def request( error, api_key=self.api_key, ) - if self._retry_strategy.should_retry_on_exception(attempt): + if self._retry_strategy.should_retry_on_exception( + attempt, method=method, idempotent=idempotent + ): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, @@ -330,6 +369,8 @@ def request( ) time.sleep(wait_time) continue + if not self._retry_strategy.is_replay_safe(method, idempotent): + raise mark_ambiguous_write(last_exception, method) logger.error( f"Request failed after {self.max_retries} attempts: " f"{error.__class__.__name__}" @@ -354,6 +395,7 @@ def request_with_headers( params: Optional[Dict[str, Any]] = None, json_data: Optional[Dict[str, Any]] = None, timeout: Optional[float] = None, + idempotent: Optional[bool] = None, **kwargs, ) -> Tuple[Dict[str, Any], httpx.Headers]: """Make HTTP request and return (json_body, headers) tuple. @@ -388,18 +430,28 @@ def request_with_headers( if response.status_code == 429: retry_after = response.headers.get("Retry-After") - if self._retry_strategy.should_retry(attempt, 429, response.headers): - try: - wait_time = min(float(retry_after), 60.0) - except (TypeError, ValueError): - wait_time = self._retry_strategy.calculate_wait_time(attempt) + if self._retry_strategy.should_retry( + attempt, 429, response.headers, method=method, idempotent=idempotent + ): + # Bounded in BOTH directions: a server Retry-After of + # 31612 would park the process for 8.8 hours, and a + # negative one makes time.sleep() raise (#104). + wait_time = self._retry_strategy.bounded_wait( + retry_after, self._retry_strategy.calculate_wait_time(attempt) + ) logger.info( f"Rate limited. Retrying in {wait_time}s (attempt {attempt + 1}/{self.max_retries})" ) time.sleep(wait_time) continue elif response.status_code >= 500: - if self._retry_strategy.should_retry(attempt, response.status_code, response.headers): + if self._retry_strategy.should_retry( + attempt, + response.status_code, + response.headers, + method=method, + idempotent=idempotent, + ): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, @@ -420,20 +472,26 @@ def request_with_headers( api_key=self.api_key, timeout=effective_timeout, ) - if self._retry_strategy.should_retry_on_exception(attempt): + if self._retry_strategy.should_retry_on_exception( + attempt, method=method, idempotent=idempotent + ): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, "Request timeout", wait_time, is_async=False ) time.sleep(wait_time) continue + if not self._retry_strategy.is_replay_safe(method, idempotent): + raise mark_ambiguous_write(last_exception, method) raise last_exception except httpx.RequestError as error: last_exception = error_from_exception( error, api_key=self.api_key, ) - if self._retry_strategy.should_retry_on_exception(attempt): + if self._retry_strategy.should_retry_on_exception( + attempt, method=method, idempotent=idempotent + ): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, @@ -443,6 +501,8 @@ def request_with_headers( ) time.sleep(wait_time) continue + if not self._retry_strategy.is_replay_safe(method, idempotent): + raise mark_ambiguous_write(last_exception, method) raise last_exception if last_exception: diff --git a/oilpriceapi/retry.py b/oilpriceapi/retry.py index 0a9c5fe..27432bd 100644 --- a/oilpriceapi/retry.py +++ b/oilpriceapi/retry.py @@ -6,6 +6,47 @@ logger = logging.getLogger(__name__) +AMBIGUOUS_WRITE_NOTE = ( + "This {method} was NOT retried: the server may have already processed it, " + "and replaying it could create a duplicate. Check whether the write landed " + "before sending it again. Pass idempotent=True to request() if repeating " + "this call is safe." +) + + +def validated_max_retries(value: object) -> int: + """Validate an explicit ``max_retries``. + + ``max_retries`` counts total ATTEMPTS, not retries after the first — that + is what the clients have always documented and what the request loop does + (``for attempt in range(self.max_retries)``). One attempt is the minimum; + zero attempts would send nothing. This used to be swallowed by an ``or`` + default, which silently turned an explicit 0 into 3. + """ + from .exceptions import ConfigurationError + + if isinstance(value, bool) or not isinstance(value, int): + raise ConfigurationError( + f"max_retries must be an int counting total attempts, got " + f"{type(value).__name__}. Pass max_retries=1 for a single attempt " + f"with no retries." + ) + if value < 1: + raise ConfigurationError( + f"max_retries counts total attempts and must be at least 1, got {value}. " + f"Pass max_retries=1 for a single attempt with no retries." + ) + return value + + +def mark_ambiguous_write(error, method: Optional[str]): + """Tell the caller the write was sent once and its outcome is unknown.""" + error.ambiguous_write = True + note = AMBIGUOUS_WRITE_NOTE.format(method=str(method or "request").upper()) + error.message = f"{error.message} {note}" + error.args = (error.message,) + return error + class RetryStrategy: """ @@ -29,7 +70,9 @@ def __init__( jitter: Add randomized jitter to backoff to prevent thundering herd (default: True) """ self.max_retries = max_retries - self.retry_on = retry_on or [500, 502, 503, 504] + # None means "not configured"; an explicit [] means "retry on no status + # code at all" and must survive (#104). + self.retry_on = [500, 502, 503, 504] if retry_on is None else list(retry_on) self.jitter = jitter # A 429 means two completely different things, and retrying is only correct @@ -50,11 +93,75 @@ def __init__( # recoverable hourly circuit breaker also emits exhausted/0. PERSISTENT_QUOTA_WINDOWS = frozenset({"daily_counter", "monthly_counter", "trial_counter"}) + # Replaying a request is only safe when repeating it has the same effect as + # doing it once. RFC 9110 calls that idempotent. POST and PATCH are not: + # a create the server committed just before the response was lost becomes + # two creates on replay -- two subscriptions, two webhooks, two charges. + # + # A timeout or a transport error is AMBIGUOUS, not a failure: the SDK cannot + # know whether the server processed the write. A 5xx is equally ambiguous, + # because a gateway can return 502 after the origin already committed. Both + # are therefore sent once for a non-idempotent method. + # + # 429 is the exception in the other direction: it is an outright refusal, so + # the write definitively did not happen and replay is safe. + IDEMPOTENT_METHODS = frozenset({"GET", "HEAD", "OPTIONS", "TRACE", "PUT", "DELETE"}) + + # Bound on any wait, whether computed by backoff or handed to us by the + # server in Retry-After. The keyless demo endpoint returns + # `retry-after: 31612`, which unbounded would block a process for 8.8 hours. + MAX_WAIT_SECONDS = 60.0 + + @classmethod + def is_replay_safe( + cls, + method: Optional[str] = None, + idempotent: Optional[bool] = None, + ) -> bool: + """ + May this request be sent again after an ambiguous outcome? + + Args: + method: HTTP method. ``None`` means the caller did not say, and is + treated as safe so the public RetryStrategy contract is + unchanged for existing callers. The SDK's own clients always + pass it. + idempotent: Caller's explicit assertion, which wins over the method. + Pass True for a write you know is safe to repeat (your own + server-side deduplication, a naturally idempotent endpoint). + + Returns: + True if the request may be replayed. + """ + if idempotent is not None: + return bool(idempotent) + if method is None: + return True + return str(method).upper() in cls.IDEMPOTENT_METHODS + + @classmethod + def bounded_wait(cls, seconds: object, fallback: float) -> float: + """ + Clamp a wait to [0, MAX_WAIT_SECONDS], falling back when unparseable. + + Both ends matter. Unbounded above, a server Retry-After can park a + process for hours; below zero, ``time.sleep()`` raises ValueError. + """ + try: + value = float(seconds) # type: ignore[arg-type] + except (TypeError, ValueError): + value = float(fallback) + if value != value: # NaN + value = float(fallback) + return max(0.0, min(value, cls.MAX_WAIT_SECONDS)) + def should_retry( self, attempt: int, status_code: int, headers: Optional[Mapping[str, str]] = None, + method: Optional[str] = None, + idempotent: Optional[bool] = None, ) -> bool: """ Determine if request should be retried. @@ -65,6 +172,9 @@ def should_retry( headers: Response headers. When they identify a durable counter window whose allowance is exhausted, the request is not retried because waiting briefly cannot help. + method: HTTP method, so a non-idempotent write is never replayed + after an ambiguous 5xx. + idempotent: Caller's explicit override of the method check. Returns: True if request should be retried, False otherwise @@ -78,6 +188,11 @@ def should_retry( if status_code == 429 and self.quota_exhausted(headers): return False + # A 429 refused the request outright, so replaying a write is safe. + # A 5xx may have committed it; do not replay. + if status_code != 429 and not self.is_replay_safe(method, idempotent): + return False + return True @classmethod @@ -103,17 +218,30 @@ def quota_exhausted(cls, headers: Optional[Mapping[str, str]]) -> bool: window = str(lookup.get("x-ratelimit-window", "")).strip().lower() return state == "exhausted" and window in cls.PERSISTENT_QUOTA_WINDOWS - def should_retry_on_exception(self, attempt: int) -> bool: + def should_retry_on_exception( + self, + attempt: int, + method: Optional[str] = None, + idempotent: Optional[bool] = None, + ) -> bool: """ Determine if request should be retried on exception. + A timeout or transport error is an ambiguous outcome, not a failure: + the server may have processed the request before the response was lost. + A non-idempotent write is therefore never replayed. + Args: attempt: Current attempt number (0-indexed) + method: HTTP method. Omitted means "unknown", treated as safe. + idempotent: Caller's explicit override of the method check. Returns: True if request should be retried, False otherwise """ - return attempt < self.max_retries - 1 + if attempt >= self.max_retries - 1: + return False + return self.is_replay_safe(method, idempotent) def calculate_wait_time(self, attempt: int) -> float: """ diff --git a/tests/unit/test_write_replay_and_retry_config.py b/tests/unit/test_write_replay_and_retry_config.py new file mode 100644 index 0000000..984b8d0 --- /dev/null +++ b/tests/unit/test_write_replay_and_retry_config.py @@ -0,0 +1,353 @@ +"""Automatic retry must never replay a non-idempotent write (#104). + +A POST that the server committed before the response was lost is ambiguous, not +failed. Replaying it double-creates: two subscriptions, two webhooks, two of +whatever the caller was writing. These tests count what reaches the transport. + +Also covers the constructor's `or` defaults, which silently turned +`max_retries=0` into 3 and `retry_on=[]` into the default status list, and the +bounding of a server `Retry-After`. +""" + +import asyncio +from unittest.mock import patch + +import httpx +import pytest + +from oilpriceapi import AsyncOilPriceAPI, OilPriceAPI +from oilpriceapi.exceptions import ConfigurationError, OilPriceAPIError +from oilpriceapi.retry import RetryStrategy + +# Not a credential: a fixture string, every request here hits a mock transport. +FIXTURE_KEY = "-".join(["fixture", "not", "a", "real", "key"]) + + +class _Counter: + """Mock transport that counts attempts and replays a scripted outcome.""" + + def __init__(self, outcome): + self.methods = [] + self._outcome = outcome + + def __call__(self, request): + self.methods.append(request.method) + return self._outcome(request) + + +def _timeout(request): + raise httpx.ReadTimeout("server committed, response lost", request=request) + + +def _connect_error(request): + raise httpx.ConnectError("connection reset", request=request) + + +def _status(code, headers=None): + def handler(request): + return httpx.Response(code, headers=headers or {}, json={"error": "nope"}) + + return handler + + +def _sync_client(counter, **kwargs): + client = OilPriceAPI(api_key=FIXTURE_KEY, **kwargs) + client._client = httpx.Client( + base_url=client.base_url, + headers=client.headers, + transport=httpx.MockTransport(counter), + ) + return client + + +def _async_client(counter, **kwargs): + client = AsyncOilPriceAPI(api_key=FIXTURE_KEY, **kwargs) + client._client = httpx.AsyncClient( + base_url=client.base_url, + headers=client.headers, + transport=httpx.MockTransport(counter), + ) + return client + + +# --------------------------------------------------------------------------- +# (a) non-idempotent writes are sent once +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("outcome", [_timeout, _connect_error, _status(503), _status(500)]) +@pytest.mark.parametrize("method", ["POST", "PATCH"]) +def test_write_is_sent_exactly_once(method, outcome): + counter = _Counter(outcome) + client = _sync_client(counter) + + with patch("time.sleep") as slept: + with pytest.raises(OilPriceAPIError): + client.request(method, "/v1/subscriptions", json_data={"code": "BRENT_CRUDE_USD"}) + + assert counter.methods == [method], f"write was replayed: {counter.methods}" + assert slept.call_count == 0 + + +@pytest.mark.parametrize("outcome", [_timeout, _status(503)]) +def test_async_write_is_sent_exactly_once(outcome): + counter = _Counter(outcome) + + async def scenario(): + client = _async_client(counter) + with pytest.raises(OilPriceAPIError): + await client.request("POST", "/v1/webhooks", json_data={"url": "https://x.example"}) + await client._client.aclose() + + with patch("asyncio.sleep") as slept: + asyncio.run(scenario()) + + assert counter.methods == ["POST"] + assert slept.call_count == 0 + + +def test_request_with_headers_does_not_replay_a_write(): + counter = _Counter(_timeout) + client = _sync_client(counter) + + with patch("time.sleep"): + with pytest.raises(OilPriceAPIError): + client.request_with_headers("POST", "/v1/webhooks", json_data={"url": "https://x"}) + + assert counter.methods == ["POST"] + + +def test_ambiguous_write_error_says_it_was_not_retried(): + counter = _Counter(_timeout) + client = _sync_client(counter) + + with pytest.raises(OilPriceAPIError) as excinfo: + client.request("POST", "/v1/subscriptions", json_data={"code": "BRENT_CRUDE_USD"}) + + message = str(excinfo.value).lower() + assert "not retried" in message or "not replayed" in message + assert getattr(excinfo.value, "ambiguous_write", False) is True + + +def test_caller_can_opt_into_replaying_a_write(): + """An explicit idempotent=True is the caller asserting the write is safe.""" + counter = _Counter(_timeout) + client = _sync_client(counter) + + with patch("time.sleep"): + with pytest.raises(OilPriceAPIError): + client.request("POST", "/v1/echo", json_data={}, idempotent=True) + + assert counter.methods == ["POST", "POST", "POST"] + + +# --------------------------------------------------------------------------- +# reads must keep recovering +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("method", ["GET", "HEAD", "PUT", "DELETE"]) +def test_idempotent_methods_still_retry_transient_failures(method): + counter = _Counter(_timeout) + client = _sync_client(counter) + + with patch("time.sleep"): + with pytest.raises(OilPriceAPIError): + client.request(method, "/v1/prices/latest") + + assert counter.methods == [method] * 3 + + +def test_get_recovers_after_a_transient_failure(): + state = {"n": 0} + + def flaky(request): + state["n"] += 1 + if state["n"] == 1: + raise httpx.ReadTimeout("transient", request=request) + return httpx.Response(200, json={"status": "success", "data": {"price": 1}}) + + counter = _Counter(flaky) + client = _sync_client(counter) + + with patch("time.sleep"): + body = client.request("GET", "/v1/prices/latest") + + assert body["data"]["price"] == 1 + assert counter.methods == ["GET", "GET"] + + +def test_a_429_still_retries_a_write(): + """A 429 is a refusal: the write definitively did not happen, so replay is safe.""" + counter = _Counter(_status(429, {"Retry-After": "1"})) + client = _sync_client(counter) + + with patch("time.sleep"): + with pytest.raises(OilPriceAPIError): + client.request("POST", "/v1/subscriptions", json_data={}) + + assert counter.methods == ["POST", "POST", "POST"] + + +# --------------------------------------------------------------------------- +# (b) explicit configuration must survive the constructor +# --------------------------------------------------------------------------- + + +def test_empty_retry_on_is_preserved(): + client = OilPriceAPI(api_key=FIXTURE_KEY, retry_on=[]) + assert client.retry_on == [] + assert client._retry_strategy.retry_on == [] + + +def test_empty_retry_on_actually_stops_status_retries(): + counter = _Counter(_status(503)) + client = _sync_client(counter, retry_on=[]) + + with patch("time.sleep"): + with pytest.raises(OilPriceAPIError): + client.request("GET", "/v1/prices/latest") + + assert counter.methods == ["GET"] + + +def test_async_empty_retry_on_is_preserved(): + client = AsyncOilPriceAPI(api_key=FIXTURE_KEY, retry_on=[]) + assert client.retry_on == [] + + +@pytest.mark.parametrize("bad", [0, -1, -5]) +def test_invalid_max_retries_fails_loudly(bad): + """max_retries counts ATTEMPTS. Zero attempts is not a thing; say so.""" + with pytest.raises(ConfigurationError) as excinfo: + OilPriceAPI(api_key=FIXTURE_KEY, max_retries=bad) + assert "attempt" in str(excinfo.value).lower() + + +@pytest.mark.parametrize("bad", ["3", 2.5, True]) +def test_non_integer_max_retries_fails_loudly(bad): + with pytest.raises(ConfigurationError): + OilPriceAPI(api_key=FIXTURE_KEY, max_retries=bad) + + +def test_async_invalid_max_retries_fails_loudly(): + with pytest.raises(ConfigurationError): + AsyncOilPriceAPI(api_key=FIXTURE_KEY, max_retries=0) + + +def test_max_retries_one_means_a_single_attempt(): + counter = _Counter(_status(503)) + client = _sync_client(counter, max_retries=1) + + with patch("time.sleep"): + with pytest.raises(OilPriceAPIError): + client.request("GET", "/v1/prices/latest") + + assert counter.methods == ["GET"] + + +# --------------------------------------------------------------------------- +# (c) Retry-After must be bounded in both directions +# --------------------------------------------------------------------------- + + +def test_retry_after_is_capped(): + """The keyless demo returns retry-after: 31612 — 8.8 hours of blocked process.""" + counter = _Counter(_status(429, {"Retry-After": "31612"})) + client = _sync_client(counter) + waits = [] + + with patch("time.sleep", side_effect=waits.append): + with pytest.raises(OilPriceAPIError): + client.request("GET", "/v1/prices/latest") + + assert waits, "expected at least one retry" + assert max(waits) <= 60.0 + + +@pytest.mark.parametrize("value", ["-30", "-1"]) +def test_negative_retry_after_never_reaches_sleep(value): + """time.sleep() raises ValueError on a negative argument.""" + counter = _Counter(_status(429, {"Retry-After": value})) + client = _sync_client(counter) + waits = [] + + with patch("time.sleep", side_effect=waits.append): + with pytest.raises(OilPriceAPIError): + client.request("GET", "/v1/prices/latest") + + assert all(w >= 0 for w in waits), waits + + +def test_async_retry_after_is_bounded(): + counter = _Counter(_status(429, {"Retry-After": "31612"})) + waits = [] + + async def scenario(): + client = _async_client(counter) + with pytest.raises(OilPriceAPIError): + await client.request("GET", "/v1/prices/latest") + await client._client.aclose() + + async def fake_sleep(seconds): + waits.append(seconds) + + with patch("asyncio.sleep", side_effect=fake_sleep): + asyncio.run(scenario()) + + assert waits and max(waits) <= 60.0 and min(waits) >= 0 + + +def test_request_with_headers_bounds_retry_after(): + counter = _Counter(_status(429, {"Retry-After": "31612"})) + client = _sync_client(counter) + waits = [] + + with patch("time.sleep", side_effect=waits.append): + with pytest.raises(OilPriceAPIError): + client.request_with_headers("GET", "/v1/prices/latest") + + assert waits and max(waits) <= 60.0 + + +# --------------------------------------------------------------------------- +# already-fixed behaviour that must not regress +# --------------------------------------------------------------------------- + + +def test_durable_quota_exhaustion_is_still_never_retried(): + headers = { + "Retry-After": "600", + "X-RateLimit-State": "exhausted", + "X-RateLimit-Window": "monthly_counter", + } + counter = _Counter(_status(429, headers)) + client = _sync_client(counter) + + with patch("time.sleep") as slept: + with pytest.raises(OilPriceAPIError): + client.request("GET", "/v1/prices/latest") + + assert counter.methods == ["GET"] + assert slept.call_count == 0 + + +# --------------------------------------------------------------------------- +# RetryStrategy keeps its existing public contract +# --------------------------------------------------------------------------- + + +def test_retry_strategy_defaults_to_replay_safe_when_method_is_unknown(): + """The public helper is unchanged for callers that pass no method.""" + strategy = RetryStrategy(max_retries=3) + assert strategy.should_retry_on_exception(0) is True + assert strategy.should_retry(0, 500) is True + + +def test_retry_strategy_knows_which_methods_are_replay_safe(): + strategy = RetryStrategy(max_retries=3, retry_on=[429, 500, 502, 503, 504]) + assert strategy.should_retry_on_exception(0, method="GET") is True + assert strategy.should_retry_on_exception(0, method="post") is False + assert strategy.should_retry_on_exception(0, method="POST", idempotent=True) is True + assert strategy.should_retry(0, 500, method="POST") is False + assert strategy.should_retry(0, 429, method="POST") is True