Fix lost-wakeup race and non-atomic close() in AbstractChangeSet by MattBDev · Pull Request #3597 · IntellectualSites/FastAsyncWorldEdit

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

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

The bugs

Lost wakeup. drainQueue() polled until the queue was empty and only then released its permit. A producer that enqueued in the window between the final poll() and the release() saw availablePermits() == 0 in triggerWorker() and skipped scheduling a worker. The task then sat in the queue with nothing to drain it, and the CompletableFuture returned by addWriteTask() / postProcessSet() never completed.
Permit released without being acquired. When the blocking acquire() path was interrupted, the InterruptedException was caught but execution fell through into the drain and its finally { release(); }. That pushes the semaphore above its single permit and lets two workers drain concurrently, breaking the single-drainer guarantee processSet() relies on. (Found while fixing Adding gradle #1 — same method, same invariant.)
Non-atomic close(). A check-then-act on closed let two callers both observe false and both run flush().

The fix (revised after code review — see commits `4e3489e` and `bda8f63`)

Re-check the queue after releasing the permit and schedule a worker for anything left behind.
close() synchronizes on a dedicated private closeLock, not a CAS. The original CAS guaranteed a single flush but let a losing caller return from close() before the winner finished flushing — violating Closeable semantics for callers like RollbackOptimizedHistory.close(), which logs to the rollback DB immediately after super.close(). Synchronizing on this instead would fix that but risks an AB-BA deadlock: flush() can block on the drain semaphore while the thread holding it runs a queued write task that itself synchronizes on this (subclasses' lazy stream getters do). A dedicated lock object that nothing else in the hierarchy touches gives both properties: losing callers block until the winner finishes, with no possible lock-ordering cycle.
drainQueue(true)'s blocking wait is now uninterruptible (acquireUninterruptibly()), not interruptible-with-early-return. An interrupted closer used to return early with the queue still undrained while close() marked the changeset closed anyway — silent history loss, since flush() swallows exceptions and nothing surfaced. The interrupt flag is preserved for the caller to observe afterwards.

Verification

:worldedit-core:compileJava / compileTestJava pass.
AbstractChangeSetConcurrencyTest: 3/3 pass with the fix.
The original two tests (lost-wakeup stress test, concurrent-close stress test) also pass against pre-fix code, so they do not reproduce those races — the windows are narrow interleavings a stress test doesn't reliably hit. Treat them as smoke tests, not proof. This is stated explicitly in the test's class javadoc.
interruptedCloseStillDrainsQueue (added in bda8f63) is a deterministic regression test for the interrupt hole: it fails against the pre-fix code with close() returned before pending write tasks were drained, and passes with the fix, including asserting the interrupt flag survives.

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).
Requesting a second reviewer specifically on 4e3489e and bda8f63 before merge — see the PR comment below for what to double-check (deadlock reasoning across subclasses I didn't audit, and whether any caller actually relies on close() being interruptible).

🤖 Generated with Claude Code

drainQueue() polled the queue until empty and only then released its permit. A producer that enqueued a task in the window between the final poll() and the release() saw availablePermits() == 0 in triggerWorker() and skipped scheduling a worker, so the task sat in the queue with nothing left to drain it and the CompletableFuture returned by addWriteTask()/postProcessSet() never completed. Re-check the queue after releasing the permit and schedule a worker for anything left behind. drainQueue() also released a permit it had never acquired when the blocking acquire() path was interrupted: the InterruptedException was caught but execution fell through into the drain and its finally block. That pushed the semaphore above its single permit and allowed two workers to drain concurrently, breaking the single-drainer guarantee processSet() relies on. Return instead, without draining or releasing. close() used a check-then-act on `closed`, letting two callers both observe false and both run flush(). Guard the sequence with a CAS. The guard is a separate field so `closed` keeps its existing type, visibility and semantics for subclasses that read and write it directly (e.g. RollbackOptimizedHistory). Adds AbstractChangeSetConcurrencyTest covering both races. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

This PR hardens AbstractChangeSet concurrency behavior in com.fastasyncworldedit.core.history by addressing a lost-wakeup risk in the write-worker drain loop, fixing an interrupt path that could violate the single-drainer invariant, and making close() safe under concurrent callers.

Changes:

Reworks drainQueue(...) semaphore acquisition logic to avoid releasing a permit that was never acquired and to reschedule draining if tasks arrive during the final poll/release window.
Adds a CAS guard around close() to prevent concurrent callers from executing flush() multiple times.
Adds a new concurrency-focused unit test covering contention on addWriteTask(...) and concurrent close() calls.

Reviewed changes

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

File	Description
`worldedit-core/src/main/java/com/fastasyncworldedit/core/history/changeset/AbstractChangeSet.java`	Fixes the queue-drainer scheduling race, interrupt handling during permit acquisition, and attempts to make `close()` atomic under contention.
`worldedit-core/src/test/java/com/fastasyncworldedit/core/history/changeset/AbstractChangeSetConcurrencyTest.java`	Adds a multithreaded smoke test for queue draining and concurrent close behavior using thread-scoped static mocking.

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

MattBDev · 2026-07-15T11:50:01Z

    public void close() throws IOException {
-        if (!closed) {
-            flush();
-            closed = true;
+        if (closed) {
+            return;
+        }
+        // Ensure only one concurrent caller runs the flush-and-mark-closed sequence; the CAS
+        // atomically decides a single "winner" thread, so no two threads can ever both pass the
+        // `!closed` check and both call flush() as could happen with the old check-then-act code.
+        // Callers that lose the race simply return; `closed` is still guaranteed to become true.
+        if (closing.compareAndSet(false, true)) {
+            try {
+                flush();
+            } finally {
+                closed = true;
+            }
        }
    }


Fixed in 4e3489e: close() now synchronizes on a dedicated private lock instead of a CAS, so a caller that loses the race blocks until the winning flush completes rather than returning early. A dedicated lock (not this) avoids the AB-BA deadlock with queued write tasks that synchronize on this. See the PR comment requesting a second reviewer specifically on this design.

MattBDev · 2026-07-15T11:50:21Z

+        // The core assertion: none of these futures should hang. On the unfixed code this
+        // occasionally times out under enough contention because a task got stranded in the
+        // queue with no worker left to drain it.


Fixed in bda8f63: the comment no longer claims the unfixed code occasionally times out here. It now states plainly that this stress test does not reliably hit the lost-wakeup interleaving and serves as a contention smoke test.

…ix + deadlock risk) The previous fix used an AtomicBoolean CAS to ensure only one caller runs flush(), but a losing caller returned from close() immediately rather than waiting for the winner - so close() could return before the changeset was actually flushed. Any caller assuming close()'s postcondition ("fully flushed") on the losing thread would be wrong. Synchronizing close() on `this` instead (the obvious fix for that) was rejected: flush() -> drainQueue(true) can block in workerSemaphore.acquire() waiting for another thread's in-flight drain to finish, and that other thread may be running a queued write task that itself synchronizes on `this` (e.g. DiskStorageHistory#getBlockOS). If this thread already held `this`'s monitor while waiting on the semaphore, and the other thread needed `this`'s monitor to make progress and release the semaphore, that's an AB-BA deadlock. A private lock object that nothing else in the class hierarchy ever synchronizes on gives both properties without that risk: losing callers block until the winner's synchronized block exits (at which point `closed` is already true, so they return immediately without re-running flush()), and there is no path by which this lock can participate in a cycle with `this`'s monitor or workerSemaphore. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

MattBDev · 2026-07-15T06:56:20Z

Pushed 4e3489e, which replaces the close() fix in the initial commit with a different approach. This specific commit needs a second reviewer's sign-off before merge - not the rest of the PR, just this change - for two reasons:

The original fix (CAS-based) was incomplete. A caller of close() that lost the CAS race returned immediately rather than waiting for the winning thread's flush() to finish. Anything relying on close()'s implicit postcondition ("fully flushed once this returns") - e.g. RollbackOptimizedHistory.close() calling db.logEdit(this) right after super.close() - could act on a not-yet-flushed changeset if two threads ever call close() on the same instance concurrently. I found this on a second read of my own fix, not from any automated check.
The obvious replacement (make close() synchronized) has a real deadlock risk I almost shipped. Subclasses like DiskStorageHistory and MemoryOptimizedHistory synchronize their lazy stream getters on this. If close() also synchronized on this, a thread blocked inside flush() -> drainQueue(true)'s workerSemaphore.acquire() (waiting for another thread's in-flight drain) could deadlock against that other thread trying to enter a synchronized(this) getter for a queued write task - classic AB-BA. I only caught this by tracing through the actual lock-acquisition order across AbstractChangeSet and its subclasses; it isn't something a test would easily surface, since it requires a specific interleaving under real contention.

The fix landed instead uses a dedicated, never-shared lock object (closeLock) instead of this, which gives correct blocking semantics for losing callers without touching any lock that subclasses or drainQueue/workerSemaphore ever wait on. The reasoning is in the method's javadoc.

Why this needs another set of eyes specifically: this is exactly the class of bug (lock-ordering across a class hierarchy, under contention that's hard to reproduce in a unit test) where a single reviewer - including the one who wrote the fix - is the weakest link. The existing AbstractChangeSetConcurrencyTest does not and cannot prove either the completeness fix or the deadlock-avoidance reasoning; it passes identically against origin/main with no fix at all (same disclosed limitation as the original PR description). This change is correct by construction/reasoning, not by test evidence, which is precisely when an independent review matters most.

Copilot

Pull request overview

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

drainQueue(true)'s interruptible acquire() created a hole: a closer thread interrupted while waiting for an in-flight drainer returned early, flush() surfaced nothing, and close() marked the changeset closed with write tasks still queued - silently losing history. RollbackOptimizedHistory.close() would then log the edit to the rollback database while its writes were unflushed. Use acquireUninterruptibly() instead: flush()'s contract (and close()'s, which flushes before setting `closed`) is that pending tasks are drained on return, so an interrupt must not abort the wait. The interrupt status is preserved for callers to observe afterwards, and the wait is bounded by the concurrent drainer's finite queue - the same bound the interruptible acquire had. Adds interruptedCloseStillDrainsQueue, a deterministic regression test: against the previous code it fails immediately ("close() returned before pending write tasks were drained"); with the fix, close() blocks until the drainer is released and the interrupt flag survives. Also rewords the test-class javadoc to stop claiming the stress test reproduces the lost-wakeup race (it does not - it is a contention smoke test; the PR description already discloses this) and fixes a stale "CAS guard" reference from before the closeLock rework. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

MattBDev · 2026-07-15T11:47:33Z

Request for a second reviewer on the two follow-up commits (4e3489e and bda8f63) before approving/merging. They rework the synchronization design of close()/drainQueue rather than just patching a line, and concurrency changes of this shape are exactly where a fresh set of eyes pays off:

4e3489e — close() now synchronizes on a dedicated private closeLock. The original PR used an AtomicBoolean CAS, which guaranteed a single flush but let a losing caller return from close() before the winner finished flushing — violating Closeable semantics for callers like RollbackOptimizedHistory.close(), which logs to the rollback DB immediately after super.close(). Synchronizing on this instead would have fixed that but risks AB-BA deadlock: flush() can block on the drain semaphore while the thread holding it runs a queued write task that takes this (the subclasses' lazy stream getters synchronize on this). The dedicated lock gives both properties — losers block until the winner finishes, and the lock cannot participate in a cycle with this or the semaphore. What to double-check: that no current or future subclass synchronizes close() against this in a way that reintroduces an ordering with closeLock, and that the deadlock reasoning in the close() javadoc holds for the platform-specific subclasses I did not audit (worldedit-bukkit adapters).
bda8f63 — drainQueue(true) now waits uninterruptibly. The interruptible acquire let an interrupted closer return early with the queue undrained while close() still marked the changeset closed — silent history loss (flush() swallows exceptions, so nothing surfaced). acquireUninterruptibly() preserves the interrupt flag for callers and is bounded by the concurrent drainer's finite queue. What to double-check: whether any caller of flush()/close() genuinely needs interruption to abort the wait (e.g. server shutdown watchdogs) — if a watchdog kills a hung drainer thread, the closer now waits forever where it previously returned early; I judged silent data loss to be the worse failure mode, but that's a judgment call a maintainer should confirm.

The new interruptedCloseStillDrainsQueue test is deterministic in both directions (fails on the old code, passes on the new), but the lost-wakeup stress test remains a smoke test only — see the updated class javadoc.

MattBDev · 2026-07-15T11:49:51Z

Request for a second reviewer on the two follow-up commits (4e3489e and bda8f63) before approving/merging. They rework the synchronization design of close()/drainQueue rather than just patching a line, and concurrency changes of this shape are exactly where a fresh set of eyes pays off:

4e3489e - close() now synchronizes on a dedicated private closeLock. The original PR used an AtomicBoolean CAS, which guaranteed a single flush but let a losing caller return from close() before the winner finished flushing - violating Closeable semantics for callers like RollbackOptimizedHistory.close(), which logs to the rollback DB immediately after super.close(). Synchronizing on this instead would have fixed that but risks AB-BA deadlock: flush() can block on the drain semaphore while the thread holding it runs a queued write task that takes this (the subclasses' lazy stream getters synchronize on this). The dedicated lock gives both properties - losers block until the winner finishes, and the lock cannot participate in a cycle with this or the semaphore. What to double-check: that no current or future subclass synchronizes close() against this in a way that reintroduces an ordering with closeLock, and that the deadlock reasoning in the close() javadoc holds for the platform-specific subclasses I did not audit (worldedit-bukkit adapters).
bda8f63 - drainQueue(true) now waits uninterruptibly. The interruptible acquire let an interrupted closer return early with the queue undrained while close() still marked the changeset closed - silent history loss (flush() swallows exceptions, so nothing surfaced). acquireUninterruptibly() preserves the interrupt flag for callers and is bounded by the concurrent drainer's finite queue. What to double-check: whether any caller of flush()/close() genuinely needs interruption to abort the wait (e.g. server shutdown watchdogs) - if a watchdog kills a hung drainer thread, the closer now waits forever where it previously returned early; I judged silent data loss to be the worse failure mode, but that's a judgment call a maintainer should confirm.

The new interruptedCloseStillDrainsQueue test is deterministic in both directions (fails on the old code, passes on the new), but the lost-wakeup stress test remains a smoke test only - see the updated class javadoc.

Copilot

Pull request overview

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

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

Copilot

Pull request overview

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

MattBDev · 2026-07-15T13:02:25Z

+        if (!queue.isEmpty()) {
+            triggerWorker();
+        }


Confirmed and fixed in 76ba0ce - you're right, the async re-trigger reopened exactly the completeness gap the earlier fixes were meant to close, just for the last straggler task. drainQueue(true) now loops synchronously (acquire, drain, release, re-check) until a release is immediately followed by an empty queue, instead of delegating to triggerWorker(). Added a stress test for the actual contract (isDone() immediately after close() returns, no timeout tolerance) - ran it 5x against the pre-fix code and it didn't reproduce the race either, so it's a smoke test like its sibling, not a guaranteed repro; the fix itself is what closes the gap.

A subtler version of the completeness gap the closeLock/acquireUninterruptibly fixes already addressed: after draining and releasing the permit, if a straggler task had landed in the queue during the (last poll()==null ... release()) window, the old code called triggerWorker() and returned - which schedules an ASYNC worker on another thread and does not wait for it. flush() (and therefore close(), which flushes before setting `closed`) could then return while that worker was still about to run, on a resource the caller now considers closed - e.g. RollbackOptimizedHistory.close() logging the edit to the rollback database before the straggler write actually landed. drainQueue(true) (the blocking path used by flush()/close()) now loops: acquire, drain, release, and if the queue is non-empty immediately after release, loop back and drain again, rather than delegating to an async re-trigger. It only returns once a release() is immediately followed by an empty queue. The non-blocking path (an async worker triggered via triggerWorker()) is unchanged: it still just re-triggers another async worker for anything left behind and returns promptly, since it never needed a synchronous-drain guarantee. Adds closeDoesNotReturnWithAnyPreviouslyEnqueuedTaskStillPending, checking the actual contract at stake: every future submitted before close() is called must be isDone() immediately after close() returns, not merely eventually. Ran it 5 times against the pre-fix drainQueue and it did not reproduce the race (the window is narrow, same as the existing lost-wakeup test) - it's a contention smoke test for the completeness contract, not a deterministic reproduction, and says so in its own javadoc. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot

Pull request overview

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

+    private ExecutorService newFaweAwareFixedThreadPool(int threads, Fawe faweMock) {
+        ThreadFactory factory = runnable -> new Thread(() -> {
+            try (MockedStatic<Fawe> faweStatic = Mockito.mockStatic(Fawe.class)) {
+                faweStatic.when(Fawe::instance).thenReturn(faweMock);
+                runnable.run();
+            }
+        });


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

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

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