From 376094feff6a40d08086d60d71bea14688a45105 Mon Sep 17 00:00:00 2001 From: ishaanko Date: Thu, 10 Sep 2026 14:20:57 -0700 Subject: [PATCH 1/2] fix(storage): make cleanup delete the backups it lists Storage cleanup removed only the ZIP copy of an unprotected backup and left its Git snapshot and catalog entry in place, so the backup was still there after the cleanup and after a restart. An unprotected backup is now deleted the same way the Delete button deletes it: Git snapshot, remote copy, ZIP, and catalog record. Compaction now keeps exactly the LFS objects that a remaining snapshot points at and removes the rest. The old `git lfs prune` path failed in a bare repository and could not tell a deleted snapshot from a kept one. A protected backup that loses its local Git copy keeps its synchronized remote copy in the catalog instead of orphaning the ref on the remote. --- CHANGELOG.md | 14 + README.md | 6 + gradle.properties | 2 +- .../ui/CleanupConfirmationScreen.java | 9 +- .../worldarchive/ui/CleanupPreviewScreen.java | 21 +- .../storage/git/GitBackupBackend.java | 7 +- .../storage/git/GitSnapshotVerifier.java | 67 ++-- .../storage/git/GitStorageCompactor.java | 78 +++-- .../storage/management/CleanupExecutor.java | 75 ++-- .../storage/management/CleanupItem.java | 13 +- .../storage/management/CleanupPlanner.java | 126 ++++--- .../management/ManagedStorageService.java | 2 +- .../management/CleanupIntegrationTest.java | 319 ++++++++++++++++++ .../management/ManagedStorageServiceTest.java | 2 +- 14 files changed, 604 insertions(+), 137 deletions(-) create mode 100644 src/test/java/dev/ishaanko/worldarchive/storage/management/CleanupIntegrationTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index ccb7219..3a099a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ # Changelog +## 0.3.7.1 (2026-09-10) + +### Fixed + +- Storage cleanup now deletes the backups it lists. Before, it only removed + the ZIP copy of a backup and left the Git snapshot and the catalog entry in + place, so the backup was still there after the cleanup and after a restart. + A backup the keep settings do not protect is now deleted the same way the + Delete button deletes it: Git snapshot, remote copy, ZIP, and catalog entry. +- Cleanup frees the Git LFS space of deleted snapshots. Only the objects a + remaining snapshot still points at are kept. +- A protected backup that loses its local Git copy keeps its synchronized + remote copy in the catalog, so it stays visible and can be deleted later. + ## 0.3.7 (2026-09-10) ### Added diff --git a/README.md b/README.md index a953c5c..4760526 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,12 @@ Guided cleanup keeps: Within a period, manual backups and backups with more changed files win. Cleanup always shows a preview first. Nothing is deleted until you confirm. +Cleanup deletes the backups it lists the same way the Delete button does: the +Git snapshot, its copy on the configured remote, the ZIP, and the catalog +entry. A protected backup is never deleted. When space is still short after +that, cleanup can drop the local Git copies of protected backups, but only when +each of them keeps a ZIP or a verified remote copy. + ## Minecraft versions Every new backup records the Minecraft version it was made with. The restore diff --git a/gradle.properties b/gradle.properties index 6751749..8ac9949 100644 --- a/gradle.properties +++ b/gradle.properties @@ -9,6 +9,6 @@ loom_version=1.17.19 fabric_api_version=0.157.0+26.2 modmenu_version=20.0.1 -mod_version=0.3.7 +mod_version=0.3.7.1 maven_group=dev.ishaanko automatic_release_channel=alpha diff --git a/src/client/java/dev/ishaanko/worldarchive/ui/CleanupConfirmationScreen.java b/src/client/java/dev/ishaanko/worldarchive/ui/CleanupConfirmationScreen.java index 2a6dba7..1fe3ab3 100644 --- a/src/client/java/dev/ishaanko/worldarchive/ui/CleanupConfirmationScreen.java +++ b/src/client/java/dev/ishaanko/worldarchive/ui/CleanupConfirmationScreen.java @@ -52,7 +52,7 @@ final class CleanupConfirmationScreen extends Screen { BackupClientFacade facade, CleanupPlan plan, Set selected) { - super(Component.literal("Confirm Local Cleanup")); + super(Component.literal("Confirm Cleanup")); this.preview = Objects.requireNonNull(preview, "preview"); this.returnTo = Objects.requireNonNull(returnTo, "returnTo"); this.world = Objects.requireNonNull(world, "world"); @@ -76,7 +76,7 @@ protected void init() { contentWidth, 18, Component.literal( - "These backups will be deleted from this computer. This cannot be undone.") + "These backups will be deleted. This cannot be undone.") .withStyle(ChatFormatting.RED), font)); int pageSize = Math.max(1, Math.min(6, (height - 142) / 24)); @@ -91,8 +91,9 @@ protected void init() { + " · " + item.label().orElse("unlabeled") + " · " - + (item.removeLocalGit() ? "Git " : "") - + (item.removeZip() ? "ZIP" : ""); + + (plan.protectedBackups().contains(item.backupId()) + ? "local Git copy only" + : (item.removeGit() ? "Git " : "") + (item.removeZip() ? "ZIP" : "")); StringWidget row = new StringWidget( x, y, diff --git a/src/client/java/dev/ishaanko/worldarchive/ui/CleanupPreviewScreen.java b/src/client/java/dev/ishaanko/worldarchive/ui/CleanupPreviewScreen.java index afa4bd1..75d2e07 100644 --- a/src/client/java/dev/ishaanko/worldarchive/ui/CleanupPreviewScreen.java +++ b/src/client/java/dev/ishaanko/worldarchive/ui/CleanupPreviewScreen.java @@ -62,7 +62,7 @@ protected void init() { addRenderableOnly(Widgets.title(font, x, 9, contentWidth, 20, title)); String summary = plan.items().isEmpty() ? "Nothing to clean up right now. Your keep settings protect every backup." - : "Choose which backups to delete from this computer. Copies on GitHub or in linked folders are not touched."; + : "Backups your keep settings do not protect are deleted everywhere, including GitHub. Protected backups only lose their local Git copy."; addRenderableOnly(new MultiLineTextWidget( x, 31, @@ -89,7 +89,7 @@ private void addItems(int x, int contentWidth, int pageSize) { + " · " + identity(item) + " · " - + actions(item) + + actions(plan, item) + " · " + item.changedFileCount() + " changed"; @@ -105,9 +105,10 @@ private void addItems(int x, int contentWidth, int pageSize) { } } + /** Protected backups give up their local Git copies together or not at all. */ private void toggle(CleanupItem item) { boolean removing = selected.contains(item.backupId()); - if (!item.removeLocalGit()) { + if (!item.removeGit() || !plan.protectedBackups().contains(item.backupId())) { if (removing) { selected.remove(item.backupId()); } else { @@ -116,8 +117,9 @@ private void toggle(CleanupItem item) { return; } plan.items().stream() - .filter(CleanupItem::removeLocalGit) + .filter(CleanupItem::removeGit) .map(CleanupItem::backupId) + .filter(plan.protectedBackups()::contains) .forEach(backupId -> { if (removing) { selected.remove(backupId); @@ -192,11 +194,14 @@ private static String identity(CleanupItem item) { return item.label().orElse(item.backupId().toString().substring(0, 8)); } - private static String actions(CleanupItem item) { - if (item.removeLocalGit() && item.removeZip()) { - return "Git + ZIP"; + private static String actions(CleanupPlan plan, CleanupItem item) { + if (plan.protectedBackups().contains(item.backupId())) { + return "local Git copy only"; } - return item.removeLocalGit() ? "local Git" : "ZIP"; + if (item.removeGit() && item.removeZip()) { + return "delete Git + ZIP"; + } + return item.removeGit() ? "delete Git" : "delete ZIP"; } static String details(CleanupItem item) { diff --git a/src/main/java/dev/ishaanko/worldarchive/storage/git/GitBackupBackend.java b/src/main/java/dev/ishaanko/worldarchive/storage/git/GitBackupBackend.java index 23885e8..b32bc4a 100644 --- a/src/main/java/dev/ishaanko/worldarchive/storage/git/GitBackupBackend.java +++ b/src/main/java/dev/ishaanko/worldarchive/storage/git/GitBackupBackend.java @@ -246,17 +246,18 @@ public CompletionStage deleteLocalSnapshot(WorldId worldId, BackupId ba Objects.requireNonNull(worldId, "worldId"); Objects.requireNonNull(backupId, "backupId"); return submit(() -> lock.withLock(() -> - new GitStorageCompactor(settings, repository, refs, commands) + new GitStorageCompactor(settings, repository, refs, commands, verifier) .deleteLocalSnapshot(worldId, backupId))); } + /** Frees Git and LFS storage that no remaining snapshot in this repository uses. */ public CompletionStage compactStorage(WorldId worldId) { Objects.requireNonNull(worldId, "worldId"); return submit(() -> lock.withLock(() -> { repository.requireWorld(worldId); repository.requireBare(); - boolean noSnapshots = operations.listSnapshotsBlocking(Optional.of(worldId)).isEmpty(); - new GitStorageCompactor(settings, repository, refs, commands).compact(worldId, noSnapshots); + new GitStorageCompactor(settings, repository, refs, commands, verifier) + .compact(worldId, operations.listSnapshotsBlocking(Optional.empty())); return null; })); } diff --git a/src/main/java/dev/ishaanko/worldarchive/storage/git/GitSnapshotVerifier.java b/src/main/java/dev/ishaanko/worldarchive/storage/git/GitSnapshotVerifier.java index 5dabef0..cf8eb56 100644 --- a/src/main/java/dev/ishaanko/worldarchive/storage/git/GitSnapshotVerifier.java +++ b/src/main/java/dev/ishaanko/worldarchive/storage/git/GitSnapshotVerifier.java @@ -207,6 +207,22 @@ private void requireCommitMessage( } } + /** + * Lists the LFS objects a snapshot tree points at without checking that the + * objects exist. Compaction uses this to decide which objects must stay. + */ + List readLfsPointers(String commit) + throws IOException, InterruptedException, GitStorageException { + List pointers = new ArrayList<>(); + for (GitTreeEntry entry : readTreeEntries(commit)) { + if (entry.path().equals(GitBackupBackend.MANIFEST_PATH)) { + continue; + } + readBlob(entry).pointer().ifPresent(pointers::add); + } + return List.copyOf(pointers); + } + private List findAndVerifySnapshotFiles( List treeEntries, BackupManifest manifest) @@ -217,26 +233,9 @@ private List findAndVerifySnapshotFiles( if (entry.path().equals(GitBackupBackend.MANIFEST_PATH)) { continue; } - GitCommandResult contents = commands.run( - List.of( - "cat-file", - "blob", - entry.objectId()), - settings.repository(), - Map.of(), - new byte[0], - LFS_POINTER_OUTPUT_BYTES); - if (!contents.successful()) { - throw new GitStorageException(GitCommands.failureMessage(contents)); - } - if (contents.standardErrorTruncated()) { - throw new GitStorageException( - "Git LFS pointer inspection exceeded its safety limit"); - } - Optional pointer = GitLfsPointer.parse( - entry, - contents.standardOutput(), - contents.standardOutputTruncated()); + SnapshotBlob blob = readBlob(entry); + GitCommandResult contents = blob.contents(); + Optional pointer = blob.pointer(); if (pointer.isPresent()) { verifyLfsObject(pointer.get()); pointers.add(pointer.get()); @@ -255,6 +254,31 @@ private List findAndVerifySnapshotFiles( return List.copyOf(pointers); } + /** Reads one tree blob up to the pointer size limit and parses it as an LFS pointer. */ + private SnapshotBlob readBlob(GitTreeEntry entry) + throws IOException, InterruptedException, GitStorageException { + GitCommandResult contents = commands.run( + List.of( + "cat-file", + "blob", + entry.objectId()), + settings.repository(), + Map.of(), + new byte[0], + LFS_POINTER_OUTPUT_BYTES); + if (!contents.successful()) { + throw new GitStorageException(GitCommands.failureMessage(contents)); + } + if (contents.standardErrorTruncated()) { + throw new GitStorageException( + "Git LFS pointer inspection exceeded its safety limit"); + } + return new SnapshotBlob(contents, GitLfsPointer.parse( + entry, + contents.standardOutput(), + contents.standardOutputTruncated())); + } + private void verifyLfsObject(GitLfsPointer pointer) throws IOException, GitStorageException { Path object = pointer.objectPath(settings.repository()); Path parent = object.getParent(); @@ -279,6 +303,9 @@ private static String sha256(Path path) throws IOException { return Digests.sha256(path); } + private record SnapshotBlob(GitCommandResult contents, Optional pointer) { + } + record VerifiedSnapshot( GitSnapshotManifest manifest, List lfsPointers) { diff --git a/src/main/java/dev/ishaanko/worldarchive/storage/git/GitStorageCompactor.java b/src/main/java/dev/ishaanko/worldarchive/storage/git/GitStorageCompactor.java index 7d3ccc7..99c7e43 100644 --- a/src/main/java/dev/ishaanko/worldarchive/storage/git/GitStorageCompactor.java +++ b/src/main/java/dev/ishaanko/worldarchive/storage/git/GitStorageCompactor.java @@ -3,15 +3,23 @@ import dev.ishaanko.worldarchive.model.WorldId; import dev.ishaanko.worldarchive.model.BackupId; import java.io.IOException; +import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.LinkOption; import java.nio.file.Path; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; +import java.util.regex.Pattern; /** Exact local-only compaction after confirmed snapshot-ref cleanup. */ final class GitStorageCompactor { + private static final Pattern LFS_OBJECT_NAME = Pattern.compile("[0-9a-f]{64}"); + private final GitBackendSettings settings; private final GitRepositoryManager repository; @@ -20,20 +28,40 @@ final class GitStorageCompactor { private final GitCommands commands; + private final GitSnapshotVerifier verifier; + GitStorageCompactor( GitBackendSettings settings, GitRepositoryManager repository, GitRefStore refs, - GitCommands commands) { + GitCommands commands, + GitSnapshotVerifier verifier) { this.settings = Objects.requireNonNull(settings, "settings"); this.repository = Objects.requireNonNull(repository, "repository"); this.refs = Objects.requireNonNull(refs, "refs"); this.commands = Objects.requireNonNull(commands, "commands"); + this.verifier = Objects.requireNonNull(verifier, "verifier"); } - void compact(WorldId worldId, boolean noSnapshots) + /** + * Reclaims space after snapshot refs were deleted. Snapshot commits chain onto + * each other, so Git's own reachability keeps every older commit alive and + * {@code git lfs prune} cannot tell a deleted snapshot from a kept one. The LFS + * objects that stay are therefore computed exactly: every object that a + * remaining snapshot's tree points at. Anything else under {@code lfs/objects} + * is removed. When the world has no snapshots left its history ref goes too. + */ + void compact(WorldId worldId, List remainingSnapshots) throws IOException, InterruptedException, GitStorageException { - if (noSnapshots) { + Set retained = new HashSet<>(); + for (GitSnapshot snapshot : remainingSnapshots) { + for (GitLfsPointer pointer : verifier.readLfsPointers(snapshot.commitId())) { + retained.add(pointer.sha256()); + } + } + boolean worldEmpty = remainingSnapshots.stream() + .noneMatch(snapshot -> snapshot.worldId().equals(worldId)); + if (worldEmpty) { refs.deleteIfPresent(repository.historyRef(worldId)); } commands.checked( @@ -52,18 +80,7 @@ void compact(WorldId worldId, boolean noSnapshots) settings.repository(), Map.of(), new byte[0]); - if (noSnapshots) { - deleteAllLfsObjects(); - } else { - commands.checked( - List.of( - "lfs", - "prune", - "--force"), - settings.repository(), - Map.of(), - new byte[0]); - } + deleteUnreferencedLfsObjects(retained); } boolean deleteLocalSnapshot(WorldId worldId, BackupId backupId) @@ -79,11 +96,34 @@ boolean deleteLocalSnapshot(WorldId worldId, BackupId backupId) return true; } - private void deleteAllLfsObjects() throws GitStorageException { + private void deleteUnreferencedLfsObjects(Set retained) + throws IOException, GitStorageException { Path lfsObjects = settings.repository().resolve("lfs").resolve("objects"); - GitTemporaryFiles.deleteTree(lfsObjects); - if (Files.exists(lfsObjects, LinkOption.NOFOLLOW_LINKS)) { - throw new GitStorageException("Unreferenced Git LFS objects could not be removed"); + if (!Files.isDirectory(lfsObjects, LinkOption.NOFOLLOW_LINKS)) { + return; + } + for (String sha256 : retained) { + Path object = lfsObjects.resolve(sha256.substring(0, 2)) + .resolve(sha256.substring(2, 4)) + .resolve(sha256); + if (!Files.isRegularFile(object, LinkOption.NOFOLLOW_LINKS)) { + throw new GitStorageException( + "A Git LFS object needed by a remaining snapshot is missing"); + } } + Files.walkFileTree(lfsObjects, new SimpleFileVisitor<>() { + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attributes) + throws IOException { + String name = file.getFileName().toString(); + if (attributes.isRegularFile() + && !attributes.isSymbolicLink() + && LFS_OBJECT_NAME.matcher(name).matches() + && !retained.contains(name)) { + Files.delete(file); + } + return FileVisitResult.CONTINUE; + } + }); } } diff --git a/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupExecutor.java b/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupExecutor.java index 5f9c939..73e0911 100644 --- a/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupExecutor.java +++ b/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupExecutor.java @@ -25,7 +25,11 @@ import java.util.Optional; import java.util.Set; -/** Applies a confirmed {@link CleanupPlan}, including its remote-safety guardrails. */ +/** + * Applies a confirmed {@link CleanupPlan}. A backup the keep settings do not protect is + * deleted the same way the Delete button deletes it: Git snapshot (local and remote), + * ZIP, and catalog record. A protected backup only loses its local Git copy. + */ final class CleanupExecutor { private final BackupCatalog catalog; @@ -60,12 +64,10 @@ CleanupResult apply(CleanupPlan plan, CleanupRequest request) throws Exception { throw new IOException("Storage changed after the preview; review cleanup again"); } requireVerifiedSafetyFloor(current, plan.verifiedSafetyFloor()); - Set remoteCopies = requireCurrentRemoteCopies( - plan, request, current); + requireCurrentRemoteCopies(plan, request, current); long before = current.totalBytes(); Map failures = new LinkedHashMap<>(); - boolean removedGit = applyItems( - plan, request, current, remoteCopies, failures); + boolean removedGit = applyItems(plan, request, current, failures); if (removedGit) { try { ManagedStorageSupport.await(git.compactCurrentStorage(plan.worldId())); @@ -75,7 +77,7 @@ CleanupResult apply(CleanupPlan plan, CleanupRequest request) throws Exception { } catch (Exception exception) { failures.putIfAbsent( plan.items().stream() - .filter(CleanupItem::removeLocalGit) + .filter(CleanupItem::removeGit) .map(CleanupItem::backupId) .findFirst() .orElseThrow(), @@ -106,9 +108,12 @@ private static void validateSelection( || removesSafetyFloor) { throw new IOException("Cleanup selection does not match its preview"); } + // Protected backups lose their local Git copies as one group: the space only + // comes back once none of them keeps the shared history alive. Set gitGroup = plan.items().stream() - .filter(CleanupItem::removeLocalGit) + .filter(CleanupItem::removeGit) .map(CleanupItem::backupId) + .filter(plan.protectedBackups()::contains) .collect(java.util.stream.Collectors.toSet()); boolean someGitSelected = request.selectedBackups().stream() .anyMatch(gitGroup::contains); @@ -118,11 +123,14 @@ private static void validateSelection( } } + /** + * Git goes first: a remote that refuses the deletion fails the item before its ZIP + * is touched, so the backup stays whole and restorable. + */ private boolean applyItems( CleanupPlan plan, CleanupRequest request, Snapshot current, - Set remoteCopies, Map failures) throws InterruptedException { boolean removedGit = false; for (CleanupItem item : plan.items()) { @@ -130,18 +138,28 @@ private boolean applyItems( continue; } try { + if (item.removeGit()) { + if (plan.protectedBackups().contains(item.backupId())) { + ManagedStorageSupport.await(git.deleteCurrentLocalSnapshot( + plan.worldId(), + item.backupId())); + // The catalog keeps pointing at a synchronized remote copy, so + // the backup stays visible, verifiable, and deletable later. + removeDestination( + item.backupId(), + DestinationType.GIT, + synchronizedRemoteCopy(current, item.backupId())); + } else { + ManagedStorageSupport.await(git.deleteSnapshot( + plan.worldId(), + item.backupId())); + removeDestination(item.backupId(), DestinationType.GIT, false); + } + removedGit = true; + } if (item.removeZip()) { removeZip(current, item.backupId()); } - if (item.removeLocalGit()) { - ManagedStorageSupport.await(git.deleteCurrentLocalSnapshot( - plan.worldId(), - item.backupId())); - removeGitCatalogCopy( - item.backupId(), - remoteCopies.contains(item.backupId())); - removedGit = true; - } } catch (InterruptedException exception) { Thread.currentThread().interrupt(); throw exception; @@ -248,14 +266,22 @@ private boolean currentRemoteContainsSnapshot(Snapshot snapshot, BackupId backup } } - private Set requireCurrentRemoteCopies( + private static boolean synchronizedRemoteCopy(Snapshot snapshot, BackupId backupId) { + return ManagedStorageSupport.destination( + ManagedStorageSupport.record(snapshot, backupId), DestinationType.GIT) + .filter(result -> result.ownership() == ArtifactOwnership.MANAGED + && result.syncStatus() == SyncStatus.SYNCED) + .isPresent(); + } + + /** A protected backup may lose its last local copy only if the remote provably has it. */ + private void requireCurrentRemoteCopies( CleanupPlan plan, CleanupRequest request, Snapshot snapshot) throws Exception { - Set remoteCopies = new HashSet<>(); for (CleanupItem item : plan.items()) { if (!request.selectedBackups().contains(item.backupId()) - || !item.removeLocalGit() + || !item.removeGit() || !plan.protectedBackups().contains(item.backupId())) { continue; } @@ -280,9 +306,7 @@ private Set requireCurrentRemoteCopies( throw new IOException( "The configured remote changed after the cleanup preview"); } - remoteCopies.add(item.backupId()); } - return Set.copyOf(remoteCopies); } private void removeZip(Snapshot snapshot, BackupId backupId) throws Exception { @@ -319,13 +343,6 @@ private void restoreRecord(BackupRecord previous, Exception deletionFailure) { } } - private void removeGitCatalogCopy( - BackupId backupId, - boolean verifiedRemoteRemains) throws IOException { - removeDestination( - backupId, DestinationType.GIT, verifiedRemoteRemains); - } - private void removeDestination( BackupId backupId, DestinationType type, diff --git a/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupItem.java b/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupItem.java index 54e1c7a..324b265 100644 --- a/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupItem.java +++ b/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupItem.java @@ -5,13 +5,18 @@ import java.util.Objects; import java.util.Optional; -/** One exact local cleanup action shown before confirmation. */ +/** + * One cleanup action shown before confirmation. For a backup the keep settings do not + * protect, {@code removeGit} deletes the Git snapshot everywhere, including the + * configured remote, and the backup leaves the catalog. For a protected backup it drops + * only the local Git copy; the ZIP or the verified remote copy stays. + */ public record CleanupItem( BackupId backupId, Instant createdAt, Optional label, long changedFileCount, - boolean removeLocalGit, + boolean removeGit, boolean removeZip, Optional gitRef, Optional zipArtifactId, @@ -27,10 +32,10 @@ public record CleanupItem( if (changedFileCount < 0 || estimatedGitBytes < 0 || exactZipBytes < 0) { throw new IllegalArgumentException("Cleanup counts must not be negative"); } - if (!removeLocalGit && !removeZip) { + if (!removeGit && !removeZip) { throw new IllegalArgumentException("Cleanup item must remove at least one local artifact"); } - if (removeLocalGit != gitRef.isPresent() || removeZip != zipArtifactId.isPresent()) { + if (removeGit != gitRef.isPresent() || removeZip != zipArtifactId.isPresent()) { throw new IllegalArgumentException("Cleanup artifact identities do not match their actions"); } } diff --git a/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupPlanner.java b/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupPlanner.java index 7bbfb0f..e3bb7c2 100644 --- a/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupPlanner.java +++ b/src/main/java/dev/ishaanko/worldarchive/storage/management/CleanupPlanner.java @@ -44,6 +44,12 @@ final class CleanupPlanner { this.zoneId = Objects.requireNonNull(zoneId, "zoneId"); } + /** + * Picks the lightest backups the keep settings do not protect and deletes each one + * completely. Only when that still leaves the world over its target does the plan + * drop the local Git copies of protected backups, and only when every one of them + * keeps a ZIP or a verified remote copy. + */ CleanupPlan prepare(WorldId worldId, Snapshot snapshot) throws Exception { BackupId safetyFloor = verifiedSafetyFloor(snapshot) .orElseThrow(() -> new IOException( @@ -59,55 +65,19 @@ CleanupPlan prepare(WorldId worldId, Snapshot snapshot) throws Exception { protectedIds); long target = Math.multiplyExact(policy.budgetBytes(), CLEANUP_TARGET_BUDGET_NUMERATOR) / CLEANUP_TARGET_BUDGET_DENOMINATOR; - long projected = snapshot.totalBytes(); + long gitShare = snapshot.localGitSnapshots().isEmpty() + ? 0 + : snapshot.gitBytes() / snapshot.localGitSnapshots().size(); Map selected = new LinkedHashMap<>(); - for (BackupRecord record : cleanupOrder) { - if (projected <= target) { - break; - } - ZipBackupArtifact zip = snapshot.zipArtifacts().get( - record.manifest().backupId()); - if (zip == null - || !ManagedStorageSupport.managedDestination(record, DestinationType.ZIP)) { - continue; + long projected = deleteUnprotected( + snapshot, cleanupOrder, target, gitShare, selected); + if (projected > target) { + Optional> remoteCopies = executor.protectedRemoteCopiesForGitRemoval( + snapshot, protectedIds, safetyFloor); + if (remoteCopies.isPresent()) { + projected = evictRemainingLocalGit( + snapshot, remoteCopies.orElseThrow(), gitShare, projected, selected); } - long bytes = ManagedStorageSupport.artifactBytes(zip); - CleanupItem item = item(record, new CleanupItemFlags(false, true, false, 0, bytes)); - selected.put(record.manifest().backupId(), item); - projected = Math.max(0, projected - bytes); - } - - Optional> protectedRemoteCopies = projected > target - ? executor.protectedRemoteCopiesForGitRemoval( - snapshot, protectedIds, safetyFloor) - : Optional.empty(); - boolean removeCompleteGit = protectedRemoteCopies.isPresent(); - if (removeCompleteGit) { - Set remoteCopies = protectedRemoteCopies.orElseThrow(); - List localGitIds = snapshot.localGitSnapshots().keySet().stream() - .sorted() - .toList(); - long quotient = localGitIds.isEmpty() - ? 0 - : snapshot.gitBytes() / localGitIds.size(); - long remainder = localGitIds.isEmpty() - ? 0 - : snapshot.gitBytes() % localGitIds.size(); - for (int index = 0; index < localGitIds.size(); index++) { - BackupId backupId = localGitIds.get(index); - BackupRecord record = ManagedStorageSupport.record(snapshot, backupId); - CleanupItem previous = selected.get(backupId); - long estimate = quotient + (index == 0 ? remainder : 0); - selected.put(backupId, item( - record, - new CleanupItemFlags( - true, - previous != null && previous.removeZip(), - remoteCopies.contains(backupId), - estimate, - previous == null ? 0 : previous.exactZipBytes()))); - } - projected = Math.max(0, projected - snapshot.gitBytes()); } OperationId token = OperationId.create(); @@ -126,6 +96,68 @@ CleanupPlan prepare(WorldId worldId, Snapshot snapshot) throws Exception { snapshot.fingerprint()); } + /** Adds whole-backup deletions, lightest first, until the target is met. */ + private static long deleteUnprotected( + Snapshot snapshot, + List cleanupOrder, + long target, + long gitShare, + Map selected) throws IOException { + long projected = snapshot.totalBytes(); + for (BackupRecord record : cleanupOrder) { + if (projected <= target) { + break; + } + BackupId backupId = record.manifest().backupId(); + ZipBackupArtifact zip = snapshot.zipArtifacts().get(backupId); + boolean removeZip = zip != null + && ManagedStorageSupport.managedDestination(record, DestinationType.ZIP); + boolean removeGit = ManagedStorageSupport.managedDestination( + record, DestinationType.GIT); + if (!removeZip && !removeGit) { + continue; + } + long zipBytes = removeZip ? ManagedStorageSupport.artifactBytes(zip) : 0; + long gitBytes = removeGit && snapshot.localGitSnapshots().containsKey(backupId) + ? gitShare + : 0; + selected.put(backupId, item( + record, + new CleanupItemFlags(removeGit, removeZip, false, gitBytes, zipBytes))); + projected = Math.max(0, projected - zipBytes - gitBytes); + } + return projected; + } + + /** Drops the local Git copies that remain after whole-backup deletions, as one group. */ + private static long evictRemainingLocalGit( + Snapshot snapshot, + Set remoteCopies, + long gitShare, + long projected, + Map selected) { + List remainingGitIds = snapshot.localGitSnapshots().keySet().stream() + .filter(backupId -> !selected.containsKey(backupId) + || !selected.get(backupId).removeGit()) + .sorted() + .toList(); + long remaining = projected; + for (BackupId backupId : remainingGitIds) { + BackupRecord record = ManagedStorageSupport.record(snapshot, backupId); + CleanupItem previous = selected.get(backupId); + selected.put(backupId, item( + record, + new CleanupItemFlags( + true, + previous != null && previous.removeZip(), + remoteCopies.contains(backupId), + gitShare, + previous == null ? 0 : previous.exactZipBytes()))); + remaining = Math.max(0, remaining - gitShare); + } + return remaining; + } + private static Optional verifiedSafetyFloor(Snapshot snapshot) { return snapshot.records().stream() .filter(record -> hasVerifiedLocalArtifact(snapshot, record)) diff --git a/src/main/java/dev/ishaanko/worldarchive/storage/management/ManagedStorageService.java b/src/main/java/dev/ishaanko/worldarchive/storage/management/ManagedStorageService.java index c177608..ac94ca7 100644 --- a/src/main/java/dev/ishaanko/worldarchive/storage/management/ManagedStorageService.java +++ b/src/main/java/dev/ishaanko/worldarchive/storage/management/ManagedStorageService.java @@ -19,7 +19,7 @@ import java.util.concurrent.Executor; import java.util.function.Supplier; -/** Measures, forecasts, previews, and explicitly applies managed-local cleanup. */ +/** Measures, forecasts, previews, and explicitly applies storage cleanup. */ public final class ManagedStorageService { private final FileStorageReviewStore reviews; diff --git a/src/test/java/dev/ishaanko/worldarchive/storage/management/CleanupIntegrationTest.java b/src/test/java/dev/ishaanko/worldarchive/storage/management/CleanupIntegrationTest.java new file mode 100644 index 0000000..782758c --- /dev/null +++ b/src/test/java/dev/ishaanko/worldarchive/storage/management/CleanupIntegrationTest.java @@ -0,0 +1,319 @@ +package dev.ishaanko.worldarchive.storage.management; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import dev.ishaanko.worldarchive.catalog.FileBackupCatalog; +import dev.ishaanko.worldarchive.catalog.FileBackupDeletionRegistry; +import dev.ishaanko.worldarchive.config.GitDestinationConfig; +import dev.ishaanko.worldarchive.config.StoragePolicy; +import dev.ishaanko.worldarchive.config.TriggerConfig; +import dev.ishaanko.worldarchive.config.WorldArchiveConfig; +import dev.ishaanko.worldarchive.config.WorldConfig; +import dev.ishaanko.worldarchive.config.ZipDestinationConfig; +import dev.ishaanko.worldarchive.core.BackupCapture; +import dev.ishaanko.worldarchive.core.ProgressListener; +import dev.ishaanko.worldarchive.core.WorldInventory; +import dev.ishaanko.worldarchive.importing.FileBackupImportService; +import dev.ishaanko.worldarchive.importing.FileImportSourceRegistry; +import dev.ishaanko.worldarchive.model.BackupId; +import dev.ishaanko.worldarchive.model.BackupManifest; +import dev.ishaanko.worldarchive.model.BackupRecord; +import dev.ishaanko.worldarchive.model.BackupResult; +import dev.ishaanko.worldarchive.model.BackupTrigger; +import dev.ishaanko.worldarchive.model.DestinationResult; +import dev.ishaanko.worldarchive.model.DestinationType; +import dev.ishaanko.worldarchive.model.SyncStatus; +import dev.ishaanko.worldarchive.model.VerificationStatus; +import dev.ishaanko.worldarchive.model.WorldId; +import dev.ishaanko.worldarchive.storage.git.GitBackendSettings; +import dev.ishaanko.worldarchive.storage.git.SystemGitCommandRunner; +import dev.ishaanko.worldarchive.storage.git.WorldGitSnapshotStore; +import dev.ishaanko.worldarchive.storage.zip.ZipBackupArtifact; +import dev.ishaanko.worldarchive.storage.zip.ZipBackupStore; +import dev.ishaanko.worldarchive.storage.zip.ZipBackupStoreResolver; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.security.MessageDigest; +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.time.ZoneOffset; +import java.util.ArrayList; +import java.util.HexFormat; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.CompletionStage; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +/** Cleanup against real Git and Git LFS: what is deleted, what stays, what survives a restart. */ +final class CleanupIntegrationTest { + private static final Instant NOW = Instant.parse("2026-09-10T12:00:00Z"); + + /** One byte of budget forces cleanup to go as far as the keep settings allow. */ + private static final StoragePolicy KEEP_ONE_DAILY = new StoragePolicy(1, 1, 0, 0); + + private static final StoragePolicy KEEP_ONLY_SAFETY_FLOOR = new StoragePolicy(1, 0, 0, 0); + + @TempDir + Path temporaryDirectory; + + private final ExecutorService executor = Executors.newCachedThreadPool(); + + private WorldId worldId; + + private Path remote; + + private WorldGitSnapshotStore git; + + private ZipBackupStore zipStore; + + private FileBackupCatalog catalog; + + private FileBackupDeletionRegistry deletions; + + @BeforeEach + void setUp() throws Exception { + worldId = WorldId.create(); + remote = temporaryDirectory.resolve("remote.git"); + nativeGit("init", "--bare", remote.toString()); + git = new WorldGitSnapshotStore( + new GitBackendSettings( + true, + temporaryDirectory.resolve("git"), + "git", + "origin", + Optional.empty(), + GitDestinationConfig.DEFAULT_LFS_PATTERNS, + GitBackendSettings.DEFAULT_COMMAND_TIMEOUT, + GitBackendSettings.DEFAULT_MAXIMUM_OUTPUT_BYTES), + Optional.empty(), + Map.of(worldId, remote.toUri().toString()), + new SystemGitCommandRunner(), + executor); + Assumptions.assumeTrue(await(git.probeTools()).available(), "git and git-lfs required"); + zipStore = new ZipBackupStore(temporaryDirectory.resolve("archives")); + catalog = new FileBackupCatalog(temporaryDirectory.resolve("catalog.json")); + deletions = new FileBackupDeletionRegistry(temporaryDirectory.resolve("deleted.txt")); + } + + @AfterEach + void tearDown() { + git.close(); + executor.shutdownNow(); + } + + @Test + void unprotectedBackupsAreDeletedEverywhereAndTheirLfsObjectsFreed() throws Exception { + BackupId oldest = backup(3, true); + BackupId middle = backup(2, true); + BackupId newest = backup(1, true); + assertEquals(6, lfsObjectCount()); + + ManagedStorageService service = service(KEEP_ONE_DAILY); + CleanupPlan plan = await(service.prepareCleanup(worldId)); + assertEquals(Set.of(newest), plan.protectedBackups()); + Map items = plan.items().stream() + .collect(Collectors.toMap(CleanupItem::backupId, item -> item)); + for (BackupId deleted : List.of(oldest, middle)) { + assertTrue(items.get(deleted).removeGit() && items.get(deleted).removeZip()); + assertTrue(items.get(deleted).removesRestorePoint()); + } + assertFalse(items.get(newest).removesRestorePoint(), + "the protected backup is only offered as a local Git eviction"); + + CleanupResult result = await(service.applyCleanup( + new CleanupRequest(plan.confirmationToken(), Set.of(oldest, middle)))); + assertEquals(Map.of(), result.failures()); + + assertEquals(List.of(newest), catalog.list(worldId).stream() + .map(record -> record.manifest().backupId()).toList()); + assertEquals(Set.of(newest), await(git.listCurrentSnapshots(worldId)).stream() + .map(snapshot -> snapshot.backupId()).collect(Collectors.toSet())); + assertEquals(Set.of(newest), remoteBackupIds()); + assertEquals(1, zipStore.listArchives().size()); + assertTrue(deletions.contains(oldest) && deletions.contains(middle)); + assertEquals(2, lfsObjectCount(), "only the newest snapshot's objects remain"); + assertTrue(await(git.verifyCurrentSnapshot(worldId, newest)).valid()); + assertTrue(result.reclaimedBytes() > 0); + + imports().rebuildLocal().toCompletableFuture().get(30, TimeUnit.SECONDS); + assertEquals(List.of(newest), catalog.list(worldId).stream() + .map(record -> record.manifest().backupId()).toList()); + } + + @Test + void protectedBackupKeepsItsRemoteCopyWhenLocalGitIsEvicted() throws Exception { + BackupId older = backup(2, true); + BackupId newest = backup(1, false); + + ManagedStorageService service = service(KEEP_ONLY_SAFETY_FLOOR); + CleanupPlan plan = await(service.prepareCleanup(worldId)); + assertEquals(Set.of(older), plan.protectedBackups(), + "the verified safety floor is the only backup with a ZIP"); + Map items = plan.items().stream() + .collect(Collectors.toMap(CleanupItem::backupId, item -> item)); + assertTrue(items.get(newest).removeGit() && !items.get(newest).removeZip()); + assertTrue(items.get(newest).removesRestorePoint()); + assertTrue(items.get(older).removeGit() && !items.get(older).removeZip()); + assertFalse(items.get(older).removesRestorePoint()); + + CleanupResult result = await(service.applyCleanup( + new CleanupRequest(plan.confirmationToken(), items.keySet()))); + assertEquals(Map.of(), result.failures()); + + assertEquals(Set.of(older), remoteBackupIds(), "the unprotected backup left the remote"); + assertTrue(await(git.listCurrentSnapshots(worldId)).isEmpty()); + assertEquals(0, lfsObjectCount()); + BackupRecord kept = catalog.find(older).orElseThrow(); + assertEquals( + List.of(DestinationType.GIT, DestinationType.ZIP), + kept.result().destinations().stream() + .map(DestinationResult::destination).toList()); + assertFalse(catalog.find(newest).isPresent()); + } + + private BackupId backup(int daysAgo, boolean withZip) throws Exception { + BackupId backupId = BackupId.create(); + Path source = Files.createDirectories(temporaryDirectory.resolve("world-" + backupId)); + byte[] level = ("level " + backupId).getBytes(StandardCharsets.UTF_8); + byte[] region = ("region " + backupId).getBytes(StandardCharsets.UTF_8); + Files.write(source.resolve("level.dat"), level); + Files.write(source.resolve("r.0.0.mca"), region); + WorldInventory inventory = WorldInventory.create(List.of( + new WorldInventory.Entry("level.dat", level.length, sha256(level)), + new WorldInventory.Entry("r.0.0.mca", region.length, sha256(region)))); + BackupManifest manifest = BackupManifest.create( + backupId, + worldId, + "Cleanup World", + Optional.empty(), + NOW.minus(Duration.ofDays(daysAgo)), + BackupTrigger.SCHEDULED, + inventory.fileCount(), + inventory.byteCount(), + inventory.fileCount(), + inventory.contentSha256(), + inventory.inventorySha256()); + BackupCapture capture = new BackupCapture(source, manifest); + DestinationResult gitResult = await(git.createBackup(capture, ProgressListener.NO_OP)); + assertEquals(SyncStatus.SYNCED, gitResult.syncStatus()); + List destinations = new ArrayList<>(); + destinations.add(gitResult); + if (withZip) { + ZipBackupArtifact artifact = zipStore.create(capture); + destinations.add(DestinationResult.success(DestinationType.ZIP, artifact.artifactId()) + .withVerification(VerificationStatus.VERIFIED)); + } + catalog.add(new BackupRecord( + manifest, + BackupResult.aggregate(backupId, worldId, destinations, NOW))); + return backupId; + } + + private ManagedStorageService service(StoragePolicy policy) { + WorldConfig world = new WorldConfig( + worldId, + true, + temporaryDirectory.resolve("live-world"), + Optional.of(remote.toUri().toString()), + Optional.empty(), + policy); + WorldArchiveConfig config = new WorldArchiveConfig( + WorldArchiveConfig.CURRENT_SCHEMA_VERSION, + TriggerConfig.defaults(), + GitDestinationConfig.defaults(), + ZipDestinationConfig.defaults(), + List.of(world)); + return new ManagedStorageService( + () -> config, + catalog, + deletions, + git, + zipStore, + new FileStorageHistoryStore(temporaryDirectory.resolve("history")), + new FileStorageReviewStore(temporaryDirectory.resolve("reviews")), + ignored -> () -> { + }, + executor, + Clock.fixed(NOW, ZoneOffset.UTC), + ZoneOffset.UTC); + } + + /** The catalog rebuild that runs at every game start. */ + private FileBackupImportService imports() { + ZipBackupStoreResolver stores = new ZipBackupStoreResolver() { + @Override + public ZipBackupStore store(WorldId ignored) { + return zipStore; + } + + @Override + public ZipBackupStore defaultStore() { + return zipStore; + } + }; + return new FileBackupImportService( + catalog, + new FileImportSourceRegistry(temporaryDirectory.resolve("sources.json")), + deletions, + git, + stores, + () -> Set.of(worldId), + executor); + } + + private Set remoteBackupIds() throws Exception { + return nativeGit("--git-dir=" + remote, "for-each-ref", "--format=%(refname)", + "refs/heads/backups/") + .lines() + .filter(line -> !line.isBlank()) + .map(line -> BackupId.parse(line.substring(line.length() - 36))) + .collect(Collectors.toSet()); + } + + private long lfsObjectCount() throws Exception { + Path objects = git.repositoryFor(worldId).resolve("lfs").resolve("objects"); + if (!Files.isDirectory(objects)) { + return 0; + } + try (Stream paths = Files.walk(objects)) { + return paths.filter(Files::isRegularFile).count(); + } + } + + private String nativeGit(String... arguments) throws Exception { + List command = new ArrayList<>(); + command.add("git"); + command.addAll(List.of(arguments)); + Process process = new ProcessBuilder(command) + .directory(temporaryDirectory.toFile()) + .redirectErrorStream(true) + .start(); + String output = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8); + assertTrue(process.waitFor(30, TimeUnit.SECONDS)); + assertEquals(0, process.exitValue(), output); + return output; + } + + private static T await(CompletionStage stage) throws Exception { + return stage.toCompletableFuture().get(60, TimeUnit.SECONDS); + } + + private static String sha256(byte[] bytes) throws Exception { + return HexFormat.of().formatHex(MessageDigest.getInstance("SHA-256").digest(bytes)); + } +} diff --git a/src/test/java/dev/ishaanko/worldarchive/storage/management/ManagedStorageServiceTest.java b/src/test/java/dev/ishaanko/worldarchive/storage/management/ManagedStorageServiceTest.java index 2374c78..2802b44 100644 --- a/src/test/java/dev/ishaanko/worldarchive/storage/management/ManagedStorageServiceTest.java +++ b/src/test/java/dev/ishaanko/worldarchive/storage/management/ManagedStorageServiceTest.java @@ -350,7 +350,7 @@ void staleSynchronizedStatusCannotAuthorizeLastLocalGitCopyRemoval() CleanupPlan plan = await(fixture.service().prepareCleanup(worldId)); assertTrue(plan.items().stream() - .noneMatch(CleanupItem::removeLocalGit)); + .noneMatch(CleanupItem::removeGit)); assertTrue(Files.exists(repository.resolve("objects.bin"))); } } From 90160664546ae491c1026216fd89438621635650 Mon Sep 17 00:00:00 2001 From: ishaanko Date: Thu, 10 Sep 2026 14:27:27 -0700 Subject: [PATCH 2/2] fix(git): drop the history ref when compaction follows a deletion --- .../storage/git/GitStorageCompactor.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/main/java/dev/ishaanko/worldarchive/storage/git/GitStorageCompactor.java b/src/main/java/dev/ishaanko/worldarchive/storage/git/GitStorageCompactor.java index 99c7e43..d27bfd4 100644 --- a/src/main/java/dev/ishaanko/worldarchive/storage/git/GitStorageCompactor.java +++ b/src/main/java/dev/ishaanko/worldarchive/storage/git/GitStorageCompactor.java @@ -44,12 +44,15 @@ final class GitStorageCompactor { } /** - * Reclaims space after snapshot refs were deleted. Snapshot commits chain onto - * each other, so Git's own reachability keeps every older commit alive and - * {@code git lfs prune} cannot tell a deleted snapshot from a kept one. The LFS - * objects that stay are therefore computed exactly: every object that a - * remaining snapshot's tree points at. Anything else under {@code lfs/objects} - * is removed. When the world has no snapshots left its history ref goes too. + * Reclaims space after snapshot refs were deliberately deleted. Snapshot commits + * chain onto each other, so Git's own reachability keeps every older commit alive + * and {@code git lfs prune} cannot tell a deleted snapshot from a kept one. The LFS + * objects that stay are therefore computed exactly: every object that a remaining + * snapshot's tree points at. Anything else under {@code lfs/objects} is removed. + * + *

The world's history ref exists so an import can rebuild snapshot refs that + * went missing by accident. After a deletion it would rebuild the deleted snapshot + * with its content gone, so it is removed here as well. */ void compact(WorldId worldId, List remainingSnapshots) throws IOException, InterruptedException, GitStorageException { @@ -59,11 +62,7 @@ void compact(WorldId worldId, List remainingSnapshots) retained.add(pointer.sha256()); } } - boolean worldEmpty = remainingSnapshots.stream() - .noneMatch(snapshot -> snapshot.worldId().equals(worldId)); - if (worldEmpty) { - refs.deleteIfPresent(repository.historyRef(worldId)); - } + refs.deleteIfPresent(repository.historyRef(worldId)); commands.checked( List.of( "reflog",