From b46a8f157167d201405b8a5de894d71fa7f948cb Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Sat, 1 Aug 2026 00:40:29 +0200 Subject: [PATCH] fix(rules): merge collision guard - stale tenant rule copies no longer crash rule notifications Tenants seeded in the pre-global-partition era can still carry per-tenant copies of built-in rules (IsBuiltIn=false debris, e.g. ANALYZE-ID-002 on one prod tenant). They pass the custom-rule filter and put the same RuleId twice into the merged list, blowing up ToDictionary(r => r.RuleId) in AnalyzeOnEnrollmentEndHandler.SafeNotifyRuleChannelsAsync and silently dropping rule channel notifications for that tenant. Guard the merge in AnalyzeRuleService + GatherRuleService: a tenant row whose RuleId collides with a merged global rule is skipped (global wins) with a warning log. Co-Authored-By: Claude Fable 5 --- .../RuleMergeCollisionGuardTests.cs | 139 ++++++++++++++++++ .../Services/AnalyzeRuleService.cs | 19 ++- .../Services/GatherRuleService.cs | 20 ++- 3 files changed, 175 insertions(+), 3 deletions(-) create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/RuleMergeCollisionGuardTests.cs diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/RuleMergeCollisionGuardTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/RuleMergeCollisionGuardTests.cs new file mode 100644 index 00000000..ebcaa593 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/RuleMergeCollisionGuardTests.cs @@ -0,0 +1,139 @@ +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 merge collision guard in +/// and : a tenant-partition row whose +/// RuleId collides with a merged global rule (legacy debris from the pre-global-partition +/// seeding era, e.g. tenant 5ca2b350's ANALYZE-ID-002 copy from 2026-03) must be skipped — +/// the global definition wins. Without the guard the same RuleId appears twice in the merged +/// list and every ToDictionary(r => r.RuleId) consumer throws (prod: ArgumentException in +/// AnalyzeOnEnrollmentEndHandler.SafeNotifyRuleChannelsAsync, killing rule notifications). +/// +public class RuleMergeCollisionGuardTests +{ + private const string TenantId = "a1b2c3d4-e5f6-7890-abcd-ef1234567890"; + + [Fact] + public async Task Analyze_stale_tenant_copy_of_global_rule_is_skipped() + { + var collidingId = BuiltInAnalyzeRules.GetAll().First().RuleId; + var repo = CreateAnalyzeRepo(tenantRules: new List + { + StaleTenantAnalyzeCopy(collidingId), + }); + + var service = new AnalyzeRuleService(repo.Object, NullLogger.Instance); + var merged = await service.GetAllRulesForTenantAsync(TenantId); + + var instances = merged.Where(r => r.RuleId == collidingId).ToList(); + Assert.Single(instances); + Assert.True(instances[0].IsBuiltIn, "The global built-in definition must win over the stale tenant copy."); + // The exact downstream consumer that crashed in prod must work again. + _ = merged.ToDictionary(r => r.RuleId); + } + + [Fact] + public async Task Analyze_non_colliding_custom_rule_still_merges() + { + var repo = CreateAnalyzeRepo(tenantRules: new List + { + StaleTenantAnalyzeCopy("TENANT-CUSTOM-XYZ"), + }); + + var service = new AnalyzeRuleService(repo.Object, NullLogger.Instance); + var merged = await service.GetAllRulesForTenantAsync(TenantId); + + Assert.Contains(merged, r => r.RuleId == "TENANT-CUSTOM-XYZ"); + } + + [Fact] + public async Task Gather_stale_tenant_copy_of_global_rule_is_skipped() + { + var collidingId = BuiltInGatherRules.GetAll().First().RuleId; + var repo = CreateGatherRepo(tenantRules: new List + { + StaleTenantGatherCopy(collidingId), + }); + + var service = new GatherRuleService(repo.Object, NullLogger.Instance); + var merged = await service.GetAllRulesForTenantAsync(TenantId); + + var instances = merged.Where(r => r.RuleId == collidingId).ToList(); + Assert.Single(instances); + Assert.True(instances[0].IsBuiltIn, "The global built-in definition must win over the stale tenant copy."); + _ = merged.ToDictionary(r => r.RuleId); + } + + [Fact] + public async Task Gather_non_colliding_custom_rule_still_merges() + { + var repo = CreateGatherRepo(tenantRules: new List + { + StaleTenantGatherCopy("TENANT-CUSTOM-XYZ"), + }); + + var service = new GatherRuleService(repo.Object, NullLogger.Instance); + var merged = await service.GetAllRulesForTenantAsync(TenantId); + + Assert.Contains(merged, r => r.RuleId == "TENANT-CUSTOM-XYZ"); + } + + // ===== Helpers ===== + + private static AnalyzeRule StaleTenantAnalyzeCopy(string ruleId) => new() + { + RuleId = ruleId, + Title = "Stale tenant copy", + Description = "Legacy per-tenant seeded row", + Severity = "warning", + Category = "test", + IsBuiltIn = false, + IsCommunity = false, + Enabled = true, + }; + + private static GatherRule StaleTenantGatherCopy(string ruleId) => new() + { + RuleId = ruleId, + Title = "Stale tenant copy", + Description = "Legacy per-tenant seeded row", + Category = "test", + CollectorType = "registry", + Target = "HKLM\\SOFTWARE\\X", + Trigger = "startup", + IsBuiltIn = false, + IsCommunity = false, + Enabled = true, + }; + + /// DB state = exactly the shipped catalog (clean seed diff), plus the given tenant rows. + private static Mock CreateAnalyzeRepo(List tenantRules) + { + var repo = new Mock(); + repo.Setup(r => r.GetAnalyzeRulesAsync("global")).ReturnsAsync(BuiltInAnalyzeRules.GetAll().ToList()); + repo.Setup(r => r.GetAnalyzeRulesAsync(TenantId)).ReturnsAsync(tenantRules); + repo.Setup(r => r.GetRuleStatesAsync(TenantId)).ReturnsAsync(new Dictionary()); + repo.Setup(r => r.StoreAnalyzeRuleAsync(It.IsAny(), It.IsAny())).ReturnsAsync(true); + repo.Setup(r => r.DeleteAnalyzeRuleAsync(It.IsAny(), It.IsAny())).ReturnsAsync(true); + repo.Setup(r => r.DeleteRuleStatesForRuleIdAcrossTenantsAsync(It.IsAny())).ReturnsAsync((0, 0)); + return repo; + } + + private static Mock CreateGatherRepo(List tenantRules) + { + var repo = new Mock(); + repo.Setup(r => r.GetGatherRulesAsync("global")).ReturnsAsync(BuiltInGatherRules.GetAll().ToList()); + repo.Setup(r => r.GetGatherRulesAsync(TenantId)).ReturnsAsync(tenantRules); + repo.Setup(r => r.GetRuleStatesAsync(TenantId)).ReturnsAsync(new Dictionary()); + repo.Setup(r => r.StoreGatherRuleAsync(It.IsAny(), It.IsAny())).ReturnsAsync(true); + repo.Setup(r => r.DeleteGatherRuleAsync(It.IsAny(), It.IsAny())).ReturnsAsync(true); + repo.Setup(r => r.DeleteRuleStatesForRuleIdAcrossTenantsAsync(It.IsAny())).ReturnsAsync((0, 0)); + return repo; + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs index 1a087da0..902d33f2 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs @@ -230,7 +230,24 @@ namespace AutopilotMonitor.Functions.Services // Tenant custom rules carry their own MarkSessionAsFailedDefault / NotifyDefault / // NotifyChannelIds on the rule row; no override needed since the tenant already // fully owns the rule definition. - mergedRules.AddRange(customRules); + // + // Collision guard: tenant-partition rows sharing a RuleId with a merged global + // rule are legacy debris from the pre-global-partition seeding era (custom rule + // creation rejects global-id collisions). The global definition wins — merging + // the stale copy would put the same RuleId in the list twice and break every + // ToDictionary(r => r.RuleId) consumer downstream. + var mergedIds = new HashSet(mergedRules.Select(r => r.RuleId), StringComparer.Ordinal); + foreach (var custom in customRules) + { + if (!mergedIds.Add(custom.RuleId)) + { + _logger.LogWarning( + "Tenant {TenantId} has a stale tenant-partition analyze rule {RuleId} colliding with a global rule — skipping the tenant copy", + tenantId, custom.RuleId); + continue; + } + mergedRules.Add(custom); + } return mergedRules; } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/GatherRuleService.cs b/src/Backend/AutopilotMonitor.Functions/Services/GatherRuleService.cs index 1ed0ad8f..51aa2951 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/GatherRuleService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/GatherRuleService.cs @@ -104,8 +104,24 @@ namespace AutopilotMonitor.Functions.Services mergedRules.Add(rule); } - // Add tenant-specific custom rules - mergedRules.AddRange(customRules); + // Add tenant-specific custom rules. + // + // Collision guard (mirrors AnalyzeRuleService): tenant-partition rows sharing a + // RuleId with a merged global rule are legacy debris from the pre-global-partition + // seeding era — the global definition wins, the stale copy is skipped so the same + // RuleId can never appear twice in the merged result. + var mergedIds = new HashSet(mergedRules.Select(r => r.RuleId), StringComparer.Ordinal); + foreach (var custom in customRules) + { + if (!mergedIds.Add(custom.RuleId)) + { + _logger.LogWarning( + "Tenant {TenantId} has a stale tenant-partition gather rule {RuleId} colliding with a global rule — skipping the tenant copy", + tenantId, custom.RuleId); + continue; + } + mergedRules.Add(custom); + } return mergedRules; }