diff --git a/docs/design/enrollment-status-reclassification-plan.md b/docs/design/enrollment-status-reclassification-plan.md index 6e128587..8ff5eca3 100644 --- a/docs/design/enrollment-status-reclassification-plan.md +++ b/docs/design/enrollment-status-reclassification-plan.md @@ -62,16 +62,18 @@ timeline AND let the backend terminalize precisely instead of guessing with grac minimal emergency client wired at the call site (or move the emit to a point where `auth` exists). May be lost if the device is fully offline — acceptable (best-effort, that case falls back to grace). -## PR3 — Reconciliation of late completions +## PR3 — Reconciliation of late completions ✅ done -- [ ] Allow `Failed` / `Incomplete` / `AwaitingUser` → `Succeeded` when a genuine terminal - arrives (`enrollment_complete`, `whiteglove_complete`, AccountSetup all-succeeded). - Audit the `UpdateSessionStatusAsync` terminal-guard so ONLY these reconcile signals may - upgrade an already-terminal row (no other Failed→X path opens up). -- [ ] Verify `EventIngestProcessor.Classification.cs` routes a late `enrollment_complete` - through this path (it already sets `Succeeded`; confirm the guard). -- [ ] Tests: `SessionCompletionTimestampTests` + new reconcile test (sweep→Failed, then late - `enrollment_complete` → Succeeded). +- [x] `UpdateSessionStatusAsync` terminal-guard replaced by pure `IsTerminalTransitionAllowed`: + a genuine completion (`Succeeded`) UPGRADES a prior `Failed`/`Incomplete`/`AwaitingUser` + verdict (late reconcile); silent-terminal verdicts + `AwaitingUser` never overwrite a terminal. +- [x] On `Succeeded`, clear stale `FailureReason` + `FailureSnapshotJson` (reconcile hygiene). +- [x] Admin-marked terminal decisions are preserved — a late completion does NOT auto-override a + hand-marked `Failed`/`Incomplete`. +- [x] Ingest already routes late `enrollment_complete`/`gather` → `Succeeded` with no pre-terminal + skip, so the relaxed guard makes reconcile work end-to-end (no ingest change needed). +- [x] Tests `SessionStatusTransitionTests` (20, full matrix). Suite: 2705 pass (one pre-existing + timing-flaky maintenance test, green on retry/isolation — unrelated). ## PR4 — Stats / metrics expose 3 states diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/SessionStatusTransitionTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/SessionStatusTransitionTests.cs new file mode 100644 index 00000000..03302d16 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/SessionStatusTransitionTests.cs @@ -0,0 +1,63 @@ +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared.Models; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Pins the terminal/reconcile transition matrix (docs/design/enrollment-status-reclassification.md): +/// a genuine completion may upgrade a prior Failed/Incomplete/AwaitingUser verdict (late reconcile), +/// while the silent-terminal verdicts never overwrite an existing terminal. This is the pure core of +/// PR3 — the reconcile that lets a device which completed after a sweep-timeout heal to Succeeded. +/// +public class SessionStatusTransitionTests +{ + // ---- Succeeded may UPGRADE any non-Succeeded state (reconcile) ---- + [Theory] + [InlineData("Failed", true)] + [InlineData("Incomplete", true)] + [InlineData("AwaitingUser", true)] + [InlineData("InProgress", true)] + [InlineData("Stalled", true)] + [InlineData("Succeeded", false)] // idempotent — already succeeded + public void Succeeded_upgrades_everything_except_an_existing_success(string existing, bool allowed) + { + Assert.Equal(allowed, TableStorageService.IsTerminalTransitionAllowed(existing, SessionStatus.Succeeded)); + } + + // ---- Failed / Incomplete never overwrite an existing terminal ---- + [Theory] + [InlineData("Succeeded", false)] + [InlineData("Failed", false)] + [InlineData("Incomplete", false)] + [InlineData("AwaitingUser", true)] // AwaitingUser is non-terminal + [InlineData("InProgress", true)] + [InlineData("Stalled", true)] + public void Failed_and_Incomplete_never_overwrite_a_terminal(string existing, bool allowed) + { + Assert.Equal(allowed, TableStorageService.IsTerminalTransitionAllowed(existing, SessionStatus.Failed)); + Assert.Equal(allowed, TableStorageService.IsTerminalTransitionAllowed(existing, SessionStatus.Incomplete)); + } + + // ---- AwaitingUser never regresses a terminal ---- + [Theory] + [InlineData("Succeeded", false)] + [InlineData("Failed", false)] + [InlineData("Incomplete", false)] + [InlineData("InProgress", true)] + [InlineData("Stalled", true)] + public void AwaitingUser_never_regresses_a_terminal(string existing, bool allowed) + { + Assert.Equal(allowed, TableStorageService.IsTerminalTransitionAllowed(existing, SessionStatus.AwaitingUser)); + } + + // ---- Non-terminal incoming is governed by downstream guards, allowed here ---- + [Theory] + [InlineData(SessionStatus.InProgress)] + [InlineData(SessionStatus.Stalled)] + [InlineData(SessionStatus.Pending)] + public void Non_terminal_incoming_is_allowed_here(SessionStatus incoming) + { + Assert.True(TableStorageService.IsTerminalTransitionAllowed("Failed", incoming)); + Assert.True(TableStorageService.IsTerminalTransitionAllowed("Succeeded", incoming)); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs index bcda14a9..232842d9 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs @@ -1345,6 +1345,33 @@ namespace AutopilotMonitor.Functions.Services /// back to the session's LastEventAt and only to UtcNow as a last resort, so DurationSeconds /// reflects when the session went silent rather than when the operator clicked the button. /// + /// + /// Pure gate for the terminal/reconcile status rules (docs/design/enrollment-status-reclassification.md). + /// Returns whether a write to may proceed given the persisted + /// string: + /// + /// Succeeded may UPGRADE any non-Succeeded state (late completion reconcile); idempotent no-op if already Succeeded. + /// Failed / Incomplete (silent-terminal verdicts) never overwrite an existing terminal (Succeeded/Failed/Incomplete). + /// AwaitingUser never regresses an existing terminal. + /// Non-terminal incoming statuses (InProgress/Stalled/Pending) are allowed here and governed by their own downstream guards. + /// + /// Static + pure so the reconcile matrix is unit-testable without Table Storage. + /// + internal static bool IsTerminalTransitionAllowed(string? existingStatus, SessionStatus incoming) + { + bool existingTerminal = existingStatus == SessionStatus.Succeeded.ToString() + || existingStatus == SessionStatus.Failed.ToString() + || existingStatus == SessionStatus.Incomplete.ToString(); + + return incoming switch + { + SessionStatus.Succeeded => existingStatus != SessionStatus.Succeeded.ToString(), + SessionStatus.Failed or SessionStatus.Incomplete => !existingTerminal, + SessionStatus.AwaitingUser => !existingTerminal, + _ => true, + }; + } + public async Task UpdateSessionStatusAsync(string tenantId, string sessionId, SessionStatus status, EnrollmentPhase? currentPhase = null, string? failureReason = null, DateTime? completedAt = null, DateTime? earliestEventTimestamp = null, DateTime? latestEventTimestamp = null, bool? isPreProvisioned = null, bool? isUserDriven = null, DateTime? resumedAt = null, DateTime? stalledAt = null, bool clearStalledAt = false, bool clearFailureReason = false, string? failureSource = null, string? adminMarkedAction = null, string? failureSnapshotJson = null) { SecurityValidator.EnsureValidGuid(tenantId, nameof(tenantId)); @@ -1363,26 +1390,28 @@ namespace AutopilotMonitor.Functions.Services var entity = await tableClient.GetEntityAsync(tenantId, sessionId); var session = entity.Value; - // Idempotency: never overwrite one terminal state with another. Terminal states - // are Succeeded, Failed and Incomplete (docs/design/enrollment-status-reclassification.md). - // Reconciling a Failed/Incomplete session UP to Succeeded on a late completion is - // done on the dedicated reconcile path, not here. + // Terminal / reconcile transition gate (docs/design/enrollment-status-reclassification.md): + // a genuine completion (Succeeded) may UPGRADE a prior Failed/Incomplete/AwaitingUser + // verdict — a device that reached the desktop is enrolled regardless of an earlier timeout + // guess — but the silent-terminal verdicts (Failed/Incomplete) and AwaitingUser never + // overwrite an existing terminal. Non-terminal incoming statuses fall through to their + // own guards below. var existingStatusStr = session.GetString("Status"); - bool incomingTerminal = status == SessionStatus.Succeeded - || status == SessionStatus.Failed - || status == SessionStatus.Incomplete; - bool existingTerminal = existingStatusStr == SessionStatus.Succeeded.ToString() - || existingStatusStr == SessionStatus.Failed.ToString() - || existingStatusStr == SessionStatus.Incomplete.ToString(); - if (incomingTerminal && existingTerminal) + if (!IsTerminalTransitionAllowed(existingStatusStr, status)) { - _logger.LogInformation($"Session {sessionId} already in terminal state '{existingStatusStr}', skipping status update to '{status}'"); + _logger.LogInformation($"Session {sessionId}: transition '{existingStatusStr}' → '{status}' not allowed (terminal/reconcile guard), skipping"); return false; } - // AwaitingUser is non-terminal but must never regress a terminal session. - if (status == SessionStatus.AwaitingUser && existingTerminal) + + // Preserve an administrator's explicit terminal decision: the late-completion reconcile + // (Succeeded upgrading a Failed/Incomplete row) must never silently override a session an + // admin marked terminal by hand (AdminMarkedAction is set only by the Mark*Session functions). + if (status == SessionStatus.Succeeded + && !string.IsNullOrEmpty(session.GetString("AdminMarkedAction")) + && (existingStatusStr == SessionStatus.Failed.ToString() + || existingStatusStr == SessionStatus.Incomplete.ToString())) { - _logger.LogInformation($"Session {sessionId} already terminal '{existingStatusStr}', skipping AwaitingUser update"); + _logger.LogInformation($"Session {sessionId}: admin-marked terminal '{existingStatusStr}', not auto-reconciling to Succeeded"); return false; } @@ -1425,6 +1454,11 @@ namespace AutopilotMonitor.Functions.Services if (status == SessionStatus.Succeeded) { update["CurrentPhase"] = (int)EnrollmentPhase.Complete; + // Reconcile hygiene: a session that (re)reaches Succeeded carries no failure reason or + // snapshot — clear anything left from a prior Failed/Incomplete/AwaitingUser verdict so + // a late-completed device doesn't keep showing a stale "timed out" reason. + update["FailureReason"] = string.Empty; + update["FailureSnapshotJson"] = string.Empty; } else if (status == SessionStatus.Failed) {