From 508ca85a3a66b050e91aabfaf89991e7d17523bd Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Sat, 11 Jul 2026 17:29:42 -0300 Subject: [PATCH 1/3] fix(web): correct idempotent replay payload and serialize concurrent duplicates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in IdempotencyEndpointFilter: API-01 — the filter cached JsonSerializer.SerializeToUtf8Bytes(result) where result is the wrapped IResult (Ok/Created), so it stored {"value":...,"statusCode":200} instead of the wire DTO, and it read Response.StatusCode before the IResult executed, so a 201 Created replayed as 200. The handler result is now executed into a buffer to capture the real wire body + status, which is what gets served and cached. CONC-01 — probe->execute->write had no atomic reservation, so two concurrent requests with the same key both missed the probe and both executed 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 that arrives while the original is still running gets 409 Conflict. Redis stays optional: without it the app falls back to the in-memory reservation, correct for a single instance where a cross-container race cannot occur. --- .../Idempotency/IdempotencyEndpointFilter.cs | 193 ++++++++++++---- .../IdempotencyEndpointFilterReplayTests.cs | 215 ++++++++++++++++++ 2 files changed, 369 insertions(+), 39 deletions(-) create mode 100644 src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index fa73b35c7c..002cf4cbdb 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Security.Cryptography; using System.Text; using System.Text.Json; @@ -9,6 +10,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using StackExchange.Redis; namespace FSH.Framework.Web.Idempotency; @@ -21,13 +23,21 @@ namespace FSH.Framework.Web.Idempotency; /// Uses directly for the probe read (bypassing /// 's factory-mandatory API) and /// for the write path so replays benefit from L1 and the regular tag invalidation story. -/// Using HybridCache with DisableUnderlyingData as a "get-only probe" is a -/// known anti-pattern tracked at dotnet/aspnetcore#57191. +/// The handler result is executed into a buffer so the cached payload is the real wire body and +/// status code (an Ok<T>/Created<T> wrapper would otherwise be serialized +/// verbatim, and Response.StatusCode is still the default at filter time — the IResult sets +/// it only when it executes). Concurrent duplicate keys are serialized by an atomic in-flight +/// reservation (Redis SET NX when a multiplexer is registered, an in-process set otherwise). /// public sealed class IdempotencyEndpointFilter : IEndpointFilter { private static readonly JsonSerializerOptions JsonOpts = new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + // In-process reservation used when no Redis multiplexer is registered. Single-instance only — + // a multi-instance host in this stack already runs Redis (shared Data Protection key ring), so + // the Redis branch below covers every deployment where cross-instance duplicates are possible. + private static readonly ConcurrentDictionary InFlight = new(StringComparer.Ordinal); + public async ValueTask InvokeAsync(EndpointFilterInvocationContext context, EndpointFilterDelegate next) { ArgumentNullException.ThrowIfNull(context); @@ -59,60 +69,165 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter // Probe-only read via IDistributedCache (real GetAsync, null on miss — unlike HybridCache's // factory). Bypasses L1: replays are rare vs first-calls, so L1 warmth has little value. - var cachedBytes = await distributedCache.GetAsync(cacheKey, httpContext.RequestAborted).ConfigureAwait(false); - if (cachedBytes is not null && cachedBytes.Length > 0) + var cached = await ProbeAsync(distributedCache, cacheKey, httpContext.RequestAborted).ConfigureAwait(false); + if (cached is not null) + { + return await ReplayAsync(httpContext, cached, idempotencyKey, logger).ConfigureAwait(false); + } + + // Atomically reserve the key so concurrent duplicates don't both execute the handler. + var multiplexer = httpContext.RequestServices.GetService(); + var reservationKey = cacheKey + ":inflight"; + if (!await TryReserveAsync(multiplexer, reservationKey, options.DefaultTtl, httpContext.RequestAborted).ConfigureAwait(false)) { - var cached = JsonSerializer.Deserialize(cachedBytes, JsonOpts); - if (cached is not null) + // Another request with this key is in flight. It may have finished between the probe + // and the reservation — re-probe once, otherwise report the in-progress conflict. + var raced = await ProbeAsync(distributedCache, cacheKey, httpContext.RequestAborted).ConfigureAwait(false); + return raced is not null + ? await ReplayAsync(httpContext, raced, idempotencyKey, logger).ConfigureAwait(false) + : TypedResults.Conflict("A request with this Idempotency-Key is already being processed."); + } + + try + { + var result = await next(context).ConfigureAwait(false); + + // Execute the result into a buffer to capture the real wire body + status code, then + // serve that buffer to the client. Returning the IResult unexecuted would leave + // Response.StatusCode at its default and cache the wrapper object, not the wire body. + var (statusCode, contentType, body) = await ExecuteAndCaptureAsync(result, httpContext).ConfigureAwait(false); + + httpContext.Response.StatusCode = statusCode; + if (contentType is not null) { - if (logger.IsEnabled(LogLevel.Debug)) - { - logger.LogDebug("Idempotent replay for key {KeyHash}", HashKey(idempotencyKey)); - } - httpContext.Response.Headers["Idempotency-Replayed"] = "true"; - httpContext.Response.StatusCode = cached.StatusCode; - if (cached.ContentType is not null) - { - httpContext.Response.ContentType = cached.ContentType; - } + httpContext.Response.ContentType = contentType; + } - if (cached.Body.Length > 0) + if (body.Length > 0) + { + await httpContext.Response.Body.WriteAsync(body, httpContext.RequestAborted).ConfigureAwait(false); + } + + // Cache the response through HybridCache so the tag invalidation path works for purges. + try + { + var responseToCache = new CachedIdempotentResponse { - await httpContext.Response.Body.WriteAsync(cached.Body, httpContext.RequestAborted).ConfigureAwait(false); - } + StatusCode = statusCode, + ContentType = contentType ?? "application/json", + Body = body, + }; - return null; // Response already written + var setOptions = new HybridCacheEntryOptions + { + Expiration = options.DefaultTtl, + LocalCacheExpiration = options.DefaultTtl < TimeSpan.FromMinutes(2) ? options.DefaultTtl : TimeSpan.FromMinutes(2), + }; + await hybridCache.SetAsync(cacheKey, responseToCache, setOptions, tags, httpContext.RequestAborted).ConfigureAwait(false); + } + // Best-effort caching: idempotency replay is a convenience, not a correctness requirement + catch (Exception ex) when (ex is not OperationCanceledException) + { + logger.LogWarning(ex, "Failed to cache idempotent response for key {KeyHash}", HashKey(idempotencyKey)); } + + // Response already written to the body directly; return an empty result so the framework + // doesn't serialize a null return and append "null" after the captured payload. + return Results.Empty; + } + finally + { + await ReleaseReservationAsync(multiplexer, reservationKey).ConfigureAwait(false); + } + } + + private static async ValueTask ProbeAsync( + IDistributedCache cache, string cacheKey, CancellationToken ct) + { + var bytes = await cache.GetAsync(cacheKey, ct).ConfigureAwait(false); + return bytes is { Length: > 0 } + ? JsonSerializer.Deserialize(bytes, JsonOpts) + : null; + } + + private static async ValueTask ReplayAsync( + HttpContext httpContext, CachedIdempotentResponse cached, string idempotencyKey, ILogger logger) + { + if (logger.IsEnabled(LogLevel.Debug)) + { + logger.LogDebug("Idempotent replay for key {KeyHash}", HashKey(idempotencyKey)); } - // Execute the handler - var result = await next(context).ConfigureAwait(false); + httpContext.Response.Headers["Idempotency-Replayed"] = "true"; + httpContext.Response.StatusCode = cached.StatusCode; + if (cached.ContentType is not null) + { + httpContext.Response.ContentType = cached.ContentType; + } - // Cache the response through HybridCache so the tag invalidation path works for purges. + if (cached.Body.Length > 0) + { + await httpContext.Response.Body.WriteAsync(cached.Body, httpContext.RequestAborted).ConfigureAwait(false); + } + + // Empty result (not null) so the framework doesn't append a serialized "null". + return Results.Empty; + } + + private static async Task<(int StatusCode, string? ContentType, byte[] Body)> ExecuteAndCaptureAsync( + object? result, HttpContext httpContext) + { + var originalBody = httpContext.Response.Body; + await using var buffer = new MemoryStream(); + httpContext.Response.Body = buffer; try { - var body = result is not null ? JsonSerializer.SerializeToUtf8Bytes(result, JsonOpts) : []; - var responseToCache = new CachedIdempotentResponse + switch (result) { - StatusCode = httpContext.Response.StatusCode is > 0 and < 600 ? httpContext.Response.StatusCode : 200, - ContentType = "application/json", - Body = body - }; + case null: + break; + case IResult endpointResult: + await endpointResult.ExecuteAsync(httpContext).ConfigureAwait(false); + break; + default: + // A non-IResult return is serialized as JSON by the framework — mirror that. + await httpContext.Response.WriteAsJsonAsync(result, result.GetType(), options: null, contentType: null, httpContext.RequestAborted).ConfigureAwait(false); + break; + } - var setOptions = new HybridCacheEntryOptions - { - Expiration = options.DefaultTtl, - LocalCacheExpiration = options.DefaultTtl < TimeSpan.FromMinutes(2) ? options.DefaultTtl : TimeSpan.FromMinutes(2), - }; - await hybridCache.SetAsync(cacheKey, responseToCache, setOptions, tags, httpContext.RequestAborted).ConfigureAwait(false); + var statusCode = httpContext.Response.StatusCode is > 0 and < 600 + ? httpContext.Response.StatusCode + : StatusCodes.Status200OK; + return (statusCode, httpContext.Response.ContentType, buffer.ToArray()); + } + finally + { + httpContext.Response.Body = originalBody; } - // Best-effort caching: idempotency replay is a convenience, not a correctness requirement - catch (Exception ex) when (ex is not OperationCanceledException) + } + + private static async ValueTask TryReserveAsync( + IConnectionMultiplexer? multiplexer, string reservationKey, TimeSpan ttl, CancellationToken ct) + { + if (multiplexer is not null) + { + var db = multiplexer.GetDatabase(); + return await db.StringSetAsync(reservationKey, "1", ttl, When.NotExists).ConfigureAwait(false); + } + + _ = ct; + return InFlight.TryAdd(reservationKey, 0); + } + + private static async ValueTask ReleaseReservationAsync(IConnectionMultiplexer? multiplexer, string reservationKey) + { + if (multiplexer is not null) { - logger.LogWarning(ex, "Failed to cache idempotent response for key {KeyHash}", HashKey(idempotencyKey)); + await multiplexer.GetDatabase().KeyDeleteAsync(reservationKey).ConfigureAwait(false); + return; } - return result; + InFlight.TryRemove(reservationKey, out _); } private static string HashKey(string key) diff --git a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs new file mode 100644 index 0000000000..3460d98f62 --- /dev/null +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -0,0 +1,215 @@ +using System.Text.Json; +using FSH.Framework.Web.Idempotency; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Caching.Distributed; +using Microsoft.Extensions.Caching.Hybrid; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; + +namespace Framework.Tests.Web; + +/// +/// Runtime repro for audit findings API-01 (idempotency replay stores the wrong wire shape and +/// status) and CONC-01 (no in-flight reservation, so concurrent duplicate keys execute twice). +/// +/// These exercise the REAL . The one part we substitute is a +/// faithful single-store idempotency backend (write via HybridCache round-trips to the SAME +/// IDistributedCache the probe reads, using the filter's own serialization). This isolates the +/// filter's shape/status logic from the app's test-env cache split — the caveat that permanently +/// skips ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused. +/// +public sealed class IdempotencyEndpointFilterReplayTests +{ + private const string Key = "fixed-idempotency-key"; + + private static readonly JsonSerializerOptions CamelCase = + new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + + // ─── API-01: replayed status ───────────────────────────────────── + + [Fact] + public async Task Replay_Should_PreserveCreatedStatus_When_FirstResponseWas201() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + var id = Guid.NewGuid(); + + // First call: handler returns a 201 Created (the framework would execute it AFTER the filter + // returns — so at cache time Response.StatusCode is still the default 200). + var first = NewContext(provider); + await filter.InvokeAsync( + new TestFilterContext(first), + _ => ValueTask.FromResult(TypedResults.Created($"/samples/{id}", new SampleDto(id, "widget")))); + + // Second call, same key: must replay. + var replayBody = new MemoryStream(); + var second = NewContext(provider, replayBody); + await filter.InvokeAsync( + new TestFilterContext(second), + _ => throw new InvalidOperationException("handler must NOT run on an idempotent replay")); + + second.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeTrue( + "sanity: the replay path must actually engage, otherwise this test would be vacuous"); + second.Response.StatusCode.ShouldBe( + StatusCodes.Status201Created, + "a correct replay must reproduce the original 201 Created — the filter captures Response.StatusCode " + + "BEFORE the IResult executes, so it caches (and replays) 200 instead."); + } + + // ─── API-01: replayed body wire shape ──────────────────────────── + + [Fact] + public async Task Replay_Should_ReturnPlainDtoBody_Not_WrappedIResult() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + var id = Guid.NewGuid(); + + var first = NewContext(provider); + await filter.InvokeAsync( + new TestFilterContext(first), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(id, "widget")))); + + var replayBody = new MemoryStream(); + var second = NewContext(provider, replayBody); + await filter.InvokeAsync( + new TestFilterContext(second), + _ => throw new InvalidOperationException("handler must NOT run on an idempotent replay")); + + replayBody.Position = 0; + using var doc = JsonDocument.Parse(replayBody.ToArray()); + + doc.RootElement.TryGetProperty("value", out _).ShouldBeFalse( + "a correct replay body is the wire DTO; the filter caches SerializeToUtf8Bytes(result) where " + + "result is the wrapped Ok/Created, leaking the {\"value\":...} envelope onto the wire."); + doc.RootElement.TryGetProperty("id", out _).ShouldBeTrue( + "the plain DTO's own properties should be at the JSON root"); + } + + // ─── CONC-01: no in-flight reservation ─────────────────────────── + + [Fact] + public async Task Filter_Should_ExecuteHandlerOnce_When_TwoConcurrentRequestsShareKey() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + + int executions = 0; + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + // First request enters the handler and holds the in-flight reservation until released. + EndpointFilterDelegate first = async _ => + { + Interlocked.Increment(ref executions); + started.SetResult(); + await release.Task.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); + return TypedResults.Ok(new SampleDto(Guid.NewGuid(), "first")); + }; + + // Second request shares the key; its handler must never run while the first is in flight. + EndpointFilterDelegate second = _ => + { + Interlocked.Increment(ref executions); + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "second"))); + }; + + var firstCall = filter.InvokeAsync(new TestFilterContext(NewContext(provider)), first).AsTask(); + await started.Task.WaitAsync(TimeSpan.FromSeconds(5)); // first now holds the reservation + + var secondResult = await filter.InvokeAsync(new TestFilterContext(NewContext(provider)), second); + + release.SetResult(); + await firstCall.WaitAsync(TimeSpan.FromSeconds(10)); + + executions.ShouldBe( + 1, + "an idempotent endpoint must execute the handler exactly once for concurrent duplicate keys; " + + "the second request should be rejected while the first is in flight."); + (secondResult as IStatusCodeHttpResult)?.StatusCode.ShouldBe( + StatusCodes.Status409Conflict, + "a concurrent duplicate that arrives while the original is still running gets 409 Conflict."); + } + + // ─── harness ───────────────────────────────────────────────────── + + private static ServiceProvider BuildProvider() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddDistributedMemoryCache(); + services.AddSingleton>(Options.Create(new IdempotencyOptions())); + services.AddSingleton(sp => + new WriteThroughHybridCache(sp.GetRequiredService(), CamelCase)); + return services.BuildServiceProvider(); + } + + private static DefaultHttpContext NewContext(IServiceProvider provider, Stream? responseBody = null) + { + var context = new DefaultHttpContext { RequestServices = provider }; + context.Request.Method = "POST"; + context.Request.Headers["Idempotency-Key"] = Key; + if (responseBody is not null) + { + context.Response.Body = responseBody; + } + + return context; + } + + private sealed record SampleDto(Guid Id, string Name); + + private sealed class TestFilterContext : EndpointFilterInvocationContext + { + public TestFilterContext(HttpContext httpContext) => HttpContext = httpContext; + + public override HttpContext HttpContext { get; } + + public override IList Arguments { get; } = new List(); + + public override T GetArgument(int index) => (T)Arguments[index]!; + } + + /// + /// A faithful single-store idempotency backend: HybridCache.SetAsync serializes the cached + /// response with the exact options the filter's probe uses and writes it into the same + /// IDistributedCache, so a correctly-wired backend's replay is what surfaces the filter's bug. + /// + private sealed class WriteThroughHybridCache : HybridCache + { + private readonly IDistributedCache _store; + private readonly JsonSerializerOptions _options; + + public WriteThroughHybridCache(IDistributedCache store, JsonSerializerOptions options) + { + _store = store; + _options = options; + } + + public override ValueTask GetOrCreateAsync( + string key, + TState state, + Func> factory, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default) => factory(state, cancellationToken); + + public override async ValueTask SetAsync( + string key, + T value, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default) + { + var bytes = JsonSerializer.SerializeToUtf8Bytes(value, _options); + await _store.SetAsync(key, bytes, new DistributedCacheEntryOptions(), cancellationToken) + .ConfigureAwait(false); + } + + public override ValueTask RemoveAsync(string key, CancellationToken cancellationToken = default) => + ValueTask.CompletedTask; + + public override ValueTask RemoveByTagAsync(string tag, CancellationToken cancellationToken = default) => + ValueTask.CompletedTask; + } +} From 219ecbaeeabc7094f6956d3280eadebbab9e245c Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Sat, 11 Jul 2026 18:20:03 -0300 Subject: [PATCH 2/3] fix(web): make idempotency replay actually engage (symmetric cache store) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 probe never found the entry and replay silently never engaged — even in production. Proven by un-skipping ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused, which now passes. 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. --- .../Idempotency/IdempotencyEndpointFilter.cs | 26 ++++---- .../IdempotencyEndpointFilterReplayTests.cs | 61 ++----------------- .../Tests/Chat/ChatSendMessageTests.cs | 2 +- 3 files changed, 18 insertions(+), 71 deletions(-) diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index 002cf4cbdb..39869bbebe 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -6,7 +6,6 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Caching.Distributed; -using Microsoft.Extensions.Caching.Hybrid; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -20,9 +19,9 @@ namespace FSH.Framework.Web.Idempotency; /// for subsequent requests with the same key. /// /// -/// Uses directly for the probe read (bypassing -/// 's factory-mandatory API) and -/// for the write path so replays benefit from L1 and the regular tag invalidation story. +/// Uses for both the probe read and the write, on the same raw key +/// and serializer, so the two are symmetric (a HybridCache write keys its L2 entries under its own +/// scheme, which a raw-key probe never finds — replay then silently never engages). /// The handler result is executed into a buffer so the cached payload is the real wire body and /// status code (an Ok<T>/Created<T> wrapper would otherwise be serialized /// verbatim, and Response.StatusCode is still the default at filter time — the IResult sets @@ -59,13 +58,11 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter } var distributedCache = httpContext.RequestServices.GetRequiredService(); - var hybridCache = httpContext.RequestServices.GetRequiredService(); var logger = httpContext.RequestServices.GetRequiredService>(); // Include tenant context in cache key for isolation var tenantId = httpContext.User.FindFirst("tenant")?.Value ?? "global"; var cacheKey = CacheKeys.IdempotencyEntry(tenantId, idempotencyKey); - var tags = new[] { CacheKeys.Tags.Idempotency, CacheKeys.Tags.Tenant(tenantId) }; // Probe-only read via IDistributedCache (real GetAsync, null on miss — unlike HybridCache's // factory). Bypasses L1: replays are rare vs first-calls, so L1 warmth has little value. @@ -108,7 +105,10 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter await httpContext.Response.Body.WriteAsync(body, httpContext.RequestAborted).ConfigureAwait(false); } - // Cache the response through HybridCache so the tag invalidation path works for purges. + // Write to the SAME store + key the probe reads. HybridCache.SetAsync keys its L2 entries + // under its own scheme, so a raw-key IDistributedCache probe never found them and replay + // silently never engaged. Idempotency entries are short-lived (TTL) and their tag-purge + // path was unused, so IDistributedCache alone — symmetric with the probe — is correct. try { var responseToCache = new CachedIdempotentResponse @@ -118,12 +118,12 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter Body = body, }; - var setOptions = new HybridCacheEntryOptions - { - Expiration = options.DefaultTtl, - LocalCacheExpiration = options.DefaultTtl < TimeSpan.FromMinutes(2) ? options.DefaultTtl : TimeSpan.FromMinutes(2), - }; - await hybridCache.SetAsync(cacheKey, responseToCache, setOptions, tags, httpContext.RequestAborted).ConfigureAwait(false); + var payload = JsonSerializer.SerializeToUtf8Bytes(responseToCache, JsonOpts); + await distributedCache.SetAsync( + cacheKey, + payload, + new DistributedCacheEntryOptions { AbsoluteExpirationRelativeToNow = options.DefaultTtl }, + httpContext.RequestAborted).ConfigureAwait(false); } // Best-effort caching: idempotency replay is a convenience, not a correctness requirement catch (Exception ex) when (ex is not OperationCanceledException) diff --git a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs index 3460d98f62..154d3d7496 100644 --- a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -2,29 +2,21 @@ using FSH.Framework.Web.Idempotency; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Caching.Distributed; -using Microsoft.Extensions.Caching.Hybrid; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; namespace Framework.Tests.Web; /// -/// Runtime repro for audit findings API-01 (idempotency replay stores the wrong wire shape and -/// status) and CONC-01 (no in-flight reservation, so concurrent duplicate keys execute twice). -/// -/// These exercise the REAL . The one part we substitute is a -/// faithful single-store idempotency backend (write via HybridCache round-trips to the SAME -/// IDistributedCache the probe reads, using the filter's own serialization). This isolates the -/// filter's shape/status logic from the app's test-env cache split — the caveat that permanently -/// skips ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused. +/// Regression for audit findings API-01 (idempotency replay stored the wrong wire shape and status) +/// and CONC-01 (no in-flight reservation, so concurrent duplicate keys executed twice). These +/// exercise the REAL against a real in-memory +/// — the same store the filter now uses for both probe and write. /// public sealed class IdempotencyEndpointFilterReplayTests { private const string Key = "fixed-idempotency-key"; - private static readonly JsonSerializerOptions CamelCase = - new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; - // ─── API-01: replayed status ───────────────────────────────────── [Fact] @@ -139,8 +131,6 @@ private static ServiceProvider BuildProvider() services.AddLogging(); services.AddDistributedMemoryCache(); services.AddSingleton>(Options.Create(new IdempotencyOptions())); - services.AddSingleton(sp => - new WriteThroughHybridCache(sp.GetRequiredService(), CamelCase)); return services.BuildServiceProvider(); } @@ -169,47 +159,4 @@ private sealed class TestFilterContext : EndpointFilterInvocationContext public override T GetArgument(int index) => (T)Arguments[index]!; } - - /// - /// A faithful single-store idempotency backend: HybridCache.SetAsync serializes the cached - /// response with the exact options the filter's probe uses and writes it into the same - /// IDistributedCache, so a correctly-wired backend's replay is what surfaces the filter's bug. - /// - private sealed class WriteThroughHybridCache : HybridCache - { - private readonly IDistributedCache _store; - private readonly JsonSerializerOptions _options; - - public WriteThroughHybridCache(IDistributedCache store, JsonSerializerOptions options) - { - _store = store; - _options = options; - } - - public override ValueTask GetOrCreateAsync( - string key, - TState state, - Func> factory, - HybridCacheEntryOptions? options = null, - IEnumerable? tags = null, - CancellationToken cancellationToken = default) => factory(state, cancellationToken); - - public override async ValueTask SetAsync( - string key, - T value, - HybridCacheEntryOptions? options = null, - IEnumerable? tags = null, - CancellationToken cancellationToken = default) - { - var bytes = JsonSerializer.SerializeToUtf8Bytes(value, _options); - await _store.SetAsync(key, bytes, new DistributedCacheEntryOptions(), cancellationToken) - .ConfigureAwait(false); - } - - public override ValueTask RemoveAsync(string key, CancellationToken cancellationToken = default) => - ValueTask.CompletedTask; - - public override ValueTask RemoveByTagAsync(string tag, CancellationToken cancellationToken = default) => - ValueTask.CompletedTask; - } } diff --git a/src/Tests/Integration.Tests/Tests/Chat/ChatSendMessageTests.cs b/src/Tests/Integration.Tests/Tests/Chat/ChatSendMessageTests.cs index 004a72066a..28bbd65535 100644 --- a/src/Tests/Integration.Tests/Tests/Chat/ChatSendMessageTests.cs +++ b/src/Tests/Integration.Tests/Tests/Chat/ChatSendMessageTests.cs @@ -71,7 +71,7 @@ public async Task SendMessage_Should_Trim_Body_Whitespace() // ─── idempotency ───────────────────────────────────────────────── - [Fact(Skip = "Idempotency replay does not engage in the test environment — IDistributedCache (probe) and HybridCache (write-through) are wired to separate in-process stores, so the second call never sees the cached response. Same caveat as IdempotencyFilterTests.cs: 'full replay-with-matching-body coverage is not yet possible'. Backlog item 2.4b tracks the fix.")] + [Fact] public async Task SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused() { using var client = await _auth.CreateRootAdminClientAsync(); From 08b34dcc9938f995e0b3547d8140c4d513f824b8 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 13 Jul 2026 15:47:33 -0300 Subject: [PATCH 3/3] fix(web): short-TTL, fail-open idempotency reservation Address review on #1333: - Reservation used the 24h response TTL, so a crash between reserving and the finally-release stranded the Redis lock for a day (every retry 409s). Add IdempotencyOptions.ReservationTtl (default 1m), decoupled from DefaultTtl. - Reserve now fails open on a transient Redis error instead of 500ing the request, matching the best-effort stance of the response write. - Guard the release KeyDeleteAsync so a Redis fault can't throw out of the finally. Tests: reservation uses ReservationTtl not DefaultTtl; a faulting Redis on reserve/release proceeds without throwing (exercises the Redis NX branch the prior tests skipped). --- .../Idempotency/IdempotencyEndpointFilter.cs | 35 +++++++-- .../Web/Idempotency/IdempotencyOptions.cs | 9 +++ .../IdempotencyEndpointFilterReplayTests.cs | 72 ++++++++++++++++++- 3 files changed, 107 insertions(+), 9 deletions(-) diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index 39869bbebe..f2172103a6 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -75,7 +75,7 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter // Atomically reserve the key so concurrent duplicates don't both execute the handler. var multiplexer = httpContext.RequestServices.GetService(); var reservationKey = cacheKey + ":inflight"; - if (!await TryReserveAsync(multiplexer, reservationKey, options.DefaultTtl, httpContext.RequestAborted).ConfigureAwait(false)) + if (!await TryReserveAsync(multiplexer, reservationKey, options.ReservationTtl, logger, idempotencyKey, httpContext.RequestAborted).ConfigureAwait(false)) { // Another request with this key is in flight. It may have finished between the probe // and the reservation — re-probe once, otherwise report the in-progress conflict. @@ -137,7 +137,7 @@ await distributedCache.SetAsync( } finally { - await ReleaseReservationAsync(multiplexer, reservationKey).ConfigureAwait(false); + await ReleaseReservationAsync(multiplexer, reservationKey, logger, idempotencyKey).ConfigureAwait(false); } } @@ -207,23 +207,44 @@ await distributedCache.SetAsync( } private static async ValueTask TryReserveAsync( - IConnectionMultiplexer? multiplexer, string reservationKey, TimeSpan ttl, CancellationToken ct) + IConnectionMultiplexer? multiplexer, string reservationKey, TimeSpan ttl, ILogger logger, string idempotencyKey, CancellationToken ct) { if (multiplexer is not null) { - var db = multiplexer.GetDatabase(); - return await db.StringSetAsync(reservationKey, "1", ttl, When.NotExists).ConfigureAwait(false); + try + { + var db = multiplexer.GetDatabase(); + return await db.StringSetAsync(reservationKey, "1", ttl, When.NotExists).ConfigureAwait(false); + } + // Fail open on a Redis blip: the reservation is a concurrency convenience, not a correctness + // requirement (the response cache still dedups later retries). Proceed rather than 500 the + // request, matching the best-effort stance the response write already takes. + catch (Exception ex) when (ex is not OperationCanceledException) + { + logger.LogWarning(ex, "Idempotency reservation failed for key {KeyHash}; proceeding without it", HashKey(idempotencyKey)); + return true; + } } _ = ct; return InFlight.TryAdd(reservationKey, 0); } - private static async ValueTask ReleaseReservationAsync(IConnectionMultiplexer? multiplexer, string reservationKey) + private static async ValueTask ReleaseReservationAsync(IConnectionMultiplexer? multiplexer, string reservationKey, ILogger logger, string idempotencyKey) { if (multiplexer is not null) { - await multiplexer.GetDatabase().KeyDeleteAsync(reservationKey).ConfigureAwait(false); + try + { + await multiplexer.GetDatabase().KeyDeleteAsync(reservationKey).ConfigureAwait(false); + } + // Best-effort release: a Redis fault here must not throw out of the finally. The short + // ReservationTtl expires the key anyway, so a missed delete self-heals in seconds. + catch (Exception ex) when (ex is not OperationCanceledException) + { + logger.LogWarning(ex, "Failed to release idempotency reservation for key {KeyHash}", HashKey(idempotencyKey)); + } + return; } diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyOptions.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyOptions.cs index fd8bdac92b..73abe99891 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyOptions.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyOptions.cs @@ -15,6 +15,15 @@ public sealed class IdempotencyOptions /// public TimeSpan DefaultTtl { get; set; } = TimeSpan.FromHours(24); + /// + /// Time-to-live for the in-flight reservation that serializes concurrent duplicate keys. + /// Decoupled from : it must only outlast the handler's execution, so a + /// crash between reserving and releasing frees the key in seconds instead of stranding it for the + /// full response TTL (every retry would 409 until it expired). Must exceed the longest expected + /// handler runtime — if it lapses mid-request a concurrent duplicate can slip through. Default: 1 minute. + /// + public TimeSpan ReservationTtl { get; set; } = TimeSpan.FromMinutes(1); + /// /// Maximum allowed length for the idempotency key. Default: 128 characters. /// diff --git a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs index 154d3d7496..bba14bff0c 100644 --- a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -4,6 +4,8 @@ using Microsoft.Extensions.Caching.Distributed; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; +using NSubstitute; +using StackExchange.Redis; namespace Framework.Tests.Web; @@ -123,17 +125,83 @@ public async Task Filter_Should_ExecuteHandlerOnce_When_TwoConcurrentRequestsSha "a concurrent duplicate that arrives while the original is still running gets 409 Conflict."); } + // ─── HIGH: reservation TTL is the short ReservationTtl, not the 24h response TTL ───── + + [Fact] + public async Task Reservation_Should_UseReservationTtl_Not_ResponseTtl() + { + var options = new IdempotencyOptions + { + ReservationTtl = TimeSpan.FromSeconds(37), // distinct from DefaultTtl to prove which one is used + }; + var db = Substitute.For(); + TimeSpan? capturedTtl = null; + db.StringSetAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(ci => { capturedTtl = ci.ArgAt(2); return Task.FromResult(true); }); + var provider = BuildProvider(options, RedisMultiplexer(db)); + var filter = new IdempotencyEndpointFilter(); + + await filter.InvokeAsync( + new TestFilterContext(NewContext(provider)), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget")))); + + capturedTtl.ShouldBe( + options.ReservationTtl, + "the in-flight reservation must use the short ReservationTtl; keying it to the 24h response TTL " + + "would strand the lock for a day if the process is killed before the finally-release runs."); + capturedTtl.ShouldNotBe(options.DefaultTtl); + } + + // ─── MEDIUM + nit: a Redis fault on reserve/release fails open, never 500s ─────────── + + [Fact] + public async Task Filter_Should_ProceedWithoutThrowing_When_RedisReservationFaults() + { + var db = Substitute.For(); + db.StringSetAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromException(new RedisException("reserve blip"))); + db.KeyDeleteAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromException(new RedisException("release blip"))); + var provider = BuildProvider(new IdempotencyOptions(), RedisMultiplexer(db)); + var filter = new IdempotencyEndpointFilter(); + + int executions = 0; + var result = await filter.InvokeAsync( + new TestFilterContext(NewContext(provider)), + _ => { executions++; return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget"))); }); + + executions.ShouldBe( + 1, + "a transient Redis error on the reservation must fail open — the handler still runs. On main " + + "idempotency degraded gracefully; treating the reservation as authoritative would 500 the request."); + result.ShouldNotBeNull("the request must complete normally, not throw out of the filter"); + } + // ─── harness ───────────────────────────────────────────────────── - private static ServiceProvider BuildProvider() + private static ServiceProvider BuildProvider() => BuildProvider(new IdempotencyOptions(), multiplexer: null); + + private static ServiceProvider BuildProvider(IdempotencyOptions options, IConnectionMultiplexer? multiplexer) { var services = new ServiceCollection(); services.AddLogging(); services.AddDistributedMemoryCache(); - services.AddSingleton>(Options.Create(new IdempotencyOptions())); + services.AddSingleton>(Options.Create(options)); + if (multiplexer is not null) + { + services.AddSingleton(multiplexer); + } + return services.BuildServiceProvider(); } + private static IConnectionMultiplexer RedisMultiplexer(IDatabase db) + { + var mux = Substitute.For(); + mux.GetDatabase(Arg.Any(), Arg.Any()).Returns(db); + return mux; + } + private static DefaultHttpContext NewContext(IServiceProvider provider, Stream? responseBody = null) { var context = new DefaultHttpContext { RequestServices = provider };