From 4667033fb7fb105e408baae36d1fd405bf64f1a8 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Thu, 28 May 2026 17:01:06 +0200 Subject: [PATCH] fix(rules): sunset path is safe-state-first to protect tenant opt-outs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review (Medium): even with the GC-first ordering from b855b2b9, a race remained — after the orphan-GC deleted tenant RuleState rows but BEFORE the global tombstone delete completed, a transient Azure failure on the tombstone would leave the global rule alive while tenant overrides were already gone. For a future default-ENABLED sunset rule this would silently flip RuleState{Enabled=false} (explicit tenant opt-out) into "now using the global default (enabled)" — exactly the regression class the orphan-GC was meant to prevent. Fix: a strict three-step sequence with mutual dependencies. Extracted into a single ProcessSunsetRuleAsync helper so EnsureBuiltInRulesSeededAsync, ReseedBuiltInRulesAsync, and ReseedFromGitHubFunction.ReseedAnalyzeAsync all use the same primitive: 1. Safe-state: write the global rule with Enabled=false + MarkSessionAsFailedDefault=false. Neutralises the default BEFORE touching tenant state. If a default-enabled sunset rule's RuleState{Enabled=false} (opt-out) survives every later step failing, the override now matches the global default — same effective result, no silent flip. 2. Orphan GC across tenants. Only attempted if safe-state succeeded. 3. Tombstone (DeleteAnalyzeRuleAsync global). Only attempted if GC reported zero failures. Returns a SunsetOutcome enum with three explicit failure values (SkippedOnSafeStateFailure / SkippedOnGcFailure / SkippedOnGlobalDeleteFailure) so callers and tests can pin which step bailed. Additionally fixes the same-instance retry gap Codex called out: EnsureBuiltInRulesSeededAsync only sets _seeded=true when EVERY sunset rule reached Completed. Any partial outcome leaves the seed flag false so the next GetAllRulesForTenantAsync call retries without waiting for a process restart. The Function-App can recycle on its own schedule (hours on a warm instance) and we don't want sunset cleanup to wait that long. Behaviour for the original CORR-001/APP-009 case: both shipped Enabled=false globally and the tenant overrides were Enabled=true (opt-IN). Safe-state is a no-op (already Enabled=false), GC removes the opt-ins, tombstone drops the global row. Same end state as before, just retry-safe across transient Azure errors. Tests (10 new sunset cases on top of the previous 6): - Full-path happy case asserts safe-state PRECEDES GC PRECEDES tombstone via a shared CallOrder log. - Safe-state failure path: NO GC + NO tombstone (tenant state preserved). - Per-step failure paths for SkippedOnGcFailure / SkippedOnGlobalDeleteFailure. - _seeded flag stays false on partial sunset, flips to true only on a clean cycle; the "next call retries" scenario is exercised by reconfiguring the mock between two GetAllRulesForTenantAsync calls. Backend 2082/2082. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../AnalyzeRuleServiceSunsetTests.cs | 312 +++++++++++++----- .../Admin/ReseedFromGitHubFunction.cs | 24 +- .../Services/AnalyzeRuleService.cs | 199 +++++++---- 3 files changed, 360 insertions(+), 175 deletions(-) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs index b28c1ac0..31a83e1a 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs @@ -19,83 +19,189 @@ public class AnalyzeRuleServiceSunsetTests private const string SunsetRuleId = "ANALYZE-LEGACY-001"; [Fact] - public async Task EnsureSeed_drops_sunset_rule_and_GCs_orphan_RuleStates() + public async Task EnsureSeed_full_sunset_path_runs_safeState_then_GC_then_tombstone() { - // Arrange: DB has a built-in rule that's no longer in the code catalog - // (BuiltInAnalyzeRules.GetAll() returns the embedded analyze-rules.json — we - // can't easily inject a fake catalog, so we use a ruleId that the real catalog - // doesn't contain). Plus the rule has TWO per-tenant RuleState overrides that - // would normally keep it firing. - var (ruleRepo, deleteAnalyzeCalls, gcCalls) = BuildRepoWithSunsetRule(SunsetRuleId, gcDeleted: 2, gcFailed: 0); + // Happy path: every step succeeds. We assert the side-effects of each step + // landed AND that they happened in the correct order — safe-state (write + // with Enabled=false) MUST precede GC (delete orphan tenant state). + var rig = new SunsetRig(SunsetRuleId, gcDeleted: 2, gcFailed: 0); - var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); - - // Act: a regular GetAllRulesForTenantAsync triggers EnsureBuiltInRulesSeededAsync. + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); await service.GetAllRulesForTenantAsync("a1b2c3d4-e5f6-7890-abcd-ef1234567890"); - // Assert: the sunset rule's per-tenant orphan RuleStates got GC'd AND its - // global row got deleted. The order matters (GC first), but for a clean GC - // the end-state is the same: both call sites recorded. - Assert.Contains(SunsetRuleId, deleteAnalyzeCalls); - Assert.Contains(SunsetRuleId, gcCalls); + Assert.Contains(SunsetRuleId, rig.SafeStateWrites); + Assert.Contains(SunsetRuleId, rig.GcCalls); + Assert.Contains(SunsetRuleId, rig.DeleteAnalyzeCalls); + // Order: safe-state was recorded BEFORE the first GC call. + Assert.True( + rig.CallOrder.IndexOf($"store:{SunsetRuleId}") < rig.CallOrder.IndexOf($"gc:{SunsetRuleId}"), + "Safe-state write must precede the orphan-GC for the same ruleId."); + Assert.True( + rig.CallOrder.IndexOf($"gc:{SunsetRuleId}") < rig.CallOrder.IndexOf($"delete:{SunsetRuleId}"), + "Orphan-GC must precede the global tombstone delete for the same ruleId."); + } + + [Fact] + public async Task EnsureSeed_safeState_failure_short_circuits_before_GC_and_delete() + { + // Codex review (Medium): the previous fix still let tenant RuleStates get + // deleted before a transient Azure error blew up the global tombstone delete, + // which could silently flip a future default-ENABLED sunset rule's + // RuleState{Enabled=false} opt-outs back to enabled (because the tenant + // override is gone and the global is still enabled). + // Fix: safe-state (global Enabled=false) MUST succeed before any tenant state + // is touched. If the safe-state write itself fails, we abort the whole sunset + // sequence for that rule — no GC, no tombstone. + var rig = new SunsetRig(SunsetRuleId, gcDeleted: 0, gcFailed: 0, safeStateOk: false); + + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); + await service.GetAllRulesForTenantAsync("a1b2c3d4-e5f6-7890-abcd-ef1234567890"); + + Assert.Contains(SunsetRuleId, rig.SafeStateWrites); // safe-state was attempted + Assert.DoesNotContain(SunsetRuleId, rig.GcCalls); // GC NOT attempted (tenant state preserved) + Assert.DoesNotContain(SunsetRuleId, rig.DeleteAnalyzeCalls); // global delete NOT attempted } [Fact] public async Task EnsureSeed_partial_GC_failure_keeps_global_row_so_retry_works() { - // Codex review (Medium): if GC reports >0 failed rows, the global delete must NOT - // run — otherwise the rule falls out of the sunset-diff and the remaining orphan - // RuleState rows are unreachable. With the global row kept, the next startup will - // see the rule in the diff again and re-attempt the GC. - var (ruleRepo, deleteAnalyzeCalls, gcCalls) = BuildRepoWithSunsetRule(SunsetRuleId, gcDeleted: 1, gcFailed: 1); + // GC reports >0 failed rows → global delete must NOT run, the rule stays in + // the sunset diff for the next cycle's retry. Safe-state (Enabled=false) is + // already written, so default-enabled sunset rules don't reactivate any opt-out + // tenant in the meantime. + var rig = new SunsetRig(SunsetRuleId, gcDeleted: 1, gcFailed: 1); - var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); await service.GetAllRulesForTenantAsync("a1b2c3d4-e5f6-7890-abcd-ef1234567890"); - Assert.Contains(SunsetRuleId, gcCalls); // GC was attempted - Assert.DoesNotContain(SunsetRuleId, deleteAnalyzeCalls); // global delete SKIPPED on partial GC + Assert.Contains(SunsetRuleId, rig.SafeStateWrites); + Assert.Contains(SunsetRuleId, rig.GcCalls); + Assert.DoesNotContain(SunsetRuleId, rig.DeleteAnalyzeCalls); } [Fact] public async Task EnsureSeed_GC_enumeration_failure_keeps_global_row_too() { - // The gcFailed == -1 sentinel signals "we couldn't enumerate the RuleStates at - // all". Same contract as partial failure: do NOT delete the global row. - var (ruleRepo, deleteAnalyzeCalls, gcCalls) = BuildRepoWithSunsetRule(SunsetRuleId, gcDeleted: 0, gcFailed: -1); + // gcFailed == -1 sentinel = enumeration itself failed (we don't know how many + // orphans are out there). Same contract as partial failure: keep global row. + var rig = new SunsetRig(SunsetRuleId, gcDeleted: 0, gcFailed: -1); - var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); await service.GetAllRulesForTenantAsync("a1b2c3d4-e5f6-7890-abcd-ef1234567890"); - Assert.Contains(SunsetRuleId, gcCalls); - Assert.DoesNotContain(SunsetRuleId, deleteAnalyzeCalls); + Assert.Contains(SunsetRuleId, rig.GcCalls); + Assert.DoesNotContain(SunsetRuleId, rig.DeleteAnalyzeCalls); + } + + [Fact] + public async Task EnsureSeed_partial_sunset_failure_leaves_seed_flag_clearable_for_same_instance_retry() + { + // Codex review (Medium): the previous fix unconditionally set _seeded=true at + // the end of EnsureSeed, so a sunset that didn't complete had to wait for a + // process restart to retry. On long-running Function-App instances this can + // mean hours of delay. New contract: if ANY sunset rule didn't reach + // Completed, _seeded stays false → next GetAllRulesForTenantAsync re-enters + // EnsureSeed and retries the sunset path. + var rig = new SunsetRig(SunsetRuleId, gcDeleted: 0, gcFailed: 1); + + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); + var tenantId = "a1b2c3d4-e5f6-7890-abcd-ef1234567890"; + + await service.GetAllRulesForTenantAsync(tenantId); + var gcCallsAfterFirst = rig.GcCalls.Count; + // Now switch the mock so the retry has a clean GC — should complete on retry. + rig.SetGcReturn(gcDeleted: 1, gcFailed: 0); + + await service.GetAllRulesForTenantAsync(tenantId); + Assert.True(rig.GcCalls.Count > gcCallsAfterFirst, + "Sunset should be retried on the next call when the previous attempt didn't complete."); + Assert.Contains(SunsetRuleId, rig.DeleteAnalyzeCalls); + } + + [Fact] + public async Task EnsureSeed_all_sunsets_completed_sets_seeded_flag() + { + // Complement of the previous test: on a clean sunset cycle, _seeded gets set + // so we don't re-scan on every call. (Verified indirectly: a second call + // doesn't issue another GC against the now-deleted rule.) + var rig = new SunsetRig(SunsetRuleId, gcDeleted: 1, gcFailed: 0); + + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); + var tenantId = "a1b2c3d4-e5f6-7890-abcd-ef1234567890"; + + await service.GetAllRulesForTenantAsync(tenantId); + var gcCallsAfterFirst = rig.GcCalls.Count; + await service.GetAllRulesForTenantAsync(tenantId); + + Assert.Equal(gcCallsAfterFirst, rig.GcCalls.Count); } [Fact] public async Task EnsureSeed_does_not_GC_RuleStates_for_rules_still_in_catalog() { - // Arrange: pick a rule that IS still in the catalog. EnsureSeed must NOT touch - // its RuleState rows even if tenants have overrides — those are legitimate opt-out - // / opt-in settings. + // Live rules with tenant overrides MUST NOT be touched — those are legitimate + // enabled/disabled preferences, not orphans. var stillLiveRuleId = BuiltInAnalyzeRules.GetAll().First().RuleId; - var (ruleRepo, deleteAnalyzeCalls, gcCalls) = BuildRepoWithSunsetRule(stillLiveRuleId, gcDeleted: 1, gcFailed: 0); + var rig = new SunsetRig(stillLiveRuleId, gcDeleted: 1, gcFailed: 0); - var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); await service.GetAllRulesForTenantAsync("a1b2c3d4-e5f6-7890-abcd-ef1234567890"); - // Live rule's RuleState must NOT be GC'd. - Assert.DoesNotContain(stillLiveRuleId, gcCalls); + Assert.DoesNotContain(stillLiveRuleId, rig.GcCalls); + } + + [Fact] + public async Task ProcessSunsetRuleAsync_returns_SkippedOnSafeStateFailure_when_safe_state_fails() + { + // Direct unit test of the helper — covers the safe-state branch independent + // of EnsureSeed plumbing. + var rig = new SunsetRig(SunsetRuleId, gcDeleted: 0, gcFailed: 0, safeStateOk: false); + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); + + var sunsetRule = new AnalyzeRule { RuleId = SunsetRuleId, IsBuiltIn = true, Enabled = true }; + var (outcome, gcd) = await service.ProcessSunsetRuleAsync(sunsetRule); + + Assert.Equal(SunsetOutcome.SkippedOnSafeStateFailure, outcome); + Assert.Equal(0, gcd); + // Safe-state mutated the in-memory rule (defense-in-depth for callers that + // re-use the object — they see the neutralised values, not the original). + Assert.False(sunsetRule.Enabled); + Assert.False(sunsetRule.MarkSessionAsFailedDefault); + } + + [Fact] + public async Task ProcessSunsetRuleAsync_returns_SkippedOnGcFailure_when_GC_reports_failures() + { + var rig = new SunsetRig(SunsetRuleId, gcDeleted: 2, gcFailed: 1); + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); + + var sunsetRule = new AnalyzeRule { RuleId = SunsetRuleId, IsBuiltIn = true, Enabled = true }; + var (outcome, gcd) = await service.ProcessSunsetRuleAsync(sunsetRule); + + Assert.Equal(SunsetOutcome.SkippedOnGcFailure, outcome); + Assert.Equal(2, gcd); + } + + [Fact] + public async Task ProcessSunsetRuleAsync_returns_SkippedOnGlobalDeleteFailure_when_tombstone_fails() + { + var rig = new SunsetRig(SunsetRuleId, gcDeleted: 3, gcFailed: 0, globalDeleteOk: false); + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); + + var sunsetRule = new AnalyzeRule { RuleId = SunsetRuleId, IsBuiltIn = true, Enabled = true }; + var (outcome, gcd) = await service.ProcessSunsetRuleAsync(sunsetRule); + + Assert.Equal(SunsetOutcome.SkippedOnGlobalDeleteFailure, outcome); + Assert.Equal(3, gcd); } [Fact] public async Task ReseedBuiltIn_returns_orphanGc_count_in_tuple() { - // The new ReseedBuiltInRulesAsync return shape is (deleted, written, orphanStatesGcd) - // — pin that so external callers (admin UI, future scripts) can surface the - // cleanup count to the operator. - var (ruleRepo, _, _) = BuildRepoWithSunsetRule(SunsetRuleId, gcDeleted: 3, gcFailed: 0); + var rig = new SunsetRig(SunsetRuleId, gcDeleted: 3, gcFailed: 0); - var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); var (deleted, written, orphanStatesGcd) = await service.ReseedBuiltInRulesAsync(); Assert.True(deleted >= 1, $"Expected at least 1 deletion (incl. sunset), got {deleted}"); @@ -106,70 +212,96 @@ public class AnalyzeRuleServiceSunsetTests [Fact] public async Task ReseedBuiltIn_skips_global_delete_on_partial_GC_failure() { - // Same invariant as the EnsureSeed test — the explicit Reseed path must also - // refuse to drop the global row when GC didn't go all the way through. - var (ruleRepo, deleteAnalyzeCalls, gcCalls) = BuildRepoWithSunsetRule(SunsetRuleId, gcDeleted: 2, gcFailed: 1); + var rig = new SunsetRig(SunsetRuleId, gcDeleted: 2, gcFailed: 1); - var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); + var service = new AnalyzeRuleService(rig.Repo.Object, NullLogger.Instance); var (deleted, written, orphanStatesGcd) = await service.ReseedBuiltInRulesAsync(); - Assert.Contains(SunsetRuleId, gcCalls); - Assert.DoesNotContain(SunsetRuleId, deleteAnalyzeCalls); - // orphanStatesGcd still surfaces the partial successes (2/3 rows deleted before - // the failure) so operators can see progress was made. + Assert.Contains(SunsetRuleId, rig.GcCalls); + Assert.DoesNotContain(SunsetRuleId, rig.DeleteAnalyzeCalls); Assert.Equal(2, orphanStatesGcd); } // ===== Helpers ===== /// - /// Builds a mock that: - /// (a) returns the full built-in catalog PLUS one extra rule with the given - /// on the "global" partition — so the sunset - /// detection has something to find when extraRuleId is not in the catalog; - /// (b) reports the configured GC result via - /// . - /// Returns the mock plus two recorder lists capturing which ruleIds got - /// DeleteAnalyzeRuleAsync and which got DeleteRuleStatesForRuleIdAcrossTenantsAsync - /// invoked. + /// Mock harness for sunset-path tests. Records every storage call against the + /// sunset ruleId — safe-state writes (StoreAnalyzeRuleAsync), orphan GC, and + /// tombstone deletes — and lets the test configure success/failure of each step. + /// The sequence captures relative ordering so tests can + /// assert the safe-state → GC → tombstone invariant directly. /// - private static (Mock repo, List deleteAnalyzeCalls, List gcCalls) - BuildRepoWithSunsetRule(string extraRuleId, int gcDeleted, int gcFailed) + private sealed class SunsetRig { - var deleteAnalyzeCalls = new List(); - var gcCalls = new List(); + public Mock Repo { get; } + public List SafeStateWrites { get; } = new(); + public List GcCalls { get; } = new(); + public List DeleteAnalyzeCalls { get; } = new(); + public List CallOrder { get; } = new(); - // The "existing" set in the DB: real built-ins (so version-bump logic doesn't - // re-write everything every test) PLUS one extra synthetic entry. The extra - // entry mimics a previous build-in that has since been removed from the code. - var existingInDb = BuiltInAnalyzeRules.GetAll().ToList(); - if (!existingInDb.Any(r => r.RuleId == extraRuleId)) + private (int gcDeleted, int gcFailed) _gcReturn; + + public SunsetRig(string sunsetRuleId, int gcDeleted, int gcFailed, + bool safeStateOk = true, bool globalDeleteOk = true) { - existingInDb.Add(new AnalyzeRule + _gcReturn = (gcDeleted, gcFailed); + + // Existing DB = real catalog + the synthetic sunset row. + var existingInDb = BuiltInAnalyzeRules.GetAll().ToList(); + if (!existingInDb.Any(r => r.RuleId == sunsetRuleId)) { - RuleId = extraRuleId, - Title = "Legacy", - Description = "Legacy", - Severity = "warning", - Category = "test", - Version = "1.0.0", - IsBuiltIn = true, - Enabled = false, - }); + existingInDb.Add(new AnalyzeRule + { + RuleId = sunsetRuleId, + Title = "Legacy", + Description = "Legacy", + Severity = "warning", + Category = "test", + Version = "1.0.0", + IsBuiltIn = true, + Enabled = true, + }); + } + + Repo = new Mock(); + Repo.Setup(r => r.GetAnalyzeRulesAsync("global")).ReturnsAsync(existingInDb); + Repo.Setup(r => r.GetAnalyzeRulesAsync(It.Is(s => s != "global"))).ReturnsAsync(new List()); + Repo.Setup(r => r.GetRuleStatesAsync(It.IsAny())).ReturnsAsync(new Dictionary()); + + // StoreAnalyzeRuleAsync: only flip safeStateOk for writes against the + // sunset ruleId — survivors / new rules in the catalog must still write OK. + Repo.Setup(r => r.StoreAnalyzeRuleAsync(It.IsAny(), It.IsAny())) + .Callback((rule, _) => + { + if (rule.RuleId == sunsetRuleId) + { + SafeStateWrites.Add(rule.RuleId); + CallOrder.Add($"store:{rule.RuleId}"); + } + }) + .ReturnsAsync((AnalyzeRule rule, string _) => rule.RuleId == sunsetRuleId ? safeStateOk : true); + + Repo.Setup(r => r.DeleteAnalyzeRuleAsync(It.IsAny(), It.IsAny())) + .Callback((_, ruleId) => + { + DeleteAnalyzeCalls.Add(ruleId); + CallOrder.Add($"delete:{ruleId}"); + }) + .ReturnsAsync((string _, string ruleId) => ruleId == sunsetRuleId ? globalDeleteOk : true); + + Repo.Setup(r => r.DeleteRuleStatesForRuleIdAcrossTenantsAsync(It.IsAny())) + .Callback(ruleId => + { + GcCalls.Add(ruleId); + CallOrder.Add($"gc:{ruleId}"); + }) + .ReturnsAsync(() => _gcReturn); } - var ruleRepo = new Mock(); - ruleRepo.Setup(r => r.GetAnalyzeRulesAsync("global")).ReturnsAsync(existingInDb); - ruleRepo.Setup(r => r.GetAnalyzeRulesAsync(It.Is(s => s != "global"))).ReturnsAsync(new List()); - ruleRepo.Setup(r => r.GetRuleStatesAsync(It.IsAny())).ReturnsAsync(new Dictionary()); - ruleRepo.Setup(r => r.StoreAnalyzeRuleAsync(It.IsAny(), It.IsAny())).ReturnsAsync(true); - ruleRepo.Setup(r => r.DeleteAnalyzeRuleAsync(It.IsAny(), It.IsAny())) - .Callback((_, ruleId) => deleteAnalyzeCalls.Add(ruleId)) - .ReturnsAsync(true); - ruleRepo.Setup(r => r.DeleteRuleStatesForRuleIdAcrossTenantsAsync(It.IsAny())) - .Callback(ruleId => gcCalls.Add(ruleId)) - .ReturnsAsync((gcDeleted, gcFailed)); - - return (ruleRepo, deleteAnalyzeCalls, gcCalls); + /// + /// Reconfigure the GC return value mid-test. Used by the + /// "next call retries after a transient failure" scenario. + /// + public void SetGcReturn(int gcDeleted, int gcFailed) => _gcReturn = (gcDeleted, gcFailed); } } diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs index 59d0cf43..8f7831eb 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs @@ -181,33 +181,21 @@ namespace AutopilotMonitor.Functions.Functions.Admin deleted++; } - // Sunset path: GC-first ordering. If GC fails partially, leave the global - // row in place so the next reseed cycle sees the rule in the sunset diff - // again and retries. + // Sunset path: shared helper on AnalyzeRuleService — same safe-state -> + // GC -> tombstone ordering as the in-process EnsureSeed and ReseedBuiltIn + // paths, so every reseed flavor leaves the same DB invariants. var orphanStatesGcd = 0; var sunsetSkipped = 0; foreach (var sunsetRule in sunset) { - var (gcDeleted, gcFailed) = await _ruleRepo.DeleteRuleStatesForRuleIdAcrossTenantsAsync(sunsetRule.RuleId); - orphanStatesGcd += gcDeleted; - if (gcFailed != 0) - { - _logger.LogWarning( - "GitHub reseed sunset GC partial failure for {RuleId} (deleted={Deleted}, failed={Failed}); keeping global row to retry on next reseed", - sunsetRule.RuleId, gcDeleted, gcFailed); - sunsetSkipped++; - continue; - } - var globalDeleted = await _ruleRepo.DeleteAnalyzeRuleAsync("global", sunsetRule.RuleId); - if (globalDeleted) + var (outcome, gcd) = await _analyzeRuleService.ProcessSunsetRuleAsync(sunsetRule); + orphanStatesGcd += gcd; + if (outcome == AutopilotMonitor.Functions.Services.SunsetOutcome.Completed) { deleted++; } else { - _logger.LogWarning( - "GitHub reseed sunset GC clean but global delete failed for {RuleId}; will retry on next reseed", - sunsetRule.RuleId); sunsetSkipped++; } } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs index fc8cfae1..d3fb720a 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs @@ -12,6 +12,25 @@ namespace AutopilotMonitor.Functions.Services /// in the RuleStates table, so rule definitions can be updated centrally without /// losing per-tenant enabled/disabled preferences. /// + /// + /// Outcome of . Callers use + /// this to decide whether the seed cycle is fully consistent (only + /// ) or whether a retry on the next cycle is needed (any + /// other value). Order of the failure values mirrors the safe-state → GC → delete + /// sequence — earlier failures indicate less state was changed. + /// + public enum SunsetOutcome + { + /// Safe-state, orphan-GC, and global-delete all succeeded. + Completed, + /// Safe-state (set global Enabled=false) failed. No GC or delete attempted. Rule keeps whatever Enabled value it had before — at worst this leaves it active, never silently flipping a tenant opt-out. + SkippedOnSafeStateFailure, + /// Safe-state succeeded, but the cross-tenant orphan-GC failed (per-row or enumeration). Global rule stays as Enabled=false so for the typical default-disabled sunset case the rule is already effectively dead; default-enabled sunsets are also safe because tenant opt-outs still win over the new Enabled=false global default until the GC retry deletes them next cycle. + SkippedOnGcFailure, + /// Safe-state + GC succeeded, but the global tombstone delete failed. Rule is effectively dead (no tenant overrides, global Enabled=false); next seed cycle will retry the tombstone. + SkippedOnGlobalDeleteFailure, + } + public class AnalyzeRuleService { private readonly IRuleRepository _ruleRepo; @@ -24,6 +43,71 @@ namespace AutopilotMonitor.Functions.Services _logger = logger; } + /// + /// Drives the sunset sequence for a single built-in / community rule that is no + /// longer in the code catalog. Three steps, executed in order, with strict + /// "later step requires earlier success" semantics so a partial failure can + /// never silently flip a tenant's enabled/disabled preference for the rule: + /// + /// Safe-state: re-write the global row with + /// Enabled = false and MarkSessionAsFailedDefault = false. + /// Defends future default-ENABLED sunset rules: even if the next two + /// steps fail, the global default is now off, so tenants with + /// RuleState{Enabled=false} (explicit opt-outs) keep their opt-out + /// behaviour (override matches global default, both off). + /// Orphan GC: delete every per-tenant RuleState row whose + /// RowKey matches the sunset rule. Skipped if safe-state failed — we never + /// destroy tenant state before the global default has been neutralised. + /// Tombstone: delete the global rule row. Skipped if GC reported + /// any failures, so the rule remains in the sunset diff for the next seed + /// cycle's retry. + /// + /// + /// The current global rule object (will be mutated by the + /// safe-state step; callers should not reuse it). + /// An outcome + count of orphan RuleState rows that this attempt + /// actually deleted (always >= 0). The count surfaces partial progress even + /// on a result so operators can + /// see the rule getting cleaned up across cycles. + public async Task<(SunsetOutcome outcome, int orphanStatesGcd)> ProcessSunsetRuleAsync(AnalyzeRule rule) + { + // Step 1: safe-state. Neutralise the global default first so any orphan + // state we fail to clean below is harmless. + rule.Enabled = false; + rule.MarkSessionAsFailedDefault = false; + rule.UpdatedAt = DateTime.UtcNow; + var safeStateOk = await _ruleRepo.StoreAnalyzeRuleAsync(rule, "global"); + if (!safeStateOk) + { + _logger.LogWarning( + "Sunset safe-state write failed for {RuleId}; skipping GC + tombstone, will retry next seed cycle", + rule.RuleId); + return (SunsetOutcome.SkippedOnSafeStateFailure, 0); + } + + // Step 2: orphan-GC. + var (gcDeleted, gcFailed) = await _ruleRepo.DeleteRuleStatesForRuleIdAcrossTenantsAsync(rule.RuleId); + if (gcFailed != 0) + { + _logger.LogWarning( + "Sunset GC partial failure for {RuleId} (deleted={Deleted}, failed={Failed}); rule is now safe-stated (Enabled=false) globally, will retry GC + tombstone next seed cycle", + rule.RuleId, gcDeleted, gcFailed); + return (SunsetOutcome.SkippedOnGcFailure, gcDeleted); + } + + // Step 3: tombstone. + var globalDeleted = await _ruleRepo.DeleteAnalyzeRuleAsync("global", rule.RuleId); + if (!globalDeleted) + { + _logger.LogWarning( + "Sunset GC clean but global delete failed for {RuleId}; rule is effectively dead (Enabled=false + no tenant overrides) but the row will be retried next seed cycle", + rule.RuleId); + return (SunsetOutcome.SkippedOnGlobalDeleteFailure, gcDeleted); + } + + return (SunsetOutcome.Completed, gcDeleted); + } + /// /// Gets all active analyze rules for a tenant (enabled only) /// @@ -250,39 +334,23 @@ namespace AutopilotMonitor.Functions.Services deleted++; } - // Sunset path: GC-first ordering. + // Sunset path: full safe-state -> GC -> tombstone sequence via the shared + // helper. Each rule that reaches Completed contributes one to `deleted`. + // Partial-failure paths leave the global row in place for the next cycle. var orphanStatesGcd = 0; - var sunsetSkippedOnGcFailure = 0; - var sunsetSkippedOnGlobalDeleteFailure = 0; + var outcomeCounts = new Dictionary(); foreach (var sunsetRule in sunset) { - var (gcDeleted, gcFailed) = await _ruleRepo.DeleteRuleStatesForRuleIdAcrossTenantsAsync(sunsetRule.RuleId); - orphanStatesGcd += gcDeleted; - if (gcFailed != 0) - { - _logger.LogWarning( - "Reseed sunset GC partial failure for {RuleId} (deleted={Deleted}, failed={Failed}); keeping global row to retry next cycle", - sunsetRule.RuleId, gcDeleted, gcFailed); - sunsetSkippedOnGcFailure++; - continue; - } - var globalDeleted = await _ruleRepo.DeleteAnalyzeRuleAsync("global", sunsetRule.RuleId); - if (globalDeleted) - { + var (outcome, gcd) = await ProcessSunsetRuleAsync(sunsetRule); + orphanStatesGcd += gcd; + outcomeCounts[outcome] = outcomeCounts.GetValueOrDefault(outcome) + 1; + if (outcome == SunsetOutcome.Completed) deleted++; - } - else - { - _logger.LogWarning( - "Reseed sunset GC clean but global delete failed for {RuleId}; will retry next cycle", - sunsetRule.RuleId); - sunsetSkippedOnGlobalDeleteFailure++; - } } _logger.LogInformation( - "Reseed analyze rules: {Deleted} old rows deleted, {Survivors} survivors, {Sunset} sunset (GC skip={GcSkip}, delete skip={DelSkip}, orphan states removed={OrphanGcd})", - deleted, survivors.Count, sunset.Count, sunsetSkippedOnGcFailure, sunsetSkippedOnGlobalDeleteFailure, orphanStatesGcd); + "Reseed analyze rules: {Deleted} rows deleted, {Survivors} survivors, {Sunset} sunset (outcomes {Outcomes}, orphan states removed={OrphanGcd})", + deleted, survivors.Count, sunset.Count, FormatOutcomes(outcomeCounts), orphanStatesGcd); foreach (var rule in builtInRules) { @@ -351,62 +419,59 @@ namespace AutopilotMonitor.Functions.Services } // Sunset detection: rules that existed as built-in but are no longer in - // the code catalog. Delete the global row AND fan out the per-tenant - // RuleState GC so opt-in overrides don't linger as dead state. - // - // Order matters and is deliberate: orphan-GC FIRST, then global delete. - // If GC fails partially we leave the global row in place so the next - // startup sees the rule in the sunset diff again and retries the cleanup. - // The previous order (global-delete-first) lost retry-ability because - // once the global row is gone the diff goes empty and any remaining - // orphan RuleStates would linger indefinitely. + // the code catalog. ProcessSunsetRuleAsync handles the safe-state -> GC + // -> tombstone sequence per rule with strict ordering guarantees (see + // its XML doc). Any non-Completed outcome means there's still work to + // do — we leave _seeded = false so the next GetAllRulesForTenantAsync + // call retries the seed without waiting for a process restart. var sunsetRules = existingRules .Where(r => r.IsBuiltIn && !newCatalogIds.Contains(r.RuleId)) .ToList(); + var allSunsetsClean = true; if (sunsetRules.Count > 0) { var orphanStatesGcd = 0; - var skippedOnGcFailure = 0; - var skippedOnGlobalDeleteFailure = 0; + var outcomeCounts = new Dictionary(); foreach (var sunset in sunsetRules) { - var (gcDeleted, gcFailed) = await _ruleRepo.DeleteRuleStatesForRuleIdAcrossTenantsAsync(sunset.RuleId); - if (gcFailed != 0) - { - // Either per-row error (failed > 0) or enumeration failure (-1). - // Either way we cannot safely drop the global row — retry next - // startup. Orphan-State rows that DID get deleted in this pass - // are gone for good, but that's fine — the per-row delete is - // already 404-idempotent. - _logger.LogWarning( - "Sunset GC partial failure for {RuleId} (deleted={Deleted}, failed={Failed}); keeping global row to retry on next seed cycle", - sunset.RuleId, gcDeleted, gcFailed); - skippedOnGcFailure++; - orphanStatesGcd += gcDeleted; - continue; - } - - var globalDeleted = await _ruleRepo.DeleteAnalyzeRuleAsync("global", sunset.RuleId); - if (!globalDeleted) - { - // GC was clean but the global delete itself failed (transient - // Azure error). The orphan states are already gone; the global - // row will be retried on next startup (GC will then no-op). - _logger.LogWarning( - "Sunset GC clean but global delete failed for {RuleId}; will retry next seed cycle", - sunset.RuleId); - skippedOnGlobalDeleteFailure++; - } - orphanStatesGcd += gcDeleted; + var (outcome, gcd) = await ProcessSunsetRuleAsync(sunset); + orphanStatesGcd += gcd; + outcomeCounts[outcome] = outcomeCounts.GetValueOrDefault(outcome) + 1; + if (outcome != SunsetOutcome.Completed) + allSunsetsClean = false; } _logger.LogInformation( - "Sunset {RuleCount} built-in analyze rule(s); GC'd {OrphanCount} orphan per-tenant RuleState row(s); skipped {GcSkip} on GC failure, {DelSkip} on global-delete failure", - sunsetRules.Count, orphanStatesGcd, skippedOnGcFailure, skippedOnGlobalDeleteFailure); + "Sunset {RuleCount} built-in analyze rule(s); orphan RuleState rows GC'd {OrphanCount}; outcomes {Outcomes}", + sunsetRules.Count, orphanStatesGcd, FormatOutcomes(outcomeCounts)); + } + + // Only flip the seed-once flag when EVERY sunset rule reached the + // Completed terminal. If any was partial, the next call will retry — + // important on long-running Function-App instances that don't recycle + // often. (Survivors / new-rule writes above are idempotent: they all + // go through Upsert and don't churn DB state on retry.) + if (!allSunsetsClean) + { + _logger.LogInformation("One or more sunset rules did not complete; leaving _seeded=false so the next GetAllRulesForTenantAsync retries."); + return; } } _seeded = true; } + + /// + /// Compact log-friendly rendering of the sunset-outcome histogram. Inline so + /// the LogInformation site stays readable. Order matches the SunsetOutcome enum + /// (Completed first, then failures in execution-step order). + /// + private static string FormatOutcomes(Dictionary counts) + { + return string.Join(", ", + Enum.GetValues() + .Where(o => counts.GetValueOrDefault(o) > 0) + .Select(o => $"{o}={counts[o]}")); + } } }