check: use one chunk index for the checker and the repository, refs #10364

[mr-raj12]

Description

borg check holds the chunk index in memory twice. Refs #10364, same change as #10359 does for borg compact.

Where the second index comes from:

• full check: Repository.check() loads the index from the index/ fragments and keeps it as repository.chunks. ArchiveChecker.check() then builds its own index while that one is still in memory.
• --archives-only: ArchiveChecker.check() builds its index, then the first repository.get() builds a second one through the Repository.chunks property.
• --repair with changed packs: finish() rebuilds the index from the packs while the checker's index is still in memory.

Changes in ArchiveChecker:

• check(): calls repository.invalidate_chunk_index() before building the index, then sets repository.chunks = self.chunks. get(), put() and delete() of the repository use this index from then on.
• check() with --repair: calls self.chunks.clear_new() after the rebuild from the packs. The rebuild sets F_NEW (entry not stored in the index/ fragments yet) on every entry, and Repository.close() stores all F_NEW entries as a new fragment. finish() stores the complete index.
• add_reference(): with --repair, only calls repository.put(), which adds the entry to the index and sets its pack location. Calling self.chunks.add() before put() on the same index fails the assert v.size == 0 or v.size == size in ChunkIndex.add(). Without --repair, it does not change the index: entries without a pack location (F_PENDING) in the repository's index make Repository.close() fail with AssertionError: chunk ... has no pack location yet.
• verify_data() with --repair: removes del self.chunks[defect_chunk]. repository.delete() already removes the entry from the index, so the del raises KeyError.
• finish() with changed packs: calls repository.invalidate_chunk_index() and sets self.chunks = None before rebuilding the index from the packs.

Tests:

• test_check_holds_a_single_chunk_index, for --archives-only, full check and --repair, on a repo with a missing item metadata chunk. It asserts that repository.chunks is not loaded when the checker builds an index, that repository.chunks is the checker's index in rebuild_archives(), and that the Repository.chunks property builds no index. All three cases fail on master.
• test_check_without_repair_leaves_the_chunk_index_alone: a check without --repair on the same kind of repo does not change the index/ fragments or the chunk ids in the index.
• With only repository.chunks = self.chunks added to master, 10 tests in check_cmd_test.py fail with the AssertionErrors and the KeyError above, among them the existing test_verify_data, test_missing_file_chunk and test_missing_archive_item_chunk.

Full test suite: 3061 passed, 1128 skipped.

Checklist

• PR is against master (or maintenance branch if only applicable there)
• New code has tests and docs where appropriate
• Tests pass (run tox or the relevant test subset)
• Commit messages are clean and reference related issues

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.06%. Comparing base (bd7282c) to head (4b6d54c).
⚠️ Report is 3 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #10368   +/-   ##
=======================================
  Coverage   88.06%   88.06%           
=======================================
  Files         103      103           
  Lines       18913    18915    +2     
  Branches     2919     2919           
=======================================
+ Hits        16655    16657    +2     
  Misses       1565     1565           
  Partials      693      693           

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

[ThomasWaldmann]

Reviewed on the PR base 96085d51 vs head 4b6d54ca. The change does what it says, the reasoning in the description holds up under testing, and the two things that could have gone wrong — a non-repair check writing to the index, and the repair paths corrupting the now-shared index — are both handled and both covered by tests. Comments below are non-blocking.

What I verified

The memory claim, measured. Build counts alone can't show this (they're mostly the same as master), so I counted non-empty ChunkIndex objects alive after every build:

run	master: builds / alive at once	this PR: builds / alive at once
check --archives-only	2 / 2	1 / 1
check	2 / 2	2 / 1
check --verify-data	2 / 2	2 / 1
check --repair, healthy repo	2 / 2	2 / 1
check --repair, item chunk missing (packs change)	3 / 3	3 / 1

Peak is one index everywhere, including the repair-with-changed-packs case that master had at three.

"The one-liner alone is not enough". With only self.repository.chunks = self.chunks applied to the base, 28 tests in check_cmd_test.py fail (the description says 10 — an undercount, the direction is right). 24 of them die in close() with AssertionError: chunk … has no pack location yet: without repair, the old add_reference left F_PENDING entries for the re-chunked item stream in what is now the repository's index. That hits even test_check_format on a healthy repo, so the add_reference change is load-bearing.

Tests. The two new tests pass locally (local + remote kinds); check_cmd_test.py + repository/cache/compact tests: 298 passed. test_check_holds_a_single_chunk_index is well designed — repository_builds == 0 catches any accidental lazy build in the window between invalidate_chunk_index() and the assignment.

Reasoning I checked and agree with. Nothing in the checker reads entry.size (only item.size), so put() recording size 0 instead of the plaintext size changes nothing; finish() either rebuilds or zeroes sizes on write anyway. The removed del self.chunks[defect_chunk] would indeed KeyError, since delete() now removes it from the same object. And the sharing is a small correctness gain for --repair: reads now resolve through the pack-walked index rather than a fragment-built one, so a chunk that exists in a pack but not in stale fragments is readable during repair.

Comments

1. The clear_new() comment gives the wrong reason (archive.py#L2248). It says clearing F_NEW stops close() re-storing the entries "as an extra fragment" — but on the normal path finish() invalidates the index before close() runs, so close() stores nothing regardless. Where it matters is an exception before finish(): then close() would persist the entire pack-walked index as an "incremental" fragment next to the old ones. Worth saying that's the case being guarded.

2. A full check now discards an index it just loaded. Repository.check() merges the fragments and installs the result with the comment "so later reads reuse it", and the very next thing ArchiveChecker.check() does is invalidate it and merge the same fragments again (that's the 2 / 1 in the table). For a non-repair check with repository.is_chunk_index_loaded, the checker could take that index instead of building — identical content, same F_NEW state — saving one fragment merge per full check and making that comment true again. Optional; peak memory is one either way.

3. Is the slow rebuild in finish() still needed? (archive.py#L2761) Its old justification ("delete repoints that pack's other objects in the repository's index, so our offsets are stale") is gone by construction — there is only one index now and compact_pack()/put() maintain it in place. What remains is a re-validation of every object header after repair. That may well be worth keeping as belt-and-braces, but it's now a choice rather than a necessity, and it's the last double pack walk in --repair. A follow-up question, not for this PR.

4. add_reference(id_, size, cdata) — size is now unused (archive.py#L2535). It has to stay (it's the callback signature archive_put_items uses), but a one-word comment would stop the next reader from wondering.

5. Merge interaction with compact: build the chunk index once, not three times #10359. The follow-up there rewrites the Repository.chunks setter comment to list compact as an installing caller; this PR adds the checker as another one and doesn't touch that comment. Whichever lands second should mention both.

6. Description nit: "10 tests" → 28, per above.

Out of scope, noted for later

check --repair on a repo whose repository check finds a corrupt fragment still does two full pack walks in sequence (Repository.check() rebuilds and persists, then the checker rebuilds again). Pre-existing, unchanged here, peak stays one.

🤖 Review drafted with Claude Code