gsadmin/AutopilotMonitor
1
0
Fork 0
mirror of https://github.com/okieselbach/AutopilotMonitor.git synced 2026-10-25 05:00:07 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(rules): retry-safe sunset GC + idempotent point-deletes

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Oliver Kieselbach
2026-05-28 16:46:24 +02:00
parent dff5ba65f4
commit b855b2b927
6 changed files with 251 additions and 85 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleServiceSunsetTests.cs
+57 -11
View File
@@ -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<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: 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<AnalyzeRuleService>.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<AnalyzeRuleService>.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<AnalyzeRuleService>.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<AnalyzeRuleService>.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<AnalyzeRuleService>.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 =====
/// <summary>
@@ -81,14 +127,14 @@ public class AnalyzeRuleServiceSunsetTests
/// (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.
/// (b) reports the configured GC result via
/// <see cref="IRuleRepository.DeleteRuleStatesForRuleIdAcrossTenantsAsync"/>.
/// 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)
BuildRepoWithSunsetRule(string extraRuleId, int gcDeleted, int gcFailed)
{
var deleteAnalyzeCalls = new List<string>();
var gcCalls = new List<string>();
@@ -122,7 +168,7 @@ public class AnalyzeRuleServiceSunsetTests
.ReturnsAsync(true);
ruleRepo.Setup(r => r.DeleteRuleStatesForRuleIdAcrossTenantsAsync(It.IsAny<string>()))
.Callback<string>(ruleId => gcCalls.Add(ruleId))
.ReturnsAsync(orphanCount);
.ReturnsAsync((gcDeleted, gcFailed));
return (ruleRepo, deleteAnalyzeCalls, gcCalls);
}
src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableRuleRepository.cs
+1 -1
View File
@@ -48,7 +48,7 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage
public Task<bool> DeleteRuleStateAsync(string tenantId, string ruleId)
=> _storage.DeleteRuleStateAsync(tenantId, ruleId);
public Task<int> DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId)
public Task<(int deleted, int failed)> DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId)
=> _storage.DeleteRuleStatesForRuleIdAcrossTenantsAsync(ruleId);
public Task<bool> StoreAnalyzeRuleAsync(AnalyzeRule rule, string tenantId = "global")
src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs
+49 -25
View File
@@ -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<AutopilotMonitor.Shared.Models.AnalyzeRule> rules)
private async Task<(int deleted, int written, int orphanStatesGcd, int sunsetSkipped)> 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();
// 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<AutopilotMonitor.Shared.Models.ImeLogPattern> patterns)
src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs
+92 -34
View File
@@ -215,11 +215,16 @@ 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. 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.
/// 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 <see cref="BuiltInAnalyzeRules"/>) 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.
/// <para>
/// Returns <c>(deleted, written, orphanStatesGcd)</c> so the admin endpoint can
/// surface the cleanup count to the operator.
/// </para>
/// </summary>
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);
}
}
src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs
+43 -12
View File
@@ -318,7 +318,9 @@ namespace AutopilotMonitor.Functions.Services
}
/// <summary>
/// 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.
/// </summary>
public async Task<bool> 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 <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.
/// the moment the rule was ever re-introduced under the same ruleId.
/// <para>
/// Returns a tuple of <c>(deleted, failed)</c>. <c>deleted</c> counts both fresh
/// deletes and 404 (the row was already gone — idempotent retry). <c>failed</c>
/// is non-zero if any individual delete threw a non-404 error, or
/// <c>-1</c> if the cross-partition enumeration itself failed (we can't tell how
/// many rows are still out there). Callers must check <c>failed</c> 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.
/// </para>
/// </summary>
public async Task<int> 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<TableEntity>(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
}
/// <summary>
/// Deletes an analyze rule
/// Deletes an analyze rule. Idempotent: a 404 (the rule was already gone) counts
/// as success so the sunset orchestration in
/// <see cref="AnalyzeRuleService.EnsureBuiltInRulesSeededAsync"/> can retry without
/// flipping its happy-path on the missing-row condition.
/// </summary>
public async Task<bool> 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}");
src/Shared/AutopilotMonitor.Shared/DataAccess/IRuleRepository.cs
+9 -2
View File
@@ -28,9 +28,16 @@ namespace AutopilotMonitor.Shared.DataAccess
/// <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.
/// linger as dead state.
/// <para>
/// Returns <c>(deleted, failed)</c>. A 404 on an individual row is counted as
/// deleted (idempotent retry). <c>failed</c> is non-zero on per-row errors;
/// <c>-1</c> 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.
/// </para>
/// </summary>
Task<int> DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId);
Task<(int deleted, int failed)> DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId);
// --- Analyze Rules ---
Task<bool> StoreAnalyzeRuleAsync(AnalyzeRule rule, string tenantId = "global");