fix(agent-v2): force REG_NOTIFY_THREAD_AGNOSTIC + wait for in-flight callback on Stop

Two review findings on the ThreadPool-wait RegistryWatcher (993ebbaa):

P1 (correctness, would silently break in production): force
REG_NOTIFY_THREAD_AGNOSTIC into the effective filter regardless of the
caller's argument. Without a dedicated long-lived thread, the first
RegNotifyChangeKeyValue is issued on the transient Start() caller thread
and re-armed on rotating ThreadPool threads; without this flag the kernel
ties the registration to the issuing thread and cancels the notification
once it ends/recycles. AadJoinWatcher / ProvisioningStatusTracker /
RealmJoinWatcher all pass filters WITHOUT the flag — OR'ing it in the
ctor keeps them correct. (Win8+ only; all Autopilot devices are Win10+.)

P2 (semantic parity with the old Thread.Join): Stop() now issues a
blocking Unregister(waitObject) and waits for any in-flight OnSignalled
callback to finish BEFORE disposing _signal / _keyHandle, so Changed can
no longer fire (nor a freed handle be used) after Stop()/Dispose()
returns. The wait runs outside _sync so the callback's re-arm path can
still observe _stopped; it is deadlock-free because Stop() is never
called from within a handler (callers use RequestStop or queue Stop to
another thread). OnSignalled no longer nulls _registeredWait up front so
Stop can wait on the live registration.

Test: deterministic guard asserting the watcher forces ThreadAgnostic
into its effective filter for callers that omit it. A behavioural
thread-death test is unreliable — without the flag the kernel emits a
*spurious* signal on arming-thread death that a Changed-based test cannot
distinguish from a real change. Full V2.Core suite 1265/1265 green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/RegistryWatcherIntegrationTests.cs

@@ -164,5 +164,46 @@ namespace AutopilotMonitor.Agent.V2.Core.Tests.Security
 
             Assert.True(fireCount >= 2, $"expected ≥2 fire events, saw {fireCount}");
         }
+
+        [Fact]
+        public void Watcher_forces_ThreadAgnostic_even_when_caller_omits_it()
+        {
+            // Regression guard for REG_NOTIFY_THREAD_AGNOSTIC. In the ThreadPool-wait model the
+            // first RegNotifyChangeKeyValue is issued on the (transient) Start() caller thread and
+            // re-armed on rotating ThreadPool threads. Without this flag the kernel ties the
+            // registration to the issuing thread and silently cancels the notification once that
+            // thread ends/recycles — the watcher goes quiet in production. Several callers
+            // (AadJoinWatcher / ProvisioningStatusTracker / RealmJoinWatcher) omit it in their
+            // explicit filter, so the watcher MUST OR it in itself.
+            //
+            // Asserted on the effective filter rather than via a live thread-death test: without
+            // the flag the kernel emits a *spurious* signal when the arming thread dies, which a
+            // behavioural test cannot distinguish from a real change — so only the effective-filter
+            // invariant is a reliable guard. (RegChangeNotifyFilter is internal, so the cases are
+            // iterated in-body rather than via [Theory] parameters.)
+            var callerFilters = new[]
+            {
+                RegistryNativeMethods.RegChangeNotifyFilter.LastSet,
+                RegistryNativeMethods.RegChangeNotifyFilter.Name,
+                RegistryNativeMethods.RegChangeNotifyFilter.LastSet | RegistryNativeMethods.RegChangeNotifyFilter.Name,
+            };
+
+            foreach (var callerFilter in callerFilters)
+            {
+                using var watcher = new RegistryWatcher(
+                    hive: RegistryHive.CurrentUser,
+                    subKey: TestSubKey,
+                    watchSubtree: false,
+                    view: RegistryView.Default,
+                    filter: callerFilter, // deliberately WITHOUT ThreadAgnostic
+                    trace: null);
+
+                Assert.True(
+                    watcher.EffectiveFilter.HasFlag(RegistryNativeMethods.RegChangeNotifyFilter.ThreadAgnostic),
+                    $"RegistryWatcher must force REG_NOTIFY_THREAD_AGNOSTIC into its effective filter (caller passed {callerFilter})");
+                // The caller's requested bits must be preserved alongside the forced flag.
+                Assert.Equal(callerFilter, watcher.EffectiveFilter & callerFilter);
+            }
+        }
     }
 }

src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Runtime/RegistryWatcher.cs

@@ -67,7 +67,15 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
             _subKey = subKey ?? throw new ArgumentNullException(nameof(subKey));
             _watchSubtree = watchSubtree;
             _view = view;
-            _filter = filter;
+            // REG_NOTIFY_THREAD_AGNOSTIC (Win8+) is MANDATORY for the ThreadPool model and is
+            // forced on regardless of what the caller passed. Without a dedicated long-lived
+            // thread, RegNotifyChangeKeyValue is issued on the (transient) Start() caller thread
+            // and re-armed on rotating ThreadPool threads; without this flag the kernel ties the
+            // registration to the issuing thread and silently cancels the notification once that
+            // thread ends/recycles. All Autopilot devices are Win10+, so the flag is always
+            // available. Several callers (AadJoinWatcher / ProvisioningStatusTracker /
+            // RealmJoinWatcher) omit it in their explicit filter — OR'ing it here keeps them correct.
+            _filter = filter | RegistryNativeMethods.RegChangeNotifyFilter.ThreadAgnostic;
             _trace = trace;
         }
 
@@ -82,6 +90,14 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
             }
         }
 
+        /// <summary>
+        /// The effective notification filter actually passed to RegNotifyChangeKeyValue — the
+        /// caller's filter with REG_NOTIFY_THREAD_AGNOSTIC forced on. Exposed for tests that guard
+        /// the thread-agnostic invariant deterministically (kernel thread-death timing is unreliable
+        /// to assert against directly).
+        /// </summary>
+        internal RegistryNativeMethods.RegChangeNotifyFilter EffectiveFilter => _filter;
+
         public void Start()
         {
             ThrowIfDisposed();
@@ -137,6 +153,9 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
         /// </summary>
         public void Stop()
         {
+            RegisteredWaitHandle waitToUnregister;
+            AutoResetEvent signalToDispose;
+            SafeRegistryHandle keyToDispose;
             Exception backgroundException;
 
             lock (_sync)
@@ -147,19 +166,42 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
                 _running = false;
                 _stopped = true;
 
-                UnregisterWait();
+                waitToUnregister = _registeredWait;
-
+                _registeredWait = null;
-                try { _signal?.Set(); } catch { /* release any pending wait */ }
+                signalToDispose = _signal;
-                try { _signal?.Dispose(); } catch { /* dispose must not throw */ }
                 _signal = null;
-
+                keyToDispose = _keyHandle;
-                try { _keyHandle?.Dispose(); } catch { /* dispose must not throw */ }
                 _keyHandle = null;
 
                 backgroundException = _backgroundException;
                 _backgroundException = null;
             }
 
+            // Block until any in-flight OnSignalled callback has completed BEFORE disposing the
+            // handles it touches — restores the Thread.Join() guarantee of the old model so a
+            // Changed event can never fire (nor a freed handle be used) after Stop() returns.
+            // Unregister(waitObject) signals the event once all queued callbacks finish; the wait
+            // is done OUTSIDE _sync so the callback's re-arm path can still acquire the lock and
+            // observe _stopped. This blocking form is safe because Stop() is contractually never
+            // called from within a Changed handler — callers use RequestStop() or queue Stop()/
+            // Dispose() to another thread (AadJoinWatcher / RealmJoinWatcher do exactly this).
+            if (waitToUnregister != null)
+            {
+                using (var unregistered = new ManualResetEvent(false))
+                {
+                    try
+                    {
+                        if (waitToUnregister.Unregister(unregistered))
+                            unregistered.WaitOne();
+                    }
+                    catch { /* best-effort — never let teardown throw here */ }
+                }
+            }
+
+            try { signalToDispose?.Set(); } catch { /* release any pending wait */ }
+            try { signalToDispose?.Dispose(); } catch { /* dispose must not throw */ }
+            try { keyToDispose?.Dispose(); } catch { /* dispose must not throw */ }
+
             _trace?.Invoke("Stopped (cancellation requested)");
 
             if (backgroundException != null &&
@@ -242,12 +284,10 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
 
         private void OnSignalled(object state, bool timedOut)
         {
-            // Drop the (now-consumed, executeOnlyOnce) registration reference.
+            // NOTE: _registeredWait is deliberately NOT cleared here. Stop()/Dispose() needs the
-            lock (_sync)
+            // live RegisteredWaitHandle to issue a blocking Unregister(waitObject) and wait for
-            {
+            // THIS in-flight callback to finish before disposing _signal / _keyHandle. The handle
-                _registeredWait = null;
+            // is cleared+replaced by ArmNotify (re-arm path below) or by Stop().
-            }
-
             bool proceed;
             lock (_sync)
             {