fix(v2): 882fef64 codex review followup — TableStorage force-update CurrentPhase + persist phase package snapshots

Codex review of merged PR3 + PR5 surfaced two correctness gaps that match real-world session traffic. Finding 1 — TableStorageService.Sessions.cs force-update path: The normal status-update path mirrors `update["CurrentPhase"] = Complete` whenever status flips to Succeeded (line ~1019), so V2 enrollment_complete events (which carry Phase=Unknown per feedback_phase_strategy) still land at CurrentPhase=Complete. The ETag-exhaustion / force-update block at line ~1224 was missing the same override, leaving CurrentPhase stuck at the prior in-flight value (or -1) when ETag retries are exhausted exactly at the completion update. 6-line fix mirrors the well-tested normal-path override. Finding 2 — ImeTrackerStateData.PhasePackageSnapshots persistence: GetAllKnownPackageStates() returns the union of live _packageStates and the _phasePackageSnapshots dict (DeviceSetup apps captured before _packageStates.Clear() on the AccountSetup transition). SaveState only persisted the live list, so an agent restart between phases — common on hybrid-join + multi-reboot enrollments — lost the DeviceSetup snapshot and dropped those apps from FinalStatus + app_tracking_summary. ImeTrackerStateData now carries `Dictionary<string, List<PackageStateData>> PhasePackageSnapshots`, SaveState serialises it, LoadState restores it tolerantly (null on old state files → empty dict, no crash). Refactor side-product: extracted ToPackageStateData / FromPackageStateData as private static helpers in ImeLogTracker so the mapping is single-source-of-truth across live + snapshot serialisation. Skipped: Finding 3 (DetectHelloPolicy first-match-wins between Device/User CSP scopes) not fixed — Intune CSPs are exclusive per setting (Device OR User, never both), CSP wins over GPO, the first-match iteration is correct in practice. Saved feedback_csp_scope_exclusive.md so a future review doesn't re-suggest the defensive-aggregation patch. Tests: V2.Core 729 (+2 new for save/load round-trip + old-snapshot backwards-compat), DecisionCore 231, Functions 736, vitest 179, solution build 0/0. No new test for Finding 1's force-update path — the method has no existing test infrastructure (no Azurite fixture, no mock TableClient); the fix mirrors a well-tested normal-path override and risk is essentially zero. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-10-24 12:42:38 +00:00 · 2026-04-25 20:11:24 +02:00
parent d0200d061a
commit 96191c8740
6 changed files with 202 additions and 55 deletions
@@ -108,6 +108,84 @@ namespace AutopilotMonitor.Agent.V2.Core.Tests.Monitoring.Ime
            Assert.Equal(3, all.Count(p => p.Targeted == AppTargeted.User));
        }

+        [Fact]
+        public void PhasePackageSnapshots_survive_save_and_load_roundtrip()
+        {
+            // Codex follow-up (882fef64 PR3-PR5 review): on hybrid-join + multi-reboot
+            // enrollments the agent commonly restarts mid-AccountSetup. Before this fix the
+            // snapshot dict was lost on restart and DeviceSetup apps disappeared from
+            // FinalStatus + app_tracking_summary. Round-trip the persistence path on the
+            // same state directory to prove the dict survives.
+            using var tmp = new TempDirectory();
+            var stateDir = System.IO.Path.Combine(tmp.Path, "State");
+            var logger = new AgentLogger(tmp.Path, AgentLogLevel.Info);
+
+            using (var trackerA = new ImeLogTracker(
+                logFolder: tmp.Path,
+                patterns: new List<ImeLogPattern>(),
+                logger: logger,
+                stateDirectory: stateDir))
+            {
+                trackerA.SeedPhaseSnapshotForTesting("DeviceSetup", new[]
+                {
+                    NewPkg("device-1", AppTargeted.Device, AppInstallationState.Installed),
+                    NewPkg("device-2", AppTargeted.Device, AppInstallationState.Installed),
+                });
+                trackerA.PackageStates.Add(NewPkg("user-1", AppTargeted.User, AppInstallationState.Installed));
+
+                trackerA.SaveStateForTest();
+            }
+
+            // Fresh tracker on the same state dir — simulates an agent restart picking up the
+            // persisted state. LoadStateForTest replaces the synchronous Start() path so file
+            // watchers stay out of the test.
+            using (var trackerB = new ImeLogTracker(
+                logFolder: tmp.Path,
+                patterns: new List<ImeLogPattern>(),
+                logger: logger,
+                stateDirectory: stateDir))
+            {
+                trackerB.LoadStateForTest();
+
+                var all = trackerB.GetAllKnownPackageStates();
+                Assert.Equal(3, all.Count);
+                Assert.Equal(2, all.Count(p => p.Targeted == AppTargeted.Device));
+                Assert.Equal(1, all.Count(p => p.Targeted == AppTargeted.User));
+                Assert.Contains(all, p => p.Id == "device-1");
+                Assert.Contains(all, p => p.Id == "device-2");
+                Assert.Contains(all, p => p.Id == "user-1");
+            }
+        }
+
+        [Fact]
+        public void LoadState_tolerates_old_snapshot_files_without_PhasePackageSnapshots_field()
+        {
+            // Backwards-compat: agent rolled out before this field exists has snapshot files
+            // with no PhasePackageSnapshots entry. Loading must NOT throw and must leave the
+            // in-memory dict empty (degrades to live-only view).
+            using var tmp = new TempDirectory();
+            var stateDir = System.IO.Path.Combine(tmp.Path, "State");
+            System.IO.Directory.CreateDirectory(stateDir);
+
+            // Hand-craft an old-shape state file: contains only fields that pre-dated the
+            // PhasePackageSnapshots addition. JSON-deserialiser must default the new field
+            // to null without complaining.
+            var oldJson = "{\"CurrentPhaseOrder\":1,\"LastEspPhaseDetected\":\"DeviceSetup\",\"AllAppsCompletedFired\":false,\"LogPhaseIsCurrentPhase\":true,\"SeenAppIds\":[],\"IgnoreList\":[],\"CurrentPackageId\":null,\"Packages\":[],\"FilePositions\":{}}";
+            System.IO.File.WriteAllText(System.IO.Path.Combine(stateDir, "ime-tracker-state.json"), oldJson);
+
+            var logger = new AgentLogger(tmp.Path, AgentLogLevel.Info);
+            using var tracker = new ImeLogTracker(
+                logFolder: tmp.Path,
+                patterns: new List<ImeLogPattern>(),
+                logger: logger,
+                stateDirectory: stateDir);
+
+            // Must not throw.
+            tracker.LoadStateForTest();
+            // No snapshots restored, no live packages → empty consolidated view.
+            Assert.Empty(tracker.GetAllKnownPackageStates());
+        }
+
        [Fact]
        public void GetAllKnownPackageStates_dedupes_by_id_with_live_winning()
        {
@@ -382,23 +382,30 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.Ime
                _packageStates.Clear();
                foreach (var p in state.Packages)
                {
-                    var pkg = AppPackageState.Restore(
-                        p.Id, p.ListPos, p.Name,
-                        (AppRunAs)p.RunAs, (AppIntent)p.Intent, (AppTargeted)p.Targeted,
-                        p.DependsOn != null ? new HashSet<string>(p.DependsOn) : new HashSet<string>(),
-                        (AppInstallationState)p.InstallationState, p.DownloadingOrInstallingSeen,
-                        p.ProgressPercent, p.BytesDownloaded, p.BytesTotal,
-                        errorPatternId: p.ErrorPatternId, errorDetail: p.ErrorDetail, errorCode: p.ErrorCode,
-                        exitCode: p.ExitCode, hresultFromWin32: p.HResultFromWin32,
-                        doFileSize: p.DoFileSize, doTotalBytesDownloaded: p.DoTotalBytesDownloaded,
-                        doBytesFromPeers: p.DoBytesFromPeers, doPercentPeerCaching: p.DoPercentPeerCaching,
-                        doBytesFromLanPeers: p.DoBytesFromLanPeers, doBytesFromGroupPeers: p.DoBytesFromGroupPeers,
-                        doBytesFromInternetPeers: p.DoBytesFromInternetPeers, doDownloadMode: p.DoDownloadMode,
-                        doDownloadDuration: p.DoDownloadDuration, doBytesFromHttp: p.DoBytesFromHttp,
-                        hasDoTelemetry: p.HasDoTelemetry,
-                        appVersion: p.AppVersion, appType: p.AppType,
-                        attemptNumber: p.AttemptNumber, detectionResult: p.DetectionResult);
-                    _packageStates.Add(pkg);
+                    _packageStates.Add(FromPackageStateData(p));
+                }
+            }
+
+            // Codex follow-up (882fef64 PR3-PR5 review): restore per-phase snapshots so
+            // GetAllKnownPackageStates() still sees DeviceSetup apps after a mid-AccountSetup
+            // restart. Old state files that pre-date the field have null here — leave the
+            // dict empty in that case (degrades to live-only view, no crash).
+            _phasePackageSnapshots.Clear();
+            if (state.PhasePackageSnapshots != null)
+            {
+                foreach (var kv in state.PhasePackageSnapshots)
+                {
+                    if (string.IsNullOrEmpty(kv.Key) || kv.Value == null) continue;
+                    var restored = new List<AppPackageState>(kv.Value.Count);
+                    foreach (var p in kv.Value)
+                    {
+                        try { restored.Add(FromPackageStateData(p)); }
+                        catch (Exception ex)
+                        {
+                            _logger.Warning($"ImeLogTracker: failed to restore phase-snapshot package '{p?.Id ?? "(null)"}' for phase '{kv.Key}': {ex.Message}");
+                        }
+                    }
+                    _phasePackageSnapshots[kv.Key] = restored;
                }
            }

@@ -438,42 +445,18 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.Ime
                SeenAppIds = _seenAppIds.ToList(),
                IgnoreList = _packageStates.IgnoreList.ToList(),
                CurrentPackageId = _packageStates.CurrentPackageId,
-                Packages = _packageStates.Select(p => new PackageStateData
-                {
-                    Id = p.Id,
-                    ListPos = p.ListPos,
-                    Name = p.Name,
-                    RunAs = (int)p.RunAs,
-                    Intent = (int)p.Intent,
-                    Targeted = (int)p.Targeted,
-                    DependsOn = p.DependsOn?.ToList() ?? new List<string>(),
-                    InstallationState = (int)p.InstallationState,
-                    DownloadingOrInstallingSeen = p.DownloadingOrInstallingSeen,
-                    ProgressPercent = p.ProgressPercent,
-                    BytesDownloaded = p.BytesDownloaded,
-                    BytesTotal = p.BytesTotal,
-                    ErrorPatternId = p.ErrorPatternId,
-                    ErrorDetail = p.ErrorDetail,
-                    ErrorCode = p.ErrorCode,
-                    ExitCode = p.ExitCode,
-                    HResultFromWin32 = p.HResultFromWin32,
-                    DoFileSize = p.DoFileSize,
-                    DoTotalBytesDownloaded = p.DoTotalBytesDownloaded,
-                    DoBytesFromPeers = p.DoBytesFromPeers,
-                    DoPercentPeerCaching = p.DoPercentPeerCaching,
-                    DoBytesFromLanPeers = p.DoBytesFromLanPeers,
-                    DoBytesFromGroupPeers = p.DoBytesFromGroupPeers,
-                    DoBytesFromInternetPeers = p.DoBytesFromInternetPeers,
-                    DoDownloadMode = p.DoDownloadMode,
-                    DoDownloadDuration = p.DoDownloadDuration,
-                    DoBytesFromHttp = p.DoBytesFromHttp,
-                    HasDoTelemetry = p.HasDoTelemetry,
-                    AppVersion = p.AppVersion,
-                    AppType = p.AppType,
-                    AttemptNumber = p.AttemptNumber,
-                    DetectionResult = p.DetectionResult
-                }).ToList(),
-                FilePositions = new Dictionary<string, FilePositionData>()
+                Packages = _packageStates.Select(ToPackageStateData).ToList(),
+                FilePositions = new Dictionary<string, FilePositionData>(),
+
+                // Codex follow-up (882fef64 PR3-PR5 review): persist per-phase snapshots so
+                // hybrid-join / multi-reboot enrollments don't lose DeviceSetup apps from
+                // FinalStatus + app_tracking_summary across an agent restart. The dict key is
+                // the ESP phase name (e.g. "DeviceSetup"); the value is the dehydrated snapshot
+                // captured on _packageStates.Clear() during the next-phase transition.
+                PhasePackageSnapshots = _phasePackageSnapshots.ToDictionary(
+                    kv => kv.Key,
+                    kv => kv.Value.Select(ToPackageStateData).ToList(),
+                    StringComparer.Ordinal),
            };

            // Store file positions by filename only (log folder is known)
@@ -490,6 +473,64 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.Ime
            _statePersistence.Save(state);
        }

+        // Codex follow-up (882fef64 PR3-PR5 review): single source of truth for AppPackageState
+        // <-> PackageStateData mapping. Used both for the live _packageStates list and for the
+        // _phasePackageSnapshots restore path so the field set never drifts between the two.
+        private static PackageStateData ToPackageStateData(AppPackageState p) =>
+            new PackageStateData
+            {
+                Id = p.Id,
+                ListPos = p.ListPos,
+                Name = p.Name,
+                RunAs = (int)p.RunAs,
+                Intent = (int)p.Intent,
+                Targeted = (int)p.Targeted,
+                DependsOn = p.DependsOn?.ToList() ?? new List<string>(),
+                InstallationState = (int)p.InstallationState,
+                DownloadingOrInstallingSeen = p.DownloadingOrInstallingSeen,
+                ProgressPercent = p.ProgressPercent,
+                BytesDownloaded = p.BytesDownloaded,
+                BytesTotal = p.BytesTotal,
+                ErrorPatternId = p.ErrorPatternId,
+                ErrorDetail = p.ErrorDetail,
+                ErrorCode = p.ErrorCode,
+                ExitCode = p.ExitCode,
+                HResultFromWin32 = p.HResultFromWin32,
+                DoFileSize = p.DoFileSize,
+                DoTotalBytesDownloaded = p.DoTotalBytesDownloaded,
+                DoBytesFromPeers = p.DoBytesFromPeers,
+                DoPercentPeerCaching = p.DoPercentPeerCaching,
+                DoBytesFromLanPeers = p.DoBytesFromLanPeers,
+                DoBytesFromGroupPeers = p.DoBytesFromGroupPeers,
+                DoBytesFromInternetPeers = p.DoBytesFromInternetPeers,
+                DoDownloadMode = p.DoDownloadMode,
+                DoDownloadDuration = p.DoDownloadDuration,
+                DoBytesFromHttp = p.DoBytesFromHttp,
+                HasDoTelemetry = p.HasDoTelemetry,
+                AppVersion = p.AppVersion,
+                AppType = p.AppType,
+                AttemptNumber = p.AttemptNumber,
+                DetectionResult = p.DetectionResult,
+            };
+
+        private static AppPackageState FromPackageStateData(PackageStateData p) =>
+            AppPackageState.Restore(
+                p.Id, p.ListPos, p.Name,
+                (AppRunAs)p.RunAs, (AppIntent)p.Intent, (AppTargeted)p.Targeted,
+                p.DependsOn != null ? new HashSet<string>(p.DependsOn) : new HashSet<string>(),
+                (AppInstallationState)p.InstallationState, p.DownloadingOrInstallingSeen,
+                p.ProgressPercent, p.BytesDownloaded, p.BytesTotal,
+                errorPatternId: p.ErrorPatternId, errorDetail: p.ErrorDetail, errorCode: p.ErrorCode,
+                exitCode: p.ExitCode, hresultFromWin32: p.HResultFromWin32,
+                doFileSize: p.DoFileSize, doTotalBytesDownloaded: p.DoTotalBytesDownloaded,
+                doBytesFromPeers: p.DoBytesFromPeers, doPercentPeerCaching: p.DoPercentPeerCaching,
+                doBytesFromLanPeers: p.DoBytesFromLanPeers, doBytesFromGroupPeers: p.DoBytesFromGroupPeers,
+                doBytesFromInternetPeers: p.DoBytesFromInternetPeers, doDownloadMode: p.DoDownloadMode,
+                doDownloadDuration: p.DoDownloadDuration, doBytesFromHttp: p.DoBytesFromHttp,
+                hasDoTelemetry: p.HasDoTelemetry,
+                appVersion: p.AppVersion, appType: p.AppType,
+                attemptNumber: p.AttemptNumber, detectionResult: p.DetectionResult);
+
        /// <summary>
        /// Deletes persisted state file. Called on enrollment complete to ensure
        /// a fresh state on the next enrollment cycle.
@@ -499,6 +540,12 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.Ime
            _statePersistence?.Delete();
        }

+        // Test-only seams: exercise the persistence round-trip without driving Start() (which
+        // also spins up file watchers). Codex follow-up (882fef64 PR3-PR5 review) needs these
+        // to verify PhasePackageSnapshots survive a simulated agent restart.
+        internal void SaveStateForTest() => SaveState();
+        internal void LoadStateForTest() => LoadState();
+
        #endregion

    }
@@ -106,6 +106,16 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.Ime
        public string CurrentPackageId { get; set; }
        public List<PackageStateData> Packages { get; set; }
        public Dictionary<string, FilePositionData> FilePositions { get; set; }
+
+        // Codex follow-up (882fef64 PR3-PR5 review): per-phase package snapshots that the
+        // tracker captures just before _packageStates.Clear() on the next-phase transition.
+        // Without persistence, an agent restart between phases (common on hybrid-join +
+        // multi-reboot enrollments) loses the earlier-phase apps from the union returned by
+        // GetAllKnownPackageStates, which in turn drops them from FinalStatus + the
+        // app_tracking_summary terminal event. Null on agents from before this field was
+        // added — LoadState handles that as "no snapshots" and the in-memory dict stays empty
+        // (degrades gracefully back to the live _packageStates view).
+        public Dictionary<string, List<PackageStateData>> PhasePackageSnapshots { get; set; }
    }

    public class PackageStateData
@@ -1 +1 @@
-339
+341
@@ -1 +1 @@
-1165
+1166
@@ -1222,6 +1222,18 @@ namespace AutopilotMonitor.Functions.Services
                            if (currentPhase.HasValue)
                                forceUpdate["CurrentPhase"] = (int)currentPhase.Value;

+                            // Codex follow-up (882fef64 PR3-PR5 review): mirror the
+                            // terminal-phase override from the normal path (line ~1019). V2
+                            // emits enrollment_complete with Phase=Unknown (per
+                            // feedback_phase_strategy), so currentPhase.HasValue is false
+                            // here. Without this override the force-update path leaves
+                            // CurrentPhase at its prior (in-flight or -1) value forever
+                            // when ETag retries are exhausted exactly at completion.
+                            if (status == SessionStatus.Succeeded)
+                            {
+                                forceUpdate["CurrentPhase"] = (int)EnrollmentPhase.Complete;
+                            }
+
                            var freshStartedAt = freshSession.GetDateTimeOffset("StartedAt")?.UtcDateTime ?? DateTime.MaxValue;
                            if (earliestEventTimestamp.HasValue && earliestEventTimestamp.Value < freshStartedAt)
                                forceUpdate["StartedAt"] = EnsureUtc(earliestEventTimestamp.Value);