lmxopcua/docs/reviews/2026-10-21/01-opcuaserver.md

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:

  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):

  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.