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

[mr-raj12]

Description

A superseded duplicate is an unindexed object in a pack whose chunk id the index maps to another location (see superseded_gap_ranges). compact_packs only counted indexed-but-unused bytes, so a pack holding used objects and superseded duplicates, or only superseded duplicates, was never rewritten or deleted. This is the state after a crashed borg create is re-run.

• Pass 1 walks the gaps of every pack that has unindexed bytes and is not recorded corrupt, and counts its superseded duplicates. A read is 64 kiB after a small object, so small objects share a store request, and 1 kiB after a larger one. The walk refreshes the lock per pack, shows progress and stops on Ctrl-C.
• A rewrite drops the superseded duplicates pass 1 found (compact_pack(superseded_ranges=...)), so the gaps are walked once and the logged count is what is dropped.
• A pack's reclaimable bytes are its unused indexed bytes plus its superseded duplicates: they decide the --threshold rewrite and the all-packs gate. A pack holding only unused indexed objects and superseded duplicates is deleted.
• Pass 1 checks the index entries of every pack. A pack in which they overlap or reach past the end of the file is logged as an error and left unchanged. A gap object whose indexed copy is in such a pack is kept.
• borg repo-compress uses the same check for its untrusted packs, in place of the test for a pack ending before one of its index entries.
• A tiny pack that the all-packs gate keeps from being deleted or rewritten for its superseded duplicates is a merge candidate.
• The unindexed-bytes hint no longer counts superseded duplicates.

Tested with the repro from the issue without the borg check --repair step, which leaves the same superseded duplicates (800 MB random file, kill -9 after 3s, re-run kept): packs go from 1.1G to 764M in one borg compact, the next run logs no hint, borg check is rc 0.

Depends on #10475, whose commits this branch contains.

fixes #10471

Checklist

• PR is against master (or maintenance branch if only applicable there)
• New code has tests and docs where appropriate
• Tests pass (run tox or the relevant test subset)
• Commit messages are clean and reference related issues

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.91%. Comparing base (f8e5ca1) to head (86efd29).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10476      +/-   ##
==========================================
+ Coverage   88.80%   88.91%   +0.11%     
==========================================
  Files         103      103              
  Lines       19515    19589      +74     
  Branches     3050     3061      +11     
==========================================
+ Hits        17330    17418      +88     
+ Misses       1516     1505      -11     
+ Partials      669      666       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[ThomasWaldmann]

Reviewed at 399dea2. The approach looks right: counting the gaps in pass 1 lets both --threshold and the drop decision see them, and merge_packs still copies whole files, so gaps survive a merge. The PR's tests pass locally. Two problems reproduced with tests, plus some smaller points.

1. Data loss when the indexed copy is in a truncated pack (blocking)

untrusted_pack_ids is stale_packs | corrupt_packs. A pack whose index entries claim more bytes than the file holds is logged as an error and skipped in the fate loop, but is not untrusted. So a gap copy whose indexed copy lives in that pack still counts as a superseded duplicate.

Repro: pack A holds only copies of X and Y. Pack B indexes X and Y but is truncated to 100 bytes in the store. compact logs index claims more data than the file holds, run "borg check", then Deleting 0 unused objects and 2 superseded duplicates, and deletes pack A, which held the only intact copies of X and Y.

def test_compact_keeps_duplicates_indexed_in_truncated_pack(tmp_path):
    location = os.fspath(tmp_path / "repo")
    with Repository(location, exclusive=True, create=True, key_loader=make_test_key) as repository:
        manifest = gc_manifest(repository)
        repo_objs = manifest.repo_objs
        datas = [os.urandom(1000), os.urandom(1000)]
        _, pack_a = put_pack(repository, repo_objs, datas)
        ids, pack_b = put_pack(repository, repo_objs, [b"new"] + datas)
        key_b = "packs/" + bin_to_hex(pack_b)
        repository.store.store(key_b, repository.store.load(key_b)[:100])  # X and Y are gone from B
        mark_used(repository, set(ids))
        gc = ArchiveGarbageCollector(repository, manifest, stats=False, threshold=10)
        gc.chunks = repository.chunks
        gc.compact_packs()
        assert pack_a in pack_sizes(repository)  # fails: pack A was dropped

compact_pack already had this gap before, but there it only mattered when a pack was rewritten for its unused indexed bytes. Now a pack holding nothing but such copies is deleted outright. Suggested fix: determine the packs failing the indexed > total check (and those failing check_pack_objects) before the gap walk, and add them to untrusted_pack_ids for both pass 1 and compact_pack, the same reasoning as #10474.

2. Orphan packs are walked on every run

A pack without any index entries (e.g. left by a crashed borg create) is in gap_packs on every run, and the gap walk does one store range read per object. Measured: 500 range reads per run for a 500-object orphan pack; master does none, as it never looks at such a pack. On ssh/REST/S3 stores that is one request per object, on every compact (including --dry-run), until someone runs borg check --repair.

Skipping such packs is not an option, as the #10471 case is exactly an unindexed pack holding only superseded duplicates. But the walk could read a gap pack in one piece (bounded by the pack size limit), so it costs one request per pack instead of one per object.

3. Smaller points

• superseded_duplicates() iterates the whole chunk index even when gap_packs is empty, the normal case. Return early on an empty set.
• An overlap detected by check_pack_objects in any gap pack now raises IntegrityError and aborts the whole compact run. Before, that only happened for a pack being rewritten. The neighbouring case indexed > total is handled per pack with an error log, so this is inconsistent.
• The superseded count logged for rewritten packs comes from pass 1. When a dropped pack held the indexed copy, its index entry is deleted before the rewrite, so compact_pack keeps that gap copy (safe) and the log line overstates the count.
• The nothing-to-do path still logs Deleting 0 unused objects..., not matching the new message.
• A tiny pack whose superseded duplicates reach --threshold is now a rewrite candidate instead of a merge candidate. If the all-packs gate then cancels the rewrites, that pack is neither rewritten nor merged.

[ThomasWaldmann]

please rebase

[mr-raj12]

Rebased on #10475 (now with 4eca5c3) and addressed in 86efd29 (the commits of this PR are squashed into it).

1. Truncated pack

Pass 1 now checks the index entries of every pack with check_pack_objects, not only the summed bytes. A pack in which they overlap or reach past the end of the file is logged as an error, left unchanged and is in untrusted_pack_ids before the first gap is walked. This replaces the indexed > total check. Your test is added (using store_store / store_load). test_compact_keeps_duplicates_indexed_in_inconsistent_pack covers an overlap in a pack with a gap, an overlap in a pack without one, and entries summing to more than the file size.

For that check, pass 1 keeps the byte range of each index entry as one int in an array("Q") per pack, 8 bytes per entry. Sorting and checking takes 0.2 s for 1M entries and 2 s for 10M (synthetic, 500 entries per pack, on my machine).

borg repo-compress uses the same check for its untrusted set, in place of the short_packs test from #10475, so both commands also cover overlapping entries. It still stops with the IntegrityError at such a pack.

2. Orphan packs

superseded_gap_ranges reads GAP_READ_SIZE (64 kiB) at the start of a gap and after an object smaller than that, so small objects share a store request and the 500-object pack is read with a few requests. After a larger object it reads META_READ_SIZE (1 kiB), as before, so a pack of large objects is not downloaded. test_compact_gap_walk_store_requests checks both.

A rewrite drops the superseded duplicates pass 1 found (compact_pack(superseded_ranges=...)), so the gaps of a rewritten pack are walked once.

The walk refreshes the repository lock before each pack (PackReader reads from the store, which does not), shows progress and stops on Ctrl-C at a pack boundary, before anything is changed.

3. Smaller points

• No gap packs: superseded_duplicates gets the ranges from pass 1 and does not iterate the index.
• check_pack_objects failures are handled per pack, see 1.
• The logged count is what is dropped, as the rewrite uses the ranges from pass 1. That is also the case when the indexed copy is unused and its pack is dropped or rewritten in the same run: the chunk is unused, so its gap copy is not needed. test_compact_drops_duplicate_of_unused_chunk compares the logged bytes with the freed ones for both.
• The nothing-to-do path logs Deleting 0 unused objects and 0 superseded duplicates, freeing 0 B....
• A tiny pack without unused indexed objects that is a drop or rewrite candidate becomes a merge candidate when the all-packs gate cancels the reclaim.

docs: the crash paragraph in packs.rst still had the TODO for #10471 and said superseded duplicates are only reclaimed with a rewrite for unused objects; updated.

[ThomasWaldmann]

please rebase, so i can merge.

[mr-raj12]

Rebased on top of the rebased #10475, no conflicts. It still shows the two #10475 commits until that one is merged.