Skip to content

Send an idempotency key on write requests - #17

Open
JeyKip wants to merge 4 commits into
dualentry:mainfrom
JeyKip:fix/retry-idempotency-key
Open

Send an idempotency key on write requests#17
JeyKip wants to merge 4 commits into
dualentry:mainfrom
JeyKip:fix/retry-idempotency-key

Conversation

@JeyKip

@JeyKip JeyKip commented Aug 16, 2026

Copy link
Copy Markdown

Summary

The --retry flag repeats failed requests. It repeats all of them, including POST, PUT, PATCH and DELETE.

The problem is that when a request fails with 502 or 504, we do not know what happened on the server. The request may have failed before anything was saved. Or it may have been processed correctly and only the response was lost. We cannot tell the difference from the client side.

If we retry in the second case, we create a second record. For an accounting CLI this means a duplicated invoice or journal entry.

The DualEntry API added idempotency keys on 2026-08-12:

The public API now accepts an Idempotency-Key header so retried requests do not create duplicate records.
Payroll review and API idempotency keys (Aug 12, 2026)

This PR uses that header. Now every write request sends a key. All retries of the same request send the same key. The server then replays the first response instead of doing the work again, so we no longer need to know what happened on the server.

What the API promises

From the endpoint docs, for example create recurring request, update recurring request, delete recurring request and partial update of a customer payment:

  • The header is optional. Maximum length is 255 characters. "A unique value (a UUID works well)."
  • "If the request is repeated with the same key, the original response is replayed instead of the operation running again, so a retry cannot create a duplicate record."
  • Results are replayable for 48 hours.
  • Reusing a key with a different request body returns 422.

The last point is important. Each logical request must get its own key. We cannot reuse one key for several requests.

The idempotency guide adds two more cases. Both return 409, but they mean opposite things:

Situation Status Header What we must do
First request is still running 409 Retry-After Retry after that many seconds, same key
Original response is larger than 256 KB 409 none Do not retry. The write was not repeated

The guide doesn't explain the type of value sent in the Retry-After header in this case. However, the rate limiting guide says about 429: "The response includes a Retry-After header (seconds) telling you exactly how long to wait before the bucket refills enough for one more request." So I assumed that the Retry-After header in case of a 409 error also contains the amount of seconds.

Changes

In src/dualentry_cli/client.py:

  • _request now adds an Idempotency-Key header for POST, PUT, PATCH and DELETE.
  • GET does not get the header. It does not change anything on the server, so the header has no meaning there.
  • The key is a uuid.uuid4(). It is created once per _request call, before the retry loop. This is the important part. If we created a new key for each attempt, the bug would still be there.
  • The header is sent also when --retry is off. Something else may repeat the request, for example a proxy. We use setdefault, so if a caller passes its own key, we keep it.
  • Added a patch() method. The API documents PATCH for partial updates, but the client could not send one.
  • New helper _is_retryable() separates the two 409 cases. A 409 with Retry-After is retried. A 409 without it is not retried at all.
  • New helper _retry_after_seconds() reads the header. When it is present, the client waits exactly that long.
  • This applies to 429 as well: before, the client ignored what the server told it and used its own numbers.
  • Error messages for 409 and 429 now include the wait time from the header.
  • Every retry waits, including the last one. Before, the wait was skipped on the final attempt, so the last request was sent immediately after a 429 or a 409, exactly when the server had just told us to wait. The attempt counter in the message was also wrong and said 2/3, 3/3 for four requests; now it says 2/4, 3/4, 4/4.
  • Only transient transport errors are retried now. The loop caught httpx.RequestError, which is every transport failure, including ones that fail the same way every time: a wrong scheme in DUALENTRY_API_URL (UnsupportedProtocol), LocalProtocolError, DecodingError, TooManyRedirects and ProxyError. A typo in the URL meant 4 requests and 1 + 2 + 4 seconds of waiting before showing an error that was already known after the first one. The new _RETRYABLE_EXCEPTIONS keeps timeouts, network errors and RemoteProtocolError; everything else is reported immediately.
  • The last_error variable is gone. It was never read, and after the changes above there was nothing left for it to do.

Tests

tests/test_client.py had no tests for the retry logic at all. A new class TestIdempotencyKey with 10 cases was added:

Test What it checks
test_write_methods_send_an_idempotency_key POST, PUT, PATCH, DELETE: header is present, is a valid UUID, is not longer than 255 characters
test_get_does_not_send_an_idempotency_key GET sends no header
test_retry_reuses_the_same_key_across_attempts First 502, then 201. Two calls, one key, correct result
test_every_retry_attempt_carries_the_key All attempts fail. Every attempt uses the same key
test_separate_requests_use_different_keys Two POST requests get two different keys
test_caller_supplied_key_is_not_overwritten A key passed by the caller is kept
test_key_is_sent_even_when_retry_is_disabled Header is present when retry=False

Second class TestRetryAfterAndConflicts with 9 cases (27 including parameters):

Test What it checks
test_conflict_with_retry_after_is_retried 409 with Retry-After: 2, then 201. Waits exactly 2s, reuses the key
test_conflict_without_retry_after_is_not_retried 409 with no header: only one request is sent, error mentions 256 KB
test_rate_limit_waits_for_retry_after_not_the_hardcoded_backoff 429 with Retry-After: 7 waits 7s, not the 1s from _RETRY_DELAYS
test_rate_limit_without_retry_after_falls_back_to_backoff No header, so the waits are 1s, 2s and 4s
test_the_last_attempt_also_waits_for_retry_after All attempts get Retry-After: 3. The waits are 3s, 3s, 3s, so the request after the loop waits too
test_unparsable_retry_after_falls_back_to_backoff Retry-After: next tuesday, inf, Infinity, 1e9, 2.5, -5, empty: all fall back
test_transient_transport_error_is_retried All 4 timeout and network errors plus RemoteProtocolError: 4 requests, waits 1s, 2s, 4s, then the error propagates
test_non_transient_transport_error_is_not_retried LocalProtocolError, UnsupportedProtocol, ProxyError, DecodingError, TooManyRedirects: one request, no waiting
test_storage_unavailable_is_retried_with_the_same_key 503, then 201, one key

Two fixtures keep the tests fast and precise:

  1. no_backoff sets _RETRY_DELAYS to zeros, otherwise the tests would really wait 1s, 2s and 4s
  2. sleeps replaces time in the client module and records the waits, so a test can check the exact number of seconds instead of measuring real time

Note about the changes

The PR [#16] needs to be merged into main first, and those changes need to be pulled into this branch for the tests to pass. The changes were originally made and tested on top of the fix/ci-dependency-drift branch, then moved to the current branch, which was created from main.

Test plan

  • Unit tests pass (uv run pytest)
  • Linter passes (uv run ruff check .)
  • Manually tested with dualentry <command> (since I do not have a valid API key, I tested in a mocked environment)

@Warkanlock

Copy link
Copy Markdown
Contributor

thanks!

_MAX_RETRIES = 3
_RETRY_DELAYS = [1, 2, 4] # Exponential backoff: 1s, 2s, 4s

# The API replays the original response for a repeated Idempotency-Key instead of

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing _RETRYABLE_STATUS_CODES constant: The refactored _is_retryable() function at line 36 references _RETRYABLE_STATUS_CODES but it is never defined in the diff. The code will raise NameError at runtime when a non-409 retryable status (502, 503, 429) is encountered. Define the constant before line 20.

Suggested change
# The API replays the original response for a repeated Idempotency-Key instead of
# 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.
# https://docs.dualentry.com/developers/release-notes/2026-08-12
_IDEMPOTENCY_HEADER = "Idempotency-Key"
_IDEMPOTENCY_METHODS = frozenset({"POST", "PUT", "PATCH", "DELETE"})
# 429 and the in-flight 409 both report exactly how long to wait.
# https://docs.dualentry.com/developers/guides/rate-limiting
_RETRY_AFTER_HEADER = "Retry-After"
_RETRYABLE_STATUS_CODES = frozenset({502, 503, 429})

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mentioned constant was already in place, so it wasn't included into the PR.

response = self._client.request(method, path, **kwargs)
return self._handle_response(response)

# Retry logic with visible feedback

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-by-one in retry loop: The loop at line 148 runs _MAX_RETRIES times (3 iterations: attempts 0, 1, 2), then line 165 unconditionally issues a 4th request after the loop exits. This produces 4 total attempts instead of the advertised _MAX_RETRIES=3. Additionally, the stderr message at line 163 prints attempt + 2 and _MAX_RETRIES + 1 (producing "attempt 2/4"), but the final 4th request after the loop has no message. The user sees "Retrying" three times then a silent 4th attempt. Fix: move the final request inside the loop and return after each successful response; remove the unconditional request after line 164.

Suggested change
# Retry logic with visible feedback
# Retry logic with visible feedback
last_error = None
for attempt in range(_MAX_RETRIES):
retry_after = None
try:
response = self._client.request(method, path, **kwargs)
if not _is_retryable(response):
return self._handle_response(response)
retry_after = _retry_after_seconds(response)
# Retryable error - will retry
last_error = APIError(response.status_code, f"Temporary error ({response.status_code})")
except httpx.RequestError as e:
last_error = e
if attempt < _MAX_RETRIES - 1:
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})\033[0m", file=sys.stderr)
time.sleep(delay)
# Final attempt
response = self._client.request(method, path, **kwargs)
return self._handle_response(response)

# 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)
time.sleep(delay)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent retry messaging and logic: Line 163 prints "attempt {attempt + 2}/{_MAX_RETRIES + 1}" (printing 2/4), but this message is shown only when attempt < _MAX_RETRIES - 1 is true (line 162). After the loop exits (all 3 iterations done), line 165 issues the 4th request without printing a message or waiting. The message at line 163 should print "{attempt + 2}/{_MAX_RETRIES}" to match the fixed loop logic; see prior comment.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the comment below I concluded that the 4th attempt was intentional, so left it unchanged and just fixed delay to honor the Retry-After header and the exponential backoff value used by default. Honestly, we can simply remove the "Final attempt" block and stay with only 3 attempts to retry, or increase the value of _MAX_RETRIES to 4 (I will do this). This would keep current behavior and make the code a bit cleaner.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to do it the way I wrote above, but in the end it made the code more complex instead of cleaner, so I left _MAX_RETRIES = 3 and the "Final attempt" block as they are.

One more thing I changed in this method is what exactly we catch. It was except httpx.RequestError, which is basically everything, including errors that can never succeed on a second attempt: a wrong scheme in the URL (UnsupportedProtocol), LocalProtocolError, DecodingError, TooManyRedirects. So if somebody sets a wrong DUALENTRY_API_URL, the CLI was sending 4 requests and sleeping 1+2+4 seconds before showing an error that was already known after the first one. Now we retry only timeouts, network errors and RemoteProtocolError, and everything else is reported immediately. Both lists are covered with tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants