fix(client): correct 409 classification, cap Retry-After, keep server errors

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)

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