From bf090c436e1c23375f8d436851a87c6d14e16e80 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Tue, 21 Apr 2026 17:10:58 +0200 Subject: [PATCH] fix(agent): create SessionId before startup guards to prevent first-run ghost-detect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The V2 agent was deterministically self-destructing on every first run after --install on a fresh VM. CheckEnrollmentCompleteMarker reads HKLM\SOFTWARE\AutopilotMonitor\Deployed (written by --install) and treats "marker present + session.id absent" as a ghost restart, triggering ExecuteSelfDestruct and wiping %ProgramData%\AutopilotMonitor before FetchConfigAsync can run. Remote config's SelfDestructOnComplete=false was never honored because the config fetch never happened. Legacy V1 agent creates the session in LoadConfiguration (Program.Configuration.cs:131) before the guard runs (Program.cs:169). V2 reversed this ordering with a comment claiming it prevents "dead session resurrection" — but the guards key on file-system state (SessionPersistence.SessionExists()), not the runtime-loaded session object, so the reordering was functionally unnecessary and introduced this regression. Fix: move SessionIdPersistence.GetOrCreate + --new-session handling back before the guards, restoring Legacy boot order. Ghost-restart detection still triggers in its intended case (session.id file deleted by incomplete self-destruct, but Scheduled Task + registry marker survive). Test coverage: adds 3 missing tests for CheckEnrollmentCompleteMarker in ProgramGuardsTests.cs (first-run with session, marker without selfdestruct, marker with selfdestruct retry). The M4.6.alpha guard suite had no coverage for this path, which is how the regression shipped. 425 V2.Core tests green. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Program/ProgramGuardsTests.cs | 93 +++++++++++++++++++ .../AutopilotMonitor.Agent.V2/Program.cs | 22 +++-- 2 files changed, 106 insertions(+), 9 deletions(-) diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Program/ProgramGuardsTests.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Program/ProgramGuardsTests.cs index 3a0d2daa..c2a1b293 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Program/ProgramGuardsTests.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Program/ProgramGuardsTests.cs @@ -75,6 +75,99 @@ namespace AutopilotMonitor.Agent.V2.Core.Tests.Program $"Expected hard_kill or reboot_kill, got {summary.ExitType}."); } + // ================================================================= CheckEnrollmentCompleteMarker + + [Fact] + public void CheckEnrollmentCompleteMarker_returns_false_on_first_run_after_install_when_session_exists() + { + // Regression guard for the first-run-after-install bug: Program.cs must create the + // SessionId BEFORE invoking this guard, so a fresh install (Deployed registry marker + // written by --install, no prior session) does not get misdiagnosed as a ghost + // restart and trigger self-destruct before FetchConfigAsync. + using var tmp = new TempDirectory(); + var logger = NewLogger(tmp.Path); + new SessionIdPersistence(tmp.Path).GetOrCreate(logger); + var stateDir = Path.Combine(tmp.Path, "State"); + var factoryCalls = 0; + + var shouldExit = AutopilotMonitor.Agent.V2.Program.CheckEnrollmentCompleteMarker( + dataDirectory: tmp.Path, + stateDirectory: stateDir, + selfDestructOnComplete: true, // worst case — cleanup armed + cleanupServiceFactory: () => + { + factoryCalls++; + throw new InvalidOperationException( + "test-harness: cleanup factory must not be invoked in this scenario"); + }, + logger: logger, + consoleMode: false); + + Assert.False(shouldExit); + Assert.Equal(0, factoryCalls); + } + + [Fact] + public void CheckEnrollmentCompleteMarker_returns_true_without_firing_cleanup_when_marker_present_and_no_selfdestruct() + { + // Remote config said "don't self-destruct": agent must exit cleanly but leave the + // ProgramData directory intact for post-mortem inspection. + using var tmp = new TempDirectory(); + var logger = NewLogger(tmp.Path); + new SessionIdPersistence(tmp.Path).GetOrCreate(logger); + var stateDir = Path.Combine(tmp.Path, "State"); + Directory.CreateDirectory(stateDir); + File.WriteAllText(Path.Combine(stateDir, "enrollment-complete.marker"), "previous completion"); + var factoryCalls = 0; + + var shouldExit = AutopilotMonitor.Agent.V2.Program.CheckEnrollmentCompleteMarker( + dataDirectory: tmp.Path, + stateDirectory: stateDir, + selfDestructOnComplete: false, + cleanupServiceFactory: () => + { + factoryCalls++; + throw new InvalidOperationException( + "test-harness: cleanup factory must not be invoked in this scenario"); + }, + logger: logger, + consoleMode: false); + + Assert.True(shouldExit); + Assert.Equal(0, factoryCalls); + } + + [Fact] + public void CheckEnrollmentCompleteMarker_fires_cleanup_retry_when_marker_present_and_selfdestruct_armed() + { + // Scheduled-task-survived-self-destruct retry path. CleanupService actually spawns a + // PowerShell cleanup script — we throw from the factory so the real side-effect does + // not fire; TryRetryCleanup swallows the exception and logs a warning. + using var tmp = new TempDirectory(); + var logger = NewLogger(tmp.Path); + new SessionIdPersistence(tmp.Path).GetOrCreate(logger); + var stateDir = Path.Combine(tmp.Path, "State"); + Directory.CreateDirectory(stateDir); + File.WriteAllText(Path.Combine(stateDir, "enrollment-complete.marker"), "previous completion"); + var factoryCalls = 0; + + var shouldExit = AutopilotMonitor.Agent.V2.Program.CheckEnrollmentCompleteMarker( + dataDirectory: tmp.Path, + stateDirectory: stateDir, + selfDestructOnComplete: true, + cleanupServiceFactory: () => + { + factoryCalls++; + throw new InvalidOperationException( + "test-harness: must not spawn real PowerShell cleanup in unit tests"); + }, + logger: logger, + consoleMode: false); + + Assert.True(shouldExit); + Assert.Equal(1, factoryCalls); + } + // ================================================================= Emergency break [Fact] diff --git a/src/Agent/AutopilotMonitor.Agent.V2/Program.cs b/src/Agent/AutopilotMonitor.Agent.V2/Program.cs index 04937b7c..c4989313 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/Program.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2/Program.cs @@ -36,11 +36,11 @@ namespace AutopilotMonitor.Agent.V2 /// Load cached remote-config.json → + AllowAgentDowngrade /// — on success restarts the process; on failure continues with current binary /// — reads markers + event log to classify last shutdown - /// — ghost-restart detection + cleanup retry - /// — absolute session-age watchdog /// Resolve TenantId (registry → bootstrap-config.json fallback) /// Build (CLI args + persisted bootstrap / await-enrollment config) - /// Get/create SessionId via + /// Get/create SessionId via — before the guards, matching Legacy boot order. The ghost-restart guard keys on session.id absence to detect "self-destruct ran but Scheduled Task survived"; creating the session after the guard would misdiagnose every first-run-after-install as a ghost restart and trigger self-destruct before the remote-config fetch. + /// — ghost-restart detection + cleanup retry + /// — absolute session-age watchdog /// (Optional) Wait for MDM certificate in --await-enrollment mode /// Build + → fetch config /// — H-2 mitigation post-cert @@ -197,13 +197,21 @@ namespace AutopilotMonitor.Agent.V2 // --install time may have written a bootstrap config that holds a tenantId even when // the registry is not yet populated (bootstrap token path). Honour that. + var agentConfig = BuildAgentConfiguration(args, tenantId, sessionId: null, bootstrapConfig, awaitConfig); + + // Create/recover SessionId BEFORE the startup guards. Legacy-parity boot order — + // the ghost-restart guard below keys on session.id absence to detect + // "self-destruct ran but Scheduled Task survived". Creating the session after the + // guard would misdiagnose every first-run-after-install (Deployed registry marker + // set by --install, no session.id yet) as a ghost restart and trigger + // ExecuteSelfDestruct before the remote-config fetch, deleting the ProgramData + // directory on every fresh deployment. if (args.Contains("--new-session")) { new SessionIdPersistence(dataDirectory).Delete(logger); logger.Info("--new-session: cleared persisted SessionId."); } - - var agentConfig = BuildAgentConfiguration(args, tenantId, sessionId: null, bootstrapConfig, awaitConfig); + agentConfig.SessionId = new SessionIdPersistence(dataDirectory).GetOrCreate(logger); // Build a cleanup-service factory used by guards: instantiated lazily and with the // current agentConfig so command-line overrides (e.g. --no-cleanup) take effect. @@ -226,10 +234,6 @@ namespace AutopilotMonitor.Agent.V2 return 0; } - // Session is safe to start — create/recover SessionId now (AFTER guards so a dead - // session is not resurrected before the watchdog can kill it). - agentConfig.SessionId = new SessionIdPersistence(dataDirectory).GetOrCreate(logger); - if (agentConfig.AwaitEnrollment) { logger.Info($"--await-enrollment: polling for MDM certificate (timeout: {agentConfig.AwaitEnrollmentTimeoutMinutes}min).");