From b421c7ca9119ef9d3ee763f7b7dbcd475a463c65 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 11 Nov 2026 13:25:58 -0500 Subject: [PATCH] =?UTF-8?q?fix(runtime):=20#869=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20key=20a=20native-ack=20revert=20to=20the=20ack=20th?= =?UTF-8?q?at=20failed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pending local ack can outlive its own delivery (sources that send no Acknowledge back; a Clear now only carries it through), so a later failed re-ack — or a stale Galaxy confirmation timing out after the alarm re-occurred and was acked again — could revert an ack it does not own. MarkAcknowledged now issues a sequence (0 when the ack changed nothing), carried on RouteAlarmAck and echoed on AlarmAckFailed; TryRevertAcknowledge reverts only on a matching sequence. Claude-Session: https://claude.ai/code/session_01GT67AD99zQQqpLCVm3p7xk --- .claude/rules/alarms.md | 2 +- docs/AlarmTracking.md | 4 +- .../Drivers/DriverHostActor.cs | 18 ++++--- .../Drivers/DriverInstanceActor.cs | 10 ++-- .../Drivers/NativeAlarmProjector.cs | 50 ++++++++++++++++--- ...DriverInstanceActorAckConfirmationTests.cs | 19 +++++++ .../Drivers/NativeAlarmProjectorTests.cs | 26 ++++++++++ 7 files changed, 110 insertions(+), 19 deletions(-) diff --git a/.claude/rules/alarms.md b/.claude/rules/alarms.md index 5372086b1..7d70f2de3 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` (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 +`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; the revert is keyed to that ack's `MarkAcknowledged` sequence, echoed on `RouteAlarmAck`/`AlarmAckFailed`, so another ack's failure never undoes it). 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 e353a9551..6837ac6e3 100644 --- a/docs/AlarmTracking.md +++ b/docs/AlarmTracking.md @@ -477,7 +477,9 @@ from the scripted `AlarmCommandRouter`: 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 + through (a Clear, a same-message Raise) leaves it revertible (#869) — and + only by a failure of THAT acknowledge: the revert is keyed to the sequence + `MarkAcknowledged` issued, echoed on `RouteAlarmAck` / `AlarmAckFailed`. 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/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs index 3d58971c9..9a8d4b85a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs @@ -2406,7 +2406,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS // projector too, or the next driver Clear projects (Active=false, Acked=prev=false) and un-acknowledges it. // Most sources (MTConnect, FOCAS) send no Acknowledge transition back, and the projector now survives a // deploy rather than resetting to its acked default. - _nativeAlarmProjector.MarkAcknowledged(msg.ConditionNodeId); + var ackSequence = _nativeAlarmProjector.MarkAcknowledged(msg.ConditionNodeId); if (!_children.TryGetValue(target.DriverInstanceId, out var entry) || entry.Stubbed) { @@ -2417,7 +2417,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS var reason = entry is null ? "the owning driver is not running" : "the owning driver is stubbed on this node"; _log.Debug("DriverHost {Node}: driver {Driver} cannot take the alarm ack of {Cond} ({Reason}) — dropped", _localNode, target.DriverInstanceId, msg.ConditionNodeId, reason); - RevertNativeAlarmAck(msg.ConditionNodeId, reason); + RevertNativeAlarmAck(msg.ConditionNodeId, reason, ackSequence); return; } @@ -2426,7 +2426,8 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS // carried here) and reverts the node (HandleAlarmAckFailed). The driver correlates on ConditionId // (= the authored alarm FullName the inverse map keyed on). entry.Actor.Tell(new DriverInstanceActor.RouteAlarmAck( - target.RawPath, msg.Comment, msg.OperatorUser, msg.ApiKeyId, ConditionNodeId: msg.ConditionNodeId)); + target.RawPath, msg.Comment, msg.OperatorUser, msg.ApiKeyId, ConditionNodeId: msg.ConditionNodeId, + AckSequence: ackSequence)); } /// @@ -2437,7 +2438,7 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS /// /// The failure report from the driver child. private void HandleAlarmAckFailed(DriverInstanceActor.AlarmAckFailed msg) => - RevertNativeAlarmAck(msg.ConditionNodeId, msg.Reason); + RevertNativeAlarmAck(msg.ConditionNodeId, msg.Reason, msg.AckSequence); /// /// Re-projects as unacknowledged when its local acknowledge is still @@ -2446,12 +2447,15 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS /// /// The bare condition NodeId (Raw realm). /// Why the acknowledge did not reach the upstream alarm system. - private void RevertNativeAlarmAck(string conditionNodeId, string reason) + /// The failed acknowledge's projector sequence (lmxopcua#869). + private void RevertNativeAlarmAck(string conditionNodeId, string reason, long ackSequence) { - if (!_nativeAlarmProjector.TryRevertAcknowledge(conditionNodeId, out var snapshot) || snapshot is null) + // Keyed to the failed ack's own sequence (lmxopcua#869): a pending ack from an earlier or later acknowledge of + // the same condition is not this failure's to undo. + if (!_nativeAlarmProjector.TryRevertAcknowledge(conditionNodeId, ackSequence, out var snapshot) || snapshot is null) { _log.Info("DriverHost {Node}: native-alarm acknowledge of {Cond} did not reach the alarm system ({Reason}); " + - "the condition has since been re-stated by its driver, so it is left as projected", + "no acknowledge of THIS request is pending (re-stated by its driver since, superseded by another ack, or it changed nothing), so it is left as projected", _localNode, conditionNodeId, reason); return; } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs index 7be56023b..b7baf1949 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs @@ -90,8 +90,11 @@ public sealed class DriverInstanceActor : ReceiveActor, IWithTimers /// acknowledge that does not reach the upstream alarm system (refused, failed, or dropped because the driver /// is not connected) is reported back to the sender as an so the host can revert /// the locally-applied acknowledge (lmxopcua#760). Null ⇒ no report. + /// The host projector's sequence for this acknowledge (lmxopcua#869), echoed on + /// so only a failure of THIS ack reverts it; 0 ⇒ the ack changed nothing. public sealed record RouteAlarmAck( - string ConditionId, string? Comment, string OperatorUser, string? ApiKeyId = null, string? ConditionNodeId = null); + string ConditionId, string? Comment, string OperatorUser, string? ApiKeyId = null, string? ConditionNodeId = null, + long AckSequence = 0); /// /// Reply to a carrying a whose @@ -99,7 +102,8 @@ public sealed class DriverInstanceActor : ReceiveActor, IWithTimers /// /// The OPC UA condition NodeId the client acknowledged. /// Why the acknowledge did not go through; never contains a secret. - public sealed record AlarmAckFailed(string ConditionNodeId, string Reason); + /// The failed acknowledge's (lmxopcua#869). + public sealed record AlarmAckFailed(string ConditionNodeId, string Reason, long AckSequence = 0); public sealed record Subscribe(IReadOnlyList FullReferences, TimeSpan PublishingInterval); /// /// Sets the set of references this driver should keep subscribed for the lifetime of the @@ -1451,7 +1455,7 @@ public sealed class DriverInstanceActor : ReceiveActor, IWithTimers private static void ReportAckFailed(IActorRef replyTo, RouteAlarmAck msg, string reason) { if (string.IsNullOrEmpty(msg.ConditionNodeId) || replyTo.IsNobody()) return; - replyTo.Tell(new AlarmAckFailed(msg.ConditionNodeId, reason)); + replyTo.Tell(new AlarmAckFailed(msg.ConditionNodeId, reason, msg.AckSequence)); } private async Task HandleSubscribeAsync(Subscribe msg) 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 0c93ef5da..308c08beb 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/NativeAlarmProjector.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/NativeAlarmProjector.cs @@ -18,9 +18,13 @@ public sealed class NativeAlarmProjector /// /// Conditions a client acknowledged locally () whose acknowledge has not yet /// been confirmed or superseded by a driver transition — the only ones - /// may un-acknowledge (lmxopcua#760). + /// may un-acknowledge (lmxopcua#760) — keyed to the sequence issued, so only a + /// failure of THAT acknowledge reverts it (lmxopcua#869). /// - private readonly HashSet _pendingLocalAck = new(StringComparer.Ordinal); + private readonly Dictionary _pendingLocalAck = new(StringComparer.Ordinal); + + /// Last sequence issued by ; 0 is never issued. + private long _ackSequence; /// Project an alarm transition onto the full condition snapshot for . /// The materialised condition node's id (the projection's state key). @@ -180,12 +184,23 @@ public sealed class NativeAlarmProjector /// stale unacknowledged prior. An untracked condition already defaults to acknowledged. /// /// The acknowledged condition node id. - public void MarkAcknowledged(string nodeId) + /// + /// The sequence identifying this pending acknowledge — pass it back to + /// with its failure — or 0 when the + /// ack changed nothing (untracked, or already acknowledged), so its failure has nothing to undo. + /// + public long MarkAcknowledged(string nodeId) { - if (!_prior.TryGetValue(nodeId, out var p)) return; + if (!_prior.TryGetValue(nodeId, out var p)) return 0; // Only an ack that CHANGED the projected state is pending; re-acking an acked condition has nothing to undo. - if (!p.Acked) _pendingLocalAck.Add(nodeId); + long sequence = 0; + if (!p.Acked) + { + sequence = ++_ackSequence; + _pendingLocalAck[nodeId] = sequence; + } _prior[nodeId] = p with { Acked = true }; + return sequence; } /// @@ -198,10 +213,31 @@ public sealed class NativeAlarmProjector /// The condition node id whose acknowledge failed. /// The unacknowledged snapshot to project; null when nothing was reverted. /// True when the condition was reverted to unacknowledged. - public bool TryRevertAcknowledge(string nodeId, out AlarmConditionSnapshot? snapshot) + public bool TryRevertAcknowledge(string nodeId, out AlarmConditionSnapshot? snapshot) => + TryRevert(nodeId, ackSequence: null, out snapshot); + + /// + /// Like , but only when the pending + /// acknowledge is the one issued for + /// (lmxopcua#869). A pending ack can outlive its own delivery — a source that never sends an Acknowledge + /// transition back (FOCAS, MTConnect), across a Clear that only carried it through — so a failure of a LATER, + /// unrelated acknowledge (a re-ack while the driver is down) or of an EARLIER one (a Galaxy confirmation timing + /// out after the alarm re-occurred and was acked again) must not revert it. + /// + /// The condition node id whose acknowledge failed. + /// The sequence of the failed acknowledge; 0 never matches. + /// The unacknowledged snapshot to project; null when nothing was reverted. + /// True when the condition was reverted to unacknowledged. + public bool TryRevertAcknowledge(string nodeId, long ackSequence, out AlarmConditionSnapshot? snapshot) => + TryRevert(nodeId, ackSequence, out snapshot); + + private bool TryRevert(string nodeId, long? ackSequence, out AlarmConditionSnapshot? snapshot) { snapshot = null; - if (!_pendingLocalAck.Remove(nodeId) || !_prior.TryGetValue(nodeId, out var p)) return false; + if (!_pendingLocalAck.TryGetValue(nodeId, out var pending) + || (ackSequence is { } wanted && wanted != pending) + || !_prior.TryGetValue(nodeId, out var p)) return false; + _pendingLocalAck.Remove(nodeId); _prior[nodeId] = p with { Acked = false }; snapshot = new AlarmConditionSnapshot( Active: p.Active, Acknowledged: false, Confirmed: true, Enabled: true, 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 ef5b7f1d8..b29cb7126 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 @@ -83,6 +83,25 @@ public sealed class DriverInstanceActorAckConfirmationTests : RuntimeActorTestBa router.ExpectMsg(Timeout, cancellationToken: TestContext.Current.CancellationToken).ConditionNodeId.ShouldBe("n-bad"); } + /// + /// lmxopcua#869 review — the failure echoes the routed ack's , + /// which is what lets the host revert only the acknowledge that failed (keyed in + /// NativeAlarmProjector.TryRevertAcknowledge(nodeId, ackSequence, …)). + /// + [Fact] + public void A_failed_confirmation_echoes_the_ack_sequence() + { + var driver = new ConfirmingDriver(); + var actor = SpawnConnected(driver); + driver.Confirmation = () => Task.FromException(new DriverOperationRefusedException("unconfirmed")); + var router = CreateTestProbe(); + + actor.Tell(Ack("n-seq") with { AckSequence = 42 }, router.Ref); + + router.ExpectMsg(Timeout, cancellationToken: TestContext.Current.CancellationToken) + .AckSequence.ShouldBe(42L); + } + private static DriverInstanceActor.RouteAlarmAck Ack(string conditionNodeId) => new("Tank.Hi", "c", "alice", ConditionNodeId: conditionNodeId); 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 9e5f135c8..bcb413d86 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 @@ -630,6 +630,32 @@ public class NativeAlarmProjectorTests sut.TryRevertAcknowledge("n1", out _).ShouldBeFalse("an Acknowledge transition states the ack — authoritative"); } + /// + /// lmxopcua#869 review — a pending ack can outlive its own delivery (a source that sends no Acknowledge back, + /// across a Clear that only carried it), so a revert is keyed to the failed ack's sequence: a later re-ack that + /// changed nothing (sequence 0) or a stale earlier ack's failure does not undo it. Positive control: the + /// matching sequence does. + /// + [Fact] + public void A_revert_only_undoes_the_acknowledge_that_failed() + { + var sut = new NativeAlarmProjector(); + sut.Project("n1", Evt(AlarmTransitionKind.Raise)); + var first = sut.MarkAcknowledged("n1"); + first.ShouldBeGreaterThan(0); + sut.Project("n1", Evt(AlarmTransitionKind.Clear)); // first ack delivered; nothing settles it + + var reAck = sut.MarkAcknowledged("n1"); // a re-ack of the acked, cleared condition + reAck.ShouldBe(0L, "it changed nothing, so its failure has nothing to undo"); + sut.TryRevertAcknowledge("n1", reAck, out _).ShouldBeFalse("a later re-ack's failure must not revert the first"); + + sut.Project("n1", Evt(AlarmTransitionKind.Raise, msg: "again")); // a new occurrence (un-acks) + var second = sut.MarkAcknowledged("n1"); + sut.TryRevertAcknowledge("n1", first, out _).ShouldBeFalse("a stale earlier ack's failure must not revert the newer one"); + sut.TryRevertAcknowledge("n1", second, out var reverted).ShouldBeTrue(); + reverted!.Acknowledged.ShouldBeFalse(); + } + private static AlarmEventArgs Evt( AlarmTransitionKind kind, AlarmSeverity sev = AlarmSeverity.High,