From 1dd89d523b963610d96a6bfb3551fc86d42774db Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 9 Jul 2026 08:03:30 -0400 Subject: [PATCH] fix(config-db): EnableRetryOnFailure fleet-wide + execution-strategy wrap of the combined-telemetry dual-write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Installs SqlServerRetryingExecutionStrategy (5 retries, 30s cap) so transient SQL faults retry transparently on every read-side path. Manual transactions aren't auto-retried, so AuditLogIngestActor's idempotent dual-write is wrapped in an explicit CreateExecutionStrategy().ExecuteAsync to become retriable. Verified empirically that existing manual transactions (BundleImporter) are unaffected — EF skips the strategy while a transaction is already active. --- .../Central/AuditLogIngestActor.cs | 63 ++++++++++++------- .../Repositories/AuditLogRepository.cs | 7 +++ .../ServiceCollectionExtensions.cs | 17 ++++- .../ServiceCollectionExtensionsTests.cs | 29 ++++++++- 4 files changed, 90 insertions(+), 26 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogIngestActor.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogIngestActor.cs index 725c0e81..3e9d2af6 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogIngestActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogIngestActor.cs @@ -1,4 +1,5 @@ using Akka.Actor; +using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using ZB.MOM.WW.Audit; @@ -259,37 +260,51 @@ public class AuditLogIngestActor : ReceiveActor // skip the registration. var failureCounter = scope.ServiceProvider.GetService(); + // Task 12 (arch-review 04 S5): retry the dual-write under the DbContext's + // configured resiliency strategy. A manual (user-initiated) transaction is + // NOT auto-retried by EnableRetryOnFailure — EF executes directly while a + // transaction is active — so the whole { BEGIN; insert; upsert; COMMIT } + // unit is wrapped here to become a single retriable operation. The unit is + // idempotent (InsertIfNotExists on EventId + monotonic upsert on + // TrackedOperationId), so replay after a rolled-back transient failure is + // safe. NoOpExecutionStrategy on non-configured/test contexts runs it once. + var strategy = dbContext.Database.CreateExecutionStrategy(); + foreach (var entry in cmd.Entries) { try { - await using var tx = await dbContext.Database - .BeginTransactionAsync() - .ConfigureAwait(false); + await strategy.ExecuteAsync(async () => + { + await using var tx = await dbContext.Database + .BeginTransactionAsync() + .ConfigureAwait(false); - // Stamp IngestedAtUtc on both rows from a single - // central-side instant so a join on the two tables sees - // matching timestamps (debugging convenience, not a - // correctness invariant). - var ingestedAt = DateTime.UtcNow; - // Redact the audit half BEFORE the dual-write — only the - // AuditLog row's payload columns are redactable; SiteCalls - // carries operational state only (status, retry count) and - // is left untouched. Null redactor falls back - // to SafeDefault so header redaction always runs. - // IngestedAtUtc is a DetailsJson field - // on the canonical record, so stamp it via the projection helper. - var safeRedactor = redactor ?? SafeDefaultAuditRedactor.Instance; - var filteredAudit = safeRedactor.Apply(entry.Audit); - var auditStamped = AuditRowProjection.WithIngestedAtUtc(filteredAudit, ingestedAt); - var siteCallStamped = entry.SiteCall with { IngestedAtUtc = ingestedAt }; + // Stamp IngestedAtUtc on both rows from a single + // central-side instant so a join on the two tables sees + // matching timestamps (debugging convenience, not a + // correctness invariant). + var ingestedAt = DateTime.UtcNow; + // Redact the audit half BEFORE the dual-write — only the + // AuditLog row's payload columns are redactable; SiteCalls + // carries operational state only (status, retry count) and + // is left untouched. Null redactor falls back + // to SafeDefault so header redaction always runs. + // IngestedAtUtc is a DetailsJson field + // on the canonical record, so stamp it via the projection helper. + var safeRedactor = redactor ?? SafeDefaultAuditRedactor.Instance; + var filteredAudit = safeRedactor.Apply(entry.Audit); + var auditStamped = AuditRowProjection.WithIngestedAtUtc(filteredAudit, ingestedAt); + var siteCallStamped = entry.SiteCall with { IngestedAtUtc = ingestedAt }; - await auditRepo.InsertIfNotExistsAsync(auditStamped) - .ConfigureAwait(false); - await siteCallRepo.UpsertAsync(siteCallStamped) - .ConfigureAwait(false); + await auditRepo.InsertIfNotExistsAsync(auditStamped) + .ConfigureAwait(false); + await siteCallRepo.UpsertAsync(siteCallStamped) + .ConfigureAwait(false); + + await tx.CommitAsync().ConfigureAwait(false); + }).ConfigureAwait(false); - await tx.CommitAsync().ConfigureAwait(false); accepted.Add(entry.Audit.EventId); } catch (Exception ex) diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/AuditLogRepository.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/AuditLogRepository.cs index 9857b9d1..bcb784a7 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/AuditLogRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/AuditLogRepository.cs @@ -229,6 +229,13 @@ VALUES /// public async Task SwitchOutPartitionAsync(DateTime monthBoundary, TimeSpan? commandTimeout = null, CancellationToken ct = default) { + // Task 12 (arch-review 04 S5) note: the drop-and-rebuild batch below runs via + // ExecuteSqlRaw with NO EF user-transaction — it carries its own server-side + // BEGIN TRANSACTION / TRY-CATCH / ROLLBACK — so the DbContext's retrying + // execution strategy (EnableRetryOnFailure) MAY auto-replay the whole batch on + // a transient fault. That replay is safe: every step is IF-EXISTS / IF-NOT-EXISTS + // guarded and the staging table is GUID-suffixed, so a re-run is idempotent. + // // Maintenance timeout in whole seconds (ADO.NET CommandTimeout unit). Null leaves the // provider default in place. See AuditLogPurgeOptions.MaintenanceCommandTimeoutMinutes / // arch-review 04 S2 for why the ~30s default is unsafe for the switch-out dance. diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/ServiceCollectionExtensions.cs index d4dad317..47c2941b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/ServiceCollectionExtensions.cs @@ -28,7 +28,22 @@ public static class ServiceCollectionExtensions // CreateProtector during model building. services.AddDbContext((serviceProvider, options) => { - options.UseSqlServer(connectionString) + options.UseSqlServer( + connectionString, + // Task 12 (arch-review 04 S5): connection resiliency. A transient + // SQL fault (failover, throttling, dropped connection) previously + // surfaced raw to every read-side caller — KPI asks, outbox + // queries, Tracking.Status, execution-tree walks. EnableRetryOnFailure + // installs the SqlServerRetryingExecutionStrategy so those retry + // transparently. Note: manual (user-initiated) transactions are NOT + // auto-retried — EF executes them directly while a transaction is + // active — so the combined-telemetry dual-write is wrapped in an + // explicit CreateExecutionStrategy() to become retriable (see + // AuditLogIngestActor.OnCachedTelemetryAsync). + sql => sql.EnableRetryOnFailure( + maxRetryCount: 5, + maxRetryDelay: TimeSpan.FromSeconds(30), + errorNumbersToAdd: null)) .ConfigureWarnings(w => w.Ignore( Microsoft.EntityFrameworkCore.Diagnostics.RelationalEventId.PendingModelChangesWarning)); }); diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/ServiceCollectionExtensionsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/ServiceCollectionExtensionsTests.cs index 647fc147..88bc9826 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/ServiceCollectionExtensionsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/ServiceCollectionExtensionsTests.cs @@ -1,4 +1,5 @@ using System.Reflection; +using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; @@ -13,7 +14,7 @@ public class ServiceCollectionExtensionsTests { var services = new ServiceCollection(); - services.AddConfigurationDatabase("DataSource=:memory:"); + services.AddConfigurationDatabase("Server=unused;Database=unused;"); Assert.Contains(services, d => d.ServiceType == typeof(ITemplateEngineRepository)); Assert.Contains(services, d => d.ServiceType == typeof(IAuditService)); @@ -21,6 +22,32 @@ public class ServiceCollectionExtensionsTests Assert.Contains(services, d => d.ServiceType == typeof(IAuditLogRepository)); } + [Fact] + public void AddConfigurationDatabase_Configures_RetryingExecutionStrategy() + { + // Task 12 (arch-review 04 S5): the central DbContext must survive transient + // SQL faults. EnableRetryOnFailure installs a non-null ExecutionStrategyFactory + // on the SqlServer options extension — covering every read-side path (KPI asks, + // outbox queries, Tracking.Status, execution trees) that previously surfaced + // transient faults raw. + var services = new ServiceCollection(); + services.AddConfigurationDatabase("Server=unused;Database=unused;"); + using var sp = services.BuildServiceProvider(); + + var options = sp.GetRequiredService>(); + + // EF1001: SqlServerOptionsExtension is an internal EF surface, but it is the + // only place the retrying-strategy factory is observable — asserting it is the + // point of this test. Scoped suppression, not a project-wide one. +#pragma warning disable EF1001 + var sqlExt = options.Extensions + .OfType() + .Single(); + + Assert.NotNull(sqlExt.ExecutionStrategyFactory); +#pragma warning restore EF1001 + } + // The no-arg overload is [Obsolete(error: true)], so it cannot be referenced directly // from source — that is the compile-time guard. Invoke it via reflection to verify the // runtime defence-in-depth behaviour.