diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/RegistryWatcherIntegrationTests.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/RegistryWatcherIntegrationTests.cs index ad04cf72..b6edb1d2 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/RegistryWatcherIntegrationTests.cs +++ b/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); + } + } } } diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Runtime/RegistryWatcher.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Runtime/RegistryWatcher.cs index 2c6bf07d..f5ce33e8 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Runtime/RegistryWatcher.cs +++ b/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 } } + /// + /// 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). + /// + internal RegistryNativeMethods.RegChangeNotifyFilter EffectiveFilter => _filter; + public void Start() { ThrowIfDisposed(); @@ -137,6 +153,9 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime /// 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(); - - try { _signal?.Set(); } catch { /* release any pending wait */ } - try { _signal?.Dispose(); } catch { /* dispose must not throw */ } + waitToUnregister = _registeredWait; + _registeredWait = null; + signalToDispose = _signal; _signal = null; - - try { _keyHandle?.Dispose(); } catch { /* dispose must not throw */ } + keyToDispose = _keyHandle; _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. - lock (_sync) - { - _registeredWait = null; - } - + // NOTE: _registeredWait is deliberately NOT cleared here. Stop()/Dispose() needs the + // live RegisteredWaitHandle to issue a blocking Unregister(waitObject) and wait for + // THIS in-flight callback to finish before disposing _signal / _keyHandle. The handle + // is cleared+replaced by ArmNotify (re-arm path below) or by Stop(). bool proceed; lock (_sync) {