From 4aea916c14351e38e1968f124df97c1ce2a840cb Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Wed, 29 Apr 2026 21:37:12 +0200 Subject: [PATCH] fix(agent): correct V2 HAADJ detection + drop duplicate snapshot ticks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two V2 bugs surfaced by Bayer session c4c8d206-… (HAADJ on Lenovo T14 Gen 5, Win11 23H2 22631.4317): * `IsHybridJoin=false` in the Sessions row though the Autopilot profile is `Win_Hybrid physical`. Root cause: `EnrollmentRegistryDetector.DetectHybridJoin` reads only the top-level `CloudAssignedDomainJoinMethod` registry value. On this device that value is persisted only inside the `PolicyJsonCache` JSON blob — `DeviceInfoCollector` already had the JSON fallback, the registration- time detector did not. Refactored into a pure `ResolveHybridJoinFromValues` helper with the same fallback ladder; full xUnit coverage incl. the real payload from the failing session. * Duplicate `performance_snapshot` events with timestamps 0–1 ms apart, the second one carrying a fresh-baseline net delta of 0. Root cause: System .Threading.Timer dispatches callbacks on different ThreadPool threads when a callback exceeds the period, and after the long stall the OS network re-init slowed `NetworkInterface.GetAllNetworkInterfaces()` enough to overlap the next 30 s tick → two parallel `Collect()` calls racing on shared baseline fields. Added an `Interlocked.CompareExchange` reentrancy gate in `CollectorBase.CollectSafe`; protects all three subclasses (Performance, AgentSelfMetrics, DeliveryOptimization) at once. xUnit coverage for the single-tick, parallel-tick-dropped, post-exception, and 16-thread stress cases. V2-only per phase-out policy. Full V2 suite green (812 + 14 new); pre-existing RecoveryPathTests parallel-flake confirmed unrelated. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../EnrollmentRegistryDetectorTests.cs | 131 +++++++++++++ .../Telemetry/CollectorBaseReentrancyTests.cs | 173 ++++++++++++++++++ .../Monitoring/Telemetry/CollectorBase.cs | 26 ++- .../Security/EnrollmentRegistryDetector.cs | 52 +++++- .../buildcounter.txt | 2 +- .../AutopilotMonitor.Agent/buildcounter.txt | 2 +- 6 files changed, 381 insertions(+), 5 deletions(-) create mode 100644 src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/EnrollmentRegistryDetectorTests.cs create mode 100644 src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Telemetry/CollectorBaseReentrancyTests.cs diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/EnrollmentRegistryDetectorTests.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/EnrollmentRegistryDetectorTests.cs new file mode 100644 index 00000000..c78a6aae --- /dev/null +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/EnrollmentRegistryDetectorTests.cs @@ -0,0 +1,131 @@ +using System; +using System.Collections.Generic; +using AutopilotMonitor.Agent.V2.Core.Security; +using Xunit; + +namespace AutopilotMonitor.Agent.V2.Core.Tests.Security +{ + /// + /// Regression coverage for the IsHybridJoin detection that runs at session-registration + /// time. Field evidence (session c4c8d206-…, Lenovo ThinkPad T14 Gen 5, Win11 23H2 + /// 22631.4317): the AutopilotPolicyCache key on this device exposes + /// CloudAssignedDomainJoinMethod only inside the embedded PolicyJsonCache + /// blob, not as a top-level registry value. Without the JSON fallback the agent registered + /// the session with IsHybridJoin=false even though the device was a HAADJ target, + /// while the later-running DeviceInfoCollector (which already had the fallback) + /// emitted autopilot_profile with isHybridJoin=true, producing a confusing + /// inconsistency in the Sessions table vs. the Events stream. + /// + public sealed class EnrollmentRegistryDetectorTests + { + private static Func Lookup(Dictionary values) + => name => values.TryGetValue(name, out var v) ? v : null!; + + [Fact] + public void TopLevel_DomainJoinMethod_one_returns_true() + { + var values = new Dictionary + { + ["CloudAssignedDomainJoinMethod"] = 1, + }; + Assert.True(EnrollmentRegistryDetector.ResolveHybridJoinFromValues(Lookup(values))); + } + + [Fact] + public void TopLevel_DomainJoinMethod_zero_returns_false_even_with_hybrid_in_json() + { + // Top-level value is authoritative: explicit "0" wins over a contradicting JSON + // blob — the JSON fallback only kicks in when the top-level is absent. + var values = new Dictionary + { + ["CloudAssignedDomainJoinMethod"] = 0, + ["PolicyJsonCache"] = "{\"CloudAssignedDomainJoinMethod\":1}", + }; + Assert.False(EnrollmentRegistryDetector.ResolveHybridJoinFromValues(Lookup(values))); + } + + [Fact] + public void Missing_TopLevel_falls_back_to_PolicyJsonCache_one_returns_true() + { + // Real-world payload from the Bayer Lenovo T14 Gen 5 / Win11 23H2 22631.4317 + // device that triggered this fix. Truncated for readability — the only relevant + // key is CloudAssignedDomainJoinMethod=1. + const string policyJson = + "{\r\n \"CloudAssignedTenantDomain\": \"bayergroup.onmicrosoft.com\",\r\n" + + " \"CloudAssignedDomainJoinMethod\": 1,\r\n" + + " \"AutopilotMode\": 2\r\n}"; + + var values = new Dictionary + { + ["PolicyJsonCache"] = policyJson, + }; + Assert.True(EnrollmentRegistryDetector.ResolveHybridJoinFromValues(Lookup(values))); + } + + [Fact] + public void Missing_TopLevel_falls_back_to_PolicyJsonCache_zero_returns_false() + { + var values = new Dictionary + { + ["PolicyJsonCache"] = "{\"CloudAssignedDomainJoinMethod\":0}", + }; + Assert.False(EnrollmentRegistryDetector.ResolveHybridJoinFromValues(Lookup(values))); + } + + [Fact] + public void Missing_TopLevel_and_PolicyJsonCache_without_key_returns_false() + { + var values = new Dictionary + { + ["PolicyJsonCache"] = "{\"CloudAssignedTenantDomain\":\"contoso.onmicrosoft.com\"}", + }; + Assert.False(EnrollmentRegistryDetector.ResolveHybridJoinFromValues(Lookup(values))); + } + + [Fact] + public void Missing_TopLevel_and_malformed_PolicyJsonCache_returns_false_without_throw() + { + var values = new Dictionary + { + ["PolicyJsonCache"] = "{not valid json", + }; + // Must not throw — the detector runs on the registration hot-path and a failure + // here would block session registration. + Assert.False(EnrollmentRegistryDetector.ResolveHybridJoinFromValues(Lookup(values))); + } + + [Fact] + public void Missing_TopLevel_and_empty_PolicyJsonCache_returns_false() + { + var values = new Dictionary + { + ["PolicyJsonCache"] = string.Empty, + }; + Assert.False(EnrollmentRegistryDetector.ResolveHybridJoinFromValues(Lookup(values))); + } + + [Fact] + public void All_values_missing_returns_false() + { + var values = new Dictionary(); + Assert.False(EnrollmentRegistryDetector.ResolveHybridJoinFromValues(Lookup(values))); + } + + [Fact] + public void Null_lookup_returns_false() + { + Assert.False(EnrollmentRegistryDetector.ResolveHybridJoinFromValues(null)); + } + + [Fact] + public void DetectHybridJoin_returns_false_when_registry_key_is_unreadable() + { + // Smoke test against the real registry. On a developer machine the AutopilotPolicyCache + // key is typically absent so DetectHybridJoin must degrade to false without throwing. + // (CI / test boxes that DO have the key — vanishingly unlikely — would still observe a + // boolean, never an exception.) + var actual = EnrollmentRegistryDetector.DetectHybridJoin(); + Assert.IsType(actual); + } + } +} diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Telemetry/CollectorBaseReentrancyTests.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Telemetry/CollectorBaseReentrancyTests.cs new file mode 100644 index 00000000..e8733eec --- /dev/null +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Telemetry/CollectorBaseReentrancyTests.cs @@ -0,0 +1,173 @@ +using System; +using System.IO; +using System.Threading; +using System.Threading.Tasks; +using AutopilotMonitor.Agent.V2.Core.Logging; +using AutopilotMonitor.Agent.V2.Core.Monitoring.Telemetry; +using AutopilotMonitor.Agent.V2.Core.Orchestration; +using AutopilotMonitor.Agent.V2.Core.Tests.Harness; +using AutopilotMonitor.Agent.V2.Core.Tests.Orchestration; +using Xunit; + +namespace AutopilotMonitor.Agent.V2.Core.Tests.Telemetry +{ + /// + /// Regression coverage for the duplicate performance_snapshot events observed on + /// session c4c8d206-… (Bayer / Lenovo T14 Gen 5): two snapshots fired with timestamps + /// 0–1ms apart, the second one carrying a fresh-baseline net delta of 0. Root cause: + /// is documented to dispatch callbacks on different + /// ThreadPool threads when a callback exceeds the period, and the + /// PerformanceCollector's Collect() reads shared baseline fields without + /// any coordination. After the long stall the OS network re-init slowed + /// NetworkInterface.GetAllNetworkInterfaces() enough to overlap the next 30s tick + /// → two parallel Collect() calls → race. + /// + /// The fix lives in CollectorBase.CollectSafe: any tick that arrives while a + /// previous Collect() is still running is dropped via an + /// gate. These tests pin the + /// gate behaviour so a future "let's just remove the guard, it never fires" refactor + /// has a hard signal. + /// + /// + public sealed class CollectorBaseReentrancyTests + { + private static AgentLogger NewLogger(TempDirectory tmp) + => new AgentLogger(Path.Combine(tmp.Path, "logs"), AgentLogLevel.Debug); + + private static InformationalEventPost NewPost() + => new InformationalEventPost( + new FakeSignalIngressSink(), + new VirtualClock(new DateTime(2026, 4, 29, 17, 16, 0, DateTimeKind.Utc))); + + [Fact] + public void CollectSafe_runs_collect_when_no_previous_call_is_in_flight() + { + using var tmp = new TempDirectory(); + var collector = new BlockingCollector(NewPost(), NewLogger(tmp)); + + collector.ReleaseImmediately = true; + collector.CollectSafe(); + + Assert.Equal(1, collector.CollectInvocationCount); + } + + [Fact] + public async Task CollectSafe_skips_when_previous_collect_is_still_running() + { + using var tmp = new TempDirectory(); + var collector = new BlockingCollector(NewPost(), NewLogger(tmp)); + + // First call enters Collect() and parks on the gate, simulating a slow tick. + var firstTick = Task.Run(() => collector.CollectSafe()); + Assert.True(collector.WaitUntilInsideCollect(TimeSpan.FromSeconds(2)), + "First Collect did not enter within the timeout."); + + // Second tick fires while the first is still parked — must be dropped. + collector.CollectSafe(); + + // Release the first one and wait for it to finish. + collector.Release(); + await firstTick; + + Assert.Equal(1, collector.CollectInvocationCount); + } + + [Fact] + public void CollectSafe_resets_gate_after_collect_throws() + { + using var tmp = new TempDirectory(); + var collector = new BlockingCollector(NewPost(), NewLogger(tmp)); + + // First tick throws — base must catch and reset the gate so the next tick runs. + collector.ThrowOnNextCollect = true; + collector.ReleaseImmediately = true; + collector.CollectSafe(); + Assert.Equal(1, collector.CollectInvocationCount); + + // Second tick must execute; the gate is no longer locked. + collector.CollectSafe(); + Assert.Equal(2, collector.CollectInvocationCount); + } + + [Fact] + public async Task Concurrent_CollectSafe_calls_serialize_through_the_gate() + { + // Stress the Interlocked gate: many parallel ticks against a slow Collect must + // produce exactly one Collect invocation while the slow one is in flight. + using var tmp = new TempDirectory(); + var collector = new BlockingCollector(NewPost(), NewLogger(tmp)); + + var firstTick = Task.Run(() => collector.CollectSafe()); + Assert.True(collector.WaitUntilInsideCollect(TimeSpan.FromSeconds(2)), + "First Collect did not enter within the timeout."); + + // 16 concurrent racing ticks — none of them should manage to enter Collect. + var racers = new Task[16]; + for (int i = 0; i < racers.Length; i++) + racers[i] = Task.Run(() => collector.CollectSafe()); + await Task.WhenAll(racers); + + collector.Release(); + await firstTick; + + Assert.Equal(1, collector.CollectInvocationCount); + + // After the slow one finished the gate is open again — next tick runs normally. + collector.ReleaseImmediately = true; + collector.CollectSafe(); + Assert.Equal(2, collector.CollectInvocationCount); + } + + /// + /// Test-only collector subclass: signals when is entered and + /// blocks until the test releases it, so we can exercise the reentrancy gate + /// deterministically without relying on real timer scheduling. + /// + private sealed class BlockingCollector : CollectorBase + { + private readonly ManualResetEventSlim _insideCollect = new ManualResetEventSlim(false); + private readonly ManualResetEventSlim _release = new ManualResetEventSlim(false); + private int _collectInvocationCount; + + public BlockingCollector(InformationalEventPost post, AgentLogger logger) + : base( + sessionId: Guid.NewGuid().ToString(), + tenantId: Guid.NewGuid().ToString(), + post: post, + logger: logger, + intervalSeconds: 30) + { + } + + public int CollectInvocationCount => Volatile.Read(ref _collectInvocationCount); + + /// When true, returns immediately. + public bool ReleaseImmediately { get; set; } + + /// One-shot: next call throws. + public bool ThrowOnNextCollect { get; set; } + + public bool WaitUntilInsideCollect(TimeSpan timeout) => _insideCollect.Wait(timeout); + + public void Release() => _release.Set(); + + protected override void Collect() + { + Interlocked.Increment(ref _collectInvocationCount); + _insideCollect.Set(); + + if (ThrowOnNextCollect) + { + ThrowOnNextCollect = false; + throw new InvalidOperationException("simulated collect failure"); + } + + if (ReleaseImmediately) return; + + _release.Wait(TimeSpan.FromSeconds(5)); + _release.Reset(); + _insideCollect.Reset(); + } + } + } +} diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Telemetry/CollectorBase.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Telemetry/CollectorBase.cs index 05574f0c..be8d594a 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Telemetry/CollectorBase.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Telemetry/CollectorBase.cs @@ -26,6 +26,14 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Telemetry private readonly int _intervalSeconds; private Timer _timer; private bool _disposed; + // 0 = idle, 1 = a Collect call is in flight. System.Threading.Timer is documented to + // fire callbacks on different ThreadPool threads when the previous callback exceeds + // the period — observed in production on stall-recovery (network reinit makes the + // PerformanceCollector's NetworkInterface.GetAllNetworkInterfaces() call slow enough + // to overlap the next 30s tick), producing two parallel Collect calls that race on + // the collector's shared baseline state and emit duplicate snapshots. Using + // Interlocked here keeps the sample interval honest and prevents ghost samples. + private int _collecting; protected CollectorBase( string sessionId, @@ -96,10 +104,26 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Telemetry _timer.Change(TimeSpan.Zero, interval); } - private void CollectSafe() + /// + /// Reentrancy-guarded entry point invoked by . internal so the + /// V2 test assembly (via InternalsVisibleTo) can drive the guard directly without + /// having to spin up a real timer + wait on real wall-clock time. + /// + internal void CollectSafe() { + // Skip the tick if a previous Collect is still running. The Timer keeps firing in + // the background, so the next eligible tick will pick up where this one declined — + // the sample cadence stays aligned with the configured interval rather than the + // duration of any single slow Collect. + if (Interlocked.CompareExchange(ref _collecting, 1, 0) != 0) + { + Logger.Debug($"[{GetType().Name}] tick skipped — previous Collect still running"); + return; + } + try { Collect(); } catch (Exception ex) { Logger.Error($"[{GetType().Name}] Collection error: {ex.Message}", ex); } + finally { Interlocked.Exchange(ref _collecting, 0); } } } } diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Security/EnrollmentRegistryDetector.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Security/EnrollmentRegistryDetector.cs index 39eff4f7..804e33cf 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core/Security/EnrollmentRegistryDetector.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Security/EnrollmentRegistryDetector.cs @@ -22,6 +22,15 @@ namespace AutopilotMonitor.Agent.V2.Core.Security /// Reads CloudAssignedDomainJoinMethod from the Autopilot policy cache. Returns /// true when the profile was deployed with Hybrid Azure AD Join /// (CloudAssignedDomainJoinMethod == 1), false otherwise or on any error. + /// + /// On some Windows builds (observed on Win11 23H2 Lenovo / 22631.4317) the + /// CloudAssignedDomainJoinMethod value is not persisted as a top-level registry + /// value — it lives only inside the JSON blob stored under PolicyJsonCache. The + /// DeviceInfoCollector already handles this when emitting the + /// autopilot_profile event; this detector mirrors the same fallback so the + /// session is registered with the correct IsHybridJoin flag instead of a stale + /// false. + /// /// public static bool DetectHybridJoin() { @@ -30,8 +39,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Security using (var key = Registry.LocalMachine.OpenSubKey(AutopilotPolicyCacheKey)) { if (key == null) return false; - var domainJoinMethod = key.GetValue("CloudAssignedDomainJoinMethod")?.ToString(); - return domainJoinMethod == "1"; + return ResolveHybridJoinFromValues(key.GetValue); } } catch @@ -40,6 +48,46 @@ namespace AutopilotMonitor.Agent.V2.Core.Security } } + /// + /// Pure decision logic for . Exposed internally so the + /// fallback can be exercised without a real registry. is + /// expected to behave like — returns + /// null when the named value is absent. + /// + internal static bool ResolveHybridJoinFromValues(Func getValue) + { + if (getValue == null) return false; + + // Top-level value is authoritative when present (covers both "0" and "1"). + var topLevel = getValue("CloudAssignedDomainJoinMethod")?.ToString(); + if (topLevel != null) + { + return topLevel == "1"; + } + + // Fallback: on devices where the policy cache only carries the embedded JSON + // blob, look up the same key inside PolicyJsonCache. + var policyJson = getValue("PolicyJsonCache")?.ToString(); + if (string.IsNullOrWhiteSpace(policyJson)) return false; + + try + { + using (var doc = System.Text.Json.JsonDocument.Parse(policyJson)) + { + if (doc.RootElement.TryGetProperty("CloudAssignedDomainJoinMethod", out var prop)) + { + return prop.ToString() == "1"; + } + } + } + catch + { + // Malformed PolicyJsonCache → conservative non-HAADJ default. Detection is + // best-effort and must never throw at session-registration time. + } + return false; + } + /// /// Classifies the Autopilot flow based on the AutopilotSettings registry: /// diff --git a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt index de2a00c7..ccbd68fb 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt @@ -1 +1 @@ -457 +460 diff --git a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt index 44ba2fc6..1f8248ce 100644 --- a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt @@ -1 +1 @@ -1211 +1212