From bc4a564f554315cca1cf2744bb0d792e41a75357 Mon Sep 17 00:00:00 2001 From: tastybento Date: Sat, 8 Aug 2026 11:53:33 -0700 Subject: [PATCH 1/2] fix: do not lose island progress when the server restarts A player breaking blocks and then sitting through a restart would come back to their block count rolled back to the last checkpoint - up to 49 blocks of progress gone, repeatedly, on every restart. The shutdown save was queued, not written. onDisable() called saveCache(), which uses saveObjectAsync(), and this addon is a Pladdon: the server disables it before BentoBox, so the write landed in a queue that BentoBox had to drain on its way out. BentoBox 3.22.0 added that drain (flushAll), but every earlier version discarded it silently. Write directly on shutdown instead, via a new saveCacheNow() using saveObjectNow(). That removes the dependency on core behaviour entirely rather than relying on a specific BentoBox version getting it right. saveObjectNow() is BentoBox 3.22.0 API, so bump the dependency and raise api-version to match. Without the api-version bump the addon would still load on an older core and throw NoSuchMethodError at shutdown, which is worse than the bug being fixed. It now refuses to load with "Please update BentoBox". Also make the periodic save interval configurable as island.save-every, defaulting to 10 rather than the hardcoded 50. Nothing helps if the server is SIGKILLed, but this caps what an unclean kill can cost at 9 blocks. Reported on Discord: progress reverting on every restart, BentoBox 3.15.0. Test notes: the BentoBox bump broke 5 PhasesPanelTest tests, and fixing those exposed 2 more. None were regressions - all 7 called when(user.getTranslation(...)) on a real User rather than a mock, which stubs nothing on the User and instead attaches to whichever mock the real method last touched. 3.22.0's getTranslation(World, ...) calls getIWM().getAddon() first, moving the target. Added a stubTranslation() helper that stubs the LocalesManager these actually read from. The same pattern remains elsewhere in that class and is worth a follow-up sweep. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014t1DSo2wMbTWZLcwXpwUmQ --- pom.xml | 2 +- .../world/bentobox/aoneblock/AOneBlock.java | 5 +- .../world/bentobox/aoneblock/Settings.java | 26 ++++++++++ .../aoneblock/listeners/BlockListener.java | 21 +++++--- src/main/resources/addon.yml | 2 +- src/main/resources/config.yml | 5 ++ .../bentobox/aoneblock/SettingsTest.java | 29 +++++++++++ .../listeners/BlockListenerTest.java | 39 +++++++++++++- .../aoneblock/panels/PhasesPanelTest.java | 52 ++++++++++++------- 9 files changed, 151 insertions(+), 30 deletions(-) diff --git a/pom.xml b/pom.xml index ef8b554..458ea87 100644 --- a/pom.xml +++ b/pom.xml @@ -56,7 +56,7 @@ 5.11.0 4.110.0 - 3.15.0-SNAPSHOT + 3.22.0 4.0.10 1.8.0 0.0.67 diff --git a/src/main/java/world/bentobox/aoneblock/AOneBlock.java b/src/main/java/world/bentobox/aoneblock/AOneBlock.java index ea187e4..6d84d64 100644 --- a/src/main/java/world/bentobox/aoneblock/AOneBlock.java +++ b/src/main/java/world/bentobox/aoneblock/AOneBlock.java @@ -237,9 +237,10 @@ public boolean loadData() { @Override public void onDisable() { - // save cache + // Save cache. This must be a direct write, not a queued one: the server disables this + // Pladdon before BentoBox, so anything queued here depends on BentoBox draining it later. if (blockListener != null) { - blockListener.saveCache(); + blockListener.saveCacheNow(); } // Clear holograms diff --git a/src/main/java/world/bentobox/aoneblock/Settings.java b/src/main/java/world/bentobox/aoneblock/Settings.java index 70e0a8a..0652386 100644 --- a/src/main/java/world/bentobox/aoneblock/Settings.java +++ b/src/main/java/world/bentobox/aoneblock/Settings.java @@ -400,6 +400,13 @@ public class Settings implements WorldSettings { @ConfigEntry(path = "island.water-mob-protection") private boolean waterMobProtection = true; + @ConfigComment("How often island progress is written to the database, in blocks broken") + @ConfigComment("Progress is also saved whenever a phase changes, a player logs out and the server shuts down,") + @ConfigComment("so this only decides how much is lost if the server dies without shutting down cleanly.") + @ConfigComment("Lower is safer but writes more often. Minimum is 1 (save every block)") + @ConfigEntry(path = "island.save-every") + private int saveEvery = 10; + @ConfigComment("Default max team size") @ConfigComment("Permission size cannot be less than the default below. ") @ConfigEntry(path = "island.max-team-size") @@ -1865,6 +1872,25 @@ public void setMobWarning(int mobWarning) { this.mobWarning = mobWarning; } + /** + * How many blocks are broken between periodic saves of island progress. + * A value below 1 would make the modulo check throw, so it is clamped. + * @return the saveEvery value, never less than 1 + */ + public int getSaveEvery() { + if (saveEvery < 1) { + saveEvery = 1; + } + return saveEvery; + } + + /** + * @param saveEvery the saveEvery to set + */ + public void setSaveEvery(int saveEvery) { + this.saveEvery = saveEvery; + } + /** * @return the waterMobProtection */ diff --git a/src/main/java/world/bentobox/aoneblock/listeners/BlockListener.java b/src/main/java/world/bentobox/aoneblock/listeners/BlockListener.java index 4f3410f..36e1d77 100644 --- a/src/main/java/world/bentobox/aoneblock/listeners/BlockListener.java +++ b/src/main/java/world/bentobox/aoneblock/listeners/BlockListener.java @@ -124,11 +124,6 @@ private record BrushSession(BukkitTask task, Block block) {} */ public static final int MAX_LOOK_AHEAD = 5; - /** - * How often island data is saved to the database (in blocks broken). - */ - public static final int SAVE_EVERY = 50; - /* * Loot tables for suspicious blocks */ @@ -161,11 +156,25 @@ public BlockListener(@NonNull AOneBlock addon) { /** * Saves all island data from the cache to the database asynchronously. + *

+ * Only safe while the server is running. On shutdown use {@link #saveCacheNow()}. */ public void saveCache() { cache.values().forEach(handler::saveObjectAsync); } + /** + * Saves all island data from the cache to the database on the calling thread. + *

+ * Used on shutdown, where an asynchronous save cannot be retried if it does not complete. + * BentoBox drains writes queued by addons as they are disabled, but this addon is a Pladdon, + * so the server disables it before BentoBox and that drain is the only thing standing between + * a queued block count and a rolled-back island. Writing directly removes the dependency. + */ + public void saveCacheNow() { + cache.values().forEach(handler::saveObjectNow); + } + // --------------------------------------------------------------------- // Section: Listeners // --------------------------------------------------------------------- @@ -448,7 +457,7 @@ private ProcessPhaseResult processPhase(Cancellable e, Island i, OneBlockIslands return new ProcessPhaseResult(phase, true, 0); } handleNewPhase(player, i, is, phase, block, prevPhaseName); - } else if (is.getBlockNumber() % SAVE_EVERY == 0) { + } else if (is.getBlockNumber() % addon.getSettings().getSaveEvery() == 0) { // Periodically save the island's progress. saveIsland(i); } diff --git a/src/main/resources/addon.yml b/src/main/resources/addon.yml index 203743b..dbfb578 100755 --- a/src/main/resources/addon.yml +++ b/src/main/resources/addon.yml @@ -1,7 +1,7 @@ name: AOneBlock main: world.bentobox.aoneblock.AOneBlock version: ${version}${build.number} -api-version: 3.13.0 +api-version: 3.22.0 metrics: true icon: "STONE" repository: "BentoBoxWorld/AOneBlock" diff --git a/src/main/resources/config.yml b/src/main/resources/config.yml index d339659..c366d19 100644 --- a/src/main/resources/config.yml +++ b/src/main/resources/config.yml @@ -404,6 +404,11 @@ island: mob-warning: 5 # Whether spawned mobs that need water to survive will spawn in a generated water block water-mob-protection: true + # How often island progress is written to the database, in blocks broken + # Progress is also saved whenever a phase changes, a player logs out and the server shuts down, + # so this only decides how much is lost if the server dies without shutting down cleanly. + # Lower is safer but writes more often. Minimum is 1 (save every block) + save-every: 10 # Default max team size # Permission size cannot be less than the default below. max-team-size: 4 diff --git a/src/test/java/world/bentobox/aoneblock/SettingsTest.java b/src/test/java/world/bentobox/aoneblock/SettingsTest.java index 0238e34..1f94692 100644 --- a/src/test/java/world/bentobox/aoneblock/SettingsTest.java +++ b/src/test/java/world/bentobox/aoneblock/SettingsTest.java @@ -1732,6 +1732,35 @@ void testSetHologramDuration() { s.setHologramDuration(2345); assertEquals(2345, s.getHologramDuration()); } + + /** + * Test method for {@link world.bentobox.aoneblock.Settings#getSaveEvery()}. + */ + @Test + void testGetSaveEveryDefault() { + assertEquals(10, s.getSaveEvery()); + } + + /** + * Test method for {@link world.bentobox.aoneblock.Settings#setSaveEvery(int)}. + */ + @Test + void testSetSaveEvery() { + s.setSaveEvery(25); + assertEquals(25, s.getSaveEvery()); + } + + /** + * The value is used as a modulo divisor, so anything below 1 has to be clamped or + * the block break handler would throw an ArithmeticException on every block. + */ + @Test + void testGetSaveEveryClampsZeroAndBelow() { + s.setSaveEvery(0); + assertEquals(1, s.getSaveEvery()); + s.setSaveEvery(-50); + assertEquals(1, s.getSaveEvery()); + } diff --git a/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest.java b/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest.java index 81b1dac..fcd087c 100644 --- a/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest.java +++ b/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest.java @@ -5,6 +5,8 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import java.io.File; @@ -49,6 +51,8 @@ public class BlockListenerTest extends CommonTestSetup { // Class under test private BlockListener bl; + private AbstractDatabaseHandler h; + @Mock AOneBlock addon; @Mock @@ -78,13 +82,14 @@ public class BlockListenerTest extends CommonTestSetup { public void setUp() throws Exception { super.setUp(); // This has to be done beforeClass otherwise the tests will interfere with each other - AbstractDatabaseHandler h = mock(AbstractDatabaseHandler.class); + h = mock(AbstractDatabaseHandler.class); // Database MockedStatic mockDb = Mockito.mockStatic(DatabaseSetup.class); DatabaseSetup dbSetup = mock(DatabaseSetup.class); mockDb.when(DatabaseSetup::getDatabase).thenReturn(dbSetup); when(dbSetup.getHandler(any())).thenReturn(h); when(h.saveObject(any())).thenReturn(CompletableFuture.completedFuture(true)); + when(h.saveObjectNow(any())).thenReturn(CompletableFuture.completedFuture(true)); // Addon when(addon.getPlugin()).thenReturn(plugin); @@ -187,4 +192,36 @@ void testOnBlockFromToCenterBlock() { assertTrue(e.isCancelled()); } + /** + * Test method for {@link world.bentobox.aoneblock.listeners.BlockListener#saveCache()}. + */ + @Test + void testSaveCacheQueuesTheWrite() throws Exception { + island.setUniqueId(UUID.randomUUID().toString()); + bl.getIsland(island); + + bl.saveCache(); + + verify(h).saveObject(any()); + verify(h, never()).saveObjectNow(any()); + } + + /** + * The shutdown save has to write directly. This addon is a Pladdon, so the server disables it + * before BentoBox, and a queued write only lands if BentoBox drains the queue afterwards - + * which older BentoBox versions did not do, silently rolling islands back on every restart. + * + * Test method for {@link world.bentobox.aoneblock.listeners.BlockListener#saveCacheNow()}. + */ + @Test + void testSaveCacheNowWritesDirectly() throws Exception { + island.setUniqueId(UUID.randomUUID().toString()); + bl.getIsland(island); + + bl.saveCacheNow(); + + verify(h).saveObjectNow(any()); + verify(h, never()).saveObject(any()); + } + } diff --git a/src/test/java/world/bentobox/aoneblock/panels/PhasesPanelTest.java b/src/test/java/world/bentobox/aoneblock/panels/PhasesPanelTest.java index 850b1f5..06b6bf8 100644 --- a/src/test/java/world/bentobox/aoneblock/panels/PhasesPanelTest.java +++ b/src/test/java/world/bentobox/aoneblock/panels/PhasesPanelTest.java @@ -7,6 +7,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.verify; @@ -71,6 +72,24 @@ class PhasesPanelTest extends CommonTestSetup { private PhasesPanel panel; + /** + * Sets what a locale reference translates to for these tests. + *

+ * The {@code user} here is a real {@link User} wrapping a mock player, not a mock, so + * {@code when(user.getTranslation(...))} does not stub anything on it - it runs the real + * method and Mockito attaches the stub to whichever mock that method happened to touch last. + * That is an implementation detail of BentoBox and moves between versions. Stub the + * {@link world.bentobox.bentobox.managers.LocalesManager} that {@code getTranslation} actually + * reads from instead, which is stable. + * + * @param reference locale key, without any addon prefix + * @param value what it should translate to + */ + private void stubTranslation(String reference, String value) { + when(lm.get(any(), eq(reference))).thenReturn(value); + when(lm.get(any(), eq("aoneblock." + reference))).thenReturn(value); + } + private void setUpAddonMocks() { when(addon.getPlugin()).thenReturn(plugin); when(addon.getOneBlockManager()).thenReturn(oneBlockManager); @@ -325,10 +344,9 @@ void testBuildBlocksText() throws Exception { OneBlockPhase phase = createTestPhase("Plains"); - when(user.getTranslation("aoneblock.gui.buttons.phase.blocks-prefix")).thenReturn("Blocks: "); - when(user.getTranslation("aoneblock.gui.buttons.phase.wrap-at")).thenReturn("50"); - when(user.getTranslation("aoneblock.gui.buttons.phase.blocks", "name", "Stone")).thenReturn("Stone, "); - when(user.getTranslation("aoneblock.gui.buttons.phase.blocks", "name", "Dirt")).thenReturn("Dirt, "); + stubTranslation("aoneblock.gui.buttons.phase.blocks-prefix", "Blocks: "); + stubTranslation("aoneblock.gui.buttons.phase.wrap-at", "50"); + stubTranslation("aoneblock.gui.buttons.phase.blocks", "[name], "); when(hooksManager.getHook("LangUtils")).thenReturn(Optional.empty()); mockedUtil.when(() -> Util.prettifyText(anyString())).thenAnswer(i -> { String arg = i.getArgument(0); @@ -827,7 +845,7 @@ void testCollectTooltipsWithRealTooltip() throws Exception { new ItemTemplateRecord.ActionRecords(ClickType.LEFT, "SELECT", "content", "tooltip.key") ); - when(user.getTranslation(world, "tooltip.key")).thenReturn("Real tooltip"); + stubTranslation("tooltip.key", "Real tooltip"); Method method = PhasesPanel.class.getDeclaredMethod("collectTooltips", List.class); method.setAccessible(true); @@ -1482,8 +1500,8 @@ void testCollectTooltipsAllBlank() throws Exception { new ItemTemplateRecord.ActionRecords(ClickType.LEFT, "VIEW", "content", "tooltip2") ); - when(user.getTranslation(world, "tooltip1")).thenReturn(" "); // Blank after translation - when(user.getTranslation(world, "tooltip2")).thenReturn(""); // Empty + stubTranslation("tooltip1", " "); // Blank after translation + stubTranslation("tooltip2", ""); // Empty Method method = PhasesPanel.class.getDeclaredMethod("collectTooltips", List.class); method.setAccessible(true); @@ -2144,8 +2162,7 @@ void testBuildDescriptionTextTemplatedWithBiome() throws Exception { try (MockedStatic ms = mockStatic(LangUtilsHook.class)) { ms.when(() -> LangUtilsHook.getBiomeName(biome, user)).thenReturn("Plains"); - when(user.getTranslationOrNothing("custom.desc", "number", "0", "[biome]", "Plains", "[bank]", "", "[economy]", "", "[level]", "", "[permission]", "", "[blocks]", "")) - .thenReturn("Plains Description"); + stubTranslation("custom.desc", "[biome] Description"); Method method = PhasesPanel.class.getDeclaredMethod("buildDescriptionText", ItemTemplateRecord.class, OneBlockPhase.class, reqTextClass, String.class); method.setAccessible(true); @@ -2179,9 +2196,8 @@ void testBuildDefaultDescription() throws Exception { reqConstructor.setAccessible(true); Object reqTexts = reqConstructor.newInstance("", "", "", ""); - when(user.getTranslationOrNothing("aoneblock.gui.buttons.phase.starting-block", "number", "0")).thenReturn("Block 0"); - when(user.getTranslationOrNothing("aoneblock.gui.buttons.phase.description", "[starting-block]", "Block 0", "[biome]", "", "[bank]", "", "[economy]", "", "[level]", "", "[permission]", "", "[blocks]", "")) - .thenReturn("Default Desc"); + stubTranslation("aoneblock.gui.buttons.phase.starting-block", "Block [number]"); + stubTranslation("aoneblock.gui.buttons.phase.description", "Default Desc [starting-block]"); Method method = PhasesPanel.class.getDeclaredMethod("buildDefaultDescription", OneBlockPhase.class, reqTextClass, String.class); method.setAccessible(true); @@ -2398,10 +2414,9 @@ void testBuildDefaultDescriptionWithBiome() throws Exception { reqConstructor.setAccessible(true); Object reqTexts = reqConstructor.newInstance("", "", "", ""); - when(user.getTranslationOrNothing("aoneblock.gui.buttons.phase.starting-block", "number", "0")).thenReturn("Block 0"); - when(user.getTranslationOrNothing("aoneblock.gui.buttons.phase.biome", "[biome]", "Plains")).thenReturn("Biome: Plains"); - when(user.getTranslationOrNothing("aoneblock.gui.buttons.phase.description", "[starting-block]", "Block 0", "[biome]", "Biome: Plains", "[bank]", "", "[economy]", "", "[level]", "", "[permission]", "", "[blocks]", "")) - .thenReturn("Description with biome"); + stubTranslation("aoneblock.gui.buttons.phase.starting-block", "Block [number]"); + stubTranslation("aoneblock.gui.buttons.phase.biome", "Biome: [biome]"); + stubTranslation("aoneblock.gui.buttons.phase.description", "Description with biome [biome]"); try (MockedStatic ms = mockStatic(LangUtilsHook.class)) { ms.when(() -> LangUtilsHook.getBiomeName(biome, user)).thenReturn("Plains"); @@ -2440,9 +2455,8 @@ void testBuildDescriptionTextNullTemplate() throws Exception { reqConstructor.setAccessible(true); Object reqTexts = reqConstructor.newInstance("", "", "", ""); - when(user.getTranslationOrNothing("aoneblock.gui.buttons.phase.starting-block", "number", "0")).thenReturn("Block 0"); - when(user.getTranslationOrNothing("aoneblock.gui.buttons.phase.description", "[starting-block]", "Block 0", "[biome]", "", "[bank]", "", "[economy]", "", "[level]", "", "[permission]", "", "[blocks]", "")) - .thenReturn("Default Description"); + stubTranslation("aoneblock.gui.buttons.phase.starting-block", "Block [number]"); + stubTranslation("aoneblock.gui.buttons.phase.description", "Default Description [starting-block]"); Method method = PhasesPanel.class.getDeclaredMethod("buildDescriptionText", ItemTemplateRecord.class, OneBlockPhase.class, reqTextClass, String.class); method.setAccessible(true); From f5d3131c5a166502f1cfa00b94ce4e2f5614ee0b Mon Sep 17 00:00:00 2001 From: tastybento Date: Sat, 8 Aug 2026 12:51:13 -0700 Subject: [PATCH 2/2] ci: build on JDK 25 so BentoBox 3.22.0 class files can be read BentoBox 3.18.0 onwards is compiled for Java 25 (Minecraft 26.x), so its class files are version 69. A JDK 21 javac cannot parse those at all, and the build died with "class file has wrong version 69.0, should be 65.0" against every BentoBox type before it reached any of our code. This only surfaced once the dependency moved to 3.22.0. It did not show up locally because the dev machine is already on JDK 25 - the compiler reads the newer class files happily and 21 still emits Java 21 bytecode, which is what the addon ships. The addon's own target is unchanged: still Java 21 via in the pom. Only the JDK doing the compiling moves. Also moves setup-java to v4 and the 'adopt' distribution to 'temurin', since neither v3 nor adopt offers a Java 25 build. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014t1DSo2wMbTWZLcwXpwUmQ --- .github/workflows/build.yml | 12 ++++++++---- .github/workflows/modrinth-publish.yml | 7 ++++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 6e88943..23d614a 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -14,11 +14,15 @@ jobs: - uses: actions/checkout@v3 with: fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis - - name: Set up JDK 21 - uses: actions/setup-java@v3 + # BentoBox 3.18.0+ is compiled for Java 25 (Minecraft 26.x), so its class files + # cannot be read by a JDK 21 javac at all - the build fails with + # "class file has wrong version 69.0, should be 65.0" before reaching our code. + # The addon itself still targets 21 via in the pom. + - name: Set up JDK 25 + uses: actions/setup-java@v4 with: - distribution: 'adopt' - java-version: 21 + distribution: 'temurin' + java-version: 25 - name: Cache SonarCloud packages uses: actions/cache@v3 with: diff --git a/.github/workflows/modrinth-publish.yml b/.github/workflows/modrinth-publish.yml index 960aa3a..667c352 100644 --- a/.github/workflows/modrinth-publish.yml +++ b/.github/workflows/modrinth-publish.yml @@ -22,11 +22,12 @@ jobs: - name: Checkout repository uses: actions/checkout@v4 - # 2. Set up Java 21 (required by AOneBlock' build) - - name: Set up Java 21 + # 2. Set up Java 25 - required to read BentoBox 3.18.0+ class files, which are + # compiled for Java 25. The addon itself still targets 21 via in the pom. + - name: Set up Java 25 uses: actions/setup-java@v4 with: - java-version: '21' + java-version: '25' distribution: 'temurin' # 3. Cache Maven dependencies to speed up builds