From 72b5f7a20c3de3e3ce68648a4e8c6345d76e7e51 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 08:16:44 -0400 Subject: [PATCH] fix(scripted-alarms): resolve Medium code-review finding (Core.ScriptedAlarms-004) Split the LoadAsync seed-read + subscribe loop: ReadTag seed fills _valueCache first, then persisted-state restore runs, then _loaded = true, then SubscribeTag is called. Any synchronous initial push from the upstream now arrives after _alarms is fully initialised and _loaded = true, so ReevaluateAsync will queue correctly behind the gate rather than racing the half-built state. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../ScriptedAlarmEngine.cs | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) 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 e42b629..de8301d 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs @@ -143,12 +143,17 @@ public sealed class ScriptedAlarmEngine : IDisposable + string.Join("\n ", compileFailures)); } - // Seed the value cache with current upstream values + subscribe for changes. + // Seed the value cache with current tag values before subscribing. The + // ReadTag calls happen first so that the initial predicate evaluation below + // (startup recovery, decision #14) uses a consistent snapshot. + // Subscriptions are established AFTER _loaded = true so that any synchronous + // initial-push an ITagUpstreamSource delivers from inside SubscribeTag arrives + // when _alarms is fully initialised. Before _loaded = true, a synchronous push + // would race the in-progress state restore and could overwrite the carefully + // seeded cache with a push that has no defined ordering relative to ReadTag. + // (Core.ScriptedAlarms-004) foreach (var path in _alarmsReferencing.Keys) - { _valueCache[path] = _upstream.ReadTag(path); - _upstreamSubscriptions.Add(_upstream.SubscribeTag(path, OnUpstreamChange)); - } // Restore persisted state, falling back to Fresh where nothing was saved, // then re-derive ActiveState from the current predicate per decision #14. @@ -163,6 +168,14 @@ public sealed class ScriptedAlarmEngine : IDisposable } _loaded = true; + + // Subscribe after _loaded = true and full state restore. If an upstream + // implementation pushes its initial value synchronously from inside + // SubscribeTag, OnUpstreamChange will queue a ReevaluateAsync that acquires + // _evalGate — it will correctly block until LoadAsync releases the gate, then + // re-evaluate against the fully-populated _alarms dict. + foreach (var path in _alarmsReferencing.Keys) + _upstreamSubscriptions.Add(_upstream.SubscribeTag(path, OnUpstreamChange)); _engineLogger.Information("ScriptedAlarmEngine loaded {Count} alarm(s)", _alarms.Count); // Dispose any previously-created timer before reassigning; a second LoadAsync