diff --git a/src/borg/archive.py b/src/borg/archive.py index 64ab905b82..195fcb86d0 100644 --- a/src/borg/archive.py +++ b/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 diff --git a/src/borg/cache.py b/src/borg/cache.py index 30f91dbd3d..92933dc001 100644 --- a/src/borg/cache.py +++ b/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 diff --git a/src/borg/repository.py b/src/borg/repository.py index 3ff393bc42..5435f1d040 100644 --- a/src/borg/repository.py +++ b/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 diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index bdfc14dbb4..5deab28b9f 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/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): diff --git a/src/borg/testsuite/cache_test.py b/src/borg/testsuite/cache_test.py index a92cd6559f..e4425fef1a 100644 --- a/src/borg/testsuite/cache_test.py +++ b/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 diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 80be75677f..b56b957455 100644 --- a/src/borg/testsuite/repository_test.py +++ b/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