Public Access
Audit follow-ups: keyboard, wording, performance, transfer, sync, retry and relay fixes #22
No files matched your search
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -0,0 +1,72 @@
|
||||
using DodoSSH.Infrastructure;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
|
||||
namespace DodoSSH.Api.Setup;
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <b>The strategy re-runs the delegate on the same context, and the context remembers the attempt that
|
||||
/// failed.</b> When the failure is the commit itself — a connection dropped after <c>SaveChanges</c> 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 <c>EnrolledAtUtc</c> that way, and a team hand-over
|
||||
/// its two role changes, each reporting success.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// 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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
internal static class FreshAttempts
|
||||
{
|
||||
/// <summary>Runs <paramref name="attempt"/> under the context's execution strategy, freshly each time.</summary>
|
||||
/// <param name="database">The request's context.</param>
|
||||
/// <param name="carried">Entities loaded before the transaction that the attempt reads or modifies.</param>
|
||||
/// <param name="attempt">The transaction, which begins and commits its own.</param>
|
||||
internal static Task<T> ExecuteFreshAsync<T>(
|
||||
this DodoDbContext database,
|
||||
IReadOnlyList<object> carried,
|
||||
Func<Task<T>> 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);
|
||||
});
|
||||
}
|
||||
|
||||
/// <inheritdoc cref="ExecuteFreshAsync{T}" />
|
||||
internal static Task ExecuteFreshAsync(
|
||||
this DodoDbContext database,
|
||||
IReadOnlyList<object> carried,
|
||||
Func<Task> attempt) =>
|
||||
database.ExecuteFreshAsync(
|
||||
carried,
|
||||
async () =>
|
||||
{
|
||||
await attempt().ConfigureAwait(false);
|
||||
return true;
|
||||
});
|
||||
}
|
||||
@@ -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<DodoDbContext>(options => options
|
||||
services.AddDbContext<DodoDbContext>((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<IInterceptor>()));
|
||||
|
||||
services.AddHealthChecks()
|
||||
.AddCheck<DatabaseHealthCheck>(
|
||||
|
||||
@@ -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<Program>, IAsyncLifetime
|
||||
Environment.SetEnvironmentVariable(name, value);
|
||||
}
|
||||
|
||||
builder.ConfigureTestServices(services => services.AddSingleton<IRelayNetwork>(RelayNetwork));
|
||||
builder.ConfigureTestServices(services =>
|
||||
{
|
||||
services.AddSingleton<IRelayNetwork>(RelayNetwork);
|
||||
services.AddSingleton<IInterceptor>(Commits);
|
||||
});
|
||||
}
|
||||
|
||||
/// <summary>Fails a commit on request, so a test can watch the retrying execution strategy at work.</summary>
|
||||
internal FlakyCommits Commits { get; } = new();
|
||||
|
||||
/// <summary>
|
||||
/// What the relay resolves and dials, in place of the machine's own resolver and sockets.
|
||||
/// </summary>
|
||||
|
||||
@@ -0,0 +1,43 @@
|
||||
using System.Data.Common;
|
||||
using Microsoft.EntityFrameworkCore.Diagnostics;
|
||||
|
||||
namespace DodoSSH.Api.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Fails the next commit the way a connection dropped on the way to the server would, when asked to.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// 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 <see cref="TimeoutException"/>, 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 <c>FreshAttempts</c>.
|
||||
/// </remarks>
|
||||
internal sealed class FlakyCommits : DbTransactionInterceptor
|
||||
{
|
||||
private int armed;
|
||||
|
||||
private int failed;
|
||||
|
||||
/// <summary>How many commits this has failed.</summary>
|
||||
internal int Failed => Volatile.Read(ref failed);
|
||||
|
||||
/// <summary>Makes the next commit, whoever's it is, fail once.</summary>
|
||||
internal void FailNextCommit() => Interlocked.Exchange(ref armed, 1);
|
||||
|
||||
/// <inheritdoc />
|
||||
public override ValueTask<InterceptionResult> 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);
|
||||
}
|
||||
}
|
||||
@@ -647,6 +647,38 @@ public sealed class TeamEndpointTests(ApiFixture fixture)
|
||||
.Role.ShouldBe(TeamMemberRole.Owner);
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// 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 <c>FreshAttempts</c>.
|
||||
/// </remarks>
|
||||
[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<IReadOnlyList<TeamMemberSummary>>(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);
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// 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
|
||||
|
||||
Reference in new issue
Block a user