refactor(scripted-alarms): review-fix polish for T5/T7/T8 (observer isolation, warning hoist, doc)

2026-06-10 14:32:49 -04:00
parent b28c6bdb62
commit 55101baaa4
3 changed files with 64 additions and 34 deletions
@@ -206,6 +206,13 @@ public sealed record EquipmentScriptedAlarmPlan(
    /// <see cref="DependencyRefs"/> (an interface-typed list) BY REFERENCE, flagging every alarm as
    /// "changed" on every parse (fresh list instances). Compare it element-wise so a no-op redeploy
    /// diffs empty (mirrors <see cref="EquipmentVirtualTagPlan"/>).</summary>
+    /// <remarks>
+    ///     <b>DependencyRefs equality is order-sensitive</b> (SequenceEqual).
+    ///     <see cref="Phase7Composer.MergeAlarmDependencyRefs"/> is the canonical, deterministic
+    ///     producer of that order (predicate <c>ctx.GetTag</c> reads first, then first-seen message
+    ///     template tokens). Downstream byte-parity between the live composer and the artifact-decode
+    ///     mirror depends on both sides calling <c>MergeAlarmDependencyRefs</c> with identical inputs.
+    /// </remarks>
    public bool Equals(EquipmentScriptedAlarmPlan? other) =>
        other is not null &&
        ScriptedAlarmId == other.ScriptedAlarmId &&
@@ -419,37 +426,37 @@ public static class Phase7Composer
        // template tokens; the reserved {{equip}} double-brace form is excluded). Enabled is carried
        // (never dropped) — the runtime host decides whether to host a disabled alarm. Ordered by
        // EquipmentId then ScriptedAlarmId so the upcoming artifact byte-parity test is reliable.
-        var equipmentScriptedAlarms = scriptedAlarms
+        //
+        // Eager foreach (not lazy LINQ Select) so the Trace.TraceWarning fires exactly once per
+        // compose call; a lazy select would re-fire on every re-enumeration of the LINQ chain.
+        var equipmentScriptedAlarms = new List<EquipmentScriptedAlarmPlan>();
+        foreach (var a in scriptedAlarms
            .OrderBy(a => a.EquipmentId, StringComparer.Ordinal)
-            .ThenBy(a => a.ScriptedAlarmId, StringComparer.Ordinal)
-            .Select(a =>
+            .ThenBy(a => a.ScriptedAlarmId, StringComparer.Ordinal))
+        {
+            if (!scriptsById.TryGetValue(a.PredicateScriptId, out var s))
            {
-                if (!scriptsById.TryGetValue(a.PredicateScriptId, out var s))
-                {
-                    Trace.TraceWarning(
-                        "Phase7Composer: scripted alarm '{0}' (equipment '{1}') references predicate " +
-                        "script '{2}' which is not in the supplied scripts — skipping.",
-                        a.ScriptedAlarmId, a.EquipmentId, a.PredicateScriptId);
-                    return null;
-                }
-                var source = s.SourceCode;
-                return new EquipmentScriptedAlarmPlan(
-                    ScriptedAlarmId: a.ScriptedAlarmId,
-                    EquipmentId: a.EquipmentId,
-                    Name: a.Name,
-                    AlarmType: a.AlarmType,
-                    Severity: a.Severity,
-                    MessageTemplate: a.MessageTemplate,
-                    PredicateScriptId: a.PredicateScriptId,
-                    PredicateSource: source,
-                    DependencyRefs: MergeAlarmDependencyRefs(source, a.MessageTemplate),
-                    HistorizeToAveva: a.HistorizeToAveva,
-                    Retain: a.Retain,
-                    Enabled: a.Enabled);
-            })
-            .Where(p => p is not null)
-            .Select(p => p!)
-            .ToList();
+                Trace.TraceWarning(
+                    "Phase7Composer: scripted alarm '{0}' (equipment '{1}') references predicate " +
+                    "script '{2}' which is not in the supplied scripts — skipping.",
+                    a.ScriptedAlarmId, a.EquipmentId, a.PredicateScriptId);
+                continue;
+            }
+            var source = s.SourceCode;
+            equipmentScriptedAlarms.Add(new EquipmentScriptedAlarmPlan(
+                ScriptedAlarmId: a.ScriptedAlarmId,
+                EquipmentId: a.EquipmentId,
+                Name: a.Name,
+                AlarmType: a.AlarmType,
+                Severity: a.Severity,
+                MessageTemplate: a.MessageTemplate,
+                PredicateScriptId: a.PredicateScriptId,
+                PredicateSource: source,
+                DependencyRefs: MergeAlarmDependencyRefs(source, a.MessageTemplate),
+                HistorizeToAveva: a.HistorizeToAveva,
+                Retain: a.Retain,
+                Enabled: a.Enabled));
+        }

        return new Phase7CompositionResult(areas, lines, nodes, plans, alarms, galaxyTags)
        {
@@ -32,6 +32,8 @@ public sealed class DependencyMuxTagUpstreamSource : ITagUpstreamSource
    // AreInputsReady gate tests exactly this bit — so an "unknown path" snapshot uses it too.
    private const uint StatusBad = 0x80000000u;

+    // Intentionally never pruned: cold-start semantics depend on retained last values so
+    // ReadTag can answer synchronously for any path that has ever been pushed.
    private readonly ConcurrentDictionary<string, DataValueSnapshot> _cache
        = new(StringComparer.Ordinal);
    private readonly ConcurrentDictionary<string, ImmutableList<Action<string, DataValueSnapshot>>> _observers
@@ -72,7 +74,10 @@ public sealed class DependencyMuxTagUpstreamSource : ITagUpstreamSource
        if (_observers.TryGetValue(path, out var observers))
        {
            foreach (var observer in observers)
-                observer(path, snapshot);
+            {
+                try { observer(path, snapshot); }
+                catch { /* one misbehaving observer must not silence the rest */ }
+            }
        }
    }

@@ -91,6 +96,12 @@ public sealed class DependencyMuxTagUpstreamSource : ITagUpstreamSource
    }

    /// <inheritdoc/>
+    /// <remarks>
+    ///     <b>No-replay contract</b>: a new subscriber does NOT receive a synthetic initial
+    ///     notification for the current cached value. Callers that need the current value must
+    ///     call <see cref="ReadTag"/> immediately after subscribing. The engine's cold-start path
+    ///     (startup-recovery + read-cache-refill) already does this.
+    /// </remarks>
    public IDisposable SubscribeTag(string path, Action<string, DataValueSnapshot> observer)
    {
        ArgumentNullException.ThrowIfNull(path);
@@ -76,6 +76,14 @@ public sealed class EfAlarmConditionStateStore : IAlarmStateStore
    }

    /// <inheritdoc />
+    /// <remarks>
+    ///     <b>Concurrency assumption</b>: saves for a given <c>alarmId</c> are serialized by the
+    ///     owning host actor (one actor owns the engine per equipment), mirroring
+    ///     <c>EfAlarmActorStateStore</c>. The check-then-insert pattern is therefore safe under
+    ///     that guarantee — two concurrent inserts for the same alarm cannot occur in the live
+    ///     runtime. The <see cref="DbUpdateConcurrencyException"/> catch handles the edge case of a
+    ///     racing concurrent restart during crash recovery.
+    /// </remarks>
    public async Task SaveAsync(AlarmConditionState state, CancellationToken ct)
    {
        using var db = await _dbFactory.CreateDbContextAsync(ct).ConfigureAwait(false);
@@ -85,6 +93,8 @@ public sealed class EfAlarmConditionStateStore : IAlarmStateStore

        if (row is null)
        {
+            // Required members are set here to satisfy the compiler; ApplyState is the single
+            // source of truth for all columns and immediately overwrites these values below.
            row = new ScriptedAlarmState
            {
                ScriptedAlarmId = state.AlarmId,
@@ -151,15 +161,15 @@ public sealed class EfAlarmConditionStateStore : IAlarmStateStore
        AlarmId: row.ScriptedAlarmId,
        Enabled: string.Equals(row.EnabledState, "Disabled", StringComparison.Ordinal)
            ? AlarmEnabledState.Disabled
-            : AlarmEnabledState.Enabled,
+            : AlarmEnabledState.Enabled,  // unknown string → Enabled (safe default)
        // Active is not persisted — the engine re-derives it from the predicate at startup.
        Active: AlarmActiveState.Inactive,
        Acked: string.Equals(row.AckedState, "Acknowledged", StringComparison.Ordinal)
            ? AlarmAckedState.Acknowledged
-            : AlarmAckedState.Unacknowledged,
+            : AlarmAckedState.Unacknowledged,  // unknown string → Unacknowledged (safe default)
        Confirmed: string.Equals(row.ConfirmedState, "Confirmed", StringComparison.Ordinal)
            ? AlarmConfirmedState.Confirmed
-            : AlarmConfirmedState.Unconfirmed,
+            : AlarmConfirmedState.Unconfirmed,  // unknown string → Unconfirmed (safe default)
        Shelving: new ShelvingState(MapShelvingFromColumn(row.ShelvingState), row.ShelvingExpiresUtc),
        // No transition column — UpdatedAtUtc carries the last transition timestamp.
        LastTransitionUtc: row.UpdatedAtUtc,
@@ -194,7 +204,7 @@ public sealed class EfAlarmConditionStateStore : IAlarmStateStore
    {
        "OneShotShelved" => ShelvingKind.OneShot,
        "TimedShelved" => ShelvingKind.Timed,
-        _ => ShelvingKind.Unshelved,
+        _ => ShelvingKind.Unshelved,  // unknown string → Unshelved (safe default)
    };

    private static string SerializeComments(ImmutableList<AlarmComment> comments)
@@ -202,6 +212,8 @@ public sealed class EfAlarmConditionStateStore : IAlarmStateStore
        if (comments.IsEmpty) return "[]";
        var dtos = comments.Select(c => new CommentDto
        {
+            // AlarmComment.TimestampUtc must be DateTimeKind.Utc for correct ISO-8601 round-trip;
+            // the engine always creates AlarmComment instances with Utc kind.
            TimestampUtc = c.TimestampUtc,
            User = c.User,
            Kind = c.Kind,