From dafe3cb92f11d437e7d9b07df3178abb9ad08f69 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 23 May 2026 19:42:50 -0400 Subject: [PATCH] NEO-54: fix in-memory inventory races with atomic TryMutateSnapshot Re-read snapshot under per-player lock; route add/remove through TryMutateSnapshot so read-modify-write is serialized for in-memory and wrapped in one transaction for Postgres. --- .../Game/Items/IPlayerInventoryStore.cs | 9 ++ .../Items/InMemoryPlayerInventoryStore.cs | 49 +++++++- .../Items/PlayerInventoryMutationWrite.cs | 5 + .../Game/Items/PlayerInventoryOperations.cs | 112 ++++++++++-------- .../Items/PostgresPlayerInventoryStore.cs | 62 ++++++++-- 5 files changed, 175 insertions(+), 62 deletions(-) create mode 100644 server/NeonSprawl.Server/Game/Items/PlayerInventoryMutationWrite.cs diff --git a/server/NeonSprawl.Server/Game/Items/IPlayerInventoryStore.cs b/server/NeonSprawl.Server/Game/Items/IPlayerInventoryStore.cs index 3057fed..4bd42de 100644 --- a/server/NeonSprawl.Server/Game/Items/IPlayerInventoryStore.cs +++ b/server/NeonSprawl.Server/Game/Items/IPlayerInventoryStore.cs @@ -8,4 +8,13 @@ public interface IPlayerInventoryStore /// Atomically replaces the full snapshot for a known player; false when the player is not in the store. bool TryReplaceSnapshot(string playerId, PlayerInventorySnapshot snapshot); + + /// + /// Atomically reads the current snapshot and optionally persists a replacement under per-player lock (in-memory) + /// or within one database transaction (Postgres). + /// + bool TryMutateSnapshot( + string playerId, + Func mutator, + out PlayerInventorySnapshot result); } diff --git a/server/NeonSprawl.Server/Game/Items/InMemoryPlayerInventoryStore.cs b/server/NeonSprawl.Server/Game/Items/InMemoryPlayerInventoryStore.cs index 38e587b..6a67d79 100644 --- a/server/NeonSprawl.Server/Game/Items/InMemoryPlayerInventoryStore.cs +++ b/server/NeonSprawl.Server/Game/Items/InMemoryPlayerInventoryStore.cs @@ -23,7 +23,7 @@ public sealed class InMemoryPlayerInventoryStore(IOptions o public bool TryGetSnapshot(string playerId, out PlayerInventorySnapshot snapshot) { var key = NormalizePlayerId(playerId); - if (key.Length == 0 || !byPlayer.TryGetValue(key, out var existing)) + if (key.Length == 0) { snapshot = PlayerInventorySnapshot.Empty(); return false; @@ -31,7 +31,13 @@ public sealed class InMemoryPlayerInventoryStore(IOptions o lock (playerLocks.GetOrAdd(key, _ => new object())) { - snapshot = Clone(existing); + if (!byPlayer.TryGetValue(key, out var current)) + { + snapshot = PlayerInventorySnapshot.Empty(); + return false; + } + + snapshot = Clone(current); return true; } } @@ -40,18 +46,55 @@ public sealed class InMemoryPlayerInventoryStore(IOptions o public bool TryReplaceSnapshot(string playerId, PlayerInventorySnapshot snapshot) { var key = NormalizePlayerId(playerId); - if (key.Length == 0 || !byPlayer.ContainsKey(key)) + if (key.Length == 0) { return false; } lock (playerLocks.GetOrAdd(key, _ => new object())) { + if (!byPlayer.ContainsKey(key)) + { + return false; + } + byPlayer[key] = Clone(snapshot); return true; } } + /// + public bool TryMutateSnapshot( + string playerId, + Func mutator, + out PlayerInventorySnapshot result) + { + var key = NormalizePlayerId(playerId); + if (key.Length == 0) + { + result = PlayerInventorySnapshot.Empty(); + return false; + } + + lock (playerLocks.GetOrAdd(key, _ => new object())) + { + if (!byPlayer.TryGetValue(key, out var current)) + { + result = PlayerInventorySnapshot.Empty(); + return false; + } + + var write = mutator(Clone(current)); + result = Clone(write.Value); + if (write.Write) + { + byPlayer[key] = Clone(write.Value); + } + + return true; + } + } + private static PlayerInventorySnapshot Clone(PlayerInventorySnapshot source) => new() { diff --git a/server/NeonSprawl.Server/Game/Items/PlayerInventoryMutationWrite.cs b/server/NeonSprawl.Server/Game/Items/PlayerInventoryMutationWrite.cs new file mode 100644 index 0000000..bcba549 --- /dev/null +++ b/server/NeonSprawl.Server/Game/Items/PlayerInventoryMutationWrite.cs @@ -0,0 +1,5 @@ +namespace NeonSprawl.Server.Game.Items; + +/// Result of an atomic inventory transform (NEO-54). +/// true to persist ; false to leave storage unchanged. +public readonly record struct PlayerInventoryMutationWrite(bool Write, PlayerInventorySnapshot Value); diff --git a/server/NeonSprawl.Server/Game/Items/PlayerInventoryOperations.cs b/server/NeonSprawl.Server/Game/Items/PlayerInventoryOperations.cs index 33c5982..b92543b 100644 --- a/server/NeonSprawl.Server/Game/Items/PlayerInventoryOperations.cs +++ b/server/NeonSprawl.Server/Game/Items/PlayerInventoryOperations.cs @@ -25,32 +25,36 @@ public static class PlayerInventoryOperations return Deny(playerId, store, PlayerInventoryReasonCodes.InvalidItem); } - if (!store.TryGetSnapshot(playerId, out var before)) + string? denyReason = null; + if (!store.TryMutateSnapshot( + playerId, + before => + { + var container = PlayerInventorySnapshot.ContainerFromSlotKind(def.InventorySlotKind); + var slots = CloneSlots(before.GetSlots(container)); + var remaining = quantity; + remaining = MergeIntoExistingStacks(slots, def.Id, remaining, def.StackMax); + remaining = FillEmptySlots(slots, def.Id, remaining, def.StackMax); + + if (remaining > 0) + { + denyReason = PlayerInventoryReasonCodes.InventoryFull; + return new PlayerInventoryMutationWrite(Write: false, before); + } + + return new PlayerInventoryMutationWrite(Write: true, before.WithSlots(container, slots)); + }, + out var result)) { return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.StoreMissing, null, null); } - var container = PlayerInventorySnapshot.ContainerFromSlotKind(def.InventorySlotKind); - var slots = CloneSlots(before.GetSlots(container)); - var remaining = quantity; - remaining = MergeIntoExistingStacks(slots, def.Id, remaining, def.StackMax); - remaining = FillEmptySlots(slots, def.Id, remaining, def.StackMax); - - if (remaining > 0) + if (denyReason is not null) { - return new PlayerInventoryMutationOutcome( - PlayerInventoryMutationKind.Denied, - PlayerInventoryReasonCodes.InventoryFull, - before); + return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.Denied, denyReason, result); } - var after = before.WithSlots(container, slots); - if (!store.TryReplaceSnapshot(playerId, after)) - { - return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.StoreMissing, null, null); - } - - return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.Applied, null, after); + return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.Applied, null, result); } /// Removes from stacks of (lowest slot index first). @@ -72,46 +76,50 @@ public static class PlayerInventoryOperations return Deny(playerId, store, PlayerInventoryReasonCodes.InvalidItem); } - if (!store.TryGetSnapshot(playerId, out var before)) + string? denyReason = null; + if (!store.TryMutateSnapshot( + playerId, + before => + { + var container = PlayerInventorySnapshot.ContainerFromSlotKind(def.InventorySlotKind); + var slots = CloneSlots(before.GetSlots(container)); + var available = CountQuantity(slots, def.Id); + if (available < quantity) + { + denyReason = PlayerInventoryReasonCodes.InsufficientQuantity; + return new PlayerInventoryMutationWrite(Write: false, before); + } + + var remaining = quantity; + for (var i = 0; i < slots.Length && remaining > 0; i++) + { + var slot = slots[i]; + if (slot.IsEmpty || !string.Equals(slot.ItemId, def.Id, StringComparison.Ordinal)) + { + continue; + } + + var take = Math.Min(remaining, slot.Quantity); + var left = slot.Quantity - take; + slots[i] = left <= 0 + ? new InventorySlotState(slot.SlotIndex, null, 0) + : new InventorySlotState(slot.SlotIndex, def.Id, left); + remaining -= take; + } + + return new PlayerInventoryMutationWrite(Write: true, before.WithSlots(container, slots)); + }, + out var result)) { return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.StoreMissing, null, null); } - var container = PlayerInventorySnapshot.ContainerFromSlotKind(def.InventorySlotKind); - var slots = CloneSlots(before.GetSlots(container)); - var available = CountQuantity(slots, def.Id); - if (available < quantity) + if (denyReason is not null) { - return new PlayerInventoryMutationOutcome( - PlayerInventoryMutationKind.Denied, - PlayerInventoryReasonCodes.InsufficientQuantity, - before); + return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.Denied, denyReason, result); } - var remaining = quantity; - for (var i = 0; i < slots.Length && remaining > 0; i++) - { - var slot = slots[i]; - if (slot.IsEmpty || !string.Equals(slot.ItemId, def.Id, StringComparison.Ordinal)) - { - continue; - } - - var take = Math.Min(remaining, slot.Quantity); - var left = slot.Quantity - take; - slots[i] = left <= 0 - ? new InventorySlotState(slot.SlotIndex, null, 0) - : new InventorySlotState(slot.SlotIndex, def.Id, left); - remaining -= take; - } - - var after = before.WithSlots(container, slots); - if (!store.TryReplaceSnapshot(playerId, after)) - { - return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.StoreMissing, null, null); - } - - return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.Applied, null, after); + return new PlayerInventoryMutationOutcome(PlayerInventoryMutationKind.Applied, null, result); } private static PlayerInventoryMutationOutcome Deny(string playerId, IPlayerInventoryStore store, string reasonCode) diff --git a/server/NeonSprawl.Server/Game/Items/PostgresPlayerInventoryStore.cs b/server/NeonSprawl.Server/Game/Items/PostgresPlayerInventoryStore.cs index 13d9442..e857def 100644 --- a/server/NeonSprawl.Server/Game/Items/PostgresPlayerInventoryStore.cs +++ b/server/NeonSprawl.Server/Game/Items/PostgresPlayerInventoryStore.cs @@ -43,19 +43,63 @@ public sealed class PostgresPlayerInventoryStore(Npgsql.NpgsqlDataSource dataSou return false; } + ReplaceSnapshotRows(conn, norm, snapshot, tx); + tx.Commit(); + return true; + } + + /// + public bool TryMutateSnapshot( + string playerId, + Func mutator, + out PlayerInventorySnapshot result) + { + var norm = NormalizePlayerId(playerId); + if (norm.Length == 0) + { + result = PlayerInventorySnapshot.Empty(); + return false; + } + + PostgresPlayerInventoryBootstrap.EnsureSchema(dataSource); + using var conn = dataSource.OpenConnection(); + using var tx = conn.BeginTransaction(); + if (!PlayerExists(conn, norm, tx)) + { + tx.Rollback(); + result = PlayerInventorySnapshot.Empty(); + return false; + } + + var current = ReadSnapshot(conn, norm, tx); + var write = mutator(current); + result = write.Value; + if (write.Write) + { + ReplaceSnapshotRows(conn, norm, write.Value, tx); + } + + tx.Commit(); + return true; + } + + private static void ReplaceSnapshotRows( + Npgsql.NpgsqlConnection conn, + string playerIdNormalized, + PlayerInventorySnapshot snapshot, + Npgsql.NpgsqlTransaction tx) + { using (var del = new Npgsql.NpgsqlCommand( "DELETE FROM player_inventory WHERE player_id = @pid;", conn, tx)) { - del.Parameters.AddWithValue("pid", norm); + del.Parameters.AddWithValue("pid", playerIdNormalized); del.ExecuteNonQuery(); } - WriteOccupiedSlots(conn, norm, snapshot.BagSlots, InventoryContainerKind.Bag, tx); - WriteOccupiedSlots(conn, norm, snapshot.EquipmentSlots, InventoryContainerKind.Equipment, tx); - tx.Commit(); - return true; + WriteOccupiedSlots(conn, playerIdNormalized, snapshot.BagSlots, InventoryContainerKind.Bag, tx); + WriteOccupiedSlots(conn, playerIdNormalized, snapshot.EquipmentSlots, InventoryContainerKind.Equipment, tx); } private static void WriteOccupiedSlots( @@ -89,7 +133,10 @@ public sealed class PostgresPlayerInventoryStore(Npgsql.NpgsqlDataSource dataSou } } - private static PlayerInventorySnapshot ReadSnapshot(Npgsql.NpgsqlConnection conn, string playerIdNormalized) + private static PlayerInventorySnapshot ReadSnapshot( + Npgsql.NpgsqlConnection conn, + string playerIdNormalized, + Npgsql.NpgsqlTransaction? tx = null) { var bag = PlayerInventorySnapshot.CreateEmptySlots(PlayerInventorySnapshot.BagSlotCount); var equipment = PlayerInventorySnapshot.CreateEmptySlots(PlayerInventorySnapshot.EquipmentSlotCount); @@ -101,7 +148,8 @@ public sealed class PostgresPlayerInventoryStore(Npgsql.NpgsqlDataSource dataSou WHERE player_id = @pid ORDER BY container_kind, slot_index; """, - conn); + conn, + tx); cmd.Parameters.AddWithValue("pid", playerIdNormalized); using var reader = cmd.ExecuteReader(); while (reader.Read())