From 0bd3ec00e476c6755b9557ff0c6d6c818426415c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 07:12:29 -0400 Subject: [PATCH 1/2] fix(address-space): announce surgically-added raw nodes; equipment-with-references removal stays surgical (#599) - AnnounceAddedNodes raises NodeAdded in the Raw realm for the existing parents of added raw containers/tags (ComputeRawAddAnnouncements); Uns side unchanged. - ApplyPureRemove removes UNS reference variables BEFORE the equipment subtree, so a reference the subtree's id-prefix scope reaches no longer ratchets to a full rebuild (03/P5). A NotSubsumed filter was not used: in production a reference NodeId is Area/Line/Equipment/Name, outside the subtree scope, so filtering would leak it. The matching RemoveEquipmentSubtree severing (03/S20) is in the node-manager commit. - UnsReferenceBackingParityTests: reflection guard discovering the classifier's surgical raw-tag whitelist and requiring a same-typed mirror field (03/U11). Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/Raw.md | 15 +- .../AddressSpaceApplier.cs | 103 +++++++++--- .../OpcUa/OpcUaPublishActor.cs | 14 +- .../AddressSpaceApplierTests.cs | 64 ++++++- .../NodeManagerSurgicalRemoveTests.cs | 98 +++++++++++ .../UnsReferenceBackingParityTests.cs | 158 ++++++++++++++++++ 6 files changed, 414 insertions(+), 38 deletions(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/UnsReferenceBackingParityTests.cs diff --git a/docs/Raw.md b/docs/Raw.md index 022451bc..7a2b8e6b 100644 --- a/docs/Raw.md +++ b/docs/Raw.md @@ -99,9 +99,18 @@ mirror **in place** (its MonitoredItems survive); a re-point or effective-name c removes and recreates **that one mirror node**. See [`Uns.md`](Uns.md) and [`plans/2026-08-15-surgical-deploys.md`](plans/2026-08-15-surgical-deploys.md). -⚠️ A surgically added raw container/tag is materialised but **not** announced — -`AnnounceAddedNodes` is UNS-only, so model-aware clients see new raw nodes on their next -browse. This mattered less when every add forced a rebuild (and therefore a re-browse). +A surgically added raw container/tag is **announced** (Gitea #599): `AnnounceAddedNodes` +raises a `GeneralModelChangeEvent(NodeAdded)` in the **Raw** realm for each distinct +existing parent of the added nodes (a parent that is itself new in the same deploy is +skipped — its own parent is announced, and a re-browse there finds the whole new subtree; +a new top-level folder announces itself). UNS-side adds announce in the Uns realm as +before. Until #599 raw adds were materialised but never announced, so model-aware clients +only saw them on their next browse — invisible while every add still forced a rebuild. + +Removing an equipment that holds UNS references is also surgical: the references are +removed (terminal Bad, then `RemoveVariableNode`, which severs the backing raw node's +inverse `Organizes` edge) **before** the equipment subtree, and the subtree removal severs +the same edge for anything it reaches itself (archreview 03/P5 + 03/S20). ## The `Calculation` driver diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs index eee64688..89e48cb4 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs @@ -397,29 +397,40 @@ public sealed class AddressSpaceApplier if (!SafeRemoveAlarmCondition(surgical, alarm.ScriptedAlarmId, AddressSpaceRealm.Uns)) return false; } - // Removed equipment — one subtree teardown each (subsumes their child tags/vtags/alarms). The - // top-of-Apply block already wrote the equipment id's terminal condition state. - foreach (var eq in plan.RemovedEquipment) - { - if (!SafeRemoveEquipmentSubtree(surgical, eq.EquipmentId, AddressSpaceRealm.Uns)) return false; - } - // v3 Batch 4 — removed UNS reference variables (Uns realm, plain value nodes): terminal Bad then // in-place remove. // // WHAT REACHES THIS LOOP (the "→ rebuild" this comment used to claim is long dead — re-points are // surgical now): only genuine DEREFERENCES, i.e. the equipment dropped the reference row, so the node - // is gone for good. A CHANGED reference never lands in RemovedUnsReferenceVariables at all — the - // planner reports it as a UnsReferenceDelta, which Apply routes above either to an in-place - // UpdateTagAttributes (identity unchanged) or to its own remove-the-previous + recreate-the-current - // pair (identity changed). That second shape does remove a node, but it removes it from Apply's delta - // loop, not from here. + // is gone for good — INCLUDING every reference of a removed equipment, which the planner lists here + // too. A CHANGED reference never lands in RemovedUnsReferenceVariables at all — the planner reports it + // as a UnsReferenceDelta, which Apply routes above either to an in-place UpdateTagAttributes (identity + // unchanged) or to its own remove-the-previous + recreate-the-current pair (identity changed). That + // second shape does remove a node, but it removes it from Apply's delta loop, not from here. + // + // Gitea #599 (archreview 03/P5): this runs BEFORE the equipment-subtree removal below, deliberately, + // instead of skipping a removed equipment's references with the NotSubsumed filter the loops above + // use. Whether the subtree removal reaches a reference depends on its NodeId, not on its parent: the + // node manager scopes a subtree by id prefix, and a reference's NodeId is Area/Line/Equipment/Name + // while the equipment folder's is its EquipmentId — so in production the subtree does NOT reach it, + // and a filter would leak the node. Where an id does sit under the equipment id, the old order had the + // subtree take the node first, this loop then reported it unknown, and the deploy ratcheted to a full + // rebuild — severing every subscription on the server to remove one equipment. Removing references + // first is correct for both shapes: each is still present, gets its terminal Bad, and goes through + // RemoveVariableNode, which also severs its inverse Organizes edge on the surviving raw node. foreach (var v in plan.RemovedUnsReferenceVariables) { SafeWriteValue(v.NodeId, null, OpcUaStatus.Bad, ts, AddressSpaceRealm.Uns); if (!SafeRemoveVariable(surgical, v.NodeId, AddressSpaceRealm.Uns)) return false; } + // Removed equipment — one subtree teardown each (subsumes their child tags/vtags/alarms). The + // top-of-Apply block already wrote the equipment id's terminal condition state. + foreach (var eq in plan.RemovedEquipment) + { + if (!SafeRemoveEquipmentSubtree(surgical, eq.EquipmentId, AddressSpaceRealm.Uns)) return false; + } + // v3 Batch 4 — removed RAW tags (Raw realm): an alarm-bearing raw tag is a condition node (terminal // RemovedConditionState + RemoveAlarmConditionNode); a plain raw value tag is a variable (terminal Bad // + RemoveVariableNode). @@ -522,10 +533,8 @@ public sealed class AddressSpaceApplier foreach (var a in plan.AddedAlarms) ids.Add(a.EquipmentId); // v3 Batch 4 / review I5 — an added UNS reference variable is materialised directly under its // equipment folder (MaterialiseUnsReferences passes EquipmentId as the parent), and the equipment - // folder is a UNS-realm node — the realm AnnounceAddedNodes hardcodes. So it announces correctly with - // no realm plumbing. (Added RAW containers/tags are still NOT announced: their parents are Raw-realm - // nodes, which needs a realm-aware RaiseNodesAddedModelChange call — see the gap note in - // OpcUaPublishActor.HandleRebuild.) + // folder is a UNS-realm node, announced in the Uns realm like every id this method returns. Added RAW + // containers/tags have Raw-realm parents and are computed separately (ComputeRawAddAnnouncements). foreach (var v in plan.AddedUnsReferenceVariables) ids.Add(v.EquipmentId); foreach (var eq in plan.AddedEquipment) ids.Add(string.IsNullOrWhiteSpace(eq.UnsLineId) ? eq.EquipmentId : eq.UnsLineId); @@ -535,6 +544,42 @@ public sealed class AddressSpaceApplier return list; } + /// + /// Gitea #599 (archreview 01/U-8 = 03/P6) — the Raw-realm counterpart of + /// : the deduplicated, ordinal-sorted RAW parent node ids to announce + /// after a non-rebuild apply materialised added raw containers (Folder/Driver/Device/TagGroup) and raw + /// tags. Each added node announces its ParentNodeId — the exact parent + /// MaterialiseRawSubtree hangs it under. A parent that is ITSELF added in the same plan is skipped: + /// its own parent is announced, and a client re-browsing there discovers the whole new subtree, so the + /// inner announcements would only be noise. A top-level container (no parent — it hangs under the shared + /// root folder, which has no realm-qualified id to announce) announces itself, the same convention an + /// added equipment with no line uses. + /// + /// The plan whose added raw nodes' parents to announce. + /// The distinct affected Raw-realm parent node ids, ordinal-sorted. + public IReadOnlyList ComputeRawAddAnnouncements(AddressSpacePlan plan) + { + ArgumentNullException.ThrowIfNull(plan); + + var added = new HashSet(StringComparer.Ordinal); + foreach (var c in plan.AddedRawContainers) added.Add(c.NodeId); + foreach (var t in plan.AddedRawTags) added.Add(t.NodeId); + + var ids = new HashSet(StringComparer.Ordinal); + void Announce(string nodeId, string? parentNodeId) + { + if (string.IsNullOrWhiteSpace(parentNodeId)) ids.Add(nodeId); + else if (!added.Contains(parentNodeId)) ids.Add(parentNodeId); + } + + foreach (var c in plan.AddedRawContainers) Announce(c.NodeId, c.ParentNodeId); + foreach (var t in plan.AddedRawTags) Announce(t.NodeId, t.ParentNodeId); + + var list = ids.ToList(); + list.Sort(StringComparer.Ordinal); + return list; + } + /// The materialise-parent node id a tag/vtag's variable (or native-alarm condition) hangs /// under: the equipment folder when is null/empty, else the FolderPath /// sub-folder. Mirrors the parent derivation the Materialise passes use. @@ -561,21 +606,29 @@ public sealed class AddressSpaceApplier /// /// Announce a Part 3 GeneralModelChangeEvent(NodeAdded) per distinct affected parent from - /// , so model-aware clients re-browse the parents the new nodes - /// were added under. Called by OpcUaPublishActor AFTER the Materialise passes (the nodes exist - /// by then) and ONLY on a non-rebuild apply — after a full rebuild the announcement is moot - /// (subscriptions are dead anyway). Uses the existing, guard-covered + /// (Uns realm) and + /// (Raw realm), so model-aware clients re-browse the parents the new nodes were added under. Called by + /// OpcUaPublishActor AFTER the Materialise passes (the nodes exist by then) and ONLY on a + /// non-rebuild apply — after a full rebuild the announcement is moot (subscriptions are dead anyway). + /// Uses the existing, guard-covered, realm-aware /// — no new sink member. Each call is /// Safe-wrapped: a faulting announcement can never break a deploy (the nodes already stand). + /// Gitea #599: until the raw half existed, surgically-added raw containers/tags were materialised + /// but never announced, so a connected model-aware client could not discover them until it happened to + /// re-browse. (Before surgical deploys every raw add rebuilt the address space, which hid the gap.) /// /// The plan whose added nodes to announce. public void AnnounceAddedNodes(AddressSpacePlan plan) { - foreach (var id in ComputeAddAnnouncements(plan)) - { - try { _sink.RaiseNodesAddedModelChange(id, AddressSpaceRealm.Uns); } - catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: RaiseNodesAddedModelChange threw for {Node}", id); } - } + foreach (var id in ComputeAddAnnouncements(plan)) SafeAnnounce(id, AddressSpaceRealm.Uns); + foreach (var id in ComputeRawAddAnnouncements(plan)) SafeAnnounce(id, AddressSpaceRealm.Raw); + } + + /// Raise one NodeAdded model change, swallowing (and Warning-logging) any sink fault. + private void SafeAnnounce(string nodeId, AddressSpaceRealm realm) + { + try { _sink.RaiseNodesAddedModelChange(nodeId, realm); } + catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: RaiseNodesAddedModelChange threw for {Node} ({Realm})", nodeId, realm); } } /// diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs index b7f8b2af..27a321f7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs @@ -592,15 +592,11 @@ public sealed class OpcUaPublishActor : ReceiveActor, IWithTimers // (subscriptions are dead anyway) — the guard skips it. AnnounceAddedNodes is Safe-wrapped // internally, so a faulting announce can never break the deploy. // - // KNOWN GAP, and NOT a neutral pre-existing one — the delta work made it matter. Before this - // branch, a plan carrying raw adds went through a full RebuildAddressSpace, so every client had to - // re-browse anyway and the missing announcement changed nothing observable. Now those adds are - // materialised surgically while existing subscriptions stay alive, so an unannounced add is - // genuinely INVISIBLE until a client happens to browse again. - // Added UNS reference variables ARE announced (their equipment folder is the parent, which is the - // realm AnnounceAddedNodes hardcodes). Added RAW containers/tags are still not: the announce is - // Uns-only, and a raw parent needs a realm-aware RaiseNodesAddedModelChange call. That is a real - // follow-up, not an accepted limitation. + // The announce is realm-aware (Gitea #599): UNS parents in the Uns realm, and the parents of + // added RAW containers/tags in the Raw realm. It matters because of the delta work — before it, a + // plan carrying raw adds went through a full RebuildAddressSpace, so every client re-browsed + // anyway; now those adds are materialised surgically while existing subscriptions stay alive, and + // an unannounced add would stay invisible until a client happened to browse again. if (!outcome.RebuildCalled) _applier.AnnounceAddedNodes(plan); OtOpcUaTelemetry.OpcUaSinkWrite.Add(1, new KeyValuePair("kind", "rebuild")); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs index 1251f581..db71e68d 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs @@ -1459,6 +1459,62 @@ public sealed class AddressSpaceApplierTests new[] { "eq-1", V3NodeIds.Uns("eq-1", "Diag"), "line-7" }.OrderBy(x => x)); } + /// + /// Gitea #599 (archreview 01/U-8 = 03/P6) — surgically-added RAW nodes are announced in the RAW realm. + /// A new tag under an existing device announces that device; a new TagGroup carrying a new tag under a + /// new Device announces only the Device's existing parent — the inner parents are themselves new, and a + /// client re-browsing the outer one discovers the whole new subtree. Before the fix only Uns-realm + /// parents were announced, so these adds were invisible to connected model-aware clients. + /// + [Fact] + public void AnnounceAddedNodes_announces_added_raw_nodes_in_the_raw_realm() + { + var sink = new RecordingSink(); + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + + var plan = EmptyPlan with + { + AddedRawContainers = new[] + { + new RawContainerNode("Plant/Modbus/dev2", "Plant/Modbus", "dev2", RawNodeKind.Device), + new RawContainerNode("Plant/Modbus/dev2/grp", "Plant/Modbus/dev2", "grp", RawNodeKind.TagGroup), + }, + AddedRawTags = new[] + { + RawTag("Plant/Modbus/dev2/grp/speed") with { ParentNodeId = "Plant/Modbus/dev2/grp" }, + RawTag("Plant/Modbus/dev1/temp"), // parent Plant/Modbus/dev1 already exists + }, + // A UNS add in the same plan still announces in the Uns realm. + AddedUnsReferenceVariables = new[] { UnsRef("filling/line-1/station-1/speed", "speed") }, + }; + + applier.AnnounceAddedNodes(plan); + + sink.ModelChangeRealmQueue.OrderBy(x => x.NodeId, StringComparer.Ordinal).ToArray().ShouldBe(new[] + { + ("Plant/Modbus", AddressSpaceRealm.Raw), + ("Plant/Modbus/dev1", AddressSpaceRealm.Raw), + ("eq-1", AddressSpaceRealm.Uns), + }); + } + + /// A top-level raw container (no parent: it hangs under the shared root, which has no + /// realm-qualified id) announces itself; duplicates collapse; the Uns-side computation is unaffected. + [Fact] + public void ComputeRawAddAnnouncements_top_level_container_announces_itself_and_dedups() + { + var applier = new AddressSpaceApplier(new RecordingSink(), NullLogger.Instance); + + var plan = EmptyPlan with + { + AddedRawContainers = new[] { new RawContainerNode("Plant2", null, "Plant2", RawNodeKind.Folder) }, + AddedRawTags = new[] { RawTag("Plant/Modbus/dev1/a"), RawTag("Plant/Modbus/dev1/b") }, + }; + + applier.ComputeRawAddAnnouncements(plan).ShouldBe(new[] { "Plant/Modbus/dev1", "Plant2" }); + applier.ComputeAddAnnouncements(plan).ShouldBeEmpty(); + } + // ----- Surgical v3 deltas: raw-tag attribute edits (Raw realm) + UNS-reference remove ----- // Task 4 taught AddressSpaceChangeClassifier that a non-alarm RawTagDelta and any UnsReferenceDelta are // surgical-eligible; these pin the applier actually APPLYING them (before this, such a plan classified @@ -1963,7 +2019,13 @@ public sealed class AddressSpaceApplierTests public List ModelChangeCalls => ModelChangeQueue.ToList(); /// Records a NodeAdded model-change announcement. /// The node under which discovered nodes were added. - public void RaiseNodesAddedModelChange(string affectedNodeId, AddressSpaceRealm realm = AddressSpaceRealm.Uns) => ModelChangeQueue.Enqueue(affectedNodeId); + public void RaiseNodesAddedModelChange(string affectedNodeId, AddressSpaceRealm realm = AddressSpaceRealm.Uns) + { + ModelChangeQueue.Enqueue(affectedNodeId); + ModelChangeRealmQueue.Enqueue((affectedNodeId, realm)); + } + /// Gets the NodeAdded announcements WITH the realm each was raised in (Gitea #599). + public ConcurrentQueue<(string NodeId, AddressSpaceRealm Realm)> ModelChangeRealmQueue { get; } = new(); public void WireAlarmNotifiers(string alarmNodeId, AddressSpaceRealm alarmRealm, IReadOnlyList notifierFolderNodeIds, AddressSpaceRealm notifierFolderRealm) { } public void AddReference(string sourceNodeId, AddressSpaceRealm sourceRealm, string targetNodeId, AddressSpaceRealm targetRealm, string referenceType = "Organizes") { } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs index b064ddf0..f50a2d57 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs @@ -182,6 +182,104 @@ public sealed class NodeManagerSurgicalRemoveTests : IDisposable await host.DisposeAsync(); } + // ---------- Gitea #599 (archreview 03/P5 + 03/S20): equipment-with-references removal ---------- + + /// + /// S20 — the subtree removal severs the inverse half of every cross-tree Organizes edge a removed + /// variable holds. The raw node survives a UNS-scoped removal, so without this it keeps a browsable, + /// unresolvable reference to a node that no longer exists — the hazard RemoveVariableNode was + /// already fixed for (2026-08-15 review I3), left open on the subtree path. + /// + [Trait("Category", "Unit")] + [Fact] + public async Task RemoveEquipmentSubtree_severs_the_inverse_Organizes_edge_on_the_surviving_raw_node() + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.EnsureVariable("Plant/Modbus/dev1/temp", null, "temp", "Float", writable: false, realm: AddressSpaceRealm.Raw); + nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1", realm: AddressSpaceRealm.Uns); + nm.EnsureVariable("eq-1/Temp", "eq-1", "Temp", "Float", writable: false, realm: AddressSpaceRealm.Uns); + nm.AddReference("eq-1/Temp", AddressSpaceRealm.Uns, "Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw); + var mirror = nm.TryGetVariable("eq-1/Temp", AddressSpaceRealm.Uns)!.NodeId; + InverseOrganizesTo(nm, mirror).ShouldBeTrue(); // precondition: the edge really is there + + nm.RemoveEquipmentSubtree("eq-1", AddressSpaceRealm.Uns).ShouldBeTrue(); + + nm.TryGetVariable("Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw).ShouldNotBeNull(); + InverseOrganizesTo(nm, mirror).ShouldBeFalse(); + } + finally + { + await host.DisposeAsync(); + } + } + + /// + /// P5 — removing an equipment that holds UNS references is a scoped in-place removal, NOT a full rebuild, + /// and leaves the surviving raw node with no dangling inverse edge. Run for both id shapes: a reference + /// whose NodeId sits under the equipment id (so the subtree removal reaches it — before the fix the + /// reference loop then found it gone and ratcheted to a rebuild, severing every subscription on the + /// server), and the production shape, an Area/Line/Equipment/Name NodeId parented to an + /// id-named equipment folder (which the subtree's id-prefix scope does not reach). + /// + [Trait("Category", "Unit")] + [Theory] + [InlineData("eq-1/Temp")] + [InlineData("Area/Line/Eq/Temp")] + public async Task Removing_equipment_with_references_is_surgical_and_leaves_no_dangling_edge(string mirrorId) + { + var (host, server) = await BootAsync(); + try + { + var nm = server.NodeManager!; + nm.EnsureVariable("Plant/Modbus/dev1/temp", null, "temp", "Float", writable: false, realm: AddressSpaceRealm.Raw); + nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1", realm: AddressSpaceRealm.Uns); + nm.EnsureVariable(mirrorId, "eq-1", "Temp", "Float", writable: false, realm: AddressSpaceRealm.Uns); + nm.AddReference(mirrorId, AddressSpaceRealm.Uns, "Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw); + // A bystander whose value must survive (a rebuild would drop it). + nm.EnsureFolder("eq-2", parentNodeId: null, displayName: "Equipment 2", realm: AddressSpaceRealm.Uns); + nm.EnsureVariable("eq-2/S", "eq-2", "S", "Float", writable: false, realm: AddressSpaceRealm.Uns); + var mirror = nm.TryGetVariable(mirrorId, AddressSpaceRealm.Uns)!.NodeId; + + var plan = new AddressSpacePlan( + AddedEquipment: Array.Empty(), + RemovedEquipment: new[] { new EquipmentNode("eq-1", "Equipment 1", "") }, + ChangedEquipment: Array.Empty(), + AddedDrivers: Array.Empty(), + RemovedDrivers: Array.Empty(), + ChangedDrivers: Array.Empty(), + AddedAlarms: Array.Empty(), + RemovedAlarms: Array.Empty(), + ChangedAlarms: Array.Empty()) + { + RemovedUnsReferenceVariables = new[] + { + new UnsReferenceVariable("ref-1", "eq-1", mirrorId, "Temp", "Plant/Modbus/dev1/temp", "Float", Writable: false), + }, + }; + + var outcome = new AddressSpaceApplier(new SdkAddressSpaceSink(nm), + Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance).Apply(plan); + + outcome.RebuildCalled.ShouldBeFalse(); + nm.TryGetVariable(mirrorId, AddressSpaceRealm.Uns).ShouldBeNull(); + nm.TryGetFolder("eq-1").ShouldBeNull(); + nm.TryGetVariable("eq-2/S", AddressSpaceRealm.Uns).ShouldNotBeNull(); // positive control: no rebuild + nm.TryGetVariable("Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw).ShouldNotBeNull(); + InverseOrganizesTo(nm, mirror).ShouldBeFalse(); + } + finally + { + await host.DisposeAsync(); + } + } + + private static bool InverseOrganizesTo(OtOpcUaNodeManager nm, NodeId mirror) => + nm.GetNodeReferences("Plant/Modbus/dev1/temp", AddressSpaceRealm.Raw) + .Any(r => r.ReferenceTypeId == ReferenceTypeIds.Organizes && r.IsInverse && r.TargetId == mirror); + private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync() { var host = new OpcUaApplicationHost( diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/UnsReferenceBackingParityTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/UnsReferenceBackingParityTests.cs new file mode 100644 index 00000000..69855bab --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/UnsReferenceBackingParityTests.cs @@ -0,0 +1,158 @@ +using System.Reflection; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; + +/// +/// Gitea #599 / archreview 03/U11 — a mechanical guard on the Backing*-field parity invariant. +/// +/// +/// +/// A non-alarm raw-tag delta over the classifier's surgical whitelist is applied IN PLACE on the raw +/// node, and every referencing UNS mirror is repaired only because the same edit makes its +/// record unequal — the planner then emits a coincident +/// UnsReferenceDelta. That works only while every whitelisted raw-tag field the mirror inherits +/// has a counterpart on (same name, or Backing + name). +/// Omitting one is silent: the raw node updates, the mirror stays stale, and nothing fails. Until this +/// test the invariant was enforced by comments alone. +/// +/// +/// The whitelist is DISCOVERED, not restated: each property is perturbed in +/// isolation and asked of . So +/// widening the classifier whitelist without mirroring the field fails here, and so does adding a +/// Backing* field that names no raw-tag field. +/// +/// +public sealed class UnsReferenceBackingParityTests +{ + /// + /// Surgically-eligible raw-tag fields that a UNS mirror deliberately does NOT inherit, each with the + /// reason. Anything else the classifier lets through surgically must be mirrored. + /// + private static readonly IReadOnlyDictionary NotInheritedByMirror = new Dictionary + { + // Node-inert for a value tag: it only drives native-alarm notifier wiring, and an alarm-bearing delta + // never takes the surgical path. The mirror is the referencing side, so it has nothing to inherit. + [nameof(RawTagPlan.ReferencingEquipmentPaths)] = "notifier wiring only; the mirror is the referencing side", + }; + + private static RawTagPlan Baseline() => new( + TagId: "tag-1", + NodeId: "MAIN/modbus/dev/grp/temp", + ParentNodeId: "MAIN/modbus/dev/grp", + DriverInstanceId: "drv-1", + Name: "temp", + DataType: "Float", + Writable: false, + Alarm: null, + ReferencingEquipmentPaths: new[] { "Area/Line/Eq" }, + IsHistorized: false, + HistorianTagname: null, + IsArray: false, + ArrayLength: null, + Realm: AddressSpaceRealm.Raw); + + /// The raw-tag fields the classifier lets through surgically, found by perturbing each one. + private static IReadOnlyList SurgicallyEligibleRawTagFields() + { + var baseline = Baseline(); + var eligible = new List(); + foreach (var p in RecordProperties(typeof(RawTagPlan))) + { + var current = Perturb(baseline, p); + if (AddressSpaceChangeClassifier.RawTagDeltaIsSurgicalEligible(new AddressSpacePlan.RawTagDelta(baseline, current))) + eligible.Add(p); + } + return eligible; + } + + /// + /// Every raw-tag field the classifier applies surgically is mirrored on the UNS reference record (or is + /// explicitly listed as not inherited) — with the same type, so the planner's record equality sees the + /// edit. + /// + [Trait("Category", "Unit")] + [Fact] + public void Every_surgically_eligible_raw_tag_field_is_mirrored_on_the_UNS_reference_record() + { + var eligible = SurgicallyEligibleRawTagFields(); + + // Positive control: the perturbation machinery really does find the whitelist. If it found nothing, + // the loop below would pass vacuously. + eligible.Select(p => p.Name).ShouldBe( + new[] + { + nameof(RawTagPlan.DataType), nameof(RawTagPlan.Writable), nameof(RawTagPlan.ReferencingEquipmentPaths), + nameof(RawTagPlan.IsHistorized), nameof(RawTagPlan.HistorianTagname), nameof(RawTagPlan.IsArray), + nameof(RawTagPlan.ArrayLength), + }, + ignoreOrder: true); + + var mirror = RecordProperties(typeof(UnsReferenceVariable)).ToDictionary(p => p.Name, StringComparer.Ordinal); + foreach (var p in eligible) + { + if (NotInheritedByMirror.ContainsKey(p.Name)) continue; + + var counterpart = mirror.GetValueOrDefault(p.Name) ?? mirror.GetValueOrDefault("Backing" + p.Name); + counterpart.ShouldNotBeNull( + $"RawTagPlan.{p.Name} is applied surgically on the raw node, but UnsReferenceVariable has neither " + + $"'{p.Name}' nor 'Backing{p.Name}' — a raw-side edit of it would leave every referencing UNS mirror stale."); + counterpart.PropertyType.ShouldBe(p.PropertyType, $"UnsReferenceVariable.{counterpart.Name} vs RawTagPlan.{p.Name}"); + } + } + + /// Every Backing* field on the UNS reference record names a real raw-tag field of the same + /// type — a misspelt or orphaned mirror field repairs nothing. + [Trait("Category", "Unit")] + [Fact] + public void Every_Backing_field_names_a_raw_tag_field_of_the_same_type() + { + var raw = RecordProperties(typeof(RawTagPlan)).ToDictionary(p => p.Name, StringComparer.Ordinal); + var backing = RecordProperties(typeof(UnsReferenceVariable)) + .Where(p => p.Name.StartsWith("Backing", StringComparison.Ordinal) && p.Name != nameof(UnsReferenceVariable.BackingRawPath)) + .ToList(); + + backing.ShouldNotBeEmpty(); + foreach (var b in backing) + { + var source = raw.GetValueOrDefault(b.Name["Backing".Length..]); + source.ShouldNotBeNull($"UnsReferenceVariable.{b.Name} names no RawTagPlan field"); + source.PropertyType.ShouldBe(b.PropertyType, $"UnsReferenceVariable.{b.Name} vs RawTagPlan.{source.Name}"); + } + } + + /// The allow-list stays honest: every entry must still be a surgically-eligible raw-tag field. + [Trait("Category", "Unit")] + [Fact] + public void Not_inherited_allow_list_names_only_surgically_eligible_fields() + { + var eligible = SurgicallyEligibleRawTagFields().Select(p => p.Name).ToHashSet(StringComparer.Ordinal); + foreach (var name in NotInheritedByMirror.Keys) eligible.ShouldContain(name); + } + + private static IEnumerable RecordProperties(Type t) => + t.GetProperties(BindingFlags.Public | BindingFlags.Instance) + .Where(p => p.CanWrite && p.Name != "EqualityContract"); + + /// A copy of differing ONLY in . + private static RawTagPlan Perturb(RawTagPlan source, PropertyInfo property) + { + var clone = (RawTagPlan)typeof(RawTagPlan).GetMethod("$")!.Invoke(source, null)!; + var value = property.GetValue(source); + object? changed = property.PropertyType switch + { + var t when t == typeof(bool) => !(bool)value!, + var t when t == typeof(string) => (string?)value + "-changed", + var t when t == typeof(uint?) => ((uint?)value ?? 0u) + 1u, + var t when t == typeof(IReadOnlyList) => ((IReadOnlyList)value!).Append("Other/Eq").ToArray(), + var t when t == typeof(AddressSpaceRealm) => (AddressSpaceRealm)value! == AddressSpaceRealm.Raw ? AddressSpaceRealm.Uns : AddressSpaceRealm.Raw, + var t when t == typeof(EquipmentTagAlarmInfo) => new EquipmentTagAlarmInfo("OffNormalAlarm", 500), + var t => throw new InvalidOperationException( + $"RawTagPlan.{property.Name} has type {t.Name}, which this guard cannot perturb — add a case so the field is checked."), + }; + property.SetValue(clone, changed); + return clone; + } +} From bd63810645650b9bc55acd9e789b407b994bc11b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 07:12:29 -0400 Subject: [PATCH 2/2] fix(alarms): native ack refused off-Primary, commented re-ack reported, real UnshelveTime (#597) - Native-condition Acknowledge on a non-Primary node returns BadInvalidState (nothing routed/applied/reported) via NativeAlarmAckPrimaryGate, fed from IDriverPrimaryRoleSource by OtOpcUaServerHostedService. - A re-ack carrying a comment reports one comment event (Acked/Confirmed unchanged). - TimedShelve applies its requested duration and the scripted projection carries UnshelveAtUtc, so ShelvingState/UnshelveTime counts down instead of ~24.8 days. - Native re-ack still routed to the driver (deliberate: documented retry path), pinned. - AdminUI /alerts client ack/shelve path test; docs/AlarmTracking.md decisions incl. accepted SDK EventsMonitored residual (item 6). - Also #599/03/S20: RemoveEquipmentSubtree severs inverse Organizes edges. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/AlarmTracking.md | 86 ++++++-- docs/ScriptedAlarms.md | 2 +- .../OpcUa/AlarmConditionSnapshot.cs | 9 +- .../OpcUa/OtOpcUaServerHostedService.cs | 26 ++- .../OtOpcUaNodeManager.cs | 88 ++++++-- .../OtOpcUaSdkServer.cs | 16 ++ .../ScriptedAlarms/ScriptedAlarmHostActor.cs | 4 +- .../AlertsAckPathTests.cs | 81 ++++++++ .../ConditionMethodEventTests.cs | 195 ++++++++++++++++-- .../ServerEventRig.cs | 3 + 10 files changed, 459 insertions(+), 51 deletions(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AlertsAckPathTests.cs diff --git a/docs/AlarmTracking.md b/docs/AlarmTracking.md index 6504c719..85f6365c 100644 --- a/docs/AlarmTracking.md +++ b/docs/AlarmTracking.md @@ -292,8 +292,9 @@ branches on native-ness and routes through a dedicated path — separate from the scripted `AlarmCommandRouter`: 1. **`OtOpcUaNodeManager.HandleNativeAlarmAck`** — gates on the caller's - `AlarmAck` role (fails closed: no role → `BadUserAccessDenied`), then - dispatches a `NativeAlarmAck(ConditionNodeId, Comment, OperatorUser)` + `AlarmAck` role (fails closed: no role → `BadUserAccessDenied`), then on the + node's redundancy role (not the Primary → `BadInvalidState`, see + §"Acknowledge on a non-Primary node" below), then dispatches a `NativeAlarmAck(ConditionNodeId, Comment, OperatorUser)` to the `NativeAlarmAckRouter` seam (fire-and-forget, non-blocking under the node-manager Lock). `OperatorUser` carries the authenticated session principal's display name. @@ -331,20 +332,38 @@ projection's Retain rule, `Active || !Acked`) after the SDK handler, before the one ack event. ScadaBridge maps `ConfirmedState` into its alarm state, so this is client-visible. -> **Secondary-node ack flap.** On a node that is not the redundancy Primary, -> an OPC UA Acknowledge is applied to the local condition and reported (one -> event, Acked), but `DriverHostActor`'s Primary gate drops the route upstream. -> The driver never acknowledges, so its next transition for that alarm is -> projected with `Acked = false` and the node reverts — with its own event. A -> client acking against the Secondary sees acked → unacked. Ack against the -> Primary (the higher ServiceLevel) for the ack to stick. +> **Acknowledge on a non-Primary node is refused** (Gitea #597). On a node +> that is not the redundancy Primary, an OPC UA Acknowledge of a **native** +> condition returns **`BadInvalidState`** ("not the redundancy Primary; +> acknowledge on the Primary") — nothing is routed, applied or reported. The +> verdict is the same data-plane one the device-write gate and +> `DriverHostActor`'s ack route use (`PrimaryGatePolicy.ShouldServiceAsPrimary`, +> published to `IDriverPrimaryRoleSource` on every redundancy snapshot and +> wired onto the node manager by `OtOpcUaServerHostedService`). It is checked +> after authorization, so a caller who may not ack learns nothing about the +> node's role. +> +> This replaces a documented flap: the Secondary used to apply and report the +> ack locally while `DriverHostActor` dropped the route upstream, so the driver +> never acknowledged, its next transition projected `Acked = false`, and the +> client saw acked → unacked. An honest refusal is client-visible — a client +> (ScadaBridge included) that acks against the Secondary now gets a Bad result +> instead of a Good that later reverts — so ack against the Primary (the higher +> ServiceLevel). Scripted conditions are **not** gated: their engine runs on +> both nodes, so an ack on the Secondary is real. Residual: the published +> verdict changes only on a redundancy snapshot, so in the boot window of a +> multi-node cluster (no snapshot yet) the node still accepts, while +> `DriverHostActor`'s live check may drop the route — the old flap, limited to +> that window. > **Best-effort during reconnect.** If a native ack arrives while the owning -> driver is `Connecting`/`Reconnecting` (or on a non-Primary node), it is -> dropped with a debug log and **not** retried upstream — the OPC UA Part 9 -> `AlarmConditionState` was already committed locally at step 1, so the rare -> outcome is "acked locally but not propagated to AVEVA." Re-issue the ack once -> the driver is reconnected if upstream propagation is required. +> driver is `Connecting`/`Reconnecting` (or, in the boot window above, on a +> node whose role is not yet known), it is dropped with a debug log and **not** +> retried upstream — the OPC UA Part 9 `AlarmConditionState` was already +> committed locally at step 1, so the rare outcome is "acked locally but not +> propagated to AVEVA." Re-issue the ack once the driver is reconnected if +> upstream propagation is required — a re-ack of a node already showing Acked +> is still routed to the driver for exactly this reason. ### Legacy sub-attribute path @@ -412,8 +431,9 @@ reports nothing. `AreEventsMonitored` bookkeeping, which is imprecise for conditions materialised under an already-subscribed folder. - **The SDK's shelve methods never apply shelving state** (they only call - `OnShelve`), so the wrapper applies it — with the same - `(shelved, oneShot, 0)` mapping the projection uses — before reporting. + `OnShelve`), so the wrapper applies it — with the same `(shelved, oneShot)` + mapping the projection uses, and a TimedShelve with its requested duration — + before reporting. - **The re-projection does not duplicate it.** The engine's (or driver's) answer to the routed command reaches `WriteAlarmCondition`'s delta-gate, which compares against the node's current state — already the new state — @@ -425,7 +445,16 @@ reports nothing. nothing. The SDK accepts acknowledging an acknowledged condition and sets `Confirmed = false` again, so a re-ack also restores the pre-call Confirmed and Retain — silent, and a scripted node never drifts from its engine. - AddComment always reports (the comment is the change). + AddComment always reports (the comment is the change), and so does a re-ack + **carrying a comment** (Gitea #597): the SDK writes that comment to the node, + so it is reported once, with Acked/Confirmed unchanged. Before #597 the + comment landed on the node but no subscriber ever saw it. +- **A native re-ack is still routed to the driver** (deliberate, #597). The + route is fire-and-forget, so a node showing Acked does not prove the + upstream system acknowledged — an ack dropped during a reconnect leaves + exactly that state — and re-acking is the documented retry (§"Best-effort + during reconnect"). The cost is a duplicate ack upstream for an alarm that + was already acknowledged there. - **Native conditions: Acknowledge and AddComment only** — see §"Native alarm acknowledge → AVEVA" above. The other methods are refused (`BadNotSupported`, no event), and a native ack keeps the condition @@ -433,9 +462,26 @@ reports nothing. - **TimedShelve works (scripted).** `Create()` left `MaxTimeShelved` at 0, and the SDK rejects any shelving time above it, so every client TimedShelve failed with `BadShelvingTimeOutOfRange`. It is now `int.MaxValue` ms (≈24.8 - days). The node applies the shelve with no SDK timer of its own; the engine - owns the expiry and its Unshelved re-projection reports the one expiry - event. + days). The engine owns the expiry and its Unshelved re-projection reports + the one expiry event. +- **`UnshelveTime` is the real remaining time** (Gitea #597). Both the method + path and the projection used to apply a timed shelve with `shelvingTime 0`, + which the SDK reads as `MaxTimeShelved` — so every timed shelve advertised + ≈24.8 days remaining. The method now applies the requested duration, and the + engine's projection carries its expiry (`AlarmConditionSnapshot.UnshelveAtUtc`), + so `ShelvingState/UnshelveTime` counts down to the engine's expiry whichever + path shelved it (the AdminUI `/alerts` timed shelve included). This arms the + SDK's own unshelve timer at the real instant; it changes no node state and + only routes an `Unshelve` (user `system`) back to the engine — a no-op if the + engine's 5 s expiry sweep got there first, otherwise it simply lands the + expiry on time. +- **Accepted, not fixed — the SDK's `EventsMonitored` unit for late conditions** + (Gitea #597 item 6, from the #586 review). The SDK's `AreEventsMonitored` + counter never reaches a condition materialised under a folder that already had + an event subscriber. Our own reporting does not depend on it (above); the one + thing it still gates is the SDK's method **audit** event, and only while the + server is auditing — which no configuration in this repo enables. Closed as + accepted. - Method **audit** events (`AuditConditionAcknowledgeEventType`, …) are still the SDK's, delivered only while the server is auditing over an encrypted channel. diff --git a/docs/ScriptedAlarms.md b/docs/ScriptedAlarms.md index ce2717f3..9b2c467c 100644 --- a/docs/ScriptedAlarms.md +++ b/docs/ScriptedAlarms.md @@ -358,7 +358,7 @@ On allow, the handler publishes a `Commons.OpcUa.AlarmCommand` (containing comma ### Delta-gate de-duplication -`WriteAlarmCondition` fires a Part 9 condition event only when the incoming state differs from the node's current live state. For an OPC UA method call (Acknowledge, Confirm, AddComment, the shelve methods, Enable/Disable) the SDK applies the new state to the node, and the node manager then reports **the one** condition event for it from the method path (Gitea #587 — the SDK reports none of its own, since `AutoReportStateChanges` stays false; the shelve methods' state is applied by the node manager, because the SDK's never applies it; a call that changed nothing — e.g. a re-ack — reports nothing). Native (driver-fed) conditions support only Acknowledge and AddComment; their Confirm, shelve and Enable/Disable methods return `BadNotSupported`. The engine's later re-projection of the same transition finds the node already in that state and is suppressed, so each accepted call yields exactly one event. See `docs/AlarmTracking.md` §"One condition event per method call". +`WriteAlarmCondition` fires a Part 9 condition event only when the incoming state differs from the node's current live state. For an OPC UA method call (Acknowledge, Confirm, AddComment, the shelve methods, Enable/Disable) the SDK applies the new state to the node, and the node manager then reports **the one** condition event for it from the method path (Gitea #587 — the SDK reports none of its own, since `AutoReportStateChanges` stays false; the shelve methods' state is applied by the node manager, because the SDK's never applies it; a call that changed nothing — e.g. an uncommented re-ack — reports nothing; a re-ack carrying a comment reports that comment once, Gitea #597). Native (driver-fed) conditions support only Acknowledge and AddComment; their Confirm, shelve and Enable/Disable methods return `BadNotSupported`, and their Acknowledge is refused with `BadInvalidState` on a node that is not the redundancy Primary (Gitea #597). A timed shelve's `UnshelveTime` counts down to the engine's expiry (the projection carries `UnshelveAtUtc`). The engine's later re-projection of the same transition finds the node already in that state and is suppressed, so each accepted call yields exactly one event. See `docs/AlarmTracking.md` §"One condition event per method call". ### AdminUI path diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/AlarmConditionSnapshot.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/AlarmConditionSnapshot.cs index 5fd1b276..1330ce2c 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/AlarmConditionSnapshot.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/AlarmConditionSnapshot.cs @@ -24,6 +24,12 @@ namespace ZB.MOM.WW.OtOpcUa.Commons.OpcUa; /// Carries the full sub-code since #519, so a comms-lost condition reports /// rather than a bare . /// +/// +/// For , the UTC instant the engine expires the shelve; otherwise +/// null. Carried so the node's Part 9 ShelvingState.UnshelveTime reports the real remaining +/// time instead of the SDK's ~24.8-day ceiling (Gitea #597). A timed snapshot without it (a caller that +/// predates the field) falls back to that ceiling — the engine still owns the expiry either way. +/// public sealed record AlarmConditionSnapshot( bool Active, bool Acknowledged, @@ -32,7 +38,8 @@ public sealed record AlarmConditionSnapshot( AlarmShelvingKind Shelving, ushort Severity, string Message, - uint StatusCode = OpcUaStatus.Good); + uint StatusCode = OpcUaStatus.Good, + DateTime? UnshelveAtUtc = null); /// /// Commons-local mirror of the Core ShelvingKind enum so this assembly carries no diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs index adea5467..7f12e9ed 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs @@ -44,6 +44,7 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl private readonly ActorRegistry _actorRegistry; private readonly ILoggerFactory _loggerFactory; private readonly ILogger _logger; + private readonly IDriverPrimaryRoleSource? _primaryRoleSource; private OpcUaApplicationHost? _appHost; private OtOpcUaSdkServer? _server; @@ -88,6 +89,11 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl /// The shared clock — the one instance the authorizer and the API-key store bound /// also read. Its CreateTimer arms the revalidation tick. /// The logger factory for creating loggers. + /// Gitea #597 — the data-plane Primary verdict DriverHostActor + /// publishes on every redundancy snapshot (the same one its native-ack route and device-write gates apply). + /// Wired onto the node manager so a native-condition Acknowledge on a non-Primary node is refused rather than + /// applied and then reverted. Optional: a node with no driver role registers none, and null means "allow" — + /// the single-node posture UnmanagedDriverPrimaryRoleSource also answers with. public OtOpcUaServerHostedService( IOptions options, DeferredAddressSpaceSink deferredSink, @@ -101,7 +107,8 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl DataPlaneAuthorizer? dataPlaneAuthorizer, IOptions aclOptions, TimeProvider time, - ILoggerFactory loggerFactory) + ILoggerFactory loggerFactory, + IDriverPrimaryRoleSource? primaryRoleSource = null) { ArgumentNullException.ThrowIfNull(aclOptions); ArgumentNullException.ThrowIfNull(time); @@ -123,6 +130,7 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl _actorSystemAccessor = actorSystemAccessor; _actorRegistry = actorRegistry; _loggerFactory = loggerFactory; + _primaryRoleSource = primaryRoleSource; _logger = loggerFactory.CreateLogger(); } @@ -225,6 +233,21 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl } }); + // Gitea #597: a native ack is accepted only on the Primary. The verdict is the one DriverHostActor + // publishes and gates the route above on, so a Secondary refuses (BadInvalidState) instead of applying an + // ack the driver will never see and then reverting it. A volatile property read — safe under the SDK Lock. + if (_primaryRoleSource is { } primaryRoleSource) + { + _server.SetNativeAlarmAckPrimaryGate(() => primaryRoleSource.ShouldActAsPrimary); + } + else + { + // Every driver node registers one (AddOtOpcUaRuntime), so this is a wiring regression, not a mode: + // say so rather than let a Secondary silently go back to accepting acks it will revert. + _logger.LogWarning( + "OtOpcUaServerHostedService: no IDriverPrimaryRoleSource registered — native-alarm acknowledges are accepted on every node regardless of redundancy role"); + } + // Wire the reverse-path inbound operator-write gateway: a client write to a writable equipment-tag // node that passes the node manager's WriteOperate gate routes the write to the owning driver child // (RouteNodeWrite → NodeWriteResult) via the local DriverHostActor. The node manager calls the @@ -342,6 +365,7 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl _server?.SetNodeWriteGateway(null); // Clear the native-alarm-ack router so a late ack doesn't Tell a stopping DriverHostActor. _server?.SetNativeAlarmAckRouter(null); + _server?.SetNativeAlarmAckPrimaryGate(null); // Restore the Null historian so a late HistoryRead doesn't hit a disposed read client. _server?.SetHistorianDataSource(null); // Restore the empty authority so a late session activation cannot authorize against a stopping node. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 6e2e910c..b078231a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -205,6 +205,26 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// public Action? NativeAlarmAckRouter { get; set; } + /// + /// Gitea #597 — whether this node may accept an OPC UA Acknowledge of a NATIVE (driver-fed) condition + /// right now: the redundancy data-plane verdict (PrimaryGatePolicy.ShouldServiceAsPrimary, as + /// published to IDriverPrimaryRoleSource) — the same verdict DriverHostActor gates the + /// upstream ack route and device writes on. + /// + /// false ⇒ the Acknowledge is REFUSED with BadInvalidState before anything is routed or + /// applied. Without this the Secondary applied and reported the ack locally while + /// DriverHostActor dropped the route upstream, so the driver never acknowledged and its next + /// transition reverted the node — the client saw acked → unacked. Scripted conditions are not gated: + /// their engine runs on both nodes, so an ack against the Secondary is real. + /// + /// + /// Read under Lock on the SDK's Call thread, so it MUST be a cheap, non-blocking read (the + /// host passes a volatile-backed property). Null (the default) allows — the single-node posture, the + /// same default UnmanagedDriverPrimaryRoleSource answers with. + /// + /// + public Func? NativeAlarmAckPrimaryGate { get; set; } + private volatile IOpcUaNodeWriteGateway _nodeWriteGateway = NullOpcUaNodeWriteGateway.Instance; /// @@ -849,13 +869,16 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 if (condition.ShelvingState is not null) { // SetShelvingState(shelved, oneShot, shelvingTime): map our 3-way kind onto the SDK's - // (shelved, oneShot) flag pair. Timed shelving's expiry is owned by the engine, not the - // SDK timer, so we pass shelvingTime=0 (no SDK-managed auto-unshelve). + // (shelved, oneShot) flag pair. Timed shelving's expiry is owned by the engine; the time + // passed here is the engine's remaining time, so the node's UnshelveTime counts down to the + // real expiry (Gitea #597 — a 0 here reads as MaxTimeShelved, ~24.8 days). The SDK timer it + // arms only routes an Unshelve back to the engine (OnTimedUnshelve), a no-op once the + // engine's own expiry has run. condition.SetShelvingState( SystemContext, shelved: state.Shelving != AlarmShelvingKind.Unshelved, oneShot: state.Shelving == AlarmShelvingKind.OneShot, - shelvingTime: 0); + shelvingTime: state.Shelving == AlarmShelvingKind.Timed ? RemainingShelveMs(state.UnshelveAtUtc) : 0); } condition.SetSeverity(SystemContext, MapSeverity(state.Severity)); condition.Message.Value = new LocalizedText(state.Message); @@ -909,6 +932,22 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 } } + /// + /// Gitea #597 — the shelvingTime (ms) to hand SetShelvingState for a timed shelve expiring at + /// . 0 when the expiry is unknown (the SDK then uses + /// MaxTimeShelved, the pre-#597 behaviour); otherwise the remaining time, clamped to at least 1 ms + /// (an already-passed expiry must not read as "unknown" — the engine's own expiry is about to land) and to + /// at most int.MaxValue (the SDK timer's range). + /// + /// The engine's expiry instant, or null. + /// The shelving time in milliseconds. + internal static double RemainingShelveMs(DateTime? unshelveAtUtc) + { + if (unshelveAtUtc is not { } at) return 0; + var remaining = (at - DateTime.UtcNow).TotalMilliseconds; + return Math.Clamp(Math.Ceiling(remaining), 1, int.MaxValue); + } + /// /// #477 Layer 2 — apply a source-data quality annotation to a materialised condition, OUT OF BAND /// from any alarm transition. Used by the driver-connectivity path (comms lost → BadNoCommunication, @@ -1545,9 +1584,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// /// /// Shelving is applied here. The SDK's shelve methods never change ShelvingState - /// themselves, so an accepted shelve call applies it — with the same (shelved, oneShot, 0) - /// mapping projects — before reporting. The engine's re-projection of - /// the shelve then finds no delta. Every other method's state the SDK has already applied. + /// themselves, so an accepted shelve call applies it — with the same (shelved, oneShot) mapping + /// projects, and a TimedShelve with its requested duration so + /// UnshelveTime is real (Gitea #597) — before reporting. The engine's re-projection of the shelve + /// then finds no delta. Every other method's state the SDK has already applied. /// /// /// Exactly once. The whole call runs under Lock. The method handlers hand the command @@ -1569,7 +1609,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// the SDK handler, and nothing is reported when it did not move. The SDK accepts acknowledging an /// acknowledged condition and sets Confirmed = false again; a re-ack therefore also restores the /// pre-call Confirmed and Retain, so a scripted node never drifts from its engine (which ignores a - /// re-ack) and the call is silent. AddComment always reports: adding a comment is itself the change. + /// re-ack) and the call is silent — unless it carried a comment, which the SDK writes and which is then + /// reported once like AddComment (Gitea #597). AddComment always reports: adding a comment is itself + /// the change. A NATIVE re-ack is still routed to the driver: the route is fire-and-forget, so a node + /// showing Acked does not prove the upstream system acknowledged, and re-acking is the documented retry. /// /// /// A rejected call (bad , e.g. a denied gate, or a refused method on a native @@ -1582,8 +1625,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 if (alarm.Acknowledge?.OnCall is { } acknowledge) { + // A commented acknowledge always reports (Gitea #597): the SDK writes the comment even on a re-ack, + // and a new comment is a Part 9 change in its own right — exactly as AddComment always reports. An + // uncommented re-ack still changes nothing and stays silent. alarm.Acknowledge.OnCall = (context, m, objectId, eventId, comment) => - ReportAfterMethod(() => acknowledge(context, m, objectId, eventId, comment), apply: null, isAcknowledge: true); + ReportAfterMethod(() => acknowledge(context, m, objectId, eventId, comment), apply: null, isAcknowledge: true, + alwaysReport: !LocalizedText.IsNullOrEmpty(comment)); } WrapComment(alarm.Confirm, alwaysReport: false); @@ -1597,11 +1644,13 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 WrapGeneric(shelving.Unshelve, () => alarm.SetShelvingState(SystemContext, shelved: false, oneShot: false, shelvingTime: 0)); if (shelving.TimedShelve is { OnCall: { } timed } timedShelve) { - // shelvingTime 0, as WriteAlarmCondition projects it: the scripted engine owns the expiry and - // re-projects Unshelved when it passes (the SDK's own timer only routes an Unshelve back to it). + // The requested duration, so UnshelveTime counts down to the real expiry (Gitea #597 — 0 reads as + // MaxTimeShelved, ~24.8 days). The scripted engine still owns the expiry and re-projects Unshelved + // when it passes (with its own remaining time meanwhile — WriteAlarmCondition); the SDK timer this + // arms only routes an Unshelve back to it (OnTimedUnshelve), a no-op once the engine has expired it. timedShelve.OnCall = (context, method, objectId, shelvingTime) => ReportAfterMethod( () => timed(context, method, objectId, shelvingTime), - () => alarm.SetShelvingState(SystemContext, shelved: true, oneShot: false, shelvingTime: 0)); + () => alarm.SetShelvingState(SystemContext, shelved: true, oneShot: false, shelvingTime: shelvingTime)); } } @@ -1729,7 +1778,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// folder-scoped condition node id the driver-bound router resolves back to a driver ref. /// The acknowledge comment text, or null when none was supplied. /// ServiceResult.Good when allowed (the SDK then applies state; the method path reports the event); - /// BadUserAccessDenied when the gate vetoes (no route, no state mutation). + /// BadUserAccessDenied when the gate vetoes, BadInvalidState when this node is not the redundancy + /// Primary (, Gitea #597) — in both cases no route, no state mutation. private ServiceResult HandleNativeAlarmAck(ISystemContext context, ConditionState condition, LocalizedText? comment) { // Resolve + gate the SAME way HandleAlarmCommand does so native and scripted acks share one authz @@ -1738,6 +1788,16 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 var gate = EvaluateAlarmCommandGate(identity, condition, "Acknowledge"); if (gate is not null) return gate; + // Gitea #597 — PRIMARY GATE, after authorization (a caller who may not ack learns nothing about the + // node's role). Only the Primary routes a native ack upstream (DriverHostActor applies the same verdict + // to the route), so on any other node the ack is refused outright rather than applied locally and then + // reverted by the driver's next transition. Nothing is routed, applied or reported. + if (NativeAlarmAckPrimaryGate is { } primary && !primary()) + { + return new ServiceResult(StatusCodes.BadInvalidState, + new LocalizedText("This server is not the redundancy Primary; acknowledge the alarm on the Primary (the server with the higher ServiceLevel).")); + } + // Same condition-node-id extraction HandleAlarmCommand uses (condition.NodeId.Identifier?.ToString()): // for a native condition this is the folder-scoped NodeId string the DriverHostActor inverse map // (_alarmNodeIdByDriverRef value set) keys by, so the next task can resolve it back to (driver, ref). @@ -3046,6 +3106,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 { if (_variables.TryRemove(id, out var v)) { + // Gitea #599 (archreview 03/S20): sever the inverse half of each cross-tree Organizes edge + // first, exactly as RemoveVariableNode does — a UNS reference variable in scope points at a + // RAW node that survives this UNS-scoped removal and would otherwise keep a dangling edge. + SeverOutgoingReferences(v); v.Parent?.RemoveChild(v); PredefinedNodes?.Remove(v.NodeId); } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaSdkServer.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaSdkServer.cs index e563dd4b..9cdd91ce 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaSdkServer.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaSdkServer.cs @@ -60,6 +60,22 @@ public sealed class OtOpcUaSdkServer : StandardServer return true; } + /// + /// Gitea #597 — wire the redundancy verdict that decides whether this node accepts an OPC UA Acknowledge + /// of a NATIVE condition (); on any other node + /// the ack is refused with BadInvalidState instead of being applied locally and reverted by the + /// driver's next transition. Passing null clears it (allow). No-op (returns false) when the + /// node manager has not been created yet (mirrors ). + /// + /// A cheap, non-blocking read of the data-plane Primary verdict; null to allow. + /// true when set on a live node manager; false when no node manager exists yet. + public bool SetNativeAlarmAckPrimaryGate(Func? isPrimary) + { + if (_otOpcUaNodeManager is null) return false; + _otOpcUaNodeManager.NativeAlarmAckPrimaryGate = isPrimary; + return true; + } + /// /// Wire the reverse-path gateway for inbound operator writes to writable equipment-tag nodes onto the /// created . The host calls this after start with an diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs index bc7bfc1b..dce63eb6 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs @@ -739,7 +739,9 @@ public sealed class ScriptedAlarmHostActor : ReceiveActor // #478 — the condition's status is the worst status across the script's input tags at evaluation // time (carried on the event by the engine). A transition fired while an input is Uncertain projects // Uncertain here so the full-snapshot write doesn't clobber quality back to Good. - StatusCode: worstInputStatusCode); + StatusCode: worstInputStatusCode, + // Gitea #597 — the engine's own expiry, so the node's UnshelveTime counts down to it. + UnshelveAtUtc: condition.Shelving.Kind == ShelvingKind.Timed ? condition.Shelving.UnshelveAtUtc : null); /// Maps the Core onto the Commons /// mirror (the Commons assembly can't see the Core enum). diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AlertsAckPathTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AlertsAckPathTests.cs new file mode 100644 index 00000000..9ff01069 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/AlertsAckPathTests.cs @@ -0,0 +1,81 @@ +using Akka.Hosting; +using Akka.TestKit.Xunit2; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.AdminUI.Clients; +using ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Acls; +using ZB.MOM.WW.OtOpcUa.Commons.Messages.Admin; +using ZB.MOM.WW.OtOpcUa.Commons.Messages.Mesh; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; +using ZB.MOM.WW.OtOpcUa.ControlPlane; +using ZB.MOM.WW.OtOpcUa.ControlPlane.AdminOperations; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests; + +/// +/// Gitea #597 — the AdminUI /alerts acknowledge / shelve path, from the page's client call down to +/// the command the cluster receives: (what Alerts.razor's +/// per-row buttons call) Asks the real registered under +/// , which maps the request to an and hands +/// it to the mesh transport. Until now only the actor half had a test; the client seam the page goes +/// through had none. +/// +/// +/// The razor click handler itself is not driven here — this repo has no bUnit. What it adds on top of the +/// client call is the operator name and comment: null, both reproduced below exactly as the page +/// passes them. +/// +public sealed class AlertsAckPathTests : TestKit +{ + private (AdminOperationsClient Client, Akka.TestKit.TestProbe Mesh) Wire() + { + var mesh = CreateTestProbe("mesh-router"); + var actor = Sys.ActorOf(AdminOperationsActor.Props( + AclScopeTestDb.Factory($"alerts-ack-{Guid.NewGuid():N}"), + coordinator: CreateTestProbe("coordinator").Ref, + probes: Enumerable.Empty(), + meshRouter: mesh.Ref)); + var registry = new ActorRegistry(); + registry.Register(actor); + return (new AdminOperationsClient(registry), mesh); + } + + /// The page's Acknowledge reaches the cluster as an Acknowledge AlarmCommand on the + /// alarm-commands topic carrying the alarm id and the operator, and the page is told it was sent. + [Fact] + public async Task Acknowledge_from_the_alerts_page_dispatches_an_Acknowledge_command_and_reports_sent() + { + var (client, mesh) = Wire(); + + var result = await client.AcknowledgeAlarmAsync("alarm-42", "operator-jo", comment: null, CancellationToken.None); + + result.Ok.ShouldBeTrue(); + result.Message.ShouldBeNull(); + var sent = mesh.ExpectMsg(TimeSpan.FromSeconds(5)); + sent.Topic.ShouldBe(AlarmCommandsTopic.Name); + var command = sent.Message.ShouldBeOfType(); + command.AlarmId.ShouldBe("alarm-42"); + command.Operation.ShouldBe("Acknowledge"); + command.User.ShouldBe("operator-jo"); + command.Comment.ShouldBeNull(); + command.UnshelveAtUtc.ShouldBeNull(); + } + + /// The page's timed shelve carries its expiry through to the command — the instant the engine + /// projects as the node's UnshelveTime. + [Fact] + public async Task Timed_shelve_from_the_alerts_page_carries_its_expiry_to_the_command() + { + var (client, mesh) = Wire(); + var until = new DateTime(2026, 10, 8, 12, 30, 0, DateTimeKind.Utc); + + var result = await client.ShelveAlarmAsync( + "alarm-7", "operator-kim", ShelveKind.Timed, unshelveAtUtc: until, comment: null, CancellationToken.None); + + result.Ok.ShouldBeTrue(); + var command = mesh.ExpectMsg(TimeSpan.FromSeconds(5)).Message.ShouldBeOfType(); + command.Operation.ShouldBe("TimedShelve"); + command.UnshelveAtUtc.ShouldBe(until); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ConditionMethodEventTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ConditionMethodEventTests.cs index 5b9c7daf..a20c6c2c 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ConditionMethodEventTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ConditionMethodEventTests.cs @@ -266,10 +266,11 @@ public sealed class ConditionMethodEventTests /// reports exactly one unshelve event after the one shelve event. /// /// - /// The method path applies the shelve with shelvingTime 0, as the projection does, so the expiry is - /// the engine's (computed from the command's UnshelveAtUtc), never the SDK timer's — a node timer - /// would only route an Unshelve back to the engine anyway. The stand-in engine here schedules its expiry - /// from the real UnshelveAtUtc the method handler computed. + /// The expiry is the engine's (computed from the command's UnshelveAtUtc). Since Gitea #597 the node + /// is shelved for the real duration, so the SDK's own timer fires at the same instant — but it only routes + /// an Unshelve back to the engine (ignored by this stand-in, a no-op on an unshelved engine alarm) and + /// changes no node state, so it adds no event. The stand-in engine here schedules its expiry from the real + /// UnshelveAtUtc the method handler computed. /// [Fact] public async Task A_scripted_timed_shelve_expires_with_exactly_one_unshelve_event() @@ -326,17 +327,157 @@ public sealed class ConditionMethodEventTests } /// - /// Acknowledging an acknowledged condition reports nothing, and does not flip a scripted condition's - /// Confirmed out of step with its engine. + /// A scripted condition's events carry the real remaining shelve time in + /// ShelvingState/UnshelveTime — after a client TimedShelve, after the engine's re-projection of it, + /// and after an engine-driven timed shelve (the AdminUI /alerts path, which never calls the method). /// /// - /// The SDK accepts a re-ack and sets Confirmed false again; the engine ignores it. The scripted leg - /// acknowledges and confirms first, so the flip would be visible on the sentinel. + /// Gitea #597: both paths applied the shelve with shelvingTime 0, which the SDK reads as "the + /// longest shelve allowed" — MaxTimeShelved, int.MaxValue ms — so every timed shelve + /// advertised ~24.8 days remaining while the engine expired it on time. Read through the event select + /// clause rather than a Read of the child node: the condition's children carry their type's + /// instance-declaration NodeIds (i=9189 for UnshelveTime), so a direct Read reaches the type, not + /// this condition. + /// + [Fact] + public async Task A_scripted_timed_shelve_reports_the_real_remaining_unshelve_time() + { + var ct = TestContext.Current.CancellationToken; + var rig = await StartAsync("CondUnshelveTime", encrypted: false, ct); + try + { + rig.Enforce(true, [Key()], ClusterGrant(KeyPrincipal, "acl-key", All)); + rig.MaterialiseScripted(); + var current = PreState("TimedShelve", native: false) with { Message = "base" }; + rig.Nm.AlarmCommandRouter = cmd => + { + if (cmd.Operation != "TimedShelve") return; + // The engine's answer carries its own expiry — what ScriptedAlarmHostActor projects. + current = current with { Shelving = AlarmShelvingKind.Timed, UnshelveAtUtc = cmd.UnshelveAtUtc }; + rig.ProjectLater(native: false, current); + }; + + using var session = await rig.ConnectAsync(anonymous: false, ct); + var events = new RecordedEvents(); + var subscription = await RecordedEvents.SubscribeAsync(session, events, ct); + var item = RecordedEvents.AddEventItem(subscription, ObjectIds.Server); + await subscription.ApplyChangesAsync(ct); + double RemainingMs(string message) => + (double)events.ConditionEvents(item, message)[^1][RecordedEvents.UnshelveTimeField].Value; + + rig.Project(false, current); + await RecordedEvents.WaitUntilAsync(() => events.Has(item, "base")); + RemainingMs("base").ShouldBe(0d, "unshelved"); + + // The method's one event carries the shelve's remaining time … + await CallAsync(session, ConditionNodeId(session, false), "TimedShelve", null, ct, shelvingTimeMs: 60_000); + await rig.DrainProjectionsAsync(); + await RecordedEvents.WaitUntilAsync(() => events.ConditionEvents(item, "base").Count == 2); + RemainingMs("base").ShouldBeInRange(50_000, 60_000, "the TimedShelve method event"); + + // … and so does a later transition, after the engine's re-projection re-applied the shelve. + rig.Project(false, current = current with { Message = "after-reprojection" }); + await RecordedEvents.WaitUntilAsync(() => events.Has(item, "after-reprojection")); + RemainingMs("after-reprojection").ShouldBeInRange(50_000, 60_000, "after the engine's re-projection"); + + // Engine-driven (the AdminUI path): unshelve, then a timed shelve that reaches the node only as a + // projection, never through the method. + rig.Project(false, current = current with { Shelving = AlarmShelvingKind.Unshelved, UnshelveAtUtc = null, Message = "unshelved" }); + await RecordedEvents.WaitUntilAsync(() => events.Has(item, "unshelved")); + RemainingMs("unshelved").ShouldBe(0d); + rig.Project(false, current with + { + Shelving = AlarmShelvingKind.Timed, UnshelveAtUtc = DateTime.UtcNow.AddSeconds(30), Message = "projected", + }); + await RecordedEvents.WaitUntilAsync(() => events.Has(item, "projected")); + RemainingMs("projected").ShouldBeInRange(20_000, 30_000, "a projected timed shelve"); + + await subscription.DeleteAsync(true, ct); + } + finally { await rig.DisposeAsync(); } + } + + /// + /// On a node that is not the redundancy Primary, an OPC UA Acknowledge of a NATIVE condition is refused + /// with BadInvalidState: nothing is routed to the driver, nothing is applied, nothing is reported. + /// On the Primary the same call is accepted (the positive control). + /// + /// + /// Gitea #597 decision. The ack used to be applied locally and reported, while DriverHostActor's + /// Primary gate dropped the route upstream — so the driver never acknowledged, its next transition + /// projected Acked = false, and a client acking against the Secondary saw acked → unacked. An + /// honest refusal replaces an ack that visibly sticks and then reverts. Scripted conditions are + /// unaffected: their engine runs on both nodes, so the Secondary's ack is real. + /// + [Fact] + public async Task A_native_acknowledge_on_a_non_primary_node_is_refused() + { + var ct = TestContext.Current.CancellationToken; + var rig = await StartAsync("CondNativeAckSecondary", encrypted: false, ct); + try + { + rig.Enforce(true, [Key()], ClusterGrant(KeyPrincipal, "acl-key", All)); + rig.MaterialiseNative(); + var current = PreState("Acknowledge", native: true) with { Message = "base" }; + WireSimulatedOwners(rig, native: true, () => current, s => current = s); + var routed = 0; + var driverAck = rig.Nm.NativeAlarmAckRouter!; + rig.Nm.NativeAlarmAckRouter = ack => { Interlocked.Increment(ref routed); driverAck(ack); }; + var primary = false; + rig.Nm.NativeAlarmAckPrimaryGate = () => primary; + + using var session = await rig.ConnectAsync(anonymous: false, ct); + var events = new RecordedEvents(); + var subscription = await RecordedEvents.SubscribeAsync(session, events, ct); + var item = RecordedEvents.AddEventItem(subscription, ObjectIds.Server); + await subscription.ApplyChangesAsync(ct); + + rig.Project(native: true, current); + await RecordedEvents.WaitUntilAsync(() => events.Has(item, "base")); + var condition = ConditionNodeId(session, native: true); + + var refused = await Should.ThrowAsync( + CallAsync(session, condition, "Acknowledge", events.LatestEventId(item, "base"), ct)); + refused.StatusCode.ShouldBe(StatusCodes.BadInvalidState); + routed.ShouldBe(0, "a refused ack reaches no driver"); + rig.Nm.TryGetAlarmCondition(RawAlarmPath, AddressSpaceRealm.Raw)!.AckedState.Id.Value + .ShouldBeFalse("a refused ack applies nothing"); + + // Positive control: the same call on the Primary is accepted, routed and reported. + primary = true; + await CallAsync(session, condition, "Acknowledge", events.LatestEventId(item, "base"), ct); + await rig.DrainProjectionsAsync(); + routed.ShouldBe(1); + + rig.Project(native: true, current = current with { Message = "sentinel" }); + await RecordedEvents.WaitUntilAsync(() => events.Has(item, "sentinel")); + var baseEvents = events.ConditionEvents(item, "base"); + baseEvents.Count.ShouldBe(2, "the base transition plus the Primary's one ack event; the refusal reported nothing"); + baseEvents[1][RecordedEvents.AckedField].Value.ShouldBe(true); + + await subscription.DeleteAsync(true, ct); + } + finally { await rig.DisposeAsync(); } + } + + /// + /// Acknowledging an acknowledged condition WITHOUT a comment reports nothing, and does not flip a scripted + /// condition's Confirmed out of step with its engine. With a comment it reports exactly one event carrying + /// that comment — the comment is a Part 9 change in its own right, as AddComment is (Gitea #597) — and + /// still leaves Acked/Confirmed alone. + /// + /// + /// The SDK accepts a re-ack, writes its comment, and sets Confirmed false again; the engine ignores it. + /// The scripted leg acknowledges and confirms first, so the flip would be visible on the sentinel. Before + /// #597 a commented re-ack wrote the comment to the node but reported nothing, so no subscriber ever saw + /// it. /// [Theory] - [InlineData(false)] - [InlineData(true)] - public async Task A_re_acknowledge_reports_nothing_and_changes_nothing(bool native) + [InlineData(false, null)] + [InlineData(true, null)] + [InlineData(false, "again")] + [InlineData(true, "again")] + public async Task A_re_acknowledge_reports_only_its_comment_and_changes_nothing_else(bool native, string? reAckComment) { var ct = TestContext.Current.CancellationToken; var rig = await StartAsync($"CondReAck{native}", encrypted: false, ct); @@ -347,6 +488,9 @@ public sealed class ConditionMethodEventTests rig.MaterialiseScripted(); var current = PreState("Acknowledge", native) with { Message = "base" }; WireSimulatedOwners(rig, native, () => current, s => current = s); + var routedAcks = 0; + var driverAck = rig.Nm.NativeAlarmAckRouter!; + rig.Nm.NativeAlarmAckRouter = ack => { Interlocked.Increment(ref routedAcks); driverAck(ack); }; using var session = await rig.ConnectAsync(anonymous: false, ct); var events = new RecordedEvents(); @@ -370,13 +514,32 @@ public sealed class ConditionMethodEventTests } await RecordedEvents.WaitUntilAsync(() => events.ConditionEvents(item, "base").Count == expected); - await CallAsync(session, condition, "Acknowledge", events.LatestEventId(item, "base"), ct); + var routedBefore = routedAcks; + await CallAsync(session, condition, "Acknowledge", events.LatestEventId(item, "base"), ct, ackComment: reAckComment); await rig.DrainProjectionsAsync(); rig.Project(native, current = current with { Message = "sentinel" }); await RecordedEvents.WaitUntilAsync(() => events.Has(item, "sentinel")); - events.ConditionEvents(item, "base").Count.ShouldBe(expected, "the re-ack must report nothing"); + var baseEvents = events.ConditionEvents(item, "base"); + if (reAckComment is null) + { + baseEvents.Count.ShouldBe(expected, "an uncommented re-ack must report nothing"); + } + else + { + baseEvents.Count.ShouldBe(expected + 1, "a commented re-ack reports exactly one comment event"); + var commented = baseEvents[^1]; + (commented[RecordedEvents.CommentField].Value as LocalizedText)?.Text.ShouldBe(reAckComment); + commented[RecordedEvents.AckedField].Value.ShouldBe(true); + commented[RecordedEvents.ConfirmedField].Value.ShouldBe(true, "the commented re-ack must not unconfirm the node"); + } + + // A native re-ack is still routed to the driver: the route is fire-and-forget, so a node showing + // Acked does not prove the upstream system acknowledged, and re-acking is the documented retry + // (docs/AlarmTracking.md, "Best-effort during reconnect"). Pinned so it is changed deliberately. + if (native) (routedAcks - routedBefore).ShouldBe(1); + var sentinel = events.ConditionEvents(item, "sentinel"); sentinel.Count.ShouldBe(1); sentinel[0][RecordedEvents.AckedField].Value.ShouldBe(true); @@ -467,11 +630,13 @@ public sealed class ConditionMethodEventTests /// condition's method declarations — the shelve methods included); throws on a bad result. /// private static async Task CallAsync( - ISession session, NodeId condition, string method, byte[]? eventId, CancellationToken ct, double shelvingTimeMs = 60_000) + ISession session, NodeId condition, string method, byte[]? eventId, CancellationToken ct, double shelvingTimeMs = 60_000, + string? ackComment = "ack") { var (methodId, args) = method switch { - "Acknowledge" => (MethodIds.AcknowledgeableConditionType_Acknowledge, new object[] { eventId!, new LocalizedText("ack") }), + "Acknowledge" => (MethodIds.AcknowledgeableConditionType_Acknowledge, + new object[] { eventId!, ackComment is null ? LocalizedText.Null : new LocalizedText(ackComment) }), "Confirm" => (MethodIds.AcknowledgeableConditionType_Confirm, [eventId!, new LocalizedText("confirm")]), "AddComment" => (MethodIds.ConditionType_AddComment, [eventId!, new LocalizedText("note")]), "Enable" => (MethodIds.ConditionType_Enable, []), diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ServerEventRig.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ServerEventRig.cs index 46a90c35..ff8a9cc6 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ServerEventRig.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ServerEventRig.cs @@ -292,6 +292,7 @@ internal sealed class RecordedEvents public const int CommentField = 7; public const int ChangesField = 8; public const int SourceNodeField = 9; + public const int UnshelveTimeField = 10; private readonly object _gate = new(); private readonly List<(uint Handle, VariantCollection Fields)> _events = []; @@ -349,6 +350,8 @@ internal sealed class RecordedEvents filter.AddSelectClause(ObjectTypeIds.ConditionType, BrowseNames.Comment); filter.AddSelectClause(ObjectTypeIds.GeneralModelChangeEventType, BrowseNames.Changes); filter.AddSelectClause(ObjectTypes.BaseEventType, BrowseNames.SourceNode); + filter.SelectClauses.Add(new SimpleAttributeOperand( + ObjectTypeIds.AlarmConditionType, [BrowseNames.ShelvingState, BrowseNames.UnshelveTime])); var item = new MonitoredItem(subscription.DefaultItem) {