From 49135e7227c18b2a807d17e5cac729412129c965 Mon Sep 17 00:00:00 2001 From: Daniel Bohlin Date: Mon, 11 May 2026 13:55:32 +0200 Subject: [PATCH 1/2] fix: race-proof user creation + unique Identity index + collection extension point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves Tharga/Platform#65 — closes the check-then-act race in UserServiceRepositoryBase.GetUserAsync that produces duplicate UserEntity rows when two near-simultaneous first-time logins for the same identity hit a fresh per-environment User collection. - Added unique index on User.Identity declared in UserRepositoryCollection.Indices. - Added MongoWriteException/DuplicateKey catch in UserServiceRepositoryBase.GetUserAsync: on conflict, re-read by Identity and return the winning row. - Promoted UserRepositoryCollection to public so consumers can subclass it to declare per-deployment indices. - Added ThargaTeamOptions.RegisterUserRepository() overload for registering the consumer subclass. New Tharga.Team.MongoDB.Tests project with 7 tests. 296 / 296 passing. Migration note for the PR: existing deployments with duplicate User rows must dedupe before upgrading; the unique index will fail to create against conflicting data and Tharga.MongoDB's AssureIndex surfaces this in startup logs. --- Tharga.Platform.sln | 14 ++ .../RegisterUserRepositoryTests.cs | 52 +++++++ .../Tharga.Team.MongoDB.Tests.csproj | 27 ++++ .../UserRepositoryCollectionIndicesTests.cs | 37 +++++ .../UserServiceRepositoryBaseRaceTests.cs | 139 ++++++++++++++++++ Tharga.Team.MongoDB/README.md | 26 ++++ Tharga.Team.MongoDB/ThargaTeamOptions.cs | 20 +++ Tharga.Team.MongoDB/ThargaTeamRegistration.cs | 3 +- .../UserRepositoryCollection.cs | 15 +- .../UserServiceRepositoryBase.cs | 19 ++- plan/feature.md | 46 ++++++ plan/plan.md | 84 +++++++++++ 12 files changed, 475 insertions(+), 7 deletions(-) create mode 100644 Tharga.Team.MongoDB.Tests/RegisterUserRepositoryTests.cs create mode 100644 Tharga.Team.MongoDB.Tests/Tharga.Team.MongoDB.Tests.csproj create mode 100644 Tharga.Team.MongoDB.Tests/UserRepositoryCollectionIndicesTests.cs create mode 100644 Tharga.Team.MongoDB.Tests/UserServiceRepositoryBaseRaceTests.cs create mode 100644 plan/feature.md create mode 100644 plan/plan.md diff --git a/Tharga.Platform.sln b/Tharga.Platform.sln index fd4f33d..1851820 100644 --- a/Tharga.Platform.sln +++ b/Tharga.Platform.sln @@ -30,6 +30,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Tharga.Platform.Mcp", "Thar EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Tharga.Platform.Mcp.Tests", "Tharga.Platform.Mcp.Tests\Tharga.Platform.Mcp.Tests.csproj", "{32F3F58A-486D-44FB-AAD1-DF6CB19100B0}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Tharga.Team.MongoDB.Tests", "Tharga.Team.MongoDB.Tests\Tharga.Team.MongoDB.Tests.csproj", "{F53219A8-CB6D-423C-9314-1397F08E1202}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -148,6 +150,18 @@ Global {32F3F58A-486D-44FB-AAD1-DF6CB19100B0}.Release|x64.Build.0 = Release|Any CPU {32F3F58A-486D-44FB-AAD1-DF6CB19100B0}.Release|x86.ActiveCfg = Release|Any CPU {32F3F58A-486D-44FB-AAD1-DF6CB19100B0}.Release|x86.Build.0 = Release|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Debug|Any CPU.Build.0 = Debug|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Debug|x64.ActiveCfg = Debug|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Debug|x64.Build.0 = Debug|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Debug|x86.ActiveCfg = Debug|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Debug|x86.Build.0 = Debug|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Release|Any CPU.ActiveCfg = Release|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Release|Any CPU.Build.0 = Release|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Release|x64.ActiveCfg = Release|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Release|x64.Build.0 = Release|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Release|x86.ActiveCfg = Release|Any CPU + {F53219A8-CB6D-423C-9314-1397F08E1202}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/Tharga.Team.MongoDB.Tests/RegisterUserRepositoryTests.cs b/Tharga.Team.MongoDB.Tests/RegisterUserRepositoryTests.cs new file mode 100644 index 0000000..f397c18 --- /dev/null +++ b/Tharga.Team.MongoDB.Tests/RegisterUserRepositoryTests.cs @@ -0,0 +1,52 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Tharga.MongoDB; + +namespace Tharga.Team.MongoDB.Tests; + +/// +/// Verifies the consumer-facing index extension point added under Tharga/Platform#65: +/// registers a +/// subclass of as the implementation of +/// , so consumers can declare per-deployment indices. +/// +/// We inspect the on the registered +/// rather than building the provider, because the underlying DiskRepositoryCollectionBase +/// ctor casts the injected factory to a concrete type that a test substitute can't satisfy. +/// +public class RegisterUserRepositoryTests +{ + [Fact] + public void RegisterUserRepository_Default_Overload_Registers_Builtin_Collection() + { + var services = new ServiceCollection(); + services.AddThargaTeamRepository(o => o.RegisterUserRepository()); + + var descriptor = services.Single(s => + s.ServiceType == typeof(IUserRepositoryCollection)); + + Assert.Equal( + typeof(UserRepositoryCollection), + descriptor.ImplementationType); + } + + [Fact] + public void RegisterUserRepository_Subclass_Overload_Registers_Consumer_Subclass() + { + var services = new ServiceCollection(); + services.AddThargaTeamRepository(o => + o.RegisterUserRepository()); + + var descriptor = services.Single(s => + s.ServiceType == typeof(IUserRepositoryCollection)); + + Assert.Equal(typeof(CustomCollection), descriptor.ImplementationType); + } + + public class CustomCollection : UserRepositoryCollection + { + public CustomCollection(IMongoDbServiceFactory mongoDbServiceFactory, ILogger> logger, IOptions options = null) + : base(mongoDbServiceFactory, logger, options) { } + } +} diff --git a/Tharga.Team.MongoDB.Tests/Tharga.Team.MongoDB.Tests.csproj b/Tharga.Team.MongoDB.Tests/Tharga.Team.MongoDB.Tests.csproj new file mode 100644 index 0000000..f7f0794 --- /dev/null +++ b/Tharga.Team.MongoDB.Tests/Tharga.Team.MongoDB.Tests.csproj @@ -0,0 +1,27 @@ + + + net10.0 + enable + false + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + + + + + + + diff --git a/Tharga.Team.MongoDB.Tests/UserRepositoryCollectionIndicesTests.cs b/Tharga.Team.MongoDB.Tests/UserRepositoryCollectionIndicesTests.cs new file mode 100644 index 0000000..4da4bbf --- /dev/null +++ b/Tharga.Team.MongoDB.Tests/UserRepositoryCollectionIndicesTests.cs @@ -0,0 +1,37 @@ +using System.Runtime.CompilerServices; +using MongoDB.Bson; +using MongoDB.Bson.Serialization; +using MongoDB.Driver; + +namespace Tharga.Team.MongoDB.Tests; + +/// +/// Verifies that declares the +/// unique Identity index added under Tharga/Platform#65. +/// +/// The base DiskRepositoryCollectionBase ctor casts the injected factory to a concrete +/// MongoDbService, which an NSubstitute proxy can't satisfy. Since the override is purely +/// declarative and doesn't depend on instance state, we bypass the ctor via +/// . +/// +public class UserRepositoryCollectionIndicesTests +{ + [Fact] + public void Indices_Includes_Unique_Identity_Index() + { + var collection = (UserRepositoryCollection) + RuntimeHelpers.GetUninitializedObject(typeof(UserRepositoryCollection)); + + var indices = collection.Indices.ToArray(); + + var identityIndex = Assert.Single(indices); + Assert.Equal("Identity", identityIndex.Options.Name); + Assert.True(identityIndex.Options.Unique); + + var keyDoc = identityIndex.Keys.Render(new RenderArgs( + BsonSerializer.SerializerRegistry.GetSerializer(), + BsonSerializer.SerializerRegistry)); + Assert.True(keyDoc.Contains("Identity")); + Assert.Equal(1, keyDoc["Identity"].AsInt32); + } +} diff --git a/Tharga.Team.MongoDB.Tests/UserServiceRepositoryBaseRaceTests.cs b/Tharga.Team.MongoDB.Tests/UserServiceRepositoryBaseRaceTests.cs new file mode 100644 index 0000000..2c974a5 --- /dev/null +++ b/Tharga.Team.MongoDB.Tests/UserServiceRepositoryBaseRaceTests.cs @@ -0,0 +1,139 @@ +using System.Reflection; +using System.Runtime.CompilerServices; +using System.Security.Claims; +using Microsoft.AspNetCore.Components.Authorization; +using MongoDB.Bson; +using MongoDB.Driver; +using Tharga.MongoDB; + +namespace Tharga.Team.MongoDB.Tests; + +/// +/// Verifies the catch-DuplicateKey recovery path in . +/// When two first-time logins for the same Identity race, the unique Identity index on +/// guarantees one wins. The losing thread catches +/// the with and re-reads +/// the winning row instead of throwing (issue Tharga/Platform#65). +/// +public class UserServiceRepositoryBaseRaceTests +{ + [Fact] + public async Task GetUserAsync_NoExistingUser_AddsAndReturnsCandidate() + { + var repo = Substitute.For>(); + repo.GetAsync("alice@example.com").Returns((TestUserEntity)null); + repo.AddAsync(Arg.Any()).Returns(Task.CompletedTask); + + var sut = new TestUserService(repo, new TestUserEntity { Identity = "alice@example.com", Key = "u-alice" }); + var result = await sut.InvokeGetUserAsync(BuildClaims("alice@example.com")); + + Assert.NotNull(result); + Assert.Equal("u-alice", result.Key); + await repo.Received(1).AddAsync(Arg.Any()); + } + + [Fact] + public async Task GetUserAsync_ExistingUser_DoesNotAdd() + { + var existing = new TestUserEntity { Identity = "bob@example.com", Key = "u-bob" }; + var repo = Substitute.For>(); + repo.GetAsync("bob@example.com").Returns(existing); + + var sut = new TestUserService(repo, new TestUserEntity { Identity = "bob@example.com", Key = "should-not-be-used" }); + var result = await sut.InvokeGetUserAsync(BuildClaims("bob@example.com")); + + Assert.Same(existing, result); + await repo.DidNotReceive().AddAsync(Arg.Any()); + } + + [Fact] + public async Task GetUserAsync_AddThrowsDuplicateKey_ReReadsAndReturnsWinner() + { + // Race: first GetAsync returns null, candidate is created, AddAsync hits the unique-Identity + // index conflict and throws. Recovery re-reads by Identity and returns the winner. + var winner = new TestUserEntity { Identity = "carol@example.com", Key = "u-winner" }; + var repo = Substitute.For>(); + repo.GetAsync("carol@example.com").Returns((TestUserEntity)null, winner); + repo.AddAsync(Arg.Any()).Returns(_ => throw BuildDuplicateKeyException()); + + var sut = new TestUserService(repo, new TestUserEntity { Identity = "carol@example.com", Key = "u-loser" }); + var result = await sut.InvokeGetUserAsync(BuildClaims("carol@example.com")); + + Assert.Same(winner, result); + await repo.Received(2).GetAsync("carol@example.com"); + await repo.Received(1).AddAsync(Arg.Any()); + } + + [Fact] + public async Task GetUserAsync_AddThrowsNonDuplicateKey_Propagates() + { + var repo = Substitute.For>(); + repo.GetAsync("dave@example.com").Returns((TestUserEntity)null); + repo.AddAsync(Arg.Any()).Returns(_ => throw new InvalidOperationException("unrelated")); + + var sut = new TestUserService(repo, new TestUserEntity { Identity = "dave@example.com", Key = "u-dave" }); + + await Assert.ThrowsAsync(() => sut.InvokeGetUserAsync(BuildClaims("dave@example.com"))); + } + + private static ClaimsPrincipal BuildClaims(string identity) + { + // Tharga.Toolkit's GetIdentity().Identity reads ClaimTypes.NameIdentifier off the principal. + var claims = new[] { new Claim(ClaimTypes.NameIdentifier, identity) }; + return new ClaimsPrincipal(new ClaimsIdentity(claims, "test")); + } + + /// + /// Construct a carrying a with + /// via reflection. Both types have only + /// internal constructors in MongoDB.Driver 3.x; we bypass them via + /// and set backing fields directly. + /// Fragile across driver upgrades but sufficient for a single test fixture. + /// + private static MongoWriteException BuildDuplicateKeyException() + { + var writeError = (WriteError)RuntimeHelpers.GetUninitializedObject(typeof(WriteError)); + SetField(writeError, "_category", ServerErrorCategory.DuplicateKey); + SetField(writeError, "_code", 11000); + SetField(writeError, "_message", "E11000 duplicate key error"); + SetField(writeError, "_details", new BsonDocument()); + + var exception = (MongoWriteException)RuntimeHelpers.GetUninitializedObject(typeof(MongoWriteException)); + SetField(exception, "_writeError", writeError); + return exception; + } + + private static void SetField(object instance, string fieldName, object value) + { + var field = instance.GetType().GetField(fieldName, BindingFlags.NonPublic | BindingFlags.Instance); + if (field == null) throw new InvalidOperationException($"Field '{fieldName}' not found on {instance.GetType().FullName}. The MongoDB.Driver internal layout may have changed."); + field.SetValue(instance, value); + } + + public record TestUserEntity : EntityBase, IUser + { + public string Identity { get; init; } + public string Key { get; init; } + public string EMail { get; init; } + public string Name { get; init; } + } + + private sealed class TestUserService : UserServiceRepositoryBase + { + private readonly TestUserEntity _candidate; + + public TestUserService(IUserRepository repo, TestUserEntity candidate) + : base(Substitute.For(), repo) + { + _candidate = candidate; + } + + protected override Task CreateUserEntityAsync(ClaimsPrincipal claimsPrincipal, string identity) + { + return Task.FromResult(_candidate); + } + + // Expose the protected method for direct invocation in tests. + public Task InvokeGetUserAsync(ClaimsPrincipal principal) => GetUserAsync(principal); + } +} diff --git a/Tharga.Team.MongoDB/README.md b/Tharga.Team.MongoDB/README.md index f7716c5..61d591a 100644 --- a/Tharga.Team.MongoDB/README.md +++ b/Tharga.Team.MongoDB/README.md @@ -26,6 +26,32 @@ builder.Services.AddThargaTeamRepository(o => Requires [Tharga.MongoDB](https://www.nuget.org/packages/Tharga.MongoDB) to be configured with a connection string. +## Adding per-deployment User indices + +The built-in `UserRepositoryCollection` declares a unique index on `Identity`. To add additional indices (e.g. on a custom email column), subclass the collection and register the subclass via the `RegisterUserRepository` overload: + +```csharp +public class MyUserRepositoryCollection : UserRepositoryCollection +{ + public MyUserRepositoryCollection(IMongoDbServiceFactory factory, ILogger> logger, IOptions options = null) + : base(factory, logger, options) { } + + public override IEnumerable> Indices => + [ + // Keep the base Identity index + ..base.Indices, + // Plus your own + new(Builders.IndexKeys.Ascending(x => x.EMail), + new CreateIndexOptions { Unique = true, Name = "EMail" }) + ]; +} + +builder.Services.AddThargaTeamRepository(o => +{ + o.RegisterUserRepository(); +}); +``` + ## Dependencies - [Tharga.Team](https://www.nuget.org/packages/Tharga.Team) - Domain models and service abstractions. diff --git a/Tharga.Team.MongoDB/ThargaTeamOptions.cs b/Tharga.Team.MongoDB/ThargaTeamOptions.cs index 2a430be..ba6cb21 100644 --- a/Tharga.Team.MongoDB/ThargaTeamOptions.cs +++ b/Tharga.Team.MongoDB/ThargaTeamOptions.cs @@ -5,6 +5,7 @@ namespace Tharga.Team.MongoDB; public record ThargaTeamOptions { internal Type _userEntity; + internal Type _userCollectionType; internal Type _teamEntity; internal Type _teamMemberModel; @@ -18,10 +19,29 @@ public record ThargaTeamOptions /// public string UserCollectionName { get; set; } = "User"; + /// + /// Registers the User repository using the built-in . + /// Use the RegisterUserRepository<TUserEntity, TCollection> overload to register a consumer + /// subclass that declares additional per-deployment indices. + /// public void RegisterUserRepository() where TUserEntity : EntityBase, IUser { _userEntity = typeof(TUserEntity); + _userCollectionType = null; + } + + /// + /// Registers the User repository with a consumer-provided collection subclass. + /// Use this when you need to add per-deployment indices on top of the built-in + /// unique Identity index (e.g. a unique index on a custom email field). + /// + public void RegisterUserRepository() + where TUserEntity : EntityBase, IUser + where TCollection : UserRepositoryCollection + { + _userEntity = typeof(TUserEntity); + _userCollectionType = typeof(TCollection); } public void RegisterTeamRepository() diff --git a/Tharga.Team.MongoDB/ThargaTeamRegistration.cs b/Tharga.Team.MongoDB/ThargaTeamRegistration.cs index 019ace1..9d74888 100644 --- a/Tharga.Team.MongoDB/ThargaTeamRegistration.cs +++ b/Tharga.Team.MongoDB/ThargaTeamRegistration.cs @@ -21,7 +21,8 @@ public static void AddThargaTeamRepository(this IServiceCollection services, Act var userRepositoryImplementationType = typeof(UserRepository<>).MakeGenericType(userEntityType); var userRepositoryCollectionInterfaceType = typeof(IUserRepositoryCollection<>).MakeGenericType(userEntityType); - var userRepositoryCollectionImplementationType = typeof(UserRepositoryCollection<>).MakeGenericType(userEntityType); + var userRepositoryCollectionImplementationType = o._userCollectionType + ?? typeof(UserRepositoryCollection<>).MakeGenericType(userEntityType); services.AddTransient(userRepositoryInterfaceType, userRepositoryImplementationType); services.AddTransient(userRepositoryCollectionInterfaceType, userRepositoryCollectionImplementationType); diff --git a/Tharga.Team.MongoDB/UserRepositoryCollection.cs b/Tharga.Team.MongoDB/UserRepositoryCollection.cs index e8bed6d..68a7c8f 100644 --- a/Tharga.Team.MongoDB/UserRepositoryCollection.cs +++ b/Tharga.Team.MongoDB/UserRepositoryCollection.cs @@ -1,11 +1,18 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using MongoDB.Bson; +using MongoDB.Driver; using Tharga.MongoDB; using Tharga.MongoDB.Disk; namespace Tharga.Team.MongoDB; -internal class UserRepositoryCollection : DiskRepositoryCollectionBase, IUserRepositoryCollection +/// +/// MongoDB collection definition for User documents. Public so consumers can subclass it +/// to declare per-deployment indices on their own shape. +/// Register the subclass via . +/// +public class UserRepositoryCollection : DiskRepositoryCollectionBase, IUserRepositoryCollection where TUserEntity : EntityBase, IUser { private readonly string _collectionName; @@ -17,4 +24,10 @@ public UserRepositoryCollection(IMongoDbServiceFactory mongoDbServiceFactory, IL } public override string CollectionName => _collectionName; + + public override IEnumerable> Indices => + [ + new(Builders.IndexKeys.Ascending(x => x.Identity), + new CreateIndexOptions { Unique = true, Name = "Identity" }) + ]; } diff --git a/Tharga.Team.MongoDB/UserServiceRepositoryBase.cs b/Tharga.Team.MongoDB/UserServiceRepositoryBase.cs index e11aea9..5cd3021 100644 --- a/Tharga.Team.MongoDB/UserServiceRepositoryBase.cs +++ b/Tharga.Team.MongoDB/UserServiceRepositoryBase.cs @@ -1,4 +1,5 @@ using Microsoft.AspNetCore.Components.Authorization; +using MongoDB.Driver; using System.Security.Claims; using Tharga.MongoDB; using Tharga.Toolkit; @@ -23,13 +24,21 @@ protected override async Task GetUserAsync(ClaimsPrincipal claimsPrincipa var identity = claimsPrincipal.GetIdentity().Identity; var user = await _userRepository.GetAsync(identity); - if (user == null) + if (user != null) return user; + + var candidate = await CreateUserEntityAsync(claimsPrincipal, identity); + try { - user = await CreateUserEntityAsync(claimsPrincipal, identity); - await _userRepository.AddAsync(user); + await _userRepository.AddAsync(candidate); + return candidate; + } + catch (MongoWriteException ex) when (ex.WriteError?.Category == ServerErrorCategory.DuplicateKey) + { + // Lost the race against a concurrent first-time login for the same identity. + // The unique Identity index on UserRepositoryCollection guarantees only one wins; + // re-read and return the winner. Issue Tharga/Platform#65. + return await _userRepository.GetAsync(identity); } - - return user; } protected override IAsyncEnumerable GetAllAsync() diff --git a/plan/feature.md b/plan/feature.md new file mode 100644 index 0000000..8943245 --- /dev/null +++ b/plan/feature.md @@ -0,0 +1,46 @@ +# Feature: User upsert race fix + +GitHub issue: [Tharga/Platform#65](https://github.com/Tharga/Platform/issues/65) + +## Goal + +Close the check-then-act race in `UserServiceRepositoryBase.GetUserAsync` that produces duplicate `UserEntity` rows when two near-simultaneous first-time logins for the same identity hit a fresh per-environment User collection. The race surfaces as two user-visible failures across every Tharga.Team consumer: + +1. `NavMenu` / any path through `UserServiceBase.GetUserAsync` throws `InvalidOperationException: Sequence contains more than one element` (from `SingleOrDefaultAsync` in the underlying `_collection.GetOneAsync`). +2. The Tharga.Team.Blazor `/developer/user` admin page crashes — RadzenDataGrid rejects two rows with the same Key in the render tree. + +Quilt4Net.Server has shipped downstream mitigations (catch-and-recover in `UserService.GetUserAsync`, a `MongoWriteException`/`DuplicateKey` catch on the same path, and a manual `UserIdentityIndexHostedService` that creates the unique index out-of-band) — but none of those prevent the *creating* race. The upstream fix landing here removes the need for every consumer to carry its own workaround. + +## Scope + +Three coordinated pieces, all in `Tharga.Team.MongoDB`: + +1. **Unique index on `User.Identity`** declared on `UserRepositoryCollection.Indices` so Tharga.MongoDB's `AssureIndex` creates and maintains it. After this, MongoDB rejects the racing `AddAsync` with a `MongoWriteException` of category `DuplicateKey`. +2. **Race-proof `GetUserAsync`** in `UserServiceRepositoryBase`: catch `MongoWriteException` with `WriteError.Category == ServerErrorCategory.DuplicateKey` from `AddAsync`, re-read by Identity, and return the winning row. The unique index is the load-bearing safety net; the catch is the recovery path. (Approach selected per user direction over `FindOneAndUpdate $setOnInsert`; both close the race, this one is significantly simpler.) +3. **Consumer-facing index extension point.** Today `UserRepositoryCollection` is `internal` and consumers cannot subclass it to declare per-deployment indices. Promote it to `public`; the `Indices` member is already `override` on the base. Add a new `RegisterUserRepository()` overload to `ThargaTeamOptions` that accepts an explicit collection-type override. The default `RegisterUserRepository()` continues to register the built-in collection class. + +## Behaviour + +- **No data migration runs as part of this fix.** Existing deployments that already have duplicate User rows must dedupe before deploying — Tharga.MongoDB's `AssureIndex` will surface the conflict in startup logs and refuse to create the index until the data is clean. This is called out in the PR description so admins see it. (Decision per user direction over a built-in dedupe-on-startup migration.) +- The race fix is correctness-only — no behaviour change for non-racing callers. Consumers do not need to touch their `UserService` override after upgrading (Quilt4Net.Server's downstream mitigations become no-ops once the upstream is in place; they can be removed in a separate consumer-side bump). + +## Acceptance criteria + +1. `UserRepositoryCollection.Indices` declares a unique index on `Identity`. +2. `UserServiceRepositoryBase.GetUserAsync` catches `MongoWriteException` with `DuplicateKey` category from `AddAsync`, re-reads by Identity, returns the winning row. +3. `UserRepositoryCollection` is `public`; `Indices` can be overridden by a consumer subclass. +4. `ThargaTeamOptions.RegisterUserRepository()` overload exists; the DI registration in `ThargaTeamRegistration.AddThargaTeamRepository` uses the configured collection type. +5. New unit tests: (a) duplicate-key add → recovery returns the winning row from a fake repository, (b) the new `RegisterUserRepository` overload plumbs through to DI (the registered `IUserRepositoryCollection<>` resolves to the consumer's subclass), (c) `UserRepositoryCollection<>.Indices` contains the expected `Identity` unique index. +6. `dotnet build -c Release` and `dotnet test -c Release` both green. + +## Done condition + +PR opened from `feature/user-upsert-race-fix` → `master`, all CI checks green, user has confirmed. Once the next `Tharga.Team.*` release is consumed in Quilt4Net.Server, its `UserIdentityIndexHostedService` + `MongoWriteException` catch in `UserService.GetUserAsync` can be removed (tracked separately, not part of this feature). + +## Out of scope + +- `FindOneAndUpdate $setOnInsert` upsert path — both approaches close the race; catch-DuplicateKey is the chosen minimal change. +- Built-in dedupe migration — admins clean up existing duplicates manually. The Tharga.MongoDB `AssureIndex` startup error is the surface. +- Symmetric fix for `TeamRepository` user-key races — separate scope; the Team collection already has `UniqueTeamMemberKey` index per `TeamRepositoryCollection.Indices`. +- Framework package bumps (Microsoft.AspNetCore.* 9.0.15 → 10.0.7) — deferred, separate PR. +- Consumer-side cleanup in Quilt4Net.Server — separate downstream bump after this lands. diff --git a/plan/plan.md b/plan/plan.md new file mode 100644 index 0000000..35002d8 --- /dev/null +++ b/plan/plan.md @@ -0,0 +1,84 @@ +# Plan: User upsert race fix + +## Steps + +- [x] **1. Unique index on `User.Identity`** + - Override `Indices` on `UserRepositoryCollection` (currently inherits the empty default from `DiskRepositoryCollectionBase<>`): + ```csharp + public override IEnumerable> Indices => + [ + new(Builders.IndexKeys.Ascending(x => x.Identity), + new CreateIndexOptions { Unique = true, Name = "Identity" }) + ]; + ``` + - Verify `IUser.Identity` is a property reachable via `x => x.Identity` (it is — defined on `IUser` in `Tharga.Team/IUser.cs`). + +- [x] **2. Catch-DuplicateKey recovery in `UserServiceRepositoryBase.GetUserAsync`** + - In `Tharga.Team.MongoDB/UserServiceRepositoryBase.cs`, wrap the `AddAsync` call: + ```csharp + protected override async Task GetUserAsync(ClaimsPrincipal claimsPrincipal) + { + var identity = claimsPrincipal.GetIdentity().Identity; + + var user = await _userRepository.GetAsync(identity); + if (user != null) return user; + + var candidate = await CreateUserEntityAsync(claimsPrincipal, identity); + try + { + await _userRepository.AddAsync(candidate); + return candidate; + } + catch (MongoWriteException ex) when (ex.WriteError?.Category == ServerErrorCategory.DuplicateKey) + { + // Lost the race; another request inserted first. Return the winning row. + return await _userRepository.GetAsync(identity); + } + } + ``` + - `MongoDB.Driver` is already a transitive dependency via `Tharga.MongoDB` — no new package reference needed. + +- [x] **3. Promote `UserRepositoryCollection` to `public`** + - Change the class declaration from `internal class` to `public class`. + - `Indices` is already `override` on the base type → already virtual; consumers can subclass and override. + +- [x] **4. `RegisterUserRepository` overload in `ThargaTeamOptions`** + - Add an internal `Type _userCollectionType` field (defaults to null). + - Existing `RegisterUserRepository()` continues to set `_userEntity = typeof(TUserEntity)` and leaves `_userCollectionType = null` (registration falls back to the built-in collection class). + - New overload: + ```csharp + public void RegisterUserRepository() + where TUserEntity : EntityBase, IUser + where TCollection : UserRepositoryCollection + { + _userEntity = typeof(TUserEntity); + _userCollectionType = typeof(TCollection); + } + ``` + - In `ThargaTeamRegistration.AddThargaTeamRepository`, the `userRepositoryCollectionImplementationType` becomes `o._userCollectionType ?? typeof(UserRepositoryCollection<>).MakeGenericType(userEntityType)`. + +- [x] **5. Tests** — New `Tharga.Team.MongoDB.Tests` project created with 7 tests (4 race-recovery + 2 registration + 1 index declaration). All passing. + - `UserServiceRepositoryBase`/`GetUserAsync` race-recovery test: build a `TestUserService` derivative whose `IUserRepository<>` is a fake that throws `MongoWriteException` (DuplicateKey) on first `AddAsync` and returns a known winning row on subsequent `GetAsync`. Assert the service returns the winning row, not the candidate. Construct `MongoWriteException` via its public constructor (or reflection if the `WriteError`/`Category` fields are read-only) — verify the reachable surface first. + - `RegisterUserRepository` plumbing test: register a derived collection type via the new overload, build an `IServiceProvider`, resolve `IUserRepositoryCollection`, assert the runtime type is the consumer subclass. + - `UserRepositoryCollection.Indices` declaration test: instantiate a `UserRepositoryCollection`, read `Indices`, assert one entry with `Unique = true`, `Name = "Identity"`, and the key path is `Identity`. + - Existing `Tharga.Team.Blazor.Tests` test `UserServiceBaseDefaultsTests` already covers `UserServiceBase.GetCurrentUserAsync` cache behaviour — confirm it still passes. + +- [x] **6. Build + test** — solution builds clean; 296 / 296 tests passing (289 before + 7 new). + - `dotnet build c:/dev/tharga/Toolkit/Platform/Tharga.Platform.sln -c Release` — 0 warnings, 0 errors. + - `dotnet test c:/dev/tharga/Toolkit/Platform/Tharga.Platform.sln -c Release` — full suite green. + +- [x] **7. README check** — `Tharga.Team.MongoDB/README.md` updated with a "Adding per-deployment User indices" section showing the subclass + `RegisterUserRepository` pattern. + - Per shared-instructions, skip README updates if the feature has no consumer-visible surface. This feature does add a public extension point (`UserRepositoryCollection<>` is now subclassable, new `RegisterUserRepository` overload). Decide whether `Tharga.Team.MongoDB/README.md` should mention the new extension point briefly. The migration prerequisite (dedupe before deploy) goes in the PR description, not the README. + +- [ ] **8. Commit + push the feature branch** + - Conventional commit prefix: `fix:` — this resolves a bug. + - Suggested message: `fix: race-proof user creation + unique Identity index + collection extension point`. + +- [ ] **9. Pause for user verification** (per the principle: plan/ stays on the feature branch through Completing; deleted in the close-out commit before PR opens). + +## Open questions + +(none — design choices locked via `AskUserQuestion` during planning) + +## Last session +2026-05-11 — All implementation steps complete. 7 new tests in a new `Tharga.Team.MongoDB.Tests` project; 296 / 296 passing. README extension-point section added. Ready for commit + push + user verification. From e08b4b142badec3509a4088f80345074a4d07094 Mon Sep 17 00:00:00 2001 From: Daniel Bohlin Date: Mon, 11 May 2026 14:00:27 +0200 Subject: [PATCH 2/2] fix: user-upsert-race-fix complete --- plan/feature.md | 46 --------------------------- plan/plan.md | 84 ------------------------------------------------- 2 files changed, 130 deletions(-) delete mode 100644 plan/feature.md delete mode 100644 plan/plan.md diff --git a/plan/feature.md b/plan/feature.md deleted file mode 100644 index 8943245..0000000 --- a/plan/feature.md +++ /dev/null @@ -1,46 +0,0 @@ -# Feature: User upsert race fix - -GitHub issue: [Tharga/Platform#65](https://github.com/Tharga/Platform/issues/65) - -## Goal - -Close the check-then-act race in `UserServiceRepositoryBase.GetUserAsync` that produces duplicate `UserEntity` rows when two near-simultaneous first-time logins for the same identity hit a fresh per-environment User collection. The race surfaces as two user-visible failures across every Tharga.Team consumer: - -1. `NavMenu` / any path through `UserServiceBase.GetUserAsync` throws `InvalidOperationException: Sequence contains more than one element` (from `SingleOrDefaultAsync` in the underlying `_collection.GetOneAsync`). -2. The Tharga.Team.Blazor `/developer/user` admin page crashes — RadzenDataGrid rejects two rows with the same Key in the render tree. - -Quilt4Net.Server has shipped downstream mitigations (catch-and-recover in `UserService.GetUserAsync`, a `MongoWriteException`/`DuplicateKey` catch on the same path, and a manual `UserIdentityIndexHostedService` that creates the unique index out-of-band) — but none of those prevent the *creating* race. The upstream fix landing here removes the need for every consumer to carry its own workaround. - -## Scope - -Three coordinated pieces, all in `Tharga.Team.MongoDB`: - -1. **Unique index on `User.Identity`** declared on `UserRepositoryCollection.Indices` so Tharga.MongoDB's `AssureIndex` creates and maintains it. After this, MongoDB rejects the racing `AddAsync` with a `MongoWriteException` of category `DuplicateKey`. -2. **Race-proof `GetUserAsync`** in `UserServiceRepositoryBase`: catch `MongoWriteException` with `WriteError.Category == ServerErrorCategory.DuplicateKey` from `AddAsync`, re-read by Identity, and return the winning row. The unique index is the load-bearing safety net; the catch is the recovery path. (Approach selected per user direction over `FindOneAndUpdate $setOnInsert`; both close the race, this one is significantly simpler.) -3. **Consumer-facing index extension point.** Today `UserRepositoryCollection` is `internal` and consumers cannot subclass it to declare per-deployment indices. Promote it to `public`; the `Indices` member is already `override` on the base. Add a new `RegisterUserRepository()` overload to `ThargaTeamOptions` that accepts an explicit collection-type override. The default `RegisterUserRepository()` continues to register the built-in collection class. - -## Behaviour - -- **No data migration runs as part of this fix.** Existing deployments that already have duplicate User rows must dedupe before deploying — Tharga.MongoDB's `AssureIndex` will surface the conflict in startup logs and refuse to create the index until the data is clean. This is called out in the PR description so admins see it. (Decision per user direction over a built-in dedupe-on-startup migration.) -- The race fix is correctness-only — no behaviour change for non-racing callers. Consumers do not need to touch their `UserService` override after upgrading (Quilt4Net.Server's downstream mitigations become no-ops once the upstream is in place; they can be removed in a separate consumer-side bump). - -## Acceptance criteria - -1. `UserRepositoryCollection.Indices` declares a unique index on `Identity`. -2. `UserServiceRepositoryBase.GetUserAsync` catches `MongoWriteException` with `DuplicateKey` category from `AddAsync`, re-reads by Identity, returns the winning row. -3. `UserRepositoryCollection` is `public`; `Indices` can be overridden by a consumer subclass. -4. `ThargaTeamOptions.RegisterUserRepository()` overload exists; the DI registration in `ThargaTeamRegistration.AddThargaTeamRepository` uses the configured collection type. -5. New unit tests: (a) duplicate-key add → recovery returns the winning row from a fake repository, (b) the new `RegisterUserRepository` overload plumbs through to DI (the registered `IUserRepositoryCollection<>` resolves to the consumer's subclass), (c) `UserRepositoryCollection<>.Indices` contains the expected `Identity` unique index. -6. `dotnet build -c Release` and `dotnet test -c Release` both green. - -## Done condition - -PR opened from `feature/user-upsert-race-fix` → `master`, all CI checks green, user has confirmed. Once the next `Tharga.Team.*` release is consumed in Quilt4Net.Server, its `UserIdentityIndexHostedService` + `MongoWriteException` catch in `UserService.GetUserAsync` can be removed (tracked separately, not part of this feature). - -## Out of scope - -- `FindOneAndUpdate $setOnInsert` upsert path — both approaches close the race; catch-DuplicateKey is the chosen minimal change. -- Built-in dedupe migration — admins clean up existing duplicates manually. The Tharga.MongoDB `AssureIndex` startup error is the surface. -- Symmetric fix for `TeamRepository` user-key races — separate scope; the Team collection already has `UniqueTeamMemberKey` index per `TeamRepositoryCollection.Indices`. -- Framework package bumps (Microsoft.AspNetCore.* 9.0.15 → 10.0.7) — deferred, separate PR. -- Consumer-side cleanup in Quilt4Net.Server — separate downstream bump after this lands. diff --git a/plan/plan.md b/plan/plan.md deleted file mode 100644 index 35002d8..0000000 --- a/plan/plan.md +++ /dev/null @@ -1,84 +0,0 @@ -# Plan: User upsert race fix - -## Steps - -- [x] **1. Unique index on `User.Identity`** - - Override `Indices` on `UserRepositoryCollection` (currently inherits the empty default from `DiskRepositoryCollectionBase<>`): - ```csharp - public override IEnumerable> Indices => - [ - new(Builders.IndexKeys.Ascending(x => x.Identity), - new CreateIndexOptions { Unique = true, Name = "Identity" }) - ]; - ``` - - Verify `IUser.Identity` is a property reachable via `x => x.Identity` (it is — defined on `IUser` in `Tharga.Team/IUser.cs`). - -- [x] **2. Catch-DuplicateKey recovery in `UserServiceRepositoryBase.GetUserAsync`** - - In `Tharga.Team.MongoDB/UserServiceRepositoryBase.cs`, wrap the `AddAsync` call: - ```csharp - protected override async Task GetUserAsync(ClaimsPrincipal claimsPrincipal) - { - var identity = claimsPrincipal.GetIdentity().Identity; - - var user = await _userRepository.GetAsync(identity); - if (user != null) return user; - - var candidate = await CreateUserEntityAsync(claimsPrincipal, identity); - try - { - await _userRepository.AddAsync(candidate); - return candidate; - } - catch (MongoWriteException ex) when (ex.WriteError?.Category == ServerErrorCategory.DuplicateKey) - { - // Lost the race; another request inserted first. Return the winning row. - return await _userRepository.GetAsync(identity); - } - } - ``` - - `MongoDB.Driver` is already a transitive dependency via `Tharga.MongoDB` — no new package reference needed. - -- [x] **3. Promote `UserRepositoryCollection` to `public`** - - Change the class declaration from `internal class` to `public class`. - - `Indices` is already `override` on the base type → already virtual; consumers can subclass and override. - -- [x] **4. `RegisterUserRepository` overload in `ThargaTeamOptions`** - - Add an internal `Type _userCollectionType` field (defaults to null). - - Existing `RegisterUserRepository()` continues to set `_userEntity = typeof(TUserEntity)` and leaves `_userCollectionType = null` (registration falls back to the built-in collection class). - - New overload: - ```csharp - public void RegisterUserRepository() - where TUserEntity : EntityBase, IUser - where TCollection : UserRepositoryCollection - { - _userEntity = typeof(TUserEntity); - _userCollectionType = typeof(TCollection); - } - ``` - - In `ThargaTeamRegistration.AddThargaTeamRepository`, the `userRepositoryCollectionImplementationType` becomes `o._userCollectionType ?? typeof(UserRepositoryCollection<>).MakeGenericType(userEntityType)`. - -- [x] **5. Tests** — New `Tharga.Team.MongoDB.Tests` project created with 7 tests (4 race-recovery + 2 registration + 1 index declaration). All passing. - - `UserServiceRepositoryBase`/`GetUserAsync` race-recovery test: build a `TestUserService` derivative whose `IUserRepository<>` is a fake that throws `MongoWriteException` (DuplicateKey) on first `AddAsync` and returns a known winning row on subsequent `GetAsync`. Assert the service returns the winning row, not the candidate. Construct `MongoWriteException` via its public constructor (or reflection if the `WriteError`/`Category` fields are read-only) — verify the reachable surface first. - - `RegisterUserRepository` plumbing test: register a derived collection type via the new overload, build an `IServiceProvider`, resolve `IUserRepositoryCollection`, assert the runtime type is the consumer subclass. - - `UserRepositoryCollection.Indices` declaration test: instantiate a `UserRepositoryCollection`, read `Indices`, assert one entry with `Unique = true`, `Name = "Identity"`, and the key path is `Identity`. - - Existing `Tharga.Team.Blazor.Tests` test `UserServiceBaseDefaultsTests` already covers `UserServiceBase.GetCurrentUserAsync` cache behaviour — confirm it still passes. - -- [x] **6. Build + test** — solution builds clean; 296 / 296 tests passing (289 before + 7 new). - - `dotnet build c:/dev/tharga/Toolkit/Platform/Tharga.Platform.sln -c Release` — 0 warnings, 0 errors. - - `dotnet test c:/dev/tharga/Toolkit/Platform/Tharga.Platform.sln -c Release` — full suite green. - -- [x] **7. README check** — `Tharga.Team.MongoDB/README.md` updated with a "Adding per-deployment User indices" section showing the subclass + `RegisterUserRepository` pattern. - - Per shared-instructions, skip README updates if the feature has no consumer-visible surface. This feature does add a public extension point (`UserRepositoryCollection<>` is now subclassable, new `RegisterUserRepository` overload). Decide whether `Tharga.Team.MongoDB/README.md` should mention the new extension point briefly. The migration prerequisite (dedupe before deploy) goes in the PR description, not the README. - -- [ ] **8. Commit + push the feature branch** - - Conventional commit prefix: `fix:` — this resolves a bug. - - Suggested message: `fix: race-proof user creation + unique Identity index + collection extension point`. - -- [ ] **9. Pause for user verification** (per the principle: plan/ stays on the feature branch through Completing; deleted in the close-out commit before PR opens). - -## Open questions - -(none — design choices locked via `AskUserQuestion` during planning) - -## Last session -2026-05-11 — All implementation steps complete. 7 new tests in a new `Tharga.Team.MongoDB.Tests` project; 296 / 296 passing. README extension-point section added. Ready for commit + push + user verification.