diff --git a/archreview/plans/R2-04-failure-visibility-plan.md.tasks.json b/archreview/plans/R2-04-failure-visibility-plan.md.tasks.json index 1d922cbe..f0274d82 100644 --- a/archreview/plans/R2-04-failure-visibility-plan.md.tasks.json +++ b/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"] }, diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs index 3fba3d99..763ec95e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs +++ b/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); } /// @@ -370,15 +371,22 @@ public sealed class AddressSpaceApplier string.IsNullOrWhiteSpace(tag.HistorianTagname) ? tag.FullName : tag.HistorianTagname) : null; - private void SafeRebuild() + /// Rebuild the sink's address space, swallowing (and Error-logging) any fault. + /// Returns true on success, false when the sink threw — the caller threads the + /// result into so a broken rebuild is + /// visible instead of reported as optimistic success (archreview 01/S-1). + /// true when the rebuild completed; false when the sink threw. + 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 of one apply pass. Useful for tests + audit-log entries on the deploy path. +/// Summary of one apply pass. Useful for tests + audit-log entries on the deploy path. +/// means a rebuild was ATTEMPTED; means the +/// attempt threw (the sink's address space is now stale/partial). counts +/// swallowed per-node sink failures in 's own passes (the removal +/// alarm-condition writes). The Materialise* passes report their own failed-node tallies via their +/// int returns — the publish actor sums both channels so a degraded apply is operator-visible +/// (Error log + otopcua.opcua.apply.failed meter) instead of reported as optimistic success +/// (archreview 01/S-1). New trailing fields are defaulted so every existing construction compiles. public sealed record AddressSpaceApplyOutcome( int RemovedNodes, int AddedNodes, int ChangedNodes, - bool RebuildCalled); + bool RebuildCalled, + bool RebuildFailed = false, + int FailedNodes = 0); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierFailureSurfaceTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierFailureSurfaceTests.cs new file mode 100644 index 00000000..d46cc7fb --- /dev/null +++ b/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; + +/// +/// Guards the failure-visibility surface added for archreview 01/S-1: the applier must report a +/// failed rebuild () and tally swallowed per-node +/// sink failures ( for Apply's own passes; the +/// Materialise* passes return their own failed-node counts) instead of reporting optimistic +/// success while the running address space is stale or partial. +/// +public sealed class AddressSpaceApplierFailureSurfaceTests +{ + // ---------------- T1: RebuildFailed ---------------- + + /// A rebuild whose sink call throws is reported as attempted-but-failed, not optimistic success. + [Fact] + public void Apply_WhenRebuildThrows_ReportsRebuildFailed() + { + var sink = new ConfigurableThrowingSink { ThrowOnRebuild = true }; + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + + var outcome = applier.Apply(AddedEquipmentPlan("new")); + + outcome.RebuildCalled.ShouldBeTrue(); + outcome.RebuildFailed.ShouldBeTrue(); + } + + /// A clean apply reports no failure on either channel — the regression guard for the new fields. + [Fact] + public void Apply_HappyPath_ReportsNoFailure() + { + var sink = new ConfigurableThrowingSink(); + var applier = new AddressSpaceApplier(sink, NullLogger.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(), + ChangedEquipment: Array.Empty(), + AddedDrivers: Array.Empty(), + RemovedDrivers: Array.Empty(), + ChangedDrivers: Array.Empty(), + AddedAlarms: Array.Empty(), + RemovedAlarms: Array.Empty(), + ChangedAlarms: Array.Empty()); + + /// An 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. + 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) { } + } +}