diff --git a/rules/analyze/ANALYZE-APP-009.json b/rules/analyze/ANALYZE-APP-009.json deleted file mode 100644 index 9b1b8011..00000000 --- a/rules/analyze/ANALYZE-APP-009.json +++ /dev/null @@ -1,53 +0,0 @@ -{ - "$schema": "../schema/analyze-rule.schema.json", - "ruleId": "ANALYZE-APP-009", - "title": "App Download Stalled", - "description": "Detects when app downloads appear to have stalled, based on failed download progress events. DISABLED: the V2 EnrollmentTerminationHandler emits a synthetic download_progress with status=failed when it promotes Installing -> Error on terminal ESP-Apps failure. That synthetic event fires AFTER the download already completed (e.g. session 080edee9 saw download_progress 100% at 10:34:30 then download_progress failed at 10:41:08), so 'status=failed' alone is a false signal. The engine's `suppressByEvent` cannot filter by a third event's `status` field, so we can't directly say 'suppress when a prior download_progress for the same appId reached 100%' — re-enable only after the engine grows a status-aware suppression filter OR after we introduce a dedicated `download_stalled` event type that doesn't share its eventType with the post-mortem synthesis path.", - "severity": "warning", - "category": "apps", - "version": "1.1.0", - "enabled": false, - "trigger": "correlation", - "baseConfidence": 55, - "conditions": [ - { - "signal": "download_failed", - "source": "event_type", - "eventType": "download_progress", - "dataField": "status", - "operator": "equals", - "value": "failed", - "required": true - }, - { - "signal": "app_failed", - "source": "event_type", - "eventType": "app_install_failed", - "operator": "exists", - "value": "", - "required": true, - "suppressByEvent": { - "eventType": "app_install_completed", - "joinField": "appId" - } - } - ], - "confidenceFactors": [ - { "signal": "proxy_active", "condition": "exists", "weight": 15 }, - { "signal": "multiple_failures", "condition": "count >= 2", "weight": 15 } - ], - "confidenceThreshold": 50, - "explanation": "Download progress events show a failed download, and at least one app installation also failed. This pattern suggests content delivery issues preventing apps from being downloaded successfully.", - "remediation": [ - { - "title": "Investigate download issues", - "steps": [ - "Check network connectivity and bandwidth during the enrollment", - "Verify DNS resolution for *.delivery.mp.microsoft.com", - "Check if a proxy or firewall is blocking content delivery", - "Review Delivery Optimization settings and peer availability" - ] - } - ], - "tags": ["apps", "download", "stall", "correlation"] -} diff --git a/rules/analyze/ANALYZE-CORR-001.json b/rules/analyze/ANALYZE-CORR-001.json deleted file mode 100644 index 9c0da1ae..00000000 --- a/rules/analyze/ANALYZE-CORR-001.json +++ /dev/null @@ -1,68 +0,0 @@ -{ - "$schema": "../schema/analyze-rule.schema.json", - "ruleId": "ANALYZE-CORR-001", - "title": "Slow Network Causing App Installation Failure", - "description": "Correlates slow download speeds with app installation failures while disk space is adequate, confirming network as root cause (not disk). DISABLED: the current conditions do not actually verify slow downloads — they fire on the simple combination of any app-failure + any download event + sufficient disk, which was a false positive on session 080edee9 (Office detection-failure with a 10.58 MB / 1.5 s download). Re-enable only after the rule grows a real bytes-per-second / download-duration check.", - "severity": "high", - "category": "apps", - "version": "1.1.0", - "enabled": false, - "trigger": "correlation", - "baseConfidence": 55, - "conditions": [ - { - "signal": "app_failed", - "source": "event_type", - "eventType": "app_install_failed", - "operator": "exists", - "value": "", - "required": true, - "suppressByEvent": { - "eventType": "app_install_completed", - "joinField": "appId" - } - }, - { - "signal": "download_events", - "source": "event_type", - "eventType": "download_progress", - "operator": "exists", - "value": "", - "required": true - }, - { - "signal": "disk_ok", - "source": "event_type", - "eventType": "performance_snapshot", - "dataField": "disk_free_gb", - "operator": "gt", - "value": "10" - } - ], - "confidenceFactors": [ - { "signal": "disk_ok", "condition": "exists", "weight": 15 }, - { "signal": "multiple_failures", "condition": "count >= 2", "weight": 15 }, - { "signal": "enrollment_failed", "condition": "exists", "weight": 15 } - ], - "confidenceThreshold": 50, - "explanation": "App installation failed while the device has sufficient disk space (>10 GB free). Download progress events indicate network-related issues are the likely root cause.\n\nThis rules out disk space as a factor and points to network bandwidth, proxy throttling, or CDN connectivity issues.", - "remediation": [ - { - "title": "Investigate network throughput", - "steps": [ - "Check bandwidth to *.delivery.mp.microsoft.com (CDN)", - "Verify proxy is not throttling large downloads", - "Consider enabling Delivery Optimization for peer-to-peer caching" - ] - }, - { - "title": "Reduce download load", - "steps": [ - "Stagger app deployments to reduce concurrent downloads", - "Pre-cache large apps using Delivery Optimization or ConfigMgr", - "Increase ESP timeout if downloads are slow but completing" - ] - } - ], - "tags": ["correlation", "apps", "network", "download", "root-cause"] -} diff --git a/rules/dist/analyze-rules.json b/rules/dist/analyze-rules.json index 739f02e9..4c47ac71 100644 --- a/rules/dist/analyze-rules.json +++ b/rules/dist/analyze-rules.json @@ -320,71 +320,6 @@ "warning" ] }, - { - "ruleId": "ANALYZE-APP-009", - "title": "App Download Stalled", - "description": "Detects when app downloads appear to have stalled, based on failed download progress events. DISABLED: the V2 EnrollmentTerminationHandler emits a synthetic download_progress with status=failed when it promotes Installing -> Error on terminal ESP-Apps failure. That synthetic event fires AFTER the download already completed (e.g. session 080edee9 saw download_progress 100% at 10:34:30 then download_progress failed at 10:41:08), so 'status=failed' alone is a false signal. The engine's `suppressByEvent` cannot filter by a third event's `status` field, so we can't directly say 'suppress when a prior download_progress for the same appId reached 100%' — re-enable only after the engine grows a status-aware suppression filter OR after we introduce a dedicated `download_stalled` event type that doesn't share its eventType with the post-mortem synthesis path.", - "severity": "warning", - "category": "apps", - "version": "1.1.0", - "enabled": false, - "trigger": "correlation", - "baseConfidence": 55, - "conditions": [ - { - "signal": "download_failed", - "source": "event_type", - "eventType": "download_progress", - "dataField": "status", - "operator": "equals", - "value": "failed", - "required": true - }, - { - "signal": "app_failed", - "source": "event_type", - "eventType": "app_install_failed", - "operator": "exists", - "value": "", - "required": true, - "suppressByEvent": { - "eventType": "app_install_completed", - "joinField": "appId" - } - } - ], - "confidenceFactors": [ - { - "signal": "proxy_active", - "condition": "exists", - "weight": 15 - }, - { - "signal": "multiple_failures", - "condition": "count >= 2", - "weight": 15 - } - ], - "confidenceThreshold": 50, - "explanation": "Download progress events show a failed download, and at least one app installation also failed. This pattern suggests content delivery issues preventing apps from being downloaded successfully.", - "remediation": [ - { - "title": "Investigate download issues", - "steps": [ - "Check network connectivity and bandwidth during the enrollment", - "Verify DNS resolution for *.delivery.mp.microsoft.com", - "Check if a proxy or firewall is blocking content delivery", - "Review Delivery Optimization settings and peer availability" - ] - } - ], - "tags": [ - "apps", - "download", - "stall", - "correlation" - ] - }, { "ruleId": "ANALYZE-APP-010", "title": "App Reverted to Error After Successful Install", @@ -591,91 +526,6 @@ "intune" ] }, - { - "ruleId": "ANALYZE-CORR-001", - "title": "Slow Network Causing App Installation Failure", - "description": "Correlates slow download speeds with app installation failures while disk space is adequate, confirming network as root cause (not disk). DISABLED: the current conditions do not actually verify slow downloads — they fire on the simple combination of any app-failure + any download event + sufficient disk, which was a false positive on session 080edee9 (Office detection-failure with a 10.58 MB / 1.5 s download). Re-enable only after the rule grows a real bytes-per-second / download-duration check.", - "severity": "high", - "category": "apps", - "version": "1.1.0", - "enabled": false, - "trigger": "correlation", - "baseConfidence": 55, - "conditions": [ - { - "signal": "app_failed", - "source": "event_type", - "eventType": "app_install_failed", - "operator": "exists", - "value": "", - "required": true, - "suppressByEvent": { - "eventType": "app_install_completed", - "joinField": "appId" - } - }, - { - "signal": "download_events", - "source": "event_type", - "eventType": "download_progress", - "operator": "exists", - "value": "", - "required": true - }, - { - "signal": "disk_ok", - "source": "event_type", - "eventType": "performance_snapshot", - "dataField": "disk_free_gb", - "operator": "gt", - "value": "10" - } - ], - "confidenceFactors": [ - { - "signal": "disk_ok", - "condition": "exists", - "weight": 15 - }, - { - "signal": "multiple_failures", - "condition": "count >= 2", - "weight": 15 - }, - { - "signal": "enrollment_failed", - "condition": "exists", - "weight": 15 - } - ], - "confidenceThreshold": 50, - "explanation": "App installation failed while the device has sufficient disk space (>10 GB free). Download progress events indicate network-related issues are the likely root cause.\n\nThis rules out disk space as a factor and points to network bandwidth, proxy throttling, or CDN connectivity issues.", - "remediation": [ - { - "title": "Investigate network throughput", - "steps": [ - "Check bandwidth to *.delivery.mp.microsoft.com (CDN)", - "Verify proxy is not throttling large downloads", - "Consider enabling Delivery Optimization for peer-to-peer caching" - ] - }, - { - "title": "Reduce download load", - "steps": [ - "Stagger app deployments to reduce concurrent downloads", - "Pre-cache large apps using Delivery Optimization or ConfigMgr", - "Increase ESP timeout if downloads are slow but completing" - ] - } - ], - "tags": [ - "correlation", - "apps", - "network", - "download", - "root-cause" - ] - }, { "ruleId": "ANALYZE-CORR-002", "title": "Disk Space Exhaustion Caused Installation Failure", diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs new file mode 100644 index 00000000..d06bd162 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs @@ -0,0 +1,129 @@ +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared.DataAccess; +using AutopilotMonitor.Shared.Models; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Pins the sunset-rule invariant on : when a built-in +/// rule is removed from the code catalog (e.g. ANALYZE-CORR-001 and ANALYZE-APP-009 in +/// the session 080edee9 follow-up), both the global rule row AND all per-tenant +/// RuleState overrides for that ruleId must be deleted on the next seed/reseed +/// cycle. Without this, opt-in tenants would keep the rule alive via stale RuleState +/// records that point to a non-existent global rule. +/// +public class AnalyzeRuleServiceSunsetTests +{ + private const string SunsetRuleId = "ANALYZE-LEGACY-001"; + + [Fact] + public async Task EnsureSeed_drops_sunset_rule_and_GCs_orphan_RuleStates() + { + // 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, orphanCount: 2); + + 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.Contains(SunsetRuleId, deleteAnalyzeCalls); + Assert.Contains(SunsetRuleId, gcCalls); + } + + [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. + var stillLiveRuleId = BuiltInAnalyzeRules.GetAll().First().RuleId; + var (ruleRepo, deleteAnalyzeCalls, gcCalls) = BuildRepoWithSunsetRule(stillLiveRuleId, orphanCount: 1); + + var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); + + await service.GetAllRulesForTenantAsync("a1b2c3d4-e5f6-7890-abcd-ef1234567890"); + + // 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] + 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, orphanCount: 3); + + var service = new AnalyzeRuleService(ruleRepo.Object, NullLogger.Instance); + var (deleted, written, orphanStatesGcd) = await service.ReseedBuiltInRulesAsync(); + + Assert.True(deleted >= 1, $"Expected at least 1 deletion (incl. sunset), got {deleted}"); + Assert.Equal(BuiltInAnalyzeRules.GetAll().Count, written); + Assert.Equal(3, 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 from the cross-tenant + /// RuleState GC method. + /// 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) + { + var deleteAnalyzeCalls = new List(); + var gcCalls = new List(); + + // 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)) + { + existingInDb.Add(new AnalyzeRule + { + RuleId = extraRuleId, + Title = "Legacy", + Description = "Legacy", + Severity = "warning", + Category = "test", + Version = "1.0.0", + IsBuiltIn = true, + Enabled = false, + }); + } + + 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(orphanCount); + + return (ruleRepo, deleteAnalyzeCalls, gcCalls); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineDetectionFailureTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineDetectionFailureTests.cs index 8f4449f2..342b33a8 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineDetectionFailureTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineDetectionFailureTests.cs @@ -151,27 +151,27 @@ public class RuleEngineDetectionFailureTests } [Fact] - public async Task ANALYZE_CORR_001_is_disabled_after_version_bump() + public void ANALYZE_CORR_001_is_removed_from_builtin_catalog() { - // The "Slow Network" rule fired as a false positive on session 080edee9 because - // its conditions don't actually check for slow downloads. With the v1.1.0 bump, - // EnsureBuiltInRulesSeededAsync will re-import it as enabled=false on the next - // backend boot — verify the in-code shape is correct. - var rule = BuiltInAnalyzeRules.GetAll().First(r => r.RuleId == "ANALYZE-CORR-001"); - Assert.Equal("1.1.0", rule.Version); - Assert.False(rule.Enabled); + // "Slow Network Causing App Installation Failure" was sunset because its + // conditions never verified a slow download — it fired on the simple combo + // of any app-failure + any download event + sufficient disk. The first attempt + // to ship it as enabled:false was a half-measure that left orphan per-tenant + // RuleState{Enabled=true} overrides in place. Sunset = removed from source + + // EnsureBuiltInRulesSeededAsync GCs the orphan state on the next backend boot. + Assert.DoesNotContain(BuiltInAnalyzeRules.GetAll(), r => r.RuleId == "ANALYZE-CORR-001"); } [Fact] - public async Task ANALYZE_APP_009_is_disabled_after_version_bump() + public void ANALYZE_APP_009_is_removed_from_builtin_catalog() { - // The "App Download Stalled" rule fired on the V2 termination handler's - // synthetic download_progress with status=failed (emitted AFTER the download - // already completed). Disabled until the engine grows status-aware suppression - // OR we introduce a dedicated download_stalled event type. - var rule = BuiltInAnalyzeRules.GetAll().First(r => r.RuleId == "ANALYZE-APP-009"); - Assert.Equal("1.1.0", rule.Version); - Assert.False(rule.Enabled); + // "App Download Stalled" was sunset for the same reason CORR-001 was: the rule + // fired on the V2 termination handler's synthetic `download_progress: failed` + // (emitted AFTER the download already completed), and the engine's + // suppressByEvent can't filter by a third event's `status` field. Re-introduce + // only with a real stall-detection mechanism (e.g. a dedicated `download_stalled` + // event type that isn't post-hoc-synthesized). + Assert.DoesNotContain(BuiltInAnalyzeRules.GetAll(), r => r.RuleId == "ANALYZE-APP-009"); } // ===== Event builders (faithful to session 080edee9's actual payloads) ===== diff --git a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableRuleRepository.cs b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableRuleRepository.cs index f3b305d6..681b25c9 100644 --- a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableRuleRepository.cs +++ b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableRuleRepository.cs @@ -48,6 +48,9 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage public Task DeleteRuleStateAsync(string tenantId, string ruleId) => _storage.DeleteRuleStateAsync(tenantId, ruleId); + public Task DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId) + => _storage.DeleteRuleStatesForRuleIdAcrossTenantsAsync(ruleId); + public Task StoreAnalyzeRuleAsync(AnalyzeRule rule, string tenantId = "global") => _storage.StoreAnalyzeRuleAsync(rule, tenantId); diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs index 0f201c5d..eb8402f7 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs @@ -162,6 +162,19 @@ namespace AutopilotMonitor.Functions.Functions.Admin private async Task<(int deleted, int written)> 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(); + var deleted = 0; foreach (var rule in existing.Where(r => r.IsBuiltIn || r.IsCommunity)) { @@ -179,7 +192,15 @@ namespace AutopilotMonitor.Functions.Functions.Admin await _ruleRepo.StoreAnalyzeRuleAsync(rule, "global"); } - _logger.LogInformation($"GitHub reseed analyze: {deleted} deleted, {rules.Count} written"); + 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); } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs index 5c3e471d..94ae554d 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs @@ -215,15 +215,28 @@ 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. - /// Tenant RuleStates are preserved. + /// 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. /// - public async Task<(int deleted, int written)> ReseedBuiltInRulesAsync() + public async Task<(int deleted, int written, int orphanStatesGcd)> ReseedBuiltInRulesAsync() { _logger.LogInformation("Reseeding built-in analyze rules (full re-import)..."); var existingGlobalRules = await _ruleRepo.GetAnalyzeRulesAsync("global"); + 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(); var deleted = 0; foreach (var rule in existingGlobalRules.Where(r => r.IsBuiltIn)) @@ -233,21 +246,37 @@ namespace AutopilotMonitor.Functions.Services } _logger.LogInformation($"Deleted {deleted} old global built-in analyze rules"); - var builtInRules = BuiltInAnalyzeRules.GetAll(); foreach (var rule in builtInRules) { await _ruleRepo.StoreAnalyzeRuleAsync(rule, "global"); } _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); + return (deleted, builtInRules.Count, orphanStatesGcd); } /// - /// Seeds built-in analyze rules if not already done. - /// Also updates existing built-in rules when the code definitions change (version or key fields). + /// Seeds built-in analyze rules if not already done. Also updates existing + /// built-in rules when the code definitions change (version or key fields), + /// removes built-in rules that no longer exist in the code (sunset), and GCs + /// per-tenant RuleState rows for sunset rules so no orphan overrides linger. /// private async Task EnsureBuiltInRulesSeededAsync() { @@ -255,6 +284,7 @@ namespace AutopilotMonitor.Functions.Services var existingRules = await _ruleRepo.GetAnalyzeRulesAsync("global"); var builtInRules = BuiltInAnalyzeRules.GetAll(); + var newCatalogIds = builtInRules.Select(r => r.RuleId).ToHashSet(StringComparer.Ordinal); if (existingRules.Count == 0) { @@ -296,6 +326,26 @@ namespace AutopilotMonitor.Functions.Services { _logger.LogInformation($"Updated {updated} built-in analyze rules from code definitions"); } + + // 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. + var sunsetRules = existingRules + .Where(r => r.IsBuiltIn && !newCatalogIds.Contains(r.RuleId)) + .ToList(); + + if (sunsetRules.Count > 0) + { + var orphanStatesGcd = 0; + foreach (var sunset in sunsetRules) + { + await _ruleRepo.DeleteAnalyzeRuleAsync("global", sunset.RuleId); + orphanStatesGcd += await _ruleRepo.DeleteRuleStatesForRuleIdAcrossTenantsAsync(sunset.RuleId); + } + _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); + } } _seeded = true; diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs index b062184f..d085e92d 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs @@ -337,6 +337,57 @@ namespace AutopilotMonitor.Functions.Services } } + /// + /// Cross-partition cleanup: deletes every RuleState row across all tenants whose + /// 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. + /// + public async Task DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId) + { + if (string.IsNullOrWhiteSpace(ruleId)) return 0; + try + { + var tableClient = _tableServiceClient.GetTableClient(Constants.TableNames.RuleStates); + // Cross-partition scan is acceptable here — RuleStates is a small table + // (one row per tenant × overridden rule). Deletion is rare (only on built-in + // sunset), so we don't need a partition-key-indexed shortcut. + var query = tableClient.QueryAsync(filter: $"RowKey eq '{ruleId.Replace("'", "''")}'"); + + var deleted = 0; + await foreach (var entity in query) + { + try + { + await tableClient.DeleteEntityAsync(entity.PartitionKey, entity.RowKey); + deleted++; + } + catch (Exception ex) + { + _logger.LogWarning( + ex, + "Failed to delete orphan RuleState {RuleId} for tenant {TenantId}", + ruleId, entity.PartitionKey); + } + } + if (deleted > 0) + { + _logger.LogInformation( + "Deleted {Count} orphan RuleState row(s) for sunset rule {RuleId}", + deleted, ruleId); + } + return deleted; + } + catch (Exception ex) + { + _logger.LogError(ex, "Failed to enumerate RuleStates for orphan GC of {RuleId}", ruleId); + return 0; + } + } + // ===== ANALYZE RULES METHODS ===== /// diff --git a/src/Shared/AutopilotMonitor.Shared/DataAccess/IRuleRepository.cs b/src/Shared/AutopilotMonitor.Shared/DataAccess/IRuleRepository.cs index 902da106..02f57b6f 100644 --- a/src/Shared/AutopilotMonitor.Shared/DataAccess/IRuleRepository.cs +++ b/src/Shared/AutopilotMonitor.Shared/DataAccess/IRuleRepository.cs @@ -23,6 +23,14 @@ namespace AutopilotMonitor.Shared.DataAccess Task StoreRuleStateAsync(string tenantId, string ruleId, RuleState state); Task> GetRuleStatesAsync(string tenantId); Task DeleteRuleStateAsync(string tenantId, string ruleId); + /// + /// Removes every RuleStates row (across all tenants) whose RowKey equals + /// . 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. + /// + Task DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId); // --- Analyze Rules --- Task StoreAnalyzeRuleAsync(AnalyzeRule rule, string tenantId = "global");