From 5001e4069043d76851cc006c79f4817e70cfee61 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 13:19:52 -0500 Subject: [PATCH] =?UTF-8?q?fix(runtime):=20#869=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20a=20carry-through=20transition=20no=20longer=20sett?= =?UTF-8?q?les=20a=20pending=20local=20ack?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .claude/rules/alarms.md | 2 +- docs/AlarmTracking.md | 5 ++- docs/drivers/Galaxy.md | 4 +-- .../Drivers/NativeAlarmProjector.cs | 17 ++++++--- ...iverHostActorNativeAlarmAckRoutingTests.cs | 35 +++++++++++++++++++ ...DriverInstanceActorAckConfirmationTests.cs | 21 +++++++---- .../Drivers/NativeAlarmProjectorTests.cs | 24 +++++++++++++ 7 files changed, 93 insertions(+), 15 deletions(-) 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,