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, BackendConnectionError or ReadRangeError while reading. Other store backend errors propagate, 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) and Repository.PermissionDenied unless the repo permissions allow compaction. 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.74%. Comparing base (26ff43f) to head (296c9a0).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10419      +/-   ##
==========================================
+ Coverage   88.68%   88.74%   +0.06%     
==========================================
  Files         103      103              
  Lines       19305    19387      +82     
  Branches     3005     3023      +18     
==========================================
+ Hits        17120    17205      +85     
+ Misses       1515     1514       -1     
+ Partials      670      668       -2     

☔ 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

[ThomasWaldmann]

Trying to rebase and finish this now.

[mr-raj12]

Thanks for the rebase. Narrowed the catch in 82c31cc: only OSError, BackendConnectionError and ReadRangeError give SALVAGE_READ_ERROR. Other backend errors (BackendMustBeOpen, PermissionDenied, QuotaExceeded, ...) now propagate, since they are caller or config errors, not failed reads.

Docstring and test fixes in 296c9a0: a test for objects the header walk skips, the metadata slot flip now hits the tag, documented the index update contract and before_old_pack_delete timing.

[ThomasWaldmann]

can i merge after ci is green?

[mr-raj12]

yes, please. I pushed 296c9a0 after your question (only docstring and test fixes), so CI restarted on it. Nothing else planned here, wiring it into check --repair will be a separate PR.