diff --git a/.claude/rules/alarms.md b/.claude/rules/alarms.md index a4c527978..5372086b1 100644 --- a/.claude/rules/alarms.md +++ b/.claude/rules/alarms.md @@ -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). diff --git a/docs/AlarmTracking.md b/docs/AlarmTracking.md index a30b2bf98..e353a9551 100644 --- a/docs/AlarmTracking.md +++ b/docs/AlarmTracking.md @@ -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 diff --git a/docs/drivers/Galaxy.md b/docs/drivers/Galaxy.md index d7f70391b..a97a611f0 100644 --- a/docs/drivers/Galaxy.md +++ b/docs/drivers/Galaxy.md @@ -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) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/NativeAlarmProjector.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/NativeAlarmProjector.cs index 47f04db98..0c93ef5da 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/NativeAlarmProjector.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/NativeAlarmProjector.cs @@ -78,9 +78,6 @@ public sealed class NativeAlarmProjector /// The full Part 9 condition snapshot to write to the node. public AlarmConditionSnapshot Project(string nodeId, AlarmEventArgs e, out IReadOnlyList 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 /// /// 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 - /// is reverted: once any driver transition for the condition has been - /// projected, that transition is authoritative and a late failure changes nothing. + /// 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). /// /// The condition node id whose acknowledge failed. /// The unacknowledged snapshot to project; null when nothing was reverted. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorNativeAlarmAckRoutingTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorNativeAlarmAckRoutingTests.cs index b466ea2b7..455bf635c 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorNativeAlarmAckRoutingTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverHostActorNativeAlarmAckRoutingTests.cs @@ -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"); + } + + /// + /// 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. + /// + [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"); } /// diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorAckConfirmationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorAckConfirmationTests.cs index 8ec27749d..ef5b7f1d8 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorAckConfirmationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DriverInstanceActorAckConfirmationTests.cs @@ -9,9 +9,9 @@ using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness; namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers; /// -/// lmxopcua#869 — asks an driver to +/// lmxopcua#869 — asks an driver to /// confirm an ack AFTER returned, off its mailbox, and reports -/// to the router when the confirmation fails — including one that +/// to the router when the confirmation fails — including one that /// fails long after the ack handler returned, and one abandoned because the actor stopped. /// public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBase @@ -35,6 +35,7 @@ public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBa var failed = router.ExpectMsg(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"); } /// @@ -61,7 +62,7 @@ public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBa /// /// 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. /// [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 _acks = new(); + private readonly ConcurrentQueue> _ackedLists = new(); + private int _foreignConfirmations; + + /// Confirmations asked for a request list that was NOT the instance passed to AcknowledgeAsync — + /// the contract a driver matching by instance (Galaxy) relies on. + public int ForeignConfirmations => Volatile.Read(ref _foreignConfirmations); public Func? Confirmation { get; set; } @@ -126,7 +134,7 @@ public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBa public Task AcknowledgeAsync( IReadOnlyList 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 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); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/NativeAlarmProjectorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/NativeAlarmProjectorTests.cs index edb69b6d7..9e5f135c8 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/NativeAlarmProjectorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/NativeAlarmProjectorTests.cs @@ -606,6 +606,30 @@ public class NativeAlarmProjectorTests snapshot.ShouldBeNull(); } + /// + /// 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. + /// + [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,