diff --git a/oilpriceapi/async_client.py b/oilpriceapi/async_client.py index eced494..8784191 100644 --- a/oilpriceapi/async_client.py +++ b/oilpriceapi/async_client.py @@ -55,7 +55,7 @@ class AsyncOilPriceAPI: api_key: API key for authentication base_url: Base URL for API timeout: Request timeout in seconds - max_retries: Maximum retry attempts + max_retries: Maximum request attempts Example: >>> async with AsyncOilPriceAPI() as client: @@ -238,7 +238,7 @@ async def request( ) # Auto-retry with Retry-After if we have attempts left - if self._retry_strategy.should_retry(attempt, 429): + if self._retry_strategy.should_retry(attempt, 429, response.headers): try: wait_time = min(float(retry_after), 60.0) except (TypeError, ValueError): @@ -249,7 +249,7 @@ async def request( await asyncio.sleep(wait_time) continue elif response.status_code >= 500: - if self._retry_strategy.should_retry(attempt, response.status_code): + if self._retry_strategy.should_retry(attempt, response.status_code, response.headers): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, diff --git a/oilpriceapi/client.py b/oilpriceapi/client.py index 86f9cc0..ceb8991 100644 --- a/oilpriceapi/client.py +++ b/oilpriceapi/client.py @@ -63,7 +63,7 @@ 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 retry attempts for failed requests. Defaults to 3. + max_retries: Maximum request attempts for failed requests. Defaults to 3. retry_on: Status codes to retry on. Defaults to [429, 500, 502, 503, 504]. Example: @@ -274,7 +274,7 @@ def request( ) # Auto-retry with Retry-After if we have attempts left - if self._retry_strategy.should_retry(attempt, 429): + if self._retry_strategy.should_retry(attempt, 429, response.headers): try: wait_time = min(float(retry_after), 60.0) except (TypeError, ValueError): @@ -285,7 +285,7 @@ def request( time.sleep(wait_time) continue elif response.status_code >= 500: - if self._retry_strategy.should_retry(attempt, response.status_code): + if self._retry_strategy.should_retry(attempt, response.status_code, response.headers): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, @@ -388,7 +388,7 @@ def request_with_headers( if response.status_code == 429: retry_after = response.headers.get("Retry-After") - if self._retry_strategy.should_retry(attempt, 429): + if self._retry_strategy.should_retry(attempt, 429, response.headers): try: wait_time = min(float(retry_after), 60.0) except (TypeError, ValueError): @@ -399,7 +399,7 @@ def request_with_headers( time.sleep(wait_time) continue elif response.status_code >= 500: - if self._retry_strategy.should_retry(attempt, response.status_code): + if self._retry_strategy.should_retry(attempt, response.status_code, response.headers): wait_time = self._retry_strategy.calculate_wait_time(attempt) self._retry_strategy.log_retry( attempt, diff --git a/oilpriceapi/retry.py b/oilpriceapi/retry.py index 8e108a1..0a9c5fe 100644 --- a/oilpriceapi/retry.py +++ b/oilpriceapi/retry.py @@ -2,7 +2,7 @@ import logging import random -from typing import List, Optional +from typing import List, Mapping, Optional logger = logging.getLogger(__name__) @@ -24,7 +24,7 @@ def __init__( Initialize retry strategy. Args: - max_retries: Maximum number of retry attempts + max_retries: Maximum number of request attempts retry_on: HTTP status codes to retry on (default: [500, 502, 503, 504]) jitter: Add randomized jitter to backoff to prevent thundering herd (default: True) """ @@ -32,21 +32,76 @@ def __init__( self.retry_on = retry_on or [500, 502, 503, 504] self.jitter = jitter - def should_retry(self, attempt: int, status_code: int) -> bool: + # A 429 means two completely different things, and retrying is only correct + # for one of them: + # + # "you are bursting" -> wait and retry. Correct. + # "you are out of quota" -> retrying CANNOT succeed until the billing + # period resets. Two retries produce two more + # refusals and nothing else. + # + # This method used to take the status code alone, so it could not tell them + # apart and always retried. Measured against production over 30 days, free + # accounts on this SDK were rate-limited on 26.2% of requests against 15.6% + # for the Node SDK on the same tier -- 1.7x worse, self-inflicted. + # + # The API identifies durable quota exhaustion with both `state=exhausted` + # and a counter-backed window. State or remaining alone are ambiguous: the + # recoverable hourly circuit breaker also emits exhausted/0. + PERSISTENT_QUOTA_WINDOWS = frozenset({"daily_counter", "monthly_counter", "trial_counter"}) + + def should_retry( + self, + attempt: int, + status_code: int, + headers: Optional[Mapping[str, str]] = None, + ) -> bool: """ Determine if request should be retried. Args: attempt: Current attempt number (0-indexed) status_code: HTTP status code from response + headers: Response headers. When they identify a durable counter + window whose allowance is exhausted, the request is not + retried because waiting briefly cannot help. Returns: True if request should be retried, False otherwise """ - return ( - status_code in self.retry_on - and attempt < self.max_retries - 1 - ) + if attempt >= self.max_retries - 1: + return False + if status_code not in self.retry_on: + return False + + # Only 429 carries a remedy. Server errors are always worth a retry. + if status_code == 429 and self.quota_exhausted(headers): + return False + + return True + + @classmethod + def quota_exhausted(cls, headers: Optional[Mapping[str, str]]) -> bool: + """ + Has the caller run out of allowance, as opposed to merely bursting? + + Requires `X-RateLimit-State: exhausted` together with one of the API's + durable counter windows. `state=exhausted` and `remaining=0` cannot be + used independently because the recoverable hourly circuit breaker + deliberately emits both values as well. + + Returns False when headers are absent or unparseable -- an unknown state + must behave exactly as before this change, so a missing header can never + turn a retryable burst into a hard failure. + """ + if not headers: + return False + + lookup = {str(k).lower(): v for k, v in headers.items()} + + state = str(lookup.get("x-ratelimit-state", "")).strip().lower() + 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: """ diff --git a/tests/test_client.py b/tests/test_client.py index aff66a8..f88b973 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -163,6 +163,8 @@ def test_rate_limit_error(self, mock_request): "X-RateLimit-Limit": "1000", "X-RateLimit-Remaining": "0", "X-RateLimit-Reset": "1705320000", + "X-RateLimit-State": "exhausted", + "X-RateLimit-Window": "monthly_counter", } mock_response.json.return_value = {"error": "Rate limit exceeded"} mock_request.return_value = mock_response @@ -175,6 +177,7 @@ def test_rate_limit_error(self, mock_request): assert error.status_code == 429 assert error.limit == "1000" assert error.remaining == "0" + assert mock_request.call_count == 1 @patch('httpx.Client.request') def test_data_not_found_error(self, mock_request): diff --git a/tests/test_retry_remedy.py b/tests/test_retry_remedy.py new file mode 100644 index 0000000..3ae2632 --- /dev/null +++ b/tests/test_retry_remedy.py @@ -0,0 +1,124 @@ +"""Retry must distinguish "you are bursting" from "you are out of quota". + +Measured against production over 30 days: free accounts on this SDK were +rate-limited on 26.2% of requests against 15.6% for the Node SDK on the same +tier. `should_retry` took the status code alone, so a quota-exhausted 429 -- +which cannot succeed until the billing period resets -- was tried three times, +turning one refusal into three. + +The API distinguishes the two cases with the combination of +`X-RateLimit-State` and `X-RateLimit-Window` (oilpriceapi-api#5664). The state +alone is ambiguous: both a durable quota wall and the recoverable hourly +circuit breaker use `exhausted`. +""" + +import pytest + +from oilpriceapi.retry import RetryStrategy + + +@pytest.fixture +def strategy(): + return RetryStrategy(max_retries=3, retry_on=[429, 500, 502, 503, 504]) + + +class TestQuotaExhaustedIsNotRetried: + @pytest.mark.parametrize("window", ["daily_counter", "monthly_counter", "trial_counter"]) + def test_durable_quota_windows_stop_the_retry(self, strategy, window): + headers = { + "X-RateLimit-State": "exhausted", + "X-RateLimit-Window": window, + "X-RateLimit-Remaining": "0", + } + assert strategy.should_retry(0, 429, headers) is False + + def test_header_name_is_matched_case_insensitively(self, strategy): + # HTTP header names are case-insensitive and clients normalise them + # differently. Matching on exact case would silently disable this. + headers = { + "x-ratelimit-state": "EXHAUSTED", + "x-ratelimit-window": "MONTHLY_COUNTER", + } + assert strategy.should_retry(0, 429, headers) is False + + +class TestBurstingIsStillRetried: + def test_hourly_circuit_breaker_preserves_retry_behavior(self, strategy): + # The API deliberately emits `state=exhausted` for this recoverable + # safety limit. Looking at state or remaining alone would suppress the + # existing bounded retry path even though this is not a durable quota. + headers = { + "X-RateLimit-State": "exhausted", + "X-RateLimit-Window": "hourly_circuit_breaker", + "X-RateLimit-Remaining": "0", + "Retry-After": "1050", + } + assert strategy.should_retry(0, 429, headers) is True + + @pytest.mark.parametrize( + "headers", + [ + {"X-RateLimit-State": "exhausted"}, + {"X-RateLimit-Remaining": "0"}, + { + "X-RateLimit-State": "unavailable", + "X-RateLimit-Window": "enforcement_check", + }, + { + "X-RateLimit-State": "exhausted", + "X-RateLimit-Window": "future_counter_contract", + }, + ], + ) + def test_ambiguous_or_recoverable_metadata_fails_open(self, strategy, headers): + assert strategy.should_retry(0, 429, headers) is True + + def test_retries_when_headers_are_absent(self, strategy): + # The critical safety property. An unknown state must behave exactly as + # it did before this change, so a missing header can never convert a + # retryable burst into a hard failure. + assert strategy.should_retry(0, 429, None) is True + assert strategy.should_retry(0, 429, {}) is True + + def test_server_errors_retry_regardless_of_rate_limit_headers(self, strategy): + # A 500 carries no remedy. Exhausted allowance must not suppress it. + headers = {"X-RateLimit-State": "exhausted"} + for code in (500, 502, 503, 504): + assert strategy.should_retry(0, code, headers) is True + + +class TestExistingBehaviourUnchanged: + def test_attempt_budget_still_respected(self, strategy): + assert strategy.should_retry(2, 429, None) is False + + def test_non_retryable_status_still_not_retried(self, strategy): + # 402 must never be retried: it is a payment problem, not a timing one. + assert strategy.should_retry(0, 402, None) is False + assert strategy.should_retry(0, 404, None) is False + + def test_two_argument_calls_still_work(self, strategy): + # `headers` is optional so third-party callers of this public method do + # not break on upgrade. + assert strategy.should_retry(0, 500) is True + assert strategy.should_retry(0, 404) is False + + +class TestTheProductionScenario: + def test_a_free_account_out_of_quota_makes_exactly_one_request(self, strategy): + """The defect, stated as a test. + + A free account that has spent its 200 daily requests previously issued + 1 request + 2 retries = 3 refusals per call. It must now issue 1. + """ + headers = { + "X-RateLimit-Limit": "200", + "X-RateLimit-Remaining": "0", + "X-RateLimit-State": "exhausted", + "X-RateLimit-Window": "daily_counter", + } + attempts = sum( + 1 + for attempt in range(strategy.max_retries) + if strategy.should_retry(attempt, 429, headers) + ) + assert attempts == 0, "a quota-exhausted 429 must not be retried at all" diff --git a/tests/unit/test_async_client.py b/tests/unit/test_async_client.py index 2b999ef..471163c 100644 --- a/tests/unit/test_async_client.py +++ b/tests/unit/test_async_client.py @@ -240,6 +240,8 @@ async def test_rate_limit_error(self, mock_request, api_key): "X-RateLimit-Limit": "1000", "X-RateLimit-Remaining": "0", "X-RateLimit-Reset": "1705320000", + "X-RateLimit-State": "exhausted", + "X-RateLimit-Window": "monthly_counter", } mock_response.json = Mock(return_value={"error": "Rate limit exceeded"}) mock_request.return_value = mock_response @@ -249,6 +251,7 @@ async def test_rate_limit_error(self, mock_request, api_key): await client.prices.get("BRENT_CRUDE_USD") assert exc_info.value.status_code == 429 + assert mock_request.call_count == 1 @pytest.mark.asyncio @patch('httpx.AsyncClient.request') @@ -356,4 +359,4 @@ async def make_response(is_historical): assert isinstance(current1, Price) assert isinstance(history, HistoricalResponse) - assert isinstance(current2, Price) \ No newline at end of file + assert isinstance(current2, Price)