Fix broken double-checked locking in DiskStorageHistory stream getters by MattBDev · Pull Request #3594 · IntellectualSites/FastAsyncWorldEdit

MattBDev · 2026-07-15T06:05:15Z

Part of Phase 1 (concurrency hardening) of the com.fastasyncworldedit.core.history improvement plan. One of five independent PRs; this one only touches DiskStorageHistory.

The bug

All six lazy output-stream getters checked their backing field outside the lock without re-checking it inside, the four NBT getters were not synchronized at all, and none of the six fields were volatile. Two threads could both pass the null check and each construct a FileOutputStream for the same history file — the second truncating the file and rewriting its header over an edit already being recorded.

The fix

Six stream fields are now volatile.
All six getters use the standard shape: check → synchronize → re-check → construct.
getBlockOS() writes the header into a local before publishing to the volatile field. Publishing first would let a fast-path reader observe a non-null stream and write block data while the constructing thread was still writing the header, interleaving records ahead of it on a stream that is not thread-safe.

Verification

:worldedit-core:compileJava / compileTestJava pass.
DiskStorageHistoryConcurrencyTest is a genuine regression test: 2 failures against pre-fix code, 2/2 pass with the fix.

Notes

Not rebased on any sibling PR; mergeable independently.
Builds inside a git worktree additionally need the Grgit fix from Add baseline benchmark for history write path (pre Phase 1 concurrency fixes) #3593; that change is deliberately not included here since CI uses a normal checkout.
testImplementation(libs.lz4Java) is added because lz4-java is compileOnly for main, so the test JVM cannot resolve the LZ4 stream types reached via getCompressedOS().

🤖 Generated with Claude Code

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 <noreply@anthropic.com>

Copilot

Pull request overview

This PR hardens DiskStorageHistory’s lazy output-stream initialization against concurrent access by fixing broken double-checked locking patterns and adding a regression test to prevent stream duplication/corruption.

Changes:

Make the six lazily-initialized output stream fields volatile and fix all getters to use check → synchronize → re-check → construct.
Ensure getBlockOS() writes the header before publishing the stream reference.
Add DiskStorageHistoryConcurrencyTest and add lz4-java to the test classpath.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java	Fixes DCL for stream getters and adds `volatile` for safe publication under concurrency.
worldedit-core/src/test/java/com/fastasyncworldedit/core/history/DiskStorageHistoryConcurrencyTest.java	Adds regression coverage for concurrent access to lazy stream getters.
worldedit-core/build.gradle.kts	Adds `lz4-java` to test dependencies so compression-related classes resolve in tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

MattBDev · 2026-07-15T06:37:05Z

+            FaweOutputStream stream = getCompressedOS(new FileOutputStream(bdFile));
+            writeHeader(stream, x, y, z);
+            osBD = stream;
            return osBD;


Fixed: the local stream is now closed in a catch block if writeHeader() throws, before rethrowing. See 9331dee.

MattBDev · 2026-07-15T06:37:06Z

+ * stream getters: racing threads must always observe the same, single, lazily-constructed
+ * stream instance.
+ */
+class DiskStorageHistoryConcurrencyTest {


Fixed: added @execution(ExecutionMode.SAME_THREAD) to the test class so the two methods can't race on the global COMPRESSION_LEVEL field. See 9331dee.

MattBDev · 2026-07-15T06:37:08Z

+        done.await(30, TimeUnit.SECONDS);
+        executor.shutdownNow();
+
+        assertEquals(0, failures.get(), "no thread should have failed while racing to acquire the lazy stream");


Fixed: runConcurrently() now checks done.await()'s return value and awaits executor termination, failing loudly on either timeout instead of silently passing. See 9331dee.

MattBDev · 2026-07-15T06:37:09Z

+        // 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.


Fixed: corrected the comment. COMPRESSION_LEVEL is forced to 0 to bypass MainUtils compression backend selection, not because LZ4/Zstd are missing from the classpath (zstd is an implementation dependency, and lz4-java is now a testImplementation dependency added by this PR). See 9331dee.

MattBDev · 2026-07-15T06:37:10Z

+    // 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.


Fixed: corrected the attribution to MainUtil#getCompressedOS(), which is what actually references the LZ4 types; FaweStreamChangeSet#getCompressedOS() is a one-line delegate to it. See 9331dee.

- 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 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

worldedit-core/src/main/java/com/fastasyncworldedit/core/history/DiskStorageHistory.java:342

If getCompressedOS(...) throws after the FileOutputStream is opened, the FileOutputStream is leaked. Capture the FileOutputStream in a local and close it on both IOException and RuntimeException before rethrowing.

            bioFile.getParentFile().mkdirs();
            bioFile.createNewFile();
            osBIO = getCompressedOS(new FileOutputStream(bioFile));
            return osBIO;

MattBDev · 2026-07-15T07:04:41Z

+            FaweOutputStream stream = getCompressedOS(new FileOutputStream(bdFile));
+            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;


Fixed: the FileOutputStream is now captured in a local and closed directly if getCompressedOS() throws, in all six getters (getBlockOS included, which had the same gap for this specific failure point). See the latest commit on this branch.

MattBDev · 2026-07-15T07:04:42Z

+            enttFile.getParentFile().mkdirs();
+            enttFile.createNewFile();
+            osENTCT = new NBTOutputStream(getCompressedOS(new FileOutputStream(enttFile)));
+            return osENTCT;


Fixed: the FileOutputStream is now captured in a local and closed directly if getCompressedOS() throws, in all six getters (getBlockOS included, which had the same gap for this specific failure point). See the latest commit on this branch.

MattBDev · 2026-07-15T07:04:44Z

+            entfFile.getParentFile().mkdirs();
+            entfFile.createNewFile();
+            osENTCF = new NBTOutputStream(getCompressedOS(new FileOutputStream(entfFile)));
+            return osENTCF;


Fixed: the FileOutputStream is now captured in a local and closed directly if getCompressedOS() throws, in all six getters (getBlockOS included, which had the same gap for this specific failure point). See the latest commit on this branch.

MattBDev · 2026-07-15T07:04:45Z

+            nbttFile.getParentFile().mkdirs();
+            nbttFile.createNewFile();
+            osNBTT = new NBTOutputStream(getCompressedOS(new FileOutputStream(nbttFile)));
+            return osNBTT;


Fixed: the FileOutputStream is now captured in a local and closed directly if getCompressedOS() throws, in all six getters (getBlockOS included, which had the same gap for this specific failure point). See the latest commit on this branch.

MattBDev · 2026-07-15T07:04:46Z

+            nbtfFile.getParentFile().mkdirs();
+            nbtfFile.createNewFile();
+            osNBTF = new NBTOutputStream(getCompressedOS(new FileOutputStream(nbtfFile)));
+            return osNBTF;


Fixed: the FileOutputStream is now captured in a local and closed directly if getCompressedOS() throws, in all six getters (getBlockOS included, which had the same gap for this specific failure point). See the latest commit on this branch.

Both barrier.await() and done.await() were unbounded. If the getter under test ever deadlocked - exactly the failure mode this test exists to catch - the test would hang indefinitely instead of failing, potentially hanging CI. Bring this in line with the equivalent test in DiskStorageHistoryConcurrencyTest (PR #3594), which already does this: bound the barrier wait, and check done.await()'s return value, failing loudly with a diagnostic on timeout instead of silently proceeding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…Test 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 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

MattBDev · 2026-07-15T12:20:34Z

+            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;
+            }


Fixed in 2c623ae: added a shared closeQuietly helper that attaches any close failure via addSuppressed instead of letting it replace the original exception. Added a deterministic test (getBlockOSCloseFailureIsSuppressedNotMasked) that forces both failures and confirms the original propagates with the close failure suppressed.

MattBDev · 2026-07-15T12:20:44Z

+        boolean completed = done.await(30, TimeUnit.SECONDS);
+        executor.shutdownNow();
+        boolean terminated = executor.awaitTermination(10, TimeUnit.SECONDS);
+
+        if (!completed) {
+            fail("Timed out waiting for racing threads to finish (possible barrier deadlock or hang)");
+        }
+        if (!terminated) {
+            fail("Executor did not terminate after shutdownNow(); worker threads may still be running");
+        }
+        assertEquals(0, failures.get(), "no thread should have failed while racing to acquire the lazy stream");


Fixed in 2c623ae: executor shutdownNow()/awaitTermination now run in a finally block, so an interrupted done.await() no longer skips them and leaks the pool.

…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 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

MattBDev · 2026-07-15T12:22:36Z

+            FileOutputStream fos = new FileOutputStream(entfFile);
+            try {
+                osENTCF = new NBTOutputStream(getCompressedOS(fos));
+            } catch (IOException e) {
+                fos.close();
+                throw e;
+            }


Fixed in 2c623ae: getEntityRemoveOS() now closes fos via the shared closeQuietly helper on IOException, RuntimeException, or Error, with addSuppressed used if the close itself fails.

MattBDev · 2026-07-15T12:22:44Z

+            FileOutputStream fos = new FileOutputStream(nbttFile);
+            try {
+                osNBTT = new NBTOutputStream(getCompressedOS(fos));
+            } catch (IOException e) {
+                fos.close();
+                throw e;
+            }


Fixed in 2c623ae: getTileCreateOS() now closes fos via the shared closeQuietly helper on IOException, RuntimeException, or Error, with addSuppressed used if the close itself fails.

MattBDev · 2026-07-15T12:23:04Z

+            FileOutputStream fos = new FileOutputStream(nbtfFile);
+            try {
+                osNBTF = new NBTOutputStream(getCompressedOS(fos));
+            } catch (IOException e) {
+                fos.close();
+                throw e;
+            }


Fixed in 2c623ae: getTileRemoveOS() now closes fos via the shared closeQuietly helper on IOException, RuntimeException, or Error, with addSuppressed used if the close itself fails.

…ters 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: <header failed> but was: <close failed>"). 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 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

MattBDev · 2026-07-15T12:29:32Z

+        try {
+            closeable.close();
+        } catch (Exception suppressed) {
+            primary.addSuppressed(suppressed);
+        }


Good catch, and my own first attempt at a regression test for this missed it too (a RuntimeException is itself an Exception, so it didn't distinguish the two catch clauses). Fixed in df5c355: closeQuietly now catches Throwable, and the added test uses a genuine Error (AssertionError) to verify the distinction - it fails with catch(Exception) and passes with catch(Throwable).

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 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

MattBDev · 2026-07-15T12:46:27Z

+            FileOutputStream fos = new FileOutputStream(enttFile);
+            try {
+                osENTCT = new NBTOutputStream(getCompressedOS(fos));
+            } catch (IOException | RuntimeException | Error e) {
+                closeQuietly(fos, e);
+                throw e;
+            }
+            return osENTCT;


Fixed in f194ad5: split into two steps like getBlockOS() - the FaweOutputStream from getCompressedOS() is now captured in a local, and if the NBTOutputStream constructor around it then fails, that wrapper is closed (cascading down to fos) instead of fos alone.

MattBDev · 2026-07-15T12:46:37Z

+            FileOutputStream fos = new FileOutputStream(entfFile);
+            try {
+                osENTCF = new NBTOutputStream(getCompressedOS(fos));
+            } catch (IOException | RuntimeException | Error e) {
+                closeQuietly(fos, e);
+                throw e;
+            }
+            return osENTCF;


Fixed in f194ad5, same fix as getEntityCreateOS().

MattBDev · 2026-07-15T12:46:59Z

+            FileOutputStream fos = new FileOutputStream(nbttFile);
+            try {
+                osNBTT = new NBTOutputStream(getCompressedOS(fos));
+            } catch (IOException | RuntimeException | Error e) {
+                closeQuietly(fos, e);
+                throw e;
+            }
+            return osNBTT;


Fixed in f194ad5, same fix as getEntityCreateOS().

MattBDev · 2026-07-15T12:47:22Z

+            FileOutputStream fos = new FileOutputStream(nbtfFile);
+            try {
+                osNBTF = new NBTOutputStream(getCompressedOS(fos));
+            } catch (IOException | RuntimeException | Error e) {
+                closeQuietly(fos, e);
+                throw e;
+            }
+            return osNBTF;


Fixed in f194ad5, same fix as getEntityCreateOS().

…etters 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 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

MattBDev · 2026-07-15T13:11:07Z

+                } catch (BrokenBarrierException | java.util.concurrent.TimeoutException e) {
+                    failures.incrementAndGet();
+                } catch (RuntimeException e) {
+                    failures.incrementAndGet();


Good catch, fixed: the catch clause is now Throwable instead of RuntimeException, so an escaped Error is recorded as a failure instead of silently vanishing.

…meException 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 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 15, 2026 06:05

MattBDev requested a review from a team as a code owner July 15, 2026 06:05

MattBDev mentioned this pull request Jul 15, 2026

Fix broken double-checked locking in MemoryOptimizedHistory stream getters #3595

Open

Copilot started reviewing on behalf of MattBDev July 15, 2026 06:05 View session

MattBDev mentioned this pull request Jul 15, 2026

Make FaweStreamChangeSet.blockSize atomic and document read reentrancy #3598

Open