fix(web): correct idempotent replay payload and serialize concurrent duplicates

[marcelo-maciel]

Fixes three defects in IdempotencyEndpointFilter (audit findings API-01 and CONC-01, plus a replay path that never engaged).

API-03 (root) — replay never engaged, even in production

The write went through HybridCache.SetAsync while the probe read IDistributedCache by the raw key. HybridCache keys its L2 entries under its own scheme, so the raw-key probe never found the entry and idempotency replay silently never engaged — not just in tests. The repo's ChatSendMessageTests replay test was skipped with this exact symptom attributed to a "test-env cache split"; un-skipping it proves it was the filter.

Fix: write to the same IDistributedCache, key and serializer the probe uses. Idempotency entries are short-lived (TTL) and their HybridCache tag-purge path was unused, so dropping HybridCache here loses nothing.

API-01 — replay served the wrong shape and status

The filter cached SerializeToUtf8Bytes(result) where result is the wrapped IResult (Ok<T>/Created<T>), storing {"value":{...},"statusCode":200} instead of the wire DTO, and read Response.StatusCode before the IResult executed, so a 201 replayed as 200. The handler result is now executed into a buffer to capture the real wire body and status.

CONC-01 — concurrent duplicates both executed

probe -> execute -> write had no atomic reservation, so two concurrent same-key requests both ran the handler. An atomic in-flight reservation now serializes duplicates: Redis SET NX when an IConnectionMultiplexer is registered (the multi-instance case — this stack already requires Redis there for the shared Data Protection key ring), an in-process set otherwise (single instance). A duplicate in flight gets 409 Conflict. Redis stays optional.

Tests

IdempotencyEndpointFilterReplayTests (unit): replay preserves 201, replay body is the plain DTO, concurrent duplicates execute once (second gets 409). ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused is un-skipped and passes. Framework + Integration suites green.

Notes

Docs changelog entry to follow in the docs repo.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[iammukeshm]

The three fixes are the right idea — same-store/same-key symmetry, buffer-and-capture for the true wire body+status, and an atomic SET NX / TryAdd reservation with re-probe-on-race. Tenant isolation is preserved (reservation key derives from the tenant-scoped cache key). Two things to fix before merge:

🔴 HIGH — reservation TTL is the 24h response TTL; a crash mid-request strands the lock for a day. The in-flight reservation uses options.DefaultTtl (24h). If the process is killed between reserving and the finally release (OOM, pod eviction, SIGKILL), the Redis key survives 24h and every retry of that idempotency key 409s for a full day. Use a short, request-timeout-scale TTL for the in-flight reservation (seconds/minutes), decoupled from the 24h stored-response TTL.

⚠️ MEDIUM — reservation now fails closed on a Redis blip. StringSetAsync is treated as authoritative, so a transient Redis error throws and 500s the request. On main idempotency degraded gracefully (convenience, not correctness). Wrap the reserve/release in try/catch and fail open (proceed) to match the existing stance used for the response write.

nit — ReleaseReservationAsync's KeyDeleteAsync is unguarded; a Redis fault at release throws out of finally and (with the TTL issue) strands the lock. Make it best-effort too.

⚠️ Also touches protected src/BuildingBlocks/Web (Golden Rule #4) — needs explicit maintainer sign-off.

Tests are good (replay shape + concurrent-once/409, integration un-skipped); note the concurrent path only exercises the in-process branch, not the Redis NX branch.

[marcelo-maciel]

Addressed in 08b34dcc.

🔴 HIGH — reservation TTL. Split out IdempotencyOptions.ReservationTtl (default 1m), decoupled from the 24h DefaultTtl. The reservation now keys to the short TTL, so a SIGKILL between reserve and the finally-release frees the lock in ~1m instead of stranding it for a day. Documented the constraint on the option: it must outlast the longest handler runtime or a concurrent duplicate can slip through.

⚠️ MEDIUM — fail closed on Redis blip. TryReserveAsync now wraps StringSetAsync in try/catch (ex is not OperationCanceledException) and fails open (proceeds), logging a warning — same best-effort stance as the response write. A transient Redis error no longer 500s the request; the response cache still dedups later retries.

nit — unguarded release. ReleaseReservationAsync's KeyDeleteAsync is now best-effort under the same filtered catch, so a Redis fault can't throw out of the finally. The short ReservationTtl expires a missed delete anyway.

Redis NX branch coverage. Added two unit tests over a substituted IConnectionMultiplexer: one asserts the reservation TTL is ReservationTtl and not DefaultTtl (guards the HIGH fix from regressing), the other faults both StringSetAsync and KeyDeleteAsync and asserts the handler still runs without throwing. Framework suite green (126/126).

Golden Rule #4 — src/BuildingBlocks/Web. Acknowledged; this needs your maintainer sign-off. The change is scoped to the Idempotency/ filter + options.