lmxopcua/archreview/01-core-composition.md

Architecture Review — Core Composition Pipeline and Core Libraries

Date: 2026-07-08 Commit: 9cad9ed0 (master) Reviewer scope:

• src/Core/ZB.MOM.WW.OtOpcUa.Core (driver hosting, resilience, stability, authorization trie)
• src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions (driver capability interfaces, PollGroupEngine, historian seams)
• src/Core/ZB.MOM.WW.OtOpcUa.Commons (cluster message contracts, deferred sinks, NodeId scheme, telemetry)
• src/Core/ZB.MOM.WW.OtOpcUa.Configuration (EF config persistence, generation-sealed local cache, draft validation)
• src/Core/ZB.MOM.WW.OtOpcUa.Cluster (Akka bootstrap, role info, ServiceLevel)
• Plus the address-space composition pipeline itself — AddressSpaceComposer / AddressSpacePlanner / AddressSpaceApplier

Scope note (location drift). The review brief places the composition pipeline in ZB.MOM.WW.OtOpcUa.Core, but since the Phase7→AddressSpace rename it lives in src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/ (AddressSpaceComposer.cs, AddressSpacePlan.cs, AddressSpaceApplier.cs), with its tests under tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/. The Core project's own OpcUa/ folder now holds only the retired EquipmentNodeWalker and an unused GenericDriverNodeManager (see U-1). The pipeline was reviewed in full regardless.

---

Architecture Overview

Composition pipeline (deploy → address space)

The pipeline is a clean three-stage compose/diff/apply design:

1. AddressSpaceComposer.Compose (OpcUaServer/AddressSpaceComposer.cs:290) — a pure static projection from EF config entities (UnsArea, UnsLine, Equipment, DriverInstance, Tag, VirtualTag, Script, ScriptedAlarm, Device, Namespace) into an AddressSpaceComposition: sorted lists of UnsAreaProjection/UnsLineProjection/ EquipmentNode/DriverInstancePlan/EquipmentTagPlan/EquipmentVirtualTagPlan/ EquipmentScriptedAlarmPlan. Tag intent (driver FullName, native-alarm object, historize flags, array shape) is parsed out of the schemaless Tag.TagConfig JSON here. VirtualTag scripts get {{equip}} token substitution and dependency-ref extraction via the shared Commons.Types.EquipmentScriptPaths helper. Everything is ordinally sorted so the composition is deterministic — the foundation of the "byte-parity" contract with the artifact-decode mirror in Runtime/Drivers/DeploymentArtifact.cs.

2. AddressSpacePlanner.Compute (OpcUaServer/AddressSpacePlan.cs:96) — a pure diff of two compositions keyed on stable logical ids, using record value-equality as the changed check (with hand-written Equals/GetHashCode on the plans carrying IReadOnlyList members so a no-op redeploy diffs empty). Emits Added/Removed/Changed sets per entity class plus UNS folder renames. AddressSpacePlan.IsEmpty is the short-circuit gate OpcUaPublishActor uses.

3. AddressSpaceApplier.Apply (OpcUaServer/AddressSpaceApplier.cs:73) — the only side-effecting stage. Decides between a full structural rebuild (any topology change) and a surgical in-place update (ISurgicalAddressSpaceSink.UpdateTagAttributes / UpdateFolderDisplayName for whitelisted node-irrelevant deltas — the F10b optimization), falling back to rebuild when the sink lacks the capability or any surgical call fails. After the address-space work it dispatches two non-blocking hooks: fire-and-forget historian tag provisioning (IHistorianProvisioning.EnsureTagsAsync) and the historized-ref delta feed to the continuous-historization recorder (IHistorizedTagSubscriptionSink.UpdateHistorizedRefs). Separate Materialise* passes (Hierarchy / EquipmentTags / VirtualTags / ScriptedAlarms / DiscoveredNodes) re-derive nodes from the composition after a rebuild; NodeIds are folder-scoped (Commons.OpcUa.EquipmentNodeIds: {equipment}/{folder}/{name}), never the driver FullName, to avoid collisions across identical machines.

The sink boundary (Commons.OpcUa.IOpcUaAddressSpaceSink) keeps the whole pipeline SDK-free: production binds SdkAddressSpaceSink via the late-swap DeferredAddressSpaceSink (actors resolve the wrapper at DI time, the OPC UA hosted service swaps the real sink in after StandardServer starts); dev/tests bind the NullOpcUaAddressSpaceSink no-op.

Supporting Core libraries

• Core/Hosting — DriverFactoryRegistry (driver-type → factory), DriverFactoryRegistryAdapter (v1 registry → v2 IDriverFactory), DriverHost (id → IDriver lifecycle registry).
• Core/Resilience — Polly pipelines cached per (DriverInstanceId, HostName, Capability) (DriverResiliencePipelineBuilder), executed via CapabilityInvoker (with the non-idempotent-write no-retry override), fanned out per host by AlarmSurfaceInvoker, observed by DriverResilienceStatusTracker for Admin /hosts.
• Core/Stability — MemoryTracking (median-baseline soft/hard breach), MemoryRecycle and ScheduledRecycleScheduler (Tier C recycle via IDriverSupervisor), WedgeDetector (demand-aware stall detection).
• Core/Authorization — generation-sealed PermissionTrie per (ClusterId, GenerationId) cached in PermissionTrieCache (CAS-pruned), walked by TriePermissionEvaluator against per-session UserAuthorizationState (freshness window + fail-closed staleness ceiling).
• Commons — immutable record message contracts for the Akka DPS topics (deploy dispatch/ack, alerts AlarmTransitionEvent, redundancy state, admin ops, script log), strongly-typed ids (DeploymentId, RevisionHash, CorrelationId, NodeId), the deferred sink/publisher pair, EquipmentScriptPaths (shared script-path/dependency extraction), and OtOpcUaTelemetry (central Meter/ActivitySource).
• Configuration — OtOpcUaConfigDbContext (26 entities, logical-id unique indexes, JSON check constraints, RowVersion concurrency), the generation-sealed LiteDB fallback cache (GenerationSealedCache + atomic CURRENT pointer, fail-closed on corruption), ResilientConfigReader (timeout→retry→cache-fallback with secret-scrubbed logging), DraftValidator (all-errors-in-one-pass pre-publish rules).
• Cluster — AddOtOpcUaCluster / WithOtOpcUaClusterBootstrap (Akka.Hosting bootstrap with embedded HOCON + Serilog logger wiring), ClusterRoleInfo (lock-guarded role topology snapshot fed by a subscriber actor), ServiceLevelCalculator (pure 0–255 tiering), RoleParser.

Dependency direction: Core.Abstractions (leaf) ← Configuration ← Core; Commons (leaf, Akka+Audit packages) ← Cluster. The composer/applier in OpcUaServer reference both Configuration (entities) and Commons (sink contracts).

---

Findings

1. Stability

S-1 — Applier swallows every sink failure; a deploy is reported applied even when the address space is broken — High

AddressSpaceApplier wraps every sink call in catch-all "Safe" helpers that log and continue: SafeEnsureFolder/SafeEnsureVariable (AddressSpaceApplier.cs:612–622), SafeRebuild (AddressSpaceApplier.cs:373–383), SafeWriteAlarmCondition/SafeMaterialiseAlarmCondition (AddressSpaceApplier.cs:677–687). AddressSpaceApplyOutcome (AddressSpaceApplier.cs:691) carries only Added/Removed/Changed counts and RebuildCalled — no failure count. If RebuildAddressSpace() throws, rebuilt is still reported true (AddressSpaceApplier.cs:150–153) and the outcome flows back to the deploy coordinator as a success; the deployment seals while the running server holds a partially-materialised (or entirely stale) address space. Individual EnsureVariable failures during a materialise pass likewise vanish into per-node Warnings.

The never-fail-a-deploy posture is defensible for the detached hooks (provisioning, historized-ref feed — both correctly isolated), but structural materialisation failures are the deploy's core contract.

Recommendation: add FailedNodes/RebuildFailed to AddressSpaceApplyOutcome, propagate into the ApplyAck/DeploymentFailed decision and the audit log, and escalate SafeRebuild failure to at least a degraded ack. Tests should assert the failure surface (the fixture sink can throw).

S-2 — PollGroupEngine.Unsubscribe blocks the calling thread up to 5 s per subscription — Medium

StopState does a synchronous task.Wait(TimeSpan.FromSeconds(5)) (Core.Abstractions/PollGroupEngine.cs:99–112). Unsubscribe is the teardown path drivers call from OPC UA subscription-management callbacks and actor message handlers; a reader stuck in a slow network call turns every unsubscribe into a 5-second stall of the calling thread (and N stalls when tearing down N subscriptions serially). DisposeAsync gets this right (parallel cancel, awaited WhenAll with one shared timeout, PollGroupEngine.cs:213–236); the single-subscription path does not.

Recommendation: add an UnsubscribeAsync (or make Unsubscribe cancel + hand the await to a background drain), keeping the "no callback after teardown" guarantee via the existing LoopTask/CTS-dispose ordering.

S-3 — PollGroupEngine.Subscribe has no disposed guard — Low

Subscribe racing DisposeAsync can insert a fresh SubscriptionState after the dispose loop has snapshotted _subscriptions.Values (PollGroupEngine.cs:73–84 vs 213–236), leaking a live poll loop with no owner. Same pattern in DriverHost.RegisterAsync vs DisposeAsync (Core/Hosting/DriverHost.cs:53–71 vs 92–106): a registration that wins the race after the snapshot+clear is initialized but never shut down. Both are shutdown-window races, unlikely in practice but cheap to close with a _disposed flag checked inside the lock / before insert.

S-4 — ScheduledRecycleScheduler catch-up recycle storm after a stall — Low

TickAsync advances _nextRecycleUtc by exactly one interval per fire (Core/Stability/ScheduledRecycleScheduler.cs:72–84). If the host was suspended (VM pause, long outage) across K intervals, the next K ticks each trigger a full Tier C process recycle back-to-back. Recommendation: fast-forward _nextRecycleUtc past utcNow after a fire.

S-5 — GenericDriverNodeManager re-walk teardown is not synchronized — Low

BuildAddressSpaceAsync tears down the previous alarm forwarder and clears _alarmSinks non-atomically (Core/OpcUa/GenericDriverNodeManager.cs:58–71); a concurrent driver alarm event during the window between unsubscribe and re-subscribe is dropped, and two concurrent Build calls would interleave badly. Currently moot because the class has no production caller (see U-1) — fold this into whatever decision is made there.

Positive observations. The failure-mode engineering in Configuration/LocalCache is exemplary: GenerationSealedCache writes temp-file + atomic File.Replace pointer swaps, fails closed on any corruption rather than silently serving an older generation (GenerationSealedCache.cs:111–155); LiteDbConfigCache documents and defuses two real LiteDB concurrency traps (global BsonMapper races, find-then-insert upsert races) (LiteDbConfigCache.cs:16–36). ResilientConfigReader correctly distinguishes SQL-command TaskCanceledException from genuine caller cancellation (ResilientConfigReader.cs:117–138) and scrubs connection-string secrets from log messages. PermissionTrieCache.Prune is a correct reference-equality CAS loop (PermissionTrieCache.cs:73–102). The applier's two historian hooks are properly detached (continuation observes the task, never .Result on a fault — AddressSpaceApplier.cs:259–286). PollGroupEngine classifies fatal exceptions and hardens the onError callback (PollGroupEngine.cs:153–169).

2. Performance

P-1 — Compose parses each tag's TagConfig JSON four times — Medium

ExtractTagFullName, ExtractTagAlarm, ExtractTagHistorize and ExtractTagArray each do their own JsonDocument.Parse of the same blob per tag (AddressSpaceComposer.cs:418–433 calling :536–686). A compose over a 10k-tag fleet performs 40k JSON parses (plus the same again on the artifact-decode side). Compose runs on every deploy and every diff baseline, not on the data hot path, so this is a scalability tax rather than a latency bug today.

Recommendation: parse once per tag into a small TagConfigIntent record (which also collapses the byte-parity duplication — see C-1).

P-2 — Per-operation allocations on the authorization hot path — Medium

PermissionTrie.CollectMatches allocates a fresh HashSet<string> from the session's LDAP groups plus a List<MatchedGrant> on every call (Core/Authorization/PermissionTrie.cs:42–45), and TriePermissionEvaluator.Authorize runs per node-operation. A recursive browse or a large CreateMonitoredItems batch turns this into per-node garbage. UserAuthorizationState.LdapGroups is immutable per membership refresh — the case-insensitive set can be computed once per session version and reused.

P-3 — PollGroupEngine: one background task + independent timer per subscription, no cross-subscription batching — Medium

Each Subscribe spawns a dedicated Task.Run loop (PollGroupEngine.cs:73–84). Drivers that map each OPC UA monitored-item group (or each poll group) to a subscription get linear task/timer growth, and two subscriptions polling overlapping tag sets at the same interval issue duplicate protocol reads — the engine has no interval-bucketed scheduler or read coalescing. Fine at dozens of subscriptions; a scaling wall at hundreds per driver instance. Worth a note in the driver-facing docs at minimum; an interval-bucketed shared loop is the structural fix if tag counts grow.

P-4 — Per-write allocation in the non-idempotent write path — Low

CapabilityInvoker.ExecuteWriteAsync builds a new options record + Dictionary per non-idempotent write (Core/Resilience/CapabilityInvoker.cs:135–152) even though GetOrCreate keys only on (id, host, capability) and ignores the options after the first build — the comment even cites the "1% pipeline budget". Cache the no-retry snapshot per options generation, or key the fast path on a precomputed pipeline.

Positive observations. Pipeline resolution is lock-free ConcurrentDictionary reads (DriverResiliencePipelineBuilder.cs:58–71). The planner's diff is O(N) dictionary passes with deterministic sorted outputs. The surgical-apply path exists precisely to avoid the expensive full rebuild + subscription teardown for attribute-only edits, and its whitelist-via-with-expression technique (AddressSpaceApplier.cs:630–663) is future-field-safe (unknown new fields force the safe rebuild). Telemetry instruments are no-op until a listener attaches (OtOpcUaTelemetry.cs).

3. Conventions

C-1 — "Byte-parity by convention": TagConfig parsing replicated in four projects with comment-enforced sync — High

ExtractTagFullName (and friends) exist as deliberate copies in:

• OpcUaServer/AddressSpaceComposer.cs:536–551 (+ ExtractTagAlarm/ExtractTagHistorize/ExtractTagArray/TryExtractDeviceHost)
• Runtime/Drivers/DeploymentArtifact.cs (the artifact-decode mirror, per its own comments)
• Core/OpcUa/EquipmentNodeWalker.cs:193–208
• Configuration/Validation/DraftValidator.cs:60–71 (a fourth "small local copy, consistent with this codebase where the composer keeps its own")

Each copy carries a "MUST parse identically (byte-parity)" comment. The equality of the deployed address space across the live-compose and artifact-decode seams — and therefore the correctness of the diff/no-op-redeploy behavior — rests on humans keeping four JSON parsers in sync. The codebase already demonstrated the fix once: EquipmentScriptPaths (Commons/Types/EquipmentScriptPaths.cs) was created exactly to de-duplicate the ctx.GetTag extraction "those two seams used to duplicate".

Recommendation: move the TagConfig/DeviceConfig intent parsing into Commons (it has no EF or SDK dependency; every current copy's host project already references Commons or could) as a single TagConfigIntent.Parse(string) — also resolving P-1 — and keep one cross-seam parity test instead of N replicated implementations.

C-2 — Core depends on Configuration: EF entities and enums are the domain model — Medium

ZB.MOM.WW.OtOpcUa.Core.csproj references Configuration; PermissionTrie consumes Configuration.Enums.NodePermissions/NodeAclScopeKind (Core/Authorization/PermissionTrie.cs:1), EquipmentNodeWalker consumes Configuration.Entities directly, and the composer's plan records are projections of EF entities. There is no separate domain layer — persistence types are the model. This is a deliberate, consistent choice (and the plan records do decouple the applier), but it means: (a) any EF-driven entity change ripples straight into authorization and composition logic; (b) Configuration (with EF Core, SqlServer, LiteDB, DataProtection packages) is pulled into anything referencing Core. Flagging as accepted-risk to document rather than refactor — new code should keep preferring the plan-record boundary the composer established.

C-3 — DraftValidator re-derives the NodeId scheme and hard-codes a driver-type string — Medium

ValidateNoEquipmentSignalNameCollision re-implements the folder-scoped NodeId key by hand (Configuration/Validation/DraftValidator.cs:75–97) because Configuration cannot reference Commons.OpcUa.EquipmentNodeIds (dependency direction) — a third copy of the {equipment}/{folder}/{name} scheme whose drift would silently break the collision check. ValidateGalaxyTagFullName hard-codes dtype != "GalaxyMxGateway" (DraftValidator.cs:41–55) rather than a shared driver-type constant. Both are symptoms of the same missing shared-contract home as C-1; a Commons-level scheme/constants type (or moving EquipmentNodeIds down to a project Configuration can see) fixes both.

C-4 — Composer violates its own purity contract with Trace.TraceWarning — Low

The class doc says "Same inputs → same outputs, no logging" (AddressSpaceComposer.cs:280–283), but the dangling-predicate-script skip emits Trace.TraceWarning (AddressSpaceComposer.cs:493–496) — the only System.Diagnostics.Trace usage in a Serilog/ILogger codebase, invisible in the production sinks. Either return skipped-alarm ids in the composition for the caller to log, or accept an injected logger and fix the doc.

C-5 — Inconsistent duplicate-key defensiveness inside Compose — Low

deviceHostById is built with a deliberate last-wins foreach and a comment explaining why ToDictionary (throw-on-dupe) would diverge from the decode side (AddressSpaceComposer.cs:352–364), yet three lines later driversById/namespacesById/scriptsById use plain ToDictionary (AddressSpaceComposer.cs:401–402, 453) — a duplicate logical id (only possible on DB-constraint bypass) crashes the whole compose. Pick one posture; the defensive one is already argued for.

C-6 — Vestigial Akka package reference in Commons — Low

Commons.csproj references the Akka package but no Commons source uses an Akka type (only doc comments mention actors). Harmless today because every consumer is Akka-hosted anyway, but it contradicts Commons' role as the dependency-light contracts layer. Remove it.

C-7 — Stale scaffold-era XML docs on load-bearing classes — Low

AddressSpaceApplier's class doc still describes the F14 scaffold ("For now we record the work", "the SDK adapter that lands in F10b will decide…" — AddressSpaceApplier.cs:7–26) although both features shipped. DriverHost's doc promises "per-process isolation for Tier C … implemented in Phase 2 via named-pipe RPC" (Core/Hosting/DriverHost.cs:5–10), an architecture superseded by the out-of-repo mxaccessgw gateway. In a codebase whose XML documentation is otherwise a genuine asset (and evidently relied on), stale top-of-class narratives are actively misleading.

Positive observations. Naming and structure are highly consistent (logical-id + RowVersion entity pattern, Null*/Deferred* sink pattern, options records with Validate(), message records per DPS topic). Zero TODO/HACK/FIXME/Obsolete markers across all five projects. The DbContext matches a documented schema with an introspection compliance test (SchemaComplianceTests). The DeferredAddressSpaceSink explicitly forwards the surgical capability with a comment memorializing the F10b prod-inertness bug (DeferredAddressSpaceSink.cs:52–70) — institutional memory encoded where the next person will trip.

4. Underdeveloped Areas

U-1 — Dead/dormant code retained in Core: GenericDriverNodeManager, EquipmentNodeWalker, TryParseRelayBody — Medium

• GenericDriverNodeManager (Core/OpcUa/GenericDriverNodeManager.cs) has no production references — only its own test file. The production path is the composer→applier→sink chain.
• EquipmentNodeWalker (Core/OpcUa/EquipmentNodeWalker.cs:160–165) self-declares "retained for unit-test support only; real server deployments never invoke Walk" — ~280 lines plus IdentificationFolderBuilder and a full test suite exercising a code path that cannot run in production. Its ExtractFullName copy is also one of the four C-1 duplicates.
• EquipmentScriptPaths.TryParseRelayBody (Commons/Types/EquipmentScriptPaths.cs:164–172) serviced the relay→alias converter that was deleted when Galaxy became a standard driver (2026-06-12); only tests call it now.

Dormant code with green tests is worse than deleted code: it passes every gate while silently diverging from the live path. Recommendation: delete the walker + GDNM + relay parser (git preserves them), or, if the walker must survive as a test fixture, move it into the test project.

U-2 — The entire Tier C recycle machinery has no IDriverSupervisor implementation — Medium

MemoryRecycle, ScheduledRecycleScheduler, and DriverResilienceStatusTracker.RecordRecycle all drive IDriverSupervisor.RecycleAsync, but a repo-wide search finds zero concrete implementations of IDriverSupervisor (Core.Abstractions/IDriverSupervisor.cs + the two Stability consumers are the only references). Since the out-of-process driver story moved to the external mxaccessgw/HistorianGateway sidecars, no in-repo driver is Tier C, so the recycle path is compiled, tested, and unreachable. Either document it as speculative infrastructure for a future tier-migration workflow (the MemoryRecycle doc hints at one) or prune it alongside U-1.

U-3 — Continuous-historization interest set: delta feed landed, initial set still empty — Medium (known-limitation debt)

The applier now pushes add/remove historized-ref deltas per deploy (AddressSpaceApplier.cs:310–353 → IHistorizedTagSubscriptionSink), which is the convergent design for subsequent deploys — but per the documented KNOWN LIMITATION 2 (CLAUDE.md), the recorder is still spawned with an empty ref set, so after a process restart the recorder historizes nothing until the next deploy produces a delta. The seam contract (Core.Abstractions/Historian/IHistorizedTagSubscriptionSink.cs) supports the fix; the missing piece is a full-set replay at spawn/rebuild (e.g. have the applier feed the complete current set on RebuildCalled, not just the diff). This is the highest-leverage half-finished feature in the subsystem.

U-4 — Additive-only ACL model (no Deny) is a documented v2.0 gap — Low

PermissionTrie is explicitly pure-union ("no explicit Deny in v2.0", Core/Authorization/PermissionTrie.cs:13–17): a broad cluster-root grant cannot be carved back at a sub-scope. Acceptable for the current flat-role deployments (AdminUI roles are global by design), but it bounds how fine-grained OT authorization can get without a trie-walk semantics change — worth keeping visible in the security docs.

U-5 — Test coverage is strong but unevenly distributed — Low

Test method counts: Core.Tests 208, Configuration.Tests 94, Core.Abstractions.Tests 65, Commons.Tests 45, Cluster.Tests 21. The composition pipeline itself is thoroughly covered (10+ dedicated files in tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpace*). Gaps: ClusterRoleInfo — the only concurrency-bearing class in the Cluster project (lock + subscriber actor + event fan-out) — has no test (Cluster.Tests covers only the three pure classes); DriverHost dispose/register races (S-3) and the applier failure surface (S-1) are untested because the surface doesn't exist. An Akka TestKit harness for ClusterRoleInfo leader-change sequencing would close the riskiest untested seam.

---

Maturity Ratings

Dimension	Rating (1–5)	Justification
Stability	4	Deliberate, well-commented failure-mode engineering throughout (fail-closed caches, atomic pointer swaps, detached hooks, fatal-exception classification); docked for the applier's swallow-everything success reporting (S-1) and the blocking unsubscribe (S-2).
Performance	3	Deploy-path costs are deterministic and diffed to near-zero for no-op redeploys, and the surgical-apply path avoids rebuild churn; but hot paths carry avoidable per-operation allocations (P-2), redundant JSON parsing scales 4× with tag count (P-1), and the poll engine is task-per-subscription with no coalescing (P-3).
Conventions	4	Exceptionally consistent naming, patterns, and XML docs with zero TODO debt; docked for the systemic replicate-and-comment "byte-parity" duplication (C-1/C-3) and the Core→Configuration persistence coupling (C-2).
Underdeveloped areas	3	The live pipeline is feature-complete and well-tested, but Core retains three dead/dormant code paths with green tests (U-1), a fully-built-but-unreachable Tier C recycle subsystem (U-2), and the known continuous-historization initial-set gap (U-3).