fix(opcua,drivers): carry driver status sub-codes to clients (#519) + health-write ordering (#517)

#519 — a driver's OPC UA status code now reaches a client with its sub-code intact. DriverInstanceActor projected the driver's uint onto a 3-state Good/Uncertain/Bad enum and OtOpcUaNodeManager re-expanded Bad to a bare 0x80000000, so BadNoCommunication, BadTypeMismatch and BadNotSupported were indistinguishable on the wire and every driver's status constants (incl. the 16 #497 had just corrected) were diagnostic in logs only. Resolved by option (b): the raw uint rides AttributeValuePublished -> AttributeValueUpdate -> IOpcUaAddressSpaceSink.WriteValue, OpcUaQuality is deleted, and severity classification is a helper (OpcUaStatus.IsGood/IsUncertain/IsBad). The surface was wider than the issue costed (41 files, not "the sink and 3 implementations"): OpcUaQuality had non-driver producers with no uint to carry. Each got its natural code — BadInternalError for a failed virtual-tag script, BadNoCommunication for driver comms loss — and ScriptedAlarmHostActor's pair of mutually-inverse uint<->enum mappers is deleted, having round-tripped a code the engine already held. AddressSpaceApplier's removal writes deliberately keep the generic Bad: not losing a code the driver had is this issue; inventing one for the deploy path is a separate call. Behaviour change: WriteAlarmQuality fires its Part 9 event on a status-code change rather than a severity-bucket change. While the annotation was 3-state those were the same predicate; now a move between two Bad reasons changes what a subscriber reads. New SDK-free named constants live on Commons.OpcUa.OpcUaStatus, and StatusCodeParityTests is widened to cover them — that guard is reflection-driven, so it can only see a named constant, never an inline literal. #517 — the symptom is real but the issue named the wrong path and its fix was unsafe. ShutdownAsync cannot race: DriverInstanceActor calls it from PostStop, after the mailbox drains. (Core/Hosting/DriverHost, the one class with a concurrent GetHealth, has no production call site.) InitializeAsync can: it runs on a fire-and-forget Task.Run, and MTConnectDriver writes Healthy then starts loops that can throw into a catch which disposes the client before writing Faulted. Moving the write earlier, as suggested, inverts the terminal state — teardown drains in-flight loops that write health as they unwind, so a late loop could leave Healthy at rest with every connection closed. The fix is a bracket (conservative before, terminal after), applied to MTConnect's init-failure path only; shutdown keeps its single trailing write, and the rule plus the reachability table are documented on IDriver.ShutdownAsync. CalculationDriver.GetHealth returned a hard-coded Healthy that consulted nothing — Healthy before initialize, after shutdown and after dispose. Now derived from _running. Its test, named Health_is_always_Connected, asserted the defect and is replaced. Every new guard was falsified by breaking the production code and watching it go red. The two sub-code tests each assert two different Bad codes, since a single-code assertion could pass against a hard-coded constant under the old projection. Pre-existing failures, verified unchanged on a stashed tree and not chased: 4 in Driver.AbLegacy.IntegrationTests (slc500/micrologix/plc5 fixtures not up) and 3 in Host.IntegrationTests. Closes #517 Closes #519 Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo
2026-07-31 05:25:27 -04:00
parent ba85e871df
commit 2d20aeb779
53 changed files with 721 additions and 311 deletions
@@ -279,7 +279,7 @@ public sealed class AddressSpaceApplier
            else
            {
                // Plain value variable → terminal Bad so an in-flight MonitoredItem sees the removal.
-                SafeWriteValue(nodeId, null, OpcUaQuality.Bad, ts, AddressSpaceRealm.Uns);
+                SafeWriteValue(nodeId, null, OpcUaStatus.Bad, ts, AddressSpaceRealm.Uns);
                if (!SafeRemoveVariable(surgical, nodeId, AddressSpaceRealm.Uns)) return false;
            }
        }
@@ -288,7 +288,7 @@ public sealed class AddressSpaceApplier
        foreach (var v in plan.RemovedEquipmentVirtualTags.Where(t => NotSubsumed(t.EquipmentId)))
        {
            var nodeId = UnsEquipmentVar(v.EquipmentId, v.FolderPath, v.Name);
-            SafeWriteValue(nodeId, null, OpcUaQuality.Bad, ts, AddressSpaceRealm.Uns);
+            SafeWriteValue(nodeId, null, OpcUaStatus.Bad, ts, AddressSpaceRealm.Uns);
            if (!SafeRemoveVariable(surgical, nodeId, AddressSpaceRealm.Uns)) return false;
        }

@@ -311,7 +311,7 @@ public sealed class AddressSpaceApplier
        // ref only reaches here on a genuine dereference (the equipment dropped the reference).
        foreach (var v in plan.RemovedUnsReferenceVariables)
        {
-            SafeWriteValue(v.NodeId, null, OpcUaQuality.Bad, ts, AddressSpaceRealm.Uns);
+            SafeWriteValue(v.NodeId, null, OpcUaStatus.Bad, ts, AddressSpaceRealm.Uns);
            if (!SafeRemoveVariable(surgical, v.NodeId, AddressSpaceRealm.Uns)) return false;
        }

@@ -327,7 +327,7 @@ public sealed class AddressSpaceApplier
            }
            else
            {
-                SafeWriteValue(t.NodeId, null, OpcUaQuality.Bad, ts, AddressSpaceRealm.Raw);
+                SafeWriteValue(t.NodeId, null, OpcUaStatus.Bad, ts, AddressSpaceRealm.Raw);
                if (!SafeRemoveVariable(surgical, t.NodeId, AddressSpaceRealm.Raw)) return false;
            }
        }
@@ -365,11 +365,16 @@ public sealed class AddressSpaceApplier
    }

    /// <summary>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.</summary>
+    /// on a removed variable so an in-flight MonitoredItem observes the removal.
+    /// <para>Every caller passes the generic <see cref="OpcUaStatus.Bad"/>, which is exactly what the
+    /// retired 3-state projection produced here. A removal could say something more specific (the node is
+    /// about to stop existing, so <c>BadNodeIdUnknown</c> would be truer), but #519 was about not LOSING a
+    /// sub-code the driver already had — inventing a new one for the deploy path is a separate call, so
+    /// this deliberately stays behaviour-identical.</para></summary>
    /// <returns><c>true</c> when the write landed; <c>false</c> when the sink threw.</returns>
-    private bool SafeWriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime ts, AddressSpaceRealm realm)
+    private bool SafeWriteValue(string nodeId, object? value, uint statusCode, DateTime ts, AddressSpaceRealm realm)
    {
-        try { _sink.WriteValue(nodeId, value, quality, ts, realm); return true; }
+        try { _sink.WriteValue(nodeId, value, statusCode, ts, realm); return true; }
        catch (Exception ex) { _logger.LogWarning(ex, "AddressSpaceApplier: WriteValue threw for {Node}", nodeId); return false; }
    }

@@ -408,7 +408,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
    /// <param name="value">The new value to write.</param>
    /// <param name="quality">The OPC UA quality status code.</param>
    /// <param name="sourceTimestampUtc">The timestamp of the value in UTC.</param>
-    public void WriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime sourceTimestampUtc, AddressSpaceRealm realm)
+    public void WriteValue(string nodeId, object? value, uint statusCode, DateTime sourceTimestampUtc, AddressSpaceRealm realm)
    {
        ArgumentException.ThrowIfNullOrEmpty(nodeId);
        EnsureAddressSpaceCreated();
@@ -426,7 +426,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            }

            variable.Value = value;
-            variable.StatusCode = StatusFromQuality(quality);
+            // #519 — the caller's status code reaches the SDK verbatim, sub-code included. This used to
+            // re-expand a 3-state enum, so every Bad became a bare StatusCodes.Bad (0x80000000) and a
+            // driver's BadNoCommunication was indistinguishable from BadTypeMismatch at the client.
+            variable.StatusCode = new StatusCode(statusCode);
            variable.Timestamp = sourceTimestampUtc;
            variable.ClearChangeMasks(SystemContext, includeChildren: false);
        }
@@ -503,7 +506,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
                // null-guard it like Confirmed/Shelving in case a leaner SDK child set omits it.
                if (condition.Quality is not null)
                {
-                    condition.Quality.Value = StatusFromQuality(state.Quality);
+                    condition.Quality.Value = new StatusCode(state.StatusCode);
                    if (condition.Quality.SourceTimestamp is not null) condition.Quality.SourceTimestamp.Value = sourceTimestampUtc;
                }

@@ -547,19 +550,23 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2

    /// <summary>
    ///     #477 Layer 2 — apply a source-data quality annotation to a materialised condition, OUT OF BAND
-    ///     from any alarm transition. Used by the driver-connectivity path (comms lost → Bad, restored →
-    ///     Good) so a native condition whose device is unreachable stops reporting the accidentally-Good
-    ///     default. This sets ONLY <see cref="ConditionState.Quality"/> — it never touches
+    ///     from any alarm transition. Used by the driver-connectivity path (comms lost → BadNoCommunication,
+    ///     restored → Good) so a native condition whose device is unreachable stops reporting the
+    ///     accidentally-Good default. This sets ONLY <see cref="ConditionState.Quality"/> — it never touches
    ///     Active / Acked / Severity / Retain (a comms-lost active alarm must stay active). A change in the
-    ///     quality bucket is a genuine Part 9 condition change, so it fires one condition event carrying the
-    ///     new Quality; an unchanged bucket suppresses (no spurious event). Unknown / unmaterialised node ⇒
+    ///     status code is a genuine Part 9 condition change, so it fires one condition event carrying the
+    ///     new Quality; an unchanged code suppresses (no spurious event). Unknown / unmaterialised node ⇒
    ///     safe no-op (a mid-rebuild race must not fault a connectivity update), mirroring the other writes.
+    ///     <para><b>#519 changed the fire test from bucket to full code.</b> While the annotation was a
+    ///     3-state enum, "the bucket moved" and "the value changed" were the same predicate. Now that the
+    ///     sub-code survives, a move from one Bad reason to another Bad reason genuinely changes what a
+    ///     client reads, so it fires — suppressing it would leave subscribers holding a stale reason.</para>
    /// </summary>
    /// <param name="alarmNodeId">The condition node id (RawPath for a native alarm).</param>
-    /// <param name="quality">The source-data quality to annotate.</param>
+    /// <param name="statusCode">The source-data status code to annotate, sub-code included.</param>
    /// <param name="sourceTimestampUtc">Timestamp of the connectivity transition in UTC.</param>
    /// <param name="realm">The condition's address-space realm.</param>
-    public void WriteAlarmQuality(string alarmNodeId, OpcUaQuality quality, DateTime sourceTimestampUtc, AddressSpaceRealm realm)
+    public void WriteAlarmQuality(string alarmNodeId, uint statusCode, DateTime sourceTimestampUtc, AddressSpaceRealm realm)
    {
        ArgumentException.ThrowIfNullOrEmpty(alarmNodeId);
        EnsureAddressSpaceCreated();
@@ -573,13 +580,13 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            if (!_alarmConditions.TryGetValue(key, out var condition) || condition.Quality is null)
                return;

-            var newCode = StatusFromQuality(quality);
+            var newCode = new StatusCode(statusCode);
            var changed = condition.Quality.Value.Code != newCode.Code;

            condition.Quality.Value = newCode;
            if (condition.Quality.SourceTimestamp is not null) condition.Quality.SourceTimestamp.Value = sourceTimestampUtc;

-            // Fire ONLY on a real bucket change so a steady-state connectivity re-assert doesn't spam events.
+            // Fire ONLY on a real change so a steady-state connectivity re-assert doesn't spam events.
            if (changed)
            {
                condition.Time.Value = sourceTimestampUtc;
@@ -758,7 +765,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        Message: state.Message ?? string.Empty,
        // If the node has no Quality child, WriteAlarmCondition's projection is a no-op there; fold to the
        // node's read-back default (Good) so a snapshot Quality can't register a spurious delta.
-        Quality: condition.Quality is not null ? StatusFromQuality(state.Quality) : StatusCodes.Good);
+        Quality: condition.Quality is not null ? new StatusCode(state.StatusCode) : StatusCodes.Good);

    /// <summary>Map the live shelving state machine's CurrentState back to our 3-way
    /// <see cref="AlarmShelvingKind"/> by matching its well-known Part 9 state object id. Any node without
@@ -3315,10 +3322,4 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        return v;
    }

-    private static StatusCode StatusFromQuality(OpcUaQuality quality) => quality switch
-    {
-        OpcUaQuality.Good => StatusCodes.Good,
-        OpcUaQuality.Uncertain => StatusCodes.Uncertain,
-        _ => StatusCodes.Bad,
-    };
 }
@@ -21,15 +21,15 @@ public sealed class SdkAddressSpaceSink : IOpcUaAddressSpaceSink, ISurgicalAddre
    }

    /// <inheritdoc />
-    public void WriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime sourceTimestampUtc, AddressSpaceRealm realm)
-        => _nodeManager.WriteValue(nodeId, value, quality, sourceTimestampUtc, realm);
+    public void WriteValue(string nodeId, object? value, uint statusCode, DateTime sourceTimestampUtc, AddressSpaceRealm realm)
+        => _nodeManager.WriteValue(nodeId, value, statusCode, sourceTimestampUtc, realm);

    /// <inheritdoc />
    public void WriteAlarmCondition(string alarmNodeId, AlarmConditionSnapshot state, DateTime sourceTimestampUtc, AddressSpaceRealm realm)
        => _nodeManager.WriteAlarmCondition(alarmNodeId, state, sourceTimestampUtc, realm);

-    public void WriteAlarmQuality(string alarmNodeId, OpcUaQuality quality, DateTime sourceTimestampUtc, AddressSpaceRealm realm)
-        => _nodeManager.WriteAlarmQuality(alarmNodeId, quality, sourceTimestampUtc, realm);
+    public void WriteAlarmQuality(string alarmNodeId, uint statusCode, DateTime sourceTimestampUtc, AddressSpaceRealm realm)
+        => _nodeManager.WriteAlarmQuality(alarmNodeId, statusCode, sourceTimestampUtc, realm);

    /// <inheritdoc />
    public void MaterialiseAlarmCondition(string alarmNodeId, string equipmentNodeId, string displayName, string alarmType, int severity, AddressSpaceRealm realm, bool isNative = false)