From 93bd45d49a5782d995e3fb7464609f9c0c693d50 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 24 Jun 2026 21:13:42 -0400 Subject: [PATCH] NEO-146: fix in-memory outcome idempotency race and clear parity Lock idempotency keys alongside row ids in TryAppend so concurrent duplicate keys cannot both succeed; return true from TryClearInstance when the row is absent to match Postgres idempotent DELETE behavior. --- .../InMemoryContractInstanceStoreTests.cs | 12 ++++++ .../InMemoryContractOutcomeStoreTests.cs | 23 ++++++++++++ .../InMemoryContractInstanceStore.cs | 2 +- .../Contracts/InMemoryContractOutcomeStore.cs | 37 +++++++++++++++++-- 4 files changed, 70 insertions(+), 4 deletions(-) diff --git a/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractInstanceStoreTests.cs b/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractInstanceStoreTests.cs index 2f53aba..b3a3919 100644 --- a/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractInstanceStoreTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractInstanceStoreTests.cs @@ -265,6 +265,18 @@ public sealed class InMemoryContractInstanceStoreTests } } + [Fact] + public void TryClearInstance_ShouldReturnTrue_WhenRowMissing() + { + // Arrange + var store = CreateStore(); + // Act + var cleared = store.TryClearInstance(PlayerId, InstanceId); + // Assert + Assert.True(cleared); + Assert.False(store.TryGet(PlayerId, InstanceId, out _)); + } + [Fact] public async Task Host_ShouldResolveContractInstanceStoresFromDi() { diff --git a/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractOutcomeStoreTests.cs b/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractOutcomeStoreTests.cs index ac1783c..2aa283d 100644 --- a/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractOutcomeStoreTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Contracts/InMemoryContractOutcomeStoreTests.cs @@ -107,6 +107,29 @@ public sealed class InMemoryContractOutcomeStoreTests Assert.Empty(store.GetOutcomesForInstance(InstanceId)); } + [Fact] + public async Task TryAppend_ShouldAllowOnlyOneSuccess_WhenConcurrentDuplicateIdempotencyKeysRace() + { + // Arrange + var (instanceStore, store) = CreateStores(); + SeedActiveInstance(instanceStore); + var idempotencyKey = ContractOutcomeIds.MakeIdempotencyKey(PlayerId, InstanceId); + var baseRow = CreatePrototypeOutcome(); + // Act + var results = await Task.WhenAll( + Enumerable.Range(0, 8).Select(i => Task.Run(() => + { + var row = baseRow with { Id = $"outcome-row-race-{i:D3}" }; + Assert.Equal(idempotencyKey, row.IdempotencyKey); + return store.TryAppend(row); + }))); + + // Assert + Assert.Equal(1, results.Count(static r => r)); + Assert.Equal(7, results.Count(static r => !r)); + Assert.Single(store.GetOutcomesForInstance(InstanceId)); + } + private static (InMemoryContractInstanceStore InstanceStore, InMemoryContractOutcomeStore OutcomeStore) CreateStores() { var options = Options.Create(new GamePositionOptions { DevPlayerId = PlayerId }); diff --git a/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs b/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs index acd30b6..5c7d208 100644 --- a/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs +++ b/server/NeonSprawl.Server/Game/Contracts/InMemoryContractInstanceStore.cs @@ -263,7 +263,7 @@ public sealed class InMemoryContractInstanceStore(IOptions if (!byInstanceId.TryGetValue(instanceId, out var row) || !string.Equals(row.PlayerId, player, StringComparison.Ordinal)) { - return false; + return true; } _ = byInstanceId.TryRemove(instanceId, out _); diff --git a/server/NeonSprawl.Server/Game/Contracts/InMemoryContractOutcomeStore.cs b/server/NeonSprawl.Server/Game/Contracts/InMemoryContractOutcomeStore.cs index 7340c06..87080aa 100644 --- a/server/NeonSprawl.Server/Game/Contracts/InMemoryContractOutcomeStore.cs +++ b/server/NeonSprawl.Server/Game/Contracts/InMemoryContractOutcomeStore.cs @@ -25,17 +25,47 @@ public sealed class InMemoryContractOutcomeStore(IContractInstanceStore instance return false; } - lock (idLocks.GetOrAdd(normalized.Id, _ => new object())) + var appended = false; + LockRowAndIdempotencyKey(normalized.Id, normalized.IdempotencyKey, () => { if (rowsById.ContainsKey(normalized.Id) || rowIdByIdempotencyKey.ContainsKey(normalized.IdempotencyKey)) { - return false; + return; } rowsById[normalized.Id] = normalized; rowIdByIdempotencyKey[normalized.IdempotencyKey] = normalized.Id; - return true; + appended = true; + }); + + return appended; + } + + private static string IdempotencyLockKey(string idempotencyKey) => $"idem\0{idempotencyKey}"; + + private void LockRowAndIdempotencyKey(string rowId, string idempotencyKey, Action action) + { + var idemLockKey = IdempotencyLockKey(idempotencyKey); + if (string.CompareOrdinal(rowId, idemLockKey) <= 0) + { + lock (idLocks.GetOrAdd(rowId, _ => new object())) + { + lock (idLocks.GetOrAdd(idemLockKey, _ => new object())) + { + action(); + } + } + + return; + } + + lock (idLocks.GetOrAdd(idemLockKey, _ => new object())) + { + lock (idLocks.GetOrAdd(rowId, _ => new object())) + { + action(); + } } } @@ -99,6 +129,7 @@ public sealed class InMemoryContractOutcomeStore(IContractInstanceStore instance if (rowsById.TryRemove(id, out var removed)) { rowIdByIdempotencyKey.TryRemove(removed.IdempotencyKey, out _); + idLocks.TryRemove(IdempotencyLockKey(removed.IdempotencyKey), out _); } idLocks.TryRemove(id, out _);