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 b8950db9..7c716217 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs @@ -569,32 +569,77 @@ public sealed class ScriptedAlarmEngine : IDisposable /// reads the set under, so the last drainer out and the next push in can never /// both decide "someone else will handle it". /// + /// + /// INVARIANT: _drainScheduled is released on EVERY exit path, the + /// exceptional one included. The flag is the only thing that lets + /// start a drainer, so an escape that leaves it + /// stuck true permanently and silently stops evaluation for every scripted + /// alarm in the process: pushes keep filling _valueCache and the dirty set, + /// no drainer is ever started again, no predicate ever runs again, and nothing is + /// logged. catches everything today, so the throw + /// surface is currently empty — the finally below is what keeps the + /// invariant true when it stops being (an audit-mode Serilog sink rethrowing out + /// of that catch block's own Error() call is enough). + /// private async Task DrainDirtyAsync(CancellationToken ct) { - while (true) + // Set inside _dirtyLock, together with the flag clear, so the finally can tell + // "I already released" from "I am unwinding" without ever double-releasing — + // a second clear could stomp a _drainScheduled = true a racing push had just + // set for the drainer it started, leaving two drainers running. + var released = false; + try { - string[] batch; - lock (_dirtyLock) + while (true) { - // Bail promptly on a disposing engine, but ALWAYS release - // _drainScheduled on the way out — leaving it stuck true would make a - // hypothetical post-dispose push believe a drainer is coming. (Such - // pushes are already dropped in OnUpstreamChange; this keeps the flag - // honest regardless.) - if (_disposed || _dirtyAlarmIds.Count == 0) + string[] batch; + lock (_dirtyLock) { + // Bail promptly on a disposing engine, but ALWAYS release + // _drainScheduled on the way out — leaving it stuck true would make a + // hypothetical post-dispose push believe a drainer is coming. (Such + // pushes are already dropped in OnUpstreamChange; this keeps the flag + // honest regardless.) + if (_disposed || _dirtyAlarmIds.Count == 0) + { + _dirtyAlarmIds.Clear(); + _drainScheduled = false; + released = true; + return; + } + + batch = [.. _dirtyAlarmIds]; _dirtyAlarmIds.Clear(); - _drainScheduled = false; - return; } - batch = [.. _dirtyAlarmIds]; - _dirtyAlarmIds.Clear(); + // Reused as-is: the gate / evaluate / persist / emit-outside-the-gate shape + // is already correct for a batch. Only the spawn-per-push is gone. + await ReevaluateAsync(batch, ct).ConfigureAwait(false); } + } + finally + { + // Exceptional exit only (see `released`). The batch the faulted pass was + // carrying dies with it — ReevaluateAsync's own catch is what normally makes + // that a logged non-event — but ids that queued BEHIND it are still in the + // set and would otherwise sit there until some later push happened to arrive, + // so hand the latch straight to a replacement drainer instead of releasing + // it. Handing over under the same lock keeps "exactly one drainer" true with + // no window a push could start a second one in. This cannot spin: every pass + // swaps the set out and clears it before evaluating, so a replacement finds + // work only if fresh pushes arrived, and a permanently-throwing evaluation + // costs one pass per push — the same rate as a healthy one. + if (!released) + { + bool restart; + lock (_dirtyLock) + { + restart = !_disposed && _dirtyAlarmIds.Count > 0; + if (!restart) _drainScheduled = false; + } - // Reused as-is: the gate / evaluate / persist / emit-outside-the-gate shape - // is already correct for a batch. Only the spawn-per-push is gone. - await ReevaluateAsync(batch, ct).ConfigureAwait(false); + if (restart) TrackBackgroundTask(DrainDirtyAsync(ct)); + } } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Fleet.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Fleet.razor index d3a70049..dbdf4435 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Fleet.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Fleet.razor @@ -114,6 +114,10 @@ else // goes away aborts its query instead of running it to completion against a component that's gone. private readonly CancellationTokenSource _cts = new(); + // Set first thing in DisposeAsync. A tick that resumes after teardown must not call + // StateHasChanged — the renderer is gone and it throws, inside a task nobody awaits. + private volatile bool _disposed; + protected override async Task OnInitializedAsync() { await LoadAsync(); @@ -170,7 +174,10 @@ else // Release first: a StateHasChanged on a torn-down circuit must never leave the guard set. Volatile.Write(ref _tickRunning, 0); _refreshing = false; - StateHasChanged(); + // This one sits OUTSIDE the try, so it has no catch to fall into — and a render + // request against a disposed renderer throws. Skip it once the page is gone; there + // is nothing left to render to. + if (!_disposed) StateHasChanged(); } } @@ -186,13 +193,20 @@ else public async ValueTask DisposeAsync() { + _disposed = true; // Cancel before draining so a tick already in flight aborts its queries rather than running // them to completion against a component that's gone. _cts.Cancel(); // Drain the timer so an in-flight callback can't touch a component that's already gone // (System.Threading.Timer's async dispose awaits any in-flight callback — .NET 6+). if (_timer is not null) await _timer.DisposeAsync(); - _cts.Dispose(); + // ...but the callback body is `_ = InvokeAsync(LoadAsync)`, so that await covered only the + // SCHEDULING of the tick, never the tick itself: a query can still be running against + // _cts.Token right now. Disposing the source out from under it turns the clean + // cancellation the line above arranged into an ObjectDisposedException mid-query, so skip + // the dispose while a tick holds it. An undisposed CancellationTokenSource with no timer + // and no materialised WaitHandle costs nothing and is collected with the component. + if (Volatile.Read(ref _tickRunning) == 0) _cts.Dispose(); } private sealed record NodeRow( diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Hosts.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Hosts.razor index 43eb8d89..453abd93 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Hosts.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Hosts.razor @@ -337,6 +337,10 @@ else // goes away aborts its queries instead of running them against a component that's gone. private readonly CancellationTokenSource _cts = new(); + // Set first thing in DisposeAsync. A tick that resumes after teardown must not call + // StateHasChanged — the renderer is gone and it throws, inside a task nobody awaits. + private volatile bool _disposed; + protected override async Task OnInitializedAsync() { Refresh(); @@ -390,7 +394,10 @@ else // Release first: a StateHasChanged on a torn-down circuit must never leave the guard set. Volatile.Write(ref _tickRunning, 0); _refreshing = false; - StateHasChanged(); + // This one sits OUTSIDE the try, so it has no catch to fall into — and a render + // request against a disposed renderer throws. Skip it once the page is gone; there + // is nothing left to render to. + if (!_disposed) StateHasChanged(); } } @@ -409,10 +416,15 @@ else .Select(d => new HostsDriverInstanceInfo(d.DriverInstanceId, d.ClusterId, d.Name, d.DriverType, d.Enabled)) .ToListAsync(_cts.Token); } - catch (OperationCanceledException) + catch (Exception ex) when (ex is OperationCanceledException or ObjectDisposedException) { // The circuit went away mid-load. Rethrow so the caller treats it as a clean exit — it // must NOT fall into the swallow-and-clear below, which would blank the cached config. + // ObjectDisposedException belongs here for the same reason: teardown disposes _cts (and + // with it the DbContext's token registration) while a query may still be running, and + // that shape reaches this method as ODE rather than OCE. Sending it to the generic catch + // blanked _nodes/_instances on the way out — the exact swallow-and-clear this rethrow + // exists to prevent, and permanent, because a torn-down page never ticks again. throw; } catch (Exception ex) @@ -513,6 +525,7 @@ else public async ValueTask DisposeAsync() { + _disposed = true; // Unsubscribe first so the singleton store can't invoke a handler on a disposed component. DriverStore.SnapshotChanged -= OnSnapshotChanged; // Cancel before draining so a tick already in flight aborts its queries rather than running @@ -521,7 +534,13 @@ else // Drain the timer so an in-flight callback can't touch a component that's already gone // (System.Threading.Timer's async dispose awaits any in-flight callback — .NET 6+). if (_timer is not null) await _timer.DisposeAsync(); - _cts.Dispose(); + // ...but the callback body is `_ = InvokeAsync(TickAsync)`, so that await covered only the + // SCHEDULING of the tick, never the tick itself: LoadConfigAsync can still be running + // against _cts.Token right now. Disposing the source out from under it turns the clean + // cancellation the line above arranged into an ObjectDisposedException mid-query, so skip + // the dispose while a tick holds it. An undisposed CancellationTokenSource with no timer + // and no materialised WaitHandle costs nothing and is collected with the component. + if (Volatile.Read(ref _tickRunning) == 0) _cts.Dispose(); } private sealed record MemberRow( 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 85f419d7..e000031b 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 @@ -1546,6 +1546,83 @@ public sealed class ScriptedAlarmEngineTests probe.Count.ShouldBe(2, "the in-flight pass plus exactly one follow-up pass for the late push"); } + /// + /// Verifies the drain latch (_drainScheduled) is released even when an exception + /// escapes the drain loop, so a later push still starts a drainer. A latch left stuck true + /// stops evaluation for EVERY scripted alarm, permanently and silently. + /// + /// + /// The escape is produced without touching the engine: the store's SaveAsync throws (which + /// ReevaluateAsync catches, as designed), and the engine logger is configured with a + /// Serilog audit sink that throws on that catch block's own "reevaluate failed" + /// Error() call. Audit sinks propagate by contract, so the throw leaves ReevaluateAsync + /// through its catch and unwinds DrainDirtyAsync — the only escape path the current + /// code actually has, and a real one for any deployment auditing its logs. + /// + [Fact] + public async Task A_throw_escaping_the_drain_loop_still_releases_the_drain_latch() + { + var up = new FakeUpstream(); + up.Set("Temp", 50); + var probe = new CountingSink("EVALPROBE"); + var logger = new LoggerConfiguration() + .WriteTo.Sink(probe) + .AuditTo.Sink(new ThrowingSink("reevaluate failed")) + .CreateLogger(); + var store = new CountingAlarmStateStore(); + using var eng = new ScriptedAlarmEngine(up, store, new ScriptLoggerFactory(logger), logger); + await eng.LoadAsync([ + Alarm("A", """ctx.Logger.Information("EVALPROBE"); return (int)ctx.GetTag("Temp").Value > 100;"""), + ], TestContext.Current.CancellationToken); + // LoadAsync itself evaluates + persists once (startup recovery) — count from zero. + probe.Reset(); + store.ResetCount(); + + // Push #1: transition → SaveAsync throws → the catch's Error() hits the audit + // sink and throws → the exception unwinds out of the drain loop. Waiting on the + // save ATTEMPT (counted before the throw) rather than on the probe keeps the + // predicate-evaluation count below assertable. + store.ThrowOnNextSave = true; + up.Push("Temp", 150); + await WaitForAsync(() => store.SaveCount == 1, timeoutMs: 3000); + probe.Count.ShouldBe(1, "the faulted pass evaluated the predicate exactly once"); + eng.GetState("A")!.Active.ShouldBe(AlarmActiveState.Inactive, + "persist-before-update: a failed save must leave the in-memory condition at the prior state"); + + // The pin. This push either starts a fresh drainer (latch already released) or is + // handed to the replacement drainer the unwinding pass starts (latch still held, + // dirty set non-empty) — the two orderings the race between "SaveAsync was called" + // and "the drain finished unwinding" produces, and both must evaluate it. With the + // latch stuck true, OnUpstreamChange only grows the dirty set, no predicate ever + // runs again, and the alarm silently never activates. + up.Push("Temp", 160); + await WaitForAsync(() => eng.GetState("A")!.Active == AlarmActiveState.Active, timeoutMs: 3000); + probe.Count.ShouldBe(2, "the push after the faulted drain must itself be evaluated"); + } + + /// + /// A Serilog sink that throws when a marker event is emitted. Registered through + /// AuditTo so the failure propagates to the caller instead of being swallowed by + /// Serilog's normal best-effort emit — the mechanism the drain-latch test uses to make an + /// exception escape a method that otherwise catches everything. + /// + private sealed class ThrowingSink : Serilog.Core.ILogEventSink + { + private readonly string _marker; + + /// Initializes the sink with the marker substring that triggers the throw. + /// Substring the message template must contain to throw. + public ThrowingSink(string marker) => _marker = marker; + + /// Throws if the event's message template carries the marker; otherwise ignores it. + /// The Serilog event being emitted. + public void Emit(Serilog.Events.LogEvent logEvent) + { + if (logEvent.MessageTemplate.Text.Contains(_marker, StringComparison.Ordinal)) + throw new InvalidOperationException($"audit sink refused '{_marker}' (test)"); + } + } + /// /// A Serilog sink that counts the events whose message template contains a marker string. /// Used to count predicate EVALUATIONS (the predicate emits the marker through @@ -1598,6 +1675,12 @@ public sealed class ScriptedAlarmEngineTests /// Gets a value indicating whether a SaveAsync call is currently blocked waiting on BlockNextSave. public bool SaveInProgress { get; private set; } + /// + /// Gets or sets a value indicating whether the next SaveAsync throws instead of persisting. + /// Self-clearing, so one failed save can be followed by a healthy one. + /// + public bool ThrowOnNextSave { get; set; } + /// Loads an alarm condition state by ID from the inner store. /// The ID of the alarm condition state to load. /// A cancellation token. @@ -1609,12 +1692,18 @@ public sealed class ScriptedAlarmEngineTests public Task> LoadAllAsync(CancellationToken ct) => _inner.LoadAllAsync(ct); - /// Counts the save, optionally blocking on the BlockNextSave gate, then delegates. + /// Counts the save, optionally throwing or blocking on the BlockNextSave gate, then delegates. /// The alarm condition state to save. /// A cancellation token. public async Task SaveAsync(AlarmConditionState state, CancellationToken ct) { Interlocked.Increment(ref _saveCount); + if (ThrowOnNextSave) + { + ThrowOnNextSave = false; + throw new InvalidOperationException("store unavailable (test)"); + } + var gate = BlockNextSave; if (gate is not null) { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/PollTimerTeardownOrderingTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/PollTimerTeardownOrderingTests.cs new file mode 100644 index 00000000..9dc72b9b --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/PollTimerTeardownOrderingTests.cs @@ -0,0 +1,117 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests; + +/// +/// Pins the teardown ordering of the two self-polling pages (/fleet, /hosts). +/// Both schedule their refresh as _ = InvokeAsync(...) from a +/// callback, so await _timer.DisposeAsync() awaits only the SCHEDULING of a tick, never the +/// tick itself: a query can still be running against _cts.Token after that await returns. +/// Three properties follow, and none of them is visible in a passing render test. +/// +/// +/// Source scans rather than rendering assertions because the repo has no bUnit (same technique as +/// HostsPanelWiringTests). A scan proves the shape of the teardown, not that a real circuit +/// tears down cleanly — the fix itself is inspection-verified against the disposal race. +/// +[Trait("Category", "Unit")] +public sealed class PollTimerTeardownOrderingTests +{ + private static string ReadPageSource(string fileName) + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.OtOpcUa.slnx"))) + dir = dir.Parent; + dir.ShouldNotBeNull("could not locate the repository root (no ZB.MOM.WW.OtOpcUa.slnx above the test binary)"); + + return File.ReadAllText(Path.Combine( + dir!.FullName, "src", "Server", "ZB.MOM.WW.OtOpcUa.AdminUI", + "Components", "Pages", fileName)); + } + + /// + /// The CTS must be cancelled BEFORE the timer is drained. Reversed, a tick that is in flight + /// during teardown runs its queries to completion against a component that is already gone + /// instead of aborting on a cancelled token. + /// + [Theory] + [InlineData("Fleet.razor")] + [InlineData("Hosts.razor")] + public void The_page_cancels_its_token_source_before_draining_the_poll_timer(string page) + { + var source = ReadPageSource(page); + + var cancelIndex = source.IndexOf("_cts.Cancel();", StringComparison.Ordinal); + var drainIndex = source.IndexOf("_timer.DisposeAsync();", StringComparison.Ordinal); + + cancelIndex.ShouldBeGreaterThan(-1, $"{page} no longer cancels its page-lifetime CancellationTokenSource"); + drainIndex.ShouldBeGreaterThan(-1, $"{page} no longer drains its poll timer on dispose"); + cancelIndex.ShouldBeLessThan(drainIndex, + $"{page} must cancel before draining — a tick in flight has to observe cancellation, not " + + "run its queries out against a torn-down component"); + } + + /// + /// Disposing the CTS is gated on the overlap guard. Ungated it can be disposed out from under a + /// still-running tick, which converts the clean OperationCanceledException the cancel + /// above arranged into an ObjectDisposedException thrown mid-query. + /// + [Theory] + [InlineData("Fleet.razor")] + [InlineData("Hosts.razor")] + public void The_page_only_disposes_its_token_source_when_no_tick_holds_it(string page) + { + ReadPageSource(page).ShouldContain( + "if (Volatile.Read(ref _tickRunning) == 0) _cts.Dispose();", + Case.Sensitive, + customMessage: + $"{page} disposes its CancellationTokenSource without checking the overlap guard — " + + "the timer callback discards its InvokeAsync task, so DisposeAsync awaited only the " + + "scheduling of the tick and a query may still hold the token"); + } + + /// + /// The StateHasChanged in the refresh finally sits outside the try, so it has no + /// catch to fall into; rendering into a disposed renderer throws inside a task nobody awaits. + /// + [Theory] + [InlineData("Fleet.razor")] + [InlineData("Hosts.razor")] + public void The_page_skips_the_teardown_render_once_disposed(string page) + { + ReadPageSource(page).ShouldContain( + "if (!_disposed) StateHasChanged();", + Case.Sensitive, + customMessage: + $"{page}'s finally-block StateHasChanged is unguarded — it is outside the try, so a " + + "render against a torn-down circuit escapes into a discarded task"); + } + + /// + /// THE regression pin for the /hosts swallow-and-clear: an ObjectDisposedException raised + /// by teardown must rethrow as a clean exit, never reach the generic catch that blanks + /// _nodes/_instances. That blanking is permanent — a torn-down page never ticks + /// again to refill them — and its own comment forbids it. + /// + [Fact] + public void The_hosts_config_load_rethrows_a_disposal_race_instead_of_blanking_its_cache() + { + var source = ReadPageSource("Hosts.razor"); + + var rethrowIndex = source.IndexOf( + "catch (Exception ex) when (ex is OperationCanceledException or ObjectDisposedException)", + StringComparison.Ordinal); + // Anchored on the warning line, not on `_nodes = new();` — that assignment also appears as a + // field initializer far earlier in the file, which would make the ordering check vacuous. + var blankIndex = source.IndexOf( + "/hosts: failed to load driver-instance config", StringComparison.Ordinal); + + rethrowIndex.ShouldBeGreaterThan(-1, + "/hosts LoadConfigAsync no longer rethrows the teardown exceptions — an ObjectDisposedException " + + "from disposing _cts under a live tick falls into the generic catch and blanks the cached config"); + blankIndex.ShouldBeGreaterThan(-1, "/hosts LoadConfigAsync no longer has the swallow-and-clear fallback"); + rethrowIndex.ShouldBeLessThan(blankIndex, + "the rethrow filter must precede the blanking catch, or it never runs"); + } +}