Fix block count leaks and parallelize recount chunk loading

[daniel-skopek]

Bug 1: BlockSpreadEvent never decremented the old block count
When BlockSpreadEvent fired (grass spreading to dirt, fire spreading, etc.), the handler only incremented the new block's count — it never decremented the old block that was replaced. Over time, this permanently inflated counts for any material that could be overwritten by spread events. The fix follows the same pattern as BlockFormEvent: decrement the old block, attempt to add the new (with limit check), and if the limit is hit, cancel the event and revert.

Bug 2: BlockGrowEvent leaked a count when the event was cancelled
In the BlockGrowEvent handler, the new state's count was incremented before the limit check. If the check determined the limit was hit and cancelled the event, the increment was never rolled back — each failed growth attempt permanently added +1 to the count. The fix adds a process(newBlock, false) call after cancellation to revert the premature increment.

Bug 3: RecountCalculator loaded chunks sequentially, causing timeout on large islands
loadChunks() loaded chunks one at a time via recursive async chaining — 100 chunks × 3 environments = 300 sequential loads per iteration. For larger islands this easily exceeded the 5-minute CALCULATION_TIMEOUT, at which point tidyUp() was never called and the recount produced no results at all. The fix loads all chunks within an environment in parallel (CompletableFuture.allOf), and also loads all three environments in parallel, while keeping the actual block scanning sequential.

Bug 4: RecountCalculator could create an IslandBlockCount without permission limits
The constructor used the nullable getIsland(String) overload. If the island wasn't in the in-memory map, tidyUp() created a brand-new empty IslandBlockCount — losing all permission-based limits, offsets, and any limits set via JoinListener. The fix switches both the constructor and tidyUp() to the @nonnull getIsland(Island) overload (computeIfAbsent), which ensures limits are preserved from the existing map entry.

[Copilot]

Pull request overview

Fixes multiple block-limit accounting edge cases in event listeners and improves recount performance by parallelizing chunk loading, while also ensuring recounts always operate on the canonical in-memory IslandBlockCount (preserving permission-derived limits/offsets).

Changes:

• Fix BlockSpreadEvent handling to decrement the replaced block’s count and only apply the new block count when within limits (restore on cancel).
• Adjust BlockGrowEvent cancellation path and update tests to provide BlockData for spread events.
• Parallelize RecountCalculator chunk loading across chunks and environments; ensure IslandBlockCount is retrieved via the non-null getIsland(Island) path in constructor and tidyUp().

Reviewed changes

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

File	Description
src/test/java/world/bentobox/limits/listeners/BlockLimitsListenerTest.java	Updates the BlockSpreadEvent test setup to supply BlockData for the new listener logic.
src/main/java/world/bentobox/limits/listeners/BlockLimitsListener.java	Fixes BlockSpreadEvent accounting by decrementing old material and conditionally adding new material with rollback on cancel; adjusts BlockGrowEvent cancel handling.
src/main/java/world/bentobox/limits/calculators/RecountCalculator.java	Parallelizes chunk loading and ensures recount writes back into the canonical IslandBlockCount entry (preserving limits/offsets).

---

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

[tastybento]

Thanks for this, Daniel — genuinely nice set of catches. I pulled the branch locally and ran the full suite, then dug into each of the four fixes. Quick summary:

• Bug 1 (BlockSpreadEvent) ✅ Correct. It mirrors the BlockFormEvent pattern exactly (decrement old → try-add new → revert old on limit). I wrote two tests to lock it in (below).
• Bug 3 (parallel chunk loading) ✅ Good improvement and clearly addresses the timeout. One non-blocking note inline about concurrency.
• Bug 4 (NonNull getIsland) ✅ Good catch — preserving perm limits/offsets is the right call. One minor inline note.
• Bug 2 (BlockGrowEvent) ⚠️ This is the one I'd hold on — I think the added decrement introduces a regression rather than fixing a leak. Details + a reproducing test inline.

Rather than bounce this back with a "change this", here are the tests I added while reviewing (all green for bugs 1/3/4; the grow one fails red against the current bug-2 change and passes once the one-line tweak below is applied). Happy to push these onto your branch or land them in a follow-up — whichever you prefer. Everything except the one bug-2 line is mergeable as-is. 🙂

@Test
public void testBlockSpreadDecrementsOldAddsNew() {
    // Grass spreading onto dirt: DIRT count should drop, GRASS_BLOCK count should rise.
    IslandBlockCount ibc = new IslandBlockCount("test-island-id", "BSkyBlock");
    ibc.add(Environment.NORMAL, Material.DIRT.getKey());
    listener.setIsland("test-island-id", ibc);

    Block block = mockBlock(Material.DIRT, blockLocation);
    Block source = mockBlock(Material.GRASS_BLOCK, new Location(world, 101, 65, 100));
    BlockState newState = mock(BlockState.class);
    BlockData newBlockData = mock(BlockData.class);
    when(newBlockData.getMaterial()).thenReturn(Material.GRASS_BLOCK);
    when(newState.getBlockData()).thenReturn(newBlockData);
    BlockSpreadEvent event = new BlockSpreadEvent(block, source, newState);

    listener.onBlock(event);

    assertFalse(event.isCancelled());
    assertEquals(0, ibc.getBlockCount(Material.DIRT.getKey()));
    assertEquals(1, ibc.getBlockCount(Material.GRASS_BLOCK.getKey()));
}

@Test
public void testBlockSpreadAtLimitCancelsAndRestoresOld() {
    // GRASS_BLOCK is at limit; spread onto DIRT must be cancelled and the DIRT count preserved.
    IslandBlockCount ibc = new IslandBlockCount("test-island-id", "BSkyBlock");
    ibc.add(Environment.NORMAL, Material.DIRT.getKey());
    ibc.setBlockLimit(Environment.NORMAL, Material.GRASS_BLOCK.getKey(), 1);
    ibc.add(Environment.NORMAL, Material.GRASS_BLOCK.getKey());
    listener.setIsland("test-island-id", ibc);

    Block block = mockBlock(Material.DIRT, blockLocation);
    Block source = mockBlock(Material.GRASS_BLOCK, new Location(world, 101, 65, 100));
    BlockState newState = mock(BlockState.class);
    BlockData newBlockData = mock(BlockData.class);
    when(newBlockData.getMaterial()).thenReturn(Material.GRASS_BLOCK);
    when(newState.getBlockData()).thenReturn(newBlockData);
    BlockSpreadEvent event = new BlockSpreadEvent(block, source, newState);

    listener.onBlock(event);

    assertTrue(event.isCancelled());
    assertEquals(1, ibc.getBlockCount(Material.DIRT.getKey()));
    assertEquals(1, ibc.getBlockCount(Material.GRASS_BLOCK.getKey()));
}

@Test
public void testBlockGrowAtNonZeroLimitDoesNotDecrement() {
    // GRASS_BLOCK is at a non-zero limit (5 of 5). A blocked grow must NOT change the count.
    IslandBlockCount ibc = new IslandBlockCount("test-island-id", "BSkyBlock");
    ibc.setBlockLimit(Environment.NORMAL, Material.GRASS_BLOCK.getKey(), 5);
    for (int i = 0; i < 5; i++) {
        ibc.add(Environment.NORMAL, Material.GRASS_BLOCK.getKey());
    }
    listener.setIsland("test-island-id", ibc);

    Block block = mockBlock(Material.DIRT, blockLocation);
    Block newBlock = mockBlock(Material.GRASS_BLOCK, blockLocation);
    BlockState newState = mock(BlockState.class);
    when(newState.getBlock()).thenReturn(newBlock);

    Block worldBlock = mock(Block.class);
    when(world.getBlockAt(blockLocation)).thenReturn(worldBlock);

    BlockGrowEvent event = new BlockGrowEvent(block, newState);
    listener.onBlock(event);

    assertTrue(event.isCancelled());
    // The physical world still has 5 grass blocks, so the count must remain 5.
    assertEquals(5, ibc.getBlockCount(Material.GRASS_BLOCK.getKey()));
}

[tastybento]

⚠️ I think this line introduces a new bug rather than fixing one.

The PR description says the count "was incremented before the limit check", but in the current process(Block, BlockData, boolean) the increment happens after the check and only when the limit is not hit:

if (add) {
    int limit = checkLimit(...);
    if (limit > -1) {
        return limit;          // returns here — no increment happened
    }
    islandCountMap.get(id).add(env, key);   // only runs when limit == -1
}

We only enter this if (process(...) > -1) branch when the limit was hit — which means nothing was incremented. So this extra process(..., false) decrements a count that was never added.

I reproduced it with a test: GRASS_BLOCK at limit 5/5, a blocked grow drops the stored count to 4 while the world still physically has 5 — so the next grow is then wrongly allowed and the count desyncs from reality (expected: <5> but was: <4>).

Suggested fix — just drop this one line; the original cancel branch was already balanced (the increment only ever happens on the success path):

if (process(e.getNewState().getBlock(), true) > -1) {
    e.setCancelled(true);
    e.getBlock().getWorld().getBlockAt(e.getBlock().getLocation()).setBlockData(e.getBlock().getBlockData());
} else {
    process(e.getBlock(), false);
}

With the line removed, all 233 tests pass (including testBlockGrowAtNonZeroLimitDoesNotDecrement from the summary).

[tastybento]

Nice — this is the heart of the timeout fix. 👍

Non-blocking thought for a possible follow-up: this now fires up to CHUNKS_TO_SCAN (100) async chunk loads per environment, and scanNextChunk() kicks off all three environments together — so up to ~300 concurrent getChunkAtAsync requests per scan iteration. On a large/busy server that could be a load spike (chunk gen still hits the main thread). It's a reasonable trade against the timeout that was breaking recounts entirely, but capping the in-flight batch (or making it configurable) might be worth a later look. Not blocking.

[tastybento]

👍 Good catch switching to the @NonNull getIsland(Island) overload — preserving perm-based limits and offsets is exactly right, and it removes the silent "create an empty IBC" path.

Minor: ibc is assigned here and then re-fetched in tidyUp() (ibc = bll.getIsland(island)), so this constructor assignment is overwritten before it's ever read. Harmless, but you could drop one. I'd keep the tidyUp() re-fetch — it also picks up anything JoinListener may have set on the island between construction and completion.

[tastybento]

Thanks again for these fixes, @daniel-skopek 🙏 — three of the four were spot on. I've taken the branch and opened #264, which keeps your original commit (authorship intact) and adds:

• the one-line correction for the BlockGrowEvent case (the extra process(..., false) decremented a count that was never incremented, since process(block, true) returns before incrementing when the limit is hit), and
• tests covering all three block-count paths, including a regression guard for that grow case.

Since I couldn't push directly to your fork branch, #264 carries your work forward. Cheers!