Fix broken double-checked locking in MemoryOptimizedHistory stream getters

[MattBDev]

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

The bugs

1. Same shape as Fix broken double-checked locking in DiskStorageHistory stream getters #3594, in the in-memory sibling class. All six lazy stream getters checked their field outside the lock without re-checking inside, the four NBT getters were unsynchronized, and none of the six fields were volatile. Two threads could both construct a compressed stream over a fresh buffer, silently discarding one thread's recorded changes.
2. Found during a follow-up code review, fixed in 7df0cdb: all six getters assigned their buffer field (e.g. idsStream) before constructing the wrapping compressed/NBT stream, which can throw. A failure there left the buffer field non-null while the wrapper field (e.g. idsStreamZip) stayed null. flush()/close() gate on the buffer field being non-null and then unconditionally dereference the wrapper field, so instead of cleanly skipping the failed stream they threw NullPointerException — which, since close() calls super.close() first, would propagate out through AbstractChangeSet.close() to whatever called close() on the changeset.

The fix

• Six *Zip fields are now volatile.
• All six getters use: check → synchronize → re-check → construct — deliberately the same shape as Fix broken double-checked locking in DiskStorageHistory stream getters #3594, since the two classes are read side by side.
• getBlockOS() writes the header before publishing to the volatile field (see Fix broken double-checked locking in DiskStorageHistory stream getters #3594 for the reasoning).
• Each getter now builds its buffer and wrapper into locals and publishes both fields together only once construction fully succeeds, so a failed construction leaves neither field set (fix Fix build - gradle URLs & bad imports #2 above).

Verification

• :worldedit-core:compileJava / compileTestJava pass.
• MemoryOptimizedHistoryConcurrencyTest: 121/121 pass with the fix (extended in 7df0cdb to cover all six getters, daemon worker threads, and a non-null assertion on the getter's return value).
• Against pre-fix code the DCL race's test JVM crashes and the build fails (Could not initialize class ... ExceptionUtils) rather than failing an assertion cleanly — the race allocates duplicate ~520KB buffers per racing thread until the 1G test heap is exhausted. So there is a real fail→pass transition, but the pre-fix failure mode is a dead executor, not a clean assertion. Flagging that rather than claiming a clean regression test.
• getBlockOSFailureLeavesNoPartialStateForFlushOrClose (added in 7df0cdb) is a deterministic regression test for the partial-state bug: a Mockito spy forces getCompressedOS() to throw, and the test asserts flush()/close() don't NPE afterward. Against the pre-fix code it fails with the exact NPE (Cannot invoke FaweOutputStream.flush() because idsStreamZip is null); with the fix it passes.

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; deliberately not included (CI uses a normal checkout).
• testRuntimeOnly(libs.lz4Java) added: lz4-java is compileOnly for main, so the test JVM otherwise hits NoClassDefFoundError: LZ4BlockOutputStream via getCompressedOS().
• Requesting a second reviewer specifically on 7df0cdb before merge — see the PR comment below for what to double-check.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR hardens MemoryOptimizedHistory against a lost-update race caused by broken double-checked locking in its lazy, cached stream getters, aligning the implementation with the concurrency-fix pattern used for the disk-backed history variant.

Changes:

• Make the six cached “*Zip” stream fields volatile and update all six getters to use check → synchronized → re-check → construct.
• Ensure getBlockOS() fully initializes (writes header) before publishing the stream reference.
• Add a concurrency regression test and adjust test runtime dependencies to ensure LZ4 stream classes are present at test runtime.

Reviewed changes

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

File	Description
worldedit-core/src/main/java/com/fastasyncworldedit/core/history/MemoryOptimizedHistory.java	Fix lazy stream initialization races via volatile + correct double-checked locking; adjust getBlockOS() publication ordering.
worldedit-core/src/test/java/com/fastasyncworldedit/core/history/MemoryOptimizedHistoryConcurrencyTest.java	Add regression test validating stream getter returns a single instance under concurrent access.
worldedit-core/build.gradle.kts	Add LZ4 library on the test runtime classpath to avoid NoClassDefFoundError during concurrency tests.

---

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

[Copilot]

Pull request overview

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

[MattBDev]

Flagging commit 7df0cdb for a second reviewer — this fixes a genuinely critical bug found during a follow-up code review, not just the original DCL race this PR started with:

The bug: every one of the six lazy getters (getBlockOS, getBiomeOS, getEntityCreateOS, getEntityRemoveOS, getTileCreateOS, getTileRemoveOS) assigned its buffer field (e.g. idsStream) before constructing the wrapping compressed/NBT stream (getCompressedOS(...), or writeHeader(...) for getBlockOS specifically) - both of which can throw. A failure there left the buffer field non-null while the wrapper field (e.g. idsStreamZip) stayed null. flush()/close() gate on the buffer field being non-null and then unconditionally dereference the wrapper field, so instead of just skipping the failed stream, they NPE - and since MemoryOptimizedHistory.close() calls super.close() first, this NPE would propagate up through AbstractChangeSet.close() and out to whatever called close() on the changeset (e.g. EditSession's session-completion path), rather than the changeset just cleanly closing whatever streams did succeed.

The fix: each getter now builds the buffer and its wrapper into locals and only publishes both fields together once construction fully succeeds, so a failure leaves neither field set.

What to double-check:

1. That I haven't missed a seventh call site with the same buffer-then-wrapper-field pattern (I audited all six getOS methods declared in this class; getBiomeIS/getIS read-side methods don't have this issue since they don't construct+publish a pair of fields).
2. The new regression test (getBlockOSFailureLeavesNoPartialStateForFlushOrClose) uses a Mockito spy to force getCompressedOS() to throw, rather than a real compression-library failure - worth confirming that's a faithful enough substitute for the failure modes that actually occur in production (disk-full-style IOExceptions from the underlying FastByteArrayOutputStream, not just an injected throw).
3. Whether any caller depends on the previous (buggy) behavior in a way I haven't considered - I don't believe so, since the old behavior was an uncaught NPE rather than any deliberate contract, but flagging in case.

[Copilot]

Pull request overview

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