A concurrent repack_chunkindex (another client closing its cache, under a
shared lock) stores a merged replacement fragment and then deletes the small
source fragments. A reader that listed index/* before the replacement existed
and loaded the fragments after the deletion would silently skip the vanished
fragment and return a partially merged chunk index: chunks that exist in the
repo become invisible (spurious ObjectNotFound on read, lost deduplication,
duplicate chunk copies written into new packs).
Make the fragment merge all-or-nothing: if any listed fragment fails to load,
discard the partial merge, re-list and retry - the fresh listing contains the
replacement fragment. If no complete, consistent fragment set can be read
within CHUNKINDEX_MERGE_ATTEMPTS, fall back to the slow (but correct) index
rebuild from the pack headers instead of returning an incomplete index.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The files cache stores each file's chunk list "compressed" to chunk-index
hash-table positions, valid only while the index is unmutated. When a backup
runs out of space and aborts, pending chunks are rolled back out of the
index; a files-cache entry memorized earlier still points at a now-stale
position, so decompress_entry's self.chunks[id] raises KeyError. This crashed
close() while it was cleaning up the already-failed backup, masking the real
ENOSPC error with a "Key not found" traceback.
Guard the two call sites that resolve cached entries against the index, both
mirroring the existing compress_entry KeyError handling in _read_files_cache:
- _write_files_cache drops the unresolvable entry (the file is re-chunked next
backup) instead of raising.
- file_known_and_unchanged treats the file as unknown so it gets re-chunked.
Reproduced on a space-limited ramdisk (borg create -> ENOSPC): the crash is
gone and the genuine "No space left on device" error surfaces instead; the
repo remains consistent (check ok) and recoverable (compact + backup ok).
Add a regression test that memorizes a file, deletes its chunk from the index,
and asserts neither a cache lookup nor the files-cache save raises.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
write_chunkindex_to_repo gated deletion of superseded index fragments on
whether it actually uploaded the replacement (stored_anything). Combined
with repack's force_write=True, every repack re-uploaded byte-identical
merged fragments; and had repack simply dropped force_write, a crash
between store and delete would leave the sources undeleted forever (the
re-derived fragments dedupe-skip, stored_anything stays False, deletion
never runs) -- small fragments piling up and being re-merged on every run.
Gate deletion on new_hashes (the fragment set making up the index just
written) instead. A dedupe-skipped fragment means its content is verifiably
already in the repo, so deleting the superseded fragments is safe; an empty
write (new_hashes empty) still skips deletion, so we never leave the repo
without an index. repack now passes force_write=False, gaining idempotence
with no redundant uploads.
Add a regression test covering the crash/concurrent-repack recovery case.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the unconditional "collapse all index/* fragments into one
all-in-one index" behavior with an entry-count-based repacking policy that
keeps each fragment within [CHUNKINDEX_FRAGMENT_ENTRIES_MIN,
CHUNKINDEX_FRAGMENT_ENTRIES_MAX] entries where possible.
- write_chunkindex_to_repo now always splits its output into fragments of at
most MAX entries (streamed, so memory stays bounded), collecting all new
hashes so delete_other/delete_these never delete a fragment just written.
- repack_chunkindex() merges small (< MIN) fragments, deferring until they can
seal a full fragment (sum >= MIN) or too many piled up (> SMALL_FRAGMENT_CAP);
fragments already in range are left untouched and immutable.
- Wire repack into AdHocWithFilesCache.close() and the build_chunkindex_from_repo
merge>1 branch (which no longer collapses-and-deletes everything).
- list_chunkindex_fragments() estimates a fragment's entry count from its stored
byte size, so fragments can be classified without loading them.
This bounds both fragment size and count, and keeps large fragments stable
(groundwork for a future writethrough cache on the index/ namespace, which an
all-in-one consolidation would invalidate for every client of the repo).
Also:
Make chunk index fragment partitioning deterministic
Sort the selected keys before partitioning them into fragments, so an
identical set of entries always produces an identical fragment set
(identical content hashes), no matter in which order the entries were
inserted into the hash table.
This makes writing/repacking idempotent and convergent across clients:
a fragment that already exists in the repo is not stored again, and no
differently-partitioned duplicates of the same entries can pile up
(previously, two clients repacking the same small fragments could each
produce a different - possibly sealed, thus never cleaned up - merged
fragment).
Sorting cost is negligible on the hot paths: incremental writes and
repacks sort ~100k-200k keys (~0.02s). Only full rewrites (compact,
slow rebuild) sort the whole index, where the sort is dwarfed by the
rest of the operation.
Also:
Don't fail an already-committed backup on repack error
The repack in ChunksMixin.close() runs after the archive and its
incremental chunk-index fragment are durably stored, so it is optional
maintenance. Catch a transient repack failure and warn instead of letting
it propagate out of close() and fail the whole command; the next run
repacks again.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
process_file() ran process_file_chunks() inside `with backup_io("read")`.
That block was meant to guard reading the *source* file, but the source
reads are already guarded individually by backup_io_iter(). The outer
wrapper additionally caught add_chunk()'s *repository* writes, so a critical
repository IO failure -- e.g. the repo running out of space during a pack
flush -- was wrapped into a per-file BackupOSError. Borg then only warned,
skipped the file, and continued, and create_inner() still committed the
archive via archive.save().
The result: `borg create` on an out-of-space repo printed a normal success
summary ("Error files: 0"), exited 0, and committed an archive that
references chunks which were never durably stored. `borg check` afterwards
reports "Missing file chunk detected" and `borg compact` reports "Repository
has N missing objects!" -- silent, unrestorable-backup data loss.
Drop the outer backup_io("read") wrapper. Source reads stay per-file
warnings (backup_io_iter is unchanged); repository OSErrors are now left
unwrapped and therefore critical, aborting create before archive.save()
runs, exactly as the BackupOSError docstring prescribes.
Reproduced on a space-limited macOS ramdisk (source > free space): before,
create exited 0 with a corrupt archive; after, create fails and commits
nothing, and the repository stays consistent across a create/delete/compact
churn matrix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
compact_pack() updated repository.chunks, but compact computes keep/drop from
and persists its own index, which kept pointing at the deleted packs. Also:
tolerate deleting already-gone packs, start rebuilt index entries as unused.
max_count=3 was a testing value. The default is now max_count=None,
max_size=50_000_000; the testsuite sets BORG_PACK_MAX_COUNT=3 instead,
so small test data still produces multiple multi-object packs.
close() persists the files cache but never drops its reference to
self._files, unlike self._chunks which is cleared right after being
saved. For a backup of many files this dict can hold hundreds of MB and
stays alive for as long as the closed cache object does.
Set self._files = None after writing it, mirroring the _chunks handling.
Accessing .files after close() was never supported anyway (close() also
tears down cache_config, which _read_files_cache needs), so this only
frees memory earlier.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AdHocWithFilesCache._chunks is the very same ChunkIndex object the
Repository holds (bound via the .chunks property). On close() we persist
it and clear the table in place (_maybe_write_chunks_cache(clear=True)),
then set self._chunks = None. But that only drops the *cache's* reference:
the Repository still points at the now-empty table and
is_chunk_index_loaded stays True.
A later access to repository.chunks in the same process (e.g.
delete_object doing self.chunks.get(id)) would then see a valid-looking
but empty index and conclude every chunk is gone, instead of rebuilding
from the repository. delete_chunkindex_from_repo() already guards its
analogous case with invalidate_chunk_index(); do the same here.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
get_config/cache/data/runtime_dir now accept *subdirs and ensure_dir the
full path. get_keys_dir and get_security_dir use this instead of
hand-joining "keys"/"security", and the store cache in repository.py uses
get_cache_dir("storecache") instead of its own makedirs.
