diff --git a/src/ScadaLink.Commons/Interfaces/Transport/IAuditCorrelationContext.cs b/src/ScadaLink.Commons/Interfaces/Transport/IAuditCorrelationContext.cs index f042822..962246a 100644 --- a/src/ScadaLink.Commons/Interfaces/Transport/IAuditCorrelationContext.cs +++ b/src/ScadaLink.Commons/Interfaces/Transport/IAuditCorrelationContext.cs @@ -4,6 +4,17 @@ namespace ScadaLink.Commons.Interfaces.Transport; /// Scoped service the bundle importer sets to thread a BundleImportId through to /// the audit log entries emitted by the audited repository methods invoked during /// ApplyAsync. AuditService reads this and stamps every AuditLogEntry it writes. +/// +/// Re-entrancy / thread-safety: mutating is NOT +/// thread-safe. The service is registered scoped, and the assumed usage is a +/// single Blazor Server circuit (or single API request) at a time — within that +/// scope is the sole writer, and the +/// audit service is the sole reader, in a strictly sequential await chain. +/// Callers that perform concurrent imports within a shared scope (e.g. two +/// ApplyAsync calls awaited via Task.WhenAll on the same circuit) +/// MUST serialize access externally — there is no internal lock and the last +/// writer wins, which would cross-contaminate audit rows between imports. +/// /// public interface IAuditCorrelationContext { diff --git a/src/ScadaLink.Transport/Import/BundleImporter.cs b/src/ScadaLink.Transport/Import/BundleImporter.cs index 751a595..13dfe26 100644 --- a/src/ScadaLink.Transport/Import/BundleImporter.cs +++ b/src/ScadaLink.Transport/Import/BundleImporter.cs @@ -25,6 +25,19 @@ namespace ScadaLink.Transport.Import; /// resolutions through the audited repositories. Only LoadAsync is /// implemented in this slice — the other two are wired into DI now so /// follow-up tasks can fill them in without churning the constructor. +/// +/// Audit-row responsibility: repository mutation methods in +/// ScadaLink.ConfigurationDatabase.Repositories are thin EF wrappers +/// and do NOT emit audit rows. therefore writes +/// each per-entity audit row explicitly via ; +/// the scoped is +/// automatically stamped on each row by the audit service. +/// +/// +/// If repository methods are ever changed to emit audit rows themselves, +/// the explicit LogAsync calls in this class must be removed to +/// avoid double-logging. +/// /// public sealed class BundleImporter : IBundleImporter { @@ -562,11 +575,41 @@ public sealed class BundleImporter : IBundleImporter } catch (Exception ex) { - await tx.RollbackAsync(ct).ConfigureAwait(false); + // Rollback can itself throw (connection drop mid-rollback, provider + // bug, etc). If it does, we must STILL write the BundleImportFailed + // audit row — otherwise a rollback-failure path silently swallows + // the import's audit trail. Capture the rollback exception (if any) + // and surface it on the failure row alongside the original cause. + Exception? rollbackFailure = null; + try + { + await tx.RollbackAsync(ct).ConfigureAwait(false); + } + catch (Exception rbEx) + { + rollbackFailure = rbEx; + } + + // If rollback threw the IDbContextTransaction is in an indeterminate + // state and still associated with the DbContext — a subsequent + // SaveChangesAsync would attempt to enlist in (or commit to) that + // broken transaction, and the failure-row would itself be rolled + // back when the transaction is finally disposed. Dispose it now so + // the audit-row write below uses a fresh implicit transaction. On + // the happy rollback path Dispose is a benign no-op (the using + // would call it on scope exit anyway). + if (rollbackFailure is not null) + { + try { await tx.DisposeAsync().ConfigureAwait(false); } + catch { /* dispose-after-throw must not mask the original cause */ } + } // Clear the change tracker before writing the failure row — on the // in-memory provider the rollback is a no-op and the staged adds - // would otherwise persist when the next SaveChangesAsync runs. + // would otherwise persist when the next SaveChangesAsync runs. This + // also matters when rollback threw: the change tracker is in an + // ambiguous state and we don't want the failure-write to sweep up + // any of the staged apply mutations. _dbContext.ChangeTracker.Clear(); // Clear correlation FIRST so the failure row doesn't carry the now- @@ -574,20 +617,32 @@ public sealed class BundleImporter : IBundleImporter // exists at top level (no correlation) so audit consumers can see // imports that aborted before any rows landed. _correlationContext.BundleImportId = null; - await _auditService.LogAsync( - user: user, - action: "BundleImportFailed", - entityType: "Bundle", - entityId: bundleImportId.ToString(), - entityName: session.Manifest.SourceEnvironment, - afterState: new - { - BundleImportId = bundleImportId, - Reason = ex.Message, - ExceptionType = ex.GetType().FullName, - }, - cancellationToken: ct).ConfigureAwait(false); - await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false); + try + { + await _auditService.LogAsync( + user: user, + action: "BundleImportFailed", + entityType: "Bundle", + entityId: bundleImportId.ToString(), + entityName: session.Manifest.SourceEnvironment, + afterState: new + { + BundleImportId = bundleImportId, + Reason = ex.Message, + ExceptionType = ex.GetType().FullName, + RollbackException = rollbackFailure?.Message, + }, + cancellationToken: ct).ConfigureAwait(false); + await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false); + } + catch + { + // Audit-write is best-effort per design §10 ("Audit-write failure + // NEVER aborts the user-facing action — audit is best-effort, the + // action's own success/failure path is authoritative"). Swallow + // any failure here so the original exception below propagates + // unchanged rather than being masked by an audit-layer fault. + } throw; } finally diff --git a/src/ScadaLink.Transport/Import/BundleSessionStore.cs b/src/ScadaLink.Transport/Import/BundleSessionStore.cs index a647623..55cbbf1 100644 --- a/src/ScadaLink.Transport/Import/BundleSessionStore.cs +++ b/src/ScadaLink.Transport/Import/BundleSessionStore.cs @@ -11,6 +11,17 @@ namespace ScadaLink.Transport.Import; /// at read time () and on-demand via ; /// there is no background timer. /// +/// Thread-safety: backed by of +/// to . All store operations +/// ( / / / +/// ) use the concurrent dictionary's safe primitives +/// (TryGetValue, indexer assignment, TryRemove) and are safe +/// under concurrent callers. The instance itself +/// is NOT thread-safe — callers that share a session reference (e.g. two +/// importers mutating FailedUnlockAttempts on the same session) MUST +/// serialize their mutations on that shared reference. +/// +/// /// TTL is supplied by the importer via ; /// this store does not impose its own. The injected /// is used purely to determine now when checking ExpiresAt, which diff --git a/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterRollbackFailureTests.cs b/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterRollbackFailureTests.cs new file mode 100644 index 0000000..c8c1878 --- /dev/null +++ b/tests/ScadaLink.Transport.IntegrationTests/Import/BundleImporterRollbackFailureTests.cs @@ -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; + +/// +/// Covers the catch-path invariant in : +/// even when the EF RollbackAsync itself throws (connection drop mid- +/// rollback, provider bug, etc.) the BundleImportFailed audit row MUST +/// still land, and the ORIGINAL exception (not the rollback failure) MUST +/// propagate to the caller. +/// +/// Uses SQLite rather than the in-memory provider because the in-memory +/// provider's transaction is a no-op — its RollbackAsync 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. +/// +/// +/// The interceptor is wired to throw on +/// and the async equivalent — this is the hook EF invokes synchronously inside +/// IDbContextTransaction.RollbackAsync, so a throw there surfaces as +/// the RollbackAsync call itself throwing, which is exactly the +/// scenario the catch block must survive. +/// +/// +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( + 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(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 (the base type's options) so the + // single options registration serves both. This avoids the multi-options + // pitfall of AddDbContext which keys options on TImpl. + services.AddSingleton(sp => + { + var builder = new DbContextOptionsBuilder(); + builder.UseSqlite(_sharedConnection); + builder.ConfigureWarnings(w => w.Ignore(RelationalEventId.PendingModelChangesWarning)); + builder.AddInterceptors(_interceptor); + return builder.Options; + }); + services.AddScoped(sp => new SqliteCompatibleScadaLinkDbContext( + sp.GetRequiredService>(), + sp.GetRequiredService())); + + services.AddScoped(); + services.AddScoped(); + services.AddScoped(); + services.AddScoped(); + services.AddScoped(); + services.AddScoped(); + services.AddTransport(); + + _provider = services.BuildServiceProvider(); + + // Build schema once on the shared connection. + using var scope = _provider.CreateScope(); + var ctx = scope.ServiceProvider.GetRequiredService(); + 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(); + 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(); + thrown = await Assert.ThrowsAsync(() => + importer.ApplyAsync(sessionId, + new List { 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(); + 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 ExportAndLoadAsync() + { + Stream bundleStream; + await using (var scope = _provider.CreateAsyncScope()) + { + var exporter = scope.ServiceProvider.GetRequiredService(); + var ctx = scope.ServiceProvider.GetRequiredService(); + var templateIds = await ctx.Templates.Select(t => t.Id).ToListAsync(); + var selection = new ExportSelection( + TemplateIds: templateIds, + SharedScriptIds: Array.Empty(), + ExternalSystemIds: Array.Empty(), + DatabaseConnectionIds: Array.Empty(), + NotificationListIds: Array.Empty(), + SmtpConfigurationIds: Array.Empty(), + ApiKeyIds: Array.Empty(), + ApiMethodIds: Array.Empty(), + 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(); + 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(); + ctx.Templates.RemoveRange(ctx.Templates); + ctx.SharedScripts.RemoveRange(ctx.SharedScripts); + ctx.TemplateFolders.RemoveRange(ctx.TemplateFolders); + await ctx.SaveChangesAsync(); + } + + /// + /// EF transaction interceptor that throws on rollback when armed. Used by + /// + /// to simulate the connection-dropped-during-rollback scenario. EF calls + /// the async hook from inside IDbContextTransaction.RollbackAsync, + /// so a throw here surfaces as RollbackAsync itself throwing — + /// exactly the contract the catch block must survive. + /// + private sealed class ThrowingRollbackInterceptor : DbTransactionInterceptor + { + public const string RollbackErrorMarker = "simulated rollback failure"; + + public bool ThrowOnRollback { get; set; } + + public override ValueTask 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); + } + } +} + +/// +/// SQLite-compatible variant of used by +/// . Mirrors the adaptations in +/// SqliteTestDbContext 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. +/// +internal sealed class SqliteCompatibleScadaLinkDbContext : ScadaLinkDbContext +{ + public SqliteCompatibleScadaLinkDbContext( + DbContextOptions options, + IDataProtectionProvider dataProtectionProvider) + : base(options, dataProtectionProvider) + { + } + + protected override void OnModelCreating(ModelBuilder modelBuilder) + { + base.OnModelCreating(modelBuilder); + + modelBuilder.Entity(builder => + { + builder.Property(d => d.RowVersion) + .IsRequired(false) + .IsConcurrencyToken(false) + .ValueGeneratedNever(); + }); + + var converter = new ValueConverter( + v => v.UtcDateTime.ToString("o"), + v => DateTimeOffset.Parse(v)); + var nullableConverter = new ValueConverter( + 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"); + } + } + } + } +}