From 122fa47aa78b7a9fc020da0717f64097cd99fbaf Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Mon, 14 Sep 2026 19:12:03 +0530 Subject: [PATCH 1/6] 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 da266d6258..beac515b83 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2274,6 +2274,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, @@ -2287,10 +2289,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: @@ -2422,10 +2430,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: @@ -2568,14 +2576,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.""" @@ -2797,9 +2802,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 8706c12059..26c58cd4fd 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -465,6 +465,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) From d1897e498d6ff2c24e8071da45e71298938ba500 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Wed, 16 Sep 2026 14:49:24 +0530 Subject: [PATCH 2/6] check: review feedback, refs #10364 A check without --repair uses the index the repository check loaded. Tighten the comments. --- src/borg/archive.py | 52 +++++++++++-------- src/borg/repository.py | 9 ++-- src/borg/testsuite/archiver/check_cmd_test.py | 15 +++--- 3 files changed, 42 insertions(+), 34 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index beac515b83..c6b5c42802 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2274,29 +2274,34 @@ 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, - 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, - ) - 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 + if not repair and self.repository.is_chunk_index_loaded: + # the repository check loaded the index from the index/ fragments: use it. + self.chunks = self.repository.chunks + else: + # build the index, from the packs with --repair. Free the index the repository check may + # have loaded first, so only one is in memory. + self.repository.invalidate_chunk_index() + self.chunks = build_chunkindex_from_repo( + self.repository, + slow_rebuild=repair, + 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, + ) + if repair: + # the rebuild from the packs sets F_NEW (entry not stored in the index/ fragments yet) on + # every entry. Clear it, so Repository.close() does not store the whole index as a new + # fragment if the check stops before finish(). + 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 @@ -2576,6 +2581,7 @@ def add_callback(chunk): return id_ def add_reference(id_, size, cdata): + # size: unused, archive_put_items passes it. # --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 diff --git a/src/borg/repository.py b/src/borg/repository.py index c5b7df3548..4af54e1595 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -1145,11 +1145,10 @@ def chunks(self): @chunks.setter def chunks(self, value): - # The index is normally built lazily; this setter exists for the few callers - # that must install a specific index: wiping the cache, restoring an index - # captured before close(), or compact sharing the index it built itself (it - # needs the usage flags) so the repository does not build a second one. To - # drop a stale index so it rebuilds, do not assign None here -- call + # The index is normally built lazily; this setter installs a specific index: wiping the + # cache, restoring an index captured before close(), or an index compact (it needs the + # usage flags) or check built itself, so the repository does not build a second one. + # To drop a stale index so it rebuilds, do not assign None here -- call # invalidate_chunk_index() instead. self._chunks = value diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 26c58cd4fd..ac5ceb5651 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -465,11 +465,15 @@ def test_missing_archive_metadata(archivers, request): cmd(archiver, "check", exit_code=0) +# 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(). @pytest.mark.parametrize( - "args, exit_code", [(["--archives-only"], 1), ([], 1), (["--repair"], 0)], ids=["archives-only", "full", "repair"] + "args, exit_code, checker_builds", + [(["--archives-only"], 1, [False]), ([], 1, []), (["--repair"], 0, [False, False])], + 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.""" +def test_check_holds_a_single_chunk_index(archiver, monkeypatch, args, exit_code, checker_builds): + """check has at most one chunk index in memory: the repository and the checker use the same index.""" # 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. @@ -477,7 +481,7 @@ def test_check_holds_a_single_chunk_index(archiver, monkeypatch, args, exit_code 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 + loaded_at_build = [] real_build = archive_module.build_chunkindex_from_repo def build_chunkindex_from_repo(repository, **kwargs): @@ -505,8 +509,7 @@ def rebuild_archives(self, **kwargs): 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 loaded_at_build == checker_builds assert same_index == [True] assert repository_builds == 0 if "--repair" in args: From 753a41c8e3affe8fc34f8ffcb036f6f686f0473f Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Wed, 16 Sep 2026 18:23:15 +0530 Subject: [PATCH 3/6] check: review round 3, refs #10364 Mark the index invalid before verify_data() deletes, flush before using the index, never store the checker's index at close(). --- src/borg/archive.py | 35 ++++-- src/borg/repository.py | 17 +-- src/borg/testsuite/archiver/check_cmd_test.py | 115 +++++++++++++++--- 3 files changed, 130 insertions(+), 37 deletions(-) diff --git a/src/borg/archive.py b/src/borg/archive.py index c6b5c42802..67d3387318 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -23,6 +23,7 @@ from . import xattr from .chunkers import get_chunker, Chunk, release_chunk_data from .cache import ChunkListEntry, build_chunkindex_from_repo, write_chunkindex_to_repo +from .cache import write_chunkindex_invalid, delete_chunkindex_invalid from .crypto.key import key_from_repository from .constants import * # NOQA from .digests import ContentDigester @@ -2265,7 +2266,7 @@ def check( # The rebuild validates every object header it walks, because a corrupt data_size parses fine # and points the walk into the middle of the pack. That costs one metadata slot read and one # decryption per object and it needs the key, so read the key here if we do not have it yet. - # manifest_only=True: the other key source make_key uses is self.chunks, built just below. + # manifest_only=True: the other key source make_key uses is self.chunks, loaded or built just below. if repair and self.key is None: self.key = self.make_key(repository, manifest_only=True) if self.key is not None: @@ -2274,6 +2275,9 @@ def check( validate = object_validator(self.repo_objs) else: validate = None + # store the chunks the pack writer still buffers: an index rebuild would drop their entries, + # and get() can not read them from the index loaded below. + self.repository.flush() if not repair and self.repository.is_chunk_index_loaded: # the repository check loaded the index from the index/ fragments: use it. self.chunks = self.repository.chunks @@ -2294,18 +2298,13 @@ def check( drop_corrupt_tail=repair, write_immediately=False, ) - if repair: - # the rebuild from the packs sets F_NEW (entry not stored in the index/ fragments yet) on - # every entry. Clear it, so Repository.close() does not store the whole index as a new - # fragment if the check stops before finish(). - self.chunks.clear_new() + # a rebuild from the packs sets F_NEW (entry not stored in the index/ fragments yet) on every + # entry. Clear it, so Repository.close() does not store this index as a new fragment: only + # finish() stores the index, and only with --repair. + 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: self.key = self.make_key(repository) self.repo_objs = RepoObj(self.key) @@ -2415,6 +2414,7 @@ def verify_data(self): if self.repair: logger.warning("Found defect chunks, removing them from the repository.") validate = object_validator(self.repo_objs) + index_marked_invalid = False for defect_chunk in defect_chunks: # remote repo (ssh): retry might help for strange network / NIC / RAM errors # as the chunk will be retransmitted from remote server. @@ -2437,6 +2437,13 @@ def verify_data(self): # 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. + if not index_marked_invalid: + # the index/ fragments point the other chunks of a rewritten pack at the + # deleted pack until finish() stores the new index. Mark them invalid before + # the first delete, so if the check stops before finish(), the next use + # rebuilds the index from the packs. + write_chunkindex_invalid(self.repository) + index_marked_invalid = True self.repository.delete(defect_chunk, update_index=False, validate=validate) self.chunks_modified = True else: @@ -2576,7 +2583,10 @@ def record_missing_chunk(archive_name, path, chunk_id, size): def add_callback(chunk): id_ = self.key.id_hash(chunk) - cdata = self.repo_objs.format(id_, {}, chunk, ro_type=ROBJ_ARCHIVE_STREAM) + cdata = None + if self.repair and id_ not in self.chunks: + # only add_reference stores it, so only compress and encrypt the chunk for that. + cdata = self.repo_objs.format(id_, {}, chunk, ro_type=ROBJ_ARCHIVE_STREAM) add_reference(id_, len(chunk), cdata) return id_ @@ -2826,6 +2836,9 @@ def finish(self): write_chunkindex_to_repo( self.repository, self.chunks, incremental=False, clear=False, force_write=True, delete_other=True ) + # the index just written matches the packs: clear the invalid marker, set by verify_data() or left + # by an interrupted operation. + 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() self.manifest.write() diff --git a/src/borg/repository.py b/src/borg/repository.py index 4af54e1595..7dab058afa 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -953,10 +953,10 @@ 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, set by ArchiveChecker.check() (see - # PackReader.iter_headers): a validate callable makes the rebuild resync past a corrupt - # object header, drop_corrupt_tail - only set when repairing - makes it index the pack up - # to that header and drop the rest. Without either, such a header aborts the rebuild. + # 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. No caller sets them: ArchiveChecker.check() installs its own index. 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 @@ -1146,7 +1146,7 @@ def chunks(self): @chunks.setter def chunks(self, value): # The index is normally built lazily; this setter installs a specific index: wiping the - # cache, restoring an index captured before close(), or an index compact (it needs the + # cache, restoring an index captured before close(), or an index that compact (it needs the # usage flags) or check built itself, so the repository does not build a second one. # To drop a stale index so it rebuilds, do not assign None here -- call # invalidate_chunk_index() instead. @@ -1155,9 +1155,10 @@ def chunks(self, value): def invalidate_chunk_index(self): """Drop the in-memory chunk index so close() will not persist a stale copy. - Called when the on-disk chunk index is deleted; the next access to - .chunks rebuilds the index from actual repository contents. PackWriter - reads the index through this Repository, so it follows automatically. + Called when the on-disk chunk index is deleted, and before a caller builds + its own index, so the old one is freed first. The next access to .chunks + rebuilds the index from actual repository contents. PackWriter reads the + index through this Repository, so it follows automatically. """ self._chunks = None diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index ac5ceb5651..ab2acac101 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -1,7 +1,9 @@ +import gc from pathlib import Path import re import shutil import struct +import weakref from unittest.mock import patch import pytest @@ -9,11 +11,13 @@ from ...crypto.key import store_hash, STORE_HASH_NAME from ... import archive as archive_module from ...archive import Archive, ArchiveChecker, ChunkBuffer -from ...cache import Cache, delete_chunkindex_from_repo +from ...cache import Cache, chunkindex_is_invalid, delete_chunkindex_from_repo, list_chunkindex_hashes +from ...cache import write_chunkindex_invalid from ...constants import * # NOQA from ...helpers import bin_to_hex, msgpack, CommandError, CorruptPack, Error, IntegrityError, sig_int from ...helpers import BackupDamagedChunksError from ...helpers.passphrase import PassphraseWrong +from ...hashindex import ChunkIndex from ...item import Item from ...manifest import Archives, Manifest from ...repoobj import RepoObj @@ -26,6 +30,7 @@ create_src_archive, create_regular_file, open_archive, + open_repository, generate_archiver_tests, read_chunk, write_wrong_content_chunk, @@ -443,12 +448,17 @@ def test_missing_file_chunk_refs_truncated(archivers, request): assert f"only the first {cap} files are listed" in output # the remaining referencing files are truncated -def test_missing_archive_item_chunk(archivers, request): - archiver = request.getfixturevalue(archivers) +def delete_first_item_chunk(archiver): + """Set up two archives and delete the first item metadata chunk of archive1.""" check_cmd_setup(archiver) archive, repository = open_archive(archiver.repository_path, "archive1") with repository: repository.delete(archive.item_ids[0], validate=None) + + +def test_missing_archive_item_chunk(archivers, request): + archiver = request.getfixturevalue(archivers) + delete_first_item_chunk(archiver) cmd(archiver, "check", exit_code=1) cmd(archiver, "check", "--repair", exit_code=0) cmd(archiver, "check", exit_code=0) @@ -475,18 +485,21 @@ def test_missing_archive_metadata(archivers, request): def test_check_holds_a_single_chunk_index(archiver, monkeypatch, args, exit_code, checker_builds): """check has at most one chunk index in memory: the repository and the checker use the same index.""" # 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) + delete_first_item_chunk(archiver) loaded_at_build = [] + built = [] # weak references to the indexes the checker built + alive_at_build = [] # per index build in ArchiveChecker: whether an index it built before is still alive 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) + gc.collect() # PyPy frees objects only on collection + alive_at_build.append(any(ref() is not None for ref in built)) + chunks = real_build(repository, **kwargs) + built.append(weakref.ref(chunks)) + return chunks repository_builds = 0 # index builds by the Repository.chunks property real_chunks = Repository.chunks @@ -510,35 +523,101 @@ def rebuild_archives(self, **kwargs): cmd(archiver, "check", *args, exit_code=exit_code) assert loaded_at_build == checker_builds + assert alive_at_build == [False] * len(checker_builds) 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): +@pytest.mark.parametrize("delete_index", [False, True], ids=["index", "no-index"]) +def test_check_without_repair_leaves_the_chunk_index_alone(archivers, request, delete_index): """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. + chunks the repository does not have. Without index/ fragments, the checker builds the index from the + packs and does not store it either. """ 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")} + delete_first_item_chunk(archiver) + with open_repository(archiver) as repository: chunk_ids_before = {chunk_id for chunk_id, _ in repository.chunks.iteritems()} + if delete_index: + delete_chunkindex_from_repo(repository) + index_before = list_chunkindex_hashes(repository) 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 + with open_repository(archiver) as repository: + assert list_chunkindex_hashes(repository) == index_before assert {chunk_id for chunk_id, _ in repository.chunks.iteritems()} == chunk_ids_before +@pytest.mark.parametrize("repair", [False, True], ids=["check", "repair"]) +def test_check_with_buffered_chunks(archiver, repair): + """ArchiveChecker.check() stores the chunks the pack writer still buffers before it uses the index.""" + check_cmd_setup(archiver) + archive, repository = open_archive(archiver.repository_path, "archive1") + with repository: + data = b"buffered chunk" + chunk_id = archive.key.id_hash(data) + repository.put(chunk_id, archive.repo_objs.format(chunk_id, {}, data, ro_type=ROBJ_FILE_STREAM)) + assert repository.chunks[chunk_id].flags & ChunkIndex.F_PENDING + ArchiveChecker().check(repository, verify_data=True, repair=repair, sort_by="ts", format="{archive}") + with open_repository(archiver) as repository: + assert repository.get(chunk_id) + + +def test_check_repair_verify_data_aborted_marks_the_index_invalid(archiver, monkeypatch): + """A --repair --verify-data check that stops after a delete leaves the chunk index marked invalid. + + delete() rewrites the pack of the defect chunk, so the index/ fragments point its other chunks at a + pack that is gone. The marker makes the next use rebuild the index from the packs. + """ + check_cmd_setup(archiver) + archive, repository = open_archive(archiver.repository_path, "archive1") + with repository: + for item in archive.iter_items(): + if item.path.endswith(src_file): + defect_id = item.chunks[-1].id + break + corrupt_chunk_on_disk(repository, defect_id) + chunk_ids = {chunk_id for chunk_id, _ in repository.chunks.iteritems()} - {defect_id} + + def rebuild_archives(self, **kwargs): + raise Error("stopped before finish()") + + with monkeypatch.context() as m: + m.setattr(ArchiveChecker, "rebuild_archives", rebuild_archives) + with open_repository(archiver) as repository: + with pytest.raises(Error, match="stopped before finish"): + ArchiveChecker().check(repository, verify_data=True, repair=True, sort_by="ts", format="{archive}") + + with open_repository(archiver) as repository: + assert chunkindex_is_invalid(repository) + # the index rebuilt from the packs finds every other chunk. + for chunk_id in chunk_ids: + repository.get(chunk_id) + cmd(archiver, "check", "--repair", exit_code=0) + with open_repository(archiver) as repository: + assert not chunkindex_is_invalid(repository) + + +def test_check_repair_clears_the_invalid_marker(archivers, request): + """check --repair clears the invalid marker when it stores the index, also without index/ fragments before.""" + archiver = request.getfixturevalue(archivers) + check_cmd_setup(archiver) + with open_repository(archiver) as repository: + delete_chunkindex_from_repo(repository) + write_chunkindex_invalid(repository) + cmd(archiver, "check", "--repair", exit_code=0) + with open_repository(archiver) as repository: + assert not chunkindex_is_invalid(repository) + assert list_chunkindex_hashes(repository) + cmd(archiver, "check", exit_code=0) + + def test_check_format(archivers, request): archiver = request.getfixturevalue(archivers) check_cmd_setup(archiver) From 9c05196849a1e49cc7788c2358558be3cc8acfa7 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Thu, 17 Sep 2026 15:06:53 +0530 Subject: [PATCH 4/6] check: review round 4, refs #10364 Repository.delete() writes the chunk index invalid marker before rewriting a pack, finish() writes it before rebuilding the index, so an abort leaves no stale index. Tighten comments and docs, fix the vacuous verify-data abort test, add marker tests. --- docs/internals/data-structures.rst | 5 +- docs/internals/packs.rst | 7 +- src/borg/archive.py | 51 ++++++-------- src/borg/cache.py | 8 ++- src/borg/constants.py | 9 +-- src/borg/repository.py | 28 ++++---- src/borg/testsuite/archiver/check_cmd_test.py | 61 +++++++++++++--- src/borg/testsuite/repository_test.py | 70 ++++++++++++++++++- 8 files changed, 176 insertions(+), 63 deletions(-) diff --git a/docs/internals/data-structures.rst b/docs/internals/data-structures.rst index a2187bfa33..ed76c28072 100644 --- a/docs/internals/data-structures.rst +++ b/docs/internals/data-structures.rst @@ -92,7 +92,10 @@ cache/ a marker object: while it is present, the chunks index in ``index/`` is considered invalid. It is written before deleting index fragments and removed after the last stale fragment is gone, so an interrupted run does not leave the remaining - fragments looking like a complete index. + fragments looking like a complete index. Deleting a single object rewrites its + pack without it and deletes the old pack, leaving the fragments pointing the pack's + other objects at a deleted pack: the marker is written before that rewrite and + removed after the 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 4774d7ff42..52c4305dc3 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -322,8 +322,11 @@ and deletes all the fragments it supersedes. A deletion that could drop entries -- dropping the index entirely, or the full rewrite above -- is guarded by a marker object, ``cache/chunkindex-invalid``, written before -the first deletion and removed after the last one. While the marker is present, -leftover fragments could be an incomplete index, so they are not merged; the index is +the first deletion and removed after the last one. Deleting a single object rewrites +its pack without it and deletes the old pack, leaving the fragments pointing the pack's +other objects at a deleted pack: the marker is written before that rewrite and removed +after the index is stored. While the marker is present, +leftover fragments could be an incomplete or stale index, 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. diff --git a/src/borg/archive.py b/src/borg/archive.py index 67d3387318..3401f6e1de 100644 --- a/src/borg/archive.py +++ b/src/borg/archive.py @@ -2266,7 +2266,7 @@ def check( # The rebuild validates every object header it walks, because a corrupt data_size parses fine # and points the walk into the middle of the pack. That costs one metadata slot read and one # decryption per object and it needs the key, so read the key here if we do not have it yet. - # manifest_only=True: the other key source make_key uses is self.chunks, loaded or built just below. + # manifest_only=True: self.chunks, the other key source of make_key, is set up below. if repair and self.key is None: self.key = self.make_key(repository, manifest_only=True) if self.key is not None: @@ -2275,15 +2275,14 @@ def check( validate = object_validator(self.repo_objs) else: validate = None - # store the chunks the pack writer still buffers: an index rebuild would drop their entries, - # and get() can not read them from the index loaded below. + # store the chunks buffered in the pack writer, so the index below has their pack locations + # (pack id, offset and size in the pack). self.repository.flush() if not repair and self.repository.is_chunk_index_loaded: - # the repository check loaded the index from the index/ fragments: use it. + # without --repair, use the loaded index. self.chunks = self.repository.chunks else: - # build the index, from the packs with --repair. Free the index the repository check may - # have loaded first, so only one is in memory. + # free the loaded index first, so only one index is in memory. --repair builds it from the packs. self.repository.invalidate_chunk_index() self.chunks = build_chunkindex_from_repo( self.repository, @@ -2298,12 +2297,12 @@ def check( drop_corrupt_tail=repair, write_immediately=False, ) - # a rebuild from the packs sets F_NEW (entry not stored in the index/ fragments yet) on every - # entry. Clear it, so Repository.close() does not store this index as a new fragment: only - # finish() stores the index, and only with --repair. + # clear F_NEW (entry not in the index/ fragments yet), so Repository.close() does not store + # this index; finish() stores it with --repair. Without --repair, a repository without index/ + # fragments keeps none. With the invalid marker set (see write_chunkindex_invalid), the build + # deletes the fragments. 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. + # get(), put() and delete() use the repository's index. self.repository.chunks = self.chunks if self.key is None: self.key = self.make_key(repository) @@ -2414,7 +2413,6 @@ def verify_data(self): if self.repair: logger.warning("Found defect chunks, removing them from the repository.") validate = object_validator(self.repo_objs) - index_marked_invalid = False for defect_chunk in defect_chunks: # remote repo (ssh): retry might help for strange network / NIC / RAM errors # as the chunk will be retransmitted from remote server. @@ -2433,17 +2431,9 @@ def verify_data(self): ) except IntegrityErrorBase: # 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. - if not index_marked_invalid: - # the index/ fragments point the other chunks of a rewritten pack at the - # deleted pack until finish() stores the new index. Mark them invalid before - # the first delete, so if the check stops before finish(), the next use - # rebuilds the index from the packs. - write_chunkindex_invalid(self.repository) - index_marked_invalid = True + # 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) self.chunks_modified = True else: @@ -2585,14 +2575,14 @@ def add_callback(chunk): id_ = self.key.id_hash(chunk) cdata = None if self.repair and id_ not in self.chunks: - # only add_reference stores it, so only compress and encrypt the chunk for that. + # cdata: the compressed and encrypted chunk, which only add_reference stores. cdata = self.repo_objs.format(id_, {}, chunk, ro_type=ROBJ_ARCHIVE_STREAM) add_reference(id_, len(chunk), cdata) return id_ def add_reference(id_, size, cdata): - # size: unused, archive_put_items passes it. - # --repair: store a chunk the repository does not have. put() adds it to self.chunks. + # size: unused, part of the archive_put_items callback signature. + # 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) @@ -2818,8 +2808,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: rebuild the index from them, validating every object header, and - # store it. Free the current index first, so only one is in memory. + # 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. + write_chunkindex_invalid(self.repository) self.repository.invalidate_chunk_index() self.chunks = None logger.info("Rebuilding and writing the repository chunks index.") @@ -2836,8 +2828,7 @@ def finish(self): write_chunkindex_to_repo( self.repository, self.chunks, incremental=False, clear=False, force_write=True, delete_other=True ) - # the index just written matches the packs: clear the invalid marker, set by verify_data() or left - # by an interrupted operation. + # 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() diff --git a/src/borg/cache.py b/src/borg/cache.py index 50807d075a..8a42639d9d 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -619,15 +619,17 @@ def chunkindex_is_invalid(repository): def write_chunkindex_invalid(repository): - """Mark the chunk index as invalid. Call before deleting index fragments. + """Store the invalid marker, cache/chunkindex-invalid. - If the deletion is interrupted, the marker remains and the index is rebuilt on next load. + While the marker is present, the index/ fragments may be incomplete (some of them deleted) or stale + (pointing objects at a pack that Repository.delete() rewrote and deleted), and build_chunkindex_from_repo + rebuilds the index from the packs instead of merging them. """ repository.store_store(f"cache/{CHUNKINDEX_INVALID_SENTINEL}", b"") def delete_chunkindex_invalid(repository): - """Clear the chunk-index-invalid marker. Call after all fragment deletions have completed.""" + """Delete the invalid marker, if present. The index/ fragments must be complete and match the packs.""" try: repository.store_delete(f"cache/{CHUNKINDEX_INVALID_SENTINEL}") except StoreObjectNotFound: diff --git a/src/borg/constants.py b/src/borg/constants.py index 9baac146ad..913b57b279 100644 --- a/src/borg/constants.py +++ b/src/borg/constants.py @@ -138,10 +138,11 @@ # mid-merge (a concurrent repack replaced it). After that, fall back to the slow rebuild from packs. CHUNKINDEX_MERGE_ATTEMPTS = 3 # Marker object in the cache/ namespace: the chunk index is invalid. Written before deleting index -# fragments, removed after the last fragment is gone. While it is present, the chunk index is rebuilt -# from the packs on next load and the leftover index/ fragments are deleted. Removing the marker while -# index/ fragments remain makes those fragments look like a complete index, so only -# delete_chunkindex_invalid() removes it, and clearing cache/ requires clearing index/ too. +# fragments, removed after the last fragment is gone. Written before Repository.delete() rewrites a pack +# without an object and deletes the old pack, removed after the index is stored. While it is present, the +# chunk index is rebuilt from the packs on next load and the leftover index/ fragments are deleted. +# Removing the marker while index/ fragments remain makes those fragments look like a complete index, so +# only delete_chunkindex_invalid() removes it, and clearing cache/ requires clearing index/ too. CHUNKINDEX_INVALID_SENTINEL = "chunkindex-invalid" FD_MAX_AGE = 4 * 60 # 4 minutes diff --git a/src/borg/repository.py b/src/borg/repository.py index 7dab058afa..a198443527 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -956,7 +956,7 @@ def __init__( # 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. No caller sets them: ArchiveChecker.check() installs its own index. + # aborts the rebuild. TODO: nothing sets them, remove them (#10378). 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 @@ -1153,12 +1153,10 @@ def chunks(self, value): self._chunks = value def invalidate_chunk_index(self): - """Drop the in-memory chunk index so close() will not persist a stale copy. + """Drop the in-memory chunk index, so close() does not persist it and its memory is freed. - Called when the on-disk chunk index is deleted, and before a caller builds - its own index, so the old one is freed first. The next access to .chunks - rebuilds the index from actual repository contents. PackWriter reads the - index through this Repository, so it follows automatically. + The next access to .chunks builds the index again. PackWriter reads the index through this + Repository, so it uses the new one. """ self._chunks = None @@ -1341,8 +1339,8 @@ def store_list(namespace): # the index is checked first and in full, on partial checks too: it is small, and index errors # stop the pack check below. index_infos = store_list("index") - # an interrupted fragment deletion leaves the invalid marker set; the index is rebuilt on next - # use, so warn rather than fail. + # with the invalid marker set, the index/ fragments may be incomplete or stale (see + # write_chunkindex_invalid). The next use rebuilds the index from the packs, so warn. from .cache import chunkindex_is_invalid, build_chunkindex_from_repo index_invalid = chunkindex_is_invalid(self) @@ -1685,12 +1683,16 @@ def put(self, id, data): def delete(self, id, *, validate, update_index=True): """Delete a single repo object by rewriting its pack without it (via compact_pack). - With update_index=True the full chunk index is written back so the next borg process sees the - deletion; callers that rebuild the index themselves (check --repair) pass update_index=False to - skip the per-object index rewrite. + The rewrite deletes the old pack, so the index/ fragments point the pack's other objects at a + deleted pack until the index is stored again. delete() writes the invalid marker (see + write_chunkindex_invalid) before the rewrite. + 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. """ + from .cache import write_chunkindex_to_repo, write_chunkindex_invalid, delete_chunkindex_invalid + self._lock_refresh() entry = self.chunks.get(id) if entry is None: @@ -1699,13 +1701,13 @@ 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) + write_chunkindex_invalid(self) self.compact_pack(pack_id, keep_ids=keep_ids, drop_ids={id}, validate=validate) if update_index: # close() only persists new entries incrementally, so write the full index here to record # the removal for the next borg process. - from .cache import write_chunkindex_to_repo - write_chunkindex_to_repo(self, self.chunks, incremental=False, force_write=True, delete_other=True) + delete_chunkindex_invalid(self) def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, validate, chunks=None): """Rewrite pack , keeping and dropping , then delete the old pack. diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index ab2acac101..bc6f78bc74 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -12,6 +12,7 @@ from ... import archive as archive_module from ...archive import Archive, ArchiveChecker, ChunkBuffer from ...cache import Cache, chunkindex_is_invalid, delete_chunkindex_from_repo, list_chunkindex_hashes +from ...cache import read_chunkindex_from_repo from ...cache import write_chunkindex_invalid from ...constants import * # NOQA from ...helpers import bin_to_hex, msgpack, CommandError, CorruptPack, Error, IntegrityError, sig_int @@ -530,27 +531,33 @@ def rebuild_archives(self, **kwargs): cmd(archiver, "check", exit_code=0) -@pytest.mark.parametrize("delete_index", [False, True], ids=["index", "no-index"]) -def test_check_without_repair_leaves_the_chunk_index_alone(archivers, request, delete_index): - """check without --repair does not change the chunk index. +@pytest.mark.parametrize("index", ["index", "no-index", "marker"]) +def test_check_without_repair_stores_no_chunk_index(archivers, request, index): + """check without --repair does not store a 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. Without index/ fragments, the checker builds the index from the - packs and does not store it either. + chunks the repository does not have. With index/ fragments, the check leaves them as they are. Without + them, the checker builds the index from the packs and does not store it. With the invalid marker set, + that build deletes the fragments and the marker, and the check does not store its index either. """ archiver = request.getfixturevalue(archivers) delete_first_item_chunk(archiver) with open_repository(archiver) as repository: chunk_ids_before = {chunk_id for chunk_id, _ in repository.chunks.iteritems()} - if delete_index: + if index == "no-index": delete_chunkindex_from_repo(repository) index_before = list_chunkindex_hashes(repository) + assert bool(index_before) is (index != "no-index") + if index == "marker": + write_chunkindex_invalid(repository) cmd(archiver, "check", "--archives-only", exit_code=1) cmd(archiver, "check", exit_code=1) with open_repository(archiver) as repository: - assert list_chunkindex_hashes(repository) == index_before + # check the stored state before .chunks rebuilds the index and close() stores it. + assert list_chunkindex_hashes(repository) == (index_before if index == "index" else []) + assert not chunkindex_is_invalid(repository) assert {chunk_id for chunk_id, _ in repository.chunks.iteritems()} == chunk_ids_before @@ -595,11 +602,47 @@ def rebuild_archives(self, **kwargs): ArchiveChecker().check(repository, verify_data=True, repair=True, sort_by="ts", format="{archive}") with open_repository(archiver) as repository: + # no .chunks access or get() here: rebuilding the index would clear the marker. assert chunkindex_is_invalid(repository) - # the index rebuilt from the packs finds every other chunk. + # the index/ fragments are still there and point chunks at the pack delete() removed. + packs = {info.name for info in repository.store_list("packs")} + stale = set() + for hash in list_chunkindex_hashes(repository): + fragment = read_chunkindex_from_repo(repository, hash) + stale |= {chunk_id for chunk_id, entry in fragment.items() if bin_to_hex(entry.pack_id) not in packs} + assert stale & chunk_ids + cmd(archiver, "check", "--repair", exit_code=0) + with open_repository(archiver) as repository: + assert not chunkindex_is_invalid(repository) + # the stored index finds every other chunk. for chunk_id in chunk_ids: repository.get(chunk_id) - cmd(archiver, "check", "--repair", exit_code=0) + + +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. + + 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) + + with monkeypatch.context() as m: + m.setattr(archive_module, "build_chunkindex_from_repo", build_chunkindex_from_repo) + with open_repository(archiver) as repository: + with pytest.raises(Error, match="stopped in the index rebuild"): + ArchiveChecker().check(repository, repair=True, sort_by="ts", format="{archive}") + + with open_repository(archiver) as repository: + assert chunkindex_is_invalid(repository) + assert list_chunkindex_hashes(repository) + cmd(archiver, "check", exit_code=0) with open_repository(archiver) as repository: assert not chunkindex_is_invalid(repository) diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 80525924c4..1d11653ab6 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -11,7 +11,7 @@ from ..crypto.key import store_hash from .. import repository as repository_module -from ..cache import write_chunkindex_invalid +from ..cache import chunkindex_is_invalid, delete_chunkindex_from_repo, write_chunkindex_invalid from ..compress import CNONE from ..constants import MAX_CLOCK_SKEW, ROBJ_FILE_STREAM from ..crypto.key import CHPOKey, ChecksumKey @@ -443,6 +443,74 @@ def test_delete_with_stale_earlier_object_in_pack(repo_fixtures, request): assert pdchunk(repository.get(H(0))) == b"ccc" # H(0) still served from its new pack +@pytest.mark.parametrize("stored", [True, False], ids=["stored-index", "no-stored-index"]) +@pytest.mark.parametrize("update_index", [True, False]) +def test_delete_marks_the_chunk_index_invalid(repo_fixtures, request, update_index, stored): + """delete() marks the chunk index invalid before it rewrites the pack; update_index=True clears the marker. + + Without stored fragments, the full index write deletes none and leaves the marker; delete() deletes it. + """ + with get_repository_from_fixture(repo_fixtures, request) as repository: + if not stored: + delete_chunkindex_from_repo(repository) # repository creation stored an empty index + repository._pack_writer.max_count = 2 # H(0) and H(1) share a pack + repository.put(H(0), fchunk(b"aaa", chunk_id=H(0))) + repository.put(H(1), fchunk(b"bbb", chunk_id=H(1))) + repository.flush() + if not stored: + assert not list(repository.store_list("index")) + repository.delete(H(1), validate=None, update_index=update_index) + assert chunkindex_is_invalid(repository) is not update_index + if stored: + with reopen(repository) as repository: + assert list(repository.store_list("index")) + repository.delete(H(1), validate=None, update_index=update_index) + assert chunkindex_is_invalid(repository) is not update_index + with reopen(repository) as repository: + assert pdchunk(repository.get(H(0))) == b"aaa" + with pytest.raises(Repository.ObjectNotFound): + repository.get(H(1)) + + +def test_delete_missing_object_leaves_the_chunk_index_valid(repo_fixtures, request): + """delete() of an object the index does not have does not mark the chunk index invalid.""" + with get_repository_from_fixture(repo_fixtures, request) as repository: + assert H(0) not in repository.chunks # load the index before delete(): loading it deletes the marker + with pytest.raises(Repository.ObjectNotFound): + repository.delete(H(0), validate=None) + assert not chunkindex_is_invalid(repository) + + +def test_delete_stopped_in_the_pack_rewrite_leaves_the_chunk_index_invalid(repo_fixtures, request, monkeypatch): + """A delete() that stops after deleting the old pack leaves the marker; the next use rebuilds the index.""" + with get_repository_from_fixture(repo_fixtures, request) as repository: + repository._pack_writer.max_count = 2 # H(0) and H(1) share a pack + repository.put(H(0), fchunk(b"aaa", chunk_id=H(0))) + repository.put(H(1), fchunk(b"bbb", chunk_id=H(1))) + repository.flush() + old_pack_id = None + with pytest.raises(OSError, match="stopped"): + with reopen(repository) as repository: + old_pack_id = repository.chunks[H(0)].pack_id + real_store_delete = repository.store_delete + + def store_delete(name, *args, **kwargs): + if name.startswith("packs/"): + real_store_delete(name, *args, **kwargs) # the old pack is gone, the index not stored yet + raise OSError("stopped") + return real_store_delete(name, *args, **kwargs) + + monkeypatch.setattr(repository, "store_delete", store_delete) + repository.delete(H(1), validate=None) + with reopen(repository) as repository: + assert chunkindex_is_invalid(repository) + with reopen(repository) as repository: + # the index rebuilt from the packs points H(0) at the rewritten pack. + assert repository.chunks[H(0)].pack_id != old_pack_id + assert pdchunk(repository.get(H(0))) == b"aaa" + assert not chunkindex_is_invalid(repository) + + def test_multi_object_pack_roundtrip(repo_fixtures, request): # Two objects fill one pack and must both read back: the second from a non-zero offset, and # read_data=False returning only its header+meta. The test pins max_count=2 so it does not depend From c331fa20eb47a17e7f5006a04f0c5a93f0831f85 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Thu, 17 Sep 2026 19:17:25 +0530 Subject: [PATCH 5/6] check: review round 5, refs #10364 compact_pack writes the invalid marker just before deleting the old pack; tighten marker docs and tests. --- docs/internals/data-structures.rst | 11 ++--- docs/internals/packs.rst | 12 ++--- src/borg/cache.py | 15 +++--- src/borg/constants.py | 7 ++- src/borg/repository.py | 27 +++++++---- src/borg/testsuite/archiver/check_cmd_test.py | 11 +++-- src/borg/testsuite/repository_test.py | 47 +++++++++++++------ 7 files changed, 82 insertions(+), 48 deletions(-) diff --git a/docs/internals/data-structures.rst b/docs/internals/data-structures.rst index ed76c28072..5bb863dfb1 100644 --- a/docs/internals/data-structures.rst +++ b/docs/internals/data-structures.rst @@ -90,12 +90,11 @@ cache/ of an unchanged archive. chunkindex-invalid a marker object: while it is present, the chunks index in ``index/`` is considered - invalid. It is written before deleting index fragments and removed after the last - stale fragment is gone, so an interrupted run does not leave the remaining - fragments looking like a complete index. Deleting a single object rewrites its - pack without it and deletes the old pack, leaving the fragments pointing the pack's - other objects at a deleted pack: the marker is written before that rewrite and - removed after the index is stored. + invalid, because its fragments may be missing entries or point at deleted packs. + It is written before deleting index fragments, before deleting a single object + (which rewrites its pack and deletes the old one), and before ``borg check --repair`` + rebuilds the index after changing the packs. It is removed after the last fragment + is deleted or once a matching 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 52c4305dc3..6de318555f 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -322,12 +322,12 @@ and deletes all the fragments it supersedes. A deletion that could drop entries -- dropping the index entirely, or the full rewrite above -- is guarded by a marker object, ``cache/chunkindex-invalid``, written before -the first deletion and removed after the last one. Deleting a single object rewrites -its pack without it and deletes the old pack, leaving the fragments pointing the pack's -other objects at a deleted pack: the marker is written before that rewrite and removed -after the index is stored. While the marker is present, -leftover fragments could be an incomplete or stale index, so they are not merged; the index is -rebuilt from the pack files on the next load instead. A consolidation needs no marker: +the first deletion and removed after the last one. The marker also guards deleting a +single object, which rewrites its pack and deletes the old one, and ``borg check +--repair`` rebuilding the index after it changed the packs; there it is removed 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. diff --git a/src/borg/cache.py b/src/borg/cache.py index 8a42639d9d..a10209fe1b 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -621,15 +621,18 @@ def chunkindex_is_invalid(repository): def write_chunkindex_invalid(repository): """Store the invalid marker, cache/chunkindex-invalid. - While the marker is present, the index/ fragments may be incomplete (some of them deleted) or stale - (pointing objects at a pack that Repository.delete() rewrote and deleted), and build_chunkindex_from_repo - rebuilds the index from the packs instead of merging them. + Store it before a store change that leaves the index/ fragments missing entries or pointing at deleted + packs. While it is present, build_chunkindex_from_repo rebuilds the index from the packs instead of merging + the fragments. """ repository.store_store(f"cache/{CHUNKINDEX_INVALID_SENTINEL}", b"") def delete_chunkindex_invalid(repository): - """Delete the invalid marker, if present. The index/ fragments must be complete and match the packs.""" + """Delete the invalid marker, if present. + + The index/ fragments, if any, must list every chunk in the packs and point only at existing packs. + """ try: repository.store_delete(f"cache/{CHUNKINDEX_INVALID_SENTINEL}") except StoreObjectNotFound: @@ -650,7 +653,7 @@ def delete_chunkindex_from_repo(repository): pass if hashes or invalid: # clear the marker after every fragment is gone; also clears a marker left behind by an - # earlier interrupted deletion. + # interrupted operation. delete_chunkindex_invalid(repository) logger.debug(f"chunk indexes deleted: {hashes}") # the in-memory index is now stale; drop it so close() does not write it back into the @@ -923,7 +926,7 @@ def build_chunkindex_from_repo( if chunkindex_is_invalid(repository): if fragments_only: return None - # leftover fragments may be incomplete or stale. Finish the interrupted deletion + # the fragments may be missing entries or point at deleted packs. Delete them # (best-effort; a read-only client rebuilds in memory only), then rebuild from packs. logger.warning("chunk index is invalid (interrupted operation), rebuilding it.") try: diff --git a/src/borg/constants.py b/src/borg/constants.py index 913b57b279..8bfc3499f6 100644 --- a/src/borg/constants.py +++ b/src/borg/constants.py @@ -137,10 +137,9 @@ # How often to restart merging the fragments into a chunk index when a listed fragment vanishes # mid-merge (a concurrent repack replaced it). After that, fall back to the slow rebuild from packs. CHUNKINDEX_MERGE_ATTEMPTS = 3 -# Marker object in the cache/ namespace: the chunk index is invalid. Written before deleting index -# fragments, removed after the last fragment is gone. Written before Repository.delete() rewrites a pack -# without an object and deletes the old pack, removed after the index is stored. While it is present, the -# chunk index is rebuilt from the packs on next load and the leftover index/ fragments are deleted. +# Marker object in the cache/ namespace: the chunk index is invalid. While it is present, the index/ fragments +# may be missing entries or point at deleted packs, so the chunk index is rebuilt from the packs on next load +# and the leftover fragments are deleted. # Removing the marker while index/ fragments remain makes those fragments look like a complete index, so # only delete_chunkindex_invalid() removes it, and clearing cache/ requires clearing index/ too. CHUNKINDEX_INVALID_SENTINEL = "chunkindex-invalid" diff --git a/src/borg/repository.py b/src/borg/repository.py index a198443527..c24d4d127f 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -956,7 +956,7 @@ def __init__( # 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: nothing sets them, remove them (#10378). + # 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 @@ -1339,8 +1339,8 @@ def store_list(namespace): # the index is checked first and in full, on partial checks too: it is small, and index errors # stop the pack check below. index_infos = store_list("index") - # with the invalid marker set, the index/ fragments may be incomplete or stale (see - # write_chunkindex_invalid). The next use rebuilds the index from the packs, so warn. + # with the invalid marker set, the index/ fragments may be missing entries or point at deleted + # packs (see write_chunkindex_invalid). The next use rebuilds the index from the packs, so warn. from .cache import chunkindex_is_invalid, build_chunkindex_from_repo index_invalid = chunkindex_is_invalid(self) @@ -1684,8 +1684,8 @@ def delete(self, id, *, validate, update_index=True): """Delete a single repo object by rewriting its pack without it (via compact_pack). The rewrite deletes the old pack, so the index/ fragments point the pack's other objects at a - deleted pack until the index is stored again. delete() writes the invalid marker (see - write_chunkindex_invalid) before the rewrite. + deleted pack until the index is stored again. The invalid marker (see write_chunkindex_invalid) is + written via compact_pack's before_change, just before the old pack is deleted. 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. @@ -1701,15 +1701,20 @@ 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) - write_chunkindex_invalid(self) - self.compact_pack(pack_id, keep_ids=keep_ids, drop_ids={id}, validate=validate) + self.compact_pack( + pack_id, + keep_ids=keep_ids, + drop_ids={id}, + validate=validate, + before_change=lambda: write_chunkindex_invalid(self), + ) if update_index: # close() only persists new entries incrementally, so write the full index here to record # 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) - def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, validate, chunks=None): + def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, validate, chunks=None, before_change=None): """Rewrite pack , keeping and dropping , then delete the old pack. keep_ids: chunk ids in this pack to copy into the new pack. @@ -1717,6 +1722,8 @@ def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, validate, chunk validate: passed to superseded_gap_ranges, whose ranges are dropped. chunks: the ChunkIndex to look up the objects' pack locations in and to apply the index updates to. Must be the index keep_ids and drop_ids were derived from. Default: self.chunks. + before_change: callable without arguments, called once just before the old pack is deleted. Not + called when the pack is kept. Together, keep_ids and drop_ids must cover every object the chunk index lists for this pack; an unlisted indexed object would keep its bytes in the new pack but its index entry would go @@ -1794,6 +1801,10 @@ def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, validate, chunk for drop_id in drop_ids: # remove dropped objects from the index del chunks[drop_id] + # the new pack is not in the index/ fragments yet, so they still match the store; deleting the old + # pack makes them point at a deleted pack. + if before_change is not None and new_pack_id != pack_id: + before_change() if new_pack_id is None: # nothing kept: drop the pack, no replacement self.store_delete(pack_key) return None, dropped_bytes diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index bc6f78bc74..f0d53d9ac3 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -11,9 +11,14 @@ from ...crypto.key import store_hash, STORE_HASH_NAME from ... import archive as archive_module from ...archive import Archive, ArchiveChecker, ChunkBuffer -from ...cache import Cache, chunkindex_is_invalid, delete_chunkindex_from_repo, list_chunkindex_hashes -from ...cache import read_chunkindex_from_repo -from ...cache import write_chunkindex_invalid +from ...cache import ( + Cache, + chunkindex_is_invalid, + delete_chunkindex_from_repo, + list_chunkindex_hashes, + read_chunkindex_from_repo, + write_chunkindex_invalid, +) from ...constants import * # NOQA from ...helpers import bin_to_hex, msgpack, CommandError, CorruptPack, Error, IntegrityError, sig_int from ...helpers import BackupDamagedChunksError diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 1d11653ab6..e561725342 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -8,6 +8,7 @@ import pytest from borghash import HashTableNT +from borgstore.backends.errors import PermissionDenied as StorePermissionDenied from ..crypto.key import store_hash from .. import repository as repository_module @@ -450,22 +451,14 @@ def test_delete_marks_the_chunk_index_invalid(repo_fixtures, request, update_ind Without stored fragments, the full index write deletes none and leaves the marker; delete() deletes it. """ - with get_repository_from_fixture(repo_fixtures, request) as repository: - if not stored: - delete_chunkindex_from_repo(repository) # repository creation stored an empty index - repository._pack_writer.max_count = 2 # H(0) and H(1) share a pack - repository.put(H(0), fchunk(b"aaa", chunk_id=H(0))) - repository.put(H(1), fchunk(b"bbb", chunk_id=H(1))) - repository.flush() + repository = get_repository_from_fixture(repo_fixtures, request) + build_one_pack(repository, [(H(0), fchunk(b"aaa", chunk_id=H(0))), (H(1), fchunk(b"bbb", chunk_id=H(1)))]) + with reopen(repository) as repository: if not stored: - assert not list(repository.store_list("index")) - repository.delete(H(1), validate=None, update_index=update_index) - assert chunkindex_is_invalid(repository) is not update_index - if stored: - with reopen(repository) as repository: - assert list(repository.store_list("index")) - repository.delete(H(1), validate=None, update_index=update_index) - assert chunkindex_is_invalid(repository) is not update_index + delete_chunkindex_from_repo(repository) + assert bool(list(repository.store_list("index"))) is stored + repository.delete(H(1), validate=None, update_index=update_index) + assert chunkindex_is_invalid(repository) is not update_index with reopen(repository) as repository: assert pdchunk(repository.get(H(0))) == b"aaa" with pytest.raises(Repository.ObjectNotFound): @@ -481,6 +474,30 @@ def test_delete_missing_object_leaves_the_chunk_index_valid(repo_fixtures, reque assert not chunkindex_is_invalid(repository) +@pytest.mark.parametrize("fail", ["overlap", "past-end", "no-delete"]) +def test_delete_refused_leaves_the_chunk_index_valid(repo_fixtures, request, monkeypatch, fail): + """A delete() that compact_pack refuses before changing the store leaves no marker and the fragments.""" + repository = get_repository_from_fixture(repo_fixtures, request) + build_one_pack(repository, [(H(0), fchunk(b"aaa", chunk_id=H(0))), (H(1), fchunk(b"bbb", chunk_id=H(1)))]) + if fail == "no-delete": + monkeypatch.setenv("BORG_REPO_PERMISSIONS", "no-delete") + with reopen(repository) as repository: + pack_key = "packs/" + bin_to_hex(repository.chunks[H(0)].pack_id) + fragments = sorted(info.name for info in repository.store_list("index")) + packs = sorted(info.name for info in repository.store_list("packs")) + if fail == "overlap": # check_pack_objects: H(1) overlaps H(0) + entry = repository.chunks[H(1)] + repository.chunks[H(1)] = entry._replace(obj_offset=repository.chunks[H(0)].obj_offset) + elif fail == "past-end": # check_pack_objects: H(1) ends past the truncated pack + repository.store.store(pack_key, repository.store_load(pack_key)[:-1]) + expected = StorePermissionDenied if fail == "no-delete" else IntegrityError + with pytest.raises(expected): + repository.delete(H(0), validate=None) + assert not chunkindex_is_invalid(repository) + assert sorted(info.name for info in repository.store_list("index")) == fragments + assert sorted(info.name for info in repository.store_list("packs")) == packs + + def test_delete_stopped_in_the_pack_rewrite_leaves_the_chunk_index_invalid(repo_fixtures, request, monkeypatch): """A delete() that stops after deleting the old pack leaves the marker; the next use rebuilds the index.""" with get_repository_from_fixture(repo_fixtures, request) as repository: From 4d9af7691f889866a6a094aa1df8cc6bfdd866f6 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Thu, 17 Sep 2026 20:18:32 +0530 Subject: [PATCH 6/6] check: review round 6, refs #10364 delete() checks permissions before any store change; compact_pack's hook is renamed before_old_pack_delete and runs before index entries are dropped. Tighten marker docs. --- docs/internals/data-structures.rst | 8 ++++---- docs/internals/packs.rst | 16 +++++++-------- src/borg/cache.py | 9 +++++---- src/borg/repository.py | 28 ++++++++++++++++----------- src/borg/testsuite/repository_test.py | 7 +++---- 5 files changed, 37 insertions(+), 31 deletions(-) diff --git a/docs/internals/data-structures.rst b/docs/internals/data-structures.rst index 5bb863dfb1..cb4bd9603a 100644 --- a/docs/internals/data-structures.rst +++ b/docs/internals/data-structures.rst @@ -91,10 +91,10 @@ cache/ chunkindex-invalid 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 deleting a single object - (which rewrites its pack and deletes the old one), and before ``borg check --repair`` - rebuilds the index after changing the packs. It is removed after the last fragment - is deleted or once a matching index is stored. + 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. 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 6de318555f..3269948a71 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -322,14 +322,14 @@ and deletes all the fragments it supersedes. A deletion that could drop entries -- dropping the index entirely, or the full rewrite above -- is guarded by a marker object, ``cache/chunkindex-invalid``, written before -the first deletion and removed after the last one. The marker also guards deleting a -single object, which rewrites its pack and deletes the old one, and ``borg check ---repair`` rebuilding the index after it changed the packs; there it is removed 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. +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. 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/cache.py b/src/borg/cache.py index a10209fe1b..e038e607c8 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -621,9 +621,10 @@ def chunkindex_is_invalid(repository): def write_chunkindex_invalid(repository): """Store the invalid marker, cache/chunkindex-invalid. - Store it before a store change that leaves the index/ fragments missing entries or pointing at deleted - packs. While it is present, build_chunkindex_from_repo rebuilds the index from the packs instead of merging - the fragments. + 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. + While it is present, build_chunkindex_from_repo rebuilds the index from the packs instead of merging the + fragments. """ repository.store_store(f"cache/{CHUNKINDEX_INVALID_SENTINEL}", b"") @@ -631,7 +632,7 @@ def write_chunkindex_invalid(repository): def delete_chunkindex_invalid(repository): """Delete the invalid marker, if present. - The index/ fragments, if any, must list every chunk in the packs and point only at existing packs. + The index/ fragments, if any, must hold the complete current index and point only at existing packs. """ try: repository.store_delete(f"cache/{CHUNKINDEX_INVALID_SENTINEL}") diff --git a/src/borg/repository.py b/src/borg/repository.py index c24d4d127f..eab4c0c037 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -1685,7 +1685,10 @@ def delete(self, id, *, validate, update_index=True): The rewrite deletes the old pack, so the index/ fragments point the pack's other objects at a deleted pack until the index is stored again. The invalid marker (see write_chunkindex_invalid) is - written via compact_pack's before_change, just before the old pack is deleted. + written via compact_pack's before_old_pack_delete, just before the old pack is deleted. + + Raises PermissionDenied before any store change unless the repo permissions grant write and delete + on packs/ and index/ (see assert_writable). 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. @@ -1694,6 +1697,7 @@ def delete(self, id, *, validate, update_index=True): from .cache import write_chunkindex_to_repo, write_chunkindex_invalid, delete_chunkindex_invalid self._lock_refresh() + self.assert_writable() entry = self.chunks.get(id) if entry is None: raise self.ObjectNotFound(id, str(self._location)) @@ -1706,7 +1710,7 @@ def delete(self, id, *, validate, update_index=True): keep_ids=keep_ids, drop_ids={id}, validate=validate, - before_change=lambda: write_chunkindex_invalid(self), + before_old_pack_delete=lambda: write_chunkindex_invalid(self), ) if update_index: # close() only persists new entries incrementally, so write the full index here to record @@ -1714,7 +1718,9 @@ def delete(self, id, *, validate, update_index=True): write_chunkindex_to_repo(self, self.chunks, incremental=False, force_write=True, delete_other=True) delete_chunkindex_invalid(self) - def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, validate, chunks=None, before_change=None): + def compact_pack( + self, pack_id, *, keep_ids: set, drop_ids: set, validate, chunks=None, before_old_pack_delete=None + ): """Rewrite pack , keeping and dropping , then delete the old pack. keep_ids: chunk ids in this pack to copy into the new pack. @@ -1722,8 +1728,8 @@ def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, validate, chunk validate: passed to superseded_gap_ranges, whose ranges are dropped. chunks: the ChunkIndex to look up the objects' pack locations in and to apply the index updates to. Must be the index keep_ids and drop_ids were derived from. Default: self.chunks. - before_change: callable without arguments, called once just before the old pack is deleted. Not - called when the pack is kept. + before_old_pack_delete: callable without arguments, called once just before the old pack is deleted. + Not called when no bytes are dropped, since the old pack then stays. Together, keep_ids and drop_ids must cover every object the chunk index lists for this pack; an unlisted indexed object would keep its bytes in the new pack but its index entry would go @@ -1739,8 +1745,7 @@ def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, validate, chunk unchanged pack_id if nothing was dropped; dropped_bytes is the on-disk bytes this rewrite freed (unused indexed objects plus superseded duplicates), for --stats accounting. - Updates the in-memory chunk index only; the caller holds the exclusive lock and writes the - index back to the store afterwards. + Updates the in-memory chunk index only; requires the exclusive lock. """ self._lock_refresh() if chunks is None: @@ -1798,13 +1803,14 @@ def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, validate, chunk else: new_pack_id = None # every byte was dropped: no replacement pack + # the new pack is not in the index/ fragments yet, so they still match the store; deleting the old + # pack makes them point at a deleted pack. + if before_old_pack_delete is not None and new_pack_id != pack_id: + before_old_pack_delete() + for drop_id in drop_ids: # remove dropped objects from the index del chunks[drop_id] - # the new pack is not in the index/ fragments yet, so they still match the store; deleting the old - # pack makes them point at a deleted pack. - if before_change is not None and new_pack_id != pack_id: - before_change() if new_pack_id is None: # nothing kept: drop the pack, no replacement self.store_delete(pack_key) return None, dropped_bytes diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index e561725342..c075a362c7 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -8,7 +8,6 @@ import pytest from borghash import HashTableNT -from borgstore.backends.errors import PermissionDenied as StorePermissionDenied from ..crypto.key import store_hash from .. import repository as repository_module @@ -447,7 +446,7 @@ def test_delete_with_stale_earlier_object_in_pack(repo_fixtures, request): @pytest.mark.parametrize("stored", [True, False], ids=["stored-index", "no-stored-index"]) @pytest.mark.parametrize("update_index", [True, False]) def test_delete_marks_the_chunk_index_invalid(repo_fixtures, request, update_index, stored): - """delete() marks the chunk index invalid before it rewrites the pack; update_index=True clears the marker. + """delete() marks the chunk index invalid before it deletes the old pack; update_index=True clears the marker. Without stored fragments, the full index write deletes none and leaves the marker; delete() deletes it. """ @@ -476,7 +475,7 @@ def test_delete_missing_object_leaves_the_chunk_index_valid(repo_fixtures, reque @pytest.mark.parametrize("fail", ["overlap", "past-end", "no-delete"]) def test_delete_refused_leaves_the_chunk_index_valid(repo_fixtures, request, monkeypatch, fail): - """A delete() that compact_pack refuses before changing the store leaves no marker and the fragments.""" + """A delete() refused before changing the store leaves no marker and the fragments and packs unchanged.""" repository = get_repository_from_fixture(repo_fixtures, request) build_one_pack(repository, [(H(0), fchunk(b"aaa", chunk_id=H(0))), (H(1), fchunk(b"bbb", chunk_id=H(1)))]) if fail == "no-delete": @@ -490,7 +489,7 @@ def test_delete_refused_leaves_the_chunk_index_valid(repo_fixtures, request, mon repository.chunks[H(1)] = entry._replace(obj_offset=repository.chunks[H(0)].obj_offset) elif fail == "past-end": # check_pack_objects: H(1) ends past the truncated pack repository.store.store(pack_key, repository.store_load(pack_key)[:-1]) - expected = StorePermissionDenied if fail == "no-delete" else IntegrityError + expected = Repository.PermissionDenied if fail == "no-delete" else IntegrityError with pytest.raises(expected): repository.delete(H(0), validate=None) assert not chunkindex_is_invalid(repository)