fix(central): review findings — no client-side audit truncation, insert-first upsert, QI-safe scripts, honest operator replies
Six adversarial-review findings in the central SQL/ingest layer. F1 (AuditLogRepository.InsertChunkAsync) — the set-based ingest declared each string parameter at its COLUMN width (Actor/Target 256, Action 64, Outcome 16, Category 32, SourceNode 64), so SqlClient truncated an over-long value at bind time and committed the mutilated row — silent, in an append-only store, with no PayloadTruncated flag — while the per-row and reconciliation paths sent the same value in full and let the server reject it with 2628. Bind at the value's own length instead; explicit SqlDbType is kept (it fixes the VALUES constructor's derived column types and datetime2 precision). Design: reject everywhere, truncate nowhere — matching today's per-row behaviour. F2 (SiteCallAuditRepository.UpsertAsync) — the single-statement upsert ran the monotonic UPDATE first and INSERTed only if nothing matched. Two writers racing the first packet of one TrackedOperationId (the cached dual-write and the reconciliation pull carry DIFFERENT lifecycle states) both matched nothing, and the loser then skipped its INSERT or swallowed a 2627 — dropping its Status/RetryCount/HttpStatus/TerminalAtUtc. Legs swapped to `IF NOT EXISTS … INSERT; UPDATE <monotonic>` — still one round trip, and the loser's UPDATE now lands on the winner's row. The duplicate-key catch re-runs the monotonic UPDATE for the same reason. Moved to raw SQL with explicitly-typed parameters so the intricate rank predicate exists in exactly one place (an untyped DateTime would bind as `datetime` and round the freshness tiebreaker). F3 (docs/plans/sql/*.sql) — filtered-index DDL failed with error 1934 under the documented `docker exec … sqlcmd` path, which defaults QUOTED_IDENTIFIER OFF; once IX_Notifications_Delivered exists, QI-OFF DML on Notifications fails too. All four scripts now open with `SET QUOTED_IDENTIFIER ON; SET ANSI_NULLS ON; GO` (own batch, so it is in force when the next batch parses), and the migration convention in Component-ConfigurationDatabase.md documents `sqlcmd -I`. Verified live: the pre-fix script fails 1934 without -I, the fixed one applies. F4 (SiteCallAuditActor) — the off-mailbox reconciliation/purge passes reuse the injected repository, so tests drove one DbContext from the pass and a mailbox handler concurrently. Serialized at the CALL via a private SerializedRepository wrapper applied only by the test constructors, rather than running the pass on-mailbox: production keeps its PipeTo shape untouched, and the existing "a blocked drain does not stall ingest/query/KPI" regression tests stay meaningful (they would have been invalidated by suspending the mailbox). F5 (AuditLogIngestActor) — when the batch failed because the 20 s IngestBudget expired, the per-row fallback reused the same expired token: N instant failures, N counter bumps, zero accepted. The fallback now gets a fresh 5 s budget (inside the 30 s outer Ask), and a blown budget bumps the failure counter ONCE for the batch instead of once per row. F6 (NotificationOutboxRepository.UpdateAsync) — ExecuteUpdate's row count was discarded, so an operator Retry/Discard of a notification the retention purge had already deleted reported success (the pre-ExecuteUpdate code threw DbUpdateConcurrencyException). UpdateAsync now returns whether a row matched; the operator one-shots answer "notification not found" and emit no audit row for the action that did not happen, while the dispatcher logs a warning (its delivery already happened; nothing to retry). GetByIdAsync switched to AsNoTracking since the write is out-of-band. Tests: 5 new SQL-backed regressions (over-long Target rejected on both paths + boundary round-trip; concurrent first-write and already-created-by-another-writer upserts; vanished-row UpdateAsync), a token-identity pin on the ingest fallback, a repository-concurrency detector for the SiteCallAudit passes, and vanished-row operator-path tests. The F1/F2/F4 regressions were each confirmed failing against the pre-fix code. Suites: ConfigurationDatabase 369, AuditLog 378, SiteCallAudit 66, NotificationOutbox 152 — all green, solution builds with 0 warnings.
This commit is contained in:
Joseph Doherty
+137
-72
@@ -1,3 +1,4 @@
|
||||
using System.Data;
|
||||
using Microsoft.Data.SqlClient;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Logging;
|
||||
@@ -72,20 +73,26 @@ public class SiteCallAuditRepository : ISiteCallAuditRepository
|
||||
var idText = siteCall.TrackedOperationId.Value.ToString("D");
|
||||
var incomingRank = GetRankOrThrow(siteCall.Status);
|
||||
|
||||
// ONE round trip, UPDATE-first (WP2.2). The predecessor issued an
|
||||
// unconditional IF NOT EXISTS … INSERT and THEN a monotonic UPDATE — two
|
||||
// statements, two round trips, on every single packet, of which the insert
|
||||
// half was wasted work for every packet after the first (the steady state:
|
||||
// a cached call emits Submitted → Forwarded → Attempted → terminal, so
|
||||
// three of four packets hit an existing row).
|
||||
// ONE round trip, INSERT-first. Both statements ship in a single command
|
||||
// text, so the round-trip saving of the WP2.2 rewrite is preserved — but
|
||||
// the ORDER is back to insert-then-update, because UPDATE-first LOSES
|
||||
// DATA under a concurrent first-write.
|
||||
//
|
||||
// The combined batch below runs UPDATE first and inserts only when the
|
||||
// UPDATE matched nothing AND the row genuinely does not exist. The
|
||||
// NOT EXISTS re-check is load-bearing: @@ROWCOUNT = 0 is ALSO what a
|
||||
// monotonic REJECTION looks like (a stale or regressive packet against an
|
||||
// existing row), and inserting there would resurrect a row the guard just
|
||||
// refused. Both statements ship in one command text, so this is one
|
||||
// round trip, not two.
|
||||
// The UPDATE-first shape was: UPDATE; SET @updated = @@ROWCOUNT;
|
||||
// IF @updated = 0 AND NOT EXISTS(…) INSERT. Two writers racing the FIRST
|
||||
// packet of one TrackedOperationId — the cached dual-write and the
|
||||
// reconciliation pull, which routinely carry DIFFERENT lifecycle states —
|
||||
// both find no row, so both UPDATEs match nothing. The loser then either
|
||||
// fails its own NOT EXISTS re-check (READ COMMITTED, after the winner
|
||||
// committed) and skips the INSERT, or attempts it and eats a 2627 in the
|
||||
// catch below. EITHER WAY the loser's Status/RetryCount/HttpStatus/
|
||||
// TerminalAtUtc are silently dropped: it never ran an UPDATE against the
|
||||
// winner's row.
|
||||
//
|
||||
// INSERT-first has no such hole. The loser's INSERT is skipped or faults,
|
||||
// and the monotonic UPDATE that FOLLOWS it applies its state to whichever
|
||||
// row won — so the newer lifecycle state survives regardless of
|
||||
// interleaving, and a stale one is still rejected by the rank guard.
|
||||
//
|
||||
// Monotonic update semantics are unchanged: mutate when EITHER the
|
||||
// incoming rank is strictly greater, OR the incoming rank equals the
|
||||
@@ -95,7 +102,19 @@ public class SiteCallAuditRepository : ISiteCallAuditRepository
|
||||
// live instead of freezing at the first Attempted packet. Terminal ranks
|
||||
// are excluded from the tiebreaker, so a later terminal NEVER overwrites
|
||||
// an earlier one; equal stamps are inert (idempotent replay) and a lower
|
||||
// rank is always a no-op.
|
||||
// rank is always a no-op. That inertness is what makes the UPDATE
|
||||
// harmless immediately after this same call's own INSERT: equal rank,
|
||||
// equal UpdatedAtUtc, zero rows changed.
|
||||
//
|
||||
// Raw SQL with explicitly-typed parameters (rather than
|
||||
// ExecuteSqlInterpolated) so the monotonic UPDATE exists as ONE statement
|
||||
// text shared by the combined batch and the duplicate-key retry below —
|
||||
// the predicate is far too intricate to keep in two copies. Explicit
|
||||
// SqlDbType is load-bearing: an untyped DateTime parameter binds as
|
||||
// `datetime` (3.33 ms rounding), which would corrupt both the stored
|
||||
// datetime2 stamps and the `UpdatedAtUtc <` freshness tiebreaker. Sizes
|
||||
// are deliberately left at the value's own length so the server enforces
|
||||
// the column widths (see AuditLogRepository.AddParameter).
|
||||
//
|
||||
// SourceNode-stamping: the column is included in the INSERT column list /
|
||||
// VALUES so a fresh row carries the originating node name (node-a/node-b
|
||||
@@ -117,57 +136,11 @@ public class SiteCallAuditRepository : ISiteCallAuditRepository
|
||||
// idempotent.
|
||||
try
|
||||
{
|
||||
await _context.Database.ExecuteSqlInterpolatedAsync(
|
||||
$@"DECLARE @updated int;
|
||||
|
||||
UPDATE dbo.SiteCalls
|
||||
SET Status = {siteCall.Status},
|
||||
RetryCount = {siteCall.RetryCount},
|
||||
LastError = {siteCall.LastError},
|
||||
HttpStatus = {siteCall.HttpStatus},
|
||||
UpdatedAtUtc = {siteCall.UpdatedAtUtc},
|
||||
TerminalAtUtc = {siteCall.TerminalAtUtc},
|
||||
IngestedAtUtc = {siteCall.IngestedAtUtc},
|
||||
SourceNode = COALESCE({siteCall.SourceNode}, SourceNode)
|
||||
WHERE TrackedOperationId = {idText}
|
||||
AND ( {incomingRank} > (CASE Status
|
||||
WHEN 'Submitted' THEN 0
|
||||
WHEN 'Forwarded' THEN 1
|
||||
WHEN 'Attempted' THEN 2
|
||||
WHEN 'Skipped' THEN 2
|
||||
WHEN 'Delivered' THEN 3
|
||||
WHEN 'Failed' THEN 3
|
||||
WHEN 'Parked' THEN 3
|
||||
WHEN 'Discarded' THEN 3
|
||||
ELSE -1
|
||||
END)
|
||||
OR ( {incomingRank} = (CASE Status
|
||||
WHEN 'Submitted' THEN 0
|
||||
WHEN 'Forwarded' THEN 1
|
||||
WHEN 'Attempted' THEN 2
|
||||
WHEN 'Skipped' THEN 2
|
||||
WHEN 'Delivered' THEN 3
|
||||
WHEN 'Failed' THEN 3
|
||||
WHEN 'Parked' THEN 3
|
||||
WHEN 'Discarded' THEN 3
|
||||
ELSE -1
|
||||
END)
|
||||
AND {incomingRank} < {TerminalRank}
|
||||
AND UpdatedAtUtc < {siteCall.UpdatedAtUtc} ) );
|
||||
|
||||
-- Captured IMMEDIATELY after the UPDATE: @@ROWCOUNT is reset by the next
|
||||
-- statement, and reading it inline inside a compound IF condition alongside a
|
||||
-- subquery is not safe (the subquery's own execution can clobber it).
|
||||
SET @updated = @@ROWCOUNT;
|
||||
|
||||
IF @updated = 0 AND NOT EXISTS (SELECT 1 FROM dbo.SiteCalls WHERE TrackedOperationId = {idText})
|
||||
INSERT INTO dbo.SiteCalls
|
||||
(TrackedOperationId, Channel, Target, SourceSite, SourceNode, Status, RetryCount,
|
||||
LastError, HttpStatus, CreatedAtUtc, UpdatedAtUtc, TerminalAtUtc, IngestedAtUtc)
|
||||
VALUES
|
||||
({idText}, {siteCall.Channel}, {siteCall.Target}, {siteCall.SourceSite}, {siteCall.SourceNode}, {siteCall.Status}, {siteCall.RetryCount},
|
||||
{siteCall.LastError}, {siteCall.HttpStatus}, {siteCall.CreatedAtUtc}, {siteCall.UpdatedAtUtc}, {siteCall.TerminalAtUtc}, {siteCall.IngestedAtUtc});",
|
||||
ct);
|
||||
await _context.Database.ExecuteSqlRawAsync(
|
||||
InsertIfAbsentSql + "\n\n" + MonotonicUpdateSql,
|
||||
BuildUpsertParameters(siteCall, idText, incomingRank),
|
||||
ct)
|
||||
.ConfigureAwait(false);
|
||||
}
|
||||
catch (SqlException ex) when (
|
||||
ex.Number == SqlErrorUniqueIndexViolation
|
||||
@@ -175,20 +148,112 @@ VALUES
|
||||
{
|
||||
// Two concurrent sessions both found the row absent and both raced to
|
||||
// INSERT; the loser raises 2601/2627 against the TrackedOperationId
|
||||
// primary key. The winner's row IS the first-write, and this packet's
|
||||
// content is by construction the same lifecycle state, so the race
|
||||
// outcome is semantically a no-op. Swallow at Debug — the same
|
||||
// check-then-act contract the sibling AuditLog/Notification repos
|
||||
// document. Note the loser's UPDATE leg already ran (against no row),
|
||||
// so nothing is left half-applied.
|
||||
// primary key. The winner's row IS the first-write, but it is NOT
|
||||
// necessarily this packet's lifecycle state — the reconciliation pull
|
||||
// and the cached dual-write feed this method with different states —
|
||||
// so the loser MUST still apply its monotonic UPDATE against the
|
||||
// winner's row. The batch aborted at the faulting INSERT, so re-run
|
||||
// the UPDATE alone here; it is idempotent and rank-guarded, so
|
||||
// re-running it is safe even if the batch did reach it.
|
||||
_logger.LogDebug(
|
||||
ex,
|
||||
"SiteCallAuditRepository.UpsertAsync swallowed duplicate-key violation (error {SqlErrorNumber}) for TrackedOperationId {TrackedOperationId}; treating as no-op.",
|
||||
"SiteCallAuditRepository.UpsertAsync swallowed duplicate-key violation (error {SqlErrorNumber}) for TrackedOperationId {TrackedOperationId}; re-running the monotonic update against the winning row.",
|
||||
ex.Number,
|
||||
idText);
|
||||
|
||||
await _context.Database.ExecuteSqlRawAsync(
|
||||
MonotonicUpdateSql,
|
||||
BuildUpsertParameters(siteCall, idText, incomingRank),
|
||||
ct)
|
||||
.ConfigureAwait(false);
|
||||
}
|
||||
}
|
||||
|
||||
// Leg 1 of the upsert: create the row when it does not exist yet. Runs FIRST
|
||||
// so a concurrent first-write loser still has a row to update (see UpsertAsync).
|
||||
private const string InsertIfAbsentSql = @"
|
||||
IF NOT EXISTS (SELECT 1 FROM dbo.SiteCalls WHERE TrackedOperationId = @Id)
|
||||
INSERT INTO dbo.SiteCalls
|
||||
(TrackedOperationId, Channel, Target, SourceSite, SourceNode, Status, RetryCount,
|
||||
LastError, HttpStatus, CreatedAtUtc, UpdatedAtUtc, TerminalAtUtc, IngestedAtUtc)
|
||||
VALUES
|
||||
(@Id, @Channel, @Target, @SourceSite, @SourceNode, @Status, @RetryCount,
|
||||
@LastError, @HttpStatus, @CreatedAtUtc, @UpdatedAtUtc, @TerminalAtUtc, @IngestedAtUtc);";
|
||||
|
||||
// Leg 2 of the upsert: the monotonic guard. Also re-run standalone from the
|
||||
// duplicate-key catch, which is why it lives in its own constant.
|
||||
private const string MonotonicUpdateSql = @"
|
||||
UPDATE dbo.SiteCalls
|
||||
SET Status = @Status,
|
||||
RetryCount = @RetryCount,
|
||||
LastError = @LastError,
|
||||
HttpStatus = @HttpStatus,
|
||||
UpdatedAtUtc = @UpdatedAtUtc,
|
||||
TerminalAtUtc = @TerminalAtUtc,
|
||||
IngestedAtUtc = @IngestedAtUtc,
|
||||
SourceNode = COALESCE(@SourceNode, SourceNode)
|
||||
WHERE TrackedOperationId = @Id
|
||||
AND ( @Rank > (CASE Status
|
||||
WHEN 'Submitted' THEN 0
|
||||
WHEN 'Forwarded' THEN 1
|
||||
WHEN 'Attempted' THEN 2
|
||||
WHEN 'Skipped' THEN 2
|
||||
WHEN 'Delivered' THEN 3
|
||||
WHEN 'Failed' THEN 3
|
||||
WHEN 'Parked' THEN 3
|
||||
WHEN 'Discarded' THEN 3
|
||||
ELSE -1
|
||||
END)
|
||||
OR ( @Rank = (CASE Status
|
||||
WHEN 'Submitted' THEN 0
|
||||
WHEN 'Forwarded' THEN 1
|
||||
WHEN 'Attempted' THEN 2
|
||||
WHEN 'Skipped' THEN 2
|
||||
WHEN 'Delivered' THEN 3
|
||||
WHEN 'Failed' THEN 3
|
||||
WHEN 'Parked' THEN 3
|
||||
WHEN 'Discarded' THEN 3
|
||||
ELSE -1
|
||||
END)
|
||||
AND @Rank < @TerminalRank
|
||||
AND UpdatedAtUtc < @UpdatedAtUtc ) );";
|
||||
|
||||
/// <summary>
|
||||
/// Builds one FRESH parameter set for the upsert statements. Fresh per call
|
||||
/// because a <see cref="SqlParameter"/> instance cannot be attached to two
|
||||
/// commands, and the duplicate-key retry issues a second command.
|
||||
/// </summary>
|
||||
private static object[] BuildUpsertParameters(SiteCall siteCall, string idText, int incomingRank)
|
||||
{
|
||||
return
|
||||
[
|
||||
Param("@Id", SqlDbType.VarChar, idText),
|
||||
Param("@Channel", SqlDbType.VarChar, siteCall.Channel),
|
||||
Param("@Target", SqlDbType.VarChar, siteCall.Target),
|
||||
Param("@SourceSite", SqlDbType.VarChar, siteCall.SourceSite),
|
||||
Param("@SourceNode", SqlDbType.VarChar, siteCall.SourceNode),
|
||||
Param("@Status", SqlDbType.VarChar, siteCall.Status),
|
||||
Param("@RetryCount", SqlDbType.Int, siteCall.RetryCount),
|
||||
Param("@LastError", SqlDbType.NVarChar, siteCall.LastError),
|
||||
Param("@HttpStatus", SqlDbType.Int, siteCall.HttpStatus),
|
||||
Param("@CreatedAtUtc", SqlDbType.DateTime2, siteCall.CreatedAtUtc),
|
||||
Param("@UpdatedAtUtc", SqlDbType.DateTime2, siteCall.UpdatedAtUtc),
|
||||
Param("@TerminalAtUtc", SqlDbType.DateTime2, siteCall.TerminalAtUtc),
|
||||
Param("@IngestedAtUtc", SqlDbType.DateTime2, siteCall.IngestedAtUtc),
|
||||
Param("@Rank", SqlDbType.Int, incomingRank),
|
||||
Param("@TerminalRank", SqlDbType.Int, TerminalRank),
|
||||
];
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Binds one explicitly-typed parameter, mapping a null CLR value to
|
||||
/// <see cref="DBNull"/> while KEEPING the declared type. Size is never set —
|
||||
/// SqlClient sizes from the value, so an over-long string is rejected by the
|
||||
/// server rather than truncated client-side.
|
||||
/// </summary>
|
||||
private static SqlParameter Param(string name, SqlDbType type, object? value) =>
|
||||
new(name, type) { Value = value ?? DBNull.Value };
|
||||
|
||||
/// <inheritdoc />
|
||||
public async Task<SiteCall?> GetAsync(TrackedOperationId id, CancellationToken ct = default)
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user