fix(transport): robust failure-audit when rollback throws + doc clarifications
Address one Blocker and three Important findings from code review of
2c34f12 (BundleImporter.ApplyAsync):
- BLOCKER: wrap RollbackAsync in nested try/catch so a rollback fault
does not swallow the BundleImportFailed audit row. Dispose the
failed transaction before the audit-write so the new SaveChangesAsync
uses a fresh implicit transaction instead of enlisting in the broken
one. Surface the rollback exception's message on the failure row
alongside the original cause, and swallow audit-write faults per the
design's best-effort-audit invariant. Add regression integration
test using a SQLite transaction interceptor that throws on rollback.
- Document re-entrancy assumption on IAuditCorrelationContext: scoped
lifetime, single circuit, concurrent imports within a shared scope
must serialize externally.
- Document repository audit responsibility on BundleImporter: repos
are thin EF wrappers; ApplyAsync writes audit rows explicitly. If
repos ever start emitting audit rows, the explicit calls here must
be removed to avoid double-logging.
- Document BundleSessionStore thread-safety: ConcurrentDictionary
primitives are safe under concurrent callers; BundleSession itself
is not thread-safe.
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,304 @@
|
||||
using System.Data.Common;
|
||||
using Microsoft.AspNetCore.DataProtection;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.EntityFrameworkCore.Diagnostics;
|
||||
using Microsoft.EntityFrameworkCore.Storage;
|
||||
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
|
||||
using Microsoft.Extensions.Configuration;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using ScadaLink.Commons.Entities.Deployment;
|
||||
using ScadaLink.Commons.Entities.Scripts;
|
||||
using ScadaLink.Commons.Entities.Templates;
|
||||
using ScadaLink.Commons.Interfaces.Repositories;
|
||||
using ScadaLink.Commons.Interfaces.Services;
|
||||
using ScadaLink.Commons.Interfaces.Transport;
|
||||
using ScadaLink.Commons.Types.Transport;
|
||||
using ScadaLink.ConfigurationDatabase;
|
||||
using ScadaLink.ConfigurationDatabase.Repositories;
|
||||
using ScadaLink.ConfigurationDatabase.Services;
|
||||
using ScadaLink.Transport;
|
||||
using ScadaLink.Transport.Import;
|
||||
|
||||
namespace ScadaLink.Transport.IntegrationTests.Import;
|
||||
|
||||
/// <summary>
|
||||
/// Covers the catch-path invariant in <see cref="BundleImporter.ApplyAsync"/>:
|
||||
/// even when the EF <c>RollbackAsync</c> itself throws (connection drop mid-
|
||||
/// rollback, provider bug, etc.) the <c>BundleImportFailed</c> audit row MUST
|
||||
/// still land, and the ORIGINAL exception (not the rollback failure) MUST
|
||||
/// propagate to the caller.
|
||||
/// <para>
|
||||
/// Uses SQLite rather than the in-memory provider because the in-memory
|
||||
/// provider's transaction is a no-op — its <c>RollbackAsync</c> never invokes
|
||||
/// the interceptor, so the throw-on-rollback path can't be exercised. SQLite
|
||||
/// :memory: is keyed per-connection, so the fixture pins a single open
|
||||
/// connection across the whole test.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The interceptor is wired to throw on <see cref="IDbTransactionInterceptor.TransactionRollingBack"/>
|
||||
/// and the async equivalent — this is the hook EF invokes synchronously inside
|
||||
/// <c>IDbContextTransaction.RollbackAsync</c>, so a throw there surfaces as
|
||||
/// the <c>RollbackAsync</c> call itself throwing, which is exactly the
|
||||
/// scenario the catch block must survive.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public sealed class BundleImporterRollbackFailureTests : IDisposable
|
||||
{
|
||||
private readonly ServiceProvider _provider;
|
||||
private readonly DbConnection _sharedConnection;
|
||||
private readonly ThrowingRollbackInterceptor _interceptor = new();
|
||||
|
||||
public BundleImporterRollbackFailureTests()
|
||||
{
|
||||
var services = new ServiceCollection();
|
||||
services.AddSingleton<IConfiguration>(
|
||||
new ConfigurationBuilder().AddInMemoryCollection().Build());
|
||||
|
||||
// Pin a single SQLite :memory: connection for the lifetime of the
|
||||
// fixture — :memory: is per-connection so the schema would otherwise
|
||||
// vanish between DbContext instances.
|
||||
_sharedConnection = new Microsoft.Data.Sqlite.SqliteConnection("DataSource=:memory:");
|
||||
_sharedConnection.Open();
|
||||
|
||||
services.AddSingleton<IDataProtectionProvider>(new EphemeralDataProtectionProvider());
|
||||
|
||||
// Register options once under the BASE DbContextOptions key, then
|
||||
// register the subclass as the scoped service used by repositories +
|
||||
// AuditService + BundleImporter. The subclass's ctor accepts
|
||||
// DbContextOptions<ScadaLinkDbContext> (the base type's options) so the
|
||||
// single options registration serves both. This avoids the multi-options
|
||||
// pitfall of AddDbContext<TBase, TImpl> which keys options on TImpl.
|
||||
services.AddSingleton(sp =>
|
||||
{
|
||||
var builder = new DbContextOptionsBuilder<ScadaLinkDbContext>();
|
||||
builder.UseSqlite(_sharedConnection);
|
||||
builder.ConfigureWarnings(w => w.Ignore(RelationalEventId.PendingModelChangesWarning));
|
||||
builder.AddInterceptors(_interceptor);
|
||||
return builder.Options;
|
||||
});
|
||||
services.AddScoped<ScadaLinkDbContext>(sp => new SqliteCompatibleScadaLinkDbContext(
|
||||
sp.GetRequiredService<DbContextOptions<ScadaLinkDbContext>>(),
|
||||
sp.GetRequiredService<IDataProtectionProvider>()));
|
||||
|
||||
services.AddScoped<ITemplateEngineRepository, TemplateEngineRepository>();
|
||||
services.AddScoped<IExternalSystemRepository, ExternalSystemRepository>();
|
||||
services.AddScoped<INotificationRepository, NotificationRepository>();
|
||||
services.AddScoped<IInboundApiRepository, InboundApiRepository>();
|
||||
services.AddScoped<IAuditCorrelationContext, AuditCorrelationContext>();
|
||||
services.AddScoped<IAuditService, AuditService>();
|
||||
services.AddTransport();
|
||||
|
||||
_provider = services.BuildServiceProvider();
|
||||
|
||||
// Build schema once on the shared connection.
|
||||
using var scope = _provider.CreateScope();
|
||||
var ctx = scope.ServiceProvider.GetRequiredService<ScadaLinkDbContext>();
|
||||
ctx.Database.EnsureCreated();
|
||||
}
|
||||
|
||||
public void Dispose()
|
||||
{
|
||||
_provider.Dispose();
|
||||
_sharedConnection.Dispose();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task ApplyAsync_writes_BundleImportFailed_even_when_RollbackAsync_throws()
|
||||
{
|
||||
// Arrange: seed a template whose script body references MissingHelper()
|
||||
// so semantic validation will reject the apply (same broken-bundle shape
|
||||
// as BundleImporterApplyTests.ApplyAsync_rolls_back_all_changes_…). Then
|
||||
// arm the interceptor to throw on rollback so the catch path has to
|
||||
// survive a rollback failure.
|
||||
await using (var scope = _provider.CreateAsyncScope())
|
||||
{
|
||||
var ctx = scope.ServiceProvider.GetRequiredService<ScadaLinkDbContext>();
|
||||
var t = new Template("BrokenPump") { Description = "broken" };
|
||||
t.Scripts.Add(new TemplateScript("init", "var x = MissingHelper();"));
|
||||
ctx.Templates.Add(t);
|
||||
await ctx.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var sessionId = await ExportAndLoadAsync();
|
||||
await WipeContentAsync();
|
||||
|
||||
_interceptor.ThrowOnRollback = true;
|
||||
|
||||
// Act: ApplyAsync must propagate the ORIGINAL exception
|
||||
// (SemanticValidationException) — NOT the InvalidOperationException
|
||||
// that the interceptor raises from inside RollbackAsync.
|
||||
SemanticValidationException? thrown = null;
|
||||
await using (var scope = _provider.CreateAsyncScope())
|
||||
{
|
||||
var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
|
||||
thrown = await Assert.ThrowsAsync<SemanticValidationException>(() =>
|
||||
importer.ApplyAsync(sessionId,
|
||||
new List<ImportResolution> { new("Template", "BrokenPump", ResolutionAction.Add, null) },
|
||||
user: "bob"));
|
||||
}
|
||||
Assert.NotNull(thrown);
|
||||
|
||||
// Assert: even with a rollback failure, the BundleImportFailed audit row
|
||||
// must have landed — that's the whole point of the fix. The row should
|
||||
// also carry the rollback failure's message in its AfterStateJson so
|
||||
// post-mortem readers can see both faults.
|
||||
_interceptor.ThrowOnRollback = false; // let post-condition reads roll back cleanly
|
||||
await using (var scope = _provider.CreateAsyncScope())
|
||||
{
|
||||
var ctx = scope.ServiceProvider.GetRequiredService<ScadaLinkDbContext>();
|
||||
var failed = await ctx.AuditLogEntries
|
||||
.SingleOrDefaultAsync(a => a.Action == "BundleImportFailed");
|
||||
Assert.NotNull(failed);
|
||||
Assert.Equal("Bundle", failed!.EntityType);
|
||||
// Correlation MUST be null on the failure row — the rolled-back
|
||||
// BundleImportId is intentionally disowned (same contract as the
|
||||
// happy-path rollback test in BundleImporterApplyTests).
|
||||
Assert.Null(failed.BundleImportId);
|
||||
Assert.NotNull(failed.AfterStateJson);
|
||||
// The rollback exception message must be surfaced in the failure
|
||||
// row so operators can see both the cause and the rollback fault.
|
||||
Assert.Contains(
|
||||
ThrowingRollbackInterceptor.RollbackErrorMarker,
|
||||
failed.AfterStateJson!,
|
||||
StringComparison.Ordinal);
|
||||
}
|
||||
}
|
||||
|
||||
// ---- helpers (copies of the patterns from BundleImporterApplyTests) ----
|
||||
|
||||
private async Task<Guid> ExportAndLoadAsync()
|
||||
{
|
||||
Stream bundleStream;
|
||||
await using (var scope = _provider.CreateAsyncScope())
|
||||
{
|
||||
var exporter = scope.ServiceProvider.GetRequiredService<IBundleExporter>();
|
||||
var ctx = scope.ServiceProvider.GetRequiredService<ScadaLinkDbContext>();
|
||||
var templateIds = await ctx.Templates.Select(t => t.Id).ToListAsync();
|
||||
var selection = new ExportSelection(
|
||||
TemplateIds: templateIds,
|
||||
SharedScriptIds: Array.Empty<int>(),
|
||||
ExternalSystemIds: Array.Empty<int>(),
|
||||
DatabaseConnectionIds: Array.Empty<int>(),
|
||||
NotificationListIds: Array.Empty<int>(),
|
||||
SmtpConfigurationIds: Array.Empty<int>(),
|
||||
ApiKeyIds: Array.Empty<int>(),
|
||||
ApiMethodIds: Array.Empty<int>(),
|
||||
IncludeDependencies: false);
|
||||
bundleStream = await exporter.ExportAsync(selection, user: "alice", sourceEnvironment: "dev",
|
||||
passphrase: null, cancellationToken: CancellationToken.None);
|
||||
}
|
||||
|
||||
using var ms = new MemoryStream();
|
||||
await bundleStream.CopyToAsync(ms);
|
||||
ms.Position = 0;
|
||||
|
||||
await using var loadScope = _provider.CreateAsyncScope();
|
||||
var importer = loadScope.ServiceProvider.GetRequiredService<IBundleImporter>();
|
||||
var session = await importer.LoadAsync(ms, passphrase: null);
|
||||
return session.SessionId;
|
||||
}
|
||||
|
||||
private async Task WipeContentAsync()
|
||||
{
|
||||
await using var scope = _provider.CreateAsyncScope();
|
||||
var ctx = scope.ServiceProvider.GetRequiredService<ScadaLinkDbContext>();
|
||||
ctx.Templates.RemoveRange(ctx.Templates);
|
||||
ctx.SharedScripts.RemoveRange(ctx.SharedScripts);
|
||||
ctx.TemplateFolders.RemoveRange(ctx.TemplateFolders);
|
||||
await ctx.SaveChangesAsync();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// EF transaction interceptor that throws on rollback when armed. Used by
|
||||
/// <see cref="ApplyAsync_writes_BundleImportFailed_even_when_RollbackAsync_throws"/>
|
||||
/// to simulate the connection-dropped-during-rollback scenario. EF calls
|
||||
/// the async hook from inside <c>IDbContextTransaction.RollbackAsync</c>,
|
||||
/// so a throw here surfaces as <c>RollbackAsync</c> itself throwing —
|
||||
/// exactly the contract the catch block must survive.
|
||||
/// </summary>
|
||||
private sealed class ThrowingRollbackInterceptor : DbTransactionInterceptor
|
||||
{
|
||||
public const string RollbackErrorMarker = "simulated rollback failure";
|
||||
|
||||
public bool ThrowOnRollback { get; set; }
|
||||
|
||||
public override ValueTask<InterceptionResult> TransactionRollingBackAsync(
|
||||
DbTransaction transaction,
|
||||
TransactionEventData eventData,
|
||||
InterceptionResult result,
|
||||
CancellationToken cancellationToken = default)
|
||||
{
|
||||
if (ThrowOnRollback)
|
||||
{
|
||||
throw new InvalidOperationException(RollbackErrorMarker);
|
||||
}
|
||||
return base.TransactionRollingBackAsync(transaction, eventData, result, cancellationToken);
|
||||
}
|
||||
|
||||
public override InterceptionResult TransactionRollingBack(
|
||||
DbTransaction transaction,
|
||||
TransactionEventData eventData,
|
||||
InterceptionResult result)
|
||||
{
|
||||
if (ThrowOnRollback)
|
||||
{
|
||||
throw new InvalidOperationException(RollbackErrorMarker);
|
||||
}
|
||||
return base.TransactionRollingBack(transaction, eventData, result);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// SQLite-compatible variant of <see cref="ScadaLinkDbContext"/> used by
|
||||
/// <see cref="BundleImporterRollbackFailureTests"/>. Mirrors the adaptations in
|
||||
/// <c>SqliteTestDbContext</c> over in ScadaLink.ConfigurationDatabase.Tests
|
||||
/// (rowversion is nullable, DateTimeOffset stored as ISO 8601 text) but is
|
||||
/// duplicated here to avoid taking a project reference to that test project.
|
||||
/// </summary>
|
||||
internal sealed class SqliteCompatibleScadaLinkDbContext : ScadaLinkDbContext
|
||||
{
|
||||
public SqliteCompatibleScadaLinkDbContext(
|
||||
DbContextOptions<ScadaLinkDbContext> options,
|
||||
IDataProtectionProvider dataProtectionProvider)
|
||||
: base(options, dataProtectionProvider)
|
||||
{
|
||||
}
|
||||
|
||||
protected override void OnModelCreating(ModelBuilder modelBuilder)
|
||||
{
|
||||
base.OnModelCreating(modelBuilder);
|
||||
|
||||
modelBuilder.Entity<DeploymentRecord>(builder =>
|
||||
{
|
||||
builder.Property(d => d.RowVersion)
|
||||
.IsRequired(false)
|
||||
.IsConcurrencyToken(false)
|
||||
.ValueGeneratedNever();
|
||||
});
|
||||
|
||||
var converter = new ValueConverter<DateTimeOffset, string>(
|
||||
v => v.UtcDateTime.ToString("o"),
|
||||
v => DateTimeOffset.Parse(v));
|
||||
var nullableConverter = new ValueConverter<DateTimeOffset?, string?>(
|
||||
v => v.HasValue ? v.Value.UtcDateTime.ToString("o") : null,
|
||||
v => v != null ? DateTimeOffset.Parse(v) : null);
|
||||
|
||||
foreach (var entityType in modelBuilder.Model.GetEntityTypes())
|
||||
{
|
||||
foreach (var property in entityType.GetProperties())
|
||||
{
|
||||
if (property.ClrType == typeof(DateTimeOffset))
|
||||
{
|
||||
property.SetValueConverter(converter);
|
||||
property.SetColumnType("TEXT");
|
||||
}
|
||||
else if (property.ClrType == typeof(DateTimeOffset?))
|
||||
{
|
||||
property.SetValueConverter(nullableConverter);
|
||||
property.SetColumnType("TEXT");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user