fix(r2-04): Safe* helpers report failure; Apply tallies removal-pass failures into FailedNodes

archreview/plans/R2-04-failure-visibility-plan.md.tasks.json

@@ -3,7 +3,7 @@
   "lastUpdated": "2026-07-12",
   "tasks": [
     { "id": "T1", "subject": "Extend AddressSpaceApplyOutcome with RebuildFailed/FailedNodes + SafeRebuild returns bool (01/S-1)", "status": "completed", "blockedBy": [] },
-    { "id": "T2", "subject": "Safe* helpers return bool; Apply tallies removal-pass failures into FailedNodes", "status": "pending", "blockedBy": ["T1"] },
+    { "id": "T2", "subject": "Safe* helpers return bool; Apply tallies removal-pass failures into FailedNodes", "status": "completed", "blockedBy": ["T1"] },
     { "id": "T3", "subject": "Materialise* passes return swallowed-failure counts (void -> int, five methods)", "status": "pending", "blockedBy": ["T2"] },
     { "id": "T4", "subject": "OpcUaApplyFailed counter + OpcUaPublishActor Error/meter surfacing of degraded applies", "status": "pending", "blockedBy": ["T3"] },
     { "id": "T5", "subject": "Galaxy internal IMxWriteOps seam under GatewayGalaxyDataWriter (sealed SDK session untestable) (06/S-1)", "status": "pending", "blockedBy": [] },

src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs

@@ -82,14 +82,17 @@ public sealed class AddressSpaceApplier
 
         var ts = DateTime.UtcNow;
         var removedCount = 0;
+        // Swallowed removal-pass condition-write failures are tallied here (not lost to a per-node Warning)
+        // so a degraded removal is operator-visible via AddressSpaceApplyOutcome.FailedNodes (archreview 01/S-1).
+        var failedNodes = 0;
         foreach (var eq in plan.RemovedEquipment)
         {
-            SafeWriteAlarmCondition(eq.EquipmentId, RemovedConditionState, ts);
+            if (!SafeWriteAlarmCondition(eq.EquipmentId, RemovedConditionState, ts)) failedNodes++;
             removedCount++;
         }
         foreach (var alarm in plan.RemovedAlarms)
         {
-            SafeWriteAlarmCondition(alarm.ScriptedAlarmId, RemovedConditionState, ts);
+            if (!SafeWriteAlarmCondition(alarm.ScriptedAlarmId, RemovedConditionState, ts)) failedNodes++;
             removedCount++;
         }
         // Removed equipment tags / VirtualTags are plain variable nodes (no Part 9 condition to write
@@ -213,7 +216,7 @@ public sealed class AddressSpaceApplier
         // currently-historized set. Same non-blocking + throw-safe discipline as the provisioning hook.
         FeedHistorizedRefs(plan);
 
-        return new AddressSpaceApplyOutcome(removedCount, addedCount, changedCount, rebuilt, rebuildFailed);
+        return new AddressSpaceApplyOutcome(removedCount, addedCount, changedCount, rebuilt, rebuildFailed, failedNodes);
     }
 
     /// <summary>
@@ -617,16 +620,24 @@ public sealed class AddressSpaceApplier
                 .Select(a => a.EquipmentId).Distinct(StringComparer.Ordinal).Count());
     }
 
-    private void SafeEnsureFolder(string nodeId, string? parentNodeId, string displayName)
+    /// <summary>Ensure a folder node, swallowing (and Warning-logging) any sink fault. Returns <c>true</c>
+    /// on success, <c>false</c> when the sink threw — callers tally the false into their pass's failed-node
+    /// count so a swallowed materialise failure is operator-visible (archreview 01/S-1).</summary>
+    /// <returns><c>true</c> when the folder was ensured; <c>false</c> when the sink threw.</returns>
+    private bool SafeEnsureFolder(string nodeId, string? parentNodeId, string displayName)
     {
-        try { _sink.EnsureFolder(nodeId, parentNodeId, displayName); }
+        try { _sink.EnsureFolder(nodeId, parentNodeId, displayName); return true; }
-        catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: EnsureFolder threw for {Node}", nodeId); }
+        catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: EnsureFolder threw for {Node}", nodeId); return false; }
     }
 
-    private void SafeEnsureVariable(string nodeId, string? parentNodeId, string displayName, string dataType, bool writable, string? historianTagname = null, bool isArray = false, uint? arrayLength = null)
+    /// <summary>Ensure a variable node, swallowing (and Warning-logging) any sink fault. Returns <c>true</c>
+    /// on success, <c>false</c> when the sink threw — callers tally the false into their pass's failed-node
+    /// count (archreview 01/S-1).</summary>
+    /// <returns><c>true</c> when the variable was ensured; <c>false</c> when the sink threw.</returns>
+    private bool SafeEnsureVariable(string nodeId, string? parentNodeId, string displayName, string dataType, bool writable, string? historianTagname = null, bool isArray = false, uint? arrayLength = null)
     {
-        try { _sink.EnsureVariable(nodeId, parentNodeId, displayName, dataType, writable, historianTagname, isArray, arrayLength); }
+        try { _sink.EnsureVariable(nodeId, parentNodeId, displayName, dataType, writable, historianTagname, isArray, arrayLength); return true; }
-        catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: EnsureVariable threw for {Node}", nodeId); }
+        catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: EnsureVariable threw for {Node}", nodeId); return false; }
     }
 
     // A VirtualTag's materialised OPC UA node (MaterialiseEquipmentVirtualTags) is derived ONLY from
@@ -682,16 +693,24 @@ public sealed class AddressSpaceApplier
         Severity: 0,
         Message: string.Empty);
 
-    private void SafeWriteAlarmCondition(string nodeId, AlarmConditionSnapshot state, DateTime ts)
+    /// <summary>Write an alarm-condition snapshot, swallowing (and Warning-logging) any sink fault.
+    /// Returns <c>true</c> on success, <c>false</c> when the sink threw — the removal pass tallies the
+    /// false into <see cref="AddressSpaceApplyOutcome.FailedNodes"/> (archreview 01/S-1).</summary>
+    /// <returns><c>true</c> when the write landed; <c>false</c> when the sink threw.</returns>
+    private bool SafeWriteAlarmCondition(string nodeId, AlarmConditionSnapshot state, DateTime ts)
     {
-        try { _sink.WriteAlarmCondition(nodeId, state, ts); }
+        try { _sink.WriteAlarmCondition(nodeId, state, ts); return true; }
-        catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: WriteAlarmCondition threw for {Node}", nodeId); }
+        catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: WriteAlarmCondition threw for {Node}", nodeId); return false; }
     }
 
-    private void SafeMaterialiseAlarmCondition(string alarmNodeId, string equipmentNodeId, string displayName, string alarmType, int severity, bool isNative)
+    /// <summary>Materialise an alarm-condition node, swallowing (and Warning-logging) any sink fault.
+    /// Returns <c>true</c> on success, <c>false</c> when the sink threw — callers tally the false into
+    /// their pass's failed-node count (archreview 01/S-1).</summary>
+    /// <returns><c>true</c> when the condition was materialised; <c>false</c> when the sink threw.</returns>
+    private bool SafeMaterialiseAlarmCondition(string alarmNodeId, string equipmentNodeId, string displayName, string alarmType, int severity, bool isNative)
     {
-        try { _sink.MaterialiseAlarmCondition(alarmNodeId, equipmentNodeId, displayName, alarmType, severity, isNative); }
+        try { _sink.MaterialiseAlarmCondition(alarmNodeId, equipmentNodeId, displayName, alarmType, severity, isNative); return true; }
-        catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: MaterialiseAlarmCondition threw for {Node}", alarmNodeId); }
+        catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: MaterialiseAlarmCondition threw for {Node}", alarmNodeId); return false; }
     }
 }
 

tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierFailureSurfaceTests.cs

@@ -43,6 +43,21 @@ public sealed class AddressSpaceApplierFailureSurfaceTests
         outcome.FailedNodes.ShouldBe(0);
     }
 
+    // ---------------- T2: FailedNodes (removal-pass tally) ----------------
+
+    /// <summary>A removal-pass alarm-condition write that throws is counted into FailedNodes (swallowed today).</summary>
+    [Fact]
+    public void Apply_WhenRemovalConditionWriteThrows_CountsFailedNodes()
+    {
+        var sink = new ConfigurableThrowingSink { ThrowOnAlarmWrite = true };
+        var applier = new AddressSpaceApplier(sink, NullLogger<AddressSpaceApplier>.Instance);
+
+        var outcome = applier.Apply(EquipmentRemovalPlan("eq-1"));
+
+        outcome.RemovedNodes.ShouldBe(1);
+        outcome.FailedNodes.ShouldBe(1);
+    }
+
     // ---------------- fixtures ----------------
 
     private static AddressSpacePlan AddedEquipmentPlan(string id) => new(
@@ -56,6 +71,17 @@ public sealed class AddressSpaceApplierFailureSurfaceTests
         RemovedAlarms: Array.Empty<ScriptedAlarmPlan>(),
         ChangedAlarms: Array.Empty<AddressSpacePlan.AlarmDelta>());
 
+    private static AddressSpacePlan EquipmentRemovalPlan(params string[] ids) => new(
+        AddedEquipment: Array.Empty<EquipmentNode>(),
+        RemovedEquipment: ids.Select(id => new EquipmentNode(id, id, "line-1")).ToArray(),
+        ChangedEquipment: Array.Empty<AddressSpacePlan.EquipmentDelta>(),
+        AddedDrivers: Array.Empty<DriverInstancePlan>(),
+        RemovedDrivers: Array.Empty<DriverInstancePlan>(),
+        ChangedDrivers: Array.Empty<AddressSpacePlan.DriverDelta>(),
+        AddedAlarms: Array.Empty<ScriptedAlarmPlan>(),
+        RemovedAlarms: Array.Empty<ScriptedAlarmPlan>(),
+        ChangedAlarms: Array.Empty<AddressSpacePlan.AlarmDelta>());
+
     /// <summary>An <see cref="IOpcUaAddressSpaceSink"/> whose every sink call can be independently made to
     /// throw, so tests can drive each Safe* helper's catch branch. Non-throwing calls are no-ops.</summary>
     private sealed class ConfigurableThrowingSink : IOpcUaAddressSpaceSink