fix(siteruntime): MV-8 review fixes (construct list inside try; dictionary attr lookup; test hygiene)

src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs

@@ -60,6 +60,14 @@ public class InstanceActor : ReceiveActor
     private readonly Dictionary<string, AlarmStateChanged> _latestAlarmEvents = new();
     private FlattenedConfiguration? _configuration;
 
+    // MV-8: resolved attributes indexed by canonical name. The TagValueUpdate
+    // ingest path is the highest-frequency message this actor handles, so the
+    // attribute lookup must be O(1) rather than a linear scan of
+    // _configuration.Attributes. Built once in the constructor from the
+    // deserialized configuration (last-wins on duplicate canonical names,
+    // mirroring the rest of the actor's by-name dictionaries).
+    private readonly Dictionary<string, ResolvedAttribute> _resolvedAttributeByName = new();
+
     // DCL manager actor reference for subscribing to tag values
     private readonly IActorRef? _dclManager;
     // Maps each tag path to every attribute canonical name that references it.
@@ -119,6 +127,10 @@ public class InstanceActor : ReceiveActor
                 _attributes[attr.CanonicalName] = attr.Value;
                 _attributeQualities[attr.CanonicalName] =
                     string.IsNullOrEmpty(attr.DataSourceReference) ? "Good" : "Uncertain";
+
+                // MV-8: index resolved attributes for O(1) lookup on the hot
+                // TagValueUpdate ingest path (last-wins on duplicate names).
+                _resolvedAttributeByName[attr.CanonicalName] = attr;
             }
         }
 
@@ -439,8 +451,8 @@ public class InstanceActor : ReceiveActor
         // we resolve and convert per attribute rather than once for the tag.
         foreach (var attrName in attrNames)
         {
-            var resolved = _configuration?.Attributes
-                .FirstOrDefault(a => a.CanonicalName == attrName);
+            // MV-8: O(1) lookup off the hot ingest path (was a linear FirstOrDefault).
+            _resolvedAttributeByName.TryGetValue(attrName, out var resolved);
 
             // MV-8: a List-typed attribute coerces the incoming OPC UA array
             // (a CLR array/IEnumerable from the SDK) into a typed List<T>. On an
@@ -510,23 +522,32 @@ public class InstanceActor : ReceiveActor
         if (incoming is not System.Collections.IEnumerable enumerable || incoming is string)
             return false;
 
-        var clrType = ListElementClrType(elementType);
-        var list = (System.Collections.IList)Activator.CreateInstance(
-            typeof(List<>).MakeGenericType(clrType))!;
-
         try
         {
+            // Construct the typed list INSIDE the try: although the six valid
+            // element types resolved by ListElementClrType cannot throw today,
+            // keeping ListElementClrType / MakeGenericType / CreateInstance inside
+            // the guarded block means any future change that introduces a throw
+            // here is caught and turned into a Bad-quality result rather than
+            // escaping into the actor and tripping supervision.
+            var clrType = ListElementClrType(elementType);
+            var list = (System.Collections.IList)Activator.CreateInstance(
+                typeof(List<>).MakeGenericType(clrType))!;
+
             foreach (var element in enumerable)
                 list.Add(CoerceElement(element, elementType));
+
+            typedList = list;
+            return true;
         }
-        catch (Exception ex) when (ex is FormatException or InvalidCastException
-            or OverflowException or ArgumentNullException)
+        catch (Exception ex)
         {
+            // Any coercion / construction failure → Bad quality, never a crash.
+            _logger.LogWarning(ex,
+                "Failed to coerce value to List<{Element}> for instance {Instance}; marking quality Bad",
+                attr.ElementDataType, _instanceUniqueName);
             return false;
         }
-
-        typedList = list;
-        return true;
     }
 
     private static Type ListElementClrType(DataType t) => t switch

tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs

@@ -583,6 +583,49 @@ public class InstanceActorTests : TestKit, IDisposable
         ExpectNoTerminated(actor, TimeSpan.FromMilliseconds(500));
     }
 
+    /// <summary>
+    /// MV-8 design contract: a failed coercion keeps the PRIOR value. A good
+    /// array is delivered first and stored as a typed list; a subsequent array
+    /// with a non-coercible element must NOT overwrite that value — the stored
+    /// value stays the prior list while the quality flips to <c>Bad</c>.
+    /// </summary>
+    [Fact]
+    public void InstanceActor_DataSourcedListAttribute_BadCoercion_PreservesPriorValue()
+    {
+        const string tag = "ns=3;s=Pump.Keep";
+        var config = new FlattenedConfiguration
+        {
+            InstanceUniqueName = "Pump-ListKeep",
+            Attributes =
+            [
+                new ResolvedAttribute
+                {
+                    CanonicalName = "Counts", Value = null,
+                    DataType = "List", ElementDataType = "Int32",
+                    DataSourceReference = tag, BoundDataConnectionName = "PLC"
+                }
+            ]
+        };
+
+        var dcl = CreateTestProbe();
+        var actor = CreateInstanceActorWithDcl("Pump-ListKeep", config, dcl);
+
+        // (1) A good array establishes the prior value.
+        actor.Tell(new TagValueUpdate("PLC", tag, new[] { 1, 2, 3 }, QualityCode.Good, DateTimeOffset.UtcNow));
+
+        // (2) A second array with a non-coercible element must not overwrite it.
+        actor.Tell(new TagValueUpdate("PLC", tag, new object[] { 4, "not-a-number", 6 }, QualityCode.Good, DateTimeOffset.UtcNow));
+
+        // (3) The stored value is still the prior list; quality is Bad.
+        actor.Tell(new GetAttributeRequest("corr-keep", "Pump-ListKeep", "Counts", DateTimeOffset.UtcNow));
+        var response = ExpectMsg<GetAttributeResponse>(TimeSpan.FromSeconds(5));
+
+        Assert.True(response.Found);
+        Assert.Equal("Bad", response.Quality);
+        var list = Assert.IsType<List<int>>(response.Value);
+        Assert.Equal(new[] { 1, 2, 3 }, list);
+    }
+
     /// <summary>
     /// MV-8 guard: scalar (non-List) data-sourced attributes keep the existing
     /// pass-through behaviour — a scalar value is stored unchanged.
@@ -620,7 +663,7 @@ public class InstanceActorTests : TestKit, IDisposable
     private void ExpectNoTerminated(IActorRef actor, TimeSpan within)
     {
         // The actor is Watch()ed; assert no Terminated arrives in the window.
+        // (Liveness is also proven by the preceding successful GetAttributeResponse.)
         ExpectNoMsg(within);
-        Assert.False(actor.IsNobody());
     }
 }