[dordsor21]

• also fix error when editing newly generated chunks due to postprocessgen not having been completed
• fixes Plugin "stopping" to work. #3490

[PierreSchwang]

Untested, but looks reasonable so far

[SirYwell]

There shouldn't be any obfuscation anymore, right?

[dordsor21]

No idea, I was just going along with everything... If it's been removed we can clean up a bunch of this class I guess? (This field is both paper and spigot fwiw)

[MattBDev]

Code review

Found 2 issues. Both are single-adapter divergences — a change applied correctly to 7 of the 8 per-version copies, missed in the 8th.

1. Lock-order inversion in adapter-26.2 only. internalCall now enters synchronized (this) before taking sectionLock.writeLock(), but trim(true) in the same file takes sectionLock.writeLock() first and then synchronized (this). That is an AB-BA inversion between an edit thread and cache trimming — the exact interaction this PR is meant to fix. The base revision had no synchronized (this) here at all, so the PR introduces it.

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-26.2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_2/PaperweightGetBlocks.java

Lines 536 to 540 in 45c46ae

	try {
	synchronized (this) {
	sectionLock.writeLock().lock();
	if (this.getChunk() != nmsChunk) {
	this.levelChunk = nmsChunk;

Opposite order in trim(boolean aggressive) in the same file:

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-26.2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_2/PaperweightGetBlocks.java

Lines 1051 to 1056 in 45c46ae

	if (aggressive) {
	sectionLock.writeLock().lock();
	try {
	synchronized (this) {
	skyLight = new DataLayer[getSectionCount()];
	blockLight = new DataLayer[getSectionCount()];

The other seven adapters lock in the order that matches their own trim(), e.g. adapter-26.1:

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-26.1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_1/PaperweightGetBlocks.java

Lines 534 to 538 in 45c46ae

	try {
	sectionLock.writeLock().lock();
	synchronized (this) {
	if (this.getChunk() != nmsChunk) {
	this.levelChunk = nmsChunk;

2. adapter-1_21 is missing the boolean createCopy = this.createCopy; snapshot that the other seven adapters gained in this PR. copy is decided once from the field, but the deferred syncTasks lambda re-reads the live, non-volatile AbstractBukkitGetBlocks.createCopy when it later runs on the sync thread. If setCreateCopy flips the field in between, createCopy reads true while copy is still null, NPE-ing on copy.storeEntity(entity).

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-1_21/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_R1/PaperweightGetBlocks.java

Lines 339 to 343 in 45c46ae

	) throws Exception {
	Map<BlockPos, CompoundTag> tilesToInit = PaperweightPlatformAdapter.clearPostProcessing(nmsChunk, false);
	PaperweightGetBlocks_Copy copy = createCopy ? new PaperweightGetBlocks_Copy(nmsChunk) : null;
	if (createCopy) {
	if (copies.containsKey(copyKey)) {

Deferred read of the live field:

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-1_21/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_R1/PaperweightGetBlocks.java

Lines 632 to 636 in 45c46ae

	if (entityRemoves.contains(uuid)) {
	if (createCopy) {
	copy.storeEntity(entity);
	}
	removeEntity(entity);

Compare adapter-1_21_11, which has the snapshot:

FastAsyncWorldEdit/worldedit-bukkit/adapters/adapter-1_21_11/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_11/PaperweightGetBlocks.java

Lines 336 to 338 in 45c46ae

	Map<BlockPos, CompoundTag> tilesToInit = PaperweightPlatformAdapter.clearPostProcessing(nmsChunk, false);
	boolean createCopy = this.createCopy;
	PaperweightGetBlocks_Copy copy = createCopy ? new PaperweightGetBlocks_Copy(nmsChunk) : null;

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[Copilot]

Pull request overview

This PR focuses on improving concurrency/locking behavior during chunk “call” execution and trimming, and adds handling for editing chunks that are newly generated but have not completed post-processing (pending block entity initialization), addressing issue #3490.

Changes:

• Adjust chunk GET call synchronization/visibility behavior in the shared Bukkit adapter base.
• Add logic across multiple Paperweight adapters to temporarily manage pendingBlockEntities during edits when post-processing hasn’t completed.
• Refactor sync-task construction and trim logic in multiple versioned adapters.

Reviewed changes

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

Show a summary per file

File	Description
worldedit-bukkit/src/main/java/com/fastasyncworldedit/bukkit/adapter/AbstractBukkitGetBlocks.java	Removes synchronized from call() (relying on callLock) and makes forceLoadSections volatile for cross-thread visibility.
worldedit-bukkit/adapters/adapter-26.2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_2/PaperweightPlatformAdapter.java	Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-26.2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_2/PaperweightGetBlocks.java	Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-26.1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_1/PaperweightPlatformAdapter.java	Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-26.1/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_1/PaperweightGetBlocks.java	Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-1_21/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_R1/PaperweightPlatformAdapter.java	Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_R1/PaperweightGetBlocks.java	Uses new post-processing helpers; refactors sync task construction and trim logic.
worldedit-bukkit/adapters/adapter-1_21_9/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_9/PaperweightPlatformAdapter.java	Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21_9/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_9/PaperweightGetBlocks.java	Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-1_21_6/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_6/PaperweightPlatformAdapter.java	Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21_6/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_6/PaperweightGetBlocks.java	Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-1_21_5/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_5/PaperweightPlatformAdapter.java	Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21_5/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_5/PaperweightGetBlocks.java	Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-1_21_4/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_4/PaperweightPlatformAdapter.java	Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21_4/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_4/PaperweightGetBlocks.java	Uses new post-processing helpers; refactors sync task construction; adjusts trimming flow.
worldedit-bukkit/adapters/adapter-1_21_11/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_11/PaperweightPlatformAdapter.java	Adds reflective accessors to clear/restore pendingBlockEntities for post-processing handling.
worldedit-bukkit/adapters/adapter-1_21_11/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_11/PaperweightGetBlocks.java	Uses new post-processing helpers; refactors sync task construction; includes corrected trim section-index mapping.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

Please take a moment and address the merge conflicts of your pull request. Thanks!

[MattBDev]

I think the loop bound here might be off by one. forcedSections, super.blocks and trim all look like they're sectionCount long, so with <= the last pass would be i == sectionCount, and I think forcedSections[i] would throw an AAOBE there. . The second loop further down (i < trim.length) already does it that way, which is partly why I noticed.

Suggested change

	for (int i = 0; i <= getSectionCount(); i++) {
	for (int i = 0; i < getSectionCount(); i++) {

I think the same line is in all 8 adapters, so if this is right, they'd all need it. As far as I can tell it only gets hit on a non-aggressive trim when sections is already loaded (like ChunkHolder.getOrCreateGet() reusing a cached get).
Let me know if this is right because sometimes, I can get a little overwhelmed by all of the arrays and misread something.

[dordsor21]

Max section position should be inclusive from what I can tell

[MattBDev]

Agreed that the max section position is inclusive, that's why the old loop (getMinSectionPosition()..getMaxSectionPosition() with <=) was fine. The new loop runs over array indices though, not positions. getSectionCount() is max - min + 1 (CharBlocks constructor), so the inclusive max position is index getSectionCount() - 1.

For a -64..319 world that's positions -4..19, 24 sections, indices 0..23. With i <= getSectionCount() the last pass is i == 24, so forcedSections[24] throws.

I reviewed it with Claude and it produced this test. I know AI isn't always the best at these things so I provided the test so you can run for yourself. On the current head (a36901c) it fails with:

java.lang.ArrayIndexOutOfBoundsException: Index 24 out of bounds for length 24
    at com.sk89q.worldedit.bukkit.adapter.impl.fawe.v26_3.PaperweightGetBlocks.trim(PaperweightGetBlocks.java:1066)

and passes with i < getSectionCount(). It only needs sections to already be loaded, which is the case when ChunkHolder.getOrCreateGet() reuses a cached get and calls trim(false). The same line is in all 9 adapters.

PaperweightGetBlocksTrimTest (adapter-26.3)

package com.sk89q.worldedit.bukkit.adapter.impl.fawe.v26_3;

import com.fastasyncworldedit.bukkit.adapter.AbstractBukkitGetBlocks;
import com.fastasyncworldedit.core.queue.implementation.blocks.CharBlocks;
import com.sk89q.worldedit.bukkit.WorldEditPlugin;
import net.minecraft.world.level.chunk.LevelChunkSection;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.mockito.Answers;
import org.mockito.MockedStatic;

import java.lang.reflect.Field;
import java.util.concurrent.locks.ReentrantReadWriteLock;

import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.when;
import static org.mockito.Mockito.withSettings;

class PaperweightGetBlocksTrimTest {

    // Overworld, y -64..319: section positions -4..19, 24 sections, array indices 0..23
    private static final int MIN_SECTION = -4;
    private static final int MAX_SECTION = 19;

    @BeforeAll
    static void initGetBlocksClass() throws ClassNotFoundException {
        // PaperweightGetBlocks.NMS_TO_TILE is read from the running plugin in a static initializer
        try (MockedStatic<WorldEditPlugin> plugin = mockStatic(WorldEditPlugin.class)) {
            WorldEditPlugin instance = mock(WorldEditPlugin.class);
            when(instance.getBukkitImplAdapter()).thenReturn(mock(PaperweightFaweAdapter.class));
            plugin.when(WorldEditPlugin::getInstance).thenReturn(instance);
            Class.forName(PaperweightGetBlocks.class.getName(), true, PaperweightGetBlocksTrimTest.class.getClassLoader());
        }
    }

    /**
     * A non-aggressive trim on a get whose sections are already loaded (for example one reused from the per-world
     * ChunkCache by ChunkHolder#getOrCreateGet) must stay within the section arrays.
     */
    @Test
    void nonAggressiveTrimWithLoadedSectionsStaysInBounds() throws ReflectiveOperationException {
        PaperweightGetBlocks getBlocks = getBlocksWithLoadedSections();

        boolean trimmed = assertDoesNotThrow(() -> getBlocks.trim(false));
        assertTrue(trimmed);
    }

    /**
     * Builds the get without its constructor, which needs a live ServerLevel, and sets only the state trim(false)
     * reads. The Mockito instance runs every real method; it is only used to skip the constructor. Every section is
     * null, so the loop takes its {@code continue} branch and never touches NMS palettes.
     */
    private static PaperweightGetBlocks getBlocksWithLoadedSections() throws ReflectiveOperationException {
        int sectionCount = MAX_SECTION - MIN_SECTION + 1;
        PaperweightGetBlocks getBlocks = mock(
                PaperweightGetBlocks.class,
                withSettings().defaultAnswer(Answers.CALLS_REAL_METHODS)
        );

        Object[] sectionLocks = new Object[sectionCount];
        for (int i = 0; i < sectionCount; i++) {
            sectionLocks[i] = new Object();
        }
        setField(CharBlocks.class, getBlocks, "minSectionPosition", MIN_SECTION);
        setField(CharBlocks.class, getBlocks, "maxSectionPosition", MAX_SECTION);
        setField(CharBlocks.class, getBlocks, "sectionCount", sectionCount);
        setField(CharBlocks.class, getBlocks, "blocks", new char[sectionCount][]);
        setField(CharBlocks.class, getBlocks, "sectionLocks", sectionLocks);

        setField(PaperweightGetBlocks.class, getBlocks, "sectionLock", new ReentrantReadWriteLock());
        setField(PaperweightGetBlocks.class, getBlocks, "sections", new LevelChunkSection[sectionCount]);
        // As during a concurrent call(): getSections(true) returns the cached array instead of re-reading the
        // LevelChunk. Outside call() it re-reads LevelChunk#getSections(), which has the same length.
        setField(AbstractBukkitGetBlocks.class, getBlocks, "forceLoadSections", false);
        return getBlocks;
    }

    private static void setField(Class<?> owner, Object target, String name, Object value)
            throws ReflectiveOperationException {
        Field field = owner.getDeclaredField(name);
        field.setAccessible(true);
        field.set(target, value);
    }

}

Run with ./gradlew :worldedit-bukkit:adapters:adapter-26.3:test --tests "*PaperweightGetBlocksTrimTest".

[dordsor21]

AHH yes you're right, I guess I just didn't pay any attention to the code snippet

[NotMyFault]

Unresolved merge conflict, but works, thanks!

[Copilot]

🟡 Changes recommended

Critical issues remain around chunk selection, pending block-entity synchronization, and adapter correctness.

19 open findings

• Use the processed chunk instead of mutable cached levelChunk · New
• Unsynchronized pending block entity map access causes races · New
• Use the processed chunk instead of mutable cached levelChunk · New
• Unsynchronized pending block entity map access causes races · New
• Use the processed chunk instead of mutable cached levelChunk · New
• Unsynchronized pending block entity map access causes races · New
• Use the processed chunk instead of mutable cached levelChunk · New
• Unsynchronized pending block entity map access causes races · New
• Use the processed chunk instead of mutable cached levelChunk · New
• Unsynchronized pending block entity map access causes races · New
• Use the processed chunk instead of mutable cached levelChunk · New
• Unsynchronized pending block entity map access causes races · New
• Use the processed chunk instead of mutable cached levelChunk · New
• Unsynchronized pending block entity map access causes races · New
• Use nmsWorld for palettedContainerFactory calls · New
• Use the processed chunk instead of mutable cached levelChunk · New
• Unsynchronized pending block entity map access causes races · New
• Use the processed chunk instead of mutable cached levelChunk · New
• Unsynchronized pending block entity map access causes races · New

7 resolved since last review

• In this trim() refactor, the post-loop call to super.trim(false, i) now passes the 0-based array…
• In this trim() refactor, the post-loop call to super.trim(false, i) now passes the 0-based array…
• In this trim() refactor, the post-loop call to super.trim(false, i) now passes the 0-based array…
• In this trim() refactor, the post-loop call to super.trim(false, i) now passes the 0-based array…
• In this trim() refactor, the post-loop call to super.trim(false, i) now passes the 0-based array…
• In this trim() refactor, the post-loop call to super.trim(false, i) now passes the 0-based array…
• In this trim() refactor, the post-loop call to super.trim(false, i) now passes the 0-based array…

🧠 Review effort: Lite

[github-actions]

Please take a moment and address the merge conflicts of your pull request. Thanks!