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

fix(core-virtual-tags): resolve Low code-review findings (Core.VirtualTags-004,006,007,009,010,011,013)

Browse Source 
- Core.VirtualTags-004: CoerceResult now covers every scalar
  DriverDataType and throws on the default arm; Load rejects unsupported
  declared types.
- Core.VirtualTags-006: Subscribe/Unsub prune empty observer-list
  entries from _observers under the same lock with a reconfirm-on-add
  race guard.
- Core.VirtualTags-007: rewrote TimerTriggerScheduler so each TickGroup
  tracks an InFlight flag (Interlocked CAS); ticks that overlap a still-
  running tick for the same group are skipped + counted.
- Core.VirtualTags-009: DirectDependencies / DirectDependents return a
  shared static empty set on miss instead of allocating per call.
- Core.VirtualTags-010: corrected XML docs to reference the real engine
  symbols (OnUpstreamChange, CascadeAsync, etc.) instead of phantom types.
- Core.VirtualTags-011: Load now rejects scripts whose declared Writes
  target a non-registered virtual-tag path.
- Core.VirtualTags-013: DependencyCycleException renders SCC members as
  a set rather than a fabricated arrow-traversal edge path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 07:23:53 -04:00
parent 0a20de728d
commit 3f01a24b45
10 changed files with 464 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -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<KeyValuePair>.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<string>` 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<string>` 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.