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

❌ Patch coverage is 95.74468% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.92%. Comparing base (5b9da3b) to head (8b80d14).
⚠️ Report is 9 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/borg/repository.py	92.85%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #10048   +/-   ##
=======================================
  Coverage   86.92%   86.92%           
=======================================
  Files          99       99           
  Lines       17404    17486   +82     
  Branches     2642     2663   +21     
=======================================
+ Hits        15128    15200   +72     
- Misses       1582     1587    +5     
- Partials      694      699    +5     

☔ 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.

[ThomasWaldmann]

needs another rebase.

[ThomasWaldmann]

And another one...

[ThomasWaldmann]

Re-reviewed at 9a4a3764f. This is a big improvement — both of my blocking findings are genuinely fixed, and I verified each one rather than taking the diff at face value. Thanks for the thorough rework.

Setup note: I rebuilt the C extensions from scratch, since the rebase pulled in .pyx changes and stale .sos would have given misleading results. Tests green — repository_test.py, cache_test.py, check_cmd_test.py, compact_cmd_test.py: 207 passed, 2 skipped.

There is one new blocking bug, and one structural issue that moved rather than went away.

Fixed, verified

Previous finding	Status
No effect on the default check --repair	Fixed. ArchiveChecker.finish persists instead of deleting. Confirmed end to end: corrupt index + intact packs → check --repair ends with one valid fragment and all 43 entries, follow-up check rc=0, extract --dry-run rc=0. That is item 6 of #10026.
Whole-pack drop discarded recoverable data	Fixed, and better than what I suggested. The lossy path is gone entirely; the rebuild only runs when every pack is intact. --repository-only --repair with a corrupt pack leaves index and pack untouched and returns rc=1.
sha256 is not authentication	Fixed as an honesty matter — the docstring now says so plainly, refs #9901, #10026.
only_packs silently ignored on the fast path	Gone — the parameter was removed.
Stale self._chunks	Fixed — invalidate_chunk_index() in both places.
Exit code	Fixed for repo_only; see below for the full-check case.
Docs epilog, progress bar never reaching 100%, duplicated pack loop, missing archiver-level test	All fixed.

I also confirmed the deferral design is sound where it applies: a full check --repair --verify-data against a repo with a corrupt pack identified exactly the one damaged chunk, deleted it, rewrote the pack, and a follow-up --repository-only check then reports 0 pack errors. The 10 salvageable chunks I complained about losing in the previous revision are preserved. That is the right answer.

New blocker: Ctrl-C during pack verification still triggers the rebuild

The guard is if index_errors and pack_errors == 0:, but the loop above it breaks on sig_int. "No errors found" and "no packs checked" are not distinguished. Simulated by forcing sig_int truthy at loop entry, on a repo with a corrupt index and a corrupt pack:

Repository index is corrupted; rebuilding it from the packs.
Interrupted repository check, 0 packs checked so far.
Checked 2 index files (2 errors) and 0 packs (0 errors).
Repository index was corrupted and has been rebuilt from the packs.
Interrupted full repository check, repaired so far.
check(repair=True) -> True            # index fragments now: 1

Zero packs verified, one of them corrupt, and the index is rebuilt from all of them and persisted — while reporting "repaired". This is exactly the failure the new precondition exists to prevent, and it is reachable by pressing Ctrl-C. check_cmd only raises on sig_int after check() returns, so nothing catches it. The guard needs not sig_int, and ideally pack_files == len(pack_infos).

Unresolved: the archives phase undoes the repository phase's caution

The repository phase now refuses to rebuild when any pack is corrupt — but ArchiveChecker.check still calls build_chunkindex_from_repo(slow_rebuild=repair) over all packs, and finish() now persists it. So on a full check --repair, the archives phase writes precisely the index the repository phase declined to write. Same structural shape as the previous revision's issue, mirrored into the other phase.

With a corrupt payload byte: rc=0, "Archive consistency check complete, no problems found", an index persisted containing all 11 entries from the corrupt pack — and a subsequent plain borg check returns 1.

With a corrupt header byte it is worse, because PackReader.iter_headers never validates OBJ_MAGIC and the walk goes off the rails. For an 11-object pack the persisted index got 2 entries, both bogus:

entry d5c39f63ccea920a off 102526 size 2393111698 -> BOGUS: IntegrityError
entry 9da217e92e86958e off      0 size     102526 -> BOGUS: IntegrityError

An index entry claiming a 2.4 GB object, and a chunk id that does not exist — the #8476 poisoning, in the path that still runs. check --repair exited 0, and then:

$ borg compact
Pack 1991e081…: index claims more data than the file holds, run "borg check".

compact telling the user to run the check that just declared the repository repaired.

Two things follow:

1. The deferral's justification does not hold by default. The docstring says the archives phase "can repair a corrupt pack holding metadata, or file content with --verify-data", but the deferral is keyed on repo_only alone. The "file content" half needs --verify-data, which is not the default, so without it the deferral is never honored and nothing notices. It should key on verify_data too, or the archives phase should re-report the packs the repository phase deferred.
2. finish() should mirror the repository phase's guard. When packs are known corrupt, don't persist — falling back to the previous delete_chunkindex_from_repo behavior at least avoids blessing a bogus header walk as the stored index.

Related and cheap: having iter_headers validate OBJ_MAGIC and that offset + obj_size <= pack_size would turn the header-corruption case into a clean "pack unreadable" instead of silent index poisoning. RepoObj.parse and extract_crypted_data both check the magic; the header walk does not. Arguably its own PR — but this is the change that makes that walk load-bearing.

Smaller

• Premature warning: "Repository index is corrupted; rebuilding it from the packs." is logged before the pack loop decides whether a rebuild will happen, so in the corrupt-pack case it is printed and then no rebuild occurs. Move it after the loop, or reword to something like "verifying packs before rebuilding it".
• Double header scan on a full repair: the happy path scans every pack's headers twice (the repository-phase rebuild, then ArchiveChecker.check's slow_rebuild=True) on top of a full sha256 read of every pack — and finish() overwrites the repository phase's fragment anyway. When not repo_only that first rebuild is pure waste; skipping it there, or letting the archives phase reuse the just-persisted index, would save a full pass on large repositories.
• Docs: the new epilog describes the repository phase's caution but not that the archives phase rebuilds and persists from all packs regardless, so it promises more restraint than the command currently has.
• Test gap: the new archiver test covers the happy path (item 6), which is exactly what was missing before. Nothing yet covers a full --repair with a corrupt pack, which is where both remaining issues live.
• store_list("packs") still runs twice; the comment now justifies the correctness side via the exclusive lock, so this is only an extra listing on high-latency stores.

Verdict

The lossy behavior is gone and item 6 landed, which were the two things that made the previous revision unmergeable. What is left is the sig_int guard (small, clearly a bug) and a decision on how much of the repository phase's new caution the archives phase should inherit.

If you would rather keep this PR tight: the sig_int fix plus making finish() not persist when packs are known corrupt would get it to a defensible state, with the iter_headers hardening and the --verify-data deferral logic as follow-ups.

[mr-raj12]

Once #10083 gets merged, I will rebase over it and wire the repair rebuild to use the resync path

[ThomasWaldmann]

needs another rebase.

[ThomasWaldmann]

Re-reviewed after the rebase, at 8b80d1481.

The PR's own patch is byte-identical to the previous head — I diffed both against their respective merge bases and the added/removed lines match exactly — so this is a pure rebase and my last review still applies. Rebuilt the extensions and re-ran the suites: 222 passed, 2 skipped.

The rebase was not onto neutral ground, though: it pulled in #10069 (vanished-pack detection, #9898), which reworks check() right around these hunks. That interaction produced one regression.

The interaction is mostly clean

Re-verified the core paths on the new base:

• corrupt index only, full --repair → index rebuilt and persisted, rc=0, follow-up check clean
• corrupt pack, --repository-only --repair → rc=1, index and pack untouched
• the sig_int guard → holds, returns False, index untouched

The new cross-check also composes correctly with the repair path: it sets self.chunks from the fragment-loaded index, and invalidate_chunk_index() after the rebuild runs later, so there is no stale write-back. And when the fragments are corrupt — which is precisely this feature's precondition — the cross-check bows out cleanly instead of fighting it:

Cannot cross-check packs against the chunk index: the index could not be loaded
from its fragments; skipping missing-pack detection.
Repository index was corrupted and has been rebuilt from the packs.
Finished full repository check, repaired.

Regression: missing packs are now reported as a corrupt index

#10069 added a fourth repair-mode failure mode (missing_pack_ids), and it falls straight into the else branch this PR added for "index corrupt but not rebuilt". On a repository with one pack deleted and a perfectly intact index:

$ borg check --repository-only --repair          # rc=0
1 pack(s) referenced by the index are missing:
Missing pack: ab3b2123b0db6ab4…
The chunks stored in these packs are lost. Repairing the index … issues/8572.
Finished full repository check, index still corrupt.

The index is not corrupt — the last line contradicts the three above it. Master at e0f5068a0 gets this right:

Finished full repository check, errors found (repository repair not implemented).

The fix is small: either handle missing_pack_ids explicitly in the chain, or tighten the last branch to elif index_errors and not index_repaired: and keep a generic fallback for anything else.

Adjacent gap in the same few lines

objs_errors now includes len(missing_pack_ids), but the repair return is return not (repo_only and (pack_errors or corrupt_ids)), so missing packs never reach it and --repository-only --repair exits 0 on confirmed data loss.

To be fair: rc=0 happens on master too (return not problems or repair), so this is not a regression introduced here. But this PR already narrows that return by adding index_errors and not index_repaired -> False, and a missing pack is strictly more severe than the corrupt-pack case it does handle — so it seems natural to fold in while these lines are being touched.

Nit

The base now imports build_chunkindex_from_repo at the top of check(), and the rebuild block re-imports the same name a bit further down. Harmless shadowing, but a leftover the rebase can drop.

Unrelated to this PR, but worth knowing

The iter_headers validation from #10083 raises IntegrityError, and nothing catches it, so any command falling back to a slow index rebuild over a header-corrupt pack dies with a traceback. On current master (e0f5068a0, this PR not applied): borg check --repair → rc=90, borg repo-list → rc=90.

Not caused by this PR, and it deserves its own issue — but it does mean the repair path added here is unreachable in the header-corruption case, since check --repair aborts before it gets there.

Verdict

Unchanged from my last review: this is mergeable. The one thing I would like fixed first is the "index still corrupt" misdiagnosis, since the rebase introduced it and it points the user at the wrong problem. The return-value gap and the duplicate import are cheap to fold into the same commit.

Still missing: a regression test for the sig_int guard. Monkeypatching borg.repository.sig_int with a truthy object is enough to drive it, which is how I verified the fix.