From 96e817a7e13d9c2aa2f3f10cf0cb831648584cc8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 16 Jun 2026 15:48:25 -0400 Subject: [PATCH] fix(siteruntime): MV-8 review fixes (construct list inside try; dictionary attr lookup; test hygiene) --- .../Actors/InstanceActor.cs | 43 +++++++++++++----- .../Actors/InstanceActorTests.cs | 45 ++++++++++++++++++- 2 files changed, 76 insertions(+), 12 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs index 21da0ecd..d6424d98 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs @@ -60,6 +60,14 @@ public class InstanceActor : ReceiveActor private readonly Dictionary _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 _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. 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 diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs index ffa04e29..ae7dcfa2 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs @@ -583,6 +583,49 @@ public class InstanceActorTests : TestKit, IDisposable ExpectNoTerminated(actor, TimeSpan.FromMilliseconds(500)); } + /// + /// 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 Bad. + /// + [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(TimeSpan.FromSeconds(5)); + + Assert.True(response.Found); + Assert.Equal("Bad", response.Quality); + var list = Assert.IsType>(response.Value); + Assert.Equal(new[] { 1, 2, 3 }, list); + } + /// /// 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()); } }