From 9110a4eb01b7ee421d9b07a657b254e76ccffe6f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 16 Jul 2026 23:31:19 -0400 Subject: [PATCH] fix(auditlog,health): harden hosted-service shutdown against disposed CTS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The host does not guarantee IHostedService.StopAsync is driven before the DI container is disposed — WebApplicationFactory's teardown reaches Dispose first — so cancelling the internal CTS from StopAsync threw ObjectDisposedException and aborted the host's whole shutdown sequence. Four services shared the same copy-pasted lifecycle and the same two races: StopAsync cancelling an already- disposed CTS, and StartAsync reading _cts.Token lazily inside the Task.Run lambda, which faults the loop task the host awaits when Dispose wins that race. Each service now captures the token on the caller's thread, tolerates a disposed CTS, and cancels-before-disposing so the loop is always signalled and its pending Task.Delay sees a cancelled token rather than a dead source. SiteAuditBacklogReporter also gains the outer OperationCanceledException guard its sibling SiteAuditRetentionService already carried (arch-review 04 R2, R7), without which a shutdown landing mid-probe threw TaskCanceledException out of Host.StopAsync. Surfaced while verifying the Gitea #15 test-harness fix: in Host.Tests the aborted teardown skipped the fixture's env-var restore, contaminating every later test in the run. Refs: Gitea #15 --- .../AuditLogPartitionMaintenanceService.cs | 44 ++++++++++- .../Site/SiteAuditBacklogReporter.cs | 74 ++++++++++++++----- .../Site/SiteAuditRetentionService.cs | 34 ++++++++- .../SiteEventLogFailureCountReporter.cs | 34 ++++++++- ...uditLogPartitionMaintenanceServiceTests.cs | 36 +++++++++ .../SiteAuditBacklogReporterCadenceTests.cs | 55 ++++++++++++++ .../Site/SiteAuditRetentionServiceTests.cs | 24 ++++++ .../SiteEventLogFailureCountReporterTests.cs | 21 ++++++ 8 files changed, 294 insertions(+), 28 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogPartitionMaintenanceService.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogPartitionMaintenanceService.cs index 3a113436..8243d2ad 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogPartitionMaintenanceService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogPartitionMaintenanceService.cs @@ -86,8 +86,16 @@ public sealed class AuditLogPartitionMaintenanceService : IHostedService, IDispo // Linked CTS lets StopAsync's cancellation AND the host's shutdown // token both terminate the loop; either side firing aborts the // pending Task.Delay. - _cts = CancellationTokenSource.CreateLinkedTokenSource(ct); - _loop = Task.Run(() => RunLoopAsync(_cts.Token)); + var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); + _cts = cts; + + // Read Token here, on the caller's thread, rather than inside the lambda: + // the lambda runs whenever the thread pool gets to it, so a Dispose landing + // first would make the deferred _cts.Token read throw ObjectDisposedException + // and fault the loop task — which StopAsync hands to the host to await. The + // token struct stays usable once captured. + var token = cts.Token; + _loop = Task.Run(() => RunLoopAsync(token), CancellationToken.None); return Task.CompletedTask; } @@ -148,15 +156,43 @@ public sealed class AuditLogPartitionMaintenanceService : IHostedService, IDispo /// The background loop task, or a completed task if the loop was never started. public Task StopAsync(CancellationToken ct) { - _cts?.Cancel(); + try + { + _cts?.Cancel(); + } + catch (ObjectDisposedException) + { + // Stop-after-Dispose is a legal ordering — WebApplicationFactory's + // teardown disposes the container before driving StopAsync. Dispose + // already cancelled the loop, so there is nothing left to signal, and + // letting this escape would abort the host's whole shutdown sequence. + } + return _loop ?? Task.CompletedTask; } /// - /// Disposes the internal used to stop the maintenance loop. + /// Cancels and disposes the internal used to + /// stop the maintenance loop. /// + /// + /// Cancels before disposing so the loop is always signalled, even when the host + /// disposes the container without having driven first. + /// It also keeps the loop's pending Task.Delay(interval, token) safe: an + /// already-cancelled token makes Delay complete as cancelled rather than register + /// a callback against a dead source and throw. + /// public void Dispose() { + try + { + _cts?.Cancel(); + } + catch (ObjectDisposedException) + { + // Already disposed — Dispose is idempotent. + } + _cts?.Dispose(); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditBacklogReporter.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditBacklogReporter.cs index 9a09a81f..ce1d1ec7 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditBacklogReporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditBacklogReporter.cs @@ -102,30 +102,46 @@ public sealed class SiteAuditBacklogReporter : IHostedService, IDisposable // Linked CTS lets StopAsync's cancellation AND the host's shutdown // token both terminate the loop; either side firing aborts the // pending Task.Delay. - _cts = CancellationTokenSource.CreateLinkedTokenSource(ct); - _loop = Task.Run(() => RunLoopAsync(_cts.Token)); + var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); + _cts = cts; + + // Read Token on the caller's thread, not inside the lambda: the lambda runs + // whenever the thread pool gets to it, so a Dispose landing first would make + // the deferred _cts.Token read throw and fault the loop task the host awaits. + var token = cts.Token; + _loop = Task.Run(() => RunLoopAsync(token), CancellationToken.None); return Task.CompletedTask; } private async Task RunLoopAsync(CancellationToken ct) { - // First tick runs immediately so the very first health report after - // process start carries a real backlog snapshot — without this the - // dashboard would show null for the first 30 s after a deploy. - await SafeProbeAsync(ct).ConfigureAwait(false); - - while (!ct.IsCancellationRequested) + try { - try - { - await Task.Delay(_refreshInterval, ct).ConfigureAwait(false); - } - catch (OperationCanceledException) - { - break; - } - + // First tick runs immediately so the very first health report after + // process start carries a real backlog snapshot — without this the + // dashboard would show null for the first 30 s after a deploy. await SafeProbeAsync(ct).ConfigureAwait(false); + + while (!ct.IsCancellationRequested) + { + try + { + await Task.Delay(_refreshInterval, ct).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + break; + } + + await SafeProbeAsync(ct).ConfigureAwait(false); + } + } + catch (OperationCanceledException) + { + // Shutdown landed mid-probe: SafeProbeAsync rethrows OCE by design so the probe + // aborts promptly, but the loop task must complete CLEANLY — StopAsync hands + // _loop straight to the host, and a canceled task there is shutdown-log noise + // (arch-review 04 round 2, R7). Cancellation here IS the clean exit. } } @@ -155,13 +171,35 @@ public sealed class SiteAuditBacklogReporter : IHostedService, IDisposable /// A task that represents the asynchronous operation. public Task StopAsync(CancellationToken ct) { - _cts?.Cancel(); + try + { + _cts?.Cancel(); + } + catch (ObjectDisposedException) + { + // Stop-after-Dispose is a legal ordering; Dispose already cancelled the + // loop. Letting this escape would abort the host's shutdown sequence. + } + return _loop ?? Task.CompletedTask; } /// Releases the internal used to stop the polling loop. public void Dispose() { + // Cancel before disposing so the loop is always signalled even when the host + // disposes the container without having driven StopAsync first, and so the + // loop's pending Task.Delay(interval, token) sees an already-cancelled token + // rather than registering against a dead source. + try + { + _cts?.Cancel(); + } + catch (ObjectDisposedException) + { + // Already disposed — Dispose is idempotent. + } + _cts?.Dispose(); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditRetentionService.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditRetentionService.cs index 82014323..e60f002d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditRetentionService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditRetentionService.cs @@ -54,8 +54,14 @@ public sealed class SiteAuditRetentionService : IHostedService, IDisposable public Task StartAsync(CancellationToken ct) { // Linked CTS so both StopAsync and the host shutdown token abort the loop. - _cts = CancellationTokenSource.CreateLinkedTokenSource(ct); - _loop = Task.Run(() => RunLoopAsync(_cts.Token)); + var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); + _cts = cts; + + // Read Token on the caller's thread, not inside the lambda: the lambda runs + // whenever the thread pool gets to it, so a Dispose landing first would make + // the deferred _cts.Token read throw and fault the loop task the host awaits. + var token = cts.Token; + _loop = Task.Run(() => RunLoopAsync(token), CancellationToken.None); return Task.CompletedTask; } @@ -131,13 +137,35 @@ public sealed class SiteAuditRetentionService : IHostedService, IDisposable /// A task that represents the asynchronous operation. public Task StopAsync(CancellationToken ct) { - _cts?.Cancel(); + try + { + _cts?.Cancel(); + } + catch (ObjectDisposedException) + { + // Stop-after-Dispose is a legal ordering; Dispose already cancelled the + // loop. Letting this escape would abort the host's shutdown sequence. + } + return _loop ?? Task.CompletedTask; } /// Releases the internal . public void Dispose() { + // Cancel before disposing so the loop is always signalled even when the host + // disposes the container without having driven StopAsync first, and so the + // loop's pending Task.Delay(interval, token) sees an already-cancelled token + // rather than registering against a dead source. + try + { + _cts?.Cancel(); + } + catch (ObjectDisposedException) + { + // Already disposed — Dispose is idempotent. + } + _cts?.Dispose(); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteEventLogFailureCountReporter.cs b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteEventLogFailureCountReporter.cs index dacc7fcc..19de05f3 100644 --- a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteEventLogFailureCountReporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteEventLogFailureCountReporter.cs @@ -85,8 +85,14 @@ public sealed class SiteEventLogFailureCountReporter : IHostedService, IDisposab // Linked CTS lets StopAsync's cancellation AND the host's shutdown // token both terminate the loop; either side firing aborts the // pending Task.Delay. - _cts = CancellationTokenSource.CreateLinkedTokenSource(ct); - _loop = Task.Run(() => RunLoopAsync(_cts.Token)); + var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); + _cts = cts; + + // Read Token on the caller's thread, not inside the lambda: the lambda runs + // whenever the thread pool gets to it, so a Dispose landing first would make + // the deferred _cts.Token read throw and fault the loop task the host awaits. + var token = cts.Token; + _loop = Task.Run(() => RunLoopAsync(token), CancellationToken.None); return Task.CompletedTask; } @@ -134,13 +140,35 @@ public sealed class SiteEventLogFailureCountReporter : IHostedService, IDisposab /// A task that represents the asynchronous operation. public Task StopAsync(CancellationToken ct) { - _cts?.Cancel(); + try + { + _cts?.Cancel(); + } + catch (ObjectDisposedException) + { + // Stop-after-Dispose is a legal ordering; Dispose already cancelled the + // loop. Letting this escape would abort the host's shutdown sequence. + } + return _loop ?? Task.CompletedTask; } /// Releases the internal used to stop the polling loop. public void Dispose() { + // Cancel before disposing so the loop is always signalled even when the host + // disposes the container without having driven StopAsync first, and so the + // loop's pending Task.Delay(interval, token) sees an already-cancelled token + // rather than registering against a dead source. + try + { + _cts?.Cancel(); + } + catch (ObjectDisposedException) + { + // Already disposed — Dispose is idempotent. + } + _cts?.Dispose(); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPartitionMaintenanceServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPartitionMaintenanceServiceTests.cs index b9d879d1..69055d58 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPartitionMaintenanceServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Central/AuditLogPartitionMaintenanceServiceTests.cs @@ -151,4 +151,40 @@ public class AuditLogPartitionMaintenanceServiceTests Assert.Equal(LogLevel.Error, errorEntry.Level); Assert.Equal(1, maintenance.EnsureCallCount); } + + [Fact] + public async Task StopAsync_AfterDispose_DoesNotThrow() + { + // Regression (Gitea #15 follow-up): Dispose tears down the CTS that + // StopAsync cancels, and the host does not guarantee that every + // IHostedService.StopAsync is driven before the DI container is + // disposed — WebApplicationFactory's teardown reaches Dispose first and + // then calls StopAsync. Cancel() on a disposed CTS throws + // ObjectDisposedException, and letting it escape aborts the host's whole + // shutdown sequence (in Host.Tests it stranded the fixture's env-var + // restore, contaminating every later test in the run). Stop-after-Dispose + // must therefore be a no-op, not a throw. + var opts = Options.Create(new AuditLogPartitionMaintenanceOptions + { + IntervalSeconds = 60, + LookaheadMonths = 1, + }); + var maintenance = new RecordingMaintenance(); + var sp = BuildProvider(maintenance); + + var svc = new AuditLogPartitionMaintenanceService( + sp.GetRequiredService(), + opts, + NullLogger.Instance); + + await svc.StartAsync(CancellationToken.None); + + svc.Dispose(); + + // The assertion is the absence of ObjectDisposedException. + await svc.StopAsync(CancellationToken.None); + + // Dispose is also idempotent — the host may reach it twice on the same path. + svc.Dispose(); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditBacklogReporterCadenceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditBacklogReporterCadenceTests.cs index 87a97118..4798aad9 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditBacklogReporterCadenceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditBacklogReporterCadenceTests.cs @@ -1,8 +1,10 @@ +using System.Diagnostics; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using NSubstitute; using ZB.MOM.WW.ScadaBridge.AuditLog.Site; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Types; using ZB.MOM.WW.ScadaBridge.HealthMonitoring; namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site; @@ -23,6 +25,59 @@ public class SiteAuditBacklogReporterCadenceTests explicitInterval, options); + [Fact] + public async Task StopAsync_WhileProbeInFlight_LoopCompletesCleanly() + { + // SafeProbeAsync rethrows OperationCanceledException by design so a shutdown + // aborts the probe promptly — but RunLoopAsync must absorb it, because StopAsync + // hands _loop straight to the host and a canceled task there throws out of + // Host.StopAsync. Mirrors the guard SiteAuditRetentionService already carries + // (arch-review 04 round 2, R7). + var probeStarted = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var queue = Substitute.For(); + queue.GetBacklogStatsAsync(Arg.Any()) + .Returns(ci => BlockUntilCancelledAsync(probeStarted, ci.Arg())); + + var reporter = new SiteAuditBacklogReporter( + queue, + Substitute.For(), + NullLogger.Instance, + TimeSpan.FromHours(1), + null); + + await reporter.StartAsync(CancellationToken.None); + await probeStarted.Task; // the immediate first probe is now in flight + + // The assertion is that awaiting the loop task does not throw. + await reporter.StopAsync(CancellationToken.None); + reporter.Dispose(); + } + + private static async Task BlockUntilCancelledAsync( + TaskCompletionSource started, CancellationToken ct) + { + started.TrySetResult(); + await Task.Delay(Timeout.Infinite, ct); + throw new UnreachableException("the delay above always throws on cancellation"); + } + + [Fact] + public async Task StopAsync_AfterDispose_DoesNotThrow() + { + // Regression (Gitea #15 follow-up): Dispose tears down the CTS StopAsync + // cancels, and the host does not guarantee StopAsync is driven before the DI + // container is disposed. Cancel() on a disposed CTS throws, and letting that + // escape an IHostedService aborts the host's whole shutdown sequence. + var reporter = Create(Options.Create(new SqliteAuditWriterOptions()), TimeSpan.FromHours(1)); + + await reporter.StartAsync(CancellationToken.None); + reporter.Dispose(); + + // The assertion is the absence of ObjectDisposedException. + await reporter.StopAsync(CancellationToken.None); + reporter.Dispose(); + } + [Fact] public void Cadence_ComesFromOptions_WhenConfigured() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditRetentionServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditRetentionServiceTests.cs index cdd4a7df..3a3bd811 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditRetentionServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditRetentionServiceTests.cs @@ -19,6 +19,30 @@ namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site; /// public class SiteAuditRetentionServiceTests { + [Fact] + public async Task StopAsync_AfterDispose_DoesNotThrow() + { + // Regression (Gitea #15 follow-up): Dispose tears down the CTS StopAsync + // cancels, and the host does not guarantee StopAsync is driven before the DI + // container is disposed. Cancel() on a disposed CTS throws, and letting that + // escape an IHostedService aborts the host's whole shutdown sequence. + var queue = new RecordingSiteAuditQueue(); + var options = Options.Create(new SiteAuditRetentionOptions + { + RetentionDays = 7, + PurgeIntervalOverride = TimeSpan.FromMilliseconds(50), + InitialDelay = TimeSpan.Zero, + }); + var svc = new SiteAuditRetentionService(queue, options, NullLogger.Instance); + + await svc.StartAsync(CancellationToken.None); + svc.Dispose(); + + // The assertion is the absence of ObjectDisposedException. + await svc.StopAsync(CancellationToken.None); + svc.Dispose(); + } + [Fact] public async Task Tick_Purges_With_RetentionCutoff() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/SiteEventLogFailureCountReporterTests.cs b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/SiteEventLogFailureCountReporterTests.cs index 632bb60f..c2ac2a9c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/SiteEventLogFailureCountReporterTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/SiteEventLogFailureCountReporterTests.cs @@ -10,6 +10,27 @@ namespace ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests; /// public class SiteEventLogFailureCountReporterTests { + [Fact] + public async Task StopAsync_AfterDispose_DoesNotThrow() + { + // Regression (Gitea #15 follow-up): Dispose tears down the CTS StopAsync + // cancels, and the host does not guarantee StopAsync is driven before the DI + // container is disposed. Cancel() on a disposed CTS throws, and letting that + // escape an IHostedService aborts the host's whole shutdown sequence. + var reporter = new SiteEventLogFailureCountReporter( + failedWriteCountProvider: () => 0L, + collector: new SiteHealthCollector(), + logger: NullLogger.Instance, + refreshInterval: TimeSpan.FromHours(1)); + + await reporter.StartAsync(CancellationToken.None); + reporter.Dispose(); + + // The assertion is the absence of ObjectDisposedException. + await reporter.StopAsync(CancellationToken.None); + reporter.Dispose(); + } + [Fact] public async Task StartAsync_ImmediatelyProbes_FailedWriteCount() {