fix(worker): StaWaitHelper regression tests + full-drain contract notes; consolidate wait P/Invoke

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

@@ -100,6 +100,20 @@ public sealed class MxAccessCommandExecutor : IStaCommandExecutor
         this.variantConverter = variantConverter ?? throw new ArgumentNullException(nameof(variantConverter));
         this.statusProxyConverter = new MxStatusProxyConverter();
         this.alarmCommandHandler = alarmCommandHandler;
+
+        // The no-op fallback is a TEST affordance only — never pass null on a
+        // real STA. The cache wait loops this pump step feeds
+        // (MxAccessValueCache.TryWaitForUpdate,
+        // MxAccessWriteCompletionCache.TryWaitForCompletion) block in
+        // StaWaitHelper.WaitForSignalOrMessages, whose contract is that the pump
+        // step drains the message queue to empty on every call: the wait wakes on
+        // input that is merely present, so a pump step that removes nothing makes
+        // every wait return instantly and turns the loop into a busy spin at 100%
+        // CPU until its deadline (deadline and reply shape still hold — it is a
+        // CPU fault, not a correctness one). It is harmless here only because the
+        // fakes that pass null pre-populate the caches and run on threads with no
+        // windows and no pending input, so there is never a message left behind
+        // for the wait to keep waking on.
         this.pumpStep = pumpStep ?? (static () => { });
         this.writeCompletionTimeout = writeCompletionTimeout ?? DefaultWriteCompletionTimeout;
     }

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

@@ -32,11 +32,26 @@ public sealed class MxAccessValueCache
     // the value is recorded from inside the waiter's own pumpStep (COM
     // dispatches OnDataChange on the STA), so the message wake is what does the
     // work there; this handle covers the cross-thread producers — the tests, and
-    // any future off-STA writer. Not IDisposable on purpose: the cache is
-    // co-owned by MxAccessBaseEventSink and MxAccessSession with no single
-    // teardown owner, and a late COM callback must never Set a disposed handle.
-    // One kernel event per session (one session per worker process), released by
-    // the handle's finalizer.
+    // any future off-STA writer.
+    //
+    // Single-waiter invariant: an AutoResetEvent releases exactly one waiter per
+    // Set. Today the sole waiter is the STA (one worker per session, and ReadBulk
+    // holds the apartment while it waits), so no wake is ever lost. A second
+    // concurrent waiter would not hang — the loser still re-checks the entry
+    // every fallback tick and still honors its deadline — it would just fall
+    // back to 50 ms latency for that iteration.
+    //
+    // NOT IDisposable, and that is one invariant with the wait's use of
+    // DangerousGetHandle: the cache is co-owned by MxAccessBaseEventSink and
+    // MxAccessSession with no single teardown owner, so a late COM callback
+    // could otherwise Set a disposed handle. StaWaitHelper deliberately does not
+    // DangerousAddRef the handle; GC.KeepAlive there closes the *finalization*
+    // race but not a dispose race — if this field were ever disposed while the
+    // STA sat inside the wait, the OS could recycle that handle value and the
+    // wait would silently attach to an unrelated object. Making these caches
+    // IDisposable is therefore not a free refactor: it must add DangerousAddRef/
+    // DangerousRelease around the wait in the same change. One kernel event per
+    // session (one session per worker process), released by the finalizer.
     private readonly AutoResetEvent updateSignal = new(false);
 
     /// <summary>Records a fresh OnDataChange payload for the given handle pair.</summary>

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

@@ -31,11 +31,26 @@ public sealed class MxAccessWriteCompletionCache
     // worker the completion is recorded from inside the waiter's own pumpStep
     // (COM dispatches OnWriteComplete on the STA), so the message wake is what
     // does the work there; this handle covers the cross-thread producers — the
-    // tests, and any future off-STA recorder. Not IDisposable on purpose: the
-    // cache is co-owned by MxAccessBaseEventSink and MxAccessSession with no
-    // single teardown owner, and a late COM callback must never Set a disposed
-    // handle. One kernel event per session (one session per worker process),
-    // released by the handle's finalizer.
+    // tests, and any future off-STA recorder.
+    //
+    // Single-waiter invariant: an AutoResetEvent releases exactly one waiter per
+    // Set. Today the sole waiter is the STA (one worker per session, and the
+    // write path holds the apartment while it waits), so no wake is ever lost. A
+    // second concurrent waiter would not hang — the loser still re-checks the
+    // entry every fallback tick and still honors its deadline — it would just
+    // fall back to 50 ms latency for that iteration.
+    //
+    // NOT IDisposable, and that is one invariant with the wait's use of
+    // DangerousGetHandle: the cache is co-owned by MxAccessBaseEventSink and
+    // MxAccessSession with no single teardown owner, so a late COM callback
+    // could otherwise Set a disposed handle. StaWaitHelper deliberately does not
+    // DangerousAddRef the handle; GC.KeepAlive there closes the *finalization*
+    // race but not a dispose race — if this field were ever disposed while the
+    // STA sat inside the wait, the OS could recycle that handle value and the
+    // wait would silently attach to an unrelated object. Making these caches
+    // IDisposable is therefore not a free refactor: it must add DangerousAddRef/
+    // DangerousRelease around the wait in the same change. One kernel event per
+    // session (one session per worker process), released by the finalizer.
     private readonly AutoResetEvent completionSignal = new(false);
 
     /// <summary>Records the status rows of a fresh OnWriteComplete callback for the given handle pair.</summary>

src/ZB.MOM.WW.MxGateway.Worker/Sta/StaMessagePump.cs

@@ -9,10 +9,7 @@ namespace ZB.MOM.WW.MxGateway.Worker.Sta;
 public sealed class StaMessagePump
 {
     private const uint Infinite = 0xFFFFFFFF;
-    private const uint MsgWaitFailed = 0xFFFFFFFF;
-    private const uint MwmoInputAvailable = 0x0004;
     private const uint PmRemove = 0x0001;
-    private const uint QsAllInput = 0x04FF;
 
     /// <summary>Waits for a command wake event or Windows messages, pumping any pending messages.</summary>
     /// <param name="commandWakeEvent">Event to signal when work is available.</param>
@@ -28,14 +25,14 @@ public sealed class StaMessagePump
 
         SafeWaitHandle safeHandle = commandWakeEvent.SafeWaitHandle;
         IntPtr[] handles = [safeHandle.DangerousGetHandle()];
-        uint result = MsgWaitForMultipleObjectsEx(
+        uint result = StaNativeMethods.MsgWaitForMultipleObjectsEx(
             (uint)handles.Length,
             handles,
             timeoutMilliseconds,
-            QsAllInput,
-            MwmoInputAvailable);
+            StaNativeMethods.QsAllInput,
+            StaNativeMethods.MwmoInputAvailable);
 
-        if (result == MsgWaitFailed)
+        if (result == StaNativeMethods.MsgWaitFailed)
         {
             throw new InvalidOperationException(
                 "The worker STA message pump failed while waiting for command work or Windows messages.");
@@ -75,14 +72,6 @@ public sealed class StaMessagePump
             : (uint)Math.Ceiling(timeout.TotalMilliseconds);
     }
 
-    [DllImport("user32.dll", SetLastError = true)]
-    private static extern uint MsgWaitForMultipleObjectsEx(
-        uint count,
-        IntPtr[] handles,
-        uint milliseconds,
-        uint wakeMask,
-        uint flags);
-
     [DllImport("user32.dll", SetLastError = true)]
     private static extern bool PeekMessage(
         out NativeMessage message,

src/ZB.MOM.WW.MxGateway.Worker/Sta/StaNativeMethods.cs

@@ -0,0 +1,58 @@
+using System;
+using System.Runtime.InteropServices;
+
+namespace ZB.MOM.WW.MxGateway.Worker.Sta;
+
+/// <summary>
+///     The single declaration of the Win32 wait the worker's STA is built on.
+///     Both waiters use it: <see cref="StaMessagePump"/> for the runtime's outer
+///     idle wait, and <see cref="StaWaitHelper"/> for the inner completion waits
+///     a command performs while it already holds the apartment.
+/// </summary>
+/// <remarks>
+///     Deliberately one copy. This is the subtlest Win32 contract in the worker —
+///     the wake mask decides which events can wake the STA, and
+///     <see cref="MwmoInputAvailable"/> decides whether a message already seen by
+///     a <c>PeekMessage</c> still counts as a wake. Two copies of it could drift
+///     apart silently, and a divergence would show up only as COM events
+///     arriving late on a live provider.
+/// </remarks>
+internal static class StaNativeMethods
+{
+    /// <summary>Return value of a failed wait (<c>WAIT_FAILED</c>).</summary>
+    internal const uint MsgWaitFailed = 0xFFFFFFFF;
+
+    /// <summary>
+    ///     Wake even when the pending input has already been seen (but not
+    ///     removed) by an earlier <c>PeekMessage</c>. Both callers peek before
+    ///     they wait, so without this flag a message that arrived during the peek
+    ///     would not wake the wait that follows it.
+    /// </summary>
+    internal const uint MwmoInputAvailable = 0x0004;
+
+    /// <summary>Wake on any queued input, including sent and posted messages.</summary>
+    internal const uint QsAllInput = 0x04FF;
+
+    /// <summary>
+    ///     Waits for the given handles, for queued input matching
+    ///     <paramref name="wakeMask"/>, or for the timeout — whichever comes
+    ///     first.
+    /// </summary>
+    /// <param name="count">Number of handles in <paramref name="handles"/>.</param>
+    /// <param name="handles">Handles to wait on; marshaled as a pointer to the array.</param>
+    /// <param name="milliseconds">Timeout in milliseconds; <c>0xFFFFFFFF</c> means INFINITE.</param>
+    /// <param name="wakeMask">Input types that may wake the wait (<see cref="QsAllInput"/>).</param>
+    /// <param name="flags">Wait flags (<see cref="MwmoInputAvailable"/>).</param>
+    /// <returns>
+    ///     <c>WAIT_OBJECT_0 + i</c> for a signaled handle, <c>WAIT_OBJECT_0 + count</c>
+    ///     for available input, <c>WAIT_TIMEOUT</c> (258), or
+    ///     <see cref="MsgWaitFailed"/>.
+    /// </returns>
+    [DllImport("user32.dll", SetLastError = true)]
+    internal static extern uint MsgWaitForMultipleObjectsEx(
+        uint count,
+        IntPtr[] handles,
+        uint milliseconds,
+        uint wakeMask,
+        uint flags);
+}

src/ZB.MOM.WW.MxGateway.Worker/Sta/StaWaitHelper.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Diagnostics;
 using System.Runtime.InteropServices;
 using System.Threading;
 using Microsoft.Win32.SafeHandles;
@@ -17,23 +18,22 @@ namespace ZB.MOM.WW.MxGateway.Worker.Sta;
 ///     Deliberately separate from <see cref="StaMessagePump"/> rather than a new
 ///     method on it: the two caches that use this are plain per-session objects
 ///     with no pump dependency (they take the pump step as an injected delegate
-///     so tests can drive them off the STA). Keeping the helper in this folder
-///     keeps every <c>user32</c> wait signature in the worker reviewable in one
-///     place; the declaration below mirrors the pump's exactly.
+///     so tests can drive them off the STA). The wait itself is not duplicated —
+///     both this and the pump call the single declaration in
+///     <see cref="StaNativeMethods"/>.
 /// </remarks>
 internal static class StaWaitHelper
 {
     /// <summary>
-    ///     Ceiling on the blind fallback tick. A caller's poll interval is
-    ///     clamped to this so no wait can go longer than 50 ms without calling
-    ///     its pump step, even when the process has no message queue to wake it
-    ///     (unit-test threads) and the wait handle is never signaled.
+    ///     Ceiling on the fallback tick — the longest a wait may block when
+    ///     nothing wakes it. A caller's poll interval is clamped to this, so a
+    ///     caller's pump step keeps running at least this often no matter what
+    ///     interval it asked for, even on a thread that never receives a message
+    ///     and whose signal is never set.
     /// </summary>
     internal const int MaxFallbackTickMilliseconds = 50;
 
-    private const uint MsgWaitFailed = 0xFFFFFFFF;
-    private const uint MwmoInputAvailable = 0x0004;
-    private const uint QsAllInput = 0x04FF;
+    private static int waitFailureReported;
 
     /// <summary>
     ///     Blocks the calling thread until <paramref name="signal"/> is set, a
@@ -42,6 +42,29 @@ internal static class StaWaitHelper
     ///     never blinds the thread to inbound COM event messages: the wait
     ///     returns the instant one arrives, so the caller can dispatch it.
     /// </summary>
+    /// <remarks>
+    ///     <para>
+    ///         <em>Caller contract: the pump step must drain the message queue
+    ///         to empty on every call.</em> The wait uses
+    ///         <see cref="StaNativeMethods.MwmoInputAvailable"/>, which reports
+    ///         input that is merely <em>present</em> — including input an earlier
+    ///         <c>PeekMessage</c> has already seen but not removed. A caller
+    ///         whose pump step drains only part of the queue, or does nothing at
+    ///         all, therefore leaves a message that satisfies the wake condition
+    ///         forever: every wait returns immediately, and the caller's poll
+    ///         loop degenerates into a busy loop that spins the thread at 100%
+    ///         until its deadline. That is a CPU fault, not a correctness one —
+    ///         the loop still honors its deadline and return shape — but it must
+    ///         not ship. Drain with <c>PeekMessage(..., PM_REMOVE)</c> in a loop
+    ///         until it returns false, exactly as
+    ///         <see cref="StaMessagePump.PumpPendingMessages"/> does.
+    ///     </para>
+    ///     <para>
+    ///         The wait does not <c>DangerousAddRef</c> the handle. That is
+    ///         sound only because the callers never dispose their wait handles —
+    ///         see the coupled invariant documented on the caches' signal fields.
+    ///     </para>
+    /// </remarks>
     /// <param name="signal">Wait handle set by whoever produces the awaited result.</param>
     /// <param name="timeoutMilliseconds">Maximum wait; values at or below zero return immediately.</param>
     internal static void WaitForSignalOrMessages(
@@ -60,20 +83,28 @@ internal static class StaWaitHelper
 
         SafeWaitHandle safeHandle = signal.SafeWaitHandle;
         IntPtr[] handles = [safeHandle.DangerousGetHandle()];
-        uint result = MsgWaitForMultipleObjectsEx(
+        uint result = StaNativeMethods.MsgWaitForMultipleObjectsEx(
             (uint)handles.Length,
             handles,
             (uint)timeoutMilliseconds,
-            QsAllInput,
-            MwmoInputAvailable);
+            StaNativeMethods.QsAllInput,
+            StaNativeMethods.MwmoInputAvailable);
+
+        // Read before anything else can run another P/Invoke and overwrite the
+        // thread's last-error slot.
+        int lastError = result == StaNativeMethods.MsgWaitFailed
+            ? Marshal.GetLastWin32Error()
+            : 0;
 
         // The handle is a field of a live caller object, so it cannot be
         // collected mid-call; KeepAlive states that dependency explicitly
         // rather than relying on it.
         GC.KeepAlive(signal);
 
-        if (result == MsgWaitFailed)
+        if (result == StaNativeMethods.MsgWaitFailed)
         {
+            ReportWaitFailureOnce(lastError);
+
             // Degrade to the pre-existing blind sleep instead of throwing or
             // hot-spinning: the caller is a bounded, deadline-driven poll loop,
             // and a failed wait must not turn a timed-out write into a fault.
@@ -108,11 +139,30 @@ internal static class StaWaitHelper
             : tick;
     }
 
-    [DllImport("user32.dll", SetLastError = true)]
-    private static extern uint MsgWaitForMultipleObjectsEx(
-        uint count,
-        IntPtr[] handles,
-        uint milliseconds,
-        uint wakeMask,
-        uint flags);
+    /// <summary>
+    ///     Traces the first wait failure of the process and stays silent after
+    ///     that.
+    /// </summary>
+    /// <remarks>
+    ///     The degrade path is silent by design — a failed wait still honors the
+    ///     caller's deadline — but a <em>permanently</em> failing wait means every
+    ///     completion wait has quietly reverted to the sleep-polled behavior this
+    ///     class exists to replace, and nothing else would ever say so. One trace
+    ///     makes that discoverable; latching it keeps a repeating failure from
+    ///     flooding the trace listener from inside a poll loop.
+    /// </remarks>
+    /// <param name="lastError">The Win32 error captured immediately after the failed wait.</param>
+    private static void ReportWaitFailureOnce(int lastError)
+    {
+        if (Interlocked.Exchange(ref waitFailureReported, 1) != 0)
+        {
+            return;
+        }
+
+        Trace.TraceWarning(
+            "MsgWaitForMultipleObjectsEx failed in the worker STA completion wait (Win32 error {0}). "
+            + "The wait has degraded to blind sleeping: deadlines are still honored, but inbound "
+            + "MXAccess COM events will only dispatch at poll-tick granularity. Reported once per process.",
+            lastError);
+    }
 }