[mr-raj12]

Description

Fixes #10491.

borg check --repair --verify-data could leave a chunk unindexed although the repository holds an intact copy of it. The archives check then treated the chunk as missing.

Cause

The salvage is not required to hit this. The index rebuild of --repair walks the packs in pack id order and indexes the copy of a chunk id it walks last. It validates the object header and metadata only, so that copy can be the defect one. verify_data then removes it, and no other copy is looked at unless it happens to be in a pack the repair wrote.

Runs of the probe from the issue and two variants on master, 18 runs each:

Scenario	Failures
probe from the issue (D and X corrupt, full check)	11 of 18
D intact, full check	10 of 18
D intact, --archives-only (no salvage runs)	10 of 18

With D intact it fails exactly when pack A sorts before pack B. Re-reading the packs the salvage wrote would thus not fix the --archives-only case.

Fix

• build_chunkindex_from_repo has a new on_duplicate callback. The slow rebuild calls it for each object whose chunk id is indexed already, with the pack id of the copy indexed before.
• With --repair --verify-data, ArchiveChecker records these packs per chunk id (other_copies).
• After verify_data removed the defect chunks, index_other_copies walks the packs recorded for the removed chunk ids (a pack the removal rewrote: its replacement). It runs the verify_data read on each copy and indexes the first one that passes. A copy that fails is logged and stays unindexed.
• With --verify-data, the re-read of the written packs in finish() (verify_written_packs) runs the same read before it indexes an unindexed object. A pack the removal rewrote can hold a defect copy of a removed chunk; that copy is logged and stays unindexed.

index_other_copies runs before the archives check, so the archives referencing the chunk are not reported as damaged.

other_copies holds one entry per chunk id that is stored more than once, and only with --repair --verify-data.

Relation to other issues

• check --repair: a corrupt pack in which no object authenticates is never removed, check and compact keep failing #10479: independent. Removing a pack in which nothing authenticates hides the repro of the issue, but not the --archives-only case and not a check that skips the salvage.
• compact, repo-compress: superseded duplicate dropped when its indexed copy is in a missing or corrupt pack #10474 / compact, repo-compress: keep a superseded duplicate indexed in a missing or corrupt pack, fixes #10474 #10475 (merged, this PR is rebased onto it): if the other copy is in a pack that also holds a defect chunk, the rewrite of that pack keeps the copy and index_other_copies finds it in the replacement pack.

Tests

• test_verify_data_repair_indexes_other_copy_of_a_defect_chunk: a file content chunk of an archive with a defect indexed copy. Two cases: a flipped byte with --archives-only, and a copy with wrong content (it authenticates, only the id check fails) with the full check. Neither depends on how the repository check treats a pack in which nothing authenticates (check --repair: a corrupt pack in which no object authenticates is never removed, check and compact keep failing #10479). Both fail on master.
• test_verify_data_repair_does_not_index_a_defect_other_copy: the other copy is defect too.
• test_verify_data_repair_does_not_index_a_defect_gap_copy_in_a_rewritten_pack: the other copy is defect too and is in a pack the removal rewrote. The next check --verify-data passes.
• test_repair_finish_fixes_a_wrong_index_entry_for_a_written_pack: now also runs with --verify-data, where an intact unindexed object is indexed after the verify read.
• test_index_other_copies_follows_replaced_packs: the other copy is in a pack that was replaced once, replaced twice or removed; an archive metadata object is returned as such.
• test_verify_data_repair_keeps_gap_copy_of_a_defect_chunk (from compact, repo-compress: keep a superseded duplicate indexed in a missing or corrupt pack, fixes #10474 #10475): docstring updated, uses put_copy_in_later_pack.
• test_build_chunkindex_reports_duplicates: the on_duplicate callback.

The probe from the issue is not added as a test: without the fix it only fails in some runs.

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

[ThomasWaldmann]

The approach is sound: recording the other copies during the rebuild and verifying them before the archives check fixes #10491, including the --archives-only case. Tested on current master (includes #10475) plus this PR: check_cmd_test.py, cache_test.py and repository_test.py pass, and the new check tests fail without the archive.py change.

Please rebase onto master (#10475 is merged now) and address the following.

A defect other copy still gets indexed

index_other_copies says a copy that fails the verify read "is logged and not indexed". That does not hold if the copy is in a pack the repair rewrote: verify_written_packs() in finish() indexes it afterwards, without verifying it. You mention this in the description ("The PR that lands second should make that re-read skip it") and this PR is now the second one.

Reproduced on master + this PR:

def test_defect_gap_copy_in_rewritten_pack(archiver):
    """A = [D defect, W, X defect (not indexed)], B = [X defect, indexed]."""
    cmd(archiver, "repo-create", RK_ENCRYPTION)
    with open_repository(archiver) as repository:
        repo_objs = Manifest.load(repository).repo_objs
        d, w, x = b"defect", b"other", b"duplicate"
        d_id, w_id, x_id = (repo_objs.id_hash(data) for data in (d, w, x))
        for cid, data in [(d_id, d), (w_id, w), (x_id, x)]:
            repository.put(cid, repo_objs.format(cid, {}, data, ro_type=ROBJ_FILE_STREAM))
        repository.flush()
        pack_a = repository.chunks[x_id].pack_id
        corrupt_chunk_on_disk(repository, d_id)
        corrupt_chunk_on_disk(repository, x_id)  # the copy in pack A
        put_copy_in_later_pack(repository, repo_objs, x_id, x, pack_a)
        corrupt_chunk_on_disk(repository, x_id)  # the indexed copy, in pack B
    cmd(archiver, "check", "--archives-only", "--repair", "--verify-data", exit_code=0)
    with open_repository(archiver) as repository:
        assert x_id not in repository.chunks  # fails

The log shows the new copy in pack ..., integrity error line, then objects in the pack with an unindexed chunk id: 1. Fixed the index.. Afterwards X is indexed at the defect copy and the next borg check --verify-data fails with rc 1 on X.

Two ways to fix it:

• remember the (pack_id, obj_offset) of the copies index_other_copies rejected and skip them in pass 2 of verify_written_packs, or
• more general: if verify_data ran, pass 2 does the verify read before it indexes an unindexed object.

test_verify_data_repair_does_not_index_a_defect_other_copy does not catch this, because its pack A is not rewritten. Please add a case like the one above.

Minor

• After the rebase, the docstring of test_verify_data_repair_keeps_gap_copy_of_a_defect_chunk (from #10475) is stale: X is then indexed by index_other_copies, not by finish().
• That test has the "retry until the pack id is higher" loop inline, it could use put_copy_in_later_pack.
• on_duplicate is silently ignored if the index is built from the fragments. Only the slow rebuild passes it today, so an assert (or a comment) would do.
• The other copy is read once, while the indexed copy must fail twice before it is removed. That fails closed, so it is fine, but worth a comment.
• other_copies costs about 350-400 bytes per chunk id that is stored more than once. OK as long as duplicates are rare.

[mr-raj12]

Rebased onto master and pushed: with --verify-data, finish() now does the verify read before it indexes an unindexed object, your test case is added, and the minor points are addressed.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.10%. Comparing base (4d359e5) to head (35ecc47).
⚠️ Report is 58 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10493      +/-   ##
==========================================
+ Coverage   88.80%   89.10%   +0.29%     
==========================================
  Files         103      103              
  Lines       19516    19743     +227     
  Branches     3050     3092      +42     
==========================================
+ Hits        17332    17592     +260     
+ Misses       1512     1490      -22     
+ Partials      672      661      -11     

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