dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(ui): AuditLogQueryService uses scope-per-query to avoid DbContext race (#23 M7)

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-20 21:33:38 -04:00
parent 9c955da2e7
commit fac31c6018
5 changed files with 190 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs
View File

@@ -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<IAuditLogQueryService, AuditLogQueryService>();
//
// 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<IAuditLogQueryService>(sp => new AuditLogQueryService(
sp.GetRequiredService<IServiceScopeFactory>(),
sp.GetRequiredService<ICentralHealthAggregator>()));
// 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.

78
src/ScadaLink.CentralUI/Services/AuditLogQueryService.cs
View File

@@ -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 <see cref="IAuditLogRepository.QueryAsync"/>. Default page size is 100 (the
/// AuditResultsGrid default for #23 M7).
/// </summary>
/// <remarks>
/// <para>
/// #23 M7 (Bundle H follow-up): each query opens its OWN DI scope and resolves a
/// fresh <see cref="IAuditLogRepository"/> — and therefore a fresh
/// <c>ScadaLinkDbContext</c> — rather than sharing the scoped Blazor-circuit
/// context. Without this, the Audit Log page's query-string auto-load
/// (<c>/audit/log?correlationId=…</c>) races <c>AuditFilterBar.GetAllSitesAsync()</c>
/// on the single circuit-scoped <c>ScadaLinkDbContext</c>, 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 <c>AuditLogIngestActor</c>.
/// </para>
/// <para>
/// A second constructor takes an <see cref="IAuditLogRepository"/> directly — a
/// test seam (mirroring <c>AuditLogIngestActor</c>'s dual ctor) so unit tests can
/// substitute a stub without standing up a DI container.
/// </para>
/// </remarks>
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;
/// <summary>
/// Production constructor — resolves <see cref="IAuditLogRepository"/> from a
/// fresh DI scope on every call so each query gets its own
/// <c>ScadaLinkDbContext</c> and never contends with the circuit-scoped
/// context the filter bar uses.
/// </summary>
public AuditLogQueryService(
IServiceScopeFactory scopeFactory,
ICentralHealthAggregator healthAggregator)
{
_scopeFactory = scopeFactory ?? throw new ArgumentNullException(nameof(scopeFactory));
_healthAggregator = healthAggregator ?? throw new ArgumentNullException(nameof(healthAggregator));
}
/// <summary>
/// Test-seam constructor — injects a repository instance whose lifetime the
/// caller owns. Used by unit tests that substitute a stub repository.
/// </summary>
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<IReadOnlyList<AuditEvent>> QueryAsync(
public async Task<IReadOnlyList<AuditEvent>> 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<IAuditLogRepository>();
return await repository.QueryAsync(filter, effective, ct);
}
/// <inheritdoc/>
@@ -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<IAuditLogRepository>();
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

21
tests/ScadaLink.CentralUI.PlaywrightTests/Audit/AuditLogPageTests.cs
View File

@@ -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
{

7
tests/ScadaLink.CentralUI.Tests/ScadaLink.CentralUI.Tests.csproj
View File

@@ -12,6 +12,7 @@
<PackageReference Include="bunit" />
<PackageReference Include="coverlet.collector" />
<PackageReference Include="Microsoft.AspNetCore.TestHost" />
<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" />
<PackageReference Include="Microsoft.NET.Test.Sdk" />
<PackageReference Include="NSubstitute" />
<PackageReference Include="xunit" />
@@ -28,6 +29,12 @@
<ItemGroup>
<ProjectReference Include="../../src/ScadaLink.CentralUI/ScadaLink.CentralUI.csproj" />
<!--
The DbContext-race regression test (AuditLogQueryServiceTests) exercises a
real ScadaLinkDbContext + AuditLogRepository over SQLite in-memory to prove
scope-per-query isolation. Pulls in the ConfigurationDatabase project.
-->
<ProjectReference Include="../../src/ScadaLink.ConfigurationDatabase/ScadaLink.ConfigurationDatabase.csproj" />
</ItemGroup>
</Project>

88
tests/ScadaLink.CentralUI.Tests/Services/AuditLogQueryServiceTests.cs
View File

@@ -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<ScadaLinkDbContext>(options =>
options.UseSqlite(connection)
.ConfigureWarnings(w => w.Ignore(RelationalEventId.PendingModelChangesWarning)));
services.AddScoped<IAuditLogRepository, AuditLogRepository>();
await using var provider = services.BuildServiceProvider();
// Create the schema once on a throwaway scope.
using (var seedScope = provider.CreateScope())
{
var ctx = seedScope.ServiceProvider.GetRequiredService<ScadaLinkDbContext>();
await ctx.Database.EnsureCreatedAsync();
}
var scopeFactory = provider.GetRequiredService<IServiceScopeFactory>();
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<IAuditLogRepository>();
var services = new ServiceCollection();
services.AddScoped<IAuditLogRepository>(_ =>
{
var repo = Substitute.For<IAuditLogRepository>();
repo.QueryAsync(Arg.Any<AuditLogQueryFilter>(), Arg.Any<AuditLogPaging>(), Arg.Any<CancellationToken>())
.Returns(Task.FromResult<IReadOnlyList<AuditEvent>>(Array.Empty<AuditEvent>()));
resolvedRepos.Add(repo);
return repo;
});
await using var provider = services.BuildServiceProvider();
var sut = new AuditLogQueryService(
provider.GetRequiredService<IServiceScopeFactory>(),
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