Centralize entity count persistence in BlockLimitsListener#274
Merged
Conversation
Follow-up to #273. That fix required every entity-mutation site to remember to pair the count change with a markChanged() call — a pattern repeated five times that a future call site could silently forget, reintroducing the unpersisted-counts bug. Replace the public markChanged() with incrementEntity(Island, ...) and decrementEntity(String, ...) wrappers on BlockLimitsListener that mutate the count and enroll the change in the batch-save cycle in one step, mirroring how block changes are auto-batched inside process(). EntityLimitListener now calls the wrappers, so the mutate/save pairing can no longer be forgotten. Also: - Add tests covering the new persistence path: entity changes trigger the batched async save after CHANGE_LIMIT is exceeded, no save fires before the threshold, and no-op decrements on unknown islands don't count toward the batch. - Drop the redundant saveMap.putIfAbsent() in updateSaveMap() — merge() already creates the entry. - Document the new invariant in CLAUDE.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVX8dZq1uKYKS53p1zvpku
|
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
1 participant
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.
Summary
Follow-up to #273, addressing the review findings on that PR.
Centralize the mutate + save pairing (once and for all)
#273 fixed entity-count persistence by requiring every entity-mutation site to pair the count change with a
markChanged()call — a two-step pattern repeated at five sites inEntityLimitListener, with nothing enforcing the pairing. A future entity-mutation site that forgot the second call would silently reintroduce the unpersisted-counts bug.The public
markChanged()is replaced by two wrappers onBlockLimitsListenerthat do both steps in one call, mirroring how block changes are auto-batched insideprocess():incrementEntity(Island, Environment, EntityType)— creates the island's count record if needed, increments, batches the savedecrementEntity(String, Environment, EntityType)— decrements and batches; no-op if the island has no record (a no-op doesn't count toward the batch threshold)EntityLimitListener's five call sites now use the wrappers, so the mutate/save pairing can no longer be forgotten. The invariant is documented in CLAUDE.md.Test coverage for the persistence path
#273 shipped without a test asserting that entity changes actually trigger a save. Six new tests cover the wrappers: increment creates the record and counts, decrement lowers the count, no-op decrement doesn't create records or count toward the batch, the 10th entity change triggers
saveObjectAsync(CHANGE_LIMIT = 9), 9 changes don't, and mixed increments/decrements batch together.EntityLimitListenerTestnow stubs the mocked wrappers to delegate to the realIslandBlockCountso its count assertions keep exercising the real flow.Cleanup
updateSaveMap()'ssaveMap.putIfAbsent(id, 0)was redundant —merge(id, 1, Integer::sum)already creates the entry. Removed.Testing
All 256 tests pass (
mvn test), including the 6 new persistence tests.🤖 Generated with Claude Code
https://claude.ai/code/session_01LVX8dZq1uKYKS53p1zvpku