From 463748e395fc2ce967c09d3ec52ad778392028df Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 20 Oct 2026 07:18:02 -0400 Subject: [PATCH] fix(worker): classify ack interop failures at the COM call; ignore late completions Address review of the #164-#166 fixes: - The HRESULT check could not tell interop from programming errors (a managed ArgumentException already defaults to 0x80070057). Wrap each MXAccess call (AuthenticateUser, AddItem, Advise, Write) in a narrow filter that converts an interop failure to a private marker exception, and catch only that marker, so exceptions from the writer's own code (argument checks, caching, the pump) propagate. - A late OnWriteComplete for a dropped handle recreated the cache entry that DropItem removed. The writer now records completions only for item handles it holds on its live registration. - The item-handle-reuse probe now asserts that handles within one registration strictly increase, so a rerun is a regression check. Refs #164, #165, #166 Claude-Session: https://claude.ai/code/session_016J6isfcgqy4SB2RK3ZvFvT --- docs/AlarmProbeFindings.md | 3 +- docs/Grpc.md | 2 +- docs/MxAccessWorkerInstanceDesign.md | 5 +- .../MxAccess/AlarmAckWriterTests.cs | 63 ++++----- .../Probes/ItemHandleReuseProbeTests.cs | 33 ++++- .../MxAccess/AlarmAckWriter.cs | 120 +++++++++++++----- 6 files changed, 145 insertions(+), 81 deletions(-) diff --git a/docs/AlarmProbeFindings.md b/docs/AlarmProbeFindings.md index 17a54f7..018ac09 100644 --- a/docs/AlarmProbeFindings.md +++ b/docs/AlarmProbeFindings.md @@ -505,7 +505,8 @@ empty password returned `PROTOCOL_STATUS_CODE_OK`, `hresult` 0. `.Acked` turned A wrong password, or a user Galaxy security does not know, makes `AuthenticateUser` throw `ArgumentException` (`E_INVALIDARG`, 0x80070057) rather than a `COMException`. -That is why the writer classifies interop failures by HRESULT across exception types. +That is why the writer treats `ArgumentException` from the MXAccess call itself as an +interop failure alongside `COMException`, and reports the HRESULT the exception carries. Such an ack returns `PROTOCOL_STATUS_CODE_MXACCESS_FAILURE`, the alarm stays unacked, the Historian gains no ack row, and the password string does not appear in the gateway log, `stdout.log`, or `stderr.log`. A non-Galaxy provider reference returns diff --git a/docs/Grpc.md b/docs/Grpc.md index 8873ed9..28d1cc4 100644 --- a/docs/Grpc.md +++ b/docs/Grpc.md @@ -115,7 +115,7 @@ The worker calls `AuthenticateUser` as the operator, then writes `operator_comme | `AuthenticateUser` failed with a server-gone HRESULT (`RPC_E_DISCONNECTED`, `RPC_S_SERVER_UNAVAILABLE`, `RPC_E_SERVERFAULT`); the writer tears down its MXAccess registration and re-registers on the next ack | `MXACCESS_FAILURE` | that HRESULT | | `AddItem`, `Advise` or `Write` on the `AckMsg` item raised an MXAccess error (`COMException`, or `ArgumentException` for `E_INVALIDARG`); the item is dropped and re-added on the next ack, and a server-gone HRESULT also tears down the registration | `MXACCESS_FAILURE` | the exception's HRESULT | | Write rejected (`OnWriteComplete` non-OK) | `MXACCESS_FAILURE` | the `detail` of the `OnWriteComplete` `MXSTATUS_PROXY`, or `-1` when `detail` is `0` | -| Any other exception escaping the ack (e.g. the ack writer's MXAccess `Register` failing, the monitor's worker session having no active alarm subscription — `InvalidOperationException` — or an argument error that is not the interop's `E_INVALIDARG`: `ArgumentNullException`, `ArgumentOutOfRangeException`, or an `ArgumentException` with another HRESULT, which signal a bug rather than an MXAccess failure) | `MXACCESS_FAILURE` | the exception's `HResult`; the diagnostic names only the exception type and HRESULT | +| Any other exception escaping the ack (e.g. the ack writer's MXAccess `Register` failing, the monitor's worker session having no active alarm subscription — `InvalidOperationException` — or an exception from the worker's own code rather than from an MXAccess call: interop failures are classified only when the `AuthenticateUser`, `AddItem`, `Advise` or `Write` call itself raises them, and an `ArgumentNullException` or `ArgumentOutOfRangeException` is never classified because the interop does not raise them for an HRESULT) | `MXACCESS_FAILURE` | the exception's `HResult`; the diagnostic names only the exception type and HRESULT | `reply.Statuses` carries the `OnWriteComplete` `MxStatusProxy` rows when they were observed. Because an unconfirmed write also returns `OK`, a client that must be certain should watch for the resulting transition on `StreamAlarms`. `AcknowledgeAlarm` stays under the `invoke:write` scope; see [Authorization](./Authorization.md). diff --git a/docs/MxAccessWorkerInstanceDesign.md b/docs/MxAccessWorkerInstanceDesign.md index 8da84e7..4124bb0 100644 --- a/docs/MxAccessWorkerInstanceDesign.md +++ b/docs/MxAccessWorkerInstanceDesign.md @@ -363,7 +363,10 @@ registration, which a live probe confirmed (see "Item handle reuse" in drop of an item (dirty re-add, eviction, or a failed `AddItem`/`Advise`/`Write`) also removes that handle's completion entry, and tearing down the registration removes every entry for its server handle, so the cache does not grow by one -entry per dropped handle for the life of the worker. +entry per dropped handle for the life of the worker. The writer records a +completion only for an item handle it currently holds on its live +registration, so a late `OnWriteComplete` for a dropped handle cannot recreate +its entry. ## Live alarm watch-list replacement diff --git a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmAckWriterTests.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmAckWriterTests.cs index 48960bc..8c7a14a 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmAckWriterTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/MxAccess/AlarmAckWriterTests.cs @@ -187,24 +187,6 @@ public sealed class AlarmAckWriterTests Assert.DoesNotContain("Unregister", server.Operations); } - [Fact] - public void Acknowledge_WhenAuthenticateUserThrowsArgumentExceptionWithEInvalidArg_ReturnsAuthenticationFailed() - { - // The HRESULT is what the narrowed filter keys on; set it explicitly so the - // test does not depend on ArgumentException's default HResult. - RecordingAckServer server = new RecordingAckServer - { - AuthenticateResult = 5, - AuthenticateException = new HResultArgumentException("bad credentials", unchecked((int)0x80070057)), - }; - using AlarmAckWriter writer = new AlarmAckWriter(server, new MxAccessWriteCompletionCache(), () => { }, TimeSpan.FromMilliseconds(10), "MxGateway"); - - AlarmAckResult result = writer.Acknowledge("T.A.AckMsg", "one", Alice()); - - Assert.Equal(AlarmAckOutcome.AuthenticationFailed, result.Outcome); - Assert.Equal(unchecked((int)0x80070057), result.NativeStatus); - } - [Fact] public void Acknowledge_WhenAuthenticateUserThrowsArgumentNullException_Propagates() { @@ -220,16 +202,17 @@ public sealed class AlarmAckWriterTests } [Fact] - public void Acknowledge_WhenAuthenticateUserThrowsArgumentExceptionWithOtherHResult_Propagates() + public void Acknowledge_WhenOwnCodeOutsideComCallThrowsArgumentException_Propagates() { - RecordingAckServer server = new RecordingAckServer - { - AuthenticateResult = 5, - AuthenticateException = new HResultArgumentException("not interop", unchecked((int)0x80131500)), - }; - using AlarmAckWriter writer = new AlarmAckWriter(server, new MxAccessWriteCompletionCache(), () => { }, TimeSpan.FromMilliseconds(10), "MxGateway"); + // The pump runs between Advise and Write. An ArgumentException from it carries + // the same default HRESULT (0x80070057) as an interop E_INVALIDARG, so only + // catching at the COM call itself keeps it from being reported as WriteRejected. + RecordingAckServer server = new RecordingAckServer { AuthenticateResult = 5 }; + using AlarmAckWriter writer = new AlarmAckWriter(server, new MxAccessWriteCompletionCache(), () => throw new ArgumentException("bug"), TimeSpan.FromMilliseconds(10), "MxGateway"); - Assert.Throws(() => writer.Acknowledge("T.A.AckMsg", "one", Alice())); + ArgumentException thrown = Assert.Throws(() => writer.Acknowledge("T.A.AckMsg", "one", Alice())); + Assert.Equal("bug", thrown.Message); + Assert.DoesNotContain(server.Operations, op => op.StartsWith("Write", StringComparison.Ordinal)); } [Fact] @@ -353,6 +336,24 @@ public sealed class AlarmAckWriterTests Assert.Equal(1UL, cache.CurrentVersion(7, 101)); } + [Fact] + public void LateCompletionForDroppedHandle_DoesNotRecreateCacheEntry() + { + RecordingAckServer server = new RecordingAckServer { AuthenticateResult = 5 }; + MxAccessWriteCompletionCache cache = new MxAccessWriteCompletionCache(); + using AlarmAckWriter writer = new AlarmAckWriter(server, cache, () => { }, TimeSpan.FromMilliseconds(20), "MxGateway"); + writer.Acknowledge("T.A.AckMsg", "one", Alice()); // handle 100, Unconfirmed + server.OnWrite = (handle, item) => writer.RecordWriteCompletion(handle, item, Rows(MxStatusCategory.Ok, 0)); + writer.Acknowledge("T.A.AckMsg", "two", Alice()); // drops 100, adds 101, applied + + writer.RecordWriteCompletion(7, 100, Rows(MxStatusCategory.Ok, 0)); // 100's late completion + writer.RecordWriteCompletion(8, 101, Rows(MxStatusCategory.Ok, 0)); // another registration + + Assert.Equal(1, cache.Count); + Assert.Equal(0UL, cache.CurrentVersion(7, 100)); + Assert.Equal(1UL, cache.CurrentVersion(7, 101)); + } + [Fact] public void Acknowledge_PastCacheCap_ForgetsEvictedHandleCompletion() { @@ -424,16 +425,6 @@ public sealed class AlarmAckWriterTests } } -/// ArgumentException with an explicit HRESULT (the setter is protected on net48). -internal sealed class HResultArgumentException : ArgumentException -{ - public HResultArgumentException(string message, int hresult) - : base(message) - { - HResult = hresult; - } -} - internal sealed class RecordingAckServer : IMxAccessServer { private int nextItemHandle = 100; diff --git a/src/ZB.MOM.WW.MxGateway.Worker.Tests/Probes/ItemHandleReuseProbeTests.cs b/src/ZB.MOM.WW.MxGateway.Worker.Tests/Probes/ItemHandleReuseProbeTests.cs index 9c294a7..baaa8aa 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker.Tests/Probes/ItemHandleReuseProbeTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker.Tests/Probes/ItemHandleReuseProbeTests.cs @@ -46,6 +46,9 @@ public sealed class ItemHandleReuseProbeTests private readonly ITestOutputHelper output; private readonly List log = new List(); + // Item handles returned on the first registration, in the order they were handed out. + private readonly List firstRegistrationHandles = new List(); + /// Initializes a new instance of the class. /// Test output helper for logging. public ItemHandleReuseProbeTests(ITestOutputHelper output) @@ -76,6 +79,16 @@ public sealed class ItemHandleReuseProbeTests { throw new InvalidOperationException("Probe failed: " + threadException.Message, threadException); } + + // The ack writer's drop-and-re-add mitigation depends on this: within one + // registration a handle number is never handed out twice. + Assert.Equal((2 * Rounds) + 5, firstRegistrationHandles.Count); + for (int i = 1; i < firstRegistrationHandles.Count; i++) + { + Assert.True( + firstRegistrationHandles[i] > firstRegistrationHandles[i - 1], + $"Item handle {firstRegistrationHandles[i]} did not increase past {firstRegistrationHandles[i - 1]}; MXAccess reused or reordered item handles within one registration."); + } } private void RunProbe() @@ -90,7 +103,7 @@ public sealed class ItemHandleReuseProbeTests // A: the AlarmAckWriter drop path (Advise, then UnAdvise + RemoveItem). for (int i = 0; i < Rounds; i++) { - int h = mx.AddItem(server, AckItem); + int h = Held(mx.AddItem(server, AckItem)); mx.Advise(server, h); Log($"A{i}: AddItem+Advise {AckItem} -> {h}"); mx.UnAdvise(server, h); @@ -101,28 +114,28 @@ public sealed class ItemHandleReuseProbeTests // B: RemoveItem without Advise. for (int i = 0; i < Rounds; i++) { - int h = mx.AddItem(server, AckItem); + int h = Held(mx.AddItem(server, AckItem)); Log($"B{i}: AddItem {AckItem} -> {h}"); mx.RemoveItem(server, h); Log($"B{i}: RemoveItem {h}"); } // C: interleave another item to see whether a freed number is handed to any item. - int c1 = mx.AddItem(server, AckItem); + int c1 = Held(mx.AddItem(server, AckItem)); Log($"C: AddItem {AckItem} -> {c1}"); mx.RemoveItem(server, c1); Log($"C: RemoveItem {c1}"); - int c2 = mx.AddItem(server, OtherItem); + int c2 = Held(mx.AddItem(server, OtherItem)); Log($"C: AddItem {OtherItem} -> {c2}"); - int c3 = mx.AddItem(server, AckItem); + int c3 = Held(mx.AddItem(server, AckItem)); Log($"C: AddItem {AckItem} -> {c3}"); mx.RemoveItem(server, c2); mx.RemoveItem(server, c3); Log($"C: RemoveItem {c2}, {c3}"); // D: the same item added twice while the first is still held. - int d1 = mx.AddItem(server, AckItem); - int d2 = mx.AddItem(server, AckItem); + int d1 = Held(mx.AddItem(server, AckItem)); + int d2 = Held(mx.AddItem(server, AckItem)); Log($"D: AddItem {AckItem} twice while held -> {d1}, {d2}"); mx.RemoveItem(server, d1); mx.RemoveItem(server, d2); @@ -159,6 +172,12 @@ public sealed class ItemHandleReuseProbeTests } } + private int Held(int itemHandle) + { + firstRegistrationHandles.Add(itemHandle); + return itemHandle; + } + private void Log(string line) { log.Add(DateTime.Now.ToString("HH:mm:ss.fff") + " " + line); diff --git a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmAckWriter.cs b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmAckWriter.cs index d6d774c..d79df70 100644 --- a/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmAckWriter.cs +++ b/src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmAckWriter.cs @@ -34,9 +34,6 @@ public sealed class AlarmAckWriter : IAlarmAckWriter /// public const int DefaultMaxCachedItems = 256; - /// E_INVALIDARG, which the interop raises as ArgumentException. - private const int EInvalidArg = unchecked((int)0x80070057); - private readonly IMxAccessComObjectFactory? factory; private readonly MxAccessWriteCompletionCache completionCache; private readonly Action pumpStep; @@ -48,6 +45,11 @@ public sealed class AlarmAckWriter : IAlarmAckWriter // Most-recently-used first; the tail is the eviction candidate. private readonly LinkedList itemLru = new LinkedList(); + + // Item handles currently added on this registration. A completion for any + // other handle (a dropped one whose OnWriteComplete arrives late) is ignored + // so it cannot recreate a completion-cache entry for a dead handle. + private readonly HashSet heldItemHandles = new HashSet(); private object? comObject; private LMXProxyServerClass? comEventSource; private IMxAccessServer? server; @@ -107,9 +109,6 @@ public sealed class AlarmAckWriter : IAlarmAckWriter if (string.IsNullOrWhiteSpace(itemAddress)) throw new ArgumentException("Ack item address is required.", nameof(itemAddress)); if (ackOperator is null) throw new ArgumentNullException(nameof(ackOperator)); - // Resolve every argument before the interop try blocks: their catch - // filters accept E_INVALIDARG ArgumentExceptions as MXAccess failures, so - // an argument error raised by this code must fail before reaching them. if (ackOperator.UserName is null) throw new ArgumentException("Operator user name is required.", nameof(ackOperator)); string verifyUser = ackOperator.VerifyUser; string password = ackOperator.Password ?? string.Empty; @@ -120,12 +119,10 @@ public sealed class AlarmAckWriter : IAlarmAckWriter int userId; try { - userId = mx.AuthenticateUser(serverHandle, verifyUser, password); + userId = Interop("AuthenticateUser", () => mx.AuthenticateUser(serverHandle, verifyUser, password)); } - catch (Exception ex) when (IsInteropFailure(ex)) + catch (AckInteropException ex) { - // Bad credentials surface as ArgumentException (E_INVALIDARG) from the - // interop, not only COMException, so classify by the carried HRESULT. int authHresult = ex.HResult; if (IsServerGone(authHresult)) { @@ -156,7 +153,6 @@ public sealed class AlarmAckWriter : IAlarmAckWriter int itemHandle = 0; LinkedListNode? node; bool cached = itemHandles.TryGetValue(itemAddress, out node); - string step = "AddItem"; try { if (cached && node!.Value.Dirty) @@ -185,23 +181,20 @@ public sealed class AlarmAckWriter : IAlarmAckWriter } else { - itemHandle = mx.AddItem(serverHandle, itemAddress); + itemHandle = Interop("AddItem", () => mx.AddItem(serverHandle, itemAddress)); node = CacheItem(mx, itemAddress, itemHandle); - step = "Advise"; + int added = itemHandle; // MXAccess rejects writes to added-but-unadvised items with E_INVALIDARG. - mx.Advise(serverHandle, itemHandle); + Interop("Advise", () => mx.Advise(serverHandle, added)); } pumpStep(); - step = "Write"; baselineVersion = completionCache.CurrentVersion(serverHandle, itemHandle); - mx.Write(serverHandle, itemHandle, commentValue, userId); + int target = itemHandle; + Interop("Write", () => mx.Write(serverHandle, target, commentValue, userId)); } - catch (Exception ex) when (IsInteropFailure(ex)) + catch (AckInteropException ex) { - // The interop maps E_INVALIDARG to ArgumentException (and - // E_ACCESSDENIED to UnauthorizedAccessException), so the HRESULT, not - // the exception type, is what identifies the MXAccess failure. int hresult = ex.HResult; if (itemHandles.TryGetValue(itemAddress, out LinkedListNode? failed)) { @@ -217,7 +210,7 @@ public sealed class AlarmAckWriter : IAlarmAckWriter return AlarmAckResult.Create( AlarmAckOutcome.WriteRejected, hresult, - $"AckMsg {step} failed (HRESULT 0x{hresult:X8})."); + $"AckMsg {ex.Step} failed (HRESULT 0x{hresult:X8})."); } ulong baseline = baselineVersion; @@ -262,28 +255,52 @@ public sealed class AlarmAckWriter : IAlarmAckWriter } /// - /// Exceptions the MXAccess COM interop raises for a failed HRESULT: the - /// runtime maps well-known HRESULTs to typed exceptions (E_INVALIDARG to - /// ArgumentException, E_ACCESSDENIED to UnauthorizedAccessException) and - /// the rest to COMException. Each carries the HRESULT in Exception.HResult. + /// Runs one MXAccess COM call and converts a failure the interop raised + /// for its HRESULT into . Only the COM + /// call itself runs inside the filter, so an exception thrown by this + /// writer's own code (argument checks, caching, the pump) propagates as + /// a bug instead of being reported as an MXAccess failure. /// /// - /// The ArgumentException arm exists because a wrong password or unknown + /// The runtime maps well-known HRESULTs to typed exceptions and the rest + /// to COMException; each carries the HRESULT in Exception.HResult. The + /// ArgumentException arm is needed because a wrong password or unknown /// user makes AuthenticateUser throw ArgumentException with - /// E_INVALIDARG (0x80070057), observed live on windev on 2026-10-19. It - /// is narrowed to that HRESULT, and excludes ArgumentNullException and - /// ArgumentOutOfRangeException, so a programming error inside the try - /// blocks propagates as a bug instead of being reported as an MXAccess - /// authentication or write failure. The interop never raises those two - /// subtypes for a COM HRESULT. + /// E_INVALIDARG (0x80070057), observed live on windev on 2026-10-19. + /// ArgumentNullException and ArgumentOutOfRangeException are excluded: + /// the interop does not raise them for an HRESULT, so one escaping the + /// call is an argument bug in the adapter, not an MXAccess failure. /// + private static T Interop(string step, Func call) + { + try + { + return call(); + } + catch (Exception ex) when (IsInteropFailure(ex)) + { + throw new AckInteropException(step, ex); + } + } + + private static void Interop(string step, Action call) + { + try + { + call(); + } + catch (Exception ex) when (IsInteropFailure(ex)) + { + throw new AckInteropException(step, ex); + } + } + private static bool IsInteropFailure(Exception ex) => ex is ExternalException || ex is UnauthorizedAccessException || (ex is ArgumentException && ex is not ArgumentNullException - && ex is not ArgumentOutOfRangeException - && ex.HResult == EInvalidArg); + && ex is not ArgumentOutOfRangeException); private static bool IsServerGone(int hresult) => hresult == unchecked((int)0x80010108) // RPC_E_DISCONNECTED @@ -315,6 +332,7 @@ public sealed class AlarmAckWriter : IAlarmAckWriter itemHandles.Clear(); itemLru.Clear(); + heldItemHandles.Clear(); server = null; comEventSource = null; object? toRelease = comObject; @@ -335,6 +353,7 @@ public sealed class AlarmAckWriter : IAlarmAckWriter LinkedListNode node = itemLru.AddFirst(new AckItem(itemAddress, itemHandle)); itemHandles[itemAddress] = node; + heldItemHandles.Add(itemHandle); return node; } @@ -343,6 +362,7 @@ public sealed class AlarmAckWriter : IAlarmAckWriter { itemHandles.Remove(node.Value.Address); itemLru.Remove(node); + heldItemHandles.Remove(node.Value.Handle); try { mx.UnAdvise(serverHandle, node.Value.Handle); } catch { /* best effort */ } try { mx.RemoveItem(serverHandle, node.Value.Handle); } catch { /* best effort */ } @@ -397,7 +417,37 @@ public sealed class AlarmAckWriter : IAlarmAckWriter { RepeatedField rows = new RepeatedField(); rows.AddRange(statusConverter.ConvertMany(pVars)); - completionCache.Record(hLMXServerHandle, phItemHandle, rows); + RecordWriteCompletion(hLMXServerHandle, phItemHandle, rows); + } + + /// + /// Records a write completion only for an item handle this writer + /// currently holds on its live registration. A late completion for a + /// dropped handle would otherwise recreate the cache entry that + /// removed. Internal so tests can stand in for + /// the COM event. + /// + internal void RecordWriteCompletion(int completionServerHandle, int itemHandle, RepeatedField rows) + { + if (server is null || completionServerHandle != serverHandle || !heldItemHandles.Contains(itemHandle)) + { + return; + } + + completionCache.Record(completionServerHandle, itemHandle, rows); + } + + /// An MXAccess failure raised by one COM call, carrying that call's HRESULT. + private sealed class AckInteropException : Exception + { + public AckInteropException(string step, Exception inner) + : base("MXAccess " + step + " failed.", inner) + { + Step = step; + HResult = inner.HResult; + } + + public string Step { get; } } /// A cached ack item. Plain class (not a record) for the net48 worker target.