check: make the chunk index rebuild interruptible, fixes #10042

[mr-raj12]

Description

Fixes #10042. Also the re-check asked for in #7893.

build_chunkindex_from_repo() reads every pack of the repository when it has to do a slow rebuild. On a large repository that takes a long time, and it did not look at sig_int, so a Ctrl-C during it had no visible effect until it was done.

It now takes an interruptible kwarg, default False. With it set, the pack loop checks sig_int before reading the next pack, discards the index it built so far and raises ChunkIndexRebuildInterrupted. A partial index is never returned nor stored: it lacks chunks that are still in the packs, so a check would report them as lost and --repair would drop the archives' references to them.

Why opt-in per call site

ArchiveChecker.finish() rebuilds the index with sig_int already set (check() calls it after the interrupt, on purpose, #9850) and then calls delete_chunkindex_invalid(), which declares the stored index to match the packs. That only holds once every pack was indexed, so that rebuild has to run to completion. A blanket sig_int check inside the rebuild would break it, hence the kwarg.

Call sites that get interruptible=True:

• ArchiveChecker.check() (archive.py), the rebuild that runs before any archive is looked at. The exception propagates out of borg check; invalidate_chunk_index() already ran and Repository.close() guards on is_chunk_index_loaded, so nothing is persisted.
• the --repair index rebuild in Repository.check() (repository.py). It catches the exception, warns, and leaves index_repaired False, so the existing summary reports Interrupted full repository check, index still corrupt so far. and check() returns False. The corrupt index/ fragments are still there and the next use rebuilds from the packs. No new summary message was needed.

Deliberately left uninterruptible

The index object verification loop in Repository.check(). index_errors decides whether the packs are verified and whether the index is rebuilt, so breaking out of it early makes an interrupted check on a fully corrupt index report no problems, which is a false all clear. I implemented it, test_check_repair_leaves_index_when_interrupted caught it, and I dropped it. That loop is bounded by the index size, not by the repository size.

Docs

The check epilog still described a SIGINT-deaf key recovery scan in ArchiveChecker.make_key(). That scan does not exist anymore: #10377 reduced make_key() to key_factory(repository), and key_from_repository is gone from the tree. The epilog now describes the rebuild and what Ctrl-C does to it. Generated usage docs and man pages are not touched, as usual.

Tests

Five new tests, all using a fake sig_int object plus a patched PackReader.iter_headers that trips it after the first pack, so they assert on the pack read count rather than on timing.

• cache_test: default (not interruptible) walks all packs and returns a complete index; interruptible=True raises, does not read the second pack and stores nothing even with write_immediately=True.
• repository_test: check(repair=True) on a repository with rotten index/ fragments returns False, logs the interruption and the index still corrupt so far summary, reads one pack, leaves the fragments untouched; a later check still reports the corruption.
• check_cmd_test: ArchiveChecker.check(repair=True) raises Got Ctrl-C, stores no partial index, and a second borg check --repair completes; plus a regression guard for borg2: crash while compact #9850 that asserts finish() walks every pack and clears the invalid marker although sig_int is set.

Checklist

• PR is against master
• New code has tests and docs where appropriate
• Tests pass: pytest --benchmark-skip -n auto -k "not remote and not binary", 2927 passed, 530 skipped. ruff check . and black --check clean.
• Commit messages are clean and reference related issues

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.14%. Comparing base (1110bcd) to head (6cd082e).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #10382   +/-   ##
=======================================
  Coverage   88.14%   88.14%           
=======================================
  Files         103      103           
  Lines       18896    18906   +10     
  Branches     2931     2932    +1     
=======================================
+ Hits        16655    16664    +9     
- Misses       1557     1558    +1     
  Partials      684      684           

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

[ThomasWaldmann]

Thanks, this is a careful PR. The opt-in interruptible kwarg with the finish() reason written down, discarding the partial index rather than persisting it, and tests that assert on pack counts instead of timing are all the right calls. The epilog cleanup is correct too: make_key() is just key_factory(repository) since #10377.

What I checked, beyond reading the diff:

• ruff / black clean; cache_test.py, repository_test.py, check_cmd_test.py pass locally (macOS, -k "not remote and not binary").
• A real SIGINT through the CLI: 22-pack repo, PackReader.iter_headers slowed to 0.2 s per pack. Both borg check --repair --archives-only and a full borg check --repair exited 0.1-0.2 s after the signal with rc 2 and the new message, and a plain borg check afterwards returned rc 0.
• A mutation test: with interruptible=True added to the rebuild in finish(), only test_check_repair_finish_completes_index_rebuild_after_interrupt fails (assert 0 == 52); test_check_repair_soft_interrupt still passes. So the new test is the only guard for the #9850 behaviour and earns its place.

Requested changes (details inline for 1, 2 and 5):

1. An interrupted --repair index rebuild can report that the index was rebuilt. Must fix, one line.
2. The epilog overpromises for the rebuild in finish(). One caveat sentence, and a warning instead of info when sig_int is set.
3. Fixes #10042 would close the issue with one item open. #10042 also asks for read-only checks to break inside the item loop of rebuild_archives() ("Worth branching on repair"). Please either do that too or say refs #10042 and note what is left.
4. ChunkIndexRebuildInterrupted is missing from docs/internals/frontends.rst. It belongs to the rc 2 generic errors in the error list there; we update that list together with a new Error class (see 12360cb, 5a5c889). rc 2 is the right choice, the existing Error("Got Ctrl-C / SIGINT.") uses it as well.
5. Optional: stopping only at a pack boundary is an unnecessary limit.

Nits:

• "the stored chunk index was left unchanged" (exception docstring, epilog) is not strictly true on the non-repair path with the invalid marker set: build_chunkindex_from_repo deletes the fragments before it falls through to the slow rebuild, so an interrupt there leaves no fragments. Same outcome (full rebuild next time), so only wording.
• cache_test.py imports fchunk inside two_pack_repo() while Interrupter is imported at module top; pick one.

[ThomasWaldmann]

drops keeps whatever the discarded rebuild counted, and in the summary below elif drops: is evaluated before elif index_errors and not index_repaired:. So if the walk dropped a byte range in a pack it read before the Ctrl-C, the check reports a rebuilt index although the index is still corrupt.

Reproduced with this PR's own rotten-fragments setup plus a validator that rejects every object (packs pass the store hash check, objects fail the object validator):

WARNING  Index rebuild interrupted; the index stays corrupt and is rebuilt on next use.
INFO     Checked 2 index files (2 errors) and 4 packs (0 errors).
ERROR    The index rebuild skipped 1 pack byte range(s) it could not authenticate.
WARNING  Interrupted full repository check, index rebuilt without pack byte range(s) it could not authenticate so far.

check() still returns False, so only the messages are wrong, but "index rebuilt" during a corruption repair is a bad thing to get wrong. The count of a partial, discarded walk carries no information anyway:

Suggested change

	except ChunkIndexRebuildInterrupted:
	except ChunkIndexRebuildInterrupted:
	drops = 0 # the discarded rebuild walked only a part of the packs, its count says nothing.

A test for it would be good: the existing test_check_repair_index_rebuild_interrupted with a rejecting validator, asserting on "index still corrupt so far".

[ThomasWaldmann]

Agreed that this rebuild must run to completion. It only runs if the repair stored or deleted chunks (chunks_modified), so not on a healthy repository, but on a big damaged one a Ctrl-C during the archives phase is followed by a full walk over all packs that is announced at info level only, i.e. not at all at the default log level. That is the "Ctrl-C looks like it did nothing" symptom of #10042 again.

Suggestion: if sig_int is set here, log a warning instead, along the lines of "Rebuilding and writing the repository chunks index. This reads every pack and can not be interrupted."

[ThomasWaldmann]

As written, this reads as if every chunk index rebuild of borg check responds to Ctrl-C. The one in finish() (after a --repair that stored or deleted chunks) deliberately does not, and that is the one a user waits for after interrupting the archives phase. Please add a sentence about it, e.g. that after a repair that changed the repository, borg rebuilds and stores the chunk index once more and that this rebuild always runs to completion.

Also "The stored index is left unchanged" needs a small caveat, see the nit in the review summary.

[ThomasWaldmann]

Optional. In Repository.check() the pack boundary matters, because the tracker records one result per pack. Here nothing of the partial walk is kept, so the rebuild can stop anywhere.

With a validator (--repair always has one) iter_headers costs one store request per object. A 50 MB pack of small objects means thousands of round trips, so on ssh:// or rest:// a Ctrl-C may have to wait quite a while for the current pack to end (estimated from the code, not measured).

The same check inside the for chunk_id, obj_offset, obj_size in reader.iter_headers(...) loop below would make it respond at once, e.g. by moving these lines into a small local function that both places call. The tests would still see len(packs_read) == 1.

[ThomasWaldmann]

ping @mr-raj12