From ad1db948908d9be544240c83767288eb0368d123 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 11:41:00 +0000 Subject: [PATCH 1/2] fix(auditlogs): load audit settings sequentially to avoid DbContext race (#232) AuditMiddleware fanned out five GetSettingAsync calls via Task.WhenAll on every request. ISettingsContracts is scoped over a single SettingsDbContext, so on a cold FusionCache entry the concurrent queries raced the context ("A second operation was started on this context instance"), which the global exception handler turned into intermittent HTTP 500s on the first hit after startup. Await the five lookups sequentially instead. Adds a regression test that fails if the middleware ever invokes the scoped settings service concurrently. --- .../Middleware/AuditMiddleware.cs | 56 +++++++---- .../Unit/AuditMiddlewareTests.cs | 94 ++++++++++++++++++- 2 files changed, 129 insertions(+), 21 deletions(-) diff --git a/modules/AuditLogs/src/SimpleModule.AuditLogs/Middleware/AuditMiddleware.cs b/modules/AuditLogs/src/SimpleModule.AuditLogs/Middleware/AuditMiddleware.cs index 2c10d630..96686ceb 100644 --- a/modules/AuditLogs/src/SimpleModule.AuditLogs/Middleware/AuditMiddleware.cs +++ b/modules/AuditLogs/src/SimpleModule.AuditLogs/Middleware/AuditMiddleware.cs @@ -175,32 +175,48 @@ private static async Task BuildSettingsAsync(ISettingsCont ISettingsContracts settings ) { - var results = await Task.WhenAll( - settings.GetSettingAsync("auditlogs.capture.http", Core.Settings.SettingScope.System), - settings.GetSettingAsync( - "auditlogs.capture.requestbodies", - Core.Settings.SettingScope.System - ), - settings.GetSettingAsync( - "auditlogs.capture.querystrings", - Core.Settings.SettingScope.System - ), - settings.GetSettingAsync( - "auditlogs.capture.useragent", - Core.Settings.SettingScope.System - ), - settings.GetSettingAsync("auditlogs.excluded.paths", Core.Settings.SettingScope.System) + // Awaited sequentially, not via Task.WhenAll: ISettingsContracts is scoped over a + // single SettingsDbContext, and on a cold cache each call queries it. Running them + // concurrently races the DbContext ("A second operation was started on this context + // instance"), which surfaces as intermittent 500s on the first request after start. + var captureHttpRaw = await settings.GetSettingAsync( + "auditlogs.capture.http", + Core.Settings.SettingScope.System + ); + var captureBodyRaw = await settings.GetSettingAsync( + "auditlogs.capture.requestbodies", + Core.Settings.SettingScope.System + ); + var captureQsRaw = await settings.GetSettingAsync( + "auditlogs.capture.querystrings", + Core.Settings.SettingScope.System + ); + var captureUaRaw = await settings.GetSettingAsync( + "auditlogs.capture.useragent", + Core.Settings.SettingScope.System + ); + var excludedPathsRaw = await settings.GetSettingAsync( + "auditlogs.excluded.paths", + Core.Settings.SettingScope.System ); - var captureHttp = !string.Equals(results[0], "false", StringComparison.OrdinalIgnoreCase); + var captureHttp = !string.Equals( + captureHttpRaw, + "false", + StringComparison.OrdinalIgnoreCase + ); - var captureBody = !string.Equals(results[1], "false", StringComparison.OrdinalIgnoreCase); + var captureBody = !string.Equals( + captureBodyRaw, + "false", + StringComparison.OrdinalIgnoreCase + ); - var captureQs = !string.Equals(results[2], "false", StringComparison.OrdinalIgnoreCase); + var captureQs = !string.Equals(captureQsRaw, "false", StringComparison.OrdinalIgnoreCase); - var captureUa = string.Equals(results[3], "true", StringComparison.OrdinalIgnoreCase); + var captureUa = string.Equals(captureUaRaw, "true", StringComparison.OrdinalIgnoreCase); - return (captureHttp, captureBody, captureQs, captureUa, results[4]); + return (captureHttp, captureBody, captureQs, captureUa, excludedPathsRaw); } private static string[] ParseExcludedPaths(string? rawPaths) diff --git a/modules/AuditLogs/tests/SimpleModule.AuditLogs.Tests/Unit/AuditMiddlewareTests.cs b/modules/AuditLogs/tests/SimpleModule.AuditLogs.Tests/Unit/AuditMiddlewareTests.cs index 211a7e29..45ef226c 100644 --- a/modules/AuditLogs/tests/SimpleModule.AuditLogs.Tests/Unit/AuditMiddlewareTests.cs +++ b/modules/AuditLogs/tests/SimpleModule.AuditLogs.Tests/Unit/AuditMiddlewareTests.cs @@ -1,3 +1,4 @@ +using System.Text.Json; using FluentAssertions; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Options; @@ -54,7 +55,7 @@ await settings .Received(1) .GetSettingAsync("auditlogs.excluded.paths", Arg.Any()); - // Verify all settings were fetched via Task.WhenAll (batch call) + // Verify all five settings were fetched (sequentially, over the scoped DbContext) // Total calls should be exactly 5 await settings.Received(5).GetSettingAsync(Arg.Any(), Arg.Any()); } @@ -277,6 +278,29 @@ public async Task InvokeAsync_HandlesNullSettingsGracefully() await next.Received(1).Invoke(context); } + [Fact] + public async Task InvokeAsync_LoadsSettingsSequentially_NeverConcurrently() + { + // Regression for #232: GetSettingAsync runs over a single scoped SettingsDbContext, + // which is not thread-safe. The middleware must await each lookup sequentially rather + // than fanning out via Task.WhenAll, otherwise the DbContext races and throws. + var settings = new ConcurrencyTrackingSettings(); + var channel = new AuditChannel(); + var next = Substitute.For(); + + var context = new DefaultHttpContext(); + context.Request.Path = "/api/test"; + context.Request.Method = "GET"; + context.RequestServices = CreateServiceProvider(settings, channel); + + var middleware = new AuditMiddleware(next, _cache); + + await middleware.InvokeAsync(context); + + settings.MaxObservedConcurrency.Should().Be(1); + await next.Received(1).Invoke(context); + } + private static IServiceProvider CreateServiceProvider( ISettingsContracts settings, AuditChannel channel @@ -295,4 +319,72 @@ private static IServiceProvider CreateServiceProviderWithoutSettings(AuditChanne serviceProvider.GetService(typeof(ISettingsContracts)).Returns(null); return serviceProvider; } + + /// + /// Stand-in for the scoped settings service that records the peak number of overlapping + /// calls. A correctly + /// sequential caller keeps this at 1; a Task.WhenAll fan-out drives it above 1 (mirroring + /// the DbContext race in #232). + /// + private sealed class ConcurrencyTrackingSettings : ISettingsContracts + { + private int _current; + + public int MaxObservedConcurrency { get; private set; } + + public async Task GetSettingAsync( + string key, + SettingScope scope, + string? userId = null + ) + { + var observed = Interlocked.Increment(ref _current); + MaxObservedConcurrency = Math.Max(MaxObservedConcurrency, observed); + try + { + // Yield so overlapping callers (if any) have a chance to enter before this returns. + await Task.Yield(); + return "true"; + } + finally + { + Interlocked.Decrement(ref _current); + } + } + + public Task GetSettingAsync(string key, SettingScope scope, string? userId = null) => + throw new NotSupportedException(); + + public Task ResolveUserSettingAsync(string key, string userId) => + throw new NotSupportedException(); + + public Task ResolveUserSettingElementAsync(string key, string userId) => + throw new NotSupportedException(); + + public Task SetSettingAsync( + string key, + JsonElement value, + SettingScope scope, + string? userId = null + ) => throw new NotSupportedException(); + + public Task SetManyAsync(IReadOnlyList updates) => + throw new NotSupportedException(); + + public Task DeleteSettingAsync(string key, SettingScope scope, string? userId = null) => + throw new NotSupportedException(); + + public Task ResetToDefaultAsync(string key, SettingScope scope, string? userId = null) => + throw new NotSupportedException(); + + public Task> GetSettingValuesAsync( + SettingsFilter? filter = null + ) => throw new NotSupportedException(); + + public Task GetSettingValueAsync( + string key, + SettingScope scope, + string? userId = null + ) => throw new NotSupportedException(); + } } From 8f470486bffcea593de07076fccdbc30f76fc065 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 11:41:08 +0000 Subject: [PATCH 2/2] fix(ratelimiting): tolerate missing RateLimiting_Rules table on refresh (#223) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RateLimitRuleCache is an IHostedService that queries RateLimiting_Rules in StartAsync. If the table doesn't exist — e.g. EnsureCreated() ran on an existing dev database before RateLimiting was added, or migrations weren't applied — the unhandled provider exception crashed the entire host before it could start listening. Catch DbException in RefreshAsync, log a warning, and keep the current (empty at startup) snapshot so the host starts and degrades gracefully with no DB-defined rules. Adds a regression test covering the missing-table path. --- .../RateLimitRuleCache.cs | 29 ++++++++++++-- .../RateLimitRuleCacheTests.cs | 38 +++++++++++++++++++ 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/modules/RateLimiting/src/SimpleModule.RateLimiting/RateLimitRuleCache.cs b/modules/RateLimiting/src/SimpleModule.RateLimiting/RateLimitRuleCache.cs index b42ae83b..bd7be9e0 100644 --- a/modules/RateLimiting/src/SimpleModule.RateLimiting/RateLimitRuleCache.cs +++ b/modules/RateLimiting/src/SimpleModule.RateLimiting/RateLimitRuleCache.cs @@ -1,3 +1,4 @@ +using System.Data.Common; using Microsoft.AspNetCore.Http; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; @@ -30,10 +31,23 @@ public async Task RefreshAsync(CancellationToken cancellationToken = default) await using var scope = scopeFactory.CreateAsyncScope(); var db = scope.ServiceProvider.GetRequiredService(); - var rules = await db - .Rules.AsNoTracking() - .Where(r => r.IsEnabled) - .ToListAsync(cancellationToken); + List rules; + try + { + rules = await db + .Rules.AsNoTracking() + .Where(r => r.IsEnabled) + .ToListAsync(cancellationToken); + } + catch (DbException ex) + { + // The RateLimiting_Rules table may not exist yet — e.g. EnsureCreated() ran on an + // existing dev database before this module was added, or migrations haven't been + // applied. Degrade gracefully (keep the current snapshot, no DB-defined rules at + // startup) instead of crashing the host from a hosted-service StartAsync. + LogRefreshFailed(logger, ex); + return; + } var compiled = rules .Select(Compile) @@ -141,4 +155,11 @@ public bool Matches(string path) => Message = "Rate-limit rule cache refreshed: {Count} enabled rules loaded" )] private static partial void LogRefreshed(ILogger logger, int count); + + [LoggerMessage( + Level = LogLevel.Warning, + Message = "Rate-limit rule cache refresh failed; continuing without DB-defined rules. " + + "The RateLimiting_Rules table may be missing or unmigrated." + )] + private static partial void LogRefreshFailed(ILogger logger, Exception exception); } diff --git a/modules/RateLimiting/tests/SimpleModule.RateLimiting.Tests/RateLimitRuleCacheTests.cs b/modules/RateLimiting/tests/SimpleModule.RateLimiting.Tests/RateLimitRuleCacheTests.cs index 9993e5c3..7ecdb884 100644 --- a/modules/RateLimiting/tests/SimpleModule.RateLimiting.Tests/RateLimitRuleCacheTests.cs +++ b/modules/RateLimiting/tests/SimpleModule.RateLimiting.Tests/RateLimitRuleCacheTests.cs @@ -130,6 +130,44 @@ public async Task RefreshAsync_PicksUpNewRules() _cache.FindForPath("/api/things").Should().NotBeNull(); } + [Fact] + public async Task RefreshAsync_DoesNotThrow_WhenRulesTableMissing() + { + // Regression for #223: a missing RateLimiting_Rules table (e.g. EnsureCreated ran + // before the module was added) must degrade gracefully rather than crash the host. + var dbOptions = new DbContextOptionsBuilder() + .UseSqlite("Data Source=:memory:") + .Options; + var databaseOptions = Options.Create( + new DatabaseOptions + { + ModuleConnections = new Dictionary + { + ["RateLimiting"] = "Data Source=:memory:", + }, + } + ); + + await using var emptyDb = new RateLimitingDbContext(dbOptions, databaseOptions); + await emptyDb.Database.OpenConnectionAsync(); // open the connection but never create tables + + var services = new ServiceCollection(); + services.AddSingleton(emptyDb); + await using var provider = services.BuildServiceProvider(); + + var cache = new RateLimitRuleCache( + provider.GetRequiredService(), + NullLogger.Instance + ); + + var act = () => cache.RefreshAsync(); + + await act.Should().NotThrowAsync(); + cache.FindForPath("/api/users").Should().BeNull(); + + await emptyDb.Database.CloseConnectionAsync(); + } + private static RateLimitRule NewRule( string name, string? pattern,