diff --git a/src/DodoSSH.Api/Features/Identity/EnrollmentService.cs b/src/DodoSSH.Api/Features/Identity/EnrollmentService.cs index 8e61083..7bf49c5 100644 --- a/src/DodoSSH.Api/Features/Identity/EnrollmentService.cs +++ b/src/DodoSSH.Api/Features/Identity/EnrollmentService.cs @@ -1,5 +1,6 @@ using System.Security.Cryptography; using System.Text.Json; +using DodoSSH.Api.Setup; using DodoSSH.Contracts; using DodoSSH.Crypto; using DodoSSH.Domain; @@ -106,9 +107,7 @@ internal sealed class EnrollmentService( byte[] fingerprint, CancellationToken cancellationToken) { - var strategy = database.Database.CreateExecutionStrategy(); - - return await strategy.ExecuteAsync(async () => + return await database.ExecuteFreshAsync([user], async () => { var transaction = await database.Database .BeginTransactionAsync(cancellationToken) diff --git a/src/DodoSSH.Api/Features/Sync/SyncService.cs b/src/DodoSSH.Api/Features/Sync/SyncService.cs index 7245aaf..bd59d94 100644 --- a/src/DodoSSH.Api/Features/Sync/SyncService.cs +++ b/src/DodoSSH.Api/Features/Sync/SyncService.cs @@ -100,9 +100,7 @@ internal sealed class SyncService( { ValidateBatchLimits(request); - var strategy = database.Database.CreateExecutionStrategy(); - - return await strategy.ExecuteAsync(async () => + return await database.ExecuteFreshAsync([vault], async () => { var transaction = await database.Database .BeginTransactionAsync(cancellationToken) diff --git a/src/DodoSSH.Api/Features/Teams/TeamService.cs b/src/DodoSSH.Api/Features/Teams/TeamService.cs index ec952fd..0417432 100644 --- a/src/DodoSSH.Api/Features/Teams/TeamService.cs +++ b/src/DodoSSH.Api/Features/Teams/TeamService.cs @@ -1,5 +1,6 @@ using System.Globalization; using DodoSSH.Api.Features.Events; +using DodoSSH.Api.Setup; using DodoSSH.Contracts; using DodoSSH.Domain; using DodoSSH.Infrastructure; @@ -265,9 +266,7 @@ internal sealed class TeamService( } var now = clock.GetUtcNow(); - var strategy = database.Database.CreateExecutionStrategy(); - - var archived = await strategy.ExecuteAsync(async () => + var archived = await database.ExecuteFreshAsync([team], async () => { var transaction = await database.Database .BeginTransactionAsync(cancellationToken) @@ -343,9 +342,7 @@ internal sealed class TeamService( var incoming = await RequireMembershipAsync(teamId, request.UserId, cancellationToken) .ConfigureAwait(false); - var strategy = database.Database.CreateExecutionStrategy(); - - await strategy.ExecuteAsync(async () => + await database.ExecuteFreshAsync([incoming, outgoing], async () => { var transaction = await database.Database .BeginTransactionAsync(cancellationToken) @@ -724,9 +721,7 @@ internal sealed class TeamService( } var now = clock.GetUtcNow(); - var strategy = database.Database.CreateExecutionStrategy(); - - var revoked = await strategy.ExecuteAsync(async () => + var revoked = await database.ExecuteFreshAsync([membership], async () => { var transaction = await database.Database .BeginTransactionAsync(cancellationToken) diff --git a/src/DodoSSH.Api/Setup/FreshAttempts.cs b/src/DodoSSH.Api/Setup/FreshAttempts.cs new file mode 100644 index 0000000..30e5f99 --- /dev/null +++ b/src/DodoSSH.Api/Setup/FreshAttempts.cs @@ -0,0 +1,72 @@ +using DodoSSH.Infrastructure; +using Microsoft.EntityFrameworkCore; + +namespace DodoSSH.Api.Setup; + +/// +/// Runs a write transaction under the retrying execution strategy so that a retry starts from the database +/// rather than from what the failed attempt left in the change tracker. +/// +/// +/// +/// The strategy re-runs the delegate on the same context, and the context remembers the attempt that +/// failed. When the failure is the commit itself — a connection dropped after SaveChanges had +/// already succeeded — every entity that attempt wrote is marked unchanged, holding values the database +/// rolled back. The retry then assigns the same values again, change detection sees nothing to save, and +/// the transaction commits without them. Enrolment lost EnrolledAtUtc that way, and a team hand-over +/// its two role changes, each reporting success. +/// +/// +/// So a retry clears the tracker, and reloads each entity the delegate was handed from outside it — the ones +/// it modifies or reads but did not load itself — so they are tracked again with what the database actually +/// holds and the delegate's own assignments register as changes. The first attempt runs exactly as it did +/// before: nothing is cleared or reloaded until something has failed. +/// +/// +internal static class FreshAttempts +{ + /// Runs under the context's execution strategy, freshly each time. + /// The request's context. + /// Entities loaded before the transaction that the attempt reads or modifies. + /// The transaction, which begins and commits its own. + internal static Task ExecuteFreshAsync( + this DodoDbContext database, + IReadOnlyList carried, + Func> attempt) + { + ArgumentNullException.ThrowIfNull(database); + ArgumentNullException.ThrowIfNull(carried); + ArgumentNullException.ThrowIfNull(attempt); + + var strategy = database.Database.CreateExecutionStrategy(); + var attempts = 0; + + return strategy.ExecuteAsync(async () => + { + if (attempts++ > 0) + { + database.ChangeTracker.Clear(); + + foreach (var entity in carried) + { + await database.Entry(entity).ReloadAsync().ConfigureAwait(false); + } + } + + return await attempt().ConfigureAwait(false); + }); + } + + /// + internal static Task ExecuteFreshAsync( + this DodoDbContext database, + IReadOnlyList carried, + Func attempt) => + database.ExecuteFreshAsync( + carried, + async () => + { + await attempt().ConfigureAwait(false); + return true; + }); +} diff --git a/src/DodoSSH.Api/Setup/Persistence.cs b/src/DodoSSH.Api/Setup/Persistence.cs index f50d21d..aca0a80 100644 --- a/src/DodoSSH.Api/Setup/Persistence.cs +++ b/src/DodoSSH.Api/Setup/Persistence.cs @@ -1,5 +1,6 @@ using DodoSSH.Infrastructure; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.Extensions.Diagnostics.HealthChecks; using Microsoft.Extensions.Options; using Npgsql; @@ -29,14 +30,18 @@ internal static class Persistence ?? throw new InvalidOperationException( $"ConnectionStrings:{ConnectionStringName} is not configured."); - services.AddDbContext(options => options + services.AddDbContext((provider, options) => options .UseNpgsql(connectionString, npgsql => npgsql .MigrationsHistoryTable("__EFMigrationsHistory", DodoDbContext.SchemaName) - // Retry on transient faults only. Note this is safe here because writes go - // through explicitly-managed transactions rather than relying on the execution - // strategy to replay ambient ones. + // Retry on transient faults only. A write transaction replayed by the strategy runs on + // the same context, which still holds what the failed attempt tracked — every such + // transaction goes through FreshAttempts.ExecuteFreshAsync, which starts a retry from + // the database instead. .EnableRetryOnFailure(3)) - .UseSnakeCaseNamingConvention()); + .UseSnakeCaseNamingConvention() + // None in production. The seam a test uses to make a commit fail the way a dropped + // connection would, which is the only way to watch a retry happen. + .AddInterceptors(provider.GetServices())); services.AddHealthChecks() .AddCheck( diff --git a/tests/DodoSSH.Api.Tests/ApiFixture.cs b/tests/DodoSSH.Api.Tests/ApiFixture.cs index 0376b45..283dfb3 100644 --- a/tests/DodoSSH.Api.Tests/ApiFixture.cs +++ b/tests/DodoSSH.Api.Tests/ApiFixture.cs @@ -14,6 +14,7 @@ using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Testcontainers.PostgreSql; using Xunit; +using Microsoft.EntityFrameworkCore.Diagnostics; namespace DodoSSH.Api.Tests; @@ -108,9 +109,16 @@ public sealed class ApiFixture : WebApplicationFactory, IAsyncLifetime Environment.SetEnvironmentVariable(name, value); } - builder.ConfigureTestServices(services => services.AddSingleton(RelayNetwork)); + builder.ConfigureTestServices(services => + { + services.AddSingleton(RelayNetwork); + services.AddSingleton(Commits); + }); } + /// Fails a commit on request, so a test can watch the retrying execution strategy at work. + internal FlakyCommits Commits { get; } = new(); + /// /// What the relay resolves and dials, in place of the machine's own resolver and sockets. /// diff --git a/tests/DodoSSH.Api.Tests/FlakyCommits.cs b/tests/DodoSSH.Api.Tests/FlakyCommits.cs new file mode 100644 index 0000000..4a35e98 --- /dev/null +++ b/tests/DodoSSH.Api.Tests/FlakyCommits.cs @@ -0,0 +1,43 @@ +using System.Data.Common; +using Microsoft.EntityFrameworkCore.Diagnostics; + +namespace DodoSSH.Api.Tests; + +/// +/// Fails the next commit the way a connection dropped on the way to the server would, when asked to. +/// +/// +/// Before the commit reaches the database, so the transaction is rolled back — which is what a lost commit +/// looks like from the server's side — and with a , which the retrying +/// execution strategy treats as transient. That is the only way to watch a retry happen against a real +/// database, and the retry is the thing worth watching: see FreshAttempts. +/// +internal sealed class FlakyCommits : DbTransactionInterceptor +{ + private int armed; + + private int failed; + + /// How many commits this has failed. + internal int Failed => Volatile.Read(ref failed); + + /// Makes the next commit, whoever's it is, fail once. + internal void FailNextCommit() => Interlocked.Exchange(ref armed, 1); + + /// + public override ValueTask TransactionCommittingAsync( + DbTransaction transaction, + TransactionEventData eventData, + InterceptionResult result, + CancellationToken cancellationToken = default) + { + if (Interlocked.Exchange(ref armed, 0) == 1) + { + Interlocked.Increment(ref failed); + + throw new TimeoutException("The commit was lost on its way to the server."); + } + + return base.TransactionCommittingAsync(transaction, eventData, result, cancellationToken); + } +} diff --git a/tests/DodoSSH.Api.Tests/TeamEndpointTests.cs b/tests/DodoSSH.Api.Tests/TeamEndpointTests.cs index 1bcf22a..b9a2f36 100644 --- a/tests/DodoSSH.Api.Tests/TeamEndpointTests.cs +++ b/tests/DodoSSH.Api.Tests/TeamEndpointTests.cs @@ -647,6 +647,38 @@ public sealed class TeamEndpointTests(ApiFixture fixture) .Role.ShouldBe(TeamMemberRole.Owner); } + /// + /// The commit lost on its way to the database, which the execution strategy answers by running the + /// transaction again on the same context. That context still held the two memberships as the failed + /// attempt left them — already owner, already admin, and marked unchanged — so the retry saved nothing, + /// committed, and answered 204 over a team whose roles had not moved. See FreshAttempts. + /// + [Fact] + public async Task ATransferWhoseCommitIsLost_IsRetriedFromTheDatabase_AndStillLands() + { + var owner = await EnrolledClientAsync("retry-owner", "rowner@example.com"); + await EnrolledClientAsync("retry-successor", "rsuccessor@example.com"); + + var team = await CreateTeamAsync(owner, "Retried handover"); + var entry = await LookupAsync(owner, "rsuccessor@example.com"); + + await AddMemberAsync(owner, team.TeamId, entry.UserId, TeamMemberRole.Member); + + var failedBefore = fixture.Commits.Failed; + fixture.Commits.FailNextCommit(); + + var response = await owner.PostContractAsync( + OwnerUrl(team.TeamId), new TransferTeamOwnershipRequest(entry.UserId)); + + response.StatusCode.ShouldBe(HttpStatusCode.NoContent); + fixture.Commits.Failed.ShouldBe(failedBefore + 1, "the first commit has to have been lost for this to mean anything"); + + var members = await ReadAsync>(owner, MembersUrl(team.TeamId)); + + members.Where(row => row.UserId == entry.UserId).ShouldHaveSingleItem().Role.ShouldBe(TeamMemberRole.Owner); + members.Count(row => row.Role == TeamMemberRole.Owner).ShouldBe(1); + } + /// /// The thing that was impossible before this endpoint existed. An owner could not be removed and /// could not be demoted, so somebody who left the company owning a team left it owned by them for