From 99ce8535673d1f94f8df87fef386b935514f6da9 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sat, 12 Sep 2026 22:47:16 +0200 Subject: [PATCH 1/2] compact: build the chunk index once, not three times "borg compact" built the chunk index up to three times per run and kept two copies of it in memory at the same time: 1. get_repository_chunks() builds compact's own index (it needs the usage flags). 2. analyze_archives() reads the archive metadata objects, and resolving their pack locations lazily builds a second, identical index inside the Repository. 3. compact_packs() calls delete_chunkindex_from_repo(), which drops the repository's in-memory index, so the archive listing in cleanup_files_cache() built a third one. The chunk index is the biggest structure borg keeps in memory (about 80 bytes per chunk plus hash table overhead), so a large repository paid for that twice over. Share compact's index with the repository while the repository reads through it: get_repository_chunks() installs it as repository.chunks, so analyze_archives() resolves pack locations through that same index. The repository only reads pack locations and F_PENDING from an index, never the F_USED flags or the sizes compact tracks for --stats, and stored index fragments zero the flags either way - so the index the repository would have built is the very same one. The sharing ends in compact_packs(): delete_chunkindex_from_repo() (#9748) drops the repository's reference before the first store change, and compact does not hand it back. Nothing needs it there - compact_pack() and merge_packs() get chunks=self.chunks, --stats and save_chunk_index() use self.chunks directly. So while the persisted index is gone, the repository holds no in-memory index that Repository.close() could persist on an aborted run, and save_chunk_index() can empty the index in place with clear=True. cleanup_files_cache() used to list the archives itself, after save_chunk_index() had cleared the index, which is what built the third index. It now reuses the names analyze_archives() already collected, so it needs no repository access at all - and every archive's metadata is read once per compact run instead of twice. Verified on the same repository: master and this produce byte-identical repositories, identical persisted index/* fragments (completed, dry-run, no-op, interrupted and slow-rebuild runs alike) and identical output, with 3 index builds before and 1 after. Co-Authored-By: Claude Fable 5.1 Co-Authored-By: Claude Opus 5 --- src/borg/archiver/compact_cmd.py | 25 +++++++++- .../testsuite/archiver/compact_cmd_test.py | 48 +++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) 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") From 0e974b26f8654f986c06e0e6e6329f8f91260410 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Mon, 14 Sep 2026 17:25:11 +0200 Subject: [PATCH 2/2] compact: review follow-up - docs match every compact path, test covers them Review feedback on the shared chunk index: - Repository.chunks: the docstring said the index is always built lazily and the setter comment listed the callers that install one; compact now does that too, say so. - compact_cmd.get_repository_chunks(): the comment claimed the sharing ends when compact_packs() invalidates the index, but on a dry run and when there is nothing to compact nothing is invalidated and the index stays shared 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. Describe all of it. - cleanup_files_cache(): it runs after save_chunk_index() only when the store changed, so "may run after". - test_compact_builds_the_chunk_index_only_once() covered one path through compact_packs(). Parametrize it over the four: a pack rewrite, a dry run, a slow rebuild from the packs with no index fragments in the repo, and a merge of tiny packs. Each builds the index once now; against master's compact they build it 3, 2, 3 and 3 times, so each scenario fails there on its own. Co-Authored-By: Claude Fable 5.1 --- src/borg/archiver/compact_cmd.py | 10 +++-- src/borg/repository.py | 13 +++--- .../testsuite/archiver/compact_cmd_test.py | 45 +++++++++++++------ 3 files changed, 47 insertions(+), 21 deletions(-) diff --git a/src/borg/archiver/compact_cmd.py b/src/borg/archiver/compact_cmd.py index a6e942a91d..4ba33d1af6 100644 --- a/src/borg/archiver/compact_cmd.py +++ b/src/borg/archiver/compact_cmd.py @@ -67,7 +67,10 @@ def get_repository_chunks(self) -> ChunkIndex: # 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. + # 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 @@ -88,8 +91,9 @@ 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). + 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. 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 f6dc4bf281..0257fddcf8 100644 --- a/src/borg/testsuite/archiver/compact_cmd_test.py +++ b/src/borg/testsuite/archiver/compact_cmd_test.py @@ -767,25 +767,42 @@ def test_compact_files_cache_cleanup(archivers, request): assert expected_cache_files == remaining_cache_files, "Unexpected cache files found" -def test_compact_builds_the_chunk_index_only_once(archivers, request, monkeypatch): +@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. + 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) - # 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") + 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 @@ -799,15 +816,17 @@ def counting_build(repository, **kwargs): 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) + gc = ArchiveGarbageCollector(repository, manifest, stats=True, threshold=0.0, dry_run=dry_run) gc.garbage_collect() - assert gc.store_changed, "this repo must really get compacted, or the test proves nothing" + # 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", "archive2") + cmd(archiver, "list", survivor)