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=...) validates every header it walks and, when one fails, scans for the next object that validates and resumes there, so the objects after the damaged region are still indexed.

Without a validate function (repository-only check, routine rebuild) a header that does not parse still 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. It returns the header it accepted, so the walk resumes at that object without reading and validating it a second time.
• object_validator builds validate from the key: it parses the candidate's metadata slot, whose tag covers the slot itself and the chunk id, and at object version 0x02 the header's magic and version as well. At version 0x01 those two are not in the tag: a wrong magic fails the explicit magic check, and a wrong version fails because the version selects the AAD the slot is parsed with. data_size, the one header field outside the tag at either version, must equal csize - the data slot's payload size, recorded in the tagged metadata - plus the key's fixed PAYLOAD_OVERHEAD. So the whole header is confirmed from a few hundred bytes.
• Every walked header is validated, not only the candidates the scan finds. A meta_size or data_size corrupted to a value that keeps the object inside the pack is therefore caught at that header, rather than at the misaligned offset it points to, and the intact objects in between are kept. The object with the failed header is dropped: its id, its extent or its metadata is wrong, so it can not be read back.
• 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.
• ArchiveChecker.finish() rebuilds the index from the packs once more when repair changed them (borg2 check: implement repository repair (index and pack rebuild) #10026), so it passes the same validator.
• _parse_header names which check a header failed, so the raise and the resync warning report no object header, unsupported object version N, object extends past end of file or object of N bytes exceeds the maximum of M. The first and third are the messages master reports for those cases.
• A metadata slot larger than MAX_VALIDATED_META_SIZE (64 KiB) is rejected without being fetched, reporting its size rather than a failed authentication. A real slot is a few dozen bytes, so this only bounds what a corrupt meta_size can make the walk or the scan read.
• The walk stays inside the pack: it stops once fewer than a header's bytes are left, and never asks for bytes past the pack end, the way _find_header and _get_object already do.
• The walk rebuilds the index; the damaged bytes stay in the pack as a gap no index entry covers. A pack is named by the sha256 of its content, so a pack damaged in the store keeps failing the store-level check borg check runs over packs/, also after a repair has rebuilt the index from it. Rewriting such a pack is repository-level repair, borg2 check: implement repository repair (index and pack rebuild) #10026.
• Repository.check rebuilds a corrupt index from the packs without a validator: validating needs the key, which a Repository does not have. It is reached only with pack_errors == 0, and a pack damaged in the store fails the sha256 check before that, so what still reaches it is a pack matching its name whose object header is bad anyway. There iter_headers raises, borg2 check: implement repository repair (index and pack rebuild) #10026.
• docs/internals/packs.rst updated to describe the validation and the repair scan.

Behaviour change without a validator: iter_headers() now also raises on an unsupported object version and on an object larger than MAX_DATA_SIZE. master checked the magic and the pack bound only.

In the none-* modes the metadata tag is an unkeyed checksum, so the walk accepts any well-formed blob, including one a backed up file contains. The authenticated-* modes accept a blob written with the key the repository uses: their tag is deterministic and binds a blob to its chunk id alone, so a blob copied verbatim out of a repository sharing that key validates at any offset in any pack. Backing up such a repository puts its blobs into the payloads, as long as compression leaves them as they are.

A blob that arrives this way reads back as itself, so indexing it adds a chunk nothing references. Its data_size describes the blob as it was written, though, and chunking cuts a payload where the content dictates: a blob whose header and metadata slot fall inside the payload and whose data is cut off still validates, and the extent it claims covers the blobs that follow it, which the walk then skips. Bytes crafted to pass an unkeyed checksum claim an extent the same way. The scan reaches a payload only after the blob owning it failed to validate, so a corrupt header is what makes any of this reachable.

Known cost: the validating walk reads 1 KiB per object where the plain walk reads a 49-byte header, so a pack of objects smaller than that is read several times over. Left as a TODO - a window buffer, like the one _find_header scans with, would read each byte once, but reading a window to skip a multi-megabyte object costs more than a short read per header.

Not covered: a resync drops an object, and ArchiveChecker does not count that as an error, so a repair that drops an object no archive references still ends with "no problems found".

Merges cleanly with master. Tested in repository_test, cache_test and check_cmd_test.

[codecov]

Codecov Report

❌ Patch coverage is 88.42105% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.50%. Comparing base (29df5b6) to head (c7e6123).
⚠️ Report is 159 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/borg/archive.py	78.78%	6 Missing and 1 partial ⚠️
src/borg/repository.py	93.44%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10094      +/-   ##
==========================================
+ Coverage   87.18%   87.50%   +0.31%     
==========================================
  Files         102      103       +1     
  Lines       18372    18607     +235     
  Branches     2824     2857      +33     
==========================================
+ Hits        16018    16282     +264     
+ Misses       1645     1626      -19     
+ Partials      709      699      -10     

☔ 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.

[mr-raj12]

Fixed as suggested: the scan now starts just past the last accepted header, with a test for the within-pack shape, the wrong-size entry is dropped rather than emitted, since check_pack_objects treats overlapping index entries as index corruption and compact would abort on that pack.

[ThomasWaldmann]

By Claude:

Re-reviewed at ab1762f — the third commit fixes the shape I reported (a data_size inflated into a later object: obj2+obj3 are recovered now), tests and lint are green locally, and the red CI jobs are unrelated (pypy memoryview slice assignment; argon2-cffi-bindings cmake builds on the BSD/illumos VMs).

But while probing the neighbouring shapes I found that the look-back heuristic does not cover the class it is meant for, and one case got worse — sorry, that one follows from my own suggestion. Three-object pack, 100-byte payloads, none-* key, no compression:

shape	master	this PR @ d196c0a	this PR @ ab1762f
A data_size inflated, lands inside obj3	aborts	obj2+obj3 lost	fixed
B data_size shrunk	aborts	obj1 kept with a wrong size	same
C data_size inflated, lands exactly on obj3's header	obj2 silently lost	same	same
D intact decoy object inside obj1's payload (none-*/authenticated-*), obj2's header broken	aborts	correct	obj1 (intact) dropped, decoy indexed
E obj2's header broken	aborts	correct	correct

B keeps an entry that can not be read back (and --verify-data is the only thing that removes it), C loses an intact object without a word, and D evicts an intact object because the scan now starts inside the last accepted payload. All three come from the same root: the walk trusts a header's sizes, and then tries to notice a wrong one indirectly — from where the jump lands, or from what the scan finds. That is inherently a heuristic: a wrong size that lands on a valid header, or a scan that finds something plausible, fools it.

It does not have to be a heuristic, though: the sizes are authenticated, just not in the header. parse_meta already verifies the metadata slot's tag over header AAD + slot, so a wrong meta_size fails it. And the slot carries csize, the data slot's payload size (every compressor records it, ObfuscateSize included, and RepoObj.parse already relies on it), while each key family's envelope adds a fixed number of bytes (MACKeyBase.PAYLOAD_OVERHEAD = 34, AEADKeyBase.PAYLOAD_OVERHEAD = 48). So data_size == csize + key.PAYLOAD_OVERHEAD is a complete, authenticated check of the one header field the tag does not cover — I verified it holds for none-sha256, authenticated, chpo and aes-ocb.

That suggests a simpler and stronger walk: validate every header the repair walk accepts, not only resync candidates — read hdr + 1024 bytes per object (the meta slot is ~80–95 bytes, so it is the same single range read the header costs anyway; _get_object uses the same 1024-byte guess), call validate(chunk_id, obj) on it, and treat a header that fails exactly like a corrupt one: resync from offset + 1. resync_validator gets the csize comparison. Then:

• A, B, C, plus a corrupted meta_size or chunk_id, are all caught at the corrupt object, which is dropped (it could not be read back anyway); the objects after it are recovered from their true offsets. No jumping to wherever a wrong size points, so no pending/look-back/overlap-drop machinery — the walk goes back to commit 2's shape.
• D disappears, because the scan never enters a payload whose object validated; the none-* decoy limitation shrinks to "inside an object that already failed".
• Cost: one tag check per object, 2–4 µs (measured), only on check --repair, which reads every header anyway.
• What stays is what a header walk can not see: bit flips inside the data (caught on read / --verify-data), and none-* decoys inside a failed object's extent.

I prototyped it on top of ab1762f to make sure it holds up: repository.py gets shorter than it is now, all PR tests pass, and I added tests with real none-* objects for the three data_size shapes, a wrong meta_size, a wrong chunk_id, the decoy-inside-a-validated-object case, a validator check across all four key families, and the end-to-end check --repair test parametrized over magic vs. data_size damage (asserting the damaged chunk is gone from the rebuilt index and the next one is still at its offset). Against the current branch those new tests fail exactly as the table predicts. The patch, on top of ab1762f and deliberately not a commit, so you can take it apart as you like: https://gist.github.com/ThomasWaldmann/2043b43ea560d0f33f68b700f18dd066 — curl -sL https://gist.githubusercontent.com/ThomasWaldmann/2043b43ea560d0f33f68b700f18dd066/raw/e599be3c8c090c7360963630b2c8f0ab4c1f9e07/10094-validate-every-header.patch | git apply.

Two small things from last time still apply: cache_test.py has two separate from ..helpers import lines, and with the bound in _parse_header the walk rejects oversize headers too (the patch does that).

[ThomasWaldmann]

review by claude fable 5

Head reviewed: 90fecfc48 (pushed 2026-08-23 ~14:14Z, no accompanying comment), 5 commits on base 775ec8aac (master just after the 2.0.0b23 release prep). Previous reviewed head: c3b9ec0c4 (3 commits).

Commit structure (git range-diff old→new)

#	commit	status
1	a21d85554 resync past corrupt object headers when rebuilding the chunks index	unchanged (= c0362555b)
2	90c4c0771 validate a resync candidate from its metadata slot alone	unchanged (= 81c522cb1)
3	46162be7f validate every object header the repair walk accepts	new — replaces the look-back commit; adopts the gist patch
4	f2b5d0e71 report a failed object validation apart from a corrupt header	new
5	90fecfc48 report which header check failed and cap the metadata read	new

Full-PR diffstat: 7 files, +546/−38 (was +406/−31).

Commit 3 — the adoption

Content-wise this is the posted patch: the walk validates every header it accepts (reading the metadata slot along with it), a failed header triggers a resync from offset + 1, the pending/look-back/overlap-drop machinery of the old third commit is gone, _parse_header carries the MAX_DATA_SIZE bound, and the validator adds the data_size == csize + key.PAYLOAD_OVERHEAD check. Differences from the patch as posted:

1. resync_validator → object_validator — better name, it validates every walked object now, not only resync candidates.
2. csize lookup moved inside the try: a meta slot without csize now fails validation (object dropped) instead of raising KeyError. Reviewed and accepted: RepoObj.parse reads meta["csize"] unconditionally, so an object without it is unreadable by borg anyway, and no borg2 repo has such objects (every compressor path sets it). The theoretical bad case — a whole pack of csize-less objects being silently skipped — requires objects current borg could not read at all.

Commit 4 — distinct failure reporting

Validation failures now report as object does not authenticate, distinct from structural header problems. Warnings and the IntegrityError name the actual problem instead of a generic "invalid object header".

Commit 5 — named problems + bounded reads

• _parse_header returns (hdr, problem); problems: no object header, unsupported object version N, object extends past end of file, object of N bytes exceeds the maximum of M. The first and third match master's existing messages. Check order puts the pack-bound test before the max-size test, so small-pack cases keep master's wording.
• New shared helper _validates(hdr, offset, buf, buf_offset, validate) used by both the walk and _find_header — replaces my duplicated slice-or-reread logic. Index math verified for both call sites (walk: start = 0; scan: start = pos).
• MAX_VALIDATED_META_SIZE = 64 KiB: a header claiming a bigger meta slot fails validation without the slot being fetched. This closes a small hole in my patch: a corrupt-but-parseable meta_size (or a payload false-magic in the scan) could previously trigger reads up to ~20 MiB per candidate. Genuine slots are ~80–95 bytes, so 64 KiB is generous; the constant is documented in packs.rst and trivially bumpable.
• META_READ_SIZE = 1024 replaces my VALIDATE_READ_SIZE and is now also used by _get_object's meta read (consolidating the literal 1024 that was already there).
• Honest TODO in iter_headers + PR body: a validating walk reads 1 KiB per object, so packs of tiny objects get read several times over; a window buffer would fix it but costs more for large objects. Correctly deferred.

Behavior change, documented

Without a validator, iter_headers() now also raises on an unsupported object version and on an object over MAX_DATA_SIZE (master checked magic + pack bound only). The body states this explicitly. Same behavior as my patch, but they documented it.

Body / docs

• The old "Not covered: a size corrupted to land exactly on the pack end" paragraph is gone — correctly: with the csize check, that shape is now caught at the header.
• packs.rst claim "indexing a none-* decoy is harmless" — verified: a decoy only gets indexed when its enclosing object failed validation (so that object is dropped → no overlapping index entries → check_pack_objects stays happy for compact/merge/undelete), it is contained in the dropped extent, and it reads back as itself (own chunk id in header + AAD, id check passes). It becomes a spurious but valid chunk.
• The none-* limitation paragraph now also notes the scan reaches a payload only after the blob owning it failed to validate — the D-shape fix, stated.

Tests

All tests from the patch kept (validator name updated), plus improvements:

• test_repair_resyncs_pack_with_corrupt_object_header[magic|data_size] (end-to-end, real repo): my random corrupt() bit-flip on the size field replaced by a deterministic data_size + 16 (always parseable, always in-pack), and it now asserts the exact problem string per damage kind, that the damaged chunk is gone from the rebuilt index, and that the next object is still indexed at its offset.
• New test_pack_reader_rejects_an_object_over_max_data_size.
• New test_pack_reader_does_not_fetch_an_oversized_metadata_slot — monkeypatches reader.read and validate to pytest.fail if called: genuinely proves the 64 KiB cap short-circuits before any I/O.
• Decoy test renamed test_pack_reader_resync_starts_past_the_last_validated_object.
• The round-2 nit (two from ..helpers import lines in cache_test.py) is fixed as a side effect.

Verification performed

• Probe (real none-* objects, real object_validator) on 90fecfc48 — all five shapes correct:
  A size→into obj3: [obj2, obj3] · B size shrunk: [obj2, obj3] · C size lands exactly on obj3's header: [obj2, obj3] · D decoy in obj1's payload + obj2 broken: [obj1, obj3] · E obj2 header broken: [obj1, obj3].
• 267 passed, 2 skipped locally (repository_test, cache_test, check_cmd_test, repoobj_test; worktree borg-10094, branch pr-10094-review-v4, absolute PYTHONPATH).
• CI fully green (only pypi/release skipped). Codecov 97.50 % patch coverage; 2 lines in repository.py uncovered (1 missing + 1 partial) — not chased, non-blocking.
• Rebase safety: new base 775ec8aac; the pyx churn since the previous base (incl. rabin_aes/toeplitz_aes chunkers) is not exercised by these test modules; copied .sos valid for them.

Remaining, all non-blocking

1. Codecov's 2 uncovered lines in repository.py.
2. Author's TODO: window-buffered validating walk for small-object packs.
3. Round-1 note still true: with BORG_WORKAROUNDS=authenticated_no_key the tag verify is skipped, so validation degrades to structural + csize checks — pre-existing workaround semantics, unchanged by this PR.

Verdict

Approve. The adoption is faithful, the additions are genuine improvements (bounded reads, precise diagnostics, stronger tests), the one deviation is sound, and the docs/body are accurate about what is and is not covered. Ready for maintainer review/merge.

[ThomasWaldmann]

@mr-raj12 Some small stuff, we are getting closer to merging. :-)

Is IRC working again for you?

[mr-raj12]

@mr-raj12 Some small stuff, we are getting closer to merging. :-)

Is IRC working again for you?

Fixing and updating reviews soon, got in with a new username now it reconnects frequently but works somehow , checking that as well

[mr-raj12]

@mr-raj12 Some small stuff, we are getting closer to merging. :-)

Is IRC working again for you?

[ThomasWaldmann]

@mr-raj12 You can find my mobile phone number on my company home page and use it with signal messengar.