From fac31c60185ab6576c02459cdf550e77d6289ab7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 20 May 2026 21:33:38 -0400 Subject: [PATCH] fix(ui): AuditLogQueryService uses scope-per-query to avoid DbContext race (#23 M7) --- .../ServiceCollectionExtensions.cs | 12 ++- .../Services/AuditLogQueryService.cs | 78 ++++++++++++++-- .../Audit/AuditLogPageTests.cs | 21 +++-- .../ScadaLink.CentralUI.Tests.csproj | 7 ++ .../Services/AuditLogQueryServiceTests.cs | 88 +++++++++++++++++++ 5 files changed, 190 insertions(+), 16 deletions(-) diff --git a/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs b/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs index 14d59ba..acec819 100644 --- a/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs @@ -4,6 +4,7 @@ using ScadaLink.CentralUI.Auth; using ScadaLink.CentralUI.Components.Shared; using ScadaLink.CentralUI.ScriptAnalysis; using ScadaLink.CentralUI.Services; +using ScadaLink.HealthMonitoring; namespace ScadaLink.CentralUI; @@ -30,7 +31,16 @@ public static class ServiceCollectionExtensions // Audit Log (#23 M7-T3): CentralUI facade over IAuditLogRepository so the // results grid can be tested with a stubbed query source. - services.AddScoped(); + // + // Registered with an explicit factory so the IServiceScopeFactory ctor is + // always chosen — AuditLogQueryService has a second (test-seam) ctor that + // takes IAuditLogRepository directly, and both are constructor-resolvable, + // so default activation would be ambiguous. The scope-factory ctor opens a + // fresh DbContext per query, which is what keeps the page's auto-load from + // racing AuditFilterBar's site enumeration on the shared scoped context. + services.AddScoped(sp => new AuditLogQueryService( + sp.GetRequiredService(), + sp.GetRequiredService())); // Audit Log (#23 M7-T14 / Bundle F): server-side streaming CSV export. // Backs the Audit Log page's Export button via GET /api/centralui/audit/export. diff --git a/src/ScadaLink.CentralUI/Services/AuditLogQueryService.cs b/src/ScadaLink.CentralUI/Services/AuditLogQueryService.cs index 6a566b3..f4bd2f4 100644 --- a/src/ScadaLink.CentralUI/Services/AuditLogQueryService.cs +++ b/src/ScadaLink.CentralUI/Services/AuditLogQueryService.cs @@ -1,3 +1,4 @@ +using Microsoft.Extensions.DependencyInjection; using ScadaLink.Commons.Entities.Audit; using ScadaLink.Commons.Interfaces.Repositories; using ScadaLink.Commons.Types; @@ -11,6 +12,24 @@ namespace ScadaLink.CentralUI.Services; /// to . Default page size is 100 (the /// AuditResultsGrid default for #23 M7). /// +/// +/// +/// #23 M7 (Bundle H follow-up): each query opens its OWN DI scope and resolves a +/// fresh — and therefore a fresh +/// ScadaLinkDbContext — rather than sharing the scoped Blazor-circuit +/// context. Without this, the Audit Log page's query-string auto-load +/// (/audit/log?correlationId=…) races AuditFilterBar.GetAllSitesAsync() +/// on the single circuit-scoped ScadaLinkDbContext, producing EF Core's +/// "A second operation was started on this context instance" error. Scope-per-query +/// removes the shared state so the two initial loads can run concurrently. This +/// mirrors the scope-per-message pattern used by AuditLogIngestActor. +/// +/// +/// A second constructor takes an directly — a +/// test seam (mirroring AuditLogIngestActor's dual ctor) so unit tests can +/// substitute a stub without standing up a DI container. +/// +/// public sealed class AuditLogQueryService : IAuditLogQueryService { // M7 Bundle E (T13): trailing window for the Health dashboard's Audit KPI tiles. @@ -19,27 +38,62 @@ public sealed class AuditLogQueryService : IAuditLogQueryService // and "% errors over the last hour" as the KPI definition. private static readonly TimeSpan KpiWindow = TimeSpan.FromHours(1); - private readonly IAuditLogRepository _repository; + // Production path: open a fresh scope per operation. Null in the test-seam ctor. + private readonly IServiceScopeFactory? _scopeFactory; + + // Test seam: a directly-injected repository whose lifetime the test owns. + // Null in the production ctor. + private readonly IAuditLogRepository? _injectedRepository; + private readonly ICentralHealthAggregator _healthAggregator; + /// + /// Production constructor — resolves from a + /// fresh DI scope on every call so each query gets its own + /// ScadaLinkDbContext and never contends with the circuit-scoped + /// context the filter bar uses. + /// + public AuditLogQueryService( + IServiceScopeFactory scopeFactory, + ICentralHealthAggregator healthAggregator) + { + _scopeFactory = scopeFactory ?? throw new ArgumentNullException(nameof(scopeFactory)); + _healthAggregator = healthAggregator ?? throw new ArgumentNullException(nameof(healthAggregator)); + } + + /// + /// Test-seam constructor — injects a repository instance whose lifetime the + /// caller owns. Used by unit tests that substitute a stub repository. + /// public AuditLogQueryService( IAuditLogRepository repository, ICentralHealthAggregator healthAggregator) { - _repository = repository ?? throw new ArgumentNullException(nameof(repository)); + _injectedRepository = repository ?? throw new ArgumentNullException(nameof(repository)); _healthAggregator = healthAggregator ?? throw new ArgumentNullException(nameof(healthAggregator)); } public int DefaultPageSize => 100; - public Task> QueryAsync( + public async Task> QueryAsync( AuditLogQueryFilter filter, AuditLogPaging? paging = null, CancellationToken ct = default) { ArgumentNullException.ThrowIfNull(filter); var effective = paging ?? new AuditLogPaging(DefaultPageSize); - return _repository.QueryAsync(filter, effective, ct); + + // Test-seam ctor: use the injected repository directly. + if (_injectedRepository is not null) + { + return await _injectedRepository.QueryAsync(filter, effective, ct); + } + + // Production: a fresh scope (and thus a fresh DbContext) per query so the + // page's auto-load never shares the circuit-scoped context. + await using var scope = _scopeFactory!.CreateAsyncScope(); + var repository = scope.ServiceProvider.GetRequiredService(); + return await repository.QueryAsync(filter, effective, ct); } /// @@ -47,8 +101,20 @@ public sealed class AuditLogQueryService : IAuditLogQueryService { // 1. Volume + error counts: aggregate over the trailing 1h window. // BacklogTotal is left at 0 by the repository — we fill it from the - // in-memory health aggregator below. - var repoSnapshot = await _repository.GetKpiSnapshotAsync(KpiWindow, nowUtc: null, ct); + // in-memory health aggregator below. Resolved via a per-call scope on + // the production path for the same context-isolation reason as + // QueryAsync; the test-seam ctor uses the injected repository. + AuditLogKpiSnapshot repoSnapshot; + if (_injectedRepository is not null) + { + repoSnapshot = await _injectedRepository.GetKpiSnapshotAsync(KpiWindow, nowUtc: null, ct); + } + else + { + await using var scope = _scopeFactory!.CreateAsyncScope(); + var repository = scope.ServiceProvider.GetRequiredService(); + repoSnapshot = await repository.GetKpiSnapshotAsync(KpiWindow, nowUtc: null, ct); + } // 2. Backlog: sum PendingCount across every site's latest report. // Sites that have not yet reported or whose reporter is disabled diff --git a/tests/ScadaLink.CentralUI.PlaywrightTests/Audit/AuditLogPageTests.cs b/tests/ScadaLink.CentralUI.PlaywrightTests/Audit/AuditLogPageTests.cs index d3c2da7..59cffcf 100644 --- a/tests/ScadaLink.CentralUI.PlaywrightTests/Audit/AuditLogPageTests.cs +++ b/tests/ScadaLink.CentralUI.PlaywrightTests/Audit/AuditLogPageTests.cs @@ -270,15 +270,18 @@ public class AuditLogPageTests await Assertions.Expect(page.Locator("[data-test='audit-filter-bar']")).ToBeVisibleAsync(); await Assertions.Expect(page.Locator("[data-test='audit-results-grid']")).ToBeVisibleAsync(); - // NOTE (deviation, #23 M7-T16): Bundle D's auto-load (the grid - // populating without an Apply click) currently fails on this drill-in - // path with an EF "A second operation was started on this context - // instance" error — the page-level query-string auto-load races the - // AuditFilterBar's GetAllSitesAsync() on the shared scoped Blazor - // DbContext. This test therefore asserts the drill-in *navigation* - // contract only; auto-load population is intentionally NOT asserted - // here. Filed as a Bundle D follow-up. The Apply-driven query path is - // covered green by FilterNarrowing / DrilldownDrawer tests. + // Bundle D auto-load: the query-string drill-in populates the grid + // WITHOUT an Apply click. The grid resolves the ?correlationId= filter + // on OnInitialized and the seeded row appears. + // + // This was previously blocked by an EF "A second operation was started + // on this context instance" error — the page-level auto-load raced + // AuditFilterBar.GetAllSitesAsync() on the shared scoped Blazor + // DbContext. AuditLogQueryService now opens its own DI scope per query + // (scope-per-query), so the auto-load no longer contends with the + // filter bar's site enumeration and the assertion is restored. + var seededRow = page.Locator($"[data-test='grid-row-{eventId}']"); + await Assertions.Expect(seededRow).ToBeVisibleAsync(); } finally { diff --git a/tests/ScadaLink.CentralUI.Tests/ScadaLink.CentralUI.Tests.csproj b/tests/ScadaLink.CentralUI.Tests/ScadaLink.CentralUI.Tests.csproj index 4b1faa7..59b20e8 100644 --- a/tests/ScadaLink.CentralUI.Tests/ScadaLink.CentralUI.Tests.csproj +++ b/tests/ScadaLink.CentralUI.Tests/ScadaLink.CentralUI.Tests.csproj @@ -12,6 +12,7 @@ + @@ -28,6 +29,12 @@ + + \ No newline at end of file diff --git a/tests/ScadaLink.CentralUI.Tests/Services/AuditLogQueryServiceTests.cs b/tests/ScadaLink.CentralUI.Tests/Services/AuditLogQueryServiceTests.cs index 181f3bc..3e86a0d 100644 --- a/tests/ScadaLink.CentralUI.Tests/Services/AuditLogQueryServiceTests.cs +++ b/tests/ScadaLink.CentralUI.Tests/Services/AuditLogQueryServiceTests.cs @@ -1,3 +1,6 @@ +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.Extensions.DependencyInjection; using NSubstitute; using ScadaLink.CentralUI.Services; using ScadaLink.Commons.Entities.Audit; @@ -6,6 +9,8 @@ using ScadaLink.Commons.Messages.Health; using ScadaLink.Commons.Types; using ScadaLink.Commons.Types.Audit; using ScadaLink.Commons.Types.Enums; +using ScadaLink.ConfigurationDatabase; +using ScadaLink.ConfigurationDatabase.Repositories; using ScadaLink.HealthMonitoring; namespace ScadaLink.CentralUI.Tests.Services; @@ -134,6 +139,89 @@ public class AuditLogQueryServiceTests Assert.Equal(4, snapshot.BacklogTotal); } + // ───────────────────────────────────────────────────────────────────────── + // #23 M7 — DbContext concurrency race regression (Bundle H follow-up) + // + // The drill-in deep link (/audit/log?correlationId=…) triggers an OnInitialized + // auto-load query that races AuditFilterBar.GetAllSitesAsync() on the SAME + // scoped Blazor-circuit ScadaLinkDbContext. EF Core then throws + // "A second operation was started on this context instance before a previous + // operation completed." AuditLogQueryService now opens its OWN DI scope per + // QueryAsync call (scope-per-query) so it never shares the page's scoped + // context — these tests pin that contract. + // ───────────────────────────────────────────────────────────────────────── + + [Fact] + public async Task AuditLogQueryService_ConcurrentQueries_DoNotRace() + { + // A real ScadaLinkDbContext (SQLite in-memory) registered as SCOPED with + // the real AuditLogRepository — exactly the shared-scoped-context shape + // that produces the EF race when one context services two operations. + using var connection = new Microsoft.Data.Sqlite.SqliteConnection("DataSource=:memory:"); + connection.Open(); + + var services = new ServiceCollection(); + services.AddLogging(); + services.AddDbContext(options => + options.UseSqlite(connection) + .ConfigureWarnings(w => w.Ignore(RelationalEventId.PendingModelChangesWarning))); + services.AddScoped(); + + await using var provider = services.BuildServiceProvider(); + + // Create the schema once on a throwaway scope. + using (var seedScope = provider.CreateScope()) + { + var ctx = seedScope.ServiceProvider.GetRequiredService(); + await ctx.Database.EnsureCreatedAsync(); + } + + var scopeFactory = provider.GetRequiredService(); + var sut = new AuditLogQueryService(scopeFactory, EmptyAggregator()); + + var filter = new AuditLogQueryFilter(Channel: AuditChannel.ApiOutbound); + + // Fire two QueryAsync calls in parallel. With scope-per-query each gets a + // fresh DbContext, so this completes cleanly; with a shared scoped context + // EF throws "A second operation was started on this context instance". + var t1 = sut.QueryAsync(filter); + var t2 = sut.QueryAsync(filter); + + var results = await Task.WhenAll(t1, t2); + + Assert.All(results, Assert.NotNull); + } + + [Fact] + public async Task QueryAsync_OpensFreshScopePerCall_NotSharedAcrossCalls() + { + // Two sequential calls must each resolve the repository from a distinct + // scope — the service must never cache a single repository instance. + var resolvedRepos = new List(); + + var services = new ServiceCollection(); + services.AddScoped(_ => + { + var repo = Substitute.For(); + repo.QueryAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult>(Array.Empty())); + resolvedRepos.Add(repo); + return repo; + }); + + await using var provider = services.BuildServiceProvider(); + var sut = new AuditLogQueryService( + provider.GetRequiredService(), + EmptyAggregator()); + + await sut.QueryAsync(new AuditLogQueryFilter()); + await sut.QueryAsync(new AuditLogQueryFilter()); + + // Each QueryAsync opened its own scope → two distinct repo instances. + Assert.Equal(2, resolvedRepos.Count); + Assert.NotSame(resolvedRepos[0], resolvedRepos[1]); + } + private static SiteHealthState StateWithBacklog(string siteId, int? pending) { SiteAuditBacklogSnapshot? backlog = pending.HasValue