fix(retry): never replay a non-idempotent write; bound server retry signals (#104)

[karlwaldman]

Closes #104.

Confirmed live, not already fixed

Reproduced on origin/main (6e1a66c) with a mock transport and a fixture key:

POST timeout -> attempts: 3 err: TimeoutError
GET  timeout -> attempts: 3 err: TimeoutError
POST 503     -> attempts: 3 err: ServerError
max_retries=0 -> 3 | retry_on=[] -> [429, 500, 502, 503, 504]
429 Retry-After=31612 -> waits: [60.0, 60.0]     <- already capped on main
429 Retry-After=-30   -> waits: [-30.0, -30.0]   <- time.sleep() raises ValueError

One part of the issue is already fixed on main and was not re-filed: durable quota exhaustion (X-RateLimit-State: exhausted + a counter window) is already suppressed, and the 429 Retry-After already had a 60s upper cap. A regression test now pins both.

(a) Writes are no longer replayed

A timeout or transport error is an ambiguous outcome, not a failure — the server may have committed the write before the response was lost. A 5xx is equally ambiguous, since a gateway can return 502 after the origin already committed. Subscription and webhook creates go through these helpers, so a lost response became two subscriptions.

• POST and PATCH are now sent exactly once on timeout, transport error, and 5xx.
• GET, HEAD, OPTIONS, TRACE, PUT, DELETE retry exactly as before — a transient GET still recovers.
• A 429 still retries any method. It is an outright refusal, so the write definitively did not happen and replay is safe. This is the one deliberate asymmetry.
• A caller who knows a write is repeatable passes idempotent=True to request(). That is a caller's assertion; no server-side idempotency-key contract is claimed, because none was verified.
• When a write is not replayed, the error explains that it was not retried and carries .ambiguous_write = True, so the caller checks whether it landed rather than blindly resending.

Applies to OilPriceAPI.request, OilPriceAPI.request_with_headers and AsyncOilPriceAPI.request alike.

(b) Explicit configuration survives the constructor

max_retries or DEFAULT and retry_on or DEFAULT discarded a caller's explicit 0 and []. Now explicit None checks.

max_retries counts total attempts — that is what the docstring always said and what for attempt in range(self.max_retries) does. It must be ≥ 1. 0, a negative, or a non-int now raises ConfigurationError naming the fix, instead of silently becoming 3:

max_retries=0 -> ConfigurationError - max_retries counts total attempts and must be
at least 1, got 0. Pass max_retries=1 for a single attempt with no retries.
retry_on=[] -> []

retry_on=[] is preserved and really does disable status-code retries (RetryStrategy.__init__ had the same or bug and is fixed too).

(c) Retry-After is bounded in both directions

One shared RetryStrategy.bounded_wait() clamps to [0, 60], used by all three request paths:

429 Retry-After=31612 -> waits: [60.0, 60.0]
429 Retry-After=-30   -> waits: [0.0, 0.0]

The upper bound is the one with scale behind it: the keyless demo returns retry-after: 31612, which unbounded parks a process for 8.8 hours (that is what the Go SDK does today). The lower bound stops time.sleep() raising ValueError on a negative header.

Red

Test written first, against unmodified origin/main:

$ .venv/bin/python -m pytest tests/unit/test_write_replay_and_retry_config.py --no-cov -q
>       assert counter.methods == [method], f"write was replayed: {counter.methods}"
E       AssertionError: write was replayed: ['POST', 'POST', 'POST']
E       assert ['POST', 'POST', 'POST'] == ['POST']
...
FAILED ...::test_write_is_sent_exactly_once[POST-_timeout]
FAILED ...::test_write_is_sent_exactly_once[POST-_connect_error]
FAILED ...::test_write_is_sent_exactly_once[POST-handler0]     # 503
FAILED ...::test_write_is_sent_exactly_once[POST-handler1]     # 500
FAILED ...::test_write_is_sent_exactly_once[PATCH-...]         # x4
FAILED ...::test_async_write_is_sent_exactly_once[...]         # x2
FAILED ...::test_request_with_headers_does_not_replay_a_write
FAILED ...::test_ambiguous_write_error_says_it_was_not_retried
FAILED ...::test_caller_can_opt_into_replaying_a_write
FAILED ...::test_empty_retry_on_is_preserved
FAILED ...::test_empty_retry_on_actually_stops_status_retries
FAILED ...::test_async_empty_retry_on_is_preserved
FAILED ...::test_invalid_max_retries_fails_loudly[0|-1|-5]
FAILED ...::test_non_integer_max_retries_fails_loudly[3|2.5|True]
FAILED ...::test_async_invalid_max_retries_fails_loudly
FAILED ...::test_negative_retry_after_never_reaches_sleep[-30|-1]
FAILED ...::test_retry_strategy_knows_which_methods_are_replay_safe
======================== 26 failed, 12 passed in 4.03s =========================

Every assertion counts what reached the transport, not an internal flag.

Green

$ .venv/bin/python -m pytest tests/unit/test_write_replay_and_retry_config.py --no-cov -q
============================== 38 passed in 0.44s ==============================

And the original repro script, re-run after the change:

POST timeout -> attempts: 1 err: TimeoutError
GET  timeout -> attempts: 3 err: TimeoutError
POST 503     -> attempts: 1 err: ServerError

Pre-existing failures unchanged

tests/unit on clean origin/main: 109 failed, 453 passed (mostly test_streaming.py).
tests/unit on this branch: 109 failed, 491 passed — same 109, +38 new passing.

Rest of the suite: 115 passed, 21 skipped, including the existing tests/unit/test_retry.py and tests/test_retry_remedy.py, untouched and still green.

Compatibility

RetryStrategy.should_retry() and should_retry_on_exception() keep their existing signatures — method and idempotent are optional keyword arguments, and omitting them means "unknown", which is treated as replay-safe so the public helper behaves as before. The SDK's own clients always pass the method.

Two intentional behaviour changes for existing callers:

• A caller passing max_retries=0 now gets a loud ConfigurationError instead of silently getting 3 attempts. There is no value of max_retries for which behaviour changes silently.
• client.diesel.get_stations() is a POST and so is now sent once rather than three times on a 503. That is the point of the change, but it is worth naming.

🤖 Generated with Claude Code

https://claude.ai/code/session_015ao5paex73xXvuM424Libo

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 390887d1-18e3-40a2-bfd4-0c2b45a79705

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}