Merge pull request #171 from simoleo89/fix/trading-persistence-abort

fix(trading): harden trade lifecycle

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<HabboItem> 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<HabboItem> itemsUserOne = new THashSet<>(userOne.getItems());

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<RoomTrade> 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;
+    }
 }

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");
+    }
+}

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<HabboItem> 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");
+    }
+}