fix(runtime): #869 review round 2 — a carry-through transition no longer settles a pending local ack

NativeAlarmProjector dropped the pending local ack on EVERY driver transition, so an operator ack followed by the alarm returning to normal (a Clear carrying the optimistic ack) inside the confirmation window made a later "not confirmed" report a no-op — the condition stayed inactive+acked while Galaxy showed UNACK_RTN. Only a transition that states the ack (Acknowledge, ActiveAcknowledged, or an un-acking Raise/Retrigger/InactiveUnacknowledged) now settles it; a Clear / same-message Raise / Unspecified update leaves it revertible, and the revert projects inactive+unacked. Tests: projector + host end-to-end (ack, Clear, confirmation fails -> inactive + unacked); tightened the confirmed-ack positive controls; pinned that the confirmation receives the same request instance as AcknowledgeAsync. Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk
2026-11-11 13:19:52 -05:00
parent 77872b1d1c
commit 5001e40690
7 changed files with 93 additions and 15 deletions
@@ -42,7 +42,7 @@ yet re-listed. Only an identity written straight into the store by the `secret`
 refused until the next refresh, which the refusal itself requests. (2) anything else (a keyed session the cache cannot vouch for, MXAccess
 non-zero `hresult`, driver still connecting/reconnecting) comes back as `DriverInstanceActor.AlarmAckFailed` — or, for a
 disabled/unspawned/stubbed driver, is decided in the host itself (#765) — and `DriverHostActor` re-projects the condition unacknowledged — only while the ack is still pending in
-`NativeAlarmProjector` (any driver transition since then wins). A host that came up **Stale** cannot route any
+`NativeAlarmProjector` (a driver transition since then that STATES the ack wins; a Clear / same-message Raise only carries it through and leaves it revertible, #869). A host that came up **Stale** cannot route any
 native ack, so it marks the whole `NativeAlarmAckPreflightView` unavailable and every native ack is refused
 synchronously until `BecomeSteady` (#765). A test that routes an ack and expects it to STAND must start the owning
 driver (`AckingAlarmDriver` in Runtime.Tests).
@@ -474,7 +474,10 @@ from the scripted `AlarmCommandRouter`:
   condition event). An ack whose owning driver is not running (disabled,
   not yet spawned) or is a stub on this node is reverted the same way by
   the host itself (#765). Only an ack still pending in `NativeAlarmProjector` is
-   reverted: a driver transition projected since then is authoritative. A
+   reverted: a driver transition projected since then that states the ack
+   (Acknowledge, ActiveAcknowledged, or an un-acking Raise/Retrigger/
+   InactiveUnacknowledged) is authoritative; one that only carries the ack
+   through (a Clear, a same-message Raise) leaves it revertible (#869). A
   refusal the driver can decide without I/O (Galaxy, with no fallback ack
   account: an anonymous session, or a keyed session whose key the cached
   `GalaxyAckIdentityDirectory` knows has no ack identity — #765) is instead
@@ -129,8 +129,8 @@ Galaxy password, so `GalaxyAckCredentialResolver` picks the account per acknowle
     identity is broken (a user with no password secret), or MXAccess rejecting the account (a non-zero
     native `hresult`, e.g. a wrong password, now thrown as a refusal): the call returned `Good`, then
     `DriverHostActor` re-projects the condition **unacknowledged** (a fresh condition event,
-     `Retain=true`) and logs the reason. A driver transition projected after the ack wins over a late
-     failure.
+     `Retain=true`) and logs the reason. A driver transition projected after the ack that states the ack
+     state wins over a late failure; a Clear only carries it through and leaves it revertible (#869).

 ### Ack confirmation (#869)

@@ -78,9 +78,6 @@ public sealed class NativeAlarmProjector
    /// <returns>The full Part 9 condition snapshot to write to the node.</returns>
    public AlarmConditionSnapshot Project(string nodeId, AlarmEventArgs e, out IReadOnlyList<AlarmTransitionKind> historyKinds)
    {
-        // Any driver transition supersedes a pending local acknowledge: from here the projected state is the
-        // driver's word, so a late ack failure must not rewrite it.
-        _pendingLocalAck.Remove(nodeId);
        var prev = _prior.TryGetValue(nodeId, out var p)
            ? p : (Active: false, Acked: true, Message: (string?)null, Severity: (ushort)0, StatusCode: OpcUaStatus.Good, Class: (AlarmConditionClass?)null);
        // A source with a finer scale than the four buckets supplies the exact Part 9 severity.
@@ -153,6 +150,15 @@ public sealed class NativeAlarmProjector
                break;
        }

+        // A driver transition that STATES the ack (Acknowledge / ActiveAcknowledged, or an un-acking Raise /
+        // Retrigger / InactiveUnacknowledged) supersedes a pending local acknowledge: from here the ack state is the
+        // driver's word, so a late ack failure must not rewrite it. One that merely CARRIES the prior ack through
+        // (a Clear, a same-message Raise, a message/severity update) says nothing about it — the optimistic local
+        // ack rode along, so it stays pending and a later failure still reverts it (lmxopcua#869: an operator acks,
+        // the value returns to normal while the unconfirmed ack is still being confirmed, Galaxy shows UNACK_RTN).
+        if (acked != prev.Acked || e.Kind is AlarmTransitionKind.Acknowledge or AlarmTransitionKind.ActiveAcknowledged)
+            _pendingLocalAck.Remove(nodeId);
+
        var statusCode = e.QualityStatusCode ?? OpcUaStatus.Good;
        // A source that classifies its conditions (#839) states the class on its transitions; one that does not (or a
        // quality-only annotation) keeps whatever the condition last carried.
@@ -185,8 +191,9 @@ public sealed class NativeAlarmProjector
    /// <summary>
    ///     Undo a client acknowledgement the driver refused or failed to deliver upstream (lmxopcua#760), returning
    ///     the unacknowledged snapshot to re-project onto the node. Only an acknowledge still pending since
-    ///     <see cref="MarkAcknowledged"/> is reverted: once any driver transition for the condition has been
-    ///     projected, that transition is authoritative and a late failure changes nothing.
+    ///     <see cref="MarkAcknowledged"/> is reverted: once a driver transition that STATES the condition's ack state
+    ///     has been projected, that transition is authoritative and a late failure changes nothing. A transition that
+    ///     only carries the prior ack through (e.g. a Clear) leaves it pending (lmxopcua#869).
    /// </summary>
    /// <param name="nodeId">The condition node id whose acknowledge failed.</param>
    /// <param name="snapshot">The unacknowledged snapshot to project; null when nothing was reverted.</param>
@@ -225,6 +225,41 @@ public sealed class DriverHostActorNativeAlarmAckRoutingTests : RuntimeActorTest
        var next = NextAlarmUpdate(publish);
        next.State.Active.ShouldBeFalse();
        next.State.Acknowledged.ShouldBeTrue("the successful ack must have survived — no revert");
+
+        // A Clear carries the ack through and so leaves a still-pending ack revertible (#869): a wrong revert landing
+        // after it would be the NEXT update. A carry-through update must therefore still show it acknowledged.
+        actor.Tell(new DriverInstanceActor.AttributeAlarmPublished("drv-1", Evt(AlarmTransitionKind.Unspecified)));
+        NextAlarmUpdate(publish).State.Acknowledged.ShouldBeTrue("no late revert may follow a confirmed ack");
+    }
+
+    /// <summary>
+    ///     lmxopcua#869 — the common sequence: an operator acks, the alarm returns to normal (a Clear, carrying the
+    ///     optimistic ack) while the unconfirmed ack is still being confirmed, then the confirmation fails. The ack
+    ///     must still revert — to inactive + UNacknowledged, Galaxy's UNACK_RTN — not stay hidden behind the Clear.
+    /// </summary>
+    [Fact]
+    public void An_unconfirmed_ack_reverts_even_after_a_clear_inside_the_window()
+    {
+        var db = NewInMemoryDbFactory();
+        var gate = new TaskCompletionSource();
+        var recorder = new RecordingAlarmDriverFactory("GalaxyMxGateway") { Confirmation = () => gate.Task };
+        var deploymentId = SeedV3AlarmDeployment(db, RevA, Driver: "drv-1", Tag: "temp_hi");
+        var publish = this.CreateCleanOutcomePublishProbe();
+        var actor = SpawnHostAndApply(db, deploymentId, recorder, publish.Ref);
+
+        RaiseAndExpectUnacked(actor, publish);
+        actor.Tell(new DriverHostActor.RouteNativeAlarmAck(AlarmRawPath, "cmt", "alice"));
+        AwaitAssert(() => recorder.ConfirmCalls.ShouldBe(1), duration: Timeout, cancellationToken: TestContext.Current.CancellationToken);
+
+        actor.Tell(new DriverInstanceActor.AttributeAlarmPublished("drv-1", Evt(AlarmTransitionKind.Clear)));
+        var cleared = NextAlarmUpdate(publish);
+        cleared.State.Active.ShouldBeFalse();
+        cleared.State.Acknowledged.ShouldBeTrue("the optimistic ack rides the Clear");
+
+        gate.SetException(new DriverOperationRefusedException("Galaxy did not confirm the acknowledge (BadTimeout)"));
+        var reverted = NextAlarmUpdate(publish);
+        reverted.State.Active.ShouldBeFalse();
+        reverted.State.Acknowledged.ShouldBeFalse("the unconfirmed ack must not survive behind the Clear");
    }

    /// <summary>
@@ -9,9 +9,9 @@ using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness;
 namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers;

 /// <summary>
-///     lmxopcua#869 — <see cref="DriverInstanceActor"/> asks an <see cref="IAlarmAcknowledgeConfirmation"/> driver to
+///     lmxopcua#869 â <see cref="DriverInstanceActor"/> asks an <see cref="IAlarmAcknowledgeConfirmation"/> driver to
 ///     confirm an ack AFTER <see cref="IAlarmSource.AcknowledgeAsync"/> returned, off its mailbox, and reports
-///     <see cref="DriverInstanceActor.AlarmAckFailed"/> to the router when the confirmation fails — including one that
+///     <see cref="DriverInstanceActor.AlarmAckFailed"/> to the router when the confirmation fails â including one that
 ///     fails long after the ack handler returned, and one abandoned because the actor stopped.
 /// </summary>
 public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBase
@@ -35,6 +35,7 @@ public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBa
        var failed = router.ExpectMsg<DriverInstanceActor.AlarmAckFailed>(Timeout, cancellationToken: TestContext.Current.CancellationToken);
        failed.ConditionNodeId.ShouldBe("n1");
        failed.Reason.ShouldContain("BadTimeout");
+        driver.ForeignConfirmations.ShouldBe(0, "the confirmation must get the SAME request instance the ack was sent with");
    }

    /// <summary>
@@ -61,7 +62,7 @@ public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBa

    /// <summary>
    ///     A confirmation that succeeds reports nothing. Positive control: a later ack whose confirmation fails is
-    ///     reported, and it is the FIRST report the router receives — so none came for the confirmed one.
+    ///     reported, and it is the FIRST report the router receives â so none came for the confirmed one.
    /// </summary>
    [Fact]
    public void A_confirmed_ack_reports_nothing()
@@ -70,7 +71,9 @@ public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBa
        var actor = SpawnConnected(driver);
        var router = CreateTestProbe();

-        driver.Confirmation = () => Task.Delay(20);
+        // Already complete: the actor's confirmation task finishes (and would report) right after the call that
+        // bumps ConfirmCalls, long before the second ack below makes its actor round trip.
+        driver.Confirmation = () => Task.CompletedTask;
        actor.Tell(Ack("n-ok"), router.Ref);
        AwaitCondition(() => driver.ConfirmCalls == 1, Timeout, TestContext.Current.CancellationToken);

@@ -96,7 +99,12 @@ public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBa
    {
        private int _ackCalls;
        private int _confirmCalls;
-        private readonly ConcurrentQueue<AlarmAcknowledgeRequest> _acks = new();
+        private readonly ConcurrentQueue<IReadOnlyList<AlarmAcknowledgeRequest>> _ackedLists = new();
+        private int _foreignConfirmations;
+
+        /// <summary>Confirmations asked for a request list that was NOT the instance passed to AcknowledgeAsync —
+        /// the contract a driver matching by instance (Galaxy) relies on.</summary>
+        public int ForeignConfirmations => Volatile.Read(ref _foreignConfirmations);

        public Func<Task>? Confirmation { get; set; }

@@ -126,7 +134,7 @@ public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBa
        public Task AcknowledgeAsync(
            IReadOnlyList<AlarmAcknowledgeRequest> acknowledgements, CancellationToken cancellationToken)
        {
-            foreach (var a in acknowledgements) _acks.Enqueue(a);
+            _ackedLists.Enqueue(acknowledgements);
            Interlocked.Increment(ref _ackCalls);
            return Task.CompletedTask;
        }
@@ -134,6 +142,7 @@ public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBa
        public Task ConfirmAcknowledgedAsync(
            IReadOnlyList<AlarmAcknowledgeRequest> acknowledgements, CancellationToken cancellationToken)
        {
+            if (!_ackedLists.Any(l => ReferenceEquals(l, acknowledgements))) Interlocked.Increment(ref _foreignConfirmations);
            Interlocked.Increment(ref _confirmCalls);
            return (Confirmation?.Invoke() ?? Task.CompletedTask).WaitAsync(cancellationToken);
        }
@@ -606,6 +606,30 @@ public class NativeAlarmProjectorTests
        snapshot.ShouldBeNull();
    }

+    /// <summary>
+    ///     lmxopcua#869 — a transition that only CARRIES the optimistic ack through (a Clear: the alarm returned to
+    ///     normal while the ack was still being confirmed upstream) does not settle it: a late failure still reverts,
+    ///     to inactive + UNacked (Galaxy's UNACK_RTN). Positive control: an Acknowledge transition does settle it.
+    /// </summary>
+    [Fact]
+    public void A_carry_through_transition_keeps_the_local_ack_pending()
+    {
+        var sut = new NativeAlarmProjector();
+        sut.Project("n1", Evt(AlarmTransitionKind.Raise));
+        sut.MarkAcknowledged("n1");
+        sut.Project("n1", Evt(AlarmTransitionKind.Clear)).Acknowledged.ShouldBeTrue("the optimistic ack rides along");
+
+        sut.TryRevertAcknowledge("n1", out var reverted).ShouldBeTrue();
+        reverted!.Active.ShouldBeFalse();
+        reverted.Acknowledged.ShouldBeFalse();
+
+        sut.Project("n1", Evt(AlarmTransitionKind.Raise));
+        sut.MarkAcknowledged("n1");
+        sut.Project("n1", Evt(AlarmTransitionKind.Raise)).Acknowledged.ShouldBeTrue("a same-message Raise carries it too");
+        sut.Project("n1", Evt(AlarmTransitionKind.Acknowledge));
+        sut.TryRevertAcknowledge("n1", out _).ShouldBeFalse("an Acknowledge transition states the ack — authoritative");
+    }
+
    private static AlarmEventArgs Evt(
        AlarmTransitionKind kind,
        AlarmSeverity sev = AlarmSeverity.High,