remove the drop_corrupt_tail mechanism, refs #10369

Every check --repair rebuild of the chunk index has an object validator
(#10369), so drop_corrupt_tail was only reachable from tests.

- PackReader.iter_headers: remove the drop_corrupt_tail parameter. Without
  a validator, a corrupt object header raises IntegrityError.
- build_chunkindex_from_repo: remove the drop_corrupt_tail parameter.
- Repository: remove chunkindex_drop_corrupt_tail and chunkindex_validate.
  Since #10368 the checker hands its index to the repository, so the lazy
  .chunks rebuild never runs during a check and nothing set either of them.
- ArchiveChecker.check: stop passing drop_corrupt_tail.
- tests: remove the 5 tests for drop_corrupt_tail, rewrite
  test_check_without_repair_does_not_drop_a_pack_tail as
  test_check_without_key_aborts_on_a_corrupt_pack_header.

Author: mr-raj12

src/borg/archive.py

@@ -2285,14 +2285,11 @@ def check(
             self.chunks = build_chunkindex_from_repo(
                 self.repository,
                 slow_rebuild=repair,
+                # validate is None only without --repair and without the key: a corrupt object header then
+                # raises CorruptPack.
                 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

src/borg/cache.py

@@ -884,20 +884,15 @@ def build_chunkindex_from_repo(
     fragments_only=False,
     validate=None,
     on_drop=None,
-    drop_corrupt_tail=False,
     write_immediately=False,
     init_flags=ChunkIndex.F_USED,
 ):
     # fragments_only: build the index from the index/ fragments only, returning None if they cannot be
     # read completely, and never write to the repo.
-    # validate: a repo object validator, handed to PackReader.iter_headers so the rebuild skips the
-    # objects that fail it.
-    # on_drop: a callable, handed to PackReader.iter_headers, which calls it once per place where
-    # the walk skips content. It only reports, it does not change what the walk does.
-    # drop_corrupt_tail: without a validator, index a pack with a corrupt object header up to that
-    # header and drop the rest of it, instead of raising, see PackReader.iter_headers.
-    # With neither of the two, a corrupt object header aborts the rebuild with CorruptPack: the
-    # index would be missing every object after it.
+    # validate: a repo object validator or None, passed to PackReader.iter_headers. With a validator,
+    # the rebuild skips the objects that fail it; without one, a corrupt object header raises CorruptPack.
+    # on_drop: a callable or None, passed to PackReader.iter_headers, called once per byte range the
+    # validating walk skips.
     assert not (slow_rebuild and fragments_only)
     assert not (fragments_only and write_immediately)  # fragments_only never writes to the repo
     # first, try to build a fresh, mostly complete chunk index from centrally stored index fragments:
@@ -994,9 +989,7 @@ def build_chunkindex_from_repo(
         pack_id = hex_to_bin(info.name)
         reader = PackReader(repository.store, pack_id)
         try:
-            for chunk_id, obj_offset, obj_size in reader.iter_headers(
-                validate=validate, on_drop=on_drop, drop_corrupt_tail=drop_corrupt_tail
-            ):
+            for chunk_id, obj_offset, obj_size in reader.iter_headers(validate=validate, on_drop=on_drop):
                 num_chunks += 1
                 chunks[chunk_id] = ChunkIndexEntry(
                     flags=init_flags, size=0, pack_id=pack_id, obj_offset=obj_offset, obj_size=obj_size

src/borg/repository.py

@@ -494,7 +494,7 @@ def _find_header(self, offset, pack_size, validate):
             offset += max(len(buf) - (hdr_size - 1), 1)
         return None
 
-    def iter_headers(self, validate=None, on_drop=None, drop_corrupt_tail=False):
+    def iter_headers(self, validate=None, on_drop=None):
         """Yield (chunk_id, offset, size) for each object by walking the fixed object headers.
 
         The walk reads one range per object (or a slice, for a pack in memory), plus one store
@@ -510,13 +510,11 @@ def iter_headers(self, validate=None, on_drop=None, drop_corrupt_tail=False):
         validates, the walk yields nothing and raises nothing.
 
         Without a validator a resync is impossible, because payload bytes can look like a header.
-        A header that _parse_header rejects then raises IntegrityError naming what is wrong with
-        it, or, with drop_corrupt_tail, ends the walk there and drops the rest of the pack.
+        A header that _parse_header rejects then raises IntegrityError naming what is wrong with it.
 
-        on_drop, if given, is called once per place where the walk discards content: once for the
-        object with the failed header plus whatever the resync scan skips before the object it
-        resumes at, once for a tail dropped because the scan found no such object or because there
-        was no validator to scan with. It only reports, it does not change what the walk does.
+        on_drop, if given, is called once per byte range a validating walk skips: the object with
+        the failed header plus the bytes up to the next object validate accepts, or up to the end of
+        the pack if there is none.
 
         headers_parsed is set to the number of headers _parse_header accepted in this walk, the
         candidates the resync scan tried included. A pack whose bytes hold no object header at all
@@ -542,19 +540,9 @@ def iter_headers(self, validate=None, on_drop=None, drop_corrupt_tail=False):
                     problem = self._validation_problem(hdr, offset, buf, offset, validate)
             if problem is not None:
                 if validate is None:
-                    # no validator, so payload bytes that look like a header can not be told from
-                    # an object: there is no way to resync past this header.
-                    if not drop_corrupt_tail:
-                        # the callers that can say something more useful than "there is corruption
-                        # here" wrap this, see build_chunkindex_from_repo.
-                        raise IntegrityError(f"pack {pack_hex}: {problem} at offset {offset} (pack corruption)")
-                    if on_drop is not None:
-                        on_drop()
-                    logger.warning(
-                        f"pack {pack_hex}: {problem} at offset {offset}, no validator to resync with, "
-                        f"skipping the remaining {pack_size - offset} bytes."
-                    )
-                    break
+                    # without a validator, payload bytes that look like a header can not be told
+                    # apart from an object, so the walk can not continue past this header.
+                    raise IntegrityError(f"pack {pack_hex}: {problem} at offset {offset} (pack corruption)")
                 if on_drop is not None:
                     on_drop()  # content is discarded either way below: this object, or the tail.
                 found = self._find_header(offset + 1, pack_size, validate)
@@ -976,12 +964,6 @@ def __init__(
         self.exclusive = exclusive
         self._pack_writer = None
         self._chunks = None  # ChunkIndex; loaded lazily on first access to .chunks
-        # corrupt-header handling for the lazy .chunks rebuild (see PackReader.iter_headers): a
-        # validate callable makes the rebuild resync past a corrupt object header, drop_corrupt_tail
-        # makes it index the pack up to that header and drop the rest. Without either, such a header
-        # aborts the rebuild. TODO(#10378): nothing sets them, remove both.
-        self.chunkindex_validate = None
-        self.chunkindex_drop_corrupt_tail = False
         # pack_id -> PackReader holding the whole pack; get_many loads into it, get() reuses it
         self._pack_cache = LRUCache(capacity=self.PACK_READER_CACHE_SIZE)
 
@@ -1277,9 +1259,7 @@ def chunks(self):
         if self._chunks is None:
             from .cache import build_chunkindex_from_repo
 
-            self._chunks = build_chunkindex_from_repo(
-                self, validate=self.chunkindex_validate, drop_corrupt_tail=self.chunkindex_drop_corrupt_tail
-            )
+            self._chunks = build_chunkindex_from_repo(self)
         return self._chunks
 
     @chunks.setter

src/borg/testsuite/archiver/check_cmd_test.py

@@ -964,12 +964,11 @@ def test_check_repair_validates_index_rebuild(archivers, request):
         assert neighbour_id in repository.chunks
 
 
-def test_check_without_repair_does_not_drop_a_pack_tail(archivers, request, monkeypatch):
-    """A check without --repair reports a corrupt object header, it does not index the pack up to it.
+def test_check_without_key_aborts_on_a_corrupt_pack_header(archivers, request, monkeypatch):
+    """A check without --repair and without the key raises CorruptPack at a corrupt object header.
 
-    Without a validator the walk can not resync past a corrupt object header. --repair passes
-    drop_corrupt_tail, so the rest of that pack is dropped and the repair gets on; a check without
-    --repair passes drop_corrupt_tail=False and the walk raises instead.
+    Without the key there is no object validator, and without one the pack walk raises at a corrupt
+    object header.
 
     The rebuild only walks the packs when the chunk index fragments are unusable, and it only walks
     without a validator when the key can not be read, so the test arranges both.
@@ -1013,9 +1012,9 @@ def build_chunkindex_from_repo(repository, **kwargs):
         try:
             index = real_build(repository, **kwargs)
         except Exception as err:
-            rebuilds.append((kwargs.get("drop_corrupt_tail"), err))
+            rebuilds.append((kwargs.get("validate"), err))
             raise
-        rebuilds.append((kwargs.get("drop_corrupt_tail"), index))
+        rebuilds.append((kwargs.get("validate"), index))
         return index
 
     monkeypatch.setattr(ArchiveChecker, "make_key", make_key)
@@ -1025,10 +1024,9 @@ def build_chunkindex_from_repo(repository, **kwargs):
     with pytest.raises(CorruptPack) as excinfo:
         cmd(archiver, "check", "--archives-only")
     assert f"no object header at offset {damaged_offset} (pack corruption)" in str(excinfo.value)
-    drop_corrupt_tail, outcome = rebuilds[0]
-    # the rebuild raised, it did not return an index with the pack's tail missing
+    validate, outcome = rebuilds[0]
+    assert validate is None
     assert isinstance(outcome, CorruptPack)
-    assert drop_corrupt_tail is False  # a check that only diagnoses does not ask for the drop
 
 
 def test_repo_list_aborts_cleanly_on_corrupt_pack(archivers, request):

src/borg/testsuite/cache_test.py

@@ -546,40 +546,6 @@ def test_build_chunkindex_reports_a_pack_without_any_object_header(tmp_path):
         assert len(drops) == 1
 
 
-def test_build_chunkindex_without_a_validator_drops_the_rest_of_a_damaged_pack(tmp_path):
-    """With drop_corrupt_tail and no validator, a corrupt object header ends the pack's walk."""
-    from .repository_test import fchunk
-
-    obj1 = fchunk(b"first", chunk_id=H(90))
-    obj2 = bytearray(fchunk(b"second", chunk_id=H(91)))
-    obj2[0] ^= 0xFF  # break the magic of the second object's header
-    obj3 = fchunk(b"third", chunk_id=H(92))
-    drops = []
-    with Repository(os.fspath(tmp_path / "repository"), exclusive=True, create=True) as repository:
-        repository.store_store("packs/" + bin_to_hex(H(93)), obj1 + bytes(obj2) + obj3)
-        index = build_chunkindex_from_repo(
-            repository, slow_rebuild=True, on_drop=lambda: drops.append(True), drop_corrupt_tail=True
-        )
-        assert H(90) in index  # the pack is indexed up to the damaged header
-        assert H(91) not in index and H(92) not in index  # from there on the pack is dropped
-        assert len(drops) == 1
-
-
-def test_build_chunkindex_without_drop_corrupt_tail_raises_on_a_damaged_pack(tmp_path):
-    """on_drop alone does not let the rebuild past a corrupt object header, it only reports."""
-    from .repository_test import fchunk
-
-    obj1 = fchunk(b"first", chunk_id=H(90))
-    obj2 = bytearray(fchunk(b"second", chunk_id=H(91)))
-    obj2[0] ^= 0xFF  # break the magic of the second object's header
-    drops = []
-    with Repository(os.fspath(tmp_path / "repository"), exclusive=True, create=True) as repository:
-        repository.store_store("packs/" + bin_to_hex(H(93)), obj1 + bytes(obj2))
-        with pytest.raises(CorruptPack, match="no object header at offset"):
-            build_chunkindex_from_repo(repository, slow_rebuild=True, on_drop=lambda: drops.append(True))
-        assert drops == []  # nothing was discarded: the walk did not get that far
-
-
 def test_build_chunkindex_drops_a_pack_that_validates_nothing_when_others_do(tmp_path):
     """A single pack of which nothing validates is dropped, the objects of the other packs are indexed."""
     from .repository_test import fchunk

src/borg/testsuite/repository_test.py

@@ -2206,43 +2206,6 @@ def test_pack_reader_raises_on_bad_magic():
         list(reader.iter_headers())
 
 
-def test_pack_reader_drops_a_corrupt_tail_only_when_asked():
-    # without a validator there is nothing to resync with, so the walk can not get past a corrupt
-    # header. drop_corrupt_tail alone decides what happens then; on_drop only reports it.
-    obj1 = fchunk(b"payload-one", chunk_id=H(1))
-    obj2 = bytearray(fchunk(b"payload-two", chunk_id=H(2)))
-    obj2[0] ^= 0xFF  # break the magic of the second object's header
-    pack = obj1 + bytes(obj2)
-    drops = []
-    reader = PackReader(pack_contents=pack)
-    with pytest.raises(IntegrityError, match="no object header at offset"):
-        list(reader.iter_headers(on_drop=lambda: drops.append(1)))
-    assert drops == []  # nothing was discarded: the walk raised instead
-    headers = list(reader.iter_headers(on_drop=lambda: drops.append(1), drop_corrupt_tail=True))
-    assert headers == [(H(1), 0, len(obj1))]  # up to the corrupt header, the rest of the pack is gone
-    assert len(drops) == 1
-
-
-def test_pack_reader_drops_a_corrupt_tail_without_an_on_drop():
-    # drop_corrupt_tail works without an on_drop to report the drop to.
-    obj1 = fchunk(b"payload-one", chunk_id=H(1))
-    obj2 = bytearray(fchunk(b"payload-two", chunk_id=H(2)))
-    obj2[0] ^= 0xFF
-    reader = PackReader(pack_contents=obj1 + bytes(obj2))
-    assert list(reader.iter_headers(drop_corrupt_tail=True)) == [(H(1), 0, len(obj1))]
-
-
-def test_pack_reader_drop_corrupt_tail_does_not_affect_a_validating_walk():
-    # with a validator the walk resyncs, so drop_corrupt_tail changes nothing.
-    obj1 = bytearray(fchunk(b"payload-one", chunk_id=H(1)))
-    obj2 = fchunk(b"payload-two", chunk_id=H(2))
-    obj1[0] ^= 0xFF
-    reader = PackReader(pack_contents=bytes(obj1) + obj2)
-    resynced = [(H(2), len(obj1), len(obj2))]
-    assert list(reader.iter_headers(validate=accept_all)) == resynced
-    assert list(reader.iter_headers(validate=accept_all, drop_corrupt_tail=True)) == resynced
-
-
 def test_pack_reader_raises_on_bad_magic_through_store(tmp_path):
     obj = bytearray(fchunk(b"FIRST", chunk_id=H(47)))
     obj[0] ^= 0xFF