fix(transport): run bundle import inside the execution strategy (Gitea #28)
BundleImporter.ApplyAsync opened a user-initiated transaction directly,
but the central ConfigurationDb context is configured with
EnableRetryOnFailure. SqlServerRetryingExecutionStrategy rejects
user-initiated transactions, so every bundle import threw on any real
SQL Server ('...does not support user-initiated transactions.'). The
whole Transport suite ran on the in-memory provider (no retrying
strategy, BeginTransaction is a no-op) so it never surfaced.
Extract the transactional apply into ApplyMergeAsync and drive it via
_dbContext.Database.CreateExecutionStrategy().ExecuteAsync, so the
strategy owns the BeginTransaction -> apply -> Commit unit as one
retriable block. The delegate resets per-attempt state (change tracker
+ ApplyMergeAsync rebuilds its own summary/resolution accumulators) so a
retried attempt cannot double-apply; a rollback failure is captured and
surfaced on the BundleImportFailed audit row exactly as before. Post-
commit side effects (ScriptArtifactsChanged publish, session zero/
remove) moved outside the retriable delegate so they run once.
Regression test: BundleImporterRetryingStrategyTests runs the import on
SQLite with a retrying execution strategy (RetriesOnFailure == true,
arming the same guard as production). It fails with the exact production
error on the pre-fix code and passes after. Existing rollback/apply
contracts unchanged (104 integration + 154 unit tests green).
This commit is contained in:
Joseph Doherty
+219
@@ -0,0 +1,219 @@
|
||||
using System.Data.Common;
|
||||
using Microsoft.AspNetCore.DataProtection;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.EntityFrameworkCore.Diagnostics;
|
||||
using Microsoft.EntityFrameworkCore.Storage;
|
||||
using Microsoft.Extensions.Configuration;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Entities.Scripts;
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates;
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories;
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Transport;
|
||||
using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport;
|
||||
using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase;
|
||||
using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Repositories;
|
||||
using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Services;
|
||||
using ZB.MOM.WW.ScadaBridge.DeploymentManager;
|
||||
using ZB.MOM.WW.ScadaBridge.TemplateEngine;
|
||||
using ZB.MOM.WW.ScadaBridge.Transport;
|
||||
using ZB.MOM.WW.ScadaBridge.Transport.Import;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests.Import;
|
||||
|
||||
/// <summary>
|
||||
/// Regression guard for Gitea #28: <see cref="BundleImporter.ApplyAsync"/> must
|
||||
/// run its multi-<c>SaveChanges</c> apply through the context's execution
|
||||
/// strategy, because the production central <c>ScadaBridgeDbContext</c> is
|
||||
/// configured with <c>EnableRetryOnFailure</c> and
|
||||
/// <c>SqlServerRetryingExecutionStrategy</c> REJECTS a user-initiated
|
||||
/// transaction opened outside the strategy
|
||||
/// (<c>CoreStrings.ExecutionStrategyExistingTransaction</c>) — so before the
|
||||
/// fix every bundle import threw on any real SQL Server.
|
||||
/// <para>
|
||||
/// The rest of the Transport suite runs on the in-memory provider, which has NO
|
||||
/// retrying strategy and where <c>BeginTransactionAsync</c> is a no-op, so it
|
||||
/// could never surface this. Here we run on SQLite (real relational
|
||||
/// transactions) AND configure a retrying execution strategy whose only job is
|
||||
/// to have <c>RetriesOnFailure == true</c>, which arms the exact same guard as
|
||||
/// the production SQL Server strategy. If <c>ApplyAsync</c> opened the
|
||||
/// transaction directly, this test would throw before importing anything.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Reuses <see cref="SqliteCompatibleScadaBridgeDbContext"/> defined alongside
|
||||
/// <see cref="BundleImporterRollbackFailureTests"/> in this assembly.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public sealed class BundleImporterRetryingStrategyTests : IDisposable
|
||||
{
|
||||
private readonly ServiceProvider _provider;
|
||||
private readonly DbConnection _sharedConnection;
|
||||
|
||||
public BundleImporterRetryingStrategyTests()
|
||||
{
|
||||
var services = new ServiceCollection();
|
||||
services.AddSingleton<IConfiguration>(
|
||||
new ConfigurationBuilder().AddInMemoryCollection().Build());
|
||||
|
||||
// SQLite :memory: is per-connection, so pin one open connection for the
|
||||
// whole fixture or the schema vanishes between DbContext instances.
|
||||
_sharedConnection = new Microsoft.Data.Sqlite.SqliteConnection("DataSource=:memory:");
|
||||
_sharedConnection.Open();
|
||||
|
||||
services.AddSingleton<IDataProtectionProvider>(new EphemeralDataProtectionProvider());
|
||||
|
||||
services.AddSingleton(sp =>
|
||||
{
|
||||
var builder = new DbContextOptionsBuilder<ScadaBridgeDbContext>();
|
||||
// Configure a RETRYING execution strategy — this is the crux of the
|
||||
// regression. RetriesOnFailure == true (MaxRetryCount > 0) makes EF
|
||||
// reject any user-initiated BeginTransaction opened outside
|
||||
// strategy.ExecuteAsync, exactly like the production SQL Server
|
||||
// EnableRetryOnFailure configuration.
|
||||
builder.UseSqlite(
|
||||
_sharedConnection,
|
||||
sqlite => sqlite.ExecutionStrategy(deps => new TestRetryingExecutionStrategy(deps)));
|
||||
builder.ConfigureWarnings(w => w.Ignore(RelationalEventId.PendingModelChangesWarning));
|
||||
return builder.Options;
|
||||
});
|
||||
services.AddScoped<ScadaBridgeDbContext>(sp => new SqliteCompatibleScadaBridgeDbContext(
|
||||
sp.GetRequiredService<DbContextOptions<ScadaBridgeDbContext>>(),
|
||||
sp.GetRequiredService<IDataProtectionProvider>()));
|
||||
|
||||
services.AddScoped<ITemplateEngineRepository, TemplateEngineRepository>();
|
||||
services.AddScoped<IExternalSystemRepository, ExternalSystemRepository>();
|
||||
services.AddScoped<INotificationRepository, NotificationRepository>();
|
||||
services.AddScoped<IInboundApiRepository, InboundApiRepository>();
|
||||
services.AddScoped<ISiteRepository, SiteRepository>();
|
||||
services.AddScoped<IDeploymentManagerRepository, DeploymentManagerRepository>();
|
||||
services.AddScoped<ISharedSchemaRepository, SharedSchemaRepository>();
|
||||
services.AddScoped<IAuditCorrelationContext, AuditCorrelationContext>();
|
||||
services.AddScoped<IAuditService, AuditService>();
|
||||
services.AddTransport();
|
||||
// IStaleInstanceProbe (DeploymentManager) + flatten/hash (TemplateEngine)
|
||||
// so ApplyAsync's StaleInstanceIds computation resolves a real probe.
|
||||
services.AddTemplateEngine();
|
||||
services.AddDeploymentManager();
|
||||
|
||||
_provider = services.BuildServiceProvider();
|
||||
|
||||
using var scope = _provider.CreateScope();
|
||||
var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
|
||||
ctx.Database.EnsureCreated();
|
||||
}
|
||||
|
||||
public void Dispose()
|
||||
{
|
||||
_provider.Dispose();
|
||||
_sharedConnection.Dispose();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task ApplyAsync_succeeds_under_a_retrying_execution_strategy()
|
||||
{
|
||||
// Arrange: seed → export → wipe → apply(Add). Same shape as
|
||||
// BundleImporterApplyTests.ApplyAsync_adds_new_artifacts_in_single_transaction,
|
||||
// but the fixture's context carries a retrying execution strategy.
|
||||
await using (var scope = _provider.CreateAsyncScope())
|
||||
{
|
||||
var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
|
||||
ctx.SharedScripts.Add(new SharedScript("HelperFn", "return 1;"));
|
||||
ctx.Templates.Add(new Template("Pump") { Description = "fresh" });
|
||||
await ctx.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var sessionId = await ExportAndLoadAsync();
|
||||
await WipeContentAsync();
|
||||
|
||||
// Act: before the fix this threw CoreStrings.ExecutionStrategyExistingTransaction
|
||||
// ("... does not support user-initiated transactions.") from the direct
|
||||
// BeginTransactionAsync. With the fix the apply runs inside the strategy.
|
||||
ImportResult result;
|
||||
await using (var scope = _provider.CreateAsyncScope())
|
||||
{
|
||||
var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
|
||||
var resolutions = new List<ImportResolution>
|
||||
{
|
||||
new("Template", "Pump", ResolutionAction.Add, null),
|
||||
new("SharedScript", "HelperFn", ResolutionAction.Add, null),
|
||||
};
|
||||
result = await importer.ApplyAsync(sessionId, resolutions, user: "bob");
|
||||
}
|
||||
|
||||
// Assert: the whole transactional apply committed.
|
||||
Assert.Equal(2, result.Added);
|
||||
Assert.Equal(0, result.Overwritten);
|
||||
Assert.NotEqual(Guid.Empty, result.BundleImportId);
|
||||
await using (var scope = _provider.CreateAsyncScope())
|
||||
{
|
||||
var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
|
||||
Assert.Equal(1, await ctx.Templates.CountAsync(t => t.Name == "Pump"));
|
||||
Assert.Equal(1, await ctx.SharedScripts.CountAsync(s => s.Name == "HelperFn"));
|
||||
// The BundleImported audit row landed inside the committed transaction.
|
||||
Assert.Equal(1, await ctx.AuditLogEntries.CountAsync(a => a.Action == "BundleImported"));
|
||||
}
|
||||
}
|
||||
|
||||
// ---- helpers (mirror 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<ScadaBridgeDbContext>();
|
||||
var templateIds = await ctx.Templates.Select(t => t.Id).ToListAsync();
|
||||
var sharedScriptIds = await ctx.SharedScripts.Select(s => s.Id).ToListAsync();
|
||||
var selection = new ExportSelection(
|
||||
TemplateIds: templateIds,
|
||||
SharedScriptIds: sharedScriptIds,
|
||||
ExternalSystemIds: Array.Empty<int>(),
|
||||
DatabaseConnectionIds: Array.Empty<int>(),
|
||||
NotificationListIds: Array.Empty<int>(),
|
||||
SmtpConfigurationIds: 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<ScadaBridgeDbContext>();
|
||||
ctx.Templates.RemoveRange(ctx.Templates);
|
||||
ctx.SharedScripts.RemoveRange(ctx.SharedScripts);
|
||||
ctx.TemplateFolders.RemoveRange(ctx.TemplateFolders);
|
||||
await ctx.SaveChangesAsync();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A minimal retrying execution strategy for the test. Its sole purpose is
|
||||
/// to report <c>RetriesOnFailure == true</c> (inherited from
|
||||
/// <see cref="ExecutionStrategy"/>, since <c>MaxRetryCount > 0</c>), which
|
||||
/// arms EF's user-initiated-transaction guard just like the production
|
||||
/// <c>SqlServerRetryingExecutionStrategy</c>. It never actually retries —
|
||||
/// <see cref="ShouldRetryOn"/> returns false — so a genuine fault surfaces
|
||||
/// immediately instead of looping.
|
||||
/// </summary>
|
||||
private sealed class TestRetryingExecutionStrategy : ExecutionStrategy
|
||||
{
|
||||
public TestRetryingExecutionStrategy(ExecutionStrategyDependencies dependencies)
|
||||
: base(dependencies, maxRetryCount: 3, maxRetryDelay: TimeSpan.FromMilliseconds(1))
|
||||
{
|
||||
}
|
||||
|
||||
protected override bool ShouldRetryOn(Exception exception) => false;
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user