check --repair: resync past corrupt object headers when rebuilding the chunks index

[mr-raj12]

Follow-up to #10083 (header validation, #8476).

When borg check --repair rebuilds the chunks index from the packs, a corrupt object header no longer aborts the whole pack. iter_headers(validate=...) scans forward for the next object that validates and resumes there, so the objects after the damaged region are still indexed. The skipped byte range is logged.

Without a validate function (repository-only check, routine rebuild) the behavior is unchanged: a corrupt header raises IntegrityError.

Details:

• _find_header searches for OBJ_MAGIC a window at a time, the windows overlapping by one header so a header on a boundary is still found. OBJ_MAGIC also occurs inside payloads, so a candidate is accepted only when its header parses, describes an object of at most MAX_DATA_SIZE bytes that fits into the pack, and validate confirms it.
• resync_validator builds validate from the key: it parses the candidate's metadata slot, whose tag is computed over the header's magic, version and chunk id as well. That confirms a chunk id from the header plus a few hundred bytes.
• Reading the key is what --repair does before the rebuild, with manifest_only=True (self.chunks is only built afterwards). A repair that cannot read the manifest has no key, warns and walks without resyncing.
• docs/internals/packs.rst updated to describe the validation and the repair scan.

Rebased on master. The none-*/authenticated-* modes from #10095 changed what this needs: every mode now covers the object header by the metadata slot's AAD, so parse_meta decides for all of them and the validate.needs_data branch I asked about above is gone (second commit). In the none-* modes that tag is an unkeyed checksum, so the scan accepts any well-formed object, including one a backed up file contains - those modes have no secret, so reading the payload and checking chunk_id == id_hash(content) would not separate the two either.

Tested in repository_test, cache_test and check_cmd_test.

[codecov]

Codecov Report

❌ Patch coverage is 95.08197% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.05%. Comparing base (abd9c3d) to head (ff0f7a0).
⚠️ Report is 11 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/borg/repository.py	93.33%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10094      +/-   ##
==========================================
- Coverage   87.05%   87.05%   -0.01%     
==========================================
  Files         101      101              
  Lines       17742    17846     +104     
  Branches     2680     2702      +22     
==========================================
+ Hits        15445    15535      +90     
- Misses       1597     1609      +12     
- Partials      700      702       +2     

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

[mr-raj12]

Does #10092 (comment) apply here too?

validate.needs_data is what it costs: AEAD keys authenticate a candidate from header+meta, the other keys read the whole object and get the id check from parse(). Removing it drops that attribute, makes _find_header always slice hdr_size + meta_size, and inverts one test. Four lines.

The consequence is in authenticated mode, where an object stored verbatim inside a backed up file looks exactly like a real one, so the scan indexes it. If its chunk id matches an object living elsewhere in the repo, the rebuilt index points into the file and the real object becomes unreachable. check --repair is an unfortunate place for that, though it is only modes you want gone anyway.

Either is fine by me, just say which.

[mr-raj12]

Answered by #10095: every mode now covers the object header by the metadata slot's AAD, so parse_meta decides for all of them and needs_data is gone (second commit, body updated).

The none-* modes keep the weaker guarantee either way - their tag is an unkeyed checksum and their chunk ids are unkeyed too, so the scan takes any well-formed object, including one inside a backed up file.

Rebased on master, the cache.py conflict was fragments_only and validate landing on the same signature.

[ThomasWaldmann]

Reviewed this with a focus on the resync logic — nice work overall. One gap:

A corrupted size field that stays within the pack still loses intact objects.

The tests cover a broken magic and a data_size pointing past the pack end — both are detected at the corrupt header itself, so _find_header starts scanning at the right place. But when meta_size/data_size are corrupted to a value that keeps the object inside the pack, the header is structurally valid: the walk yields a bogus entry, jumps to the misaligned offset, and only detects the corruption there. _find_header(offset + 1, ...) then scans forward from that misaligned point, so every intact object between the corrupt object's true end and the landing point of the bad jump is silently lost.

Repro (3-object pack, obj1's data_size inflated so the jump lands 60 bytes into obj3):

obj1 = bytearray(fchunk(b"A" * 100, meta=b"m1", chunk_id=H(1)))
obj2 = fchunk(b"B" * 100, meta=b"m2", chunk_id=H(2))
obj3 = fchunk(b"C" * 100, meta=b"m3", chunk_id=H(3))
obj1[45:49] = struct.pack("<I", 100 + len(obj2) + 60)
pack = bytes(obj1) + obj2 + obj3
list(PackReader(pack_contents=pack).iter_headers(validate=accept_all))
# pack <no id>: invalid object header at offset 362 and none after it, skipping the remaining 91 bytes.
# [(H(1), 0, 362)]   <- obj2 and obj3 both lost, although they are intact

Scanning from just past the last accepted header instead finds obj2 at its correct offset (151).

Suggestion: on resync, scan from just after the last accepted object's header rather than from the misaligned offset (or equivalently, only trust a jump once the header at its landing point checks out — lookahead by one). Trade-offs to weigh: it re-reads up to one object's payload per resync, and the index can end up with the bogus wrong-size entry overlapping the recovered ones — harmless, since reading it fails either way (the chunk is unrecoverable without its size field), but worth a code comment.

Either way, a test for the within-pack shape would be good: it behaves qualitatively differently from the past-pack-end shape the tests already cover.