From a7d2062770541bee3723e8d724f1d80733a511e9 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Wed, 16 Sep 2026 16:18:42 +0530 Subject: [PATCH 1/2] check --repair: re-read only the packs the repair wrote, refs #8466 finish() validates the written packs against the shared index instead of rebuilding it from all packs. --- src/borg/archive.py | 160 ++++++--- src/borg/archiver/check_cmd.py | 6 +- src/borg/repository.py | 27 +- src/borg/testsuite/archiver/check_cmd_test.py | 339 ++++++++++++++++-- src/borg/testsuite/repository_test.py | 12 + 5 files changed, 471 insertions(+), 73 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index 38109885bb..74f89e87e2 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -53,7 +53,8 @@ from .patterns import PathPrefixPattern, FnmatchPattern, IECommand from .item import Item, ArchiveItem, ItemDiff from .platform import acl_get, acl_set, set_flags, get_flags, set_times, swidth -from .repository import Repository +from .hashindex import ChunkIndex, ChunkIndexEntry +from .repository import Repository, PackReader from .repoobj import RepoObj, object_validator # macOS: SF_DATALESS marks dataless placeholder files (e.g. cloud files not materialized locally). @@ -2236,9 +2237,24 @@ 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. + # True once repair wrote a pack: it stored a chunk or deleted a defect chunk. self.chunks_modified = False + # ids of the existing packs repair stored (put(), flush()) or wrote by rewriting a pack (delete()). + self.written_packs = set() + + def record_stored(self, results): + """Add the pack ids in results to written_packs. + + results: the (chunk_id, pack_id, obj_offset, obj_size) tuples Repository.put() or .flush() returns + for the packs it stored, or None if it stored no pack. + """ + if results: + self.written_packs.update(pack_id for _, pack_id, _, _ in results) + + def create_archive_entry(self, name, id, ts): + """Store the pack writer buffer, record the packs it wrote, create the archives directory entry.""" + self.record_stored(self.repository.flush()) + self.manifest.archives.create(name, id, ts) def note_dropped_objects(self): # The chunk index rebuild skipped repository content to get past a corrupt object header. @@ -2423,9 +2439,14 @@ def verify_data(self): # failed twice -> remove this defect chunk. delete rewrites its pack without it, # keeping the other chunks, and removes it from self.chunks, so rebuild_archives # reports the file it belongs to. update_index=False: finish() stores the index - # rebuilt from the packs and clears the invalid marker delete() writes. - self.repository.delete(defect_chunk, update_index=False, validate=validate) + # and clears the invalid marker delete() writes. + # new_pack_id holds the other objects of the old pack, None if there were none. + old_pack_id = self.chunks[defect_chunk].pack_id + new_pack_id, _ = self.repository.delete(defect_chunk, update_index=False, validate=validate) self.chunks_modified = True + self.written_packs.discard(old_pack_id) + if new_pack_id is not None: + self.written_packs.add(new_pack_id) else: logger.warning("chunk %s not deleted, did not consistently fail.", bin_to_hex(defect_chunk)) else: @@ -2513,7 +2534,7 @@ def valid_archive(obj): self.error_found = True if self.repair: logger.warning(f"Creating archives directory entry for {name} {archive_id_hex}.") - self.manifest.archives.create(name, archive_id, archive.time) + self.create_archive_entry(name, archive_id, archive.time) else: logger.warning(f"Would create archives directory entry for {name} {archive_id_hex}.") @@ -2569,7 +2590,7 @@ def add_reference(id_, size, cdata): # with --repair, store a chunk the repository does not have; put() adds it to self.chunks. if self.repair and id_ not in self.chunks: assert cdata is not None - self.repository.put(id_, cdata) + self.record_stored(self.repository.put(id_, cdata)) self.chunks_modified = True def verify_file_chunks(archive_name, item): @@ -2782,51 +2803,110 @@ def valid_item(obj): logger.debug(f"archive id new: {bin_to_hex(new_archive_id)}") cdata = self.repo_objs.format(new_archive_id, {}, data, ro_type=ROBJ_ARCHIVE_META) add_reference(new_archive_id, len(data), cdata) - self.manifest.archives.create(info.name, new_archive_id, info.ts) + self.create_archive_entry(info.name, new_archive_id, info.ts) if archive_id != new_archive_id: self.manifest.archives.delete_by_id(archive_id) finally: pi.finish() report_missing_chunks() + def verify_written_packs(self): + """Read the object headers of the packs in written_packs and make the chunks index match them. + + put() and delete() compute the index entries of the packs they write without reading the packs. + This compares the (chunk_id, obj_offset, obj_size) of each object header in a written pack, read + with a validator, with the index entries that name the pack. Each difference is a check finding, + logged and fixed in the index: + + - an index entry names an object the pack does not hold: the entry is removed. + - the pack holds an object whose chunk id is not indexed: the object is indexed. + - the pack does not exist: its index entries are removed. + + An object whose chunk id is indexed at another location is a superseded duplicate, not a finding: a + pack delete() wrote can hold one, in a byte range compact_pack copied with no index entry covering it. + """ + pack_ids = sorted(self.written_packs) + if not pack_ids: + return + logger.info(f"Re-reading the packs written by the repair: {len(pack_ids)}.") + # (chunk_id, obj_offset, obj_size) of the index entries, per written pack. + indexed = {pack_id: set() for pack_id in pack_ids} + for chunk_id, entry in self.chunks.iteritems(): + entries = indexed.get(entry.pack_id) + if entries is not None: + entries.add((chunk_id, entry.obj_offset, entry.obj_size)) + validate = object_validator(self.repo_objs) + for pack_id in pack_ids: + # PackReader reads from the store, which does not refresh the repository lock. + self.repository._lock_refresh() + pack_hex = bin_to_hex(pack_id) + expected = indexed.pop(pack_id) + reader = PackReader(self.repository.store, pack_id) + # iter_headers() yields nothing for a missing pack: the store reports size 0 for it. + if not self.repository.store.info(reader.key).exists: + self.error_found = True + logger.error(f"pack {pack_hex}: written by the repair, but it is missing. Removing its index entries.") + for chunk_id, _, _ in expected: + del self.chunks[chunk_id] + continue + found = list(reader.iter_headers(validate=validate, on_drop=self.note_dropped_objects)) + not_found = sorted(expected.difference(found)) + for chunk_id, _, _ in not_found: + del self.chunks[chunk_id] + # the loop indexes each unindexed object, so of several unindexed copies of a chunk, the first + # is indexed and the others are superseded duplicates. + unindexed = [] + for obj in found: + chunk_id, obj_offset, obj_size = obj + if obj in expected: + continue + if chunk_id in self.chunks: + logger.debug( + f"pack {pack_hex}: {bin_to_hex(chunk_id)} at offset {obj_offset}, {obj_size} bytes: " + "superseded duplicate" + ) + continue + unindexed.append(obj) + # size=0: the object header does not hold the plaintext size. + self.chunks[chunk_id] = ChunkIndexEntry( + flags=ChunkIndex.F_USED, size=0, pack_id=pack_id, obj_offset=obj_offset, obj_size=obj_size + ) + if not (not_found or unindexed): + continue + self.error_found = True + logger.error( + f"pack {pack_hex}: the chunks index does not match the pack. Indexed objects not in the pack: " + f"{len(not_found)}, objects in the pack with an unindexed chunk id: {len(unindexed)}. " + "Fixed the index." + ) + for chunk_id, obj_offset, obj_size in not_found: + logger.debug( + f"pack {pack_hex}: {bin_to_hex(chunk_id)} at offset {obj_offset}, {obj_size} bytes: not in pack" + ) + for chunk_id, obj_offset, obj_size in unindexed: + logger.debug( + f"pack {pack_hex}: {bin_to_hex(chunk_id)} at offset {obj_offset}, {obj_size} bytes: not indexed" + ) + def finish(self): if self.repair: - # 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() + # store the pack writer buffer before the index is written (close() requires an empty buffer, #10055). + self.record_stored(self.repository.flush()) if self.chunks_modified: - # the packs changed: rebuild the index from them and store it. The index/ fragments lack - # the chunks this repair stored, so the index is invalid until the rebuilt one is stored. - # Free the current index first, so only one index is in memory. + # the index/ fragments do not have the chunks this repair stored. write_chunkindex_invalid(self.repository) - self.repository.invalidate_chunk_index() - self.chunks = None - # Runs to completion, also after a Ctrl-C: delete_chunkindex_invalid() below declares - # the stored index to match the packs, which holds only once every pack was indexed. - if sig_int: - logger.warning( - "Rebuilding and writing the repository chunks index. " - "This reads every pack and can not be interrupted." - ) - else: - logger.info("Rebuilding and writing the repository chunks index.") - build_chunkindex_from_repo( - self.repository, - slow_rebuild=True, - validate=object_validator(self.repo_objs), - on_drop=self.note_dropped_objects, - 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 - ) + # Runs to completion, also after a Ctrl-C: delete_chunkindex_invalid() below declares the + # stored index to match the packs, which holds only once every written pack was re-read. + self.verify_written_packs() + 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 + ) + # close() persists the in-memory index: drop it, the stored one is current. + self.repository.invalidate_chunk_index() + self.chunks = None # the stored index matches the packs: clear the invalid marker. delete_chunkindex_invalid(self.repository) - # drop the in-memory index so close() does not persist it over the index just written. - self.repository.invalidate_chunk_index() class ArchiveRecreater: diff --git a/src/borg/archiver/check_cmd.py b/src/borg/archiver/check_cmd.py index c2f7a04736..b4396f5a1d 100644 --- a/src/borg/archiver/check_cmd.py +++ b/src/borg/archiver/check_cmd.py @@ -222,9 +222,9 @@ def build_parser_check(self, subparsers, common_parser, mid_common_parser): ``borg check`` rebuilds the chunk index from the packs when ``--repair`` is given or when the stored index cannot be used. Ctrl-C stops that rebuild after the current object and discards the partial index: it lacks chunks that are still in the repository, so the check - would report them as lost. After a ``--repair`` that stored or deleted chunks, borg rebuilds and - stores the chunk index once more; that rebuild always runs to completion, also after a - Ctrl-C, and reads every pack. + would report them as lost. After a ``--repair`` that stored or deleted chunks, borg re-reads the + packs the repair wrote, makes the chunk index match them and stores it; that always runs to + completion, also after a Ctrl-C. About repair mode +++++++++++++++++ diff --git a/src/borg/repository.py b/src/borg/repository.py index 2445bee9bc..d37710d91f 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -1290,10 +1290,15 @@ def is_chunk_index_loaded(self): return self._chunks is not None def flush(self): - """Flush any buffered pack writer chunks.""" + """Store the pack writer buffer as a pack, after waiting for the pack the background store-thread is storing. + + Returns the (chunk_id, pack_id, obj_offset, obj_size) tuples of the objects in the packs this call + stored or waited for, or None if there were none. + """ if self._pack_writer is not None: self._lock_refresh() - self._pack_writer.flush() # PackWriter updates _chunks internally + return self._pack_writer.flush() # PackWriter updates _chunks internally + return None def close(self, *, aborting=False): """Close the repository: join an in-flight pack store, persist the chunk index, tear down. @@ -1376,12 +1381,10 @@ def check(self, repair=False, max_duration=0, max_age=0, repo_only=False, valida 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 left unchanged, refs #8572, #10026. Pack ids found corrupt are kept in cache/checked-packs, - refs #9696. + persisted. 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 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 @@ -1820,9 +1823,12 @@ def delete(self, id, *, validate, update_index=True): Raises PermissionDenied before any store change unless the repo permissions grant write and delete on packs/ and index/ (see assert_writable). + validate: passed to compact_pack. update_index: True: store the full chunk index and delete the invalid marker. False: update the in-memory index only; the marker stays until the index is stored and the marker deleted. - validate: passed to compact_pack. + + Returns compact_pack's (new_pack_id, dropped_bytes): the id of the pack holding the other objects + of the old pack (None if there were none), and the number of bytes the rewrite dropped. """ from .cache import write_chunkindex_to_repo, write_chunkindex_invalid, delete_chunkindex_invalid @@ -1835,7 +1841,7 @@ def delete(self, id, *, validate, update_index=True): # keep every object the chunk index lists for this pack, except the one being deleted. keep_ids = {cid for cid, e in self.chunks.iteritems() if e.pack_id == pack_id} keep_ids.discard(id) - self.compact_pack( + result = self.compact_pack( pack_id, keep_ids=keep_ids, drop_ids={id}, @@ -1847,6 +1853,7 @@ def delete(self, id, *, validate, update_index=True): # the removal for the next borg process. write_chunkindex_to_repo(self, self.chunks, incremental=False, force_write=True, delete_other=True) delete_chunkindex_invalid(self) + return result def compact_pack( self, pack_id, *, keep_ids: set, drop_ids: set, validate, chunks=None, before_old_pack_delete=None diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index f7adfa78d1..54ac85ec8f 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -219,13 +219,12 @@ def iter_headers_then_interrupt(self, **kwargs): cmd(archiver, "check", exit_code=0) -def test_check_repair_finish_completes_index_rebuild_after_interrupt(archiver, monkeypatch, capsys): - """finish() runs with sig_int already set, #9850: its chunk index rebuild walks every pack and stores - an index that matches them, so the invalid marker is cleared and a plain check passes afterwards. - It warns that this rebuild can not be interrupted.""" +def test_check_repair_finish_completes_after_interrupt(archiver, monkeypatch): + """finish() runs with sig_int already set, #9850: it re-reads the packs the repair wrote and stores an + index that matches them, so the invalid marker is cleared and a plain check passes afterwards.""" # local-only: this patches in-process internals, including check_cmd_setup's small ChunkBuffer.BUFFER_SIZE. # With the default buffer size an archive's item metadata is a single chunk, which the repair rewrites to - # the same id, so it stores nothing and finish() skips its index rebuild. + # the same id, so it stores nothing and finish() has no written pack to re-read. check_cmd_setup(archiver) # two archives orig_create = Archives.create @@ -267,10 +266,10 @@ def count_packs_read(self, **kwargs): monkeypatch.setattr(ArchiveChecker, "finish", orig_finish) monkeypatch.setattr(PackReader, "iter_headers", orig_iter_headers) - # the repair stored re-packed item metadata chunks, so finish() takes its rebuild branch. + # the repair stored re-packed item metadata chunks, so finish() re-reads the packs it wrote. assert checker.chunks_modified is True - assert len(packs_read_in_finish) == pack_count - assert "This reads every pack and can not be interrupted." in capsys.readouterr().err + assert set(packs_read_in_finish) == checker.written_packs + assert 0 < len(packs_read_in_finish) < pack_count # not every pack of the repository with Repository(archiver.repository_path, exclusive=True) as repository: assert not chunkindex_is_invalid(repository) # finish() reached delete_chunkindex_invalid() cmd(archiver, "check", exit_code=0) # the stored index matches the packs @@ -606,10 +605,11 @@ def test_missing_archive_metadata(archivers, request): # checker_builds: per index build in ArchiveChecker, whether repository.chunks was loaded at that time. -# A full check without --repair uses the index the repository check loaded, --repair also builds in finish(). +# A full check without --repair uses the index the repository check loaded. --repair builds once: finish() +# re-reads only the packs the repair wrote, see test_repair_finish_reads_only_the_packs_put_wrote. @pytest.mark.parametrize( "args, exit_code, checker_builds", - [(["--archives-only"], 1, [False]), ([], 1, []), (["--repair"], 0, [False, False])], + [(["--archives-only"], 1, [False]), ([], 1, []), (["--repair"], 0, [False])], ids=["archives-only", "full", "repair"], ) def test_check_holds_a_single_chunk_index(archiver, monkeypatch, args, exit_code, checker_builds): @@ -748,24 +748,21 @@ def rebuild_archives(self, **kwargs): repository.get(chunk_id) -def test_check_repair_stopped_in_the_index_rebuild_marks_the_index_invalid(archiver, monkeypatch): - """A --repair check that stops in the index rebuild of finish() leaves the chunk index marked invalid. +def test_check_repair_stopped_in_the_index_store_marks_the_index_invalid(archiver, monkeypatch): + """A --repair check that stops while finish() stores the chunk index leaves it marked invalid. The repair stored a new item metadata stream and new archive metadata, which the index/ fragments do not have. The marker makes the next use rebuild the index from the packs, which have them. """ delete_first_item_chunk(archiver) - real_build = archive_module.build_chunkindex_from_repo - def build_chunkindex_from_repo(repository, **kwargs): - if kwargs.get("write_immediately"): # the rebuild in finish() - raise Error("stopped in the index rebuild") - return real_build(repository, **kwargs) + def write_chunkindex_to_repo(repository, chunks, **kwargs): + raise Error("stopped in the index store") with monkeypatch.context() as m: - m.setattr(archive_module, "build_chunkindex_from_repo", build_chunkindex_from_repo) + m.setattr(archive_module, "write_chunkindex_to_repo", write_chunkindex_to_repo) with open_repository(archiver) as repository: - with pytest.raises(Error, match="stopped in the index rebuild"): + with pytest.raises(Error, match="stopped in the index store"): ArchiveChecker().check(repository, repair=True, sort_by="ts", format="{archive}") with open_repository(archiver) as repository: @@ -1179,7 +1176,7 @@ def test_repair_finish_flushes_pack_writer(archivers, request): checker.key = checker.make_key(repository) checker.repo_objs = RepoObj(checker.key) checker.manifest = Manifest.load(repository, key=checker.key) - # re-adding a chunk makes the chunks index no longer match the packs, so finish() rebuilds it. + checker.chunks = repository.chunks checker.chunks_modified = True # a chunk re-added during repair, buffered in the pack writer: @@ -1191,6 +1188,308 @@ def test_repair_finish_flushes_pack_writer(archivers, request): assert not repository._pack_writer._pieces # finish() stored it +def record_finish_walks(monkeypatch): + """Return a list that collects the id of every pack whose object headers finish() walks. + + ArchiveChecker.verify_written_packs walks a pack with PackReader.iter_headers. + """ + walked = [] + in_finish = False + real_finish = ArchiveChecker.finish + real_iter_headers = PackReader.iter_headers + + def finish(self): + nonlocal in_finish + in_finish = True + try: + return real_finish(self) + finally: + in_finish = False + + def iter_headers(self, *args, **kwargs): + if in_finish: + walked.append(self.pack_id) + return real_iter_headers(self, *args, **kwargs) + + monkeypatch.setattr(ArchiveChecker, "finish", finish) + monkeypatch.setattr(PackReader, "iter_headers", iter_headers) + return walked + + +def list_packs(archiver): + with Repository(archiver.repository_location, exclusive=True) as repository: + return {info.name for info in repository.store_list("packs")} + + +def put_objects_in_one_pack(archiver, contents): + """Store an encrypted repo object per contents entry, all in one new pack no archive references. + + Returns the object ids, in pack order, and the pack id. + """ + with Repository(archiver.repository_location, exclusive=True) as repository: + manifest = Manifest.load(repository) + ids = [] + for data in contents: + chunk_id = manifest.key.id_hash(data) + repository.put(chunk_id, manifest.repo_objs.format(chunk_id, {}, data, ro_type=ROBJ_FILE_STREAM)) + ids.append(chunk_id) + repository.flush() + entries = [repository.chunks[chunk_id] for chunk_id in ids] + assert {entry.pack_id for entry in entries} == {entries[0].pack_id} + assert [entry.obj_offset for entry in entries] == sorted(entry.obj_offset for entry in entries) + return ids, entries[0].pack_id + + +def test_repair_finish_reads_only_the_rewritten_pack(archiver, monkeypatch): + """--verify-data --repair removes a defect chunk; finish() re-reads only the pack delete() wrote.""" + # local-only: this patches in-process archive and repository internals. + monkeypatch.setenv("BORG_PACK_MAX_COUNT", "2") # many packs, so a full walk would be noticed + check_cmd_setup(archiver) + # a defect chunk that no archive references, so the check after the repair finds nothing missing. + # delete() rewrites its pack, keeping the bystander. + (bystander_id, defect_id), pack_id = put_objects_in_one_pack(archiver, [b"bystander", b"defect"]) + with Repository(archiver.repository_location, exclusive=True) as repository: + corrupt_chunk_on_disk(repository, defect_id) + packs_before = list_packs(archiver) + assert len(packs_before) > 10 + + walked = record_finish_walks(monkeypatch) + # the BUFFER_SIZE check_cmd_setup used: rebuild_archives re-chunks the item metadata into the same + # chunks, so it stores nothing and the rewritten pack is the only pack the repair writes. + with patch.object(ChunkBuffer, "BUFFER_SIZE", 10): + output = cmd(archiver, "check", "--repair", "--verify-data", exit_code=0) + assert f"{bin_to_hex(defect_id)}, integrity error" in output + + new_packs = list_packs(archiver) - packs_before + assert packs_before - list_packs(archiver) == {bin_to_hex(pack_id)} + assert len(new_packs) == 1 + assert [bin_to_hex(pack_id) for pack_id in walked] == list(new_packs) + with Repository(archiver.repository_location, exclusive=True) as repository: + assert defect_id not in repository.chunks + assert bin_to_hex(repository.chunks[bystander_id].pack_id) in new_packs + cmd(archiver, "check", exit_code=0) + + +def test_repair_finish_reads_no_pack_after_deleting_a_whole_pack(archiver, monkeypatch): + """--verify-data --repair removes a defect chunk that is alone in its pack; finish() re-reads no pack. + + delete() drops the whole pack and writes no new one, so the repair wrote no pack. + """ + # local-only: this patches in-process archive and repository internals. + check_cmd_setup(archiver) + (defect_id,), pack_id = put_objects_in_one_pack(archiver, [b"defect"]) + with Repository(archiver.repository_location, exclusive=True) as repository: + corrupt_chunk_on_disk(repository, defect_id) + packs_before = list_packs(archiver) + + walked = record_finish_walks(monkeypatch) + findings = record_verify_findings(monkeypatch) + with patch.object(ChunkBuffer, "BUFFER_SIZE", 10): # see test_repair_finish_reads_only_the_rewritten_pack + output = cmd(archiver, "check", "--repair", "--verify-data", "--info", exit_code=0) + assert f"{bin_to_hex(defect_id)}, integrity error" in output + assert findings == [False] + assert "Re-reading the packs written by the repair" not in output + assert walked == [] + assert list_packs(archiver) == packs_before - {bin_to_hex(pack_id)} + with Repository(archiver.repository_location, exclusive=True) as repository: + assert defect_id not in repository.chunks + cmd(archiver, "check", exit_code=0) + + +def test_repair_finish_reads_only_the_packs_put_wrote(archiver, monkeypatch): + """--repair re-stores a missing item metadata chunk; finish() re-reads only the packs put() wrote.""" + # local-only: this patches in-process archive and repository internals. + # a pack per object, so every put() after the first returns the pack the background store-thread + # stored before it. + monkeypatch.setenv("BORG_PACK_MAX_COUNT", "1") + check_cmd_setup(archiver) + archive, repository = open_archive(archiver.repository_path, "archive1") + with repository: + repository.delete(archive.item_ids[0], validate=None) + packs_before = list_packs(archiver) + + walked = record_finish_walks(monkeypatch) + findings = record_verify_findings(monkeypatch) + cmd(archiver, "check", "--repair", exit_code=0) + assert findings == [False] + + new_packs = list_packs(archiver) - packs_before + assert new_packs + assert sorted(bin_to_hex(pack_id) for pack_id in walked) == sorted(new_packs) # each one once + cmd(archiver, "check", exit_code=0) + + +def test_repair_finish_reads_the_pack_its_flush_stores(archiver, monkeypatch): + """finish() re-reads the pack its own flush stores, e.g. for chunks buffered when a Ctrl-C stopped the repair.""" + # local-only: this patches in-process archive and repository internals. + check_cmd_setup(archiver) + walked = record_finish_walks(monkeypatch) + with Repository(archiver.repository_location, exclusive=True) as repository: + checker = ArchiveChecker() + checker.repair = True + checker.repository = repository + checker.key = checker.make_key(repository) + checker.repo_objs = RepoObj(checker.key) + checker.manifest = Manifest.load(repository, key=checker.key) + checker.chunks = repository.chunks + checker.chunks_modified = True + data = b"repaired" + chunk_id = checker.key.id_hash(data) + assert repository.put(chunk_id, checker.repo_objs.format(chunk_id, {}, data, ro_type=ROBJ_FILE_STREAM)) is None + checker.finish() + assert not checker.error_found + with Repository(archiver.repository_location, exclusive=True) as repository: + assert walked == [repository.chunks[chunk_id].pack_id] + + +def test_repair_finish_reads_a_rewritten_pack_no_index_entry_names(archiver, monkeypatch): + """finish() re-reads a pack delete() wrote, also when no index entry names that pack. + + The pack holds an object with a corrupt header, which the rebuild in check() drops, and a defect + chunk, which --verify-data --repair deletes. compact_pack copies the dropped object's bytes (no + index entry covers them) into the new pack, so the new pack exists, but no index entry points at it. + """ + # local-only: this patches in-process archive and repository internals. + check_cmd_setup(archiver) + (dropped_id, defect_id), pack_id = put_objects_in_one_pack(archiver, [b"dropped", b"defect"]) + with Repository(archiver.repository_location, exclusive=True) as repository: + corrupt_chunk_on_disk(repository, defect_id) # the payload: the header still validates + key = "packs/" + bin_to_hex(pack_id) + dropped = repository.chunks[dropped_id] + repository.store_store(key, corrupt(repository.store_load(key), dropped.obj_offset)) # the magic + packs_before = list_packs(archiver) + + walked = record_finish_walks(monkeypatch) + with patch.object(ChunkBuffer, "BUFFER_SIZE", 10): # see test_repair_finish_reads_only_the_rewritten_pack + output = cmd(archiver, "check", "--archives-only", "--repair", "--verify-data", "--debug", exit_code=0) + assert "no object header at offset 0" in output + assert f"{bin_to_hex(defect_id)}, integrity error" in output + + assert packs_before - list_packs(archiver) == {bin_to_hex(pack_id)} + new_packs = list_packs(archiver) - packs_before + assert len(new_packs) == 1 + assert [bin_to_hex(pack_id) for pack_id in walked] == list(new_packs) + with Repository(archiver.repository_location, exclusive=True) as repository: + assert not any(bin_to_hex(entry.pack_id) in new_packs for _, entry in repository.chunks.iteritems()) + + +def test_repair_finish_accepts_a_superseded_duplicate_in_a_rewritten_pack(archiver, monkeypatch): + """A superseded duplicate that delete() copies into the new pack is not a finding of finish(). + + The pack holds an object with a corrupt header, two copies of one chunk and a defect chunk. The + rebuild in check() drops the first object and indexes the second copy. compact_pack copies the bytes + before the second copy, which no index entry covers, into the new pack: its search for superseded + duplicates there stops at the corrupt header. So the new pack holds both copies, the index names + only the second. + """ + # local-only: this patches in-process archive and repository internals. + check_cmd_setup(archiver) + monkeypatch.setenv("BORG_PACK_MAX_COUNT", "4") # the four objects below go into one pack + (dropped_id, dup_id, _, defect_id), pack_id = put_objects_in_one_pack( + archiver, [b"dropped", b"duplicate", b"duplicate", b"defect"] + ) + with Repository(archiver.repository_location, exclusive=True) as repository: + corrupt_chunk_on_disk(repository, defect_id) # the payload: the header still validates + key = "packs/" + bin_to_hex(pack_id) + dropped = repository.chunks[dropped_id] + repository.store_store(key, corrupt(repository.store_load(key), dropped.obj_offset)) # the magic + + walked = record_finish_walks(monkeypatch) + with patch.object(ChunkBuffer, "BUFFER_SIZE", 10): # see test_repair_finish_reads_only_the_rewritten_pack + output = cmd(archiver, "check", "--archives-only", "--repair", "--verify-data", "--debug", exit_code=0) + assert f"{bin_to_hex(defect_id)}, integrity error" in output + assert "in a gap, keeping the remaining" in output + assert len(walked) == 1 + assert "the chunks index does not match the pack" not in output + with Repository(archiver.repository_location, exclusive=True) as repository: + entry = repository.chunks[dup_id] + assert entry.pack_id == walked[0] + # the second copy: the first one starts where the dropped object ends. + assert entry.obj_offset > dropped.obj_size + cmd(archiver, "check", exit_code=0) + + +def record_verify_findings(monkeypatch, tamper=None): + """Return a list that collects, per verify_written_packs call, whether that call found a problem. + + tamper(checker) runs right before the call. The problems found before it (e.g. the damage the + repair fixed) stay recorded in checker.error_found, but do not count for the call. + """ + findings = [] + real_verify = ArchiveChecker.verify_written_packs + + def verify_written_packs(self): + if tamper is not None: + tamper(self) + error_found, self.error_found = self.error_found, False + try: + return real_verify(self) + finally: + findings.append(self.error_found) + self.error_found = self.error_found or error_found + + monkeypatch.setattr(ArchiveChecker, "verify_written_packs", verify_written_packs) + return findings + + +def test_repair_finish_fixes_a_wrong_index_entry_for_a_written_pack(archiver, monkeypatch): + """finish() compares the written packs with their index entries, reports a difference and fixes it.""" + # local-only: this patches in-process archive and repository internals. + check_cmd_setup(archiver) + archive, repository = open_archive(archiver.repository_path, "archive1") + with repository: + repository.delete(archive.item_ids[0], validate=None) + + tampered = {} + + def tamper(checker): + # an index entry with a wrong offset, as a bug in the offset arithmetic would make one. + pack_id = min(checker.written_packs) + chunk_id, entry = next((cid, e) for cid, e in checker.chunks.iteritems() if e.pack_id == pack_id) + checker.chunks[chunk_id] = entry._replace(obj_offset=entry.obj_offset + 1) + tampered[chunk_id] = entry + + findings = record_verify_findings(monkeypatch, tamper) + output = cmd(archiver, "check", "--repair", exit_code=0) + assert findings == [True] + ((chunk_id, entry),) = tampered.items() + assert f"pack {bin_to_hex(entry.pack_id)}: the chunks index does not match the pack" in output + assert "Indexed objects not in the pack: 1, objects in the pack with an unindexed chunk id: 1." in output + assert "Archive consistency check complete, problems found." in output + + with Repository(archiver.repository_location, exclusive=True) as repository: + stored = repository.chunks[chunk_id] + assert (stored.pack_id, stored.obj_offset, stored.obj_size) == (entry.pack_id, entry.obj_offset, entry.obj_size) + cmd(archiver, "check", exit_code=0) + + +def test_repair_finish_reports_a_missing_written_pack(archiver, monkeypatch): + """finish() reports a written pack that is gone and removes the index entries that name it.""" + # local-only: this patches in-process archive and repository internals. + check_cmd_setup(archiver) + archive, repository = open_archive(archiver.repository_path, "archive1") + with repository: + repository.delete(archive.item_ids[0], validate=None) + + removed = [] + + def tamper(checker): + # a written pack that vanished, as a store losing it would make it. + pack_id = min(checker.written_packs) + checker.repository.store_delete("packs/" + bin_to_hex(pack_id)) + removed.append(pack_id) + + findings = record_verify_findings(monkeypatch, tamper) + output = cmd(archiver, "check", "--repair", exit_code=0) + assert findings == [True] + (pack_id,) = removed + assert f"pack {bin_to_hex(pack_id)}: written by the repair, but it is missing." in output + assert "the chunks index does not match the pack" not in output + assert "Archive consistency check complete, problems found." in output + with Repository(archiver.repository_location, exclusive=True) as repository: + assert not any(entry.pack_id == pack_id for _, entry in repository.chunks.iteritems()) + + @pytest.mark.parametrize("init_args", [["--encryption=aes256-ocb"], ["--encryption", "authenticated-sha256"]]) def test_verify_data(archivers, request, init_args): archiver = request.getfixturevalue(archivers) diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 0ac592a378..4f17a0af24 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -421,6 +421,18 @@ def test_read_data(repo_fixtures, request): assert repository.get(H(0), read_data=False) == chunk_short +def test_flush_returns_the_stored_objects(repository): + assert repository.flush() is None # not opened, no pack writer + with repository: + assert repository.flush() is None # nothing buffered + repository.put(H(0), fchunk(b"foo")) + ((chunk_id, pack_id, obj_offset, obj_size),) = repository.flush() + entry = repository.chunks[H(0)] + assert (chunk_id, pack_id, obj_offset, obj_size) == (H(0), entry.pack_id, entry.obj_offset, entry.obj_size) + assert repository.flush() is None + assert repository.flush() is None # closed + + def test_consistency(repo_fixtures, request): with get_repository_from_fixture(repo_fixtures, request) as repository: repository.put(H(0), fchunk(b"foo")) From 4d970f4db5d3575c8ad87fcce861052e1b88becf Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Tue, 22 Sep 2026 13:14:48 +0530 Subject: [PATCH 2/2] check --repair: verify the written packs in two passes, refs #8466 Remove stale index entries of all written packs before indexing any object, so the result does not depend on the pack order. Look up each pack once, update the docs. --- docs/internals/data-structures.rst | 6 +- docs/internals/packs.rst | 12 ++-- src/borg/archive.py | 69 ++++++++++++------- src/borg/cache.py | 2 +- src/borg/repository.py | 24 ++++--- src/borg/testsuite/archiver/check_cmd_test.py | 57 +++++++++++++-- src/borg/testsuite/repository_test.py | 12 ++++ 7 files changed, 131 insertions(+), 51 deletions(-) diff --git a/docs/internals/data-structures.rst b/docs/internals/data-structures.rst index 4d2ade5cc5..1f64d1021f 100644 --- a/docs/internals/data-structures.rst +++ b/docs/internals/data-structures.rst @@ -86,9 +86,9 @@ cache/ a marker object: while it is present, the chunks index in ``index/`` is considered invalid, because its fragments may be missing entries or point at deleted packs. It is written before deleting index fragments, before a single-object delete removes - the old pack, and before ``borg check --repair`` rebuilds the index after changing - the packs. It is removed after the last fragment is deleted or once the complete - current index is stored. + the old pack, and by ``borg check --repair`` after storing packs, before it re-reads + them and stores the index. It is removed after the last fragment is deleted or once + the complete current index is stored. Note that this ``cache/`` namespace is inside the repository (and thus shared by all clients); it is not the client-local cache described in diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index 16b97fba9f..5bc1d3a804 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -323,12 +323,12 @@ A deletion that could drop entries -- dropping the index entirely, or the full r above -- is guarded by a marker object, ``cache/chunkindex-invalid``, written before the first deletion and removed after the last one. A single-object delete writes the marker just before it removes the old pack, and ``borg check --repair`` writes it -before rebuilding the index after changing the packs; both remove it once the index -is stored. While the marker is present, the fragments may be missing entries or point -at deleted packs, so they are not merged; the index is rebuilt from the pack files on -the next load instead. A consolidation needs no marker: the entries of the small -fragments it deletes are already contained in the merged fragments it wrote before -deleting them. +after storing packs, before it re-reads them and stores the index; both remove it +once the index is stored. While the marker is present, the fragments may be missing +entries or point at deleted packs, so they are not merged; the index is rebuilt from +the pack files on the next load instead. A consolidation needs no marker: the entries +of the small fragments it deletes are already contained in the merged fragments it +wrote before deleting them. If the entire ``index/`` namespace is lost or corrupt, the ChunkIndex can be rebuilt by scanning pack files directly; see :ref:`pack-recovery`. diff --git a/src/borg/archive.py b/src/borg/archive.py index 74f89e87e2..e927d448fb 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2237,22 +2237,25 @@ class ArchiveChecker: def __init__(self): self.error_found = False self.key = None - # True once repair wrote a pack: it stored a chunk or deleted a defect chunk. + # True once repair changed the packs: it stored a chunk or deleted a defect chunk. self.chunks_modified = False - # ids of the existing packs repair stored (put(), flush()) or wrote by rewriting a pack (delete()). + # ids of the packs repair wrote: stored by put() and flush(), or written by delete() rewriting a pack. self.written_packs = set() def record_stored(self, results): """Add the pack ids in results to written_packs. - results: the (chunk_id, pack_id, obj_offset, obj_size) tuples Repository.put() or .flush() returns - for the packs it stored, or None if it stored no pack. + results: (chunk_id, pack_id, obj_offset, obj_size) tuples of the objects in the stored packs, as + Repository.put() and flush() return them, or None. """ if results: self.written_packs.update(pack_id for _, pack_id, _, _ in results) def create_archive_entry(self, name, id, ts): - """Store the pack writer buffer, record the packs it wrote, create the archives directory entry.""" + """Store the pack writer buffer, record the packs it wrote, create the archives directory entry. + + Archives.create() stores the pack writer buffer too, but does not return the packs it wrote. + """ self.record_stored(self.repository.flush()) self.manifest.archives.create(name, id, ts) @@ -2311,7 +2314,8 @@ def check( self.repo_objs = RepoObj(self.key) validate = object_validator(self.repo_objs) # store the chunks buffered in the pack writer, so the index below has their pack locations - # (pack id, offset and size in the pack). + # (pack id, offset and size in the pack). The result is ignored: written_packs holds only the packs + # the repair writes. self.repository.flush() if not repair and self.repository.is_chunk_index_loaded: # without --repair, use the loaded index. @@ -2440,11 +2444,11 @@ def verify_data(self): # keeping the other chunks, and removes it from self.chunks, so rebuild_archives # reports the file it belongs to. update_index=False: finish() stores the index # and clears the invalid marker delete() writes. - # new_pack_id holds the other objects of the old pack, None if there were none. old_pack_id = self.chunks[defect_chunk].pack_id + # new_pack_id: the pack holding the other objects of the old pack, None if there were none. new_pack_id, _ = self.repository.delete(defect_chunk, update_index=False, validate=validate) self.chunks_modified = True - self.written_packs.discard(old_pack_id) + self.written_packs.discard(old_pack_id) # delete() removed the old pack if new_pack_id is not None: self.written_packs.add(new_pack_id) else: @@ -2813,22 +2817,23 @@ def valid_item(obj): def verify_written_packs(self): """Read the object headers of the packs in written_packs and make the chunks index match them. - put() and delete() compute the index entries of the packs they write without reading the packs. + put() and delete() compute the index entries of the packs they write from the data they write. This compares the (chunk_id, obj_offset, obj_size) of each object header in a written pack, read - with a validator, with the index entries that name the pack. Each difference is a check finding, - logged and fixed in the index: + with a validator, with the index entries that name the pack. Each difference sets error_found, is + logged and is fixed in the index: - an index entry names an object the pack does not hold: the entry is removed. - the pack holds an object whose chunk id is not indexed: the object is indexed. - the pack does not exist: its index entries are removed. - An object whose chunk id is indexed at another location is a superseded duplicate, not a finding: a - pack delete() wrote can hold one, in a byte range compact_pack copied with no index entry covering it. + A superseded duplicate is an object whose chunk id is indexed at another location. It is logged at + debug level. A pack delete() wrote holds one if compact_pack copied a byte range that no index entry + covers. """ pack_ids = sorted(self.written_packs) if not pack_ids: return - logger.info(f"Re-reading the packs written by the repair: {len(pack_ids)}.") + logger.info(f"Re-reading {len(pack_ids)} pack(s) written by the repair.") # (chunk_id, obj_offset, obj_size) of the index entries, per written pack. indexed = {pack_id: set() for pack_id in pack_ids} for chunk_id, entry in self.chunks.iteritems(): @@ -2836,25 +2841,37 @@ def verify_written_packs(self): if entries is not None: entries.add((chunk_id, entry.obj_offset, entry.obj_size)) validate = object_validator(self.repo_objs) + # pass 1 removes the index entries of every pack before pass 2 indexes any object, so whether an object + # is unindexed does not depend on the order the packs are read in. + found_in = {} # pack_id -> (chunk_id, obj_offset, obj_size) of the objects in the pack + not_found_in = {} # pack_id -> sorted index entries naming an object the pack does not hold for pack_id in pack_ids: # PackReader reads from the store, which does not refresh the repository lock. self.repository._lock_refresh() - pack_hex = bin_to_hex(pack_id) - expected = indexed.pop(pack_id) - reader = PackReader(self.repository.store, pack_id) - # iter_headers() yields nothing for a missing pack: the store reports size 0 for it. - if not self.repository.store.info(reader.key).exists: + expected = indexed[pack_id] + key = "packs/" + bin_to_hex(pack_id) + info = self.repository.store.info(key) + if not info.exists: self.error_found = True - logger.error(f"pack {pack_hex}: written by the repair, but it is missing. Removing its index entries.") + logger.error( + f"pack {bin_to_hex(pack_id)}: written by the repair, but it is missing. Removing its index entries." + ) for chunk_id, _, _ in expected: del self.chunks[chunk_id] continue + reader = PackReader(self.repository.store, pack_id, pack_size=info.size) found = list(reader.iter_headers(validate=validate, on_drop=self.note_dropped_objects)) not_found = sorted(expected.difference(found)) for chunk_id, _, _ in not_found: del self.chunks[chunk_id] - # the loop indexes each unindexed object, so of several unindexed copies of a chunk, the first - # is indexed and the others are superseded duplicates. + found_in[pack_id] = found + not_found_in[pack_id] = not_found + # pass 2 indexes each unindexed object, so of several unindexed copies of a chunk, the first in pack id and + # offset order is indexed and the others are superseded duplicates. + for pack_id, found in found_in.items(): + pack_hex = bin_to_hex(pack_id) + expected = indexed[pack_id] + not_found = not_found_in[pack_id] unindexed = [] for obj in found: chunk_id, obj_offset, obj_size = obj @@ -2874,6 +2891,8 @@ def verify_written_packs(self): if not (not_found or unindexed): continue self.error_found = True + # an object the pack holds whose index entry has a wrong offset or size counts in both numbers, + # unless pass 2 indexed another copy of it first. logger.error( f"pack {pack_hex}: the chunks index does not match the pack. Indexed objects not in the pack: " f"{len(not_found)}, objects in the pack with an unindexed chunk id: {len(unindexed)}. " @@ -2892,13 +2911,13 @@ def finish(self): if self.repair: # store the pack writer buffer before the index is written (close() requires an empty buffer, #10055). self.record_stored(self.repository.flush()) - if self.chunks_modified: - # the index/ fragments do not have the chunks this repair stored. + if self.chunks_modified or self.written_packs: + # the index/ fragments lack the pack changes of this repair. write_chunkindex_invalid(self.repository) # Runs to completion, also after a Ctrl-C: delete_chunkindex_invalid() below declares the # stored index to match the packs, which holds only once every written pack was re-read. self.verify_written_packs() - logger.info("Writing the rebuilt repository chunks index.") + logger.info("Writing the repository chunks index.") write_chunkindex_to_repo( self.repository, self.chunks, incremental=False, clear=False, force_write=True, delete_other=True ) diff --git a/src/borg/cache.py b/src/borg/cache.py index 64a1e2d5a6..7dc6e25532 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -609,7 +609,7 @@ def write_chunkindex_invalid(repository): """Store the invalid marker, cache/chunkindex-invalid. Store it before deleting index/ fragments whose entries no other fragment holds, before deleting a pack - the fragments point at, and before rebuilding the index after pack changes the fragments do not record. + the fragments point at, and after storing packs the fragments do not record. While it is present, build_chunkindex_from_repo rebuilds the index from the packs instead of merging the fragments. """ diff --git a/src/borg/repository.py b/src/borg/repository.py index d37710d91f..195fa7b63d 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -406,14 +406,16 @@ def flush(self): class PackReader: """Reads pack files, the read-side counterpart to PackWriter. - Pass pack_id to read from the store, or pack_contents for a pack already in memory. + Pass pack_id to read from the store, or pack_contents for a pack already in memory. pack_size, if given, + is the size of the pack in the store, so size() does not look it up. """ - def __init__(self, store=None, pack_id=None, pack_contents=None): + def __init__(self, store=None, pack_id=None, pack_contents=None, pack_size=None): self.store = store self.pack_id = pack_id self.key = "packs/" + bin_to_hex(pack_id) if pack_id is not None else None self.pack_contents = pack_contents + self.pack_size = pack_size self.headers_parsed = 0 # headers _parse_header accepted in the last iter_headers walk def read(self, offset, size): @@ -423,9 +425,11 @@ def read(self, offset, size): return self.store.load(self.key, offset=offset, size=size) def size(self): - """Return the pack size in bytes (a store metadata lookup, unless the pack is in memory).""" + """Return the pack size in bytes (a store metadata lookup, unless the pack is in memory or pack_size is set).""" if self.pack_contents is not None: return len(self.pack_contents) + if self.pack_size is not None: + return self.pack_size return self.store.info(self.key).size @staticmethod @@ -498,8 +502,8 @@ 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 - metadata lookup for the pack size. Fewer than a header's bytes left ends the walk: that is - the end of the pack. + metadata lookup for the pack size unless pack_size is set. Fewer than a header's bytes left + ends the walk: that is the end of the pack. validate(chunk_id, obj) tells whether obj - an object's header and metadata slot - is the repo object with id chunk_id. Given one, the walk validates every header, reading the @@ -1381,10 +1385,12 @@ def check(self, repair=False, max_duration=0, max_age=0, repo_only=False, valida 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. 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 left - unchanged, refs #8572, #10026. Pack ids found corrupt are kept in cache/checked-packs, refs #9696. + 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 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 diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 54ac85ec8f..fe9765d08f 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -1246,7 +1246,7 @@ def test_repair_finish_reads_only_the_rewritten_pack(archiver, monkeypatch): monkeypatch.setenv("BORG_PACK_MAX_COUNT", "2") # many packs, so a full walk would be noticed check_cmd_setup(archiver) # a defect chunk that no archive references, so the check after the repair finds nothing missing. - # delete() rewrites its pack, keeping the bystander. + # delete() rewrites its pack, keeping the other object in it (the bystander). (bystander_id, defect_id), pack_id = put_objects_in_one_pack(archiver, [b"bystander", b"defect"]) with Repository(archiver.repository_location, exclusive=True) as repository: corrupt_chunk_on_disk(repository, defect_id) @@ -1257,8 +1257,9 @@ def test_repair_finish_reads_only_the_rewritten_pack(archiver, monkeypatch): # the BUFFER_SIZE check_cmd_setup used: rebuild_archives re-chunks the item metadata into the same # chunks, so it stores nothing and the rewritten pack is the only pack the repair writes. with patch.object(ChunkBuffer, "BUFFER_SIZE", 10): - output = cmd(archiver, "check", "--repair", "--verify-data", exit_code=0) + output = cmd(archiver, "check", "--repair", "--verify-data", "--info", exit_code=0) assert f"{bin_to_hex(defect_id)}, integrity error" in output + assert "Re-reading 1 pack(s) written by the repair." in output new_packs = list_packs(archiver) - packs_before assert packs_before - list_packs(archiver) == {bin_to_hex(pack_id)} @@ -1288,7 +1289,7 @@ def test_repair_finish_reads_no_pack_after_deleting_a_whole_pack(archiver, monke output = cmd(archiver, "check", "--repair", "--verify-data", "--info", exit_code=0) assert f"{bin_to_hex(defect_id)}, integrity error" in output assert findings == [False] - assert "Re-reading the packs written by the repair" not in output + assert "pack(s) written by the repair." not in output assert walked == [] assert list_packs(archiver) == packs_before - {bin_to_hex(pack_id)} with Repository(archiver.repository_location, exclusive=True) as repository: @@ -1353,10 +1354,10 @@ def test_repair_finish_reads_a_rewritten_pack_no_index_entry_names(archiver, mon check_cmd_setup(archiver) (dropped_id, defect_id), pack_id = put_objects_in_one_pack(archiver, [b"dropped", b"defect"]) with Repository(archiver.repository_location, exclusive=True) as repository: - corrupt_chunk_on_disk(repository, defect_id) # the payload: the header still validates + corrupt_chunk_on_disk(repository, defect_id) # corrupts the payload, the header still validates key = "packs/" + bin_to_hex(pack_id) dropped = repository.chunks[dropped_id] - repository.store_store(key, corrupt(repository.store_load(key), dropped.obj_offset)) # the magic + repository.store_store(key, corrupt(repository.store_load(key), dropped.obj_offset)) # corrupts the magic packs_before = list_packs(archiver) walked = record_finish_walks(monkeypatch) @@ -1389,10 +1390,10 @@ def test_repair_finish_accepts_a_superseded_duplicate_in_a_rewritten_pack(archiv archiver, [b"dropped", b"duplicate", b"duplicate", b"defect"] ) with Repository(archiver.repository_location, exclusive=True) as repository: - corrupt_chunk_on_disk(repository, defect_id) # the payload: the header still validates + corrupt_chunk_on_disk(repository, defect_id) # corrupts the payload, the header still validates key = "packs/" + bin_to_hex(pack_id) dropped = repository.chunks[dropped_id] - repository.store_store(key, corrupt(repository.store_load(key), dropped.obj_offset)) # the magic + repository.store_store(key, corrupt(repository.store_load(key), dropped.obj_offset)) # corrupts the magic walked = record_finish_walks(monkeypatch) with patch.object(ChunkBuffer, "BUFFER_SIZE", 10): # see test_repair_finish_reads_only_the_rewritten_pack @@ -1490,6 +1491,48 @@ def tamper(checker): assert not any(entry.pack_id == pack_id for _, entry in repository.chunks.iteritems()) +@pytest.mark.parametrize("holder", ["first", "second"]) +def test_verify_written_packs_does_not_depend_on_the_pack_order(archiver, monkeypatch, holder): + """A chunk in one written pack, indexed at a bogus location in the other one, is indexed where it is. + + holder: which of the two written packs, in the order verify_written_packs reads them, holds the chunk. + """ + # local-only: this patches in-process archive and repository internals. + monkeypatch.setenv("BORG_PACK_MAX_COUNT", "1") # a pack per object + cmd(archiver, "repo-create", RK_ENCRYPTION) + with Repository(archiver.repository_location, exclusive=True) as repository: + manifest = Manifest.load(repository) + ids = [] + for data in [b"aaa", b"bbb"]: + chunk_id = manifest.key.id_hash(data) + repository.put(chunk_id, manifest.repo_objs.format(chunk_id, {}, data, ro_type=ROBJ_FILE_STREAM)) + ids.append(chunk_id) + repository.flush() + entries = {chunk_id: repository.chunks[chunk_id] for chunk_id in ids} + first_id, second_id = sorted(ids, key=lambda chunk_id: entries[chunk_id].pack_id) + chunk_id, other_id = (first_id, second_id) if holder == "first" else (second_id, first_id) + + checker = ArchiveChecker() + checker.repair = True + checker.repository = repository + checker.key = manifest.key + checker.repo_objs = manifest.repo_objs + checker.chunks = repository.chunks + checker.written_packs = {entry.pack_id for entry in entries.values()} + checker.chunks[chunk_id] = entries[chunk_id]._replace(pack_id=entries[other_id].pack_id, obj_offset=1) + + checker.verify_written_packs() + + assert checker.error_found + fixed = checker.chunks[chunk_id] + expected = entries[chunk_id] + assert (fixed.pack_id, fixed.obj_offset, fixed.obj_size) == ( + expected.pack_id, + expected.obj_offset, + expected.obj_size, + ) + + @pytest.mark.parametrize("init_args", [["--encryption=aes256-ocb"], ["--encryption", "authenticated-sha256"]]) def test_verify_data(archivers, request, init_args): archiver = request.getfixturevalue(archivers) diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 4f17a0af24..ec59dc317d 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -2272,6 +2272,18 @@ def test_pack_reader_iter_headers_reads_through_store(tmp_path): assert list(reader.iter_headers()) == [(H(47), 0, len(obj1)), (H(48), len(obj1), len(obj2))] +def test_pack_reader_with_pack_size_does_not_look_up_the_size(tmp_path, monkeypatch): + obj1 = fchunk(b"FIRST", chunk_id=H(47)) + obj2 = fchunk(b"SECOND", chunk_id=H(48)) + pack = obj1 + obj2 + pack_id = H(43) + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + repository.store_store("packs/" + bin_to_hex(pack_id), pack) + reader = PackReader(repository.store, pack_id, pack_size=len(pack)) + monkeypatch.setattr(repository.store, "info", None) # a size lookup would raise TypeError + assert list(reader.iter_headers()) == [(H(47), 0, len(obj1)), (H(48), len(obj1), len(obj2))] + + def test_pack_reader_raises_on_bad_magic(): # a header without OBJ_MAGIC means the walk desynced onto payload bytes: corruption, not EOF. obj1 = fchunk(b"payload-one", meta=b"meta1", chunk_id=H(1))