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

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<HResultArgumentException>(() => writer.Acknowledge("T.A.AckMsg", "one", Alice()));
+        ArgumentException thrown = Assert.Throws<ArgumentException>(() => 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
     }
 }
 
-/// <summary>ArgumentException with an explicit HRESULT (the setter is protected on net48).</summary>
-internal sealed class HResultArgumentException : ArgumentException
-{
-    public HResultArgumentException(string message, int hresult)
-        : base(message)
-    {
-        HResult = hresult;
-    }
-}
-
 internal sealed class RecordingAckServer : IMxAccessServer
 {
     private int nextItemHandle = 100;

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<string> log = new List<string>();
 
+    // Item handles returned on the first registration, in the order they were handed out.
+    private readonly List<int> firstRegistrationHandles = new List<int>();
+
     /// <summary>Initializes a new instance of the <see cref="ItemHandleReuseProbeTests"/> class.</summary>
     /// <param name="output">Test output helper for logging.</param>
     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);

src/ZB.MOM.WW.MxGateway.Worker/MxAccess/AlarmAckWriter.cs

@@ -34,9 +34,6 @@ public sealed class AlarmAckWriter : IAlarmAckWriter
     /// </summary>
     public const int DefaultMaxCachedItems = 256;
 
-    /// <summary>E_INVALIDARG, which the interop raises as ArgumentException.</summary>
-    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<AckItem> itemLru = new LinkedList<AckItem>();
+
+    // 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<int> heldItemHandles = new HashSet<int>();
     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<AckItem>? 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<AckItem>? 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
     }
 
     /// <summary>
-    ///     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 <see cref="AckInteropException"/>. 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.
     /// </summary>
     /// <remarks>
-    ///     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 <c>AuthenticateUser</c> 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.
     /// </remarks>
+    private static T Interop<T>(string step, Func<T> 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<AckItem> 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<MxStatusProxy> rows = new RepeatedField<MxStatusProxy>();
         rows.AddRange(statusConverter.ConvertMany(pVars));
-        completionCache.Record(hLMXServerHandle, phItemHandle, rows);
+        RecordWriteCompletion(hLMXServerHandle, phItemHandle, rows);
+    }
+
+    /// <summary>
+    ///     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
+    ///     <see cref="DropItem"/> removed. Internal so tests can stand in for
+    ///     the COM event.
+    /// </summary>
+    internal void RecordWriteCompletion(int completionServerHandle, int itemHandle, RepeatedField<MxStatusProxy> rows)
+    {
+        if (server is null || completionServerHandle != serverHandle || !heldItemHandles.Contains(itemHandle))
+        {
+            return;
+        }
+
+        completionCache.Record(completionServerHandle, itemHandle, rows);
+    }
+
+    /// <summary>An MXAccess failure raised by one COM call, carrying that call's HRESULT.</summary>
+    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; }
     }
 
     /// <summary>A cached ack item. Plain class (not a record) for the net48 worker target.</summary>