repository: add salvage_pack, keeping only the authenticated objects of a corrupt pack, refs #10026

[mr-raj12]

Description

Adds Repository.salvage_pack and repoobj.whole_object_authenticator. Not wired into check --repair yet, refs #10026.

salvage_pack replaces a pack whose content no longer matches its store hash name by a pack holding only the objects that authenticate (tags of both slots verified), then updates the chunk index (self.chunks unless chunks is given):

• entries of the pack are pointed at the kept object, or at a kept copy of the same id, or removed
• kept objects the index does not list get an entry (F_USED, size 0)
• entries of other packs and F_PENDING entries stay as they are

The store and the index change only for SALVAGE_DONE. Other outcomes:

• SALVAGE_INTACT: the store hash matches the name
• SALVAGE_READS_DIFFER: the loaded bytes hash to the name although the store hash did not, or a second load differs from the first
• SALVAGE_NOTHING_AUTHENTICATES: no object authenticates
• SALVAGE_READ_ERROR: OSError or a store backend error while reading; a missing pack raises StoreObjectNotFound

All reads happen before the first store change. The order of changes is: store the new pack, call before_old_pack_delete, update the index, delete the old pack. If the kept bytes are the undamaged pack, the new pack has the old name, so before_old_pack_delete and the delete are skipped.

salvage_pack raises Error with BORG_STORE_CACHE (two loads may return the same cached copy). whole_object_authenticator raises Error for an authenticated-* key with BORG_WORKAROUNDS=authenticated_no_key (tags not verified).

The check log message for a gap object failing object_validator is now "object header or metadata does not authenticate".

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 Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.41%. Comparing base (b2de6a9) to head (88e5b04).
⚠️ Report is 5 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10419      +/-   ##
==========================================
+ Coverage   88.39%   88.41%   +0.02%     
==========================================
  Files         103      103              
  Lines       19061    19163     +102     
  Branches     2968     2988      +20     
==========================================
+ Hits        16849    16943      +94     
- Misses       1540     1548       +8     
  Partials      672      672              

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

[ThomasWaldmann]

check the names, some are a bit off.

[ThomasWaldmann]

Review of fd40682b. The 21 new tests pass locally. Items 1 and 2 should be fixed before this gets wired into check --repair; the rest is naming and polish.

1. Read errors from non-local backends are not caught as SALVAGE_READ_ERROR

salvage_pack catches only OSError. For rest:// (the ssh:// transport), sftp and s3, a read failure surfaces as borgstore's BackendError / BackendConnectionError, which are not OSError subclasses. The rest backend turns a server-side I/O error into BackendError(response.text). Nothing gets corrupted, since all reads happen before the first write, but the caller gets an exception instead of a status. Once this is wired into check --repair, one unreadable pack would abort the whole repair on a remote repo, while a local repo just skips the pack. Suggestion: also catch StoreBackendError, but let StoreObjectNotFound (a subclass of it) through, as the docstring promises.

2. chunks=None means the opposite of what it means in the sibling methods

In compact_pack and transform_pack, chunks=None means "use self.chunks". Here it means "do not update the index", and the old pack is still deleted. Every self.chunks entry for the pack is then left pointing at a deleted pack. test_salvage_pack_without_chunks_leaves_the_index_alone enshrines that. Please either follow the siblings (None -> self.chunks) or drop the None option. If a caller really needs "no index update", make it an explicit flag.

3. before_old_pack_delete is called when nothing is deleted

When the kept bytes hash to pack_id (e.g. the damage is appended bytes), store_store overwrites the old pack and the delete is skipped, but the callback is still called. compact_pack calls it only if new_pack_id != pack_id. The effect is harmless (at worst an unneeded "chunk index invalid" marker), but the name and the sibling's behaviour say otherwise. Either match compact_pack or pick a name that fits. No test covers the callback in the same-id case.

4. The authenticated_no_key refusal is in the wrong layer

repository.py now imports crypto.key to read AUTHENTICATED_NO_KEY. Apart from that, the repository layer never deals with keys. repoobj.py already has its own AUTHENTICATED_NO_KEY. Having object_authenticator() raise under the workaround would protect every future caller, keep salvage_pack key-agnostic, and remove the docstring paragraph that describes the "accepts any object" behaviour. Also, the workaround only affects the authenticated-* key classes, so refusing for every mode is broader than needed (harmless, though).

5. Names

• pack_store_cache is a bool but reads like the cache itself. Something like uses_pack_store_cache would be clearer.
• SALVAGE_UNSTABLE also covers "the loaded bytes hash to the pack's name", which is not instability. Use a separate status, or a name that covers both cases.
• validate vs. authenticate: when object_validator (header + metadata slot) fails, iter_headers already logs "object does not authenticate". Now object_authenticator is the whole-object check. The naming should make the header+meta vs. whole-object difference clear.
• Test helpers: offsets_end returns the position of an object's last byte, not an end offset. store_salvage_pack stores a pack and damages it; it does not salvage anything.

6. Nits

• Do the store_hash(pack_contents).digest() == pack_id check right after the first load. As it is, it runs after the authenticate loop (wasted work), and an intact read where nothing authenticates reports SALVAGE_NOTHING_AUTHENTICATES.
• On posixfs, the second load will usually come from the page cache, so it catches in-memory or transfer corruption rather than flaky media. The docstring could say what the second read actually guards against.
• test_salvage_pack_refuses_without_write_permission does not assert that the store and the index are unchanged.

What looks good

• The order of changes (store the new pack, callback, index update, delete the old pack) is crash-safe.
• The index update handles duplicate ids and F_PENDING entries correctly.
• Adding unindexed objects as F_USED with size 0 follows the precedent in archive.py.
• The _pack_cache entry is dropped even when the new pack gets the old name.

---

Generated by Claude Code

[mr-raj12]

All 6 items addressed: 3 in 3ffd8ed, the rest (backend errors, chunks default, refusal moved to whole_object_authenticator, renames, nits) in 88e5b04.

[ThomasWaldmann]

Found another nit:

except (OSError, StoreBackendError) also swallows borgstore's PermissionDenied and BackendMustBeOpen. BackendMustBeOpen means a bug in the calling code, but it would now show up as SALVAGE_READ_ERROR with only a warning.

Can you catch more specific exception classes? Also: please rebase.

[ThomasWaldmann]

ping @mr-raj12

[ThomasWaldmann]

ping @mr-raj12