Joseph Doherty
176 lines
21 KiB
Markdown
176 lines
21 KiB
Markdown
# Review 01 — `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer` (master @ d51d35bd)
|
||
|
||
Scope: `OtOpcUaNodeManager` (6,988 lines), `AddressSpaceApplier`, `AddressSpaceComposer`, `AddressSpacePlan`/`Planner`,
|
||
`AddressSpaceChangeClassifier`, `SdkAddressSpaceSink`, `OtOpcUaSdkServer`, `OpcUaApplicationHost`, `Security/*`,
|
||
`HistoryContinuationStore`, the HistoryRead helpers (`ReverseWalk`, `ReverseRawReader`, `ReverseEventsReader`,
|
||
`HistoryBounds`, `HistoryPaging`, `RawPageLimit`), `AdvertisedAggregateFunctions`, `SdkServiceLevelPublisher`.
|
||
|
||
SDK behaviour claims below were verified against the pinned SDK sources
|
||
(`~/Desktop/OpcUaTestServer/external/UA-.NETStandard`, 1.5.378 line): `NodeState.cs`,
|
||
`Diagnostics/CustomNodeManager.cs`, `NodeManager/MonitoredItem/MonitoredNodeMonitoredItemManager.cs`,
|
||
`NodeManager/MasterNodeManager.cs`. All paths are absolute; line numbers are from the current master.
|
||
|
||
---
|
||
|
||
## 1. Memory / resource leaks
|
||
|
||
### L1. Lazy `CreateVariable` resurrects removed (or never-declared) NodeIds as orphan variables under the shared root — and then poisons a later re-add
|
||
- **Severity:** High · **Confidence:** High (mechanism) / Medium (trigger frequency)
|
||
- **Location:** `/Users/dohertj2/Desktop/OtOpcUa/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs:937-945` (`WriteValueLocked`), `:1059-1071` (`WriteAlarmCondition` fallback), `:6964-6986` (`CreateVariable`), `:2974-2975` (`EnsureVariable` early return)
|
||
- **Evidence:**
|
||
```csharp
|
||
if (!_variables.TryGetValue(key, out var variable))
|
||
{
|
||
variable = CreateVariable(nodeId, ns); // parented under _root, DataType=BaseDataType, CurrentRead only
|
||
_variables[key] = variable;
|
||
}
|
||
```
|
||
and `EnsureVariable`: `if (_variables.ContainsKey(key)) return;`
|
||
- **Failure scenario:** `WriteStatuses` was deliberately made non-creating because "the caller's per-driver ref index can legitimately name a node a surgical remove has just dropped" (`:825-828`). `WriteValues`/`WriteValue` still create. So a driver publish (or the UNS fan-out) that races a surgical `RemoveVariableNode` re-creates `s=<RawPath>` (and `s=<Area/Line/Equip/Name>`) as a flat, read-only, `BaseDataType` variable **directly under `OtOpcUa`**, browsable, with no `OnWriteValue`, no historian registration, no `_arrayOnScalarRefused`. It stays until the next *full* rebuild — surgical deploys made that rare. Worse: if the same tag is later re-added surgically, `EnsureVariable` early-returns on the phantom, so the "re-added" tag permanently keeps the phantom's wrong parent/DataType/writability. The planner never names the phantom, so no surgical remove ever reaches it.
|
||
- **Suggested fix:** Make `WriteValueLocked` non-creating (mirror `WriteStatusLocked`: unknown key ⇒ skipped, counted, Debug-logged). The create-on-first-write path predates `EnsureVariable` and has no legitimate production producer left. If some test relies on it, gate it behind an explicit `AllowLazyNodeCreation` that defaults off.
|
||
|
||
### L2. Stale `MonitoredNode2` survives a rebuild / remove+recreate: new subscriptions bind to the dead `NodeState`, and ACL "restore" re-attaches to dead nodes
|
||
- **Severity:** High · **Confidence:** High
|
||
- **Location:** `OtOpcUaNodeManager.cs:3471-3537` (`RebuildAddressSpace`), `:3573-3607` (`RemoveVariableNode`), `:4801` / `:4889` (revalidation restore). SDK: `MonitoredNodeMonitoredItemManager.cs:75-83`.
|
||
- **Evidence (SDK):**
|
||
```csharp
|
||
if (!MonitoredNodes.TryGetValue(handle.Node.NodeId, out MonitoredNode2 monitoredNode)) { ... new MonitoredNode2(m_nodeManager, cachedNode); }
|
||
handle.Node = monitoredNode.Node; // <-- the OLD NodeState
|
||
```
|
||
`RebuildAddressSpace`/`RemoveVariableNode` detach nodes from `PredefinedNodes` and the parent, but never touch the manager's `MonitoredNodes` map. `DeleteMonitoredItem` only drops the entry when its item count hits zero.
|
||
- **Failure scenario:** Client A (e.g. ScadaBridge) holds an item on `Plant/Modbus/dev1/Speed`. A deploy rebuilds (or surgically removes + re-adds) that node. A's item goes silent (known). Now ANY session — A after reconnecting, or a UaExpert operator — creates a new item on the same NodeId: the SDK reuses the existing `MonitoredNode2` and sets `handle.Node` to the **dead** NodeState, so the new item never receives a value either, with `Good` create status. Only when *every* session has deleted its items on that NodeId does a fresh create work. Same class: `RevalidateMonitoredItems` restore does `tracked.Handle.MonitoredNode?.Add(item)` and `RevalidateEventItems` calls `SubscribeToEvents(SystemContext, tracked.Handle.Node, …)` — after a rebuild both re-attach to dead nodes. Memory-wise each live item pins its dead subtree via `LiveItem.Handle` (bounded by item count, but it's the whole detached subtree).
|
||
- **Suggested fix:** In `RebuildAddressSpace`, `RemoveVariableNode`, `RemoveAlarmConditionNode`, `RemoveEquipmentSubtree`: for each removed NodeId, `MonitoredNodes.TryGetValue(nodeId, out var mn)` → queue `BadNodeIdUnknown` to each `mn.DataChangeMonitoredItems`, then `MonitoredNodes.Remove(nodeId)` (and drop the matching `_liveItems`/`_eventItems` entries). For a rebuild that recreates the same NodeId you could instead re-point the `MonitoredNode2` to the new `NodeState` — but removing it and letting clients re-create is simpler and matches the documented "rebuild kills subscriptions" contract while making re-create actually work.
|
||
|
||
### L3. `OnEquipmentTagWrite` continuation never observes a faulted write task
|
||
- **Severity:** Low · **Confidence:** High
|
||
- **Location:** `OtOpcUaNodeManager.cs:2202-2209`
|
||
- **Evidence:** `var outcome = t.IsCompletedSuccessfully ? t.Result : new NodeWriteOutcome(false, "write dispatch faulted");` — `t.Exception` is never read on the faulted branch.
|
||
- **Failure scenario:** Every faulted `WriteAsync` (gateway Ask timeout/exception) raises `TaskScheduler.UnobservedTaskException` at finalization; with a handler that logs, each failed device write becomes a late, context-free error line; the real reason is never logged here either (`outcome.Reason` is the constant string).
|
||
- **Suggested fix:** On the non-success branch read `t.Exception?.GetBaseException()` into the reason and log it (the pattern `AddressSpaceApplier.ProvisionHistorizedTags:727-733` already uses).
|
||
|
||
### L4. Legacy `bool[2]` placeholder path is reachable in production and transiently shadows the equipment folder in `PredefinedNodes`
|
||
- **Severity:** Medium · **Confidence:** Medium
|
||
- **Location:** `OtOpcUaNodeManager.cs:1059-1071`; producer `AddressSpaceApplier.cs:100-103` (`SafeWriteAlarmCondition(eq.EquipmentId, RemovedConditionState, …)`).
|
||
- **Evidence:** The applier writes `RemovedConditionState` against `eq.EquipmentId` for every removed equipment; an equipment id is a *folder*, never a condition, so `_alarmConditions` misses and the "legacy two-element placeholder … (and the existing unit tests)" branch runs `CreateVariable(alarmNodeId, ns)` + `AddPredefinedNode`. SDK `AddPredefinedNode` is `PredefinedNodes.AddOrUpdate(...)` (`CustomNodeManager.cs:586`), so the phantom variable **replaces the equipment folder's `PredefinedNodes` entry** (same realm + same `s=` id) until the folder loop in `RemoveEquipmentSubtree` (or the rebuild) removes it.
|
||
- **Failure scenario:** During a PureRemove/Rebuild apply a concurrent Read/Browse of the equipment folder resolves to a `bool[2]` variable. The phantom is cleaned up by the same apply on every path I traced (InScope matches `key.Id == equipmentNodeId`; an early `false` ratchets to rebuild), so it is transient — but it is a production path running through a self-described test fallback, and it does one needless `ClearChangeMasks` per removed equipment.
|
||
- **Suggested fix:** Make `WriteAlarmCondition` a logged no-op for unknown ids (as `WriteAlarmQuality` already is), and drop the applier's equipment-id condition write (an equipment never has a condition node).
|
||
|
||
### L5. (bounded, note) `_notifierFolders` / `_eventNotifierSources` not demoted on `RemoveAlarmConditionNode`
|
||
- **Severity:** Low · **Confidence:** High
|
||
- **Location:** `OtOpcUaNodeManager.cs:3637-3664`
|
||
- Documented and bounded by folder count; a raw device/group folder promoted for a native alarm stays a root notifier (and an advertised event-history source) after its last condition is removed until a rebuild. Harmless except `HistoryReadEvents` on it answers from the historian for a folder with no alarms. No action needed beyond awareness.
|
||
|
||
---
|
||
|
||
## 2. Unimplemented / stubbed / dormant
|
||
|
||
### U1. `HistoryReadEvents` projects `EventType`, `SourceNode`, `ConditionName` (and every other field) as `Variant.Null`
|
||
- **Severity:** Low · **Confidence:** High
|
||
- **Location:** `OtOpcUaNodeManager.cs:5942-5957`
|
||
- Only `EventId/SourceName/Time/ReceiveTime/Message/Severity` are populated. A client whose select clause includes `EventType` (most generic alarm viewers) gets nulls; a where-clause on `OfType` cannot be honoured. Documented as "spec-conformant", but it is a functional gap for historical alarm browsing. Fix: `HistoricalEvent` would need to carry `EventType`/`ConditionName`; map `SourceNode` from the source name when resolvable.
|
||
|
||
### U2. Equipment-tag materialisation + its surgical branch are dead code in v3
|
||
- **Severity:** Low · **Confidence:** High
|
||
- **Location:** `AddressSpaceComposer.cs:590-592` (`EquipmentTags = Array.Empty<EquipmentTagPlan>()` always), `AddressSpaceApplier.cs:197-215` (surgical `ChangedEquipmentTags` loop), `:1098-1161` (`MaterialiseEquipmentTags`), `AddressSpaceChangeClassifier.cs:162-172`.
|
||
- The composer never emits equipment tags, so the applier's `MaterialiseEquipmentTags` pass, the `UnsEquipmentVar`/alarm-under-equipment branch and `TagDeltaIsSurgicalEligible` are unreachable in production (kept "structurally empty"). Not a bug; it is maintained, tested dead weight that still participates in `IsEmpty`/classification. Candidate for removal.
|
||
|
||
### U3. Historian capability bits are fixed at promotion/materialise time
|
||
- **Severity:** Low · **Confidence:** High
|
||
- **Location:** `OtOpcUaNodeManager.cs:2831-2848` (`EnsureFolderIsEventNotifier` registers `_eventNotifierSources` only if `_historianDataSource is not NullHistorianDataSource` *now*), `:2987` / `:3084` (`HistoryWrite` bit depends on `_historianDataUpdater` *now*).
|
||
- Documented. Consequence: a `HistorianDataSource`/`HistorianDataUpdater` assigned after the first apply leaves existing folders without event history / HistoryUpdate until a full rebuild, and `SetHistorianDataSource(null)` later does **not** remove the HistoryRead bit from already-promoted folders (they then answer `BadHistoryOperationUnsupported`). Acceptable given the Host ordering, but there is no runtime guard that the ordering held.
|
||
|
||
### U4. Native conditions: Confirm / shelve / Enable / Disable ⇒ `BadNotSupported`; HistoryUpdate events/structure arms not overridden
|
||
- **Severity:** Low · **Confidence:** High
|
||
- **Location:** `:1496-1540`, base `HistoryUpdate` dispatch (not overridden for `UpdateEventDetails`/`UpdateStructureDataDetails`).
|
||
- Both are deliberate and documented; listed so the parent does not re-investigate.
|
||
|
||
### U5. Test seams present in production type (not dormant DI, just noting)
|
||
- `HistoryContinuationStore` internal setter + `InMemoryHistoryContinuationStore` (`HistoryContinuationStore.cs:104-158`), `SessionAclDecisionCache.GetOrAdd` ("production has no caller"), `AllEventsMonitoringConditionVisits`, `TrackLiveMonitoredItem` internal. All default to the production implementation; none alters behaviour. Fine.
|
||
|
||
---
|
||
|
||
## 3. Performance limiters
|
||
|
||
### P1. `RebuildAddressSpace` is O(k²) per folder because `NodeState.RemoveChild` is a linear scan + `RemoveAt`
|
||
- **Severity:** Medium · **Confidence:** High
|
||
- **Location:** `OtOpcUaNodeManager.cs:3475-3479`, `:3495-3499`, `:3513-3517`; SDK `NodeState.cs:4247-4265`.
|
||
- **Evidence:** `foreach (var v in _variables.Values) { v.Parent?.RemoveChild(v); PredefinedNodes?.Remove(v.NodeId); }` and SDK `RemoveChild` walks `m_children` with `ReferenceEquals` then `RemoveAt(ii)` (element shift). Removing all k children of one folder, in map order, costs ~k²/2 compares + shifts, all under `Lock`.
|
||
- **Failure scenario:** A device folder with 10k tags ≈ 50M compares under the manager lock on every full rebuild (alarm-bearing delta, container rename, etc.), stalling every Read/Browse/publish for the duration; UNS equipment folders with many references multiply it. `ForgetPredefinedSubtree` also allocates one `List<BaseInstanceState>` per node recursively.
|
||
- **Suggested fix:** Detach whole subtrees instead of leaves: remove top-level folders from `_root` once (children go with them), then clear the maps and `PredefinedNodes` keys in bulk; or iterate `folder.GetChildren` and build a new child list. Keep `PredefinedNodes.Remove` per id (dictionary, O(1)).
|
||
|
||
### P2. `RemoveEquipmentSubtree` scans every map on every call ⇒ O(M·N) for M removed equipment
|
||
- **Severity:** Medium · **Confidence:** High
|
||
- **Location:** `OtOpcUaNodeManager.cs:3685-3752`
|
||
- **Evidence:** `_variables.Keys.Where(InScope).ToList()`, same for `_alarmConditions`, `_notifierFolders`, `_eventNotifierSources`, `_folders`, plus `foreach (var wiring in _alarmNotifierWiring.Values) wiring.Folders.Where(...)` — each a full scan with `StartsWith` per key, under `Lock`.
|
||
- **Failure scenario:** A PureRemove deploy that drops 200 equipment on a 50k-node server does 200 × ~6 full-map scans ≈ 60M key comparisons under the lock, on the pinned publish thread, serialising all SDK service calls.
|
||
- **Suggested fix:** Walk the equipment `FolderState`'s children (the SDK tree already has them) to enumerate descendants, or maintain a per-equipment child index populated by `EnsureVariable`/`EnsureFolder`/`MaterialiseAlarmCondition`.
|
||
|
||
### P3. `ReconcileAllEventsMonitoring` runs a full O(C) reconcile on every `RemoveAlarmConditionNode` and every all-events (un)subscribe
|
||
- **Severity:** Medium · **Confidence:** High
|
||
- **Location:** `OtOpcUaNodeManager.cs:3659`, `:3714`, `:4240`, `:4298-4322`
|
||
- **Evidence:** Builds a new `HashSet` over `_alarmConditions.Values` plus `.Where().ToList()` each call. The materialise side was already made incremental ("made a deploy of N alarms O(N²) under Lock") but the removal side was not.
|
||
- **Failure scenario:** Surgically removing M alarm-bearing raw tags or scripted alarms costs O(M·C) under `Lock`; a client that creates/modifies many `i=2253` items (e.g. per-subscription) pays O(C) per call.
|
||
- **Suggested fix:** Add `LowerAllEventsMonitoring(condition)` (mirror of `RaiseAllEventsMonitoring`) for the two surgical removal sites; keep the full reconcile for rebuild and all-events (un)subscribe.
|
||
|
||
### P4. HistoryRead/HistoryUpdate arms block SDK request threads (sync-over-async) with a 5 s admission wait
|
||
- **Severity:** Low · **Confidence:** High
|
||
- **Location:** `OtOpcUaNodeManager.cs:5997-6014` (`RunBounded` → `Task.WhenAll(...).GetAwaiter().GetResult()`), `:6065-6080` (`gate.Wait(≤5 s)`).
|
||
- Documented and bounded (16 batches × 4 nodes), and HistoryRead is not under `Lock`. Under a slow historian (deadline 60 s) up to 16 SDK request threads sit blocked for a minute while 16 more block 5 s each before fast-failing; with `MaxRequestThreadCount = 100` this is survivable but eats 1/3 of request capacity. Consider `MaxConcurrentHistoryReadBatches` default lower on small nodes, or an async `HistoryReadAsync` override if the SDK version exposes one (`IAsyncNodeManager` exists in this SDK line).
|
||
|
||
### P5. `WireAlarmNotifiers` / SDK notifier lists are linear per folder
|
||
- **Severity:** Low · **Confidence:** High
|
||
- **Location:** `:1723-1760`; SDK `NodeState.AddNotifier` (`NodeState.cs:2385-2410`, linear dedup).
|
||
- An equipment folder referencing K native alarms costs O(K²) `ReferenceEquals` on wiring plus per-call `ToHashSet`/`Where().ToList()`/`Any` allocations. Bounded by K (tens–hundreds); only matters on rebuild of alarm-heavy equipment.
|
||
|
||
### P6. Hard-coded SDK request quotas
|
||
- **Severity:** Low · **Confidence:** High
|
||
- **Location:** `OpcUaApplicationHost.cs:435-441` — `MinRequestThreadCount = 5, MaxRequestThreadCount = 100, MaxQueuedRequestCount = 200`, `TransportQuotas = new TransportQuotas()` (defaults).
|
||
- Not configurable from `appsettings`; a burst above 200 queued requests is refused by the SDK. Fine for current fleet sizes; flagging as the ceiling.
|
||
|
||
---
|
||
|
||
## 4. Other correctness / concurrency hazards
|
||
|
||
### H1. Removed-tag resurrection + re-add poisoning (see L1) — correctness, not just leak
|
||
Already covered; the correctness half (a re-added tag silently inherits the phantom's shape) is the more serious consequence.
|
||
|
||
### H2. `AddPredefinedNode` is AddOrUpdate: any id collision between a variable and a folder in the same realm silently replaces the folder's lookup entry
|
||
- **Severity:** Medium · **Confidence:** High
|
||
- **Location:** `OtOpcUaNodeManager.cs:2901`, `:3014`, `:1559`, `:6984`; SDK `CustomNodeManager.cs:586`.
|
||
- `_variables`, `_folders`, `_alarmConditions` are separate maps keyed by `(realm, id)`, so nothing prevents the same `(realm, id)` existing as both a folder and a variable (L4 shows a production producer). The SDK dictionary keeps whichever was added last. Suggest a cross-map guard in `EnsureFolder`/`EnsureVariable`/`MaterialiseAlarmCondition` (`ResolveNodeState(key) is not null ⇒ log + return`).
|
||
|
||
### H3. `ConditionRefresh` (ACL path) walks notifier subtrees outside `Lock`
|
||
- **Severity:** Low · **Confidence:** High
|
||
- **Location:** `OtOpcUaNodeManager.cs:4646-4651`
|
||
- Only the `RootNotifiers` copy is taken under `Lock`; `notifier.ConditionRefresh(...)` then walks children while a rebuild can be mutating child lists. **The SDK base does exactly the same** (`CustomNodeManager.cs:3488-3515`), and `NodeState` guards its child list with `m_childrenLock`, so this is parity with the SDK rather than a regression. Listed so it is not re-raised.
|
||
|
||
### H4. `RemainingShelveMs` and condition timestamps use `DateTime.UtcNow`, not `Clock`
|
||
- **Severity:** Low · **Confidence:** High
|
||
- **Location:** `:1087`, `:1452`, `:1514`, `:1926`, `:2621-2626`, `:4778`.
|
||
- Inconsistent with `HistoryUpdate`/`EventHorizon`, which use the injected `TimeProvider`; purely a testability/consistency nit.
|
||
|
||
### H5. `OnEquipmentTagWrite` relies on `gateway.WriteAsync` never throwing synchronously
|
||
- **Severity:** Low · **Confidence:** Medium
|
||
- **Location:** `:2202`
|
||
- A synchronous throw escapes the handler while the SDK holds `Lock` in `Write`; the SDK's per-node `try` (`CustomNodeManager.cs` Write, around line 1885) converts it to a Bad result, so the lock is released — no deadlock, but the optimistic/prior bookkeeping is skipped and no audit event is raised. Wrap the call in try/catch → treat as failed outcome.
|
||
|
||
---
|
||
|
||
## 5. Checked and fine (do not re-investigate)
|
||
|
||
- **`_liveItems` / `_eventItems` / `_allEventsItems` lifecycle.** `OnMonitoredItemDeleted` removes data items; `MasterNodeManager` routes every delete/transfer of event items through `SubscribeToEvents(…, unsubscribe:true)` / `SubscribeToAllEvents(…, true)` (`MasterNodeManager.cs:3329-3345`, `:3528-3545`), which the overrides handle. Not session-leaking.
|
||
- **`DataPlaneAuthority` event handler** is detached in the setter and in `Dispose` (`:448-454`, `:488-500`). Correct.
|
||
- **`SessionAclDecisionCache`** is doubly bounded (20k/shard, 100k global) with sealed-shard counters; sharded by key, cleared on generation advance. No unbounded growth.
|
||
- **`HistoryContinuationStore`** production path is the SDK per-session store (capped at `MaxHistoryContinuationPoints`, disposed with the session). `InMemoryHistoryContinuationStore` is test-only and capacity-bounded.
|
||
- **`CreateDeadline()` CTS** is `using`-disposed per arm; `RunBounded`'s `SemaphoreSlim` is `using`-disposed; the process-wide `_historyReadBatchLimiter` is never disposed but holds no OS handle.
|
||
- **`_rebuildCarryOver` / `_restoredConditionReports`** cleared at `CompleteAddressSpaceApply`, which the publish actor calls in a `finally` (`OpcUaPublishActor.cs:500`). Bounded.
|
||
- **`AddRootNotifier` cost**: SDK copies `Server.EventManager.GetMonitoredItems()` per newly-promoted folder and calls `SubscribeToEvents(…, true)` for all-events items; the protected override short-circuits those (`:4257-4261`). O(E) per folder on first promotion only.
|
||
- **`WriteValues` chunking** (512 per `Lock` take) is a deliberate, documented trade-off; `ClearChangeMasks` under `Lock` is the SDK pattern.
|
||
- **Alarm method handlers under `Lock`** (`ReportAfterMethod`, `IsNativeAlarmNode`) — `Lock` is a reentrant `Monitor`; the router delegates are fire-and-forget by contract.
|
||
- **Revalidation lock ordering**: `QueueValue` (item lock) is taken *outside* `Lock`, then `Lock` is taken for attach/detach — no nested inversion with the publish path (`Lock` → item lock).
|
||
- **`AdvertisedAggregateFunctions.Apply`** takes the Diagnostics node manager's `Lock` from the `HistorianDataSource` setter — called from host start, never while our `Lock` is held.
|
||
- **`OpcUaApplicationHost.DisposeAsync`** detaches `ImpersonateUser` and stops the application; the hosted service stops/disposes the server (`OtOpcUaServerHostedService.cs:357-365`).
|
||
- **Impersonation `GetAwaiter().GetResult()`** is irreducible (SDK sync callback under `SessionManager` lock) and the API-key authenticator does no I/O; documented.
|
||
- **`AddressSpacePlanner.DiffById`** is dictionary-based O(n); `AddressSpaceComposer` builds all lookups as dictionaries (no quadratic joins found). `BuildEquipmentFolderPaths` is computed twice per compose (once for refs, once for raw) — trivial.
|
||
- **`ReverseWalk`** has explicit termination arguments (segment always narrows; `granularity` floor), a `LookAhead`/`Floor` clamp, and the tie-cluster budget throws rather than looping.
|