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,24 @@ 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):
         # 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 +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
 

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

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)