From 4b6d54ca0c3add0badb1469e72c5ffbc81dbd987 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Mon, 14 Sep 2026 19:12:03 +0530 Subject: [PATCH] check: use one chunk index for the checker and the repository, refs #10364 The repository uses the index ArchiveChecker builds instead of loading or building a second one. --- src/borg/archive.py | 38 +++++----- src/borg/testsuite/archiver/check_cmd_test.py | 71 +++++++++++++++++++ 2 files changed, 93 insertions(+), 16 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index 6c3dde5c19..bf223b65a4 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2226,6 +2226,8 @@ def check( validate = object_validator(self.repo_objs) else: validate = None + # free the chunk index the repository check may have loaded, so only one is in memory. + self.repository.invalidate_chunk_index() self.chunks = build_chunkindex_from_repo( self.repository, slow_rebuild=repair, @@ -2239,10 +2241,16 @@ def check( drop_corrupt_tail=repair, write_immediately=False, ) - # repository.chunks is a separate index, lazily built when repository.get() resolves a - # chunk location. It walks the same packs, so give it the same corrupt-header handling the - # rebuild above got - otherwise the check aborts at a header it just resynced past, halfway - # through its diagnosis. Dropping the rest of that pack stays a --repair action. + if repair: + # the rebuild from the packs sets F_NEW (entry not stored in the index/ fragments yet) on every + # entry. finish() stores the complete index and deletes the old fragments. Clear F_NEW, so + # Repository.close() does not store the entries again as an extra fragment. + self.chunks.clear_new() + # the repository uses this index: get() looks up pack locations in it, put() adds entries to + # it, delete() removes entries from it. + self.repository.chunks = self.chunks + # corrupt object header handling for a rebuild of repository.chunks after invalidate_chunk_index(): + # the same as for the rebuild above. self.repository.chunkindex_validate = validate self.repository.chunkindex_drop_corrupt_tail = repair if self.key is None: @@ -2374,10 +2382,10 @@ def verify_data(self): # failed twice -> remove this defect chunk. delete rewrites its pack without it, # keeping the other chunks. update_index=False: finish() rebuilds the index from # the rewritten packs anyway, so a per-chunk full index write would be wasted. + # delete() also removes the chunk from self.chunks, so rebuild_archives reports + # the file it belongs to. self.repository.delete(defect_chunk, update_index=False, validate=validate) self.chunks_modified = True - # drop it from our own index too, so rebuild_archives reports the file it belongs to. - del self.chunks[defect_chunk] else: logger.warning("chunk %s not deleted, did not consistently fail.", bin_to_hex(defect_chunk)) else: @@ -2525,14 +2533,11 @@ def add_callback(chunk): return id_ def add_reference(id_, size, cdata): - # either we already have this chunk in repo and chunks index or we add it now - if id_ not in self.chunks: + # --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.chunks.add(id_, size) - if self.repair: - pack_results = self.repository.put(id_, cdata) - self.chunks.update_pack_info(pack_results) - self.chunks_modified = True + self.repository.put(id_, cdata) + self.chunks_modified = True def verify_file_chunks(archive_name, item): """Verify that all of a file's chunks are present, collecting any missing ones for the report.""" @@ -2750,9 +2755,10 @@ def finish(self): # writer buffer (close() requires an empty buffer, #10055) before we (re)build the index. self.repository.flush() if self.chunks_modified: - # the packs changed, so the index no longer matches them: rebuild it from the packs - # and persist it: deleting a defect chunk rewrites its pack and repoints that - # pack's other objects in the repository's index, so our offsets for them are stale. + # the packs changed: rebuild the index from them, validating every object header, and + # store it. Free the current index first, so only one is in memory. + self.repository.invalidate_chunk_index() + self.chunks = None logger.info("Rebuilding and writing the repository chunks index.") build_chunkindex_from_repo( self.repository, diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index d2539d679f..80964297a1 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -462,6 +462,77 @@ def test_missing_archive_metadata(archivers, request): cmd(archiver, "check", exit_code=0) +@pytest.mark.parametrize( + "args, exit_code", [(["--archives-only"], 1), ([], 1), (["--repair"], 0)], ids=["archives-only", "full", "repair"] +) +def test_check_holds_a_single_chunk_index(archiver, monkeypatch, args, exit_code): + """check has at most one chunk index in memory: the repository uses the index the checker builds.""" + # local-only: this patches in-process archive and repository internals. + check_cmd_setup(archiver) + # with an item metadata chunk missing, --repair stores a new item metadata stream. + archive, repository = open_archive(archiver.repository_path, "archive1") + with repository: + repository.delete(archive.item_ids[0], validate=None) + + loaded_at_build = [] # per checker index build: whether repository.chunks was loaded at that time + real_build = archive_module.build_chunkindex_from_repo + + def build_chunkindex_from_repo(repository, **kwargs): + loaded_at_build.append(repository.is_chunk_index_loaded) + return real_build(repository, **kwargs) + + repository_builds = 0 # index builds by the Repository.chunks property + real_chunks = Repository.chunks + + def chunks(self): + nonlocal repository_builds + if not self.is_chunk_index_loaded: + repository_builds += 1 + return real_chunks.fget(self) + + same_index = [] # per rebuild_archives call: whether repository.chunks is the checker's index + real_rebuild_archives = ArchiveChecker.rebuild_archives + + def rebuild_archives(self, **kwargs): + same_index.append(self.repository.chunks is self.chunks) + return real_rebuild_archives(self, **kwargs) + + monkeypatch.setattr(archive_module, "build_chunkindex_from_repo", build_chunkindex_from_repo) + monkeypatch.setattr(Repository, "chunks", property(chunks, real_chunks.fset)) + monkeypatch.setattr(ArchiveChecker, "rebuild_archives", rebuild_archives) + cmd(archiver, "check", *args, exit_code=exit_code) + + # builds: in check(), and with --repair also in finish(). + assert loaded_at_build == ([False, False] if "--repair" in args else [False]) + assert same_index == [True] + assert repository_builds == 0 + if "--repair" in args: + cmd(archiver, "check", exit_code=0) + + +def test_check_without_repair_leaves_the_chunk_index_alone(archivers, request): + """check without --repair does not change the chunk index. + + The archive has an item metadata chunk missing: the checker re-chunks its item metadata stream into + chunks the repository does not have. + """ + archiver = request.getfixturevalue(archivers) + check_cmd_setup(archiver) + archive, repository = open_archive(archiver.repository_path, "archive1") + with repository: + repository.delete(archive.item_ids[0], validate=None) + with Repository(archiver.repository_location, exclusive=True) as repository: + index_before = {info.name for info in repository.store_list("index")} + chunk_ids_before = {chunk_id for chunk_id, _ in repository.chunks.iteritems()} + + cmd(archiver, "check", "--archives-only", exit_code=1) + cmd(archiver, "check", exit_code=1) + + with Repository(archiver.repository_location, exclusive=True) as repository: + assert {info.name for info in repository.store_list("index")} == index_before + assert {chunk_id for chunk_id, _ in repository.chunks.iteritems()} == chunk_ids_before + + def test_check_format(archivers, request): archiver = request.getfixturevalue(archivers) check_cmd_setup(archiver)