From 6b8fd49c83646f04a487259493bb36583fb488ff Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 18:40:41 +0200 Subject: [PATCH 01/13] fix: harden world codec defaults against missing/invalid data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Difficulty was the one enum parsed via valueOf with no fallback, so a bad value threw and the whole world was skipped on load — contradicting the codec's documented "unknown enums fall back" contract. Add a parseDifficulty helper mirroring parseType/parseStatus. The boolean and timestamp fields were read with getBoolean/getLong and no default, so a key absent from disk loaded as false/0 rather than the value a freshly created world gets (protections silently disabled, timestamps at epoch-0). Make WorldDataImpl own the defaults as constants used by both the builder and the codec so the two paths can't drift. --- .../buildsystem/storage/codec/WorldCodec.java | 47 +++++++++++++------ .../buildsystem/world/data/WorldDataImpl.java | 44 ++++++++++++----- .../yaml/YamlWorldStorageRoundTripTest.java | 39 +++++++++++++++ 3 files changed, 104 insertions(+), 26 deletions(-) diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/codec/WorldCodec.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/codec/WorldCodec.java index 655096b5..0e987ddb 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/codec/WorldCodec.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/codec/WorldCodec.java @@ -160,24 +160,28 @@ private WorldDataImpl parseWorldData(String worldName, ConfigurationSection sect .withCustomSpawn(parseCustomSpawn(section)) .withPermission(section.getString(DATA + "." + DATA_PERMISSION, "-")) .withProject(section.getString(DATA + "." + DATA_PROJECT, "-")) - .withDifficulty(Difficulty.valueOf(section.getString(DATA + "." + DATA_DIFFICULTY, "PEACEFUL") - .toUpperCase(Locale.ROOT))) + .withDifficulty(parseDifficulty(section, worldName)) .withMaterial(parseMaterial(section, worldName)) .withIconSkullTexture(section.getString(DATA + "." + DATA_ICON_SKULL_TEXTURE, "")) .withStatus(parseStatus(section, worldName)) - .withBlockBreaking(section.getBoolean(DATA + "." + DATA_BLOCK_BREAKING)) - .withBlockInteractions(section.getBoolean(DATA + "." + DATA_BLOCK_INTERACTIONS)) - .withBlockPlacement(section.getBoolean(DATA + "." + DATA_BLOCK_PLACEMENT)) - .withBuildersEnabled(section.getBoolean(DATA + "." + DATA_BUILDERS_ENABLED)) - .withExplosions(section.getBoolean(DATA + "." + DATA_EXPLOSIONS)) - .withMobAi(section.getBoolean(DATA + "." + DATA_MOB_AI)) - .withPhysics(section.getBoolean(DATA + "." + DATA_PHYSICS)) - .withPinned(section.getBoolean(DATA + "." + DATA_PINNED, false)) + .withBlockBreaking( + section.getBoolean(DATA + "." + DATA_BLOCK_BREAKING, WorldDataImpl.DEFAULT_BLOCK_BREAKING)) + .withBlockInteractions(section.getBoolean( + DATA + "." + DATA_BLOCK_INTERACTIONS, WorldDataImpl.DEFAULT_BLOCK_INTERACTIONS)) + .withBlockPlacement( + section.getBoolean(DATA + "." + DATA_BLOCK_PLACEMENT, WorldDataImpl.DEFAULT_BLOCK_PLACEMENT)) + .withBuildersEnabled( + section.getBoolean(DATA + "." + DATA_BUILDERS_ENABLED, WorldDataImpl.DEFAULT_BUILDERS_ENABLED)) + .withExplosions(section.getBoolean(DATA + "." + DATA_EXPLOSIONS, WorldDataImpl.DEFAULT_EXPLOSIONS)) + .withMobAi(section.getBoolean(DATA + "." + DATA_MOB_AI, WorldDataImpl.DEFAULT_MOB_AI)) + .withPhysics(section.getBoolean(DATA + "." + DATA_PHYSICS, WorldDataImpl.DEFAULT_PHYSICS)) + .withPinned(section.getBoolean(DATA + "." + DATA_PINNED, WorldDataImpl.DEFAULT_PINNED)) .withVisibility(parseVisibility(section)) - .withTimeSinceBackup(section.getInt(DATA + "." + DATA_TIME_SINCE_BACKUP, 0)) - .withLastLoaded(section.getLong(DATA + "." + DATA_LAST_LOADED)) - .withLastUnloaded(section.getLong(DATA + "." + DATA_LAST_UNLOADED)) - .withLastEdited(section.getLong(DATA + "." + DATA_LAST_EDITED)) + .withTimeSinceBackup( + section.getInt(DATA + "." + DATA_TIME_SINCE_BACKUP, WorldDataImpl.DEFAULT_TIME_SINCE_BACKUP)) + .withLastLoaded(section.getLong(DATA + "." + DATA_LAST_LOADED, WorldDataImpl.DEFAULT_TIMESTAMP)) + .withLastUnloaded(section.getLong(DATA + "." + DATA_LAST_UNLOADED, WorldDataImpl.DEFAULT_TIMESTAMP)) + .withLastEdited(section.getLong(DATA + "." + DATA_LAST_EDITED, WorldDataImpl.DEFAULT_TIMESTAMP)) .withPermissionOverrideEnabled( () -> context.configService().current().folder().overridePermissions()) .withProjectOverrideEnabled( @@ -208,6 +212,21 @@ private BuildWorldType parseType(String worldName, ConfigurationSection section) } } + /** + * Resolves a world's {@link Difficulty}, falling back to {@link Difficulty#PEACEFUL} when the persisted value is + * unknown. Like the other enums, an unparseable difficulty must not abort the world's load. + */ + private Difficulty parseDifficulty(ConfigurationSection section, String worldName) { + String raw = section.getString(DATA + "." + DATA_DIFFICULTY, Difficulty.PEACEFUL.name()); + try { + return Difficulty.valueOf(raw.toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException e) { + context.logger() + .warning("Unknown difficulty \"" + raw + "\" for \"" + worldName + "\". Defaulting to PEACEFUL."); + return Difficulty.PEACEFUL; + } + } + /** * Resolves a world's status from its persisted id, migrating pre-4.0 enum names (e.g. {@code NOT_STARTED}) to the * equivalent lower-case status id. Falls back to the registry default when the id is unknown. diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/data/WorldDataImpl.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/data/WorldDataImpl.java index 2fe09c51..3db9decf 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/data/WorldDataImpl.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/data/WorldDataImpl.java @@ -44,6 +44,26 @@ @NullMarked public class WorldDataImpl implements WorldData { + /** + * Default values for the persisted world settings. These are the single source of truth for both world creation + * (the {@link WorldDataBuilder} field initializers) and deserialization ({@code WorldCodec} passes them to + * {@code getBoolean}/{@code getLong}/{@code getInt} so a key absent from disk restores the same value a freshly + * created world would have). Keeping them here keeps the two paths from drifting apart. + */ + public static final boolean DEFAULT_BLOCK_BREAKING = true; + + public static final boolean DEFAULT_BLOCK_INTERACTIONS = true; + public static final boolean DEFAULT_BLOCK_PLACEMENT = true; + public static final boolean DEFAULT_BUILDERS_ENABLED = false; + public static final boolean DEFAULT_EXPLOSIONS = true; + public static final boolean DEFAULT_MOB_AI = true; + public static final boolean DEFAULT_PHYSICS = true; + public static final boolean DEFAULT_PINNED = false; + public static final int DEFAULT_TIME_SINCE_BACKUP = 0; + + /** Sentinel for the {@code last-*} timestamps meaning "never". */ + public static final long DEFAULT_TIMESTAMP = -1L; + private final Map> data = new HashMap<>(); private String worldName; @@ -177,19 +197,19 @@ public static class WorldDataBuilder { private XMaterial material = XMaterial.GRASS_BLOCK; private String iconSkullTexture = ""; private @Nullable BuildWorldStatus status; - private boolean blockBreaking = true; - private boolean blockInteractions = true; - private boolean blockPlacement = true; - private boolean buildersEnabled = false; - private boolean explosions = true; - private boolean mobAi = true; - private boolean physics = true; - private boolean pinned = false; + private boolean blockBreaking = DEFAULT_BLOCK_BREAKING; + private boolean blockInteractions = DEFAULT_BLOCK_INTERACTIONS; + private boolean blockPlacement = DEFAULT_BLOCK_PLACEMENT; + private boolean buildersEnabled = DEFAULT_BUILDERS_ENABLED; + private boolean explosions = DEFAULT_EXPLOSIONS; + private boolean mobAi = DEFAULT_MOB_AI; + private boolean physics = DEFAULT_PHYSICS; + private boolean pinned = DEFAULT_PINNED; private Visibility visibility = Visibility.EVERYONE; - private int timeSinceBackup = 0; - private long lastEdited = -1L; - private long lastLoaded = -1L; - private long lastUnloaded = -1L; + private int timeSinceBackup = DEFAULT_TIME_SINCE_BACKUP; + private long lastEdited = DEFAULT_TIMESTAMP; + private long lastLoaded = DEFAULT_TIMESTAMP; + private long lastUnloaded = DEFAULT_TIMESTAMP; BooleanSupplier permissionOverrideEnabled = () -> false; BooleanSupplier projectOverrideEnabled = () -> false; diff --git a/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorageRoundTripTest.java b/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorageRoundTripTest.java index 4c37da13..fb4420b8 100644 --- a/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorageRoundTripTest.java +++ b/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorageRoundTripTest.java @@ -205,6 +205,45 @@ void load_missingStatus_defaultsToNotStarted() throws Exception { assertEquals(TestData.NOT_STARTED, loaded.iterator().next().getData().get(WorldDataKey.STATUS)); } + @Test + void load_invalidDifficultyEnum_defaultsToPeaceful() throws Exception { + YamlConfiguration yaml = new YamlConfiguration(); + yaml.set("worlds.Bad.uuid", UUID.randomUUID().toString()); + yaml.set("worlds.Bad.type", "NORMAL"); + yaml.set("worlds.Bad.date", 1L); + yaml.set("worlds.Bad.data.status", "FINISHED"); + yaml.set("worlds.Bad.data.difficulty", "NOT_A_DIFFICULTY"); + yaml.save(new File(dataFolder, "worlds.yml")); + + Collection loaded = newStorage().load().join(); + assertEquals(1, loaded.size()); + assertEquals(Difficulty.PEACEFUL, loaded.iterator().next().getData().get(WorldDataKey.DIFFICULTY)); + } + + @Test + void load_missingDataKeys_restoreCreationDefaults() throws Exception { + // A world entry that predates these keys (or was hand-edited) must come back with the same defaults a freshly + // created world has — not getBoolean/getLong's implicit false/0. + YamlConfiguration yaml = new YamlConfiguration(); + yaml.set("worlds.Sparse.uuid", UUID.randomUUID().toString()); + yaml.set("worlds.Sparse.type", "NORMAL"); + yaml.set("worlds.Sparse.date", 1L); + yaml.set("worlds.Sparse.data.status", "FINISHED"); + yaml.save(new File(dataFolder, "worlds.yml")); + + BuildWorld world = newStorage().load().join().iterator().next(); + assertTrue(world.getData().get(WorldDataKey.BLOCK_BREAKING)); + assertTrue(world.getData().get(WorldDataKey.BLOCK_INTERACTIONS)); + assertTrue(world.getData().get(WorldDataKey.BLOCK_PLACEMENT)); + assertTrue(world.getData().get(WorldDataKey.EXPLOSIONS)); + assertTrue(world.getData().get(WorldDataKey.MOB_AI)); + assertTrue(world.getData().get(WorldDataKey.PHYSICS)); + assertFalse(world.getData().get(WorldDataKey.BUILDERS_ENABLED)); + assertEquals(WorldDataImpl.DEFAULT_TIMESTAMP, world.getData().get(WorldDataKey.LAST_EDITED)); + assertEquals(WorldDataImpl.DEFAULT_TIMESTAMP, world.getData().get(WorldDataKey.LAST_LOADED)); + assertEquals(WorldDataImpl.DEFAULT_TIMESTAMP, world.getData().get(WorldDataKey.LAST_UNLOADED)); + } + @Test void load_unparseableEntry_isSkipped_othersStillLoad() throws Exception { // Persist a good world, then inject a sibling entry whose UUID cannot be parsed. From cc9f0d159daa9f65fab00752f134e4a8b1319222 Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 18:54:57 +0200 Subject: [PATCH 02/13] fix: surface failed deletes in FileUtils.deleteDirectory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The walk mapped to File#delete, whose boolean return was dropped, so a locked or permission-denied file left orphaned data while the caller was told the delete succeeded — the method advertised throws IOException but only Files.walk could trigger it. Switch to Files#delete and propagate the first failure, the same care zipDirectoryToMemory already takes against silent truncation. --- .../de/eintosti/buildsystem/util/FileUtils.java | 13 ++++++++++++- .../buildsystem/util/FileUtilsTest.java | 17 +++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/FileUtils.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/FileUtils.java index 35465d10..33ae349b 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/FileUtils.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/FileUtils.java @@ -150,7 +150,18 @@ public static void deleteDirectory(File directory) throws IOException { */ public static void deleteDirectory(Path directoryPath) throws IOException { try (Stream walk = Files.walk(directoryPath)) { - walk.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete); + // Children before parents, and every delete is checked: File#delete swallows failures (a locked or + // permission-denied file would leave orphaned data and report success), so use Files#delete and surface + // the first failure as the IOException this method already advertises. + walk.sorted(Comparator.reverseOrder()).forEach(path -> { + try { + Files.delete(path); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + }); + } catch (UncheckedIOException e) { + throw e.getCause(); } } diff --git a/buildsystem-core/src/test/java/de/eintosti/buildsystem/util/FileUtilsTest.java b/buildsystem-core/src/test/java/de/eintosti/buildsystem/util/FileUtilsTest.java index e57ed00f..e47d6c1b 100644 --- a/buildsystem-core/src/test/java/de/eintosti/buildsystem/util/FileUtilsTest.java +++ b/buildsystem-core/src/test/java/de/eintosti/buildsystem/util/FileUtilsTest.java @@ -18,6 +18,7 @@ package de.eintosti.buildsystem.util; import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assumptions.assumeTrue; import java.io.ByteArrayInputStream; import java.io.File; @@ -92,6 +93,22 @@ void deleteDirectory_missingDirectoryThrows() { assertThrows(IOException.class, () -> FileUtils.deleteDirectory(missing)); } + @Test + void deleteDirectory_failedDeleteIsReportedNotSwallowed() throws IOException { + File world = createWorldLikeDirectory("locked"); + Path region = world.toPath().resolve("region"); + + // Clear write permission on the directory so its contents cannot be deleted. Skip where the platform or user + // does not enforce it (e.g. running as root, or a filesystem ignoring the bit) so the test stays deterministic. + assumeTrue(region.toFile().setWritable(false, false), "could not make directory read-only"); + try { + assumeTrue(!Files.isWritable(region), "directory write permission is not enforced here"); + assertThrows(IOException.class, () -> FileUtils.deleteDirectory(world)); + } finally { + region.toFile().setWritable(true, false); + } + } + @Test void getDirectoryCreation_returnsPlausibleTimestamp() throws IOException { File source = createWorldLikeDirectory("created"); From a398ee6d2e37079c03b0c4d495e99c95b0e55de2 Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 18:57:14 +0200 Subject: [PATCH 03/13] fix: snapshot entities on the main thread before async save The YAML stores ran codec.serialize() inside CompletableFuture.runAsync, so a ForkJoinPool thread read live domain state (e.g. a world's LAST_EDITED / LAST_LOADED, a property's value) with no happens-before against the main thread that mutates it via set(). The values stored are immutable, so the race risked a stale write rather than corruption, but it was still a data race. Capture the serialized map on the calling (main) thread and hand only that immutable snapshot to the async block, which now does pure file I/O. Applied uniformly to the world, folder and player stores. --- .../storage/yaml/YamlFolderStorage.java | 12 ++++++++++-- .../storage/yaml/YamlPlayerStorage.java | 17 +++++++++++++---- .../storage/yaml/YamlWorldStorage.java | 15 ++++++++++++--- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlFolderStorage.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlFolderStorage.java index 9e451987..75240e6f 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlFolderStorage.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlFolderStorage.java @@ -30,6 +30,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; import java.util.concurrent.CompletableFuture; @@ -77,17 +78,24 @@ protected Folder newFolder(String name, NavigatorCategory category, @Nullable Fo @Override public CompletableFuture save(Folder folder) { + // Serialize on the calling (main) thread; the async block only writes the captured map to disk. + String folderKey = codec().key(folder); + Map serialized = codec().serialize(folder); return CompletableFuture.runAsync(() -> store.atomicSave(() -> { config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); - config.set(FOLDERS_KEY + "." + codec().key(folder), codec().serialize(folder)); + config.set(FOLDERS_KEY + "." + folderKey, serialized); })); } @Override public CompletableFuture save(Collection folders) { + Map serialized = new LinkedHashMap<>(); + for (Folder folder : folders) { + serialized.put(codec().key(folder), codec().serialize(folder)); + } return CompletableFuture.runAsync(() -> store.atomicSave(() -> { config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); - folders.forEach(folder -> config.set(FOLDERS_KEY + "." + codec().key(folder), codec().serialize(folder))); + serialized.forEach((folderKey, value) -> config.set(FOLDERS_KEY + "." + folderKey, value)); })); } diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorage.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorage.java index 5220e8b9..dc6ec521 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorage.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorage.java @@ -23,6 +23,8 @@ import de.eintosti.buildsystem.storage.codec.PlayerCodec; import java.util.ArrayList; import java.util.Collection; +import java.util.LinkedHashMap; +import java.util.Map; import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.logging.Level; @@ -48,14 +50,21 @@ public YamlPlayerStorage(BuildSystemPlugin plugin) { @Override public CompletableFuture save(BuildPlayer buildPlayer) { - return CompletableFuture.runAsync(() -> store.atomicSave( - () -> config.set(PLAYERS_KEY + "." + codec.key(buildPlayer), codec.serialize(buildPlayer)))); + // Serialize on the calling (main) thread; the async block only writes the captured map to disk. + String playerKey = codec.key(buildPlayer); + Map serialized = codec.serialize(buildPlayer); + return CompletableFuture.runAsync( + () -> store.atomicSave(() -> config.set(PLAYERS_KEY + "." + playerKey, serialized))); } @Override public CompletableFuture save(Collection players) { - return CompletableFuture.runAsync(() -> store.atomicSave(() -> - players.forEach(player -> config.set(PLAYERS_KEY + "." + codec.key(player), codec.serialize(player))))); + Map serialized = new LinkedHashMap<>(); + for (BuildPlayer player : players) { + serialized.put(codec.key(player), codec.serialize(player)); + } + return CompletableFuture.runAsync(() -> store.atomicSave( + () -> serialized.forEach((playerKey, value) -> config.set(PLAYERS_KEY + "." + playerKey, value)))); } @Override diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorage.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorage.java index 0c5b9158..748fcb38 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorage.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorage.java @@ -25,6 +25,8 @@ import de.eintosti.buildsystem.storage.migration.StorageMigration; import java.util.ArrayList; import java.util.Collection; +import java.util.LinkedHashMap; +import java.util.Map; import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.logging.Level; @@ -65,18 +67,25 @@ private WorldCodec codec() { @Override public CompletableFuture save(BuildWorld buildWorld) { + // Serialize on the calling (main) thread, where the world's data is owned: the async block must only write the + // already-captured map to disk, never read live domain state off the main thread. + String worldKey = codec().key(buildWorld); + Map serialized = codec().serialize(buildWorld); return CompletableFuture.runAsync(() -> store.atomicSave(() -> { config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); - config.set(WORLDS_KEY + "." + codec().key(buildWorld), codec().serialize(buildWorld)); + config.set(WORLDS_KEY + "." + worldKey, serialized); })); } @Override public CompletableFuture save(Collection buildWorlds) { + Map serialized = new LinkedHashMap<>(); + for (BuildWorld buildWorld : buildWorlds) { + serialized.put(codec().key(buildWorld), codec().serialize(buildWorld)); + } return CompletableFuture.runAsync(() -> store.atomicSave(() -> { config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); - buildWorlds.forEach(buildWorld -> - config.set(WORLDS_KEY + "." + codec().key(buildWorld), codec().serialize(buildWorld))); + serialized.forEach((worldKey, value) -> config.set(WORLDS_KEY + "." + worldKey, value)); })); } From 72a61c91d760772a80f6df6d1462e638bfe78d39 Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 19:03:31 +0200 Subject: [PATCH 04/13] fix: order world rename for concurrent readers; document storage threading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verification corrected the original audit note here: the world and player indexes are read off the main thread (AsyncPlayerPreLoginEvent resolves a returning player's last world and settings on Bukkit's async login thread), so the ConcurrentHashMaps and the writers' synchronization are justified, not redundant — switching to plain maps would have introduced a race. The real fix is rename ordering: it removed the old name before adding the new one, so a concurrent reader could momentarily resolve neither. Publish the new name first, then drop the old (guarding case-only renames that share a key). Document the threading model on both storages so the ConcurrentHashMaps are not later 'simplified' away. --- .../buildsystem/storage/PlayerStorageImpl.java | 6 ++++++ .../buildsystem/storage/WorldStorageImpl.java | 17 +++++++++++++++-- .../storage/WorldStorageImplTest.java | 13 +++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/PlayerStorageImpl.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/PlayerStorageImpl.java index 42a902a4..726a1193 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/PlayerStorageImpl.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/PlayerStorageImpl.java @@ -34,6 +34,12 @@ import org.jspecify.annotations.NullMarked; import org.jspecify.annotations.Nullable; +/** + * In-memory cache of {@link BuildPlayer}s keyed by UUID. Like the world storage, lookups may run off the main thread — + * {@code AsyncPlayerPreLoginEvent} reads a connecting player's settings on Bukkit's async login thread — so the cache is + * a {@link ConcurrentHashMap}. The startup {@link #loadPlayers() load} publishes its entries with {@code putAll}: saved + * data wins over any blank entry a concurrent lookup may have created in the brief load window. + */ @NullMarked public abstract class PlayerStorageImpl implements PlayerStorage { diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/WorldStorageImpl.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/WorldStorageImpl.java index 2276c0b8..88177216 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/WorldStorageImpl.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/WorldStorageImpl.java @@ -37,6 +37,13 @@ import org.jspecify.annotations.NullMarked; import org.jspecify.annotations.Nullable; +/** + * In-memory index of the server's {@link BuildWorld}s, keyed both by UUID and by lower-cased name. Mutations + * ({@link #addBuildWorld}, {@link #removeBuildWorld}, {@link #rename}) run on the main thread, but lookups may be called + * off it: {@code AsyncPlayerPreLoginEvent} resolves a returning player's last world on Bukkit's async login thread. The + * indexes are therefore {@link ConcurrentHashMap}s so concurrent reads stay safe and consistently published, and the + * compound mutations are ordered so a concurrent reader never observes a world's two index entries out of step. + */ @NullMarked public abstract class WorldStorageImpl implements WorldStorage { @@ -99,8 +106,14 @@ public synchronized void removeBuildWorld(BuildWorld buildWorld) { } public synchronized void rename(BuildWorld buildWorld, String oldName, String newName) { - this.uuidByName.remove(oldName.toLowerCase()); - this.uuidByName.put(newName.toLowerCase(), buildWorld.getUniqueId()); + String oldKey = oldName.toLowerCase(); + String newKey = newName.toLowerCase(); + // Publish the new name before dropping the old one so a concurrent (async) reader never sees the world vanish. + // Guard the removal: a case-only rename maps both names to the same key, which must stay resolvable. + this.uuidByName.put(newKey, buildWorld.getUniqueId()); + if (!oldKey.equals(newKey)) { + this.uuidByName.remove(oldKey); + } } @Override diff --git a/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/WorldStorageImplTest.java b/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/WorldStorageImplTest.java index 0cf4c495..09f0f3af 100644 --- a/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/WorldStorageImplTest.java +++ b/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/WorldStorageImplTest.java @@ -173,6 +173,19 @@ void concurrentAddRemoveIsConsistent() throws InterruptedException { } } + @Test + void rename_caseOnlyChange_keepsNameResolvable() { + // Old and new names lower-case to the same key; the world must stay resolvable after the rename. + BuildWorld w = world("India"); + storage.addBuildWorld(w); + + when(w.getName()).thenReturn("INDIA"); + storage.rename(w, "India", "INDIA"); + + assertSame(w, storage.getBuildWorld("india")); + assertSame(w, storage.getBuildWorld("INDIA")); + } + @Test void rename_remapsNameLookup() { BuildWorld buildWorld = world("oldName"); From 9a9a77a816bad60686df6f3ab9f2d2ff9fe94562 Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 19:08:06 +0200 Subject: [PATCH 05/13] refactor: folder owns its sub-folder list; worlds are a set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setParent and the constructor reached into another FolderImpl's private subfolders field through a downcast, mutating a sibling's state directly. Route it through package-private addSubFolder/removeSubFolder so a folder maintains its own list and enforces de-duplication, and defensively copy the subfolders the constructor is handed (worlds already were copied). Back the world membership with a LinkedHashSet instead of a List guarded by a manual contains check — uniqueness is the intent, insertion order is preserved, and addWorld drops the guard. --- .../buildsystem/world/folder/FolderImpl.java | 44 +++++++++----- .../world/folder/FolderImplTest.java | 58 +++++++++++++++++++ 2 files changed, 88 insertions(+), 14 deletions(-) diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/folder/FolderImpl.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/folder/FolderImpl.java index 6699f405..3ebde28a 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/folder/FolderImpl.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/folder/FolderImpl.java @@ -42,7 +42,7 @@ public class FolderImpl implements Folder { private final Builder creator; private final long creation; private NavigatorCategory category; - private final List worlds; + private final Set worlds; private final List subfolders; private @Nullable Folder parent; @@ -88,15 +88,15 @@ public FolderImpl( this.creation = creation; this.category = category; this.parent = parent; - if (parent != null && !parent.getSubFolders().contains(this)) { - ((FolderImpl) parent).subfolders.add(this); - } this.creator = creator; - this.worlds = new ArrayList<>(worlds); + this.worlds = new LinkedHashSet<>(worlds); this.material = material; this.permission = permission; this.project = project; - this.subfolders = subfolders; + this.subfolders = new ArrayList<>(subfolders); + if (parent != null) { + ((FolderImpl) parent).addSubFolder(this); + } } @Override @@ -195,10 +195,10 @@ public void setParent(@Nullable Folder parent) { .formatted(this.category.getId(), parent.getCategory().getId())); } - if (parent != null && !parent.getSubFolders().contains(this)) { - ((FolderImpl) parent).subfolders.add(this); - } else if (parent == null && this.parent != null) { - ((FolderImpl) this.parent).subfolders.remove(this); + if (parent != null) { + ((FolderImpl) parent).addSubFolder(this); + } else if (this.parent != null) { + ((FolderImpl) this.parent).removeSubFolder(this); } this.parent = parent; @@ -212,7 +212,7 @@ public boolean hasParent() { @Override @Unmodifiable public List getWorldUUIDs() { - return Collections.unmodifiableList(this.worlds); + return List.copyOf(this.worlds); } @Override @@ -227,9 +227,7 @@ public boolean containsWorld(UUID uuid) { @Override public void addWorld(BuildWorld buildWorld) { - if (!this.worlds.contains(buildWorld.getUniqueId())) { - this.worlds.add(buildWorld.getUniqueId()); - } + this.worlds.add(buildWorld.getUniqueId()); // BuildWorld owns the back-reference; only update it when it is not already pointing here, which also // terminates the addWorld <-> setFolder handshake. if (buildWorld.getFolder() != this) { @@ -257,6 +255,24 @@ public List getSubFolders() { return Collections.unmodifiableList(this.subfolders); } + /** + * Adds {@code child} to this folder's sub-folder list, ignoring duplicates. Package-private: membership is driven by + * {@link #setParent}, so a folder owns its own sub-folder list rather than having it mutated through a cast from + * outside. + */ + void addSubFolder(FolderImpl child) { + if (!this.subfolders.contains(child)) { + this.subfolders.add(child); + } + } + + /** + * Removes {@code child} from this folder's sub-folder list. Counterpart to {@link #addSubFolder(FolderImpl)}. + */ + void removeSubFolder(FolderImpl child) { + this.subfolders.remove(child); + } + @Override public int getWorldCount() { int total = this.worlds.size(); diff --git a/buildsystem-core/src/test/java/de/eintosti/buildsystem/world/folder/FolderImplTest.java b/buildsystem-core/src/test/java/de/eintosti/buildsystem/world/folder/FolderImplTest.java index 65ad3e55..8417dd99 100644 --- a/buildsystem-core/src/test/java/de/eintosti/buildsystem/world/folder/FolderImplTest.java +++ b/buildsystem-core/src/test/java/de/eintosti/buildsystem/world/folder/FolderImplTest.java @@ -28,6 +28,7 @@ import de.eintosti.buildsystem.world.WorldContext; import de.eintosti.buildsystem.world.data.WorldDataImpl; import de.eintosti.buildsystem.world.data.WorldDataImpl.WorldDataBuilder; +import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -137,6 +138,63 @@ void removeWorld_clearsBothSides() { assertFalse(folder.containsWorld(world)); } + @Test + void addWorld_isIdempotent() { + FolderImpl folder = folder("Target"); + BuildWorldImpl world = world("world"); + + folder.addWorld(world); + folder.addWorld(world); + + assertEquals(1, folder.getWorldUUIDs().size()); + assertEquals(1, folder.getWorldCount()); + } + + @Test + void setParent_registersThenDeregistersSubFolder() { + FolderImpl parent = folder("Parent"); + FolderImpl child = folder("Child"); + + child.setParent(parent); + assertTrue(parent.getSubFolders().contains(child)); + + child.setParent(null); + assertTrue(parent.getSubFolders().isEmpty()); + } + + @Test + void setParent_repeatedDoesNotDuplicate() { + FolderImpl parent = folder("Parent"); + FolderImpl child = folder("Child"); + + child.setParent(parent); + child.setParent(parent); + + assertEquals(1, parent.getSubFolders().size()); + } + + @Test + void constructor_defensivelyCopiesWorlds() { + List worlds = new ArrayList<>(List.of(UUID.randomUUID())); + FolderImpl folder = new FolderImpl( + context, + UUID.randomUUID(), + "Detached", + 0L, + TestData.PUBLIC, + null, + Builder.of(UUID.randomUUID(), "Creator"), + XMaterial.CHEST, + "-", + "-", + worlds, + new ArrayList<>()); + + worlds.clear(); + + assertEquals(1, folder.getWorldUUIDs().size()); + } + @Test void identity_survivesRename() { FolderImpl folder = folder("Original"); From b64d494257ea6d4c4cecb9afae08181e14edc201 Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 19:10:14 +0200 Subject: [PATCH 06/13] feat: reject reserved and degenerate world names isPathEscape covers traversal, but names that stay inside the container can still be unusable: the current/parent aliases (./..), the empty name, and the Windows device names (CON, PRN, AUX, NUL, COM1-9, LPT1-9). Add StringCleaner.isReservedName and enforce it at the WorldService.newWorld chokepoint that menu creation and the public API both flow through. --- .../buildsystem/util/StringCleaner.java | 24 ++++++++++++++++++ .../buildsystem/world/WorldServiceImpl.java | 3 +++ .../buildsystem/util/StringCleanerTest.java | 25 +++++++++++++++++++ .../world/folder/FolderImplTest.java | 6 +---- 4 files changed, 53 insertions(+), 5 deletions(-) diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/StringCleaner.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/StringCleaner.java index 381be164..2668fe59 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/StringCleaner.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/StringCleaner.java @@ -20,6 +20,8 @@ import java.io.File; import java.io.IOException; import java.util.Arrays; +import java.util.Locale; +import java.util.Set; import java.util.regex.Pattern; import org.jspecify.annotations.NullMarked; import org.jspecify.annotations.Nullable; @@ -32,6 +34,11 @@ public final class StringCleaner { private static final Pattern INVALID_NAME_PATTERN = Pattern.compile(INVALID_NAME_CHARACTERS); + /** Windows device names that cannot back a directory regardless of extension. */ + private static final Set RESERVED_NAMES = Set.of( + "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", "LPT1", + "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9"); + private StringCleaner() {} public static boolean hasInvalidNameCharacters(String input, String configuredPattern) { @@ -57,6 +64,23 @@ public static String sanitize(String input, String configuredPattern) { .trim(); } + /** + * Checks whether a name is reserved or degenerate as a directory name. {@link #isPathEscape} already rejects names + * that traverse out of the container; this rejects ones that stay inside it but still cannot back a world folder: + * the current/parent aliases ({@code .}/{@code ..}), the empty name, and the Windows device names (CON, PRN, AUX, + * NUL, COM1-9, LPT1-9) that are unusable even on a server that later moves between platforms. + * + * @param name the candidate world or folder name + * @return {@code true} if the name must not be used + */ + public static boolean isReservedName(String name) { + String trimmed = name.trim(); + if (trimmed.isEmpty() || trimmed.equals(".") || trimmed.equals("..")) { + return true; + } + return RESERVED_NAMES.contains(trimmed.toUpperCase(Locale.ROOT)); + } + /** * Checks whether a resolved file escapes a given base directory. * diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java index 75dc5782..01a9d1cb 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java @@ -114,6 +114,9 @@ public WorldStorageImpl getWorldStorage() { public WorldBuilder newWorld(String name) { // Defense-in-depth: the menus sanitize names before they reach here, but this is public API. Reject any name // that would resolve outside the world container so a caller cannot create a directory in, say, plugins/. + if (StringCleaner.isReservedName(name)) { + throw new IllegalArgumentException("World name '" + name + "' is reserved and cannot be used"); + } File worldDirectory = new File(Bukkit.getWorldContainer(), name); if (StringCleaner.isPathEscape(Bukkit.getWorldContainer(), worldDirectory)) { throw new IllegalArgumentException("World name '" + name + "' resolves outside the world container"); diff --git a/buildsystem-core/src/test/java/de/eintosti/buildsystem/util/StringCleanerTest.java b/buildsystem-core/src/test/java/de/eintosti/buildsystem/util/StringCleanerTest.java index f82c57cf..2e90ecd7 100644 --- a/buildsystem-core/src/test/java/de/eintosti/buildsystem/util/StringCleanerTest.java +++ b/buildsystem-core/src/test/java/de/eintosti/buildsystem/util/StringCleanerTest.java @@ -84,6 +84,31 @@ void sanitize_trailingWhitespace_trimmed() { assertEquals("abc", StringCleaner.sanitize(" abc ", NO_OP)); } + @Test + void isReservedName_windowsDeviceNames_returnTrueCaseInsensitively() { + assertTrue(StringCleaner.isReservedName("CON")); + assertTrue(StringCleaner.isReservedName("con")); + assertTrue(StringCleaner.isReservedName("CoM1")); + assertTrue(StringCleaner.isReservedName("lpt9")); + assertTrue(StringCleaner.isReservedName("NUL")); + } + + @Test + void isReservedName_dotAndEmpty_returnTrue() { + assertTrue(StringCleaner.isReservedName(".")); + assertTrue(StringCleaner.isReservedName("..")); + assertTrue(StringCleaner.isReservedName("")); + assertTrue(StringCleaner.isReservedName(" ")); + } + + @Test + void isReservedName_ordinaryNames_returnFalse() { + assertFalse(StringCleaner.isReservedName("World")); + assertFalse(StringCleaner.isReservedName("COM")); // no trailing digit + assertFalse(StringCleaner.isReservedName("COM10")); // outside COM1-9 + assertFalse(StringCleaner.isReservedName("console")); + } + @Test void isPathEscape_normalChild_returnsFalse() { File base = new File("/srv/templates"); diff --git a/buildsystem-core/src/test/java/de/eintosti/buildsystem/world/folder/FolderImplTest.java b/buildsystem-core/src/test/java/de/eintosti/buildsystem/world/folder/FolderImplTest.java index 8417dd99..46b49565 100644 --- a/buildsystem-core/src/test/java/de/eintosti/buildsystem/world/folder/FolderImplTest.java +++ b/buildsystem-core/src/test/java/de/eintosti/buildsystem/world/folder/FolderImplTest.java @@ -28,11 +28,7 @@ import de.eintosti.buildsystem.world.WorldContext; import de.eintosti.buildsystem.world.data.WorldDataImpl; import de.eintosti.buildsystem.world.data.WorldDataImpl.WorldDataBuilder; -import java.util.ArrayList; -import java.util.HashSet; -import java.util.List; -import java.util.Set; -import java.util.UUID; +import java.util.*; import org.bukkit.Difficulty; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; From 257a67e648aa456ec5497630a77beb38056b9030 Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 19:39:18 +0200 Subject: [PATCH 07/13] refactor: pull config file-I/O and storage validation out of parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ConfigService.parse did filesystem I/O (WorldEdit wand detection reaching into the plugins directory), which forced a parseForTest/nullable-pluginDir seam, and built S3/SFTP storage settings with no validation — missing credentials produced a remote record with null fields that only failed later at connection time. Extract WorldEditWandDetector so parsing is a pure transform of the config (the test seam is gone; tests pass the wand explicitly), and StorageSettingsFactory which validates required credentials, logs exactly which key is missing, and falls back to local storage. Add detector + missing-credential tests. --- .../buildsystem/config/ConfigService.java | 111 ++--------------- .../config/StorageSettingsFactory.java | 115 ++++++++++++++++++ .../config/WorldEditWandDetector.java | 92 ++++++++++++++ .../buildsystem/config/PluginConfigTest.java | 21 +++- .../config/WorldEditWandDetectorTest.java | 68 +++++++++++ 5 files changed, 308 insertions(+), 99 deletions(-) create mode 100644 buildsystem-core/src/main/java/de/eintosti/buildsystem/config/StorageSettingsFactory.java create mode 100644 buildsystem-core/src/main/java/de/eintosti/buildsystem/config/WorldEditWandDetector.java create mode 100644 buildsystem-core/src/test/java/de/eintosti/buildsystem/config/WorldEditWandDetectorTest.java diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/config/ConfigService.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/config/ConfigService.java index 789400ed..8d401f38 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/config/ConfigService.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/config/ConfigService.java @@ -21,14 +21,12 @@ import com.cryptomorin.xseries.XMaterial; import de.eintosti.buildsystem.BuildSystemPlugin; import de.eintosti.buildsystem.world.menu.GameRuleEntry; -import java.io.File; import java.util.*; import java.util.logging.Logger; import java.util.stream.Collectors; import org.bukkit.Difficulty; import org.bukkit.GameMode; import org.bukkit.configuration.file.FileConfiguration; -import org.bukkit.configuration.file.YamlConfiguration; import org.jspecify.annotations.NullMarked; import org.jspecify.annotations.Nullable; @@ -36,12 +34,14 @@ public class ConfigService { private final BuildSystemPlugin plugin; + private final WorldEditWandDetector wandDetector; private volatile PluginConfig current; public ConfigService(BuildSystemPlugin plugin) { this.plugin = plugin; + this.wandDetector = new WorldEditWandDetector(plugin.getDataFolder().getParentFile()); // Initialize with a placeholder; load() must be called before current() is used. - this.current = parse(plugin.getConfig(), plugin.getLogger()); + this.current = parse(plugin.getConfig(), plugin.getLogger(), wandDetector.detect()); } /** @@ -57,7 +57,7 @@ public PluginConfig current() { * Reloads the configuration from disk and re-parses it into a new {@link PluginConfig}. */ public void load() { - this.current = parse(plugin.getConfig(), plugin.getLogger()); + this.current = parse(plugin.getConfig(), plugin.getLogger(), wandDetector.detect()); } /** @@ -97,34 +97,20 @@ public void saveConfig() { } /** - * Parses the given {@link FileConfiguration} into a {@link PluginConfig} record tree. + * Parses the given {@link FileConfiguration} into a {@link PluginConfig} record tree. Pure: it reads only from + * {@code config}. The WorldEdit wand is resolved separately (see {@link WorldEditWandDetector}) and passed in, so + * parsing never touches the filesystem. * * @param config The file configuration to parse * @param logger The logger to use for warnings + * @param worldEditWand The resolved WorldEdit wand material * @return The parsed {@link PluginConfig} */ - PluginConfig parse(FileConfiguration config, Logger logger) { - return parse(config, logger, plugin.getDataFolder().getParentFile()); + static PluginConfig parse(FileConfiguration config, Logger logger, XMaterial worldEditWand) { + return new PluginConfig(parseSettings(config, worldEditWand), parseWorld(config, logger), parseFolder(config)); } - /** - * Parses a config for testing purposes (WorldEdit wand detection skipped — returns default - * {@link XMaterial#WOODEN_AXE}). - * - * @param config The file configuration to parse - * @param logger The logger to use for warnings - * @return The parsed {@link PluginConfig} - */ - static PluginConfig parseForTest(FileConfiguration config, Logger logger) { - return parse(config, logger, null); - } - - private static PluginConfig parse(FileConfiguration config, Logger logger, @Nullable File pluginParentDir) { - return new PluginConfig( - parseSettings(config, pluginParentDir), parseWorld(config, logger), parseFolder(config)); - } - - private static PluginConfig.Settings parseSettings(FileConfiguration config, @Nullable File pluginParentDir) { + private static PluginConfig.Settings parseSettings(FileConfiguration config, XMaterial worldEditWand) { PluginConfig.Settings.Archive archive = new PluginConfig.Settings.Archive( config.getBoolean("settings.archive.vanish", true), config.getBoolean("settings.archive.change-gamemode", true), @@ -139,8 +125,7 @@ private static PluginConfig.Settings parseSettings(FileConfiguration config, @Nu config.getBoolean("settings.build-mode.move-items", true)); PluginConfig.Settings.Builder builder = new PluginConfig.Settings.Builder( - config.getBoolean("settings.builder.block-worldedit-non-builder", true), - parseWorldEditWand(pluginParentDir)); + config.getBoolean("settings.builder.block-worldedit-non-builder", true), worldEditWand); PluginConfig.Settings.Navigator navigator = new PluginConfig.Settings.Navigator( XMaterial.valueOf(Objects.requireNonNullElse(config.getString("settings.navigator.item"), "CLOCK")), @@ -211,7 +196,7 @@ private static PluginConfig.World parseWorld(FileConfiguration config, Logger lo PluginConfig.World.Backup backup = new PluginConfig.World.Backup( Math.min(config.getInt("world.backup.max-backups-per-world", 5), 18), - parseStorageSettings(config, logger), + StorageSettingsFactory.fromConfig(config, logger), autoBackup); Set deletionBlacklist = config.getStringList("world.deletion-blacklist").stream() @@ -280,76 +265,6 @@ private static PluginConfig.Folder parseFolder(FileConfiguration config) { config.getBoolean("folder.override-projects", false)); } - private static PluginConfig.World.Backup.StorageSettings parseStorageSettings( - FileConfiguration config, Logger logger) { - String type = Objects.requireNonNullElse(config.getString("world.backup.storage.type"), "local") - .toLowerCase(); - - return switch (type) { - case "s3" -> - new PluginConfig.World.Backup.S3( - config.getString("world.backup.storage.s3.url"), - config.getString("world.backup.storage.s3.access-key"), - config.getString("world.backup.storage.s3.secret-key"), - config.getString("world.backup.storage.s3.region"), - config.getString("world.backup.storage.s3.bucket"), - config.getString("world.backup.storage.s3.path")); - case "sftp" -> - new PluginConfig.World.Backup.Sftp( - config.getString("world.backup.storage.sftp.host"), - config.getInt("world.backup.storage.sftp.port", 22), - config.getString("world.backup.storage.sftp.username"), - config.getString("world.backup.storage.sftp.password"), - config.getString("world.backup.storage.sftp.path")); - default -> { - if (!type.equals("local")) { - logger.warning("Unknown backup storage type '" + type + "', defaulting to local storage."); - } - yield new PluginConfig.World.Backup.Local(); - } - }; - } - - /** - * Parses the WorldEdit wand material from the WorldEdit or FastAsyncWorldEdit config file. - * - * @param pluginDir The parent directory of all plugins, or null to skip WorldEdit detection - * @return The parsed {@link XMaterial} for the WorldEdit wand - */ - static XMaterial parseWorldEditWand(@Nullable File pluginDir) { - File configFile = null; - - if (pluginDir != null) { - File weConfig = new File(pluginDir + File.separator + "WorldEdit", "config.yml"); - if (weConfig.exists()) { - configFile = weConfig; - } - - File faweConfig = new File(pluginDir + File.separator + "FastAsyncWorldEdit", "worldedit-config.yml"); - if (faweConfig.exists()) { - configFile = faweConfig; - } - } - - XMaterial defaultWand = XMaterial.WOODEN_AXE; - if (configFile == null) { - return defaultWand; - } - - YamlConfiguration weYaml = YamlConfiguration.loadConfiguration(configFile); - String wand = weYaml.getString("wand-item"); - if (wand == null) { - return defaultWand; - } - - String namespace = "minecraft:"; - if (wand.toLowerCase(Locale.ROOT).startsWith(namespace)) { - wand = wand.substring(namespace.length()); - } - - return XMaterial.matchXMaterial(wand).orElse(defaultWand); - } - /** * Parses the {@link GameMode} from a string, defaulting to {@link GameMode#ADVENTURE} if the string is not valid. * diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/config/StorageSettingsFactory.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/config/StorageSettingsFactory.java new file mode 100644 index 00000000..5a85acd1 --- /dev/null +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/config/StorageSettingsFactory.java @@ -0,0 +1,115 @@ +/* + * Copyright (c) 2018-2026, Thomas Meaney + * Copyright (c) contributors + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package de.eintosti.buildsystem.config; + +import de.eintosti.buildsystem.config.PluginConfig.World.Backup.Local; +import de.eintosti.buildsystem.config.PluginConfig.World.Backup.S3; +import de.eintosti.buildsystem.config.PluginConfig.World.Backup.Sftp; +import de.eintosti.buildsystem.config.PluginConfig.World.Backup.StorageSettings; +import java.util.LinkedHashMap; +import java.util.Locale; +import java.util.Map; +import java.util.logging.Logger; +import org.bukkit.configuration.file.FileConfiguration; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +/** + * Builds the backup {@link StorageSettings} from config, validating that a chosen remote backend actually has its + * required credentials before selecting it. A misconfigured remote logs exactly which key is missing and falls back to + * {@link Local local} storage, rather than constructing a remote with {@code null} fields that fails only later at + * connection time. + */ +@NullMarked +final class StorageSettingsFactory { + + private static final String BASE = "world.backup.storage."; + + private StorageSettingsFactory() {} + + static StorageSettings fromConfig(FileConfiguration config, Logger logger) { + String type = config.getString(BASE + "type", "local").toLowerCase(Locale.ROOT); + return switch (type) { + case "s3" -> s3(config, logger); + case "sftp" -> sftp(config, logger); + case "local" -> new Local(); + default -> { + logger.warning("Unknown backup storage type '" + type + "', defaulting to local storage."); + yield new Local(); + } + }; + } + + private static StorageSettings s3(FileConfiguration config, Logger logger) { + String prefix = BASE + "s3."; + Map required = new LinkedHashMap<>(); + required.put(prefix + "url", config.getString(prefix + "url")); + required.put(prefix + "access-key", config.getString(prefix + "access-key")); + required.put(prefix + "secret-key", config.getString(prefix + "secret-key")); + required.put(prefix + "bucket", config.getString(prefix + "bucket")); + + if (fallbackOnMissing(required, "s3", logger)) { + return new Local(); + } + return new S3( + config.getString(prefix + "url"), + config.getString(prefix + "access-key"), + config.getString(prefix + "secret-key"), + config.getString(prefix + "region"), + config.getString(prefix + "bucket"), + config.getString(prefix + "path")); + } + + private static StorageSettings sftp(FileConfiguration config, Logger logger) { + String prefix = BASE + "sftp."; + Map required = new LinkedHashMap<>(); + required.put(prefix + "host", config.getString(prefix + "host")); + required.put(prefix + "username", config.getString(prefix + "username")); + required.put(prefix + "password", config.getString(prefix + "password")); + + if (fallbackOnMissing(required, "sftp", logger)) { + return new Local(); + } + + return new Sftp( + config.getString(prefix + "host"), + config.getInt(prefix + "port", 22), + config.getString(prefix + "username"), + config.getString(prefix + "password"), + config.getString(prefix + "path")); + } + + /** + * {@return whether a required value is missing} Logs the first blank/absent key together with the backend name when + * so, signalling the caller to fall back to local storage. + */ + private static boolean fallbackOnMissing(Map required, String backend, Logger logger) { + for (Map.Entry entry : required.entrySet()) { + if (isBlank(entry.getValue())) { + logger.warning("Backup storage '" + backend + "' is missing required setting '" + entry.getKey() + + "'; falling back to local storage."); + return true; + } + } + return false; + } + + private static boolean isBlank(@Nullable String value) { + return value == null || value.isBlank(); + } +} diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/config/WorldEditWandDetector.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/config/WorldEditWandDetector.java new file mode 100644 index 00000000..aaa0ed20 --- /dev/null +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/config/WorldEditWandDetector.java @@ -0,0 +1,92 @@ +/* + * Copyright (c) 2018-2026, Thomas Meaney + * Copyright (c) contributors + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package de.eintosti.buildsystem.config; + +import com.cryptomorin.xseries.XMaterial; +import java.io.File; +import java.util.Locale; +import org.bukkit.configuration.file.YamlConfiguration; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +/** + * Resolves the WorldEdit wand material by reading WorldEdit's (or FastAsyncWorldEdit's) own config file, so the plugin + * can recognise that item inside protected worlds. Kept out of {@link ConfigService}'s parsing so config parsing stays + * a pure, side-effect-free transform of the {@code FileConfiguration} — the filesystem lookup lives here and is easy to + * stub in tests by constructing the detector with a controlled directory (or {@code null} to skip detection). + */ +@NullMarked +public final class WorldEditWandDetector { + + /** Used when no WorldEdit install is found or its wand cannot be resolved. */ + public static final XMaterial DEFAULT_WAND = XMaterial.WOODEN_AXE; + + private final @Nullable File pluginsDir; + + /** + * @param pluginsDir The server's plugins directory (the parent of this plugin's data folder), or {@code null} to + * skip detection and always return {@link #DEFAULT_WAND} + */ + public WorldEditWandDetector(@Nullable File pluginsDir) { + this.pluginsDir = pluginsDir; + } + + /** + * {@return the configured WorldEdit wand, or {@link #DEFAULT_WAND} when WorldEdit is absent or its wand cannot be + * resolved} + */ + public XMaterial detect() { + File configFile = locateConfig(); + if (configFile == null) { + return DEFAULT_WAND; + } + + YamlConfiguration weConfig = YamlConfiguration.loadConfiguration(configFile); + String wand = weConfig.getString("wand-item"); + if (wand == null) { + return DEFAULT_WAND; + } + + String namespace = "minecraft:"; + if (wand.toLowerCase(Locale.ROOT).startsWith(namespace)) { + wand = wand.substring(namespace.length()); + } + return XMaterial.matchXMaterial(wand).orElse(DEFAULT_WAND); + } + + /** + * FastAsyncWorldEdit takes precedence over WorldEdit when both are installed, matching the prior behaviour. + */ + private @Nullable File locateConfig() { + if (pluginsDir == null) { + return null; + } + + File faweConfig = new File(pluginsDir + File.separator + "FastAsyncWorldEdit", "worldedit-config.yml"); + if (faweConfig.exists()) { + return faweConfig; + } + + File weConfig = new File(pluginsDir + File.separator + "WorldEdit", "config.yml"); + if (weConfig.exists()) { + return weConfig; + } + + return null; + } +} diff --git a/buildsystem-core/src/test/java/de/eintosti/buildsystem/config/PluginConfigTest.java b/buildsystem-core/src/test/java/de/eintosti/buildsystem/config/PluginConfigTest.java index c87385d0..a26a4b4b 100644 --- a/buildsystem-core/src/test/java/de/eintosti/buildsystem/config/PluginConfigTest.java +++ b/buildsystem-core/src/test/java/de/eintosti/buildsystem/config/PluginConfigTest.java @@ -35,7 +35,7 @@ private PluginConfig parse(String yaml) { } catch (Exception e) { throw new RuntimeException(e); } - return ConfigService.parseForTest(config, LOGGER); + return ConfigService.parse(config, LOGGER, XMaterial.WOODEN_AXE); } // ----------------------------------------------------------------------- @@ -275,6 +275,25 @@ void backupStorage_sftpType_returnsSftpRecord() { assertEquals("/backups/", sftp.path()); } + @Test + void backupStorage_s3MissingRequiredKey_fallsBackToLocal() { + // 'bucket' is required for S3; without it the remote cannot work, so it must fall back to local rather than + // construct an S3 record with a null bucket that only fails later at connection time. + PluginConfig cfg = parse(""" + world: + backup: + storage: + type: s3 + s3: + url: "https://example.com" + access-key: "MYACCESSKEY" + secret-key: "MYSECRETKEY" + """); + + assertInstanceOf( + PluginConfig.World.Backup.Local.class, cfg.world().backup().storage()); + } + // ----------------------------------------------------------------------- // 5. Backup storage: unknown type defaults to Local // ----------------------------------------------------------------------- diff --git a/buildsystem-core/src/test/java/de/eintosti/buildsystem/config/WorldEditWandDetectorTest.java b/buildsystem-core/src/test/java/de/eintosti/buildsystem/config/WorldEditWandDetectorTest.java new file mode 100644 index 00000000..017d24c6 --- /dev/null +++ b/buildsystem-core/src/test/java/de/eintosti/buildsystem/config/WorldEditWandDetectorTest.java @@ -0,0 +1,68 @@ +/* + * Copyright (c) 2018-2026, Thomas Meaney + * Copyright (c) contributors + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package de.eintosti.buildsystem.config; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import com.cryptomorin.xseries.XMaterial; +import java.nio.file.Files; +import java.nio.file.Path; +import org.jspecify.annotations.NullMarked; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +@NullMarked +class WorldEditWandDetectorTest { + + @TempDir + Path pluginsDir; + + private void writeConfig(String pluginDir, String fileName, String yaml) throws Exception { + Path dir = Files.createDirectories(pluginsDir.resolve(pluginDir)); + Files.writeString(dir.resolve(fileName), yaml); + } + + @Test + void detect_nullDir_returnsDefault() { + assertEquals(WorldEditWandDetector.DEFAULT_WAND, new WorldEditWandDetector(null).detect()); + } + + @Test + void detect_noWorldEditInstalled_returnsDefault() { + assertEquals(WorldEditWandDetector.DEFAULT_WAND, new WorldEditWandDetector(pluginsDir.toFile()).detect()); + } + + @Test + void detect_worldEditConfig_readsAndStripsNamespace() throws Exception { + writeConfig("WorldEdit", "config.yml", "wand-item: minecraft:blaze_rod"); + assertEquals(XMaterial.BLAZE_ROD, new WorldEditWandDetector(pluginsDir.toFile()).detect()); + } + + @Test + void detect_fastAsyncWorldEditTakesPrecedence() throws Exception { + writeConfig("WorldEdit", "config.yml", "wand-item: minecraft:blaze_rod"); + writeConfig("FastAsyncWorldEdit", "worldedit-config.yml", "wand-item: minecraft:stick"); + assertEquals(XMaterial.STICK, new WorldEditWandDetector(pluginsDir.toFile()).detect()); + } + + @Test + void detect_missingWandKey_returnsDefault() throws Exception { + writeConfig("WorldEdit", "config.yml", "some-other-key: 5"); + assertEquals(WorldEditWandDetector.DEFAULT_WAND, new WorldEditWandDetector(pluginsDir.toFile()).detect()); + } +} From a50db9c24b8b15e994a98906af1784d14632a7bf Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 19:46:06 +0200 Subject: [PATCH 08/13] refactor: key WorldCodec off WorldDataKey instead of duplicated constants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The codec carried its own DATA_* string constants that, per its own javadoc, 'must stay in lock-step' with the WorldDataKey ids WorldDataImpl registers and serializes by — a duplicated source of truth waiting to drift. Read the data keys straight from WorldDataKey via a dataPath() helper and delete the parallel constant list; only the pre-4.0 'private' legacy key (which has no WorldDataKey) remains a literal. Serialize already keyed off the catalog, so both directions now share one source. --- .../buildsystem/storage/codec/WorldCodec.java | 91 +++++++++---------- 1 file changed, 41 insertions(+), 50 deletions(-) diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/codec/WorldCodec.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/codec/WorldCodec.java index 0e987ddb..ca13cc1c 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/codec/WorldCodec.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/codec/WorldCodec.java @@ -24,6 +24,7 @@ import de.eintosti.buildsystem.api.world.data.BuildWorldStatus; import de.eintosti.buildsystem.api.world.data.BuildWorldType; import de.eintosti.buildsystem.api.world.data.Visibility; +import de.eintosti.buildsystem.api.world.data.WorldDataKey; import de.eintosti.buildsystem.api.world.data.WorldStatusRegistry; import de.eintosti.buildsystem.player.PlayerLookupService; import de.eintosti.buildsystem.world.BuildWorldImpl; @@ -47,10 +48,10 @@ * {@link Codec} for {@link BuildWorld}s, mapping a world to and from its section. Since v4 the section is keyed by the * world's UUID and the name is carried as a {@code name} field (a rename is then a field update, not a key move). * - *

The bulk of a world's state lives under the nested {@code data} section, whose keys mirror the property keys - * registered in {@link WorldDataImpl} — the {@code DATA_*} constants here must stay in lock-step with those. Reads are - * defensive: unknown enums fall back to safe defaults and a single unparseable entry surfaces as an exception for the - * storage to skip rather than aborting the whole load. + *

The bulk of a world's state lives under the nested {@code data} section, whose keys come straight from + * {@link WorldDataKey} via {@link #dataPath} — the same catalog {@link WorldDataImpl} registers and serializes by, so + * there is no parallel key list to keep in sync. Reads are defensive: unknown enums fall back to safe defaults and a + * single unparseable entry surfaces as an exception for the storage to skip rather than aborting the whole load. */ @NullMarked public final class WorldCodec implements Codec { @@ -65,27 +66,8 @@ public final class WorldCodec implements Codec { private static final String CHUNK_GENERATOR = "chunk-generator"; private static final String DATA = "data"; - private static final String DATA_SPAWN = "spawn"; - private static final String DATA_PERMISSION = "permission"; - private static final String DATA_PROJECT = "project"; - private static final String DATA_DIFFICULTY = "difficulty"; - private static final String DATA_MATERIAL = "material"; - private static final String DATA_ICON_SKULL_TEXTURE = "icon-skull-texture"; - private static final String DATA_STATUS = "status"; - private static final String DATA_BLOCK_BREAKING = "block-breaking"; - private static final String DATA_BLOCK_INTERACTIONS = "block-interactions"; - private static final String DATA_BLOCK_PLACEMENT = "block-placement"; - private static final String DATA_BUILDERS_ENABLED = "builders-enabled"; - private static final String DATA_EXPLOSIONS = "explosions"; - private static final String DATA_MOB_AI = "mob-ai"; - private static final String DATA_PHYSICS = "physics"; - private static final String DATA_PINNED = "pinned"; - private static final String DATA_VISIBILITY = "visibility"; - private static final String DATA_PRIVATE = "private"; - private static final String DATA_TIME_SINCE_BACKUP = "time-since-backup"; - private static final String DATA_LAST_LOADED = "last-loaded"; - private static final String DATA_LAST_UNLOADED = "last-unloaded"; - private static final String DATA_LAST_EDITED = "last-edited"; + // The data keys come straight from WorldDataKey (see dataPath); only the pre-4.0 private boolean has no key. + private static final String LEGACY_PRIVATE = "private"; private final WorldContext context; private final PlayerLookupService playerLookup; @@ -158,30 +140,31 @@ public BuildWorldImpl deserialize(String key, ConfigurationSection section) { private WorldDataImpl parseWorldData(String worldName, ConfigurationSection section) { return new WorldDataBuilder(worldName) .withCustomSpawn(parseCustomSpawn(section)) - .withPermission(section.getString(DATA + "." + DATA_PERMISSION, "-")) - .withProject(section.getString(DATA + "." + DATA_PROJECT, "-")) + .withPermission(section.getString(dataPath(WorldDataKey.PERMISSION), "-")) + .withProject(section.getString(dataPath(WorldDataKey.PROJECT), "-")) .withDifficulty(parseDifficulty(section, worldName)) .withMaterial(parseMaterial(section, worldName)) - .withIconSkullTexture(section.getString(DATA + "." + DATA_ICON_SKULL_TEXTURE, "")) + .withIconSkullTexture(section.getString(dataPath(WorldDataKey.ICON_SKULL_TEXTURE), "")) .withStatus(parseStatus(section, worldName)) .withBlockBreaking( - section.getBoolean(DATA + "." + DATA_BLOCK_BREAKING, WorldDataImpl.DEFAULT_BLOCK_BREAKING)) + section.getBoolean(dataPath(WorldDataKey.BLOCK_BREAKING), WorldDataImpl.DEFAULT_BLOCK_BREAKING)) .withBlockInteractions(section.getBoolean( - DATA + "." + DATA_BLOCK_INTERACTIONS, WorldDataImpl.DEFAULT_BLOCK_INTERACTIONS)) - .withBlockPlacement( - section.getBoolean(DATA + "." + DATA_BLOCK_PLACEMENT, WorldDataImpl.DEFAULT_BLOCK_PLACEMENT)) - .withBuildersEnabled( - section.getBoolean(DATA + "." + DATA_BUILDERS_ENABLED, WorldDataImpl.DEFAULT_BUILDERS_ENABLED)) - .withExplosions(section.getBoolean(DATA + "." + DATA_EXPLOSIONS, WorldDataImpl.DEFAULT_EXPLOSIONS)) - .withMobAi(section.getBoolean(DATA + "." + DATA_MOB_AI, WorldDataImpl.DEFAULT_MOB_AI)) - .withPhysics(section.getBoolean(DATA + "." + DATA_PHYSICS, WorldDataImpl.DEFAULT_PHYSICS)) - .withPinned(section.getBoolean(DATA + "." + DATA_PINNED, WorldDataImpl.DEFAULT_PINNED)) + dataPath(WorldDataKey.BLOCK_INTERACTIONS), WorldDataImpl.DEFAULT_BLOCK_INTERACTIONS)) + .withBlockPlacement(section.getBoolean( + dataPath(WorldDataKey.BLOCK_PLACEMENT), WorldDataImpl.DEFAULT_BLOCK_PLACEMENT)) + .withBuildersEnabled(section.getBoolean( + dataPath(WorldDataKey.BUILDERS_ENABLED), WorldDataImpl.DEFAULT_BUILDERS_ENABLED)) + .withExplosions(section.getBoolean(dataPath(WorldDataKey.EXPLOSIONS), WorldDataImpl.DEFAULT_EXPLOSIONS)) + .withMobAi(section.getBoolean(dataPath(WorldDataKey.MOB_AI), WorldDataImpl.DEFAULT_MOB_AI)) + .withPhysics(section.getBoolean(dataPath(WorldDataKey.PHYSICS), WorldDataImpl.DEFAULT_PHYSICS)) + .withPinned(section.getBoolean(dataPath(WorldDataKey.PINNED), WorldDataImpl.DEFAULT_PINNED)) .withVisibility(parseVisibility(section)) - .withTimeSinceBackup( - section.getInt(DATA + "." + DATA_TIME_SINCE_BACKUP, WorldDataImpl.DEFAULT_TIME_SINCE_BACKUP)) - .withLastLoaded(section.getLong(DATA + "." + DATA_LAST_LOADED, WorldDataImpl.DEFAULT_TIMESTAMP)) - .withLastUnloaded(section.getLong(DATA + "." + DATA_LAST_UNLOADED, WorldDataImpl.DEFAULT_TIMESTAMP)) - .withLastEdited(section.getLong(DATA + "." + DATA_LAST_EDITED, WorldDataImpl.DEFAULT_TIMESTAMP)) + .withTimeSinceBackup(section.getInt( + dataPath(WorldDataKey.TIME_SINCE_BACKUP), WorldDataImpl.DEFAULT_TIME_SINCE_BACKUP)) + .withLastLoaded(section.getLong(dataPath(WorldDataKey.LAST_LOADED), WorldDataImpl.DEFAULT_TIMESTAMP)) + .withLastUnloaded( + section.getLong(dataPath(WorldDataKey.LAST_UNLOADED), WorldDataImpl.DEFAULT_TIMESTAMP)) + .withLastEdited(section.getLong(dataPath(WorldDataKey.LAST_EDITED), WorldDataImpl.DEFAULT_TIMESTAMP)) .withPermissionOverrideEnabled( () -> context.configService().current().folder().overridePermissions()) .withProjectOverrideEnabled( @@ -189,13 +172,21 @@ private WorldDataImpl parseWorldData(String worldName, ConfigurationSection sect .build(); } + /** + * {@return the nested {@code data.} path for a key} The data keys are owned by {@link WorldDataKey}, so the + * codec never duplicates the on-disk strings. + */ + private static String dataPath(WorldDataKey key) { + return DATA + "." + key.id(); + } + /** * Reads a world's custom spawn. It is serialized under {@code data.spawn} (its property key), but pre-property-map * files stored it at the top-level {@code spawn} key, so that location is the fallback. */ private String parseCustomSpawn(ConfigurationSection section) { - String dataSpawn = section.getString(DATA + "." + DATA_SPAWN); - return dataSpawn != null ? dataSpawn : section.getString(DATA_SPAWN, ""); + String dataSpawn = section.getString(dataPath(WorldDataKey.CUSTOM_SPAWN)); + return dataSpawn != null ? dataSpawn : section.getString(WorldDataKey.CUSTOM_SPAWN.id(), ""); } private BuildWorldType parseType(String worldName, ConfigurationSection section) { @@ -217,7 +208,7 @@ private BuildWorldType parseType(String worldName, ConfigurationSection section) * unknown. Like the other enums, an unparseable difficulty must not abort the world's load. */ private Difficulty parseDifficulty(ConfigurationSection section, String worldName) { - String raw = section.getString(DATA + "." + DATA_DIFFICULTY, Difficulty.PEACEFUL.name()); + String raw = section.getString(dataPath(WorldDataKey.DIFFICULTY), Difficulty.PEACEFUL.name()); try { return Difficulty.valueOf(raw.toUpperCase(Locale.ROOT)); } catch (IllegalArgumentException e) { @@ -233,7 +224,7 @@ private Difficulty parseDifficulty(ConfigurationSection section, String worldNam */ private BuildWorldStatus parseStatus(ConfigurationSection section, String worldName) { WorldStatusRegistry registry = context.statusRegistry(); - String raw = section.getString(DATA + "." + DATA_STATUS); + String raw = section.getString(dataPath(WorldDataKey.STATUS)); if (raw == null) { return registry.getDefaultStatus(); } @@ -251,7 +242,7 @@ private BuildWorldStatus parseStatus(ConfigurationSection section, String worldN * {@code private} boolean when the new key is absent. */ private Visibility parseVisibility(ConfigurationSection section) { - String raw = section.getString(DATA + "." + DATA_VISIBILITY); + String raw = section.getString(dataPath(WorldDataKey.VISIBILITY)); if (raw != null) { try { return Visibility.valueOf(raw.toUpperCase(Locale.ROOT)); @@ -259,11 +250,11 @@ private Visibility parseVisibility(ConfigurationSection section) { // Fall through to the legacy private flag. } } - return Visibility.matchVisibility(section.getBoolean(DATA + "." + DATA_PRIVATE)); + return Visibility.matchVisibility(section.getBoolean(DATA + "." + LEGACY_PRIVATE)); } private XMaterial parseMaterial(ConfigurationSection section, String worldName) { - String itemString = section.getString(DATA + "." + DATA_MATERIAL); + String itemString = section.getString(dataPath(WorldDataKey.MATERIAL)); if (itemString == null) { itemString = XMaterial.BEDROCK.name(); context.logger().warning("Could not find material for \"" + worldName + "\". Defaulting to BEDROCK."); From fc2ad559ad5851c6c375f2b401c53f3c1309b1a4 Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 19:56:26 +0200 Subject: [PATCH 09/13] refactor: make TaskScheduler the single scheduling seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TaskScheduler was a stateless wrapper instantiated six separate times, alongside raw Bukkit.getScheduler() calls, ad-hoc mainThreadExecutor lambdas and the common ForkJoinPool — four ways to hop threads. Give it the one bounded, daemon-threaded background executor plus mainThread()/background() Executor accessors so CompletableFuture stages hop threads through the same seam, and have Services own the single instance (consolidating the six call sites). Shut the pool down once on disable, after the final saves. Adoption of background()/mainThread() at the call sites follows in subsequent commits. --- .../buildsystem/BuildSystemPlugin.java | 3 ++ .../de/eintosti/buildsystem/Services.java | 16 +++++-- .../buildsystem/command/WorldsCommand.java | 2 +- .../listener/ListenerRegistrar.java | 2 +- .../de/eintosti/buildsystem/menu/Menus.java | 2 +- .../buildsystem/util/TaskScheduler.java | 47 +++++++++++++++++-- 6 files changed, 63 insertions(+), 9 deletions(-) diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/BuildSystemPlugin.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/BuildSystemPlugin.java index 3ac6eb68..e319a8af 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/BuildSystemPlugin.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/BuildSystemPlugin.java @@ -128,6 +128,9 @@ public void onDisable() { this.configSaveTask.cancel(); } + // Shut the shared background pool down only after the final saves above have completed. + services.scheduler().shutdown(); + this.integrations.deactivate(); getServer().getServicesManager().unregister(BuildSystem.class, api); diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java index 6d48dfd9..e7f83477 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java @@ -51,6 +51,7 @@ public final class Services { private final BuildSystemPlugin plugin; + private final TaskScheduler taskScheduler; private @Nullable ConfigService configService; private @Nullable Messages messages; @@ -76,6 +77,15 @@ public final class Services { Services(BuildSystemPlugin plugin) { this.plugin = plugin; + this.taskScheduler = new TaskScheduler(plugin); + } + + /** + * The plugin's single {@link TaskScheduler}, owning the shared background executor. Available for the whole plugin + * lifetime; {@link TaskScheduler#shutdown() shut down} on disable. + */ + public TaskScheduler scheduler() { + return taskScheduler; } /** @@ -122,11 +132,11 @@ void initClasses() { navigatorItems(), player(), messages(), - new TaskScheduler(plugin), + taskScheduler, new NamespacedKey(plugin, "owner"), new NamespacedKey(plugin, "category")); this.menus = new Menus(plugin, this); - this.prompts = new Prompts(messages(), config(), new TaskScheduler(plugin)); + this.prompts = new Prompts(messages(), config(), taskScheduler); // Load persisted worlds/folders last: world entities pull collaborators from a WorldContext that bundles // services created above (e.g. MenuItems, SpawnService), so the whole service graph must exist before loading. @@ -231,7 +241,7 @@ public WorldContext worldContext() { spawn(), worldStatusRegistry(), customizableIcons(), - new TaskScheduler(plugin), + taskScheduler, plugin.getLogger()); } return worldContext; diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/command/WorldsCommand.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/command/WorldsCommand.java index 25435b2e..49d08835 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/command/WorldsCommand.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/command/WorldsCommand.java @@ -62,7 +62,7 @@ public WorldsCommand(BuildSystemPlugin plugin, Services services) { BackupServiceImpl backupService = services.backup(); Logger logger = plugin.getLogger(); File dataFolder = plugin.getDataFolder(); - TaskScheduler scheduler = new TaskScheduler(plugin); + TaskScheduler scheduler = services.scheduler(); this.dispatcher = new SubCommandDispatcher( messages, diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/listener/ListenerRegistrar.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/listener/ListenerRegistrar.java index 57148de0..ad5c055d 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/listener/ListenerRegistrar.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/listener/ListenerRegistrar.java @@ -76,7 +76,7 @@ public void registerAll() { NavigatorEditorService navigatorEditorService = services.navigatorEditor(); NoClipService noClipService = services.noClip(); SpawnService spawnService = services.spawn(); - TaskScheduler scheduler = new TaskScheduler(plugin); + TaskScheduler scheduler = services.scheduler(); PlayerLookupService playerLookupService = services.playerLookup(); UpdateChecker updateChecker = plugin.getUpdateChecker(); diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/menu/Menus.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/menu/Menus.java index 64793881..c1dfea23 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/menu/Menus.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/menu/Menus.java @@ -82,7 +82,7 @@ public final class Menus { public Menus(BuildSystemPlugin plugin, Services services) { this.plugin = plugin; this.services = services; - this.scheduler = new TaskScheduler(plugin); + this.scheduler = services.scheduler(); this.builderNameKey = new NamespacedKey(plugin, "builder_name"); } diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/TaskScheduler.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/TaskScheduler.java index 5932a7db..e13a3008 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/TaskScheduler.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/util/TaskScheduler.java @@ -17,23 +17,41 @@ */ package de.eintosti.buildsystem.util; +import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicInteger; import org.bukkit.Bukkit; import org.bukkit.plugin.Plugin; import org.bukkit.scheduler.BukkitTask; import org.jspecify.annotations.NullMarked; /** - * Thin facade over the Bukkit scheduler that owns the {@link Plugin} handle, so collaborators can schedule work by - * depending on this single injectable instead of the whole {@code BuildSystemPlugin} (and its service-locator surface). - * Confines the {@code (plugin, ...)} scheduling boilerplate to one place. + * The plugin's single scheduling seam. Wraps the Bukkit scheduler (owning the {@link Plugin} handle so collaborators + * depend on this one injectable instead of the whole {@code BuildSystemPlugin}) and owns the bounded executor that backs + * all off-main work. Exposing {@link #mainThread()} and {@link #background()} as {@link Executor}s lets + * {@link java.util.concurrent.CompletableFuture} stages hop threads through the same seam, so there is no longer a mix of + * raw {@code Bukkit.getScheduler()} calls, ad-hoc {@code mainThreadExecutor} lambdas and the common ForkJoinPool. + * + *

The background pool is bounded and daemon-threaded; nothing running on it blocks on another background task (chains + * use non-blocking {@code CompletableFuture} composition), so a single shared pool cannot self-deadlock. Call + * {@link #shutdown()} once on plugin disable, after the final saves have completed. */ @NullMarked public final class TaskScheduler { private final Plugin plugin; + private final ExecutorService background; public TaskScheduler(Plugin plugin) { this.plugin = plugin; + int threads = Math.max(4, Runtime.getRuntime().availableProcessors()); + AtomicInteger counter = new AtomicInteger(); + this.background = Executors.newFixedThreadPool(threads, runnable -> { + Thread thread = new Thread(runnable, "BuildSystem-Worker-" + counter.incrementAndGet()); + thread.setDaemon(true); + return thread; + }); } public BukkitTask run(Runnable task) { @@ -55,4 +73,27 @@ public BukkitTask runAsync(Runnable task) { public BukkitTask runTimerAsync(Runnable task, long delayTicks, long periodTicks) { return Bukkit.getScheduler().runTaskTimerAsynchronously(plugin, task, delayTicks, periodTicks); } + + /** + * {@return an {@link Executor} that runs each task on the server main thread} For marshalling a + * {@link java.util.concurrent.CompletableFuture} stage back onto the main thread, e.g. + * {@code future.thenAcceptAsync(consumer, scheduler.mainThread())}. + */ + public Executor mainThread() { + return this::run; + } + + /** + * {@return the shared bounded executor for off-main work} Used as the explicit executor for + * {@code CompletableFuture.supplyAsync/runAsync} so storage I/O and backups share one observable, bounded pool + * instead of the unbounded common ForkJoinPool. + */ + public Executor background() { + return background; + } + + /** Shuts the background executor down; call once on plugin disable, after the final saves have completed. */ + public void shutdown() { + background.shutdown(); + } } From f51f3e2db59abfb9c3685383afce54d92a7c767e Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 20:04:02 +0200 Subject: [PATCH 10/13] refactor: run storage I/O on the shared background executor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The YAML stores ran their save/load/delete via CompletableFuture.runAsync/ supplyAsync with no executor, i.e. on the unbounded common ForkJoinPool. Route them through TaskScheduler.background() instead so all disk I/O shares one bounded, observable pool. The world/folder stores already hold Services; the player store now takes the scheduler via PlayerServiceImpl. Network lookups (PlayerLookupService) deliberately stay on Bukkit's growable async pool — mixing slow network calls with disk I/O on a bounded pool would risk head-of-line blocking. Test mocks provide a real scheduler. --- .../de/eintosti/buildsystem/Services.java | 2 +- .../buildsystem/player/PlayerServiceImpl.java | 8 +- .../storage/yaml/YamlFolderStorage.java | 92 ++++++++++--------- .../storage/yaml/YamlPlayerStorage.java | 47 ++++++---- .../storage/yaml/YamlWorldStorage.java | 56 ++++++----- .../yaml/YamlPlayerStorageRoundTripTest.java | 13 ++- .../yaml/YamlWorldStorageRoundTripTest.java | 9 +- .../eintosti/buildsystem/test/TestData.java | 1 + 8 files changed, 136 insertions(+), 92 deletions(-) diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java index e7f83477..a089e8e0 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java @@ -117,7 +117,7 @@ void initClasses() { this.customBlockManager = new CustomBlockManager(plugin, this::world); this.playerLookupService = new PlayerLookupService(plugin); - (this.playerService = new PlayerServiceImpl(plugin, config(), this::world)).init(); + (this.playerService = new PlayerServiceImpl(plugin, config(), this::world, taskScheduler)).init(); this.navigatorEditorService = new NavigatorEditorService(); this.noClipService = new NoClipService(plugin); this.worldService = new WorldServiceImpl(plugin, this); diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/player/PlayerServiceImpl.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/player/PlayerServiceImpl.java index 1706f67e..b90750bb 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/player/PlayerServiceImpl.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/player/PlayerServiceImpl.java @@ -25,6 +25,7 @@ import de.eintosti.buildsystem.storage.PlayerStorageImpl; import de.eintosti.buildsystem.storage.WorldStorageImpl; import de.eintosti.buildsystem.storage.yaml.YamlPlayerStorage; +import de.eintosti.buildsystem.util.TaskScheduler; import de.eintosti.buildsystem.world.WorldServiceImpl; import java.util.Collections; import java.util.HashSet; @@ -48,11 +49,14 @@ public class PlayerServiceImpl implements PlayerService { private final Set buildModePlayers; public PlayerServiceImpl( - BuildSystemPlugin plugin, ConfigService configService, Supplier worldService) { + BuildSystemPlugin plugin, + ConfigService configService, + Supplier worldService, + TaskScheduler scheduler) { this.plugin = plugin; this.configService = configService; this.worldService = worldService; - this.playerStorage = new YamlPlayerStorage(plugin); + this.playerStorage = new YamlPlayerStorage(plugin, scheduler); this.maxWorldsResolver = new MaxWorldsResolver(plugin.getLogger()); this.buildModePlayers = new HashSet<>(); } diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlFolderStorage.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlFolderStorage.java index 75240e6f..abe37747 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlFolderStorage.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlFolderStorage.java @@ -34,6 +34,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Executor; import java.util.logging.Level; import org.bukkit.configuration.ConfigurationSection; import org.bukkit.configuration.file.FileConfiguration; @@ -50,6 +51,7 @@ public class YamlFolderStorage extends FolderStorageImpl { private final Services services; private final YamlStore store; private final FileConfiguration config; + private final Executor background; private @Nullable FolderCodec codec; public YamlFolderStorage(BuildSystemPlugin plugin, WorldStorage worldStorage, Services services) { @@ -57,6 +59,7 @@ public YamlFolderStorage(BuildSystemPlugin plugin, WorldStorage worldStorage, Se this.services = services; this.store = new YamlStore(plugin.getDataFolder(), "folders.yml", plugin.getLogger()); this.config = store.config(); + this.background = services.scheduler().background(); } /** @@ -81,10 +84,12 @@ public CompletableFuture save(Folder folder) { // Serialize on the calling (main) thread; the async block only writes the captured map to disk. String folderKey = codec().key(folder); Map serialized = codec().serialize(folder); - return CompletableFuture.runAsync(() -> store.atomicSave(() -> { - config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); - config.set(FOLDERS_KEY + "." + folderKey, serialized); - })); + return CompletableFuture.runAsync( + () -> store.atomicSave(() -> { + config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); + config.set(FOLDERS_KEY + "." + folderKey, serialized); + }), + background); } @Override @@ -93,49 +98,54 @@ public CompletableFuture save(Collection folders) { for (Folder folder : folders) { serialized.put(codec().key(folder), codec().serialize(folder)); } - return CompletableFuture.runAsync(() -> store.atomicSave(() -> { - config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); - serialized.forEach((folderKey, value) -> config.set(FOLDERS_KEY + "." + folderKey, value)); - })); + return CompletableFuture.runAsync( + () -> store.atomicSave(() -> { + config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); + serialized.forEach((folderKey, value) -> config.set(FOLDERS_KEY + "." + folderKey, value)); + }), + background); } @Override @Contract("-> new") public CompletableFuture> load() { - return CompletableFuture.supplyAsync(() -> store.locked(() -> { - Set folderKeys = loadFolderKeys(); - - // First pass: load each folder by its key (UUID) without its parent reference. - Map loadedByKey = new HashMap<>(); - for (String folderKey : folderKeys) { - ConfigurationSection section = config.getConfigurationSection(FOLDERS_KEY + "." + folderKey); - if (section == null) { - continue; - } - try { - loadedByKey.put(folderKey, codec().deserialize(folderKey, section)); - } catch (Exception e) { - logger.log(Level.WARNING, "Skipping folder \"" + folderKey + "\": could not be loaded", e); - } - } - - // Second pass: link parents (referenced by UUID) now that every folder exists. - for (Map.Entry entry : loadedByKey.entrySet()) { - ConfigurationSection section = config.getConfigurationSection(FOLDERS_KEY + "." + entry.getKey()); - if (section == null) { - continue; - } - String parentKey = codec().parentReference(section); - if (parentKey != null) { - Folder parent = loadedByKey.get(parentKey); - if (parent != null) { - entry.getValue().setParent(parent); + return CompletableFuture.supplyAsync( + () -> store.locked(() -> { + Set folderKeys = loadFolderKeys(); + + // First pass: load each folder by its key (UUID) without its parent reference. + Map loadedByKey = new HashMap<>(); + for (String folderKey : folderKeys) { + ConfigurationSection section = config.getConfigurationSection(FOLDERS_KEY + "." + folderKey); + if (section == null) { + continue; + } + try { + loadedByKey.put(folderKey, codec().deserialize(folderKey, section)); + } catch (Exception e) { + logger.log(Level.WARNING, "Skipping folder \"" + folderKey + "\": could not be loaded", e); + } + } + + // Second pass: link parents (referenced by UUID) now that every folder exists. + for (Map.Entry entry : loadedByKey.entrySet()) { + ConfigurationSection section = + config.getConfigurationSection(FOLDERS_KEY + "." + entry.getKey()); + if (section == null) { + continue; + } + String parentKey = codec().parentReference(section); + if (parentKey != null) { + Folder parent = loadedByKey.get(parentKey); + if (parent != null) { + entry.getValue().setParent(parent); + } + } } - } - } - return new ArrayList<>(loadedByKey.values()); - })); + return new ArrayList<>(loadedByKey.values()); + }), + background); } private Set loadFolderKeys() { @@ -174,6 +184,6 @@ public CompletableFuture delete(Folder folder) { @Override public CompletableFuture delete(String folderKey) { return CompletableFuture.runAsync( - () -> store.atomicSave(() -> config.set(FOLDERS_KEY + "." + folderKey, null))); + () -> store.atomicSave(() -> config.set(FOLDERS_KEY + "." + folderKey, null)), background); } } diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorage.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorage.java index dc6ec521..bd52a888 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorage.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorage.java @@ -21,12 +21,14 @@ import de.eintosti.buildsystem.api.player.BuildPlayer; import de.eintosti.buildsystem.storage.PlayerStorageImpl; import de.eintosti.buildsystem.storage.codec.PlayerCodec; +import de.eintosti.buildsystem.util.TaskScheduler; import java.util.ArrayList; import java.util.Collection; import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Executor; import java.util.logging.Level; import org.bukkit.configuration.ConfigurationSection; import org.bukkit.configuration.file.FileConfiguration; @@ -40,12 +42,14 @@ public class YamlPlayerStorage extends PlayerStorageImpl { private final YamlStore store; private final FileConfiguration config; private final PlayerCodec codec; + private final Executor background; - public YamlPlayerStorage(BuildSystemPlugin plugin) { + public YamlPlayerStorage(BuildSystemPlugin plugin, TaskScheduler scheduler) { super(plugin.getLogger()); this.store = new YamlStore(plugin.getDataFolder(), "players.yml", plugin.getLogger()); this.config = store.config(); this.codec = new PlayerCodec(plugin.getLogger()); + this.background = scheduler.background(); } @Override @@ -54,7 +58,7 @@ public CompletableFuture save(BuildPlayer buildPlayer) { String playerKey = codec.key(buildPlayer); Map serialized = codec.serialize(buildPlayer); return CompletableFuture.runAsync( - () -> store.atomicSave(() -> config.set(PLAYERS_KEY + "." + playerKey, serialized))); + () -> store.atomicSave(() -> config.set(PLAYERS_KEY + "." + playerKey, serialized)), background); } @Override @@ -63,27 +67,32 @@ public CompletableFuture save(Collection players) { for (BuildPlayer player : players) { serialized.put(codec.key(player), codec.serialize(player)); } - return CompletableFuture.runAsync(() -> store.atomicSave( - () -> serialized.forEach((playerKey, value) -> config.set(PLAYERS_KEY + "." + playerKey, value)))); + return CompletableFuture.runAsync( + () -> store.atomicSave(() -> + serialized.forEach((playerKey, value) -> config.set(PLAYERS_KEY + "." + playerKey, value))), + background); } @Override public CompletableFuture> load() { - return CompletableFuture.supplyAsync(() -> store.locked(() -> { - Collection players = new ArrayList<>(); - for (String playerUuid : loadPlayerKeys()) { - try { - ConfigurationSection section = config.getConfigurationSection(PLAYERS_KEY + "." + playerUuid); - if (section == null) { - continue; + return CompletableFuture.supplyAsync( + () -> store.locked(() -> { + Collection players = new ArrayList<>(); + for (String playerUuid : loadPlayerKeys()) { + try { + ConfigurationSection section = + config.getConfigurationSection(PLAYERS_KEY + "." + playerUuid); + if (section == null) { + continue; + } + players.add(codec.deserialize(playerUuid, section)); + } catch (Exception e) { + logger.log(Level.WARNING, "Skipping player \"" + playerUuid + "\": could not be loaded", e); + } } - players.add(codec.deserialize(playerUuid, section)); - } catch (Exception e) { - logger.log(Level.WARNING, "Skipping player \"" + playerUuid + "\": could not be loaded", e); - } - } - return players; - })); + return players; + }), + background); } private Set loadPlayerKeys() { @@ -107,6 +116,6 @@ public CompletableFuture delete(BuildPlayer buildPlayer) { @Override public CompletableFuture delete(String playerKey) { return CompletableFuture.runAsync( - () -> store.atomicSave(() -> config.set(PLAYERS_KEY + "." + playerKey, null))); + () -> store.atomicSave(() -> config.set(PLAYERS_KEY + "." + playerKey, null)), background); } } diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorage.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorage.java index 748fcb38..1a31c42f 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorage.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorage.java @@ -29,6 +29,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Executor; import java.util.logging.Level; import org.bukkit.configuration.ConfigurationSection; import org.bukkit.configuration.file.FileConfiguration; @@ -44,6 +45,7 @@ public class YamlWorldStorage extends WorldStorageImpl { private final Services services; private final YamlStore store; private final FileConfiguration config; + private final Executor background; private @Nullable WorldCodec codec; public YamlWorldStorage(BuildSystemPlugin plugin, Services services) { @@ -51,6 +53,7 @@ public YamlWorldStorage(BuildSystemPlugin plugin, Services services) { this.services = services; this.store = new YamlStore(plugin.getDataFolder(), "worlds.yml", plugin.getLogger()); this.config = store.config(); + this.background = services.scheduler().background(); } /** @@ -71,10 +74,12 @@ public CompletableFuture save(BuildWorld buildWorld) { // already-captured map to disk, never read live domain state off the main thread. String worldKey = codec().key(buildWorld); Map serialized = codec().serialize(buildWorld); - return CompletableFuture.runAsync(() -> store.atomicSave(() -> { - config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); - config.set(WORLDS_KEY + "." + worldKey, serialized); - })); + return CompletableFuture.runAsync( + () -> store.atomicSave(() -> { + config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); + config.set(WORLDS_KEY + "." + worldKey, serialized); + }), + background); } @Override @@ -83,29 +88,33 @@ public CompletableFuture save(Collection buildWorlds) { for (BuildWorld buildWorld : buildWorlds) { serialized.put(codec().key(buildWorld), codec().serialize(buildWorld)); } - return CompletableFuture.runAsync(() -> store.atomicSave(() -> { - config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); - serialized.forEach((worldKey, value) -> config.set(WORLDS_KEY + "." + worldKey, value)); - })); + return CompletableFuture.runAsync( + () -> store.atomicSave(() -> { + config.set(StorageMigration.VERSION_KEY, StorageMigration.CURRENT_VERSION); + serialized.forEach((worldKey, value) -> config.set(WORLDS_KEY + "." + worldKey, value)); + }), + background); } @Override public CompletableFuture> load() { - return CompletableFuture.supplyAsync(() -> store.locked(() -> { - Collection worlds = new ArrayList<>(); - for (String worldKey : loadWorldKeys()) { - try { - ConfigurationSection section = config.getConfigurationSection(WORLDS_KEY + "." + worldKey); - if (section == null) { - continue; + return CompletableFuture.supplyAsync( + () -> store.locked(() -> { + Collection worlds = new ArrayList<>(); + for (String worldKey : loadWorldKeys()) { + try { + ConfigurationSection section = config.getConfigurationSection(WORLDS_KEY + "." + worldKey); + if (section == null) { + continue; + } + worlds.add(codec().deserialize(worldKey, section)); + } catch (Exception e) { + logger.log(Level.WARNING, "Skipping world \"" + worldKey + "\": could not be loaded", e); + } } - worlds.add(codec().deserialize(worldKey, section)); - } catch (Exception e) { - logger.log(Level.WARNING, "Skipping world \"" + worldKey + "\": could not be loaded", e); - } - } - return worlds; - })); + return worlds; + }), + background); } private Set loadWorldKeys() { @@ -143,6 +152,7 @@ public CompletableFuture delete(BuildWorld buildWorld) { @Override public CompletableFuture delete(String worldKey) { - return CompletableFuture.runAsync(() -> store.atomicSave(() -> config.set(WORLDS_KEY + "." + worldKey, null))); + return CompletableFuture.runAsync( + () -> store.atomicSave(() -> config.set(WORLDS_KEY + "." + worldKey, null)), background); } } diff --git a/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorageRoundTripTest.java b/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorageRoundTripTest.java index 3ac5f0d4..49829104 100644 --- a/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorageRoundTripTest.java +++ b/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlPlayerStorageRoundTripTest.java @@ -26,6 +26,7 @@ import de.eintosti.buildsystem.player.BuildPlayerImpl; import de.eintosti.buildsystem.player.LogoutLocation; import de.eintosti.buildsystem.player.settings.SettingsImpl; +import de.eintosti.buildsystem.util.TaskScheduler; import java.io.File; import java.util.Collection; import java.util.UUID; @@ -39,11 +40,17 @@ class YamlPlayerStorageRoundTripTest { File dataFolder; private BuildSystemPlugin plugin; + private TaskScheduler scheduler; @BeforeEach void setUp() { plugin = mock(BuildSystemPlugin.class, RETURNS_DEEP_STUBS); when(plugin.getDataFolder()).thenReturn(dataFolder); + scheduler = new TaskScheduler(plugin); + } + + private YamlPlayerStorage newStorage() { + return new YamlPlayerStorage(plugin, scheduler); } private BuildPlayerImpl samplePlayer(UUID uuid) { @@ -65,9 +72,9 @@ void saveAndLoad_roundTripsSettingsAndLogoutLocation() throws Exception { UUID uuid = UUID.randomUUID(); BuildPlayerImpl original = samplePlayer(uuid); - new YamlPlayerStorage(plugin).save(original).join(); + newStorage().save(original).join(); - Collection loaded = new YamlPlayerStorage(plugin).load().get(); + Collection loaded = newStorage().load().get(); assertEquals(1, loaded.size()); BuildPlayerImpl reloaded = BuildPlayerImpl.of(loaded.iterator().next()); @@ -87,7 +94,7 @@ void saveAndLoad_roundTripsSettingsAndLogoutLocation() throws Exception { @Test void load_missingFile_returnsEmptyCollection() throws Exception { - Collection loaded = new YamlPlayerStorage(plugin).load().get(); + Collection loaded = newStorage().load().get(); assertTrue(loaded.isEmpty()); } diff --git a/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorageRoundTripTest.java b/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorageRoundTripTest.java index fb4420b8..872b5d9d 100644 --- a/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorageRoundTripTest.java +++ b/buildsystem-core/src/test/java/de/eintosti/buildsystem/storage/yaml/YamlWorldStorageRoundTripTest.java @@ -29,6 +29,7 @@ import de.eintosti.buildsystem.api.world.data.Visibility; import de.eintosti.buildsystem.api.world.data.WorldDataKey; import de.eintosti.buildsystem.test.TestData; +import de.eintosti.buildsystem.util.TaskScheduler; import de.eintosti.buildsystem.world.BuildWorldImpl; import de.eintosti.buildsystem.world.WorldContext; import de.eintosti.buildsystem.world.data.WorldDataImpl; @@ -167,10 +168,12 @@ void load_emptyFile_returnsEmptyCollection() { } @Test - void construction_doesNotResolveServices() { - // Worlds are loaded during plugin enable, before services such as MenuItems/SpawnService exist. Constructing - // the storage must not resolve any service (the codec is built lazily on first load); otherwise startup throws. + void construction_doesNotResolveLateServices() { + // Worlds are loaded during plugin enable, before the late services the codec needs (MenuItems/SpawnService via + // the WorldContext, and PlayerLookupService) exist. Construction must not resolve those (the codec is built + // lazily on first load); only the always-available scheduler may be pulled. Services strict = mock(Services.class); + when(strict.scheduler()).thenReturn(new TaskScheduler(plugin)); when(strict.worldContext()).thenThrow(new IllegalStateException("service not initialized yet")); when(strict.playerLookup()).thenThrow(new IllegalStateException("service not initialized yet")); diff --git a/buildsystem-core/src/test/java/de/eintosti/buildsystem/test/TestData.java b/buildsystem-core/src/test/java/de/eintosti/buildsystem/test/TestData.java index 6813f133..b30b92ca 100644 --- a/buildsystem-core/src/test/java/de/eintosti/buildsystem/test/TestData.java +++ b/buildsystem-core/src/test/java/de/eintosti/buildsystem/test/TestData.java @@ -202,6 +202,7 @@ public static Services mockServices() { Prompts prompts = mock(Prompts.class); Services services = mock(Services.class); lenient().when(services.worldContext()).thenReturn(context); + lenient().when(services.scheduler()).thenReturn(context.scheduler()); lenient().when(services.config()).thenReturn(context.configService()); lenient().when(services.messages()).thenReturn(context.messages()); lenient().when(services.player()).thenReturn(context.playerService()); From a05544b61a93b035096a47a4e28f00d47c057723 Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 20:08:23 +0200 Subject: [PATCH 11/13] refactor: route remaining disk I/O through the scheduler seam SpawnService.save, the world-rename directory copy and the save-template copy ran on the common ForkJoinPool; the rename then hopped back to the main thread via a raw Bukkit.getScheduler().runTask. Route the disk work through TaskScheduler.background() and the rename's reconstruct step through thenRunAsync(..., scheduler.mainThread()), so all disk I/O shares the one bounded pool and the main-thread hop goes through the same seam. Backups keep their own bounded pool: they do network I/O (S3/SFTP uploads), and sharing the disk pool would let a slow upload starve world saves. --- .../de/eintosti/buildsystem/Services.java | 2 +- .../worlds/SaveTemplateSubCommand.java | 2 +- .../buildsystem/world/WorldServiceImpl.java | 9 +++- .../world/lifecycle/WorldRenamer.java | 45 ++++++++++--------- .../buildsystem/world/spawn/SpawnService.java | 8 +++- 5 files changed, 41 insertions(+), 25 deletions(-) diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java index a089e8e0..61f7e2df 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/Services.java @@ -123,7 +123,7 @@ void initClasses() { this.worldService = new WorldServiceImpl(plugin, this); this.backupService = new BackupServiceImpl(plugin, config(), messages(), world(), this::spawn); this.settingsService = new SettingsService(plugin, config(), messages(), player(), world()); - this.spawnService = new SpawnService(plugin, world()); + this.spawnService = new SpawnService(plugin, world(), taskScheduler); this.menuItems = new MenuItems(plugin, messages(), settings()); this.navigatorItems = new NavigatorItems(plugin, config(), messages()); this.navigatorService = new NavigatorService( diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/command/subcommand/worlds/SaveTemplateSubCommand.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/command/subcommand/worlds/SaveTemplateSubCommand.java index 8b980390..59a15ab9 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/command/subcommand/worlds/SaveTemplateSubCommand.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/command/subcommand/worlds/SaveTemplateSubCommand.java @@ -100,7 +100,7 @@ public void execute(Player player, String worldName, String[] args) { "worlds_savetemplate_started", Map.entry("%world%", buildWorld.getName()), Map.entry("%template%", templateName)); - CompletableFuture.runAsync(() -> FileUtils.copy(worldDir, templateDir)) + CompletableFuture.runAsync(() -> FileUtils.copy(worldDir, templateDir), scheduler.background()) .whenComplete((ignored, throwable) -> scheduler.run(() -> { if (throwable != null) { logger.log( diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java index 01a9d1cb..c28e8f0a 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java @@ -283,7 +283,14 @@ public CompletableFuture deleteWorld(BuildWorld buildWorld) { * @param newName The name the world should be renamed to */ public void renameWorld(Player player, BuildWorld buildWorld, String newName) { - new WorldRenamer(plugin, this, worldStorage, services.config(), services.messages(), services.spawn()) + new WorldRenamer( + plugin, + this, + worldStorage, + services.config(), + services.messages(), + services.spawn(), + services.scheduler()) .rename(player, buildWorld, newName); } diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/lifecycle/WorldRenamer.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/lifecycle/WorldRenamer.java index 80507cd0..ad1256c0 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/lifecycle/WorldRenamer.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/lifecycle/WorldRenamer.java @@ -26,6 +26,7 @@ import de.eintosti.buildsystem.storage.WorldStorageImpl; import de.eintosti.buildsystem.util.FileUtils; import de.eintosti.buildsystem.util.StringCleaner; +import de.eintosti.buildsystem.util.TaskScheduler; import de.eintosti.buildsystem.world.WorldServiceImpl; import de.eintosti.buildsystem.world.creation.BukkitWorldFactory; import de.eintosti.buildsystem.world.spawn.SpawnService; @@ -57,6 +58,7 @@ public class WorldRenamer { private final ConfigService configService; private final Messages messages; private final SpawnService spawnService; + private final TaskScheduler scheduler; public WorldRenamer( BuildSystemPlugin plugin, @@ -64,13 +66,15 @@ public WorldRenamer( WorldStorageImpl worldStorage, ConfigService configService, Messages messages, - SpawnService spawnService) { + SpawnService spawnService, + TaskScheduler scheduler) { this.plugin = plugin; this.worldService = worldService; this.worldStorage = worldStorage; this.configService = configService; this.messages = messages; this.spawnService = spawnService; + this.scheduler = scheduler; } public void rename(Player player, BuildWorld buildWorld, String newName) { @@ -124,25 +128,26 @@ private void prepareAndMove( File oldWorldFile = new File(Bukkit.getWorldContainer(), oldName); File newWorldFile = new File(Bukkit.getWorldContainer(), sanitizedNewName); - CompletableFuture.runAsync(() -> { - try { - FileUtils.copy(oldWorldFile, newWorldFile); - FileUtils.deleteDirectory(oldWorldFile); - } catch (Exception e) { - throw new RuntimeException("Failed to rename world directory", e); - } - }) - .thenRun(() -> Bukkit.getScheduler() - .runTask( - plugin, - () -> reconstruct( - player, - buildWorld, - oldName, - sanitizedNewName, - oldWorld, - oldSpawnLocation, - removedPlayers))); + CompletableFuture.runAsync( + () -> { + try { + FileUtils.copy(oldWorldFile, newWorldFile); + FileUtils.deleteDirectory(oldWorldFile); + } catch (Exception e) { + throw new RuntimeException("Failed to rename world directory", e); + } + }, + scheduler.background()) + .thenRunAsync( + () -> reconstruct( + player, + buildWorld, + oldName, + sanitizedNewName, + oldWorld, + oldSpawnLocation, + removedPlayers), + scheduler.mainThread()); } private void reconstruct( diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/spawn/SpawnService.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/spawn/SpawnService.java index 7ac34dbd..93924841 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/spawn/SpawnService.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/spawn/SpawnService.java @@ -22,9 +22,11 @@ import de.eintosti.buildsystem.api.storage.WorldStorage; import de.eintosti.buildsystem.api.world.BuildWorld; import de.eintosti.buildsystem.storage.yaml.YamlSpawnStorage; +import de.eintosti.buildsystem.util.TaskScheduler; import de.eintosti.buildsystem.world.WorldServiceImpl; import io.papermc.lib.PaperLib; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Executor; import org.bukkit.Bukkit; import org.bukkit.Location; import org.bukkit.World; @@ -39,14 +41,16 @@ public class SpawnService { private final BuildSystemPlugin plugin; private final WorldStorage worldStorage; private final YamlSpawnStorage spawnStorage; + private final Executor background; private @Nullable String spawnName; private @Nullable Location spawn; - public SpawnService(BuildSystemPlugin plugin, WorldServiceImpl worldService) { + public SpawnService(BuildSystemPlugin plugin, WorldServiceImpl worldService, TaskScheduler scheduler) { this.plugin = plugin; this.worldStorage = worldService.getWorldStorage(); this.spawnStorage = new YamlSpawnStorage(plugin); + this.background = scheduler.background(); load(); } @@ -98,7 +102,7 @@ public void remove() { } public CompletableFuture save() { - return CompletableFuture.runAsync(() -> spawnStorage.saveSpawn(spawn)); + return CompletableFuture.runAsync(() -> spawnStorage.saveSpawn(spawn), background); } private void load() { From fc50103dbb4230daf24c7e95da57cf40df10c8e9 Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 20:57:00 +0200 Subject: [PATCH 12/13] refactor: run world-deletion directory delete on the shared pool deleteWorld's directory delete was the one disk-I/O site still on the common ForkJoinPool (thenRunAsync with no executor) after the threading unification. Route it through the shared background pool, keeping the existing pattern of capturing the Bukkit scheduler on the main thread so the async stage never touches a Bukkit static from a pool thread. --- .../buildsystem/world/WorldServiceImpl.java | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java index c28e8f0a..c0dd4d8c 100644 --- a/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java +++ b/buildsystem-core/src/main/java/de/eintosti/buildsystem/world/WorldServiceImpl.java @@ -259,20 +259,27 @@ public CompletableFuture deleteWorld(BuildWorld buildWorld) { // Bukkit statics from a pool thread. BukkitScheduler scheduler = Bukkit.getScheduler(); - // Registry removal and metadata persistence are awaited before folder deletion - // so a crash mid-delete leaves an orphaned folder (re-importable) rather than - // an orphaned registry entry pointing at a deleted folder. - return unimportWorld(buildWorld, SaveBehavior.DISCARD).thenRunAsync(() -> { - try { - FileUtils.deleteDirectory(deleteFolder); - } catch (IOException e) { - throw new CompletionException(new WorldDeletionException( - "An unexpected error occurred during directory deletion for world: " + worldName, e)); - } - scheduler.runTask( - plugin, - () -> Bukkit.getServer().getPluginManager().callEvent(new BuildWorldPostDeleteEvent(buildWorld))); - }); + // Registry removal and metadata persistence are awaited before folder deletion so a crash mid-delete leaves an + // orphaned folder (re-importable) rather than an orphaned registry entry pointing at a deleted folder. The + // directory delete is disk I/O, so it runs on the shared background pool. + return unimportWorld(buildWorld, SaveBehavior.DISCARD) + .thenRunAsync( + () -> { + try { + FileUtils.deleteDirectory(deleteFolder); + } catch (IOException e) { + throw new CompletionException(new WorldDeletionException( + "An unexpected error occurred during directory deletion for world: " + + worldName, + e)); + } + scheduler.runTask( + plugin, + () -> Bukkit.getServer() + .getPluginManager() + .callEvent(new BuildWorldPostDeleteEvent(buildWorld))); + }, + services.scheduler().background()); } /** From ec540cbd84e185ccc3b04dd4dd6e60b3b9fcb96e Mon Sep 17 00:00:00 2001 From: Thomas Meaney Date: Sun, 21 Jun 2026 20:57:39 +0200 Subject: [PATCH 13/13] docs: changelog for the audit hardening fixes Record the user-facing fixes from this branch under 4.0.0: the difficulty load-fallback, the missing-key world-setting defaults, reported delete failures, reserved/degenerate world-name rejection, and the shared bounded I/O pool. --- CHANGELOG.md | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51cbfd69..a5d01e02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -126,13 +126,22 @@ config toggles. `config.yml` and `messages.yml` migrate automatically. - Placeholder substitution is literal, not regex-based. - Inventory clicks are matched against the menu inventory only; a click in the player's own inventory no longer triggers the menu button in the same slot. +- A world whose stored difficulty is unreadable no longer vanishes on load; it + falls back to Peaceful, like the other world enums. +- World settings absent from disk (block protections, and the last-edited/loaded/ + unloaded timestamps) now load with the same defaults a freshly created world + gets, instead of loading disabled / at epoch zero. +- Failed world-directory deletions are reported instead of silently leaving files + behind. ### Security - Path-traversal guards on template and world directory resolution. World names are also checked at the public `WorldService.newWorld(String)` and on deletion, so a name resolving outside the world container (e.g. `../../plugins/x`) is - rejected rather than creating or deleting a directory outside it (#481). + rejected rather than creating or deleting a directory outside it (#481). World + creation also rejects reserved/degenerate names — the `.`/`..` aliases and + Windows device names such as `CON`/`PRN`. ### Performance @@ -140,6 +149,8 @@ config toggles. `config.yml` and `messages.yml` migrate automatically. - NoClip check no longer allocates a `Location` per tick. - Reduced allocation/work in scoreboard rendering, color processing, and the template menu. +- World, folder and player file I/O share a single bounded background thread pool + instead of the unbounded common pool. ### Migration (server admins)