From 6c4221072a90da6b8a2c9cb7be1d9bd28a724211 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 24 Jun 2026 21:05:56 -0400 Subject: [PATCH] NEO-146: fix in-memory create/complete lock ordering Nest player and instance locks in TryCreateActive, TryMarkComplete, and TryClearInstance so concurrent create and complete cannot leave a stale active index or mismatched snapshot. --- docs/plans/NEO-146-implementation-plan.md | 2 +- .../InMemoryContractInstanceStoreTests.cs | 38 ++++++ .../InMemoryContractInstanceStore.cs | 117 ++++++++++-------- 3 files changed, 102 insertions(+), 55 deletions(-) diff --git a/docs/plans/NEO-146-implementation-plan.md b/docs/plans/NEO-146-implementation-plan.md index 78be83c..dcdd51b 100644 --- a/docs/plans/NEO-146-implementation-plan.md +++ b/docs/plans/NEO-146-implementation-plan.md @@ -68,7 +68,7 @@ Durable per-player **`ContractInstance`** rows (active/completed) and append-onl - **Types:** `ContractInstanceStatus`, `ContractInstanceState`, `ContractInstanceIds`, `ContractInstanceReasonCodes`, `ContractOutcomeRow`, `ContractOutcomeIds`. - **Stores:** `IContractInstanceStore` / `IContractOutcomeStore`; in-memory + Postgres (`V011`, `V012`) when connection string set; partial unique index + app-level one-active enforcement. - **DI:** `AddContractInstanceStores` in `Program.cs`; in-memory override in `InMemoryWebApplicationFactory`. -- **Tests:** `InMemoryContractInstanceStoreTests`, `InMemoryContractOutcomeStoreTests`, Postgres persistence integration tests; host DI smoke (`871` tests green). +- **Tests:** `InMemoryContractInstanceStoreTests`, `InMemoryContractOutcomeStoreTests`, Postgres persistence integration tests; host DI smoke (`872` tests green). - **In-memory outcome guard:** `InMemoryContractOutcomeStore.TryAppend` denies when **`IContractInstanceStore.TryGet`** is false (Postgres FK + player/instance SQL guard parity). - **Docs:** `server/README.md` contract instance + outcome store sections. - **Bruno:** `bruno/neon-sprawl-server/contract-stores/` health smoke (startup-only; no contract HTTP in E7M4-03). diff --git a/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractInstanceStoreTests.cs b/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractInstanceStoreTests.cs index 8756c34..2f53aba 100644 --- a/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractInstanceStoreTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractInstanceStoreTests.cs @@ -227,6 +227,44 @@ public sealed class InMemoryContractInstanceStoreTests Assert.False(created); } + [Fact] + public async Task TryCreateActiveAndTryMarkComplete_ShouldKeepActiveIndexConsistent_WhenRacingOnSameInstanceId() + { + // Arrange + var store = CreateStore(); + // Act + await Task.WhenAll( + Enumerable.Range(0, 32).Select(i => Task.Run(() => + { + if (i % 2 == 0) + { + _ = store.TryCreateActive( + PlayerId, + InstanceId, + TemplateId, + SeedBucket, + IssuedAt, + out _); + return; + } + + _ = store.TryMarkComplete(PlayerId, InstanceId, CompletedAt, out _); + }))); + + // Assert + if (store.TryGetActiveForPlayer(PlayerId, out var active)) + { + Assert.True(store.TryGet(PlayerId, active.ContractInstanceId, out var row)); + Assert.Equal(ContractInstanceStatus.Active, row.Status); + } + + if (store.TryGet(PlayerId, InstanceId, out var final) && + final.Status == ContractInstanceStatus.Completed) + { + Assert.False(store.TryGetActiveForPlayer(PlayerId, out _)); + } + } + [Fact] public async Task Host_ShouldResolveContractInstanceStoresFromDi() { diff --git a/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs b/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs index 8b38e8b..acd30b6 100644 --- a/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs +++ b/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs @@ -153,36 +153,39 @@ public sealed class InMemoryContractInstanceStore(IOptions lock (instanceLocks.GetOrAdd(PlayerLockKey(player), _ => new object())) { - if (activeInstanceByPlayer.TryGetValue(player, out var existingActiveId)) + lock (instanceLocks.GetOrAdd(ContractInstanceIds.MakeInstanceKey(player, instanceId), _ => new object())) { - if (byInstanceId.TryGetValue(existingActiveId, out var existingActive) && - existingActive.Status == ContractInstanceStatus.Active) + if (activeInstanceByPlayer.TryGetValue(player, out var existingActiveId)) { - snapshot = existingActive.ToSnapshot(); + if (byInstanceId.TryGetValue(existingActiveId, out var existingActive) && + existingActive.Status == ContractInstanceStatus.Active) + { + snapshot = existingActive.ToSnapshot(); + return false; + } + + activeInstanceByPlayer.TryRemove(player, out _); + } + + if (byInstanceId.TryGetValue(instanceId, out var existing)) + { + snapshot = existing.ToSnapshot(); return false; } - activeInstanceByPlayer.TryRemove(player, out _); + var row = new InstanceRow( + instanceId, + normalizedTemplateId, + player, + ContractInstanceStatus.Active, + normalizedSeedBucket, + issuedAt, + null); + byInstanceId[instanceId] = row; + activeInstanceByPlayer[player] = instanceId; + snapshot = row.ToSnapshot(); + return true; } - - if (byInstanceId.TryGetValue(instanceId, out var existing)) - { - snapshot = existing.ToSnapshot(); - return false; - } - - var row = new InstanceRow( - instanceId, - normalizedTemplateId, - player, - ContractInstanceStatus.Active, - normalizedSeedBucket, - issuedAt, - null); - byInstanceId[instanceId] = row; - activeInstanceByPlayer[player] = instanceId; - snapshot = row.ToSnapshot(); - return true; } } @@ -207,30 +210,33 @@ public sealed class InMemoryContractInstanceStore(IOptions return false; } - lock (instanceLocks.GetOrAdd(lockKey, _ => new object())) + lock (instanceLocks.GetOrAdd(PlayerLockKey(player), _ => new object())) { - if (!byInstanceId.TryGetValue(instanceId, out var row) || - !string.Equals(row.PlayerId, player, StringComparison.Ordinal)) + lock (instanceLocks.GetOrAdd(lockKey, _ => new object())) { - return false; - } + if (!byInstanceId.TryGetValue(instanceId, out var row) || + !string.Equals(row.PlayerId, player, StringComparison.Ordinal)) + { + return false; + } - if (row.Status == ContractInstanceStatus.Completed) - { + if (row.Status == ContractInstanceStatus.Completed) + { + snapshot = row.ToSnapshot(); + return false; + } + + if (row.Status != ContractInstanceStatus.Active) + { + snapshot = row.ToSnapshot(); + return false; + } + + row.MarkCompleted(completedAt); + activeInstanceByPlayer.TryRemove(player, out _); snapshot = row.ToSnapshot(); - return false; + return true; } - - if (row.Status != ContractInstanceStatus.Active) - { - snapshot = row.ToSnapshot(); - return false; - } - - row.MarkCompleted(completedAt); - activeInstanceByPlayer.TryRemove(player, out _); - snapshot = row.ToSnapshot(); - return true; } } @@ -250,21 +256,24 @@ public sealed class InMemoryContractInstanceStore(IOptions return false; } - lock (instanceLocks.GetOrAdd(lockKey, _ => new object())) + lock (instanceLocks.GetOrAdd(PlayerLockKey(player), _ => new object())) { - if (!byInstanceId.TryGetValue(instanceId, out var row) || - !string.Equals(row.PlayerId, player, StringComparison.Ordinal)) + lock (instanceLocks.GetOrAdd(lockKey, _ => new object())) { - return false; - } + if (!byInstanceId.TryGetValue(instanceId, out var row) || + !string.Equals(row.PlayerId, player, StringComparison.Ordinal)) + { + return false; + } - _ = byInstanceId.TryRemove(instanceId, out _); - if (row.Status == ContractInstanceStatus.Active) - { - activeInstanceByPlayer.TryRemove(player, out _); - } + _ = byInstanceId.TryRemove(instanceId, out _); + if (row.Status == ContractInstanceStatus.Active) + { + activeInstanceByPlayer.TryRemove(player, out _); + } - return true; + return true; + } } } }