From 7e39732e98dd0cbece95cfc0aec68bb79c277fef Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:01:04 +0100 Subject: [PATCH 1/4] perf: batch table saves that move no money and skip identical rewrites Card draws, returns, mucks, recycles, reshuffles, the poker fold commit, wipeHands and the seat added after a stake now mark the table unsaved; the hand clock writes marked tables every 20 ticks and onDisable writes them after the reset. Every money save still writes the whole table and clears the mark. TableOwnershipStore.save skips the write when check() has just found the exact bytes this process last wrote in full. Bytes found on disk, a failed write or a failed publication never count as written. Deferred loading no longer re-reads, parses and hashes loaded tables whose file check() has just verified and that need no migration; the per-pass full check() of every loaded table stays. Co-Authored-By: Claude Opus 5.5 --- .../java/net/tfminecraft/games/Games.java | 8 +- .../games/ownership/TableOwnershipStore.java | 46 +++- .../tfminecraft/games/table/TableManager.java | 123 +++++++++-- .../tfminecraft/games/GamesLifecycleTest.java | 14 ++ .../ownership/TableOwnershipStoreTest.java | 125 +++++++++++ .../table/TableManagerSaveBatchingTest.java | 199 ++++++++++++++++++ 6 files changed, 480 insertions(+), 35 deletions(-) create mode 100644 src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java diff --git a/src/main/java/net/tfminecraft/games/Games.java b/src/main/java/net/tfminecraft/games/Games.java index 1d19aed..847c9f4 100644 --- a/src/main/java/net/tfminecraft/games/Games.java +++ b/src/main/java/net/tfminecraft/games/Games.java @@ -84,7 +84,13 @@ public void onEnable() { @Override public void onDisable() { TableManager.get().stopClock(); - TableManager.get().despawnWorldAll(); + try { + TableManager.get().despawnWorldAll(); + } finally { + // Tables that were not reset above (money held for review) still get their last + // card changes written. + TableManager.get().flushUnsaved(); + } DisplayManager.get().shutdown(); ProtocolLibBridge.shutdown(); if(ownershipActive && TableManager.get().tables().stream().noneMatch(table->table.ownershipHeld() || table.moneyBlocked() diff --git a/src/main/java/net/tfminecraft/games/ownership/TableOwnershipStore.java b/src/main/java/net/tfminecraft/games/ownership/TableOwnershipStore.java index a58180d..6555ef6 100644 --- a/src/main/java/net/tfminecraft/games/ownership/TableOwnershipStore.java +++ b/src/main/java/net/tfminecraft/games/ownership/TableOwnershipStore.java @@ -20,7 +20,11 @@ public Conflict(String message) { public record Row( Path path, UUID id, UUID account, UUID character, OwnershipKind kind, JsonObject raw) {} - private record Source(Path path, String hash, JsonObject raw) {} + /** + * The bytes this store last saw at a table's path. {@code durable} is set only when this + * process wrote them itself and the whole write, directory flush included, succeeded. + */ + private record Source(Path path, String hash, JsonObject raw, boolean durable) {} private final Path folder; private final OwnershipMigration migration; @@ -64,7 +68,10 @@ private void captureSource(Path file) throws IOException { conflicts.add(id); throw new Conflict("Duplicate table ID"); } - sources.put(id, new Source(file, OwnershipMigration.hash(Files.readAllBytes(file)), raw)); + String hash = OwnershipMigration.hash(Files.readAllBytes(file)); + // Reading back the very bytes this process wrote keeps them known as fully written. + boolean durable = previous != null && previous.durable() && previous.hash().equals(hash); + sources.put(id, new Source(file, hash, raw, durable)); rememberWagers(id, raw); } @@ -153,17 +160,33 @@ public void save(UUID id, JsonObject known) throws IOException { JsonObject next = previous == null ? known.deepCopy() : overlay(id, previous.raw(), known); OwnershipMigration.validateRow(next); String text = RawJson.JSON.toJson(next); + String hash = OwnershipMigration.hash(text.getBytes(java.nio.charset.StandardCharsets.UTF_8)); + // check() has just read the file and found exactly the bytes this process last wrote + // there in full, so the same bytes again need no new write or flush. A failed write never + // reaches sources, so whatever it tried to write is still written next time. + if (previous != null && previous.durable() && previous.hash().equals(hash)) return; DurableFiles.write(file, text); - sources.put( - id, - new Source( - file, - OwnershipMigration.hash( - text.getBytes(java.nio.charset.StandardCharsets.UTF_8)), - next)); + sources.put(id, new Source(file, hash, next, true)); rememberWagers(id, next); } + /** + * The file and raw row this store last wrote or read for a table, or null. Callers that + * trust it must run {@link #check(UUID)} first. + */ + public Row source(UUID id) { + Source source = sources.get(id); + if (source == null) return null; + JsonObject raw = source.raw().deepCopy(); + return new Row( + source.path(), + id, + RawJson.uuid(raw.get("ownerPlayer")), + RawJson.uuid(raw.get("ownerCharacterId")), + OwnershipKind.of(raw), + raw); + } + public void publish(Row before, JsonObject next) throws IOException { if (!before.raw().equals(row(before.path()).raw())) { conflicts.add(before.id()); @@ -182,7 +205,7 @@ public void publish(Row before, JsonObject next) throws IOException { if (Files.isRegularFile(before.path()) && afterHash.equals( OwnershipMigration.hash(Files.readAllBytes(before.path())))) { - sources.put(before.id(), new Source(before.path(), afterHash, next)); + sources.put(before.id(), new Source(before.path(), afterHash, next, false)); publicationPending.add(before.id()); } throw ex; @@ -194,7 +217,8 @@ public void publish(Row before, JsonObject next) throws IOException { before.path(), OwnershipMigration.hash( text.getBytes(java.nio.charset.StandardCharsets.UTF_8)), - next)); + next, + true)); rememberWagers(before.id(), next); } diff --git a/src/main/java/net/tfminecraft/games/table/TableManager.java b/src/main/java/net/tfminecraft/games/table/TableManager.java index 1ce3f93..30c25da 100644 --- a/src/main/java/net/tfminecraft/games/table/TableManager.java +++ b/src/main/java/net/tfminecraft/games/table/TableManager.java @@ -143,6 +143,9 @@ public final class TableManager implements Listener, WagerHost { private final Set tableDealing = new HashSet<>(); private BukkitTask handClock; private int handTicks; + private static final int UNSAVED_FLUSH_TICKS = 20; + // Tables with a change that moved no money and is not on disk yet; see saveSoon. + private final Set unsaved = new LinkedHashSet<>(); private TableManager() { WagerEngine.init(this); @@ -197,6 +200,9 @@ public void stopClock() { private void tickHands() { publishChatActors(); handTicks++; + if (handTicks % UNSAVED_FLUSH_TICKS == 0) { + flushUnsaved(); + } for (Table table : tables.values()) { tickAway(table); if (handTicks % 20 == 0) { @@ -352,10 +358,13 @@ private boolean validOwnerFile(File file) throws IOException { } invalidOwnerFiles.remove(file.getName());return true; } - private Set duplicateOwnerIds(File[] files) throws IOException { + private Set duplicateOwnerIds(File[] files, Map verified) throws IOException { ownershipInventoryComplete=true; Set seen=new HashSet<>(),duplicates=new HashSet<>(); for(File file:files) { + // A verified loaded table's file has just been checked byte for byte, so its ID is known. + UUID known=verified.get(file.toPath().toAbsolutePath().normalize()); + if(known!=null) { if(!seen.add(known)) duplicates.add(known);continue; } // No mutation is allowed before the complete raw inventory establishes uniqueness. com.google.gson.JsonObject raw; try { raw=net.tfminecraft.games.ownership.RawJson.read(file.toPath()); } @@ -367,8 +376,12 @@ private Set duplicateOwnerIds(File[] files) throws IOException { } private void loadTables(boolean onlyMissing) { + Map verified = new HashMap<>(); for (Table loaded : tables.values()) { - try { ownershipStore().check(loaded.getId()); } + try { + ownershipStore().check(loaded.getId()); + if (onlyMissing) verifiedSource(loaded, verified); + } catch (IOException ex) { ownershipStore().hold(loaded.getId()); } } File folder = tablesFolder(); @@ -377,13 +390,24 @@ private void loadTables(boolean onlyMissing) { return; } Set duplicates; - try { duplicates=duplicateOwnerIds(files); } + try { duplicates=duplicateOwnerIds(files, verified); } catch(IOException ex) { // Cache raw shape failures for the usual repair-and-retry path without publishing others. for(File file:files) try { validOwnerFile(file); } catch(IOException ignored) { } return; } for (File file : files) { + UUID verifiedId = verified.get(file.toPath().toAbsolutePath().normalize()); + if (verifiedId != null && !duplicates.contains(verifiedId)) { + // Loaded, unchanged and needing no migration: the steps below would only read the + // same bytes again and skip it, so take the owner straight from what was checked. + Table loaded = tables.get(verifiedId); + if (loaded != null) { + loaded.setOwnerCharacterId(ownershipStore().source(verifiedId).character()); + syncOwnership(loaded); + } + continue; + } try { if(!validOwnerFile(file)) continue; UUID rawId=net.tfminecraft.games.ownership.RawJson.uuid(net.tfminecraft.games.ownership.RawJson.read(file.toPath()).get("id")); @@ -471,6 +495,28 @@ private void loadTables(boolean onlyMissing) { } } + /** + * Note a loaded table whose file a deferred pass need not read again: check() has just found + * exactly the bytes this store last wrote or read, nothing holds the table, and neither the + * guild nor the ownership migration would change those bytes. Anything else takes the full + * pass, exactly as before. + */ + private void verifiedSource(Table loaded, Map verified) { + var store = ownershipStore(); + var source = store.source(loaded.getId()); + if (source == null || store.held(loaded.getId())) return; + var raw = source.raw(); + // The same no-op rules as GuildTableMigration.prepare and OwnershipMigration.prepare. + var guild = raw.get("ownerGuildId"); + boolean guildDone = guild == null || guild.isJsonNull() + || (guild.isJsonPrimitive() && guild.getAsString().isBlank()) + || (raw.has("ownerGuildMigrated") && raw.get("ownerGuildMigrated").isJsonPrimitive() + && raw.get("ownerGuildMigrated").getAsBoolean()); + boolean ownerDone = source.account() == null || source.character() != null; + if (!guildDone || !ownerDone || !net.tfminecraft.games.ownership.OwnershipKind.modeRecorded(raw)) return; + verified.put(source.path().toAbsolutePath().normalize(), loaded.getId()); + } + private static String fileSignature(File file) { return file.lastModified() + ":" + file.length(); } @@ -563,7 +609,7 @@ public void wipeHands() { } table.getHands().clear(); despawnTablePiles(table); - save(table); + saveSoon(table); try { rebuildCardStacks(table); recycleIfNeeded(table, null); @@ -927,6 +973,7 @@ private void pickup(Player player, Table table) { String guildId = table.ownerGuildId(); despawnWorld(table); tables.remove(table.getId()); + unsaved.remove(table.getId()); deleteFile(table.getId()); // A table staff put down without a deck gives none back, or placing and lifting it would mint decks. ItemStack deckItem = table.deckConsumed() @@ -1190,7 +1237,7 @@ private boolean drawOneToTable(Table table, String pile, boolean faceUp, Runnabl int destIndex = cards.size(); int destCount = destIndex + 1; rebuildCardStacks(table); - save(table); + saveSoon(table); DisplayPose end = pileSlot(table, pile, destIndex, destCount, faceUp); ItemStack show = faceUp && face != null ? face : back; if (Cache.handDealTicks <= 0) { @@ -1198,13 +1245,13 @@ private boolean drawOneToTable(Table table, String pile, boolean faceUp, Runnabl if (!DisplayManager.get().spawn(tokenId, table.getOrigin(), show, end)) { table.getDeck().discard(card); rebuildCardStacks(table); - save(table); + saveSoon(table); return false; } HandCard held = new HandCard(card, tokenId, faceUp); cards.add(held); layoutTablePile(table, pile, 0); - save(table); + saveSoon(table); playCardSound(table.getOrigin()); Bukkit.getScheduler().runTaskLater(Games.plugin, after, 1L); return true; @@ -1217,7 +1264,7 @@ private boolean drawOneToTable(Table table, String pile, boolean faceUp, Runnabl if (!DisplayManager.get().spawn(courierId, courierOrigin, show, start)) { table.getDeck().discard(card); rebuildCardStacks(table); - save(table); + saveSoon(table); return false; } playCardSound(table.getOrigin()); @@ -1231,20 +1278,20 @@ private boolean drawOneToTable(Table table, String pile, boolean faceUp, Runnabl still.getDeck().discard(card); tableDealing.remove(tableId); rebuildCardStacks(still); - save(still); + saveSoon(still); return; } UUID tokenId = UUID.randomUUID(); if (!DisplayManager.get().spawn(tokenId, still.getOrigin(), show, end)) { still.getDeck().discard(card); rebuildCardStacks(still); - save(still); + saveSoon(still); onFailure.run(); return; } still.tablePile(pile).add(new HandCard(card, tokenId, faceUp)); layoutTablePile(still, pile, 0); - save(still); + saveSoon(still); after.run(); }); return true; @@ -1291,7 +1338,7 @@ static Location discardOrigin(Table table) { public void reshuffleFull(Table table) { table.getDeck().reshuffleAll(); rebuildCardStacks(table); - save(table); + saveSoon(table); } private void recycleIfNeeded(Table table, Runnable after) { @@ -1374,7 +1421,7 @@ private void finishRecycle(Table table, Runnable after) { table.setRecycling(false); table.getDeck().recycle(); rebuildCardStacks(table); - save(table); + saveSoon(table); runPending(after); } @@ -1468,7 +1515,7 @@ public void commitStreet(Player player) { if (value < need) { // need is only above zero when a game asked for it, so game is set here. refundStreet(table, player, street); - save(table); + saveSoon(table); game.onStreetCommit(table, player, value, true); player.sendMessage(Messages.get("wager.folded")); return; @@ -2116,7 +2163,7 @@ public void muckPlayer(Table table, Player player) { public void muckPlayer(Table table, UUID playerId) { discardPlayerCards(table, playerId); rebuildCardStacks(table); - save(table); + saveSoon(table); recycleIfNeeded(table, null); } @@ -2134,7 +2181,7 @@ public void muckTable(Table table, String name) { table.tablePiles().remove(pile); } rebuildCardStacks(table); - save(table); + saveSoon(table); notifyTablePiles(table); recycleIfNeeded(table, null); } @@ -2994,7 +3041,7 @@ private boolean dumpLoot(Player player, Table table, ItemStack snapshot, int den playChipSound(table, at); if (!dealerTray) { table.actives().add(player.getUniqueId()); - save(table); + saveSoon(table); tryBeginSession(table); notifyChipIn(table, player, need, one); } @@ -3130,7 +3177,7 @@ private boolean tryPlaceChip(Player player, Location click) { playChipSound(table, hit); if (!dealerTray) { table.actives().add(player.getUniqueId()); - save(table); + saveSoon(table); tryBeginSession(table); notifyChipIn(table, player, denars, one); } @@ -4189,7 +4236,7 @@ private boolean drawOneToPlayer(Table table, Player player, int slot, Runnable a destCount = destIndex + 1; } rebuildCardStacks(table); - save(table); + saveSoon(table); HandLock lock = lockHand(table, player, true); Location anchor = lockLocation(player, lock); // The incoming card may be the first in a freshly split group, so it can widen the span. @@ -4223,7 +4270,7 @@ private boolean drawOneToPlayer(Table table, Player player, int slot, Runnable a if (!DisplayManager.get().spawn(courierId, courierOrigin, back, start)) { table.getDeck().discard(card); rebuildCardStacks(table); - save(table); + saveSoon(table); player.sendMessage(Messages.get("place.spawn_failed")); return false; } @@ -4258,7 +4305,7 @@ private HandCard spawnPrivateHandCard(Table table, Player player, Card card, Ite if (!DisplayManager.get().spawn(tokenId, table.getOrigin(), back, fan)) { table.getDeck().discard(card); rebuildCardStacks(table); - save(table); + saveSoon(table); return null; } DisplayManager.get().setLayoutOwner(tokenId, player.getUniqueId()); @@ -4269,7 +4316,7 @@ private HandCard spawnPrivateHandCard(Table table, Player player, Card card, Ite HandCard held = new HandCard(card, tokenId); held.setSlot(slot); table.handOf(player.getUniqueId()).add(held); - save(table); + saveSoon(table); return held; } @@ -4323,7 +4370,7 @@ private void finishReturnCard(Table table, Player player, Card card, UUID courie removePendingCard(playerId, card); table.getDeck().discard(card); rebuildCardStacks(table); - save(table); + saveSoon(table); if (dealStillFlying(playerId)) { return; } @@ -4810,10 +4857,40 @@ private Table tableFrom(Entity entity) { } public void saveWager(Table table) throws IOException { + // The whole table is written, so any change that was waiting goes with it. + unsaved.remove(table.getId()); ownershipStore().save(table.getId(), net.tfminecraft.games.ownership.RawJson.JSON.toJsonTree(toData(table)).getAsJsonObject()); } + /** + * Write a change that moved no money (cards drawn, returned, mucked or shuffled, a seat + * taken) on the next flush rather than now. Hands are never saved and a table that was in + * play is reset when it loads, so a crash before the flush loses nothing a restart keeps. + * Every money movement still saves the whole table at once through {@link #save}. + */ + private void saveSoon(Table table) { + unsaved.add(table.getId()); + } + + /** Write every table with a change still waiting. Runs every second and on shutdown. */ + public void flushUnsaved() { + for (UUID id : new ArrayList<>(unsaved)) { + Table table = tables.get(id); + if (table == null) { + unsaved.remove(id); + continue; + } + try { + save(table); + } catch (RuntimeException ex) { + // save() has already cleared the mark, so one bad table cannot stall the clock. + Games.plugin.getLogger().warning("[Games] Failed to save table " + id + ": " + ex); + } + } + } + private void save(Table table) { + unsaved.remove(table.getId()); File file = tableFile(table.getId()); TableData data = toData(table); try { diff --git a/src/test/java/net/tfminecraft/games/GamesLifecycleTest.java b/src/test/java/net/tfminecraft/games/GamesLifecycleTest.java index debf53e..d9474b3 100644 --- a/src/test/java/net/tfminecraft/games/GamesLifecycleTest.java +++ b/src/test/java/net/tfminecraft/games/GamesLifecycleTest.java @@ -119,12 +119,26 @@ void cleanStopRemovesTheMarkerWhileUnresolvedMoneyRetainsIt() throws Exception { MockBukkit.getMock().getPluginManager().enablePlugin(plugin); var marker=plugin.getDataFolder().toPath().resolve("ownership-active");assertTrue(Files.exists(marker)); MockBukkit.getMock().getPluginManager().disablePlugin(plugin);assertFalse(Files.exists(marker)); + // Card changes still waiting are written after the tables are reset, before stopping. + var order=inOrder(manager);order.verify(manager).despawnWorldAll();order.verify(manager).flushUnsaved(); MockBukkit.getMock().getPluginManager().enablePlugin(plugin); var table=mock(net.tfminecraft.games.table.Table.class);when(table.moneyBlocked()).thenReturn(true);when(manager.tables()).thenReturn(List.of(table)); MockBukkit.getMock().getPluginManager().disablePlugin(plugin);assertTrue(Files.exists(marker)); }); } + @Test + void waitingTableChangesAreWrittenEvenWhenResettingTablesFailsOnStop() throws Exception { + withLoadedPlugin(true, (plugin, manager, records) -> { + MockBukkit.getMock().getPluginManager().enablePlugin(plugin); + doThrow(new IllegalStateException("reset failed")).when(manager).despawnWorldAll(); + try { MockBukkit.getMock().getPluginManager().disablePlugin(plugin); } + catch (RuntimeException ignored) { /* the server reports a failing disable itself */ } + verify(manager).flushUnsaved(); + doNothing().when(manager).despawnWorldAll(); + }); + } + @Test void incompletePluginDescriptorReportsBothMissingCommandsAndStillStartsTableServices() throws Exception { withLoadedPlugin(false, (plugin, manager, records) -> { diff --git a/src/test/java/net/tfminecraft/games/ownership/TableOwnershipStoreTest.java b/src/test/java/net/tfminecraft/games/ownership/TableOwnershipStoreTest.java index c6b8614..259d060 100644 --- a/src/test/java/net/tfminecraft/games/ownership/TableOwnershipStoreTest.java +++ b/src/test/java/net/tfminecraft/games/ownership/TableOwnershipStoreTest.java @@ -120,4 +120,129 @@ void rawInventoryIncludesUnloadableRowsAndRejectsDuplicateTableIds() throws Exce assertThrows(IOException.class, () -> store.prepare(path.resolveSibling(id + ".json"))); assertTrue(store.held(id)); } + + private static org.mockito.MockedStatic writes() { + return org.mockito.Mockito.mockStatic( + net.tfminecraft.games.persistence.DurableFiles.class, + org.mockito.Mockito.CALLS_REAL_METHODS); + } + + private static void verifyWrites( + org.mockito.MockedStatic writes, + int times) { + writes.verify( + () -> + net.tfminecraft.games.persistence.DurableFiles.write( + org.mockito.ArgumentMatchers.any(Path.class), + org.mockito.ArgumentMatchers.anyString()), + org.mockito.Mockito.times(times)); + } + + @Test + void identicalBytesAreNotWrittenAgainButAnyChangeIs() throws Exception { + var store = new TableOwnershipStore(data); + Path path = data.resolve("Data/tables/" + id + ".json"); + try (var writes = writes()) { + store.save(id, row()); + verifyWrites(writes, 1); + String first = Files.readString(path); + store.save(id, row()); + store.save(id, row()); + verifyWrites(writes, 1); + assertEquals(first, Files.readString(path)); + JsonObject changed = row(); + changed.addProperty("maxBet", 9); + store.save(id, changed); + verifyWrites(writes, 2); + assertEquals(9, RawJson.read(path).get("maxBet").getAsInt()); + store.save(id, changed); + verifyWrites(writes, 2); + } + } + + @Test + void bytesFoundOnDiskAreWrittenOnceBeforeTheyCountAsWritten() throws Exception { + JsonObject row = row(); + row.addProperty("staffMint", false); + new TableOwnershipStore(data).save(id, row); + Path path = data.resolve("Data/tables/" + id + ".json"); + String before = Files.readString(path); + var restart = new TableOwnershipStore(data); + assertTrue(restart.prepare(path)); + try (var writes = writes()) { + restart.save(id, row); + // Found, not written by this store: their directory entry may never have been flushed. + verifyWrites(writes, 1); + assertEquals(before, Files.readString(path)); + restart.save(id, row); + verifyWrites(writes, 1); + // Reading back the same bytes keeps them known as written. + restart.observe(path); + restart.save(id, row); + verifyWrites(writes, 1); + } + } + + @Test + void aFailedWriteIsTriedAgainAndKeepsThrowingUntilItSucceeds() throws Exception { + var store = new TableOwnershipStore(data); + Path path = data.resolve("Data/tables/" + id + ".json"); + store.save(id, row()); + String written = Files.readString(path); + JsonObject changed = row(); + changed.addProperty("maxBet", 9); + try (var writes = writes()) { + writes.when( + () -> + net.tfminecraft.games.persistence.DurableFiles.write( + org.mockito.ArgumentMatchers.any(Path.class), + org.mockito.ArgumentMatchers.anyString())) + .thenThrow(new IOException("disk full")); + assertThrows(IOException.class, () -> store.save(id, changed)); + // The same bytes again are not mistaken for written ones: a money save still fails. + assertThrows(IOException.class, () -> store.save(id, changed)); + verifyWrites(writes, 2); + assertEquals(written, Files.readString(path)); + } + store.save(id, changed); + assertEquals(9, RawJson.read(path).get("maxBet").getAsInt()); + assertFalse(store.held(id)); + } + + @Test + void aWriteThatFailsAfterReplacingTheFileHoldsTheTableRatherThanSkipping() throws Exception { + var store = new TableOwnershipStore(data); + Path path = data.resolve("Data/tables/" + id + ".json"); + store.save(id, row()); + JsonObject changed = row(); + changed.addProperty("maxBet", 9); + try (var writes = writes()) { + writes.when( + () -> + net.tfminecraft.games.persistence.DurableFiles.write( + org.mockito.ArgumentMatchers.any(Path.class), + org.mockito.ArgumentMatchers.anyString())) + .thenAnswer( + call -> { + Files.writeString(path, call.getArgument(1)); + throw new IOException("directory flush failed"); + }); + assertThrows(IOException.class, () -> store.save(id, changed)); + } + assertThrows(TableOwnershipStore.Conflict.class, () -> store.save(id, changed)); + assertTrue(store.held(id)); + } + + @Test + void sourceReportsTheLastWrittenRowWithoutReadingTheFile() throws Exception { + var store = new TableOwnershipStore(data); + assertNull(store.source(id)); + store.save(id, row()); + var source = store.source(id); + assertEquals(data.resolve("Data/tables/" + id + ".json"), source.path()); + assertEquals(account, source.account()); + assertEquals(character, source.character()); + source.raw().addProperty("maxBet", 1); + assertFalse(store.source(id).raw().has("maxBet"), "callers get a copy"); + } } diff --git a/src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java b/src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java new file mode 100644 index 0000000..db7148f --- /dev/null +++ b/src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java @@ -0,0 +1,199 @@ +package net.tfminecraft.games.table; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.*; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.UUID; +import com.google.gson.JsonObject; +import com.google.gson.JsonParser; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.MockedStatic; +import net.tfminecraft.games.persistence.DurableFiles; + +/** + * Card changes move no money, so they wait for the next flush; money saves still write the + * whole table at once, and deferred loading does not read loaded tables' files again. + */ +class TableManagerSaveBatchingTest extends TableManagerHandFixture { + + private Path file(Table table) { + return data.resolve("Data/tables/" + table.getId() + ".json"); + } + + private JsonObject read(Table table) throws Exception { + return JsonParser.parseString(Files.readString(file(table))).getAsJsonObject(); + } + + private int remainingOnDisk(Table table) throws Exception { + return read(table).getAsJsonArray("remaining").size(); + } + + private MockedStatic countWrites() { + return mockStatic(DurableFiles.class, CALLS_REAL_METHODS); + } + + private static void verifyWrites(MockedStatic writes, Table table, int times) { + writes.verify(() -> DurableFiles.write( + argThat((Path path) -> path.getFileName().toString().equals(table.getId() + ".json")), + anyString()), times(times)); + } + + @Test void aDealtCardWaitsForTheFlushAndIsThenWrittenOnce() throws Exception { + Table table = placeAndClearMessage(); + assertEquals(2, remainingOnDisk(table)); + try (var writes = countWrites()) { + manager.dealToPlayer(table, player, 1); + tick(5); + assertEquals(1, table.handOf(player.getUniqueId()).size()); + assertEquals(2, remainingOnDisk(table), "the draw is not written straight away"); + verifyWrites(writes, table, 0); + manager.flushUnsaved(); + assertEquals(1, remainingOnDisk(table)); + verifyWrites(writes, table, 1); + manager.flushUnsaved(); + verifyWrites(writes, table, 1); + } + } + + @Test void theHandClockWritesWaitingChangesWithinASecond() throws Exception { + Table table = placeAndClearMessage(); + manager.dealToPlayer(table, player, 1); + assertEquals(1, table.handOf(player.getUniqueId()).size()); + assertEquals(2, remainingOnDisk(table)); + manager.startClock(); + try { + // Any 20 ticks of the clock include one flush. + tick(20); + assertEquals(1, remainingOnDisk(table)); + } finally { + manager.stopClock(); + } + } + + @ParameterizedTest + @ValueSource(strings = {"moneyMoved", "checkpoint"}) + void aMoneySaveWritesWaitingCardChangesAndClearsTheMark(String save) throws Exception { + Table table = placeAndClearMessage(); + try (var writes = countWrites()) { + manager.dealToPlayer(table, player, 1); + tick(5); + if (save.equals("moneyMoved")) manager.moneyMoved(table, List.of()); + else manager.checkpoint(table); + assertEquals(1, remainingOnDisk(table), "a money save writes the whole table"); + verifyWrites(writes, table, 1); + manager.flushUnsaved(); + verifyWrites(writes, table, 1); + } + } + + @Test void aFullReshuffleIsOnlyOnDiskAfterTheFlush() throws Exception { + Table table = placeAndClearMessage(); + table.getDeck().discard(table.getDeck().draw().orElseThrow()); + manager.checkpoint(table); + assertEquals(1, read(table).getAsJsonArray("discarded").size()); + try (var writes = countWrites()) { + manager.reshuffleFull(table); + assertEquals(1, read(table).getAsJsonArray("discarded").size()); + verifyWrites(writes, table, 0); + manager.flushUnsaved(); + verifyWrites(writes, table, 1); + } + assertEquals(0, read(table).getAsJsonArray("discarded").size()); + assertEquals(2, remainingOnDisk(table)); + } + + @Test void wipingHandsIsOnlyOnDiskAfterTheFlush() throws Exception { + Table table = dealt(1); + manager.flushUnsaved(); + assertEquals(1, remainingOnDisk(table)); + try (var writes = countWrites()) { + manager.wipeHands(); + assertTrue(table.getHands().isEmpty()); + assertEquals(1, remainingOnDisk(table)); + verifyWrites(writes, table, 0); + manager.flushUnsaved(); + verifyWrites(writes, table, 1); + } + assertEquals(2, remainingOnDisk(table)); + } + + @Test void aFlushSkipsATableThatWasRemovedMeanwhile() throws Exception { + Table table = placeAndClearMessage(); + manager.dealToPlayer(table, player, 1); + tick(5); + var tables = TableManager.class.getDeclaredField("tables"); + tables.setAccessible(true); + ((java.util.Map) tables.get(manager)).remove(table.getId()); + Files.delete(file(table)); + manager.flushUnsaved(); + assertFalse(Files.exists(file(table)), "a removed table is never written back"); + } + + @Test void aMuckIsOnlyOnDiskAfterTheFlush() throws Exception { + Table table = dealt(2); + manager.flushUnsaved(); + try (var writes = countWrites()) { + manager.muckPlayer(table, player); + assertEquals(0, read(table).getAsJsonArray("discarded").size() + remainingOnDisk(table)); + verifyWrites(writes, table, 0); + manager.flushUnsaved(); + verifyWrites(writes, table, 1); + } + assertEquals(2, read(table).getAsJsonArray("discarded").size() + remainingOnDisk(table)); + } + + @Test void deferredPassesDoNotReadAnUnchangedLoadedTableAgain() throws Exception { + Table table = placeAndClearMessage(); + String name = table.getId() + ".json"; + try (MockedStatic files = mockStatic(Files.class, CALLS_REAL_METHODS)) { + manager.loadDeferred(); + files.verify(() -> Files.readString(argThat((Path path) -> path.getFileName().toString().equals(name))), + never()); + // check() still compares the whole file with what was last written. + files.verify(() -> Files.readAllBytes(argThat((Path path) -> path.getFileName().toString().equals(name))), + times(1)); + } + assertSame(table, manager.table(table.getId())); + assertFalse(manager.ownershipStore().held(table.getId())); + } + + @Test void deferredPassesStillHoldALoadedTableEditedOutsideTheWriter() throws Exception { + Table table = placeAndClearMessage(); + JsonObject edited = read(table); + edited.addProperty("maxBet", 999); + Files.writeString(file(table), edited.toString()); + manager.loadDeferred(); + assertTrue(manager.ownershipStore().held(table.getId())); + assertSame(table, manager.table(table.getId())); + } + + @Test void deferredPassesStillHoldALoadedTableWhoseIdAppearsInAnotherFile() throws Exception { + Table table = placeAndClearMessage(); + Files.writeString(data.resolve("Data/tables/copy.json"), Files.readString(file(table))); + manager.loadDeferred(); + assertTrue(manager.ownershipStore().held(table.getId())); + assertSame(table, manager.table(table.getId())); + verify(net.tfminecraft.games.Games.plugin.getLogger(), atLeastOnce()) + .warning(contains("Duplicate table ID held: " + table.getId())); + } + + @Test void deferredPassesTakeTheOwnerCharacterFromTheCheckedFile() throws Exception { + Table table = placeAndClearMessage(); + UUID written = table.ownerCharacterId(); + assertNotNull(written); + table.setOwnerCharacterId(UUID.randomUUID()); + String name = table.getId() + ".json"; + try (MockedStatic files = mockStatic(Files.class, CALLS_REAL_METHODS)) { + manager.loadDeferred(); + files.verify(() -> Files.readString(argThat((Path path) -> path.getFileName().toString().equals(name))), + never()); + } + assertEquals(written, table.ownerCharacterId(), "as before, the file's owner character wins"); + } +} From 8c2d215f1bc985436664c9dd5bfd1369c580a1b7 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:01:08 +0100 Subject: [PATCH 2/4] perf: keep the posted wager intent state in memory only After Denars applies a posting the journal no longer rewrites the intent file as "applying". The in-memory state still blocks any second post or delivery; after a crash the retained "prepared" file freezes the table for review exactly as before, and review reads Denars status. Co-Authored-By: Claude Opus 5.5 --- .../tfminecraft/games/wager/WagerJournal.java | 8 +++-- .../games/wager/WagerJournalTest.java | 34 ++++++++++++++++++- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/main/java/net/tfminecraft/games/wager/WagerJournal.java b/src/main/java/net/tfminecraft/games/wager/WagerJournal.java index acc3243..33d27c2 100644 --- a/src/main/java/net/tfminecraft/games/wager/WagerJournal.java +++ b/src/main/java/net/tfminecraft/games/wager/WagerJournal.java @@ -88,9 +88,11 @@ static TxResult apply(Table table, Plan plan, java.util.function.Consumer Denars.post(anyString(), anyString(), anyList()), times(1)); + assertEquals(6, table.ledger().total(), "this process never posts or delivers an applied operation again"); + } + + @Test void appliedPostingWritesItsIntentOnceAndRemovesIt() throws Exception { + try (MockedStatic files = + mockStatic(net.tfminecraft.games.persistence.DurableFiles.class, CALLS_REAL_METHODS)) { + assertEquals(5, funding(5).commit().moved()); + files.verify(() -> net.tfminecraft.games.persistence.DurableFiles.write(eq(journal()), anyString()), times(1)); + files.verify(() -> net.tfminecraft.games.persistence.DurableFiles.delete(journal()), times(1)); + } + assertFalse(Files.exists(journal())); + verify(manager).saveWager(table); + } + + @Test void crashAfterPostingLeavesAPreparedIntentThatFreezesTheTableOnRestart() throws Exception { + // The table save fails after Denars applied the posting, as a crash at that point would. + doThrow(new IOException("disk full")).when(manager).saveWager(table); + assertFalse(funding(5).commit().ok()); + String retained = Files.readString(journal()); + assertEquals("prepared", JsonParser.parseString(retained).getAsJsonObject().get("state").getAsString()); + WagerJournal.initialize(); + assertTrue(WagerJournal.blocked(table)); + assertFalse(WagerJournal.active(table)); + doNothing().when(manager).saveWager(table); + table.setMoneyBlocked(false); + assertFalse(funding(5).commit().ok()); + assertEquals(1, postings.size()); + assertEquals(retained, Files.readString(journal())); } @Test void failedTableSavePreservesAmbiguousDeliveryAndBlocksSecondPayment() throws Exception { From ca13915818399fa56ce232a456060250f63a1e0e Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:52:11 +0100 Subject: [PATCH 3/4] fix: keep a waiting table change marked until its write succeeds CodeRabbit on #49: save() and saveWager() cleared the waiting mark before the write, so a failed write lost the batched card change for good. The mark is now cleared only after the store wrote the table; a disk failure keeps it, so the one-second flush retries it. A table that keeps failing warns once and logs once when it saves again. A non-disk RuntimeException still drops the mark so one broken table cannot stall the clock. Co-Authored-By: Claude Opus 5.5 --- .../tfminecraft/games/table/TableManager.java | 28 +++++++++--- .../table/TableManagerSaveBatchingTest.java | 45 ++++++++++++++++++- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/src/main/java/net/tfminecraft/games/table/TableManager.java b/src/main/java/net/tfminecraft/games/table/TableManager.java index 30c25da..c95d313 100644 --- a/src/main/java/net/tfminecraft/games/table/TableManager.java +++ b/src/main/java/net/tfminecraft/games/table/TableManager.java @@ -4857,9 +4857,10 @@ private Table tableFrom(Entity entity) { } public void saveWager(Table table) throws IOException { - // The whole table is written, so any change that was waiting goes with it. - unsaved.remove(table.getId()); ownershipStore().save(table.getId(), net.tfminecraft.games.ownership.RawJson.JSON.toJsonTree(toData(table)).getAsJsonObject()); + // The whole table is on disk, so any change that was waiting went with it. A failed + // write throws before this, so the waiting change stays marked for the next flush. + unsaved.remove(table.getId()); } /** @@ -4868,6 +4869,9 @@ public void saveWager(Table table) throws IOException { * play is reset when it loads, so a crash before the flush loses nothing a restart keeps. * Every money movement still saves the whole table at once through {@link #save}. */ + /** Tables whose last write failed, so a retry that keeps failing does not warn again. */ + private final java.util.Set saveFailing = new java.util.HashSet<>(); + private void saveSoon(Table table) { unsaved.add(table.getId()); } @@ -4883,21 +4887,31 @@ public void flushUnsaved() { try { save(table); } catch (RuntimeException ex) { - // save() has already cleared the mark, so one bad table cannot stall the clock. + // Not a disk failure (those keep the mark and retry): drop the mark so one bad + // table cannot stall the clock or log every second. + unsaved.remove(id); Games.plugin.getLogger().warning("[Games] Failed to save table " + id + ": " + ex); } } } private void save(Table table) { - unsaved.remove(table.getId()); - File file = tableFile(table.getId()); + UUID id = table.getId(); TableData data = toData(table); try { - ownershipStore().save(table.getId(), net.tfminecraft.games.ownership.RawJson.JSON.toJsonTree(data).getAsJsonObject()); + ownershipStore().save(id, net.tfminecraft.games.ownership.RawJson.JSON.toJsonTree(data).getAsJsonObject()); + // Only a written table clears its waiting change. + unsaved.remove(id); + if (saveFailing.remove(id)) { + Games.plugin.getLogger().info("[Games] Table " + id + " saved again after an earlier failure."); + } } catch (IOException ex) { table.setOwnershipHeld(true);table.setMoneyBlocked(true); - Games.plugin.getLogger().warning("[Games] Failed to save table " + table.getId() + ": " + ex.getMessage()); + // The change stays waiting, so a later flush retries it once the disk recovers. A table + // that keeps failing warns once, not on every one-second retry. + if (saveFailing.add(id)) { + Games.plugin.getLogger().warning("[Games] Failed to save table " + id + ": " + ex.getMessage()); + } } } diff --git a/src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java b/src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java index db7148f..ffb1c25 100644 --- a/src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java +++ b/src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java @@ -40,7 +40,7 @@ private MockedStatic countWrites() { private static void verifyWrites(MockedStatic writes, Table table, int times) { writes.verify(() -> DurableFiles.write( - argThat((Path path) -> path.getFileName().toString().equals(table.getId() + ".json")), + argThat((Path path) -> path != null && path.getFileName().toString().equals(table.getId() + ".json")), anyString()), times(times)); } @@ -123,6 +123,49 @@ void aMoneySaveWritesWaitingCardChangesAndClearsTheMark(String save) throws Exce assertEquals(2, remainingOnDisk(table)); } + private static void failWrites(MockedStatic writes, Table table) { + writes.when(() -> DurableFiles.write( + argThat((Path path) -> path != null && path.getFileName().toString().equals(table.getId() + ".json")), + anyString())).thenThrow(new java.io.IOException("disk full")); + } + + @Test void aFailedFlushKeepsTheChangeWaitingAndRetriesItWithOneWarning() throws Exception { + Table table = placeAndClearMessage(); + manager.dealToPlayer(table, player, 1); + tick(5); + var logger = net.tfminecraft.games.Games.plugin.getLogger(); + clearInvocations(logger); + try (var writes = countWrites()) { + failWrites(writes, table); + manager.flushUnsaved(); + manager.flushUnsaved(); + verifyWrites(writes, table, 2); + } + assertTrue(table.ownershipHeld(), "a failed write still holds the table"); + assertEquals(2, remainingOnDisk(table), "nothing reached the disk"); + verify(logger, times(1)).warning(contains("Failed to save table " + table.getId())); + manager.flushUnsaved(); + assertEquals(1, remainingOnDisk(table), "the waiting change is written once the disk works again"); + verify(logger).info(contains("saved again")); + try (var writes = countWrites()) { + manager.flushUnsaved(); + verifyWrites(writes, table, 0); + } + } + + @Test void aFailedWagerSaveLeavesTheWaitingChangeForTheNextFlush() throws Exception { + Table table = placeAndClearMessage(); + manager.dealToPlayer(table, player, 1); + tick(5); + try (var writes = countWrites()) { + failWrites(writes, table); + assertThrows(java.io.IOException.class, () -> manager.saveWager(table)); + } + assertEquals(2, remainingOnDisk(table)); + manager.flushUnsaved(); + assertEquals(1, remainingOnDisk(table)); + } + @Test void aFlushSkipsATableThatWasRemovedMeanwhile() throws Exception { Table table = placeAndClearMessage(); manager.dealToPlayer(table, player, 1); From df3836dce92e9cf5af7eb18562f14f7326935358 Mon Sep 17 00:00:00 2001 From: XxFran10xX <318299142+XxFran10xX@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:05:16 +0100 Subject: [PATCH 4/4] fix: leave store-held tables to their hold instead of retrying every second Opus review on #49: a table the ownership store holds (outside edit, duplicate id, pending estate publication) would fail every one-second retry and be frozen further on a timer. The flush now skips held tables and keeps their mark, so the next full save after the hold clears writes the change. Picking a table up also forgets its failing-save state. Co-Authored-By: Claude Opus 5.5 --- .../tfminecraft/games/table/TableManager.java | 6 ++++++ .../table/TableManagerSaveBatchingTest.java | 21 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/main/java/net/tfminecraft/games/table/TableManager.java b/src/main/java/net/tfminecraft/games/table/TableManager.java index c95d313..377eddf 100644 --- a/src/main/java/net/tfminecraft/games/table/TableManager.java +++ b/src/main/java/net/tfminecraft/games/table/TableManager.java @@ -974,6 +974,7 @@ private void pickup(Player player, Table table) { despawnWorld(table); tables.remove(table.getId()); unsaved.remove(table.getId()); + saveFailing.remove(table.getId()); deleteFile(table.getId()); // A table staff put down without a deck gives none back, or placing and lifting it would mint decks. ItemStack deckItem = table.deckConsumed() @@ -4884,6 +4885,11 @@ public void flushUnsaved() { unsaved.remove(id); continue; } + // The store holds a table it found edited outside, duplicated or mid-publication; writing + // it would only fail again and freeze it further on a timer. It stays marked, and the next + // full save after the hold clears writes the change. If the server stops first, only that + // card change is lost, as in a crash: hands are not saved and the table reloads from disk. + if (ownershipStore().held(id)) continue; try { save(table); } catch (RuntimeException ex) { diff --git a/src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java b/src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java index ffb1c25..99f01d7 100644 --- a/src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java +++ b/src/test/java/net/tfminecraft/games/table/TableManagerSaveBatchingTest.java @@ -153,6 +153,27 @@ private static void failWrites(MockedStatic writes, Table table) { } } + @Test void theFlushLeavesATableTheStoreHoldsAloneUntilTheHoldClears() throws Exception { + Table table = placeAndClearMessage(); + manager.dealToPlayer(table, player, 1); + tick(5); + Object store = manager.ownershipStore(); + var conflictsField = store.getClass().getDeclaredField("conflicts"); + conflictsField.setAccessible(true); + @SuppressWarnings("unchecked") var conflicts = (java.util.Set) conflictsField.get(store); + conflicts.add(table.getId()); + try (var writes = countWrites()) { + manager.flushUnsaved(); + manager.flushUnsaved(); + verifyWrites(writes, table, 0); + } + assertFalse(table.ownershipHeld() && !conflicts.contains(table.getId()), "the flush did not freeze it further"); + assertEquals(2, remainingOnDisk(table)); + conflicts.remove(table.getId()); + manager.flushUnsaved(); + assertEquals(1, remainingOnDisk(table), "the waiting change is written once the hold clears"); + } + @Test void aFailedWagerSaveLeavesTheWaitingChangeForTheNextFlush() throws Exception { Table table = placeAndClearMessage(); manager.dealToPlayer(table, player, 1);