mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 20:53:06 +00:00
fix(rules): sunset = delete, with cross-tenant orphan RuleState GC
Session 080edee9 follow-up — the customer screenshots showed ANALYZE-CORR-001
("Slow Network") and ANALYZE-APP-009 ("App Download Stalled") still firing
on a clean re-analyze, despite my previous fix shipping them as
enabled:false in the JSON. Investigation via direct DB query:
- AnalyzeRules/global rows for both: Enabled=false ✓ (version-bump landed)
- RuleStates/{tenant} rows for both: Enabled=true ✗ (orphan overrides
from the user toggling them in the portal UI on 2026-03-09)
[AnalyzeRuleService.cs:60](src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs#L60)
overlays per-tenant RuleState on top of the global Enabled, so the orphans
kept resurrecting the rules. No reseed path was cleaning them up.
The right semantic is "sunset = remove from catalog", not "ship as
enabled:false and hope nobody overrides". Three coordinated changes:
1. Both sunset rules deleted from the rules/analyze/ source tree (they
never had a working condition shape — CORR-001 didn't actually check
download speeds, APP-009 matched the V2 termination handler's
post-mortem-synthetic download_progress:failed event).
2. New IRuleRepository.DeleteRuleStatesForRuleIdAcrossTenantsAsync
cross-partition cleanup primitive — scans RuleStates by RowKey, deletes
matching rows across every tenant partition.
3. Both reseed paths now do orphan-state GC:
- AnalyzeRuleService.EnsureBuiltInRulesSeededAsync (on-startup):
detects rules that exist in DB as built-in but aren't in
BuiltInAnalyzeRules.GetAll() anymore → deletes the global row +
fans out the per-tenant RuleState GC.
- AnalyzeRuleService.ReseedBuiltInRulesAsync (delete+reimport): same
diff-based detection on the pre-delete snapshot.
- ReseedFromGitHubFunction.ReseedAnalyzeAsync (admin "Reseed from
GitHub" button — the path the user actually clicks): mirrored
identical logic so every reseed flavor leaves the same DB invariant.
4. ReseedBuiltInRulesAsync return shape extended to
(deleted, written, orphanStatesGcd) so the admin UI can surface the
cleanup count to operators.
Coverage:
- RuleEngineDetectionFailureTests: the two version-bump assertions for
CORR-001/APP-009 are replaced with `DoesNotContain` against the
built-in catalog (sunset semantic).
- AnalyzeRuleServiceSunsetTests (new, 3 tests): pins EnsureSeed orphan-GC
invariant, asserts live-rule states are NOT touched, and pins the
ReseedBuiltIn (deleted, written, orphanStatesGcd) tuple shape.
After deploy + a single GetAllRulesForTenantAsync call (any analyze run
triggers it), the orphan RuleStates for sunset rules are automatically
cleaned. The user's tenant 5ca2b350-… will see CORR-001 + APP-009
disappear from the Rules UI on the next analyze cycle.
Backend 2073/2073.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Oliver Kieselbach
@@ -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"]
|
||||
}
|
||||
@@ -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"]
|
||||
}
|
||||
-150
@@ -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",
|
||||
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// Pins the sunset-rule invariant on <see cref="AnalyzeRuleService"/>: 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
|
||||
/// <c>RuleState</c> 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.
|
||||
/// </summary>
|
||||
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<AnalyzeRuleService>.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<AnalyzeRuleService>.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<AnalyzeRuleService>.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 =====
|
||||
|
||||
/// <summary>
|
||||
/// Builds a mock <see cref="IRuleRepository"/> that:
|
||||
/// (a) returns the full built-in catalog PLUS one extra rule with the given
|
||||
/// <paramref name="extraRuleId"/> on the "global" partition — so the sunset
|
||||
/// detection has something to find when extraRuleId is not in the catalog;
|
||||
/// (b) reports the configured <paramref name="orphanCount"/> from the cross-tenant
|
||||
/// RuleState GC method.
|
||||
/// Returns the mock plus two recorder lists capturing which ruleIds got
|
||||
/// DeleteAnalyzeRuleAsync and which got DeleteRuleStatesForRuleIdAcrossTenantsAsync
|
||||
/// invoked.
|
||||
/// </summary>
|
||||
private static (Mock<IRuleRepository> repo, List<string> deleteAnalyzeCalls, List<string> gcCalls)
|
||||
BuildRepoWithSunsetRule(string extraRuleId, int orphanCount)
|
||||
{
|
||||
var deleteAnalyzeCalls = new List<string>();
|
||||
var gcCalls = new List<string>();
|
||||
|
||||
// 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<IRuleRepository>();
|
||||
ruleRepo.Setup(r => r.GetAnalyzeRulesAsync("global")).ReturnsAsync(existingInDb);
|
||||
ruleRepo.Setup(r => r.GetAnalyzeRulesAsync(It.Is<string>(s => s != "global"))).ReturnsAsync(new List<AnalyzeRule>());
|
||||
ruleRepo.Setup(r => r.GetRuleStatesAsync(It.IsAny<string>())).ReturnsAsync(new Dictionary<string, RuleState>());
|
||||
ruleRepo.Setup(r => r.StoreAnalyzeRuleAsync(It.IsAny<AnalyzeRule>(), It.IsAny<string>())).ReturnsAsync(true);
|
||||
ruleRepo.Setup(r => r.DeleteAnalyzeRuleAsync(It.IsAny<string>(), It.IsAny<string>()))
|
||||
.Callback<string, string>((_, ruleId) => deleteAnalyzeCalls.Add(ruleId))
|
||||
.ReturnsAsync(true);
|
||||
ruleRepo.Setup(r => r.DeleteRuleStatesForRuleIdAcrossTenantsAsync(It.IsAny<string>()))
|
||||
.Callback<string>(ruleId => gcCalls.Add(ruleId))
|
||||
.ReturnsAsync(orphanCount);
|
||||
|
||||
return (ruleRepo, deleteAnalyzeCalls, gcCalls);
|
||||
}
|
||||
}
|
||||
@@ -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) =====
|
||||
|
||||
@@ -48,6 +48,9 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage
|
||||
public Task<bool> DeleteRuleStateAsync(string tenantId, string ruleId)
|
||||
=> _storage.DeleteRuleStateAsync(tenantId, ruleId);
|
||||
|
||||
public Task<int> DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId)
|
||||
=> _storage.DeleteRuleStatesForRuleIdAcrossTenantsAsync(ruleId);
|
||||
|
||||
public Task<bool> StoreAnalyzeRuleAsync(AnalyzeRule rule, string tenantId = "global")
|
||||
=> _storage.StoreAnalyzeRuleAsync(rule, tenantId);
|
||||
|
||||
|
||||
@@ -162,6 +162,19 @@ namespace AutopilotMonitor.Functions.Functions.Admin
|
||||
private async Task<(int deleted, int written)> ReseedAnalyzeAsync(List<AutopilotMonitor.Shared.Models.AnalyzeRule> 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);
|
||||
}
|
||||
|
||||
|
||||
@@ -215,15 +215,28 @@ namespace AutopilotMonitor.Functions.Services
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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 <c>RuleState</c> rows that would otherwise linger as orphan
|
||||
/// state. Tenant RuleStates for rules that are still in the catalog are preserved.
|
||||
/// </summary>
|
||||
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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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;
|
||||
|
||||
@@ -337,6 +337,57 @@ namespace AutopilotMonitor.Functions.Services
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Cross-partition cleanup: deletes every RuleState row across all tenants whose
|
||||
/// RowKey matches <paramref name="ruleId"/>. Used as orphan-GC when a built-in
|
||||
/// rule is removed from the global catalog — without this, per-tenant
|
||||
/// <c>RuleState{Enabled=true}</c> 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.
|
||||
/// </summary>
|
||||
public async Task<int> 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<TableEntity>(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 =====
|
||||
|
||||
/// <summary>
|
||||
|
||||
@@ -23,6 +23,14 @@ namespace AutopilotMonitor.Shared.DataAccess
|
||||
Task<bool> StoreRuleStateAsync(string tenantId, string ruleId, RuleState state);
|
||||
Task<Dictionary<string, RuleState>> GetRuleStatesAsync(string tenantId);
|
||||
Task<bool> DeleteRuleStateAsync(string tenantId, string ruleId);
|
||||
/// <summary>
|
||||
/// Removes every <c>RuleStates</c> row (across all tenants) whose RowKey equals
|
||||
/// <paramref name="ruleId"/>. 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 <c>RuleState</c> row that would otherwise
|
||||
/// linger as dead state. Returns the number of orphans deleted.
|
||||
/// </summary>
|
||||
Task<int> DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId);
|
||||
|
||||
// --- Analyze Rules ---
|
||||
Task<bool> StoreAnalyzeRuleAsync(AnalyzeRule rule, string tenantId = "global");
|
||||
|
||||
Reference in New Issue
Block a user