Skip to content

check --repair: re-read only the packs the repair wrote, refs #8466 - #10379

Open
mr-raj12 wants to merge 2 commits into
borgbackup:masterfrom
mr-raj12:check-finish-narrow-walk-8466
Open

mr-raj12 wants to merge 2 commits into
borgbackup:masterfrom
mr-raj12:check-finish-narrow-walk-8466

Conversation

@mr-raj12

@mr-raj12 mr-raj12 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Description

Refs #8466, item 2 of #10318. Based on #10378, only the last commit is new.

With #10368 the checker and the repository share one chunk index, and put() / delete() update it for every pack they write. ArchiveChecker.finish() stores that index and re-reads only the packs the repair wrote, to confirm the index matches them.

  • ArchiveChecker records the packs repair writes in written_packs: from the put() and flush() results, and from delete(), which now returns compact_pack()'s (new_pack_id, dropped_bytes). A pack rewritten again is replaced by its new pack.
  • create_archive_entry() stores the pack writer buffer and records its packs, then creates the archives directory entry.
  • verify_written_packs() reads the object headers of each written pack with a validator and compares them with the index entries that name the pack:
    • an index entry names an object the pack does not hold: the entry is removed
    • the pack holds an object whose chunk id is not indexed: the object is indexed
    • the pack does not exist: its index entries are removed
    • each of these is a check finding
    • an object whose chunk id is indexed at another location is a superseded duplicate, not a finding: compact_pack copies one into the new pack when it lies in a byte range no index entry covers
  • finish() stores the index, then drops the in-memory one (invalidate_chunk_index() and self.chunks = None), because close() would persist it over the index just stored.
  • check() asserts that a repair has a validator, so every object it indexes is checked.
  • Repository.flush() returns the objects of the packs it stored, None if it stored none.

Tests (check_cmd_test.py), each asserting which packs finish() walks:

  • only the pack delete() wrote, with more than ten packs in the repository
  • no pack, for a defect chunk alone in its pack: delete() drops the pack and writes none
  • only the packs put() wrote
  • the pack finish()'s own flush() stores
  • a rewritten pack no index entry names, and a rewritten pack holding a superseded duplicate: neither is a finding
  • a wrong index entry for a written pack, and a written pack that is gone: both are findings, and the index is fixed
  • test_check_holds_a_single_chunk_index: the --repair case builds one index in the checker, not two
  • test_check_repair_stopped_in_the_index_store_marks_the_index_invalid: renamed, finish() stores the index instead of rebuilding it

Tests (repository_test.py): flush() returns the stored objects, None with nothing buffered or no pack writer.

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 Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10379      +/-   ##
==========================================
- Coverage   88.14%   85.65%   -2.49%     
==========================================
  Files         103      103              
  Lines       18896    18946      +50     
  Branches     2931     2944      +13     
==========================================
- Hits        16655    16228     -427     
- Misses       1557     2047     +490     
+ Partials      684      671      -13     

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

@mr-raj12
mr-raj12 force-pushed the check-finish-narrow-walk-8466 branch from 0a6e74a to 74fcb2b Compare September 16, 2026 13:29
@mr-raj12
mr-raj12 force-pushed the check-finish-narrow-walk-8466 branch from 74fcb2b to c25f8d1 Compare September 17, 2026 11:57
@ThomasWaldmann

Copy link
Copy Markdown
Member

@mr-raj12 #10368 is merged.

Every check --repair rebuild of the chunk index has an object validator
(borgbackup#10369), so drop_corrupt_tail was only reachable from tests.

- PackReader.iter_headers: remove the drop_corrupt_tail parameter. Without
  a validator, a corrupt object header raises IntegrityError.
- build_chunkindex_from_repo: remove the drop_corrupt_tail parameter.
- Repository: remove chunkindex_drop_corrupt_tail and chunkindex_validate.
  Since borgbackup#10368 the checker hands its index to the repository, so the lazy
  .chunks rebuild never runs during a check and nothing set either of them.
- ArchiveChecker.check: stop passing drop_corrupt_tail.
- tests: remove the 5 tests for drop_corrupt_tail, rewrite
  test_check_without_repair_does_not_drop_a_pack_tail as
  test_check_without_key_aborts_on_a_corrupt_pack_header.
@mr-raj12
mr-raj12 force-pushed the check-finish-narrow-walk-8466 branch from c25f8d1 to 84a6388 Compare September 18, 2026 04:42
…kup#8466

finish() validates the written packs against the shared index instead of rebuilding it from all packs.
@mr-raj12
mr-raj12 force-pushed the check-finish-narrow-walk-8466 branch from 84a6388 to 8903d7d Compare September 18, 2026 04:58
@mr-raj12
mr-raj12 marked this pull request as ready for review September 18, 2026 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants