From 097f1e4c5be64798896420bf412e050f1f793105 Mon Sep 17 00:00:00 2001 From: erwan-joly Date: Mon, 31 Aug 2026 17:33:06 +1200 Subject: [PATCH] fix(bazaar): a listing could be sold twice across channels The buy path delivered before it claimed: gold was deducted and the item was created and put in the buyer's inventory, and only then was DeleteBazaarAsync called. The listing lives on the master server and every channel reads it, so two buyers on different channels both passed the price and amount checks, both received an item, and only one claim succeeded. The loser kept the item and the gold was gone; the failure branch logged BAZAAR_BUY_ERROR and returned. The claim now comes first, and nothing is paid for or created unless it wins. The losing buyer keeps their gold, gets no item, and is told the offer changed rather than being met with silence. The claim itself was a lost update too. DeleteBazaarAsync and ModifyBazaarAsync read, check and write a listing with no serialisation, and SignalR dispatches hub calls concurrently, so two channels could both pass the amount check. They now take a lock per listing id, so unrelated trades still run in parallel. --- .../Services/BazaarService/BazaarService.cs | 11 ++++++ .../Bazaar/CBuyPacketHandler.cs | 35 ++++++++++++------- .../Bazaar/CBuyPacketHandlerTests.cs | 17 +++++++++ 3 files changed, 51 insertions(+), 12 deletions(-) diff --git a/src/NosCore.GameObject/Services/BazaarService/BazaarService.cs b/src/NosCore.GameObject/Services/BazaarService/BazaarService.cs index fc812325c..7d6fdc9da 100644 --- a/src/NosCore.GameObject/Services/BazaarService/BazaarService.cs +++ b/src/NosCore.GameObject/Services/BazaarService/BazaarService.cs @@ -6,6 +6,8 @@ using Json.More; using NodaTime; +using NosCore.Core.Concurrency; +using System.Collections.Concurrent; using NodaTime.Serialization.SystemTextJson; using NosCore.Dao.Interfaces; using NosCore.Data.Dto; @@ -26,6 +28,13 @@ public class BazaarService(IBazaarRegistry bazaarRegistry, IDao itemInstanceDao, IClock clock) : IBazaarService { + // Channels reach these through BazaarHub, and SignalR dispatches calls concurrently, so + // read-check-write on a listing is a lost update waiting to happen. One lock per listing + // rather than one for the bazaar, so unrelated trades still run in parallel. + private static readonly ConcurrentDictionary Claims = new(); + + private static AsyncLock ClaimLock(long id) => Claims.GetOrAdd(id, _ => new AsyncLock()); + public List GetBazaar(long id, byte? index, byte? pageSize, BazaarListType? typeFilter, byte? subTypeFilter, byte? levelFilter, byte? rareFilter, byte? upgradeFilter, long? sellerFilter) { @@ -154,6 +163,7 @@ public List GetBazaar(long id, byte? index, byte? pageSize, BazaarLi public async Task DeleteBazaarAsync(long id, short count, string requestCharacterName, long? requestCharacterId = null) { + using var claim = await ClaimLock(id).AcquireAsync(); var bzlink = bazaarRegistry.GetById(id); if (bzlink == null) { @@ -243,6 +253,7 @@ public async Task AddBazaarAsync(Guid itemInstanceId, long characte public async Task ModifyBazaarAsync(long id, Json.Patch.JsonPatch bzMod) { + using var claim = await ClaimLock(id).AcquireAsync(); var item = bazaarRegistry.GetById(id); if ((item?.BazaarItem == null) || (item.BazaarItem?.Amount != item.ItemInstance?.Amount)) { diff --git a/src/NosCore.PacketHandlers/Bazaar/CBuyPacketHandler.cs b/src/NosCore.PacketHandlers/Bazaar/CBuyPacketHandler.cs index 145d2dca6..7d9b4a1de 100644 --- a/src/NosCore.PacketHandlers/Bazaar/CBuyPacketHandler.cs +++ b/src/NosCore.PacketHandlers/Bazaar/CBuyPacketHandler.cs @@ -55,21 +55,25 @@ public override async Task ExecuteAsync(CBuyPacket packet, ClientSession clientS { if (clientSession.Character.Gold >= price) { - clientSession.Character.Gold -= price; - await clientSession.SendPacketAsync(clientSession.Character.GenerateGold()); - - var itemInstance = await itemInstanceDao.FirstOrDefaultAsync(s => s!.Id == bz.ItemInstance.Id); - var item = itemProvider.Convert(itemInstance!); - item.Id = Guid.NewGuid(); - var newInv = - clientSession.Character.InventoryService.AddItemToPocket( - InventoryItemInstance.Create(item, clientSession.Character.CharacterId)); - await clientSession.SendPacketAsync(newInv!.GeneratePocketChange()); - + // Claim the listing before anything is paid for or created. The listing + // is shared across channels, so two buyers can both pass the checks + // above; only one can win this call, and the loser must leave with + // neither the gold gone nor an item made. var remove = await bazaarHttpClient.DeleteBazaarAsync(packet.BazaarId, packet.Amount, clientSession.Character.Name); if (remove) { + clientSession.Character.Gold -= price; + await clientSession.SendPacketAsync(clientSession.Character.GenerateGold()); + + var itemInstance = await itemInstanceDao.FirstOrDefaultAsync(s => s!.Id == bz.ItemInstance.Id); + var item = itemProvider.Convert(itemInstance!); + item.Id = Guid.NewGuid(); + var newInv = + clientSession.Character.InventoryService.AddItemToPocket( + InventoryItemInstance.Create(item, clientSession.Character.CharacterId)); + await clientSession.SendPacketAsync(newInv!.GeneratePocketChange()); + await clientSession.HandlePacketsAsync(new[] { new CBListPacket { Index = 0, ItemVNumFilter = new List() } }); await clientSession.SendPacketAsync(new RCBuyPacket(bz.SellerName!) { @@ -95,7 +99,14 @@ await clientSession.SendPacketAsync(new SayiPacket return; } - logger.LogError(logLanguage[LogLanguageKey.BAZAAR_BUY_ERROR]); + // Someone else took it first, most likely from another channel. + logger.LogInformation(logLanguage[LogLanguageKey.BAZAAR_BUY_ERROR]); + await clientSession.SendPacketAsync(new ModaliPacket + { + Type = 1, + Message = Game18NConstString.OfferUpdated + }); + await clientSession.HandlePacketsAsync(new[] { new CBListPacket { Index = 0, ItemVNumFilter = new List() } }); } else { diff --git a/test/NosCore.PacketHandlers.Tests/Bazaar/CBuyPacketHandlerTests.cs b/test/NosCore.PacketHandlers.Tests/Bazaar/CBuyPacketHandlerTests.cs index dd7c0b460..37b88da10 100644 --- a/test/NosCore.PacketHandlers.Tests/Bazaar/CBuyPacketHandlerTests.cs +++ b/test/NosCore.PacketHandlers.Tests/Bazaar/CBuyPacketHandlerTests.cs @@ -302,5 +302,22 @@ private void ShouldReceiveInsufficientGoldMessage() var packet = (ModaliPacket?)Session.LastPackets.FirstOrDefault(s => s is ModaliPacket); Assert.IsTrue(packet?.Message == Game18NConstString.InsufficientGoldAvailable); } + + [TestMethod] + public async Task LosingTheRaceForAListingCostsNeitherGoldNorMakesAnItem() + { + // The listing lives on the master server and every channel reads it, so two buyers + // can both pass the price and amount checks. Only one wins the claim. + Session.Character.Gold = 500; + BazaarHttpClient! + .Setup(b => b.DeleteBazaarAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(false); + + await CbuyPacketHandler!.ExecuteAsync(new CBuyPacket { BazaarId = 0, Amount = 1, Price = 50 }, Session); + + Assert.AreEqual(500, Session.Character.Gold, "the loser must keep their gold"); + Assert.AreEqual(0, Session.Character.InventoryService.Count, "the loser must not receive an item"); + } + } }