Fix block count leaks and parallelize recount chunk loading (supersedes #263)#264
Merged
Conversation
- BlockSpreadEvent: decrement old block before adding new (was leaking old counts) - BlockGrowEvent: revert count increment when event is cancelled due to limit hit - RecountCalculator: load chunks in parallel instead of sequentially (fixes calc timeout on large islands) - RecountCalculator: use @nonnull getIsland(Island) to prevent permission limit loss on null ibc
The BlockGrowEvent handler decremented the new block's count after a cancelled (over-limit) grow. But process(block, true) returns before incrementing when the limit is hit, so that decrement removed a count that was never added — driving a real, physically-present block's count below its true value and letting the next grow slip past the limit. Drop the spurious decrement (the cancel branch was already balanced) and add tests covering: - BlockSpreadEvent decrementing the old block and adding the new (the leak this PR fixes) - BlockSpreadEvent reverting cleanly when the new material is at limit - BlockGrowEvent at a non-zero limit leaving the count untouched on a blocked grow (regression guard for the reverted line) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on @daniel-skopek's #263 — superseding it so the fix can land with the bug-2 correction and tests already applied. Daniel's original commit is preserved here with authorship intact; thanks for the catches. 🙏
What this brings over from #263 (unchanged)
BlockFormEventpattern.CALCULATION_TIMEOUTthat produced no result on large islands.@NonNull getIsland(Island)overload in the constructor andtidyUp(), so a recount no longer risks creating an emptyIslandBlockCountthat drops permission-based limits and offsets.What changed vs #263
The BlockGrowEvent handler in #263 added
process(e.getNewState().getBlock(), false)after a cancelled (over-limit) grow. That decrement is unbalanced:process(block, true)returns before incrementing when the limit is hit, so the extra call removes a count that was never added. For a material at a non-zero limit, a blocked grow drove the stored count below the true physical count, then let the next grow slip past the limit.This branch drops that one line (the cancel branch was already balanced) and adds tests:
testBlockSpreadDecrementsOldAddsNew— confirms the spread leak fixtestBlockSpreadAtLimitCancelsAndRestoresOld— confirms a clean revert when the new material is at limittestBlockGrowAtNonZeroLimitDoesNotDecrement— regression guard: a blocked grow at 5/5 leaves the count at 5Full discussion and the empirical 5→4 repro are in the #263 review.
All 233 tests pass.
Closes #263
🤖 Generated with Claude Code