fix(alarms): #647 re-review — re-statement needs the same message; CNC refusal is not comms loss; worst-of health

R1: NativeAlarmProjector treats Raise-on-Active as a re-statement only when the message matches what the condition shows; a different alarm appearing during a respawn un-acks and writes its alerts row. FOCAS condition messages are a deterministic function of the matched set (tested across fresh driver instances and reordered lists); Runtime E2E now covers both the same alarm and a new one during a respawn. Residual (source message determinism) documented in AlarmTracking.md. R2: a non-OK cnc_rdalmmsg2 return is thrown as FocasWireException{Rc} and classified as a CNC refusal: conditions get a quality-only BadNotSupported once, one Warning, polling continues, and driver health is untouched (no 90 s watchdog reconnect loop on a production control). Only transport faults degrade (falsification control in the test). R3: alarm-path health is tracked apart from the value path and combined in GetHealth: worst state, later LastSuccessfulRead. A good value read can no longer mask a failing alarm read (no per-poll /hosts flip) and a good alarm read cannot mask a value Degraded. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
2026-10-08 20:45:32 -04:00
parent 89961af6e6
commit bac93744ce
11 changed files with 308 additions and 57 deletions
@@ -126,6 +126,9 @@ internal class FakeFocasClient : IFocasClient
    /// <summary>Gets or sets a value indicating whether the alarm read throws (Gitea #647: a failed read).</summary>
    public bool ThrowOnReadAlarms { get; set; }

+    /// <summary>Gets or sets an exception the alarm read throws (e.g. a CNC refusal carrying an rc).</summary>
+    public Exception? AlarmReadException { get; set; }
+
    /// <summary>Gets the number of alarm reads served (successful or not).</summary>
    public int AlarmReadCount => Volatile.Read(ref _alarmReadCount);

@@ -143,6 +146,7 @@ internal class FakeFocasClient : IFocasClient
    public virtual Task<IReadOnlyList<FocasActiveAlarm>> ReadAlarmsAsync(CancellationToken ct)
    {
        Interlocked.Increment(ref _alarmReadCount);
+        if (AlarmReadException is { } refusal) throw refusal;
        if (ThrowOnReadAlarms) throw new InvalidOperationException("cnc_rdalmmsg2 failed (fake)");
        lock (Alarms) return Task.FromResult<IReadOnlyList<FocasActiveAlarm>>([.. Alarms]);
    }
@@ -545,6 +545,111 @@ public sealed class FocasAlarmProjectionTests
        }
    }

+    // ---- Gitea #647 re-review R1–R3 -------------------------------------------------------------------------
+
+    /// <summary>
+    ///     R1 — the host treats a respawned driver's Raise as a re-statement only when its message matches what the
+    ///     condition shows, so the message must be a deterministic function of the matched SET: two fresh driver
+    ///     instances reading the same alarms in a different wire (and axis) order say exactly the same thing, and a
+    ///     different set says something different.
+    /// </summary>
+    [Fact]
+    public async Task The_condition_message_is_deterministic_for_the_same_alarm_set_across_driver_instances()
+    {
+        async Task<string> FirstRaiseMessage(params FocasActiveAlarm[] alarms)
+        {
+            var (drv, factory) = NewDriver(Tag(AnyPath, "ALARM:*"));
+            await InitAndSubscribeForTicksAsync(drv, factory, AnyPath);
+            var events = Capture(drv);
+            Tick(drv, alarms);
+            return events.Single().Message;
+        }
+
+        var a = await FirstRaiseMessage(Servo401(axis: 2), Overheat700, Servo401(axis: 1), Overtravel506);
+        var b = await FirstRaiseMessage(Overtravel506, Servo401(axis: 1), Overheat700, Servo401(axis: 2));
+        b.ShouldBe(a);
+        (await FirstRaiseMessage(Overtravel506, Servo401(axis: 1))).ShouldNotBe(a, "a different set must not read as a re-statement");
+    }
+
+    private static Wire.FocasWireException CncRefusal() => new("cnc_rdalmmsg2 refused EW_FUNC", rc: 1);
+
+    /// <summary>
+    ///     R2 — a CNC REFUSAL of cnc_rdalmmsg2 (an rc; the transport is fine) is a capability answer, not comms loss:
+    ///     health stays Healthy (so the #527 watchdog can never reconnect-loop a production control), the conditions
+    ///     are annotated BadNotSupported once, one Warning is logged, and polling continues at the normal cadence.
+    ///     Falsification control: a TRANSPORT failure on the same driver does degrade.
+    /// </summary>
+    [Fact]
+    public async Task A_CNC_refusal_marks_conditions_not_supported_without_degrading_health()
+    {
+        var logger = new ListLogger();
+        var factory = new FakeFocasClientFactory { Customise = () => new FakeFocasClient { AlarmReadException = CncRefusal() } };
+        var drv = new FocasDriver(new FocasDriverOptions
+        {
+            Devices = [new FocasDeviceOptions(Host, DeviceName: "m1")],
+            RawTags = [Tag(ServoPath, "ALARM:Servo")],
+            Probe = new FocasProbeOptions { Enabled = false },
+            AlarmProjection = new FocasAlarmProjectionOptions { PollInterval = TimeSpan.FromMilliseconds(20) },
+        }, "drv-1", factory, logger);
+        await drv.InitializeAsync("{}", CancellationToken.None);
+        var events = Capture(drv);
+        await drv.SubscribeAlarmsAsync([ServoPath], CancellationToken.None);
+        await WaitFor(() => factory.Clients.Count > 0 && factory.Clients[0].AlarmReadCount >= 5);
+
+        drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a refusal is not a comms fault");
+        lock (events)
+        {
+            events.Count.ShouldBe(1);
+            events[0].IsQualityOnly.ShouldBeTrue();
+            events[0].QualityStatusCode.ShouldBe(FocasStatusMapper.BadNotSupported);
+        }
+        logger.Warnings().ShouldBe(1);
+        factory.Clients.Count.ShouldBe(1, "a refusal does not tear the session down");
+
+        // Control: the same driver losing its transport DOES degrade, and re-annotates the condition.
+        factory.Clients[0].AlarmReadException = null;
+        factory.Clients[0].ThrowOnReadAlarms = true;
+        await WaitFor(() => drv.GetHealth().State == DriverState.Degraded);
+        lock (events) events[^1].QualityStatusCode.ShouldBe(BadNoCommunication);
+        await drv.ShutdownAsync(CancellationToken.None);
+    }
+
+    /// <summary>
+    ///     R3 — on a mixed driver health is the WORST of the value path and the alarm path, computed together: a
+    ///     failing alarm read keeps the driver Degraded however many value reads succeed (no per-poll flip on
+    ///     /hosts), and a successful alarm read cannot mask a value-path Degraded.
+    /// </summary>
+    [Fact]
+    public async Task Health_is_the_worst_of_the_value_and_alarm_paths()
+    {
+        const string valuePath = "Plant/Cnc/m1/r100";
+        var (drv, factory) = NewDriver(
+            new RawTagEntry(valuePath, """{"address":"R100","dataType":"Byte"}""", false, "m1"),
+            Tag(ServoPath, "ALARM:Servo"));
+        await drv.InitializeAsync("{}", CancellationToken.None);
+
+        // Value reads good, alarm path failing ⇒ Degraded, and it stays Degraded across further good value reads.
+        (await drv.ReadAsync([valuePath], CancellationToken.None))[0].StatusCode.ShouldBe(FocasStatusMapper.Good);
+        drv.NoteAlarmReadFailed(Host, new InvalidOperationException("socket closed"));
+        for (var i = 0; i < 3; i++)
+        {
+            await drv.ReadAsync([valuePath], CancellationToken.None);
+            drv.GetHealth().State.ShouldBe(DriverState.Degraded, "a good value read must not mask the failing alarm path");
+        }
+        drv.GetHealth().LastSuccessfulRead.ShouldNotBeNull("value reads still succeed, so the watchdog is not tripped");
+
+        // Alarm path recovers ⇒ Healthy again.
+        drv.NoteAlarmReadSucceeded(Host);
+        drv.GetHealth().State.ShouldBe(DriverState.Healthy);
+
+        // Value path Degraded, alarm path good ⇒ Degraded: an alarm success cannot mask it.
+        factory.Clients.Single().ReadStatuses["R100"] = FocasStatusMapper.BadOutOfRange;
+        await drv.ReadAsync([valuePath], CancellationToken.None);
+        drv.NoteAlarmReadSucceeded(Host);
+        drv.GetHealth().State.ShouldBe(DriverState.Degraded);
+        await drv.ShutdownAsync(CancellationToken.None);
+    }
+
    private static int Count(List<AlarmEventArgs> events)
    {
        lock (events) return events.Count;
@@ -98,8 +98,15 @@ public sealed class FocasNativeAlarmEndToEndTests : RuntimeActorTestBase
    ///     sends Raise; the host must treat that as a re-statement: the operator's acknowledgement survives and no
    ///     second "Activated" alerts row is written. Driven through the real host with a FRESH driver instance.
    /// </summary>
-    [Fact]
-    public async Task A_respawned_driver_re_reading_an_acknowledged_active_alarm_keeps_the_ack()
+    /// <para>
+    ///     Re-review R1 — the other half: a DIFFERENT alarm that appeared while the driver was down (here a second
+    ///     servo alarm, so the condition's displayed set changed) is a new statement even though the fresh driver
+    ///     can only say Raise. It must un-acknowledge and write its alerts row.
+    /// </para>
+    [Theory]
+    [InlineData(false)]
+    [InlineData(true)]
+    public async Task A_respawned_driver_re_reading_an_acknowledged_active_alarm_keeps_the_ack(bool newAlarmDuringRespawn)
    {
        var db = NewInMemoryDbFactory();
        var blob = BuildArtifact();
@@ -130,14 +137,27 @@ public sealed class FocasNativeAlarmEndToEndTests : RuntimeActorTestBase
        coordinator.FishForMessage<ApplyAck>(a => a.Outcome == ApplyAckOutcome.Applied, TimeSpan.FromSeconds(5));
        publish.FishForMessage<OpcUaPublishActor.RebuildAddressSpace>(_ => true, TimeSpan.FromSeconds(5));

+        if (newAlarmDuringRespawn)
+            cnc.SetAlarms(
+                new FocasActiveAlarm(401, FocasAlarmType.Servo, 1, "SERVO V-READY OFF"),
+                new FocasActiveAlarm(410, FocasAlarmType.Servo, 2, "SERVO EXCESS ERROR"));
+
        var respawned = await StartDriverAsync(rawTags, cnc, host);
        try
        {
            var restated = publish.FishForMessage<OpcUaPublishActor.AlarmStateUpdate>(_ => true, TimeSpan.FromSeconds(5));
            restated.AlarmNodeId.ShouldBe(ExpectedRawPath);
            restated.State.Active.ShouldBeTrue();
-            restated.State.Acknowledged.ShouldBeTrue("a respawned driver's re-statement must not un-acknowledge");
-            alerts.ExpectNoMsg(TimeSpan.FromMilliseconds(500));
+            if (newAlarmDuringRespawn)
+            {
+                restated.State.Acknowledged.ShouldBeFalse("an alarm the operator has not seen must not inherit the old ack");
+                alerts.ExpectMsg<AlarmTransitionEvent>(TimeSpan.FromSeconds(5));
+            }
+            else
+            {
+                restated.State.Acknowledged.ShouldBeTrue("a respawned driver's re-statement must not un-acknowledge");
+                alerts.ExpectNoMsg(TimeSpan.FromMilliseconds(500));
+            }
        }
        finally
        {
@@ -237,7 +237,7 @@ public class NativeAlarmProjectorTests
        first.ShouldBeFalse("inactive → active is a real transition");
        sut.MarkAcknowledged("n1");

-        var restated = sut.Project("n1", Evt(AlarmTransitionKind.Raise, msg: "again"), out var restatement);
+        var restated = sut.Project("n1", Evt(AlarmTransitionKind.Raise), out var restatement);
        restatement.ShouldBeTrue();
        restated.Active.ShouldBeTrue();
        restated.Acknowledged.ShouldBeTrue("a re-statement must not un-acknowledge");
@@ -247,6 +247,27 @@ public class NativeAlarmProjectorTests
        retriggered.Acknowledged.ShouldBeFalse("a Retrigger is a new statement and un-acks");
    }

+    /// <summary>
+    ///     Gitea #647 re-review R1 — a respawned driver cannot tell "the same alarm" from "a different one that
+    ///     appeared while it was down"; its Raise is a re-statement only when its message matches what the condition
+    ///     shows. A different message is a new statement: it un-acks and is not flagged (so the alerts row is written).
+    /// </summary>
+    [Fact]
+    public void A_Raise_on_an_active_condition_with_a_different_message_is_a_new_statement()
+    {
+        var sut = new NativeAlarmProjector();
+        sut.Project("n1", Evt(AlarmTransitionKind.Raise, msg: "401 SERVO (type 4 Servo, axis 1)"));
+        sut.MarkAcknowledged("n1");
+
+        var same = sut.Project("n1", Evt(AlarmTransitionKind.Raise, msg: "401 SERVO (type 4 Servo, axis 1)"), out var r1);
+        r1.ShouldBeTrue();
+        same.Acknowledged.ShouldBeTrue();
+
+        var other = sut.Project("n1", Evt(AlarmTransitionKind.Raise, msg: "506 OVER TRAVEL (type 2 Overtravel, axis 2)"), out var r2);
+        r2.ShouldBeFalse("a different alarm is not a re-statement");
+        other.Acknowledged.ShouldBeFalse("the operator has not seen this alarm");
+    }
+
    private static AlarmEventArgs Evt(
        AlarmTransitionKind kind,
        AlarmSeverity sev = AlarmSeverity.High,