dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(virtual-tags): resolve Medium code-review findings (Core.VirtualTags-002, -003, -005, -008, -012)

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 08:31:49 -04:00
parent 11612900ba
commit 3d8c285034
5 changed files with 252 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
code-reviews/Core.VirtualTags/findings.md
View File

@@ -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

25
src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs
View File

@@ -31,6 +31,11 @@ public sealed class DependencyGraph
private readonly Dictionary<string, HashSet<string>> _dependsOn = new(StringComparer.Ordinal);
private readonly Dictionary<string, HashSet<string>> _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<string, int>? _cachedRank;
/// <summary>
/// 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<string>(StringComparer.Ordinal);
set.Add(nodeId);
}
_cachedRank = null; // graph mutated — invalidate cached rank
}
/// <summary>Tag paths <paramref name="nodeId"/> directly reads.</summary>
@@ -84,9 +90,11 @@ public sealed class DependencyGraph
var result = new List<string>();
var visited = new HashSet<string>(StringComparer.Ordinal);
var order = TopologicalSort();
var rank = new Dictionary<string, int>(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<string>();
@@ -115,6 +123,16 @@ public sealed class DependencyGraph
return result;
}
private Dictionary<string, int> GetOrBuildRank()
{
if (_cachedRank is not null) return _cachedRank;
var order = TopologicalSort();
var rank = new Dictionary<string, int>(order.Count, StringComparer.Ordinal);
for (var i = 0; i < order.Count; i++) rank[order[i]] = i;
_cachedRank = rank;
return rank;
}
/// <summary>Iterable of every registered node id (inputs-only tags excluded).</summary>
public IReadOnlyCollection<string> RegisteredNodes => _dependsOn.Keys;
@@ -249,6 +267,7 @@ public sealed class DependencyGraph
{
_dependsOn.Clear();
_dependents.Clear();
_cachedRank = null; // graph cleared — invalidate cached rank
}
}

39
src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs
View File

@@ -76,8 +76,15 @@ public sealed class VirtualTagEngine : IDisposable
_graph.Clear();
var compileFailures = new List<string>();
var seenPaths = new HashSet<string>(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)
{

15
src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagSource.cs
View File

@@ -49,19 +49,20 @@ public sealed class VirtualTagSource : IReadable, ISubscribable
var handle = new SubscriptionHandle(Guid.NewGuid().ToString("N"));
var observers = new List<IDisposable>(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<ISubscriptionHandle>(handle);
}

182
tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/VirtualTagEngineTests.cs
View File

@@ -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<ArgumentException>(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<InvalidOperationException>(() => 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<InvalidOperationException>(() =>
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<bool> cond, int timeoutMs = 2000)
{
var deadline = DateTime.UtcNow.AddMilliseconds(timeoutMs);