fix(auditlog)+fix(configdb): budget the ingest fallback loop; index + varchar-type the SiteCalls list query (arch-review 2026-08-19 P5-1 + P5-2)

P5-1: AuditLogIngestActor.OnCachedTelemetryAsync's per-entry fallback loop
(used when the cached-telemetry batch fast path fails) ran with no
budget/cancellation, unlike OnIngestAsync's fallback. A slow/wedged DB could
freeze the central ingest singleton's mailbox for minutes. The loop now runs
under its own fresh IngestFallbackBudget CancellationTokenSource (mirroring
OnIngestAsync), passes the token into BeginTransactionAsync/
InsertIfNotExistsAsync/UpsertAsync, and stops early on budget exhaustion,
logging how many entries were left for the periodic reconciliation pull to
pick up.

P5-2: SiteCallAuditRepository.QueryAsync ordered by
(CreatedAtUtc DESC, TrackedOperationId DESC) with no index leading
unconditionally on CreatedAtUtc, forcing a full scan+sort on an unfiltered
page. Added IX_SiteCalls_Created_TrackedOperationId (migration
AddSiteCallsListSortIndex + matching hand-written production-parity script
under docs/plans/sql/). Also switched the query from FromSqlInterpolated
(which binds string filters as nvarchar(4000)) to FromSqlRaw with explicitly
SqlDbType.VarChar-typed parameters, matching the SiteCalls columns'
IsUnicode(false) declaration and the write path's existing Param(...)
pattern, so seeks — including the keyset predicate — are no longer blocked
by an implicit conversion.

Tests: new AuditLogIngestActorCombinedTelemetryTests case proves the
fallback budget bounds the wait (hanging repo stub, ~5s not indefinite),
that remaining entries are skipped, and that a non-default/cancellable
token reaches the repository call; verified fail-unfixed by reverting the
actor change (test times out at its 20s bound). Existing
SiteCallAuditRepositoryTests QueryAsync/keyset/stuck-cutoff cases (real
MSSQL) confirm the raw-SQL rewrite still returns correct pages.
This commit is contained in:
Joseph Doherty
2026-08-19 14:44:44 -04:00
parent 6c11362d9f
commit eca0ab768d
8 changed files with 2488 additions and 32 deletions
@@ -325,7 +325,12 @@ public class AuditLogIngestActor : ReceiveActor
/// <see cref="Microsoft.EntityFrameworkCore.RelationalDatabaseFacadeExtensions.BeginTransactionAsync"/>
/// scope and a try/catch around it). Audit-write failure NEVER aborts the
/// user-facing action — the site keeps the row Pending and retries on the
/// next drain.
/// next drain. The per-entry loop (used when <see cref="TryIngestCachedBatchAsync"/>
/// falls back) runs under its own fresh <see cref="IngestFallbackBudget"/>
/// token, mirroring <see cref="OnIngestAsync"/>'s fallback — a slow or wedged
/// database can only cost this handler that one short budget, not an
/// unbounded per-entry loop, so the singleton's mailbox is never frozen for
/// the life of a large packet.
/// </remarks>
private async Task OnCachedTelemetryAsync(IngestCachedTelemetryCommand cmd)
{
@@ -369,14 +374,38 @@ public class AuditLogIngestActor : ReceiveActor
return;
}
// The per-entry fallback gets its OWN fresh, short budget — mirroring
// OnIngestAsync's fallback (see IngestFallbackBudget above). Without
// this, an unbounded loop of per-entry transactions (each wrapped in
// SqlServerRetryingExecutionStrategy's 5x30s retry ladder) can run for
// minutes inside this ReceiveAsync handler, freezing the singleton's
// mailbox: every queued IngestAuditEventsCommand/IngestCachedTelemetryCommand
// stalls behind it, sites time out and re-send, and the pile-up
// compounds. Budget exhaustion stops the loop early; entries left
// over are simply never added to `accepted`, so the site keeps them
// Pending and the existing lost-telemetry path (periodic
// reconciliation pull) picks up the remainder.
using var fallbackBudget = new CancellationTokenSource(IngestFallbackBudget);
var entryIndex = 0;
foreach (var entry in cmd.Entries)
{
entryIndex++;
if (fallbackBudget.IsCancellationRequested)
{
var remaining = cmd.Entries.Count - entryIndex + 1;
_logger.LogWarning(
"Combined telemetry per-entry fallback exhausted its {FallbackSeconds}s budget; leaving {Remaining} entr(ies) unaccepted for this drain — the site keeps them Pending and periodic reconciliation will pick up the remainder.",
IngestFallbackBudget.TotalSeconds,
remaining);
break;
}
try
{
await strategy.ExecuteAsync(async () =>
{
await using var tx = await dbContext.Database
.BeginTransactionAsync()
.BeginTransactionAsync(fallbackBudget.Token)
.ConfigureAwait(false);
// Stamp IngestedAtUtc on both rows from a single
@@ -396,12 +425,12 @@ public class AuditLogIngestActor : ReceiveActor
var auditStamped = AuditRowProjection.WithIngestedAtUtc(filteredAudit, ingestedAt);
var siteCallStamped = entry.SiteCall with { IngestedAtUtc = ingestedAt };
await auditRepo.InsertIfNotExistsAsync(auditStamped)
await auditRepo.InsertIfNotExistsAsync(auditStamped, fallbackBudget.Token)
.ConfigureAwait(false);
await siteCallRepo.UpsertAsync(siteCallStamped)
await siteCallRepo.UpsertAsync(siteCallStamped, fallbackBudget.Token)
.ConfigureAwait(false);
await tx.CommitAsync().ConfigureAwait(false);
await tx.CommitAsync(fallbackBudget.Token).ConfigureAwait(false);
}).ConfigureAwait(false);
accepted.Add(entry.Audit.EventId);
@@ -106,5 +106,17 @@ public class SiteCallEntityTypeConfiguration : IEntityTypeConfiguration<SiteCall
builder.HasIndex(s => s.TerminalAtUtc)
.HasDatabaseName("IX_SiteCalls_Terminal")
.HasFilter("[TerminalAtUtc] IS NOT NULL");
// Created_TrackedOperationId backs QueryAsync's list-page ORDER BY
// (CreatedAtUtc DESC, TrackedOperationId DESC) and its matching keyset
// predicate — the unfiltered/lightly-filtered Central UI Site Calls page.
// None of the indexes above lead on CreatedAtUtc unconditionally
// (Source_Created requires an equality on SourceSite first;
// NonTerminal/Terminal are filtered to one side of TerminalAtUtc), so an
// unfiltered page previously fell back to a full clustered-index scan +
// sort (arch-review 2026-08-19 P5-2, register row-35 class).
builder.HasIndex(s => new { s.CreatedAtUtc, s.TrackedOperationId })
.IsDescending(true, true)
.HasDatabaseName("IX_SiteCalls_Created_TrackedOperationId");
}
}
@@ -0,0 +1,47 @@
using Microsoft.EntityFrameworkCore.Migrations;
#nullable disable
namespace ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Migrations
{
/// <summary>
/// Adds <c>IX_SiteCalls_Created_TrackedOperationId (CreatedAtUtc DESC,
/// TrackedOperationId DESC)</c> — arch-review 2026-08-19 P5-2, register
/// row-35 class.
/// </summary>
/// <remarks>
/// <see cref="ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories.SiteCallAuditRepository.QueryAsync"/>
/// orders <c>(CreatedAtUtc DESC, TrackedOperationId DESC)</c> and its keyset
/// predicate is exactly that pair. None of the existing SiteCalls indexes
/// lead unconditionally on <c>CreatedAtUtc</c> — <c>IX_SiteCalls_Source_Created</c>
/// requires an equality on <c>SourceSite</c> first, and
/// <c>IX_SiteCalls_NonTerminal</c>/<c>IX_SiteCalls_Terminal</c> are each
/// filtered to one side of <c>TerminalAtUtc</c> — so an unfiltered (or
/// lightly filtered) list page fell back to a full clustered-index scan +
/// sort. With both key columns descending, a forward scan of this index
/// already delivers rows in the requested order, so a keyset page reads a
/// Top-N off the index instead of sorting the whole table on every page
/// request. See <c>docs/plans/sql/AddSiteCallsListSortIndex.sql</c> for the
/// handwritten production-parity script.
/// </remarks>
public partial class AddSiteCallsListSortIndex : Migration
{
/// <inheritdoc />
protected override void Up(MigrationBuilder migrationBuilder)
{
migrationBuilder.CreateIndex(
name: "IX_SiteCalls_Created_TrackedOperationId",
table: "SiteCalls",
columns: new[] { "CreatedAtUtc", "TrackedOperationId" },
descending: new bool[0]);
}
/// <inheritdoc />
protected override void Down(MigrationBuilder migrationBuilder)
{
migrationBuilder.DropIndex(
name: "IX_SiteCalls_Created_TrackedOperationId",
table: "SiteCalls");
}
}
}
@@ -171,6 +171,10 @@ namespace ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Migrations
.HasDatabaseName("IX_SiteCalls_Terminal")
.HasFilter("[TerminalAtUtc] IS NOT NULL");
b.HasIndex("CreatedAtUtc", "TrackedOperationId")
.IsDescending()
.HasDatabaseName("IX_SiteCalls_Created_TrackedOperationId");
b.HasIndex("SourceSite", "CreatedAtUtc")
.IsDescending(false, true)
.HasDatabaseName("IX_SiteCalls_Source_Created");
@@ -274,12 +274,19 @@ WHERE TrackedOperationId = @Id
throw new ArgumentNullException(nameof(paging));
}
// FormattableString interpolation parameterises every value (no concatenation)
// so this is injection-safe. EF Core resolves the parameter values, the
// composed sql is shaped to SQL Server's grammar and projected into the
// SiteCall entity via FromSqlInterpolated. The CASE expressions wrap each
// optional predicate so a null filter field degrades to a no-op (matches
// every row) instead of branching at C# level into N variants.
// Raw SQL with NAMED parameters (not FromSqlInterpolated) — arch-review
// 2026-08-19 P5-2. FromSqlInterpolated binds every interpolated string
// value as an nvarchar(4000) parameter by default; every SiteCalls string
// column is IsUnicode(false) (varchar), so those parameters forced an
// implicit CONVERT on the column side of each comparison and blocked
// index seeks — including the keyset predicate below, which is exactly
// what IX_SiteCalls_Created_TrackedOperationId exists to serve. Each
// string filter now gets an explicitly-typed SqlDbType.VarChar parameter,
// mirroring the write path's Param(...) helper above. A named ADO.NET
// parameter can appear more than once in the SQL text (unlike a
// positional FromSqlInterpolated hole) while still being added to the
// command's Parameters collection exactly once, so the "IS NULL OR
// col = @p" shape needs no duplicate parameter objects.
var afterCreated = paging.AfterCreatedAtUtc;
var afterIdString = paging.AfterId?.Value.ToString("D");
var hasCursor = afterCreated is not null && afterIdString is not null;
@@ -294,37 +301,53 @@ WHERE TrackedOperationId = @Id
// never under-filled with a non-null next cursor. Mirrors how
// NotificationOutboxRepository.QueryAsync applies NotificationOutboxFilter.StuckCutoff.
//
// SELECT-list maintenance: EF Core's FromSqlInterpolated requires every
// SELECT-list maintenance: EF Core's FromSqlRaw requires every
// entity-tracked column to appear in the result set. Adding a new column
// to the SiteCall entity means extending the list below too — otherwise
// every read trips "The required column 'X' was not present" at runtime.
FormattableString sql = $@"
SELECT TOP ({paging.PageSize})
const string sql = @"
SELECT TOP (@PageSize)
TrackedOperationId, Channel, Target, SourceSite, SourceNode, Status, RetryCount,
LastError, HttpStatus, CreatedAtUtc, UpdatedAtUtc, TerminalAtUtc, IngestedAtUtc
FROM dbo.SiteCalls
WHERE ({filter.Channel} IS NULL OR Channel = {filter.Channel})
AND ({filter.SourceSite} IS NULL OR SourceSite = {filter.SourceSite})
AND ({filter.SourceNode} IS NULL OR SourceNode = {filter.SourceNode})
AND ({filter.Status} IS NULL OR Status = {filter.Status})
AND ({filter.Target} IS NULL OR Target = {filter.Target})
AND ({fromUtc} IS NULL OR CreatedAtUtc >= {fromUtc})
AND ({toUtc} IS NULL OR CreatedAtUtc <= {toUtc})
AND ({stuckCutoff} IS NULL OR (TerminalAtUtc IS NULL AND CreatedAtUtc < {stuckCutoff}))
AND ({(hasCursor ? 1 : 0)} = 0
OR CreatedAtUtc < {afterCreated}
OR (CreatedAtUtc = {afterCreated} AND TrackedOperationId < {afterIdString}))
WHERE (@Channel IS NULL OR Channel = @Channel)
AND (@SourceSite IS NULL OR SourceSite = @SourceSite)
AND (@SourceNode IS NULL OR SourceNode = @SourceNode)
AND (@Status IS NULL OR Status = @Status)
AND (@Target IS NULL OR Target = @Target)
AND (@FromUtc IS NULL OR CreatedAtUtc >= @FromUtc)
AND (@ToUtc IS NULL OR CreatedAtUtc <= @ToUtc)
AND (@StuckCutoff IS NULL OR (TerminalAtUtc IS NULL AND CreatedAtUtc < @StuckCutoff))
AND (@HasCursor = 0
OR CreatedAtUtc < @AfterCreated
OR (CreatedAtUtc = @AfterCreated AND TrackedOperationId < @AfterId))
ORDER BY CreatedAtUtc DESC, TrackedOperationId DESC
-- Every filter above is the (@p IS NULL OR col = @p) optional-parameter shape, so a
-- single cached plan would be parameter-sniffed for whichever filters happened to be
-- non-null on first compile. RECOMPILE lets the optimizer prune the dead (@p IS NULL)
-- predicates per invocation and pick IX_SiteCalls_Status_Updated / IX_SiteCalls_NonTerminal
-- for the filters actually supplied (arch-review 04, P5). Per-invocation compile cost is
-- negligible at UI-page cadence.
-- predicates per invocation and pick IX_SiteCalls_Status_Updated / IX_SiteCalls_NonTerminal /
-- IX_SiteCalls_Created_TrackedOperationId for the filters actually supplied (arch-review 04,
-- P5). Per-invocation compile cost is negligible at UI-page cadence.
OPTION (RECOMPILE);";
var parameters = new object[]
{
Param("@PageSize", SqlDbType.Int, paging.PageSize),
Param("@Channel", SqlDbType.VarChar, filter.Channel),
Param("@SourceSite", SqlDbType.VarChar, filter.SourceSite),
Param("@SourceNode", SqlDbType.VarChar, filter.SourceNode),
Param("@Status", SqlDbType.VarChar, filter.Status),
Param("@Target", SqlDbType.VarChar, filter.Target),
Param("@FromUtc", SqlDbType.DateTime2, fromUtc),
Param("@ToUtc", SqlDbType.DateTime2, toUtc),
Param("@StuckCutoff", SqlDbType.DateTime2, stuckCutoff),
Param("@HasCursor", SqlDbType.Int, hasCursor ? 1 : 0),
Param("@AfterCreated", SqlDbType.DateTime2, afterCreated),
Param("@AfterId", SqlDbType.VarChar, afterIdString),
};
var rows = await _context.Set<SiteCall>()
.FromSqlInterpolated(sql)
.FromSqlRaw(sql, parameters)
.AsNoTracking()
.ToListAsync(ct);