Purge region files for soft-deleted islands#2933
Open
tastybento wants to merge 19 commits intodevelopfrom
Open
Purge region files for soft-deleted islands#2933tastybento wants to merge 19 commits intodevelopfrom
tastybento wants to merge 19 commits intodevelopfrom
Conversation
Scaffolding for the shift away from chunk-copy island deletion. No behavior
change yet — reset and admin delete still go through the old pipeline.
- Settings: add island.deletion.housekeeping.{enabled,interval-days,
region-age-days} (defaults off/30/60). Deprecate keep-previous-island-on-reset
and slow-deletion config entries (unbound from config; getters/setters kept
as @deprecated(forRemoval=true) for binary compat until Phase 4).
- PurgeRegionsService: extract scan/filter/delete/player-cleanup logic out
of AdminPurgeRegionsCommand so the command and the scheduler share one
code path. Handles both pre-26.1 (DIM-1/DIM1 subfolders) and 26.1.1+
(sibling world folders) dimension layouts.
- AdminPurgeRegionsCommand: reduced to ~180 LOC, delegates to the service
and retains only the two-step confirmation UX + per-island display.
- HousekeepingManager: new manager wired in BentoBox.onEnable(). Hourly
wall-clock check; runs the purge service across every gamemode overworld
if enabled and interval has elapsed. Last-run timestamp persisted to
<plugin-data-folder>/database/housekeeping.yml regardless of DB backend,
so the schedule survives restarts. Progress logged to console.
- AdminPurgeRegionsCommandTest: stub plugin.getPurgeRegionsService() with
a real service over the mocked plugin so the extraction is exercised
exactly as the command runs.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Running /bbox purge regions confirm on Paper 26.1.1 tripped AsyncCatcher
because PurgeRegionsService.delete() was saving worlds from the async
worker thread, and World.save() is main-thread-only:
IllegalStateException: Asynchronous world save!
at PurgeRegionsService.delete(PurgeRegionsService.java:151)
The pre-refactor command ran the save on the main thread inside execute()
but I collapsed it into the service. Move the save back out of the
service so all callers are responsible for flushing on the main thread
before dispatching the async delete.
- PurgeRegionsService.delete(): no longer calls Bukkit.getWorlds().save().
Javadoc updated to state the caller contract.
- AdminPurgeRegionsCommand.deleteEverything(): call Bukkit.getWorlds()
.forEach(World::save) before scheduling the async delete. Runs on the
main thread since execute() is invoked there.
- HousekeepingManager.executeCycle(): the existing runTask() save was
fire-and-forget — the async cycle could start scanning/deleting before
the save finished. Block via CompletableFuture.join() until the
main-thread save completes.
- AdminPurgeRegionsCommandTest: add regression asserting the service
never calls Bukkit.getWorlds() itself (would have caught this bug).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Paper rate-limits its built-in "plugin-induced save detected" warning, so after the scan save fired once, the confirm-path save was silent and looked like it wasn't running. Add explicit plugin.log lines on both sides of every World.save() call in the purge code paths (scan, confirm, housekeeping) so operators always see when the save is happening. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds /bbox admin purge age-regions <days> to rewrite per-chunk timestamp tables in .mca files so regions become purgable without waiting wall-clock time. The purge scanner reads timestamps from the region header, not file mtime, so `touch` cannot fake ageing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…urge-regions-reset
IslandsManager.deleteIsland() used to branch on keepPreviousIslandOnReset: false -> evict from cache, enqueue IslandChunkDeletionManager, MultiLib notify, delete DB row. true -> save with deletable=true and fire the deletion event. With the new region-file purge flow (Phase 1), physical cleanup no longer happens inline at all - old islands are left in place with deletable=true and reaped later by PurgeRegionsService / HousekeepingManager. So the hard-path branch goes away entirely: every call with removeBlocks=true now soft-deletes. Consequences in this commit: - AdminDeleteCommand also soft-deletes until Phase 3 splits it on GameModeAddon.isUsesNewChunkGeneration() (new-gen -> soft-delete, void gamemodes -> ChunkGenerator regen). - Nether/End cascade is a no-op in the soft path (nothing touches chunks); PurgeRegionsService.scan already gates nether/end on isNetherIslands/isEndIslands so vanilla-owned dimensions are skipped when the regions are eventually reaped. - keepPreviousIslandOnReset setter/getter remain as deprecated shims (no longer consulted at runtime); Phase 4 removes the field. - The bentobox-deleteIsland MultiLib subscriber is now unreachable from this server's publishers but stays until Phase 4 deletes the deletion infrastructure wholesale. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 2 made reset leave orphaned islands in place with deletable=true until the region purge reaps them. That meant admins walking around a server had no way to tell an orphan from a normal unowned island — /bbox admin info just showed "Unowned" and entering the area was silent. Two visible cues now: - IslandInfo.showAdminInfo() prints a new "deletable: flagged for deletion and awaiting region purge" line when island.isDeletable() is true, right after the purge-protected line. - LockAndBanListener notifies ops (once per entry, same pattern as the existing lock notification) when they step onto an island flagged deletable. Non-ops still see nothing; this is strictly an admin heads-up. The notification state is cleared when the op leaves the island, so walking back in re-triggers it. New locale keys commands.admin.info.deletable and protection.deletable-island-admin in en-US.yml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
/bbox admin delete used to always call deleteIsland(island, true, uuid),
which after Phase 2 soft-deletes unconditionally. That is the right
behavior for new-chunk-generation gamemodes like Boxed where chunks
are expensive and the region-file purge reaps them later on the
HousekeepingManager schedule. For void/simple-generator gamemodes it
is the wrong behavior — chunks are cheap, admins expect "delete" to
actually delete, and soft-deleted rows would linger forever because
the repainted region files always look fresh to the purge scan.
Branch on GameModeAddon.isUsesNewChunkGeneration():
- true (new-gen): soft-delete via IslandsManager.deleteIsland(),
same as /is reset. Physical cleanup happens later via
PurgeRegionsService / HousekeepingManager.
- false (void/simple): kick off DeleteIslandChunks (which
routes to WorldRegenerator.regenerateSimple with correct
nether/end cascade gating) to repaint the chunks via the
addon's own ChunkGenerator, then hard-delete the island row
immediately. DeleteIslandChunks snapshots the bounds in its
constructor so the row can be removed before the async regen
completes.
Adds IslandsManager.hardDeleteIsland(island): fires the pre-delete
event, kicks members, nulls owner, evicts from cache, deletes the DB
row. Does not touch world chunks — caller handles physical cleanup.
Phase 4 will remove DeleteIslandChunks, IslandDeletion, and the
CopyWorldRegenerator.regenerateCopy seed-world path; regenerateSimple
and the split here survive.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a second purge mode that reaps region files for any island already flagged as deletable, regardless of region-file age. Exposed as /bbox admin purge deleted and run from HousekeepingManager on a configurable hourly cadence (default 24h) alongside the existing monthly age sweep. Closes the post-reset gap where orphan island regions sat on disk for 60+ days waiting for the age threshold. Fix: evict in-memory chunks via World.unloadChunk(cx, cz, false) on the main thread before the async file delete, otherwise Paper's autosave re-flushes the deleted region files with the stale chunks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…urge-regions-reset
Paper's internal chunk cache keeps serving stale block data even after the .mca region files are deleted from disk. The chunks only clear on server restart when Paper discards its cache. Deleting the island DB row immediately left a window where players see old blocks but BentoBox reports no island at that location. The deleted sweep (days==0) now adds island IDs to a pendingDeletions set instead of removing them from the DB inline. On plugin shutdown (BentoBox.onDisable), flushPendingDeletions() processes the set. If the server crashes before a clean shutdown, the islands stay deletable=true and the next purge cycle retries safely. The age-based sweep (days>0) keeps immediate DB removal with the existing residual-region completeness check, since old regions won't be in Paper's memory cache. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Strip the diagnostic logging added during development that printed file size, removed status, and existsAfter for every .mca deletion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This setting was made obsolete by Phase 2 which changed /is reset to always soft-delete. The only remaining references were in AdminPurgeCommand for conditional logging — now simplified to always use tier-based progress reporting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a region-file purge system for soft-deleted islands, extracting purge logic into a reusable service and adding scheduled housekeeping to clean up .mca files over time.
Changes:
- Added
PurgeRegionsServicefor shared scan/filter/delete logic (age sweep + deletable-flag “deleted sweep”), including chunk eviction and deferred DB cleanup. - Added
HousekeepingManagerto run scheduled age/deleted sweeps with YAML state persistence + legacy migration. - Updated admin purge/delete commands, settings, config, and user/admin messaging to support the new soft-delete + purge flow.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/world/bentobox/bentobox/managers/PurgeRegionsService.java | New shared purge service (scan/filter/delete, chunk eviction, deferred DB deletions). |
| src/main/java/world/bentobox/bentobox/managers/HousekeepingManager.java | New scheduled housekeeping with persisted run timestamps and dual-cycle dispatch. |
| src/main/java/world/bentobox/bentobox/BentoBox.java | Wires up purge service + housekeeping and flushes deferred deletions on shutdown. |
| src/main/java/world/bentobox/bentobox/managers/IslandsManager.java | Makes reset/delete soft-delete by default; adds hard-delete helper for specific modes. |
| src/main/java/world/bentobox/bentobox/api/commands/admin/purge/AdminPurgeRegionsCommand.java | Refactors command to use PurgeRegionsService. |
| src/main/java/world/bentobox/bentobox/api/commands/admin/purge/AdminPurgeDeletedCommand.java | New command to reap regions for deletable=true islands regardless of file age. |
| src/main/java/world/bentobox/bentobox/api/commands/admin/purge/AdminPurgeAgeRegionsCommand.java | New debug command to rewrite region timestamps for testing. |
| src/main/java/world/bentobox/bentobox/api/commands/admin/purge/AdminPurgeCommand.java | Registers new purge subcommands and updates warning behavior. |
| src/main/java/world/bentobox/bentobox/api/commands/admin/AdminDeleteCommand.java | Splits delete behavior based on gamemode chunk generation strategy. |
| src/main/java/world/bentobox/bentobox/listeners/flags/protection/LockAndBanListener.java | Adds admin-only notification when standing on an island flagged deletable. |
| src/main/java/world/bentobox/bentobox/util/IslandInfo.java | Shows deletable status in /bbox admin info. |
| src/main/java/world/bentobox/bentobox/Settings.java | Removes keep-previous-on-reset, adds housekeeping settings, deprecates slow-deletion. |
| src/main/resources/config.yml | Removes obsolete keep-previous-island-on-reset option. |
| src/main/resources/locales/en-US.yml | Updates purge messaging and adds deleted-sweep / deletable notifications. |
| src/test/java/world/bentobox/bentobox/SettingsTest.java | Removes tests for deleted config option. |
| src/test/java/world/bentobox/bentobox/managers/PurgeRegionsServiceTest.java | New direct tests for deleted-sweep semantics and chunk eviction. |
| src/test/java/world/bentobox/bentobox/managers/HousekeepingManagerTest.java | New tests for housekeeping persistence, migration, and scheduling decisions. |
| src/test/java/world/bentobox/bentobox/api/commands/admin/purge/AdminPurgeDeletedCommandTest.java | New end-to-end tests for the deleted purge command. |
| src/test/java/world/bentobox/bentobox/api/commands/admin/purge/AdminPurgeRegionsCommandTest.java | Updates tests to use real service + adds regression coverage. |
| src/test/java/world/bentobox/bentobox/api/commands/admin/purge/AdminPurgeCommandTest.java | Updates expected subcommand count. |
src/main/java/world/bentobox/bentobox/managers/PurgeRegionsService.java
Outdated
Show resolved
Hide resolved
Comment on lines
+87
to
+93
| public record PurgeScanResult( | ||
| World world, | ||
| int days, | ||
| Map<Pair<Integer, Integer>, Set<String>> deleteableRegions, | ||
| boolean isNether, | ||
| boolean isEnd, | ||
| FilterStats stats) { |
There was a problem hiding this comment.
PurgeScanResult exposes deleteableRegions (misspelling). Since this record is public, the accessor name becomes part of the API surface and will propagate throughout the codebase/tests. Consider renaming it to deletableRegions (and updating callers) to avoid locking in the typo.
Comment on lines
+121
to
+125
| Bukkit.getScheduler().runTaskAsynchronously(getPlugin(), () -> { | ||
| boolean ok = getPlugin().getPurgeRegionsService().delete(scan); | ||
| Bukkit.getScheduler().runTask(getPlugin(), () -> | ||
| user.sendMessage(ok ? "general.success" : NONE_FOUND)); | ||
| }); |
There was a problem hiding this comment.
ok from PurgeRegionsService.delete(scan) is used to decide whether to send general.success vs commands.admin.purge.none-found, but delete(...) can return false due to I/O failures or freshness checks, not just "none found". This results in a misleading user message on partial/failed deletions. Consider sending a dedicated error message (and keeping none-found only for empty scans).
src/main/java/world/bentobox/bentobox/api/commands/admin/purge/AdminPurgeDeletedCommand.java
Outdated
Show resolved
Hide resolved
Comment on lines
+79
to
+93
| running = true; | ||
| final int finalDays = days; | ||
| CompletableFuture.runAsync(() -> { | ||
| try { | ||
| int count = getPlugin().getPurgeRegionsService().ageRegions(getWorld(), finalDays); | ||
| Bukkit.getScheduler().runTask(getPlugin(), () -> { | ||
| user.sendMessage("commands.admin.purge.age-regions.done", | ||
| TextVariables.NUMBER, String.valueOf(count)); | ||
| getPlugin().log("Age-regions: " + count + " region file(s) aged by " | ||
| + finalDays + " day(s) in world " + getWorld().getName()); | ||
| }); | ||
| } finally { | ||
| running = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
This command uses CompletableFuture.runAsync(...) (common pool) for plugin work. Using the Bukkit scheduler (runTaskAsynchronously) is safer so the task is tied to the plugin lifecycle (cancelled on disable) and uses the server’s managed executor.
src/main/java/world/bentobox/bentobox/api/commands/admin/AdminDeleteCommand.java
Outdated
Show resolved
Hide resolved
Comment on lines
+40
to
+46
| /** | ||
| * Tracks ops who have already been notified that they are standing on an | ||
| * island flagged for deletion (awaiting region purge), to avoid spamming | ||
| * the notice on every move event. Cleared when the op leaves a deletable | ||
| * island. | ||
| */ | ||
| private final Set<UUID> deletableNotified = new HashSet<>(); |
There was a problem hiding this comment.
deletableNotified is only cleared when the player leaves a deletable island, but not on player quit. Over time this can accumulate UUIDs (same concern as notifiedPlayers). Consider clearing both sets on PlayerQuitEvent to avoid unbounded growth on long-running servers.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…vice.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…/AdminPurgeDeletedCommand.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…DeleteCommand.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Member
Author
|
@copilot apply changes based on the comments in this thread |
|
- Rename `deleteableRegions` → `deletableRegions` in PurgeScanResult and all callers (fixes API surface typo) - Distinguish "none-found" from "purge failed" in AdminPurgeRegionsCommand.deleteEverything() with dedicated locale key - Replace CompletableFuture.runAsync() with Bukkit scheduler in AdminPurgeAgeRegionsCommand (ties task to plugin lifecycle) - Clear notifiedPlayers and deletableNotified on PlayerQuitEvent in LockAndBanListener (prevents unbounded set growth) - Add `commands.admin.purge.failed` locale key in en-US.yml Agent-Logs-Url: https://github.com/BentoBoxWorld/BentoBox/sessions/99688668-0dd8-455e-9002-3f229fbefbdc Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
Contributor
Applied all changes from the review thread in commit 204d6c5:
|
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
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
PurgeRegionsServicefrom the admin command + addHousekeepingManagerwith configurable age-sweep and deleted-sweep schedules, YAML state persistence, and legacy migration/is resetnow always soft-deletes (setDeletable(true)) — physical cleanup is deferred to the purge systemAdminDeleteCommandso new-chunk-generation gamemodes (Boxed, etc.) soft-delete; void/simple gamemodes keep the existingDeleteIslandChunkspath/bbox admin purge deletedcommand + daily housekeeping deleted-sweep that reaps region files for islands already flagged as deletable, ignoring file agekeep-previous-island-on-resetsetting (reset always soft-deletes now)Key behaviours
deletable=trueand the next purge cycle retriesTest plan
./gradlew test— 2805+ tests green/is resetseveral times, run/boxadmin purge deleted, confirm region files deleted, restart server, confirm old blueprint blocks are gonedeleted-interval-hours: 1, restart, verify deleted sweep fires on the hourly check🤖 Generated with Claude Code