check: use one chunk index for the checker and the repository, refs #10364

src/borg/archive.py

@@ -2226,6 +2226,8 @@ def check(
             validate = object_validator(self.repo_objs)
         else:
             validate = None
+        # free the chunk index the repository check may have loaded, so only one is in memory.
+        self.repository.invalidate_chunk_index()
         self.chunks = build_chunkindex_from_repo(
             self.repository,
             slow_rebuild=repair,
@@ -2239,10 +2241,16 @@ def check(
             drop_corrupt_tail=repair,
             write_immediately=False,
         )
-        # repository.chunks is a separate index, lazily built when repository.get() resolves a
-        # chunk location. It walks the same packs, so give it the same corrupt-header handling the
-        # rebuild above got - otherwise the check aborts at a header it just resynced past, halfway
-        # through its diagnosis. Dropping the rest of that pack stays a --repair action.
+        if repair:
+            # the rebuild from the packs sets F_NEW (entry not stored in the index/ fragments yet) on every
+            # entry. finish() stores the complete index and deletes the old fragments. Clear F_NEW, so
+            # Repository.close() does not store the entries again as an extra fragment.
+            self.chunks.clear_new()
+        # the repository uses this index: get() looks up pack locations in it, put() adds entries to
+        # it, delete() removes entries from it.
+        self.repository.chunks = self.chunks
+        # corrupt object header handling for a rebuild of repository.chunks after invalidate_chunk_index():
+        # the same as for the rebuild above.
         self.repository.chunkindex_validate = validate
         self.repository.chunkindex_drop_corrupt_tail = repair
         if self.key is None:
@@ -2374,10 +2382,10 @@ def verify_data(self):
                         # failed twice -> remove this defect chunk. delete rewrites its pack without it,
                         # keeping the other chunks. update_index=False: finish() rebuilds the index from
                         # the rewritten packs anyway, so a per-chunk full index write would be wasted.
+                        # delete() also removes the chunk from self.chunks, so rebuild_archives reports
+                        # the file it belongs to.
                         self.repository.delete(defect_chunk, update_index=False, validate=validate)
                         self.chunks_modified = True
-                        # drop it from our own index too, so rebuild_archives reports the file it belongs to.
-                        del self.chunks[defect_chunk]
                     else:
                         logger.warning("chunk %s not deleted, did not consistently fail.", bin_to_hex(defect_chunk))
             else:
@@ -2525,14 +2533,11 @@ def add_callback(chunk):
             return id_
 
         def add_reference(id_, size, cdata):
-            # either we already have this chunk in repo and chunks index or we add it now
-            if id_ not in self.chunks:
+            # --repair: store a chunk the repository does not have. put() adds it to self.chunks.
+            if self.repair and id_ not in self.chunks:
                 assert cdata is not None
-                self.chunks.add(id_, size)
-                if self.repair:
-                    pack_results = self.repository.put(id_, cdata)
-                    self.chunks.update_pack_info(pack_results)
-                    self.chunks_modified = True
+                self.repository.put(id_, cdata)
+                self.chunks_modified = True
 
         def verify_file_chunks(archive_name, item):
             """Verify that all of a file's chunks are present, collecting any missing ones for the report."""
@@ -2750,9 +2755,10 @@ def finish(self):
             # writer buffer (close() requires an empty buffer, #10055) before we (re)build the index.
             self.repository.flush()
             if self.chunks_modified:
-                # the packs changed, so the index no longer matches them: rebuild it from the packs
-                # and persist it: deleting a defect chunk rewrites its pack and repoints that
-                # pack's other objects in the repository's index, so our offsets for them are stale.
+                # the packs changed: rebuild the index from them, validating every object header, and
+                # store it. Free the current index first, so only one is in memory.
+                self.repository.invalidate_chunk_index()
+                self.chunks = None
                 logger.info("Rebuilding and writing the repository chunks index.")
                 build_chunkindex_from_repo(
                     self.repository,

src/borg/testsuite/archiver/check_cmd_test.py

@@ -462,6 +462,77 @@ def test_missing_archive_metadata(archivers, request):
     cmd(archiver, "check", exit_code=0)
 
 
+@pytest.mark.parametrize(
+    "args, exit_code", [(["--archives-only"], 1), ([], 1), (["--repair"], 0)], ids=["archives-only", "full", "repair"]
+)
+def test_check_holds_a_single_chunk_index(archiver, monkeypatch, args, exit_code):
+    """check has at most one chunk index in memory: the repository uses the index the checker builds."""
+    # local-only: this patches in-process archive and repository internals.
+    check_cmd_setup(archiver)
+    # with an item metadata chunk missing, --repair stores a new item metadata stream.
+    archive, repository = open_archive(archiver.repository_path, "archive1")
+    with repository:
+        repository.delete(archive.item_ids[0], validate=None)
+
+    loaded_at_build = []  # per checker index build: whether repository.chunks was loaded at that time
+    real_build = archive_module.build_chunkindex_from_repo
+
+    def build_chunkindex_from_repo(repository, **kwargs):
+        loaded_at_build.append(repository.is_chunk_index_loaded)
+        return real_build(repository, **kwargs)
+
+    repository_builds = 0  # index builds by the Repository.chunks property
+    real_chunks = Repository.chunks
+
+    def chunks(self):
+        nonlocal repository_builds
+        if not self.is_chunk_index_loaded:
+            repository_builds += 1
+        return real_chunks.fget(self)
+
+    same_index = []  # per rebuild_archives call: whether repository.chunks is the checker's index
+    real_rebuild_archives = ArchiveChecker.rebuild_archives
+
+    def rebuild_archives(self, **kwargs):
+        same_index.append(self.repository.chunks is self.chunks)
+        return real_rebuild_archives(self, **kwargs)
+
+    monkeypatch.setattr(archive_module, "build_chunkindex_from_repo", build_chunkindex_from_repo)
+    monkeypatch.setattr(Repository, "chunks", property(chunks, real_chunks.fset))
+    monkeypatch.setattr(ArchiveChecker, "rebuild_archives", rebuild_archives)
+    cmd(archiver, "check", *args, exit_code=exit_code)
+
+    # builds: in check(), and with --repair also in finish().
+    assert loaded_at_build == ([False, False] if "--repair" in args else [False])
+    assert same_index == [True]
+    assert repository_builds == 0
+    if "--repair" in args:
+        cmd(archiver, "check", exit_code=0)
+
+
+def test_check_without_repair_leaves_the_chunk_index_alone(archivers, request):
+    """check without --repair does not change the chunk index.
+
+    The archive has an item metadata chunk missing: the checker re-chunks its item metadata stream into
+    chunks the repository does not have.
+    """
+    archiver = request.getfixturevalue(archivers)
+    check_cmd_setup(archiver)
+    archive, repository = open_archive(archiver.repository_path, "archive1")
+    with repository:
+        repository.delete(archive.item_ids[0], validate=None)
+    with Repository(archiver.repository_location, exclusive=True) as repository:
+        index_before = {info.name for info in repository.store_list("index")}
+        chunk_ids_before = {chunk_id for chunk_id, _ in repository.chunks.iteritems()}
+
+    cmd(archiver, "check", "--archives-only", exit_code=1)
+    cmd(archiver, "check", exit_code=1)
+
+    with Repository(archiver.repository_location, exclusive=True) as repository:
+        assert {info.name for info in repository.store_list("index")} == index_before
+        assert {chunk_id for chunk_id, _ in repository.chunks.iteritems()} == chunk_ids_before
+
+
 def test_check_format(archivers, request):
     archiver = request.getfixturevalue(archivers)
     check_cmd_setup(archiver)