fix(auditlog,health): harden hosted-service shutdown against disposed CTS
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
This commit is contained in:
@@ -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;
|
||||
}
|
||||
|
||||
@@ -147,16 +155,44 @@ public sealed class AuditLogPartitionMaintenanceService : IHostedService, IDispo
|
||||
/// <param name="ct">Cancellation token provided by the host (unused — the internal CTS is cancelled directly).</param>
|
||||
/// <returns>The background loop task, or a completed task if the loop was never started.</returns>
|
||||
public Task StopAsync(CancellationToken ct)
|
||||
{
|
||||
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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Disposes the internal <see cref="CancellationTokenSource"/> used to stop the maintenance loop.
|
||||
/// Cancels and disposes the internal <see cref="CancellationTokenSource"/> used to
|
||||
/// stop the maintenance loop.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Cancels before disposing so the loop is always signalled, even when the host
|
||||
/// disposes the container without having driven <see cref="StopAsync"/> first.
|
||||
/// It also keeps the loop's pending <c>Task.Delay(interval, token)</c> safe: an
|
||||
/// already-cancelled token makes Delay complete as cancelled rather than register
|
||||
/// a callback against a dead source and throw.
|
||||
/// </remarks>
|
||||
public void Dispose()
|
||||
{
|
||||
try
|
||||
{
|
||||
_cts?.Cancel();
|
||||
}
|
||||
catch (ObjectDisposedException)
|
||||
{
|
||||
// Already disposed — Dispose is idempotent.
|
||||
}
|
||||
|
||||
_cts?.Dispose();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -102,12 +102,20 @@ 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)
|
||||
{
|
||||
try
|
||||
{
|
||||
// First tick runs immediately so the very first health report after
|
||||
// process start carries a real backlog snapshot — without this the
|
||||
@@ -128,6 +136,14 @@ public sealed class SiteAuditBacklogReporter : IHostedService, IDisposable
|
||||
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.
|
||||
}
|
||||
}
|
||||
|
||||
private async Task SafeProbeAsync(CancellationToken ct)
|
||||
{
|
||||
@@ -154,14 +170,36 @@ public sealed class SiteAuditBacklogReporter : IHostedService, IDisposable
|
||||
/// <param name="ct">Cancellation token (not used; the internal CTS governs shutdown).</param>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
public Task StopAsync(CancellationToken ct)
|
||||
{
|
||||
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;
|
||||
}
|
||||
|
||||
/// <summary>Releases the internal <see cref="CancellationTokenSource"/> used to stop the polling loop.</summary>
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -130,14 +136,36 @@ public sealed class SiteAuditRetentionService : IHostedService, IDisposable
|
||||
/// <param name="ct">Cancellation token (not used; the internal CTS governs shutdown).</param>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
public Task StopAsync(CancellationToken ct)
|
||||
{
|
||||
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;
|
||||
}
|
||||
|
||||
/// <summary>Releases the internal <see cref="CancellationTokenSource"/>.</summary>
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -133,14 +139,36 @@ public sealed class SiteEventLogFailureCountReporter : IHostedService, IDisposab
|
||||
/// <param name="ct">Cancellation token (not used; the internal CTS governs shutdown).</param>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
public Task StopAsync(CancellationToken ct)
|
||||
{
|
||||
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;
|
||||
}
|
||||
|
||||
/// <summary>Releases the internal <see cref="CancellationTokenSource"/> used to stop the polling loop.</summary>
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
||||
+36
@@ -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<IServiceScopeFactory>(),
|
||||
opts,
|
||||
NullLogger<AuditLogPartitionMaintenanceService>.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();
|
||||
}
|
||||
}
|
||||
|
||||
+55
@@ -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<ISiteAuditQueue>();
|
||||
queue.GetBacklogStatsAsync(Arg.Any<CancellationToken>())
|
||||
.Returns(ci => BlockUntilCancelledAsync(probeStarted, ci.Arg<CancellationToken>()));
|
||||
|
||||
var reporter = new SiteAuditBacklogReporter(
|
||||
queue,
|
||||
Substitute.For<ISiteHealthCollector>(),
|
||||
NullLogger<SiteAuditBacklogReporter>.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<SiteAuditBacklogSnapshot> 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()
|
||||
{
|
||||
|
||||
@@ -19,6 +19,30 @@ namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site;
|
||||
/// </summary>
|
||||
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<SiteAuditRetentionService>.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()
|
||||
{
|
||||
|
||||
+21
@@ -10,6 +10,27 @@ namespace ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests;
|
||||
/// </summary>
|
||||
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<SiteEventLogFailureCountReporter>.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()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user