diff --git a/src/BuildingBlocks/Persistence/AmbientDbTransactionRegistry.cs b/src/BuildingBlocks/Persistence/AmbientDbTransactionRegistry.cs index 826df5e561..8511bed9d5 100644 --- a/src/BuildingBlocks/Persistence/AmbientDbTransactionRegistry.cs +++ b/src/BuildingBlocks/Persistence/AmbientDbTransactionRegistry.cs @@ -11,6 +11,21 @@ namespace FSH.Framework.Persistence; /// attached to every Hero DbContext and records transactions as they start and end, which is what /// lets the outbox write enlist in the business transaction instead of committing separately. /// +/// +/// +/// Every method here must match exactly, sync *and* +/// async. The interface supplies default no-op implementations for all of its members, so a +/// near-miss signature still compiles — it just silently never gets called, leaving this registry +/// permanently empty and the outbox never enlisted. +/// +/// +/// That failure mode is invisible on PostgreSQL: Npgsql associates a command with whatever +/// transaction is open on its connection, so the outbox row joins the business transaction anyway. +/// SQL Server does not — SqlCommand.Transaction must be set explicitly or execution throws +/// "BeginExecuteReader requires the command to have a transaction…". Guarded by +/// AmbientDbTransactionRegistryTests. +/// +/// public sealed class AmbientDbTransactionRegistry : IDbTransactionInterceptor { private readonly Dictionary _open = []; @@ -22,21 +37,80 @@ public sealed class AmbientDbTransactionRegistry : IDbTransactionInterceptor public DbTransaction? Find(DbConnection connection) => connection is not null && _open.TryGetValue(connection, out var transaction) ? transaction : null; - public void TransactionStarted(DbConnection connection, TransactionEndEventData eventData) - => Track(connection, eventData?.Transaction); + public DbTransaction TransactionStarted( + DbConnection connection, + TransactionEndEventData eventData, + DbTransaction result) + { + Track(connection, result); + return result; + } - public void TransactionUsed(DbConnection connection, TransactionEventData eventData) - => Track(connection, eventData?.Transaction); + public ValueTask TransactionStartedAsync( + DbConnection connection, + TransactionEndEventData eventData, + DbTransaction result, + CancellationToken cancellationToken = default) + { + Track(connection, result); + return ValueTask.FromResult(result); + } + + public DbTransaction TransactionUsed( + DbConnection connection, + TransactionEventData eventData, + DbTransaction result) + { + Track(connection, result); + return result; + } + + public ValueTask TransactionUsedAsync( + DbConnection connection, + TransactionEventData eventData, + DbTransaction result, + CancellationToken cancellationToken = default) + { + Track(connection, result); + return ValueTask.FromResult(result); + } public void TransactionCommitted(DbTransaction transaction, TransactionEndEventData eventData) => Forget(transaction); + public Task TransactionCommittedAsync( + DbTransaction transaction, + TransactionEndEventData eventData, + CancellationToken cancellationToken = default) + { + Forget(transaction); + return Task.CompletedTask; + } + public void TransactionRolledBack(DbTransaction transaction, TransactionEndEventData eventData) => Forget(transaction); + public Task TransactionRolledBackAsync( + DbTransaction transaction, + TransactionEndEventData eventData, + CancellationToken cancellationToken = default) + { + Forget(transaction); + return Task.CompletedTask; + } + public void TransactionFailed(DbTransaction transaction, TransactionErrorEventData eventData) => Forget(transaction); + public Task TransactionFailedAsync( + DbTransaction transaction, + TransactionErrorEventData eventData, + CancellationToken cancellationToken = default) + { + Forget(transaction); + return Task.CompletedTask; + } + private void Track(DbConnection connection, DbTransaction? transaction) { if (connection is not null && transaction is not null) @@ -50,6 +124,17 @@ private void Forget(DbTransaction? transaction) if (transaction?.Connection is not null) { _open.Remove(transaction.Connection); + return; + } + + // A disposed transaction reports a null Connection, so fall back to identity: leaving a + // completed transaction in the map would make the next write try to enlist in it. + if (transaction is not null) + { + foreach (var entry in _open.Where(e => ReferenceEquals(e.Value, transaction)).ToList()) + { + _open.Remove(entry.Key); + } } } } diff --git a/src/Tests/Framework.Tests/Persistence/AmbientDbTransactionRegistryTests.cs b/src/Tests/Framework.Tests/Persistence/AmbientDbTransactionRegistryTests.cs new file mode 100644 index 0000000000..66d89d97ea --- /dev/null +++ b/src/Tests/Framework.Tests/Persistence/AmbientDbTransactionRegistryTests.cs @@ -0,0 +1,56 @@ +using FSH.Framework.Persistence; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Shouldly; + +namespace Framework.Tests.Persistence; + +/// +/// Guards the silent-no-op failure mode of . +/// +/// +/// gives every member a default no-op implementation, so a +/// registry method whose signature does not match the interface still compiles and is simply never +/// invoked. The registry then stays empty, the outbox never enlists in the business transaction, +/// and the transactional-outbox guarantee is silently lost — which PostgreSQL masks, because Npgsql +/// associates commands with the connection's open transaction regardless. +/// +public class AmbientDbTransactionRegistryTests +{ + private static readonly string[] MustBeImplemented = + [ + "TransactionStarted", + "TransactionStartedAsync", + "TransactionUsed", + "TransactionUsedAsync", + "TransactionCommitted", + "TransactionCommittedAsync", + "TransactionRolledBack", + "TransactionRolledBackAsync", + "TransactionFailed", + "TransactionFailedAsync" + ]; + + [Fact] + public void Registry_Should_Actually_Implement_Every_Interceptor_Hook_It_Relies_On() + { + var map = typeof(AmbientDbTransactionRegistry).GetInterfaceMap(typeof(IDbTransactionInterceptor)); + + var notWiredUp = new List(); + for (int i = 0; i < map.InterfaceMethods.Length; i++) + { + string name = map.InterfaceMethods[i].Name; + if (!MustBeImplemented.Contains(name)) continue; + + // When a signature does not match, the interface's own default implementation is the + // target — meaning the registry's method is dead code. + if (map.TargetMethods[i].DeclaringType != typeof(AmbientDbTransactionRegistry)) + { + notWiredUp.Add(name); + } + } + + notWiredUp.ShouldBeEmpty( + "these IDbTransactionInterceptor hooks fall through to the interface default, so the " + + "registry never records transactions started via them: " + string.Join(", ", notWiredUp)); + } +}