mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-26 21:47:13 +00:00
fix(analyze): slim correlation evidence + storage size guard + manual-path partial-success
Production session d434a3ba (12d stuck, ANALYZE-APP-012 first-ever fire) tripped Table
Storage's 64KB property limit on RuleResult.MatchedConditionsJson — the correlation
evaluator embedded full eventA/eventB Message + errorDetail (often multi-KB stack
traces) for every matched pair AND duplicated all pairs into an uncapped allMatches
list. The queue-path retry/poison architecture caught the StoreRuleResultAsync false
return correctly, but the underlying evidence shape was the root cause.
Three layers of fix:
1) Root cause — RuleEngine.ConditionEvaluators.cs (EvaluateEventCorrelationCondition):
- Drop eventA_message / eventB_message (free-text, can be huge)
- Drop errorDetail from AddDataFieldsToEvidence whitelist (often a stack trace)
- Cap allMatches at 10 pairs + matchesTruncated flag for the rest
- Per-pair evidence now ~500 bytes; UI lazy-loads full event via /events/{id}
2) Defense-in-depth — TableStorageService.Rules.cs (StoreRuleResultAsync):
Pre-flight check on JsonConvert.SerializeObject(MatchedConditions). If output
exceeds 60KB (4KB headroom under the 64KB hard limit), substitute a small
truncation marker {_truncated, _originalBytes, _reason} and log a warning.
Future rules that overshoot still persist + flag instead of failing opaquely.
3) Manual-path observability — GetRuleResultsFunction.cs (?reanalyze=true):
Check StoreRuleResultAsync bool, collect failed RuleIds, log structured Error,
return HTTP 200 with {success:false, persistFailureCount, persistFailureRuleIds}.
Partial-success rather than 500: UI keeps rendering the rules that DID persist.
Optional UI banner can read persistFailureCount in a follow-up.
Tests: +3 RuleEngineCorrelationEvidenceSlimTests pin the slim evidence shape, the
allMatches cap (25 pairs → 10 + truncated=true), and the no-truncate path (3 pairs
→ truncated=false). 773/773 backend pass.
Follow-up after deploy: re-queue the existing poison message (1a72afdc-...) for
session d434a3ba. With slim evidence the persist should succeed cleanly; only then
delete from poison. If it fails again, the 60KB guard adds a truncated marker and
the Rule still persists.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1 +1 @@
|
||||
443
|
||||
444
|
||||
|
||||
@@ -1 +1 @@
|
||||
1204
|
||||
1205
|
||||
|
||||
+174
@@ -0,0 +1,174 @@
|
||||
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 slim-evidence contract for <c>event_correlation</c> rules. Before this fix the
|
||||
/// per-pair evidence carried <c>eventA_message</c> / <c>eventB_message</c> plus a free-text
|
||||
/// <c>errorDetail</c> field, and <c>allMatches</c> was uncapped — long-running stuck sessions
|
||||
/// could push <c>RuleResult.MatchedConditionsJson</c> past Table Storage's 64KB property limit
|
||||
/// and the persist would silently return false. The queue worker (post-fix) caught the false
|
||||
/// and triggered a retry, but the underlying problem was still oversized evidence.
|
||||
/// </summary>
|
||||
public class RuleEngineCorrelationEvidenceSlimTests
|
||||
{
|
||||
private const string TenantId = "a1b2c3d4-e5f6-7890-abcd-ef1234567890";
|
||||
private const string SessionId = "b2c3d4e5-f6a7-8901-bcde-f12345678901";
|
||||
|
||||
[Fact]
|
||||
public async Task Correlation_evidence_omits_message_and_errorDetail_to_stay_slim()
|
||||
{
|
||||
var rule = BuildCorrelationRule();
|
||||
var baseTime = new DateTime(2026, 4, 28, 12, 0, 0, DateTimeKind.Utc);
|
||||
var events = new List<EnrollmentEvent>
|
||||
{
|
||||
BuildEvent("evt-A", "app_install_failed", appId: "abc", sequence: 1, timestamp: baseTime,
|
||||
errorDetail: new string('x', 8000), message: "long error string " + new string('y', 8000)),
|
||||
BuildEvent("evt-B", "app_install_completed", appId: "abc", sequence: 2, timestamp: baseTime.AddSeconds(5),
|
||||
errorDetail: null, message: "completion details " + new string('z', 8000)),
|
||||
};
|
||||
|
||||
var (engine, _) = SutWithRule(rule, events);
|
||||
var outcome = await engine.AnalyzeSessionAsync(TenantId, SessionId);
|
||||
|
||||
Assert.Single(outcome.Results);
|
||||
var primary = (Dictionary<string, object>)outcome.Results[0].MatchedConditions["enforcement_resolved"];
|
||||
|
||||
Assert.False(primary.ContainsKey("eventA_message"), "free-text message must not leak into evidence");
|
||||
Assert.False(primary.ContainsKey("eventB_message"), "free-text message must not leak into evidence");
|
||||
Assert.False(primary.ContainsKey("eventA_errorDetail"), "errorDetail (often stack trace) must not leak into evidence");
|
||||
Assert.False(primary.ContainsKey("eventB_errorDetail"), "errorDetail (often stack trace) must not leak into evidence");
|
||||
|
||||
// Identifying fields stay — they're short and necessary for the explanation.
|
||||
Assert.Equal("evt-A", primary["eventA_eventId"]);
|
||||
Assert.Equal("evt-B", primary["eventB_eventId"]);
|
||||
Assert.Equal("abc", primary["eventA_appId"]);
|
||||
Assert.Equal(1, primary["totalMatches"]);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Correlation_with_many_pairs_caps_allMatches_at_ten_and_flags_truncated()
|
||||
{
|
||||
var rule = BuildCorrelationRule();
|
||||
var baseTime = new DateTime(2026, 4, 28, 12, 0, 0, DateTimeKind.Utc);
|
||||
var events = new List<EnrollmentEvent>();
|
||||
// 25 distinct app failures + 25 matching completions — uncapped this would be 25 pairs.
|
||||
for (var i = 0; i < 25; i++)
|
||||
{
|
||||
events.Add(BuildEvent($"evt-A-{i}", "app_install_failed", appId: $"app-{i}", sequence: i * 2, timestamp: baseTime.AddSeconds(i * 10)));
|
||||
events.Add(BuildEvent($"evt-B-{i}", "app_install_completed", appId: $"app-{i}", sequence: i * 2 + 1, timestamp: baseTime.AddSeconds(i * 10 + 5)));
|
||||
}
|
||||
|
||||
var (engine, _) = SutWithRule(rule, events);
|
||||
var outcome = await engine.AnalyzeSessionAsync(TenantId, SessionId);
|
||||
|
||||
Assert.Single(outcome.Results);
|
||||
var primary = (Dictionary<string, object>)outcome.Results[0].MatchedConditions["enforcement_resolved"];
|
||||
|
||||
Assert.Equal(25, primary["totalMatches"]);
|
||||
Assert.True(primary.ContainsKey("allMatches"));
|
||||
var allMatches = (List<Dictionary<string, object>>)primary["allMatches"];
|
||||
Assert.Equal(10, allMatches.Count);
|
||||
Assert.True((bool)primary["matchesTruncated"]);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Correlation_with_few_pairs_does_not_truncate()
|
||||
{
|
||||
var rule = BuildCorrelationRule();
|
||||
var baseTime = new DateTime(2026, 4, 28, 12, 0, 0, DateTimeKind.Utc);
|
||||
var events = new List<EnrollmentEvent>();
|
||||
for (var i = 0; i < 3; i++)
|
||||
{
|
||||
events.Add(BuildEvent($"evt-A-{i}", "app_install_failed", appId: $"app-{i}", sequence: i * 2, timestamp: baseTime.AddSeconds(i * 10)));
|
||||
events.Add(BuildEvent($"evt-B-{i}", "app_install_completed", appId: $"app-{i}", sequence: i * 2 + 1, timestamp: baseTime.AddSeconds(i * 10 + 5)));
|
||||
}
|
||||
|
||||
var (engine, _) = SutWithRule(rule, events);
|
||||
var outcome = await engine.AnalyzeSessionAsync(TenantId, SessionId);
|
||||
|
||||
Assert.Single(outcome.Results);
|
||||
var primary = (Dictionary<string, object>)outcome.Results[0].MatchedConditions["enforcement_resolved"];
|
||||
|
||||
Assert.Equal(3, primary["totalMatches"]);
|
||||
var allMatches = (List<Dictionary<string, object>>)primary["allMatches"];
|
||||
Assert.Equal(3, allMatches.Count);
|
||||
Assert.False((bool)primary["matchesTruncated"]);
|
||||
}
|
||||
|
||||
// ============================================================ helpers
|
||||
|
||||
private static (RuleEngine engine, Mock<IRuleRepository> ruleRepo) SutWithRule(AnalyzeRule rule, List<EnrollmentEvent> events)
|
||||
{
|
||||
var ruleRepo = new Mock<IRuleRepository>();
|
||||
// The "global" partition holds built-in/community rules; the tenant-specific partition
|
||||
// holds custom rules. AnalyzeRuleService merges both. We park the rule under "global"
|
||||
// and return empty for the tenant partition so it doesn't show up twice.
|
||||
ruleRepo.Setup(r => r.GetAnalyzeRulesAsync("global")).ReturnsAsync(new List<AnalyzeRule> { rule });
|
||||
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.GetRuleResultsAsync(It.IsAny<string>(), It.IsAny<string>())).ReturnsAsync(new List<RuleResult>());
|
||||
|
||||
var sessionRepo = new Mock<ISessionRepository>();
|
||||
sessionRepo.Setup(s => s.GetSessionEventsAsync(TenantId, SessionId, It.IsAny<int>())).ReturnsAsync(events);
|
||||
|
||||
var ruleService = new AnalyzeRuleService(ruleRepo.Object, NullLogger<AnalyzeRuleService>.Instance);
|
||||
var engine = new RuleEngine(ruleService, ruleRepo.Object, sessionRepo.Object, NullLogger<RuleEngineCorrelationEvidenceSlimTests>.Instance);
|
||||
|
||||
return (engine, ruleRepo);
|
||||
}
|
||||
|
||||
private static AnalyzeRule BuildCorrelationRule() => new()
|
||||
{
|
||||
RuleId = "TEST-CORR-001",
|
||||
Title = "Test correlation rule",
|
||||
Severity = "warning",
|
||||
Category = "apps",
|
||||
Trigger = "correlation",
|
||||
Enabled = true,
|
||||
BaseConfidence = 80,
|
||||
IsBuiltIn = false,
|
||||
IsCommunity = false,
|
||||
Conditions = new List<RuleCondition>
|
||||
{
|
||||
new()
|
||||
{
|
||||
Signal = "enforcement_resolved",
|
||||
Source = "event_correlation",
|
||||
EventType = "app_install_failed",
|
||||
CorrelateEventType = "app_install_completed",
|
||||
JoinField = "appId",
|
||||
Required = true,
|
||||
}
|
||||
},
|
||||
ConfidenceFactors = new List<ConfidenceFactor>(),
|
||||
};
|
||||
|
||||
private static EnrollmentEvent BuildEvent(string eventId, string eventType, string appId, int sequence, DateTime timestamp, string? errorDetail = null, string? message = null)
|
||||
{
|
||||
var data = new Dictionary<string, object>
|
||||
{
|
||||
["appId"] = appId,
|
||||
["appName"] = $"App-{appId}",
|
||||
["errorPatternId"] = "IME-ERROR-ENFORCEMENT",
|
||||
};
|
||||
if (errorDetail != null)
|
||||
data["errorDetail"] = errorDetail;
|
||||
|
||||
return new EnrollmentEvent
|
||||
{
|
||||
EventId = eventId,
|
||||
TenantId = TenantId,
|
||||
SessionId = SessionId,
|
||||
EventType = eventType,
|
||||
Sequence = sequence,
|
||||
Timestamp = timestamp,
|
||||
Message = message ?? string.Empty,
|
||||
Data = data,
|
||||
};
|
||||
}
|
||||
}
|
||||
@@ -57,6 +57,13 @@ namespace AutopilotMonitor.Functions.Functions.Rules
|
||||
|
||||
var reanalyze = string.Equals(req.Query["reanalyze"], "true", StringComparison.OrdinalIgnoreCase);
|
||||
|
||||
// Rules whose StoreRuleResultAsync returned false during the on-demand reanalyze.
|
||||
// Reported back to the caller so the UI can render a warning banner without losing
|
||||
// the rules that DID persist. Mirrors the queue-path's throw-on-false retry trigger,
|
||||
// but stays partial-success here because the user is on a synchronous HTTP wait
|
||||
// and a 500 would blank the page even when most rules persisted fine.
|
||||
var persistFailureRuleIds = new List<string>();
|
||||
|
||||
if (reanalyze)
|
||||
{
|
||||
try
|
||||
@@ -69,10 +76,26 @@ namespace AutopilotMonitor.Functions.Functions.Rules
|
||||
|
||||
foreach (var result in outcome.Results)
|
||||
{
|
||||
await _ruleRepo.StoreRuleResultAsync(result);
|
||||
var stored = await _ruleRepo.StoreRuleResultAsync(result);
|
||||
if (!stored)
|
||||
{
|
||||
persistFailureRuleIds.Add(result.RuleId);
|
||||
}
|
||||
}
|
||||
|
||||
_logger.LogInformation($"On-demand re-analysis for session {sessionId}: {outcome.Results.Count} issue(s) detected");
|
||||
if (persistFailureRuleIds.Count > 0)
|
||||
{
|
||||
_logger.LogError(
|
||||
"On-demand re-analysis for session {SessionId}: {FailedCount} of {TotalCount} rule result(s) failed to persist: [{FailedRuleIds}]",
|
||||
sessionId,
|
||||
persistFailureRuleIds.Count,
|
||||
outcome.Results.Count,
|
||||
string.Join(", ", persistFailureRuleIds));
|
||||
}
|
||||
else
|
||||
{
|
||||
_logger.LogInformation($"On-demand re-analysis for session {sessionId}: {outcome.Results.Count} issue(s) detected");
|
||||
}
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
@@ -85,13 +108,19 @@ namespace AutopilotMonitor.Functions.Functions.Rules
|
||||
var response = req.CreateResponse(HttpStatusCode.OK);
|
||||
await response.WriteAsJsonAsync(new
|
||||
{
|
||||
success = true,
|
||||
// success=false when any rule failed to persist during the reanalyze loop above.
|
||||
// UI keeps showing the persisted results; consumers can branch on persistFailureCount
|
||||
// to render a warning banner. persistFailureRuleIds is omitted entirely (null) when
|
||||
// there were no failures, so the existing UI contract stays unchanged for the happy path.
|
||||
success = persistFailureRuleIds.Count == 0,
|
||||
sessionId,
|
||||
results,
|
||||
totalIssues = results.Count,
|
||||
criticalCount = results.Count(r => r.Severity == "critical"),
|
||||
highCount = results.Count(r => r.Severity == "high"),
|
||||
warningCount = results.Count(r => r.Severity == "warning")
|
||||
warningCount = results.Count(r => r.Severity == "warning"),
|
||||
persistFailureCount = persistFailureRuleIds.Count,
|
||||
persistFailureRuleIds = persistFailureRuleIds.Count > 0 ? persistFailureRuleIds : null
|
||||
});
|
||||
return response;
|
||||
}
|
||||
|
||||
@@ -632,7 +632,13 @@ namespace AutopilotMonitor.Functions.Services
|
||||
continue;
|
||||
matchedJoinKeys.Add(bKey);
|
||||
|
||||
// Build evidence for this correlated pair
|
||||
// Build evidence for this correlated pair. Keep it slim — store only event
|
||||
// references (id/type/sequence/timestamp) plus the join key and a small set of
|
||||
// short identifying data fields. The UI lazy-loads the full event via /events/{id}
|
||||
// when the user expands the evidence row, so we don't need to embed Message or
|
||||
// multi-KB stack-traces here. This caps per-pair evidence at ~500 bytes and keeps
|
||||
// RuleResult.MatchedConditionsJson well below Table Storage's 64KB property limit
|
||||
// even when many pairs match (e.g. ANALYZE-APP-012 on long-running stuck sessions).
|
||||
var pair = new Dictionary<string, object>
|
||||
{
|
||||
["joinField"] = condition.JoinField,
|
||||
@@ -641,16 +647,14 @@ namespace AutopilotMonitor.Functions.Services
|
||||
["eventA_eventType"] = matchingA.EventType ?? string.Empty,
|
||||
["eventA_timestamp"] = matchingA.Timestamp,
|
||||
["eventA_sequence"] = matchingA.Sequence,
|
||||
["eventA_message"] = matchingA.Message ?? string.Empty,
|
||||
["eventB_eventId"] = eventB.EventId ?? string.Empty,
|
||||
["eventB_eventType"] = eventB.EventType ?? string.Empty,
|
||||
["eventB_timestamp"] = eventB.Timestamp,
|
||||
["eventB_sequence"] = eventB.Sequence,
|
||||
["eventB_message"] = eventB.Message ?? string.Empty,
|
||||
["timeDeltaSeconds"] = (eventB.Timestamp - matchingA.Timestamp).TotalSeconds
|
||||
};
|
||||
|
||||
// Add well-known data fields from both events
|
||||
// Add slim identifying fields from both events (no free-text fields).
|
||||
AddDataFieldsToEvidence(pair, matchingA, "eventA_", condition.JoinField);
|
||||
AddDataFieldsToEvidence(pair, eventB, "eventB_", condition.JoinField);
|
||||
|
||||
@@ -660,24 +664,41 @@ namespace AutopilotMonitor.Functions.Services
|
||||
if (!matchedPairs.Any())
|
||||
return (false, "no correlated event pairs found");
|
||||
|
||||
// Single match: flat dictionary; multiple: first as primary + allMatches list
|
||||
// Single match: flat dictionary; multiple: first as primary + capped allMatches list.
|
||||
// Cap protects against runaway evidence size — sessions with hundreds of correlated
|
||||
// pairs would otherwise produce MatchedConditionsJson > 64KB and fail Table Storage
|
||||
// persistence (see also defense-in-depth check in TableStorageService.StoreRuleResultAsync).
|
||||
const int MaxAllMatches = 10;
|
||||
var primary = matchedPairs[0];
|
||||
primary["totalMatches"] = matchedPairs.Count;
|
||||
if (matchedPairs.Count > 1)
|
||||
{
|
||||
primary["allMatches"] = matchedPairs;
|
||||
if (matchedPairs.Count > MaxAllMatches)
|
||||
{
|
||||
primary["allMatches"] = matchedPairs.Take(MaxAllMatches).ToList();
|
||||
primary["matchesTruncated"] = true;
|
||||
}
|
||||
else
|
||||
{
|
||||
primary["allMatches"] = matchedPairs;
|
||||
primary["matchesTruncated"] = false;
|
||||
}
|
||||
}
|
||||
return (true, primary);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Adds well-known data fields from an event to the evidence dictionary with a prefix.
|
||||
/// Adds slim identifying data fields from an event to the evidence dictionary with a prefix.
|
||||
/// Free-text fields like <c>errorDetail</c> (often a stack trace) and <c>message</c> are
|
||||
/// deliberately excluded to keep <see cref="RuleResult.MatchedConditions"/> below Table
|
||||
/// Storage's 64KB property limit. UI fetches the full event on demand via /events/{id}.
|
||||
/// </summary>
|
||||
private void AddDataFieldsToEvidence(Dictionary<string, object> evidence, EnrollmentEvent evt, string prefix, string joinField)
|
||||
{
|
||||
if (evt.Data == null) return;
|
||||
|
||||
var knownFields = new[] { "appId", "appName", "errorPatternId", "errorDetail", "errorCode", "exitCode", "status" };
|
||||
// Whitelist: short identifiers only. errorDetail/message removed (can be multi-KB).
|
||||
var knownFields = new[] { "appId", "appName", "errorPatternId", "errorCode", "exitCode", "status" };
|
||||
|
||||
foreach (var field in knownFields)
|
||||
{
|
||||
|
||||
@@ -14,9 +14,24 @@ namespace AutopilotMonitor.Functions.Services
|
||||
{
|
||||
// ===== RULE RESULTS METHODS =====
|
||||
|
||||
/// <summary>
|
||||
/// Azure Table Storage hard-limits each property to 64 KiB (UTF-16). We trip the
|
||||
/// guard rail at 60 KiB to leave headroom for the property-name prefix + framing
|
||||
/// bytes the storage layer adds on the wire.
|
||||
/// </summary>
|
||||
private const int MatchedConditionsJsonByteLimit = 60 * 1024;
|
||||
|
||||
/// <summary>
|
||||
/// Stores a rule evaluation result
|
||||
/// PartitionKey: {TenantId}_{SessionId}, RowKey: RuleId
|
||||
/// <para>
|
||||
/// Defense-in-depth: rule authors should keep <see cref="RuleResult.MatchedConditions"/>
|
||||
/// under <see cref="MatchedConditionsJsonByteLimit"/> bytes (correlation evidence
|
||||
/// already caps and slims its pairs). If a future rule still produces oversized
|
||||
/// output, we substitute a truncation marker rather than letting the entire write
|
||||
/// fail — the rule still fires, the UI sees the explanation + flag, and we get an
|
||||
/// observable warning instead of an opaque store failure.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public async Task<bool> StoreRuleResultAsync(RuleResult result)
|
||||
{
|
||||
@@ -25,6 +40,22 @@ namespace AutopilotMonitor.Functions.Services
|
||||
var tableClient = _tableServiceClient.GetTableClient(Constants.TableNames.RuleResults);
|
||||
var partitionKey = $"{result.TenantId}_{result.SessionId}";
|
||||
|
||||
var matchedJson = JsonConvert.SerializeObject(result.MatchedConditions ?? new Dictionary<string, object>());
|
||||
if (matchedJson.Length > MatchedConditionsJsonByteLimit)
|
||||
{
|
||||
_logger.LogWarning(
|
||||
"Rule result {RuleId} (session {SessionId}) MatchedConditionsJson is {Bytes} bytes, exceeds {Limit} byte guard. Substituting truncation marker so the rule still persists.",
|
||||
result.RuleId, result.SessionId, matchedJson.Length, MatchedConditionsJsonByteLimit);
|
||||
|
||||
var truncated = new Dictionary<string, object>
|
||||
{
|
||||
["_truncated"] = true,
|
||||
["_originalBytes"] = matchedJson.Length,
|
||||
["_reason"] = "MatchedConditions exceeded Table Storage 64KB property limit"
|
||||
};
|
||||
matchedJson = JsonConvert.SerializeObject(truncated);
|
||||
}
|
||||
|
||||
var entity = new TableEntity(partitionKey, result.RuleId)
|
||||
{
|
||||
["ResultId"] = result.ResultId,
|
||||
@@ -38,7 +69,7 @@ namespace AutopilotMonitor.Functions.Services
|
||||
["Explanation"] = result.Explanation ?? string.Empty,
|
||||
["RemediationJson"] = JsonConvert.SerializeObject(result.Remediation ?? new List<RemediationStep>()),
|
||||
["RelatedDocsJson"] = JsonConvert.SerializeObject(result.RelatedDocs ?? new List<RelatedDoc>()),
|
||||
["MatchedConditionsJson"] = JsonConvert.SerializeObject(result.MatchedConditions ?? new Dictionary<string, object>()),
|
||||
["MatchedConditionsJson"] = matchedJson,
|
||||
["DetectedAt"] = result.DetectedAt
|
||||
};
|
||||
|
||||
|
||||
Reference in New Issue
Block a user