size-bound 50 MB packs by default, fix stale chunk index after compact pack rewrites

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()

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
 

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
 

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 <pack_id>, keeping <keep_ids> and dropping <drop_ids>, 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)

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)