repo-compress: do not rewrite packs recorded corrupt, refs #10410

A rewritten copy of a corrupt pack gets a new id that passes borg check. repo-compress keeps these packs unchanged and warns to run "borg check --repair --verify-data".

src/borg/archiver/compact_cmd.py

@@ -508,6 +508,8 @@ class CompactMixIn:
 
             ``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.
+            That repair does not remove damage outside any chunk (e.g. bytes appended to a pack), so such a
+            pack stays recorded corrupt and ``borg compact`` warns about it on every run (refs #10026).
 
             You usually do not want to run ``borg compact`` after every write operation, but
             either regularly (e.g., once a month, possibly together with ``borg check``) or

src/borg/archiver/repo_compress_cmd.py

@@ -7,11 +7,11 @@ from ._common import with_repository, Highlander
 from ..cache import build_chunkindex_from_repo, delete_chunkindex_from_repo, write_chunkindex_to_repo
 from ..compress import ObfuscateSize, Auto, COMPRESSOR_TABLE
 from ..constants import *  # NOQA
-from ..helpers import sig_int, ProgressIndicatorPercent, Error, CompressionSpec
-from ..helpers import format_file_size, hex_to_bin
+from ..helpers import sig_int, ProgressIndicatorPercent, Error, CompressionSpec, set_ec, EXIT_WARNING
+from ..helpers import format_file_size, bin_to_hex, hex_to_bin
 from ..helpers.argparsing import ArgumentParser
 from ..repoobj import object_validator
-from ..repository import Repository
+from ..repository import Repository, PackTracker
 
 from ..logger import create_logger
 
@@ -56,6 +56,9 @@ class PackRecompressor:
     Each pack is loaded once (via Repository.transform_pack); objects not stored with the desired
     compression get recompressed, the others are carried into the rewritten pack unchanged. A pack
     whose objects all already match is not rewritten at all.
+
+    A pack recorded corrupt in PackTracker is not rewritten: a pack's id is the hash of its content,
+    so a rewritten copy of the corrupt bytes would get a new id that passes "borg check".
     """
 
     def __init__(self, repository, manifest, *, print_stats):
@@ -91,7 +94,8 @@ class PackRecompressor:
         size_before = sum(size for _, size in packs)
         size_after = size_before
 
-        stale_packs = set(per_pack) - {pack_id for pack_id, _ in packs}
+        present_packs = {pack_id for pack_id, _ in packs}
+        stale_packs = set(per_pack) - present_packs
         if stale_packs:
             # stale entries reference a pack absent from the store. they are kept, borg check --repair removes them.
             stale = sum(len(per_pack[pack_id]) for pack_id in stale_packs)
@@ -99,6 +103,17 @@ class PackRecompressor:
                 f'index entries referencing a missing pack file: {stale}. Run "borg check --repair" to remove them.'
             )
 
+        # packs recorded corrupt in PackTracker that are still in the store
+        corrupt_packs = set(PackTracker.load(self.repository).corrupt_ids()) & present_packs
+        if corrupt_packs:
+            logger.warning(
+                f'{len(corrupt_packs)} pack(s) recorded corrupt by "borg check" are not rewritten. '
+                'Run "borg check --repair --verify-data".'
+            )
+            for pack_id in sorted(corrupt_packs):
+                logger.debug(f"Corrupt pack: {bin_to_hex(pack_id)}")
+            set_ec(EXIT_WARNING)
+
         pi = ProgressIndicatorPercent(
             total=len(packs), msg="Recompressing %3.1f%%", step=0.1, msgid="repo_compress.recompress"
         )
@@ -108,7 +123,7 @@ class PackRecompressor:
                 break  # stop cleanly at a pack boundary: save the index below, then raise
             ids = per_pack.get(pack_id)
             # a pack without indexed objects (all-gap) is left for "borg check --repair", see #9868.
-            if ids:
+            if ids and pack_id not in corrupt_packs:
                 new_pack_id, new_size = self.repository.transform_pack(
                     pack_id,
                     ids,
@@ -214,6 +229,11 @@ class RepoCompressMixIn:
         desired compression type/level - if no compression gives a shorter output, that
         might be chosen; such chunks are kept as they are.
 
+        ``borg repo-compress`` does not rewrite packs that ``borg check`` recorded as corrupt
+        and warns about them. ``borg check --repair --verify-data`` deletes the corrupt chunks.
+        That repair does not remove damage outside any chunk (e.g. bytes appended to a pack), so such a
+        pack stays recorded corrupt and is not rewritten (refs #10026).
+
         Rewriting a pack invalidates every client's cached chunk index, so the next borg
         operation of each client will re-fetch the chunk index from the repository.
 

src/borg/testsuite/archiver/repo_compress_cmd_test.py

@@ -13,7 +13,7 @@ from ...archiver.repo_compress_cmd import PackRecompressor
 
 from .. import make_test_key
 from . import create_regular_file, cmd, open_repository, RK_ENCRYPTION
-from ..repository_test import H, accept_all, fchunk, pdchunk
+from ..repository_test import H, accept_all, fchunk, pdchunk, corrupt_chunk_on_disk
 
 
 def test_repo_compress(archiver):
@@ -240,6 +240,39 @@ def test_repo_compress_soft_interrupt_persists_valid_index(archiver, monkeypatch
     cmd(archiver, "check")
 
 
+@pytest.mark.parametrize("corrupt_ctype", (CNONE.ID, ZLIB.ID))
+def test_repo_compress_keeps_corrupt_pack(archiver, corrupt_ctype):
+    # repo-compress keeps a pack "borg check" recorded corrupt unchanged and warns (#10410).
+    # with "-C none", the corrupt object is either already stored as wanted (CNONE) or needs recompressing (ZLIB).
+    create_regular_file(archiver.input_path, "rand", contents=os.urandom(100_000))  # stored uncompressed
+    create_regular_file(archiver.input_path, "text", contents=b"z" * 100_000)  # stored zlib compressed
+    cmd(archiver, "repo-create", RK_ENCRYPTION)
+    cmd(archiver, "create", "test", "input", "-C", "zlib,3")
+    with open_repository(archiver) as repository:
+        manifest = Manifest.load(repository)
+        ctypes = {}  # pack_id -> {ctype, ...}
+        for id, entry in repository.chunks.iteritems():
+            meta = manifest.repo_objs.parse_meta(id, repository.get(id, read_data=False), ro_type=ROBJ_DONTCARE)
+            ctypes.setdefault(entry.pack_id, set()).add(meta["ctype"])
+            if meta["ctype"] == corrupt_ctype and meta["size"] == 100_000:
+                corrupt_id = id
+        corrupt_pack = repository.chunks[corrupt_id].pack_id
+        # the corrupt pack holds both objects, and another pack holds an object "-C none" recompresses.
+        assert ctypes.pop(corrupt_pack) >= {CNONE.ID, ZLIB.ID}
+        assert any(ZLIB.ID in pack_ctypes for pack_ctypes in ctypes.values())
+        packs_before = {info.name for info in repository.store_list("packs")}
+        corrupt_chunk_on_disk(repository, corrupt_id)  # corrupts the data, the metadata stays readable
+    cmd(archiver, "check", exit_code=1)
+
+    output = cmd(archiver, "repo-compress", "-v", "-C", "none", exit_code=EXIT_WARNING)
+    assert '1 pack(s) recorded corrupt by "borg check" are not rewritten.' in output
+    with open_repository(archiver) as repository:
+        packs_after = {info.name for info in repository.store_list("packs")}
+    assert bin_to_hex(corrupt_pack) in packs_after
+    assert len(packs_before - packs_after) >= 1  # other packs were recompressed
+    cmd(archiver, "check", exit_code=1)
+
+
 def transform_via(replacements):
     """Make a transform_pack callback replacing the given objects, keeping all others unchanged."""