signup: close two concurrency holes from final review
(1) RegisterAnonymousViewer now catches the unique-violation
race (SQLSTATE 23505 on Postgres / code 19 on SQLite) and
re-reads by UDID, returning the existing row instead of
surfacing 500 to the second concurrent /tool/signup caller.
New repo test exercises the back-to-back register path.
(2) Add unique index on SocialAccountConnection (AccountType,
AccountId). The auth handler's find-or-link path claimed
this index existed as the dedup backstop; the claim was
accurate as design intent but the schema was missing. Now
matched. Comment in handler updated.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
3271
SVSim.Database/Migrations/20260527184324_AddSocialAccountConnectionUniqueIndex.Designer.cs
generated
Normal file
3271
SVSim.Database/Migrations/20260527184324_AddSocialAccountConnectionUniqueIndex.Designer.cs
generated
Normal file
File diff suppressed because it is too large
Load Diff
@@ -0,0 +1,28 @@
|
||||
using Microsoft.EntityFrameworkCore.Migrations;
|
||||
|
||||
#nullable disable
|
||||
|
||||
namespace SVSim.Database.Migrations
|
||||
{
|
||||
/// <inheritdoc />
|
||||
public partial class AddSocialAccountConnectionUniqueIndex : Migration
|
||||
{
|
||||
/// <inheritdoc />
|
||||
protected override void Up(MigrationBuilder migrationBuilder)
|
||||
{
|
||||
migrationBuilder.CreateIndex(
|
||||
name: "IX_SocialAccountConnection_AccountType_AccountId",
|
||||
table: "SocialAccountConnection",
|
||||
columns: new[] { "AccountType", "AccountId" },
|
||||
unique: true);
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
protected override void Down(MigrationBuilder migrationBuilder)
|
||||
{
|
||||
migrationBuilder.DropIndex(
|
||||
name: "IX_SocialAccountConnection_AccountType_AccountId",
|
||||
table: "SocialAccountConnection");
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -2923,6 +2923,9 @@ namespace SVSim.Database.Migrations
|
||||
|
||||
b1.HasKey("ViewerId", "Id");
|
||||
|
||||
b1.HasIndex("AccountType", "AccountId")
|
||||
.IsUnique();
|
||||
|
||||
b1.ToTable("SocialAccountConnection");
|
||||
|
||||
b1.WithOwner("Viewer")
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Npgsql;
|
||||
using SVSim.Database.Enums;
|
||||
using SVSim.Database.Models;
|
||||
using SVSim.Database.Models.Config;
|
||||
@@ -98,10 +99,52 @@ public class ViewerRepository : IViewerRepository
|
||||
var viewer = await BuildDefaultViewer("Player");
|
||||
viewer.Udid = udid;
|
||||
_dbContext.Set<Models.Viewer>().Add(viewer);
|
||||
await _dbContext.SaveChangesAsync();
|
||||
try
|
||||
{
|
||||
await _dbContext.SaveChangesAsync();
|
||||
}
|
||||
catch (DbUpdateException ex) when (IsUniqueViolation(ex))
|
||||
{
|
||||
// Concurrent signup for the same UDID raced us to the unique index. The other request
|
||||
// already committed a viewer with this UDID — re-read and return it. Detach the local
|
||||
// entity first so EF doesn't keep trying to insert the now-orphaned graph.
|
||||
//
|
||||
// Cross-engine: Postgres surfaces this as Npgsql.PostgresException SqlState "23505";
|
||||
// SQLite (test backend) surfaces it as Microsoft.Data.Sqlite.SqliteException with
|
||||
// SqliteErrorCode 19 (SQLITE_CONSTRAINT). Matched by type-name to avoid pulling a
|
||||
// Sqlite package dep into SVSim.Database.
|
||||
_dbContext.Entry(viewer).State = EntityState.Detached;
|
||||
var existing = await GetViewerByUdid(udid)
|
||||
?? throw new InvalidOperationException(
|
||||
$"Got unique-violation on Udid={udid} insert but subsequent lookup found no row. " +
|
||||
"This shouldn't happen — likely transaction isolation issue.");
|
||||
return existing;
|
||||
}
|
||||
return viewer;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Returns true if the given <see cref="DbUpdateException"/> wraps a backend-level unique-
|
||||
/// constraint violation. Postgres → SqlState "23505"; SQLite → SqliteErrorCode 19.
|
||||
/// </summary>
|
||||
private static bool IsUniqueViolation(DbUpdateException ex)
|
||||
{
|
||||
if (ex.InnerException is Npgsql.PostgresException pgEx && pgEx.SqlState == "23505")
|
||||
{
|
||||
return true;
|
||||
}
|
||||
// Match SQLite by type name so this assembly doesn't take a dep on Microsoft.Data.Sqlite.
|
||||
// Test backend (SQLite in-memory) raises SqliteException with SqliteErrorCode 19 on UNIQUE
|
||||
// constraint violations.
|
||||
var inner = ex.InnerException;
|
||||
if (inner is not null && inner.GetType().FullName == "Microsoft.Data.Sqlite.SqliteException")
|
||||
{
|
||||
var prop = inner.GetType().GetProperty("SqliteErrorCode");
|
||||
if (prop?.GetValue(inner) is int code && code == 19) return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
public async Task LinkSteamToViewer(long viewerId, ulong steamId)
|
||||
{
|
||||
var viewer = await _dbContext.Set<Models.Viewer>()
|
||||
|
||||
@@ -157,6 +157,17 @@ public class SVSimDbContext : DbContext
|
||||
b.HasIndex("ViewerId", "ProductId").IsUnique();
|
||||
});
|
||||
|
||||
// A given social account links to exactly one viewer — two viewers cannot share the same
|
||||
// Steam (or Facebook, etc.) account. This is the dedup backstop the auth handler's find-
|
||||
// or-link path (SteamSessionAuthenticationHandler) relies on: two concurrent first-Steam-
|
||||
// touch requests can both pass the .Any(...) check in LinkSteamToViewer, but the second
|
||||
// SaveChanges() throws unique-violation and surfaces a clean 500 instead of silently
|
||||
// appending duplicate connections.
|
||||
modelBuilder.Entity<Viewer>().OwnsMany(v => v.SocialAccountConnections, b =>
|
||||
{
|
||||
b.HasIndex("AccountType", "AccountId").IsUnique();
|
||||
});
|
||||
|
||||
modelBuilder.Entity<BuildDeckSeriesEntry>().OwnsMany(s => s.SeriesRewards);
|
||||
modelBuilder.Entity<BuildDeckProductEntry>().OwnsMany(p => p.Cards);
|
||||
modelBuilder.Entity<BuildDeckProductEntry>().OwnsMany(p => p.Rewards);
|
||||
|
||||
@@ -99,8 +99,11 @@ public class SteamSessionAuthenticationHandler : AuthenticationHandler<SteamAuth
|
||||
{
|
||||
// Find-or-link: first authenticated request after /tool/signup. The client signed up
|
||||
// anonymously and has no Steam social row yet; if the UDID resolves to a viewer, attach
|
||||
// Steam to it now so subsequent requests hit the fast SteamId path. The SteamId unique
|
||||
// index on SocialAccountConnection is the dedup backstop for concurrent first-touch.
|
||||
// Steam to it now so subsequent requests hit the fast SteamId path. The unique index
|
||||
// on SocialAccountConnection (AccountType, AccountId) — declared in OnModelCreating —
|
||||
// is the second-layer dedup backstop: if two concurrent first-touches both pass the
|
||||
// .Any(...) check in LinkSteamToViewer, the second SaveChanges throws cleanly instead
|
||||
// of silently duplicating connections.
|
||||
Guid? udid = Context.GetUdid();
|
||||
if (udid is Guid u && u != Guid.Empty)
|
||||
{
|
||||
|
||||
@@ -99,6 +99,37 @@ public class ViewerRepositoryTests
|
||||
"Default-loadout body should populate Classes (smoke-test the shared BuildDefaultViewer helper).");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task RegisterAnonymousViewer_concurrent_same_udid_returns_existing_not_duplicate()
|
||||
{
|
||||
using var factory = new SVSimTestFactory();
|
||||
var udid = Guid.NewGuid();
|
||||
|
||||
long firstId;
|
||||
using (var scope = factory.Services.CreateScope())
|
||||
{
|
||||
var repo = scope.ServiceProvider.GetRequiredService<IViewerRepository>();
|
||||
firstId = (await repo.RegisterAnonymousViewer(udid)).Id;
|
||||
}
|
||||
|
||||
// Second register with the same UDID — simulates the race losing thread after the unique
|
||||
// index has caught the duplicate. We expect a clean re-fetch, NOT an exception or duplicate.
|
||||
long secondId;
|
||||
using (var scope = factory.Services.CreateScope())
|
||||
{
|
||||
var repo = scope.ServiceProvider.GetRequiredService<IViewerRepository>();
|
||||
secondId = (await repo.RegisterAnonymousViewer(udid)).Id;
|
||||
}
|
||||
|
||||
Assert.That(secondId, Is.EqualTo(firstId),
|
||||
"Second register must return the existing row, not create a duplicate.");
|
||||
|
||||
using var verifyScope = factory.Services.CreateScope();
|
||||
var db = verifyScope.ServiceProvider.GetRequiredService<SVSimDbContext>();
|
||||
var count = await db.Viewers.CountAsync(v => v.Udid == udid);
|
||||
Assert.That(count, Is.EqualTo(1));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task RegisterAnonymousViewer_with_empty_udid_throws()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user