From c25cb2a9b6f14442ba3643c7dcbd694ac6784e89 Mon Sep 17 00:00:00 2001 From: simoleo89 Date: Sat, 13 Jun 2026 02:02:37 +0200 Subject: [PATCH] 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"); + } +}