check --repair: rebuild a corrupt repository index from the packs, #10026

src/borg/archive.py

@@ -23,7 +23,7 @@
 
 from . import xattr
 from .chunkers import get_chunker, Chunk, release_chunk_data
-from .cache import ChunkListEntry, build_chunkindex_from_repo, delete_chunkindex_from_repo
+from .cache import ChunkListEntry, build_chunkindex_from_repo, write_chunkindex_to_repo
 from .crypto.key import key_factory, UnsupportedPayloadError
 from .constants import *  # NOQA
 from .crypto.low_level import IntegrityError as IntegrityErrorBase
@@ -1884,6 +1884,9 @@ class ArchiveChecker:
     def __init__(self):
         self.error_found = False
         self.key = None
+        # True once repair drops a defect chunk or writes a new one, i.e. once the chunks index no
+        # longer matches the packs.
+        self.chunks_modified = False
 
     def check(
         self,
@@ -2081,6 +2084,7 @@ def verify_data(self):
                         # 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.
                         self.repository.delete(defect_chunk, update_index=False)
+                        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:
@@ -2237,6 +2241,7 @@ def add_reference(id_, size, cdata):
                 if self.repair:
                     pack_results = self.repository.put(id_, cdata)
                     self.chunks.update_pack_info(pack_results)
+                    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."""
@@ -2450,13 +2455,22 @@ def valid_item(obj):
 
     def finish(self):
         if self.repair:
-            # store packs still buffered from chunks re-added during repair, so their index entries
-            # are set before the index is dropped below and no chunk is left buffered at close().
+            # flush chunks re-added during repair so their packs are on the store and out of the pack
+            # writer buffer (close() requires an empty buffer, #10055) before we (re)build the index.
             self.repository.flush()
-            # we may have deleted chunks. delete_chunkindex_from_repo() removes the on-disk index and
-            # drops the stale in-memory index, so the next repository access rebuilds it from the repo.
-            logger.info("Deleting chunk indexes in repository - next repository access will cause a rebuild.")
-            delete_chunkindex_from_repo(self.repository)
+            if self.chunks_modified:
+                # the packs changed, so the index no longer matches them: rebuild it from the packs
+                # and persist it.
+                logger.info("Rebuilding and writing the repository chunks index.")
+                build_chunkindex_from_repo(self.repository, slow_rebuild=True, write_immediately=True)
+            else:
+                # the packs are unchanged, so the index still matches them: persist it as is.
+                logger.info("Writing the rebuilt repository chunks index.")
+                write_chunkindex_to_repo(
+                    self.repository, self.chunks, incremental=False, clear=False, force_write=True, delete_other=True
+                )
+            # drop the in-memory index so close() does not persist it over the index just written.
+            self.repository.invalidate_chunk_index()
             logger.info("Writing Manifest.")
             self.manifest.write()
 

src/borg/archiver/check_cmd.py

@@ -78,7 +78,9 @@ def do_check(self, args, repository):
             # the repository check has finished, which can take hours.
             ArchiveFormatter.validate_format(format)
         if not args.archives_only:
-            if not repository.check(repair=args.repair, max_duration=args.max_duration, max_age=max_age):
+            if not repository.check(
+                repair=args.repair, max_duration=args.max_duration, max_age=max_age, repo_only=args.repo_only
+            ):
                 set_ec(EXIT_WARNING)
             if sig_int:  # repository check interrupted; skip the archive check
                 raise Error("Got Ctrl-C / SIGINT.")
@@ -232,8 +234,11 @@ def build_parser_check(self, subparsers, common_parser, mid_common_parser):
 
         In practice, repair mode hooks into both the repository and archive checks:
 
-        1. When checking the repository's consistency, repair mode removes corrupted
-           objects from the repository after it did a 2nd try to read them correctly.
+        1. When checking the repository's consistency, repair mode rebuilds the repository
+           index from the packs if the index is corrupt, provided every pack is intact. If
+           any pack is corrupt, the repository check leaves the index and the packs untouched
+           and reports the corruption; salvaging a corrupt pack's still-intact objects is not
+           implemented yet (refs #8572).
 
         2. When checking the consistency and correctness of archives, repair mode might
            remove whole archives from the manifest if their archive metadata chunk is

src/borg/cache.py

@@ -30,7 +30,7 @@
 from .helpers import hex_to_bin, bin_to_hex, parse_stringified_list
 from .helpers import format_file_size, safe_encode
 from .helpers import safe_ns
-from .helpers import ProgressIndicatorMessage
+from .helpers import ProgressIndicatorMessage, ProgressIndicatorPercent
 from .helpers import msgpack
 from .helpers.msgpack import int_to_timestamp, timestamp_to_int
 from .item import ChunkListEntry
@@ -938,15 +938,23 @@ def build_chunkindex_from_repo(
     # headers and skipping the (much larger) encrypted payloads. Don't call Repository.list() here:
     # it iterates this same index we are building, so it would recurse. The headers also give each
     # object's real (chunk_id, offset, size), so every object in a pack is indexed individually.
-    for info in repository.store_list("packs"):
+    pack_infos = repository.store_list("packs")
+    pi = ProgressIndicatorPercent(
+        total=len(pack_infos), msg="Rebuilding chunk index %3.0f%%", msgid="cache.build_chunkindex_from_repo"
+    )
+    for info in pack_infos:
         # PackReader uses the store directly, so refresh the lock here; a full rebuild can be slow.
         repository._lock_refresh()
+        pi.show(increase=1)
         pack_id = hex_to_bin(info.name)
         for chunk_id, obj_offset, obj_size in PackReader(repository.store, pack_id).iter_headers():
             num_chunks += 1
             chunks[chunk_id] = ChunkIndexEntry(
                 flags=init_flags, size=0, pack_id=pack_id, obj_offset=obj_offset, obj_size=obj_size
             )
+    if pack_infos:
+        pi.show(current=len(pack_infos))  # finish at 100%
+    pi.finish()
     duration = perf_counter() - t0 or 0.001
     # Chunk IDs in a list are encoded in 34 bytes: 1 byte msgpack header, 1 byte length, 32 ID bytes.
     # Protocol overhead is neglected in this calculation.

src/borg/repository.py

@@ -992,7 +992,7 @@ def info(self):
         info = dict(id=self.id, version=self.version)
         return info
 
-    def check(self, repair=False, max_duration=0, max_age=0):
+    def check(self, repair=False, max_duration=0, max_age=0, repo_only=False):
         """Check repository consistency.
 
         packs/ and index/ objects are named by the sha256 of their content, so a pack or index file
@@ -1002,14 +1002,18 @@ def check(self, repair=False, max_duration=0, max_age=0):
         The index is hashed first and the packs only if it is intact. The packs could be hashed even
         with a corrupt index, but a corrupt index already means the user has to repair it, and that
         rebuild re-reads every pack anyway - so a read-only check just stops and reports it instead of
-        continuing. The index is never rebuilt here in any case: reading every pack to do so would be
-        far too slow and expensive for a routine (e.g. cron) check. Salvaging good objects out of
-        corrupt packs and dropping those packs is left to repair, refs #8572. The ids of the packs
-        found corrupt are kept in cache/checked-packs for repair, refs #9696.
+        continuing. A read-only check never rebuilds the index: reading every pack to do so would be
+        far too slow and expensive for a routine (e.g. cron) check. With repair=True and a corrupt
+        index, and if every pack is intact, the index is rebuilt from the packs' object headers and
+        persisted; on a full check the archives phase rebuilds and re-persists it afterwards, see
+        ArchiveChecker.finish. Packs are verified by sha256, which is content-addressing rather than a
+        MAC, so this rebuild detects accidental corruption but not tampering, refs #9901, #10026. If any
+        pack is corrupt the index is left unchanged, refs #8572, #10026. Pack ids found corrupt are kept
+        in cache/checked-packs, refs #9696.
 
         A pack recorded corrupt fails the check, also on a partial run that stops before re-reaching
         it. The record clears at the check that finds the pack intact again or gone (removed by
-        compact, or salvaged and dropped by repair; refs #8572); prune() does this from packs/.
+        compact; TODO: also when repair salvages and drops it, refs #8572); prune() does this from packs/.
 
         It also reports missing packs (refs #9898): pack ids the chunk index references but that are
         absent from packs/. The index is read from its fragments only and its referenced pack ids are
@@ -1022,6 +1026,10 @@ def check(self, repair=False, max_duration=0, max_age=0):
         max_age (seconds, 0 = verify every pack): skip packs whose intact record is younger than
         max_age, accepting a future timestamp up to MAX_CLOCK_SKEW (clock skew). Results are recorded
         regardless of max_age.
+
+        repo_only: whether this is a repository-only run. In repair mode it sets the return value for a
+        corrupt pack, which repair does not fix: fail if repo_only, else defer (a full check's archives
+        phase can repair a corrupt pack holding metadata, or file content with --verify-data).
         """
 
         def verify(namespace, name):
@@ -1061,6 +1069,8 @@ def store_list(namespace):
         index_files = index_errors = 0
         pack_files = pack_errors = pack_skipped = 0
         missing_pack_ids = []  # packs referenced by the index but absent from packs/ (refs #9898)
+        index_repaired = False
+        packs_scanned = False
         # index and packs get separate progress indicators, each running from 0% to 100%.
         # the index is checked first and in full, on partial checks too: it is small, and index errors
         # stop the pack check below.
@@ -1082,7 +1092,16 @@ def store_list(namespace):
         if index_infos:
             index_pi.show(current=len(index_infos))  # finish at 100%
         index_pi.finish()
-        if index_errors == 0:
+        if index_errors == 0 or repair:
+            # verify the packs; during repair, rebuild the corrupt index from them afterwards.
+            # --repair forbids --max-duration and --max-age, so the partial and max_age handling in
+            # the loop stays inactive during a repair.
+            packs_scanned = True
+            if index_errors:
+                logger.warning(
+                    "Repository index is corrupted; verifying all packs before deciding whether to "
+                    "rebuild it from them."
+                )
             # packs are the bulk of the work and the part --max-duration spreads over several checks.
             pack_infos = store_list("packs")
             # drop objects whose name is not a valid pack name and count them as errors; the code
@@ -1172,8 +1191,17 @@ def recorded_ts(info):
                 logger.info("Finished checking packs.")
             tracker.prune(present_pack_ids)
             pack_pi.finish()
+            # rebuild only if the index was the sole problem and every pack was verified intact this
+            # run: sig_int breaks the loop early, so "no pack errors" must be paired with "all packs
+            # scanned" (pack_files == len(pack_infos)) to not rebuild from unverified packs.
+            if index_errors and pack_errors == 0 and not sig_int and pack_files == len(pack_infos):
+                # the exclusive check lock keeps the pack set fixed, so re-listing packs/ inside
+                # build_chunkindex_from_repo matches this verification. write_immediately persists the
+                # index and drops the corrupt fragments.
+                build_chunkindex_from_repo(self, slow_rebuild=True, write_immediately=True)
+                self.invalidate_chunk_index()  # the rebuilt index is persisted; drop the in-memory copy
+                index_repaired = True
         else:
-            # TODO: --repair will rebuild the index from the packs here instead of stopping (refs #8572).
             logger.error("Repository index is corrupted and must be repaired; skipping the pack check.")
         objs_errors = index_errors + pack_errors + len(missing_pack_ids)
         summary = (
@@ -1192,11 +1220,13 @@ def recorded_ts(info):
                 "The chunks stored in these packs are lost. Repairing the index (dropping the "
                 "stale references) is tracked in https://github.com/borgbackup/borg/issues/8572."
             )
-        # corrupt_ids() is every pack recorded corrupt, including from earlier runs. with a corrupt
-        # index the packs were not scanned, so report nothing.
-        corrupt_ids = tracker.corrupt_ids() if index_errors == 0 else []
+        if index_repaired:
+            logger.info("Repository index was corrupted and has been rebuilt from the packs.")
+        # corrupt_ids() includes packs recorded corrupt in earlier runs; report them only when this
+        # run scanned the packs.
+        corrupt_ids = tracker.corrupt_ids() if packs_scanned else []
         if corrupt_ids:
-            # one id per line (the list can be long).
+            # one id per line, the list can be long.
             logger.error(f"Found {len(corrupt_ids)} corrupt pack(s):")
             for pack_id in corrupt_ids:
                 logger.error(f"Corrupt pack: {bin_to_hex(pack_id)}")
@@ -1206,12 +1236,35 @@ def recorded_ts(info):
         done, so_far = ("Interrupted", " so far") if sig_int else ("Finished", "")
         if not problems:
             logger.info(f"{done} {mode} repository check, no problems found{so_far}.")
-        elif repair:
-            logger.error(f"{done} {mode} repository check, errors found{so_far} (repository repair not implemented).")
+        elif not repair:
+            logger.error(f"{done} {mode} repository check, errors found{so_far}.")
+        elif index_repaired and not (pack_errors or corrupt_ids or missing_pack_ids):
+            # the index was the only problem and it has been rebuilt from the packs.
+            logger.info(f"{done} {mode} repository check, repaired{so_far}.")
+        elif pack_errors or corrupt_ids:
+            if repo_only:
+                logger.error(
+                    f"{done} {mode} repository check, corrupt pack(s) found{so_far}; repairing a repository "
+                    "with corrupt packs is not implemented yet (refs #8572)."
+                )
+            else:
+                # a full check's archives phase reads archive/item metadata (and file content with
+                # --verify-data), so it repairs a corrupt pack holding such objects; warn rather than fail.
+                logger.warning(f"{done} {mode} repository check, corrupt pack(s) found{so_far}.")
+        elif index_errors and not index_repaired:
+            # the index is corrupt but was not rebuilt, e.g. the pack verification was interrupted
+            # before every pack was confirmed intact; the corrupt index is left in place.
+            logger.error(f"{done} {mode} repository check, index still corrupt{so_far}.")
         else:
+            # index-referenced packs are missing, so their chunks are lost.
             logger.error(f"{done} {mode} repository check, errors found{so_far}.")
-        # True means the checked objects were clean; --repair returns True so the caller proceeds to fix them.
-        return not problems or repair
+        # in repair mode a corrupt index left unrebuilt is a failure; a corrupt or missing pack fails
+        # only a repository-only run, while a full check defers it to the archives phase.
+        if repair:
+            if index_errors and not index_repaired:
+                return False
+            return not (repo_only and (pack_errors or corrupt_ids or missing_pack_ids))
+        return not problems
 
     def list(self, limit=None, marker=None):
         """

src/borg/testsuite/archiver/check_cmd_test.py

@@ -594,6 +594,36 @@ def test_spoofed_manifest(archivers, request):
     cmd(archiver, "check", exit_code=0)
 
 
+def test_check_repair_rebuilds_corrupt_index(archivers, request):
+    # A corrupt index with all packs intact: the default (full) --repair rebuilds the index from the
+    # packs and persists it (via the archives check, see ArchiveChecker.finish), leaving the repository
+    # usable again without a slow rebuild on the next access.
+    archiver = request.getfixturevalue(archivers)
+    check_cmd_setup(archiver)
+    cmd(archiver, "check", exit_code=0)
+    archive, repository = open_archive(archiver.repository_path, "archive1")
+    with repository:
+        assert isinstance(repository, Repository)
+        for info in repository.store_list("index"):  # rot every index fragment
+            name = f"index/{info.name}"
+            data = bytearray(repository.store_load(name))
+            data[0] ^= 0xFF
+            repository.store_store(name, bytes(data))
+    cmd(archiver, "check", exit_code=1)  # read-only check reports the corrupt index
+    output = cmd(archiver, "check", "-v", "--repair", exit_code=0)
+    assert "rebuilt" in output.lower()
+    # item 6: repair persisted a fresh index instead of leaving it for a slow rebuild on the next
+    # access. confirm the on-disk index exists and every fragment is intact.
+    archive, repository = open_archive(archiver.repository_path, "archive1")
+    with repository:
+        index_infos = list(repository.store_list("index"))
+        assert index_infos  # a fresh index was persisted
+        for info in index_infos:  # each fragment's content still matches its sha256 name
+            assert repository.store.hash(f"index/{info.name}") == info.name
+    cmd(archiver, "check", exit_code=0)  # the repository is consistent again
+    assert "archive1" in cmd(archiver, "repo-list")  # and remains usable
+
+
 @pytest.mark.skip(reason="TODO: repair does not yet rewrite store-corrupted packs, refs #8572")
 def test_manifest_rebuild_corrupted_chunk(archivers, request):
     archiver = request.getfixturevalue(archivers)
@@ -689,7 +719,7 @@ def test_extra_chunks(archivers, request):
 
 
 def test_repair_finish_flushes_pack_writer(archivers, request):
-    """finish() stores chunks re-added during --repair before it drops the index (#10055).
+    """finish() stores chunks re-added during --repair before it (re)builds the index (#10055).
 
     close() asserts an empty pack writer buffer, so a chunk left buffered by finish() would
     trip it.
@@ -706,6 +736,8 @@ def test_repair_finish_flushes_pack_writer(archivers, request):
         checker.repository = repository
         checker.key = checker.make_key(repository)
         checker.manifest = Manifest.load(repository, (Manifest.Operation.CHECK,), key=checker.key)
+        # re-adding a chunk makes the chunks index no longer match the packs, so finish() rebuilds it.
+        checker.chunks_modified = True
 
         # a chunk re-added during repair, buffered in the pack writer:
         key = b"01234567890123456789012345678901"