diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs index bd7e8ba0..ce85a7ff 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs @@ -93,15 +93,13 @@ public sealed class AddressSpaceApplier } var ts = DateTime.UtcNow; - var removedCount = 0; + // A removed equipment is a FOLDER: it gets no terminal condition write (review 01 L4 — the write the + // applier used to make against the equipment id never found a condition and minted a placeholder + // variable that shadowed the folder's SDK lookup entry). Its subtree teardown is in ApplyPureRemove. + var removedCount = plan.RemovedEquipment.Count; // Swallowed removal-pass condition-write failures are tallied here (not lost to a per-node Warning) // so a degraded removal is operator-visible via AddressSpaceApplyOutcome.FailedNodes (archreview 01/S-1). var failedNodes = 0; - foreach (var eq in plan.RemovedEquipment) - { - if (!SafeWriteAlarmCondition(eq.EquipmentId, RemovedConditionState, ts, AddressSpaceRealm.Uns)) failedNodes++; - removedCount++; - } foreach (var alarm in plan.RemovedAlarms) { if (!SafeWriteAlarmCondition(alarm.ScriptedAlarmId, RemovedConditionState, ts, AddressSpaceRealm.Uns)) failedNodes++; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index f8355b90..a49edfa9 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -18,18 +18,19 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer; /// /// Custom OPC UA that owns the writable address space for -/// the OtOpcUa server. Variable nodes are created lazily on first -/// under the manager's namespace; subsequent writes update the existing node's Value + -/// StatusCode + SourceTimestamp and notify subscribed clients via the standard -/// ClearChangeMasks path. +/// the OtOpcUa server. Variable nodes are declared by (the applier's +/// materialise passes); a updates an existing node's Value + StatusCode + +/// SourceTimestamp and notifies subscribed clients via the standard ClearChangeMasks path. +/// No write ever creates a node (review 01 L1): a publish naming an id a surgical remove has just +/// dropped is skipped, so it can neither resurrect the node as a flat phantom under the shared root nor +/// hand a later re-add of the same id the phantom's shape. /// /// This is the F10b production wiring behind the v2 /// seam — once a is bound, OpcUaPublishActor's writes /// materialise as real OPC UA Variable updates that clients can browse + subscribe to. /// /// Node-id encoding uses the manager's default namespace + the caller-supplied string id -/// as the identifier portion (e.g. "ns=2;s=eq-1/temp"). Beyond lazily-created flat -/// variables, the manager also materialises the full UNS Area/Line/Equipment folder hierarchy +/// as the identifier portion (e.g. "ns=2;s=eq-1/temp"). The manager materialises the full UNS Area/Line/Equipment folder hierarchy /// (), typed equipment-tag variables with the correct built-in /// DataType / array shape / access levels (), historized /// nodes (the HistoryRead access bit + HistoryRead overrides), and real Part 9 @@ -705,9 +706,11 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 } /// - /// Apply a value write from . Creates the - /// variable node on first call; subsequent calls update Value + StatusCode + - /// SourceTimestamp and call ClearChangeMasks so subscribed clients see the change. + /// Apply a value write from : update the materialised + /// variable's Value + StatusCode + SourceTimestamp and call ClearChangeMasks so subscribed clients + /// see the change. Never creates a node — an id nothing declared via + /// (a publish racing a surgical remove) is skipped and Debug-logged, exactly as + /// always did (review 01 L1). /// /// The node identifier of the variable. /// The new value to write. @@ -720,10 +723,17 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 ArgumentException.ThrowIfNullOrEmpty(nodeId); EnsureAddressSpaceCreated(); - var ns = NamespaceIndexForRealm(realm); var key = MapKey(realm, nodeId); + bool applied; lock (Lock) - WriteValueLocked(nodeId, ns, key, value, statusCode, sourceTimestampUtc); + applied = WriteValueLocked(key, value, statusCode, sourceTimestampUtc); + + if (!applied) + { + _logger.LogDebug( + "OtOpcUaNodeManager: WriteValue skipped {NodeId} (realm {Realm}) — node not materialised.", + nodeId, realm); + } } /// The most items applies under a SINGLE Lock acquisition before @@ -741,7 +751,9 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// Batched : one Lock acquisition per chunk of /// items instead of one per value — the publish path's per-value /// lock take was the steady-state throughput ceiling. Per-item semantics are identical to - /// (create-on-first-write verbatim status codes, ClearChangeMasks per node). + /// (never creates; verbatim status codes; ClearChangeMasks per node) — an item naming a + /// node that is not materialised is skipped and counted, not failed (the caller's per-driver ref index can + /// legitimately name a node a surgical remove has just dropped). /// Items are isolated. A bad item (empty NodeId, or a throw out of the SDK) does not abandon /// the rest of the batch: it is recorded and the loop continues, and the aggregate surfaces afterwards as /// a . Dropping the tail would be silent and durable — the poll @@ -785,6 +797,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 } var succeeded = 0; + var skipped = 0; for (var start = 0; start < values.Count; start += MaxWritesPerLockAcquisition) { var end = Math.Min(start + MaxWritesPerLockAcquisition, values.Count); @@ -796,9 +809,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 var v = values[i]; try { - WriteValueLocked(resolved[i].NodeId, resolved[i].NamespaceIndex, resolved[i].Key, - v.Value, v.StatusCode, v.SourceTimestampUtc); - succeeded++; + if (WriteValueLocked(resolved[i].Key, v.Value, v.StatusCode, v.SourceTimestampUtc)) succeeded++; + else skipped++; } catch (Exception ex) { @@ -810,6 +822,16 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 } } + if (skipped > 0) + { + // Debug, not Warning: the caller's index racing a surgical remove is expected, and a driver + // whose nodes were all just removed would otherwise log once per ref. + _logger.LogDebug( + "OtOpcUaNodeManager: WriteValues skipped {Skipped} of {Total} item(s) — node not materialised.", + skipped, + values.Count); + } + if (failed > 0) { throw new ValueWriteBatchException( @@ -925,24 +947,21 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// False when the item failed the pre-pass and must be skipped. private readonly record struct ResolvedWrite(string NodeId, ushort NamespaceIndex, RealmKey Key, bool IsValid); - /// The body of a single value write. Caller MUST hold Lock — this is factored out - /// solely so can apply a whole batch under one acquisition; it is byte-identical - /// to what has always done inside its own lock. - /// The bare s= identifier of the variable. - /// The namespace index resolved from the write's realm. - /// The realm-qualified map key for . + /// The body of a single value write. Caller MUST hold Lock — factored out so + /// can apply a whole batch under one acquisition. + /// Non-creating (review 01 L1). The former create-on-first-write branch minted a flat, read-only, + /// BaseDataType variable under the shared root whenever a publish raced a surgical remove; it stayed + /// until the next full rebuild, and because early-returns on an existing id, a + /// later re-add of the same tag silently kept the phantom's parent, DataType and writability. + /// The realm-qualified map key of the variable. /// The new value to write. /// The full OPC UA status code, sub-code included. /// The timestamp of the value in UTC. - private void WriteValueLocked(string nodeId, ushort ns, RealmKey key, object? value, uint statusCode, DateTime sourceTimestampUtc) + /// when the node existed and was updated; when no + /// such node is materialised (skipped — deliberately not a create). + private bool WriteValueLocked(RealmKey key, object? value, uint statusCode, DateTime sourceTimestampUtc) { - // CreateVariable mutates the SDK address space (_root.AddChild + AddPredefinedNode), - // so it MUST run under Lock — the SDK's subscription/ConditionRefresh threads take it too. - if (!_variables.TryGetValue(key, out var variable)) - { - variable = CreateVariable(nodeId, ns); - _variables[key] = variable; - } + if (!_variables.TryGetValue(key, out var variable)) return false; variable.Value = value; // The caller's status code reaches the SDK verbatim, sub-code included. This used to @@ -952,6 +971,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 variable.Timestamp = sourceTimestampUtc; // Stays INSIDE the lock, per node: this is how the SDK notifies monitored items of the change. variable.ClearChangeMasks(SystemContext, includeChildren: false); + return true; } /// @@ -959,10 +979,11 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// been materialised for (via ), /// this projects the whole snapshot /// (Enabled / Active / Acked / Confirmed / Shelving / Severity / Message) onto the live condition - /// node and recomputes Retain (T15 — richer state; still no event firing, that lands in T16). - /// Otherwise it falls back to the legacy two-element [Active, Acknowledged] - /// placeholder so callers whose alarm node hasn't been - /// materialised (and the existing unit tests) keep working. + /// node, recomputes Retain and fires one condition event when the state actually moved. + /// An id with no materialised condition is a logged no-op (review 01 L4) — it used to mint a legacy + /// bool[2] placeholder variable whose AddPredefinedNode (an AddOrUpdate) REPLACED the SDK + /// lookup entry of any folder sharing the id, which the applier's per-removed-equipment write did on + /// every removal. /// /// The node identifier of the alarm (== ScriptedAlarmId for materialised conditions). /// The full condition state to project onto the node. @@ -974,13 +995,21 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 ArgumentNullException.ThrowIfNull(state); EnsureAddressSpaceCreated(); - var ns = NamespaceIndexForRealm(realm); var key = MapKey(realm, alarmNodeId); // Look up + project under a SINGLE Lock so a concurrent RebuildAddressSpace can't clear // _alarmConditions / detach the condition node between the lookup and the Set* calls. lock (Lock) { - if (_alarmConditions.TryGetValue(key, out var condition)) + if (!_alarmConditions.TryGetValue(key, out var condition)) + { + // Unknown / unmaterialised condition ⇒ safe no-op (a mid-rebuild race, or a caller naming a + // node that is not a condition), mirroring WriteAlarmQuality. + _logger.LogDebug( + "OtOpcUaNodeManager: WriteAlarmCondition skipped {NodeId} (realm {Realm}) — condition not materialised.", + alarmNodeId, realm); + return; + } + { // T20 delta-gate: read the node's CURRENT live condition state FIRST (before projecting // the incoming snapshot onto it), then decide fire-vs-suppress by comparing the incoming @@ -1053,22 +1082,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 { ReportConditionEvent(condition, sourceTimestampUtc); } - return; } - - // Fallback: alarm not materialised as a real condition — keep the legacy bool[2] variable so - // un-materialised callers (and the existing unit tests) keep working. CreateVariable mutates - // the SDK address space, so it MUST run under Lock (see WriteValue). - if (!_variables.TryGetValue(key, out var variable)) - { - variable = CreateVariable(alarmNodeId, ns); - _variables[key] = variable; - } - - variable.Value = new[] { state.Active, state.Acknowledged }; - variable.StatusCode = StatusCodes.Good; - variable.Timestamp = sourceTimestampUtc; - variable.ClearChangeMasks(SystemContext, includeChildren: false); } } @@ -1364,6 +1378,9 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 // stays ONLY for the kind-swap (native↔scripted) case, which flips the native flag. if (_alarmConditions.ContainsKey(conditionKey) && _nativeAlarmNodeIds.Contains(conditionKey) == isNative) return; + // Review 01 H2 — a variable or folder already holding this id in this realm wins; never shadow it. + if (!_alarmConditions.ContainsKey(conditionKey) && IsHeldByAnotherKind(conditionKey, "alarm condition")) + return; // Idempotent: drop any prior node for this id so a re-materialise (e.g. changed // type/severity on redeploy) reflects cleanly instead of leaking the old node. @@ -2886,6 +2903,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 lock (Lock) { if (_folders.ContainsKey(key)) return; + if (IsHeldByAnotherKind(key, "folder")) return; var parent = ResolveParentFolder(parentNodeId, realm); var folder = new FolderState(parent) @@ -2977,6 +2995,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 lock (Lock) { if (_variables.ContainsKey(key)) return; + if (IsHeldByAnotherKind(key, "variable")) return; var parent = ResolveParentFolder(parentFolderNodeId, realm); // Phase C: a non-null historian tagname makes the node Historizing and grants the HistoryRead @@ -6961,28 +6980,23 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 } } - private BaseDataVariableState CreateVariable(string nodeId, ushort ns) + /// + /// Review 01 H2 — the cross-map guard every node-minting entry point (, + /// , ) runs before creating a node: + /// true (and a Warning) when is already held by a node of ANOTHER kind. + /// The three maps are separate, so nothing else stops one (realm, id) being both a folder and a + /// variable — and the SDK's AddPredefinedNode is an AddOrUpdate, so the second mint silently + /// replaced the first's lookup entry. MUST be called under Lock. + /// + /// The realm-qualified id about to be minted. + /// The kind the caller is minting (for the log line). + /// when the id is taken by another kind and the caller must not mint. + private bool IsHeldByAnotherKind(RealmKey key, string requestedKind) { - var v = new BaseDataVariableState(_root) - { - NodeId = new NodeId(nodeId, ns), - BrowseName = new QualifiedName(nodeId, ns), - DisplayName = nodeId, - TypeDefinitionId = VariableTypeIds.BaseDataVariableType, - ReferenceTypeId = ReferenceTypeIds.Organizes, - DataType = DataTypeIds.BaseDataType, - // Lazy-created nodes (a WriteValue for a node never declared via EnsureVariable) carry no - // array intent, so they stay scalar with no ArrayDimensions — mirrors EnsureVariable's - // scalar branch. Array nodes are always pre-declared via EnsureVariable(isArray:true). - ValueRank = ValueRanks.Scalar, - ArrayDimensions = null, - AccessLevel = AccessLevels.CurrentRead, - UserAccessLevel = AccessLevels.CurrentRead, - Historizing = false, - }; - _root?.AddChild(v); - AddPredefinedNode(SystemContext, v); - return v; + if (ResolveNodeState(key) is not { } existing) return false; + _logger.LogWarning( + "OtOpcUaNodeManager: refusing to materialise {Kind} {NodeId} (realm {Realm}) — the id is already held by a {ExistingKind} node", + requestedKind, key.Id, key.Realm, existing.GetType().Name); + return true; } - } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierFailureSurfaceTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierFailureSurfaceTests.cs index 08076fb1..a94b824b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierFailureSurfaceTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierFailureSurfaceTests.cs @@ -45,17 +45,32 @@ public sealed class AddressSpaceApplierFailureSurfaceTests // ---------------- T2: FailedNodes (removal-pass tally) ---------------- - /// A removal-pass alarm-condition write that throws is counted into FailedNodes (swallowed today). + /// A removal-pass alarm-condition write that throws is counted into FailedNodes (swallowed today). + /// The terminal condition write is made for a removed SCRIPTED ALARM — a removed equipment gets none (review + /// 01 L4: an equipment id is a folder, never a condition, so writing one only minted a phantom). [Fact] public void Apply_WhenRemovalConditionWriteThrows_CountsFailedNodes() { var sink = new ConfigurableThrowingSink { ThrowOnAlarmWrite = true }; var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + var outcome = applier.Apply(AlarmRemovalPlan("alm-1")); + + outcome.RemovedNodes.ShouldBe(1); + outcome.FailedNodes.ShouldBe(1); + } + + /// Review 01 L4 — removing an equipment writes NO condition state against the equipment id. + [Fact] + public void Apply_RemovedEquipment_WritesNoConditionAgainstTheEquipmentId() + { + var sink = new ConfigurableThrowingSink { ThrowOnAlarmWrite = true }; + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + var outcome = applier.Apply(EquipmentRemovalPlan("eq-1")); outcome.RemovedNodes.ShouldBe(1); - outcome.FailedNodes.ShouldBe(1); + outcome.FailedNodes.ShouldBe(0, "no condition write happens for an equipment folder, so a throwing alarm write cannot be reached"); } // ---------------- T3: Materialise* passes return failed-node counts ---------------- @@ -135,6 +150,17 @@ public sealed class AddressSpaceApplierFailureSurfaceTests RemovedAlarms: Array.Empty(), ChangedAlarms: Array.Empty()); + private static AddressSpacePlan AlarmRemovalPlan(params string[] ids) => new( + AddedEquipment: Array.Empty(), + RemovedEquipment: Array.Empty(), + ChangedEquipment: Array.Empty(), + AddedDrivers: Array.Empty(), + RemovedDrivers: Array.Empty(), + ChangedDrivers: Array.Empty(), + AddedAlarms: Array.Empty(), + RemovedAlarms: ids.Select(id => new ScriptedAlarmPlan(id, "eq-1", "pred", "msg")).ToArray(), + ChangedAlarms: Array.Empty()); + private static AddressSpacePlan EquipmentRemovalPlan(params string[] ids) => new( AddedEquipment: Array.Empty(), RemovedEquipment: ids.Select(id => new EquipmentNode(id, id, "line-1")).ToArray(), diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerBatchWriteTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerBatchWriteTests.cs index bb0c8725..92233d32 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerBatchWriteTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerBatchWriteTests.cs @@ -12,8 +12,9 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; /// per-value take was the steady-state throughput ceiling on the publish path; the chunk cap keeps the /// first poll cycle's force-raise-everything batch from stalling every SDK service thread). /// These tests pin the observable contract rather than the locking: every item lands with -/// its own Value / StatusCode / Timestamp, an item for an undeclared NodeId is lazily created exactly -/// as does, and an empty batch does nothing at all. +/// its own Value / StatusCode / Timestamp, the realm travels with every item (the same s= id in the +/// two namespaces is two nodes), and an empty batch does nothing at all. An item for an undeclared NodeId +/// is skipped, never created (review 01 L1 — ). /// A non-Good status is included so the #519 verbatim-sub-code passthrough is proven on the batched /// path too — a batch that re-projected status through a 3-state enum would collapse it to a bare /// Bad. @@ -90,16 +91,16 @@ public sealed class NodeManagerBatchWriteTests : IDisposable /// assertion is therefore two nodes with DISTINCT namespace indices, not merely "both reads returned /// something" (which a realm-blind map satisfies, since the shared key resolves for either lookup). [Fact] - public async Task WriteValues_creates_missing_variables() + public async Task WriteValues_is_realm_qualified() { var (host, server) = await BootAsync(); try { var nm = server.NodeManager!; - const string SharedId = "Shared/lazy/id"; - nm.TryGetVariable(SharedId, AddressSpaceRealm.Uns).ShouldBeNull(); - nm.TryGetVariable(SharedId, AddressSpaceRealm.Raw).ShouldBeNull(); + const string SharedId = "Shared/both/id"; + nm.EnsureVariable(SharedId, null, "shared", "Int32", writable: false, realm: AddressSpaceRealm.Uns); + nm.EnsureVariable(SharedId, null, "shared", "Int32", writable: false, realm: AddressSpaceRealm.Raw); var ts = new DateTime(2026, 8, 15, 11, 0, 0, DateTimeKind.Utc); nm.WriteValues(new List @@ -143,6 +144,8 @@ public sealed class NodeManagerBatchWriteTests : IDisposable { var nm = server.NodeManager!; var ts = new DateTime(2026, 8, 15, 12, 0, 0, DateTimeKind.Utc); + nm.EnsureVariable("eq-iso/first", null, "first", "Int32", writable: false, realm: AddressSpaceRealm.Uns); + nm.EnsureVariable("eq-iso/third", null, "third", "Int32", writable: false, realm: AddressSpaceRealm.Uns); var ex = Should.Throw(() => nm.WriteValues(new List { @@ -187,6 +190,7 @@ public sealed class NodeManagerBatchWriteTests : IDisposable var items = new List(Count); for (var i = 0; i < Count; i++) { + nm.EnsureVariable($"eq-chunk/{i}", null, $"{i}", "Int32", writable: false, realm: AddressSpaceRealm.Uns); items.Add(new ValueWrite($"eq-chunk/{i}", i, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns)); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerPhantomNodeTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerPhantomNodeTests.cs new file mode 100644 index 00000000..821f0dab --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerPhantomNodeTests.cs @@ -0,0 +1,231 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Opc.Ua; +using Opc.Ua.Server; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; + +/// +/// Review 01 L1 / L4 / H2 — the node manager never materialises a node from a VALUE or ALARM write, and +/// never lets one (realm, id) be held by two different node kinds. +/// +/// The lazy create-on-first-write path predates and had +/// no production producer left — but it had a production consumer by accident: a driver publish +/// (or the UNS fan-out) racing a surgical remove re-created the removed id as a flat, read-only, +/// BaseDataType variable under the shared root, and because EnsureVariable early-returns +/// on an existing id, a later re-add of the same tag silently kept the phantom's shape. +/// +/// +public sealed class NodeManagerPhantomNodeTests : IDisposable +{ + private static CancellationToken Ct => TestContext.Current.CancellationToken; + + private readonly string _pkiRoot = Path.Combine(Path.GetTempPath(), $"otopcua-phantom-{Guid.NewGuid():N}"); + + /// A value write to an id nothing declared creates nothing. + [Fact] + public async Task WriteValue_for_an_undeclared_id_creates_no_node() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + var before = nm.VariableCount; + + nm.WriteValue("Plant/Modbus/dev1/Ghost", 1, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Raw); + + nm.VariableCount.ShouldBe(before); + nm.TryGetVariable("Plant/Modbus/dev1/Ghost", AddressSpaceRealm.Raw).ShouldBeNull(); + nm.GetManagerHandle(new NodeId("Plant/Modbus/dev1/Ghost", nm.NamespaceIndexForRealm(AddressSpaceRealm.Raw))) + .ShouldBeNull("the SDK must not resolve a node that was never declared"); + } + finally { await host.DisposeAsync(); } + } + + /// + /// A batch naming a removed id skips it (no node, no exception) while the declared ids in the same batch + /// land — the positive control that proves the skip is a skip and not a dropped batch. + /// + [Fact] + public async Task WriteValues_skips_undeclared_ids_and_still_applies_the_declared_ones() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.EnsureVariable("Plant/Modbus/dev1/Speed", null, "Speed", "Int32", writable: false, realm: AddressSpaceRealm.Raw); + var ts = DateTime.UtcNow; + + nm.WriteValues( + [ + new ValueWrite("Plant/Modbus/dev1/Ghost", 1, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw), + new ValueWrite("Plant/Modbus/dev1/Speed", 42, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw), + ]); + + nm.TryGetVariable("Plant/Modbus/dev1/Ghost", AddressSpaceRealm.Raw).ShouldBeNull(); + nm.TryGetVariable("Plant/Modbus/dev1/Speed", AddressSpaceRealm.Raw)!.Value.ShouldBe(42); + } + finally { await host.DisposeAsync(); } + } + + /// + /// The L1 scenario end to end: a publish racing a surgical remove leaves NO node, and the re-add that + /// follows gets the AUTHORED DataType, parent and writability rather than inheriting a phantom's. + /// + [Fact] + public async Task A_publish_racing_a_surgical_remove_leaves_no_node_and_the_readd_gets_the_authored_shape() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + const string id = "Plant/Modbus/dev1/Speed"; + nm.EnsureFolder("Plant/Modbus/dev1", null, "dev1", AddressSpaceRealm.Raw); + nm.EnsureVariable(id, "Plant/Modbus/dev1", "Speed", "Float", writable: false, realm: AddressSpaceRealm.Raw); + nm.RemoveVariableNode(id, AddressSpaceRealm.Raw).ShouldBeTrue(); + + // The race: the driver's fan-out publishes the id the surgical remove just dropped. + nm.WriteValue(id, 1.5f, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Raw); + nm.TryGetVariable(id, AddressSpaceRealm.Raw).ShouldBeNull("the racing publish must not resurrect the node"); + + // The re-add (a later deploy re-authors the tag with a different shape). + nm.EnsureVariable(id, "Plant/Modbus/dev1", "Speed", "Int32", writable: true, realm: AddressSpaceRealm.Raw); + + var v = nm.TryGetVariable(id, AddressSpaceRealm.Raw); + v.ShouldNotBeNull(); + v!.DataType.ShouldBe(DataTypeIds.Int32); + v.Parent.ShouldBeSameAs(nm.TryGetFolder("Plant/Modbus/dev1", AddressSpaceRealm.Raw)); + ((int)v.AccessLevel & AccessLevels.CurrentWrite).ShouldNotBe(0, "the re-added node is writable as authored"); + v.OnWriteValue.ShouldNotBeNull("a writable node carries the inbound-write handler"); + + // Positive control: the re-added node is live — a publish lands on it. + nm.WriteValue(id, 7, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Raw); + v.Value.ShouldBe(7); + } + finally { await host.DisposeAsync(); } + } + + /// A condition write for an id that was never materialised is a logged no-op (L4): no placeholder variable. + [Fact] + public async Task WriteAlarmCondition_for_an_unknown_id_creates_nothing() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.EnsureFolder("eq-1", null, "Equipment 1", AddressSpaceRealm.Uns); + var before = nm.VariableCount; + + var snapshot = new AlarmConditionSnapshot( + Active: false, Acknowledged: true, Confirmed: true, Enabled: true, + Shelving: AlarmShelvingKind.Unshelved, Severity: 0, Message: string.Empty); + // The applier used to write this against the EQUIPMENT id on every removal — a folder, never a condition. + nm.WriteAlarmCondition("eq-1", snapshot, DateTime.UtcNow, AddressSpaceRealm.Uns); + + nm.VariableCount.ShouldBe(before); + nm.TryGetVariable("eq-1").ShouldBeNull(); + // The folder's SDK lookup entry survives (the phantom used to REPLACE it — AddPredefinedNode is AddOrUpdate). + var handle = (NodeHandle?)nm.GetManagerHandle(new NodeId("eq-1", nm.NamespaceIndexForRealm(AddressSpaceRealm.Uns))); + handle.ShouldNotBeNull(); + handle!.Node.ShouldBeOfType(); + } + finally { await host.DisposeAsync(); } + } + + /// H2 — a variable may not take an id a folder already holds in the same realm. + [Fact] + public async Task EnsureVariable_refuses_an_id_held_by_a_folder() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.EnsureFolder("eq-1", null, "Equipment 1", AddressSpaceRealm.Uns); + + nm.EnsureVariable("eq-1", null, "eq-1", "Int32", writable: false, realm: AddressSpaceRealm.Uns); + + nm.TryGetVariable("eq-1").ShouldBeNull(); + nm.TryGetFolder("eq-1").ShouldNotBeNull(); + var handle = (NodeHandle?)nm.GetManagerHandle(new NodeId("eq-1", nm.NamespaceIndexForRealm(AddressSpaceRealm.Uns))); + handle!.Node.ShouldBeSameAs(nm.TryGetFolder("eq-1")); + } + finally { await host.DisposeAsync(); } + } + + /// H2 — a folder may not take an id a variable already holds in the same realm. + [Fact] + public async Task EnsureFolder_refuses_an_id_held_by_a_variable() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.EnsureVariable("Plant/X", null, "X", "Int32", writable: false, realm: AddressSpaceRealm.Raw); + + nm.EnsureFolder("Plant/X", null, "X", AddressSpaceRealm.Raw); + + nm.TryGetFolder("Plant/X", AddressSpaceRealm.Raw).ShouldBeNull(); + var handle = (NodeHandle?)nm.GetManagerHandle(new NodeId("Plant/X", nm.NamespaceIndexForRealm(AddressSpaceRealm.Raw))); + handle!.Node.ShouldBeSameAs(nm.TryGetVariable("Plant/X", AddressSpaceRealm.Raw)); + } + finally { await host.DisposeAsync(); } + } + + /// H2 — a condition may not take an id a variable already holds in the same realm. + [Fact] + public async Task MaterialiseAlarmCondition_refuses_an_id_held_by_a_variable() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.EnsureFolder("Plant/Modbus/dev1", null, "dev1", AddressSpaceRealm.Raw); + nm.EnsureVariable("Plant/Modbus/dev1/temp", "Plant/Modbus/dev1", "temp", "Float", writable: false, realm: AddressSpaceRealm.Raw); + + nm.MaterialiseAlarmCondition("Plant/Modbus/dev1/temp", "Plant/Modbus/dev1", "temp", "OffNormalAlarm", 500, AddressSpaceRealm.Raw, isNative: true); + + nm.TryGetAlarmCondition("Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw).ShouldBeNull(); + nm.TryGetVariable("Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw).ShouldNotBeNull(); + } + finally { await host.DisposeAsync(); } + } + + /// The same bare id in the OTHER realm is a different node and is still allowed (the guard is realm-qualified). + [Fact] + public async Task The_guard_is_realm_qualified() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.EnsureFolder("shared", null, "shared", AddressSpaceRealm.Uns); + + nm.EnsureVariable("shared", null, "shared", "Int32", writable: false, realm: AddressSpaceRealm.Raw); + + nm.TryGetVariable("shared", AddressSpaceRealm.Raw).ShouldNotBeNull(); + nm.TryGetFolder("shared", AddressSpaceRealm.Uns).ShouldNotBeNull(); + } + finally { await host.DisposeAsync(); } + } + + private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync() => + await OpcUaHostStart.StartAsync( + port => new OpcUaApplicationHost( + new OpcUaApplicationHostOptions + { + ApplicationName = "OtOpcUa.PhantomTest", + ApplicationUri = $"urn:OtOpcUa.PhantomTest:{Guid.NewGuid():N}", + OpcUaPort = port, + PublicHostname = "localhost", + PkiStoreRoot = _pkiRoot, + }, + NullLogger.Instance), + () => new OtOpcUaSdkServer(), Ct); + + public void Dispose() + { + if (Directory.Exists(_pkiRoot)) Directory.Delete(_pkiRoot, recursive: true); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/OtOpcUaNodeManagerWriteStatusesTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/OtOpcUaNodeManagerWriteStatusesTests.cs index 6de7a0c2..8cf7211e 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/OtOpcUaNodeManagerWriteStatusesTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/OtOpcUaNodeManagerWriteStatusesTests.cs @@ -19,8 +19,8 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; /// /// Never creates. A status carries nothing to create a node from, and the caller's /// per-driver index can legitimately name a node a surgical remove has just dropped. A missing id -/// is skipped, not materialised — the opposite of , -/// which lazily creates. +/// is skipped, not materialised — the same contract +/// carries since review 01 L1 (it used to lazily create). /// /// Boots a real , like /// , so the assertions read the SDK node state a client @@ -81,6 +81,8 @@ public sealed class OtOpcUaNodeManagerWriteStatusesTests : IDisposable var ts = new DateTime(2026, 8, 20, 10, 0, 0, DateTimeKind.Utc); const string sharedId = "Shared/both/realms"; + nm.EnsureVariable(sharedId, null, "shared", "Int32", writable: false, realm: AddressSpaceRealm.Raw); + nm.EnsureVariable(sharedId, null, "shared", "Int32", writable: false, realm: AddressSpaceRealm.Uns); nm.WriteValue(sharedId, 1, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw); nm.WriteValue(sharedId, 2, OpcUaStatus.Good, ts, AddressSpaceRealm.Uns); @@ -110,6 +112,7 @@ public sealed class OtOpcUaNodeManagerWriteStatusesTests : IDisposable { var nm = server.NodeManager!; var ts = new DateTime(2026, 8, 20, 10, 0, 0, DateTimeKind.Utc); + nm.EnsureVariable("Plant/known", null, "known", "Int32", writable: false, realm: AddressSpaceRealm.Raw); nm.WriteValue("Plant/known", 7, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw); var before = nm.VariableCount; @@ -143,6 +146,8 @@ public sealed class OtOpcUaNodeManagerWriteStatusesTests : IDisposable { var nm = server.NodeManager!; var ts = new DateTime(2026, 8, 20, 10, 0, 0, DateTimeKind.Utc); + nm.EnsureVariable("Plant/first", null, "first", "Int32", writable: false, realm: AddressSpaceRealm.Raw); + nm.EnsureVariable("Plant/third", null, "third", "Int32", writable: false, realm: AddressSpaceRealm.Raw); nm.WriteValue("Plant/first", 1, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw); nm.WriteValue("Plant/third", 3, OpcUaStatus.Good, ts, AddressSpaceRealm.Raw); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SdkAddressSpaceSinkTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SdkAddressSpaceSinkTests.cs index 932e1f95..0611c42f 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SdkAddressSpaceSinkTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SdkAddressSpaceSinkTests.cs @@ -20,18 +20,25 @@ public sealed class SdkAddressSpaceSinkTests : IDisposable Path.GetTempPath(), $"otopcua-sink-{Guid.NewGuid():N}"); - /// Verifies that WriteValue creates and updates variables in the OPC UA node manager. + /// Verifies that WriteValue updates declared variables in the OPC UA node manager (and never + /// creates one — review 01 L1, pinned by ). [Fact] - public async Task WriteValue_creates_and_updates_variable_in_node_manager() + public async Task WriteValue_updates_declared_variables_in_node_manager() { var (host, server) = await BootAsync(); - var sink = new SdkAddressSpaceSink(server.NodeManager!); + var nm = server.NodeManager!; + var sink = new SdkAddressSpaceSink(nm); + sink.EnsureVariable("eq-1/temp", null, "temp", "Double", writable: false, realm: AddressSpaceRealm.Uns); + sink.EnsureVariable("eq-1/pressure", null, "pressure", "Int32", writable: false, realm: AddressSpaceRealm.Uns); sink.WriteValue("eq-1/temp", 22.5, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Uns); sink.WriteValue("eq-1/temp", 23.1, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Uns); sink.WriteValue("eq-1/pressure", 100, OpcUaStatus.Uncertain, DateTime.UtcNow, AddressSpaceRealm.Uns); + sink.WriteValue("eq-1/undeclared", 1, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Uns); - server.NodeManager!.VariableCount.ShouldBe(2); + nm.VariableCount.ShouldBe(2); + nm.TryGetVariable("eq-1/temp")!.Value.ShouldBe(23.1); + nm.TryGetVariable("eq-1/pressure")!.StatusCode.ShouldBe((StatusCode)StatusCodes.Uncertain); await host.DisposeAsync(); } @@ -50,6 +57,8 @@ public sealed class SdkAddressSpaceSinkTests : IDisposable // (a concrete-typed call would simply stop compiling, which is not the regression being pinned). IOpcUaAddressSpaceSink sink = new SdkAddressSpaceSink(nm); var ts = new DateTime(2026, 8, 15, 9, 0, 0, DateTimeKind.Utc); + sink.EnsureVariable("Plant/dev1/Speed", null, "Speed", "Int32", writable: false, realm: AddressSpaceRealm.Raw); + sink.EnsureVariable("eq-1/Speed", null, "Speed", "Int32", writable: false, realm: AddressSpaceRealm.Uns); sink.WriteValues(new List { @@ -91,6 +100,8 @@ public sealed class SdkAddressSpaceSinkTests : IDisposable var nm = server.NodeManager!; IOpcUaAddressSpaceSink sink = new SdkAddressSpaceSink(nm); var ts = new DateTime(2026, 8, 15, 9, 30, 0, DateTimeKind.Utc); + sink.EnsureVariable("eq-fwd/first", null, "first", "Int32", writable: false, realm: AddressSpaceRealm.Uns); + sink.EnsureVariable("eq-fwd/third", null, "third", "Int32", writable: false, realm: AddressSpaceRealm.Uns); var ex = Should.Throw(() => sink.WriteValues(new List { @@ -113,18 +124,22 @@ public sealed class SdkAddressSpaceSinkTests : IDisposable } } - /// Verifies that WriteAlarmCondition (un-materialised fallback) creates a dedicated node - /// distinct from value writes. + /// A WriteAlarmCondition for an id no condition was materialised under is a no-op (review 01 L4): + /// it creates no placeholder node, and the value write beside it is unaffected. [Fact] - public async Task WriteAlarmCondition_creates_dedicated_node_distinct_from_value_writes() + public async Task WriteAlarmCondition_for_an_unmaterialised_id_creates_no_node() { var (host, server) = await BootAsync(); - var sink = new SdkAddressSpaceSink(server.NodeManager!); + var nm = server.NodeManager!; + var sink = new SdkAddressSpaceSink(nm); + sink.EnsureVariable("eq-1/temp", null, "temp", "Double", writable: false, realm: AddressSpaceRealm.Uns); sink.WriteAlarmCondition("alarm-7", Snapshot(active: true, acknowledged: false), DateTime.UtcNow, AddressSpaceRealm.Uns); sink.WriteValue("eq-1/temp", 22.5, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Uns); - server.NodeManager!.VariableCount.ShouldBe(2); + nm.VariableCount.ShouldBe(1); + nm.TryGetVariable("alarm-7").ShouldBeNull(); + nm.TryGetAlarmCondition("alarm-7").ShouldBeNull(); await host.DisposeAsync(); } @@ -242,17 +257,21 @@ public sealed class SdkAddressSpaceSinkTests : IDisposable var (host, server) = await BootAsync(); var sink = new SdkAddressSpaceSink(server.NodeManager!); + sink.EnsureVariable("a", null, "a", "Int32", writable: false, realm: AddressSpaceRealm.Uns); + sink.EnsureVariable("b", null, "b", "Int32", writable: false, realm: AddressSpaceRealm.Uns); + sink.EnsureVariable("c", null, "c", "Int32", writable: false, realm: AddressSpaceRealm.Uns); sink.WriteValue("a", 1, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Uns); sink.WriteValue("b", 2, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Uns); - sink.WriteAlarmCondition("alarm-c", Snapshot(active: true), DateTime.UtcNow, AddressSpaceRealm.Uns); server.NodeManager!.VariableCount.ShouldBe(3); sink.RebuildAddressSpace(); server.NodeManager.VariableCount.ShouldBe(0); - // After rebuild, subsequent writes start fresh. + // After rebuild, subsequent materialisation starts fresh. + sink.EnsureVariable("a", null, "a", "Int32", writable: false, realm: AddressSpaceRealm.Uns); sink.WriteValue("a", 99, OpcUaStatus.Good, DateTime.UtcNow, AddressSpaceRealm.Uns); server.NodeManager.VariableCount.ShouldBe(1); + server.NodeManager.TryGetVariable("a")!.Value.ShouldBe(99); await host.DisposeAsync(); }