From 564b1d678fcefab18c14db36c2b6995303e7624d Mon Sep 17 00:00:00 2001 From: gamer147 Date: Thu, 4 Jun 2026 22:13:20 -0400 Subject: [PATCH] fix(battle-node): collision-safe battle-id registration + viewer eviction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RegisterPending → TryRegisterPending (TryAdd instead of indexer) so battle-id collisions return false instead of silently evicting a live battle. MatchingBridge retries with fresh IDs on collision (max 5). Before registering, EvictStaleForViewer removes any stale pending battle the viewer left behind, enforcing the one-pending-per-viewer invariant that was previously comment-asserted. Store tests switched to per-test local stores to fix a race under the assembly-wide ParallelScope.All. Co-Authored-By: Claude Opus 4.6 --- SVSim.BattleNode/Bridge/MatchingBridge.cs | 31 ++++++++++++++----- .../Sessions/IBattleSessionStore.cs | 6 ++-- .../Sessions/InMemoryBattleSessionStore.cs | 4 +-- .../BattleNode/Bridge/MatchingBridgeTests.cs | 15 +++++++++ .../InMemoryBattleSessionStoreTests.cs | 31 ++++++++++--------- 5 files changed, 60 insertions(+), 27 deletions(-) diff --git a/SVSim.BattleNode/Bridge/MatchingBridge.cs b/SVSim.BattleNode/Bridge/MatchingBridge.cs index 337441d..2436dbf 100644 --- a/SVSim.BattleNode/Bridge/MatchingBridge.cs +++ b/SVSim.BattleNode/Bridge/MatchingBridge.cs @@ -24,20 +24,35 @@ public sealed class MatchingBridge : IMatchingBridge _options = options; } + private const int MaxIdRetries = 5; + public PendingMatch RegisterBattle(BattlePlayer p1, BattlePlayer? p2, BattleType type) { ValidateContract(p1, p2, type); + EvictStaleForViewer(p1.ViewerId); + if (p2 is not null) EvictStaleForViewer(p2.ViewerId); - // Decimal battle id mirrors the captures (e.g. "975695075012"): two unbiased - // BattleIdHalfDigits-wide draws concatenated. RandomNumberGenerator.GetInt32 uses - // rejection sampling so each half is uniform on [0, BattleIdHalfExclusiveMax). - var hi = RandomNumberGenerator.GetInt32(0, BattleIdHalfExclusiveMax); - var lo = RandomNumberGenerator.GetInt32(0, BattleIdHalfExclusiveMax); var halfFormat = "D" + BattleIdHalfDigits; - var battleId = hi.ToString(halfFormat) + lo.ToString(halfFormat); - _store.RegisterPending(new PendingBattle(battleId, type, p1, p2)); - return new PendingMatch(battleId, _options.NodeServerUrl); + for (var attempt = 0; attempt < MaxIdRetries; attempt++) + { + var hi = RandomNumberGenerator.GetInt32(0, BattleIdHalfExclusiveMax); + var lo = RandomNumberGenerator.GetInt32(0, BattleIdHalfExclusiveMax); + var battleId = hi.ToString(halfFormat) + lo.ToString(halfFormat); + + if (_store.TryRegisterPending(new PendingBattle(battleId, type, p1, p2))) + return new PendingMatch(battleId, _options.NodeServerUrl); + } + + throw new InvalidOperationException( + $"Failed to mint a unique battle id after {MaxIdRetries} attempts."); + } + + private void EvictStaleForViewer(long viewerId) + { + var stale = _store.TryFindPendingForViewer(viewerId); + if (stale is not null) + _store.RemovePending(stale.BattleId); } private static void ValidateContract(BattlePlayer p1, BattlePlayer? p2, BattleType type) diff --git a/SVSim.BattleNode/Sessions/IBattleSessionStore.cs b/SVSim.BattleNode/Sessions/IBattleSessionStore.cs index 1221b79..718c68c 100644 --- a/SVSim.BattleNode/Sessions/IBattleSessionStore.cs +++ b/SVSim.BattleNode/Sessions/IBattleSessionStore.cs @@ -2,8 +2,10 @@ namespace SVSim.BattleNode.Sessions; public interface IBattleSessionStore { - /// Register a battle minted by the matching bridge, awaiting a WS connect. - void RegisterPending(PendingBattle battle); + /// Register a battle minted by the matching bridge, awaiting a WS connect. + /// Returns false if a battle with the same id is already pending (caller should retry + /// with a fresh id). + bool TryRegisterPending(PendingBattle battle); /// Look up the pending battle. Returns null if not present. PendingBattle? TryGetPending(string battleId); diff --git a/SVSim.BattleNode/Sessions/InMemoryBattleSessionStore.cs b/SVSim.BattleNode/Sessions/InMemoryBattleSessionStore.cs index 360fa35..dad18d5 100644 --- a/SVSim.BattleNode/Sessions/InMemoryBattleSessionStore.cs +++ b/SVSim.BattleNode/Sessions/InMemoryBattleSessionStore.cs @@ -6,8 +6,8 @@ public sealed class InMemoryBattleSessionStore : IBattleSessionStore { private readonly ConcurrentDictionary _pending = new(); - public void RegisterPending(PendingBattle battle) => - _pending[battle.BattleId] = battle; + public bool TryRegisterPending(PendingBattle battle) => + _pending.TryAdd(battle.BattleId, battle); public PendingBattle? TryGetPending(string battleId) => _pending.TryGetValue(battleId, out var b) ? b : null; diff --git a/SVSim.UnitTests/BattleNode/Bridge/MatchingBridgeTests.cs b/SVSim.UnitTests/BattleNode/Bridge/MatchingBridgeTests.cs index a0f030d..84ac36a 100644 --- a/SVSim.UnitTests/BattleNode/Bridge/MatchingBridgeTests.cs +++ b/SVSim.UnitTests/BattleNode/Bridge/MatchingBridgeTests.cs @@ -76,6 +76,21 @@ public class MatchingBridgeTests new BattlePlayer(1, FixtureCtx()), new BattlePlayer(2, FixtureCtx()), BattleType.Bot)); } + [Test] + public void RegisterBattle_evicts_stale_pending_for_same_viewer() + { + var store = new InMemoryBattleSessionStore(); + var bridge = new MatchingBridge(store, new BattleNodeOptions()); + var p1 = new BattlePlayer(42, FixtureCtx()); + + var first = bridge.RegisterBattle(p1, p2: null, BattleType.Bot); + Assert.That(store.TryGetPending(first.BattleId), Is.Not.Null); + + var second = bridge.RegisterBattle(p1, p2: null, BattleType.Bot); + Assert.That(store.TryGetPending(first.BattleId), Is.Null, "stale entry must be evicted"); + Assert.That(store.TryGetPending(second.BattleId), Is.Not.Null); + } + private static MatchContext FixtureCtx() => new( SelfDeckCardIds: Enumerable.Range(1, 30).Select(i => 100_011_010L).ToList(), ClassId: "1", CharaId: "1", CardMasterName: "card_master_node_10015", diff --git a/SVSim.UnitTests/BattleNode/Sessions/InMemoryBattleSessionStoreTests.cs b/SVSim.UnitTests/BattleNode/Sessions/InMemoryBattleSessionStoreTests.cs index e24e31c..1460006 100644 --- a/SVSim.UnitTests/BattleNode/Sessions/InMemoryBattleSessionStoreTests.cs +++ b/SVSim.UnitTests/BattleNode/Sessions/InMemoryBattleSessionStoreTests.cs @@ -7,39 +7,40 @@ namespace SVSim.UnitTests.BattleNode.Sessions; [TestFixture] public class InMemoryBattleSessionStoreTests { - private InMemoryBattleSessionStore _store = null!; - - [SetUp] public void Setup() => _store = new InMemoryBattleSessionStore(); - [Test] - public void RegisterThenGet_ReturnsRegisteredBattle() + public void TryRegisterThenGet_ReturnsRegisteredBattle() { + var store = new InMemoryBattleSessionStore(); var battle = new PendingBattle("bid-1", BattleType.Bot, new BattlePlayer(906243102, FixtureCtx()), null); - _store.RegisterPending(battle); + Assert.That(store.TryRegisterPending(battle), Is.True); - Assert.That(_store.TryGetPending("bid-1"), Is.EqualTo(battle)); + Assert.That(store.TryGetPending("bid-1"), Is.EqualTo(battle)); } [Test] public void Get_UnknownBattleId_ReturnsNull() { - Assert.That(_store.TryGetPending("nope"), Is.Null); + var store = new InMemoryBattleSessionStore(); + Assert.That(store.TryGetPending("nope"), Is.Null); } [Test] public void Remove_ReturnsTrueWhenPresent_FalseWhenAbsent() { - _store.RegisterPending(new PendingBattle("bid", BattleType.Bot, new BattlePlayer(1, FixtureCtx()), null)); - Assert.That(_store.RemovePending("bid"), Is.True); - Assert.That(_store.RemovePending("bid"), Is.False); + var store = new InMemoryBattleSessionStore(); + store.TryRegisterPending(new PendingBattle("bid", BattleType.Bot, new BattlePlayer(1, FixtureCtx()), null)); + Assert.That(store.RemovePending("bid"), Is.True); + Assert.That(store.RemovePending("bid"), Is.False); } [Test] - public void Register_DuplicateBattleId_OverwritesPrior() + public void TryRegister_DuplicateBattleId_ReturnsFalseAndPreservesOriginal() { - _store.RegisterPending(new PendingBattle("bid", BattleType.Bot, new BattlePlayer(1, FixtureCtx()), null)); - _store.RegisterPending(new PendingBattle("bid", BattleType.Bot, new BattlePlayer(2, FixtureCtx()), null)); - Assert.That(_store.TryGetPending("bid")!.P1.ViewerId, Is.EqualTo(2)); + var store = new InMemoryBattleSessionStore(); + store.TryRegisterPending(new PendingBattle("bid", BattleType.Bot, new BattlePlayer(1, FixtureCtx()), null)); + var second = store.TryRegisterPending(new PendingBattle("bid", BattleType.Bot, new BattlePlayer(2, FixtureCtx()), null)); + Assert.That(second, Is.False); + Assert.That(store.TryGetPending("bid")!.P1.ViewerId, Is.EqualTo(1)); } private static MatchContext FixtureCtx() => new(