index rebuild: abort cleanly on a corrupt object header, #10122

[mr-raj12]

Fixes #10122. Rebased on current master; the two commits below are what is left here.

When the index/ fragments are gone or unreadable, borg rebuilds the chunks index by walking the object headers of every pack. Hit a corrupt header and iter_headers() raises IntegrityError, which nobody catches, so a plain borg repo-list dies with a traceback and rc 90.

build_chunkindex_from_repo() now catches it and raises CorruptPack, a new Error subclass with rc 93 and no traceback:

Data integrity error: pack c65e98ca7af6...: no object header at offset 30186
(pack corruption). Run "borg check --repair" to recover the objects that are still readable.

It aborts instead of skipping the damaged object, because everything after that offset would be missing from the index without the user ever knowing. borg check is the one caller that gets past the header — it passes a validator to resync, or drop_corrupt_tail under --repair when the key is unreadable — and it still does.

CorruptPack is not an IntegrityError subclass. That would print a traceback, and it would get swallowed by the except IntegrityError handlers in check_cmd.py and compact_cmd.py that handle single corrupt objects. The , run "borg check" tail is gone from the iter_headers() message now that the wrapper says something more useful.

The other commit

Testing this turned up a gap in #10094. ArchiveChecker builds its own chunks index with a validator, but repository.get() resolves chunk locations through repository.chunks, a separate index built lazily on first access with neither a validator nor drop_corrupt_tail. So a check aborted at a header it had already resynced past. Once CorruptPack was in, that turned into the command telling the user to run the command they were running.

Repository.chunkindex_validate and Repository.chunkindex_drop_corrupt_tail fix it: the lazy rebuild passes both on to iter_headers(), and ArchiveChecker.check() sets them — the validator always, so both modes resync the same way, and drop_corrupt_tail only when repairing, so a check that just diagnoses still never drops a pack tail. Handing over the checker's index object instead does not work, it stores plaintext sizes and PackWriter.add() asserts size == 0.

Verification

Real repository, one header byte flipped at a known object offset, index/ deleted:

• borg repo-list: the message above, rc 93, no traceback
• same with BORG_EXIT_CODES=legacy: rc 2
• borg compact: same message, rc 93
• borg check: resyncs past the header, finishes the diagnosis and reports the object the resync skipped as missing, rc 1
• borg check --repair: resyncs, reports the one lost chunk, rc 0

test_repo_list_aborts_cleanly_on_corrupt_pack covers the exit code and the missing traceback through the real CLI, forked so the top level error handler actually runs, and then the plain borg check run: rc 1 and the last line of the diagnosis, Archive consistency check complete, problems found. Which object the resync skips depends on how the pack was filled — a file chunk or an item metadata chunk — so the finding itself is not asserted. The two tests from #10094 that asserted IntegrityError out of build_chunkindex_from_repo (test_build_chunkindex_without_drop_corrupt_tail_raises_on_a_damaged_pack, test_check_without_repair_does_not_drop_a_pack_tail) now expect CorruptPack.

One thing the test does not claim: borg check --repair does not rewrite the damaged pack yet, so a later index-less rebuild walks into the same header again. That is a TODO in the test rather than an assertion that the repository is fine afterwards.

Full suite 3018 passed, 1117 skipped. ruff and black clean. Exit code 93 added to the table in frontends.rst, plus a note in the iter_headers() section of packs.rst.

Changed since the review

• The validator is now handed to the lazy repository.chunks rebuild unconditionally, not only under --repair. Only drop_corrupt_tail stays repair-only. Without this a plain borg check still aborted with rc 93 partway through its diagnosis, at the very header its own rebuild had just resynced past — the same gap the commit closes for --repair.
• The commit subject changed accordingly, check --repair: to check:, and the comment on the two Repository attributes no longer says they are set only when repairing.
• test_repo_list_aborts_cleanly_on_corrupt_pack gained the plain borg check run described above. It fails without the fix, with rc 93 instead of 1.
• test_check_without_repair_does_not_drop_a_pack_tail still holds: it arranges an unreadable key, so the checker's own rebuild raises before the lazy one is ever reached, and drop_corrupt_tail=repair keeps "a check that only diagnoses does not drop" explicit.

Two of the review's points are left for later, they are not this PR's subject: when the index fragments are unusable, the packs are walked twice with the validator (once for the checker's index, once for the lazy one), which is why the resync warning appears twice in the output above — copying the entries over would beat a second walk; and CorruptPack is the only top-level Error subclass not named ...Error, while the class name doubles as the JSON msgid.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.89%. Comparing base (9fe7f16) to head (5a5c889).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10168      +/-   ##
==========================================
+ Coverage   87.86%   87.89%   +0.03%     
==========================================
  Files         103      103              
  Lines       18878    18889      +11     
  Branches     2916     2916              
==========================================
+ Hits        16587    16603      +16     
+ Misses       1589     1586       -3     
+ Partials      702      700       -2     

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

[ThomasWaldmann]

@mr-raj12 please rebase onto current master. and then ask claude for feedback.

[ThomasWaldmann]

@mr-raj12 ping

[ThomasWaldmann]

review by claude fable 5.1 max

Verdict: mergeable after one small change. The first commit passes the validator to the lazy repository.chunks rebuild only when repairing, so a plain borg check (no --repair) still aborts with rc 93 in exactly the way this PR fixes for --repair. Everything else checks out: tests, lint, docs, exit code, and the CLI behaviour described in the PR.

Findings

1. A plain borg check still aborts at the header it already resynced past (archive.py#L2227-L2232)

Reproduced the PR's scenario on the PR head: one flipped header byte in the 2nd object of a pack, index/ deleted, then borg check without --repair. The checker's own rebuild resyncs fine (the validator is available, check_cmd reads the key before the check). Then rebuild_archives() lists the archives, which calls repository.get(), which triggers the lazy repository.chunks rebuild without a validator, and that raises CorruptPack:

$ borg check
Cannot cross-check packs against the chunk index: the index could not be loaded from its fragments; skipping missing-pack detection.
Store object packs/e0a3…13de is corrupted: content does not match its name (sha256).
Found 1 corrupt pack(s):
Corrupt pack: e0a3…13de
Finished full repository check, errors found.
pack e0a3…13de: no object header at offset 50186, continuing at the object at offset 100372.
Data integrity error: pack e0a3…13de: no object header at offset 50186 (pack corruption). Run "borg check --repair" to recover the objects that are still readable.
rc=93

So the check tells the user to run --repair halfway through its diagnosis instead of finishing it. Before this PR the same spot raised IntegrityError with a traceback and rc 90, so this is not a regression, but it is the same gap the first commit closes for --repair, and the fix is the same: mirror the checker's own build_chunkindex_from_repo() call unconditionally.

        # repository.chunks is a separate index, lazily built when repository.get() resolves a
        # chunk location. It walks the same packs, so give it the same corrupt-header handling
        # the rebuild above got - otherwise the check aborts at a header it just resynced past.
        self.repository.chunkindex_validate = validate
        self.repository.chunkindex_drop_corrupt_tail = repair

Verified with that change: the plain check completes its diagnosis with rc 1 (the doubled resync warning is finding 2):

$ borg check
…
Finished full repository check, errors found.
pack e25d…b880: no object header at offset 50186, continuing at the object at offset 100372.
pack e25d…b880: no object header at offset 50186, continuing at the object at offset 100372.
Missing chunk detected: ceaf…2a35, 50.00 kB.
The following chunks are missing in the repository:
- Chunk ceaf…2a35, 50.00 kB
    - input/f2: arch1
Archive consistency check complete, problems found.
rc=1

check_cmd_test.py and cache_test.py pass with it (112 passed). test_check_without_repair_does_not_drop_a_pack_tail still holds: without a validator the checker's own rebuild raises first, so the lazy one is never reached, and drop_corrupt_tail=repair keeps "a check that only diagnoses does not drop" explicit. Please add a check run without --repair expecting rc 1 to test_repo_list_aborts_cleanly_on_corrupt_pack (or a sibling test), so this stays covered.

2. Follow-up, not for this PR: --repair walks every pack twice with the validator when the fragments are unusable, once for the checker's index and once for the lazy repository.chunks. That is why the resync warning shows up twice in the check --repair output of this scenario. The validating walk reads a metadata slot and decrypts it per object, so the expensive part of the repair doubles. The PR's reason for not sharing the checker's index object is right (ChunkIndex.add() asserts v.size == 0 or v.size == size, and the checker stores plaintext sizes at archive.py#L2540), but copying the entries into a second index would still be far cheaper than a second walk. Worth an issue or a TODO.

3. Nit: the rebuild in Repository.check() (repository.py#L1328-L1336) runs only during --repair and now tells the user to run borg check --repair. Only reachable when a pack matches its sha256 name and still has a bad header, i.e. a writer bug, so leaving it is fine; maybe extend the comment there.

4. Nit, taste: CorruptPack is the only top-level Error subclass not named …Error, and the class name doubles as the JSON msgid, so it becomes API. PackCorruptionError would fit its neighbours (IntegrityError, DecompressionError, FileIntegrityError). Fine to keep if preferred.

One observation, not against the PR: any command persists a lazily rebuilt index at close() (checked with repo-list on a repo whose index/ was deleted: one fragment appears afterwards). With fix 1 a plain check therefore writes the resynced index back. That is the existing rule for every lazy rebuild, just noting it since Repository.check() otherwise takes care not to write.

Verified

• Rebased: merge base is 2 commits behind master, merges clean.
• Tests of the touched files: 301 passed, 41 skipped. The new CLI test fails without the first commit, so it does cover the attribute plumbing.
• ruff and black clean, no trailing whitespace, scripts/errorlist.py output matches the new docs entry, rc 93 was unused.
• CLI scenario: repo-list rc 93 without traceback, BORG_EXIT_CODES=legacy rc 2, --log-json msgid CorruptPack, compact rc 93, check --repair rc 0 with the resync and the one lost chunk reported. All as described in the PR.

[ThomasWaldmann]

@mr-raj12 ping?!