compact makes a corrupt pack pass `borg check`

[mr-raj12]

compact_pack() and merge_packs() copy pack bytes into a new pack via store.defrag, and the new pack is named by the hash of what was copied. Neither consults cache/checked-packs or verifies the source pack. A corrupt pack therefore becomes a correctly named pack, and check's prune() drops its corrupt record as "gone".

Reproduced on master (0dd454d), aes256-ocb:

borg create a s3            # s3/keep (200 kB), s3/drop (30 kB), one pack
borg create b s3k           # s3k/keep only, dedups against a
borg delete -a a            # s3/drop's chunk is now unused
# flip one byte in the data slot of keep's chunk
borg check                  # rc 1, corrupt pack
borg compact --threshold 0  # rewrites the pack, keeping keep's chunk
borg check                  # rc 0
borg extract b              # rc 113, s3k/keep: 1 chunk missing or corrupted, replaced by all-zero data

Proposal: compact skips packs recorded corrupt and tells the user to run borg check --repair. A pack that was never checked can still be laundered; verifying the hash of every source pack before copying would close that too, at the cost of reading it once more. Which one do you prefer?

Refs #10026.

[ThomasWaldmann]

Well, we have 2 mechanisms to check:

• whole-pack name vs. content-hash: problematic, see above
• per pack entry cryptographic check (authenticated mode, aead modes): unaffected

Checking the pack content hash via store.hash is relatively cheap IF we have a borg agent on the server.

If we don't have that, it would read the full pack (to verify the content hash) instead just parts of it - maybe no big difference usually if we make sure the pack is then cached and not read again (for the compaction).

[mr-raj12]

The cost differs per command:

repo-compress: free. transform_pack() loads the whole pack anyway and writes the new one client-side,
so store_hash(pack_contents) == pack_id is a hash over bytes we already have.

compact: compact_pack() and merge_packs() copy with store.defrag and read only the pack size and the
gaps. store.hash on top of that is server-side for rest://, a full local read for a local repo, and a
full download for sftp:// - OpenSSH has no check-file extension, so borgstore's hash falls back to
loading the file. sftp has no defrag either, so compact there already downloads the kept spans and
uploads the new pack.

Caching does not save that read: Store.defrag does not use the cache, Store.hash goes to the backend,
and the packs cache is opt-in (BORG_STORE_CACHE).

cache/checked-packs holds store-hash results only, Repository.check() is its only writer. Hashing
before the copy therefore finds the same damage as the record, the record is just the free version for
packs a check already covered. Neither replaces the per-entry check: the store hash says the pack is as it
was written, nothing about the objects in it, and compact never decrypts.

#10412 is the record-based skip. Add the hash check on top - always in repo-compress, before defrag in
compact - in that PR or as a follow-up? And always on, or behind a flag for the backends that pay a full read?

[ThomasWaldmann]

Maybe file a ticket at borgstore, guess there should be some way to query whether some operations are done server-side or not.