mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 04:32:32 +00:00
fix(agent/v2,decision): RealmJoin code-review findings — self-deadlock, Registry64, timeout idempotency
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) <noreply@anthropic.com>
This commit is contained in:
+3
-3
@@ -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<string>();
|
||||
@@ -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;
|
||||
|
||||
+20
-2
@@ -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;
|
||||
|
||||
@@ -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<string, string> { [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<string, string> { [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<string, string> { [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()
|
||||
{
|
||||
|
||||
@@ -162,9 +162,41 @@ namespace AutopilotMonitor.DecisionCore.Engine
|
||||
/// <see cref="RealmJoinFacts.Outcome"/> = <c>"Timeout"</c>, emits a
|
||||
/// <c>realmjoin_timeout</c> timeline entry, and — when other completion preconditions
|
||||
/// are in — releases the deferred completion path.
|
||||
/// <para>
|
||||
/// <b>Idempotency</b>: a stale <see cref="DecisionSignalKind.DeadlineFired"/> signal can
|
||||
/// arrive after the deadline was cancelled by <see cref="HandleRealmJoinResolvedV1"/>
|
||||
/// (race between the cancel-effect reaching the scheduler and the timer firing). In
|
||||
/// that case any further work would emit a spurious <c>realmjoin_timeout</c> timeline
|
||||
/// event and re-enter <see cref="TransitionToFinalizing"/> — duplicating
|
||||
/// <c>phase_transition(FinalizingSetup)</c> on the wire. Bail out as a bookkept dead-end
|
||||
/// when either: (a) <see cref="RealmJoinFacts.Outcome"/> is already set (Resolved or
|
||||
/// Timeout) or (b) the <see cref="DeadlineNames.RealmJoinTimeout"/> deadline is no
|
||||
/// longer in the live state — both indicate the timer has been logically retired.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
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<DecisionEffect>());
|
||||
}
|
||||
|
||||
var nextStep = state.StepIndex + 1;
|
||||
|
||||
var builder = state.ToBuilder()
|
||||
|
||||
Reference in New Issue
Block a user