perf(alarms): dirty-set coalescing — one evaluation per alarm per drain, latest values win

src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs

@@ -29,6 +29,29 @@ namespace ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms;
 ///         Operators investigating a broken predicate shouldn't see a phantom
 ///         clear-event preceding the failure.
 ///     </para>
+///     <para>
+///         <b>Upstream pushes coalesce through a dirty set.</b> A push does not spawn its own
+///         re-evaluation: it marks the referencing alarm ids dirty and, if no drainer is already
+///         running, starts one. <see cref="DrainDirtyAsync"/> then swaps the set out and evaluates
+///         the batch, looping until the set is empty. N pushes arriving during one slow evaluation
+///         therefore collapse into ONE further pass instead of N serialized ones that would each
+///         re-read the same latest values. The collapse is loss-free by construction:
+///         <c>RefillReadCache</c> always reads <see cref="_valueCache"/>, which every push has
+///         already updated, so the surviving pass evaluates the latest values — latest-wins, never
+///         a lost update. There is no timer and no wall-clock batching window; a push that lands on
+///         an idle engine is evaluated immediately, and the coalescing window is exactly "whatever
+///         queued while <see cref="_evalGate"/> was busy".
+///     </para>
+///     <para>
+///         <b><see cref="IAlarmStateStore.SaveAsync"/> deliberately stays INSIDE the gate, before
+///         the in-memory update.</b> The persist-before-update ordering is a real crash-consistency
+///         property — a store failure must leave the in-memory condition and the persisted one both
+///         at the prior state rather than diverging — and it is pinned by
+///         <c>Store_save_failure_leaves_in_memory_state_unchanged</c>. Post-coalescing a save only
+///         happens on a genuine Part 9 transition, which is rare relative to the push rate, so the
+///         gate-held store call is no longer on the hot path and a write-behind queue would trade a
+///         correctness property for throughput the engine does not need.
+///     </para>
 /// </remarks>
 public sealed class ScriptedAlarmEngine : IDisposable
 {
@@ -149,7 +172,7 @@ public sealed class ScriptedAlarmEngine : IDisposable
     private bool _disposed;
 
     // Tracks fire-and-forget background work launched by OnUpstreamChange
-    // (ReevaluateAsync) and RunShelvingCheck (ShelvingCheckAsync). Dispose drains
+    // (DrainDirtyAsync) and RunShelvingCheck (ShelvingCheckAsync). Dispose drains
     // these so a re-evaluation in flight when shutdown begins finishes its
     // SaveAsync before the engine returns control to the caller. The HashSet is
     // accessed under its own lock — never under _evalGate — so registration /
@@ -157,6 +180,15 @@ public sealed class ScriptedAlarmEngine : IDisposable
     private readonly HashSet<Task> _inFlight = [];
     private readonly object _inFlightLock = new();
 
+    // Coalescing state for the upstream-push path. _dirtyAlarmIds accumulates the
+    // alarms a push made stale; _drainScheduled says a DrainDirtyAsync loop is
+    // already running and will pick them up, so a push storm starts exactly one
+    // drainer. Both are guarded by _dirtyLock — a leaf lock, never taken while
+    // holding _evalGate or _inFlightLock, and never held across an await.
+    private readonly HashSet<string> _dirtyAlarmIds = new(StringComparer.Ordinal);
+    private readonly object _dirtyLock = new();
+    private bool _drainScheduled; // guarded by _dirtyLock
+
     /// <summary>
     /// Initializes a new ScriptedAlarmEngine with the provided dependencies.
     /// </summary>
@@ -497,10 +529,12 @@ public sealed class ScriptedAlarmEngine : IDisposable
     }
 
     /// <summary>
-    ///     Upstream-change callback. Updates the value cache + enqueues predicate
-    ///     re-evaluation for every alarm referencing the changed path. Fire-and-forget
-    ///     so driver-side dispatch isn't blocked; the background task is tracked so
-    ///     <see cref="Dispose"/> can drain it.
+    ///     Upstream-change callback. Updates the value cache + marks every alarm
+    ///     referencing the changed path dirty, starting the drainer if one isn't
+    ///     already running. Never blocks driver-side dispatch, and never spawns work
+    ///     per push: a push landing while a drain is in flight only grows the dirty
+    ///     set, so a storm collapses into one further evaluation pass over the latest
+    ///     values. The drainer is tracked so <see cref="Dispose"/> can drain it.
     /// </summary>
     /// <param name="path">The upstream tag path that changed.</param>
     /// <param name="value">The new data value snapshot.</param>
@@ -508,9 +542,59 @@ public sealed class ScriptedAlarmEngine : IDisposable
     {
         _valueCache[path] = value;
         if (_disposed) return; // don't queue new work against a disposing engine
-        if (_alarmsReferencing.TryGetValue(path, out var alarmIds))
+        if (!_alarmsReferencing.TryGetValue(path, out var alarmIds)) return;
+
+        bool startDrain;
+        lock (_dirtyLock)
         {
-            TrackBackgroundTask(ReevaluateAsync(alarmIds.ToArray(), CancellationToken.None));
+            // Iterate the referencing set directly — the pre-coalescing path
+            // allocated a fresh string[] on every single push.
+            foreach (var id in alarmIds) _dirtyAlarmIds.Add(id);
+            startDrain = !_drainScheduled;
+            if (startDrain) _drainScheduled = true;
+        }
+
+        // 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));
+    }
+
+    /// <summary>
+    ///     The single drainer for the upstream-push dirty set. Swaps the accumulated
+    ///     alarm ids out under <c>_dirtyLock</c> and re-evaluates that batch, looping
+    ///     until the set is empty — so every push that landed while the previous pass
+    ///     held <c>_evalGate</c> is served by the next pass rather than by a queue of
+    ///     redundant per-push evaluations. Exactly one instance runs at a time
+    ///     (<c>_drainScheduled</c>), and it clears that flag under the same lock it
+    ///     reads the set under, so the last drainer out and the next push in can never
+    ///     both decide "someone else will handle it".
+    /// </summary>
+    private async Task DrainDirtyAsync(CancellationToken ct)
+    {
+        while (true)
+        {
+            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;
+                    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);
         }
     }
 
@@ -925,9 +1009,11 @@ public sealed class ScriptedAlarmEngine : IDisposable
         _shelvingTimer?.Dispose();
         UnsubscribeFromUpstream();
 
-        // Drain any fire-and-forget background work (ReevaluateAsync from
+        // Drain any fire-and-forget background work (DrainDirtyAsync from
         // OnUpstreamChange + ShelvingCheckAsync from the 5s timer) that started
-        // before _disposed = true was visible. Without this, a SaveAsync in
+        // before _disposed = true was visible. The drain loop observes _disposed
+        // at the top of every iteration and exits, so it cannot spin here. Without
+        // this, a SaveAsync in
         // flight can outlive the engine and write to a (possibly disposed) store
         // after Dispose() has returned. The tasks re-check _disposed after
         // acquiring the gate and bail out, but the await still has to complete.