compact: build the chunk index once, not three times

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,13 +63,21 @@ 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):
         # as we may have deleted some chunks, we must write a full updated chunkindex to the repo
         # 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
 

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")