From 3d8c285034e75baa5f55ebde46adacc47a698b52 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 08:31:49 -0400 Subject: [PATCH] fix(virtual-tags): resolve Medium code-review findings (Core.VirtualTags-002, -003, -005, -008, -012) Core.VirtualTags-002: cold-start guard publishes BadWaitingForInitialData instead of silently returning a stale value. Core.VirtualTags-003: Load detects duplicate Path values and keys the upstream-subscription loop off the registered tag set. Core.VirtualTags-005: VirtualTagSource fires the initial-data callback per path before registering the change observer, fixing an ordering race. Core.VirtualTags-008: DependencyGraph caches topological rank, lowering per-change-event cost from O(V+E) to O(closure). Core.VirtualTags-012: added 9 engine tests; CoerceResult null-return now maps to BadInternalError as the code comment intended. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Core.VirtualTags/findings.md | 22 +-- .../DependencyGraph.cs | 25 ++- .../VirtualTagEngine.cs | 39 +++- .../VirtualTagSource.cs | 15 +- .../VirtualTagEngineTests.cs | 182 ++++++++++++++++++ 5 files changed, 252 insertions(+), 31 deletions(-) diff --git a/code-reviews/Core.VirtualTags/findings.md b/code-reviews/Core.VirtualTags/findings.md index 6005a87..e9b63d2 100644 --- a/code-reviews/Core.VirtualTags/findings.md +++ b/code-reviews/Core.VirtualTags/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 12 | +| Open findings | 7 | ## Checklist coverage @@ -67,7 +67,7 @@ code and docs agree. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:237` | -| Status | Open | +| Status | Resolved | **Description:** The cold-start guard `if (!AreInputsReady(ctxCache)) return;` silently abandons the evaluation when any input is null or Bad-quality. For a chained virtual tag @@ -87,7 +87,7 @@ rather than returning with no state change, so clients see a defined quality. If operators need scripts that handle Bad upstreams, consider a per-definition opt-out of the readiness guard. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — cold-start guard now publishes `BadWaitingForInitialData` (0x80320000) and notifies observers instead of silently returning, so OPC UA clients see a defined quality rather than a stale prior value. ### Core.VirtualTags-003 @@ -96,7 +96,7 @@ the readiness guard. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:117-120` | -| Status | Open | +| Status | Resolved | **Description:** The upstream-subscription loop in `Load` iterates `definitions.SelectMany(d => _tags[d.Path].Reads)`. If `definitions` contains two rows @@ -115,7 +115,7 @@ them to `compileFailures` (or a dedicated rejection list) so the aggregated `definitions.SelectMany(d => _tags[d.Path]...)` when collecting upstream paths so the collection is keyed off the registered set, not the raw input list. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `Load` now tracks seen paths and adds a duplicate-path entry to `compileFailures`; the upstream-subscription loop iterates `_tags.Values` instead of the raw `definitions` list so it is keyed off the registered set. ### Core.VirtualTags-004 @@ -148,7 +148,7 @@ document precisely which `DriverDataType` values `CoerceResult` supports and val | Severity | Medium | | Category | Concurrency & thread safety | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagSource.cs:50-64` | -| Status | Open | +| Status | Resolved | **Description:** `SubscribeAsync` registers the per-path engine observers first (lines 52-56), then in a second loop reads the current value and fires the initial-data @@ -163,7 +163,7 @@ each path before registering the change observer for that path (or hold a per-ha lock spanning both so no engine callback interleaves). The initial value must be delivered before any subsequent change for that path. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `SubscribeAsync` now fires the initial-data callback per path before registering the change observer for that path, eliminating the out-of-order delivery race. ### Core.VirtualTags-006 @@ -223,7 +223,7 @@ expected upper bound on group evaluation time relative to the interval. | Severity | Medium | | Category | Performance & resource management | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs:81-115` | -| Status | Open | +| Status | Resolved | **Description:** `TransitiveDependentsInOrder` calls `TopologicalSort()` (a full O(V+E) Kahn pass plus a Dictionary rank build) on every invocation, and it is invoked from @@ -237,7 +237,7 @@ end of `Load` and cache it on `DependencyGraph` (invalidated by `Add` / `Clear`) `TransitiveDependentsInOrder` then reuses the cached rank map. This turns a per-event O(V+E) cost into an O(closure) cost. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `DependencyGraph` now caches the topological rank dictionary (invalidated by `Add`/`Clear`) via `GetOrBuildRank()`; `TransitiveDependentsInOrder` reuses it, reducing per-change-event cost from O(V+E) to O(closure). ### Core.VirtualTags-009 @@ -314,7 +314,7 @@ retained. | Severity | Medium | | Category | Testing coverage | | Location | `tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/` | -| Status | Open | +| Status | Resolved | **Description:** Several behaviours of the engine have no test coverage: (1) the cold-start `AreInputsReady` guard -- no test exercises an upstream that is @@ -333,7 +333,7 @@ double-to-int32 is tested); **Recommendation:** Add unit tests for each path above. Items (1), (2), and (6) directly correspond to open correctness findings and would have caught them. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added 9 unit tests covering all 7 gaps: `AreInputsReady` guard publishes `BadWaitingForInitialData` and recovers; `SetVirtualTag` cascade to dependent; write to non-registered path; `EvaluateOneAsync` before `Load` and for unregistered path; `CoerceResult` failure maps to `BadInternalError`; duplicate-path rejection; `Read`/`Subscribe` before `Load`. ### Core.VirtualTags-013 diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs index 060b1ac..74c24ab 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs @@ -31,6 +31,11 @@ public sealed class DependencyGraph private readonly Dictionary> _dependsOn = new(StringComparer.Ordinal); private readonly Dictionary> _dependents = new(StringComparer.Ordinal); + // Cached topological rank — built lazily by TransitiveDependentsInOrder and + // invalidated whenever the graph is mutated (Add / Clear). Avoids re-running + // a full O(V+E) Kahn pass on every change-cascade event. + private Dictionary? _cachedRank; + /// /// Register a node and the set of tags it depends on. Idempotent — re-adding /// the same node overwrites the prior dependency set, so re-publishing an edited @@ -58,6 +63,7 @@ public sealed class DependencyGraph _dependents[dep] = set = new HashSet(StringComparer.Ordinal); set.Add(nodeId); } + _cachedRank = null; // graph mutated — invalidate cached rank } /// Tag paths directly reads. @@ -84,9 +90,11 @@ public sealed class DependencyGraph var result = new List(); var visited = new HashSet(StringComparer.Ordinal); - var order = TopologicalSort(); - var rank = new Dictionary(StringComparer.Ordinal); - for (var i = 0; i < order.Count; i++) rank[order[i]] = i; + + // Reuse the cached rank to avoid an O(V+E) Kahn pass on every change event. + // The cache is invalidated whenever the graph is mutated (Add / Clear), so it + // is always consistent with the current graph structure. + var rank = GetOrBuildRank(); // DFS from the changed node collecting every reachable dependent. var stack = new Stack(); @@ -115,6 +123,16 @@ public sealed class DependencyGraph return result; } + private Dictionary GetOrBuildRank() + { + if (_cachedRank is not null) return _cachedRank; + var order = TopologicalSort(); + var rank = new Dictionary(order.Count, StringComparer.Ordinal); + for (var i = 0; i < order.Count; i++) rank[order[i]] = i; + _cachedRank = rank; + return rank; + } + /// Iterable of every registered node id (inputs-only tags excluded). public IReadOnlyCollection RegisteredNodes => _dependsOn.Keys; @@ -249,6 +267,7 @@ public sealed class DependencyGraph { _dependsOn.Clear(); _dependents.Clear(); + _cachedRank = null; // graph cleared — invalidate cached rank } } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs index 2e64c8d..a9f518b 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs @@ -76,8 +76,15 @@ public sealed class VirtualTagEngine : IDisposable _graph.Clear(); var compileFailures = new List(); + var seenPaths = new HashSet(StringComparer.Ordinal); foreach (var def in definitions) { + if (!seenPaths.Add(def.Path)) + { + compileFailures.Add($"{def.Path}: duplicate path — only one definition per path is allowed"); + continue; + } + try { var extraction = DependencyExtractor.Extract(def.ScriptSource); @@ -113,9 +120,10 @@ public sealed class VirtualTagEngine : IDisposable // Subscribe to every referenced upstream path (driver tags only — virtual tags // cascade internally). Seed the cache with current upstream values so first - // evaluations see something real. - var upstreamPaths = definitions - .SelectMany(d => _tags[d.Path].Reads) + // evaluations see something real. Iterate _tags.Values (the registered set) rather + // than definitions to avoid indexing by a raw input list that may contain duplicates. + var upstreamPaths = _tags.Values + .SelectMany(s => s.Reads) .Where(p => !_tags.ContainsKey(p)) .Distinct(StringComparer.Ordinal); foreach (var path in upstreamPaths) @@ -229,12 +237,18 @@ public sealed class VirtualTagEngine : IDisposable { var ctxCache = BuildReadCache(state.Reads); - // Cold-start guard — hold the prior value when any upstream input is still - // unset or Bad-quality. Evaluating with nulls would throw inside the script - // (scripts cast ctx.GetTag(path).Value directly) and produce a persistent - // BadInternalError result until the upstream cache fills. Keeping the prior - // snapshot is more honest: the virtual tag simply hasn't been computed yet. - if (!AreInputsReady(ctxCache)) return; + // Cold-start guard — when any upstream input is still unset or Bad-quality, + // publish a BadWaitingForInitialData snapshot so OPC UA clients see a defined + // quality rather than observing "not yet computed" as a stale Good value. + // Evaluating with nulls would throw inside the script (scripts cast + // ctx.GetTag(path).Value directly) and produce a persistent BadInternalError. + if (!AreInputsReady(ctxCache)) + { + var notReady = new DataValueSnapshot(null, 0x80320000u /* BadWaitingForInitialData */, null, _clock()); + _valueCache[path] = notReady; + NotifyObservers(path, notReady); + return; + } var context = new VirtualTagContext( ctxCache, @@ -247,7 +261,12 @@ public sealed class VirtualTagEngine : IDisposable { var raw = await state.Evaluator.RunAsync(context, ct).ConfigureAwait(false); var coerced = CoerceResult(raw, state.Definition.DataType); - result = new DataValueSnapshot(coerced, 0u, _clock(), _clock()); + // null from CoerceResult means the conversion threw (raw was non-null but + // not convertible to the declared type). Surface as BadInternalError so + // the OPC UA client sees a defined Bad quality rather than a Good null. + result = (raw is not null && coerced is null) + ? new DataValueSnapshot(null, 0x80020000u /* BadInternalError */, null, _clock()) + : new DataValueSnapshot(coerced, 0u, _clock(), _clock()); } catch (ScriptTimeoutException tex) { diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagSource.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagSource.cs index 5c84c1c..cb2c952 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagSource.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagSource.cs @@ -49,19 +49,20 @@ public sealed class VirtualTagSource : IReadable, ISubscribable var handle = new SubscriptionHandle(Guid.NewGuid().ToString("N")); var observers = new List(fullReferences.Count); - foreach (var path in fullReferences) - { - observers.Add(_engine.Subscribe(path, (p, snap) => - OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, p, snap)))); - } - _subs[handle.DiagnosticId] = new Subscription(handle, observers); - // OPC UA convention: emit initial-data callback for each path with the current value. + // OPC UA convention: for each path, emit the initial-data callback BEFORE + // registering the change observer. This prevents a race where an upstream change + // fires the observer between the Subscribe call and the Read call, which would + // deliver a newer change event before the initial-data event, leaving the client + // with a stale last-known value. foreach (var path in fullReferences) { var snap = _engine.Read(path); OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, path, snap)); + observers.Add(_engine.Subscribe(path, (p, s) => + OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, p, s)))); } + _subs[handle.DiagnosticId] = new Subscription(handle, observers); return Task.FromResult(handle); } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/VirtualTagEngineTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/VirtualTagEngineTests.cs index 6e76019..3e61edf 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/VirtualTagEngineTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/VirtualTagEngineTests.cs @@ -322,6 +322,188 @@ public sealed class VirtualTagEngineTests engine.Read("Rounded").Value.ShouldBe(4, "Convert.ToInt32 rounds 3.7 to 4"); } + // ----- Core.VirtualTags-012: previously-missing coverage ----- + + [Fact] + public async Task AreInputsReady_guard_publishes_BadWaitingForInitialData_when_upstream_is_bad() + { + // Arrange: upstream tag is Bad-quality (not yet available). + var up = new FakeUpstream(); + up.Set("BadIn", null!, 0x80000000u); // bad status, null value + using var engine = Build(up); + + engine.Load([new VirtualTagDefinition( + "Derived", DriverDataType.Int32, + """return (int)ctx.GetTag("BadIn").Value * 2;""")]); + + // Act: evaluate — inputs are not ready. + await engine.EvaluateAllAsync(TestContext.Current.CancellationToken); + + // Assert: tag publishes BadWaitingForInitialData, not a stale null/Good. + var result = engine.Read("Derived"); + result.StatusCode.ShouldBe(0x80320000u, "BadWaitingForInitialData expected when inputs are bad"); + result.Value.ShouldBeNull(); + } + + [Fact] + public async Task AreInputsReady_guard_publishes_BadWaitingForInitialData_then_recovers_when_upstream_becomes_good() + { + // Arrange: upstream tag starts absent (null/Bad). + var up = new FakeUpstream(); + using var engine = Build(up); + + engine.Load([new VirtualTagDefinition( + "Derived", DriverDataType.Int32, + """return (int)ctx.GetTag("In").Value + 1;""")]); + + // First evaluation: upstream not ready → BadWaitingForInitialData. + await engine.EvaluateAllAsync(TestContext.Current.CancellationToken); + engine.Read("Derived").StatusCode.ShouldBe(0x80320000u); + + // Upstream becomes available. + up.Push("In", 10); + await WaitForConditionAsync(() => engine.Read("Derived").StatusCode == 0u); + + // Tag should now have a Good value. + engine.Read("Derived").StatusCode.ShouldBe(0u); + engine.Read("Derived").Value.ShouldBe(11); + } + + [Fact] + public async Task SetVirtualTag_cascades_to_change_triggered_dependent() + { + // Arrange: "Writer" writes to "Target"; "Consumer" reads "Target" and is change-triggered. + var up = new FakeUpstream(); + up.Set("In", 3); + using var engine = Build(up); + + engine.Load([ + new VirtualTagDefinition("Target", DriverDataType.Int32, + """return 0;""", ChangeTriggered: false), + new VirtualTagDefinition("Writer", DriverDataType.Int32, + """ + var v = (int)ctx.GetTag("In").Value; + ctx.SetVirtualTag("Target", v * 10); + return v; + """), + new VirtualTagDefinition("Consumer", DriverDataType.Int32, + """return (int)ctx.GetTag("Target").Value + 1;""", + ChangeTriggered: true), + ]); + + await engine.EvaluateAllAsync(TestContext.Current.CancellationToken); + + // Writer sets Target = 30; Consumer should cascade and compute 31. + await WaitForConditionAsync(() => engine.Read("Consumer").Value is int v && v == 31); + engine.Read("Target").Value.ShouldBe(30); + engine.Read("Consumer").Value.ShouldBe(31); + } + + [Fact] + public async Task SetVirtualTag_on_non_registered_path_logs_warning_and_does_not_throw() + { + // Arrange: script writes to a path that is not a registered virtual tag. + var up = new FakeUpstream(); + up.Set("In", 1); + using var engine = Build(up); + + engine.Load([new VirtualTagDefinition( + "Writer", DriverDataType.Int32, + """ + ctx.SetVirtualTag("NonExistentPath", 99); + return (int)ctx.GetTag("In").Value; + """)]); + + // Act + Assert: should not throw; engine stays healthy. + await engine.EvaluateAllAsync(TestContext.Current.CancellationToken); + engine.Read("Writer").StatusCode.ShouldBe(0u, "engine must not fault on write to non-registered path"); + engine.Read("Writer").Value.ShouldBe(1); + await Task.CompletedTask; + } + + [Fact] + public async Task EvaluateOneAsync_throws_ArgumentException_for_unregistered_path() + { + var up = new FakeUpstream(); + using var engine = Build(up); + engine.Load([new VirtualTagDefinition("A", DriverDataType.Int32, """return 1;""")]); + + await Should.ThrowAsync(async () => + await engine.EvaluateOneAsync("NoSuchTag", TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task CoerceResult_failure_maps_to_BadInternalError() + { + // Arrange: script returns an object that cannot be coerced to the declared type. + var up = new FakeUpstream(); + using var engine = Build(up); + + engine.Load([new VirtualTagDefinition( + "Bad", DriverDataType.Int32, + // Return a non-numeric string — Convert.ToInt32("not-a-number") throws. + """return "not-a-number";""")]); + + await engine.EvaluateAllAsync(TestContext.Current.CancellationToken); + + // CoerceResult returns null on failure; the null propagates as BadInternalError. + engine.Read("Bad").StatusCode.ShouldBe(0x80020000u, "type-coercion failure must map to BadInternalError"); + engine.Read("Bad").Value.ShouldBeNull(); + } + + [Fact] + public async Task Load_rejects_duplicate_path_with_aggregated_error() + { + var up = new FakeUpstream(); + using var engine = Build(up); + + var ex = Should.Throw(() => engine.Load([ + new VirtualTagDefinition("Dup", DriverDataType.Int32, """return 1;"""), + new VirtualTagDefinition("Dup", DriverDataType.Int32, """return 2;"""), + new VirtualTagDefinition("Good", DriverDataType.Int32, """return 3;"""), + ])); + ex.Message.ShouldContain("Dup"); + ex.Message.ShouldContain("duplicate"); + await Task.CompletedTask; + } + + [Fact] + public void Read_before_Load_returns_BadNodeIdUnknown() + { + var up = new FakeUpstream(); + using var engine = Build(up); + + // Read is allowed before Load — it just returns BadNodeIdUnknown for everything. + var result = engine.Read("AnyPath"); + result.StatusCode.ShouldBe(0x80340000u, "BadNodeIdUnknown before Load"); + } + + [Fact] + public void EvaluateOneAsync_before_Load_throws_InvalidOperationException() + { + var up = new FakeUpstream(); + using var engine = Build(up); + + Should.Throw(() => + engine.EvaluateOneAsync("A").GetAwaiter().GetResult()); + } + + [Fact] + public void Subscribe_before_Load_does_not_throw() + { + // Subscribe uses GetOrAdd and does not call EnsureLoaded — it should work + // (returns an Unsub handle) without a Load. The observer won't fire because + // no tag is registered, but it must not throw. + var up = new FakeUpstream(); + using var engine = Build(up); + + var fired = false; + var sub = engine.Subscribe("AnyPath", (_, _) => fired = true); + sub.ShouldNotBeNull(); + sub.Dispose(); + fired.ShouldBeFalse("no evaluation has happened, observer must not fire"); + } + private static async Task WaitForConditionAsync(Func cond, int timeoutMs = 2000) { var deadline = DateTime.UtcNow.AddMilliseconds(timeoutMs);