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

docs/internals/data-structures.rst

@@ -90,9 +90,11 @@ cache/
     of an unchanged archive.
   chunkindex-invalid
     a marker object: while it is present, the chunks index in ``index/`` is considered
-    invalid. It is written before deleting index fragments and removed after the last
-    stale fragment is gone, so an interrupted run does not leave the remaining
-    fragments looking like a complete index.
+    invalid, because its fragments may be missing entries or point at deleted packs.
+    It is written before deleting index fragments, before a single-object delete removes
+    the old pack, and before ``borg check --repair`` rebuilds the index after changing
+    the packs. It is removed after the last fragment is deleted or once the complete
+    current index is stored.
 
 Note that this ``cache/`` namespace is inside the repository (and thus shared by
 all clients); it is not the client-local cache described in

docs/internals/packs.rst

@@ -322,11 +322,14 @@ and deletes all the fragments it supersedes.
 
 A deletion that could drop entries -- dropping the index entirely, or the full rewrite
 above -- is guarded by a marker object, ``cache/chunkindex-invalid``, written before
-the first deletion and removed after the last one. While the marker is present,
-leftover fragments could be an incomplete index, so they are not merged; the index is
-rebuilt from the pack files on the next load instead. A consolidation needs no marker:
-the entries of the small fragments it deletes are already contained in the merged
-fragments it wrote before deleting them.
+the first deletion and removed after the last one. A single-object delete writes the
+marker just before it removes the old pack, and ``borg check --repair`` writes it
+before rebuilding the index after changing the packs; both remove it once the index
+is stored. While the marker is present, the fragments may be missing entries or point
+at deleted packs, so they are not merged; the index is rebuilt from the pack files on
+the next load instead. A consolidation needs no marker: the entries of the small
+fragments it deletes are already contained in the merged fragments it wrote before
+deleting them.
 
 If the entire ``index/`` namespace is lost or corrupt, the ChunkIndex can be rebuilt
 by scanning pack files directly; see :ref:`pack-recovery`.

src/borg/archive.py

@@ -23,6 +23,7 @@
 from . import xattr
 from .chunkers import get_chunker, Chunk, release_chunk_data
 from .cache import ChunkListEntry, build_chunkindex_from_repo, write_chunkindex_to_repo
+from .cache import write_chunkindex_invalid, delete_chunkindex_invalid
 from .crypto.key import key_from_repository
 from .constants import *  # NOQA
 from .digests import ContentDigester
@@ -2265,7 +2266,7 @@ def check(
         # The rebuild validates every object header it walks, because a corrupt data_size parses fine
         # and points the walk into the middle of the pack. That costs one metadata slot read and one
         # decryption per object and it needs the key, so read the key here if we do not have it yet.
-        # manifest_only=True: the other key source make_key uses is self.chunks, built just below.
+        # manifest_only=True: self.chunks, the other key source of make_key, is set up below.
         if repair and self.key is None:
             self.key = self.make_key(repository, manifest_only=True)
         if self.key is not None:
@@ -2274,25 +2275,35 @@ def check(
             validate = object_validator(self.repo_objs)
         else:
             validate = None
-        self.chunks = build_chunkindex_from_repo(
-            self.repository,
-            slow_rebuild=repair,
-            validate=validate,
-            # dropped content is a check finding, with or without --repair.
-            on_drop=self.note_dropped_objects,
-            # without a validator the rebuild can not resync past a corrupt object header. --repair
-            # drops the rest of that pack to get on with the repair; without --repair the rebuild
-            # raises, so an index missing objects that are still there can not make the check report
-            # them as gone.
-            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.
-        self.repository.chunkindex_validate = validate
-        self.repository.chunkindex_drop_corrupt_tail = repair
+        # store the chunks buffered in the pack writer, so the index below has their pack locations
+        # (pack id, offset and size in the pack).
+        self.repository.flush()
+        if not repair and self.repository.is_chunk_index_loaded:
+            # without --repair, use the loaded index.
+            self.chunks = self.repository.chunks
+        else:
+            # free the loaded index first, so only one index is in memory. --repair builds it from the packs.
+            self.repository.invalidate_chunk_index()
+            self.chunks = build_chunkindex_from_repo(
+                self.repository,
+                slow_rebuild=repair,
+                validate=validate,
+                # dropped content is a check finding, with or without --repair.
+                on_drop=self.note_dropped_objects,
+                # without a validator the rebuild can not resync past a corrupt object header. --repair
+                # drops the rest of that pack to get on with the repair; without --repair the rebuild
+                # raises, so an index missing objects that are still there can not make the check report
+                # them as gone.
+                drop_corrupt_tail=repair,
+                write_immediately=False,
+            )
+            # clear F_NEW (entry not in the index/ fragments yet), so Repository.close() does not store
+            # this index; finish() stores it with --repair. Without --repair, a repository without index/
+            # fragments keeps none. With the invalid marker set (see write_chunkindex_invalid), the build
+            # deletes the fragments.
+            self.chunks.clear_new()
+            # get(), put() and delete() use the repository's index.
+            self.repository.chunks = self.chunks
         if self.key is None:
             self.key = self.make_key(repository)
             self.repo_objs = RepoObj(self.key)
@@ -2420,12 +2431,11 @@ def verify_data(self):
                         )
                     except IntegrityErrorBase:
                         # 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.
+                        # keeping the other chunks, and removes it from self.chunks, so rebuild_archives
+                        # reports the file it belongs to. update_index=False: finish() stores the index
+                        # rebuilt from the packs and clears the invalid marker delete() writes.
                         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:
@@ -2563,19 +2573,20 @@ def record_missing_chunk(archive_name, path, chunk_id, size):
 
         def add_callback(chunk):
             id_ = self.key.id_hash(chunk)
-            cdata = self.repo_objs.format(id_, {}, chunk, ro_type=ROBJ_ARCHIVE_STREAM)
+            cdata = None
+            if self.repair and id_ not in self.chunks:
+                # cdata: the compressed and encrypted chunk, which only add_reference stores.
+                cdata = self.repo_objs.format(id_, {}, chunk, ro_type=ROBJ_ARCHIVE_STREAM)
             add_reference(id_, len(chunk), cdata)
             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:
+            # size: unused, part of the archive_put_items callback signature.
+            # with --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."""
@@ -2797,9 +2808,12 @@ 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 and store it. The index/ fragments lack
+                # the chunks this repair stored, so the index is invalid until the rebuilt one is stored.
+                # Free the current index first, so only one index is in memory.
+                write_chunkindex_invalid(self.repository)
+                self.repository.invalidate_chunk_index()
+                self.chunks = None
                 logger.info("Rebuilding and writing the repository chunks index.")
                 build_chunkindex_from_repo(
                     self.repository,
@@ -2814,6 +2828,8 @@ def finish(self):
                 write_chunkindex_to_repo(
                     self.repository, self.chunks, incremental=False, clear=False, force_write=True, delete_other=True
                 )
+            # the stored index matches the packs: clear the invalid marker.
+            delete_chunkindex_invalid(self.repository)
             # drop the in-memory index so close() does not persist it over the index just written.
             self.repository.invalidate_chunk_index()
             self.manifest.write()

src/borg/cache.py

@@ -619,15 +619,21 @@ def chunkindex_is_invalid(repository):
 
 
 def write_chunkindex_invalid(repository):
-    """Mark the chunk index as invalid. Call before deleting index fragments.
+    """Store the invalid marker, cache/chunkindex-invalid.
 
-    If the deletion is interrupted, the marker remains and the index is rebuilt on next load.
+    Store it before deleting index/ fragments whose entries no other fragment holds, before deleting a pack
+    the fragments point at, and before rebuilding the index after pack changes the fragments do not record.
+    While it is present, build_chunkindex_from_repo rebuilds the index from the packs instead of merging the
+    fragments.
     """
     repository.store_store(f"cache/{CHUNKINDEX_INVALID_SENTINEL}", b"")
 
 
 def delete_chunkindex_invalid(repository):
-    """Clear the chunk-index-invalid marker. Call after all fragment deletions have completed."""
+    """Delete the invalid marker, if present.
+
+    The index/ fragments, if any, must hold the complete current index and point only at existing packs.
+    """
     try:
         repository.store_delete(f"cache/{CHUNKINDEX_INVALID_SENTINEL}")
     except StoreObjectNotFound:
@@ -648,7 +654,7 @@ def delete_chunkindex_from_repo(repository):
             pass
     if hashes or invalid:
         # clear the marker after every fragment is gone; also clears a marker left behind by an
-        # earlier interrupted deletion.
+        # interrupted operation.
         delete_chunkindex_invalid(repository)
     logger.debug(f"chunk indexes deleted: {hashes}")
     # the in-memory index is now stale; drop it so close() does not write it back into the
@@ -921,7 +927,7 @@ def build_chunkindex_from_repo(
             if chunkindex_is_invalid(repository):
                 if fragments_only:
                     return None
-                # leftover fragments may be incomplete or stale. Finish the interrupted deletion
+                # the fragments may be missing entries or point at deleted packs. Delete them
                 # (best-effort; a read-only client rebuilds in memory only), then rebuild from packs.
                 logger.warning("chunk index is invalid (interrupted operation), rebuilding it.")
                 try:

src/borg/constants.py

@@ -137,11 +137,11 @@
 # How often to restart merging the fragments into a chunk index when a listed fragment vanishes
 # mid-merge (a concurrent repack replaced it). After that, fall back to the slow rebuild from packs.
 CHUNKINDEX_MERGE_ATTEMPTS = 3
-# Marker object in the cache/ namespace: the chunk index is invalid. Written before deleting index
-# fragments, removed after the last fragment is gone. While it is present, the chunk index is rebuilt
-# from the packs on next load and the leftover index/ fragments are deleted. Removing the marker while
-# index/ fragments remain makes those fragments look like a complete index, so only
-# delete_chunkindex_invalid() removes it, and clearing cache/ requires clearing index/ too.
+# Marker object in the cache/ namespace: the chunk index is invalid. While it is present, the index/ fragments
+# may be missing entries or point at deleted packs, so the chunk index is rebuilt from the packs on next load
+# and the leftover fragments are deleted.
+# Removing the marker while index/ fragments remain makes those fragments look like a complete index, so
+# only delete_chunkindex_invalid() removes it, and clearing cache/ requires clearing index/ too.
 CHUNKINDEX_INVALID_SENTINEL = "chunkindex-invalid"
 
 FD_MAX_AGE = 4 * 60  # 4 minutes