From b855b2b927122c99b2ad2f0fdd13e5c3365de0f8 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Thu, 28 May 2026 16:46:24 +0200 Subject: [PATCH] fix(rules): retry-safe sunset GC + idempotent point-deletes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of the sunset orphan-GC introduced in dff5ba65 surfaced three issues; all three are valid and now fixed: (Medium) The previous sequence was "delete global rule first, then GC per-tenant RuleStates". If the GC failed partially (per-row error) or entirely (cross-partition enumeration error), the global rule was already gone — the sunset diff went empty on the next seed cycle and the surviving orphan RuleStates would linger indefinitely. Worst case for a future default-ENABLED sunset rule: tenants that had explicitly disabled it would lose the disable, silently re-enabling something they didn't want. Three coordinated fixes: 1. Storage methods made idempotent: DeleteAnalyzeRuleAsync and DeleteRuleStateAsync now treat Azure 404 (row already gone) as success instead of returning false. The 404-on-individual-row case inside the cross-tenant GC enumeration is also counted as success — a row that was raced-deleted between the query and our delete is a clean state, not a failure. 2. Cross-tenant GC return shape changed from `int` to `(int deleted, int failed)`: - `deleted` counts fresh deletes AND 404s (idempotent retries) - `failed > 0` signals at least one per-row delete threw a non-404 - `failed == -1` signals the cross-partition enumeration itself failed — we genuinely don't know how many orphans are still out there Callers must check `failed != 0` before proceeding to the global delete. The integer-only return shape couldn't distinguish "GC found nothing" from "GC failed silently". 3. Caller sequence inverted to GC-FIRST in all three sunset code paths: - AnalyzeRuleService.EnsureBuiltInRulesSeededAsync (on-startup seed) - AnalyzeRuleService.ReseedBuiltInRulesAsync (explicit reseed) - ReseedFromGitHubFunction.ReseedAnalyzeAsync (admin UI button) GC is attempted first. The global rule is deleted ONLY when GC returns `failed == 0`. On partial GC failure the rule stays in the global catalog so the next seed/reseed cycle sees it in the sunset diff again and re-attempts the cleanup. End state is eventually consistent even across transient Azure errors. (Low) The GitHub-reseed HTTP response shape was extended from `{ deleted, written }` to `{ deleted, written, orphanStatesGcd, sunsetSkipped }` so operators can see the cleanup count + any rules that got skipped for the retry loop. Tests: - 3 new sunset-mechanism tests covering partial GC failure, enumeration failure (-1 sentinel), and the still-living-rule no-touch invariant. - Existing ReseedBuiltIn_returns_orphanGc_count_in_tuple updated to the new helper shape. - ReseedBuiltIn_skips_global_delete_on_partial_GC_failure pins the retry-safe semantics in the explicit reseed path. Coverage: backend 2076/2076 (+3 sunset edge-case tests). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../AnalyzeRuleServiceSunsetTests.cs | 68 ++++++++-- .../TableStorage/TableRuleRepository.cs | 2 +- .../Admin/ReseedFromGitHubFunction.cs | 74 ++++++---- .../Services/AnalyzeRuleService.cs | 126 +++++++++++++----- .../Services/TableStorageService.Rules.cs | 55 ++++++-- .../DataAccess/IRuleRepository.cs | 11 +- 6 files changed, 251 insertions(+), 85 deletions(-) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs index d06bd162..b28c1ac0 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs @@ -26,19 +26,50 @@ public class AnalyzeRuleServiceSunsetTests // 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, orphanCount: 2); + var (ruleRepo, deleteAnalyzeCalls, gcCalls) = BuildRepoWithSunsetRule(SunsetRuleId, gcDeleted: 2, gcFailed: 0); var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); // Act: a regular GetAllRulesForTenantAsync triggers EnsureBuiltInRulesSeededAsync. await service.GetAllRulesForTenantAsync("a1b2c3d4-e5f6-7890-abcd-ef1234567890"); - // Assert: the sunset rule's global row got deleted AND its per-tenant orphan - // RuleStates got GC'd via the cross-tenant cleanup method. + // 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); } + [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); + + var service = new AnalyzeRuleService(ruleRepo.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 + } + + [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); + + var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); + await service.GetAllRulesForTenantAsync("a1b2c3d4-e5f6-7890-abcd-ef1234567890"); + + Assert.Contains(SunsetRuleId, gcCalls); + Assert.DoesNotContain(SunsetRuleId, deleteAnalyzeCalls); + } + [Fact] public async Task EnsureSeed_does_not_GC_RuleStates_for_rules_still_in_catalog() { @@ -46,7 +77,7 @@ public class AnalyzeRuleServiceSunsetTests // its RuleState rows even if tenants have overrides — those are legitimate opt-out // / opt-in settings. var stillLiveRuleId = BuiltInAnalyzeRules.GetAll().First().RuleId; - var (ruleRepo, deleteAnalyzeCalls, gcCalls) = BuildRepoWithSunsetRule(stillLiveRuleId, orphanCount: 1); + var (ruleRepo, deleteAnalyzeCalls, gcCalls) = BuildRepoWithSunsetRule(stillLiveRuleId, gcDeleted: 1, gcFailed: 0); var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); @@ -54,8 +85,6 @@ public class AnalyzeRuleServiceSunsetTests // Live rule's RuleState must NOT be GC'd. Assert.DoesNotContain(stillLiveRuleId, gcCalls); - // Live rule may have been overwritten by version-bump update logic — that's - // fine, only the GC path matters for this invariant. } [Fact] @@ -64,7 +93,7 @@ public class AnalyzeRuleServiceSunsetTests // 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, orphanCount: 3); + var (ruleRepo, _, _) = BuildRepoWithSunsetRule(SunsetRuleId, gcDeleted: 3, gcFailed: 0); var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); var (deleted, written, orphanStatesGcd) = await service.ReseedBuiltInRulesAsync(); @@ -74,6 +103,23 @@ public class AnalyzeRuleServiceSunsetTests Assert.Equal(3, orphanStatesGcd); } + [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 service = new AnalyzeRuleService(ruleRepo.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.Equal(2, orphanStatesGcd); + } + // ===== Helpers ===== /// @@ -81,14 +127,14 @@ public class AnalyzeRuleServiceSunsetTests /// (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 from the cross-tenant - /// RuleState GC method. + /// (b) reports the configured GC result via + /// . /// Returns the mock plus two recorder lists capturing which ruleIds got /// DeleteAnalyzeRuleAsync and which got DeleteRuleStatesForRuleIdAcrossTenantsAsync /// invoked. /// private static (Mock repo, List deleteAnalyzeCalls, List gcCalls) - BuildRepoWithSunsetRule(string extraRuleId, int orphanCount) + BuildRepoWithSunsetRule(string extraRuleId, int gcDeleted, int gcFailed) { var deleteAnalyzeCalls = new List(); var gcCalls = new List(); @@ -122,7 +168,7 @@ public class AnalyzeRuleServiceSunsetTests .ReturnsAsync(true); ruleRepo.Setup(r => r.DeleteRuleStatesForRuleIdAcrossTenantsAsync(It.IsAny())) .Callback(ruleId => gcCalls.Add(ruleId)) - .ReturnsAsync(orphanCount); + .ReturnsAsync((gcDeleted, gcFailed)); return (ruleRepo, deleteAnalyzeCalls, gcCalls); } diff --git a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableRuleRepository.cs b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableRuleRepository.cs index 681b25c9..c7e18d55 100644 --- a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableRuleRepository.cs +++ b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableRuleRepository.cs @@ -48,7 +48,7 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage public Task DeleteRuleStateAsync(string tenantId, string ruleId) => _storage.DeleteRuleStateAsync(tenantId, ruleId); - public Task DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId) + public Task<(int deleted, int failed)> DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId) => _storage.DeleteRuleStatesForRuleIdAcrossTenantsAsync(ruleId); public Task StoreAnalyzeRuleAsync(AnalyzeRule rule, string tenantId = "global") diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs index eb8402f7..59d0cf43 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs @@ -64,7 +64,7 @@ namespace AutopilotMonitor.Functions.Functions.Admin _logger.LogInformation($"Reseed from GitHub triggered by Global Admin {upn}, type={typeParam}"); var gatherResult = new { deleted = 0, written = 0 }; - var analyzeResult = new { deleted = 0, written = 0 }; + var analyzeResult = new { deleted = 0, written = 0, orphanStatesGcd = 0, sunsetSkipped = 0 }; var imeResult = new { deleted = 0, written = 0 }; if (typeParam == "all" || typeParam == "gather") @@ -77,8 +77,8 @@ namespace AutopilotMonitor.Functions.Functions.Admin if (typeParam == "all" || typeParam == "analyze") { var rules = await _gitHubRepo.FetchAnalyzeRulesAsync(); - var (d, w) = await ReseedAnalyzeAsync(rules); - analyzeResult = new { deleted = d, written = w }; + var (d, w, gcd, skipped) = await ReseedAnalyzeAsync(rules); + analyzeResult = new { deleted = d, written = w, orphanStatesGcd = gcd, sunsetSkipped = skipped }; } if (typeParam == "all" || typeParam == "ime") @@ -159,29 +159,59 @@ namespace AutopilotMonitor.Functions.Functions.Admin return (deleted, rules.Count); } - private async Task<(int deleted, int written)> ReseedAnalyzeAsync(List rules) + private async Task<(int deleted, int written, int orphanStatesGcd, int sunsetSkipped)> ReseedAnalyzeAsync(List rules) { var existing = await _ruleRepo.GetAnalyzeRulesAsync("global"); - - // Identify sunset rules BEFORE the delete pass so we can fan out the - // per-tenant RuleState orphan GC after the catalog is rebuilt. A rule is - // "sunset" if it existed as built-in / community in the global partition but - // isn't in the new GitHub-fetched catalog. Mirrors the sunset handling in - // AnalyzeRuleService.EnsureBuiltInRulesSeededAsync — kept symmetric so - // every reseed path leaves the same DB invariants. var newCatalogIds = rules.Select(r => r.RuleId).ToHashSet(System.StringComparer.Ordinal); - var sunsetRuleIds = existing - .Where(r => (r.IsBuiltIn || r.IsCommunity) && !newCatalogIds.Contains(r.RuleId)) - .Select(r => r.RuleId) - .ToList(); + + // Split existing built-in / community rows into survivors (in new catalog + // → re-imported below) and sunsets (NOT in new catalog → orphan-state GC + // first, global delete only on clean GC). The sunset ordering mirrors + // AnalyzeRuleService.EnsureBuiltInRulesSeededAsync — kept symmetric so every + // reseed path leaves the same DB invariants and partial failures retry. + var survivors = existing.Where(r => (r.IsBuiltIn || r.IsCommunity) && newCatalogIds.Contains(r.RuleId)).ToList(); + var sunset = existing.Where(r => (r.IsBuiltIn || r.IsCommunity) && !newCatalogIds.Contains(r.RuleId)).ToList(); var deleted = 0; - foreach (var rule in existing.Where(r => r.IsBuiltIn || r.IsCommunity)) + + // Survivor path: delete then re-write (legacy behaviour). + foreach (var rule in survivors) { await _ruleRepo.DeleteAnalyzeRuleAsync("global", rule.RuleId); 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. + 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) + { + deleted++; + } + else + { + _logger.LogWarning( + "GitHub reseed sunset GC clean but global delete failed for {RuleId}; will retry on next reseed", + sunsetRule.RuleId); + sunsetSkipped++; + } + } + foreach (var rule in rules) { // Community rules keep IsCommunity=true from JSON; all others are built-in @@ -192,16 +222,10 @@ namespace AutopilotMonitor.Functions.Functions.Admin await _ruleRepo.StoreAnalyzeRuleAsync(rule, "global"); } - var orphanStatesGcd = 0; - foreach (var sunsetRuleId in sunsetRuleIds) - { - orphanStatesGcd += await _ruleRepo.DeleteRuleStatesForRuleIdAcrossTenantsAsync(sunsetRuleId); - } - _logger.LogInformation( - "GitHub reseed analyze: {Deleted} deleted, {Written} written, {OrphanCount} orphan per-tenant RuleState(s) cleaned for {SunsetCount} sunset rule(s)", - deleted, rules.Count, orphanStatesGcd, sunsetRuleIds.Count); - return (deleted, rules.Count); + "GitHub reseed analyze: {Deleted} deleted, {Written} written, {OrphanCount} orphan per-tenant RuleState(s) cleaned across {SunsetTotal} sunset rule(s) ({Skipped} skipped on failure for retry)", + deleted, rules.Count, orphanStatesGcd, sunset.Count, sunsetSkipped); + return (deleted, rules.Count, orphanStatesGcd, sunsetSkipped); } private async Task<(int deleted, int written)> ReseedImeAsync(List patterns) diff --git a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs index 94ae554d..fc8cfae1 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs @@ -215,11 +215,16 @@ namespace AutopilotMonitor.Functions.Services } /// - /// Re-imports all built-in analyze rules into the global partition. Deletes old - /// global built-in rules and writes current code definitions. For rules that - /// existed before but are NOT in the new catalog (sunset rules), this also GCs - /// the per-tenant RuleState rows that would otherwise linger as orphan - /// state. Tenant RuleStates for rules that are still in the catalog are preserved. + /// Re-imports all built-in analyze rules into the global partition. The previous + /// "delete all built-ins, then re-import" approach is preserved for the rules + /// that survive the new catalog; for SUNSET rules (existed in DB as built-in but + /// no longer in ) the sequence is inverted — + /// orphan-state GC first, global delete only on a clean GC — so a partial + /// failure leaves the rule in the diff to be retried on the next seed cycle. + /// + /// Returns (deleted, written, orphanStatesGcd) so the admin endpoint can + /// surface the cleanup count to the operator. + /// /// public async Task<(int deleted, int written, int orphanStatesGcd)> ReseedBuiltInRulesAsync() { @@ -229,22 +234,55 @@ namespace AutopilotMonitor.Functions.Services var builtInRules = BuiltInAnalyzeRules.GetAll(); var newCatalogIds = builtInRules.Select(r => r.RuleId).ToHashSet(StringComparer.Ordinal); - // Identify sunset rule IDs BEFORE the delete pass so we can fan out the - // per-tenant RuleState GC after the catalog is rebuilt. A rule is "sunset" - // if it existed in the global partition (as built-in) but isn't in the - // new catalog. - var sunsetRuleIds = existingGlobalRules - .Where(r => r.IsBuiltIn && !newCatalogIds.Contains(r.RuleId)) - .Select(r => r.RuleId) - .ToList(); + // Survivors: existing built-ins that ARE in the new catalog — these get the + // legacy "delete then re-write" treatment (preserves UpdatedAt etc.). + // Sunset: existing built-ins NOT in the new catalog — these get the + // retry-safe "GC first, global delete on clean GC" treatment. + var survivors = existingGlobalRules.Where(r => r.IsBuiltIn && newCatalogIds.Contains(r.RuleId)).ToList(); + var sunset = existingGlobalRules.Where(r => r.IsBuiltIn && !newCatalogIds.Contains(r.RuleId)).ToList(); var deleted = 0; - foreach (var rule in existingGlobalRules.Where(r => r.IsBuiltIn)) + + // Survivor path: same as before — delete then write. + foreach (var rule in survivors) { await _ruleRepo.DeleteAnalyzeRuleAsync("global", rule.RuleId); deleted++; } - _logger.LogInformation($"Deleted {deleted} old global built-in analyze rules"); + + // Sunset path: GC-first ordering. + var orphanStatesGcd = 0; + var sunsetSkippedOnGcFailure = 0; + var sunsetSkippedOnGlobalDeleteFailure = 0; + 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) + { + 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); foreach (var rule in builtInRules) { @@ -252,21 +290,6 @@ namespace AutopilotMonitor.Functions.Services } _logger.LogInformation($"Written {builtInRules.Count} built-in analyze rules from code"); - // Orphan-RuleState GC for sunset rules — see method-level XML doc for the - // motivation. Best-effort: failures on individual rows are logged inside the - // storage method, not surfaced here. - var orphanStatesGcd = 0; - foreach (var sunsetRuleId in sunsetRuleIds) - { - orphanStatesGcd += await _ruleRepo.DeleteRuleStatesForRuleIdAcrossTenantsAsync(sunsetRuleId); - } - if (orphanStatesGcd > 0) - { - _logger.LogInformation( - "Cleaned up {Count} orphan per-tenant RuleState row(s) for {SunsetCount} sunset built-in rule(s)", - orphanStatesGcd, sunsetRuleIds.Count); - } - _seeded = false; return (deleted, builtInRules.Count, orphanStatesGcd); @@ -330,6 +353,13 @@ 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. var sunsetRules = existingRules .Where(r => r.IsBuiltIn && !newCatalogIds.Contains(r.RuleId)) .ToList(); @@ -337,14 +367,42 @@ namespace AutopilotMonitor.Functions.Services if (sunsetRules.Count > 0) { var orphanStatesGcd = 0; + var skippedOnGcFailure = 0; + var skippedOnGlobalDeleteFailure = 0; foreach (var sunset in sunsetRules) { - await _ruleRepo.DeleteAnalyzeRuleAsync("global", sunset.RuleId); - orphanStatesGcd += await _ruleRepo.DeleteRuleStatesForRuleIdAcrossTenantsAsync(sunset.RuleId); + 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; } _logger.LogInformation( - "Sunset {RuleCount} built-in analyze rule(s) no longer in the code catalog; GC'd {OrphanCount} orphan per-tenant RuleState row(s)", - sunsetRules.Count, orphanStatesGcd); + "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); } } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs index d085e92d..bdad1414 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs @@ -318,7 +318,9 @@ namespace AutopilotMonitor.Functions.Services } /// - /// Deletes the rule state for a tenant (resets to rule's default enabled state) + /// Deletes the rule state for a tenant (resets to rule's default enabled state). + /// Idempotent: a 404 (the row was already gone) counts as success so callers can + /// retry safely without flipping their happy-path on the missing-row condition. /// public async Task DeleteRuleStateAsync(string tenantId, string ruleId) { @@ -330,6 +332,10 @@ namespace AutopilotMonitor.Functions.Services await tableClient.DeleteEntityAsync(tenantId, ruleId); return true; } + catch (Azure.RequestFailedException ex) when (ex.Status == 404) + { + return true; + } catch (Exception ex) { _logger.LogError(ex, $"Failed to delete rule state {ruleId} for {tenantId}"); @@ -342,13 +348,21 @@ namespace AutopilotMonitor.Functions.Services /// RowKey matches . Used as orphan-GC when a built-in /// rule is removed from the global catalog — without this, per-tenant /// RuleState{Enabled=true} overrides survive the sunset and would re-fire - /// the moment the rule was ever re-introduced under the same ruleId. Best-effort: - /// failures on individual rows are logged and skipped (no transactionality across - /// partitions). Returns the actual number of rows deleted. + /// the moment the rule was ever re-introduced under the same ruleId. + /// + /// Returns a tuple of (deleted, failed). deleted counts both fresh + /// deletes and 404 (the row was already gone — idempotent retry). failed + /// is non-zero if any individual delete threw a non-404 error, or + /// -1 if the cross-partition enumeration itself failed (we can't tell how + /// many rows are still out there). Callers must check failed before + /// proceeding with the global rule delete — otherwise a partial GC followed by a + /// successful catalog delete leaves orphan state with no way to re-enter the diff + /// on a later seed cycle. + /// /// - public async Task DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId) + public async Task<(int deleted, int failed)> DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId) { - if (string.IsNullOrWhiteSpace(ruleId)) return 0; + if (string.IsNullOrWhiteSpace(ruleId)) return (0, 0); try { var tableClient = _tableServiceClient.GetTableClient(Constants.TableNames.RuleStates); @@ -358,6 +372,7 @@ namespace AutopilotMonitor.Functions.Services var query = tableClient.QueryAsync(filter: $"RowKey eq '{ruleId.Replace("'", "''")}'"); var deleted = 0; + var failed = 0; await foreach (var entity in query) { try @@ -365,26 +380,35 @@ namespace AutopilotMonitor.Functions.Services await tableClient.DeleteEntityAsync(entity.PartitionKey, entity.RowKey); deleted++; } + catch (Azure.RequestFailedException ex) when (ex.Status == 404) + { + // Row was deleted between the query and our delete — idempotent. + deleted++; + } catch (Exception ex) { _logger.LogWarning( ex, "Failed to delete orphan RuleState {RuleId} for tenant {TenantId}", ruleId, entity.PartitionKey); + failed++; } } - if (deleted > 0) + if (deleted > 0 || failed > 0) { _logger.LogInformation( - "Deleted {Count} orphan RuleState row(s) for sunset rule {RuleId}", - deleted, ruleId); + "Orphan RuleState GC for {RuleId}: {Deleted} deleted, {Failed} failed", + ruleId, deleted, failed); } - return deleted; + return (deleted, failed); } catch (Exception ex) { _logger.LogError(ex, "Failed to enumerate RuleStates for orphan GC of {RuleId}", ruleId); - return 0; + // -1 signals "we don't know" — caller must NOT proceed to delete the + // global catalog row, otherwise the rule falls out of the diff and any + // orphan state we couldn't enumerate lingers indefinitely. + return (0, -1); } } @@ -468,7 +492,10 @@ namespace AutopilotMonitor.Functions.Services } /// - /// Deletes an analyze rule + /// Deletes an analyze rule. Idempotent: a 404 (the rule was already gone) counts + /// as success so the sunset orchestration in + /// can retry without + /// flipping its happy-path on the missing-row condition. /// public async Task DeleteAnalyzeRuleAsync(string tenantId, string ruleId) { @@ -478,6 +505,10 @@ namespace AutopilotMonitor.Functions.Services await tableClient.DeleteEntityAsync(tenantId, ruleId); return true; } + catch (Azure.RequestFailedException ex) when (ex.Status == 404) + { + return true; + } catch (Exception ex) { _logger.LogError(ex, $"Failed to delete analyze rule {ruleId}"); diff --git a/src/Shared/AutopilotMonitor.Shared/DataAccess/IRuleRepository.cs b/src/Shared/AutopilotMonitor.Shared/DataAccess/IRuleRepository.cs index 02f57b6f..299e6b96 100644 --- a/src/Shared/AutopilotMonitor.Shared/DataAccess/IRuleRepository.cs +++ b/src/Shared/AutopilotMonitor.Shared/DataAccess/IRuleRepository.cs @@ -28,9 +28,16 @@ namespace AutopilotMonitor.Shared.DataAccess /// . Used as orphan-GC when a built-in / community rule /// is sunset (deleted from the global catalog): tenants that previously toggled /// the rule in the UI have a per-tenant RuleState row that would otherwise - /// linger as dead state. Returns the number of orphans deleted. + /// linger as dead state. + /// + /// Returns (deleted, failed). A 404 on an individual row is counted as + /// deleted (idempotent retry). failed is non-zero on per-row errors; + /// -1 signals the cross-partition enumeration itself failed — callers + /// MUST then refuse to delete the global catalog row, or the rule falls out of + /// the sunset-diff and unreachable orphan state lingers forever. + /// /// - Task DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId); + Task<(int deleted, int failed)> DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId); // --- Analyze Rules --- Task StoreAnalyzeRuleAsync(AnalyzeRule rule, string tenantId = "global");