From 4a3bbae5027ffee2a224f1b4c0b10c56ee86ae4d Mon Sep 17 00:00:00 2001 From: Matt <4009945+MattBDev@users.noreply.github.com> Date: Wed, 15 Jul 2026 01:50:01 -0400 Subject: [PATCH 1/8] Fix broken double-checked locking in DiskStorageHistory stream getters The six lazy output-stream getters checked their backing field outside the lock without re-checking inside it, and the four NBT getters were not synchronized at all. None of the six fields were volatile. Two threads could therefore both pass the null check and each construct a FileOutputStream for the same history file, with the second truncating the file and rewriting its header over an edit that was already being recorded. Make the six stream fields volatile and give all six getters the standard double-checked-locking shape: check, synchronize, re-check, construct. getBlockOS() additionally now writes the header into a local before publishing the stream to the volatile field. Publishing first would let a thread on the unsynchronized fast path observe a non-null stream and write block data into it while the constructing thread was still writing the header, interleaving records ahead of the header on a stream that is not thread-safe. Adds DiskStorageHistoryConcurrencyTest covering concurrent getter access. Co-Authored-By: Claude Opus 4.8 --- worldedit-core/build.gradle.kts | 6 + .../core/history/DiskStorageHistory.java | 80 +++++++--- .../DiskStorageHistoryConcurrencyTest.java | 151 ++++++++++++++++++ 3 files changed, 213 insertions(+), 24 deletions(-) create mode 100644 worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java diff --git a/worldedit-core/build.gradle.kts b/worldedit-core/build.gradle.kts index dba8ca0823..3ecc84954a 100644 --- a/worldedit-core/build.gradle.kts +++ b/worldedit-core/build.gradle.kts @@ -62,6 +62,12 @@ dependencies { // Tests testRuntimeOnly(libs.log4j.core) testImplementation(libs.parallelgzip) + // lz4-java is compileOnly for the main sourceSet (expected to be shaded in by platform jars + // at runtime), but FaweStreamChangeSet#getCompressedOS() references LZ4 stream types even on + // code paths that aren't taken at COMPRESSION_LEVEL 0 - the JVM verifier still needs to + // resolve those types when the method is first invoked, so tests exercising that method (e.g. + // DiskStorageHistory's stream getters) need the real classes on the test runtime classpath. + testImplementation(libs.lz4Java) { isTransitive = false } } tasks.test { diff --git a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java index eb2e3da59e..0d18a11867 100644 --- a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java +++ b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java @@ -57,17 +57,17 @@ public class DiskStorageHistory extends FaweStreamChangeSet { * [contents]... * { short rel x, short rel z, unsigned byte y, short combinedFrom, short combinedTo } */ - private FaweOutputStream osBD; + private volatile FaweOutputStream osBD; // biome - private FaweOutputStream osBIO; + private volatile FaweOutputStream osBIO; // NBT From - private NBTOutputStream osNBTF; + private volatile NBTOutputStream osNBTF; // NBT To - private NBTOutputStream osNBTT; + private volatile NBTOutputStream osNBTT; // Entity Create From - private NBTOutputStream osENTCF; + private volatile NBTOutputStream osENTCF; // Entity Create To - private NBTOutputStream osENTCT; + private volatile NBTOutputStream osENTCT; private int index; @@ -304,10 +304,19 @@ public FaweOutputStream getBlockOS(int x, int y, int z) throws IOException { return osBD; } synchronized (this) { + if (osBD != null) { + return osBD; + } bdFile.getParentFile().mkdirs(); bdFile.createNewFile(); - osBD = getCompressedOS(new FileOutputStream(bdFile)); - writeHeader(osBD, x, y, z); + // Write the header before publishing to the volatile field: osBD is not thread-safe + // (see getCompressedOS()'s javadoc), so another thread's unsynchronized fast-path + // read (`if (osBD != null) return osBD;`) must never observe the stream until it is + // fully initialized, or it could start writing block data concurrently with the + // header write here and corrupt the file. + FaweOutputStream stream = getCompressedOS(new FileOutputStream(bdFile)); + writeHeader(stream, x, y, z); + osBD = stream; return osBD; } } @@ -318,6 +327,9 @@ public FaweOutputStream getBiomeOS() throws IOException { return osBIO; } synchronized (this) { + if (osBIO != null) { + return osBIO; + } bioFile.getParentFile().mkdirs(); bioFile.createNewFile(); osBIO = getCompressedOS(new FileOutputStream(bioFile)); @@ -330,10 +342,15 @@ public NBTOutputStream getEntityCreateOS() throws IOException { if (osENTCT != null) { return osENTCT; } - enttFile.getParentFile().mkdirs(); - enttFile.createNewFile(); - osENTCT = new NBTOutputStream(getCompressedOS(new FileOutputStream(enttFile))); - return osENTCT; + synchronized (this) { + if (osENTCT != null) { + return osENTCT; + } + enttFile.getParentFile().mkdirs(); + enttFile.createNewFile(); + osENTCT = new NBTOutputStream(getCompressedOS(new FileOutputStream(enttFile))); + return osENTCT; + } } @Override @@ -341,10 +358,15 @@ public NBTOutputStream getEntityRemoveOS() throws IOException { if (osENTCF != null) { return osENTCF; } - entfFile.getParentFile().mkdirs(); - entfFile.createNewFile(); - osENTCF = new NBTOutputStream(getCompressedOS(new FileOutputStream(entfFile))); - return osENTCF; + synchronized (this) { + if (osENTCF != null) { + return osENTCF; + } + entfFile.getParentFile().mkdirs(); + entfFile.createNewFile(); + osENTCF = new NBTOutputStream(getCompressedOS(new FileOutputStream(entfFile))); + return osENTCF; + } } @Override @@ -352,10 +374,15 @@ public NBTOutputStream getTileCreateOS() throws IOException { if (osNBTT != null) { return osNBTT; } - nbttFile.getParentFile().mkdirs(); - nbttFile.createNewFile(); - osNBTT = new NBTOutputStream(getCompressedOS(new FileOutputStream(nbttFile))); - return osNBTT; + synchronized (this) { + if (osNBTT != null) { + return osNBTT; + } + nbttFile.getParentFile().mkdirs(); + nbttFile.createNewFile(); + osNBTT = new NBTOutputStream(getCompressedOS(new FileOutputStream(nbttFile))); + return osNBTT; + } } @Override @@ -363,10 +390,15 @@ public NBTOutputStream getTileRemoveOS() throws IOException { if (osNBTF != null) { return osNBTF; } - nbtfFile.getParentFile().mkdirs(); - nbtfFile.createNewFile(); - osNBTF = new NBTOutputStream(getCompressedOS(new FileOutputStream(nbtfFile))); - return osNBTF; + synchronized (this) { + if (osNBTF != null) { + return osNBTF; + } + nbtfFile.getParentFile().mkdirs(); + nbtfFile.createNewFile(); + osNBTF = new NBTOutputStream(getCompressedOS(new FileOutputStream(nbtfFile))); + return osNBTF; + } } @Override diff --git a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java new file mode 100644 index 0000000000..1cbe0d450e --- /dev/null +++ b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java @@ -0,0 +1,151 @@ +package com.fastasyncworldedit.core.history; + +import com.fastasyncworldedit.core.configuration.Settings; +import com.sk89q.worldedit.world.World; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.File; +import java.util.Collections; +import java.util.IdentityHashMap; +import java.util.Set; +import java.util.UUID; +import java.util.concurrent.BrokenBarrierException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Regression test for the double-checked locking bug in {@link DiskStorageHistory}'s lazy + * stream getters: racing threads must always observe the same, single, lazily-constructed + * stream instance. + */ +class DiskStorageHistoryConcurrencyTest { + + private static final int THREADS = 16; + private static final int ITERATIONS = 20; + + @TempDir + File tempDir; + + private World world; + private int originalCompressionLevel; + + @BeforeEach + void setUp() { + world = mock(World.class); + when(world.getMinY()).thenReturn(-64); + when(world.getMaxY()).thenReturn(319); + when(world.getName()).thenReturn("concurrency-test-world"); + + // The LZ4/Zstd compression backends used at COMPRESSION_LEVEL > 0 are only + // `compileOnly` dependencies of worldedit-core (they're expected to be shaded in at + // runtime by the platform jars), so they aren't on the unit test classpath. Force + // uncompressed streams so getCompressedOS() doesn't throw NoClassDefFoundError - this + // test is only concerned with the identity of the lazily-constructed stream, not with + // compression behavior. + originalCompressionLevel = Settings.settings().HISTORY.COMPRESSION_LEVEL; + Settings.settings().HISTORY.COMPRESSION_LEVEL = 0; + } + + @AfterEach + void tearDown() { + Settings.settings().HISTORY.COMPRESSION_LEVEL = originalCompressionLevel; + } + + @Test + void getBlockOSReturnsSingleInstanceUnderConcurrentAccess() throws Exception { + for (int iteration = 0; iteration < ITERATIONS; iteration++) { + File folder = new File(tempDir, "block-" + iteration); + DiskStorageHistory history = new DiskStorageHistory(folder, world, UUID.randomUUID(), iteration); + + Set instances = Collections.newSetFromMap(new IdentityHashMap<>()); + runConcurrently(THREADS, () -> { + try { + return history.getBlockOS(0, 0, 0); + } catch (Exception e) { + throw new RuntimeException(e); + } + }, instances); + + assertEquals( + 1, + instances.size(), + "getBlockOS() must return exactly one distinct stream instance across racing threads (iteration " + + iteration + ")" + ); + history.close(); + } + } + + @Test + void getTileCreateOSReturnsSingleInstanceUnderConcurrentAccess() throws Exception { + for (int iteration = 0; iteration < ITERATIONS; iteration++) { + File folder = new File(tempDir, "tile-" + iteration); + DiskStorageHistory history = new DiskStorageHistory(folder, world, UUID.randomUUID(), iteration); + + Set instances = Collections.newSetFromMap(new IdentityHashMap<>()); + runConcurrently(THREADS, () -> { + try { + return history.getTileCreateOS(); + } catch (Exception e) { + throw new RuntimeException(e); + } + }, instances); + + assertEquals( + 1, + instances.size(), + "getTileCreateOS() must return exactly one distinct stream instance across racing threads (iteration " + + iteration + ")" + ); + history.close(); + } + } + + /** + * Spins up {@code threadCount} threads, gates them behind a {@link CyclicBarrier} so they all + * invoke {@code action} at roughly the same instant, and records the identity of every + * returned object into {@code resultsOut}. + */ + private void runConcurrently(int threadCount, java.util.function.Supplier action, Set resultsOut) + throws InterruptedException { + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + CyclicBarrier barrier = new CyclicBarrier(threadCount); + CountDownLatch done = new CountDownLatch(threadCount); + AtomicInteger failures = new AtomicInteger(); + + for (int i = 0; i < threadCount; i++) { + executor.submit(() -> { + try { + barrier.await(10, TimeUnit.SECONDS); + Object result = action.get(); + synchronized (resultsOut) { + resultsOut.add(result); + } + } catch (InterruptedException | BrokenBarrierException | java.util.concurrent.TimeoutException e) { + failures.incrementAndGet(); + } catch (RuntimeException e) { + failures.incrementAndGet(); + } finally { + done.countDown(); + } + }); + } + + done.await(30, TimeUnit.SECONDS); + executor.shutdownNow(); + + assertEquals(0, failures.get(), "no thread should have failed while racing to acquire the lazy stream"); + } + +} From 9331dee710451c6ba2206c7132888016e1f23161 Mon Sep 17 00:00:00 2001 From: Matt <4009945+MattBDev@users.noreply.github.com> Date: Wed, 15 Jul 2026 02:36:48 -0400 Subject: [PATCH 2/8] Address Copilot review comments on PR #3594 - getBlockOS(): close the locally-constructed stream if writeHeader() throws before it is published to the volatile osBD field, so it isn't never-closed by close(). Publishing only after a successful header write (to avoid a fast-path reader observing a headerless stream) meant a failure in between left the stream unreachable from close()'s null-guarded cleanup. - DiskStorageHistoryConcurrencyTest: run methods same-thread. The suite has method-level parallelism enabled by default and both test methods mutate the global Settings.settings().HISTORY.COMPRESSION_LEVEL in @BeforeEach/@AfterEach, so concurrent methods could race on that field. - runConcurrently(): check done.await()'s return value and await executor termination, so a hung worker thread fails the test loudly instead of passing with threads left running. - Fix a stale test comment: COMPRESSION_LEVEL is forced to 0 to bypass MainUtil's compression backend selection, not because LZ4/Zstd are missing from the test classpath (zstd is an implementation dependency, and lz4-java is now a testImplementation dependency added by this same PR). - Fix a misattributed build.gradle.kts comment: it's MainUtil#getCompressedOS, not FaweStreamChangeSet#getCompressedOS (a one-line delegate to it), that references the LZ4 types requiring the test classpath dependency. Co-Authored-By: Claude Sonnet 5 --- worldedit-core/build.gradle.kts | 9 +++--- .../core/history/DiskStorageHistory.java | 8 +++++- .../DiskStorageHistoryConcurrencyTest.java | 28 ++++++++++++++----- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/worldedit-core/build.gradle.kts b/worldedit-core/build.gradle.kts index 3ecc84954a..bf0fd8124f 100644 --- a/worldedit-core/build.gradle.kts +++ b/worldedit-core/build.gradle.kts @@ -63,10 +63,11 @@ dependencies { testRuntimeOnly(libs.log4j.core) testImplementation(libs.parallelgzip) // lz4-java is compileOnly for the main sourceSet (expected to be shaded in by platform jars - // at runtime), but FaweStreamChangeSet#getCompressedOS() references LZ4 stream types even on - // code paths that aren't taken at COMPRESSION_LEVEL 0 - the JVM verifier still needs to - // resolve those types when the method is first invoked, so tests exercising that method (e.g. - // DiskStorageHistory's stream getters) need the real classes on the test runtime classpath. + // at runtime), but MainUtil#getCompressedOS() references LZ4 stream types even on code paths + // that aren't taken at COMPRESSION_LEVEL 0 - the JVM verifier still needs to resolve those + // types when the method is first invoked, so tests exercising that method (e.g. + // DiskStorageHistory's stream getters, which delegate to it) need the real classes on the + // test runtime classpath. testImplementation(libs.lz4Java) { isTransitive = false } } diff --git a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java index 0d18a11867..50633df37b 100644 --- a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java +++ b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java @@ -315,7 +315,13 @@ public FaweOutputStream getBlockOS(int x, int y, int z) throws IOException { // fully initialized, or it could start writing block data concurrently with the // header write here and corrupt the file. FaweOutputStream stream = getCompressedOS(new FileOutputStream(bdFile)); - writeHeader(stream, x, y, z); + try { + writeHeader(stream, x, y, z); + } catch (IOException e) { + // Not yet published to osBD, so close() would never close this otherwise. + stream.close(); + throw e; + } osBD = stream; return osBD; } diff --git a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java index 1cbe0d450e..49fa4bd188 100644 --- a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java +++ b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java @@ -6,6 +6,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; import java.io.File; import java.util.Collections; @@ -21,6 +23,7 @@ import java.util.concurrent.atomic.AtomicInteger; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -28,7 +31,13 @@ * Regression test for the double-checked locking bug in {@link DiskStorageHistory}'s lazy * stream getters: racing threads must always observe the same, single, lazily-constructed * stream instance. + * + *

Runs its test methods on the same thread: {@code @BeforeEach}/{@code @AfterEach} save and + * restore the global {@code Settings.settings().HISTORY.COMPRESSION_LEVEL}, and this suite has + * method-level parallelism enabled by default, so two methods of this class racing on that field + * would corrupt each other's setting.

*/ +@Execution(ExecutionMode.SAME_THREAD) class DiskStorageHistoryConcurrencyTest { private static final int THREADS = 16; @@ -47,12 +56,10 @@ void setUp() { when(world.getMaxY()).thenReturn(319); when(world.getName()).thenReturn("concurrency-test-world"); - // The LZ4/Zstd compression backends used at COMPRESSION_LEVEL > 0 are only - // `compileOnly` dependencies of worldedit-core (they're expected to be shaded in at - // runtime by the platform jars), so they aren't on the unit test classpath. Force - // uncompressed streams so getCompressedOS() doesn't throw NoClassDefFoundError - this - // test is only concerned with the identity of the lazily-constructed stream, not with - // compression behavior. + // MainUtil.getCompressedOS() returns a plain, uncompressed stream at COMPRESSION_LEVEL 0 + // and otherwise builds an LZ4/Zstd stack. This test only cares about the identity of the + // lazily-constructed stream, not compression behavior, so force level 0 to keep it + // independent of the compression backend selection logic. originalCompressionLevel = Settings.settings().HISTORY.COMPRESSION_LEVEL; Settings.settings().HISTORY.COMPRESSION_LEVEL = 0; } @@ -142,9 +149,16 @@ private void runConcurrently(int threadCount, java.util.function.Supplier Date: Wed, 15 Jul 2026 03:00:47 -0400 Subject: [PATCH 3/8] Fix stream leak on assertion failure in DiskStorageHistoryConcurrencyTest history.close() ran after the per-iteration assertEquals(), so a failing assertion skipped it and left the racing getBlockOS()/getTileCreateOS() calls' open stream(s) unclosed. On Windows this can also make @TempDir's cleanup fail with a confusing secondary error that masks the real assertion failure. Move the close() into a finally block around the concurrent-access-and-assert sequence in both test methods. Co-Authored-By: Claude Sonnet 5 --- .../DiskStorageHistoryConcurrencyTest.java | 71 +++++++++++-------- 1 file changed, 41 insertions(+), 30 deletions(-) diff --git a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java index 49fa4bd188..c7cc2490e9 100644 --- a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java +++ b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java @@ -76,21 +76,27 @@ void getBlockOSReturnsSingleInstanceUnderConcurrentAccess() throws Exception { DiskStorageHistory history = new DiskStorageHistory(folder, world, UUID.randomUUID(), iteration); Set instances = Collections.newSetFromMap(new IdentityHashMap<>()); - runConcurrently(THREADS, () -> { - try { - return history.getBlockOS(0, 0, 0); - } catch (Exception e) { - throw new RuntimeException(e); - } - }, instances); - - assertEquals( - 1, - instances.size(), - "getBlockOS() must return exactly one distinct stream instance across racing threads (iteration " - + iteration + ")" - ); - history.close(); + try { + runConcurrently(THREADS, () -> { + try { + return history.getBlockOS(0, 0, 0); + } catch (Exception e) { + throw new RuntimeException(e); + } + }, instances); + + assertEquals( + 1, + instances.size(), + "getBlockOS() must return exactly one distinct stream instance across racing threads (iteration " + + iteration + ")" + ); + } finally { + // Must run even on assertion failure, or the open stream(s) from the racing + // getBlockOS() calls above leak - on Windows that can also make @TempDir cleanup + // fail with a confusing secondary error that masks the real assertion failure. + history.close(); + } } } @@ -101,21 +107,26 @@ void getTileCreateOSReturnsSingleInstanceUnderConcurrentAccess() throws Exceptio DiskStorageHistory history = new DiskStorageHistory(folder, world, UUID.randomUUID(), iteration); Set instances = Collections.newSetFromMap(new IdentityHashMap<>()); - runConcurrently(THREADS, () -> { - try { - return history.getTileCreateOS(); - } catch (Exception e) { - throw new RuntimeException(e); - } - }, instances); - - assertEquals( - 1, - instances.size(), - "getTileCreateOS() must return exactly one distinct stream instance across racing threads (iteration " - + iteration + ")" - ); - history.close(); + try { + runConcurrently(THREADS, () -> { + try { + return history.getTileCreateOS(); + } catch (Exception e) { + throw new RuntimeException(e); + } + }, instances); + + assertEquals( + 1, + instances.size(), + "getTileCreateOS() must return exactly one distinct stream instance across racing threads (iteration " + + iteration + ")" + ); + } finally { + // See getBlockOSReturnsSingleInstanceUnderConcurrentAccess for why this must run + // even on assertion failure. + history.close(); + } } } From 768894dd74c01ba9d10dedbac99a6a64b4af13b4 Mon Sep 17 00:00:00 2001 From: Matt <4009945+MattBDev@users.noreply.github.com> Date: Wed, 15 Jul 2026 03:04:09 -0400 Subject: [PATCH 4/8] Close the raw FileOutputStream if getCompressedOS() throws, in all 6 getters getCompressedOS(new FileOutputStream(file)) leaked the FileOutputStream if getCompressedOS() itself threw (e.g. building the LZ4/Zstd stack failed): the stream was constructed inline with no reference to close it. This affected all six lazy stream getters, including getBlockOS() - the earlier fix there only handled writeHeader() throwing after a successful getCompressedOS() call, not getCompressedOS() throwing itself. Capture the FileOutputStream in a local in each getter and close it directly if getCompressedOS() throws before returning anything. Once wrapped successfully, closing the returned stream already closes the FileOutputStream it wraps, so no extra handling is needed past that point (getBlockOS's existing writeHeader-failure handling is unaffected). Co-Authored-By: Claude Sonnet 5 --- .../core/history/DiskStorageHistory.java | 53 ++++++++++++++++--- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java index 50633df37b..d966ec4598 100644 --- a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java +++ b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java @@ -314,7 +314,18 @@ public FaweOutputStream getBlockOS(int x, int y, int z) throws IOException { // read (`if (osBD != null) return osBD;`) must never observe the stream until it is // fully initialized, or it could start writing block data concurrently with the // header write here and corrupt the file. - FaweOutputStream stream = getCompressedOS(new FileOutputStream(bdFile)); + // + // The raw FileOutputStream is kept in a local so it can be closed directly if + // getCompressedOS() itself throws before returning anything to close; once wrapped, + // closing the returned stream closes the FileOutputStream it wraps. + FileOutputStream fos = new FileOutputStream(bdFile); + FaweOutputStream stream; + try { + stream = getCompressedOS(fos); + } catch (IOException e) { + fos.close(); + throw e; + } try { writeHeader(stream, x, y, z); } catch (IOException e) { @@ -338,7 +349,13 @@ public FaweOutputStream getBiomeOS() throws IOException { } bioFile.getParentFile().mkdirs(); bioFile.createNewFile(); - osBIO = getCompressedOS(new FileOutputStream(bioFile)); + FileOutputStream fos = new FileOutputStream(bioFile); + try { + osBIO = getCompressedOS(fos); + } catch (IOException e) { + fos.close(); + throw e; + } return osBIO; } } @@ -354,7 +371,13 @@ public NBTOutputStream getEntityCreateOS() throws IOException { } enttFile.getParentFile().mkdirs(); enttFile.createNewFile(); - osENTCT = new NBTOutputStream(getCompressedOS(new FileOutputStream(enttFile))); + FileOutputStream fos = new FileOutputStream(enttFile); + try { + osENTCT = new NBTOutputStream(getCompressedOS(fos)); + } catch (IOException e) { + fos.close(); + throw e; + } return osENTCT; } } @@ -370,7 +393,13 @@ public NBTOutputStream getEntityRemoveOS() throws IOException { } entfFile.getParentFile().mkdirs(); entfFile.createNewFile(); - osENTCF = new NBTOutputStream(getCompressedOS(new FileOutputStream(entfFile))); + FileOutputStream fos = new FileOutputStream(entfFile); + try { + osENTCF = new NBTOutputStream(getCompressedOS(fos)); + } catch (IOException e) { + fos.close(); + throw e; + } return osENTCF; } } @@ -386,7 +415,13 @@ public NBTOutputStream getTileCreateOS() throws IOException { } nbttFile.getParentFile().mkdirs(); nbttFile.createNewFile(); - osNBTT = new NBTOutputStream(getCompressedOS(new FileOutputStream(nbttFile))); + FileOutputStream fos = new FileOutputStream(nbttFile); + try { + osNBTT = new NBTOutputStream(getCompressedOS(fos)); + } catch (IOException e) { + fos.close(); + throw e; + } return osNBTT; } } @@ -402,7 +437,13 @@ public NBTOutputStream getTileRemoveOS() throws IOException { } nbtfFile.getParentFile().mkdirs(); nbtfFile.createNewFile(); - osNBTF = new NBTOutputStream(getCompressedOS(new FileOutputStream(nbtfFile))); + FileOutputStream fos = new FileOutputStream(nbtfFile); + try { + osNBTF = new NBTOutputStream(getCompressedOS(fos)); + } catch (IOException e) { + fos.close(); + throw e; + } return osNBTF; } } From 2c623ae9259f9adafc2912dfc1df781c994f7868 Mon Sep 17 00:00:00 2001 From: Matt <4009945+MattBDev@users.noreply.github.com> Date: Wed, 15 Jul 2026 08:20:22 -0400 Subject: [PATCH 5/8] Fix exception masking and unchecked-exception fd leaks in all six getters Two related cleanup-on-failure bugs, found during a follow-up code review of this file's own getters: 1. getBlockOS()'s writeHeader() failure handler closed the not-yet-published stream and rethrew the original IOException - but if that close() attempt itself threw, its exception replaced (masked) the original failure being propagated, hiding the real cause from callers. 2. All six getters only closed their raw FileOutputStream on IOException. MainUtil.getCompressedOS()/writeHeader() can also fail with an unchecked RuntimeException or Error (e.g. a linkage error constructing a compression stream), which would leave the FileOutputStream open and unreachable - it's never published to the instance field, so close() would never find it either. Adds a shared closeQuietly(AutoCloseable, Throwable) helper: it closes the given resource and, if that close itself fails, attaches the failure via addSuppressed() rather than letting it replace the primary exception. All six getters' catch clauses now cover IOException, RuntimeException, and Error. Adds getBlockOSCloseFailureIsSuppressedNotMasked, a deterministic regression test using a Mockito spy: forces writeHeader() to fail and the subsequent close() to also fail, and asserts the original header-write exception is what propagates, with the close failure attached as suppressed rather than replacing it. Against the pre-fix code this fails with the close failure propagating instead of the original ("expected:
but was: "). Also hardens runConcurrently() in the same test class: executor shutdown/awaitTermination now run in a finally block, so an interrupted done.await() no longer leaks the thread pool. Co-Authored-By: Claude Sonnet 5 --- .../core/history/DiskStorageHistory.java | 53 ++++++++++++----- .../DiskStorageHistoryConcurrencyTest.java | 58 ++++++++++++++++++- 2 files changed, 93 insertions(+), 18 deletions(-) diff --git a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java index d966ec4598..51e29c2775 100644 --- a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java +++ b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java @@ -298,6 +298,25 @@ public long getSizeOnDisk() { return total; } + /** + * Closes {@code closeable} (if not null), suppressing rather than propagating or masking any + * failure from the close itself onto {@code primary} - the original failure that triggered + * the cleanup, and the one that must actually reach the caller. Used when a fallible resource + * has to be closed before rethrowing, without losing the real cause if the close itself + * fails too. No-op if {@code closeable} is null (e.g. construction failed before it was + * created). + */ + private static void closeQuietly(AutoCloseable closeable, Throwable primary) { + if (closeable == null) { + return; + } + try { + closeable.close(); + } catch (Exception suppressed) { + primary.addSuppressed(suppressed); + } + } + @Override public FaweOutputStream getBlockOS(int x, int y, int z) throws IOException { if (osBD != null) { @@ -317,20 +336,24 @@ public FaweOutputStream getBlockOS(int x, int y, int z) throws IOException { // // The raw FileOutputStream is kept in a local so it can be closed directly if // getCompressedOS() itself throws before returning anything to close; once wrapped, - // closing the returned stream closes the FileOutputStream it wraps. + // closing the returned stream closes the FileOutputStream it wraps. Both catch + // clauses below also cover unchecked RuntimeException/Error, not just IOException: + // MainUtil.getCompressedOS()/writeHeader() can fail that way too (e.g. a linkage + // error constructing a compression stream), and an unchecked failure after fos is + // opened would otherwise leak the file descriptor since it's never published to osBD. FileOutputStream fos = new FileOutputStream(bdFile); FaweOutputStream stream; try { stream = getCompressedOS(fos); - } catch (IOException e) { - fos.close(); + } catch (IOException | RuntimeException | Error e) { + closeQuietly(fos, e); throw e; } try { writeHeader(stream, x, y, z); - } catch (IOException e) { + } catch (IOException | RuntimeException | Error e) { // Not yet published to osBD, so close() would never close this otherwise. - stream.close(); + closeQuietly(stream, e); throw e; } osBD = stream; @@ -352,8 +375,8 @@ public FaweOutputStream getBiomeOS() throws IOException { FileOutputStream fos = new FileOutputStream(bioFile); try { osBIO = getCompressedOS(fos); - } catch (IOException e) { - fos.close(); + } catch (IOException | RuntimeException | Error e) { + closeQuietly(fos, e); throw e; } return osBIO; @@ -374,8 +397,8 @@ public NBTOutputStream getEntityCreateOS() throws IOException { FileOutputStream fos = new FileOutputStream(enttFile); try { osENTCT = new NBTOutputStream(getCompressedOS(fos)); - } catch (IOException e) { - fos.close(); + } catch (IOException | RuntimeException | Error e) { + closeQuietly(fos, e); throw e; } return osENTCT; @@ -396,8 +419,8 @@ public NBTOutputStream getEntityRemoveOS() throws IOException { FileOutputStream fos = new FileOutputStream(entfFile); try { osENTCF = new NBTOutputStream(getCompressedOS(fos)); - } catch (IOException e) { - fos.close(); + } catch (IOException | RuntimeException | Error e) { + closeQuietly(fos, e); throw e; } return osENTCF; @@ -418,8 +441,8 @@ public NBTOutputStream getTileCreateOS() throws IOException { FileOutputStream fos = new FileOutputStream(nbttFile); try { osNBTT = new NBTOutputStream(getCompressedOS(fos)); - } catch (IOException e) { - fos.close(); + } catch (IOException | RuntimeException | Error e) { + closeQuietly(fos, e); throw e; } return osNBTT; @@ -440,8 +463,8 @@ public NBTOutputStream getTileRemoveOS() throws IOException { FileOutputStream fos = new FileOutputStream(nbtfFile); try { osNBTF = new NBTOutputStream(getCompressedOS(fos)); - } catch (IOException e) { - fos.close(); + } catch (IOException | RuntimeException | Error e) { + closeQuietly(fos, e); throw e; } return osNBTF; diff --git a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java index c7cc2490e9..7cf93b3b67 100644 --- a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java +++ b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java @@ -1,6 +1,7 @@ package com.fastasyncworldedit.core.history; import com.fastasyncworldedit.core.configuration.Settings; +import com.fastasyncworldedit.core.internal.io.FaweOutputStream; import com.sk89q.worldedit.world.World; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -10,6 +11,7 @@ import org.junit.jupiter.api.parallel.ExecutionMode; import java.io.File; +import java.io.IOException; import java.util.Collections; import java.util.IdentityHashMap; import java.util.Set; @@ -23,8 +25,15 @@ import java.util.concurrent.atomic.AtomicInteger; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; /** @@ -160,9 +169,16 @@ private void runConcurrently(int threadCount, java.util.function.Supplier { + FaweOutputStream real = (FaweOutputStream) invocation.callRealMethod(); + FaweOutputStream streamSpy = spy(real); + doAnswer(closeInvocation -> { + real.close(); + throw closeFailure; + }).when(streamSpy).close(); + return streamSpy; + }).when(history).getCompressedOS(any()); + + IOException headerFailure = new IOException("header failed"); + doThrow(headerFailure).when(history).writeHeader(any(), anyInt(), anyInt(), anyInt()); + + IOException thrown = assertThrows(IOException.class, () -> history.getBlockOS(0, 0, 0)); + + assertSame(headerFailure, thrown, "the original header-write failure must propagate, not be masked by a " + + "failure while closing the stream"); + assertEquals(1, thrown.getSuppressed().length, "the close failure must be attached as a suppressed exception"); + assertSame(closeFailure, thrown.getSuppressed()[0]); + } + } From df5c3554c197fd6aff6b34ad63d8bcc646da9ea0 Mon Sep 17 00:00:00 2001 From: Matt <4009945+MattBDev@users.noreply.github.com> Date: Wed, 15 Jul 2026 08:29:22 -0400 Subject: [PATCH 6/8] Widen closeQuietly's catch to Throwable, not just Exception closeQuietly only caught Exception, so an Error thrown by the cleanup close() itself (e.g. an AssertionError, or in production something like a LinkageError) would still propagate in place of the primary failure - exactly the masking bug this helper exists to prevent, just one level removed. Catching Throwable doesn't swallow anything: the caught throwable is always attached to primary via addSuppressed, and primary is always rethrown by the caller. Adds getBlockOSErrorFromCloseIsSuppressedNotMasked. Note the first attempt at this test used a RuntimeException for the close failure, which is itself an Exception subtype and so didn't actually distinguish catch(Exception) from catch(Throwable) - it passed either way. Using an Error (AssertionError) instead genuinely exercises the distinction: it fails with catch(Exception) ("expected IOException but was AssertionError") and passes with catch(Throwable). Co-Authored-By: Claude Sonnet 5 --- .../core/history/DiskStorageHistory.java | 7 +++- .../DiskStorageHistoryConcurrencyTest.java | 34 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java index 51e29c2775..c3037ac158 100644 --- a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java +++ b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java @@ -312,7 +312,12 @@ private static void closeQuietly(AutoCloseable closeable, Throwable primary) { } try { closeable.close(); - } catch (Exception suppressed) { + } catch (Throwable suppressed) { + // Throwable, not just Exception: a RuntimeException or Error from close() itself + // must not propagate in place of primary either, or it would mask the original + // failure exactly like the checked-exception case this method exists to prevent. + // This still doesn't swallow anything - it's attached to primary, which callers + // always rethrow. primary.addSuppressed(suppressed); } } diff --git a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java index 7cf93b3b67..4994d2a7d5 100644 --- a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java +++ b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java @@ -225,4 +225,38 @@ void getBlockOSCloseFailureIsSuppressedNotMasked() throws Exception { assertSame(closeFailure, thrown.getSuppressed()[0]); } + /** + * Same as {@link #getBlockOSCloseFailureIsSuppressedNotMasked()}, but the cleanup close() + * itself fails with an {@link Error} rather than an {@code IOException}. {@code + * closeQuietly()} must catch {@link Throwable}, not just {@link Exception} - {@link + * RuntimeException} would not distinguish the two, since it is itself an {@code Exception} - + * or an {@code Error} from close() would propagate in place of the original and mask it + * exactly like the checked-exception case does. + */ + @Test + void getBlockOSErrorFromCloseIsSuppressedNotMasked() throws Exception { + DiskStorageHistory history = spy(new DiskStorageHistory(tempDir, world, UUID.randomUUID(), 0)); + + Error closeFailure = new AssertionError("close failed with an Error"); + doAnswer(invocation -> { + FaweOutputStream real = (FaweOutputStream) invocation.callRealMethod(); + FaweOutputStream streamSpy = spy(real); + doAnswer(closeInvocation -> { + real.close(); + throw closeFailure; + }).when(streamSpy).close(); + return streamSpy; + }).when(history).getCompressedOS(any()); + + IOException headerFailure = new IOException("header failed"); + doThrow(headerFailure).when(history).writeHeader(any(), anyInt(), anyInt(), anyInt()); + + IOException thrown = assertThrows(IOException.class, () -> history.getBlockOS(0, 0, 0)); + + assertSame(headerFailure, thrown, "the original header-write failure must propagate, not be masked by an " + + "unchecked failure while closing the stream"); + assertEquals(1, thrown.getSuppressed().length, "the close failure must be attached as a suppressed exception"); + assertSame(closeFailure, thrown.getSuppressed()[0]); + } + } From f194ad5ecf62af7539bd261c26f13f8891cbb613 Mon Sep 17 00:00:00 2001 From: Matt <4009945+MattBDev@users.noreply.github.com> Date: Wed, 15 Jul 2026 08:46:11 -0400 Subject: [PATCH 7/8] Close the wrapped compression stream, not just fos, in the four NBT getters getEntityCreateOS/getEntityRemoveOS/getTileCreateOS/getTileRemoveOS each did `osENTCT = new NBTOutputStream(getCompressedOS(fos))` in one step, with a catch clause that only closed the raw fos. If getCompressedOS(fos) succeeded (building a wrapped chain of buffered/compression streams around fos) but the NBTOutputStream constructor then failed, only fos got closed - the wrapper chain getCompressedOS built was left unclosed and unreachable, mirroring the same-shaped bug already fixed in getBlockOS(). Split each into two steps like getBlockOS(): capture the FaweOutputStream from getCompressedOS() into a local first, and if the NBTOutputStream constructor around it then fails, close that wrapper (which cascades down and closes fos too) rather than fos directly. NBTOutputStream(OutputStream) doesn't declare IOException, so the second try's catch only needs RuntimeException/Error. Also restores the thread's interrupt status in runConcurrently()'s InterruptedException handler, which was swallowing it - test code, but letting it get lost can still confuse higher-level timeout/cancellation logic and makes failures harder to diagnose. Co-Authored-By: Claude Sonnet 5 --- .../core/history/DiskStorageHistory.java | 46 +++++++++++++++++-- .../DiskStorageHistoryConcurrencyTest.java | 5 +- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java index c3037ac158..cd1779402c 100644 --- a/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java +++ b/worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java @@ -399,13 +399,27 @@ public NBTOutputStream getEntityCreateOS() throws IOException { } enttFile.getParentFile().mkdirs(); enttFile.createNewFile(); + // Two-step, like getBlockOS(): if getCompressedOS() succeeds but the NBTOutputStream + // constructor then fails, closing only fos would leave the compression wrapper chain + // getCompressedOS() built (buffered/LZ4 streams) unclosed. Close whichever the + // innermost fully-constructed object is at the point of failure - closing the wrapper + // cascades down and closes fos too. FileOutputStream fos = new FileOutputStream(enttFile); + FaweOutputStream stream; try { - osENTCT = new NBTOutputStream(getCompressedOS(fos)); + stream = getCompressedOS(fos); } catch (IOException | RuntimeException | Error e) { closeQuietly(fos, e); throw e; } + try { + osENTCT = new NBTOutputStream(stream); + } catch (RuntimeException | Error e) { + // NBTOutputStream(OutputStream) doesn't declare IOException - only unchecked + // failures are possible here. + closeQuietly(stream, e); + throw e; + } return osENTCT; } } @@ -421,13 +435,21 @@ public NBTOutputStream getEntityRemoveOS() throws IOException { } entfFile.getParentFile().mkdirs(); entfFile.createNewFile(); + // See getEntityCreateOS() for why this is two steps. FileOutputStream fos = new FileOutputStream(entfFile); + FaweOutputStream stream; try { - osENTCF = new NBTOutputStream(getCompressedOS(fos)); + stream = getCompressedOS(fos); } catch (IOException | RuntimeException | Error e) { closeQuietly(fos, e); throw e; } + try { + osENTCF = new NBTOutputStream(stream); + } catch (RuntimeException | Error e) { + closeQuietly(stream, e); + throw e; + } return osENTCF; } } @@ -443,13 +465,21 @@ public NBTOutputStream getTileCreateOS() throws IOException { } nbttFile.getParentFile().mkdirs(); nbttFile.createNewFile(); + // See getEntityCreateOS() for why this is two steps. FileOutputStream fos = new FileOutputStream(nbttFile); + FaweOutputStream stream; try { - osNBTT = new NBTOutputStream(getCompressedOS(fos)); + stream = getCompressedOS(fos); } catch (IOException | RuntimeException | Error e) { closeQuietly(fos, e); throw e; } + try { + osNBTT = new NBTOutputStream(stream); + } catch (RuntimeException | Error e) { + closeQuietly(stream, e); + throw e; + } return osNBTT; } } @@ -465,13 +495,21 @@ public NBTOutputStream getTileRemoveOS() throws IOException { } nbtfFile.getParentFile().mkdirs(); nbtfFile.createNewFile(); + // See getEntityCreateOS() for why this is two steps. FileOutputStream fos = new FileOutputStream(nbtfFile); + FaweOutputStream stream; try { - osNBTF = new NBTOutputStream(getCompressedOS(fos)); + stream = getCompressedOS(fos); } catch (IOException | RuntimeException | Error e) { closeQuietly(fos, e); throw e; } + try { + osNBTF = new NBTOutputStream(stream); + } catch (RuntimeException | Error e) { + closeQuietly(stream, e); + throw e; + } return osNBTF; } } diff --git a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java index 4994d2a7d5..5df7f01503 100644 --- a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java +++ b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java @@ -159,7 +159,10 @@ private void runConcurrently(int threadCount, java.util.function.Supplier Date: Wed, 15 Jul 2026 09:10:42 -0400 Subject: [PATCH 8/8] Catch Throwable in runConcurrently()'s worker threads, not just RuntimeException An Error (e.g. an AssertionError from a failed assertion inside a worker) would previously complete that submitted task's Future exceptionally with nothing checking it, and the test could still pass since failures wasn't incremented for it. Co-Authored-By: Claude Sonnet 5 --- .../core/history/DiskStorageHistoryConcurrencyTest.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java index 5df7f01503..4b3d51e095 100644 --- a/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java +++ b/worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java @@ -164,7 +164,11 @@ private void runConcurrently(int threadCount, java.util.function.Supplier