diff --git a/src/dualentry_cli/client.py b/src/dualentry_cli/client.py index 9c5fbf0..9e07d30 100644 --- a/src/dualentry_cli/client.py +++ b/src/dualentry_cli/client.py @@ -18,8 +18,8 @@ # UnsupportedProtocol, DecodingError and TooManyRedirects: those fail the same # way every time, so retrying only delays the error the user needs to see. _RETRYABLE_EXCEPTIONS = (httpx.TimeoutException, httpx.NetworkError, httpx.RemoteProtocolError) -_MAX_RETRIES = 3 _RETRY_DELAYS = [1, 2, 4] # Exponential backoff: 1s, 2s, 4s +_MAX_RETRIES = len(_RETRY_DELAYS) # The API replays the original response for a repeated Idempotency-Key instead of # running the operation again, so a retried write cannot create a duplicate record. @@ -31,6 +31,8 @@ # https://docs.dualentry.com/developers/guides/rate-limiting _RETRY_AFTER_HEADER = "Retry-After" +_MAX_RETRY_AFTER = 60 + def _retry_after_seconds(response: httpx.Response) -> int | None: """Seconds from the Retry-After header, or None if absent or unusable.""" @@ -55,10 +57,31 @@ def _is_retryable(response: httpx.Response) -> bool: https://docs.dualentry.com/developers/guides/idempotency-and-write-validation """ if response.status_code == 409: - return _retry_after_seconds(response) is not None + return _RETRY_AFTER_HEADER in response.headers return response.status_code in _RETRYABLE_STATUS_CODES +def _server_detail(response: httpx.Response) -> str: + try: + payload = response.json() + except Exception: + return response.text.strip() + errors = payload.get("errors", payload) if isinstance(payload, dict) else payload + if isinstance(errors, dict): + messages = [] + for field, msgs in errors.items(): + if isinstance(msgs, list): + messages.extend(str(msg) for msg in msgs) + else: + messages.append(f"{field}: {msgs}") + return "; ".join(messages) + return str(errors) + + +def _explain(message: str, detail: str) -> str: + return f"{message} Server said: {detail}" if detail else message + + class APIError(Exception): def __init__(self, status_code: int, detail: str): self.status_code = status_code @@ -88,7 +111,7 @@ def from_env(cls, api_url: str, *, retry: bool = False) -> DualEntryClient: raise ValueError(msg) return cls(api_url=api_url, api_key=api_key, retry=retry) - def _handle_response(self, response: httpx.Response) -> dict: + def _handle_response(self, response: httpx.Response, *, sent_idempotency_key: bool = False) -> dict: status = response.status_code if status == 401: raise APIError(401, "API key is invalid or expired. Run: dualentry auth login") @@ -104,19 +127,30 @@ def _handle_response(self, response: httpx.Response) -> dict: errors = response.text raise APIError(422, f"Validation error: {errors}") if status == 409: - wait = _retry_after_seconds(response) - if wait is not None: - raise APIError(409, f"The first request with this idempotency key is still being processed. Retry in {wait:g}s with the same key.") - raise APIError( - 409, "The original response is too large to replay (over 256 KB). The write was not repeated - check whether the record already exists before sending it again." - ) + detail = _server_detail(response) + if _RETRY_AFTER_HEADER in response.headers: + wait = _retry_after_seconds(response) + when = f"Retry in {wait}s with the same key." if wait is not None else "Retry shortly with the same key." + raise APIError(409, _explain(f"The first request with this idempotency key is still being processed. {when}", detail)) + if sent_idempotency_key: + raise APIError( + 409, + _explain( + "The original response is too large to replay (over 256 KB). " + "The write was not repeated - check whether the record already exists before sending it again.", + detail, + ), + ) + raise APIError(409, detail or "Conflict.") if status == 429: wait = _retry_after_seconds(response) if wait is not None: - raise APIError(429, f"Rate limited. Retry after {wait:g}s.") + raise APIError(429, f"Rate limited. Retry after {wait}s.") raise APIError(429, "Rate limited. Please wait and try again.") if status >= 500: - raise APIError(status, f"Server error ({status}). The API may be temporarily unavailable.") + wait = _retry_after_seconds(response) + when = f" The server asked to retry after {wait}s." if wait is not None else "" + raise APIError(status, f"Server error ({status}). The API may be temporarily unavailable.{when}") if status >= 400: try: detail = response.json() @@ -136,7 +170,8 @@ def _handle_response(self, response: httpx.Response) -> dict: def _request(self, method: str, path: str, **kwargs) -> dict: method = method.upper() - if method in _IDEMPOTENCY_METHODS: + keyed = method in _IDEMPOTENCY_METHODS + if keyed: # One key per logical request, deliberately generated here rather than # per attempt: reusing it across retries is what makes a retry safe. headers = dict(kwargs.pop("headers", None) or {}) @@ -145,27 +180,29 @@ def _request(self, method: str, path: str, **kwargs) -> dict: if not self._retry: response = self._client.request(method, path, **kwargs) - return self._handle_response(response) + return self._handle_response(response, sent_idempotency_key=keyed) # Retry logic with visible feedback - for attempt in range(_MAX_RETRIES): + for attempt, backoff in enumerate(_RETRY_DELAYS): retry_after = None try: response = self._client.request(method, path, **kwargs) if not _is_retryable(response): - return self._handle_response(response) + return self._handle_response(response, sent_idempotency_key=keyed) retry_after = _retry_after_seconds(response) + if retry_after is not None and retry_after > _MAX_RETRY_AFTER: + return self._handle_response(response, sent_idempotency_key=keyed) except _RETRYABLE_EXCEPTIONS: pass # every retry waits, including the one after the loop - delay = retry_after if retry_after is not None else _RETRY_DELAYS[attempt] - print(f"\033[33mRetrying in {delay:g}s... (attempt {attempt + 2}/{_MAX_RETRIES + 1})\033[0m", file=sys.stderr) + delay = retry_after if retry_after is not None else backoff + print(f"\033[33mRetrying in {delay}s... (attempt {attempt + 2}/{_MAX_RETRIES + 1})\033[0m", file=sys.stderr) time.sleep(delay) # Final attempt response = self._client.request(method, path, **kwargs) - return self._handle_response(response) + return self._handle_response(response, sent_idempotency_key=keyed) def get(self, path: str, params: dict[str, Any] | None = None) -> dict: return self._request("GET", path, params=params) diff --git a/tests/test_client.py b/tests/test_client.py index 675a0d5..18974a7 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -408,3 +408,96 @@ def test_storage_unavailable_is_retried_with_the_same_key(self): assert data == {"internal_id": 9} keys = {c.request.headers["Idempotency-Key"] for c in route.calls} assert len(keys) == 1 + + +class TestRetryAfterCeilingAndConflictDetail: + BASE = "https://api.dualentry.com/public/v2" + + @pytest.fixture + def sleeps(self, monkeypatch): + recorded = [] + monkeypatch.setattr("dualentry_cli.client.time", SimpleNamespace(sleep=recorded.append)) + return recorded + + @staticmethod + def _client(): + from dualentry_cli.client import DualEntryClient + + return DualEntryClient(api_url="https://api.dualentry.com", api_key="test_key", retry=True) + + @pytest.mark.parametrize("unreadable", ["soon", "2.5", "-5", ""]) + @respx.mock + def test_conflict_with_unreadable_retry_after_still_retries(self, sleeps, unreadable): + route = respx.post(f"{self.BASE}/invoices/").mock( + side_effect=[ + httpx.Response(409, headers={"Retry-After": unreadable}, json={}), + httpx.Response(201, json={"internal_id": 5}), + ] + ) + + data = self._client().post("/invoices/", json={}) + + assert data == {"internal_id": 5} + assert route.call_count == 2 + assert sleeps == [1], f"{unreadable!r} should fall back to the backoff, not cancel the retry" + assert len({c.request.headers["Idempotency-Key"] for c in route.calls}) == 1 + + @pytest.mark.parametrize("status", [409, 429, 503]) + @respx.mock + def test_retry_after_beyond_the_ceiling_is_reported_not_slept_through(self, sleeps, status): + from dualentry_cli.client import APIError + + route = respx.post(f"{self.BASE}/invoices/").mock(return_value=httpx.Response(status, headers={"Retry-After": "3600"}, json={})) + + with pytest.raises(APIError) as exc: + self._client().post("/invoices/", json={}) + + assert route.call_count == 1, "no point retrying on a schedule we refuse to wait for" + assert sleeps == [], "the whole point: we never sleep 3600s" + assert exc.value.status_code == status + assert "3600" in exc.value.detail, "the user still needs to know how long the server asked for" + + @respx.mock + def test_retry_after_at_the_ceiling_is_still_honoured(self, sleeps): + from dualentry_cli.client import _MAX_RETRY_AFTER + + respx.post(f"{self.BASE}/invoices/").mock( + side_effect=[ + httpx.Response(429, headers={"Retry-After": str(_MAX_RETRY_AFTER)}, json={}), + httpx.Response(201, json={"internal_id": 6}), + ] + ) + + self._client().post("/invoices/", json={}) + + assert sleeps == [_MAX_RETRY_AFTER], "the ceiling itself is allowed" + + @respx.mock + def test_conflict_without_a_key_keeps_the_server_message(self, sleeps): + from dualentry_cli.client import APIError + + respx.get(f"{self.BASE}/invoices/").mock(return_value=httpx.Response(409, json={"errors": {"__all__": ["period is closed"]}})) + + with pytest.raises(APIError) as exc: + self._client().get("/invoices/") + + assert exc.value.detail == "period is closed" + assert "256 KB" not in exc.value.detail + assert sleeps == [] + + @respx.mock + def test_conflict_on_a_write_keeps_both_the_guidance_and_the_server_message(self): + from dualentry_cli.client import APIError + + respx.post(f"{self.BASE}/invoices/").mock(return_value=httpx.Response(409, json={"errors": {"__all__": ["response cannot be replayed"]}})) + + with pytest.raises(APIError) as exc: + self._client().post("/invoices/", json={}) + + assert "256 KB" in exc.value.detail, "the write did carry a key, so the guidance applies" + assert "response cannot be replayed" in exc.value.detail, "and the server's own words survive" + + def test_backoff_table_and_retry_count_cannot_drift(self): + from dualentry_cli.client import _MAX_RETRIES, _RETRY_DELAYS + + assert len(_RETRY_DELAYS) == _MAX_RETRIES