From 83e9ffd27af6748dc24a24b7e8001195632da747 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 07:47:28 -0400 Subject: [PATCH] fix(alarms): SDK timed-unshelve timer is a no-op; commented re-ack comment is durable (#597) Review R1 M2: since #597 the SDK timer fires at real expiry, and OnTimedUnshelve routed an operator-style Unshelve by 'system' from both pair nodes, pre-empting the engine's AutoUnshelve. The engine owns expiry, so the callback now routes nothing. Integration test: an engine stand-in that acts on a routed Unshelve and expires late, plus a wrapped OnTimedUnshelve (positive control: the SDK timer fired). Review R1 m1: Part9StateMachine.ApplyAcknowledge records a commented re-ack as AddComment (persisted and replicated) instead of a NoOp; an uncommented re-ack is still a no-op. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/AlarmTracking.md | 42 ++++++++++------ docs/ScriptedAlarms.md | 7 +-- .../Part9StateMachine.cs | 12 ++++- .../OtOpcUaNodeManager.cs | 43 +++++++--------- .../Part9StateMachineTests.cs | 22 ++++++++ .../ConditionMethodEventTests.cs | 50 ++++++++++++++++--- .../AlarmCommandRouterTests.cs | 35 ++++++------- 7 files changed, 140 insertions(+), 71 deletions(-) diff --git a/docs/AlarmTracking.md b/docs/AlarmTracking.md index 85f6365c..445037d0 100644 --- a/docs/AlarmTracking.md +++ b/docs/AlarmTracking.md @@ -350,15 +350,16 @@ is client-visible. > (ScadaBridge included) that acks against the Secondary now gets a Bad result > instead of a Good that later reverts — so ack against the Primary (the higher > ServiceLevel). Scripted conditions are **not** gated: their engine runs on -> both nodes, so an ack on the Secondary is real. Residual: the published -> verdict changes only on a redundancy snapshot, so in the boot window of a -> multi-node cluster (no snapshot yet) the node still accepts, while -> `DriverHostActor`'s live check may drop the route — the old flap, limited to -> that window. +> both nodes, so an ack on the Secondary is real. The published verdict +> mirrors `DriverHostActor`'s live check from the moment the host starts, not +> only from the first redundancy snapshot: the host publishes it once at start +> and again whenever cluster membership changes, so in the boot window of a +> pair (role still unknown, a driver peer Up) the gate already denies, exactly +> as the route does (review R1 m2 — previously the view sat on its single-node +> "allow" seed until the first snapshot, reopening the flap for that window). > **Best-effort during reconnect.** If a native ack arrives while the owning -> driver is `Connecting`/`Reconnecting` (or, in the boot window above, on a -> node whose role is not yet known), it is dropped with a debug log and **not** +> driver is `Connecting`/`Reconnecting`, it is dropped with a debug log and **not** > retried upstream — the OPC UA Part 9 `AlarmConditionState` was already > committed locally at step 1, so the rare outcome is "acked locally but not > propagated to AVEVA." Re-issue the ack once the driver is reconnected if @@ -408,8 +409,10 @@ On allow, the handler publishes a `Commons.OpcUa.AlarmCommand` onto the action is a settable `Action` injected at boot by the hosted service. -`OnTimedUnshelve` (the SDK's automatic unshelve timer) bypasses the -operator gate — it is system-initiated. +`OnTimedUnshelve` (the SDK's automatic unshelve timer) is a **no-op** — it +routes nothing (review R1 M2). The engine owns timed-shelve expiry (its 5 s +shelving check records `AutoUnshelve` by `system`), and its re-projection +unshelves the node; see §"`UnshelveTime` is the real remaining time" below. ### One condition event per method call (Gitea #587) @@ -448,7 +451,14 @@ reports nothing. AddComment always reports (the comment is the change), and so does a re-ack **carrying a comment** (Gitea #597): the SDK writes that comment to the node, so it is reported once, with Acked/Confirmed unchanged. Before #597 the - comment landed on the node but no subscriber ever saw it. + comment landed on the node but no subscriber ever saw it. For a **scripted** + condition the comment is also durable (review R1 m1): the routed Acknowledge + reaches an engine that is already acknowledged, and + `Part9StateMachine.ApplyAcknowledge` records a commented re-ack exactly as an + AddComment (kind `AddComment`, the caller as user) — persisted and replicated + with the condition state, the original ack's `LastAck*` fields untouched. An + uncommented re-ack is still a no-op in the engine. The same applies to a + commented re-ack from the AdminUI `/alerts` page. - **A native re-ack is still routed to the driver** (deliberate, #597). The route is fire-and-forget, so a node showing Acked does not prove the upstream system acknowledged — an ack dropped during a reconnect leaves @@ -471,10 +481,14 @@ reports nothing. engine's projection carries its expiry (`AlarmConditionSnapshot.UnshelveAtUtc`), so `ShelvingState/UnshelveTime` counts down to the engine's expiry whichever path shelved it (the AdminUI `/alerts` timed shelve included). This arms the - SDK's own unshelve timer at the real instant; it changes no node state and - only routes an `Unshelve` (user `system`) back to the engine — a no-op if the - engine's 5 s expiry sweep got there first, otherwise it simply lands the - expiry on time. + SDK's own unshelve timer at the real instant, and `OnTimedUnshelve` — the + timer's callback — is deliberately a **no-op** (review R1 M2): it used to + route an operator-style `Unshelve` by `system`, which, firing before the + engine's 5 s expiry sweep, pre-empted the engine's `AutoUnshelve` on every + expiry, from both pair nodes, rewriting the audit trail as a manual unshelve. + The SDK's timer changes no shelving state itself, so the node stays + TimedShelved (with `UnshelveTime` at zero) for up to one sweep after expiry, + until the engine's re-projection reports the one expiry event. - **Accepted, not fixed — the SDK's `EventsMonitored` unit for late conditions** (Gitea #597 item 6, from the #586 review). The SDK's `AreEventsMonitored` counter never reaches a condition materialised under a folder that already had diff --git a/docs/ScriptedAlarms.md b/docs/ScriptedAlarms.md index 9b2c467c..b7b61e4f 100644 --- a/docs/ScriptedAlarms.md +++ b/docs/ScriptedAlarms.md @@ -118,7 +118,8 @@ rationale is recorded in the engine's class doc so it is not re-proposed: - `ApplyOneShotShelve` / `ApplyTimedShelve(unshelveAtUtc)` / `ApplyUnshelve` — shelving transitions. `Timed` requires `unshelveAtUtc > nowUtc`. - `ApplyEnable` / `ApplyDisable` — operator enable/disable. Disabled alarms ignore predicate results until re-enabled; on enable, `ActiveState` is re-derived from the next evaluation. - `ApplyAddComment(text)` — append-only audit entry, no state change. -- `ApplyShelvingCheck(nowUtc)` — called by the 5s timer; promotes expired `Timed` shelving to `Unshelved` with a `system / AutoUnshelve` audit entry. +- `ApplyShelvingCheck(nowUtc)` — called by the 5s timer; promotes expired `Timed` shelving to `Unshelved` with a `system / AutoUnshelve` audit entry. The sole owner of timed-shelve expiry — the OPC UA node's own SDK timer does nothing. +- `ApplyAcknowledge` on an already-acknowledged alarm is a no-op **unless it carries a comment**, which is recorded as an `AddComment` (emission `CommentAdded`; `LastAck*` untouched) — the OPC UA node reports a commented re-ack, so the system of record must hold the comment too. Two invariants the machine enforces: @@ -354,11 +355,11 @@ On allow, the handler publishes a `Commons.OpcUa.AlarmCommand` (containing comma - **Scripted conditions** — all eight Part 9 operations (including Enable/Disable) route through `AlarmCommandRouter` onto the `alarm-commands` topic, which `ScriptedAlarmHostActor` dispatches to the engine (`EnableAsync` / `DisableAsync` / `AcknowledgeAsync` / …). On enable, `ActiveState` is re-derived from the next predicate evaluation. - **Native (driver-fed) conditions** — `OnAcknowledge` branches to `NativeAlarmAckRouter` and routes the ack to the owning driver rather than the scripted engine (see §[Native-alarm OPC UA operator operations](#native-alarm-opc-ua-operator-operations) above). `OnEnableDisable`, `OnConfirm` and `OnShelve` return `BadNotSupported` immediately — a native condition supports Acknowledge and AddComment only (no engine-side surface, no upstream path, and the driver projection owns its Confirmed/Shelving; Gitea #587 review). -`OnTimedUnshelve` (the SDK's internal auto-unshelve timer) bypasses the client gate — it is system-initiated and not subject to operator role checks. +`OnTimedUnshelve` (the SDK's internal auto-unshelve timer, armed for the real remaining time since Gitea #597) is a **no-op**: the engine owns timed-shelve expiry via `ApplyShelvingCheck` (`system / AutoUnshelve`), and routing an `Unshelve` from the timer pre-empted that on every expiry as an operator-style unshelve from both pair nodes (review R1 M2). ### Delta-gate de-duplication -`WriteAlarmCondition` fires a Part 9 condition event only when the incoming state differs from the node's current live state. For an OPC UA method call (Acknowledge, Confirm, AddComment, the shelve methods, Enable/Disable) the SDK applies the new state to the node, and the node manager then reports **the one** condition event for it from the method path (Gitea #587 — the SDK reports none of its own, since `AutoReportStateChanges` stays false; the shelve methods' state is applied by the node manager, because the SDK's never applies it; a call that changed nothing — e.g. an uncommented re-ack — reports nothing; a re-ack carrying a comment reports that comment once, Gitea #597). Native (driver-fed) conditions support only Acknowledge and AddComment; their Confirm, shelve and Enable/Disable methods return `BadNotSupported`, and their Acknowledge is refused with `BadInvalidState` on a node that is not the redundancy Primary (Gitea #597). A timed shelve's `UnshelveTime` counts down to the engine's expiry (the projection carries `UnshelveAtUtc`). The engine's later re-projection of the same transition finds the node already in that state and is suppressed, so each accepted call yields exactly one event. See `docs/AlarmTracking.md` §"One condition event per method call". +`WriteAlarmCondition` fires a Part 9 condition event only when the incoming state differs from the node's current live state. For an OPC UA method call (Acknowledge, Confirm, AddComment, the shelve methods, Enable/Disable) the SDK applies the new state to the node, and the node manager then reports **the one** condition event for it from the method path (Gitea #587 — the SDK reports none of its own, since `AutoReportStateChanges` stays false; the shelve methods' state is applied by the node manager, because the SDK's never applies it; a call that changed nothing — e.g. an uncommented re-ack — reports nothing; a re-ack carrying a comment reports that comment once, Gitea #597, and the engine records it as an `AddComment` so it is persisted and replicated, review R1 m1). Native (driver-fed) conditions support only Acknowledge and AddComment; their Confirm, shelve and Enable/Disable methods return `BadNotSupported`, and their Acknowledge is refused with `BadInvalidState` on a node that is not the redundancy Primary (Gitea #597). A timed shelve's `UnshelveTime` counts down to the engine's expiry (the projection carries `UnshelveAtUtc`). The engine's later re-projection of the same transition finds the node already in that state and is suppressed, so each accepted call yields exactly one event. See `docs/AlarmTracking.md` §"One condition event per method call". ### AdminUI path diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/Part9StateMachine.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/Part9StateMachine.cs index 1de8baf3..e8498c1c 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/Part9StateMachine.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/Part9StateMachine.cs @@ -105,7 +105,17 @@ public static class Part9StateMachine throw new ArgumentException("User identity required for audit.", nameof(user)); if (current.Acked == AlarmAckedState.Acknowledged) - return TransitionResult.NoOp(current, "already acknowledged"); + { + // A COMMENTED re-ack carries a real comment even though nothing is acknowledged (Gitea #597 review + // m1): the OPC UA node reports it — the SDK writes the comment on a re-ack — so the system of + // record must hold it too, or the event advertises a comment that exists only on the node that + // served the call (never persisted, never replicated to the pair peer). Recorded exactly like + // AddComment; the ack fields (LastAck*) stay those of the real acknowledge. An uncommented re-ack + // is still a no-op. + return string.IsNullOrWhiteSpace(comment) + ? TransitionResult.NoOp(current, "already acknowledged") + : ApplyAddComment(current, user, comment, nowUtc); + } var audit = AppendComment(current.Comments, nowUtc, user, "Acknowledge", comment); var next = current with diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 986790e2..1152004b 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -874,8 +874,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 // (shelved, oneShot) flag pair. Timed shelving's expiry is owned by the engine; the time // passed here is the engine's remaining time, so the node's UnshelveTime counts down to the // real expiry (Gitea #597 — a 0 here reads as MaxTimeShelved, ~24.8 days). The SDK timer it - // arms only routes an Unshelve back to the engine (OnTimedUnshelve), a no-op once the - // engine's own expiry has run. + // arms does nothing (OnTimedUnshelve is a no-op): the engine's own expiry re-projects. condition.SetShelvingState( SystemContext, shelved: state.Shelving != AlarmShelvingKind.Unshelved, @@ -1375,25 +1374,17 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 : ("TimedShelve", DateTime.UtcNow + TimeSpan.FromMilliseconds(shelvingTime)); return HandleAlarmCommand(context, condition, operation, comment: null, unshelveAt); }; - // The auto-unshelve timer callback is SDK-initiated (the TimedShelve duration expired); the SDK - // fires it with the node manager's system context — there is NO session and NO user identity. - // Routing through HandleAlarmCommand would hit the AlarmAck gate and return BadUserAccessDenied, - // leaving the alarm permanently shelved. Instead, bypass the client gate, extract the AlarmId the - // same way HandleAlarmCommand does, and route an Unshelve command so the engine clears its shelve - // state. The manual-client Unshelve path goes through OnShelve(shelving:false) and stays gated. - alarm.OnTimedUnshelve = (context, condition) => - { - var alarmId = condition.NodeId.Identifier?.ToString() ?? string.Empty; - // User MUST be non-empty: the engine's Part9StateMachine.ApplyUnshelve rejects a - // null/whitespace user (ArgumentException "User required."), and that exception is swallowed - // downstream — an empty user would make the timed auto-unshelve silently no-op, so a - // TimedShelve would never auto-expire. There is no client principal on a system-timer - // unshelve, so label it the canonical engine-internal "system" user (matching the engine's - // own AutoUnshelve audit user and the codebase-wide "system" system-actor convention). The - // AlarmAck gate bypass is intentional and preserved — this is a session-less SDK timer. - AlarmCommandRouter?.Invoke(new AlarmCommand(alarmId, "Unshelve", "system", null, null)); - return ServiceResult.Good; - }; + // The SDK's auto-unshelve timer (armed by SetShelvingState for the real remaining time since Gitea + // #597) fires with a session-less system context. It is deliberately a NO-OP: the engine owns + // timed-shelve expiry (ScriptedAlarmEngine's shelving check → Part9StateMachine.ApplyShelvingCheck, + // recorded as "AutoUnshelve" by "system" with its "Timed shelving expired" comment), and its + // re-projection is what unshelves this node. Routing an operator-style "Unshelve" from here used to + // pre-empt that check on every expiry — from BOTH pair nodes, rewriting the audit trail as a manual + // unshelve — and a timer callback already queued when a re-shelve disposed it could unshelve the + // fresh shelve. The SDK's own OnTimerExpired changes no shelving state itself (it only refreshes + // UnshelveTime after this callback), so returning without routing leaves the node shelved until + // the engine's expiry lands. Native conditions are never timed-shelved (BadNotSupported above). + alarm.OnTimedUnshelve = (_, _) => ServiceResult.Good; // H4 — inbound Part 9 Enable/Disable (the condition type's built-in Enable/Disable methods route // here via this delegate). The engine handles Enable/Disable for SCRIPTED alarms @@ -1596,9 +1587,11 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 /// Only a real change is reported. The condition's gate-relevant state is read before and after /// the SDK handler, and nothing is reported when it did not move. The SDK accepts acknowledging an /// acknowledged condition and sets Confirmed = false again; a re-ack therefore also restores the - /// pre-call Confirmed and Retain, so a scripted node never drifts from its engine (which ignores a - /// re-ack) and the call is silent — unless it carried a comment, which the SDK writes and which is then - /// reported once like AddComment (Gitea #597). AddComment always reports: adding a comment is itself + /// pre-call Confirmed and Retain, so a scripted node never drifts from its engine (which ignores an + /// uncommented re-ack) and the call is silent — unless it carried a comment, which the SDK writes and + /// which is then reported once like AddComment (Gitea #597). The scripted engine records that comment + /// too (Part9StateMachine.ApplyAcknowledge treats a commented re-ack as AddComment), so the + /// reported comment is persisted and replicated, not node-local. AddComment always reports: adding a comment is itself /// the change. A NATIVE re-ack is still routed to the driver: the route is fire-and-forget, so a node /// showing Acked does not prove the upstream system acknowledged, and re-acking is the documented retry. /// @@ -1635,7 +1628,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 // The requested duration, so UnshelveTime counts down to the real expiry (Gitea #597 — 0 reads as // MaxTimeShelved, ~24.8 days). The scripted engine still owns the expiry and re-projects Unshelved // when it passes (with its own remaining time meanwhile — WriteAlarmCondition); the SDK timer this - // arms only routes an Unshelve back to it (OnTimedUnshelve), a no-op once the engine has expired it. + // arms does nothing (OnTimedUnshelve is a no-op — the engine's AutoUnshelve owns the expiry). timedShelve.OnCall = (context, method, objectId, shelvingTime) => ReportAfterMethod( () => timed(context, method, objectId, shelvingTime), () => alarm.SetShelvingState(SystemContext, shelved: true, oneShot: false, shelvingTime: shelvingTime)); diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/Part9StateMachineTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/Part9StateMachineTests.cs index 2ebe9fb4..dd8ea3b7 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/Part9StateMachineTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms.Tests/Part9StateMachineTests.cs @@ -79,6 +79,28 @@ public sealed class Part9StateMachineTests r.Emission.ShouldBe(EmissionKind.None); } + /// + /// Gitea #597 review m1 — a COMMENTED re-ack records its comment (as AddComment would) so the comment the + /// OPC UA event advertised is durable; the original acknowledge's ack fields are untouched. + /// + [Fact] + public void Commented_reack_records_the_comment_without_reacknowledging() + { + var active = Part9StateMachine.ApplyPredicate(Fresh(), true, T0.AddSeconds(1)).State; + var acked = Part9StateMachine.ApplyAcknowledge(active, "alice", "first look", T0.AddSeconds(2)).State; + + var r = Part9StateMachine.ApplyAcknowledge(acked, "bob", "still on it", T0.AddSeconds(3)); + + r.Emission.ShouldBe(EmissionKind.CommentAdded); + r.State.Comments.Count.ShouldBe(2); + r.State.Comments[1].User.ShouldBe("bob"); + r.State.Comments[1].Kind.ShouldBe("AddComment"); + r.State.Comments[1].Text.ShouldBe("still on it"); + r.State.LastAckUser.ShouldBe("alice"); + r.State.LastAckComment.ShouldBe("first look"); + r.State.LastAckUtc.ShouldBe(T0.AddSeconds(2)); + } + /// Verifies that acknowledging without a user throws ArgumentException. [Fact] public void Acknowledge_without_user_throws() diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ConditionMethodEventTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ConditionMethodEventTests.cs index a20c6c2c..d0ebede2 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ConditionMethodEventTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/ConditionMethodEventTests.cs @@ -263,14 +263,18 @@ public sealed class ConditionMethodEventTests /// /// A scripted TimedShelve expires: the engine's expiry comes back as an Unshelved re-projection, which - /// reports exactly one unshelve event after the one shelve event. + /// reports exactly one unshelve event after the one shelve event — and the SDK's own expiry timer routes + /// nothing (review R1 M2). /// /// /// The expiry is the engine's (computed from the command's UnshelveAtUtc). Since Gitea #597 the node - /// is shelved for the real duration, so the SDK's own timer fires at the same instant — but it only routes - /// an Unshelve back to the engine (ignored by this stand-in, a no-op on an unshelved engine alarm) and - /// changes no node state, so it adds no event. The stand-in engine here schedules its expiry from the real - /// UnshelveAtUtc the method handler computed. + /// is shelved for the real duration, so the SDK's own timer fires at that instant. It used to route an + /// operator-style Unshelve by system, which — because the real engine only checks expiry every + /// 5 s — pre-empted the engine's AutoUnshelve on every expiry. This stand-in engine therefore + /// (a) expires LATE, like the real one, so the SDK timer demonstrably fires first; and (b) ACTS on a routed + /// Unshelve as the real engine would, recording who unshelved it. The wrapped + /// OnTimedUnshelve is the positive control: the SDK timer did fire, and still nothing was routed and + /// the unshelve on record is the engine's own. /// [Fact] public async Task A_scripted_timed_shelve_expires_with_exactly_one_unshelve_event() @@ -283,21 +287,47 @@ public sealed class ConditionMethodEventTests rig.MaterialiseScripted(); var current = PreState("TimedShelve", native: false) with { Message = "base" }; Task? expiry = null; + var routed = new System.Collections.Concurrent.ConcurrentQueue(); + string? unshelvedBy = null; + var engineLag = TimeSpan.FromMilliseconds(1500); // the real engine's 5 s shelving-check granularity rig.Nm.AlarmCommandRouter = cmd => { + routed.Enqueue(cmd.Operation); + if (cmd.Operation == "Unshelve") + { + // What the real engine does with a routed Unshelve: an operator-style unshelve. + unshelvedBy ??= $"Unshelve:{cmd.User}"; + current = current with { Shelving = AlarmShelvingKind.Unshelved, UnshelveAtUtc = null }; + rig.ProjectLater(native: false, current); + return; + } if (cmd.Operation != "TimedShelve") return; - current = current with { Shelving = AlarmShelvingKind.Timed }; + // The real engine's projection carries its expiry, so the re-projection keeps the SDK timer armed + // for the real remaining time (without it, it would re-arm for MaxTimeShelved and never fire here). + current = current with { Shelving = AlarmShelvingKind.Timed, UnshelveAtUtc = cmd.UnshelveAtUtc }; rig.ProjectLater(native: false, current); var unshelveAt = cmd.UnshelveAtUtc!.Value; expiry = Task.Run(async () => { - var wait = unshelveAt - DateTime.UtcNow; + var wait = unshelveAt + engineLag - DateTime.UtcNow; if (wait > TimeSpan.Zero) await Task.Delay(wait); - current = current with { Shelving = AlarmShelvingKind.Unshelved }; + if (current.Shelving != AlarmShelvingKind.Timed) return; // ApplyShelvingCheck: Timed only + unshelvedBy ??= "AutoUnshelve:system"; + current = current with { Shelving = AlarmShelvingKind.Unshelved, UnshelveAtUtc = null }; rig.Project(native: false, current); }); }; + // Count the SDK's own expiry-timer firings without changing what the production handler does. + var condition = rig.Nm.TryGetAlarmCondition(ScriptedAlarmId)!; + var sdkTimerFired = 0; + var production = condition.OnTimedUnshelve!; + condition.OnTimedUnshelve = (context, c) => + { + Interlocked.Increment(ref sdkTimerFired); + return production(context, c); + }; + using var session = await rig.ConnectAsync(anonymous: false, ct); var events = new RecordedEvents(); var subscription = await RecordedEvents.SubscribeAsync(session, events, ct); @@ -315,6 +345,10 @@ public sealed class ConditionMethodEventTests rig.Project(false, current = current with { Message = "sentinel" }); await RecordedEvents.WaitUntilAsync(() => events.Has(item, "sentinel")); + Volatile.Read(ref sdkTimerFired).ShouldBeGreaterThanOrEqualTo(1, "the SDK's timed-unshelve timer fired"); + routed.ShouldNotContain("Unshelve", "the SDK timer must not route an operator-style Unshelve"); + unshelvedBy.ShouldBe("AutoUnshelve:system", "the engine's own expiry owns the unshelve"); + var shelving = events.ConditionEvents(item, "base").Select(e => e[RecordedEvents.ShelvingField].Value).ToList(); shelving.ShouldBe( [ObjectIds.ShelvedStateMachineType_Unshelved, ObjectIds.ShelvedStateMachineType_TimedShelved, diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AlarmCommandRouterTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AlarmCommandRouterTests.cs index 210c15c2..9972bd68 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AlarmCommandRouterTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AlarmCommandRouterTests.cs @@ -268,19 +268,15 @@ public sealed class AlarmCommandRouterTests : IDisposable await host.DisposeAsync(); } - /// OnTimedUnshelve fires with the SDK's system context (no session, no user identity) — - /// the real SDK path when a TimedShelve duration expires. The gate must NOT veto: the result must be - /// Good, the router must be invoked exactly once with Operation == "Unshelve", and no UnshelveAtUtc is - /// carried. - /// - /// Regression (OpcUaServer-007): the routed command's must be a - /// NON-EMPTY system user ("system"). The engine's Part9StateMachine.ApplyUnshelve rejects - /// a null/whitespace user with ArgumentException("User required."); that exception is swallowed - /// downstream, so an empty user made the timed auto-unshelve SILENTLY no-op — a TimedShelve would never - /// auto-expire. Asserting a non-empty user here pins the value the state machine requires. - /// + /// OnTimedUnshelve fires with the SDK's system context (no session, no user identity) when the + /// SDK's own TimedShelve timer expires. It must return Good and route NOTHING (review R1 M2, Gitea #597): + /// the scripted engine owns timed-shelve expiry (its shelving check records an "AutoUnshelve" by + /// "system"), and since #597 arms the SDK timer for the real remaining time, an "Unshelve" routed from + /// here pre-empted that check on every expiry, from both pair nodes, as an operator-style unshelve. + /// Positive control: on the same condition and router, a client Unshelve (OnShelve with + /// shelving:false) DOES route — so the empty capture is not a disconnected router. [Fact] - public async Task OnTimedUnshelve_with_system_context_returns_good_and_routes_unshelve() + public async Task OnTimedUnshelve_with_system_context_returns_good_and_routes_nothing() { var (host, server) = await BootAsync(); var nm = server.NodeManager!; @@ -299,14 +295,13 @@ public sealed class AlarmCommandRouterTests : IDisposable var result = condition.OnTimedUnshelve!(ctx, condition); result.ShouldBe(ServiceResult.Good); - captured.Count.ShouldBe(1); - var cmd = captured[0]; - cmd.AlarmId.ShouldBe("alm-tu"); // == ScriptedAlarmId / condition NodeId identifier - cmd.Operation.ShouldBe("Unshelve"); - // System-initiated (no client principal), but the user MUST be non-empty so ApplyUnshelve accepts it. - cmd.User.ShouldNotBeNullOrWhiteSpace(); - cmd.User.ShouldBe("system"); - cmd.UnshelveAtUtc.ShouldBeNull(); + captured.ShouldBeEmpty(); + + // Positive control: the router is live for a genuine operator Unshelve on the same condition. + var operatorCtx = SessionContext(server, "grace", OpcUaDataPlaneRoles.AlarmAck); + condition.OnShelve!(operatorCtx, condition, shelving: false, oneShot: false, shelvingTime: 0.0) + .ShouldBe(ServiceResult.Good); + captured.Select(c => c.Operation).ShouldBe(["Unshelve"]); await host.DisposeAsync(); }