FIX-8D-RETRY: wrap docx execute transaction in CreateExecutionStrategy
NpgsqlRetryingExecutionStrategy rejects user-initiated transactions: SaveChanges inside a BeginTransactionAsync block triggers OnFirstExecution which throws InvalidOperationException. Fix: CreateExecutionStrategy().ExecuteAsync wraps the entire tx block; mutable state (counters, contactByNorm, change tracker) reset at lambda top for idempotent retry. Logic extracted to RunLoopAsync local function shared by dry-run and execute paths. Regression test (Test 7): FakeRetryingStrategy with MaxRetryCount=1 reproduces the OnFirstExecution check in CI without a live Npgsql instance. 165/165 tests, ef has-pending=No, no schema change. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -2,6 +2,7 @@ using GerbilManagerWebAPI.Import;
|
|||||||
using GerbilManagerWebAPI.Models;
|
using GerbilManagerWebAPI.Models;
|
||||||
using Microsoft.Data.Sqlite;
|
using Microsoft.Data.Sqlite;
|
||||||
using Microsoft.EntityFrameworkCore;
|
using Microsoft.EntityFrameworkCore;
|
||||||
|
using Microsoft.EntityFrameworkCore.Storage;
|
||||||
|
|
||||||
namespace GerbilManager.Tests
|
namespace GerbilManager.Tests
|
||||||
{
|
{
|
||||||
@@ -226,6 +227,52 @@ namespace GerbilManager.Tests
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ── Test 7: P0 REGRESSION — execute works under a retrying execution strategy ──
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task Execute_works_under_retrying_execution_strategy()
|
||||||
|
{
|
||||||
|
// Regression: NpgsqlRetryingExecutionStrategy (MaxRetryCount>0) calls
|
||||||
|
// OnFirstExecution() at the start of ExecuteAsync, which throws
|
||||||
|
// InvalidOperationException when it detects a user-initiated transaction
|
||||||
|
// that was NOT opened through the strategy. This test wires the same check
|
||||||
|
// (via FakeRetryingStrategy, MaxRetryCount=1) so the bug would surface in CI
|
||||||
|
// without a live Npgsql instance.
|
||||||
|
//
|
||||||
|
// With the BUG (direct BeginTransactionAsync before strategy.ExecuteAsync):
|
||||||
|
// → OnFirstExecution sees active user tx → InvalidOperationException
|
||||||
|
// With the FIX (BeginTransactionAsync inside strategy.ExecuteAsync lambda):
|
||||||
|
// → OnFirstExecution: no tx yet → OK
|
||||||
|
var conn = new SqliteConnection("DataSource=:memory:");
|
||||||
|
conn.Open();
|
||||||
|
var opts = new DbContextOptionsBuilder<ApplicationContext>()
|
||||||
|
.UseSqlite(conn)
|
||||||
|
.ReplaceService<IExecutionStrategyFactory, FakeRetryingStrategyFactory>()
|
||||||
|
.Options;
|
||||||
|
var db = new ApplicationContext(opts);
|
||||||
|
db.Database.EnsureCreated();
|
||||||
|
|
||||||
|
await using (conn)
|
||||||
|
await using (db)
|
||||||
|
{
|
||||||
|
WriteLitters(Array.Empty<object>());
|
||||||
|
WriteAnimals(new[]
|
||||||
|
{
|
||||||
|
new { wsCode = "3/3", litterDob = "01.01.2023", name = "Pixie", gender = "female",
|
||||||
|
owner = "Retry Adopter", abgabeDate = "01.03.2023",
|
||||||
|
deathDate = "", deathCause = "", farbschlag = "" }
|
||||||
|
});
|
||||||
|
|
||||||
|
// Must NOT throw InvalidOperationException (user-initiated tx rejected)
|
||||||
|
var report = await new ImportDocxService(db, _dir).RunAsync(execute: true);
|
||||||
|
|
||||||
|
Assert.True(report.Executed);
|
||||||
|
Assert.Equal(1, report.Created);
|
||||||
|
Assert.Equal(1, await db.Gerbils.CountAsync());
|
||||||
|
Assert.Equal(1, await db.Contacts.CountAsync());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// ── Test 6: P0 REGRESSION — same-name siblings get distinct ExternalRefs ─
|
// ── Test 6: P0 REGRESSION — same-name siblings get distinct ExternalRefs ─
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
@@ -273,4 +320,24 @@ namespace GerbilManager.Tests
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ── Helpers for Test 7 ────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Execution strategy with MaxRetryCount=1 so that EF Core's base
|
||||||
|
/// OnFirstExecution() throws when it detects a user-initiated transaction
|
||||||
|
/// that was not opened through CreateExecutionStrategy().ExecuteAsync().
|
||||||
|
/// ShouldRetryOn=false → no actual retry; the check alone is what we need.
|
||||||
|
/// </summary>
|
||||||
|
internal sealed class FakeRetryingStrategy(ExecutionStrategyDependencies deps)
|
||||||
|
: ExecutionStrategy(deps, maxRetryCount: 1, maxRetryDelay: TimeSpan.Zero)
|
||||||
|
{
|
||||||
|
protected override bool ShouldRetryOn(Exception exception) => false;
|
||||||
|
}
|
||||||
|
|
||||||
|
internal sealed class FakeRetryingStrategyFactory(ExecutionStrategyDependencies deps)
|
||||||
|
: IExecutionStrategyFactory
|
||||||
|
{
|
||||||
|
public IExecutionStrategy Create() => new FakeRetryingStrategy(deps);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -22,7 +22,10 @@ namespace GerbilManagerWebAPI.Import
|
|||||||
/// NEVER overwrites a manually-set non-null value (fill-NULL-only for all fields).
|
/// NEVER overwrites a manually-set non-null value (fill-NULL-only for all fields).
|
||||||
///
|
///
|
||||||
/// Idempotent: running multiple times is safe. Re-run finds existing rows via ExternalRef.
|
/// Idempotent: running multiple times is safe. Re-run finds existing rows via ExternalRef.
|
||||||
/// Execute wraps all writes in a single transaction (atomic: crash → full rollback).
|
/// Execute wraps all writes in a single transaction via CreateExecutionStrategy() so that
|
||||||
|
/// providers using EnableRetryOnFailure (e.g. NpgsqlRetryingExecutionStrategy) are
|
||||||
|
/// compatible. The strategy lambda resets all mutable state at the top so it is safe
|
||||||
|
/// to re-run on transient-failure retry.
|
||||||
/// Execute is gated by the endpoint; this service only acts when asked.
|
/// Execute is gated by the endpoint; this service only acts when asked.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
public sealed class ImportDocxService
|
public sealed class ImportDocxService
|
||||||
@@ -94,6 +97,11 @@ namespace GerbilManagerWebAPI.Import
|
|||||||
.GroupBy(c => NormalizeName(c.Name))
|
.GroupBy(c => NormalizeName(c.Name))
|
||||||
.ToDictionary(g => g.Key, g => g.First().Id);
|
.ToDictionary(g => g.Key, g => g.First().Id);
|
||||||
|
|
||||||
|
// Snapshot of DB contacts before any writes.
|
||||||
|
// Used to reset contactByNorm on strategy retry (rolled-back contacts vanish from DB
|
||||||
|
// but would remain in the in-memory dict without this reset).
|
||||||
|
var contactByNormBase = new Dictionary<string, Guid>(contactByNorm);
|
||||||
|
|
||||||
// ColorVariety lookup: normalized name → Id (for CREATE path Farbschlag matching)
|
// ColorVariety lookup: normalized name → Id (for CREATE path Farbschlag matching)
|
||||||
var colorVarietyByName = (await _db.ColorVarieties
|
var colorVarietyByName = (await _db.ColorVarieties
|
||||||
.Select(cv => new { cv.Id, cv.Name })
|
.Select(cv => new { cv.Id, cv.Name })
|
||||||
@@ -105,19 +113,15 @@ namespace GerbilManagerWebAPI.Import
|
|||||||
int ownerLinked = 0, ownerCreated = 0, skipped = 0;
|
int ownerLinked = 0, ownerCreated = 0, skipped = 0;
|
||||||
|
|
||||||
// Ordinal counter for collision-free ExternalRef within this batch.
|
// Ordinal counter for collision-free ExternalRef within this batch.
|
||||||
// Two animals with the same base ref (same ws+name+litterDob) get -2, -3 suffixes.
|
|
||||||
var externalRefOrdinals = new Dictionary<string, int>();
|
var externalRefOrdinals = new Dictionary<string, int>();
|
||||||
|
|
||||||
// Belt-and-suspenders: guard against adding the same ExternalRef twice in one run.
|
// Belt-and-suspenders: guard against adding the same ExternalRef twice in one run.
|
||||||
var batchRefs = new HashSet<string>();
|
var batchRefs = new HashSet<string>();
|
||||||
|
|
||||||
// --- Planning pass (dry-run counts + execute writes) ---
|
// Inner loop — shared by dry-run and execute paths.
|
||||||
// Execute path is wrapped in a single transaction for atomicity.
|
// All local variables above are captured by reference (C# closure), so the strategy
|
||||||
Microsoft.EntityFrameworkCore.Storage.IDbContextTransaction? tx = null;
|
// lambda can reset them before each retry and RunLoopAsync sees the fresh state.
|
||||||
if (execute)
|
async Task RunLoopAsync()
|
||||||
tx = await _db.Database.BeginTransactionAsync();
|
|
||||||
|
|
||||||
try
|
|
||||||
{
|
{
|
||||||
foreach (var da in docxAnimals)
|
foreach (var da in docxAnimals)
|
||||||
{
|
{
|
||||||
@@ -276,21 +280,36 @@ namespace GerbilManagerWebAPI.Import
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Flush all gerbil inserts + enrich updates in one shot (within the tx)
|
|
||||||
if (execute && (animalsCreated + litterLinked + goHomeFilled + deathFilled + ownerCreated) > 0)
|
|
||||||
await _db.SaveChangesAsync();
|
|
||||||
|
|
||||||
if (tx is not null) await tx.CommitAsync();
|
|
||||||
}
|
}
|
||||||
catch
|
|
||||||
|
if (!execute)
|
||||||
{
|
{
|
||||||
// tx.DisposeAsync (in finally) rolls back if not committed
|
// Dry-run: just count, no writes, no transaction needed.
|
||||||
throw;
|
await RunLoopAsync();
|
||||||
}
|
}
|
||||||
finally
|
else
|
||||||
{
|
{
|
||||||
if (tx is not null) await tx.DisposeAsync();
|
// Execute: wrap the entire transaction in the execution strategy so that providers
|
||||||
|
// with EnableRetryOnFailure (NpgsqlRetryingExecutionStrategy) are compatible.
|
||||||
|
// The lambda resets all mutable state at the top so retries start clean.
|
||||||
|
var strategy = _db.Database.CreateExecutionStrategy();
|
||||||
|
await strategy.ExecuteAsync(async () =>
|
||||||
|
{
|
||||||
|
// Reset mutable state — idempotent on strategy retry
|
||||||
|
_db.ChangeTracker.Clear();
|
||||||
|
externalRefOrdinals.Clear();
|
||||||
|
batchRefs.Clear();
|
||||||
|
animalsCreated = 0; litterLinked = 0; goHomeFilled = 0; deathFilled = 0;
|
||||||
|
ownerLinked = 0; ownerCreated = 0; skipped = 0;
|
||||||
|
// Rebuild from DB snapshot: contacts added in a failed attempt were rolled back
|
||||||
|
contactByNorm = new Dictionary<string, Guid>(contactByNormBase);
|
||||||
|
|
||||||
|
await using var tx = await _db.Database.BeginTransactionAsync();
|
||||||
|
await RunLoopAsync();
|
||||||
|
if ((animalsCreated + litterLinked + goHomeFilled + deathFilled + ownerCreated) > 0)
|
||||||
|
await _db.SaveChangesAsync();
|
||||||
|
await tx.CommitAsync();
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
notes.Add($"Quelle: {docxLitters.Count} Würfe, {docxAnimals.Count} Tier-Zeilen aus der docx.");
|
notes.Add($"Quelle: {docxLitters.Count} Würfe, {docxAnimals.Count} Tier-Zeilen aus der docx.");
|
||||||
|
|||||||
Reference in New Issue
Block a user