diff --git a/src/borg/archiver/compact_cmd.py b/src/borg/archiver/compact_cmd.py index cc6e880088..a6e942a91d 100644 --- a/src/borg/archiver/compact_cmd.py +++ b/src/borg/archiver/compact_cmd.py @@ -33,6 +33,7 @@ def __init__(self, repository, manifest, *, stats, threshold, dry_run=False): self.total_files = None # overall number of source files written to all archives in this repo self.total_size = None # overall size of source file content data written to all archives self.archives_count = None # number of archives + self.archive_series_names = None # names of the existing archives, set by analyze_archives() self.stats = stats # compute repo space usage before/after - lists all repo objects, can be slow. self.threshold = threshold # rewrite a mixed pack only when its wasted-bytes fraction reaches this percent self.dry_run = dry_run @@ -62,6 +63,12 @@ def get_repository_chunks(self) -> ChunkIndex: chunks = build_chunkindex_from_repo( self.repository, write_immediately=not self.dry_run, init_flags=ChunkIndex.F_NONE ) + # Hand this index to the repository as well, so reading the archives below does not lazily + # build a second, identical copy of the biggest structure borg keeps in memory (see the + # .chunks property). The repository only reads pack locations and F_PENDING from it, never + # the F_USED flags or sizes this index tracks for compaction and --stats. It stays shared + # until compact_packs() invalidates the chunk index before its first store change. + self.repository.chunks = chunks return chunks def save_chunk_index(self): @@ -69,6 +76,8 @@ def save_chunk_index(self): # and also remove all older chunk indexes. # write_chunkindex_to_repo now removes all flags and size infos. # we need this, as we put the wrong size in there to support --stats computations. + # clear=True empties the index in place: safe, the repository dropped its reference to it in + # compact_packs() before the first store change, so it cannot see an empty index here. write_chunkindex_to_repo( self.repository, self.chunks, incremental=False, clear=True, force_write=True, delete_other=True ) @@ -78,9 +87,14 @@ def cleanup_files_cache(self): """ Clean up files cache files for archive series names that no longer exist in the repository. + Works from the archive names analyze_archives() collected, so this needs no repository access: + it runs after save_chunk_index() has cleared the chunk index, and the archive set does not + change in between (compaction only removes soft-deleted archives, which were never in it). + Note: this only works perfectly if the files cache filename suffixes are automatically generated and the user does not manually control them via more than one BORG_FILES_CACHE_SUFFIX env var value. """ + assert self.archive_series_names is not None, "analyze_archives() must run first" logger.info("Cleaning up files cache...") cache_dir = Path(get_cache_dir(self.repository.id_str, create=False)) @@ -89,7 +103,7 @@ def cleanup_files_cache(self): return # Get all existing archive series names - existing_series = set(self.manifest.archives.names()) + existing_series = self.archive_series_names logger.debug(f"Found {len(existing_series)} existing archive series.") # Get the set of all existing files cache file names. @@ -141,6 +155,9 @@ def analyze_archives(self) -> tuple[set, int, int, int]: missing_chunks: set[bytes] = set() archive_infos = self.manifest.archives.list(sort_by=["ts"]) num_archives = len(archive_infos) + # an archive's name is its series name; cleanup_files_cache() needs these later, and reading + # every archive's metadata a second time just to get them again would be wasteful. + self.archive_series_names = {info.name for info in archive_infos} cached_hex_ids = list_archive_reference_caches(self.repository) if not self.dry_run: # drop the reference caches of archives that do not exist anymore. @@ -365,7 +382,11 @@ def compact_packs(self): logger.info("Deleting 0 unused objects...") return repo_size_before, repo_size_before # nothing worth doing; chunk indexes stay valid - # crash-safety (#9748): invalidate chunk indexes before the first store change + # crash-safety (#9748): invalidate chunk indexes before the first store change. This also + # drops the repository's reference to self.chunks (shared since get_repository_chunks()). + # Do not hand it back: until save_chunk_index() has written the updated index, the repo + # must hold no in-memory index that close() could persist on an aborted run. Nothing below + # needs one, compact_pack() and merge_packs() work on chunks=self.chunks. delete_chunkindex_from_repo(self.repository) self.store_changed = True diff --git a/src/borg/testsuite/archiver/compact_cmd_test.py b/src/borg/testsuite/archiver/compact_cmd_test.py index 39482a8718..f6dc4bf281 100644 --- a/src/borg/testsuite/archiver/compact_cmd_test.py +++ b/src/borg/testsuite/archiver/compact_cmd_test.py @@ -14,7 +14,9 @@ from ...cache import delete_chunkindex_from_repo, write_chunkindex_to_repo from ...manifest import Manifest from ...archive import Archive +from ...archiver import compact_cmd from ...archiver.compact_cmd import ArchiveGarbageCollector +from ... import cache from . import cmd, create_regular_file, create_src_archive, generate_archiver_tests, open_repository, RK_ENCRYPTION from . import changedir from ..repository_test import H, fchunk, pdchunk @@ -763,3 +765,49 @@ def test_compact_files_cache_cleanup(archivers, request): # Get expected cache files for remaining archives expected_cache_files = {files_cache_name(name) for name in ["archive1", "archive3"]} assert expected_cache_files == remaining_cache_files, "Unexpected cache files found" + + +def test_compact_builds_the_chunk_index_only_once(archivers, request, monkeypatch): + """The chunk index is the biggest structure borg keeps in memory, so compact must build one, not + several copies of it. + + Compact builds its own index (it needs the usage flags) and hands it to the repository, so reading + the archives resolves pack locations through that same index instead of lazily building a second, + identical one. + """ + archiver = request.getfixturevalue(archivers) + + # file_a only ever belongs to archive1, file_b to both: deleting archive1 leaves file_a's chunks + # unused next to still-used objects in the same pack, so compaction rewrites that pack. + create_regular_file(archiver.input_path, "file_a", contents=os.urandom(1024 * 1024)) + create_regular_file(archiver.input_path, "file_b", contents=os.urandom(1024 * 1024)) + cmd(archiver, "repo-create", RK_ENCRYPTION) + cmd(archiver, "create", "archive1", "input") + os.remove(os.path.join(archiver.input_path, "file_a")) + cmd(archiver, "create", "archive2", "input") + cmd(archiver, "delete", "-a", "archive1") + + builds = 0 + original_build = cache.build_chunkindex_from_repo + + def counting_build(repository, **kwargs): + nonlocal builds + builds += 1 + return original_build(repository, **kwargs) + + # the repository imports the function inside its .chunks property, compact_cmd at module level: + monkeypatch.setattr(cache, "build_chunkindex_from_repo", counting_build) + monkeypatch.setattr(compact_cmd, "build_chunkindex_from_repo", counting_build) + + repository = open_repository(archiver) + with repository: + manifest = Manifest.load(repository, (Manifest.Operation.DELETE,)) + gc = ArchiveGarbageCollector(repository, manifest, stats=True, threshold=0.0) + gc.garbage_collect() + assert gc.store_changed, "this repo must really get compacted, or the test proves nothing" + + assert builds == 1 + + # the repository is still intact and the surviving archive still reads back + cmd(archiver, "check") + cmd(archiver, "list", "archive2")