fix(r2-04): AddressSpaceApplyOutcome carries RebuildFailed/FailedNodes; SafeRebuild returns bool (01/S-1)

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

@@ -2,7 +2,7 @@
   "planPath": "archreview/plans/R2-04-failure-visibility-plan.md",
   "lastUpdated": "2026-07-12",
   "tasks": [
-    { "id": "T1", "subject": "Extend AddressSpaceApplyOutcome with RebuildFailed/FailedNodes + SafeRebuild returns bool (01/S-1)", "status": "pending", "blockedBy": [] },
+    { "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": "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"] },

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

@@ -146,10 +146,11 @@ public sealed class AddressSpaceApplier
         // covered for free and need no separate surgical pass.
         var renamedFolders = plan.RenamedFolders;
         var rebuilt = false;
+        var rebuildFailed = false;
 
         if (structuralRebuild)
         {
-            SafeRebuild();
+            rebuildFailed = !SafeRebuild();
             rebuilt = true;
         }
         else if (surgicalTagDeltas.Count > 0 || renamedFolders.Count > 0)
@@ -188,12 +189,12 @@ public sealed class AddressSpaceApplier
                     }
                     if (!ok) { allApplied = false; break; }
                 }
-                if (!allApplied) { SafeRebuild(); rebuilt = true; }
+                if (!allApplied) { rebuildFailed = !SafeRebuild(); rebuilt = true; }
             }
             else
             {
                 // Sink lacks the surgical capability ⇒ rebuild (safe default).
-                SafeRebuild();
+                rebuildFailed = !SafeRebuild();
                 rebuilt = true;
             }
         }
@@ -212,7 +213,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);
+        return new AddressSpaceApplyOutcome(removedCount, addedCount, changedCount, rebuilt, rebuildFailed);
     }
 
     /// <summary>
@@ -370,15 +371,22 @@ public sealed class AddressSpaceApplier
                 string.IsNullOrWhiteSpace(tag.HistorianTagname) ? tag.FullName : tag.HistorianTagname)
             : null;
 
-    private void SafeRebuild()
+    /// <summary>Rebuild the sink's address space, swallowing (and Error-logging) any fault.
+    /// Returns <c>true</c> on success, <c>false</c> when the sink threw — the caller threads the
+    /// result into <see cref="AddressSpaceApplyOutcome.RebuildFailed"/> so a broken rebuild is
+    /// visible instead of reported as optimistic success (archreview 01/S-1).</summary>
+    /// <returns><c>true</c> when the rebuild completed; <c>false</c> when the sink threw.</returns>
+    private bool SafeRebuild()
     {
         try
         {
             _sink.RebuildAddressSpace();
+            return true;
         }
         catch (Exception ex)
         {
             _logger.LogError(ex, "AddressSpaceApplier: sink.RebuildAddressSpace threw");
+            return false;
         }
     }
 
@@ -687,9 +695,18 @@ public sealed class AddressSpaceApplier
     }
 }
 
-/// <summary>Summary of one apply pass. Useful for tests + audit-log entries on the deploy path.</summary>
+/// <summary>Summary of one apply pass. Useful for tests + audit-log entries on the deploy path.
+/// <para><see cref="RebuildCalled"/> means a rebuild was ATTEMPTED; <see cref="RebuildFailed"/> means the
+/// attempt threw (the sink's address space is now stale/partial). <see cref="FailedNodes"/> counts
+/// swallowed per-node sink failures in <see cref="AddressSpaceApplier.Apply"/>'s own passes (the removal
+/// alarm-condition writes). The <c>Materialise*</c> passes report their own failed-node tallies via their
+/// <c>int</c> returns — the publish actor sums both channels so a degraded apply is operator-visible
+/// (Error log + <c>otopcua.opcua.apply.failed</c> meter) instead of reported as optimistic success
+/// (archreview 01/S-1). New trailing fields are defaulted so every existing construction compiles.</para></summary>
 public sealed record AddressSpaceApplyOutcome(
     int RemovedNodes,
     int AddedNodes,
     int ChangedNodes,
-    bool RebuildCalled);
+    bool RebuildCalled,
+    bool RebuildFailed = false,
+    int FailedNodes = 0);

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

@@ -0,0 +1,98 @@
+using Microsoft.Extensions.Logging.Abstractions;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Commons.OpcUa;
+
+namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests;
+
+/// <summary>
+///     Guards the failure-visibility surface added for archreview 01/S-1: the applier must report a
+///     failed rebuild (<see cref="AddressSpaceApplyOutcome.RebuildFailed"/>) and tally swallowed per-node
+///     sink failures (<see cref="AddressSpaceApplyOutcome.FailedNodes"/> for Apply's own passes; the
+///     <c>Materialise*</c> passes return their own failed-node counts) instead of reporting optimistic
+///     success while the running address space is stale or partial.
+/// </summary>
+public sealed class AddressSpaceApplierFailureSurfaceTests
+{
+    // ---------------- T1: RebuildFailed ----------------
+
+    /// <summary>A rebuild whose sink call throws is reported as attempted-but-failed, not optimistic success.</summary>
+    [Fact]
+    public void Apply_WhenRebuildThrows_ReportsRebuildFailed()
+    {
+        var sink = new ConfigurableThrowingSink { ThrowOnRebuild = true };
+        var applier = new AddressSpaceApplier(sink, NullLogger<AddressSpaceApplier>.Instance);
+
+        var outcome = applier.Apply(AddedEquipmentPlan("new"));
+
+        outcome.RebuildCalled.ShouldBeTrue();
+        outcome.RebuildFailed.ShouldBeTrue();
+    }
+
+    /// <summary>A clean apply reports no failure on either channel — the regression guard for the new fields.</summary>
+    [Fact]
+    public void Apply_HappyPath_ReportsNoFailure()
+    {
+        var sink = new ConfigurableThrowingSink();
+        var applier = new AddressSpaceApplier(sink, NullLogger<AddressSpaceApplier>.Instance);
+
+        var outcome = applier.Apply(AddedEquipmentPlan("new"));
+
+        outcome.RebuildCalled.ShouldBeTrue();
+        outcome.RebuildFailed.ShouldBeFalse();
+        outcome.FailedNodes.ShouldBe(0);
+    }
+
+    // ---------------- fixtures ----------------
+
+    private static AddressSpacePlan AddedEquipmentPlan(string id) => new(
+        AddedEquipment: new[] { new EquipmentNode(id, id, "line-1") },
+        RemovedEquipment: Array.Empty<EquipmentNode>(),
+        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
+    {
+        public bool ThrowOnRebuild { get; init; }
+        public bool ThrowOnEnsureVariable { get; init; }
+        public bool ThrowOnEnsureFolder { get; init; }
+        public bool ThrowOnAlarmWrite { get; init; }
+        public bool ThrowOnMaterialiseAlarm { get; init; }
+
+        public void WriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime sourceTimestampUtc) { }
+
+        public void WriteAlarmCondition(string alarmNodeId, AlarmConditionSnapshot state, DateTime sourceTimestampUtc)
+        {
+            if (ThrowOnAlarmWrite) throw new InvalidOperationException("simulated WriteAlarmCondition fault");
+        }
+
+        public void MaterialiseAlarmCondition(string alarmNodeId, string equipmentNodeId, string displayName, string alarmType, int severity, bool isNative = false)
+        {
+            if (ThrowOnMaterialiseAlarm) throw new InvalidOperationException("simulated MaterialiseAlarmCondition fault");
+        }
+
+        public void EnsureFolder(string folderNodeId, string? parentNodeId, string displayName)
+        {
+            if (ThrowOnEnsureFolder) throw new InvalidOperationException("simulated EnsureFolder fault");
+        }
+
+        public void EnsureVariable(string variableNodeId, string? parentFolderNodeId, string displayName, string dataType, bool writable, string? historianTagname = null, bool isArray = false, uint? arrayLength = null)
+        {
+            if (ThrowOnEnsureVariable) throw new InvalidOperationException("simulated EnsureVariable fault");
+        }
+
+        public void RebuildAddressSpace()
+        {
+            if (ThrowOnRebuild) throw new InvalidOperationException("simulated RebuildAddressSpace fault");
+        }
+
+        public void RaiseNodesAddedModelChange(string affectedNodeId) { }
+    }
+}