Core: Bound Idempotency-Key retries to the advertised key lifetime - #18070
Draft
HotSushi wants to merge 4 commits into
Draft
Core: Bound Idempotency-Key retries to the advertised key lifetime#18070HotSushi wants to merge 4 commits into
HotSushi wants to merge 4 commits into
Conversation
ExponentialHttpRequestRetryStrategy only applied the idempotent retriable status codes (408, 500, 502, 503, 504) when the HTTP method itself was idempotent. Mutation endpoints are POST, so a request that already carries an Idempotency-Key header - which the server guarantees is safe to replay - was not retried on 502/504 or on 503 without a Retry-After header. Treat a request carrying the Idempotency-Key header as retry-safe for these codes, in both the response and network-exception retry paths. Requests without the header keep the previous conservative behavior.
- Update testIdempotentCreateReplayAfterSimulated503: the client now auto-retries a keyed POST on 503 and the server replays the finalized 200, so the call succeeds transparently. Assert success directly instead of expecting a thrown 503 followed by a manual retry. - Add testIdempotentCreateRetryCarriesSameKey pinning the invariant that every transport attempt of a retried keyed POST carries the identical Idempotency-Key. - Clarify the retry-safety comment in ExponentialHttpRequestRetryStrategy (idempotent method OR Idempotency-Key header), which no longer means strict HTTP idempotency.
- Import CopyOnWriteArrayList instead of using its fully-qualified name. - Drop the redundant equality assertion in testIdempotentCreateRetryCarriesSameKey; the filter already guarantees equality, so hasSize(2) is the real guard.
When a server advertises an Idempotency-Key lifetime (e.g. PT30M) in the config response, a keyed retry that fires after the window has elapsed would be treated as a brand-new mutation by the server, defeating the deduplication guarantee. This change tracks the wall-clock time of the first attempt in the HTTP context and declines a retry whose next scheduled interval would land at or after the advertised lifetime. The lifetime string is plumbed from ConfigResponse through RESTSessionCatalog into the HTTPClient properties, and from there into ExponentialHttpRequestRetryStrategy via a new two-argument constructor; the single-argument constructor delegates to it with a null lifetime, preserving backward compatibility.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What / Why
Follow-up to #17947 (client-side Idempotency-Key retries). #17947 auto-retries keyed POSTs on retriable errors but never honors the server-advertised key lifetime, so a slow retry can fire after the key has expired — when the server no longer dedupes — risking a duplicate mutation.
The REST spec obligation on the client:
Change
ExponentialHttpRequestRetryStrategynow accepts the advertised key lifetime and declines a retry for a keyed request once the elapsed time since the first attempt plus the next retry interval would reach the lifetime window. Enforced on both the response and network-exception retry paths; non-keyed / idempotent requests are unaffected.GET /v1/config(ISO-8601, e.g.PT30M) is plumbed to the HTTP client through the client properties and parsed once inHTTPClient; a malformed value is ignored (disables enforcement) rather than failing client construction.isRetrySafe(...)predicate to remove duplication between the two retry paths (addresses a review nit; also clarifies that "retry-safe" is not the same as strictly idempotent).Tests
Added unit tests in
TestExponentialHttpRequestRetryStrategy: keyed retry abandoned once the window is exceeded (both response and network-exception paths), keyed retry honored within the window and when no lifetime is advertised, and an idempotent GET unaffected by the key lifetime.:iceberg-corerest.*tests pass locally.Notes