Fix partial stream state in MemoryOptimizedHistory lazy getters; harden lazy init

[MattBDev]

Part of Phase 1 of the com.fastasyncworldedit.core.history improvement plan. Touches MemoryOptimizedHistory only.

Rewritten after review. This PR was originally titled and described as fixing a data-loss race. That framing was wrong and has been corrected — see Correction at the bottom. The lead item below is a real, deterministic, single-threaded bug.

The bug

Every lazy stream getter assigned its *Stream buffer field before constructing the wrapping compressed/NBT stream, which can throw:

idsStream    = new FastByteArrayOutputStream(...);   // published
idsStreamZip = getCompressedOS(idsStream);           // ← throws

flush() and close() gated on the buffer field being non-null and then unconditionally dereferenced the wrapper field. So an IOException during history setup left a half-built pair, and FaweStreamChangeSet.add() swallows that IOException with a printStackTrace() — meaning the edit carries on and the failure resurfaces much later as an uncaught NullPointerException escaping close(), after super.close() has already run.

Secondary, in getBlockOS() only: if writeHeader() throws, the compressed stream has been constructed but is never published, so nothing can ever close it. It leaked along with its ~520KB buffer.

The fix

• Publish together. Each getter builds buffer + wrapper into locals and assigns both fields only once construction has fully succeeded, so a failure leaves neither set.
• Gate on the wrapper. flush()/close() now test the *StreamZip field they actually dereference, rather than the buffer field. Correct regardless of how the pair was published.
• Close on failure. getBlockOS() closes the compressed stream if writeHeader() throws.
• Dropped a redundant setOrigin(x, z) in getBlockOS() — writeHeader() already calls it, and removing it means a failed construction no longer mutates the origin either.

Also included: lazy-init hardening

The six *Zip fields are now volatile and all six getters use check → synchronize → re-check. Four of them (the NBT getters) previously had no locking at all, and getBlockOS/getBiomeOS had an outer check with no re-check inside the lock — so in isolation, 16 concurrent callers each got their own stream (the new getBlockOSReturnsSingleInstanceUnderConcurrency test shows exactly that against pre-fix code: expected: <1> but was: <16>).

This is hardening, not a live bug fix. See the correction below.

Verification

:worldedit-core:test — 183/183 pass, whole module.

The 12 tests in MemoryOptimizedHistoryConcurrencyTest all fail cleanly against pre-fix code and all pass with the fix:

Test	Pre-fix failure
getterFailureLeavesNoPartialStateForFlushOrClose ×6 getters	NullPointerException: Cannot invoke ...flush() because "*StreamZip" is null
getBlockOSClosesCompressedStreamWhenHeaderWriteFails	WantedButNotInvoked: faweOutputStream.close()
getBlockOSReturnsSingleInstanceUnderConcurrency ×5	expected: <1> but was: <16>

This replaces the previous test suite, which had two problems the review caught: it repeated a 16-thread race detector across all six getters (~1900 threads per CI run), and its pre-fix failure mode was heap exhaustion crashing the test JVM, not a clean assertion. Both are fixed — one race test remains, and every failure above is a clean assertion.

testRuntimeOnly(libs.lz4Java) is still needed: lz4-java is compileOnly for main, so the test JVM otherwise hits NoClassDefFoundError: LZ4BlockOutputStream via getCompressedOS().

Correction

The original description claimed the DCL race could silently discard recorded changes in production. That is not reachable, and the claim should not have shipped:

• Every production caller reaches these getters via FaweStreamChangeSet.add/addTile*/addEntity*/addBiomeChange, all of which are only invoked from AbstractChangeSet.processSet(...) — which is public final synchronized on the same instance the getters lock. The unguarded fast-path read is therefore a reentrant read already under the monitor.
• Draining is additionally serialised by workerSemaphore (1 permit).
• The alternative HistoryExtent path is unsynchronised but EditSessionBuilder makes the two paths mutually exclusive (combineStages → processor; else → HistoryExtent), and that path is driven by a single EditSession thread.

The volatile/DCL work is kept because the getters should be correct in isolation rather than by accident of a caller's lock, and the class javadoc now states that contract explicitly. But it fixes a latent footgun, not an observed bug.

Notes

• Rebased onto current main (was based on 995c825da, before the Gradle deprecation fixes in Address gradle deprecations #3592).
• Not rebased on any sibling PR; mergeable independently.
• Building inside a git worktree additionally needs the Grgit fix from Add baseline benchmark for history write path (pre Phase 1 concurrency fixes) #3593; deliberately not included.
• Pre-existing, not addressed here — worth a follow-up: the getters never consult AbstractChangeSet.closed, so a late write task after close() constructs a fresh buffer that ids will never include, silently losing that history.

🤖 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.

[dordsor21]

Same here, parent functions/classes ensure the writes are single-threaded. Adding extra complexity would slow down history (which is already slow/a limiting factor for large edits)

[Copilot]

Pull request overview

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