From 1cf80a4e4e4d036f8946fb1d9e6b191d74f54616 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 24 Jun 2026 21:21:02 -0400 Subject: [PATCH] NEO-146: fix stale active index race in TryGetActiveForPlayer Read the active index under the player lock and remove stale entries only when the indexed instance id still matches, preventing concurrent get/complete/reissue from dropping a newer active pointer. --- docs/plans/NEO-146-implementation-plan.md | 2 +- .../InMemoryContractInstanceStoreTests.cs | 51 +++++++++++++++++++ .../InMemoryContractInstanceStore.cs | 18 +++---- 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/docs/plans/NEO-146-implementation-plan.md b/docs/plans/NEO-146-implementation-plan.md index 3d6bc93..f113382 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 (`874` tests green). +- **Tests:** `InMemoryContractInstanceStoreTests`, `InMemoryContractOutcomeStoreTests`, Postgres persistence integration tests; host DI smoke (`875` 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 b3a3919..e8215bf 100644 --- a/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractInstanceStoreTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractInstanceStoreTests.cs @@ -265,6 +265,57 @@ public sealed class InMemoryContractInstanceStoreTests } } + [Fact] + public async Task TryGetActiveForPlayer_ShouldKeepActiveIndexConsistent_WhenRacingWithCompleteAndReissue() + { + // Arrange + var store = CreateStore(); + Assert.True(store.TryCreateActive( + PlayerId, + InstanceId, + TemplateId, + SeedBucket, + IssuedAt, + out _)); + // Act + await Task.WhenAll( + Enumerable.Range(0, 32).Select(i => Task.Run(() => + { + if (i % 3 == 0) + { + _ = store.TryGetActiveForPlayer(PlayerId, out _); + return; + } + + if (i % 3 == 1) + { + _ = store.TryMarkComplete(PlayerId, InstanceId, CompletedAt, out _); + return; + } + + _ = store.TryCreateActive( + PlayerId, + SecondInstanceId, + TemplateId, + SeedBucket, + IssuedAt.AddDays(1), + 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); + } + + Assert.False( + store.TryGet(PlayerId, InstanceId, out var first) && + store.TryGet(PlayerId, SecondInstanceId, out var second) && + first.Status == ContractInstanceStatus.Active && + second.Status == ContractInstanceStatus.Active); + } + [Fact] public void TryClearInstance_ShouldReturnTrue_WhenRowMissing() { diff --git a/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs b/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs index 5c7d208..36591ca 100644 --- a/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs +++ b/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs @@ -108,18 +108,18 @@ public sealed class InMemoryContractInstanceStore(IOptions return false; } - if (!activeInstanceByPlayer.TryGetValue(player, out var instanceId)) - { - return false; - } - lock (instanceLocks.GetOrAdd(PlayerLockKey(player), _ => new object())) { + if (!activeInstanceByPlayer.TryGetValue(player, out var instanceId)) + { + return false; + } + if (!byInstanceId.TryGetValue(instanceId, out var row) || row.Status != ContractInstanceStatus.Active || !string.Equals(row.PlayerId, player, StringComparison.Ordinal)) { - activeInstanceByPlayer.TryRemove(player, out _); + activeInstanceByPlayer.TryRemove(new KeyValuePair(player, instanceId)); return false; } @@ -164,7 +164,7 @@ public sealed class InMemoryContractInstanceStore(IOptions return false; } - activeInstanceByPlayer.TryRemove(player, out _); + activeInstanceByPlayer.TryRemove(new KeyValuePair(player, existingActiveId)); } if (byInstanceId.TryGetValue(instanceId, out var existing)) @@ -233,7 +233,7 @@ public sealed class InMemoryContractInstanceStore(IOptions } row.MarkCompleted(completedAt); - activeInstanceByPlayer.TryRemove(player, out _); + activeInstanceByPlayer.TryRemove(new KeyValuePair(player, instanceId)); snapshot = row.ToSnapshot(); return true; } @@ -269,7 +269,7 @@ public sealed class InMemoryContractInstanceStore(IOptions _ = byInstanceId.TryRemove(instanceId, out _); if (row.Status == ContractInstanceStatus.Active) { - activeInstanceByPlayer.TryRemove(player, out _); + activeInstanceByPlayer.TryRemove(new KeyValuePair(player, instanceId)); } return true;