From 9e358b98172bdf1bdd81537485ef67800dd69a50 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Tue, 19 May 2026 23:19:55 +0200 Subject: [PATCH] fix(agent/v2,web): recover terminal-stage on restart + quiet UI for terminal sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent V2: when RecoveryPath loads a snapshot already at Stage=Completed/Failed, no reducer transition fires so OnDecisionTerminalStage was never invoked — the agent idled until AgentMaxLifetime (default 6h), emitting periodic perf/metrics events the whole time. Fire it explicitly at the end of Start() so the EnrollmentTerminationHandler runs (marker write + diagnostics + self-destruct). The WhiteGloveSealed archive-and-reset at step 0 already neutralises the Part-2 case; non-terminal stages take the normal recovery path. Web: gate the SignalR eventStream refetch and the 30s fallback polling on non-terminal session status. Once Succeeded/Failed, late perf/metrics arrivals no longer trigger a full-page refetch + re-render that scroll-jumps the timeline while the user is reading. Observed in session 17ef0e0e-... (2026-05-19): 4x enrollment_complete, only 1x agent_shutting_down; reboot mid-shutdown left subsequent agent instances running for hours emitting noise. Tests: 3 new in RecoveryPathTests (Completed/Failed -> Terminated, non-terminal -> no Terminated), 4 in sessionStatus.test.ts. Suites grun: Web 384/384, Agent V2.Core 1095/1095, Backend 1844/1844. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Orchestration/RecoveryPathTests.cs | 127 ++++++++++++++++++ .../Orchestration/EnrollmentOrchestrator.cs | 28 ++++ .../buildcounter.txt | 2 +- .../AutopilotMonitor.Agent/buildcounter.txt | 2 +- .../[sessionId]/hooks/useSessionEvents.ts | 8 +- .../[sessionId]/hooks/useSessionSignalR.ts | 20 ++- .../app/sessions/[sessionId]/page.tsx | 1 + .../lib/__tests__/sessionStatus.test.ts | 27 ++++ .../utils/sessionStatus.ts | 8 ++ 9 files changed, 216 insertions(+), 7 deletions(-) create mode 100644 src/Web/autopilot-monitor-web/lib/__tests__/sessionStatus.test.ts create mode 100644 src/Web/autopilot-monitor-web/utils/sessionStatus.ts diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Orchestration/RecoveryPathTests.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Orchestration/RecoveryPathTests.cs index e03c1ac7..1060f2b9 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Orchestration/RecoveryPathTests.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Orchestration/RecoveryPathTests.cs @@ -829,5 +829,132 @@ namespace AutopilotMonitor.Agent.V2.Core.Tests.Orchestration var task = (System.Threading.Tasks.Task)method!.Invoke(sut, new object?[] { CancellationToken.None })!; return task.GetAwaiter().GetResult(); } + + // ============================================================ Terminal-stage-on-load recovery + // + // Bug captured in session 17ef0e0e-… (2026-05-19): an agent killed mid-shutdown + // restarts, RecoveryPath loads a snapshot already at Stage=Completed, but the + // DecisionStepProcessor's OnDecisionTerminalStage callback only fires on a real + // reducer TRANSITION into a terminal stage. A pre-terminal load produces no + // transition → no callback → EnrollmentTerminationHandler never runs → marker + // never written → SelfDestruct never invoked → agent idles for the full + // AgentMaxLifetime (default 6h) emitting periodic perf/metrics events. + // + // The fix at the end of EnrollmentOrchestrator.Start synthesises the missing + // invocation when initialState.Stage ∈ {Completed, Failed}, so the regular + // termination pipeline runs immediately. + + [Fact] + public void Completed_snapshot_fires_Terminated_with_succeeded_outcome_on_start() + { + using var rig = new Rig(); + Directory.CreateDirectory(rig.StateDir); + + // Seed a snapshot whose Stage is already Completed — simulates an agent + // killed by reboot/Ctrl+C between agent_shutting_down and marker write. + var terminalState = DecisionState.CreateInitial("S1", "T1") + .ToBuilder() + .WithStage(SessionStage.Completed) + .WithStepIndex(42) + .Build(); + new SnapshotPersistence( + Path.Combine(rig.StateDir, "snapshot.json"), + () => rig.Clock.UtcNow).Save(terminalState); + + var sut = rig.Build(); + + EnrollmentTerminatedEventArgs? captured = null; + using var fired = new ManualResetEventSlim(false); + sut.Terminated += (_, args) => + { + captured = args; + fired.Set(); + }; + + sut.Start(); + try + { + Assert.True(fired.Wait(TimeSpan.FromSeconds(5)), + "Terminated event did not fire within 5s after Start with terminal initial state."); + Assert.NotNull(captured); + Assert.Equal(EnrollmentTerminationReason.DecisionTerminalStage, captured!.Reason); + Assert.Equal(EnrollmentTerminationOutcome.Succeeded, captured.Outcome); + Assert.Equal(SessionStage.Completed.ToString(), captured.StageName); + } + finally { sut.Stop(); } + } + + [Fact] + public void Failed_snapshot_fires_Terminated_with_failed_outcome_on_start() + { + using var rig = new Rig(); + Directory.CreateDirectory(rig.StateDir); + + var terminalState = DecisionState.CreateInitial("S1", "T1") + .ToBuilder() + .WithStage(SessionStage.Failed) + .WithStepIndex(17) + .Build(); + new SnapshotPersistence( + Path.Combine(rig.StateDir, "snapshot.json"), + () => rig.Clock.UtcNow).Save(terminalState); + + var sut = rig.Build(); + + EnrollmentTerminatedEventArgs? captured = null; + using var fired = new ManualResetEventSlim(false); + sut.Terminated += (_, args) => + { + captured = args; + fired.Set(); + }; + + sut.Start(); + try + { + Assert.True(fired.Wait(TimeSpan.FromSeconds(5)), + "Terminated event did not fire within 5s after Start with Failed initial state."); + Assert.NotNull(captured); + Assert.Equal(EnrollmentTerminationReason.DecisionTerminalStage, captured!.Reason); + Assert.Equal(EnrollmentTerminationOutcome.Failed, captured.Outcome); + Assert.Equal(SessionStage.Failed.ToString(), captured.StageName); + } + finally { sut.Stop(); } + } + + [Fact] + public void Non_terminal_snapshot_does_NOT_fire_Terminated_on_start() + { + // Negative control: a mid-enrollment snapshot must keep the normal recovery + // path (no synthetic Terminated fire). Without this guard the recovery hook + // would short-circuit legitimate in-flight sessions on every restart. + using var rig = new Rig(); + Directory.CreateDirectory(rig.StateDir); + + var midState = DecisionState.CreateInitial("S1", "T1") + .ToBuilder() + .WithStage(SessionStage.EspDeviceSetup) + .WithStepIndex(3) + .Build(); + new SnapshotPersistence( + Path.Combine(rig.StateDir, "snapshot.json"), + () => rig.Clock.UtcNow).Save(midState); + + var sut = rig.Build(); + + using var fired = new ManualResetEventSlim(false); + sut.Terminated += (_, _) => fired.Set(); + + sut.Start(); + try + { + // 500ms is plenty: the recovery path fires synchronously off Task.Run. + // If it were going to fire, it would have hit within tens of milliseconds. + Assert.False(fired.Wait(TimeSpan.FromMilliseconds(500)), + "Terminated event fired despite non-terminal initial state — recovery hook over-fires."); + Assert.Equal(SessionStage.EspDeviceSetup, sut.CurrentState.Stage); + } + finally { sut.Stop(); } + } } } diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Orchestration/EnrollmentOrchestrator.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Orchestration/EnrollmentOrchestrator.cs index 5e7ce3f9..1931dd02 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core/Orchestration/EnrollmentOrchestrator.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Orchestration/EnrollmentOrchestrator.cs @@ -814,6 +814,34 @@ namespace AutopilotMonitor.Agent.V2.Core.Orchestration var hostCount = _collectorHosts?.Count ?? 0; var maxLifetimeMin = _agentMaxLifetime.HasValue ? _agentMaxLifetime.Value.TotalMinutes.ToString("F0") + "min" : "off"; _logger.Info($"EnrollmentOrchestrator: started (sessionId={sessionShort}, initialStage={initialStage}, hosts={hostCount}, maxLifetime={maxLifetimeMin})."); + + // 17) Terminal-stage-on-load recovery. When recovery yields an initialState whose + // Stage is already terminal (Completed/Failed), no further reducer transition + // fires, so the regular OnDecisionTerminalStage path on the DecisionStepProcessor + // never triggers. Without this branch the agent would sit idle past completion + // until the AgentMaxLifetime watchdog (default 6h) trips — emitting periodic + // performance_snapshot / agent_metrics_snapshot events the whole time + // (observed in session 17ef0e0e-… 2026-05-19). Symmetric to the WhiteGlove + // Part-1 archive-and-reset at step 0: that path is handled there and the + // snapshot is gone by the time we reach this point, so the Completed/Failed + // check below is safe (cannot accidentally tear down a Part-2 resume). + // + // We reuse OnDecisionTerminalStage rather than re-implementing the dispatch + // so the existing _terminatedFired interlock, max-lifetime-timer disposal, + // and Task.Run off-loader all apply. The Terminated handler (terminationHandler + // in AgentRuntimeHost) is wired BEFORE Start() and the EnrollmentTerminationHandler + // itself is constructed inside the onIngressReady hook (step 13b), which has + // already fired by this point — so when our recovery-fire propagates through + // Task.Run, the handler is fully wired. + if (initialState.Stage == SessionStage.Completed || initialState.Stage == SessionStage.Failed) + { + _logger.Warning( + $"EnrollmentOrchestrator: post-recovery initialState already terminal " + + $"(stage={initialState.Stage}, stepIndex={initialState.StepIndex}) — " + + $"invoking OnDecisionTerminalStage directly. Prior run died before the " + + $"normal termination handler could complete (no transition will fire on its own)."); + OnDecisionTerminalStage(initialState); + } } /// diff --git a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt index 5fa2986d..b3fa96d2 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt @@ -1 +1 @@ -815 +819 diff --git a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt index a9b14017..6ee29474 100644 --- a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt @@ -1 +1 @@ -1383 +1384 diff --git a/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionEvents.ts b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionEvents.ts index 21412a60..3771a71b 100644 --- a/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionEvents.ts +++ b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionEvents.ts @@ -5,6 +5,7 @@ import { api } from "@/lib/api"; import { authenticatedFetch, TokenExpiredError } from "@/lib/authenticatedFetch"; import { extractContinuation, MAX_EAGER_PAGES } from "@/lib/paginationLink"; import { isGuid } from "@/utils/inputValidation"; +import { isTerminalStatus } from "@/utils/sessionStatus"; import { EnrollmentEvent, Session } from "@/types"; import type { NotificationType } from "@/contexts/NotificationContext"; @@ -225,11 +226,16 @@ export function useSessionEvents({ }, []); // Fallback polling only while SignalR is disconnected. + // Symmetric with the SignalR eventStream gate: once the session has reached a terminal + // status, suppress the 30s refetch loop too — late agent perf/metrics snapshots would + // otherwise still trigger refetches, defeating the SignalR-side gate. useEffect(() => { const effectiveTenantId = resolveEffectiveTenantId(); if (!sessionId || !effectiveTenantId || isConnected) return; const interval = setInterval(() => { - if (document.visibilityState === "visible") fetchEvents(); + if (document.visibilityState !== "visible") return; + if (isTerminalStatus(sessionRef.current?.status)) return; + fetchEvents(); }, 30_000); return () => clearInterval(interval); // eslint-disable-next-line react-hooks/exhaustive-deps diff --git a/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionSignalR.ts b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionSignalR.ts index 33683493..5593ce21 100644 --- a/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionSignalR.ts +++ b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionSignalR.ts @@ -2,6 +2,7 @@ import { useEffect, useRef } from "react"; import { Session, RuleResult } from "@/types"; +import { isTerminalStatus } from "@/utils/sessionStatus"; interface SignalRApi { // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -20,6 +21,7 @@ interface UseSessionSignalRParams { sessionTenantIdFromSession: string | undefined; globalAdminMode: boolean; sessionIdRef: React.MutableRefObject; + sessionRef: React.MutableRefObject; resolveEffectiveTenantId: () => string | null; signalR: SignalRApi; scheduleFetchEvents: (delayMs?: number) => void; @@ -42,6 +44,7 @@ export function useSessionSignalR({ sessionTenantIdFromSession, globalAdminMode, sessionIdRef, + sessionRef, resolveEffectiveTenantId, signalR, scheduleFetchEvents, @@ -97,10 +100,19 @@ export function useSessionSignalR({ console.log('Event stream signal received via SignalR:', data); if (data.sessionId !== sessionIdRef.current) return; - // Fetch full events from storage (single source of truth), but debounce bursts. - // Session updates arrive via the "newevents" message (tenant group) — no session - // object in this signal to keep payloads minimal. - scheduleFetchEvents(); + // Skip the full event refetch once the session has reached a terminal status — + // agents in the post-completion grace period keep emitting performance/metrics + // snapshots every 15-30s, which would otherwise cause the timeline to thrash and + // scroll-jump while the user is reading. The initial page load already grabbed + // everything; the user can refresh manually if they want to see late trailing events. + // Rule-result + tenant-id side effects still run. + const status = sessionRef.current?.status; + if (!isTerminalStatus(status)) { + // Fetch full events from storage (single source of truth), but debounce bursts. + // Session updates arrive via the "newevents" message (tenant group) — no session + // object in this signal to keep payloads minimal. + scheduleFetchEvents(); + } if (data.tenantId) { setSessionTenantId(prev => prev || data.tenantId); diff --git a/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/page.tsx b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/page.tsx index ef122911..735acfb6 100644 --- a/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/page.tsx +++ b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/page.tsx @@ -91,6 +91,7 @@ export default function SessionDetailPage() { sessionTenantIdFromSession: detail.session?.tenantId, globalAdminMode, sessionIdRef: detail.sessionIdRef, + sessionRef: detail.sessionRef, resolveEffectiveTenantId: detail.resolveEffectiveTenantId, signalR: { on, off, isConnected, joinGroup, leaveGroup }, scheduleFetchEvents: eventsApi.scheduleFetchEvents, diff --git a/src/Web/autopilot-monitor-web/lib/__tests__/sessionStatus.test.ts b/src/Web/autopilot-monitor-web/lib/__tests__/sessionStatus.test.ts new file mode 100644 index 00000000..2bad16f3 --- /dev/null +++ b/src/Web/autopilot-monitor-web/lib/__tests__/sessionStatus.test.ts @@ -0,0 +1,27 @@ +import { describe, it, expect } from "vitest"; +import { isTerminalStatus } from "../../utils/sessionStatus"; + +describe("isTerminalStatus", () => { + it("returns true for terminal statuses", () => { + expect(isTerminalStatus("Succeeded")).toBe(true); + expect(isTerminalStatus("Failed")).toBe(true); + }); + + it("returns false for non-terminal statuses (session can still receive events)", () => { + expect(isTerminalStatus("InProgress")).toBe(false); + expect(isTerminalStatus("Pending")).toBe(false); + expect(isTerminalStatus("Stalled")).toBe(false); + expect(isTerminalStatus("Unknown")).toBe(false); + }); + + it("returns false for null / undefined / empty (status not yet hydrated)", () => { + expect(isTerminalStatus(null)).toBe(false); + expect(isTerminalStatus(undefined)).toBe(false); + expect(isTerminalStatus("")).toBe(false); + }); + + it("is case-sensitive (matches backend enum casing)", () => { + expect(isTerminalStatus("succeeded")).toBe(false); + expect(isTerminalStatus("FAILED")).toBe(false); + }); +}); diff --git a/src/Web/autopilot-monitor-web/utils/sessionStatus.ts b/src/Web/autopilot-monitor-web/utils/sessionStatus.ts new file mode 100644 index 00000000..bfdc8598 --- /dev/null +++ b/src/Web/autopilot-monitor-web/utils/sessionStatus.ts @@ -0,0 +1,8 @@ +// Session status helpers. Mirror of the backend SessionStatus enum +// (src/Shared/AutopilotMonitor.Shared/Models/SessionApiModels.cs). +// Non-terminal: InProgress, Pending, Stalled, Unknown. +// Terminal: Succeeded, Failed. + +export function isTerminalStatus(status: string | null | undefined): boolean { + return status === "Succeeded" || status === "Failed"; +}