From e37abf36a0a699abc024509813bd177170a659a6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:41:19 -0400 Subject: [PATCH] fix(r2-04): Materialise* passes return swallowed-failure tallies (void -> int) --- ...2-04-failure-visibility-plan.md.tasks.json | 2 +- .../AddressSpaceApplier.cs | 85 ++++++++++++------- .../AddressSpaceApplierFailureSurfaceTests.cs | 61 +++++++++++++ 3 files changed, 116 insertions(+), 32 deletions(-) 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 3e79ca98..b9e419f2 100644 --- a/archreview/plans/R2-04-failure-visibility-plan.md.tasks.json +++ b/archreview/plans/R2-04-failure-visibility-plan.md.tasks.json @@ -4,7 +4,7 @@ "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": "completed", "blockedBy": ["T1"] }, - { "id": "T3", "subject": "Materialise* passes return swallowed-failure counts (void -> int, five methods)", "status": "pending", "blockedBy": ["T2"] }, + { "id": "T3", "subject": "Materialise* passes return swallowed-failure counts (void -> int, five methods)", "status": "completed", "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": [] }, { "id": "T6", "subject": "Galaxy fail-closed write on AdviseSupervisory failure -> BadCommunicationError, no raw write", "status": "pending", "blockedBy": ["T5"] }, diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs index f14f480f..eff4d63d 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs @@ -401,28 +401,32 @@ public sealed class AddressSpaceApplier /// present, so re-applies are cheap. /// /// The composition result containing the hierarchy to materialise. - public void MaterialiseHierarchy(AddressSpaceComposition composition) + /// The count of swallowed sink failures in this pass (0 on a clean apply) — the publish actor + /// sums it into the degraded-apply surface (archreview 01/S-1). + public int MaterialiseHierarchy(AddressSpaceComposition composition) { ArgumentNullException.ThrowIfNull(composition); + var failed = 0; foreach (var area in composition.UnsAreas) { - SafeEnsureFolder(area.UnsAreaId, parentNodeId: null, displayName: area.DisplayName); + if (!SafeEnsureFolder(area.UnsAreaId, parentNodeId: null, displayName: area.DisplayName)) failed++; } foreach (var line in composition.UnsLines) { - SafeEnsureFolder(line.UnsLineId, parentNodeId: line.UnsAreaId, displayName: line.DisplayName); + if (!SafeEnsureFolder(line.UnsLineId, parentNodeId: line.UnsAreaId, displayName: line.DisplayName)) failed++; } foreach (var equipment in composition.EquipmentNodes) { // Equipment with no UnsLineId (legacy / dev rows) hang under the root. var parent = string.IsNullOrWhiteSpace(equipment.UnsLineId) ? null : equipment.UnsLineId; - SafeEnsureFolder(equipment.EquipmentId, parentNodeId: parent, displayName: equipment.DisplayName); + if (!SafeEnsureFolder(equipment.EquipmentId, parentNodeId: parent, displayName: equipment.DisplayName)) failed++; } _logger.LogInformation( - "AddressSpaceApplier: hierarchy materialised (areas={Areas}, lines={Lines}, equipment={Equipment})", - composition.UnsAreas.Count, composition.UnsLines.Count, composition.EquipmentNodes.Count); + "AddressSpaceApplier: hierarchy materialised (areas={Areas}, lines={Lines}, equipment={Equipment}, failed={Failed})", + composition.UnsAreas.Count, composition.UnsLines.Count, composition.EquipmentNodes.Count, failed); + return failed; } /// @@ -440,11 +444,14 @@ public sealed class AddressSpaceApplier /// takes a plain string dataType (not a DriverAttributeInfo). /// /// The composition result containing the equipment tags to materialise. - public void MaterialiseEquipmentTags(AddressSpaceComposition composition) + /// The count of swallowed sink failures in this pass (0 on a clean apply) — the publish actor + /// sums it into the degraded-apply surface (archreview 01/S-1). + public int MaterialiseEquipmentTags(AddressSpaceComposition composition) { ArgumentNullException.ThrowIfNull(composition); - if (composition.EquipmentTags.Count == 0) return; + if (composition.EquipmentTags.Count == 0) return 0; + var failed = 0; // Sub-folders first — a tag's FolderPath becomes one folder UNDER its equipment folder // (deduped per distinct equipment+path). Tags with no FolderPath hang directly under the // equipment folder, which MaterialiseHierarchy already created (never re-create @@ -455,7 +462,7 @@ public sealed class AddressSpaceApplier if (string.IsNullOrWhiteSpace(tag.FolderPath)) continue; var folderNodeId = EquipmentNodeIds.SubFolder(tag.EquipmentId, tag.FolderPath); if (!foldersCreated.Add(folderNodeId)) continue; - SafeEnsureFolder(folderNodeId, parentNodeId: tag.EquipmentId, displayName: tag.FolderPath); + if (!SafeEnsureFolder(folderNodeId, parentNodeId: tag.EquipmentId, displayName: tag.FolderPath)) failed++; } // Variables: NodeId is FOLDER-SCOPED ("/"), NOT the raw FullName — a driver @@ -474,7 +481,7 @@ public sealed class AddressSpaceApplier { // Native alarm tag → a real Part 9 condition node (reuses the scripted-alarm path), // NOT a value variable. Parent is the sub-folder when set, else the equipment folder. - SafeMaterialiseAlarmCondition(nodeId, parent, tag.Name, tag.Alarm.AlarmType, tag.Alarm.Severity, isNative: true); + if (!SafeMaterialiseAlarmCondition(nodeId, parent, tag.Name, tag.Alarm.AlarmType, tag.Alarm.Severity, isNative: true)) failed++; } else { @@ -488,14 +495,16 @@ public sealed class AddressSpaceApplier // even if authored ReadWrite, so a client write cannot reach the driver write path which // does not handle arrays (e.g. S7 BoxValueForWrite would crash). var writable = tag.Writable && !tag.IsArray; - SafeEnsureVariable(nodeId, parent, tag.Name, tag.DataType, writable, historianTagname, tag.IsArray, tag.ArrayLength); + if (!SafeEnsureVariable(nodeId, parent, tag.Name, tag.DataType, writable, historianTagname, tag.IsArray, tag.ArrayLength)) failed++; } } _logger.LogInformation( - "AddressSpaceApplier: equipment tags materialised (tags={Tags}, equipment={Equipment})", + "AddressSpaceApplier: equipment tags materialised (tags={Tags}, equipment={Equipment}, failed={Failed})", composition.EquipmentTags.Count, - composition.EquipmentTags.Select(t => t.EquipmentId).Distinct(StringComparer.Ordinal).Count()); + composition.EquipmentTags.Select(t => t.EquipmentId).Distinct(StringComparer.Ordinal).Count(), + failed); + return failed; } /// @@ -509,7 +518,9 @@ public sealed class AddressSpaceApplier /// NodeAdded model-change is announced under this node. /// The discovered folders to ensure (parent-first by depth). /// The discovered variables to ensure (read-only value nodes). - public void MaterialiseDiscoveredNodes( + /// The count of swallowed sink failures in this pass (0 on a clean apply) — the publish actor + /// surfaces a non-zero count as a degraded discovered-node injection (archreview 01/S-1). + public int MaterialiseDiscoveredNodes( string equipmentRootNodeId, IReadOnlyList folders, IReadOnlyList variables) @@ -517,25 +528,27 @@ public sealed class AddressSpaceApplier ArgumentException.ThrowIfNullOrEmpty(equipmentRootNodeId); ArgumentNullException.ThrowIfNull(folders); ArgumentNullException.ThrowIfNull(variables); - if (folders.Count == 0 && variables.Count == 0) return; + if (folders.Count == 0 && variables.Count == 0) return 0; + var failed = 0; // Parent-first: a child folder's parent must exist before it. Ordering by '/' count == depth. foreach (var f in folders.OrderBy(f => f.NodeId.Count(c => c == '/'))) - SafeEnsureFolder(f.NodeId, f.ParentNodeId, f.DisplayName); + if (!SafeEnsureFolder(f.NodeId, f.ParentNodeId, f.DisplayName)) failed++; foreach (var v in variables) { // Mirror MaterialiseEquipmentTags: arrays forced read-only (the driver write path can't handle arrays). var writable = v.Writable && !v.IsArray; - SafeEnsureVariable(v.NodeId, v.ParentNodeId, v.DisplayName, v.DataType, writable, - historianTagname: null, isArray: v.IsArray, arrayLength: v.ArrayLength); + if (!SafeEnsureVariable(v.NodeId, v.ParentNodeId, v.DisplayName, v.DataType, writable, + historianTagname: null, isArray: v.IsArray, arrayLength: v.ArrayLength)) failed++; } _sink.RaiseNodesAddedModelChange(equipmentRootNodeId); _logger.LogInformation( - "AddressSpaceApplier: discovered nodes materialised under {Equipment} (folders={Folders}, vars={Vars})", - equipmentRootNodeId, folders.Count, variables.Count); + "AddressSpaceApplier: discovered nodes materialised under {Equipment} (folders={Folders}, vars={Vars}, failed={Failed})", + equipmentRootNodeId, folders.Count, variables.Count, failed); + return failed; } /// @@ -551,11 +564,14 @@ public sealed class AddressSpaceApplier /// Idempotent (per-variable idempotency relies on the sink's own EnsureVariable). /// /// The composition result containing the equipment VirtualTags to materialise. - public void MaterialiseEquipmentVirtualTags(AddressSpaceComposition composition) + /// The count of swallowed sink failures in this pass (0 on a clean apply) — the publish actor + /// sums it into the degraded-apply surface (archreview 01/S-1). + public int MaterialiseEquipmentVirtualTags(AddressSpaceComposition composition) { ArgumentNullException.ThrowIfNull(composition); - if (composition.EquipmentVirtualTags.Count == 0) return; + if (composition.EquipmentVirtualTags.Count == 0) return 0; + var failed = 0; // Sub-folders first — a VirtualTag's FolderPath becomes one folder UNDER its equipment folder // (deduped per distinct equipment+path). VirtualTags with no FolderPath hang directly under the // equipment folder, which MaterialiseHierarchy already created (never re-create it here). @@ -565,7 +581,7 @@ public sealed class AddressSpaceApplier if (string.IsNullOrWhiteSpace(v.FolderPath)) continue; var folderNodeId = EquipmentNodeIds.SubFolder(v.EquipmentId, v.FolderPath); if (!foldersCreated.Add(folderNodeId)) continue; - SafeEnsureFolder(folderNodeId, parentNodeId: v.EquipmentId, displayName: v.FolderPath); + if (!SafeEnsureFolder(folderNodeId, parentNodeId: v.EquipmentId, displayName: v.FolderPath)) failed++; } // Variables: NodeId is FOLDER-SCOPED ("/"), mirroring the equipment-tag pass. @@ -581,13 +597,15 @@ public sealed class AddressSpaceApplier : EquipmentNodeIds.SubFolder(v.EquipmentId, v.FolderPath); var nodeId = EquipmentNodeIds.Variable(v.EquipmentId, v.FolderPath, v.Name); // VirtualTags are computed outputs — read-only nodes (no inbound write). - SafeEnsureVariable(nodeId, parent, v.Name, v.DataType, writable: false); + if (!SafeEnsureVariable(nodeId, parent, v.Name, v.DataType, writable: false)) failed++; } _logger.LogInformation( - "AddressSpaceApplier: equipment virtualtags materialised (vtags={Vtags}, equipment={Equipment})", + "AddressSpaceApplier: equipment virtualtags materialised (vtags={Vtags}, equipment={Equipment}, failed={Failed})", composition.EquipmentVirtualTags.Count, - composition.EquipmentVirtualTags.Select(v => v.EquipmentId).Distinct(StringComparer.Ordinal).Count()); + composition.EquipmentVirtualTags.Select(v => v.EquipmentId).Distinct(StringComparer.Ordinal).Count(), + failed); + return failed; } /// @@ -600,24 +618,29 @@ public sealed class AddressSpaceApplier /// MaterialiseAlarmCondition re-creates cleanly on re-apply). /// /// The composition result containing the scripted alarms to materialise. - public void MaterialiseScriptedAlarms(AddressSpaceComposition composition) + /// The count of swallowed sink failures in this pass (0 on a clean apply) — the publish actor + /// sums it into the degraded-apply surface (archreview 01/S-1). + public int MaterialiseScriptedAlarms(AddressSpaceComposition composition) { ArgumentNullException.ThrowIfNull(composition); - if (composition.EquipmentScriptedAlarms.Count == 0) return; + if (composition.EquipmentScriptedAlarms.Count == 0) return 0; var materialised = 0; + var failed = 0; foreach (var alarm in composition.EquipmentScriptedAlarms) { if (!alarm.Enabled) continue; - SafeMaterialiseAlarmCondition(alarm.ScriptedAlarmId, alarm.EquipmentId, alarm.Name, alarm.AlarmType, alarm.Severity, isNative: false); + if (!SafeMaterialiseAlarmCondition(alarm.ScriptedAlarmId, alarm.EquipmentId, alarm.Name, alarm.AlarmType, alarm.Severity, isNative: false)) failed++; materialised++; } _logger.LogInformation( - "AddressSpaceApplier: scripted alarms materialised (alarms={Alarms}, equipment={Equipment})", + "AddressSpaceApplier: scripted alarms materialised (alarms={Alarms}, equipment={Equipment}, failed={Failed})", materialised, composition.EquipmentScriptedAlarms.Where(a => a.Enabled) - .Select(a => a.EquipmentId).Distinct(StringComparer.Ordinal).Count()); + .Select(a => a.EquipmentId).Distinct(StringComparer.Ordinal).Count(), + failed); + return failed; } /// Ensure a folder node, swallowing (and Warning-logging) any sink fault. Returns true diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierFailureSurfaceTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierFailureSurfaceTests.cs index 7b340bd9..c4a1b75d 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierFailureSurfaceTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierFailureSurfaceTests.cs @@ -58,8 +58,69 @@ public sealed class AddressSpaceApplierFailureSurfaceTests outcome.FailedNodes.ShouldBe(1); } + // ---------------- T3: Materialise* passes return failed-node counts ---------------- + + /// Every EnsureVariable throwing in MaterialiseEquipmentTags is tallied into the pass's int return. + [Fact] + public void MaterialiseEquipmentTags_WhenEnsureVariableThrows_ReturnsFailedCount() + { + var sink = new ConfigurableThrowingSink { ThrowOnEnsureVariable = true }; + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + + var composition = CompositionWithTags( + new EquipmentTagPlan("t1", "eq-1", "drv", FolderPath: "", Name: "A", DataType: "Float", FullName: "40001", Writable: false, Alarm: null), + new EquipmentTagPlan("t2", "eq-1", "drv", FolderPath: "", Name: "B", DataType: "Float", FullName: "40002", Writable: false, Alarm: null), + new EquipmentTagPlan("t3", "eq-1", "drv", FolderPath: "", Name: "C", DataType: "Float", FullName: "40003", Writable: false, Alarm: null)); + + applier.MaterialiseEquipmentTags(composition).ShouldBe(3); + } + + /// All five Materialise* passes return 0 failures on a clean sink — the happy-path regression. + [Fact] + public void Materialise_HappyPath_AllPassesReturnZero() + { + var sink = new ConfigurableThrowingSink(); + var applier = new AddressSpaceApplier(sink, NullLogger.Instance); + + var tags = CompositionWithTags( + new EquipmentTagPlan("t1", "eq-1", "drv", FolderPath: "", Name: "A", DataType: "Float", FullName: "40001", Writable: false, Alarm: null)); + var vtags = CompositionWithVirtualTags( + new EquipmentVirtualTagPlan("v1", "eq-1", FolderPath: "", Name: "VA", DataType: "Float", Expression: "1", DependencyRefs: Array.Empty(), Historize: false)); + var alarms = CompositionWithScriptedAlarms( + new EquipmentScriptedAlarmPlan("a1", "eq-1", "Alarm", "OffNormalAlarm", 500, "msg", "pred", "true", + Array.Empty(), HistorizeToAveva: false, Retain: true, Enabled: true)); + + applier.MaterialiseHierarchy(tags).ShouldBe(0); + applier.MaterialiseEquipmentTags(tags).ShouldBe(0); + applier.MaterialiseEquipmentVirtualTags(vtags).ShouldBe(0); + applier.MaterialiseScriptedAlarms(alarms).ShouldBe(0); + applier.MaterialiseDiscoveredNodes( + "eq-1", + Array.Empty(), + new[] { new DiscoveredVariable("eq-1/D", "eq-1", "D", "Float", Writable: false, IsArray: false, ArrayLength: null) }) + .ShouldBe(0); + } + // ---------------- fixtures ---------------- + private static AddressSpaceComposition CompositionWithTags(params EquipmentTagPlan[] tags) => + new(Array.Empty(), Array.Empty(), Array.Empty()) + { + EquipmentTags = tags, + }; + + private static AddressSpaceComposition CompositionWithVirtualTags(params EquipmentVirtualTagPlan[] vtags) => + new(Array.Empty(), Array.Empty(), Array.Empty()) + { + EquipmentVirtualTags = vtags, + }; + + private static AddressSpaceComposition CompositionWithScriptedAlarms(params EquipmentScriptedAlarmPlan[] alarms) => + new(Array.Empty(), Array.Empty(), Array.Empty()) + { + EquipmentScriptedAlarms = alarms, + }; + private static AddressSpacePlan AddedEquipmentPlan(string id) => new( AddedEquipment: new[] { new EquipmentNode(id, id, "line-1") }, RemovedEquipment: Array.Empty(),