diff --git a/src/borg/archiver/compact_cmd.py b/src/borg/archiver/compact_cmd.py index cc6e880088..4ba33d1af6 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,15 @@ 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 - or, on + # a dry run or when there is nothing to compact, until close(), which then persists only + # what the build flagged F_NEW: nothing after reading the fragments, everything after a + # slow rebuild from the packs (exactly what the lazy .chunks build would have done). + self.repository.chunks = chunks return chunks def save_chunk_index(self): @@ -69,6 +79,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 +90,15 @@ 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 may run after save_chunk_index() has cleared the chunk index (it does whenever the store + changed), 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 +107,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 +159,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 +386,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/repository.py b/src/borg/repository.py index ac81dae7e8..cbe00a69da 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -1070,8 +1070,9 @@ def chunks(self): This property is the single owner of the in-memory index: get() resolves pack locations through it, PackWriter updates it, and the Cache reads it - from here rather than building its own. Built lazily on first access and - persisted back to the repo cache at close(). + from here rather than building its own. Built lazily on first access, unless a + caller installed one through the setter, and persisted back to the repo cache at + close(). """ if self._chunks is None: from .cache import build_chunkindex_from_repo @@ -1084,9 +1085,11 @@ def chunks(self): @chunks.setter def chunks(self, value): # The index is normally built lazily; this setter exists for the few callers - # that must install a specific index (e.g. wiping the cache, or restoring an - # index captured before close()). To drop a stale index so it rebuilds, do not - # assign None here -- call invalidate_chunk_index() instead. + # that must install a specific index: wiping the cache, restoring an index + # captured before close(), or compact sharing the index it built itself (it + # needs the usage flags) so the repository does not build a second one. To + # drop a stale index so it rebuilds, do not assign None here -- call + # invalidate_chunk_index() instead. self._chunks = value def invalidate_chunk_index(self): diff --git a/src/borg/testsuite/archiver/compact_cmd_test.py b/src/borg/testsuite/archiver/compact_cmd_test.py index 39482a8718..0257fddcf8 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,68 @@ 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" + + +@pytest.mark.parametrize("scenario", ("rewrite", "dry_run", "slow_rebuild", "merge")) +def test_compact_builds_the_chunk_index_only_once(archivers, request, monkeypatch, scenario): + """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. Every path through compact_packs() must keep it at one build: a pack rewrite, a dry + run (nothing is invalidated, the shared index lives until close()), a slow rebuild from the packs + when the repo has no index fragments, and a merge of tiny packs. + """ + archiver = request.getfixturevalue(archivers) + + cmd(archiver, "repo-create", RK_ENCRYPTION) + if scenario == "merge": + # many tiny, fully used packs: nothing to reclaim, but their combined size reaches a full pack, + # so compact_packs() takes the merge path (see test_compact_packs_merges_tiny_packs). + monkeypatch.setenv("BORG_PACK_MAX_SIZE", "100000") + for i in range(12): + create_regular_file(archiver.input_path, "file", contents=os.urandom(12 * 1024)) + cmd(archiver, "create", f"archive{i}", "input") + survivor = "archive11" + else: + # 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, "create", "archive1", "input") + os.remove(os.path.join(archiver.input_path, "file_a")) + cmd(archiver, "create", "archive2", "input") + cmd(archiver, "delete", "-a", "archive1") + survivor = "archive2" + if scenario == "slow_rebuild": + # no index/* left: the only way to get an index is the slow walk over the pack headers. + with open_repository(archiver) as repository: + delete_chunkindex_from_repo(repository) + + 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) + + dry_run = scenario == "dry_run" + repository = open_repository(archiver) + with repository: + manifest = Manifest.load(repository, (Manifest.Operation.DELETE,)) + gc = ArchiveGarbageCollector(repository, manifest, stats=True, threshold=0.0, dry_run=dry_run) + gc.garbage_collect() + # the store must really change (or, on the dry run, really not), or the test proves nothing + assert gc.store_changed is (not dry_run) + + assert builds == 1 + + # the repository is still intact and the surviving archive still reads back + cmd(archiver, "check") + cmd(archiver, "list", survivor)