borgbackup / borgbackup/borg

check --repair: share one object header parser, stop re-validating unchanged packs

Open
#10,318 3 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
13.7k
Forks
875
Avg merge
11h 15m
Merged PRs (30d)
192

Description

Two follow-ups from the review of #10094 (see #8476). Both are about object headers in pack files and touch the same code, so they are filed together, but they can be done independently.

## 1. Object header parsing exists in five copies

The check "does this buffer start with a valid object header" (`OBJ_MAGIC`, a version in `SUPPORTED_OBJ_VERSIONS`, sizes that fit) is written out five times:

- `RepoObj.extract_crypted_data`, `repoobj.py:94`
- `RepoObj.parse_meta`, `repoobj.py:168`
- `RepoObj.parse`, `repoobj.py:197`
- `PackReader._parse_header`, `repository.py:381`
- the gap walk in `superseded_gap_ranges`, `repository.py:556`, which unpacks the header inline

The gap walk is not just a duplicate, it is a weaker one: it checks the magic and that the object
ends inside the gap, and nothing else. No version check, no `MAX_DATA_SIZE` check. So it accepts
headers `_parse_header` rejects.

The wording has already drifted: a wrong magic is "invalid object magic" in `repoobj.py` and "no object header" in `_parse_header`. A sixth copy is one new caller away.

Proposal: one `RepoObj.parse_header(buf)` classmethod that returns either the `ObjHeader` or a name for the problem it found. The `RepoObj` methods raise `IntegrityError` from that name, `_parse_header` adds only what is specific to a pack (does the object fit into this pack, is it within `MAX_DATA_SIZE`), and the gap walk calls it instead of unpacking by hand.

Refactoring only. The one visible change is that the error strings stop disagreeing.

## 2. `finish()` validates every pack a second time

Under `--repair` with `chunks_modified` set, `ArchiveChecker.finish()` (`archive.py:2747`) rebuilds the chunks index again using the same validator `check()` already ran. Per object that costs a metadata slot read of up to 1 KiB plus one decryption, where the second pass used to read the 49 byte header and nothing else. It runs over every pack, although the only packs whose contents changed are those that repair rewrote while deleting a defect chunk.

Proposal: remember which packs the first walk validated cleanly, or which packs repair rewrote (the repointed index entries name them), and re-walk only those with `validate`. For the untouched packs, reuse the entries already in `self.chunks` rather than reading them off the store again.

This one deserves a measurement on a repository with many packs, before and after, so the improvement is a number rather than a guess.

Contributor guide

Open the contributing guide

Research direction

Start with the five object-header parsing sites in repoobj.py and repository.py, then read ArchiveChecker.finish() in archive.py alongside the existing check() walk. Confirm how shared parsing can preserve the pack-specific checks, and how chunks records identify packs that need revalidation. Done means the duplicate validation paths are consolidated, unchanged packs are reused under --repair, and the repository-scale measurement shows the effect.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend, performance
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.