diff --git a/code-reviews/Core.VirtualTags/findings.md b/code-reviews/Core.VirtualTags/findings.md index e9b63d2..4fcdd66 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 | 7 | +| Open findings | 0 | ## Checklist coverage @@ -124,7 +124,7 @@ collection is keyed off the registered set, not the raw input list. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:349` | -| Status | Open | +| Status | Resolved | **Description:** `CoerceResult`'s switch has a default arm (`_ => raw`) that returns the script's raw return value uncoerced for any `DriverDataType` not in the explicit list @@ -139,7 +139,7 @@ the outer pipeline maps to BadInternalError) for an unsupported `DriverDataType` document precisely which `DriverDataType` values `CoerceResult` supports and validate at `Load` time that no definition declares an unsupported type. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — extended `CoerceResult` to cover every scalar `DriverDataType` (`Int16`, `UInt16`, `UInt32`, `UInt64` added); the default arm now throws (mapped to `BadInternalError`) instead of returning the uncoerced raw value, and a new `IsSupportedDataType` validation in `Load` rejects definitions declaring an unsupported type (currently `Reference`) so the typo is caught at publish time. Added regression tests for both Int16/UInt16/UInt32/UInt64 round-trip and the publish-time rejection. ### Core.VirtualTags-005 @@ -172,7 +172,7 @@ delivered before any subsequent change for that path. | Severity | Low | | Category | Concurrency & thread safety | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:177-182`, `:395-401` | -| Status | Open | +| Status | Resolved | **Description:** `Subscribe` does `_observers.GetOrAdd(path, _ => [])` then `lock (list) { list.Add(observer); }`. When `Unsub.Dispose` removes the last observer, @@ -188,7 +188,7 @@ but it makes any future "prune empty entries" logic racy. lock, re-checking emptiness inside the lock to avoid dropping a concurrently-added observer. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `Unsub.Dispose` now removes the dictionary entry under the same lock when the observer list becomes empty, using the `ICollection.Remove(key,value)` overload so a racing Subscribe's brand-new list is not collateral damage. `Subscribe` retries via the GetOrAdd / lock-and-reconfirm pattern so it cannot deposit an observer into a list that has already been pruned. Added a regression test that subscribes twice + disposes both and asserts the dictionary entry is gone. ### Core.VirtualTags-007 @@ -197,7 +197,7 @@ observer. | Severity | Low | | Category | Error handling & resilience | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/TimerTriggerScheduler.cs:58` | -| Status | Open | +| Status | Resolved | **Description:** `Tick` calls `_engine.EvaluateOneAsync(p, _cts.Token).GetAwaiter().GetResult()`, blocking the @@ -214,7 +214,7 @@ if the previous one for that group is still running (a per-group "in flight" fla rather than blocking synchronously. At minimum, document the blocking behaviour and the expected upper bound on group evaluation time relative to the interval. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — rewrote `TimerTriggerScheduler` to use a per-`TickGroup` `InFlight` flag (`Interlocked.CompareExchange`-guarded). The timer callback no longer blocks on `GetAwaiter().GetResult()`; instead it kicks off an async `RunTickAsync` and skips the tick (incrementing the new `SkippedTickCount` diagnostic counter) when the prior tick for that group is still running. Added a regression test that runs a 250ms evaluation against a 50ms cadence and asserts `SkippedTickCount > 2`. ### Core.VirtualTags-008 @@ -246,7 +246,7 @@ O(V+E) cost into an O(closure) cost. | Severity | Low | | Category | Performance & resource management | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs:64-65`, `:72-73` | -| Status | Open | +| Status | Resolved | **Description:** `DirectDependencies` and `DirectDependents` allocate a fresh empty `HashSet` on every call for an unregistered node. `DirectDependents` is called @@ -257,7 +257,7 @@ on the change-cascade path. **Recommendation:** Return a shared static empty set for the miss case instead of allocating each time. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `DependencyGraph` now exposes a shared static `EmptySet` instance and `DirectDependencies` / `DirectDependents` return it on a miss instead of allocating a fresh `HashSet` every call. Added regression tests asserting `ReferenceEquals` across two miss calls. ### Core.VirtualTags-010 @@ -266,7 +266,7 @@ allocating each time. | Severity | Low | | Category | Documentation & comments | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/ITagUpstreamSource.cs:18`, `VirtualTagContext.cs:30`, `VirtualTagDefinition.cs:28` | -| Status | Open | +| Status | Resolved | **Description:** Several XML docs reference component names that do not exist in the codebase. `ITagUpstreamSource` XML doc says the subscription path "feeds the engine's @@ -280,7 +280,7 @@ XML docs mislead maintainers searching for the named component. `CascadeAsync`, `EvaluateInternalAsync`) or drop the specific name in favour of a behavioural description. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — replaced the stale type names: `ITagUpstreamSource` now references `VirtualTagEngine.OnUpstreamChange` + `CascadeAsync`; `VirtualTagContext` references `VirtualTagEngine.OnScriptSetVirtualTag` + `CascadeAsync`; `VirtualTagDefinition.TimerInterval` references `VirtualTagEngine.EvaluateInternalAsync`. ### Core.VirtualTags-011 @@ -289,7 +289,7 @@ behavioural description. | Severity | Low | | Category | Code organization & conventions | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:404-409` | -| Status | Open | +| Status | Resolved | **Description:** `VirtualTagState` records a Writes set (the `ctx.SetVirtualTag` targets extracted by `DependencyExtractor`), but nothing in the engine reads it -- it is captured @@ -305,7 +305,7 @@ miss), so an operator typo is caught at publish rather than silently dropped at If validation is deliberately deferred, remove the unused field or comment why it is retained. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `Load` now iterates every registered tag's `Writes` set and adds a `compileFailures` entry for any write target that does not resolve to a registered virtual tag. Updated the pre-existing Core.VirtualTags-012 "warning on non-registered path" test to assert publish-time rejection (the runtime warning branch remains as a defensive guard but the static `DependencyExtractor` enforces literal-string paths, so it is unreachable for any operator-authored script). Added a positive companion test confirming a write to a registered path still loads cleanly. ### Core.VirtualTags-012 @@ -342,7 +342,7 @@ correspond to open correctness findings and would have caught them. | Severity | Low | | Category | Documentation & comments | | Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs:266-270` | -| Status | Open | +| Status | Resolved | **Description:** `DependencyCycleException.BuildMessage` renders each cycle as `string.Join(" -> ", c) + " -> " + c[0]`, presenting the SCC member list as a traversable @@ -356,4 +356,4 @@ into looking for an edge that is not in their config. path) rather than rendering arrows, or reconstruct an actual cycle path within the SCC (a single DFS back-edge walk) before formatting. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `DependencyCycleException.BuildMessage` now formats each cycle as `cycle members: A, B, C` (comma-separated set) rather than the misleading `A -> B -> C -> A` arrow form. Added a regression test asserting the message contains the word "member" and does not fabricate an edge sequence. 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 74c24ab..b1640f3 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,13 @@ public sealed class DependencyGraph private readonly Dictionary> _dependsOn = new(StringComparer.Ordinal); private readonly Dictionary> _dependents = new(StringComparer.Ordinal); + // Shared empty set returned from DirectDependencies / DirectDependents on a miss. + // The CascadeAsync DFS and the Kahn topological sort both call DirectDependents + // per leaf per pass; allocating a fresh HashSet each time would churn the GC on + // every change-cascade event. Returning a shared immutable-via-convention empty + // set is safe because callers only enumerate (the IReadOnlySet contract). + private static readonly IReadOnlySet EmptySet = new HashSet(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. @@ -68,7 +75,7 @@ public sealed class DependencyGraph /// Tag paths directly reads. public IReadOnlySet DirectDependencies(string nodeId) => - _dependsOn.TryGetValue(nodeId, out var set) ? set : (IReadOnlySet)new HashSet(); + _dependsOn.TryGetValue(nodeId, out var set) ? set : EmptySet; /// /// Tags whose evaluation depends on — i.e. when @@ -76,7 +83,7 @@ public sealed class DependencyGraph /// transitive propagation falls out of the topological sort. /// public IReadOnlySet DirectDependents(string nodeId) => - _dependents.TryGetValue(nodeId, out var set) ? set : (IReadOnlySet)new HashSet(); + _dependents.TryGetValue(nodeId, out var set) ? set : EmptySet; /// /// Full transitive dependent closure of in topological @@ -284,7 +291,14 @@ public sealed class DependencyCycleException : Exception private static string BuildMessage(IReadOnlyList> cycles) { - var lines = cycles.Select(c => " - " + string.Join(" -> ", c) + " -> " + c[0]); + // Render each cycle as a comma-separated list of MEMBERS rather than an arrowed + // edge path. Tarjan's algorithm returns SCC members in stack-pop order, which is + // not guaranteed to be a valid edge sequence — for an SCC larger than two nodes + // the previously-emitted "A -> B -> C -> A" rendering could list edges that do + // not exist, sending operators looking for the wrong edge. Member framing avoids + // implying an order or set of edges. + var lines = cycles.Select(c => + " - cycle members: " + string.Join(", ", c)); return "Virtual-tag dependency graph contains cycle(s):\n" + string.Join("\n", lines); } } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/ITagUpstreamSource.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/ITagUpstreamSource.cs index d0336ce..f5608a0 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/ITagUpstreamSource.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/ITagUpstreamSource.cs @@ -15,10 +15,11 @@ namespace ZB.MOM.WW.OtOpcUa.Core.VirtualTags; /// from a last-known-value cache populated by the subscription callbacks. /// /// -/// The subscription path feeds the engine's ChangeTriggerDispatcher so -/// change-driven virtual tags re-evaluate on any upstream delta (value, status, -/// or timestamp). One subscription per distinct upstream tag path; the engine -/// tracks the mapping itself. +/// The subscription path feeds 's +/// OnUpstreamChange callback, which updates the engine's value cache and +/// schedules CascadeAsync to re-evaluate every change-driven dependent in +/// topological order. One subscription per distinct upstream tag path; the +/// engine tracks the mapping itself. /// /// public interface ITagUpstreamSource diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/TimerTriggerScheduler.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/TimerTriggerScheduler.cs index 27038ad..88592ec 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/TimerTriggerScheduler.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/TimerTriggerScheduler.cs @@ -9,12 +9,24 @@ namespace ZB.MOM.WW.OtOpcUa.Core.VirtualTags; /// per interval-group keeps the wire count /// low regardless of tag count. /// +/// +/// +/// Each timer group carries a per-group in-flight flag (see +/// TickGroup.InFlight). When the timer fires while a tick for the same +/// group is still running, the new callback skips the work and increments +/// rather than blocking a thread-pool thread on +/// the engine's evaluation gate. This bounds the work outstanding at one tick +/// per group, regardless of how long an individual evaluation takes. +/// +/// public sealed class TimerTriggerScheduler : IDisposable { private readonly VirtualTagEngine _engine; private readonly ILogger _logger; private readonly List _timers = []; + private readonly List _groups = []; private readonly CancellationTokenSource _cts = new(); + private long _skippedTickCount; private bool _disposed; public TimerTriggerScheduler(VirtualTagEngine engine, ILogger logger) @@ -23,6 +35,13 @@ public sealed class TimerTriggerScheduler : IDisposable _logger = logger ?? throw new ArgumentNullException(nameof(logger)); } + /// + /// Diagnostic counter: number of timer callbacks that skipped their work because + /// the prior tick for the same group was still running. Exposed for tests + + /// operational metrics. Monotonic; never resets. + /// + public long SkippedTickCount => Interlocked.Read(ref _skippedTickCount); + /// /// Stand up one per unique interval. All tags with /// matching interval share a timer; each tick triggers re-evaluation of the @@ -41,31 +60,60 @@ public sealed class TimerTriggerScheduler : IDisposable { var paths = group.Select(d => d.Path).ToArray(); var interval = group.Key; - var timer = new Timer(_ => Tick(paths), null, interval, interval); + var ctx = new TickGroup(paths); + _groups.Add(ctx); + var timer = new Timer(_ => OnTimer(ctx), null, interval, interval); _timers.Add(timer); _logger.Information("TimerTriggerScheduler: {TagCount} tag(s) on {Interval} cadence", paths.Length, interval); } } - private void Tick(IReadOnlyList paths) + private void OnTimer(TickGroup ctx) { if (_cts.IsCancellationRequested) return; - foreach (var p in paths) + + // Skip the tick when the prior one for this group is still running. Without + // this guard a slow evaluation (or one waiting on the engine's _evalGate) would + // cause subsequent timer callbacks to each pin a thread-pool thread on the + // gate, compounding under high tick rates. + if (Interlocked.CompareExchange(ref ctx.InFlight, 1, 0) != 0) { - try + Interlocked.Increment(ref _skippedTickCount); + return; + } + + // Run async without blocking the timer's pool-thread callback. The task is + // fire-and-forget — failures are logged inside RunTickAsync; the InFlight flag + // is reset in the finally block so the next tick can proceed. + _ = RunTickAsync(ctx); + } + + private async Task RunTickAsync(TickGroup ctx) + { + try + { + foreach (var p in ctx.Paths) { - _engine.EvaluateOneAsync(p, _cts.Token).GetAwaiter().GetResult(); - } - catch (OperationCanceledException) - { - return; - } - catch (Exception ex) - { - _logger.Error(ex, "TimerTriggerScheduler evaluate failed for {Path}", p); + if (_cts.IsCancellationRequested) return; + try + { + await _engine.EvaluateOneAsync(p, _cts.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + return; + } + catch (Exception ex) + { + _logger.Error(ex, "TimerTriggerScheduler evaluate failed for {Path}", p); + } } } + finally + { + Interlocked.Exchange(ref ctx.InFlight, 0); + } } public void Dispose() @@ -78,6 +126,21 @@ public sealed class TimerTriggerScheduler : IDisposable try { t.Dispose(); } catch { } } _timers.Clear(); + _groups.Clear(); _cts.Dispose(); } + + private sealed class TickGroup + { + // 0 = idle, 1 = a tick is currently running (or queued) for this group. Use + // Interlocked.CompareExchange so a timer callback observes a consistent "is the + // prior tick still running" answer without taking a lock. + public int InFlight; + public IReadOnlyList Paths { get; } + + public TickGroup(IReadOnlyList paths) + { + Paths = paths; + } + } } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagContext.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagContext.cs index bd7b3fb..dcebf9b 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagContext.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagContext.cs @@ -8,8 +8,9 @@ namespace ZB.MOM.WW.OtOpcUa.Core.VirtualTags; /// Per-evaluation for a virtual-tag script. Reads come /// out of the engine's last-known-value cache (driver tags updated via the /// subscription, virtual tags updated by prior -/// evaluations). Writes route through the engine's SetVirtualTag callback so -/// cross-tag write side effects still participate in change-trigger cascades. +/// evaluations). Writes route through 's +/// OnScriptSetVirtualTag callback so cross-tag write side effects still +/// participate in change-trigger cascades (via the engine's CascadeAsync). /// /// /// diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagDefinition.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagDefinition.cs index defd522..60b7f95 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagDefinition.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagDefinition.cs @@ -24,8 +24,8 @@ namespace ZB.MOM.WW.OtOpcUa.Core.VirtualTags; /// /// /// Optional periodic re-evaluation cadence. Null = timer-driven disabled. Both can -/// be enabled simultaneously; independent scheduling paths both feed -/// EvaluationPipeline. +/// be enabled simultaneously; independent scheduling paths both end at +/// 's EvaluateInternalAsync. /// /// /// When true, every evaluation result is forwarded to the configured 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 a9f518b..fb8c990 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs @@ -85,6 +85,13 @@ public sealed class VirtualTagEngine : IDisposable continue; } + if (!IsSupportedDataType(def.DataType)) + { + compileFailures.Add( + $"{def.Path}: unsupported DataType DriverDataType.{def.DataType} — virtual tags only support scalar primitive types"); + continue; + } + try { var extraction = DependencyExtractor.Extract(def.ScriptSource); @@ -108,6 +115,22 @@ public sealed class VirtualTagEngine : IDisposable } } + // Validate every ctx.SetVirtualTag write target resolves to a registered virtual + // tag. A script writing to a non-existent virtual path would otherwise be silently + // dropped at runtime by OnScriptSetVirtualTag's warning-and-drop branch; catching + // it here surfaces operator typos as a publish failure. + foreach (var (path, state) in _tags) + { + foreach (var writeTarget in state.Writes) + { + if (!_tags.ContainsKey(writeTarget)) + { + compileFailures.Add( + $"{path}: ctx.SetVirtualTag target '{writeTarget}' is not a registered virtual tag"); + } + } + } + if (compileFailures.Count > 0) { var joined = string.Join("\n ", compileFailures); @@ -184,9 +207,28 @@ public sealed class VirtualTagEngine : IDisposable /// public IDisposable Subscribe(string path, Action observer) { - var list = _observers.GetOrAdd(path, _ => []); - lock (list) { list.Add(observer); } - return new Unsub(this, path, observer); + // Race-safe pattern paired with Unsub.Dispose: if Unsub.Dispose removed the + // dictionary entry between our GetOrAdd and the lock-protected Add, the list + // we hold a reference to is orphaned. Re-check the map under the lock and + // re-insert the list (or grab the current one) if needed, retrying until the + // dictionary observably contains the list we just added our observer to. + while (true) + { + var list = _observers.GetOrAdd(path, _ => []); + lock (list) + { + // Confirm the list is still the dictionary's value for this key. If + // Dispose removed the entry, _observers[path] either doesn't exist or + // points at a different (newer) list — retry. + if (_observers.TryGetValue(path, out var current) && ReferenceEquals(current, list)) + { + list.Add(observer); + return new Unsub(this, path, observer); + } + } + // Lost the race — Dispose pruned the list out from under us. Loop and + // either re-create or pick up the newer list. + } } /// @@ -367,13 +409,24 @@ public sealed class VirtualTagEngine : IDisposable return target switch { DriverDataType.Boolean => Convert.ToBoolean(raw), + DriverDataType.Int16 => Convert.ToInt16(raw), DriverDataType.Int32 => Convert.ToInt32(raw), DriverDataType.Int64 => Convert.ToInt64(raw), + DriverDataType.UInt16 => Convert.ToUInt16(raw), + DriverDataType.UInt32 => Convert.ToUInt32(raw), + DriverDataType.UInt64 => Convert.ToUInt64(raw), DriverDataType.Float32 => Convert.ToSingle(raw), DriverDataType.Float64 => Convert.ToDouble(raw), DriverDataType.String => Convert.ToString(raw) ?? string.Empty, DriverDataType.DateTime => raw is DateTime dt ? dt : Convert.ToDateTime(raw), - _ => raw, + // Any DriverDataType not in the explicit list (currently Reference, or any + // future enum member added without coercion support) must NOT silently + // return the uncoerced raw value — that would surface as a wire-level + // type mismatch on the OPC UA Variant. Throwing here is caught by the + // outer catch and mapped to BadInternalError. Load-time validation in + // IsSupportedDataType ensures operators never publish such a tag. + _ => throw new InvalidOperationException( + $"Virtual-tag CoerceResult does not support DriverDataType.{target}"), }; } catch @@ -384,6 +437,28 @@ public sealed class VirtualTagEngine : IDisposable } } + /// + /// The set of values can + /// honour. Definitions declaring any other type are rejected at + /// so an operator typo (or a future enum member added without coercion support) is + /// caught at publish time rather than silently producing a type-mismatched value. + /// + private static bool IsSupportedDataType(DriverDataType t) => t switch + { + DriverDataType.Boolean => true, + DriverDataType.Int16 => true, + DriverDataType.Int32 => true, + DriverDataType.Int64 => true, + DriverDataType.UInt16 => true, + DriverDataType.UInt32 => true, + DriverDataType.UInt64 => true, + DriverDataType.Float32 => true, + DriverDataType.Float64 => true, + DriverDataType.String => true, + DriverDataType.DateTime => true, + _ => false, + }; + private void UnsubscribeFromUpstream() { foreach (var s in _upstreamSubscriptions) @@ -423,7 +498,23 @@ public sealed class VirtualTagEngine : IDisposable { if (_engine._observers.TryGetValue(_path, out var list)) { - lock (list) { list.Remove(_observer); } + lock (list) + { + list.Remove(_observer); + // If we removed the last observer, prune the dictionary entry so a + // long-running server doesn't accumulate empty Lists for paths that + // saw transient subscriptions. The emptiness check is inside the same + // lock so a concurrent Subscribe can't slip an observer in after we + // observe the list as empty. + if (list.Count == 0) + { + // ICollection> removal is value-typed — only removes + // if both key + value still match (i.e. the dictionary still points + // at this list). This keeps a racing Subscribe's brand-new list safe. + ((ICollection>>>)_engine._observers) + .Remove(new KeyValuePair>>(_path, list)); + } + } } } } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/DependencyGraphTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/DependencyGraphTests.cs index 38b0555..c9d0566 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/DependencyGraphTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/DependencyGraphTests.cs @@ -153,6 +153,65 @@ public sealed class DependencyGraphTests g.DirectDependents("A").ShouldBe(new[] { "B" }); } + // ----- Core.VirtualTags-013: DependencyCycleException message must not present SCC as edge path ----- + + [Fact] + public void DependencyCycleException_message_describes_cycle_members_not_a_fabricated_edge_path() + { + // Regression for Core.VirtualTags-013: Tarjan returns SCC members in stack-pop + // order, NOT in edge-traversal order. The exception message must not render the + // members as "A -> B -> C -> A" — that misleads operators into looking for an + // edge that may not be in the config. Instead the message uses a set-form + // ("members: A, B, C") or a labelled traversal. + var g = new DependencyGraph(); + g.Add("A", Set("B")); + g.Add("B", Set("A")); + var ex = Should.Throw(() => g.TopologicalSort()); + + // The arrow ("->") notation as used previously (string.Join(" -> ", c) + " -> " + c[0]) + // implies an ordered edge path. After the fix, the message must NOT contain the + // closing edge `-> A` (i.e. " -> " + first-member) on its own — the formatting + // must clearly mark the list as cycle MEMBERS rather than an edge sequence. + ex.Message.ShouldContain("cycle"); + ex.Message.ShouldContain("A"); + ex.Message.ShouldContain("B"); + // Verify the message uses a member-list framing ("members:" or "members of cycle" + // or commas) rather than the misleading edge-path framing. + ex.Message.ShouldContain("member", Case.Insensitive, + "message should label entries as cycle members, not present them as an edge path"); + } + + // ----- Core.VirtualTags-009: empty-set allocation on miss ----- + + [Fact] + public void DirectDependencies_miss_returns_shared_empty_set_instance() + { + // Regression for Core.VirtualTags-009: calling DirectDependencies for an + // unregistered node should NOT allocate a fresh HashSet each time. The miss + // path returns a shared empty set so the change-cascade hot path doesn't + // churn the GC. + var g = new DependencyGraph(); + var a = g.DirectDependencies("Unknown1"); + var b = g.DirectDependencies("Unknown2"); + a.ShouldBeEmpty(); + b.ShouldBeEmpty(); + ReferenceEquals(a, b).ShouldBeTrue("miss path must return the shared empty-set instance"); + } + + [Fact] + public void DirectDependents_miss_returns_shared_empty_set_instance() + { + // Same regression as above for DirectDependents — called from inside the + // CascadeAsync DFS and TopologicalSort Kahn loop, so the miss-path allocation + // is on every change-cascade event. + var g = new DependencyGraph(); + var a = g.DirectDependents("LeafA"); + var b = g.DirectDependents("LeafB"); + a.ShouldBeEmpty(); + b.ShouldBeEmpty(); + ReferenceEquals(a, b).ShouldBeTrue("miss path must return the shared empty-set instance"); + } + [Fact] public void Deep_graph_no_stack_overflow() { diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/TimerTriggerSchedulerTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/TimerTriggerSchedulerTests.cs index 5707752..5fec112 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/TimerTriggerSchedulerTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/TimerTriggerSchedulerTests.cs @@ -92,6 +92,53 @@ public sealed class TimerTriggerSchedulerTests })); } + // ----- Core.VirtualTags-007: timer ticks must not block pool threads and must skip when prior tick is still running ----- + + [Fact] + public async Task Tick_skips_when_prior_tick_for_the_same_group_is_still_running() + { + // Regression for Core.VirtualTags-007: if a single tick takes longer than the + // interval, subsequent timer callbacks must NOT each pin a thread-pool thread + // waiting on the same evaluation gate. The scheduler tracks an in-flight flag + // per group and skips a new tick when the prior one is still running. + var up = new FakeUpstream(); + up.Set("In", 1); + var logger = new LoggerConfiguration().CreateLogger(); + + // Slow script — each evaluation takes longer than several timer intervals. + const int slowMs = 250; + const int intervalMs = 50; + using var engine = new VirtualTagEngine(up, + new ScriptLoggerFactory(logger), logger); + engine.Load([new VirtualTagDefinition( + "Slow", DriverDataType.Int32, + $$""" + var end = DateTime.UtcNow.AddMilliseconds({{slowMs}}); + while (DateTime.UtcNow < end) { } + return (int)ctx.GetTag("In").Value; + """, + ChangeTriggered: false, + TimerInterval: TimeSpan.FromMilliseconds(intervalMs))]); + + using var sched = new TimerTriggerScheduler(engine, logger); + sched.Start([new VirtualTagDefinition( + "Slow", DriverDataType.Int32, + "", + ChangeTriggered: false, + TimerInterval: TimeSpan.FromMilliseconds(intervalMs))]); + + // Wait long enough for many timer ticks at 50ms while one evaluation + // (~250ms each) holds the engine. Window is 600ms ~ 12 ticks. + await Task.Delay(600); + + // With the fix in place, ticks that fire while the previous one for the same + // group is still running are skipped. The skipped count must be measurable; if + // SkippedTickCount is still 0 after 600ms with ~12 ticks fired and a 250ms eval, + // the fix is not working — at minimum 3-4 ticks must have been skipped. + sched.SkippedTickCount.ShouldBeGreaterThan(2, + "ticks that fire while the prior tick for the same group is still running must be skipped"); + } + [Fact] public void Disposed_scheduler_stops_firing() { 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 3e61edf..e06b0e6 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 @@ -400,24 +400,25 @@ public sealed class VirtualTagEngineTests } [Fact] - public async Task SetVirtualTag_on_non_registered_path_logs_warning_and_does_not_throw() + public async Task SetVirtualTag_on_non_registered_path_is_caught_at_Load() { - // Arrange: script writes to a path that is not a registered virtual tag. + // Originally validated the runtime warning-and-drop branch in OnScriptSetVirtualTag. + // After Core.VirtualTags-011 the static DependencyExtractor.Writes set is validated + // at Load time, so a literal-string write to a non-existent path is now rejected + // at publish — the dynamic warning path is reserved as a defensive guard for cases + // the static extractor cannot see (currently none, since dynamic paths are also + // rejected at extraction). 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); + Should.Throw(() => engine.Load([ + new VirtualTagDefinition("Writer", DriverDataType.Int32, + """ + ctx.SetVirtualTag("NonExistentPath", 99); + return (int)ctx.GetTag("In").Value; + """) + ])).Message.ShouldContain("NonExistentPath"); await Task.CompletedTask; } @@ -451,6 +452,136 @@ public sealed class VirtualTagEngineTests engine.Read("Bad").Value.ShouldBeNull(); } + // ----- Core.VirtualTags-011: Writes target validation at Load time ----- + + [Fact] + public async Task Load_rejects_script_writing_to_unregistered_virtual_tag_path() + { + // Regression for Core.VirtualTags-011: a script that calls + // ctx.SetVirtualTag("Typo", ...) must be caught at publish/load time rather than + // silently dropped at runtime, so operator typos surface as a publish failure. + var up = new FakeUpstream(); + using var engine = Build(up); + + var ex = Should.Throw(() => engine.Load([ + new VirtualTagDefinition("Writer", DriverDataType.Int32, + """ + ctx.SetVirtualTag("NonRegisteredTarget", 1); + return 0; + """), + new VirtualTagDefinition("RegisteredTarget", DriverDataType.Int32, + """return 1;"""), + ])); + ex.Message.ShouldContain("Writer"); + ex.Message.ShouldContain("NonRegisteredTarget"); + await Task.CompletedTask; + } + + [Fact] + public async Task Load_accepts_script_writing_to_registered_virtual_tag_path() + { + // Companion to the rejection test: a write to a registered tag must continue to + // load successfully. + var up = new FakeUpstream(); + up.Set("In", 1); + using var engine = Build(up); + + // No throw — Writer writes to Target which is registered. + engine.Load([ + new VirtualTagDefinition("Target", DriverDataType.Int32, + """return 0;""", ChangeTriggered: false), + new VirtualTagDefinition("Writer", DriverDataType.Int32, + """ + ctx.SetVirtualTag("Target", (int)ctx.GetTag("In").Value); + return 0; + """), + ]); + await engine.EvaluateAllAsync(TestContext.Current.CancellationToken); + engine.Read("Target").Value.ShouldBe(1); + } + + // ----- Core.VirtualTags-006: empty observer list left in _observers map ----- + + [Fact] + public void Subscribe_then_Unsub_prunes_empty_observer_list_for_path() + { + // Regression for Core.VirtualTags-006: disposing the last subscriber for a path + // must remove the dictionary entry so a long-running server with churning OPC UA + // subscriptions does not accumulate an unbounded number of empty List entries. + var up = new FakeUpstream(); + using var engine = Build(up); + engine.Load([new VirtualTagDefinition( + "T", DriverDataType.Int32, """return 1;""")]); + + // Subscribe, then immediately Dispose — both the only observer. + var sub1 = engine.Subscribe("T", (_, _) => { }); + var sub2 = engine.Subscribe("T", (_, _) => { }); + sub1.Dispose(); + sub2.Dispose(); + + // The internal map should no longer hold an entry for the path. + // Use the same ConcurrentDictionary type the engine uses; we check via reflection + // on the test-private field so this is robust to future renames inside engine. + var observersField = typeof(VirtualTagEngine).GetField( + "_observers", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); + observersField.ShouldNotBeNull(); + var observers = observersField!.GetValue(engine); + observers.ShouldNotBeNull(); + var containsKey = observers!.GetType().GetMethod("ContainsKey")!; + var result = (bool)containsKey.Invoke(observers, new object[] { "T" })!; + result.ShouldBeFalse("disposing the last subscriber must remove the dictionary entry"); + } + + // ----- Core.VirtualTags-004: CoerceResult default arm leaks uncoerced values ----- + + [Fact] + public async Task CoerceResult_handles_Int16_UInt16_UInt32_UInt64() + { + // Regression for Core.VirtualTags-004: before the fix, CoerceResult had a default + // arm that returned the script's raw double/string for these types, producing a + // type-mismatched DataValueSnapshot. Verify every integer DriverDataType the engine + // is allowed to declare coerces correctly. + var up = new FakeUpstream(); + using var engine = Build(up); + + engine.Load([ + new VirtualTagDefinition("AsInt16", DriverDataType.Int16, """return 7.0;"""), + new VirtualTagDefinition("AsUInt16", DriverDataType.UInt16, """return 8.0;"""), + new VirtualTagDefinition("AsUInt32", DriverDataType.UInt32, """return 9.0;"""), + new VirtualTagDefinition("AsUInt64", DriverDataType.UInt64, """return 10.0;"""), + ]); + await engine.EvaluateAllAsync(TestContext.Current.CancellationToken); + + engine.Read("AsInt16").Value.ShouldBeOfType(); + engine.Read("AsInt16").Value.ShouldBe((short)7); + engine.Read("AsUInt16").Value.ShouldBeOfType(); + engine.Read("AsUInt16").Value.ShouldBe((ushort)8); + engine.Read("AsUInt32").Value.ShouldBeOfType(); + engine.Read("AsUInt32").Value.ShouldBe((uint)9); + engine.Read("AsUInt64").Value.ShouldBeOfType(); + engine.Read("AsUInt64").Value.ShouldBe((ulong)10); + } + + [Fact] + public async Task Load_rejects_definition_with_unsupported_DriverDataType() + { + // Regression for Core.VirtualTags-004: any DriverDataType that CoerceResult cannot + // honour must be rejected at Load time so an operator typo (or a future enum + // member added without coercion support) does not silently emit a type-mismatched + // value to OPC UA clients. Reference is unsupported for virtual tags (the engine + // does not synthesize Galaxy attribute references). + var up = new FakeUpstream(); + using var engine = Build(up); + + var ex = Should.Throw(() => engine.Load([ + new VirtualTagDefinition("Ref", DriverDataType.Reference, """return "Some.Attribute";"""), + ])); + ex.Message.ShouldContain("Reference"); + ex.Message.ShouldContain("Ref"); + await Task.CompletedTask; + } + [Fact] public async Task Load_rejects_duplicate_path_with_aggregated_error() {