check --repair: rebuild a corrupt repository index from the packs, #10026

[mr-raj12]

A corrupt chunks index currently leaves a borg2 repo stuck: Repository.check(repair=True) just logged "repository repair not implemented" and stopped. This implements the repository-level index repair from #10026.

A read-only check that finds the index corrupt still stops and reports it. With --repair:

• If every pack is intact, the chunks index is rebuilt from the packs' object headers and persisted. Packs are named and verified by the sha256 of their content, so a rebuild detects accidental corruption; sha256 is content-addressing rather than a MAC, so it does not detect tampering (borg2: index rebuild trusts pack headers #9901).
• If any pack is corrupt, the index and the packs are left unchanged and the corruption is reported. Salvaging a corrupt pack's still-intact objects needs the key and is not implemented yet (to pack or not to pack ... #8572).

On a full check the archives phase runs after the repository phase, so ArchiveChecker.finish() now persists the chunks index instead of deleting it: it rebuilds from the packs when repair changed them, else writes out the index it already holds, then drops the in-memory copy so close() does not overwrite it. Previously finish() deleted the on-disk index, forcing a slow rebuild on the next repository access.

check() gains a repo_only argument: a corrupt pack fails a repository-only repair, but a full check defers the verdict to the archives phase, which can repair a corrupt pack holding archive/item metadata (or file content with --verify-data).

The slow rebuild path shows a progress indicator.

Left for follow-up: salvaging still-intact objects out of corrupt packs and consuming the persisted corrupt-pack list (#8572, needs #9925).

Refs #10026.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.79%. Comparing base (8f2de9e) to head (c03299c).
⚠️ Report is 57 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10048      +/-   ##
==========================================
+ Coverage   86.64%   86.79%   +0.14%     
==========================================
  Files          98       98              
  Lines       17037    17132      +95     
  Branches     2581     2597      +16     
==========================================
+ Hits        14762    14869     +107     
+ Misses       1583     1570      -13     
- Partials      692      693       +1     

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

[ThomasWaldmann]

Thanks for tackling this — the code is clean, well commented, and the tests are readable. I checked it out and ran it end-to-end before reviewing: repository_test.py + cache_test.py (133 passed) and check_cmd_test.py + compact_cmd_test.py (49 passed, 2 skipped), plus a manual repro on a 5-pack repo with one flipped payload bit and all index fragments rotted.

Unfortunately I don't think it can go in as-is. The main issues are about scope and about one safety claim that doesn't hold.

Blocking

1. sha256 is not authentication, so the stated safety property is wrong

The commit message says "A pack whose sha256 no longer matches is skipped, so a corrupted header cannot put a wrong or absent chunk into the index", and the comment at src/borg/repository.py:1055 reads the same way.

packs/<name> is content-addressed, not MAC'd — anybody who can write to the store can forge headers and name the pack by its sha256. So the gate filters accidental corruption only; in the tampering model it buys nothing.

What actually catches a forged header is the AEAD, on read: RepoObj.parse feeds the index's chunk id into key.decrypt(id, …), and AEADKeyBase.decrypt uses aad=aad + id (src/borg/crypto/key.py:1174). But dedup never reads — borg create only asks chunk_id in chunks and skips the put, which is exactly the fatal case described in #8476. So #9901 and item 3 of #10026 ("Headers are unauthenticated … Part of item 2, not a follow-up") are untouched, while the PR text reads as if they were handled.

Either implement item 3, or state plainly in the comment and commit message that the rebuilt index is corruption-checked but still unauthenticated.

2. No effect on the default borg check --repair

The PR notes that ArchiveChecker.finish deletes the index, framed as wasted work. It's more than that: ArchiveChecker.check calls build_chunkindex_from_repo(slow_rebuild=repair, …) (src/borg/archive.py:1897), which bypasses the freshly repaired fragments and re-indexes all packs, including the corrupt one. The repository phase's exclusion is silently reverted seconds later:

Repository index is corrupted; rebuilding it from the packs.
Store object packs/178d525c… is corrupted: content does not match its name (sha256).
Repository index was corrupted and has been rebuilt from the intact packs.
Finished full repository check, 1 corrupt pack(s) could not be repaired …
Starting archive consistency check...
Archive consistency check complete, no problems found.        # <-- and index/ is now empty

The next access re-indexes all 11 chunks of the corrupt pack. Net effect on the default invocation: one extra full read of every pack, plus a log line that is no longer true by the time the command exits. Item 6 of #10026 isn't optional polish here — without it the feature never reaches the default path.

3. --repository-only --repair discards recoverable data, after a single read

This is the path where the new code does take effect, and one flipped bit drops every object in the pack:

$ borg check --repository-only --repair      # rc=0
$ borg check --archives-only
arch1: …/f1: Missing file chunk detected (Byte 0-102400, Chunk 48f8dc83…)
… 11 files, "problems found"

I read those 11 objects back out of the corrupt pack with assert_id forced on: 10 of the 11 decrypt and verify their chunk id perfectly. Only one is actually damaged. With the default 50 MB pack size this strands tens to thousands of intact, cryptographically verifiable chunks per bad byte — and compact won't reclaim the orphaned pack either (fully unindexed → reclaimable == 0, and above tiny_limit it never becomes a merge candidate).

Two of the constraints listed in #10026 are crossed:

• "Never delete an object after a single failed read; a second read must also fail first." — verify() is a single store.hash(), so a transient read glitch permanently drops the pack's entries.
• Item 4 says: for a pack failing Store.hash, keep every object that still AEAD-authenticates in a new pack, then drop the rest. This PR ships "drop the rest" and defers the salvage, which inverts the safe ordering.

Given issue 1, the trade is worse than it first looks: the whole-pack drop buys robustness against a garbage header walk after random corruption, not against an attacker. Validating the walk's self-consistency (monotone, non-overlapping, ending exactly at the file size — check_pack_objects already encodes that shape) would get most of that without discarding recoverable data.

4. Layering: authenticating headers needs the key, Repository.check() doesn't have one

The repository layer sits below crypto, so there is no RepoObj available to authenticate with. Roughly two ways out: drive the index repair from a key-aware layer (which is also where item 4's salvage has to live), or mark rebuilt entries "unverified" so the first real read authenticates them and borg create won't suppress a put on an unverified entry. That decision determines whether per-pack skipping is the right primitive at all, so it is worth settling before this lands.

Medium

• only_packs is silently ignored on the fast path (src/borg/cache.py:813): it is applied only after the if not slow_rebuild: block, so build_chunkindex_from_repo(only_packs=[…]) without slow_rebuild=True returns the full fragment-merged index. Combined with write_immediately=True (which implies delete_other=True), a caller getting that wrong wipes and replaces the index. Please add assert only_packs is None or slow_rebuild.
• The rebuilt index is not installed into self._chunks: check() discards the returned index and calls neither the setter nor invalidate_chunk_index(). Harmless today (nothing loads repository.chunks before check() on that path — get_manifest() doesn't), but if anything ever does, close()'s incremental write would put stale F_NEW entries — including the dropped pack's — back on top of the repaired index.
• Exit code contradicts the message: Finished … 1 corrupt pack(s) could not be repaired is followed by rc=0, because of return objs_errors == 0 or repair. Pre-existing, but this is the first code that actually knows about an unrepaired defect.
• Docs are missing. The check epilog still says repair "removes corrupted objects from the repository after it did a 2nd try to read them correctly" (src/borg/archiver/check_cmd.py:193) — now doubly wrong: no 2nd try, and whole packs' worth of index entries get dropped. Behavior this lossy should be documented in the same PR.
• Test gap: both new tests drive Repository.check() directly, which is why neither notices issue 2. An archiver-level test asserting the post-repair index would have caught it.

Nits

• The new progress bar in build_chunkindex_from_repo never reaches 100%: progress() computes the percentage from the pre-increment counter, so a plain show(increase=1) loop tops out at (n−1)/n — I saw 0/20/40/60/80% for 5 packs. The repair branch in repository.py handles this with the explicit show(current=…); cache.py needs the same.
• The pack-verify loop is duplicated between the two branches; the repair copy also skips tracker.record() and the Finished checking packs. log. Harmless (a full check clears the tracker up front), but worth factoring or commenting.
• store_list("packs") runs twice — once in check(), once inside build_chunkindex_from_repo. Not free on a high-latency store with many packs. It also means correctness leans on the exclusive lock (a pack appearing between the two listings would be dropped from the index); fine today, worth a comment.
• The local from .cache import build_chunkindex_from_repo matches the surrounding circular-import workarounds, so that one is fine.

What is good here

The per-pack sha256 gate is a sound corruption filter and a reasonable building block. only_packs is a clean way to express it. Persisting via write_chunkindex_to_repo(delete_other=True) gets the crash-safety right (invalid-marker guarded), and the failure mode is idempotent — Ctrl-C mid-rebuild leaves the corrupt fragments in place and the next run simply redoes the work. The progress indicator addresses item 7 of #10026.

Suggested way forward

I would split this:

1. This PR: the header-scan rebuild for the case where all packs are intact, plus item 6 (keep the rebuilt index across the archives phase). That is useful, non-lossy, and fixes the actual "a corrupt index leaves the repo stuck" complaint from #10026.
2. A follow-up: corrupt-pack handling together with item 4's salvage and a decision on item 3, so that nothing is dropped before there is a mechanism to keep what is still good.

[ThomasWaldmann]

needs a rebase on current master.

[mr-raj12]

needs a rebase on current master.

Updating

[ThomasWaldmann]

ping?

[mr-raj12]

also, squashed to a single commit and updated the PR description : the earlier only_packs/drop-chunks approach is gone,
now it rebuilds only when every pack is intact, finish() persists the index instead of deleting it, and a full check defers a corrupt pack to the archives phase.