Repository navigation
feat: add Idempotency-Key middleware for safe POST/PATCH retries - #139
Conversation
Implements draft-ietf-httpapi-idempotency-key-header. The middleware parses the Idempotency-Key header (SF Item String via StructuredFields), caches responses keyed by the idempotency value, and replays them on retry. Concurrent in-flight duplicates return 409 Conflict. - KeyValueStore injected via @Inject (enkan component system) - Target methods configurable (default: POST, PATCH) - String bodies cached; InputStream/File bodies re-executed on retry - IN_FLIGHT entries cleaned up on handler exception closes #131 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- IdempotencyEntry.headers changed from Map<String, String> to Map<String, List<String>> to preserve multi-valued headers like Set-Cookie when caching responses - setStore() throws MisconfigurationException on null, consistent with SessionMiddleware Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add putIfAbsent default method to KeyValueStore interface for atomic compare-and-set. MemoryStore overrides with ConcurrentHashMap.putIfAbsent for true atomicity. IdempotencyKeyMiddleware now uses putIfAbsent to atomically claim the idempotency key, eliminating the race condition between concurrent requests with the same key. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- MemoryStore.putIfAbsent: handle expired entries by atomic replace - IdempotencyKeyMiddleware: remove executeAndCache to avoid re-introducing race condition on non-String body re-execution; expired entry fallback retries putIfAbsent instead - IdempotencyEntry: deep-copy headers (Map.copyOf + List.copyOf) for immutability - KeyValueStore.putIfAbsent Javadoc: remove specific middleware reference, describe as generic CAS pattern - Add putIfAbsent test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Design: - Remove hasBody() re-execution path that allowed handler double- execution without IN_FLIGHT protection. Non-String bodies are now cached as empty string (status + headers still replayed correctly). - COMPLETED always replays from cache, never re-executes. Security: - Prefix store keys with "idempotency:" to prevent key collision with session data when the same KeyValueStore is shared. - Enforce max key length (256 chars) to prevent oversized store keys. Robustness: - Guard against null store (skips idempotency processing gracefully). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds first-class HTTP idempotency support to enkan-web by introducing an Idempotency-Key middleware that can cache/replay responses and prevent concurrent duplicate processing, backed by the existing KeyValueStore abstraction.
Changes:
- Introduces
IdempotencyKeyMiddlewareand anIdempotencyEntrycache record for storing/replaying responses. - Extends
KeyValueStorewithputIfAbsent(...)and implements an atomic version inMemoryStore. - Adds comprehensive unit tests covering pass-through, caching, replay, conflict, and cleanup behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| enkan-web/src/main/java/enkan/middleware/IdempotencyKeyMiddleware.java | Implements Idempotency-Key parsing (Structured Fields) and store-backed replay/conflict flow |
| enkan-web/src/main/java/enkan/middleware/idempotency/IdempotencyEntry.java | Defines serialized cache entry format and response reconstruction |
| enkan-web/src/main/java/enkan/middleware/session/KeyValueStore.java | Adds putIfAbsent default method for CAS-like behavior |
| enkan-web/src/main/java/enkan/middleware/session/MemoryStore.java | Provides atomic putIfAbsent using ConcurrentHashMap |
| enkan-web/src/test/java/enkan/middleware/IdempotencyKeyMiddlewareTest.java | Adds tests validating caching/replay/conflict and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String storeKey = KEY_PREFIX + key; | ||
|
|
There was a problem hiding this comment.
storeKey is derived only from the client-provided Idempotency-Key value. That means two different clients (or even two different endpoints) can collide on the same key and receive each other’s cached responses, which is a cross-request / cross-user data leakage risk. Scope the cache key by at least request method + URI (and ideally an authenticated principal/session identifier when available), or provide a configurable key-namespace function so applications can define the correct isolation boundary.
| try { | ||
| HttpResponse response = castToHttpResponse(chain.next(request)); | ||
| store.write(key, IdempotencyEntry.completed(response)); | ||
| return response; | ||
| } catch (RuntimeException e) { | ||
| store.delete(key); |
There was a problem hiding this comment.
executeRequest assumes chain.next(request) always returns a non-null HttpResponse. In this codebase middleware often tolerates null responses, and castToHttpResponse explicitly allows null. If response is null here, IdempotencyEntry.completed(response) will throw NPE and leave the key claimed. Consider treating a null response as “no cacheable result”: delete the IN_FLIGHT key and return null.
| } catch (RuntimeException e) { | ||
| store.delete(key); | ||
| throw e; |
There was a problem hiding this comment.
IN_FLIGHT cleanup currently only runs for RuntimeException. If downstream code throws an Error (or any other Throwable), the key will remain stuck IN_FLIGHT until TTL expiry, causing persistent 409s. Use a broader cleanup strategy (e.g., catch Throwable and rethrow) so the store entry is always removed on abnormal termination.
| } catch (RuntimeException e) { | |
| store.delete(key); | |
| throw e; | |
| } catch (Throwable t) { | |
| store.delete(key); | |
| throw t; |
| if (!store.putIfAbsent(storeKey, IdempotencyEntry.inFlight())) { | ||
| IdempotencyEntry existing = (IdempotencyEntry) store.read(storeKey); | ||
| if (existing == null) { | ||
| // Entry expired between putIfAbsent and read — retry claim | ||
| if (store.putIfAbsent(storeKey, IdempotencyEntry.inFlight())) { | ||
| return executeRequest(storeKey, request, chain); | ||
| } | ||
| return conflictResponse(); | ||
| } | ||
| return switch (existing.state()) { | ||
| case IN_FLIGHT -> conflictResponse(); | ||
| case COMPLETED -> existing.toResponse(); | ||
| }; |
There was a problem hiding this comment.
store.read(storeKey) is cast directly to IdempotencyEntry. If the store contains unexpected data for that key (e.g., corruption, manual writes, or a different middleware reusing the same prefix), this will throw ClassCastException and fail the request. Consider validating with instanceof and treating unknown values as a cache miss (e.g., overwrite with IN_FLIGHT / delete).
| private static final String KEY_PREFIX = "idempotency:"; | ||
| private static final int MAX_KEY_LENGTH = 256; | ||
|
|
||
| @Inject | ||
| private KeyValueStore store; | ||
|
|
||
| private Set<String> methods = Set.of("POST", "PATCH"); | ||
|
|
There was a problem hiding this comment.
The PR description/issue mentions configurable TTL for cached idempotency responses, but this middleware doesn’t expose any TTL configuration and implicitly relies on the underlying KeyValueStore TTL (MemoryStore defaults to 30 minutes). Consider adding a dedicated TTL setting for idempotency entries (or a dedicated store instance) so deployments can meet the expected retention window (often ~24h) without affecting session TTLs.
| /** | ||
| * Creates a COMPLETED entry from an HTTP response. | ||
| * Only String bodies are cached; other body types result in {@code null} body. | ||
| * | ||
| * @param response the HTTP response to cache | ||
| * @return a completed entry | ||
| */ | ||
| public static IdempotencyEntry completed(HttpResponse response) { | ||
| int status = response.getStatus(); | ||
| Map<String, List<String>> hdrs = new LinkedHashMap<>(); | ||
| if (response.getHeaders() != null) { | ||
| response.getHeaders().forEachHeader((name, value) -> | ||
| hdrs.computeIfAbsent(name, _ -> new ArrayList<>()).add(value.toString())); | ||
| } | ||
| Object responseBody = response.getBody(); | ||
| String body = responseBody instanceof String s ? s : ""; | ||
| return new IdempotencyEntry(State.COMPLETED, status, hdrs, body); | ||
| } |
There was a problem hiding this comment.
The Javadoc says non-String bodies “result in null body”, but the implementation stores an empty string ("") for non-String bodies. This will replay an empty body on retry (and mark the key COMPLETED), which contradicts both the comment and the PR description (“InputStream/File bodies re-executed on retry”). Decide on the intended semantics and encode them explicitly (e.g., don’t cache COMPLETED for non-String bodies, or store a distinct state that forces re-execution).
| /** | ||
| * Atomically writes the value only if no entry exists for the given key. | ||
| * Useful for implementing compare-and-set patterns where only the first | ||
| * writer should succeed (e.g., idempotency key processing). | ||
| * | ||
| * <p>The default implementation is not atomic (read-then-write). | ||
| * Implementations backed by {@code ConcurrentHashMap} or Redis should | ||
| * override this with a truly atomic operation. | ||
| * |
There was a problem hiding this comment.
The putIfAbsent Javadoc starts with “Atomically writes …”, but the default implementation is explicitly non-atomic (read-then-write). That wording can mislead implementers/callers into assuming atomicity. Consider rephrasing the first sentence to “Attempts to write …” (or similar) and keep the atomicity requirement in the “implementations should override” section.
| private boolean isTargetMethod(HttpRequest request) { | ||
| String method = request.getRequestMethod(); | ||
| return method != null && methods.contains(method.toUpperCase(Locale.ENGLISH)); | ||
| } |
There was a problem hiding this comment.
setMethods accepts a set that may contain lowercase/mixed-case method names, but isTargetMethod checks membership against method.toUpperCase(...) using exact Set.contains. This makes configuration case-sensitive in a surprising way. Consider normalizing the configured set to uppercase (or compare with equalsIgnoreCase like CorsMiddleware does).
| // Atomically claim the key. If putIfAbsent returns false, another | ||
| // request already owns this key — check its state. | ||
| if (!store.putIfAbsent(storeKey, IdempotencyEntry.inFlight())) { | ||
| IdempotencyEntry existing = (IdempotencyEntry) store.read(storeKey); | ||
| if (existing == null) { | ||
| // Entry expired between putIfAbsent and read — retry claim | ||
| if (store.putIfAbsent(storeKey, IdempotencyEntry.inFlight())) { | ||
| return executeRequest(storeKey, request, chain); | ||
| } | ||
| return conflictResponse(); | ||
| } | ||
| return switch (existing.state()) { | ||
| case IN_FLIGHT -> conflictResponse(); | ||
| case COMPLETED -> existing.toResponse(); | ||
| }; | ||
| } | ||
|
|
||
| return executeRequest(storeKey, request, chain); |
There was a problem hiding this comment.
Race: when putIfAbsent fails and store.read(storeKey) returns null (expired/evicted), the code retries putIfAbsent once and then returns 409 on failure. If another request completed and stored a COMPLETED entry between the retry and the return, this would incorrectly return 409 instead of replaying the cached response. A small loop that re-reads and returns COMPLETED when present would make the behavior deterministic.
| // Atomically claim the key. If putIfAbsent returns false, another | |
| // request already owns this key — check its state. | |
| if (!store.putIfAbsent(storeKey, IdempotencyEntry.inFlight())) { | |
| IdempotencyEntry existing = (IdempotencyEntry) store.read(storeKey); | |
| if (existing == null) { | |
| // Entry expired between putIfAbsent and read — retry claim | |
| if (store.putIfAbsent(storeKey, IdempotencyEntry.inFlight())) { | |
| return executeRequest(storeKey, request, chain); | |
| } | |
| return conflictResponse(); | |
| } | |
| return switch (existing.state()) { | |
| case IN_FLIGHT -> conflictResponse(); | |
| case COMPLETED -> existing.toResponse(); | |
| }; | |
| } | |
| return executeRequest(storeKey, request, chain); | |
| // Atomically claim the key. If another request already owns it, | |
| // re-read until we observe a stable state so that a completed | |
| // response is replayed instead of incorrectly returning 409. | |
| while (true) { | |
| if (store.putIfAbsent(storeKey, IdempotencyEntry.inFlight())) { | |
| return executeRequest(storeKey, request, chain); | |
| } | |
| IdempotencyEntry existing = (IdempotencyEntry) store.read(storeKey); | |
| if (existing == null) { | |
| // Entry expired or was evicted between putIfAbsent and read. | |
| // Retry so we can either claim the key or observe a newly | |
| // written COMPLETED entry deterministically. | |
| continue; | |
| } | |
| return switch (existing.state()) { | |
| case IN_FLIGHT -> conflictResponse(); | |
| case COMPLETED -> existing.toResponse(); | |
| }; | |
| } |
- Store key scoped by method + URI + idempotency value to prevent cross-endpoint response leakage - Null response from chain: delete IN_FLIGHT and return null - Catch Throwable (not just RuntimeException) for IN_FLIGHT cleanup to handle Error cases - instanceof check before casting store.read() result to IdempotencyEntry (treats unexpected data as cache miss) - Retry loop for putIfAbsent/read race: re-reads until stable state instead of returning 409 on transient eviction - setMethods normalizes to uppercase - Javadoc fixes: putIfAbsent wording, TTL guidance, body caching Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace while(true) with for-loop capped at 3 retries. Returns 409 Conflict if retries are exhausted (broken store scenario). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
IdempotencyKeyMiddlewareimplementing draft-ietf-httpapi-idempotency-key-headerIdempotency-Keyheader as SF Item String viaStructuredFields.parseItem()409 Conflictfor concurrent in-flight duplicatesKeyValueStoreinjected via@Injectfrom enkan component system (nonew MemoryStore())Flow
closes #131
Test plan
🤖 Generated with Claude Code