fix(alarms): background tasks register before they start; volatile _disposed

This commit is contained in:
Joseph Doherty
2026-08-17 04:04:45 -04:00
parent cda82c074e
commit 5ae430237f
2 changed files with 221 additions and 13 deletions
@@ -1659,6 +1659,134 @@ public sealed class ScriptedAlarmEngineTests
await WaitForAsync(() => eng.GetState("A")!.Active == AlarmActiveState.Active, timeoutMs: 5000);
}
// -------------------------------------------------------------------------
// TrackBackgroundTask: fire-and-forget work must be REGISTERED BEFORE IT STARTS.
//
// The old signature took a Task, so the caller evaluated the argument — starting
// the work — and only then called in to register it. Dispose snapshots the
// in-flight set under its lock and synchronously awaits the snapshot, so a Dispose
// landing in that start→Add window drained a set that did not yet contain a
// genuinely running task, and returned while its SaveAsync was still in flight.
// The factory form removes the window rather than shrinking it.
// -------------------------------------------------------------------------
/// <summary>Verifies the background task is registered for the Dispose drain BEFORE the factory
/// that starts it is invoked, so no snapshot can ever miss a task that runs.</summary>
[Fact]
public void Background_work_is_registered_before_the_factory_starts_it()
{
var up = new FakeUpstream();
using var eng = Build(up, out _);
var countSeenInsideFactory = -1;
var gate = new TaskCompletionSource();
eng.TrackBackgroundTaskForTest(() =>
{
// Sampled from inside the factory's own synchronous body — i.e. before the
// work's Task even exists, which is precisely the window the old shape left open.
countSeenInsideFactory = eng.InFlightCountForTest;
return gate.Task;
});
// Release BEFORE asserting: on the falsification run (registration moved after the
// factory) the assertion throws, and `using` would then Dispose into a drain that
// waits on a gate this method never reached — a hang instead of a failure.
gate.SetResult();
countSeenInsideFactory.ShouldBe(1,
"registration must happen before the factory starts the work, or Dispose can drain " +
"past a task that is genuinely in flight");
}
/// <summary>Verifies that after Dispose the factory form declines: no factory is invoked and
/// nothing is registered, so a disposed engine can never acquire new background work.</summary>
[Fact]
public void Disposed_engine_declines_to_start_new_background_work()
{
var up = new FakeUpstream();
var eng = Build(up, out _);
eng.Dispose();
var invoked = false;
eng.TrackBackgroundTaskForTest(() => { invoked = true; return Task.CompletedTask; });
invoked.ShouldBeFalse("a disposed engine must not start new background work");
eng.InFlightCountForTest.ShouldBe(0, "and must not register anything for a drain that already ran");
}
/// <summary>Verifies Dispose blocks on work started through the factory form, and that the
/// registration is released once that work completes.</summary>
[Fact]
public async Task Dispose_awaits_background_work_started_through_the_factory()
{
var up = new FakeUpstream();
var eng = Build(up, out _);
var gate = new TaskCompletionSource();
eng.TrackBackgroundTaskForTest(() => gate.Task);
eng.InFlightCountForTest.ShouldBe(1);
var disposeTask = Task.Run(() => eng.Dispose());
var raced = await Task.WhenAny(disposeTask, Task.Delay(300));
raced.ShouldNotBe(disposeTask, "Dispose must not return while registered background work is still running");
gate.SetResult();
await disposeTask.WaitAsync(TimeSpan.FromSeconds(5));
eng.InFlightCountForTest.ShouldBe(0, "completed work must unregister itself");
}
/// <summary>Verifies work whose task is already complete when the factory returns still
/// unregisters cleanly and leaves Dispose with nothing to wait on.</summary>
[Fact]
public void Already_completed_background_work_unregisters_and_leaves_Dispose_free()
{
var up = new FakeUpstream();
var eng = Build(up, out _);
eng.TrackBackgroundTaskForTest(() => Task.CompletedTask);
eng.InFlightCountForTest.ShouldBe(0,
"the self-removal continuation runs inline on an already-completed task");
Should.NotThrow(() => eng.Dispose());
}
/// <summary>Verifies a factory that throws synchronously surfaces the throw to its caller (the
/// behaviour the old evaluate-at-the-call-site shape had) without wedging Dispose on a
/// placeholder that nothing will ever complete.</summary>
[Fact]
public async Task A_factory_that_throws_synchronously_rethrows_and_does_not_wedge_Dispose()
{
var up = new FakeUpstream();
var eng = Build(up, out _);
Should.Throw<InvalidOperationException>(() => eng.TrackBackgroundTaskForTest(
() => throw new InvalidOperationException("factory failed (test)")));
eng.InFlightCountForTest.ShouldBe(0, "work that never started must not be left registered");
await Task.Run(() => eng.Dispose()).WaitAsync(TimeSpan.FromSeconds(5));
}
/// <summary>Verifies a faulted background task is still drained by Dispose and its failure is
/// swallowed there (logged at debug), matching the pre-existing shutdown contract.</summary>
[Fact]
public async Task A_faulted_background_task_is_drained_by_Dispose_without_throwing()
{
var up = new FakeUpstream();
var eng = Build(up, out _);
var gate = new TaskCompletionSource();
eng.TrackBackgroundTaskForTest(() => gate.Task);
// Fault it while Dispose is actually blocked on the drain, so the WhenAll observes
// the failure rather than finding an already-unregistered task.
var disposeTask = Task.Run(() => Should.NotThrow(() => eng.Dispose()));
await Task.Delay(200);
gate.SetException(new InvalidOperationException("background failed (test)"));
await disposeTask.WaitAsync(TimeSpan.FromSeconds(5));
eng.InFlightCountForTest.ShouldBe(0);
}
/// <summary>
/// A clock that, once armed, re-dirties an alarm by pushing to the upstream and then throws —
/// driving a deterministic chain of drain-latch handovers whose every step is synchronous —