diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs index 89b5448d..fe75952a 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs @@ -169,7 +169,13 @@ public sealed class ScriptedAlarmEngine : IDisposable private readonly SemaphoreSlim _evalGate = new(1, 1); private Timer? _shelvingTimer; private bool _loaded; - private bool _disposed; + + // volatile: written by Dispose and read unsynchronized from every background + // path (LoadAsync's guard, OnUpstreamChange, the drain loop, ReevaluateAsync / + // ShelvingCheckAsync's post-gate re-checks, TrackBackgroundTask's registration + // check). Without it those reads may be hoisted or cached by the JIT, so a + // pushing thread can keep queueing work against a disposed engine. + private volatile bool _disposed; // Tracks fire-and-forget background work launched by OnUpstreamChange // (DrainDirtyAsync) and RunShelvingCheck (ShelvingCheckAsync). Dispose drains @@ -556,7 +562,7 @@ public sealed class ScriptedAlarmEngine : IDisposable // Started outside the lock: DrainDirtyAsync runs synchronously up to its // first real await, and nothing it does should happen under _dirtyLock. - if (startDrain) TrackBackgroundTask(DrainDirtyAsync(CancellationToken.None)); + if (startDrain) TrackBackgroundTask(() => DrainDirtyAsync(CancellationToken.None)); } /// @@ -663,7 +669,9 @@ public sealed class ScriptedAlarmEngine : IDisposable if (!restart) _drainScheduled = false; } - if (restart) TrackBackgroundTask(Task.Run(() => DrainDirtyAsync(ct))); + // Task.Run stays INSIDE the factory: the queued-not-called handover is what + // keeps the replacement off this unwinding stack (see the Stack bound above). + if (restart) TrackBackgroundTask(() => Task.Run(() => DrainDirtyAsync(ct))); } } } @@ -986,22 +994,80 @@ public sealed class ScriptedAlarmEngine : IDisposable { if (_disposed) return; var ids = _alarms.Keys.ToArray(); - TrackBackgroundTask(ShelvingCheckAsync(ids, CancellationToken.None)); + TrackBackgroundTask(() => ShelvingCheckAsync(ids, CancellationToken.None)); } /// - /// Register a fire-and-forget task so can await it. - /// The task removes itself from the set on completion via a continuation. + /// Start a fire-and-forget task and register it so can await it. /// - private void TrackBackgroundTask(Task task) + /// + /// The factory shape is the point: registration happens BEFORE the work starts. + /// Taking a meant the caller evaluated the argument — which starts the + /// work — and only then called in to register it. snapshots + /// _inFlight under _inFlightLock and synchronously awaits the snapshot, so a + /// Dispose landing in that start→Add window drained an empty (or short) set and returned + /// while a genuinely running SaveAsync was still in flight — exactly the outliving + /// store the drain exists to prevent. The window is removed, not shrunk: a placeholder is + /// registered under the lock first, and only then is the factory invoked, so any task that + /// runs at all is already visible to every subsequent snapshot. + /// The factory is invoked OUTSIDE _inFlightLock — it runs engine code + /// synchronously up to its first real await (and, via the injected clock, can re-enter + /// and this method), none of which belongs under that lock. + /// Post-disposal the call DECLINES: the factory is never invoked and nothing is + /// registered (the method returns silently — no throw, since every caller is a + /// fire-and-forget path that already treats a disposing engine as "stop"). The drain-latch + /// handover is the only caller that would otherwise leave _drainScheduled stuck true; + /// that is harmless, because a disposed engine drops pushes in + /// and so never consults the latch again. + /// + /// Starts the background work. Invoked at most once, outside the lock. + private void TrackBackgroundTask(Func factory) { - lock (_inFlightLock) { _inFlight.Add(task); } - // Use ContinueWith with ExecuteSynchronously so the removal runs on the - // completing thread — avoids scheduler delay between completion and - // unregistration that would otherwise let Dispose see a stale set. - task.ContinueWith(t => + // RunContinuationsAsynchronously: the placeholder is completed from the real + // task's ExecuteSynchronously continuation, i.e. on the completing thread — + // this keeps an awaiter of the placeholder from being resumed inline there. + var placeholder = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + lock (_inFlightLock) { - lock (_inFlightLock) { _inFlight.Remove(t); } + if (_disposed) return; + _inFlight.Add(placeholder.Task); + } + + Task work; + try + { + work = factory() ?? Task.CompletedTask; + } + catch (Exception ex) + { + // A factory that throws synchronously started nothing, so the placeholder must + // not be left pending — a Dispose that already snapshotted it would block on it + // forever. Fault it (and observe the fault, so an un-snapshotted placeholder is + // not reported as an unobserved task exception), then let the caller see the throw + // it would have seen when the argument was evaluated at the call site. + lock (_inFlightLock) { _inFlight.Remove(placeholder.Task); } + placeholder.TrySetException(ex); + _ = placeholder.Task.Exception; + throw; + } + + // ExecuteSynchronously so the removal runs on the completing thread — avoids + // scheduler delay between completion and unregistration that would otherwise + // let Dispose see a stale set. The placeholder mirrors the real task's outcome + // so Dispose's WhenAll still observes (and logs) a background failure. + work.ContinueWith(t => + { + lock (_inFlightLock) { _inFlight.Remove(placeholder.Task); } + if (t.IsFaulted) + { + placeholder.TrySetException(t.Exception!.InnerExceptions); + // Observe the mirror. A Dispose that snapshotted it still sees the fault + // through WhenAll (observing does not suppress propagation); one that did + // not would otherwise leave a second unobserved task exception behind. + _ = placeholder.Task.Exception; + } + else if (t.IsCanceled) placeholder.TrySetCanceled(); + else placeholder.TrySetResult(); }, CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); } @@ -1012,6 +1078,20 @@ public sealed class ScriptedAlarmEngine : IDisposable /// internal void RunShelvingCheckForTest() => RunShelvingCheck(); + /// + /// Test hook — launches background work through the same registration path the + /// engine's own fire-and-forget callers use, so tests can pin the + /// register-before-start ordering directly. + /// + /// Starts the background work. + internal void TrackBackgroundTaskForTest(Func factory) => TrackBackgroundTask(factory); + + /// Test hook — the number of background tasks currently registered for the Dispose drain. + internal int InFlightCountForTest + { + get { lock (_inFlightLock) { return _inFlight.Count; } } + } + private async Task ShelvingCheckAsync(IReadOnlyList alarmIds, CancellationToken ct) { var pending = new List(0); diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/ScriptedAlarmEngineTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/ScriptedAlarmEngineTests.cs index e7a7d571..a8ca67b2 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/ScriptedAlarmEngineTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/ScriptedAlarmEngineTests.cs @@ -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. + // ------------------------------------------------------------------------- + + /// 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. + [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"); + } + + /// 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. + [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"); + } + + /// Verifies Dispose blocks on work started through the factory form, and that the + /// registration is released once that work completes. + [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"); + } + + /// Verifies work whose task is already complete when the factory returns still + /// unregisters cleanly and leaves Dispose with nothing to wait on. + [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()); + } + + /// 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. + [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(() => 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)); + } + + /// 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. + [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); + } + /// /// 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 —