check: a full --repair rebuilds a corrupt index once, in the archives phase, fixes #10434

src/borg/archive.py

@@ -2312,7 +2312,8 @@ def check(
         # each index object's store hash, and the index is the authoritative record of which chunks exist,
         # so we do not rebuild it from the packs (reading every pack is far too slow for a routine check).
         # --repair does rebuild from the packs (slow_rebuild=repair), working from the real packs so it
-        # can detect and fix archives that reference chunks whose pack has gone missing.
+        # can detect and fix archives that reference chunks whose pack has gone missing. It also replaces a
+        # corrupt index, see Repository.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.

src/borg/archiver/check_cmd.py

@@ -90,7 +90,7 @@ def do_check(self, args, repository):
             raise CommandError("--repair does not allow --max-duration argument.")
         if args.repair and args.max_age is not None:
             # repair verifies every pack; reusing recorded results during repair needs repository
-            # repair (refs #8572).
+            # repair (refs #10026).
             raise CommandError("--repair does not allow the --max-age option.")
         if args.archives_only and args.max_age is not None:
             # --max-age only affects the repository check; --archives-only skips it.
@@ -119,7 +119,7 @@ def do_check(self, args, repository):
                 max_duration=args.max_duration,
                 max_age=max_age,
                 repo_only=args.repo_only,
-                # the object validator for the index rebuild, which only a repair does.
+                # validates each object the index rebuild of a --repository-only repair walks.
                 validate=object_validator(RepoObj(key)),
             ):
                 set_ec(EXIT_WARNING)
@@ -297,16 +297,18 @@ 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 rebuilds the repository
-           index from the packs if the index is corrupt, provided every pack matches its
-           store hash. If any pack fails its store hash, the repository check leaves the
-           index and the packs untouched and reports it; salvaging the intact objects of
-           such a pack is not implemented yet (refs #8572). The rebuild authenticates
-           each object's header and metadata with the key, leaves an object that fails
-           this out of the index and reports it as an error. Repair mode also removes the
-           index entries of the chunks stored in missing packs (packs the index references,
-           but that are absent from the repository). Only a full ``borg check --repair``
-           repairs the archives that reference these chunks, ``--repository-only`` does not.
+        1. When checking the repository's consistency, repair mode verifies every pack if
+           the index is corrupt. A full ``borg check --repair`` then rebuilds the index from
+           the packs in the archive check (which does so on every ``--repair`` run). With
+           ``--repository-only``, the repository check rebuilds it, provided every pack
+           matches its store hash. If any pack fails its store hash, it leaves the index and
+           the packs untouched and reports it; salvaging the intact objects of such a pack
+           is not implemented yet (refs #10026). Either rebuild authenticates each object's
+           header and metadata with the key, leaves an object that fails this out of the
+           index and reports it as an error. Repair mode also removes the index entries of
+           the chunks stored in missing packs (packs the index references, but that are
+           absent from the repository). Only a full ``borg check --repair`` repairs the
+           archives that reference these chunks, ``--repository-only`` does not.
            A missing or corrupt repository defaults object is replaced by empty defaults, so
            the repository can be used again; the commands then use the built-in defaults.
 

src/borg/archiver/compact_cmd.py

@@ -321,14 +321,17 @@ def compact_packs(self):
                 logger.error(f"{stale_used} of them are still in use: repository data is missing!")
                 set_ec(EXIT_ERROR)
 
-        # bytes no index entry covers. compact_pack reclaims the redundant duplicates among them while
-        # rewriting a pack; reclaiming the rest is tracked in #8572.
+        # unindexed bytes: pack bytes no index entry covers, e.g. the objects of an interrupted borg create.
+        # compact_pack reclaims those that are copies of indexed chunks when it rewrites a pack and keeps the
+        # rest, as "borg check --repair" can recover their objects (#9868). A full check --repair adds these
+        # objects to the index (--repository-only rebuilds it only if it is corrupt), so the next compact
+        # reclaims the unused ones.
         unindexed = sum(total - pack_indexed[pid] for pid, total in pack_total.items() if total > pack_indexed[pid])
         if unindexed:
             logger.info(
-                f"{format_file_size(unindexed)} in pack files is not covered by the index; "
-                "redundant copies are reclaimed on pack rewrite, reclaiming the rest is tracked in "
-                "https://github.com/borgbackup/borg/issues/8572."
+                f"{format_file_size(unindexed)} in pack files is not covered by the index; copies of indexed "
+                'chunks are reclaimed on pack rewrite. Run "borg check --repair" (without --repository-only), '
+                'then "borg compact" to reclaim the rest.'
             )
 
         # packs recorded corrupt in PackTracker that are still in the store
@@ -502,8 +505,8 @@ def build_parser_compact(self, subparsers, common_parser, mid_common_parser):
             ``borg compact`` reclaims objects the chunk index knows about, plus redundant copies of
             indexed chunks (e.g. written by concurrent backups) that it finds while rewriting a pack.
             Other bytes no index entry covers, such as packs left behind by a backup that crashed
-            before recording its objects, are re-indexed by ``borg check --repair`` and reclaimed by
-            the next ``borg compact``.
+            before recording its objects, are re-indexed by ``borg check --repair`` (without
+            ``--repository-only``) and reclaimed by the next ``borg compact``.
 
             ``borg compact`` does not rewrite or merge packs that ``borg check`` recorded as corrupt
             and warns about them. ``borg check --repair --verify-data`` deletes the corrupt chunks by

src/borg/repository.py

@@ -1469,7 +1469,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, repo_only=False, validate=None):
+    def check(self, repair=False, max_duration=0, max_age=0, repo_only=None, validate=None):
         """Check repository consistency.
 
         packs/ and index/ objects are named by the store hash of their content, so a pack or index
@@ -1484,18 +1484,18 @@ def check(self, repair=False, max_duration=0, max_age=0, repo_only=False, valida
         rebuild re-reads every pack anyway - so a read-only check just stops and reports it instead of
         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 the store hash, which is content-addressing rather
-        than a MAC, so that check detects accidental corruption but not tampering; the rebuild therefore
-        checks every object with validate, see below, refs #9901, #10026. If any pack is corrupt the index
-        is not rebuilt, refs #8572, #10026. Pack ids found corrupt are kept in cache/checked-packs,
-        refs #9696. That object is stored in the key's envelope, too, so check() needs the key (see
-        set_key).
+        index, every pack is verified. With repo_only, and if every pack is intact, the index is then
+        rebuilt from the packs' object headers and persisted. Without repo_only, the archives phase
+        rebuilds and persists it (see ArchiveChecker.check and ArchiveChecker.finish), refs #10434. Packs
+        are verified by the store hash, which is content-addressing rather than a MAC, so that check
+        detects accidental corruption but not tampering; the rebuild therefore checks every object with
+        validate, see below, refs #9901, #10026. If any pack is corrupt the index is not rebuilt, refs
+        #10026. Pack ids found corrupt are kept in cache/checked-packs, refs #9696. That object is stored
+        in the key's envelope, too, so check() needs the key (see set_key).
 
         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; TODO: also when repair salvages and drops it, refs #8572); prune() does this from packs/.
+        compact; TODO: also when repair salvages and drops it, refs #10026); 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
@@ -1514,11 +1514,12 @@ def check(self, repair=False, max_duration=0, max_age=0, repo_only=False, valida
         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
-        damage repair does not fix, i.e. a corrupt pack, a missing pack or a skipped pack byte range (see
-        validate): 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, and reports and repairs the archives that
-        reference chunks the index lacks).
+        repo_only: whether this is a repository-only run. Required if repair. In repair mode, if True, a
+        corrupt index is rebuilt here (see above), and damage repair does not fix, i.e. a corrupt pack, a
+        missing pack or a skipped pack byte range (see validate), fails the check. If False, both are left
+        to the archives phase: it rebuilds the index, can repair a corrupt pack holding metadata (or file
+        content with --verify-data), and reports and repairs the archives that reference chunks the index
+        lacks.
 
         validate: validate(chunk_id, obj) -> bool, True if obj (an object's header plus its metadata
         slot) is the repo object with id chunk_id, see repoobj.object_validator. Required if repair.
@@ -1528,6 +1529,7 @@ def check(self, repair=False, max_duration=0, max_age=0, repo_only=False, valida
         none. Each skipped range counts as one error.
         """
         assert validate is not None or not repair
+        assert repo_only is not None or not repair
 
         def verify(namespace, name):
             # name is the store hash of the object's content, so it is intact iff store.hash() matches.
@@ -1597,11 +1599,15 @@ def store_list(namespace):
             # --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:
+            if index_errors and repo_only:
                 logger.warning(
                     "Repository index is corrupted; verifying all packs before deciding whether to "
                     "rebuild it from them."
                 )
+            elif index_errors:
+                logger.warning(
+                    "Repository index is corrupted; verifying all packs, the archives check rebuilds the index."
+                )
             # 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
@@ -1715,10 +1721,17 @@ def recorded_ts(info):
                 logger.info("Finished checking packs.")
             tracker.prune(present_pack_ids)
             pack_pi.finish()
-            # rebuild only on repair, 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 repair and index_errors and pack_errors == 0 and not sig_int and pack_files == len(pack_infos):
+            # rebuild only on a repository-only repair, 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 (
+                repair
+                and repo_only
+                and index_errors
+                and pack_errors == 0
+                and not sig_int
+                and pack_files == len(pack_infos)
+            ):
 
                 def note_drop():
                     nonlocal drops
@@ -1737,14 +1750,16 @@ def note_drop():
                         interruptible=True,
                     )
                 except ChunkIndexRebuildInterrupted:
-                    # nothing was stored: the corrupt fragments stay, so the next use rebuilds from the packs.
+                    # nothing was stored: the corrupt fragments stay.
                     drops = 0  # counted by the discarded rebuild, which covered only a part of the packs
-                    logger.warning("Index rebuild interrupted; the index stays corrupt and is rebuilt on next use.")
+                    logger.warning('Index rebuild interrupted; the index stays corrupt, run "borg check --repair".')
                 else:
                     self.invalidate_chunk_index()  # the rebuilt index is persisted; drop the in-memory copy
                     index_repaired = True
         else:
             logger.error("Repository index is corrupted and must be repaired; skipping the pack check.")
+        # index_deferred: the archives phase rebuilds the corrupt index; it runs only if this check was not interrupted.
+        index_deferred = bool(index_errors) and repair and not repo_only and not sig_int
         objs_errors = index_errors + pack_errors + len(missing_pack_ids) + drops
         summary = (
             f"Checked {index_files} index files ({index_errors} errors) "
@@ -1790,20 +1805,22 @@ def note_drop():
             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)."
+                    "with corrupt packs is not implemented yet (refs #10026)."
                 )
             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 drops:
-            # a full check's archives phase reports the chunks the archives reference but the index
-            # lacks, so warn only.
-            log = logger.error if repo_only else logger.warning
-            log(
+            # drops come from a repository-only rebuild only; no archives phase follows it.
+            logger.error(
                 f"{done} {mode} repository check, "
                 f"index rebuilt without pack byte range(s) it could not authenticate{so_far}."
             )
+        elif index_deferred:
+            logger.warning(
+                f"{done} {mode} repository check, index corrupt; the archives check rebuilds it from the packs."
+            )
         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.
@@ -1817,11 +1834,10 @@ def note_drop():
         else:
             # missing packs: the archives phase repairs the archives that reference their chunks.
             logger.warning(f"{done} {mode} repository check, missing pack(s) found{so_far}.")
-        # in repair mode a corrupt index left unrebuilt is a failure; a corrupt or missing pack, or a
-        # skipped pack byte range, fails only a repository-only run, while a full check defers it to the
-        # archives phase.
+        # in repair mode, a corrupt index neither rebuilt here nor deferred fails; a corrupt or missing pack,
+        # or a skipped pack byte range, fails a repository-only run, a full check defers it to the archives phase.
         if repair:
-            if index_errors and not index_repaired:
+            if index_errors and not index_repaired and not index_deferred:
                 return False
             return not (repo_only and (pack_errors or corrupt_ids or missing_pack_ids or drops))
         return not problems