From 6f4c7c3bf15f20537aab71d7c7faadcb80abd798 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 18 Oct 2026 13:11:01 -0400 Subject: [PATCH] Keep subtag acks and alarm reads safe on the managed ack path Review fixes for the opt-in managed GUID ack: - Subtag-mode GUID acks (forced subtag, or failover on the standby) stay on the consumer; their GUIDs are synthetic and only the subtag consumer can resolve them. Forced-subtag sessions no longer build a managed client. - A failed managed Initialize/Register/Subscribe drops back to the wnwrap ack path instead of failing the alarm subscription. - Drop stale crefs to a nonexistent Unverified member in WnalNativeAckConsumer. - Unit tests for routing, fallback, and disposal; document MXGATEWAY_USE_MANAGED_ALARM_ACK. Co-Authored-By: Claude Opus 5.5 --- docs/AlarmClientDiscovery.md | 22 +++ docs/GatewayConfiguration.md | 7 + .../MxAccess/AlarmDispatcherTests.cs | 144 ++++++++++++++++++ .../MxAccess/AlarmCommandHandler.cs | 6 +- .../MxAccess/AlarmDispatcher.cs | 37 ++++- .../MxAccess/WnalNativeAckConsumer.cs | 10 +- 6 files changed, 214 insertions(+), 12 deletions(-) diff --git a/docs/AlarmClientDiscovery.md b/docs/AlarmClientDiscovery.md index 6caa43f..7878f81 100644 --- a/docs/AlarmClientDiscovery.md +++ b/docs/AlarmClientDiscovery.md @@ -793,6 +793,28 @@ in the codebase for the forward-compat shape, but the gateway-side `AcknowledgeAlarmByName` when the public RPC supplies a recognizable `Provider!Group.Tag` reference. +**Opt-in managed GUID ack.** Setting `MXGATEWAY_USE_MANAGED_ALARM_ACK=1` +in the gateway's environment (workers inherit it) routes GUID-keyed acks +through `aaAlarmManagedClient.AlarmClient.AlarmAckByGUID` instead of the +wnwrap method. That managed method is implemented on this build and +carries operator domain and full name natively. The worker reaches it by +reflection (`AaManagedAlarmAckConsumer`) because the members are +non-public, and runs its own Initialize → Register → Subscribe lifecycle +against the alarm subscription expression. The path is deliberately +conservative: + +- If the DLL cannot load or the lifecycle returns a non-zero status, the + worker drops back to the wnwrap ack path; alarm reads never fail + because of it. +- Subtag-mode acks (forced subtag, or failover currently on the standby) + stay on the subtag consumer: their GUIDs are synthetic and only it can + resolve them. +- By-name acks stay on wnwrap — `AlarmClient` has no by-name ack. + +A zero return still means *accepted*, not *applied*; a secured +(`MxSecurityOperate`) alarm needs an authenticated operator. The skip-gated +`AlarmManagedGuidAckLiveSmokeTests` asserts the ack actually lands. + ### 5. STA / threading — resolved The wnwrap COM is `ThreadingModel=Apartment`. The consumer's diff --git a/docs/GatewayConfiguration.md b/docs/GatewayConfiguration.md index b80266e..4b0c0af 100644 --- a/docs/GatewayConfiguration.md +++ b/docs/GatewayConfiguration.md @@ -446,6 +446,13 @@ behavior. The alarm monitor is independent of client sessions: `AcknowledgeAlarm` and `StreamAlarms` are session-less RPCs served by the monitor. +GUID-keyed acknowledges can opt into AVEVA's managed alarm client by setting +the environment variable `MXGATEWAY_USE_MANAGED_ALARM_ACK=1` on the gateway +process (workers inherit it; under NSSM, add it to the service environment). +It is off by default and has no `MxGateway:*` option because it is still +awaiting live confirmation on the rig. See +[AlarmClientDiscovery](./AlarmClientDiscovery.md) ("Opt-in managed GUID ack"). + ### Alarm fallback options The `Fallback` sub-section controls how the alarm feed selects between the diff --git a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmDispatcherTests.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmDispatcherTests.cs index d41b42f..7c85796 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmDispatcherTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmDispatcherTests.cs @@ -203,6 +203,100 @@ public sealed class AlarmDispatcherTests Assert.Equal("Alice Smith", consumer.LastAckOperatorFullName); } + /// Verifies that a managed ack client takes GUID acks with full operator identity. + [Fact] + public void Acknowledge_WithManagedAckClient_RoutesGuidAckToManagedClient() + { + FakeAlarmConsumer consumer = new FakeAlarmConsumer { AcknowledgeReturn = -1 }; + FakeManagedAckClient managed = new FakeManagedAckClient { AcknowledgeReturn = 0 }; + using AlarmDispatcher dispatcher = new AlarmDispatcher( + consumer, + new MxAccessAlarmEventSink(new MxAccessEventQueue(), new MxAccessEventMapper()), + SessionId, + managed); + dispatcher.Subscribe(@"\\HOST\Galaxy!Area1"); + + Guid guid = Guid.NewGuid(); + int rc = dispatcher.Acknowledge(guid, "Acked", "alice", "WS01", "CORP", "Alice Smith"); + + Assert.Equal(0, rc); + Assert.Equal(@"\\HOST\Galaxy!Area1", managed.LastSubscription); + Assert.Equal(guid, managed.LastAckGuid); + Assert.Equal("CORP", managed.LastAckOperatorDomain); + Assert.Equal("Alice Smith", managed.LastAckOperatorFullName); + Assert.Equal(Guid.Empty, consumer.LastAckGuid); + } + + /// + /// Verifies that a failed managed-client lifecycle neither fails the alarm + /// subscription nor leaves acks routed to the broken client. + /// + [Fact] + public void Subscribe_WhenManagedAckClientFails_FallsBackToConsumerAck() + { + FakeAlarmConsumer consumer = new FakeAlarmConsumer { AcknowledgeReturn = 0 }; + FakeManagedAckClient managed = new FakeManagedAckClient { ThrowOnSubscribe = true }; + using AlarmDispatcher dispatcher = new AlarmDispatcher( + consumer, + new MxAccessAlarmEventSink(new MxAccessEventQueue(), new MxAccessEventMapper()), + SessionId, + managed); + + dispatcher.Subscribe(@"\\HOST\Galaxy!Area1"); + Guid guid = Guid.NewGuid(); + int rc = dispatcher.Acknowledge(guid, "Acked", "alice", "WS01", "CORP", "Alice Smith"); + + Assert.Equal(@"\\HOST\Galaxy!Area1", consumer.LastSubscription); + Assert.True(managed.Disposed); + Assert.Equal(0, rc); + Assert.Equal(guid, consumer.LastAckGuid); + Assert.Equal(Guid.Empty, managed.LastAckGuid); + } + + /// + /// Verifies that while the failover consumer serves the subtag standby, + /// GUID acks (synthetic GUIDs) stay on the consumer, not the managed client. + /// + [Fact] + public void Acknowledge_WhenFailoverInSubtagMode_KeepsGuidAckOnConsumer() + { + FakeAlarmConsumer primary = new FakeAlarmConsumer { ThrowOnSubscribe = true }; + FakeAlarmConsumer standby = new FakeAlarmConsumer { AcknowledgeReturn = 0 }; + FailoverAlarmConsumer failover = new FailoverAlarmConsumer( + primary, standby, new FailoverSettings(threshold: 1, probeIntervalSeconds: 0, stableProbes: 3)); + FakeManagedAckClient managed = new FakeManagedAckClient { AcknowledgeReturn = -1 }; + using AlarmDispatcher dispatcher = new AlarmDispatcher( + failover, + new MxAccessAlarmEventSink(new MxAccessEventQueue(), new MxAccessEventMapper()), + SessionId, + managed); + dispatcher.Subscribe(@"\\HOST\Galaxy!Area1"); + Assert.Equal(AlarmProviderMode.Subtag, failover.Mode); + + Guid guid = Guid.NewGuid(); + int rc = dispatcher.Acknowledge(guid, "Acked", "alice", "WS01", "CORP", "Alice Smith"); + + Assert.Equal(0, rc); + Assert.Equal(guid, standby.LastAckGuid); + Assert.Equal(Guid.Empty, managed.LastAckGuid); + } + + /// Verifies that disposing the dispatcher disposes the managed ack client. + [Fact] + public void Dispose_WithManagedAckClient_DisposesManagedClient() + { + FakeManagedAckClient managed = new FakeManagedAckClient(); + AlarmDispatcher dispatcher = new AlarmDispatcher( + new FakeAlarmConsumer(), + new MxAccessAlarmEventSink(new MxAccessEventQueue(), new MxAccessEventMapper()), + SessionId, + managed); + + dispatcher.Dispose(); + + Assert.True(managed.Disposed); + } + /// Verifies that acknowledge-by-name calls are forwarded to the consumer. [Fact] public void AcknowledgeByName_WhenInvoked_ForwardsToConsumerWithFullTuple() @@ -470,9 +564,13 @@ public sealed class AlarmDispatcherTests AlarmTransitionEmitted?.Invoke(this, transition); } + /// Gets or sets a value indicating whether throws. + public bool ThrowOnSubscribe { get; set; } + /// public void Subscribe(string subscription) { + if (ThrowOnSubscribe) throw new InvalidOperationException("subscribe failed"); LastSubscription = subscription; } @@ -533,4 +631,50 @@ public sealed class AlarmDispatcherTests Disposed = true; } } + + private sealed class FakeManagedAckClient : IManagedAlarmAckClient + { + /// Gets the last subscription expression. + public string? LastSubscription { get; private set; } + /// Gets the GUID from the last acknowledge call. + public Guid LastAckGuid { get; private set; } + /// Gets the operator domain from the last acknowledge call. + public string? LastAckOperatorDomain { get; private set; } + /// Gets the operator full name from the last acknowledge call. + public string? LastAckOperatorFullName { get; private set; } + /// Gets or sets the return code for acknowledge operations. + public int AcknowledgeReturn { get; set; } + /// Gets or sets a value indicating whether throws. + public bool ThrowOnSubscribe { get; set; } + /// Gets a value indicating whether this instance has been disposed. + public bool Disposed { get; private set; } + + /// + public void Subscribe(string subscription) + { + if (ThrowOnSubscribe) throw new InvalidOperationException("RegisterConsumer returned 5."); + LastSubscription = subscription; + } + + /// + public int AcknowledgeByGuid( + Guid alarmGuid, + string comment, + string operatorName, + string operatorNode, + string operatorDomain, + string operatorFullName) + { + LastAckGuid = alarmGuid; + LastAckOperatorDomain = operatorDomain; + LastAckOperatorFullName = operatorFullName; + return AcknowledgeReturn; + } + + /// + public void Dispose() + { + Disposed = true; + } + } } diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmCommandHandler.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmCommandHandler.cs index 8888e55..3b10631 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmCommandHandler.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmCommandHandler.cs @@ -182,7 +182,11 @@ public sealed class AlarmCommandHandler : IAlarmCommandHandler } // Optional managed ack client (opt-in via env); null → wnwrap ack. - IManagedAlarmAckClient? managedAck = managedAckFactory?.Invoke(); + // A forced-subtag session never has alarmmgr GUIDs to ack, so it + // skips the extra managed consumer entirely. + IManagedAlarmAckClient? managedAck = command.ForcedMode == AlarmProviderMode.Subtag + ? null + : managedAckFactory?.Invoke(); MxAccessAlarmEventSink sink = new MxAccessAlarmEventSink(eventQueue, mapper); dispatcher = new AlarmDispatcher(consumer, sink, subscribeSessionId, managedAck); try diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmDispatcher.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmDispatcher.cs index a76cdc4..388266e 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmDispatcher.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmDispatcher.cs @@ -36,7 +36,7 @@ public sealed class AlarmDispatcher : IDisposable private readonly MxAccessAlarmEventSink sink; private readonly string sessionId; private readonly EventHandler handler; - private readonly IManagedAlarmAckClient? managedAck; + private IManagedAlarmAckClient? managedAck; private bool disposed; /// Initializes a new alarm dispatcher for the given consumer, sink, and session ID. @@ -46,7 +46,9 @@ public sealed class AlarmDispatcher : IDisposable /// /// Optional managed ack client. When non-null, GUID-keyed acknowledges /// route to it (AVEVA's modern AlarmClient.AlarmAckByGUID) instead - /// of the wnwrap consumer; null keeps the existing wnwrap ack path. + /// of the wnwrap consumer while the alarmmgr provider is active; null + /// keeps the existing wnwrap ack path. Subtag-mode acks always stay on + /// the consumer because their GUIDs are synthetic. /// public AlarmDispatcher( IMxAccessAlarmConsumer consumer, @@ -80,10 +82,31 @@ public sealed class AlarmDispatcher : IDisposable consumer.Subscribe(subscription); // The managed ack client maintains its own registered/subscribed // consumer; run its lifecycle against the same expression so a later - // AlarmAckByGUID has a live consumer to act through. - managedAck?.Subscribe(subscription); + // AlarmAckByGUID has a live consumer to act through. It is an opt-in + // enhancement, so a failed lifecycle drops back to the wnwrap ack path + // rather than failing the alarm subscription. + if (managedAck is not null) + { + try + { + managedAck.Subscribe(subscription); + } + catch + { + try { managedAck.Dispose(); } catch { /* swallow */ } + managedAck = null; + } + } } + /// + /// True when the consumer is currently serving alarms from the subtag + /// fallback, whose alarm GUIDs are synthetic and unknown to alarmmgr. + /// + private bool IsSubtagActive() => + consumer is SubtagAlarmConsumer || + (consumer is FailoverAlarmConsumer failover && failover.Mode == AlarmProviderMode.Subtag); + /// /// Forward an AcknowledgeAlarm request to the underlying /// consumer's AlarmAckByGUID. Returns the AVEVA-native @@ -105,11 +128,13 @@ public sealed class AlarmDispatcher : IDisposable string ackOperatorFullName) { if (disposed) throw new ObjectDisposedException(nameof(AlarmDispatcher)); - if (managedAck is not null) + if (managedAck is not null && !IsSubtagActive()) { // Preferred path: AVEVA's modern AlarmClient.AlarmAckByGUID carries // operator domain + full name and is not E_NOTIMPL like the wnwrap - // COM AlarmAckByGUID. + // COM AlarmAckByGUID. Skipped in subtag mode: those GUIDs are + // synthetic and only the subtag consumer can resolve them (it acks + // via an AckMsg write). return managedAck.AcknowledgeByGuid( alarmGuid, ackComment, diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/WnalNativeAckConsumer.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/WnalNativeAckConsumer.cs index aaca704..b86daa9 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/WnalNativeAckConsumer.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/WnalNativeAckConsumer.cs @@ -27,10 +27,10 @@ namespace ZB.MOM.WW.MxGateway.Worker.MxAccess; /// , ) and the /// request-record layout are confirmed by disassembly (see /// docs/alarm-ack-findings.md in the reverse-engineering workspace). The -/// list-ACQUISITION exports in are declared from -/// their export names/ordinals but their argument signatures are NOT yet -/// reverse-engineered — DO NOT call them until the signatures are verified, -/// or the stdcall stack will be corrupted. +/// list-ACQUISITION side (obtaining hAlarmList + alarm handles) is +/// deliberately NOT implemented — see the note at the end of this class. +/// Nothing in the worker calls this type yet; it is kept for when a real +/// list handle can be supplied by a component that owns acquisition. /// public sealed class WnalNativeAckConsumer { @@ -101,7 +101,7 @@ public sealed class WnalNativeAckConsumer /// Build a request record and submit an ack for the given alarm /// handles via . Caller supplies a live /// and the handle(s) obtained from the - /// list's records (see — acquisition path). + /// list's records (acquisition is not implemented — see the class note). /// Returns the native status (0 == success). /// /// A registered, subscribed alarm-list handle.