Production hardening: fix all critical audit issues

CRITICAL fixes:
- C-1/C-2/C-4: Auto-save and logout now run on MAIN THREAD. All entity
  reads (inventory, curios, effects) were happening off-thread, causing
  duplication exploits (player interacts during save → items duplicated).
  Auto-save uses tryLock() to skip players already being saved.
- C-5: NPE fix for non-RS2 items (null check on registry key lookup)
- C-6: RS2 .dat file written atomically (temp file + rename) to prevent
  corruption of entire RS2 storage on crash mid-write

HIGH fixes:
- H-3: Deadlock prevention: lock released BEFORE latch.await() in
  doPlayerJoin. Prevents shutdown deadlock where background thread
  holds lock while waiting for main thread, and shutdown holds main
  thread while waiting for lock.
- H-5: Curios cache now works WITHOUT keepInventory. Players who die
  then disconnect before respawning no longer lose curios data.
- H-8: server_id SQL uses PreparedStatements instead of string concat

MEDIUM fixes:
- M-1: NumberFormatException in LocalJsonUtil caught per-entry instead
  of crashing entire map parse (prevents losing all cosmetic armor)

Vyrriox

src/main/java/vip/fubuki/playersync/PlayerSync.java

@@ -117,16 +117,15 @@ public class PlayerSync {
                         "PRIMARY KEY (`id`)" +
                         ");"
         );
+        // FIX H-8: Use prepared statements for server_id to prevent SQL injection from config
         long current = System.currentTimeMillis();
-        JDBCsetUp.executeUpdate(
-                "INSERT INTO `" + dbName + "`.`server_info`(id,enable,last_update) " +
-                        "VALUES(" + JdbcConfig.SERVER_ID.get() + ",true," + current + ") " +
-                        "ON DUPLICATE KEY UPDATE id= " + JdbcConfig.SERVER_ID.get() + ",enable = 1," +
-                        "last_update=" + current + ";"
+        JDBCsetUp.executePreparedUpdate(
+                "INSERT INTO `" + dbName + "`.`server_info`(id,enable,last_update) VALUES(?,true,?) ON DUPLICATE KEY UPDATE id=VALUES(id),enable=1,last_update=VALUES(last_update)",
+                JdbcConfig.SERVER_ID.get(), current
         );
-        JDBCsetUp.executeUpdate(
-                "UPDATE `" + dbName + "`.`server_info` SET last_update=" + System.currentTimeMillis() +
-                        " WHERE id='" + JdbcConfig.SERVER_ID.get() + "'"
+        JDBCsetUp.executePreparedUpdate(
+                "UPDATE `" + dbName + "`.`server_info` SET last_update=? WHERE id=?",
+                System.currentTimeMillis(), JdbcConfig.SERVER_ID.get()
         );
 
         // Create curios table if the Curios mod is loaded

src/main/java/vip/fubuki/playersync/sync/VanillaSync.java

@@ -389,16 +389,22 @@ public class VanillaSync {
                 }
             });
 
-            // Wait for main thread to finish applying (prevents lock release before data is applied)
+            // FIX H-3: Release lock BEFORE waiting on latch to prevent deadlock.
+            // If we hold the lock while waiting, onServerShutdown trying to acquire
+            // the same lock will deadlock (shutdown blocks main thread, preventing
+            // server.execute() from draining, preventing latch countdown).
+            lock.unlock();
+
             if (!applyLatch.await(15, TimeUnit.SECONDS)) {
                 PlayerSync.LOGGER.error("Timeout waiting for main thread sync for player {}", player_uuid);
                 syncNotCompletedPlayer.remove(player_uuid);
             }
+            return; // Lock already released, skip finally
         } catch (Exception e) {
             PlayerSync.LOGGER.error("Internal Exception detected!", e);
             syncNotCompletedPlayer.remove(player_uuid);
         } finally {
-            lock.unlock();
+            if (lock.isHeldByCurrentThread()) lock.unlock();
         }
     }
 
@@ -726,54 +732,54 @@ public class VanillaSync {
     }
 
     /**
-     * FIX: All save operations (inventory, curios, mod-compat) are now under the per-player lock
-     * to prevent race conditions with concurrent auto-save tasks on the executor.
+     * FIX C-2: All save operations run on the MAIN THREAD (onPlayerLogout fires on main thread).
+     * Entity state (inventory, curios, effects) is read safely on the correct thread.
+     * DB writes block briefly but this is required for correctness.
      */
-    public static void doPlayerLogout(PlayerEvent.PlayerLoggedOutEvent event) throws SQLException, IOException {
-        String player_uuid = event.getEntity().getUUID().toString();
-        Player player = event.getEntity();
-        ReentrantLock lock = getPlayerLock(player_uuid);
-        lock.lock();
-        try {
-            // Save ALL data under lock: curios, mod-compat, then main inventory, then mark offline
-            if (ModList.get().isLoaded("curios")) {
-                ModsSupport modsSupport = new ModsSupport();
-                if (player.isDeadOrDying()) {
-                    modsSupport.saveCuriosFromCacheOrApi(player);
-                } else {
-                    modsSupport.onPlayerLeave(player);
-                }
-            }
-            ModCompatSync.storeAll(player);
-            store(player, false);
-            JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid);
-        } finally {
-            lock.unlock();
-            removePlayerLock(player_uuid);
-        }
-    }
-
     @SubscribeEvent
-    public static void onPlayerLogout(PlayerEvent.PlayerLoggedOutEvent event) throws SQLException {
+    public static void onPlayerLogout(PlayerEvent.PlayerLoggedOutEvent event) {
         String player_uuid = event.getEntity().getUUID().toString();
         if (deadPlayerWhileLogging.contains(player_uuid)) {
             PlayerSync.LOGGER.warn("A dead or dying player was kicked, uuid: {}", player_uuid);
-            JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid);
+            try {
+                JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid);
+            } catch (SQLException e) {
+                PlayerSync.LOGGER.error("Error marking dead player offline: {}", player_uuid, e);
+            }
             deadPlayerWhileLogging.remove(player_uuid);
         } else if (syncNotCompletedPlayer.contains(player_uuid)) {
             PlayerSync.LOGGER.warn("Player {} logged out with uncompleted sync. Data won't be saved for safety.", player_uuid);
-            JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid);
+            try {
+                JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid);
+            } catch (SQLException e) {
+                PlayerSync.LOGGER.error("Error marking unsynced player offline: {}", player_uuid, e);
+            }
             syncNotCompletedPlayer.remove(player_uuid);
         } else {
-            // FIX: All saves moved inside doPlayerLogout under the per-player lock
-            // to prevent race conditions with auto-save
-            executorService.submit(() -> {
-                try {
-                    doPlayerLogout(event);
-                } catch (Exception e) {
-                    PlayerSync.LOGGER.error("Error during player logout save for {}", player_uuid, e);
+            Player player = event.getEntity();
+            ReentrantLock lock = getPlayerLock(player_uuid);
+            lock.lock();
+            try {
+                // Save curios (main thread - safe to read Curios API)
+                if (ModList.get().isLoaded("curios")) {
+                    ModsSupport modsSupport = new ModsSupport();
+                    if (player.isDeadOrDying()) {
+                        modsSupport.saveCuriosFromCacheOrApi(player);
+                    } else {
+                        modsSupport.onPlayerLeave(player);
+                    }
                 }
-            });
+                // Save mod compat data (main thread - safe to read Accessories/CosmeticArmor)
+                ModCompatSync.storeAll(player);
+                // Save main inventory + effects + advancements (main thread - safe)
+                store(player, false);
+                JDBCsetUp.executePreparedUpdate("UPDATE player_data SET online=0 WHERE uuid=?", player_uuid);
+            } catch (Exception e) {
+                PlayerSync.LOGGER.error("Error during player logout save for {}", player_uuid, e);
+            } finally {
+                lock.unlock();
+                removePlayerLock(player_uuid);
+            }
         }
     }
 
@@ -1002,42 +1008,34 @@ public class VanillaSync {
         }
 
         // Auto-save all online players
+        // FIX C-1/C-2/C-4: onServerTick runs on the MAIN THREAD. We call store() and mod saves
+        // directly here to safely read entity state (inventory, curios, effects, etc.).
+        // The DB writes inside store() block briefly (~1-5ms per player) but this is acceptable
+        // for a 60-second interval. This eliminates all off-thread entity access duplication exploits.
         if (autoSaveTickCounter >= AUTO_SAVE_INTERVAL_TICKS) {
             autoSaveTickCounter = 0;
             MinecraftServer server = ServerLifecycleHooks.getCurrentServer();
             if (server != null) {
                 for (ServerPlayer player : server.getPlayerList().getPlayers()) {
-                    // Skip dead players and players whose sync hasn't completed yet
                     if (player.isDeadOrDying() || syncNotCompletedPlayer.contains(player.getUUID().toString())) {
                         continue;
                     }
-                    executorService.submit(() -> {
-                        try {
-                            store(player, false);
-                        } catch (Exception e) {
-                            PlayerSync.LOGGER.error("Error auto-saving player {}", player.getUUID(), e);
+                    String puuid = player.getUUID().toString();
+                    ReentrantLock lock = getPlayerLock(puuid);
+                    if (!lock.tryLock()) continue; // Skip if already being saved (logout in progress)
+                    try {
+                        store(player, false);
+                        if (ModList.get().isLoaded("curios") && !player.isDeadOrDying()) {
+                            new ModsSupport().StoreCurios(player, false);
                         }
-                    });
-                    executorService.submit(() -> {
-                        try {
-                            // Only auto-save curios for alive players to prevent saving empty data
-                            if (!player.isDeadOrDying()) {
-                                new ModsSupport().StoreCurios(player, false);
-                            }
-                        } catch (SQLException e) {
-                            PlayerSync.LOGGER.error("Error auto-saving Curios data for player {}", player.getUUID(), e);
+                        if (!player.isDeadOrDying()) {
+                            ModCompatSync.storeAll(player);
                         }
-                    });
-                    // Auto-save mod compatibility data (Accessories, CosmeticArmor)
-                    executorService.submit(() -> {
-                        try {
-                            if (!player.isDeadOrDying()) {
-                                ModCompatSync.storeAll(player);
-                            }
-                        } catch (Exception e) {
-                            PlayerSync.LOGGER.error("Error auto-saving mod compat data for player {}", player.getUUID(), e);
-                        }
-                    });
+                    } catch (Exception e) {
+                        PlayerSync.LOGGER.error("Error auto-saving player {}", player.getUUID(), e);
+                    } finally {
+                        lock.unlock();
+                    }
                 }
             }
         }

src/main/java/vip/fubuki/playersync/sync/addons/CuriosCache.java

@@ -36,8 +36,11 @@ public class CuriosCache {
     //Create a method to store temporary curios data when player is dead.
     //Then check player status in the logged out event,and take a normal sync if player is alive.
     //If player is dead or dying,the cache will be used to prevent the empty data from the failure of getting handlerOpt.
+    // FIX H-5: Cache curios on death regardless of keepInventory. Without this,
+    // players on servers WITHOUT keepInventory who die then disconnect before respawning
+    // would have their curios data overwritten with empty data (Curios API returns empty for dead players).
     public static void tryStoreCuriosToCache(net.minecraft.world.entity.player.Player player) {
-        if (!ModList.get().isLoaded("curios") || !CuriosCache.isKeepInventoryActive(player)) {
+        if (!ModList.get().isLoaded("curios")) {
             return;
         }
 

src/main/java/vip/fubuki/playersync/sync/addons/ModsSupport.java

@@ -543,8 +543,14 @@ public class ModsSupport {
             if (modified) {
                 // Write the modified .dat file back and force RS2 to reload
                 fileNbt.put("data", dataNbt);
-                net.minecraft.nbt.NbtIo.writeCompressed(fileNbt, datFile.toPath());
-                PlayerSync.LOGGER.info("Wrote modified RS2 storage data file");
+                // FIX C-6: Atomic write - write to temp file then rename.
+                // Direct write can corrupt the ENTIRE RS2 storage for the server on crash mid-write.
+                java.nio.file.Path tmpPath = datFile.toPath().resolveSibling(datFile.getName() + ".tmp");
+                net.minecraft.nbt.NbtIo.writeCompressed(fileNbt, tmpPath);
+                java.nio.file.Files.move(tmpPath, datFile.toPath(),
+                        java.nio.file.StandardCopyOption.REPLACE_EXISTING,
+                        java.nio.file.StandardCopyOption.ATOMIC_MOVE);
+                PlayerSync.LOGGER.info("Wrote modified RS2 storage data file (atomic)");
 
                 // Force the StorageRepository to reload from disk
                 // The simplest way is via reflection on the data storage cache
@@ -599,6 +605,7 @@ public class ModsSupport {
         try {
             net.minecraft.resources.ResourceLocation loc =
                     net.minecraft.core.registries.BuiltInRegistries.ITEM.getKey(stack.getItem());
+            if (loc == null) return null; // FIX C-5: null check prevents NPE on unregistered items
             if (!loc.getNamespace().equals("refinedstorage") && !loc.getNamespace().equals("extradisks")) {
                 return null;
             }

src/main/java/vip/fubuki/playersync/util/LocalJsonUtil.java

@@ -30,7 +30,13 @@ public class LocalJsonUtil {
 
             String key = trim.substring(0, equalIndex);
             String value = trim.substring(equalIndex + 1);
-            map.put(keyParser.apply(key), value);
+            // FIX M-1: Catch parse exceptions per-entry to prevent one malformed key
+            // from emptying the entire map (e.g. cosmetic armor slots all lost)
+            try {
+                map.put(keyParser.apply(key), value);
+            } catch (Exception e) {
+                // Skip malformed entries instead of crashing the whole parse
+            }
         }
         return map;
     }