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

[mykhaylob-de]

Summary

Follow-up to #17, stacked on its branch so the diff below is only the changes on top.

#17 is the right change — one key per logical request, generated before the retry loop, is exactly what makes a retried POST safe. This PR fixes three problems in the Retry-After / 409 handling around it. Each behaviour below was checked against the server itself (core/idempotency.py, core/rate_limit.py), not against the published docs alone.

1. The two 409 meanings were told apart by whether Retry-After parses, not whether it is present.

The server encodes the distinction in presence: the in-flight 409 sets the header, the too-large 409 omits it entirely.

# core/idempotency.py — first request still running
response = _error(f"A request with this {IDEMPOTENCY_KEY_HEADER} is still being processed…", CONFLICT)
response["Retry-After"] = "1"

# core/idempotency.py — original response too large to replay: no Retry-After at all
return _error("The original response was too large to store, so it cannot be replayed…", CONFLICT)

_is_retryable asked _retry_after_seconds(...) is not None, which conflates "absent" with "present but unreadable". On an unreadable value it reported "The write was not repeated" for a write that is in flight and will very likely commit — the opposite of true. Presence now decides the meaning; the value only sets the delay, falling back to the backoff when it cannot be read.

2. Retry-After reached time.sleep() uncapped.

Nothing bounded what we would sleep for. Past _MAX_RETRY_AFTER (60s) we now stop retrying and report the wait, which the user can act on. Sleeping 60s three times to hit the same 429 is not better than saying so.

3. Every 409 got the idempotency explanation, and the server's own error was dropped.

Including on GET, which never carries a key. On main a 409 fell through to the generic handler and surfaced the API's errors payload, so this was a regression. The server sends real text on both 409s; it is now preserved either way, and the explanation is scoped to requests that actually sent a key.

Changes

In src/dualentry_cli/client.py:

• _has_retry_after() — presence of the header, independent of whether the value is readable. _is_retryable() uses it for the 409 split.
• _MAX_RETRY_AFTER = 60 — ceiling on a server-supplied wait. Beyond it, _request hands the response to _handle_response instead of sleeping.
• _server_detail() / _explain() — flatten the API's error payload and append it to our guidance.
• _handle_response(..., sent_idempotency_key=...) — three distinct 409 messages: in-flight, too-large-to-replay (only when a key was sent), and plain conflict.
• The 5xx message now carries the wait too, so declining to sleep through a long Retry-After never hides the number.
• _MAX_RETRIES derives from len(_RETRY_DELAYS). Indexing one by a range built from the other is an IndexError waiting for someone to raise the cap.
• Dropped :g on two format strings; both values are always int.

_retry_after_seconds is unchanged from #17 — integer-only, as the server only ever emits integers.

End-to-end verification

Not mocks. The real dualentry binary as a subprocess → a proxy that injects the failure → the real Django app (PublicApiIdempotencyMiddleware) on real Postgres and the real idempotency cache. The proxy exists to do the one thing a test client cannot: let the write commit upstream and then lose the response, which is what a 502 actually is.

Both builds ran against the identical server: old = #17's branch, new = this branch.

#	Scenario	old (#17)	new (#18)
S1	Response lost after the write committed	1 record	1 record
S2	Same, Idempotency-Key stripped (control)	—	write re-executed
S3	In-flight 409 + Retry-After	retries, then 201	retries, then 201
S4	Too-large 409 (no Retry-After)	CLI text only	CLI text + server's message
S5	Same key, different body	422	422

S1 — the guarantee holds end to end.

#1 t=8.26s  key=d347c790…  upstream=201  replayed=None  ->CLI=502
#2 t=9.32s  key=d347c790…  upstream=201  replayed=true  ->CLI=201
1 distinct key across attempts · 1 customer in the DB · exit=0

The retry carried the same key and the server replayed the stored response (Idempotency-Replayed: true) instead of creating a second customer.

S2 — control, header removed.

#1 key=NONE  upstream=201  replayed=None  ->CLI=502
#2 key=NONE  upstream=422  replayed=None  ->CLI=422
✗ Error: Validation error: {'__all__': ['A customer with this name already exists in your organization.']}

Without the header the second POST re-ran the write — upstream=422 is business logic, not a replay. Here a unique-name constraint on Customer stopped the duplicate; a record type without such a constraint would have got two rows. This shows re-execution vs replay, not a literal duplicate.

S3 — the server's in-flight 409.

#1 t=0.19s  409  Retry-After=1
#2 t=1.22s  409  Retry-After=1
#3 t=2.24s  409  Retry-After=1
#4 t=3.32s  201

Retry-After: 1, a plain integer. The CLI waits ~1s between attempts and reuses one key throughout.

S4 — the only scenario where the two builds differ.

old (#17):

✗ Error: 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.

new (#18):

✗ Error: 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.
  Server said: The original response was too large to store, so it cannot be replayed.
  The request was already processed and has not been repeated.

Both correctly refuse to retry (one attempt). The new build additionally keeps what the server said.

What this does and does not show. S1, S3 and S5 behave identically on both builds — expected, since they exercise #17's work, which was already correct; they are proof the mechanism holds against the real server, not a diff. The Retry-After ceiling and the presence-vs-parseability change are not observable against this server, because it only ever emits an integer ("1" on the 409, str(int(wait) + 1) in core/rate_limit.py). Both remain defensive hardening. S1's higher wall time on the new build is first-request server warm-up, not a regression.

Test plan

• Unit tests pass (uv run pytest) — 153 passed, 125 skipped, on Python 3.11, 3.12 and 3.13. The 125 skips are the pre-existing live-API tests that need X_API_KEY; 11 tests are new here, and all 37 from Send an idempotency key on write requests #17 still pass unchanged.
• Linter passes (uv run ruff check .)
• Manually tested with dualentry <command> — the end-to-end runs above drive the real binary against a real server.

New unit tests in TestRetryAfterCeilingAndConflictDetail:

Test	What it checks
test_conflict_with_unreadable_retry_after_still_retries	soon, 2.5, -5, "" fall back to the backoff instead of cancelling the retry
test_retry_after_beyond_the_ceiling_is_reported_not_slept_through	409/429/503 with Retry-After: 3600: one call, no sleep, the wait is in the message
test_retry_after_at_the_ceiling_is_still_honoured	The ceiling itself is allowed
test_conflict_without_a_key_keeps_the_server_message	A 409 on GET shows the server's words, no 256 KB text
test_conflict_on_a_write_keeps_both_the_guidance_and_the_server_message	A keyed 409 keeps both
test_backoff_table_and_retry_count_cannot_drift	_MAX_RETRIES == len(_RETRY_DELAYS)

Not changed

• ProxyError stays in the non-retryable set. The comment calls these failures that "fail the same way every time", which holds for UnsupportedProtocol / LocalProtocolError / TooManyRedirects / DecodingError but is arguable for a proxy blip. Left as Send an idempotency key on write requests #17 has it — worth a second opinion rather than a silent flip.
• patch() is still unused; update sends PUT (commands/__init__.py:280,348) while the API documents PATCH for partial updates. Separate change.

Note on the base

Based on fix/retry-idempotency-key, a copy of #17's head pushed here so the diff stays clean. If #17 lands first, retarget this to main. If its author would rather fold these commits into #17, that works too — the branch is here to be taken apart.

[vladyslavk-dual]

[Critical] _is_retryable() references undefined _RETRYABLE_STATUS_CODES
_is_retryable() calls response.status_code in _RETRYABLE_STATUS_CODES at line 64 but the constant is never defined in this diff or visible in the shown file. The code will raise NameError at runtime when a non-409 retryable status (502, 503, 429) is encountered on any retry attempt, breaking the entire retry loop for server errors.
Command: define _RETRYABLE_STATUS_CODES before line 36 in client.py.

Suggested change

	return _has_retry_after(response)
	_RETRYABLE_STATUS_CODES = (429, 502, 503)
	_MAX_RETRY_AFTER = 60

[mykhaylob-de]

It's defined at client.py:16

[vladyslavk-dual]

[High] retry_after from server is capped only for the stop decision, not the sleep duration
At line 197, retry_after = _retry_after_seconds(response) extracts a server-supplied value. Line 199 caps it only to decide whether to stop retrying (if retry_after is not None and retry_after > _MAX_RETRY_AFTER), but line 205 uses the uncapped value in time.sleep(delay). A malicious or misbehaving server responding with Retry-After: 3600 would cause the client to sleep 3600 seconds before the first retry, blocking the user indefinitely.
Command: cap retry_after before it reaches the sleep at client.py:205.

Suggested change

	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)
	retry_after = _retry_after_seconds(response)
	if retry_after is not None:
	retry_after = min(retry_after, _MAX_RETRY_AFTER)
	if retry_after is not None and retry_after > _MAX_RETRY_AFTER:
	return self._handle_response(response, sent_idempotency_key=keyed)

[mykhaylob-de]

When retry_after > _MAX_RETRY_AFTER the loop does return self._handle_response(...), which raises.

[vladyslavk-dual]

[Medium] _server_detail() swallows all exceptions including unexpected errors
The function at line 66–70 catches Exception broadly, including json.JSONDecodeError, AttributeError, KeyError, and any other exception, then silently returns .text as a fallback. An actual server bug (malformed JSON in a 5xx response) or a programming error (e.g., AttributeError from unexpected response shape) would be masked with no indication something went wrong during parsing, making debugging production issues harder.
Command: catch only ValueError (the parent of json.JSONDecodeError) at client.py:68 to let unexpected errors propagate.

Suggested change

	def _server_detail(response: httpx.Response) -> str:
	try:
	payload = response.json()
	except ValueError:
	return response.text.strip()

[mykhaylob-de]

In general yes - broad excepts hide bugs. The question is whether this is a right to do this.

_server_detail does best-effort extraction of whatever text the server sent, so it can be pasted into an error message. Every one of its callers is already on the failure path, mid-construction of an APIError. An extractor that can itself throw defeats its own purpose: the exception escapes while we're building the error, and the user gets a Python traceback in place of a clean, actionable message.
It's also consistent with the rest of the code