diff --git a/src/borg/archiver/compact_cmd.py b/src/borg/archiver/compact_cmd.py index 8689c3894e..4c8359f0cf 100644 --- a/src/borg/archiver/compact_cmd.py +++ b/src/borg/archiver/compact_cmd.py @@ -1,6 +1,8 @@ from collections import defaultdict from pathlib import Path +from borgstore.store import ObjectNotFound as StoreObjectNotFound + from ._common import with_repository from ..archive import Archive from ..cache import write_chunkindex_to_repo, build_chunkindex_from_repo, delete_chunkindex_from_repo @@ -53,10 +55,13 @@ def garbage_collect(self): def get_repository_chunks(self) -> ChunkIndex: """return a chunks index""" - # The cached index already has each object's obj_size and starts entries as F_NONE, so it - # serves both GC and --stats; no need to force the slow pack-header scan just to get sizes. + # Entries must start as unused (F_NONE); analyze_archives() marks the used ones afterwards. + # The cached index loads entries with F_NONE flags and each object's obj_size (used by --stats). + # init_flags applies when there is no cached index and the entries are read from the pack headers. logger.info("Getting object IDs from the cached chunks index...") - chunks = build_chunkindex_from_repo(self.repository, cache_immediately=not self.dry_run) + chunks = build_chunkindex_from_repo( + self.repository, cache_immediately=not self.dry_run, init_flags=ChunkIndex.F_NONE + ) return chunks def save_chunk_index(self): @@ -258,13 +263,19 @@ def compact_packs(self): ) progress = 0 for pid in drop_packs: - self.repository.store_delete("packs/" + bin_to_hex(pid)) + try: + self.repository.store_delete("packs/" + bin_to_hex(pid)) + except StoreObjectNotFound: + # happens when a stale chunk index references an already deleted pack (#9850) + logger.warning(f"Pack {bin_to_hex(pid)} to delete was already gone.") progress += 1 pi.show(progress) # report after the work, so the final pack lands on 100% for id in forget: del self.chunks[id] # their pack file is gone, so drop their index entries too for pid in rewrite_packs: - self.repository.compact_pack(pid, keep_ids=keep[pid], drop_ids=drop[pid]) # helper owns index update + # chunks=self.chunks: the index updates (repoint kept objects, remove dropped ones) + # must land in the index that save_chunk_index() persists (#9850). + self.repository.compact_pack(pid, keep_ids=keep[pid], drop_ids=drop[pid], chunks=self.chunks) progress += 1 pi.show(progress) pi.finish() diff --git a/src/borg/conftest.py b/src/borg/conftest.py index 80cd87cb28..c76cfcf626 100644 --- a/src/borg/conftest.py +++ b/src/borg/conftest.py @@ -89,6 +89,8 @@ def clean_env(tmpdir_factory, monkeypatch): # Speed up tests monkeypatch.setenv("BORG_TESTONLY_WEAKEN_KDF", "1") monkeypatch.setenv("BORG_STORE_DATA_LEVELS", "0") # flat storage for few objects + # tiny packs, so small test data still produces multiple multi-object packs + monkeypatch.setenv("BORG_PACK_MAX_COUNT", "3") yield shutil.rmtree(str(base_dir), ignore_errors=True) # clean up diff --git a/src/borg/constants.py b/src/borg/constants.py index 5b411e183e..ce58aaade7 100644 --- a/src/borg/constants.py +++ b/src/borg/constants.py @@ -59,6 +59,9 @@ # an unresolved pack location is tracked by the ChunkIndex.F_PENDING flag. UNKNOWN_BYTES32 = b"\xff" * 32 +# default pack size limit [bytes], see PackWriter in the repository module +DEFAULT_PACK_MAX_SIZE = 50 * 1000 * 1000 + # MAX_OBJECT_SIZE = MAX_DATA_SIZE + len(PUT header) MAX_OBJECT_SIZE = MAX_DATA_SIZE + 41 # see assertion at end of repository module diff --git a/src/borg/repository.py b/src/borg/repository.py index 202455535e..d3177b06a7 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -119,7 +119,7 @@ class PackWriter: at least one must be set, otherwise the pack buffer is unbounded. """ - def __init__(self, store, *, max_count=3, max_size=None, chunks=None, repository=None): + def __init__(self, store, *, max_count=None, max_size=None, chunks=None, repository=None): if repository is None and chunks is None: raise ValueError("PackWriter requires either a repository or an explicit chunks index") if max_count is None and max_size is None: @@ -515,13 +515,15 @@ def open(self, *, exclusive, lock_wait=None, lock=True): self.lock = Lock(self.store, exclusive, timeout=lock_wait).acquire() self._chunks = None # pack-sizing overrides: BORG_PACK_MAX_COUNT sets the max object count per pack, - # BORG_PACK_MAX_SIZE the max pack size in bytes. - pw_kwargs = {} - if (v := os.environ.get("BORG_PACK_MAX_COUNT")) is not None: - pw_kwargs["max_count"] = int(v) - if (v := os.environ.get("BORG_PACK_MAX_SIZE")) is not None: - pw_kwargs["max_size"] = int(v) - self._pack_writer = PackWriter(self.store, repository=self, **pw_kwargs) + # BORG_PACK_MAX_SIZE the max pack size in bytes. Default: size-bound only. + max_count_env = os.environ.get("BORG_PACK_MAX_COUNT") + max_size_env = os.environ.get("BORG_PACK_MAX_SIZE") + max_count = int(max_count_env) if max_count_env is not None else None + if max_size_env is not None: + max_size = int(max_size_env) + else: + max_size = None if max_count is not None else DEFAULT_PACK_MAX_SIZE + self._pack_writer = PackWriter(self.store, repository=self, max_count=max_count, max_size=max_size) self.opened = True @property @@ -835,13 +837,15 @@ def delete(self, id, *, update_index=True): write_chunkindex_to_repo(self, self.chunks, incremental=False, force_write=True, delete_other=True) - def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, complete: bool = True): + def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, complete: bool = True, chunks=None): """Rewrite pack , keeping and dropping , then delete the old pack. keep_ids: chunk ids in this pack to copy into the new pack. drop_ids: chunk ids in this pack to discard. Must not overlap keep_ids. complete: if True, keep_ids and drop_ids must together be every object in the pack (asserted). If False, they may name only some of the pack's objects. + 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. Kept objects are copied into a new pack via store.defrag and repointed in the chunk index; dropped objects' chunk index entries are removed. @@ -853,6 +857,8 @@ def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, complete: bool index back to the store afterwards. """ self._lock_refresh() + if chunks is None: + chunks = self.chunks pack_key = "packs/" + bin_to_hex(pack_id) assert keep_ids & drop_ids == set(), "an id cannot appear in both keep_ids and drop_ids" @@ -861,7 +867,7 @@ def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, complete: bool located = [] # (obj_offset, obj_id, obj_size, keep) for obj_id in keep_ids | drop_ids: keep = obj_id in keep_ids - entry = self.chunks[obj_id] + entry = chunks[obj_id] assert entry.pack_id == pack_id, f"{bin_to_hex(obj_id)} is not in pack {bin_to_hex(pack_id)}" located.append((entry.obj_offset, obj_id, entry.obj_size, keep)) located.sort() @@ -883,7 +889,7 @@ def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, complete: bool ), f"pack {bin_to_hex(pack_id)}: {pack_size - covered} trailing bytes unaccounted for" for drop_id in drop_ids: # remove dropped objects from the index - del self.chunks[drop_id] + del chunks[drop_id] if not kept: # nothing kept: drop the pack, no replacement self.store_delete(pack_key) @@ -899,7 +905,7 @@ def compact_pack(self, pack_id, *, keep_ids: set, drop_ids: set, complete: bool for _, keep_id, size in kept: new_locations.append((keep_id, new_pack_id, offset, size)) offset += size - self.chunks.update_pack_info(new_locations) + chunks.update_pack_info(new_locations) # delete the old pack last, after the new one is stored and indexed, so kept bytes are never the # only copy. if every object was kept in order, defrag reproduced the pack (new_pack_id == pack_id) diff --git a/src/borg/testsuite/archiver/compact_cmd_test.py b/src/borg/testsuite/archiver/compact_cmd_test.py index b36fbb7ceb..f6092627cb 100644 --- a/src/borg/testsuite/archiver/compact_cmd_test.py +++ b/src/borg/testsuite/archiver/compact_cmd_test.py @@ -8,6 +8,7 @@ from ...hashindex import ChunkIndex from ...repository import Repository from ...cache import files_cache_name, discover_files_cache_names, list_chunkindex_hashes +from ...cache import delete_chunkindex_from_repo from ...manifest import Manifest from . import cmd, create_regular_file, create_src_archive, generate_archiver_tests, open_repository, RK_ENCRYPTION from . import changedir @@ -188,6 +189,72 @@ def test_compact_packs_respects_threshold(tmp_path): assert bin_to_hex(frugal_pack) in pack_names +def test_compact_gc_after_index_loss(archivers, request): + """When no cached chunk index exists (e.g. after an interrupted compact, #9748), compact + rebuilds the index from the packs. The rebuilt entries must start unused (init_flags=F_NONE), + otherwise every object looks used and this compact run frees nothing.""" + archiver = request.getfixturevalue(archivers) + + cmd(archiver, "repo-create", RK_ENCRYPTION) + create_src_archive(archiver, "archive") + cmd(archiver, "delete", "-a", "archive") + + # drop all cached chunk indexes, like an interrupted compact leaves the repo + repository = open_repository(archiver) + with repository: + delete_chunkindex_from_repo(repository) + + output = cmd(archiver, "compact", "-v", "--stats", exit_code=0) + assert "Repository size is 0 B in 0 objects." in output + + +def test_compact_pack_rewrite_updates_persisted_index(archivers, request, monkeypatch): + """Regression test for issue #9850. + + When compact rewrites a mixed pack (still-used objects are copied into a new pack, the old + pack is deleted), the persisted chunk index must point at the new pack. A stale index makes + extract fail on the kept chunks and makes the next delete + compact crash with ObjectNotFound + when it tries to delete the already gone old pack. + """ + archiver = request.getfixturevalue(archivers) + # one big pack per create run: all of archive1's chunks land in a single pack, so that pack + # is mixed (used + unused objects) once archive1 is deleted below. + monkeypatch.setenv("BORG_PACK_MAX_COUNT", "1000") + + contents_kept = os.urandom(1024) # unique contents -> own chunk + contents_dropped = os.urandom(1024) + create_regular_file(archiver.input_path, "file_kept", contents=contents_kept) + create_regular_file(archiver.input_path, "file_dropped", contents=contents_dropped) + + cmd(archiver, "repo-create", RK_ENCRYPTION) + cmd(archiver, "create", "archive1", "input") + # archive2 references only file_kept's chunk, which deduplicates against archive1's pack. + os.remove(os.path.join(archiver.input_path, "file_dropped")) + cmd(archiver, "create", "archive2", "input") + + cmd(archiver, "delete", "-a", "archive1") + # threshold 0: every pack with any unused bytes is rewritten + cmd(archiver, "compact", "-v", "--threshold", "0", exit_code=0) + + # the persisted chunk index may only reference packs that still exist + repository = open_repository(archiver) + with repository: + pack_names = {info.name for info in repository.store_list("packs")} + for id, entry in repository.chunks.iteritems(): + assert bin_to_hex(entry.pack_id) in pack_names, f"chunk {bin_to_hex(id)} points at a deleted pack" + + # the kept chunk moved into a new pack; extracting must read it from there + with changedir("output"): + cmd(archiver, "extract", "archive2") + with open(os.path.join("input", "file_kept"), "rb") as fd: + assert fd.read() == contents_kept + + # 9850: delete the remaining archive and compact again - must not crash with ObjectNotFound + cmd(archiver, "delete", "-a", "archive2") + output = cmd(archiver, "compact", "-v", exit_code=0) + assert "Finished compaction" in output + + def test_compact_dry_run_reports_and_changes_nothing(archivers, request): # --dry-run prints the would-free estimate (issue #9379) and must not touch the repository. archiver = request.getfixturevalue(archivers)