From 709fcc2380a61a207ec0da167d6db2fd929a32c6 Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Tue, 4 Aug 2026 13:50:05 +0800 Subject: [PATCH 01/13] test(efcore-extensions): add Tier-2 architecture rule banning System.Console in library code Library code must emit diagnostics through an injected ILogger, never System.Console, so hosts can filter/redirect/redact/suppress them. Adds a NetArchTest rule over the DKNet.EfCore.Extensions assembly with a KnownViolations allow-list containing exactly today's single offender (EfCoreExceptionHandler, EfCoreExceptionHandler.cs:61). The rule fails on any NEW Console usage; the allow-list must only shrink. A companion self-check asserts the rule can still detect the known offender, so it can never silently go blind. Test-only, no production code touched. Part of monthly architecture review DRK-73. Co-Authored-By: Claude Opus 4.8 Co-authored-by: multica-agent --- .../ConsoleUsageArchitectureTests.cs | 66 +++++++++++++++++++ .../EfCore.Extensions.Tests.csproj | 1 + 2 files changed, 67 insertions(+) create mode 100644 src/EfCore/EfCore.Extensions.Tests/Architecture/ConsoleUsageArchitectureTests.cs diff --git a/src/EfCore/EfCore.Extensions.Tests/Architecture/ConsoleUsageArchitectureTests.cs b/src/EfCore/EfCore.Extensions.Tests/Architecture/ConsoleUsageArchitectureTests.cs new file mode 100644 index 00000000..a47e78c8 --- /dev/null +++ b/src/EfCore/EfCore.Extensions.Tests/Architecture/ConsoleUsageArchitectureTests.cs @@ -0,0 +1,66 @@ +using DKNet.EfCore.Extensions.Extensions; +using NetArchTest.Rules; + +namespace EfCore.Extensions.Tests.ArchRules; + +/// +/// Enforces that library code in DKNet.EfCore.Extensions never writes diagnostics through +/// . A framework primitive that writes to Console bypasses the host's +/// logging configuration entirely — it cannot be filtered, redirected, redacted, or suppressed — and pollutes +/// stdout in production. Diagnostics must go through an injected ILogger instead. +/// +/// This is a Tier-2 baseline rule (architecture-review DRK-73): there is exactly one known offender today, +/// (EfCoreExceptionHandler.cs:61), which is on the allow-list +/// below. The allow-list must only ever SHRINK — when the offender is migrated to ILogger, delete its +/// entry so the rule covers the whole assembly. Do not add new names to it. +/// +/// +public sealed class ConsoleUsageArchitectureTests +{ + #region Fields + + /// + /// Today's known offenders. Must only shrink — never add a name here to silence a new violation. + /// + private static readonly string[] KnownViolations = [nameof(EfCoreExceptionHandler)]; + + #endregion + + #region Methods + + [Fact] + public void ProductionTypes_ExceptKnownOffenders_MustNotDependOnSystemConsole() + { + var result = Types.InAssembly(typeof(EfCoreExceptionHandler).Assembly) + .That() + .DoNotHaveName(KnownViolations) + .Should() + .NotHaveDependencyOn("System.Console") + .GetResult(); + + var offenders = result.FailingTypeNames ?? []; + result.IsSuccessful.ShouldBeTrue( + "Library code must emit diagnostics through an injected ILogger, never System.Console, so hosts can " + + "filter/redirect/redact/suppress them. New offenders: " + string.Join(", ", offenders) + + ". Fix by injecting ILogger instead of calling Console.*; do not add the type to the KnownViolations allow-list."); + } + + [Fact] + public void Rule_CanDetectConsoleUsage_OnTheKnownOffender() + { + // Self-check: the allow-listed offender genuinely uses System.Console. If this ever passes, the rule above + // has gone blind (NetArchTest can no longer see the dependency) and would silently stop enforcing anything. + var result = Types.InAssembly(typeof(EfCoreExceptionHandler).Assembly) + .That() + .HaveName(nameof(EfCoreExceptionHandler)) + .Should() + .NotHaveDependencyOn("System.Console") + .GetResult(); + + result.IsSuccessful.ShouldBeFalse( + "The known offender EfCoreExceptionHandler must still be detected as depending on System.Console; " + + "if this assertion fails the enforcement rule can no longer see Console usage and is worthless."); + } + + #endregion +} diff --git a/src/EfCore/EfCore.Extensions.Tests/EfCore.Extensions.Tests.csproj b/src/EfCore/EfCore.Extensions.Tests/EfCore.Extensions.Tests.csproj index be534285..379fc54e 100644 --- a/src/EfCore/EfCore.Extensions.Tests/EfCore.Extensions.Tests.csproj +++ b/src/EfCore/EfCore.Extensions.Tests/EfCore.Extensions.Tests.csproj @@ -32,6 +32,7 @@ + runtime; build; native; contentfiles; analyzers; buildtransitive From b85e224f10664572305bd55f0173643e46dee73a Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Tue, 4 Aug 2026 16:54:06 +0800 Subject: [PATCH 02/13] fix(idempotency): close check-then-act race with atomic key reservation IsKeyProcessedAsync now atomically reserves a composite key (HTTP 102 placeholder) before returning, so concurrent requests with the same idempotency key can no longer all pass the check before any of them completes. The SQL store enforces this via UX_CompositeKey; the distributed-cache store narrows (documented, not eliminated) the same race since IDistributedCache has no compare-and-set primitive. Closes architecture finding IDEM-CONCURRENCY-001 (DRK-75). Co-authored-by: multica-agent --- .../IdempotencyIntegrationTests.cs | 19 +-- .../Data/IdempotencyKeyEntity.cs | 14 +++ .../Store/IdempotencySqlServerStore.cs | 110 ++++++++++++++---- .../IdempotencyOptions.cs | 9 ++ .../Store/IdempotencyDistributedCacheStore.cs | 62 +++++++--- .../Store/IdempotencyKeyStore.cs | 11 +- 6 files changed, 173 insertions(+), 52 deletions(-) diff --git a/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Integration/IdempotencyIntegrationTests.cs b/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Integration/IdempotencyIntegrationTests.cs index b70e53e3..f2acbda4 100644 --- a/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Integration/IdempotencyIntegrationTests.cs +++ b/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Integration/IdempotencyIntegrationTests.cs @@ -55,9 +55,9 @@ public async Task CreateItem_ConcurrentRequestsWithSameKey_OnlyOneProcessed() var idempotencyKey = Guid.NewGuid().ToString(); var request = new CreateItemRequest { Name = "Concurrent Item" }; - // Act - Send 5 concurrent requests with the same idempotency key - // multiple requests may process simultaneously when they all arrive before any is cached. - // This is a known limitation of stateless filters without distributed locking. + // Act - Send 5 concurrent requests with the same idempotency key. + // The atomic reserve-then-check in IsKeyProcessedAsync guarantees exactly one request reaches the + // handler; the other four are turned away as conflicts before they ever call next(context). var tasks = Enumerable.Range(0, 5).Select(_ => { var httpRequest = new HttpRequestMessage(HttpMethod.Post, "/api/items") @@ -70,19 +70,12 @@ public async Task CreateItem_ConcurrentRequestsWithSameKey_OnlyOneProcessed() var responses = await Task.WhenAll(tasks); - // Assert - With concurrent requests, some or all may succeed (201 or 409 Conflict) - // The first request wins, subsequent ones get 409 Conflict (if ConflictHandling = ConflictResponse) - // or the cached response (if ConflictHandling = CachedResult) + // Assert - Exactly one request succeeds, the remaining four are conflicts. var successCount = responses.Count(r => r.StatusCode == HttpStatusCode.Created); var conflictCount = responses.Count(r => r.StatusCode == HttpStatusCode.Conflict); - // At least one should succeed (201) - successCount.ShouldBeGreaterThanOrEqualTo(1); - - // The rest should be conflicts or cached responses - var otherCount = responses.Count(r => r.StatusCode != HttpStatusCode.Created && - r.StatusCode != HttpStatusCode.Conflict); - (successCount + conflictCount + otherCount).ShouldBe(5); + successCount.ShouldBe(1); + conflictCount.ShouldBe(4); // Verify only ONE entry in database (unique constraint ensures this) await using var dbContext = fixture.GetDbContext(); diff --git a/src/AspNet/DKNet.AspCore.Idempotency.MsSqlStore/Data/IdempotencyKeyEntity.cs b/src/AspNet/DKNet.AspCore.Idempotency.MsSqlStore/Data/IdempotencyKeyEntity.cs index 9284f767..e194b549 100644 --- a/src/AspNet/DKNet.AspCore.Idempotency.MsSqlStore/Data/IdempotencyKeyEntity.cs +++ b/src/AspNet/DKNet.AspCore.Idempotency.MsSqlStore/Data/IdempotencyKeyEntity.cs @@ -100,6 +100,20 @@ private IdempotencyKeyEntity() #region Methods + /// + /// Overwrites this reservation placeholder with the final, completed response. + /// Used to turn an in-flight reservation row into the durable cached result once the + /// protected handler has finished, mirroring the constructor's field assignments. + /// + /// The completed response to store. + internal void Complete(CachedResponse response) + { + StatusCode = response.StatusCode; + Body = response.Body; + ContentType = response.ContentType; + ExpiresAt = response.ExpiresAt; + } + /// /// Sanitizes an idempotency key for use as a database key. /// Removes invalid characters to prevent injection attacks. diff --git a/src/AspNet/DKNet.AspCore.Idempotency.MsSqlStore/Store/IdempotencySqlServerStore.cs b/src/AspNet/DKNet.AspCore.Idempotency.MsSqlStore/Store/IdempotencySqlServerStore.cs index 739e3921..fbcd4f68 100644 --- a/src/AspNet/DKNet.AspCore.Idempotency.MsSqlStore/Store/IdempotencySqlServerStore.cs +++ b/src/AspNet/DKNet.AspCore.Idempotency.MsSqlStore/Store/IdempotencySqlServerStore.cs @@ -8,6 +8,7 @@ using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; namespace DKNet.AspCore.Idempotency.MsSqlStore.Store; @@ -17,13 +18,22 @@ namespace DKNet.AspCore.Idempotency.MsSqlStore.Store; /// internal sealed class IdempotencySqlServerStore( IServiceProvider serviceProvider, + IOptions options, ILogger logger) : IIdempotencyKeyStore, IAsyncDisposable { #region Fields + /// + /// HTTP 102 (Processing) is used as the sentinel status code for an in-flight reservation row — + /// legal under CK_StatusCode_Valid (100-599) and outside the range any real completed + /// response would use. + /// + private const int ReservationStatusCode = 102; + private static int _dbMigrationsEnsured; private static readonly SemaphoreSlim MigrationLock = new(1, 1); + private readonly IdempotencyOptions _options = options.Value; private readonly AsyncServiceScope _scope = serviceProvider.CreateAsyncScope(); #endregion @@ -56,6 +66,19 @@ private static async ValueTask EnsureDatabaseCreatedAsync(DbContext dbContext, } } + /// + /// Maps a stored entity to its representation for replay. + /// + private static CachedResponse ToCachedResponse(IdempotencyKeyEntity entity) => + new() + { + StatusCode = entity.StatusCode, + Body = entity.Body, + ContentType = entity.ContentType ?? "application/json", + CreatedAt = entity.CreatedAt, + ExpiresAt = entity.ExpiresAt + }; + /// public async ValueTask<(bool processed, CachedResponse? response)> IsKeyProcessedAsync(IdempotentKeyInfo keyInfo) { @@ -73,33 +96,64 @@ private static async ValueTask EnsureDatabaseCreatedAsync(DbContext dbContext, .FirstOrDefaultAsync(k => k.CompositeKey == sanitizedKey && k.ExpiresAt > DateTime.UtcNow) .ConfigureAwait(false); - if (existing == null) + if (existing != null && !existing.IsExpired) { - logger.LogDebug("Idempotency key not found or expired: {Key}", sanitizedKey); - return (false, null); + if (existing.StatusCode == ReservationStatusCode) + { + logger.LogDebug("Idempotency key reservation still in-flight: {Key}", sanitizedKey); + return (true, null); + } + + logger.LogInformation( + "Idempotency key found with status code {StatusCode}: {Key}", + existing.StatusCode, + sanitizedKey); + + return (true, ToCachedResponse(existing)); } - if (existing.IsExpired) + // No completed row (and no live reservation) found — reserve this key for the current caller so a + // concurrent request for the identical key can never also observe an empty slot. + logger.LogDebug("Idempotency key not found or expired, reserving: {Key}", sanitizedKey); + + var reservation = new IdempotencyKeyEntity(keyInfo, new CachedResponse + { + StatusCode = ReservationStatusCode, + Body = null, + ContentType = string.Empty, + CreatedAt = DateTimeOffset.UtcNow, + ExpiresAt = DateTimeOffset.UtcNow.Add(_options.InFlightReservationTimeout) + }); + + try { - logger.LogDebug("Idempotency key has expired: {Key}", sanitizedKey); + dbContext.IdempotencyKeys.Add(reservation); + await dbContext.SaveChangesAsync().ConfigureAwait(false); return (false, null); } + catch (DbUpdateException ex) when ((ex.InnerException?.Message ?? ex.Message).Contains("UNIQUE", StringComparison.OrdinalIgnoreCase)) + { + // Handle race condition: another concurrent request already reserved or completed this key. + logger.LogInformation( + "Idempotency key reservation collided with a concurrent request: {Key}. Re-checking status.", + sanitizedKey); - logger.LogInformation( - "Idempotency key found with status code {StatusCode}: {Key}", - existing.StatusCode, - sanitizedKey); + var concurrent = await dbContext.IdempotencyKeys + .AsNoTracking() + .FirstOrDefaultAsync(k => k.CompositeKey == sanitizedKey) + .ConfigureAwait(false); - var cachedResponse = new CachedResponse - { - StatusCode = existing.StatusCode, - Body = existing.Body, - ContentType = existing.ContentType ?? "application/json", - CreatedAt = existing.CreatedAt, - ExpiresAt = existing.ExpiresAt - }; + if (concurrent is null || concurrent.IsExpired) + { + // The competing row is gone or has since expired — treat the key as not found, matching + // Rule R1: an abandoned reservation must not permanently block retries. + return (false, null); + } + + if (concurrent.StatusCode == ReservationStatusCode) return (true, null); - return (true, cachedResponse); + return (true, ToCachedResponse(concurrent)); + } } /// @@ -117,11 +171,23 @@ public async ValueTask MarkKeyAsProcessedAsync(IdempotentKeyInfo keyInfo, Cached var factory = _scope.ServiceProvider.GetRequiredService>(); await using var dbContext = await factory.CreateDbContextAsync(); - - var entity = new IdempotencyKeyEntity(keyInfo, cachedResponse); await EnsureDatabaseCreatedAsync(dbContext); - dbContext.IdempotencyKeys.Add(entity); + var entity = await dbContext.IdempotencyKeys + .FirstOrDefaultAsync(k => k.CompositeKey == sanitizedKey) + .ConfigureAwait(false); + + if (entity is null) + { + // Defensive only: should not happen once IsKeyProcessedAsync always reserves first. + entity = new IdempotencyKeyEntity(keyInfo, cachedResponse); + dbContext.IdempotencyKeys.Add(entity); + } + else + { + entity.Complete(cachedResponse); + } + await dbContext.SaveChangesAsync().ConfigureAwait(false); logger.LogInformation( @@ -131,7 +197,7 @@ public async ValueTask MarkKeyAsProcessedAsync(IdempotentKeyInfo keyInfo, Cached } catch (DbUpdateException ex) when ((ex.InnerException?.Message??ex.Message).Contains("UNIQUE",StringComparison.OrdinalIgnoreCase)) { - // Handle race condition: Another concurrent request already inserted this key + // Handle race condition: another concurrent request already inserted this key. logger.LogInformation( "Idempotency key already processed by concurrent request: {Key}. Continuing without duplicate insert.", sanitizedKey); diff --git a/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs b/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs index 7b97188b..d8d7f176 100644 --- a/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs +++ b/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs @@ -63,6 +63,15 @@ public sealed class IdempotencyOptions /// public string IdempotencyHeaderKey { get; set; } = "X-Idempotency-Key"; + /// + /// Gets or sets how long an in-flight reservation placeholder is honoured before it is treated as + /// expired/abandoned (e.g. the original handler crashed and never completed the request). + /// Deliberately short compared to so a genuinely failed request doesn't + /// permanently block retries. + /// Default is 30 seconds. + /// + public TimeSpan InFlightReservationTimeout { get; set; } = TimeSpan.FromSeconds(30); + /// /// Gets or sets a regular expression pattern used to validate idempotency key format. /// Keys that don't match this pattern will be rejected with a 400 Bad Request. diff --git a/src/AspNet/DKNet.AspCore.Idempotency/Store/IdempotencyDistributedCacheStore.cs b/src/AspNet/DKNet.AspCore.Idempotency/Store/IdempotencyDistributedCacheStore.cs index 4d0b5d08..4ed69e4d 100644 --- a/src/AspNet/DKNet.AspCore.Idempotency/Store/IdempotencyDistributedCacheStore.cs +++ b/src/AspNet/DKNet.AspCore.Idempotency/Store/IdempotencyDistributedCacheStore.cs @@ -5,6 +5,12 @@ namespace DKNet.AspCore.Idempotency.Store; +/// +/// has no atomic compare-and-set primitive, so this store narrows the +/// check-then-act race window to the gap between GetStringAsync and SetStringAsync in +/// rather than eliminating it the way the SQL store's unique index does. +/// Do not treat this store as fully atomic under concurrency. +/// internal sealed class IdempotencyDistributedCacheStore( IDistributedCache cache, IOptions options, @@ -12,6 +18,11 @@ internal sealed class IdempotencyDistributedCacheStore( { #region Fields + /// + /// HTTP 102 (Processing) is used as the sentinel status code for an in-flight reservation entry. + /// + private const int ReservationStatusCode = 102; + /// /// Gets the idempotency options used for cache configuration and JSON serialization. /// @@ -38,25 +49,50 @@ internal sealed class IdempotencyDistributedCacheStore( var cachedJson = await cache.GetStringAsync(cacheKey).ConfigureAwait(false); - if (string.IsNullOrWhiteSpace(cachedJson)) + if (!string.IsNullOrWhiteSpace(cachedJson)) { - logger.LogDebug("No cached response found for key: {CacheKey}", cacheKey); - return (false, null); + var cachedResponse = JsonSerializer.Deserialize(cachedJson, _options.JsonSerializerOptions); + + if (cachedResponse?.IsExpired == true) + { + logger.LogDebug("Cached response has expired for key: {CacheKey}", cacheKey); + await cache.RemoveAsync(cacheKey).ConfigureAwait(false); + } + else if (cachedResponse?.StatusCode == ReservationStatusCode) + { + logger.LogDebug("Reservation still in-flight for key: {CacheKey}", cacheKey); + return (true, null); + } + else + { + logger.LogDebug("Cached response found for key: {CacheKey} with status code: {StatusCode}", + cacheKey, cachedResponse?.StatusCode); + return (true, cachedResponse); + } } - var cachedResponse = JsonSerializer.Deserialize(cachedJson, _options.JsonSerializerOptions); + // No live entry found — reserve this key so a concurrent request for the identical key sees the + // in-flight placeholder instead of also observing a miss (see class remarks for the residual race). + logger.LogDebug("No cached response found for key: {CacheKey}. Reserving.", cacheKey); - // Check if the cached response has expired - if (cachedResponse?.IsExpired == true) + var reservation = new CachedResponse { - logger.LogDebug("Cached response has expired for key: {CacheKey}", cacheKey); - await cache.RemoveAsync(cacheKey).ConfigureAwait(false); - return (false, null); - } + StatusCode = ReservationStatusCode, + Body = null, + ContentType = string.Empty, + CreatedAt = DateTimeOffset.UtcNow, + ExpiresAt = DateTimeOffset.UtcNow.Add(_options.InFlightReservationTimeout) + }; + + await cache.SetStringAsync( + cacheKey, + JsonSerializer.Serialize(reservation, _options.JsonSerializerOptions), + new DistributedCacheEntryOptions + { + AbsoluteExpirationRelativeToNow = _options.InFlightReservationTimeout + }).ConfigureAwait(false); - logger.LogDebug("Cached response found for key: {CacheKey} with status code: {StatusCode}", - cacheKey, cachedResponse?.StatusCode); - return (true, cachedResponse); + return (false, null); } /// diff --git a/src/AspNet/DKNet.AspCore.Idempotency/Store/IdempotencyKeyStore.cs b/src/AspNet/DKNet.AspCore.Idempotency/Store/IdempotencyKeyStore.cs index 47756e6b..f6da57d3 100644 --- a/src/AspNet/DKNet.AspCore.Idempotency/Store/IdempotencyKeyStore.cs +++ b/src/AspNet/DKNet.AspCore.Idempotency/Store/IdempotencyKeyStore.cs @@ -7,14 +7,17 @@ namespace DKNet.AspCore.Idempotency.Store; public interface IIdempotencyKeyStore { /// - /// Checks if the key has been processed and retrieves the cached response if available. - /// Returns the cached response including HTTP status code, body, and content type. + /// Atomically checks whether the key has been processed and, if not, reserves it for the caller. + /// A call that returns (false, null) MUST have already durably recorded that this composite key + /// is now in-flight, such that a concurrent call for the identical key can never also observe + /// (false, null) — exactly one caller per key is granted the right to proceed. /// /// The idempotency key to check for prior processing. /// /// A tuple containing: - /// - A boolean indicating whether the key has been processed - /// - The CachedResponse if available, or null if no cached response exists + /// - A boolean indicating whether the key has been processed or is currently reserved by another caller + /// - The CachedResponse if the key was already completed, or null if no cached response exists yet + /// (either the key is new and now reserved by this call, or another caller's reservation is still in-flight) /// ValueTask<(bool processed, CachedResponse? response)> IsKeyProcessedAsync(IdempotentKeyInfo keyInfo); From 20be9fcaaedfa77e64a0657dea618b48c448aa63 Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Wed, 5 Aug 2026 08:39:35 +0800 Subject: [PATCH 03/13] fix(encryption): make _disposed volatile and correct ignoreCase docs - HmacHashing and ShaHashing _disposed fields are now volatile so a Dispose() on one thread is deterministically visible to a Compute/ Verify/Dispose read on another - IHmacHashing/IShaHashing VerifySha256/VerifySha512 ignoreCase param docs corrected: the flag has no effect on the comparison result Co-authored-by: multica-agent --- src/Services/DKNet.Svc.Encryption/HmacHashing.cs | 6 +++--- src/Services/DKNet.Svc.Encryption/ShaHashing.cs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Services/DKNet.Svc.Encryption/HmacHashing.cs b/src/Services/DKNet.Svc.Encryption/HmacHashing.cs index befe363d..b437fc1c 100644 --- a/src/Services/DKNet.Svc.Encryption/HmacHashing.cs +++ b/src/Services/DKNet.Svc.Encryption/HmacHashing.cs @@ -41,7 +41,7 @@ public interface IHmacHashing : IDisposable /// The secret key to use for hashing. /// The expected hash signature to compare against. /// If true, the signature is base64-encoded; otherwise, hexadecimal. - /// If true, ignores case when comparing signatures. + /// Has no effect on the result: the comparison always uses on the decoded signature bytes, so signatures are compared exactly regardless of this flag. /// true if the computed hash matches the expected signature; otherwise, false. bool VerifySha256( string message, @@ -57,7 +57,7 @@ bool VerifySha256( /// The secret key to use for hashing. /// The expected hash signature to compare against. /// If true, the signature is base64-encoded; otherwise, hexadecimal. - /// If true, ignores case when comparing signatures. + /// Has no effect on the result: the comparison always uses on the decoded signature bytes, so signatures are compared exactly regardless of this flag. /// true if the computed hash matches the expected signature; otherwise, false. bool VerifySha512( string message, @@ -78,7 +78,7 @@ public sealed class HmacHashing : IHmacHashing private readonly Dictionary<(HmacAlgorithm alg, string key), HMAC> _cache = []; private readonly Lock _sync = new(); - private bool _disposed; + private volatile bool _disposed; #endregion diff --git a/src/Services/DKNet.Svc.Encryption/ShaHashing.cs b/src/Services/DKNet.Svc.Encryption/ShaHashing.cs index 2223b990..5b8ad88a 100644 --- a/src/Services/DKNet.Svc.Encryption/ShaHashing.cs +++ b/src/Services/DKNet.Svc.Encryption/ShaHashing.cs @@ -44,7 +44,7 @@ public interface IShaHashing : IDisposable // now disposable so we can release c /// /// The input text to hash and compare. /// The expected hexadecimal hash string. - /// If true performs a case-insensitive comparison. + /// Has no effect on the result: hex decoding is case-insensitive, so the comparison result is unaffected by this flag. /// true if the computed hash equals ; otherwise false. bool VerifySha256(string input, string expectedHex, bool ignoreCase = true); @@ -53,7 +53,7 @@ public interface IShaHashing : IDisposable // now disposable so we can release c /// /// The input text to hash and compare. /// The expected hexadecimal hash string. - /// If true performs a case-insensitive comparison. + /// Has no effect on the result: hex decoding is case-insensitive, so the comparison result is unaffected by this flag. /// true if the computed hash equals ; otherwise false. bool VerifySha512(string input, string expectedHex, bool ignoreCase = true); @@ -69,7 +69,7 @@ public sealed class ShaHashing : IShaHashing private readonly Dictionary _algorithms = []; private readonly object _sync = new(); // changed to object for locking - private bool _disposed; + private volatile bool _disposed; #endregion From efbafdd25734ffd34fab3924a1646c8119b662f5 Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Wed, 5 Aug 2026 08:59:23 +0800 Subject: [PATCH 04/13] fix(blobstorage): wire S3 test fixture to Minio, remove hardcoded R2 creds S3BlobServiceFixture now reads connection string/access key/secret from the MinioContainer it already starts, instead of hardcoding live-looking Cloudflare R2 credentials. Adds ForcePathStyle (required for Minio's IP:port endpoint) and disables payload signing over plain HTTP. That surfaced two real S3BlobService bugs only exercised now that a path-style endpoint is in play: - GetBlobLocation()'s leading slash collided with path-style addressing (bucket//key), breaking SigV4 signatures on every request. Trimmed it at each call site; GetBlobLocation() itself is unchanged (locked contract test covers Azure/Local too). - Batch DeleteObjects requires a Content-MD5 the SDK's checksum pipeline won't auto-compute and Minio won't accept a substitute for; switched folder delete to per-key DeleteObjectAsync calls. Also replaces the same leaked R2 endpoint/key/secret literals (inert, pre-network-validation only) in BlobServiceSaveAsyncTests.cs with fake placeholders, and rewires its one test that actually saves data through the Minio-backed fixture instead of the now-fake endpoint. DisablePayloadSigning left as false is a deliberate deviation from the original plan (true fails immediately over Minio's plain-HTTP endpoint). Co-Authored-By: Claude Sonnet 5 Co-authored-by: multica-agent --- .../S3BlobService.cs | 33 +++++++++--------- .../BlobServiceSaveAsyncTests.cs | 34 ++++++++----------- .../Fixtures/S3BlobServiceFixture.cs | 12 +++---- 3 files changed, 36 insertions(+), 43 deletions(-) diff --git a/src/Services/DKNet.Svc.BlobStorage.AwsS3/S3BlobService.cs b/src/Services/DKNet.Svc.BlobStorage.AwsS3/S3BlobService.cs index d08708ee..4b0fff72 100644 --- a/src/Services/DKNet.Svc.BlobStorage.AwsS3/S3BlobService.cs +++ b/src/Services/DKNet.Svc.BlobStorage.AwsS3/S3BlobService.cs @@ -42,7 +42,9 @@ public sealed class S3BlobService(IOptions options, ILoggertrue when a matching object exists; otherwise false. public override async Task CheckExistsAsync(BlobRequest blob, CancellationToken cancellationToken = default) { - var location = GetBlobLocation(blob); + // S3 keys are not path-segments: a leading slash here would double up with the bucket + // segment under ForcePathStyle addressing (e.g. Minio) and break SigV4 signing. + var location = GetBlobLocation(blob).TrimStart('/'); var client = await GetS3ClientAsync(cancellationToken); try @@ -78,7 +80,7 @@ public override async Task CheckExistsAsync(BlobRequest blob, Cancellation /// true when deletion completed (or object did not exist); otherwise false. public override Task DeleteAsync(BlobRequest blob, CancellationToken cancellationToken = default) { - var location = GetBlobLocation(blob); + var location = GetBlobLocation(blob).TrimStart('/'); return blob.Type == BlobTypes.File ? DeleteFileAsync(location, cancellationToken) : DeleteFolderAsync(location, cancellationToken); @@ -122,16 +124,15 @@ private async Task DeleteFolderAsync(string folderLocation, CancellationTo cancellationToken.ThrowIfCancellationRequested(); - // Delete the whole page in a single request. A ListObjectsV2 page holds at most - // 1000 keys, which is exactly the S3 batch-delete limit, so one call per page suffices - // instead of one round-trip per object. - await client.DeleteObjectsAsync( - new DeleteObjectsRequest - { - BucketName = _options.BucketName, - Objects = info.S3Objects.Select(o => new KeyVersion { Key = o.Key }).ToList() - }, - cancellationToken); + // ponytail: one DeleteObjectAsync call per key instead of a single batch + // DeleteObjectsAsync for the page. The batch API requires a Content-MD5 request-body + // checksum, which the SDK's flexible-checksum pipeline won't auto-compute (MD5 is + // blocked as "unsupported") and Minio won't accept a substitute algorithm for — so + // batch delete cannot work against Minio on this SDK version. Upgrade path: switch + // back to DeleteObjectsAsync once the SDK supports precalculated Content-MD5 or Minio + // accepts a modern checksum trailer for Multi-Object Delete. + await Task.WhenAll(info.S3Objects.Select(o => + client.DeleteObjectAsync(_options.BucketName, o.Key, cancellationToken))); } while (true); await client.DeleteObjectAsync(_options.BucketName, folderLocation, cancellationToken); @@ -165,7 +166,7 @@ private void Dispose(bool disposing) BlobRequest blob, CancellationToken cancellationToken = default) { - var location = GetBlobLocation(blob); + var location = GetBlobLocation(blob).TrimStart('/'); var client = await GetS3ClientAsync(cancellationToken); try { @@ -204,7 +205,7 @@ public override async Task GetPublicAccessUrl( TimeSpan? expiresFromNow = null, CancellationToken cancellationToken = default) { - var location = GetBlobLocation(blob); + var location = GetBlobLocation(blob).TrimStart('/'); var client = await GetS3ClientAsync(cancellationToken); var request = new GetPreSignedUrlRequest { @@ -274,7 +275,7 @@ await _client.PutBucketAsync( BlobRequest blob, [EnumeratorCancellation] CancellationToken cancellationToken = default) { - var location = GetBlobLocation(blob); + var location = GetBlobLocation(blob).TrimStart('/'); var client = await GetS3ClientAsync(cancellationToken); var info = await client.ListObjectsV2Async( @@ -321,7 +322,7 @@ public override async Task SaveAsync(BlobDetails.BlobData blob, if (existed && !blob.Overwrite) throw new InvalidOperationException($"File {blob.Name} is not allowed to override."); - var location = GetBlobLocation(blob); + var location = GetBlobLocation(blob).TrimStart('/'); var client = await GetS3ClientAsync(cancellationToken); var uploadRequest = new PutObjectRequest diff --git a/src/Services/Svc.BlobStorage.Tests/BlobServiceSaveAsyncTests.cs b/src/Services/Svc.BlobStorage.Tests/BlobServiceSaveAsyncTests.cs index 2d61d62d..e0b24bc1 100644 --- a/src/Services/Svc.BlobStorage.Tests/BlobServiceSaveAsyncTests.cs +++ b/src/Services/Svc.BlobStorage.Tests/BlobServiceSaveAsyncTests.cs @@ -77,9 +77,9 @@ public async Task S3_SaveAsync_DisallowedExtension_ShouldThrowFileLoadException( using var fixture = new S3BlobServiceFixture(); var options = new S3Options { - ConnectionString = "https://c4bf6253a59daf70a445861c23b45778.r2.cloudflarestorage.com", - AccessKey = "c5240e9de9fb8f2b24d67315eed90737", - Secret = "df8dc0fe841d98c8c8429e3fbe5a6e0e784865e835860b4ffeb65913d7e7346b", + ConnectionString = "https://fake-test-account.example.com", + AccessKey = "FAKEACCESSKEYFORTESTS00", + Secret = "FAKESECRETKEYFORTESTSONLY0000000000000000000000", BucketName = "dev", DisablePayloadSigning = true, IncludedExtensions = [".txt"] @@ -97,9 +97,9 @@ public async Task S3_SaveAsync_OversizedFile_ShouldThrowFileLoadException() using var fixture = new S3BlobServiceFixture(); var options = new S3Options { - ConnectionString = "https://c4bf6253a59daf70a445861c23b45778.r2.cloudflarestorage.com", - AccessKey = "c5240e9de9fb8f2b24d67315eed90737", - Secret = "df8dc0fe841d98c8c8429e3fbe5a6e0e784865e835860b4ffeb65913d7e7346b", + ConnectionString = "https://fake-test-account.example.com", + AccessKey = "FAKEACCESSKEYFORTESTS00", + Secret = "FAKESECRETKEYFORTESTSONLY0000000000000000000000", BucketName = "dev", DisablePayloadSigning = true, MaxFileSizeInMb = 1 @@ -118,9 +118,9 @@ public async Task S3_SaveAsync_FileNameTooLong_ShouldThrowFileLoadException() using var fixture = new S3BlobServiceFixture(); var options = new S3Options { - ConnectionString = "https://c4bf6253a59daf70a445861c23b45778.r2.cloudflarestorage.com", - AccessKey = "c5240e9de9fb8f2b24d67315eed90737", - Secret = "df8dc0fe841d98c8c8429e3fbe5a6e0e784865e835860b4ffeb65913d7e7346b", + ConnectionString = "https://fake-test-account.example.com", + AccessKey = "FAKEACCESSKEYFORTESTS00", + Secret = "FAKESECRETKEYFORTESTSONLY0000000000000000000000", BucketName = "dev", DisablePayloadSigning = true, MaxFileNameLength = 5 @@ -135,16 +135,10 @@ public async Task S3_SaveAsync_FileNameTooLong_ShouldThrowFileLoadException() [Fact] public async Task S3_SaveAsync_DefaultOptions_ShouldSucceed() { + // Unlike the validation-rejection tests above, this one actually saves — it needs a live + // backend, so it goes through the Minio-backed fixture instead of a placeholder S3Options. using var fixture = new S3BlobServiceFixture(); - var options = new S3Options - { - ConnectionString = "https://c4bf6253a59daf70a445861c23b45778.r2.cloudflarestorage.com", - AccessKey = "c5240e9de9fb8f2b24d67315eed90737", - Secret = "df8dc0fe841d98c8c8429e3fbe5a6e0e784865e835860b4ffeb65913d7e7346b", - BucketName = "dev", - DisablePayloadSigning = true - }; - var service = new S3BlobService(Options.Create(options), NullLogger.Instance); + var service = fixture.Service; var blobData = new BlobDetails.BlobData("test.txt", BinaryData.FromString("test")) { Overwrite = true }; var result = await service.SaveAsync(blobData); @@ -217,10 +211,10 @@ public async Task S3_GetItemAsync_ShouldWork() // Save await service.SaveAsync(blobData); - // GetItem + // GetItem — S3 keys carry no leading slash (see S3BlobService's GetBlobLocation trim) var item = await service.GetItemAsync(new BlobRequest(blobName)); item.ShouldNotBeNull(); - item!.Name.ShouldBe($"/{blobName}"); + item!.Name.ShouldBe(blobName); // Cleanup await service.DeleteAsync(new BlobRequest(blobName)); diff --git a/src/Services/Svc.BlobStorage.Tests/Fixtures/S3BlobServiceFixture.cs b/src/Services/Svc.BlobStorage.Tests/Fixtures/S3BlobServiceFixture.cs index 8148afba..7f801984 100644 --- a/src/Services/Svc.BlobStorage.Tests/Fixtures/S3BlobServiceFixture.cs +++ b/src/Services/Svc.BlobStorage.Tests/Fixtures/S3BlobServiceFixture.cs @@ -24,14 +24,12 @@ public S3BlobServiceFixture() .AddInMemoryCollection( new Dictionary(StringComparer.OrdinalIgnoreCase) { - { - "BlobService:S3:ConnectionString", - "https://c4bf6253a59daf70a445861c23b45778.r2.cloudflarestorage.com" - }, - { "BlobService:S3:AccessKey", "c5240e9de9fb8f2b24d67315eed90737" }, - { "BlobService:S3:Secret", "df8dc0fe841d98c8c8429e3fbe5a6e0e784865e835860b4ffeb65913d7e7346b" }, + { "BlobService:S3:ConnectionString", _minioContainer.GetConnectionString() }, + { "BlobService:S3:AccessKey", _minioContainer.GetAccessKey() }, + { "BlobService:S3:Secret", _minioContainer.GetSecretKey() }, { "BlobService:S3:BucketName", "dev" }, - { "BlobService:S3:DisablePayloadSigning", "true" } + { "BlobService:S3:DisablePayloadSigning", "false" }, + { "BlobService:S3:ForcePathStyle", "true" } }) .Build(); From 42c68f558df9a7b92c4d80101cff09173fbfd809 Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Wed, 5 Aug 2026 10:26:47 +0800 Subject: [PATCH 05/13] test(encryption): add dispose and ignoreCase no-op tests for HmacHashing and ShaHashing Co-authored-by: multica-agent --- .../HmacHashingDisposeTests.cs | 45 +++++++++++++++++ .../HmacHashingIgnoreCaseTests.cs | 48 +++++++++++++++++++ .../ShaHashingDisposeTests.cs | 45 +++++++++++++++++ .../ShaHashingIgnoreCaseTests.cs | 46 ++++++++++++++++++ 4 files changed, 184 insertions(+) create mode 100644 src/Services/Svc.Encryption.Tests/HmacHashingDisposeTests.cs create mode 100644 src/Services/Svc.Encryption.Tests/HmacHashingIgnoreCaseTests.cs create mode 100644 src/Services/Svc.Encryption.Tests/ShaHashingDisposeTests.cs create mode 100644 src/Services/Svc.Encryption.Tests/ShaHashingIgnoreCaseTests.cs diff --git a/src/Services/Svc.Encryption.Tests/HmacHashingDisposeTests.cs b/src/Services/Svc.Encryption.Tests/HmacHashingDisposeTests.cs new file mode 100644 index 00000000..f1d9ec4b --- /dev/null +++ b/src/Services/Svc.Encryption.Tests/HmacHashingDisposeTests.cs @@ -0,0 +1,45 @@ +using System; +using DKNet.Svc.Encryption; +using Shouldly; +using Xunit; + +namespace Svc.Encryption.Tests; + +public class HmacHashingDisposeTests +{ + #region Methods + + [Fact] + public void HmacHashing_AfterDispose_ThrowsObjectDisposedException_OnComputeSha256() + { + var hmac = new HmacHashing(); + hmac.Dispose(); + Should.Throw(() => hmac.ComputeSha256("message", "key")); + } + + [Fact] + public void HmacHashing_AfterDispose_ThrowsObjectDisposedException_OnComputeSha512() + { + var hmac = new HmacHashing(); + hmac.Dispose(); + Should.Throw(() => hmac.ComputeSha512("message", "key")); + } + + [Fact] + public void HmacHashing_AfterDispose_ThrowsObjectDisposedException_OnVerifySha256() + { + var hmac = new HmacHashing(); + hmac.Dispose(); + Should.Throw(() => hmac.VerifySha256("message", "key", "sig")); + } + + [Fact] + public void HmacHashing_AfterDispose_ThrowsObjectDisposedException_OnVerifySha512() + { + var hmac = new HmacHashing(); + hmac.Dispose(); + Should.Throw(() => hmac.VerifySha512("message", "key", "sig")); + } + + #endregion +} diff --git a/src/Services/Svc.Encryption.Tests/HmacHashingIgnoreCaseTests.cs b/src/Services/Svc.Encryption.Tests/HmacHashingIgnoreCaseTests.cs new file mode 100644 index 00000000..42f32b08 --- /dev/null +++ b/src/Services/Svc.Encryption.Tests/HmacHashingIgnoreCaseTests.cs @@ -0,0 +1,48 @@ +using System; +using System.Linq; +using DKNet.Svc.Encryption; +using Shouldly; +using Xunit; + +namespace Svc.Encryption.Tests; + +public class HmacHashingIgnoreCaseTests +{ + #region Methods + + [Fact] + public void HmacHashing_VerifySha256_IgnoreCaseTrueOrFalse_ReturnsSameResult() + { + using var hmac = new HmacHashing(); + var message = "test message"; + var secretKey = "secret"; + var sig = hmac.ComputeSha256(message, secretKey, false); // hex signature + + var mixedCaseSig = new string([.. sig.Select((c, i) => i % 2 == 0 ? char.ToUpper(c) : char.ToLower(c))]); + + var rTrue = hmac.VerifySha256(message, secretKey, mixedCaseSig, false, true); + var rFalse = hmac.VerifySha256(message, secretKey, mixedCaseSig, false, false); + + rTrue.ShouldBe(rFalse); + rTrue.ShouldBeTrue(); + } + + [Fact] + public void HmacHashing_VerifySha512_IgnoreCaseTrueOrFalse_ReturnsSameResult() + { + using var hmac = new HmacHashing(); + var message = "test message"; + var secretKey = "secret"; + var sig = hmac.ComputeSha512(message, secretKey, false); // hex signature + + var mixedCaseSig = new string([.. sig.Select((c, i) => i % 2 == 0 ? char.ToUpper(c) : char.ToLower(c))]); + + var rTrue = hmac.VerifySha512(message, secretKey, mixedCaseSig, false, true); + var rFalse = hmac.VerifySha512(message, secretKey, mixedCaseSig, false, false); + + rTrue.ShouldBe(rFalse); + rTrue.ShouldBeTrue(); + } + + #endregion +} diff --git a/src/Services/Svc.Encryption.Tests/ShaHashingDisposeTests.cs b/src/Services/Svc.Encryption.Tests/ShaHashingDisposeTests.cs new file mode 100644 index 00000000..8b38734d --- /dev/null +++ b/src/Services/Svc.Encryption.Tests/ShaHashingDisposeTests.cs @@ -0,0 +1,45 @@ +using System; +using DKNet.Svc.Encryption; +using Shouldly; +using Xunit; + +namespace Svc.Encryption.Tests; + +public class ShaHashingDisposeTests +{ + #region Methods + + [Fact] + public void ShaHashing_AfterDispose_ThrowsObjectDisposedException_OnComputeSha256() + { + var sha = new ShaHashing(); + sha.Dispose(); + Should.Throw(() => sha.ComputeSha256("input")); + } + + [Fact] + public void ShaHashing_AfterDispose_ThrowsObjectDisposedException_OnComputeSha512() + { + var sha = new ShaHashing(); + sha.Dispose(); + Should.Throw(() => sha.ComputeSha512("input")); + } + + [Fact] + public void ShaHashing_AfterDispose_ThrowsObjectDisposedException_OnVerifySha256() + { + var sha = new ShaHashing(); + sha.Dispose(); + Should.Throw(() => sha.VerifySha256("input", "expected")); + } + + [Fact] + public void ShaHashing_AfterDispose_ThrowsObjectDisposedException_OnVerifySha512() + { + var sha = new ShaHashing(); + sha.Dispose(); + Should.Throw(() => sha.VerifySha512("input", "expected")); + } + + #endregion +} diff --git a/src/Services/Svc.Encryption.Tests/ShaHashingIgnoreCaseTests.cs b/src/Services/Svc.Encryption.Tests/ShaHashingIgnoreCaseTests.cs new file mode 100644 index 00000000..2623b288 --- /dev/null +++ b/src/Services/Svc.Encryption.Tests/ShaHashingIgnoreCaseTests.cs @@ -0,0 +1,46 @@ +using System; +using System.Linq; +using DKNet.Svc.Encryption; +using Shouldly; +using Xunit; + +namespace Svc.Encryption.Tests; + +public class ShaHashingIgnoreCaseTests +{ + #region Methods + + [Fact] + public void ShaHashing_VerifySha256_IgnoreCaseTrueOrFalse_ReturnsSameResult() + { + using var sha = new ShaHashing(); + var message = "test message"; + var hex = sha.ComputeSha256(message); + + var mixedCaseHex = new string([.. hex.Select((c, i) => i % 2 == 0 ? char.ToUpper(c) : char.ToLower(c))]); + + var rTrue = sha.VerifySha256(message, mixedCaseHex, true); + var rFalse = sha.VerifySha256(message, mixedCaseHex, false); + + rTrue.ShouldBe(rFalse); + rTrue.ShouldBeTrue(); + } + + [Fact] + public void ShaHashing_VerifySha512_IgnoreCaseTrueOrFalse_ReturnsSameResult() + { + using var sha = new ShaHashing(); + var message = "test message"; + var hex = sha.ComputeSha512(message); + + var mixedCaseHex = new string([.. hex.Select((c, i) => i % 2 == 0 ? char.ToUpper(c) : char.ToLower(c))]); + + var rTrue = sha.VerifySha512(message, mixedCaseHex, true); + var rFalse = sha.VerifySha512(message, mixedCaseHex, false); + + rTrue.ShouldBe(rFalse); + rTrue.ShouldBeTrue(); + } + + #endregion +} From 080b396f74c747f36fcb4c74730443799c8d4086 Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Wed, 5 Aug 2026 10:59:34 +0800 Subject: [PATCH 06/13] test(blob-storage): cover S3BlobService.Dispose and multi-key/empty folder delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fixture now exposes its S3Options so tests can construct their own S3BlobService instance (needed to exercise Dispose without going through the DI-registered IBlobService). - Adds DisposeReleasesUnderlyingClientAndIsIdempotent — the only gap behind S3BlobService.cs's 52.94% class-level coverage was Dispose()/Dispose(bool) never being invoked. - Adds DeleteAsyncDeletesDirectoryWithMultipleKeys and DeleteAsyncDeletesEmptyDirectoryWithoutThrowing to sanity-check the per-key DeleteFolderAsync rewrite (D135-2 checklist item). S3BlobService.cs line coverage: 52.94% -> 94.83%. Full suite: 120/120 green. Co-authored-by: multica-agent --- .../Fixtures/S3BlobServiceFixture.cs | 24 ++++++++-- .../S3BlobServiceTest.cs | 47 +++++++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/src/Services/Svc.BlobStorage.Tests/Fixtures/S3BlobServiceFixture.cs b/src/Services/Svc.BlobStorage.Tests/Fixtures/S3BlobServiceFixture.cs index 7f801984..b58e00e7 100644 --- a/src/Services/Svc.BlobStorage.Tests/Fixtures/S3BlobServiceFixture.cs +++ b/src/Services/Svc.BlobStorage.Tests/Fixtures/S3BlobServiceFixture.cs @@ -20,14 +20,24 @@ public S3BlobServiceFixture() _minioContainer.StartAsync().GetAwaiter().GetResult(); + Options = new S3Options + { + ConnectionString = _minioContainer.GetConnectionString(), + AccessKey = _minioContainer.GetAccessKey(), + Secret = _minioContainer.GetSecretKey(), + BucketName = "dev", + DisablePayloadSigning = false, + ForcePathStyle = true + }; + var config = new ConfigurationBuilder() .AddInMemoryCollection( new Dictionary(StringComparer.OrdinalIgnoreCase) { - { "BlobService:S3:ConnectionString", _minioContainer.GetConnectionString() }, - { "BlobService:S3:AccessKey", _minioContainer.GetAccessKey() }, - { "BlobService:S3:Secret", _minioContainer.GetSecretKey() }, - { "BlobService:S3:BucketName", "dev" }, + { "BlobService:S3:ConnectionString", Options.ConnectionString }, + { "BlobService:S3:AccessKey", Options.AccessKey }, + { "BlobService:S3:Secret", Options.Secret }, + { "BlobService:S3:BucketName", Options.BucketName }, { "BlobService:S3:DisablePayloadSigning", "false" }, { "BlobService:S3:ForcePathStyle", "true" } }) @@ -47,6 +57,12 @@ public S3BlobServiceFixture() public IBlobService Service { get; } + /// + /// The options this fixture's Minio container was configured with — exposed so tests can construct + /// their own instance directly (e.g. to exercise Dispose()). + /// + public S3Options Options { get; } + #endregion #region Methods diff --git a/src/Services/Svc.BlobStorage.Tests/S3BlobServiceTest.cs b/src/Services/Svc.BlobStorage.Tests/S3BlobServiceTest.cs index a811f1b0..c7adf5c4 100644 --- a/src/Services/Svc.BlobStorage.Tests/S3BlobServiceTest.cs +++ b/src/Services/Svc.BlobStorage.Tests/S3BlobServiceTest.cs @@ -1,4 +1,7 @@ using System.Text; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using DKNet.Svc.BlobStorage.AwsS3; using Svc.BlobStorage.Tests.Fixtures; namespace Svc.BlobStorage.Tests; @@ -168,5 +171,49 @@ public async Task SavesFileAndList() items.Count.ShouldBeGreaterThanOrEqualTo(1); } + [Fact] + public async Task DeleteAsyncDeletesDirectoryWithMultipleKeys() + { + // Regression test for the per-key DeleteFolderAsync rewrite (one DeleteObjectAsync call per + // key instead of a single batch DeleteObjectsAsync) — confirms it drops none of several keys. + var dir = $"delete-dir-multi-{Guid.NewGuid()}"; + for (var i = 0; i < 5; i++) + { + var blob = new BlobDetails.BlobData($"{dir}/file{i}.txt", new BinaryData("bye"u8.ToArray())) + { Overwrite = true, Type = BlobTypes.File }; + await _service.SaveAsync(blob); + } + + var deleted = await _service.DeleteAsync(new BlobRequest(dir) { Type = BlobTypes.Directory }); + deleted.ShouldBeTrue(); + + var items = new List(); + await foreach (var item in _service.ListItemsAsync(new BlobRequest(dir) { Type = BlobTypes.Directory })) + items.Add(item); + + items.ShouldBeEmpty(); + } + + [Fact] + public async Task DeleteAsyncDeletesEmptyDirectoryWithoutThrowing() + { + var dir = $"delete-dir-empty-{Guid.NewGuid()}"; + + var deleted = await _service.DeleteAsync(new BlobRequest(dir) { Type = BlobTypes.Directory }); + deleted.ShouldBeTrue(); + } + + [Fact] + public async Task DisposeReleasesUnderlyingClientAndIsIdempotent() + { + var service = new S3BlobService(Options.Create(fixture.Options), NullLogger.Instance); + + // Force the lazy AmazonS3Client to be created before disposing it. + await service.CheckExistsAsync(new BlobRequest($"dispose-check-{Guid.NewGuid()}.txt")); + + Should.NotThrow(service.Dispose); + Should.NotThrow(service.Dispose); // second call must be a no-op, not re-dispose a null client + } + #endregion } \ No newline at end of file From d529b69750aaba87c9c057cb6a6b83892b0e17d8 Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Wed, 5 Aug 2026 11:01:23 +0800 Subject: [PATCH 07/13] test(idempotency): add coverlet.collector to enable coverage collection Sibling test projects already reference it; this project was missing it, so coverage % couldn't be measured for touched non-SQL-Server classes. Co-authored-by: multica-agent --- .../AspCore.Idempotency.Tests/AspCore.Idempotency.Tests.csproj | 1 + 1 file changed, 1 insertion(+) diff --git a/src/AspNet/AspCore.Idempotency.Tests/AspCore.Idempotency.Tests.csproj b/src/AspNet/AspCore.Idempotency.Tests/AspCore.Idempotency.Tests.csproj index 9e48486d..4255de58 100644 --- a/src/AspNet/AspCore.Idempotency.Tests/AspCore.Idempotency.Tests.csproj +++ b/src/AspNet/AspCore.Idempotency.Tests/AspCore.Idempotency.Tests.csproj @@ -19,6 +19,7 @@ + From ef960f8875b2817791e3defd1d4ec90c711f7d7d Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Wed, 5 Aug 2026 11:58:24 +0800 Subject: [PATCH 08/13] test(idempotency): restore live concurrency check via file-based SQLite CreateItem_ConcurrentRequestsWithSameKey_OnlyOneProcessed (the only test that ever proved the atomic reservation under real concurrency) is permanently [Skip]d since bf50729 retired the SQL Server TestContainers path. Add IdempotencySqlServerStoreConcurrencyTests, exercising IdempotencySqlServerStore.IsKeyProcessedAsync directly against a file-based SQLite IdempotencyDbContext (not InMemory - needs real unique index enforcement across concurrent connections) using the same IdempotencyKeyConfiguration the SQL Server store ships with. Fires 5 concurrent reservation attempts for an identical composite key and asserts exactly one wins - mirroring the retired HTTP-level test, but at the store layer so it needs no Docker/SQL Server. Two Sqlite-provider incompatibilities in the shared configuration needed a test-local workaround (no production code touched): - Body's raw HasColumnType("nvarchar(max)") isn't valid SQLite syntax. - The Sqlite provider can't translate > /< on a DateTimeOffset column (only equality), which IsKeyProcessedAsync's expiry check relies on. Both are patched via a test-only IModelCustomizer that strips the column-type override and stores ExpiresAt as UTC ticks instead. Verified: reverting IsKeyProcessedAsync to the pre-fix check-then-act shape (commit b85e224's parent) makes this test fail (5/5 callers see (false, null) instead of 1/5) - confirming it would have caught DRK-75's original race. Restored, it passes cleanly, 10/10 repeated runs, and dotnet build/format come back clean. Refs DRK-176, DRK-174, DRK-75 Co-Authored-By: Claude Sonnet 5 Co-authored-by: multica-agent --- ...spCore.Idempotency.MsSqlStore.Tests.csproj | 3 + ...empotencySqlServerStoreConcurrencyTests.cs | 121 ++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Unit/IdempotencySqlServerStoreConcurrencyTests.cs diff --git a/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/AspCore.Idempotency.MsSqlStore.Tests.csproj b/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/AspCore.Idempotency.MsSqlStore.Tests.csproj index d99b75ec..6036701c 100644 --- a/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/AspCore.Idempotency.MsSqlStore.Tests.csproj +++ b/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/AspCore.Idempotency.MsSqlStore.Tests.csproj @@ -20,6 +20,9 @@ runtime; build; native; contentfiles; analyzers; buildtransitive + + + diff --git a/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Unit/IdempotencySqlServerStoreConcurrencyTests.cs b/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Unit/IdempotencySqlServerStoreConcurrencyTests.cs new file mode 100644 index 00000000..a2deb5b6 --- /dev/null +++ b/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Unit/IdempotencySqlServerStoreConcurrencyTests.cs @@ -0,0 +1,121 @@ +// +// Copyright (c) 2025 Steven Hoang. All rights reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. +// + +using DKNet.AspCore.Idempotency; +using DKNet.AspCore.Idempotency.MsSqlStore.Store; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; + +namespace AspCore.Idempotency.MsSqlStore.Tests.Unit; + +/// +/// Proves the atomic reservation in under real +/// concurrent connections, without Docker/SQL Server. Points the store at a file-based SQLite database — not +/// the EF Core InMemory provider, which does not enforce UX_CompositeKey across concurrent contexts the +/// same way a real relational database does — using the exact same +/// the SQL Server store ships with. Mirrors the retired HTTP-level +/// CreateItem_ConcurrentRequestsWithSameKey_OnlyOneProcessed test (see DRK-174), but directly at the +/// store layer so it doesn't depend on SQL Server at all. +/// +public sealed class IdempotencySqlServerStoreConcurrencyTests : IAsyncLifetime +{ + #region Fields + + private readonly string _dbFilePath = + Path.Combine(Path.GetTempPath(), $"idempotency-concurrency-{Guid.NewGuid():N}.db"); + + private ServiceProvider _serviceProvider = null!; + private IdempotencySqlServerStore _store = null!; + + #endregion + + #region Methods + + public async Task DisposeAsync() + { + await _store.DisposeAsync(); + await _serviceProvider.DisposeAsync(); + File.Delete(_dbFilePath); + } + + public async Task InitializeAsync() + { + var services = new ServiceCollection(); + services.AddDbContextFactory(o => o.UseSqlite( + $"Data Source={_dbFilePath}", + // The real "Initial" migration is authored for SQL Server, so point migrations at this test + // assembly - which has none - instead. That makes the store's own migration-check path + // (EnsureDatabaseCreatedAsync) see nothing pending and skip straight past it; EnsureCreatedAsync + // below builds the schema from the live model instead. + sqlite => sqlite.MigrationsAssembly( + typeof(IdempotencySqlServerStoreConcurrencyTests).Assembly.GetName().Name)) + // IdempotencyKeyConfiguration hardcodes the Body column's raw type as "nvarchar(max)" (SQL Server's + // way of saying "unbounded"); that literal string isn't valid SQLite syntax. Strip just that one + // provider-specific override so the rest of the same configuration - crucially UX_CompositeKey - + // builds unmodified. + .ReplaceService()); + _serviceProvider = services.BuildServiceProvider(); + + var factory = _serviceProvider.GetRequiredService>(); + await using var setupContext = await factory.CreateDbContextAsync(); + await setupContext.Database.EnsureCreatedAsync(); + + _store = new IdempotencySqlServerStore( + _serviceProvider, + Options.Create(new IdempotencyOptions()), + NullLogger.Instance); + } + + [Fact] + public async Task IsKeyProcessedAsync_ConcurrentRequestsWithSameKey_OnlyOneReservationWins() + { + // Arrange + var keyInfo = new IdempotentKeyInfo + { + Endpoint = "/api/items", + Method = "POST", + IdempotentKey = Guid.NewGuid().ToString() + }; + + // Act - fire 5 concurrent reservation attempts for the identical composite key against the same + // SQLite file, exactly as 5 concurrent HTTP requests would hit the same SQL Server row. + var tasks = Enumerable.Range(0, 5) + .Select(_ => _store.IsKeyProcessedAsync(keyInfo).AsTask()) + .ToArray(); + var results = await Task.WhenAll(tasks); + + // Assert - exactly one caller wins the reservation (false, null); the unique index forces every + // other concurrent caller onto the collision path, which reports back as already-processed/in-flight. + results.Count(r => !r.processed).ShouldBe(1); + results.Count(r => r.processed).ShouldBe(4); + } + + #endregion +} + +/// +/// Applies as-is, then drops the one column-type override in it +/// that is SQL-Server-specific raw SQL, so EnsureCreatedAsync can generate valid SQLite DDL. +/// +internal sealed class SqliteCompatibleModelCustomizer(ModelCustomizerDependencies dependencies) + : ModelCustomizer(dependencies) +{ + public override void Customize(ModelBuilder modelBuilder, DbContext context) + { + base.Customize(modelBuilder, context); + + var key = modelBuilder.Entity(); + key.Property(e => e.Body).HasColumnType(null); + + // The Sqlite provider cannot translate "> "/"<" comparisons on a DateTimeOffset column (only equality) - + // IsKeyProcessedAsync's expiry check relies on exactly that. Store ExpiresAt as UTC ticks instead so the + // comparison becomes an ordinary numeric one; the property's C# type/behaviour is unaffected. + key.Property(e => e.ExpiresAt).HasConversion( + v => v.HasValue ? v.Value.UtcTicks : (long?)null, + v => v.HasValue ? new DateTimeOffset(v.Value, TimeSpan.Zero) : null); + } +} From 4802a1bbfc01b35025e50f131ca3c5186f6647ae Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Wed, 5 Aug 2026 12:03:07 +0800 Subject: [PATCH 09/13] fix(idempotency): close check-then-act race in Postgres store - IsKeyProcessedAsync now atomically reserves the composite key by inserting a StatusCode=102 placeholder row, relying on the existing UX_CompositeKey unique index to serialize concurrent callers (only the winner proceeds to run the protected handler). - On insert collision, the blocking row is re-queried: an unexpired completed row replays its cached response, an unexpired reservation returns the existing 409/conflict path, and an expired row (stale reservation or completed entry nothing purges) lets the request proceed as new instead of permanently blocking that key. - MarkKeyAsProcessedAsync now completes the tracked reservation row in place via the new IdempotencyKeyEntity.Complete(...) instead of blindly inserting a second row. - Add IdempotencyOptions.InFlightReservationTimeout (default 30s) controlling how long a reservation is honoured before being treated as abandoned (R1). - Tighten the concurrency integration test to assert the handler ran exactly once (identical Id across every 201), and add a new test proving an expired in-flight reservation does not permanently block retries. DRK-175 / IDEM-CONCURRENCY-001 Co-authored-by: multica-agent --- .../IdempotencyIntegrationTests.cs | 75 +++++++++++--- .../Data/IdempotencyKeyEntity.cs | 15 +++ .../Store/IdempotencyPostgresStore.cs | 99 ++++++++++++++++--- .../IdempotencyOptions.cs | 10 ++ 4 files changed, 169 insertions(+), 30 deletions(-) diff --git a/src/AspNet/AspCore.Idempotency.NpgsqlStore.Tests/Integration/IdempotencyIntegrationTests.cs b/src/AspNet/AspCore.Idempotency.NpgsqlStore.Tests/Integration/IdempotencyIntegrationTests.cs index 2b71931b..b5785adf 100644 --- a/src/AspNet/AspCore.Idempotency.NpgsqlStore.Tests/Integration/IdempotencyIntegrationTests.cs +++ b/src/AspNet/AspCore.Idempotency.NpgsqlStore.Tests/Integration/IdempotencyIntegrationTests.cs @@ -54,9 +54,8 @@ public async Task CreateItem_ConcurrentRequestsWithSameKey_OnlyOneProcessed() var idempotencyKey = Guid.NewGuid().ToString(); var request = new CreateItemRequest { Name = "Concurrent Item" }; - // Act - Send 5 concurrent requests with the same idempotency key - // multiple requests may process simultaneously when they all arrive before any is cached. - // This is a known limitation of stateless filters without distributed locking. + // Act - Send 5 concurrent requests with the same idempotency key. + // The store's atomic reservation ensures at most one of them ever reaches the handler. var tasks = Enumerable.Range(0, 5).Select(_ => { var httpRequest = new HttpRequestMessage(HttpMethod.Post, "/api/items") @@ -69,19 +68,20 @@ public async Task CreateItem_ConcurrentRequestsWithSameKey_OnlyOneProcessed() var responses = await Task.WhenAll(tasks); - // Assert - With concurrent requests, some or all may succeed (201 or 409 Conflict) - // The first request wins, subsequent ones get 409 Conflict (if ConflictHandling = ConflictResponse) - // or the cached response (if ConflictHandling = CachedResult) - var successCount = responses.Count(r => r.StatusCode == HttpStatusCode.Created); - var conflictCount = responses.Count(r => r.StatusCode == HttpStatusCode.Conflict); - - // At least one should succeed (201) - successCount.ShouldBeGreaterThanOrEqualTo(1); + // Assert - the fixture uses ConflictHandling = CachedResult, so a duplicate arriving after the + // winner completes legitimately gets 201 with the replayed body, not 409; we don't assert an + // exact 201/409 split. Instead: the handler generates a fresh Guid per call, so every 201 + // response carrying the SAME Id proves the handler ran exactly once. + var createdIds = new List(); + foreach (var response in responses) + { + if (response.StatusCode != HttpStatusCode.Created) continue; + var item = await response.Content.ReadFromJsonAsync(); + createdIds.Add(item!.Id); + } - // The rest should be conflicts or cached responses - var otherCount = responses.Count(r => r.StatusCode != HttpStatusCode.Created && - r.StatusCode != HttpStatusCode.Conflict); - (successCount + conflictCount + otherCount).ShouldBe(5); + createdIds.ShouldNotBeEmpty("At least one request should have succeeded"); + createdIds.Distinct().Count().ShouldBe(1, "The handler must have executed exactly once"); // Verify only ONE entry in database (unique constraint ensures this) await using var dbContext = fixture.GetDbContext(); @@ -326,6 +326,51 @@ public async Task CreateItem_WithExpiredIdempotencyKey_ProcessesAsNewRequest() item.Id.ShouldNotBe(Guid.Empty); } + [Fact] + public async Task CreateItem_WithExpiredInFlightReservation_ProcessesAsNewRequest() + { + // Arrange - seed an already-expired StatusCode=102 reservation placeholder directly, simulating + // a prior request whose handler never completed (crashed, timed out) within the reservation window. + var idempotencyKey = Guid.NewGuid().ToString(); + var request = new CreateItemRequest { Name = "Expired Reservation Item" }; + + await using (var seedDbContext = fixture.GetDbContext()) + { + var expiredReservation = new IdempotencyKeyEntity( + new IdempotentKeyInfo + { + IdempotentKey = idempotencyKey, + Endpoint = "/api/items", + Method = "POST" + }, + new CachedResponse + { + StatusCode = 102, + Body = null, + ContentType = "application/json", + CreatedAt = DateTimeOffset.UtcNow.AddMinutes(-2), + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(-1) + }); + + seedDbContext.IdempotencyKeys.Add(expiredReservation); + await seedDbContext.SaveChangesAsync(); + } + + // Act - a request with the same key must not be permanently blocked by the stale reservation + var httpRequest = new HttpRequestMessage(HttpMethod.Post, "/api/items") + { + Headers = { { "X-Idempotency-Key", idempotencyKey } }, + Content = JsonContent.Create(request) + }; + var response = await fixture.HttpClient!.SendAsync(httpRequest); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.Created); + var item = await response.Content.ReadFromJsonAsync(); + item!.Name.ShouldBe("Expired Reservation Item"); + item.Id.ShouldNotBe(Guid.Empty); + } + public Task DisposeAsync() => Task.CompletedTask; public Task InitializeAsync() => Task.CompletedTask; diff --git a/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Data/IdempotencyKeyEntity.cs b/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Data/IdempotencyKeyEntity.cs index 5dc508cc..a96d47fc 100644 --- a/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Data/IdempotencyKeyEntity.cs +++ b/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Data/IdempotencyKeyEntity.cs @@ -101,6 +101,21 @@ private IdempotencyKeyEntity() #region Methods + /// + /// Completes an in-flight reservation placeholder in place, overwriting it with the actual + /// processed response. Used when this row was inserted as a StatusCode == 102 reservation + /// and the protected handler has now finished, so the row transitions from placeholder to + /// completed cache entry without a second insert. + /// + /// The cached response to store. + internal void Complete(CachedResponse response) + { + StatusCode = response.StatusCode; + Body = response.Body; + ContentType = response.ContentType; + ExpiresAt = response.ExpiresAt; + } + /// /// Sanitizes an idempotency key for use as a database key by hashing it. /// Hashing (rather than stripping characters) guarantees structurally distinct diff --git a/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Store/IdempotencyPostgresStore.cs b/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Store/IdempotencyPostgresStore.cs index e45a1665..8803d473 100644 --- a/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Store/IdempotencyPostgresStore.cs +++ b/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Store/IdempotencyPostgresStore.cs @@ -8,6 +8,7 @@ using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using Npgsql; namespace DKNet.AspCore.Idempotency.NpgsqlStore.Store; @@ -18,7 +19,8 @@ namespace DKNet.AspCore.Idempotency.NpgsqlStore.Store; /// internal sealed class IdempotencyPostgresStore( IServiceProvider serviceProvider, - ILogger logger) : IIdempotencyKeyStore, IAsyncDisposable + ILogger logger, + IOptions options) : IIdempotencyKeyStore, IAsyncDisposable { #region Fields @@ -84,13 +86,13 @@ private static bool IsUniqueViolation(DbUpdateException ex) => if (existing == null) { logger.LogDebug("Idempotency key not found or expired: {Key}", sanitizedKey); - return (false, null); + return await ReserveKeyAsync(dbContext, keyInfo, sanitizedKey).ConfigureAwait(false); } if (existing.IsExpired) { logger.LogDebug("Idempotency key has expired: {Key}", sanitizedKey); - return (false, null); + return await ReserveKeyAsync(dbContext, keyInfo, sanitizedKey).ConfigureAwait(false); } logger.LogInformation( @@ -98,18 +100,76 @@ private static bool IsUniqueViolation(DbUpdateException ex) => existing.StatusCode, sanitizedKey); - var cachedResponse = new CachedResponse + return (true, ToCachedResponse(existing)); + } + + /// + /// Attempts to atomically reserve by inserting a + /// StatusCode == 102 placeholder row, relying on the UX_CompositeKey unique index to + /// serialize concurrent callers — only the caller whose insert succeeds proceeds to run the + /// protected handler. + /// + /// The open database context to reserve the key on. + /// The idempotency key information for the reservation row. + /// The pre-computed, sanitized composite key. + /// + /// (false, null) if this call reserved the key and the caller should proceed with + /// processing; otherwise (true, response) with the completed duplicate's cached response, + /// or (true, null) if another caller currently holds an unexpired in-flight reservation. + /// + private async ValueTask<(bool processed, CachedResponse? response)> ReserveKeyAsync( + IdempotencyDbContext dbContext, IdempotentKeyInfo keyInfo, string sanitizedKey) + { + try { - StatusCode = existing.StatusCode, - Body = existing.Body, - ContentType = existing.ContentType ?? "application/json", - CreatedAt = existing.CreatedAt, - ExpiresAt = existing.ExpiresAt - }; + var reservation = new IdempotencyKeyEntity(keyInfo, new CachedResponse + { + StatusCode = 102, + Body = null, + ContentType = "application/json", + CreatedAt = DateTimeOffset.UtcNow, + ExpiresAt = DateTimeOffset.UtcNow + options.Value.InFlightReservationTimeout + }); + + dbContext.IdempotencyKeys.Add(reservation); + await dbContext.SaveChangesAsync().ConfigureAwait(false); - return (true, cachedResponse); + return (false, null); + } + catch (DbUpdateException ex) when (IsUniqueViolation(ex)) + { + // A concurrent request already holds this composite key - find out what state it's in. + var blocking = await dbContext.IdempotencyKeys + .AsNoTracking() + .FirstOrDefaultAsync(k => k.CompositeKey == sanitizedKey) + .ConfigureAwait(false); + + if (blocking is { IsExpired: false }) + { + logger.LogInformation( + "Idempotency key already reserved or processed by a concurrent request: {Key}", + sanitizedKey); + return (true, blocking.StatusCode == 102 ? null : ToCachedResponse(blocking)); + } + + // The row blocking our insert is itself expired (a stale reservation or completed entry + // nothing ever purged). Treat this request as not-processed and let it run the handler; + // MarkKeyAsProcessedAsync reclaims that row in place once processing completes. + logger.LogDebug("Blocking idempotency key row has expired, proceeding as new: {Key}", sanitizedKey); + return (false, null); + } } + private static CachedResponse ToCachedResponse(IdempotencyKeyEntity entity) => + new() + { + StatusCode = entity.StatusCode, + Body = entity.Body, + ContentType = entity.ContentType ?? "application/json", + CreatedAt = entity.CreatedAt, + ExpiresAt = entity.ExpiresAt + }; + /// public async ValueTask MarkKeyAsProcessedAsync(IdempotentKeyInfo keyInfo, CachedResponse cachedResponse) { @@ -125,11 +185,19 @@ public async ValueTask MarkKeyAsProcessedAsync(IdempotentKeyInfo keyInfo, Cached var factory = _scope.ServiceProvider.GetRequiredService>(); await using var dbContext = await factory.CreateDbContextAsync(); - - var entity = new IdempotencyKeyEntity(keyInfo, cachedResponse); await EnsureDatabaseCreatedAsync(dbContext); - dbContext.IdempotencyKeys.Add(entity); + var reservation = await dbContext.IdempotencyKeys + .FirstOrDefaultAsync(k => k.CompositeKey == sanitizedKey) + .ConfigureAwait(false); + + if (reservation != null) + reservation.Complete(cachedResponse); + else + // Defensive fallback only - should not happen now that IsKeyProcessedAsync always + // reserves the row before the handler runs. + dbContext.IdempotencyKeys.Add(new IdempotencyKeyEntity(keyInfo, cachedResponse)); + await dbContext.SaveChangesAsync().ConfigureAwait(false); logger.LogInformation( @@ -139,7 +207,8 @@ public async ValueTask MarkKeyAsProcessedAsync(IdempotentKeyInfo keyInfo, Cached } catch (DbUpdateException ex) when (IsUniqueViolation(ex)) { - // Handle race condition: Another concurrent request already inserted this key + // Handle race condition: Another concurrent request already inserted this key. + // Unreachable in the common path now, kept as a defensive guard around the fallback Add() above. logger.LogInformation( "Idempotency key already processed by concurrent request: {Key}. Continuing without duplicate insert.", sanitizedKey); diff --git a/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs b/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs index 7b97188b..52c56563 100644 --- a/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs +++ b/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs @@ -70,6 +70,16 @@ public sealed class IdempotencyOptions /// public string IdempotencyKeyPattern { get; set; } = @"^[a-zA-Z0-9\-_]+$"; + /// + /// Gets or sets how long an in-flight reservation placeholder is honoured before being treated as + /// expired and abandoned. A store that makes its check-and-reserve step atomic inserts such a + /// placeholder while the protected handler is running; once this timeout elapses without the + /// reservation being completed (e.g. the handler crashed), a fresh request for the same key is + /// allowed to proceed instead of being permanently blocked. + /// Default is 30 seconds. + /// + public TimeSpan InFlightReservationTimeout { get; set; } = TimeSpan.FromSeconds(30); + /// /// Gets or sets the JSON serializer options used to serialize response bodies before caching them. /// This is used when the conflict handling strategy is set to return cached results. From cf8e2935e489c7324506838e1e120d26c0e766b0 Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Wed, 5 Aug 2026 12:08:23 +0800 Subject: [PATCH 10/13] test(idempotency): prove residual race when reservation collides with an expired row Adds CreateItem_ConcurrentRequestsAgainstExpiredReservation_OnlyOneProcessed. Seeds an already-expired StatusCode=102 reservation, then fires 5 concurrent requests for the same key. All 5 requests miss the unexpired-row filter and collide on the same stale row's unique-index INSERT; ReserveKeyAsync's collision branch returns (false, null) for every one of them when the blocking row is expired, so all 5 proceed to run the handler. FAILS on 4802a1b: 5 distinct handler executions observed instead of 1. Co-Authored-By: Claude Sonnet 5 Co-authored-by: multica-agent --- .../IdempotencyIntegrationTests.cs | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/src/AspNet/AspCore.Idempotency.NpgsqlStore.Tests/Integration/IdempotencyIntegrationTests.cs b/src/AspNet/AspCore.Idempotency.NpgsqlStore.Tests/Integration/IdempotencyIntegrationTests.cs index b5785adf..86f30ae8 100644 --- a/src/AspNet/AspCore.Idempotency.NpgsqlStore.Tests/Integration/IdempotencyIntegrationTests.cs +++ b/src/AspNet/AspCore.Idempotency.NpgsqlStore.Tests/Integration/IdempotencyIntegrationTests.cs @@ -371,6 +371,64 @@ public async Task CreateItem_WithExpiredInFlightReservation_ProcessesAsNewReques item.Id.ShouldNotBe(Guid.Empty); } + [Fact] + public async Task CreateItem_ConcurrentRequestsAgainstExpiredReservation_OnlyOneProcessed() + { + // Arrange - seed an already-expired StatusCode=102 reservation so every concurrent request's + // initial unexpired-row query misses it, and every request's own reservation INSERT collides + // with this same stale row (unique index doesn't care that it's expired). + var idempotencyKey = Guid.NewGuid().ToString(); + var request = new CreateItemRequest { Name = "Expired Reservation Race Item" }; + + await using (var seedDbContext = fixture.GetDbContext()) + { + var expiredReservation = new IdempotencyKeyEntity( + new IdempotentKeyInfo + { + IdempotentKey = idempotencyKey, + Endpoint = "/api/items", + Method = "POST" + }, + new CachedResponse + { + StatusCode = 102, + Body = null, + ContentType = "application/json", + CreatedAt = DateTimeOffset.UtcNow.AddMinutes(-2), + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(-1) + }); + + seedDbContext.IdempotencyKeys.Add(expiredReservation); + await seedDbContext.SaveChangesAsync(); + } + + // Act - fire 5 concurrent requests against the same key, all racing the stale expired row + var tasks = Enumerable.Range(0, 5).Select(_ => + { + var httpRequest = new HttpRequestMessage(HttpMethod.Post, "/api/items") + { + Headers = { { "X-Idempotency-Key", idempotencyKey } }, + Content = JsonContent.Create(request) + }; + return fixture.HttpClient!.SendAsync(httpRequest); + }).ToArray(); + + var responses = await Task.WhenAll(tasks); + + // Assert - only one distinct handler execution must be observable, same as the fresh-key case + var createdIds = new List(); + foreach (var response in responses) + { + if (response.StatusCode != HttpStatusCode.Created) continue; + var item = await response.Content.ReadFromJsonAsync(); + createdIds.Add(item!.Id); + } + + createdIds.ShouldNotBeEmpty("At least one request should have succeeded"); + createdIds.Distinct().Count().ShouldBe(1, + "The handler must have executed exactly once, even when racing an already-expired reservation row"); + } + public Task DisposeAsync() => Task.CompletedTask; public Task InitializeAsync() => Task.CompletedTask; From f7438f399ca020c749f7cb1b950ece59d40b14c1 Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Wed, 5 Aug 2026 12:13:10 +0800 Subject: [PATCH 11/13] fix(idempotency): atomically reclaim expired reservation rows to close residual race ReserveKeyAsync's expired-collision branch returned (false, null) to every concurrent caller that collided with the same expired row, so none of them actually won the reservation - reopening the exact double-execution race DRK-175 closed for the fresh-key path. Replace the unconditional return with a conditional UPDATE (ExecuteUpdateAsync) that only matches while the row is still expired, giving the same single-winner guarantee the unique index gives the fresh-insert path. A caller whose UPDATE affects zero rows lost the race and re-reads the row to branch like the unexpired collision path. Fixes DRK-182. Co-authored-by: multica-agent --- .../Store/IdempotencyPostgresStore.cs | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Store/IdempotencyPostgresStore.cs b/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Store/IdempotencyPostgresStore.cs index 8803d473..df530370 100644 --- a/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Store/IdempotencyPostgresStore.cs +++ b/src/AspNet/DKNet.AspCore.Idempotency.NpgsqlStore/Store/IdempotencyPostgresStore.cs @@ -153,10 +153,40 @@ private static bool IsUniqueViolation(DbUpdateException ex) => } // The row blocking our insert is itself expired (a stale reservation or completed entry - // nothing ever purged). Treat this request as not-processed and let it run the handler; - // MarkKeyAsProcessedAsync reclaims that row in place once processing completes. - logger.LogDebug("Blocking idempotency key row has expired, proceeding as new: {Key}", sanitizedKey); - return (false, null); + // nothing ever purged). Reclaiming it with a plain read-then-write would reopen the same + // race this method exists to close, so reclaim it atomically: a conditional UPDATE that + // only matches while the row is still expired. Its affected-row count gives the same + // single-winner guarantee the unique index gives the fresh-insert path - only the caller + // whose UPDATE actually flips the row wins; every other concurrent racer's UPDATE affects + // zero rows once the winner has moved ExpiresAt into the future. + var now = DateTime.UtcNow; + var reclaimed = await dbContext.IdempotencyKeys + .Where(k => k.CompositeKey == sanitizedKey && k.ExpiresAt != null && k.ExpiresAt <= now) + .ExecuteUpdateAsync(s => s + .SetProperty(k => k.StatusCode, 102) + .SetProperty(k => k.Body, (string?)null) + .SetProperty(k => k.ContentType, "application/json") + .SetProperty(k => k.CreatedAt, now) + .SetProperty(k => k.ExpiresAt, now + options.Value.InFlightReservationTimeout)) + .ConfigureAwait(false); + + if (reclaimed == 1) + { + logger.LogDebug("Reclaimed expired idempotency key row, proceeding as new: {Key}", sanitizedKey); + return (false, null); + } + + // Another caller reclaimed (or completed) the row between our insert collision and this + // reclaim attempt - re-read its current state and branch exactly like the unexpired path. + var current = await dbContext.IdempotencyKeys + .AsNoTracking() + .FirstOrDefaultAsync(k => k.CompositeKey == sanitizedKey) + .ConfigureAwait(false); + + logger.LogInformation( + "Idempotency key already reserved or processed by a concurrent request: {Key}", + sanitizedKey); + return (true, current is null || current.StatusCode == 102 ? null : ToCachedResponse(current)); } } From 7face42ab97ffae9df3d2147e3f4e2c24f03b7ae Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Wed, 5 Aug 2026 12:50:01 +0800 Subject: [PATCH 12/13] =?UTF-8?q?test(idempotency):=20cover=20reservation?= =?UTF-8?q?=20lifecycle=20and=20collision=20paths=20for=20=E2=89=A590%=20d?= =?UTF-8?q?iff=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SQL store: full lifecycle (reserve→complete→replay), in-flight recheck, collision re-query returning a completed response, expired-reservation collision allowing a fresh reservation, and the defensive entity-is-null fallback in MarkKeyAsProcessedAsync - Distributed cache store: reservation placeholder write/in-flight recheck, reservation→complete replay, and InFlightReservationTimeout expiry - IdempotencyOptions: default and custom InFlightReservationTimeout Addresses DRK-183 (PR #340 review, coverage gap 53.65% -> local run shows IdempotencySqlServerStore.cs and IdempotencyDistributedCacheStore.cs fully exercised) Co-authored-by: multica-agent --- ...IdempotencySqlServerStoreLifecycleTests.cs | 203 ++++++++++++++++++ ...potencyDistributedCacheReservationTests.cs | 115 ++++++++++ .../Unit/IdempotencyOptionsTests.cs | 14 ++ 3 files changed, 332 insertions(+) create mode 100644 src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Unit/IdempotencySqlServerStoreLifecycleTests.cs create mode 100644 src/AspNet/AspCore.Idempotency.Tests/Unit/IdempotencyDistributedCacheReservationTests.cs diff --git a/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Unit/IdempotencySqlServerStoreLifecycleTests.cs b/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Unit/IdempotencySqlServerStoreLifecycleTests.cs new file mode 100644 index 00000000..cf84b5c1 --- /dev/null +++ b/src/AspNet/AspCore.Idempotency.MsSqlStore.Tests/Unit/IdempotencySqlServerStoreLifecycleTests.cs @@ -0,0 +1,203 @@ +// +// Copyright (c) 2025 Steven Hoang. All rights reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. +// + +using DKNet.AspCore.Idempotency; +using DKNet.AspCore.Idempotency.MsSqlStore.Store; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; + +namespace AspCore.Idempotency.MsSqlStore.Tests.Unit; + +/// +/// Covers the reserve/complete lifecycle and the collision-handler branches of +/// that +/// doesn't exercise. Uses the same file-based SQLite setup so it runs without Docker/SQL Server. +/// +public sealed class IdempotencySqlServerStoreLifecycleTests : IAsyncLifetime +{ + #region Fields + + private readonly string _dbFilePath = + Path.Combine(Path.GetTempPath(), $"idempotency-lifecycle-{Guid.NewGuid():N}.db"); + + private ServiceProvider _serviceProvider = null!; + private IdempotencySqlServerStore _store = null!; + + #endregion + + #region Methods + + public async Task DisposeAsync() + { + await _store.DisposeAsync(); + await _serviceProvider.DisposeAsync(); + File.Delete(_dbFilePath); + } + + public async Task InitializeAsync() + { + var services = new ServiceCollection(); + services.AddDbContextFactory(o => o.UseSqlite( + $"Data Source={_dbFilePath}", + sqlite => sqlite.MigrationsAssembly( + typeof(IdempotencySqlServerStoreConcurrencyTests).Assembly.GetName().Name)) + .ReplaceService()); + _serviceProvider = services.BuildServiceProvider(); + + var factory = _serviceProvider.GetRequiredService>(); + await using var setupContext = await factory.CreateDbContextAsync(); + await setupContext.Database.EnsureCreatedAsync(); + + _store = CreateStore(new IdempotencyOptions()); + } + + private IdempotencySqlServerStore CreateStore(IdempotencyOptions options) => + new(_serviceProvider, Options.Create(options), NullLogger.Instance); + + private static CachedResponse CreateResponse(int statusCode, string? body, DateTimeOffset? expiresAt) => + new() + { + StatusCode = statusCode, + Body = body, + ContentType = "application/json", + CreatedAt = DateTimeOffset.UtcNow, + ExpiresAt = expiresAt + }; + + [Fact] + public async Task IsKeyProcessedAsync_FullLifecycle_ReserveThenCompleteReturnsCachedResponse() + { + // Arrange + var keyInfo = new IdempotentKeyInfo + { + Endpoint = "/api/orders", + Method = "POST", + IdempotentKey = Guid.NewGuid().ToString() + }; + + // Act - first call finds nothing and reserves the key + var reserved = await _store.IsKeyProcessedAsync(keyInfo); + + var response = CreateResponse(201, "{\"id\":1}", DateTimeOffset.UtcNow.AddHours(1)); + await _store.MarkKeyAsProcessedAsync(keyInfo, response); + + var completed = await _store.IsKeyProcessedAsync(keyInfo); + + // Assert - reservation reports not-yet-processed, then the completed row replays the cached response + reserved.processed.ShouldBeFalse(); + reserved.response.ShouldBeNull(); + + completed.processed.ShouldBeTrue(); + completed.response.ShouldNotBeNull(); + completed.response!.StatusCode.ShouldBe(201); + completed.response.Body.ShouldBe("{\"id\":1}"); + } + + [Fact] + public async Task IsKeyProcessedAsync_WhileReservationInFlight_ReturnsTrueWithNullResponse() + { + // Arrange + var keyInfo = new IdempotentKeyInfo + { + Endpoint = "/api/orders", + Method = "POST", + IdempotentKey = Guid.NewGuid().ToString() + }; + + // Act - reserve, then re-check before anyone completes it + await _store.IsKeyProcessedAsync(keyInfo); + var inFlight = await _store.IsKeyProcessedAsync(keyInfo); + + // Assert - the caller is told the key is already being processed (409 path), with no cached body yet + inFlight.processed.ShouldBeTrue(); + inFlight.response.ShouldBeNull(); + } + + [Fact] + public async Task IsKeyProcessedAsync_CollisionOnInsert_ReQueryReturnsCompletedResponse() + { + // Arrange - seed a completed row directly. ExpiresAt is left null so the initial + // "ExpiresAt > now" lookup in IsKeyProcessedAsync misses it (null compares false), forcing the + // store down the reserve-insert path, which then collides with this row's unique CompositeKey. + var keyInfo = new IdempotentKeyInfo + { + Endpoint = "/api/orders", + Method = "POST", + IdempotentKey = Guid.NewGuid().ToString() + }; + var response = CreateResponse(200, "{\"id\":42}", null); + + var factory = _serviceProvider.GetRequiredService>(); + await using (var seedContext = await factory.CreateDbContextAsync()) + { + seedContext.IdempotencyKeys.Add(new IdempotencyKeyEntity(keyInfo, response)); + await seedContext.SaveChangesAsync(); + } + + // Act + var result = await _store.IsKeyProcessedAsync(keyInfo); + + // Assert - the collision handler re-queries and returns the already-completed response + result.processed.ShouldBeTrue(); + result.response.ShouldNotBeNull(); + result.response!.StatusCode.ShouldBe(200); + result.response.Body.ShouldBe("{\"id\":42}"); + } + + [Fact] + public async Task IsKeyProcessedAsync_ExpiredReservationCollision_ReturnsFalseForFreshReservation() + { + // Arrange - a very short InFlightReservationTimeout so the reservation row is expired by the + // time the second call collides with it, without actually waiting out the 30s default. + var shortTimeoutStore = CreateStore(new IdempotencyOptions + { + InFlightReservationTimeout = TimeSpan.FromMilliseconds(1) + }); + var keyInfo = new IdempotentKeyInfo + { + Endpoint = "/api/orders", + Method = "POST", + IdempotentKey = Guid.NewGuid().ToString() + }; + + // Act + await shortTimeoutStore.IsKeyProcessedAsync(keyInfo); // reserves, expiring almost immediately + await Task.Delay(50); + var result = await shortTimeoutStore.IsKeyProcessedAsync(keyInfo); // collides, re-query finds it expired + + // Assert - Rule R1: an abandoned/expired reservation must not permanently block retries + result.processed.ShouldBeFalse(); + result.response.ShouldBeNull(); + + await shortTimeoutStore.DisposeAsync(); + } + + [Fact] + public async Task MarkKeyAsProcessedAsync_WithoutPriorReservation_CreatesEntityDefensively() + { + // Arrange - call MarkKeyAsProcessedAsync directly, skipping IsKeyProcessedAsync's reservation. + // Exercises the defensive "entity is null" branch that should not happen in the normal flow. + var keyInfo = new IdempotentKeyInfo + { + Endpoint = "/api/orders", + Method = "POST", + IdempotentKey = Guid.NewGuid().ToString() + }; + var response = CreateResponse(204, null, DateTimeOffset.UtcNow.AddHours(1)); + + // Act + await _store.MarkKeyAsProcessedAsync(keyInfo, response); + var result = await _store.IsKeyProcessedAsync(keyInfo); + + // Assert + result.processed.ShouldBeTrue(); + result.response.ShouldNotBeNull(); + result.response!.StatusCode.ShouldBe(204); + } + + #endregion +} diff --git a/src/AspNet/AspCore.Idempotency.Tests/Unit/IdempotencyDistributedCacheReservationTests.cs b/src/AspNet/AspCore.Idempotency.Tests/Unit/IdempotencyDistributedCacheReservationTests.cs new file mode 100644 index 00000000..34e9f1b1 --- /dev/null +++ b/src/AspNet/AspCore.Idempotency.Tests/Unit/IdempotencyDistributedCacheReservationTests.cs @@ -0,0 +1,115 @@ +// +// Copyright (c) 2025 Steven Hoang. All rights reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. +// + +using DKNet.AspCore.Idempotency; +using DKNet.AspCore.Idempotency.Store; +using Microsoft.Extensions.Caching.Distributed; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Logging; + +namespace AspCore.Idempotency.Tests.Unit; + +/// +/// Covers the in-flight reservation placeholder introduced into +/// , which the pre-existing +/// mark-then-check tests never exercise because they always mark a key as processed before checking it. +/// +public sealed class IdempotencyDistributedCacheReservationTests +{ + #region Fields + + private readonly IDistributedCache _cache; + private readonly ILogger _logger; + + #endregion + + #region Constructors + + public IdempotencyDistributedCacheReservationTests() + { + _cache = new MemoryDistributedCache(Options.Create(new MemoryDistributedCacheOptions())); + _logger = LoggerFactory.Create(b => b.AddConsole()).CreateLogger(); + } + + #endregion + + #region Methods + + private IdempotencyDistributedCacheStore CreateStore(IdempotencyOptions? options = null) => + new(_cache, Options.Create(options ?? new IdempotencyOptions()), _logger); + + private static IdempotentKeyInfo MakeKey(string key) => + new() { IdempotentKey = key, Endpoint = "/api/test", Method = "POST" }; + + [Fact] + public async Task IsKeyProcessedAsync_FreshKey_WritesReservationSeenAsInFlightByNextCall() + { + // Arrange + var store = CreateStore(); + var keyInfo = MakeKey(Guid.NewGuid().ToString()); + + // Act - first call is a genuine miss and must write the in-flight placeholder itself + var first = await store.IsKeyProcessedAsync(keyInfo); + var second = await store.IsKeyProcessedAsync(keyInfo); + + // Assert - the miss reports "not processed yet"; the placeholder it wrote makes the very next + // call for the same key see an in-flight reservation (409 path) rather than another miss. + first.processed.ShouldBeFalse(); + first.response.ShouldBeNull(); + + second.processed.ShouldBeTrue(); + second.response.ShouldBeNull(); + } + + [Fact] + public async Task IsKeyProcessedAsync_AfterReservationCompletes_ReturnsCachedResponse() + { + // Arrange + var store = CreateStore(); + var keyInfo = MakeKey(Guid.NewGuid().ToString()); + var response = new CachedResponse + { + StatusCode = 201, + Body = "{\"id\":1}", + ContentType = "application/json", + CreatedAt = DateTimeOffset.UtcNow, + ExpiresAt = DateTimeOffset.UtcNow.AddHours(1) + }; + + // Act - reserve, complete, then replay + await store.IsKeyProcessedAsync(keyInfo); + await store.MarkKeyAsProcessedAsync(keyInfo, response); + var completed = await store.IsKeyProcessedAsync(keyInfo); + + // Assert - the completed response overwrites the reservation placeholder + completed.processed.ShouldBeTrue(); + completed.response.ShouldNotBeNull(); + completed.response!.StatusCode.ShouldBe(201); + completed.response.Body.ShouldBe("{\"id\":1}"); + } + + [Fact] + public async Task IsKeyProcessedAsync_AfterReservationTimeoutElapses_TreatsKeyAsFreshMiss() + { + // Arrange - a very short reservation timeout so the placeholder expires almost immediately, + // exercising InFlightReservationTimeout's actual expiry behavior rather than just its default value. + var store = CreateStore(new IdempotencyOptions + { + InFlightReservationTimeout = TimeSpan.FromMilliseconds(1) + }); + var keyInfo = MakeKey(Guid.NewGuid().ToString()); + + // Act + await store.IsKeyProcessedAsync(keyInfo); // reserves, expiring almost immediately + await Task.Delay(50); + var result = await store.IsKeyProcessedAsync(keyInfo); + + // Assert - an abandoned reservation must not permanently block retries + result.processed.ShouldBeFalse(); + result.response.ShouldBeNull(); + } + + #endregion +} diff --git a/src/AspNet/AspCore.Idempotency.Tests/Unit/IdempotencyOptionsTests.cs b/src/AspNet/AspCore.Idempotency.Tests/Unit/IdempotencyOptionsTests.cs index 5ef003ca..1c95215a 100644 --- a/src/AspNet/AspCore.Idempotency.Tests/Unit/IdempotencyOptionsTests.cs +++ b/src/AspNet/AspCore.Idempotency.Tests/Unit/IdempotencyOptionsTests.cs @@ -82,6 +82,20 @@ public void IdempotencyOptions_HasDefaultValues() options.ConflictHandling.ShouldBe(IdempotentConflictHandling.ConflictResponse); options.JsonSerializerOptions.ShouldNotBeNull(); options.JsonSerializerOptions.PropertyNamingPolicy.ShouldBe(JsonNamingPolicy.CamelCase); + options.InFlightReservationTimeout.ShouldBe(TimeSpan.FromSeconds(30)); + } + + [Fact] + public void IdempotencyOptions_CanSetCustomInFlightReservationTimeout() + { + // Arrange + var customTimeout = TimeSpan.FromSeconds(5); + + // Act + var options = new IdempotencyOptions { InFlightReservationTimeout = customTimeout }; + + // Assert + options.InFlightReservationTimeout.ShouldBe(customTimeout); } [Fact] From d318a58ec6b7de883f1a25e0029d27a9f2336d6d Mon Sep 17 00:00:00 2001 From: Steven Hoang Date: Wed, 5 Aug 2026 13:20:23 +0800 Subject: [PATCH 13/13] fix: resolve duplicate InFlightReservationTimeout after merging dev Merging origin/dev (which now carries DRK-175's independently-added identical property) produced no textual conflict but a duplicate declaration (CS0102) since both branches added the same TimeSpan property with the same default. Kept dev's canonical declaration, removed this branch's earlier duplicate. Co-authored-by: multica-agent --- .../DKNet.AspCore.Idempotency/IdempotencyOptions.cs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs b/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs index d288dd0e..52c56563 100644 --- a/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs +++ b/src/AspNet/DKNet.AspCore.Idempotency/IdempotencyOptions.cs @@ -63,15 +63,6 @@ public sealed class IdempotencyOptions /// public string IdempotencyHeaderKey { get; set; } = "X-Idempotency-Key"; - /// - /// Gets or sets how long an in-flight reservation placeholder is honoured before it is treated as - /// expired/abandoned (e.g. the original handler crashed and never completed the request). - /// Deliberately short compared to so a genuinely failed request doesn't - /// permanently block retries. - /// Default is 30 seconds. - /// - public TimeSpan InFlightReservationTimeout { get; set; } = TimeSpan.FromSeconds(30); - /// /// Gets or sets a regular expression pattern used to validate idempotency key format. /// Keys that don't match this pattern will be rejected with a 400 Bad Request.