diff --git a/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json b/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json index 6e179a63..32d39733 100644 --- a/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json +++ b/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json @@ -96,7 +96,7 @@ { "id": "T11", "subject": "Phase 2: applier PureRemove arm \u2014 terminal Bad/RemovedConditionState writes, equipment subsumption, false-to-rebuild ratchet (high-risk)", - "status": "pending", + "status": "completed", "blockedBy": [ "T10" ] diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs index 677ffb15..61a963a7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs @@ -128,8 +128,12 @@ public sealed class AddressSpaceApplier // every client subscription server-wide survives; coincident surgical tag deltas + folder // renames are applied IN PLACE via ISurgicalAddressSpaceSink below (any false/throw there falls // back to a full rebuild — the F10b contract). - // • PureRemove / AddRemoveMix ⇒ full rebuild UNTIL their phases ship (R2-07 T11 / T13); mapping - // them here keeps each phase independently shippable with today's behaviour as the floor. + // • PureRemove ⇒ NO full rebuild. The removed nodes are torn down IN PLACE, scoped to the affected + // subtree, via the ISurgicalAddressSpaceSink remove members below (each preceded by a terminal + // Bad / RemovedConditionState write so in-flight MonitoredItems observe the removal); subscribers + // of OTHER nodes are untouched. Any false/throw from a remove ⇒ full-rebuild fallback (ratchet). + // • AddRemoveMix ⇒ full rebuild UNTIL Phase 3 ships (R2-07 T13); mapping it here keeps each phase + // independently shippable with today's behaviour as the floor. // • Rebuild ⇒ the default-closed safety valve for any node-affecting change (ChangedEquipment / // ChangedAlarms / non-surgical ChangedEquipmentTags / node-relevant ChangedEquipmentVirtualTags) // — the classifier's rule 2, which also catches any future plan field that makes a changed @@ -138,7 +142,6 @@ public sealed class AddressSpaceApplier // a driver-only plan AttributeOnly, so it never rebuilds here. var kind = AddressSpaceChangeClassifier.Classify(plan); var mustRebuild = kind is AddressSpaceChangeKind.Rebuild - or AddressSpaceChangeKind.PureRemove or AddressSpaceChangeKind.AddRemoveMix; var surgicalTagDeltas = plan.ChangedEquipmentTags.Where(AddressSpaceChangeClassifier.TagDeltaIsSurgicalEligible).ToList(); @@ -155,7 +158,7 @@ public sealed class AddressSpaceApplier rebuildFailed = !SafeRebuild(); rebuilt = true; } - else if (surgicalTagDeltas.Count > 0 || renamedFolders.Count > 0) + else if (surgicalTagDeltas.Count > 0 || renamedFolders.Count > 0 || kind == AddressSpaceChangeKind.PureRemove) { if (_sink is ISurgicalAddressSpaceSink surgical) { @@ -191,6 +194,13 @@ public sealed class AddressSpaceApplier } if (!ok) { allApplied = false; break; } } + // R2-07 Phase 2 — PureRemove scoped teardown (runs after any coincident surgical + // attribute updates / renames succeeded). Terminal Bad / RemovedConditionState writes then + // in-place removes; any false/throw flips allApplied → the rebuild ratchet below. + if (allApplied && kind == AddressSpaceChangeKind.PureRemove) + { + allApplied = ApplyPureRemove(surgical, plan, ts, ref failedNodes); + } if (!allApplied) { rebuildFailed = !SafeRebuild(); rebuilt = true; } } else @@ -218,6 +228,108 @@ public sealed class AddressSpaceApplier return new AddressSpaceApplyOutcome(removedCount, addedCount, changedCount, rebuilt, rebuildFailed, failedNodes); } + /// + /// R2-07 Phase 2 — apply a PureRemove plan by tearing down ONLY the affected nodes IN PLACE (no full + /// rebuild). Removed equipment "own" their child tags/vtags/alarms, so any removed child whose + /// EquipmentId is itself in is SUBSUMED by the + /// subtree removal and skipped individually. For surviving-equipment removals: a removed value + /// variable gets a terminal Bad WriteValue then ; + /// a removed alarm-bearing tag (the pre-R2-07 today-gap: it got no terminal condition write) and a + /// removed scripted alarm get a terminal write then + /// ; each removed equipment gets one + /// . The terminal write is what an + /// in-flight MonitoredItem observes (a final Bad); after the node is gone, re-subscription gets + /// BadNodeIdUnknown. Returns false on the FIRST remove that reports the node unknown / throws + /// (the caller's one-way rebuild ratchet takes over), leaving no further surgical work attempted. + /// + /// The surgical sink to route removes through. + /// The PureRemove plan. + /// The timestamp for the terminal writes. + /// Accumulator for swallowed terminal-write failures (archreview 01/S-1). + /// True when every removal succeeded; false on the first failure (caller rebuilds). + private bool ApplyPureRemove(ISurgicalAddressSpaceSink surgical, AddressSpacePlan plan, DateTime ts, ref int failedNodes) + { + var removedEquipmentIds = new HashSet( + plan.RemovedEquipment.Select(e => e.EquipmentId), StringComparer.Ordinal); + bool NotSubsumed(string equipmentId) => !removedEquipmentIds.Contains(equipmentId); + + // Removed equipment tags — value variables OR alarm-bearing conditions — for SURVIVING equipment. + foreach (var tag in plan.RemovedEquipmentTags.Where(t => NotSubsumed(t.EquipmentId))) + { + var nodeId = EquipmentNodeIds.Variable(tag.EquipmentId, tag.FolderPath, tag.Name); + if (tag.Alarm is not null) + { + // Alarm-bearing tag → a Part 9 condition node. Terminal RemovedConditionState (today-gap + // closed), then remove the condition in place. + if (!SafeWriteAlarmCondition(nodeId, RemovedConditionState, ts)) failedNodes++; + if (!SafeRemoveAlarmCondition(surgical, nodeId)) return false; + } + else + { + // Plain value variable → terminal Bad so an in-flight MonitoredItem sees the removal. + SafeWriteValue(nodeId, null, OpcUaQuality.Bad, ts); + if (!SafeRemoveVariable(surgical, nodeId)) return false; + } + } + + // Removed VirtualTags (always plain value variables) for surviving equipment. + foreach (var v in plan.RemovedEquipmentVirtualTags.Where(t => NotSubsumed(t.EquipmentId))) + { + var nodeId = EquipmentNodeIds.Variable(v.EquipmentId, v.FolderPath, v.Name); + SafeWriteValue(nodeId, null, OpcUaQuality.Bad, ts); + if (!SafeRemoveVariable(surgical, nodeId)) return false; + } + + // Removed scripted alarms for surviving equipment — the terminal RemovedConditionState was already + // written by the top-of-Apply removal block; here we tear the condition node down in place. + foreach (var alarm in plan.RemovedAlarms.Where(a => NotSubsumed(a.EquipmentId))) + { + if (!SafeRemoveAlarmCondition(surgical, alarm.ScriptedAlarmId)) 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)) return false; + } + + return true; + } + + /// Remove a value-variable node in place, treating a throw as a false (⇒ rebuild fallback). + /// true when the node was removed; false when unknown or the sink threw. + private bool SafeRemoveVariable(ISurgicalAddressSpaceSink surgical, string nodeId) + { + try { return surgical.RemoveVariableNode(nodeId); } + catch (Exception ex) { _logger.LogError(ex, "AddressSpaceApplier: surgical RemoveVariableNode threw for {Node}", nodeId); return false; } + } + + /// Remove an alarm-condition node in place, treating a throw as a false (⇒ rebuild fallback). + /// true when the node was removed; false when unknown or the sink threw. + private bool SafeRemoveAlarmCondition(ISurgicalAddressSpaceSink surgical, string nodeId) + { + try { return surgical.RemoveAlarmConditionNode(nodeId); } + catch (Exception ex) { _logger.LogError(ex, "AddressSpaceApplier: surgical RemoveAlarmConditionNode threw for {Node}", nodeId); return false; } + } + + /// Remove an equipment subtree in place, treating a throw as a false (⇒ rebuild fallback). + /// true when the subtree was removed; false when unknown or the sink threw. + private bool SafeRemoveEquipmentSubtree(ISurgicalAddressSpaceSink surgical, string nodeId) + { + try { return surgical.RemoveEquipmentSubtree(nodeId); } + catch (Exception ex) { _logger.LogError(ex, "AddressSpaceApplier: surgical RemoveEquipmentSubtree threw for {Node}", nodeId); return false; } + } + + /// Write a value, swallowing (and Warning-logging) any sink fault — used for the terminal Bad + /// on a removed variable so an in-flight MonitoredItem observes the removal. + /// true when the write landed; false when the sink threw. + private bool SafeWriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime ts) + { + try { _sink.WriteValue(nodeId, value, quality, ts); return true; } + catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: WriteValue threw for {Node}", nodeId); return false; } + } + /// /// Compute the deduplicated, deterministically-ordered set of affected PARENT node ids to announce /// (Part 3 NodeAdded) after a pure-add apply's Materialise passes have created the new nodes. 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 86ac2f37..72021ec8 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs @@ -25,9 +25,12 @@ public sealed class AddressSpaceApplierTests sink.AlarmWrites.ShouldBeEmpty(); } - /// Verifies that removed equipment writes inactive alarm state and triggers rebuild. + /// R2-07 T11 — removed equipment is a PureRemove: the applier writes each id's terminal + /// "no-event" condition state (top-of-Apply block) then tears down each equipment SUBTREE in place via + /// RemoveEquipmentSubtree — NO full rebuild (other clients' subscriptions survive). (Supersedes the + /// pre-R2-07 "removed equipment ⇒ rebuild" pin.) [Fact] - public void Removed_equipment_writes_inactive_alarm_state_per_id_and_triggers_rebuild() + public void Removed_equipment_writes_terminal_state_and_removes_subtree_without_rebuild() { var sink = new RecordingSink(); var applier = new AddressSpaceApplier(sink, NullLogger.Instance); @@ -36,11 +39,13 @@ public sealed class AddressSpaceApplierTests var outcome = applier.Apply(plan); outcome.RemovedNodes.ShouldBe(2); - outcome.RebuildCalled.ShouldBeTrue(); + outcome.RebuildCalled.ShouldBeFalse(); + sink.RebuildCalls.ShouldBe(0); + // Terminal "no-event" condition state written per id (inactive + acked + confirmed). sink.AlarmWrites.Select(a => a.NodeId).OrderBy(x => x).ShouldBe(new[] { "eq-1", "eq-2" }); - // Removed nodes are reset to the "no-event" state: inactive + acked + confirmed + enabled. sink.AlarmWrites.All(a => !a.State.Active && a.State.Acknowledged && a.State.Confirmed).ShouldBeTrue(); - sink.RebuildCalls.ShouldBe(1); + // Each equipment torn down as a subtree. + sink.RemoveCalls.OrderBy(x => x.NodeId).ShouldBe(new[] { ("equipment", "eq-1"), ("equipment", "eq-2") }); } /// R2-07 T2 — added equipment is a PureAdd: the applier SKIPS the rebuild (the idempotent @@ -789,10 +794,11 @@ public sealed class AddressSpaceApplierTests sink.SurgicalCalls.ShouldHaveSingleItem(); // the surgical update was attempted first } - /// R2-07 T2 phase pin — a remove-only plan still triggers a full rebuild until Phase 2 - /// (PureRemove) ships. Flipped in T11. + /// R2-07 T11 — a remove-one-tag plan is a PureRemove: the applier writes a terminal Bad to the + /// removed variable then removes it in place via RemoveVariableNode — NO rebuild. (Supersedes the T2 + /// phase pin.) [Fact] - public void Removed_equipment_tags_only_still_rebuild_phase_pin() + public void Removed_equipment_tag_writes_terminal_bad_and_removes_variable_without_rebuild() { var sink = new RecordingSink(); var applier = new AddressSpaceApplier(sink, NullLogger.Instance); @@ -807,6 +813,116 @@ public sealed class AddressSpaceApplierTests var outcome = applier.Apply(plan); + outcome.RebuildCalled.ShouldBeFalse(); + sink.RebuildCalls.ShouldBe(0); + var nodeId = EquipmentNodeIds.Variable("eq-1", "", "Speed"); + // Terminal Bad written to the removed variable BEFORE the removal. + sink.ValueWrites.ShouldContain((nodeId, OpcUaQuality.Bad)); + sink.RemoveCalls.ShouldHaveSingleItem().ShouldBe(("var", nodeId)); + } + + /// R2-07 T11 — a removed alarm-bearing tag writes the terminal RemovedConditionState (closing + /// the pre-R2-07 today-gap where a removed alarm tag got no condition write) then removes the CONDITION + /// node in place (not a value variable). + [Fact] + public void Removed_alarm_bearing_tag_writes_removed_condition_state_and_removes_condition() + { + var sink = new RecordingSink(); + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + + var plan = EmptyPlan with + { + RemovedEquipmentTags = new[] + { + new EquipmentTagPlan("tag-alm", "eq-1", "drv", FolderPath: "", Name: "OverTemp", DataType: "Boolean", FullName: "00001", Writable: false, + Alarm: new EquipmentTagAlarmInfo("OffNormalAlarm", 700)), + }, + }; + + var outcome = applier.Apply(plan); + + outcome.RebuildCalled.ShouldBeFalse(); + var nodeId = EquipmentNodeIds.Variable("eq-1", "", "OverTemp"); + // Terminal RemovedConditionState (inactive/acked/confirmed) written to the condition node. + var write = sink.AlarmWrites.ShouldHaveSingleItem(); + write.NodeId.ShouldBe(nodeId); + (!write.State.Active && write.State.Acknowledged && write.State.Confirmed).ShouldBeTrue(); + // Removed as a condition node, not a value variable. + sink.RemoveCalls.ShouldHaveSingleItem().ShouldBe(("alarm", nodeId)); + sink.ValueWrites.ShouldNotContain(w => w.NodeId == nodeId); + } + + /// R2-07 T11 — a removed child (tag/vtag/alarm) whose equipment is ALSO removed is SUBSUMED by + /// the equipment-subtree removal: only RemoveEquipmentSubtree fires, no individual child remove. + [Fact] + public void Removed_child_under_removed_equipment_is_subsumed_by_subtree_removal() + { + var sink = new RecordingSink(); + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + + var plan = EmptyPlan with + { + RemovedEquipment = new[] { new EquipmentNode("eq-1", "One", "line-1") }, + RemovedEquipmentTags = new[] + { + new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40001", Writable: false, Alarm: null), + }, + RemovedEquipmentVirtualTags = new[] + { + new EquipmentVirtualTagPlan("vt-1", "eq-1", FolderPath: "", Name: "Eff", DataType: "Float", Expression: "a", DependencyRefs: new[] { "a" }), + }, + }; + + var outcome = applier.Apply(plan); + + outcome.RebuildCalled.ShouldBeFalse(); + // ONLY the subtree removal fires — no individual var removal for the subsumed child tag/vtag. + sink.RemoveCalls.ShouldHaveSingleItem().ShouldBe(("equipment", "eq-1")); + } + + /// R2-07 T11 — if a surgical remove reports the node unknown (RemoveReturns=false), the applier + /// falls back to a full rebuild (the one-way ratchet) and attempts no further surgical removes. + [Fact] + public void Pure_remove_with_remove_returns_false_falls_back_to_rebuild() + { + var sink = new RecordingSink { RemoveReturns = false }; + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + + var plan = EmptyPlan with + { + RemovedEquipmentTags = new[] + { + new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "A", DataType: "Float", FullName: "1", Writable: false, Alarm: null), + new EquipmentTagPlan("tag-2", "eq-1", "drv", FolderPath: "", Name: "B", DataType: "Float", FullName: "2", Writable: false, Alarm: null), + }, + }; + + var outcome = applier.Apply(plan); + + outcome.RebuildCalled.ShouldBeTrue(); + sink.RebuildCalls.ShouldBe(1); + // Ratchet: stopped at the FIRST failed remove — did not attempt the second. + sink.RemoveCalls.Count.ShouldBe(1); + } + + /// R2-07 T11 — a PureRemove on a sink lacking the surgical capability falls back to a full + /// rebuild (safe default), same as the F10b attribute path. + [Fact] + public void Pure_remove_on_non_surgical_sink_rebuilds() + { + var sink = new PlainRecordingSink(); + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + + var plan = EmptyPlan with + { + RemovedEquipmentTags = new[] + { + new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "A", DataType: "Float", FullName: "1", Writable: false, Alarm: null), + }, + }; + + var outcome = applier.Apply(plan); + outcome.RebuildCalled.ShouldBeTrue(); sink.RebuildCalls.ShouldBe(1); } @@ -1276,9 +1392,14 @@ public sealed class AddressSpaceApplierTests var outcome = applier.Apply(plan); - outcome.RebuildCalled.ShouldBeTrue(); - sink.RebuildCalls.ShouldBe(1); - outcome.RemovedNodes.ShouldBe(2); // both removals counted (was 0 before the fix) + // R2-07 T11 — a removed-tag + removed-vtag plan is a PureRemove: both variables are torn down in + // place (RemoveVariableNode ×2), NO full rebuild; both removals are still counted. + outcome.RebuildCalled.ShouldBeFalse(); + sink.RebuildCalls.ShouldBe(0); + outcome.RemovedNodes.ShouldBe(2); // both removals counted + sink.RemoveCalls.Count.ShouldBe(2); + sink.RemoveCalls.ShouldContain(("var", EquipmentNodeIds.Variable("eq-1", "", "Speed"))); + sink.RemoveCalls.ShouldContain(("var", EquipmentNodeIds.Variable("eq-1", "", "Efficiency"))); } // ----- F10b: surgical in-place tag-attribute writes (Writable / IsHistorized / HistorianTagname) ----- @@ -2072,12 +2193,18 @@ public sealed class AddressSpaceApplierTests /// Gets the list of recorded alarm-condition materialise calls. public List<(string AlarmNodeId, string EquipmentNodeId, string DisplayName, string AlarmType, int Severity, bool IsNative)> AlarmConditionCalls => AlarmConditionQueue.ToList(); - /// Records a value write (no-op in this recording sink). + /// Gets the queue of value writes (NodeId, quality) — used to assert the PureRemove terminal Bad. + public ConcurrentQueue<(string NodeId, OpcUaQuality Quality)> ValueWriteQueue { get; } = new(); + /// Gets the list of recorded value writes. + public List<(string NodeId, OpcUaQuality Quality)> ValueWrites => ValueWriteQueue.ToList(); + + /// Records a value write (NodeId + quality). /// The node ID. /// The value to write. /// The OPC UA quality. /// The source timestamp in UTC. - public void WriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime sourceTimestampUtc) { } + public void WriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime sourceTimestampUtc) + => ValueWriteQueue.Enqueue((nodeId, quality)); /// Records an alarm condition write call. /// The alarm node ID. /// The full condition state snapshot.