fix(configdb): InsertIfNotExistsAsync swallows duplicate-key races + add keyset tiebreaker test (#23)
Two concurrent sessions can both pass the IF NOT EXISTS check and then both attempt the INSERT against UX_AuditLog_EventId; the loser surfaced as SqlException 2601 (or 2627 for PK violations) and aborted the audit write. First-write-wins idempotency is the documented contract, so the race outcome is semantically a no-op — catch the two duplicate-key error numbers and log at Debug, let any other SqlException bubble. Tests: - InsertIfNotExistsAsync_ConcurrentDuplicateInserts_ProduceExactlyOneRow: 50 parallel inserters with the same EventId end with exactly one row and no escaped exceptions. - QueryAsync_Keyset_SameOccurredAtUtc_TiebreaksOnEventId: four rows sharing the same OccurredAtUtc page deterministically through the (OccurredAtUtc, EventId) keyset cursor — exercises the e.OccurredAtUtc == after && e.EventId.CompareTo(afterId) < 0 branch and verifies EF Core 10's Guid.CompareTo translation against SQL Server uniqueidentifier order (deferred Bundle D reviewer recommendation). AuditLogRepository now takes an optional ILogger<AuditLogRepository> (NullLogger default, mirrors InboundApiRepository); DI registration unchanged.
This commit is contained in:
@@ -1,4 +1,7 @@
|
||||
using Microsoft.Data.SqlClient;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using ScadaLink.Commons.Entities.Audit;
|
||||
using ScadaLink.Commons.Interfaces.Repositories;
|
||||
using ScadaLink.Commons.Types.Audit;
|
||||
@@ -12,11 +15,22 @@ namespace ScadaLink.ConfigurationDatabase.Repositories;
|
||||
/// </summary>
|
||||
public class AuditLogRepository : IAuditLogRepository
|
||||
{
|
||||
private readonly ScadaLinkDbContext _context;
|
||||
// SQL Server error numbers for duplicate-key violations on
|
||||
// UX_AuditLog_EventId. 2601 is a unique-index violation; 2627 is a
|
||||
// primary-key/unique-constraint violation. The IF NOT EXISTS … INSERT
|
||||
// pattern has a check-then-act race window — two sessions can both pass
|
||||
// the EXISTS check and then both attempt the INSERT — and the loser
|
||||
// surfaces as one of these errors. Idempotency demands we swallow them.
|
||||
private const int SqlErrorUniqueIndexViolation = 2601;
|
||||
private const int SqlErrorPrimaryKeyViolation = 2627;
|
||||
|
||||
public AuditLogRepository(ScadaLinkDbContext context)
|
||||
private readonly ScadaLinkDbContext _context;
|
||||
private readonly ILogger<AuditLogRepository> _logger;
|
||||
|
||||
public AuditLogRepository(ScadaLinkDbContext context, ILogger<AuditLogRepository>? logger = null)
|
||||
{
|
||||
_context = context ?? throw new ArgumentNullException(nameof(context));
|
||||
_logger = logger ?? NullLogger<AuditLogRepository>.Instance;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -44,8 +58,10 @@ public class AuditLogRepository : IAuditLogRepository
|
||||
|
||||
// FormattableString interpolation parameterises every value (no concatenation),
|
||||
// so this is safe against injection even for the string columns.
|
||||
await _context.Database.ExecuteSqlInterpolatedAsync(
|
||||
$@"IF NOT EXISTS (SELECT 1 FROM dbo.AuditLog WHERE EventId = {evt.EventId})
|
||||
try
|
||||
{
|
||||
await _context.Database.ExecuteSqlInterpolatedAsync(
|
||||
$@"IF NOT EXISTS (SELECT 1 FROM dbo.AuditLog WHERE EventId = {evt.EventId})
|
||||
INSERT INTO dbo.AuditLog
|
||||
(EventId, OccurredAtUtc, IngestedAtUtc, Channel, Kind, CorrelationId,
|
||||
SourceSiteId, SourceInstanceId, SourceScript, Actor, Target, Status,
|
||||
@@ -56,7 +72,24 @@ VALUES
|
||||
{evt.SourceSiteId}, {evt.SourceInstanceId}, {evt.SourceScript}, {evt.Actor}, {evt.Target}, {status},
|
||||
{evt.HttpStatus}, {evt.DurationMs}, {evt.ErrorMessage}, {evt.ErrorDetail}, {evt.RequestSummary},
|
||||
{evt.ResponseSummary}, {evt.PayloadTruncated}, {evt.Extra}, {forwardState});",
|
||||
ct);
|
||||
ct);
|
||||
}
|
||||
catch (SqlException ex) when (
|
||||
ex.Number == SqlErrorUniqueIndexViolation
|
||||
|| ex.Number == SqlErrorPrimaryKeyViolation)
|
||||
{
|
||||
// Two concurrent sessions both passed the IF NOT EXISTS check and
|
||||
// both attempted the INSERT — the loser raises 2601/2627 against
|
||||
// UX_AuditLog_EventId. First-write-wins idempotency is already the
|
||||
// documented contract for this method, so the race outcome is
|
||||
// semantically a no-op. Swallow at Debug; other SqlExceptions
|
||||
// bubble.
|
||||
_logger.LogDebug(
|
||||
ex,
|
||||
"InsertIfNotExistsAsync swallowed duplicate-key violation (error {SqlErrorNumber}) for EventId {EventId}; treating as no-op.",
|
||||
ex.Number,
|
||||
evt.EventId);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
||||
Reference in New Issue
Block a user