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(); + } } 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,