From c25cb2a9b6f14442ba3643c7dcbd694ac6784e89 Mon Sep 17 00:00:00 2001 From: simoleo89 Date: Sat, 13 Jun 2026 02:02:37 +0200 Subject: [PATCH 1/2] fix(trading): abort item exchange when persistence fails RoomTrade previously caught SQLException during ownership updates but continued into the in-memory inventory and credit transfer path. That could desynchronize or duplicate trade results if the database batch failed while the live session still completed the exchange. Keep item owner mutations after the successful batch, return offered items on failed completion, and add a contract test that prevents SQL failures from falling through to the transfer path. --- .../eu/habbo/habbohotel/rooms/RoomTrade.java | 31 +++++++++++++-- .../rooms/RoomTradeSafetyContractTest.java | 38 +++++++++++++++++++ 2 files changed, 65 insertions(+), 4 deletions(-) create mode 100644 Emulator/src/test/java/com/eu/habbo/habbohotel/rooms/RoomTradeSafetyContractTest.java diff --git a/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTrade.java b/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTrade.java index 38d2b66f..24ca36ea 100644 --- a/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTrade.java +++ b/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTrade.java @@ -58,7 +58,7 @@ public class RoomTrade { public synchronized void offerItem(Habbo habbo, HabboItem item) { RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); - if (user.getItems().contains(item)) + if (user == null || item == null || user.getItems().contains(item)) return; habbo.getInventory().getItemsComponent().removeHabboItem(item); @@ -71,6 +71,9 @@ public class RoomTrade { public synchronized void offerMultipleItems(Habbo habbo, THashSet items) { RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); + if (user == null || items == null) + return; + for (HabboItem item : items) { if (!user.getItems().contains(item)) { habbo.getInventory().getItemsComponent().removeHabboItem(item); @@ -85,7 +88,7 @@ public class RoomTrade { public synchronized void removeItem(Habbo habbo, HabboItem item) { RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); - if (!user.getItems().contains(item)) + if (user == null || item == null || !user.getItems().contains(item)) return; habbo.getInventory().getItemsComponent().addItem(item); @@ -98,6 +101,9 @@ public class RoomTrade { public synchronized void accept(Habbo habbo, boolean value) { RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); + if (user == null) + return; + user.setAccepted(value); this.sendMessageToUsers(new TradeAcceptedComposer(user)); @@ -120,6 +126,9 @@ public class RoomTrade { RoomTradeUser user = this.getRoomTradeUserForHabbo(habbo); + if (user == null) + return; + user.confirm(); this.sendMessageToUsers(new TradeAcceptedComposer(user)); @@ -134,6 +143,12 @@ public class RoomTrade { if (this.tradeItems()) { this.closeWindow(); this.sendMessageToUsers(new TradeCompleteComposer()); + } else { + this.returnItems(); + for (RoomTradeUser roomTradeUser : this.users) { + roomTradeUser.clearItems(); + } + this.closeWindow(); } this.room.stopTrade(this); @@ -188,7 +203,6 @@ public class RoomTrade { try (PreparedStatement statement = connection.prepareStatement("UPDATE items SET user_id = ? WHERE id = ? LIMIT 1")) { try (PreparedStatement stmt = connection.prepareStatement("INSERT INTO room_trade_log_items (id, item_id, user_id) VALUES (?, ?, ?)")) { for (HabboItem item : userOne.getItems()) { - item.setUserId(userTwoId); statement.setInt(1, userTwoId); statement.setInt(2, item.getId()); statement.addBatch(); @@ -202,7 +216,6 @@ public class RoomTrade { } for (HabboItem item : userTwo.getItems()) { - item.setUserId(userOneId); statement.setInt(1, userOneId); statement.setInt(2, item.getId()); statement.addBatch(); @@ -224,6 +237,16 @@ public class RoomTrade { } } catch (SQLException e) { LOGGER.error("Caught SQL exception", e); + this.sendMessageToUsers(new TradeClosedComposer(userOne.getHabbo().getRoomUnit().getId(), TradeClosedComposer.ITEMS_NOT_FOUND)); + return false; + } + + for (HabboItem item : userOne.getItems()) { + item.setUserId(userTwo.getHabbo().getHabboInfo().getId()); + } + + for (HabboItem item : userTwo.getItems()) { + item.setUserId(userOne.getHabbo().getHabboInfo().getId()); } THashSet itemsUserOne = new THashSet<>(userOne.getItems()); diff --git a/Emulator/src/test/java/com/eu/habbo/habbohotel/rooms/RoomTradeSafetyContractTest.java b/Emulator/src/test/java/com/eu/habbo/habbohotel/rooms/RoomTradeSafetyContractTest.java new file mode 100644 index 00000000..d1614e84 --- /dev/null +++ b/Emulator/src/test/java/com/eu/habbo/habbohotel/rooms/RoomTradeSafetyContractTest.java @@ -0,0 +1,38 @@ +package com.eu.habbo.habbohotel.rooms; + +import org.junit.jupiter.api.Test; + +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +class RoomTradeSafetyContractTest { + private static String roomTradeSource() throws Exception { + return Files.readString(Path.of("src/main/java/com/eu/habbo/habbohotel/rooms/RoomTrade.java")); + } + + @Test + void sqlFailureStopsBeforeInventoryTransfer() throws Exception { + String source = roomTradeSource(); + int catchIndex = source.indexOf("catch (SQLException e)"); + int inventoryTransferIndex = source.indexOf("THashSet itemsUserOne"); + + assertTrue(catchIndex > -1, "RoomTrade must handle SQL failures explicitly"); + assertTrue(inventoryTransferIndex > catchIndex, "Inventory transfer should happen after SQL ownership updates"); + assertTrue(source.substring(catchIndex, inventoryTransferIndex).contains("return false"), + "SQL failures must abort the trade before in-memory inventory/credit transfer"); + } + + @Test + void itemOwnersChangeOnlyAfterDatabaseBatchSucceeds() throws Exception { + String source = roomTradeSource(); + int firstOwnerMutation = source.indexOf("item.setUserId("); + int batchExecution = source.indexOf("statement.executeBatch();"); + + assertTrue(firstOwnerMutation > -1, "RoomTrade should update in-memory item owners after commit"); + assertTrue(batchExecution > -1, "RoomTrade should persist item owner changes with a batch update"); + assertTrue(firstOwnerMutation > batchExecution, + "In-memory item owners must not change until the database batch has succeeded"); + } +} From 478c4c70b885813eb54f3d36e94cfd1fedc58709 Mon Sep 17 00:00:00 2001 From: simoleo89 Date: Sun, 14 Jun 2026 19:25:16 +0200 Subject: [PATCH 2/2] fix(trading): prevent duplicate active trades Guard RoomTradeManager.startTrade while holding the activeTrades lock so concurrent trade starts cannot register the same participant in multiple active trades before room status updates settle. Add a contract test covering the lock-scoped participant guard and keep the existing trade safety tests green. --- .../habbohotel/rooms/RoomTradeManager.java | 19 +++++++++++- .../rooms/RoomTradeManagerContractTest.java | 30 +++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 Emulator/src/test/java/com/eu/habbo/habbohotel/rooms/RoomTradeManagerContractTest.java diff --git a/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTradeManager.java b/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTradeManager.java index 6b4790ec..b7c77efe 100644 --- a/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTradeManager.java +++ b/Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTradeManager.java @@ -19,8 +19,13 @@ public class RoomTradeManager { * Starts a trade between two users. */ public void startTrade(Habbo userOne, Habbo userTwo) { - RoomTrade trade = new RoomTrade(userOne, userTwo, this.room); + RoomTrade trade; synchronized (this.activeTrades) { + if (this.hasActiveTrade(userOne) || this.hasActiveTrade(userTwo)) { + return; + } + + trade = new RoomTrade(userOne, userTwo, this.room); this.activeTrades.add(trade); } @@ -58,4 +63,16 @@ public class RoomTradeManager { public THashSet getActiveTrades() { return this.activeTrades; } + + private boolean hasActiveTrade(Habbo user) { + for (RoomTrade trade : this.activeTrades) { + for (RoomTradeUser habbo : trade.getRoomTradeUsers()) { + if (habbo.getHabbo() == user) { + return true; + } + } + } + + return false; + } } diff --git a/Emulator/src/test/java/com/eu/habbo/habbohotel/rooms/RoomTradeManagerContractTest.java b/Emulator/src/test/java/com/eu/habbo/habbohotel/rooms/RoomTradeManagerContractTest.java new file mode 100644 index 00000000..07ec566d --- /dev/null +++ b/Emulator/src/test/java/com/eu/habbo/habbohotel/rooms/RoomTradeManagerContractTest.java @@ -0,0 +1,30 @@ +package com.eu.habbo.habbohotel.rooms; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.nio.file.Files; +import java.nio.file.Path; + +import org.junit.jupiter.api.Test; + +class RoomTradeManagerContractTest { + private static String roomTradeManagerSource() throws Exception { + return Files.readString(Path.of("src/main/java/com/eu/habbo/habbohotel/rooms/RoomTradeManager.java")); + } + + @Test + void startTradeRejectsParticipantsAlreadyInActiveTradeInsideLock() throws Exception { + String source = roomTradeManagerSource(); + int synchronizedBlock = source.indexOf("synchronized (this.activeTrades)"); + int activeGuard = source.indexOf("hasActiveTrade(userOne) || this.hasActiveTrade(userTwo)"); + int addTrade = source.indexOf("this.activeTrades.add(trade)"); + + assertTrue(synchronizedBlock > -1, "RoomTradeManager.startTrade must lock activeTrades before mutation"); + assertTrue(activeGuard > synchronizedBlock, + "startTrade must check both participants for an existing active trade while holding the activeTrades lock"); + assertTrue(activeGuard < addTrade, + "duplicate participant guard must run before a new RoomTrade is added"); + assertTrue(source.contains("private boolean hasActiveTrade(Habbo user)"), + "active trade lookup should be reusable under the same activeTrades lock"); + } +}