diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogIngestActor.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogIngestActor.cs index 1ca8e692..c4c57525 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogIngestActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogIngestActor.cs @@ -42,13 +42,14 @@ namespace ZB.MOM.WW.ScadaBridge.AuditLog.Central; /// placeholder. /// /// -/// Two constructors exist for a deliberate reason: the test ctor injects a -/// concrete against a per-test MSSQL fixture -/// (the only way to verify the ingest-timestamp stamp + duplicate-key -/// idempotency end to end), while the production host wiring registers the -/// actor as a cluster singleton and must therefore resolve the repository — -/// which is a scoped EF Core service — from a fresh DI scope per message. -/// Mirroring the Notification Outbox actor's pattern. +/// There is exactly ONE constructor, taking an : +/// the actor is a cluster singleton and the repository is a scoped EF Core +/// service, so it must be resolved from a fresh DI scope per message and never +/// held on a field. Mirroring the Notification Outbox actor's pattern. Tests +/// that need a concrete repository (e.g. the MSSQL-backed fixture verifying the +/// ingest-timestamp stamp and duplicate-key idempotency) supply a stub provider +/// that hands one out, rather than a second constructor that would cache it — +/// see register row 61. /// /// public class AuditLogIngestActor : ReceiveActor @@ -100,41 +101,25 @@ public class AuditLogIngestActor : ReceiveActor /// internal static readonly TimeSpan IngestFallbackBudget = TimeSpan.FromSeconds(5); - private readonly IServiceProvider? _serviceProvider; - private readonly IAuditLogRepository? _injectedRepository; + private readonly IServiceProvider _serviceProvider; private readonly ILogger _logger; /// - /// Test-mode constructor — injects a concrete repository instance whose - /// lifetime exceeds the test, so the actor reuses the same instance across - /// every message. Used by the MSSQL-backed TestKit fixture. - /// - /// Audit log repository instance shared across all messages. - /// Logger for ingest diagnostics. - public AuditLogIngestActor( - IAuditLogRepository repository, - ILogger logger) - { - ArgumentNullException.ThrowIfNull(repository); - ArgumentNullException.ThrowIfNull(logger); - - _injectedRepository = repository; - _logger = logger; - - ReceiveAsync(OnIngestAsync); - // The single-repository test ctor cannot service the dual-write — - // it has no SiteCalls repo and no DbContext. The handler still - // registers (so callers don't dead-letter) but replies empty so the - // test surface stays explicit about what this ctor supports. - ReceiveAsync(OnCachedTelemetryWithoutDualWriteAsync); - } - - /// - /// Production constructor — resolves from + /// The only constructor — resolves from /// a fresh DI scope per message because the repository is a scoped EF Core /// service registered by AddConfigurationDatabase. The actor itself /// is a long-lived cluster singleton, so it cannot hold a scope across /// messages. + /// + /// + /// Register row 61: a second, repository-taking "test-mode" constructor used + /// to cache a scoped repository on a field for the actor's whole lifetime. + /// Nothing in production selected it, but it left the captive-dependency bug + /// one call site away and had to be allowlisted in the Host tests' + /// CaptiveDependencyGuard. It is gone; test fixtures build a stub + /// instead, which keeps the actor's own + /// contract "resolve per scope, hold nothing". + /// /// /// Root service provider used to open a fresh scope per message. /// Logger for ingest diagnostics. @@ -169,59 +154,51 @@ public class AuditLogIngestActor : ReceiveActor var accepted = new List(cmd.Events.Count); // Resolve the repository for the whole batch — one DbContext per - // message, mirroring NotificationOutboxActor. The injected-repository - // mode (test ctor) skips the scope entirely. + // message, mirroring NotificationOutboxActor. // The IAuditRedactor is also resolved from the per-message scope when // one is available so the row is truncated + redacted before - // InsertIfNotExistsAsync. The single-repository test ctor has no - // service provider — it falls through with no redactor, which preserves - // the small-payload assumptions baked into the existing fixtures. + // InsertIfNotExistsAsync. A composition root that registers no redactor + // (test fixtures) falls back to the SafeDefault inside + // IngestWithRepositoryAsync rather than to pass-through. // Use CreateAsyncScope + await using so scoped EF Core // services (IAsyncDisposable DbContexts) dispose asynchronously // without blocking on sync Dispose() of pending connection cleanup. - if (_injectedRepository is not null) + // + // Guard scope-creation + repository resolution in a + // try/catch, mirroring OnCachedTelemetryAsync. A transient DI / + // DbContext-factory fault (pooled-context init, SQL-connection + // exhaustion, a resolution race during host churn) would otherwise + // propagate out of the ReceiveAsync handler, trip the parent's + // supervision, and RESTART this central singleton over a transient + // fault — dropping the captured reply so the site's Ask times out. + // Best-effort audit must never wedge the singleton: log, optionally + // bump the failure counter, and still reply with whatever was + // accepted (empty on an up-front scope-resolution throw) so the + // site keeps its rows Pending and retries on the next drain. + try { - await IngestWithRepositoryAsync(_injectedRepository, redactor: null, failureCounter: null, cmd, nowUtc, accepted) + await using var scope = _serviceProvider.CreateAsyncScope(); + var repository = scope.ServiceProvider.GetRequiredService(); + var redactor = scope.ServiceProvider.GetService(); + // Central health counter is best-effort — + // unregistered (test composition roots) means the per-row catch + // simply logs without surfacing on the health dashboard. + var failureCounter = scope.ServiceProvider.GetService(); + await IngestWithRepositoryAsync(repository, redactor, failureCounter, cmd, nowUtc, accepted) .ConfigureAwait(false); } - else + catch (Exception ex) { - // Guard scope-creation + repository resolution in a - // try/catch, mirroring OnCachedTelemetryAsync. A transient DI / - // DbContext-factory fault (pooled-context init, SQL-connection - // exhaustion, a resolution race during host churn) would otherwise - // propagate out of the ReceiveAsync handler, trip the parent's - // supervision, and RESTART this central singleton over a transient - // fault — dropping the captured reply so the site's Ask times out. - // Best-effort audit must never wedge the singleton: log, optionally - // bump the failure counter, and still reply with whatever was - // accepted (empty on an up-front scope-resolution throw) so the - // site keeps its rows Pending and retries on the next drain. - try - { - await using var scope = _serviceProvider!.CreateAsyncScope(); - var repository = scope.ServiceProvider.GetRequiredService(); - var redactor = scope.ServiceProvider.GetService(); - // Central health counter is best-effort — - // unregistered (test composition roots) means the per-row catch - // simply logs without surfacing on the health dashboard. - var failureCounter = scope.ServiceProvider.GetService(); - await IngestWithRepositoryAsync(repository, redactor, failureCounter, cmd, nowUtc, accepted) - .ConfigureAwait(false); - } - catch (Exception ex) - { - // Scope creation or a required-service resolution threw before - // (or while) processing the batch. Surface a sustained fault on - // the dashboard if the counter is registered, but never let the - // throw escape the handler and restart the singleton. - try { _serviceProvider!.GetService()?.Increment(); } - catch { /* counter must never throw — defence in depth */ } - _logger.LogError( - ex, - "Audit event batch ingest failed before/while resolving the repository scope; replying with {Accepted} accepted row(s). The site keeps unaccepted rows Pending and retries on the next drain.", - accepted.Count); - } + // Scope creation or a required-service resolution threw before + // (or while) processing the batch. Surface a sustained fault on + // the dashboard if the counter is registered, but never let the + // throw escape the handler and restart the singleton. + try { _serviceProvider.GetService()?.Increment(); } + catch { /* counter must never throw — defence in depth */ } + _logger.LogError( + ex, + "Audit event batch ingest failed before/while resolving the repository scope; replying with {Accepted} accepted row(s). The site keeps unaccepted rows Pending and retries on the next drain.", + accepted.Count); } replyTo.Tell(new IngestAuditEventsReply(accepted)); @@ -357,7 +334,7 @@ public class AuditLogIngestActor : ReceiveActor try { - await using var scope = _serviceProvider!.CreateAsyncScope(); + await using var scope = _serviceProvider.CreateAsyncScope(); var auditRepo = scope.ServiceProvider.GetRequiredService(); var siteCallRepo = scope.ServiceProvider.GetRequiredService(); var dbContext = scope.ServiceProvider.GetRequiredService(); @@ -544,18 +521,4 @@ public class AuditLogIngestActor : ReceiveActor return true; } - /// - /// Fallback handler installed on the single-repository test ctor — that - /// ctor has no DbContext and no , so - /// it cannot service the dual-write. Logs a warning and replies with an - /// empty ack so callers fall through to their retry path. - /// - private Task OnCachedTelemetryWithoutDualWriteAsync(IngestCachedTelemetryCommand cmd) - { - _logger.LogWarning( - "AuditLogIngestActor received IngestCachedTelemetryCommand on the single-repository ctor; dual-write requires the IServiceProvider ctor. Replying with empty ack ({Count} entries).", - cmd.Entries.Count); - Sender.Tell(new IngestCachedTelemetryReply(Array.Empty())); - return Task.CompletedTask; - } } diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/SiteCallAuditActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/SiteCallAuditActor.cs index aee226b5..ad2a7078 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/SiteCallAuditActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteCallAudit/SiteCallAuditActor.cs @@ -87,8 +87,7 @@ public class SiteCallAuditActor : ReceiveActor /// private const int MaxReconciliationPagesPerTick = 50; - private readonly IServiceProvider? _serviceProvider; - private readonly ISiteCallAuditRepository? _injectedRepository; + private readonly IServiceProvider _serviceProvider; private readonly SiteCallAuditOptions _options; private readonly ILogger _logger; @@ -108,28 +107,17 @@ public class SiteCallAuditActor : ReceiveActor /// () and the site list /// (). On the production path these are /// resolved once from the root (central - /// singletons registered by AddAuditLogCentralReconciliationClient); - /// in the test path they are injected directly. They are null when - /// the actor was built via the repo-only test ctor — in that case the - /// reconciliation tick is NOT started (see ). - /// The purge tick, by contrast, does NOT depend on these collaborators: - /// it needs only the repository, so it is armed by - /// alone (see ). + /// singletons registered by AddAuditLogCentralReconciliationClient). + /// They are null when the composition root did not register them (a + /// minimal host, or a test fixture that deliberately omits them) — in that + /// case the reconciliation tick is NOT started (see + /// ). The purge tick, by contrast, does + /// NOT depend on these collaborators: it needs only the repository, so it is + /// always armed (see ). /// private readonly IPullSiteCallsClient? _pullClient; private readonly ISiteEnumerator? _siteEnumerator; - /// - /// Master switch for the two background schedulers (reconciliation + purge), - /// set true by the production and reconciliation ctors and false - /// by the repo-only MSSQL test ctor. The purge timer is - /// gated on THIS flag rather than on the reconciliation collaborators, so a - /// host that omits the reconciliation client still purges (no unbounded - /// central SiteCalls growth) while the MSSQL read/upsert tests stay - /// free of any scheduled side effects. - /// - private readonly bool _backgroundTimersEnabled; - /// /// Per-site reconciliation watermark — the composite /// (UpdatedAtUtc, TrackedOperationId) keyset of the highest row seen @@ -195,98 +183,23 @@ public class SiteCallAuditActor : ReceiveActor private IActorRef? _centralCommunication; /// - /// Test-mode constructor — injects a concrete repository instance whose - /// lifetime exceeds the test, so the actor reuses the same instance - /// across every message. Used by the MSSQL-backed TestKit fixture. - /// An optional lets a test pin the stuck/KPI - /// windows; when omitted the production defaults apply. - /// - /// This ctor disables BOTH background timers (sets - /// to false) and injects no - /// reconciliation client/enumerator, so neither the reconciliation tick nor - /// the purge tick fires — the MSSQL-backed read/upsert tests must see no - /// scheduled side effects (no phantom pulls, no background purge). - /// - /// - /// Concrete repository instance to use for all messages. - /// Logger for diagnostics and error reporting. - /// Optional configuration overrides; production defaults apply when null. - /// Optional central direct-write audit sink for operator relay actions; null disables the relay audit row. - public SiteCallAuditActor( - ISiteCallAuditRepository repository, - ILogger logger, - SiteCallAuditOptions? options = null, - ICentralAuditWriter? auditWriter = null) - { - ArgumentNullException.ThrowIfNull(repository); - ArgumentNullException.ThrowIfNull(logger); - - _injectedRepository = new SerializedRepository(repository); - _logger = logger; - _options = options ?? new SiteCallAuditOptions(); - _auditWriter = auditWriter; - - // Repo-only MSSQL test ctor: keep BOTH background timers off so the - // read/upsert tests see no scheduled side effects. - _backgroundTimersEnabled = false; - - RegisterHandlers(); - } - - /// - /// Test-mode constructor for the reconciliation tick (Piece A) — injects a - /// concrete repository PLUS the two reconciliation collaborators directly, - /// so the per-site self-heal pull is unit-testable in-memory without a DI - /// container or a live gRPC channel. Background timers are enabled, so the - /// purge tick starts (it needs only the repository) and the reconciliation - /// tick starts too because the client + enumerator are present — see - /// / . - /// - /// Concrete repository instance used for upserts and purges. - /// Enumerates the sites to reconcile each tick. - /// Pull client used to fetch changed rows from each site. - /// Logger for diagnostics and error reporting. - /// Optional configuration overrides; production defaults apply when null. - /// - /// Public (not internal) because Akka's default ActivatorProducer - /// instantiates the actor via reflection with public-only binding flags — - /// an internal ctor yields a MissingMethodException at actor - /// creation. Distinguished from the production - /// ctor by its concrete-collaborator parameter list; only the test project - /// (or a host that hand-resolves the collaborators) constructs it this way. - /// - public SiteCallAuditActor( - ISiteCallAuditRepository repository, - ISiteEnumerator siteEnumerator, - IPullSiteCallsClient pullClient, - ILogger logger, - SiteCallAuditOptions? options = null) - { - ArgumentNullException.ThrowIfNull(repository); - ArgumentNullException.ThrowIfNull(siteEnumerator); - ArgumentNullException.ThrowIfNull(pullClient); - ArgumentNullException.ThrowIfNull(logger); - - _injectedRepository = new SerializedRepository(repository); - _siteEnumerator = siteEnumerator; - _pullClient = pullClient; - _logger = logger; - _options = options ?? new SiteCallAuditOptions(); - - // Reconciliation test ctor: collaborators present, so both timers arm - // (the reconciliation tick uses the collaborators; the purge tick needs - // only the repository). - _backgroundTimersEnabled = true; - - RegisterHandlers(); - } - - /// - /// Production constructor — resolves + /// The only constructor — resolves /// from a fresh DI scope per message because the repository is a scoped EF /// Core service registered by AddConfigurationDatabase. The actor /// itself is a long-lived cluster singleton, so it cannot hold a scope /// across messages. + /// + /// + /// Register row 61: two further "test-mode" constructors used to take a + /// concrete repository and cache it (wrapped in a serializing decorator) on + /// a field for the actor's whole lifetime. Nothing in production selected + /// them, but they left the captive-dependency bug one call site away and had + /// to be allowlisted in the Host tests' CaptiveDependencyGuard. They + /// are gone; test fixtures build a stub + /// instead — and own the serializing decorator themselves, since it exists + /// only to compensate for a fixture handing the SAME repository instance to + /// every scope. Production never needed it: each scope gets its own. + /// /// /// DI service provider used to create a scoped repository per message. /// Actor configuration (stuck threshold, KPI interval, relay timeout). @@ -321,12 +234,6 @@ public class SiteCallAuditActor : ReceiveActor _pullClient = serviceProvider.GetService(); _siteEnumerator = serviceProvider.GetService(); - // Production path: background timers run. The purge tick is armed - // unconditionally here (it needs only the repository); the reconciliation - // tick additionally requires its collaborators and logs a Warning if - // they were not registered. - _backgroundTimersEnabled = true; - RegisterHandlers(); } @@ -462,11 +369,6 @@ public class SiteCallAuditActor : ReceiveActor /// private void StartReconciliationTimer() { - if (!_backgroundTimersEnabled) - { - return; - } - if (_pullClient is null || _siteEnumerator is null) { _logger.LogWarning( @@ -488,23 +390,16 @@ public class SiteCallAuditActor : ReceiveActor /// /// Starts the daily purge tick. The purge needs ONLY the - /// repository — never the reconciliation collaborators — so it is gated on - /// alone, NOT on + /// repository — never the reconciliation collaborators — so it is armed + /// unconditionally, NOT gated on /// / . This decouples /// the daily terminal-row purge from the reconciliation client: a host that /// registers Site Call Audit without the reconciliation client still purges, /// so the central SiteCalls table can never grow unbounded just - /// because the self-heal puller is absent. Only the repo-only MSSQL test ctor - /// (background timers off) skips it, keeping the read/upsert tests free of - /// scheduled side effects. + /// because the self-heal puller is absent. /// private void StartPurgeTimer() { - if (!_backgroundTimersEnabled) - { - return; - } - var interval = _options.ResolvedPurgeInterval; _purgeTimer = Context.System.Scheduler.ScheduleTellRepeatedlyCancelable( // Short first tick (min(interval, 5min)): purges are idempotent, and a full-interval @@ -532,20 +427,11 @@ public class SiteCallAuditActor : ReceiveActor var replyTo = Sender; var id = cmd.SiteCall.TrackedOperationId; - // Scope-per-message mirrors AuditLogIngestActor — production EF - // repository is scoped; the injected-repository mode (tests) skips - // the scope entirely. - IServiceScope? scope = null; - ISiteCallAuditRepository repository; - if (_injectedRepository is not null) - { - repository = _injectedRepository; - } - else - { - scope = _serviceProvider!.CreateScope(); - repository = scope.ServiceProvider.GetRequiredService(); - } + // Scope-per-message mirrors AuditLogIngestActor — the EF repository is + // a scoped service, so it is resolved fresh here and never held. + IServiceScope? scope = _serviceProvider.CreateScope(); + ISiteCallAuditRepository repository = + scope.ServiceProvider.GetRequiredService(); try { @@ -624,15 +510,8 @@ public class SiteCallAuditActor : ReceiveActor // thread on a synchronous Dispose() of pending connection cleanup — the tick // holds the scope across many awaited UpsertAsync calls. Mirrors the sibling // SiteAuditReconciliationActor.OnTickAsync. ResolveRepository() (sync Dispose) - // is retained for the synchronous message-handler paths. In the injected- - // repository test path there is no scope to open and the test repo is reused. - if (_injectedRepository is not null) - { - await ReconcileSitesAsync(sites, client, _injectedRepository).ConfigureAwait(false); - return; - } - - await using var scope = _serviceProvider!.CreateAsyncScope(); + // is retained for the synchronous message-handler paths. + await using var scope = _serviceProvider.CreateAsyncScope(); var repository = scope.ServiceProvider.GetRequiredService(); await ReconcileSitesAsync(sites, client, repository).ConfigureAwait(false); } @@ -860,15 +739,8 @@ public class SiteCallAuditActor : ReceiveActor // Open the scope INLINE with CreateAsyncScope + await using // so the scoped EF Core repository (an IAsyncDisposable DbContext) disposes // asynchronously rather than blocking the Akka dispatcher thread on a - // synchronous Dispose(). Mirrors SiteAuditReconciliationActor; the - // injected-repository test path reuses the test repo with no scope. - if (_injectedRepository is not null) - { - await PurgeWithRepositoryAsync(_injectedRepository, threshold).ConfigureAwait(false); - return; - } - - await using var scope = _serviceProvider!.CreateAsyncScope(); + // synchronous Dispose(). Mirrors SiteAuditReconciliationActor. + await using var scope = _serviceProvider.CreateAsyncScope(); var repository = scope.ServiceProvider.GetRequiredService(); await PurgeWithRepositoryAsync(repository, threshold).ConfigureAwait(false); } @@ -1454,19 +1326,13 @@ public class SiteCallAuditActor : ReceiveActor /// /// Resolves an for one read message. - /// In test mode the injected instance is returned with a null scope; in - /// production a fresh DI scope is created and returned so the caller can - /// dispose it once the read completes — the same scope-per-message pattern - /// as . + /// A fresh DI scope is created and returned so the caller can dispose it + /// once the read completes — the same scope-per-message pattern as + /// . /// private (IServiceScope? Scope, ISiteCallAuditRepository Repository) ResolveRepository() { - if (_injectedRepository is not null) - { - return (null, _injectedRepository); - } - - var scope = _serviceProvider!.CreateScope(); + var scope = _serviceProvider.CreateScope(); return (scope, scope.ServiceProvider.GetRequiredService()); } @@ -1565,91 +1431,6 @@ public class SiteCallAuditActor : ReceiveActor private PurgeComplete() { } } - /// - /// Serializing wrapper applied to a repository handed in through the - /// test constructors. One call at a time, in arrival order. - /// - /// - /// - /// In production every consumer of this actor's repository gets its OWN DI - /// scope — one per message, one per background pass — so the off-mailbox - /// reconciliation and purge passes can safely run alongside mailbox handlers: - /// separate scopes mean separate DbContexts. The injected-repository - /// constructors break that assumption: ONE instance (typically wrapping one - /// DbContext) is shared by the mailbox handlers AND the background - /// passes, and DbContext forbids concurrent operations — "A second - /// operation was started on this context instance" is a hard fault, and the - /// interleaving is nondeterministic, so it surfaces as a flaky test rather - /// than a reliable one. - /// - /// - /// Serializing at the CALL, not around the whole pass, is deliberate: it - /// removes the concurrency hazard while preserving the property the - /// off-mailbox passes exist for — a long drain (blocked in a network pull, - /// holding no repository call) still lets ingest, query and KPI messages be - /// answered. Suspending the mailbox for the duration of a pass would have - /// been simpler and would have invalidated exactly those regression tests. - /// Production is untouched: it never constructs this type. - /// - /// - private sealed class SerializedRepository : ISiteCallAuditRepository - { - private readonly ISiteCallAuditRepository _inner; - private readonly SemaphoreSlim _gate = new(1, 1); - - public SerializedRepository(ISiteCallAuditRepository inner) => _inner = inner; - - public Task UpsertAsync(SiteCall siteCall, CancellationToken ct = default) => - RunAsync(() => _inner.UpsertAsync(siteCall, ct)); - - public Task GetAsync(TrackedOperationId id, CancellationToken ct = default) => - RunAsync(() => _inner.GetAsync(id, ct)); - - public Task> QueryAsync( - SiteCallQueryFilter filter, SiteCallPaging paging, CancellationToken ct = default) => - RunAsync(() => _inner.QueryAsync(filter, paging, ct)); - - public Task PurgeTerminalAsync(DateTime olderThanUtc, CancellationToken ct = default) => - RunAsync(() => _inner.PurgeTerminalAsync(olderThanUtc, ct)); - - public Task ComputeKpisAsync( - DateTime stuckCutoffUtc, DateTime deliveredSinceUtc, CancellationToken ct = default) => - RunAsync(() => _inner.ComputeKpisAsync(stuckCutoffUtc, deliveredSinceUtc, ct)); - - public Task> ComputePerSiteKpisAsync( - DateTime stuckCutoffUtc, DateTime deliveredSinceUtc, CancellationToken ct = default) => - RunAsync(() => _inner.ComputePerSiteKpisAsync(stuckCutoffUtc, deliveredSinceUtc, ct)); - - public Task> ComputePerNodeKpisAsync( - DateTime stuckCutoffUtc, DateTime deliveredSinceUtc, CancellationToken ct = default) => - RunAsync(() => _inner.ComputePerNodeKpisAsync(stuckCutoffUtc, deliveredSinceUtc, ct)); - - private async Task RunAsync(Func call) - { - await _gate.WaitAsync().ConfigureAwait(false); - try - { - await call().ConfigureAwait(false); - } - finally - { - _gate.Release(); - } - } - - private async Task RunAsync(Func> call) - { - await _gate.WaitAsync().ConfigureAwait(false); - try - { - return await call().ConfigureAwait(false); - } - finally - { - _gate.Release(); - } - } - } } /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogIngestActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogIngestActorTests.cs index 61fdc239..aa6f70e5 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogIngestActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogIngestActorTests.cs @@ -51,10 +51,15 @@ public class AuditLogIngestActorTests : TestKit, IClassFixture - Sys.ActorOf(Props.Create(() => new AuditLogIngestActor( - repository, + private IActorRef CreateActor(IAuditLogRepository repository) + { + // Register row 61: the actor's repository-taking test ctor is gone; the fixture + // supplies a stub provider that hands this repository out of every scope. + var provider = TestSupport.StubAuditRepositoryProvider.Build(repository); + return Sys.ActorOf(Props.Create(() => new AuditLogIngestActor( + provider, NullLogger.Instance))); + } [SkippableFact] public async Task Receive_BatchOf5_Calls_Repo_5Times_Acks_All_5() diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/DatabaseSyncEmissionEndToEndTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/DatabaseSyncEmissionEndToEndTests.cs index c6490b4d..0a7997b8 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/DatabaseSyncEmissionEndToEndTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/DatabaseSyncEmissionEndToEndTests.cs @@ -130,10 +130,15 @@ public class DatabaseSyncEmissionEndToEndTests : TestKit, IClassFixture - Sys.ActorOf(Props.Create(() => new AuditLogIngestActor( - repo, + private IActorRef CreateIngestActor(IAuditLogRepository repo) + { + // Register row 61: the actor's repository-taking test ctor is gone; the fixture + // supplies a stub provider that hands this repository out of every scope. + var provider = TestSupport.StubAuditRepositoryProvider.Build(repo); + return Sys.ActorOf(Props.Create(() => new AuditLogIngestActor( + provider, NullLogger.Instance))); + } private IActorRef CreateTelemetryActor( ISiteAuditQueue queue, diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/ExecutionIdCorrelationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/ExecutionIdCorrelationTests.cs index 02ab43b3..a936f0ee 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/ExecutionIdCorrelationTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/ExecutionIdCorrelationTests.cs @@ -127,10 +127,15 @@ public class ExecutionIdCorrelationTests : TestKit, IClassFixture - Sys.ActorOf(Props.Create(() => new AuditLogIngestActor( - repo, + private IActorRef CreateIngestActor(IAuditLogRepository repo) + { + // Register row 61: the actor's repository-taking test ctor is gone; the fixture + // supplies a stub provider that hands this repository out of every scope. + var provider = TestSupport.StubAuditRepositoryProvider.Build(repo); + return Sys.ActorOf(Props.Create(() => new AuditLogIngestActor( + provider, NullLogger.Instance))); + } private IActorRef CreateTelemetryActor( ISiteAuditQueue queue, diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/SyncCallEmissionEndToEndTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/SyncCallEmissionEndToEndTests.cs index 2e7633ad..409610fa 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/SyncCallEmissionEndToEndTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Integration/SyncCallEmissionEndToEndTests.cs @@ -110,10 +110,15 @@ public class SyncCallEmissionEndToEndTests : TestKit, IClassFixture - Sys.ActorOf(Props.Create(() => new AuditLogIngestActor( - repo, + private IActorRef CreateIngestActor(IAuditLogRepository repo) + { + // Register row 61: the actor's repository-taking test ctor is gone; the fixture + // supplies a stub provider that hands this repository out of every scope. + var provider = TestSupport.StubAuditRepositoryProvider.Build(repo); + return Sys.ActorOf(Props.Create(() => new AuditLogIngestActor( + provider, NullLogger.Instance))); + } private IActorRef CreateTelemetryActor( ISiteAuditQueue queue, diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/TestSupport/StubAuditRepositoryProvider.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/TestSupport/StubAuditRepositoryProvider.cs new file mode 100644 index 00000000..fb3d27da --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/TestSupport/StubAuditRepositoryProvider.cs @@ -0,0 +1,41 @@ +using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.TestSupport; + +/// +/// Deferred-work register row 61 — the replacement for AuditLogIngestActor's deleted +/// repository-taking "test-mode" constructor. +/// +/// +/// That constructor cached a scoped repository on an actor field for the actor's whole +/// lifetime. Nothing in production selected it, but the shape was one call site away from +/// re-introducing a captive-dependency bug, so it had to be allowlisted in the Host tests' +/// CaptiveDependencyGuard. The actor now has a single +/// constructor; a fixture that wants a concrete repository builds one of these instead. +/// +/// +/// +/// Behaviour is unchanged for the ingest path: the old ctor passed redactor: null and +/// failureCounter: null, and a provider with neither registered resolves both to null +/// too — so the actor still falls back to SafeDefaultAuditRedactor and still logs +/// without a health counter. The one deliberate difference is that +/// IngestCachedTelemetryCommand now takes the real dual-write handler rather than the +/// deleted "no dual write" stub; no fixture built on this helper sends that message. +/// +/// +internal static class StubAuditRepositoryProvider +{ + /// + /// Builds a provider whose every scope yields the same audit-log repository instance. + /// + /// Repository instance shared by every scope. + public static ServiceProvider Build(IAuditLogRepository repository) + { + ArgumentNullException.ThrowIfNull(repository); + + return new ServiceCollection() + .AddScoped(_ => repository) + .BuildServiceProvider(); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CaptiveDependencyGuard.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CaptiveDependencyGuard.cs index 8e323481..f95e9c7b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CaptiveDependencyGuard.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CaptiveDependencyGuard.cs @@ -72,28 +72,25 @@ internal static class CaptiveDependencyGuard /// /// /// - /// Both entries below are the same shape: a nullable repository field assigned ONLY by an - /// explicitly test-mode constructor, so the production wiring - /// (AkkaHostedService constructs both actors through their - /// overload) leaves the field null and every handler takes the create-a-scope branch. The - /// test fixtures that do use those constructors are MSSQL-backed TestKit fixtures that - /// drive the actor one message at a time, and SiteCallAuditActor additionally wraps - /// the injected instance in its own SerializedRepository — i.e. the test path - /// already carries a serialization guard for precisely the concurrency hazard row 45 hit. + /// EMPTY, and pinned empty by CaptiveDependencyGuardTests.Allowlist_IsEmpty. /// /// - /// They are allowlisted rather than removed because deleting a test-mode constructor is a - /// larger change than this row justifies, and rather than left unflagged because the shape - /// is genuinely one constructor call away from being row 45 again: if a production call - /// site ever switched to the repository overload, the actor would hold a scoped EF-backed - /// repository for its whole lifetime. That residual risk is recorded, not dismissed. + /// It previously held two entries — AuditLogIngestActor._injectedRepository and + /// SiteCallAuditActor._injectedRepository — nullable repository fields assigned only + /// by explicitly test-mode constructors. They were safe solely because no production call + /// site picked those overloads, which left the row-45 captive-dependency bug one + /// constructor call away. Register row 61 deleted the constructors instead: both actors now + /// have a single constructor, the fields are gone, and the + /// test fixtures build stub providers that hand a repository out of a scope. With the shape + /// removed there is nothing left to exempt. + /// + /// + /// Keep it empty. A new entry means someone reintroduced a scoped service cached on an + /// actor field; prefer fixing the actor. /// /// - internal static readonly IReadOnlySet Allowlist = new HashSet(StringComparer.Ordinal) - { - "ZB.MOM.WW.ScadaBridge.AuditLog.Central.AuditLogIngestActor._injectedRepository", - "ZB.MOM.WW.ScadaBridge.SiteCallAudit.SiteCallAuditActor._injectedRepository", - }; + internal static readonly IReadOnlySet Allowlist = + new HashSet(StringComparer.Ordinal); /// /// Scans the given actor types for instance fields holding captive scope state. diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CaptiveDependencyGuardTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CaptiveDependencyGuardTests.cs index 1c8419dc..57ddb5ec 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CaptiveDependencyGuardTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CaptiveDependencyGuardTests.cs @@ -166,38 +166,65 @@ public class CaptiveDependencyGuardSelfTests } /// - /// The allowlist is an escape hatch, so it is itself pinned: every entry must name a - /// field that still exists and would still be flagged. An entry that has stopped matching - /// is either a stale exemption or — worse — a typo silently covering nothing. + /// Register row 61: the allowlist is now EMPTY and must stay that way. + /// + /// + /// This replaces the former staleness self-test, which checked that each entry still named + /// a field the scanner would flag. Its two entries — AuditLogIngestActor and + /// SiteCallAuditActor's _injectedRepository — covered repository fields + /// assigned only by test-mode constructors; those constructors have been deleted and the + /// fixtures fold onto the IServiceProvider overload, so there is no longer a field + /// to exempt. An empty allowlist is a strictly stronger statement than a correct one: + /// nothing is exempt, so nothing can rot. + /// /// [Fact] - public void Allowlist_EntriesAllStillMatchARealFlaggedField() + public void Allowlist_IsEmpty_NoActorExemptFromTheCaptiveDependencyScan() { - var actorTypes = CaptiveDependencyGuard.ProductionActorTypes(); - - // Scan with the allowlist disabled against a scoped set wide enough to include the - // repositories the allowlisted fields hold. - var everyScopedType = CaptiveDependencyGuard.ScopedTypes(BuildCentralishScopedProbe()); - var unfiltered = CaptiveDependencyGuard.Scan(actorTypes, everyScopedType, applyAllowlist: false); - - var flagged = unfiltered - .Select(v => $"{v.ActorType.FullName}.{v.FieldName}") - .ToHashSet(StringComparer.Ordinal); - - foreach (var entry in CaptiveDependencyGuard.Allowlist) - { - Assert.True( - flagged.Contains(entry), - $"Allowlist entry '{entry}' no longer matches a flagged field. Remove it — a stale " + - "exemption is indistinguishable from an exemption that never worked."); - } + Assert.True( + CaptiveDependencyGuard.Allowlist.Count == 0, + "The captive-dependency allowlist must stay empty (register row 61). Entries present: " + + string.Join(", ", CaptiveDependencyGuard.Allowlist) + + ". An entry means an actor caches a scoped service on a field — fix the actor " + + "(fold test fixtures onto the IServiceProvider overload) rather than exempting it."); } /// - /// A minimal descriptor set naming the two repository types the allowlisted fields hold, - /// so does not need to boot a - /// host. Both are registered Scoped by AddConfigurationDatabase; the - /// role-scan tests below verify that against the real container. + /// Register row 61 (the other half of the pin): with the allowlist empty, the two actors + /// that used to need exemptions must now scan CLEAN against the scoped repository types + /// they resolve. This is what makes the empty allowlist meaningful — it proves the entries + /// were removed because the fields are gone, not because the scan stopped looking. + /// + [Fact] + public void FormerlyAllowlistedActors_NowScanClean() + { + var actorTypes = CaptiveDependencyGuard.ProductionActorTypes(); + var everyScopedType = CaptiveDependencyGuard.ScopedTypes(BuildCentralishScopedProbe()); + + // Allowlist disabled, so a surviving _injectedRepository-shaped field would still show. + var unfiltered = CaptiveDependencyGuard.Scan(actorTypes, everyScopedType, applyAllowlist: false); + + var offenders = unfiltered + .Where(v => v.ActorType.Name is "AuditLogIngestActor" or "SiteCallAuditActor") + .Select(v => v.ToString()) + .ToList(); + + Assert.True(offenders.Count == 0, + "AuditLogIngestActor / SiteCallAuditActor must hold no scoped repository field " + + "(register row 61). Found: " + string.Join(" | ", offenders)); + + // Sanity: the scan really did examine those two actors, so the clean result above is + // evidence rather than an empty walk. + Assert.Contains(actorTypes, t => t.Name == "AuditLogIngestActor"); + Assert.Contains(actorTypes, t => t.Name == "SiteCallAuditActor"); + } + + /// + /// A minimal descriptor set naming the two repository types the deleted test-mode + /// constructors used to cache, so does + /// not need to boot a host. Both are registered Scoped by + /// AddConfigurationDatabase; the role-scan tests below verify that against the real + /// container. /// /// The probe descriptors. private static IServiceCollection BuildCentralishScopedProbe() diff --git a/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/AuditLog/SiteAuditPushFlowTests.cs b/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/AuditLog/SiteAuditPushFlowTests.cs index abd62283..8b9cc57b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/AuditLog/SiteAuditPushFlowTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/AuditLog/SiteAuditPushFlowTests.cs @@ -149,11 +149,16 @@ public class SiteAuditPushFlowTests : TestKit public async Task SiteAuditEvent_DrainsToCentral_AndFlipsSiteRowToForwarded() { // ── Central side ────────────────────────────────────────────────── - // Real AuditLogIngestActor over an in-memory repository (test-mode ctor). + // Real AuditLogIngestActor over an in-memory repository. Register row 61: the actor's + // repository-taking test ctor is gone, so the repository is handed out of a stub + // provider's scope instead of being cached on an actor field. var centralRepo = new InMemoryAuditLogRepository(); + var ingestProvider = new ServiceCollection() + .AddScoped(_ => centralRepo) + .BuildServiceProvider(); var ingestActor = Sys.ActorOf(Props.Create(() => new ZB.MOM.WW.ScadaBridge.AuditLog.Central.AuditLogIngestActor( - centralRepo, + (IServiceProvider)ingestProvider, NullLogger.Instance))); // Real CentralCommunicationActor. It is NOT on the audit path any more (the diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditActorTests.cs index dc401484..e08ef198 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditActorTests.cs @@ -73,11 +73,19 @@ public class SiteCallAuditActorTests : TestKit, IClassFixture - Sys.ActorOf(Props.Create(() => new SiteCallAuditActor( - repository, - NullLogger.Instance, - options))); + ISiteCallAuditRepository repository, SiteCallAuditOptions? options = null) + { + // Register row 61: the actor's repository-taking test ctor is gone. This MSSQL-backed + // fixture supplies a stub provider that yields the same repository from every scope, + // serialized (one DbContext cannot service concurrent operations) — see + // StubRepositoryProvider. No reconciliation collaborators are registered, so the + // reconciliation tick stays disarmed exactly as it did under the old ctor. + var provider = TestSupport.StubRepositoryProvider.Build(repository); + return Sys.ActorOf(Props.Create(() => new SiteCallAuditActor( + provider, + options ?? new SiteCallAuditOptions(), + NullLogger.Instance))); + } [SkippableFact] public async Task Receive_UpsertSiteCallCommand_Persists_Replies_Accepted() diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditPurgeTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditPurgeTests.cs index d6114fe0..d4e8e524 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditPurgeTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditPurgeTests.cs @@ -102,13 +102,17 @@ public class SiteCallAuditPurgeTests : TestKit public Task> ComputePerNodeKpisAsync(DateTime a, DateTime b, CancellationToken ct = default) => Task.FromResult>(Array.Empty()); } - private IActorRef CreateActor(ISiteCallAuditRepository repo, SiteCallAuditOptions options) => - Sys.ActorOf(Props.Create(() => new SiteCallAuditActor( - repo, - new EmptyEnumerator(), - new NoOpPullClient(), - NullLogger.Instance, - options))); + private IActorRef CreateActor(ISiteCallAuditRepository repo, SiteCallAuditOptions options) + { + // Register row 61: the actor's repository-taking test ctor is gone. The fixture now + // supplies a stub provider that hands the recording repo out of every scope. + var provider = TestSupport.StubRepositoryProvider.Build( + repo, new EmptyEnumerator(), new NoOpPullClient()); + return Sys.ActorOf(Props.Create(() => new SiteCallAuditActor( + provider, + options, + NullLogger.Instance))); + } // --------------------------------------------------------------------- // 1. PurgeTick_CallsPurgeTerminal_WithRetentionThreshold diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditReconciliationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditReconciliationTests.cs index e131c40a..b94910fc 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditReconciliationTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallAuditReconciliationTests.cs @@ -238,13 +238,18 @@ public class SiteCallAuditReconciliationTests : TestKit ISiteEnumerator sites, IPullSiteCallsClient client, ISiteCallAuditRepository repo, - SiteCallAuditOptions options) => - Sys.ActorOf(Props.Create(() => new SiteCallAuditActor( - repo, - sites, - client, - NullLogger.Instance, - options))); + SiteCallAuditOptions options) + { + // Register row 61: the actor's repository+collaborators test ctor is gone. The + // collaborators now reach it the way production supplies them — resolved from the + // provider — and the repository comes out of a scope, serialized because this fixture + // deliberately shares one instance across every scope. + var provider = TestSupport.StubRepositoryProvider.Build(repo, sites, client); + return Sys.ActorOf(Props.Create(() => new SiteCallAuditActor( + provider, + options, + NullLogger.Instance))); + } // --------------------------------------------------------------------- // 1. AbsentRow_PulledFromSite_IsUpserted @@ -348,28 +353,30 @@ public class SiteCallAuditReconciliationTests : TestKit } // --------------------------------------------------------------------- - // 4. RepoOnly test ctor does NOT start the reconciliation tick + // 4. A composition root without the reconciliation collaborators does NOT + // start the reconciliation tick // --------------------------------------------------------------------- [Fact] - public void TestCtor_RepositoryOnly_DoesNotStartReconciliationTick() + public void NoReconciliationCollaboratorsRegistered_DoesNotStartReconciliationTick() { - // The repo-only test ctor (used by the MSSQL-backed actor tests) injects - // no client/enumerator, so the tick must be gated OFF — otherwise those - // tests would fire phantom pulls. Build the actor via that ctor and - // confirm no pull ever happens. We can't observe a non-event directly, - // so we share a ScriptedPullClient with an isolated actor that DOES run - // the tick to bound the wait, then assert the repo-only actor's client - // (a separate instance) recorded nothing. + // Register row 61: this used to be phrased as "the repo-only test ctor gates the tick + // off". With that ctor deleted, the same guarantee is expressed the way production + // actually reaches it — a composition root that registers NEITHER IPullSiteCallsClient + // NOR ISiteEnumerator leaves the tick disarmed (GetService returns null for both), so + // the MSSQL-backed actor tests fire no phantom pulls. We can't observe a non-event + // directly, so we run an isolated actor that DOES tick to bound the wait, then assert + // this actor upserted nothing on its own. var repo = new RecordingRepo(); + var bareProvider = TestSupport.StubRepositoryProvider.Build(repo); Sys.ActorOf(Props.Create(() => new SiteCallAuditActor( - repo, - NullLogger.Instance, - FastTickOptions()))); + bareProvider, + FastTickOptions(), + NullLogger.Instance))); - // Run a parallel actor with the full reconciliation ctor and a fast + // Run a parallel actor whose provider DOES carry the collaborators, with a fast // tick; once IT has pulled we know enough wall-clock elapsed that the - // repo-only actor would have ticked too, had it been wired. + // collaborator-less actor would have ticked too, had it been wired. var liveClient = new ScriptedPullClient(); var liveRepo = new RecordingRepo(); CreateActor( @@ -383,8 +390,8 @@ public class SiteCallAuditReconciliationTests : TestKit duration: TimeSpan.FromSeconds(3), interval: TimeSpan.FromMilliseconds(50)); - // The repo-only actor never reconciles: it has no client to pull with, - // so it upserts nothing on its own. + // The collaborator-less actor never reconciles: it has no client to pull + // with, so it upserts nothing on its own. Assert.Equal(0, repo.UpsertCallCount); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallRelayTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallRelayTests.cs index ddea1ce7..a5fe773f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallRelayTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/SiteCallRelayTests.cs @@ -64,10 +64,12 @@ public class SiteCallRelayTests : TestKit private IActorRef CreateActor(IActorRef centralComm) { var options = new SiteCallAuditOptions { RelayTimeout = TimeSpan.FromMilliseconds(500) }; + // Register row 61: stub provider in place of the deleted repository ctor. + var provider = TestSupport.StubRepositoryProvider.Build(new ThrowingRepository()); var actor = Sys.ActorOf(Props.Create(() => new SiteCallAuditActor( - new ThrowingRepository(), - NullLogger.Instance, - options))); + provider, + options, + NullLogger.Instance))); actor.Tell(new RegisterCentralCommunication(centralComm)); return actor; } @@ -202,10 +204,11 @@ public class SiteCallRelayTests : TestKit // No RegisterCentralCommunication — the actor has no transport to reach // any site, so the only honest answer is "unreachable". var options = new SiteCallAuditOptions { RelayTimeout = TimeSpan.FromMilliseconds(500) }; + var provider = TestSupport.StubRepositoryProvider.Build(new ThrowingRepository()); var actor = Sys.ActorOf(Props.Create(() => new SiteCallAuditActor( - new ThrowingRepository(), - NullLogger.Instance, - options))); + provider, + options, + NullLogger.Instance))); actor.Tell(new RetrySiteCallRequest("corr-7", Guid.NewGuid(), "site-north")); @@ -286,11 +289,15 @@ public class SiteCallRelayTests : TestKit { var writer = new RecordingCentralAuditWriter(); var options = new SiteCallAuditOptions { RelayTimeout = TimeSpan.FromMilliseconds(500) }; - var actor = Sys.ActorOf(Props.Create(() => new SiteCallAuditActor( + // The audit writer now reaches the actor through the provider (production resolves it + // the same way, from the root provider) rather than through a ctor parameter. + var provider = TestSupport.StubRepositoryProvider.Build( new RowReturningRepository(row), - NullLogger.Instance, + auditWriter: (ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services.ICentralAuditWriter)writer); + var actor = Sys.ActorOf(Props.Create(() => new SiteCallAuditActor( + provider, options, - (ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services.ICentralAuditWriter)writer))); + NullLogger.Instance))); actor.Tell(new RegisterCentralCommunication(centralComm)); return (actor, writer); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/TestSupport/StubRepositoryProvider.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/TestSupport/StubRepositoryProvider.cs new file mode 100644 index 00000000..83e1269e --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests/TestSupport/StubRepositoryProvider.cs @@ -0,0 +1,162 @@ +using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.ScadaBridge.AuditLog.Central; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Audit; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Types; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit; + +namespace ZB.MOM.WW.ScadaBridge.SiteCallAudit.Tests.TestSupport; + +/// +/// Deferred-work register row 61 — the replacement for SiteCallAuditActor's deleted +/// repository-taking "test-mode" constructors. +/// +/// +/// Those constructors cached a scoped repository on an actor field for the actor's whole +/// lifetime. Nothing in production selected them, but the shape was one call site away from +/// re-introducing a captive-dependency bug, so it had to be allowlisted in the Host tests' +/// CaptiveDependencyGuard. The actor now has a single +/// constructor; a fixture that wants a concrete repository builds one of these instead. +/// +/// +/// +/// The actor's contract is unchanged and unweakened — it still resolves per scope and holds +/// nothing. Handing the SAME instance out of every scope is now the FIXTURE's explicit +/// choice, which is also why the serializing decorator below lives here rather than in +/// product code: it exists solely to compensate for that choice. +/// +/// +internal static class StubRepositoryProvider +{ + /// + /// Builds a provider whose every scope yields the same repository instance, wrapped in + /// , plus any collaborators supplied. + /// + /// Repository instance shared by every scope. + /// Optional site enumerator; omit to leave the reconciliation tick disarmed. + /// Optional pull client; omit to leave the reconciliation tick disarmed. + /// Optional central direct-write audit sink for relay actions. + /// + /// False registers the repository unwrapped — for fixtures asserting on call ordering or + /// identity where the decorator would get in the way. + /// + public static ServiceProvider Build( + ISiteCallAuditRepository repository, + ISiteEnumerator? siteEnumerator = null, + IPullSiteCallsClient? pullClient = null, + ICentralAuditWriter? auditWriter = null, + bool serialize = true) + { + ArgumentNullException.ThrowIfNull(repository); + + var shared = serialize ? new SerializingSiteCallAuditRepository(repository) : repository; + + var services = new ServiceCollection(); + services.AddScoped(_ => shared); + + if (siteEnumerator is not null) + { + services.AddSingleton(siteEnumerator); + } + + if (pullClient is not null) + { + services.AddSingleton(pullClient); + } + + if (auditWriter is not null) + { + services.AddSingleton(auditWriter); + } + + return services.BuildServiceProvider(); + } +} + +/// +/// Serializing wrapper for a repository instance shared across every DI scope in a test. +/// One call at a time, in arrival order. +/// +/// +/// +/// In production every consumer of SiteCallAuditActor's repository gets its OWN DI +/// scope — one per message, one per background pass — so the off-mailbox reconciliation and +/// purge passes can safely run alongside mailbox handlers: separate scopes mean separate +/// DbContexts. A fixture that hands ONE instance (typically wrapping one +/// DbContext) to every scope breaks that assumption, and DbContext forbids +/// concurrent operations — "A second operation was started on this context instance" is a +/// hard fault, and the interleaving is nondeterministic, so it surfaces as a flaky test +/// rather than a reliable one. +/// +/// +/// Serializing at the CALL, not around the whole pass, is deliberate: it removes the +/// concurrency hazard while preserving the property the off-mailbox passes exist for — a long +/// drain (blocked in a network pull, holding no repository call) still lets ingest, query and +/// KPI messages be answered. Suspending the mailbox for the duration of a pass would have +/// been simpler and would have invalidated exactly those regression tests. +/// +/// +/// This type used to live inside the product actor, reachable only from its test +/// constructors. It moved here with register row 61: production never constructed it, and the +/// hazard it guards is created by the fixture, not by the actor. +/// +/// +internal sealed class SerializingSiteCallAuditRepository : ISiteCallAuditRepository +{ + private readonly ISiteCallAuditRepository _inner; + private readonly SemaphoreSlim _gate = new(1, 1); + + public SerializingSiteCallAuditRepository(ISiteCallAuditRepository inner) => _inner = inner; + + public Task UpsertAsync(SiteCall siteCall, CancellationToken ct = default) => + RunAsync(() => _inner.UpsertAsync(siteCall, ct)); + + public Task GetAsync(TrackedOperationId id, CancellationToken ct = default) => + RunAsync(() => _inner.GetAsync(id, ct)); + + public Task> QueryAsync( + SiteCallQueryFilter filter, SiteCallPaging paging, CancellationToken ct = default) => + RunAsync(() => _inner.QueryAsync(filter, paging, ct)); + + public Task PurgeTerminalAsync(DateTime olderThanUtc, CancellationToken ct = default) => + RunAsync(() => _inner.PurgeTerminalAsync(olderThanUtc, ct)); + + public Task ComputeKpisAsync( + DateTime stuckCutoffUtc, DateTime deliveredSinceUtc, CancellationToken ct = default) => + RunAsync(() => _inner.ComputeKpisAsync(stuckCutoffUtc, deliveredSinceUtc, ct)); + + public Task> ComputePerSiteKpisAsync( + DateTime stuckCutoffUtc, DateTime deliveredSinceUtc, CancellationToken ct = default) => + RunAsync(() => _inner.ComputePerSiteKpisAsync(stuckCutoffUtc, deliveredSinceUtc, ct)); + + public Task> ComputePerNodeKpisAsync( + DateTime stuckCutoffUtc, DateTime deliveredSinceUtc, CancellationToken ct = default) => + RunAsync(() => _inner.ComputePerNodeKpisAsync(stuckCutoffUtc, deliveredSinceUtc, ct)); + + private async Task RunAsync(Func call) + { + await _gate.WaitAsync().ConfigureAwait(false); + try + { + await call().ConfigureAwait(false); + } + finally + { + _gate.Release(); + } + } + + private async Task RunAsync(Func> call) + { + await _gate.WaitAsync().ConfigureAwait(false); + try + { + return await call().ConfigureAwait(false); + } + finally + { + _gate.Release(); + } + } +}