compact: reclaim superseded duplicates left after a full check --repair, fixes #10471

docs/internals/data-structures.rst

@@ -232,9 +232,10 @@ Compaction
 - free space one pack file at a time - a single object can not be removed from
   a pack file, only a whole pack file can be deleted or rewritten:
 
-  - a pack file whose indexed objects are all unused is deleted
-  - a pack file with some unused objects is rewritten without them, but only if
-    the wasted bytes reach the ``--threshold`` percentage
+  - a pack file holding only unused objects and superseded duplicates (copies of
+    chunks the chunks index records at another location) is deleted
+  - a pack file with some unused objects or superseded duplicates is rewritten
+    without them, but only if their bytes reach the ``--threshold`` percentage
   - very small pack files are merged into bigger ones
   - a pack file recorded corrupt by ``borg check`` is not rewritten or merged,
     it is only deleted if its indexed objects are all unused

docs/internals/packs.rst

@@ -280,18 +280,14 @@ stored as index fragment(s).
 A crash between steps 1 and 2 leaves blobs in ``packs/`` that no index entry covers
 (see `Gap bytes`_). No archive references these chunks. As the index does not
 list them, a later backup stores the chunks it needs again, which makes those blobs
-superseded duplicates. ``borg compact`` reclaims unused indexed objects, and the
-superseded duplicates in the packs it rewrites; a pack without index entries is not
-rewritten, so its bytes stay (a tiny one can be merged as a whole).
+superseded duplicates. ``borg compact`` reclaims unused indexed objects and superseded
+duplicates; the other blobs no index entry covers stay.
 
 A full ``borg check --repair`` (``--repository-only`` rebuilds the index only if it
 is corrupt) rebuilds the index from the packs (see :ref:`pack-recovery`) and indexes
 one copy per chunk id, so the blobs whose chunk id had no index entry become indexed,
 and ``borg compact`` reclaims them once unused. The other copies stay superseded
-duplicates and are only reclaimed when ``borg compact`` rewrites their pack to reclaim
-unused indexed objects.
-TODO: count superseded duplicates as reclaimable, so ``borg compact`` also rewrites a
-pack that holds only used objects and superseded duplicates, see :issue:`10471`.
+duplicates, which ``borg compact`` reclaims as well.
 
 A crash between steps 2 and 3 leaves index entries for objects no archive
 references. They point to valid, fully-written pack data, and ``borg compact``
@@ -302,8 +298,9 @@ archive pointer write is the commit point: archives are listed from the
 ``archives/`` namespace, so data not referenced by any archive pointer is
 unreachable, and ``borg compact`` treats its indexed objects as unused.
 
-Pack files are removed by ``borg compact`` (dropping packs whose indexed objects are
-all unused, rewriting packs above ``--threshold`` and merging tiny packs),
+Pack files are removed by ``borg compact`` (dropping packs that hold only unused
+objects and superseded gap blobs, see `Gap bytes`_, rewriting packs above
+``--threshold`` and merging tiny packs),
 ``borg check --repair`` (when it drops a defective object, and when it salvages a
 pack recorded corrupt), ``borg repo-compress`` (``Repository.transform_pack`` stores
 the re-compressed pack under its new content-addressed name and deletes the old one)
@@ -329,13 +326,30 @@ Rewriting a pack (``compact_pack``, ``transform_pack``) drops the superseded gap
 header and metadata slot validate, checked as in the repair walk above
 (``repoobj.object_validator``), and copies all other gap bytes into the new pack.
 Validation covers ``meta_size`` and ``data_size``, so a dropped range is exactly one blob.
-Without a validator (``validate=None``), no gap bytes are dropped. Merging packs
-(``merge_packs``) copies whole pack files, so it keeps all gap bytes.
-
-The walk over a gap steps from header to header by the blob size each header states. It ends
-at a header that does not parse or that reaches past the gap. The rest of that gap is kept,
-and so is a superseded blob that does not validate; both are logged as a warning with the
-pack id and the offset.
+Without a validator (``validate=None``), no gap bytes are dropped. A superseded blob is
+also kept when its indexed copy may be unreadable: ``borg compact`` and ``borg repo-compress``
+keep it when that copy is in a pack that is missing from the store, recorded corrupt, or
+whose index entries overlap or reach past the end of the pack file,
+``borg check --repair --verify-data`` when that copy is in a missing pack or in a pack holding
+a defect chunk. Merging packs (``merge_packs``) copies
+whole pack files, so it keeps all gap bytes.
+
+``borg compact`` walks the gaps of every pack not recorded corrupt and adds the superseded
+gap blobs to the pack's unused indexed bytes. The sum decides whether the pack is rewritten
+(``--threshold``), and a pack holding only unused indexed objects and superseded gap blobs is
+deleted. A rewrite drops the superseded gap blobs that this walk found. ``borg compact``
+checks the index entries of every pack: a pack in which they overlap or reach past the end
+of the pack file is logged as an error and left unchanged; its gaps are not walked, and a
+gap blob whose indexed copy is in such a pack is kept. If the space to reclaim in the whole
+repository is too small for ``borg compact`` to delete or rewrite packs, a tiny pack whose
+indexed objects are all used is merged, superseded gap blobs included.
+
+The walk over a gap steps from header to header by the blob size each header states. A read
+starts at a blob header. It is ``GAP_READ_SIZE`` (64 kiB) at the start of a gap and after a
+blob smaller than that, so small blobs share a store request, and ``META_READ_SIZE`` (1 kiB)
+after a larger blob. The walk ends at a header that does not parse or that reaches past the gap. The rest of
+that gap is kept, and so is a superseded blob that does not validate; both are logged as a
+warning with the pack id and the offset.
 
 
 .. _pack-index-namespace:

src/borg/archive.py

@@ -2503,6 +2503,7 @@ def verify_data(self):
         verified = 0  # chunks actually verified
         defect_chunks = []
         archive_meta_ids = set()
+        untrusted_pack_ids = set()  # ids of the packs that are missing or hold a defect chunk
         pi = ProgressIndicatorPercent(
             total=chunks_count, msg="Verifying data %6.2f%%", step=0.01, msgid="check.verify_data"
         )
@@ -2533,11 +2534,13 @@ def verify_data(self):
                         errors += 1
                         logger.error("chunk %s, integrity error: %s", bin_to_hex(chunk_id), integrity_error)
                         defect_chunks.append(chunk_id)
+                        untrusted_pack_ids.add(pack_id)
             except Repository.PackNotFound:
                 # the pack is gone, thus every chunk the index places in it is lost. get_many() loads the
                 # whole pack for the first chunk, so it raises before any chunk of this pack was read.
                 # one error line for the pack, the chunk ids at debug level: a pack holds thousands of them.
                 self.error_found = True
+                untrusted_pack_ids.add(pack_id)
                 lost = len(chunk_ids)
                 errors += lost
                 verified += lost  # they are not read, but they are accounted for, like a failed read
@@ -2580,13 +2583,18 @@ def verify_data(self):
                         # keeping the other chunks, and removes it from self.chunks, so rebuild_archives
                         # reports the file it belongs to. update_index=False: finish() stores the index
                         # and clears the invalid marker delete() writes.
+                        # untrusted_pack_ids: the rewrite keeps an object of the pack that no index entry
+                        # covers, if the index maps its chunk id into one of these packs.
                         old_pack_id = self.chunks[defect_chunk].pack_id
                         # new_pack_id: the pack holding the other objects of the old pack, None if there were none.
-                        new_pack_id, _ = self.repository.delete(defect_chunk, update_index=False, validate=validate)
+                        new_pack_id, _ = self.repository.delete(
+                            defect_chunk, update_index=False, validate=validate, untrusted_pack_ids=untrusted_pack_ids
+                        )
                         self.chunks_modified = True
                         self.written_packs.discard(old_pack_id)  # delete() removed the old pack
                         if new_pack_id is not None:
                             self.written_packs.add(new_pack_id)
+                            untrusted_pack_ids.add(new_pack_id)  # it may hold other defect chunks of the old pack
                     else:
                         logger.warning("chunk %s not deleted, did not consistently fail.", bin_to_hex(defect_chunk))
                         if meta["type"] == ROBJ_ARCHIVE_META: