fix: release the alarm drain latch on escape; cancel before disposing the page poll timers

2026-08-16 05:40:27 -04:00
parent b9e9df4081
commit 445bb855e2
5 changed files with 306 additions and 22 deletions
@@ -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".
    /// </summary>
+    /// <remarks>
+    ///     <b>INVARIANT: <c>_drainScheduled</c> is released on EVERY exit path, the
+    ///     exceptional one included.</b> The flag is the only thing that lets
+    ///     <see cref="OnUpstreamChange"/> start a drainer, so an escape that leaves it
+    ///     stuck <c>true</c> permanently and silently stops evaluation for every scripted
+    ///     alarm in the process: pushes keep filling <c>_valueCache</c> and the dirty set,
+    ///     no drainer is ever started again, no predicate ever runs again, and nothing is
+    ///     logged. <see cref="ReevaluateAsync"/> catches everything today, so the throw
+    ///     surface is currently empty — the <c>finally</c> below is what keeps the
+    ///     invariant true when it stops being (an audit-mode Serilog sink rethrowing out
+    ///     of that catch block's own <c>Error()</c> call is enough).
+    /// </remarks>
    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));
+            }
        }
    }

@@ -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(
@@ -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(
@@ -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");
    }

+    /// <summary>
+    ///     Verifies the drain latch (<c>_drainScheduled</c>) 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.
+    /// </summary>
+    /// <remarks>
+    ///     The escape is produced without touching the engine: the store's SaveAsync throws (which
+    ///     <c>ReevaluateAsync</c> catches, as designed), and the engine logger is configured with a
+    ///     Serilog <b>audit</b> sink that throws on that catch block's own "reevaluate failed"
+    ///     Error() call. Audit sinks propagate by contract, so the throw leaves <c>ReevaluateAsync</c>
+    ///     through its catch and unwinds <c>DrainDirtyAsync</c> — the only escape path the current
+    ///     code actually has, and a real one for any deployment auditing its logs.
+    /// </remarks>
+    [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");
+    }
+
+    /// <summary>
+    ///     A Serilog sink that throws when a marker event is emitted. Registered through
+    ///     <c>AuditTo</c> 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.
+    /// </summary>
+    private sealed class ThrowingSink : Serilog.Core.ILogEventSink
+    {
+        private readonly string _marker;
+
+        /// <summary>Initializes the sink with the marker substring that triggers the throw.</summary>
+        /// <param name="marker">Substring the message template must contain to throw.</param>
+        public ThrowingSink(string marker) => _marker = marker;
+
+        /// <summary>Throws if the event's message template carries the marker; otherwise ignores it.</summary>
+        /// <param name="logEvent">The Serilog event being emitted.</param>
+        public void Emit(Serilog.Events.LogEvent logEvent)
+        {
+            if (logEvent.MessageTemplate.Text.Contains(_marker, StringComparison.Ordinal))
+                throw new InvalidOperationException($"audit sink refused '{_marker}' (test)");
+        }
+    }
+
    /// <summary>
    ///     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
        /// <summary>Gets a value indicating whether a SaveAsync call is currently blocked waiting on BlockNextSave.</summary>
        public bool SaveInProgress { get; private set; }

+        /// <summary>
+        ///     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.
+        /// </summary>
+        public bool ThrowOnNextSave { get; set; }
+
        /// <summary>Loads an alarm condition state by ID from the inner store.</summary>
        /// <param name="alarmId">The ID of the alarm condition state to load.</param>
        /// <param name="ct">A cancellation token.</param>
@@ -1609,12 +1692,18 @@ public sealed class ScriptedAlarmEngineTests
        public Task<IReadOnlyList<AlarmConditionState>> LoadAllAsync(CancellationToken ct)
            => _inner.LoadAllAsync(ct);

-        /// <summary>Counts the save, optionally blocking on the BlockNextSave gate, then delegates.</summary>
+        /// <summary>Counts the save, optionally throwing or blocking on the BlockNextSave gate, then delegates.</summary>
        /// <param name="state">The alarm condition state to save.</param>
        /// <param name="ct">A cancellation token.</param>
        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)
            {
@@ -0,0 +1,117 @@
+using Shouldly;
+using Xunit;
+
+namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests;
+
+/// <summary>
+///     Pins the teardown ordering of the two self-polling pages (<c>/fleet</c>, <c>/hosts</c>).
+///     Both schedule their refresh as <c>_ = InvokeAsync(...)</c> from a <see cref="System.Threading.Timer"/>
+///     callback, so <c>await _timer.DisposeAsync()</c> awaits only the SCHEDULING of a tick, never the
+///     tick itself: a query can still be running against <c>_cts.Token</c> after that await returns.
+///     Three properties follow, and none of them is visible in a passing render test.
+/// </summary>
+/// <remarks>
+///     Source scans rather than rendering assertions because the repo has no bUnit (same technique as
+///     <c>HostsPanelWiringTests</c>). 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.
+/// </remarks>
+[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));
+    }
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    [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");
+    }
+
+    /// <summary>
+    ///     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 <c>OperationCanceledException</c> the cancel
+    ///     above arranged into an <c>ObjectDisposedException</c> thrown mid-query.
+    /// </summary>
+    [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");
+    }
+
+    /// <summary>
+    ///     The <c>StateHasChanged</c> in the refresh <c>finally</c> sits outside the try, so it has no
+    ///     catch to fall into; rendering into a disposed renderer throws inside a task nobody awaits.
+    /// </summary>
+    [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");
+    }
+
+    /// <summary>
+    ///     THE regression pin for the /hosts swallow-and-clear: an <c>ObjectDisposedException</c> raised
+    ///     by teardown must rethrow as a clean exit, never reach the generic catch that blanks
+    ///     <c>_nodes</c>/<c>_instances</c>. That blanking is permanent — a torn-down page never ticks
+    ///     again to refill them — and its own comment forbids it.
+    /// </summary>
+    [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");
+    }
+}