check: detect missing packs referenced by the index (#9898)

[mr-raj12]

Fixes #9898.

A read-only check verified pack and index integrity separately but never compared them, so a pack that the chunk index still referenced while missing from packs/ passed clean. The loss only showed up later, on extract.

With an intact index, check now reads the chunk index from its stored fragments and cross-checks its pack ids against packs/:

• pack ids referenced by the index but absent from packs/ are reported as errors and fail the check (data loss).
• packs present in packs/ that no index entry references are reported at info level and do not fail the check; they are leftovers of an interrupted operation, not an error.

The cross-check reads the index from its fragments only, never rebuilding it from the packs (too slow for a routine check) and never writing to the repo. It is skipped, and the check still passes, when the index cannot be read that way: a corrupt or invalid index is rebuilt from the packs on next use (and so can never reference a missing pack), and a pack that is truly gone then surfaces as missing chunks in the archives check. It is also skipped on a partial run (--max-duration), which the full check's cross-check covers.

Tests cover the missing-pack detection, the clean case, the invalid-index skip, the unreadable-fragments skip, the partial-run skip, and the orphan-pack report.

[codecov]

Codecov Report

❌ Patch coverage is 90.54054% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.65%. Comparing base (bf6a449) to head (f500b0f).
⚠️ Report is 20 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/borg/cache.py	80.64%	4 Missing and 2 partials ⚠️
src/borg/archiver/repo_compress_cmd.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10069      +/-   ##
==========================================
- Coverage   86.77%   84.65%   -2.13%     
==========================================
  Files          98       99       +1     
  Lines       17277    17389     +112     
  Branches     2622     2642      +20     
==========================================
- Hits        14992    14720     -272     
- Misses       1587     1982     +395     
+ Partials      698      687      -11     

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

[ThomasWaldmann]

ping @mr-raj12

[ThomasWaldmann]

Review by Claude:

Thanks for working on this! I reviewed the branch locally (built it, ran the tests, probed the behaviour). The direction is right and this is what #9898 asks for, but as it stands there is one crash regression, one silent no-op path, and a cost regression that undoes what check()'s docstring promises.

Blockers

1. borg check now crashes on an unparseable index/ object.

The cross-check calls build_chunkindex_from_repo(), which calls ChunkIndex.read() with no error handling (src/borg/cache.py, read_chunkindex_from_repo). An object under index/ whose content matches its sha256 name but is not a serialized ChunkIndex passes verify() and then blows up:

ValueError: Invalid file, magic BORGHASH not found.
  borg/cache.py read_chunkindex_from_repo -> hashindex.pyx:139 ChunkIndex.read

On master the same repo checks clean. check is the tool one runs on a broken repo, so it must report the problem, not traceback.

Note that the PR works around this in the test file rather than fixing it: test_check_detects_index_corruption and the progress test had their placeholder blobs (b"pretend this is a serialized chunk index") replaced by real serializations via the new _serialized_chunkindex() helper. That hides the regression, and no test covers the hash-valid-but-unparseable case.

2. The check silently becomes a no-op exactly when it matters.

If the index fragments cannot be loaded, build_chunkindex_from_repo() falls through to the slow rebuild from the packs - so referenced_pack_ids is a subset of present_pack_ids by construction and the diff is always empty. I reproduced this: a repo with a deleted pack and no index fragments triggers the slow rebuild, check returns True and reports no missing pack. The same repo with an intact index correctly fails.

The fallback also does a full pack-header scan of the whole repo, which the docstring a few paragraphs above explicitly rules out ("reading every pack to do so would be far too slow and expensive for a routine (e.g. cron) check").

This needs a no-fallback path (a slow_rebuild opt-out, or a dedicated helper that only reads the fragments) plus an explicit "cannot cross-check, index not loadable" message.

3. The whole chunk index is now downloaded and built on every repository check - twice.

verify() uses store.hash(), which is computed server-side, so today borg check --repo-only transfers essentially nothing from a REST/cloud repo. This PR adds a full chunk index download plus roughly 160 bytes/chunk of RAM (entry 48 B + 32 B key, load factor 0.5).

On top of that it builds a throwaway index instead of using the self.chunks property, so a full borg check builds it twice. Instrumenting build_chunkindex_from_repo:

builds after check(): 1
builds after a read:  2

Using self.chunks would let the archives check reuse it. That still leaves --repo-only and --max-duration paying full price, and --max-duration is precisely the bounded-cost mode - the cross-check runs after the time budget has been spent.

Should be fixed before merge

• Ctrl-C: master now breaks the pack loop on sig_int. The branch predates that, so after a rebase the cross-check would build the entire index after the user interrupted. Guard it with if not sig_int.
• The PR description claims something the code does not do. It says orphan packs "are reported at info level", and the reply to my "opposite case" comment says it is handled - but there is no such code on the branch. Either implement it or fix the description.
• Size checking: my comment on borg2 check: vanished packs? #9898 also suggested comparing sizes. store_list infos carry .size, and the index has obj_offset/obj_size, so max(offset + size) per pack gives a cheap expected-size floor. That is the natural companion to the presence check.
• The message is not actionable: "Missing pack: " leaves the user without a next step, and --repair neither fixes it nor stops returning success (return not problems or repair). At minimum say that the referenced chunks are lost and point at the index rebuild story (to pack or not to pack ... #8572).
• The branch is 36 commits behind master. It still merges textually, but check() has moved meanwhile (sig_int / "Interrupted" handling) - please rebase, the summary block needs another look afterwards.

Non-blocking

• Design question worth settling: with an invalid index, the new test asserts check() returns True for a repo that is missing a pack. That is the concrete form of my earlier "does it make sense to continue after this warning?" question - the answer is not obviously yes.
• The F_PENDING guard is correct and needed (pack_id is b"\xff" * 32 for pending entries), although fragments should not normally contain any. A short comment saying it is defensive would help.
• from ..cache import write_chunkindex_invalid sits inside the test function; the file imports at the top elsewhere and there is no circular-import reason here.
• _serialized_chunkindex(chunks=None): the parameter is never passed.
• The count of missing packs is printed twice (summary sentence plus the "Found N missing pack(s)" header).

One thing that is not a problem, contrary to my first reading: concurrency. check_cmd holds an exclusive lock, so no writer can add packs or invalidate the index while the check runs, and the pack listing / index snapshot cannot drift apart.

[ThomasWaldmann]

Item 1 is an attack scenario - unlikely to happen just by corruption.
Item 3 is at least one rebuild too much.

Rebase on current master seems indicated.

[ThomasWaldmann]

ping?

[ThomasWaldmann]

Re-reviewed at 578c93a22 (rebased on current master). Built it and ran it here.

Fixed since my last pass

The crash on an unparseable index/ object (now caught in read_chunkindex_from_repo), the silent no-op path (fragments_only=True returns None instead of slow-rebuilding), the Ctrl-C guard, orphan pack reporting, the actionable missing-pack message, the duplicated count line, plus the import-at-top and unused-parameter nits. Tests are green here (repository + cache: 156 passed; check + compact archiver tests: 56 passed), and end-to-end on a real repo with a deleted pack it reports correctly and exits 1, with no spurious orphan noise on a healthy repo. Thanks, that is good progress.

Still open

1. The double index build is still there - this is the "at least one rebuild too much" from my earlier comment. Verified by instrumenting build_chunkindex_from_repo over a full borg check:

builds after check(): [fragments_only=True]
builds after a read:  [fragments_only=True, fragments_only=False]

check() builds the index, reads the pack ids out of it and throws it away; the archives check then builds it again from the same fragments.

One line fixes it: replace chunks.clear() with self.chunks = chunks. The fragments-only result is exactly what the .chunks property would have built (same code path, minus the fallbacks), and clear_new() has already run, so close() writes nothing back. I tried it locally: the second build is gone and the tests still pass.

2. A partial check now says "no problems found" on a repo that lost data. Same repo, back to back:

$ borg check --repository-only
1 pack(s) referenced by the index are missing:
Missing pack: 9b32a538...
Finished full repository check, errors found.          exit=1

$ borg check --repository-only --max-duration=3600
Finished partial repository check, no problems found.  exit=0

That skip came out of my last review and I framed it badly: my cost objection was really about the double build (item 1). The cross-check itself is a single index load - O(index), not proportional to pack bytes - so skipping it buys very little, and it buys that with a false all-clear in exactly the mode people run from cron on big / cloud repos, which is the use case I described in #9898.

Better: run it on partial runs too, before the pack loop so that --max-duration covers it; or at the very least do not print "no problems found" when the cross-check was skipped. test_check_partial_skips_missing_pack_cross_check currently pins the misleading behaviour.

3. Retry amplification on an unreadable fragment. read_chunkindex_from_repo returning None makes the merge loop treat "unparseable" the same as "vanished", so it re-lists and re-merges every fragment CHUNKINDEX_MERGE_ATTEMPTS times before giving up - I measured 3 warnings and 3 restarts, i.e. the whole index is read three times to reach a conclusion that was already final after the first read. Retrying is right for a vanished fragment, pointless for a corrupt one; the two cases should be distinguishable (a sentinel or a dedicated exception).

4. This duplicates logic compact already has, and the two give contradictory advice. compact_cmd.py already does both directions: stale_ids (index entries whose pack is absent) and unindexed (pack bytes no entry covers). On the same damaged repo:

• borg compact: 1 index entries reference missing pack files; run "borg check --repair".
• borg check: The chunks stored in these packs are lost. Repairing the index ... is tracked in #8572.

So compact sends the user to a repair that check says does not exist yet. The wording should be aligned, and ideally the two should share one helper - they also differ in granularity (compact counts index entries, check counts packs). (Unrelated pre-existing nit I noticed there: "1 index entries".)

Smaller things

• fragments_only does not enforce its own "never write to the repo" comment: with write_immediately=True the fast path still calls repack_chunkindex(). Please add assert not (fragments_only and write_immediately) next to the existing assert.
• The softened read_chunkindex_from_repo means an unreadable fragment now silently degrades every operation (3 retries, then the slow rebuild) instead of failing loudly. It only self-heals when something runs with write_immediately=True (compact / repo-compress, via delete_other=True) - the warning should say so instead of just "treating it as invalid". Also, except Exception is quite broad; ValueError / struct.error would be tighter.
• The orphan message gives a count but no pack ids, not even at debug level - that is the first thing one wants when diagnosing. It also calls them "leftovers from an interrupted operation" while compact calls the same bytes "not covered by the index"; one vocabulary for both would be better.

Items 1 and 2 should be fixed before this can go in, 3 and 4 are worth doing now while the code is fresh, the rest is polish.

[ThomasWaldmann]

LGTM