From 8aeadb37288858512878370f9c013645dc81cb90 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Thu, 21 May 2026 19:49:45 +0200 Subject: [PATCH] =?UTF-8?q?fix(agent/v2,decision):=20RealmJoin=20code-revi?= =?UTF-8?q?ew=20findings=20=E2=80=94=20self-deadlock,=20Registry64,=20time?= =?UTF-8?q?out=20idempotency?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues surfaced in review of the parent-watch refactor. P1a — Watcher self-disposal deadlock in the appearance callback. Calling RegistryWatcher.Dispose() from the Changed handler re-entered Stop() → Thread.Join() on the same thread that owned the WatchLoop, hanging the watcher and skipping onAppeared(). Fix: RequestStop() (non-blocking) inside the callback, then ThreadPool.QueueUserWorkItem the actual Dispose so the join runs on a different thread after the loop has exited. P1b — All RealmJoin reads + watchers now explicitly bind to RegistryView.Registry64. The agent's intent is 64-bit, but the previous RegistryView.Default would still silently route HKLM\SOFTWARE through WOW6432Node if the host were ever spawned as a 32-bit process. Same defensive note already lives in TenantIdResolver. Affects RealmJoinInfo.cs (3 reads + enumerations), RealmJoinWatcher.KeyExists, and all four RegistryWatcher instantiations (1 parent-watch + 3 subtree-watches). P2 — Stale RealmJoinTimeout DeadlineFired after Resolved. Race: the cancel-effect for the timeout reaches the scheduler at the same time the deadline fires; the queued DeadlineFired signal is processed after RealmJoinResolved has already set Outcome="Resolved". Without a guard, the timeout handler still emitted a spurious realmjoin_timeout timeline event and re-entered TransitionToFinalizing, duplicating phase_transition(FinalizingSetup) on the wire and the FinalizingGrace deadline arm. Fix: short-circuit to a bookkept dead-end transition when either Outcome is already set OR the RealmJoinTimeout deadline is no longer in state.Deadlines (the cancel took effect). DeadEndReason carries the specific cause for the Inspector. Tests: DecisionCore 322 → 323 (new Stale_RealmJoinTimeout_after_Resolved_is_bookkept_dead_end_no_effects). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Enrollment/SystemSignals/RealmJoinInfo.cs | 6 ++-- .../SystemSignals/RealmJoinWatcher.cs | 22 +++++++++++-- .../RealmJoinGateTests.cs | 30 +++++++++++++++++ .../Engine/DecisionEngine.RealmJoin.cs | 32 +++++++++++++++++++ 4 files changed, 85 insertions(+), 5 deletions(-) diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/RealmJoinInfo.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/RealmJoinInfo.cs index bb23399a..d6cfa8a1 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/RealmJoinInfo.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/RealmJoinInfo.cs @@ -73,7 +73,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals try { var path = subPathOverride ?? ServiceRegistryPath; - using (var baseKey = RegistryKey.OpenBaseKey(hive, RegistryView.Default)) + using (var baseKey = RegistryKey.OpenBaseKey(hive, RegistryView.Registry64)) using (var key = baseKey.OpenSubKey(path, writable: false)) { if (key == null) return null; @@ -98,7 +98,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals { try { - using (var baseKey = RegistryKey.OpenBaseKey(hive, RegistryView.Default)) + using (var baseKey = RegistryKey.OpenBaseKey(hive, RegistryView.Registry64)) using (var key = baseKey.OpenSubKey(packagesPath, writable: false)) { if (key == null) return Array.Empty(); @@ -126,7 +126,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals snapshot = default; try { - using (var baseKey = RegistryKey.OpenBaseKey(hive, RegistryView.Default)) + using (var baseKey = RegistryKey.OpenBaseKey(hive, RegistryView.Registry64)) using (var key = baseKey.OpenSubKey($"{packagesPath}\\{packageId}", writable: false)) { if (key == null) return false; diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/RealmJoinWatcher.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/RealmJoinWatcher.cs index a7b18a38..99e9811d 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/RealmJoinWatcher.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/RealmJoinWatcher.cs @@ -285,6 +285,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals hive, parentPath, watchSubtree: false, + view: RegistryView.Registry64, filter: RegistryNativeMethods.RegChangeNotifyFilter.Name, trace: msg => _logger.Trace($"RealmJoinWatcher(appear:{role}): {msg}")); @@ -293,8 +294,19 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals if (_disposed) return; if (!KeyExists(hive, targetFullPath)) return; _logger.Info($"RealmJoinWatcher: {targetSubKeyName} appeared under {parentPath} — switching to subtree watcher"); - try { watcher.Dispose(); } catch { /* dispose may race with notification */ } + + // CRITICAL: this callback runs on the RegistryWatcher's own background + // thread. Calling Dispose() here would re-enter Stop() → Thread.Join() + // and self-deadlock (the thread would join itself). Use RequestStop() + // (non-blocking — see RegistryWatcher.RequestStop) and let a thread-pool + // worker dispose the watcher object after the loop has exited. + try { watcher.RequestStop(); } catch { /* nothing to do */ } clearSlot(); + ThreadPool.QueueUserWorkItem(_ => + { + try { watcher.Dispose(); } catch { /* dispose must not throw */ } + }); + try { onAppeared(); } catch (Exception ex) { _logger.Error($"RealmJoinWatcher: onAppeared({role}) threw", ex); } }; @@ -325,6 +337,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals RegistryHive.LocalMachine, RealmJoinInfo.ServiceRealmJoinKeyPath, watchSubtree: true, + view: RegistryView.Registry64, filter: RegistryNativeMethods.RegChangeNotifyFilter.LastSet | RegistryNativeMethods.RegChangeNotifyFilter.Name, trace: msg => _logger.Trace($"RealmJoinWatcher(svc): {msg}")); @@ -368,6 +381,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals RegistryHive.LocalMachine, RealmJoinInfo.MachineRealmJoinPath, watchSubtree: true, + view: RegistryView.Registry64, filter: RegistryNativeMethods.RegChangeNotifyFilter.LastSet | RegistryNativeMethods.RegChangeNotifyFilter.Name, trace: msg => _logger.Trace($"RealmJoinWatcher(hklm-rj): {msg}")); @@ -398,6 +412,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals RegistryHive.Users, fullPath, watchSubtree: true, + view: RegistryView.Registry64, filter: RegistryNativeMethods.RegChangeNotifyFilter.LastSet | RegistryNativeMethods.RegChangeNotifyFilter.Name, trace: msg => _logger.Trace($"RealmJoinWatcher(hku-rj): {msg}")); @@ -582,7 +597,10 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals { try { - using (var baseKey = RegistryKey.OpenBaseKey(hive, RegistryView.Default)) + // Registry64 is mandatory here — when the .NET host happens to run as 32-bit + // (mirroring TenantIdResolver's defensive note) HKLM\SOFTWARE silently redirects + // to WOW6432Node, so RealmJoin's 64-bit-only keys would appear missing. + using (var baseKey = RegistryKey.OpenBaseKey(hive, RegistryView.Registry64)) using (var key = baseKey.OpenSubKey(subPath, writable: false)) { return key != null; diff --git a/src/Agent/AutopilotMonitor.DecisionCore.Tests/RealmJoinGateTests.cs b/src/Agent/AutopilotMonitor.DecisionCore.Tests/RealmJoinGateTests.cs index 7433f8d4..6eba7266 100644 --- a/src/Agent/AutopilotMonitor.DecisionCore.Tests/RealmJoinGateTests.cs +++ b/src/Agent/AutopilotMonitor.DecisionCore.Tests/RealmJoinGateTests.cs @@ -137,6 +137,36 @@ namespace AutopilotMonitor.DecisionCore.Tests Assert.Equal("Resolved", step.NewState.RealmJoinFacts.Outcome!.Value); } + [Fact] + public void Stale_RealmJoinTimeout_after_Resolved_is_bookkept_dead_end_no_effects() + { + // Race: RealmJoinResolved arrives + cancels the timeout, but the queued + // DeadlineFired:realmjoin_timeout was already in flight on the signal worker. The + // idempotency guard must short-circuit before emitting a spurious realmjoin_timeout + // timeline event or re-entering TransitionToFinalizing. + var engine = new DecisionEngine(); + var state = PrimeClassicAwaitingDesktop(engine); + state = engine.Reduce(state, MakeSignal(5, DecisionSignalKind.RealmJoinDetected, T0.AddMinutes(5), + new Dictionary { [DecisionEngine.RealmJoinPayloadKeys.DeploymentPhase] = "100" })).NewState; + state = engine.Reduce(state, MakeSignal(6, DecisionSignalKind.DesktopArrived, T0.AddMinutes(6))).NewState; + state = engine.Reduce(state, MakeSignal(7, DecisionSignalKind.RealmJoinResolved, T0.AddMinutes(7), + new Dictionary { [DecisionEngine.RealmJoinPayloadKeys.DeploymentPhase] = "110" })).NewState; + Assert.Equal("Resolved", state.RealmJoinFacts.Outcome!.Value); + // Deadline already cancelled in state — but the queued DeadlineFired hasn't + // been informed yet. + + var step = engine.Reduce(state, MakeSignal(8, DecisionSignalKind.DeadlineFired, T0.AddMinutes(65), + new Dictionary { [SignalPayloadKeys.Deadline] = DeadlineNames.RealmJoinTimeout })); + + // State must NOT mutate to Timeout outcome. + Assert.Equal("Resolved", step.NewState.RealmJoinFacts.Outcome!.Value); + // Transition is recorded as DeadEnd with the stale reason — no taken-step. + Assert.False(step.Transition.Taken); + Assert.Equal("realmjoin_timeout_stale_outcome_already_set", step.Transition.DeadEndReason); + // No effects at all — no spurious realmjoin_timeout event, no FinalizingGrace re-arm. + Assert.Empty(step.Effects); + } + [Fact] public void RealmJoinTimeout_with_hello_and_desktop_in_completes_with_timeout_outcome() { diff --git a/src/Shared/AutopilotMonitor.DecisionCore/Engine/DecisionEngine.RealmJoin.cs b/src/Shared/AutopilotMonitor.DecisionCore/Engine/DecisionEngine.RealmJoin.cs index 52f16cde..45eef968 100644 --- a/src/Shared/AutopilotMonitor.DecisionCore/Engine/DecisionEngine.RealmJoin.cs +++ b/src/Shared/AutopilotMonitor.DecisionCore/Engine/DecisionEngine.RealmJoin.cs @@ -162,9 +162,41 @@ namespace AutopilotMonitor.DecisionCore.Engine /// = "Timeout", emits a /// realmjoin_timeout timeline entry, and — when other completion preconditions /// are in — releases the deferred completion path. + /// + /// Idempotency: a stale signal can + /// arrive after the deadline was cancelled by + /// (race between the cancel-effect reaching the scheduler and the timer firing). In + /// that case any further work would emit a spurious realmjoin_timeout timeline + /// event and re-enter — duplicating + /// phase_transition(FinalizingSetup) on the wire. Bail out as a bookkept dead-end + /// when either: (a) is already set (Resolved or + /// Timeout) or (b) the deadline is no + /// longer in the live state — both indicate the timer has been logically retired. + /// /// private DecisionStep HandleRealmJoinTimeoutDeadlineFired(DecisionState state, DecisionSignal signal) { + var alreadyResolvedOrTimedOut = state.RealmJoinFacts.Outcome != null; + var deadlineStillArmed = false; + foreach (var d in state.Deadlines) + { + if (d.Name == DeadlineNames.RealmJoinTimeout) { deadlineStillArmed = true; break; } + } + + if (alreadyResolvedOrTimedOut || !deadlineStillArmed) + { + var bookkept = BumpStepBookkeeping(state, signal); + var staleTransition = BuildDeadEndTransition( + state: state, + signal: signal, + nextStepIndex: bookkept.StepIndex, + trigger: $"DeadlineFired:{DeadlineNames.RealmJoinTimeout}", + deadEndReason: alreadyResolvedOrTimedOut + ? "realmjoin_timeout_stale_outcome_already_set" + : "realmjoin_timeout_stale_deadline_not_armed"); + return new DecisionStep(bookkept, staleTransition, Array.Empty()); + } + var nextStep = state.StepIndex + 1; var builder = state.ToBuilder()