upstream/borgbackup
1
0
Fork 0
mirror of https://github.com/borgbackup/borg.git synced 2026-07-15 13:06:55 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Roll back chunks on read errors after chunking (avoid bad refcounts)

Browse source 
A BackupOSError raised after a file's chunks were already added and
referenced - while re-fstat-ing the file (fstat2) or reading its
extended attributes (bsdflags / xattrs / ACLs) - left those chunk
references in place. With the per-file retry loop this leaks refcounts
on every failed attempt (up to MAX_RETRIES times), which is exactly the
flaky-media scenario retries target.

Factor the existing decref-and-clear into a rollback_chunks() helper and
call it from these two additional abort paths. Decref'ing one reference
per id in item.chunks is correct for a cache hit, a freshly chunked file,
and (leaving committed part file references intact) a file with part
files.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Thomas Waldmann 2026-07-02 01:00:34 +02:00
parent 4e13f7fc5f
commit 396b8cd09a
No known key found for this signature in database
GPG key ID: 243ACFA951F78E01
2 changed files with 75 additions and 7 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

33
src/borg/archive.py
View file

@ -1513,6 +1513,19 @@ class FilesystemObjectProcessors:
# so it can be extracted / accessed in FUSE mount like a regular file.
# this needs to be done early, so that part files also get the patched mode.
item.mode = stat.S_IFREG | stat.S_IMODE(item.mode)
def rollback_chunks():
# roll back the chunk references we added for the (still uncommitted) complete file item.
# a read error can happen after we already added and referenced chunks (e.g. while reading
# xattrs/flags or re-fstat-ing the file). if we then abort (and maybe retry) the file, those
# references would leak (inflated refcount / orphan chunk). decref one reference per id in
# item.chunks: for a cache hit this undoes the incref above, for a freshly chunked file it
# undoes the add_chunk reference, and if part files were written it undoes the extra
# "complete file" incref while leaving the (committed) part file references intact.
for chunk in item.get('chunks', []):
cache.chunk_decref(chunk.id, self.stats)
item.chunks = []
if not hardlinked or hardlink_master:
if not is_special_file:
hashed_path = safe_encode(os.path.join(self.cwd, path))
@ -1556,8 +1569,12 @@ class FilesystemObjectProcessors:
if is_win32:
changed_while_backup = False # TODO
else:
with backup_io('fstat2'):
st2 = os.fstat(fd)
try:
with backup_io('fstat2'):
st2 = os.fstat(fd)
except BackupOSError:
rollback_chunks()
raise
# special files:
# - fifos change naturally, because they are fed from the other side. no problem.
# - blk/chr devices don't change ctime anyway.
@ -1581,9 +1598,7 @@ class FilesystemObjectProcessors:
# not the last try and no part files written yet: trigger a retry by raising,
# hoping that re-reading the file gives us a consistent copy. the retry is
# done by the caller (Archiver._process_any).
for chunk in item.chunks:
cache.chunk_decref(chunk.id, self.stats)
item.chunks = []
rollback_chunks()
raise BackupError('file changed while we read it!')
if not is_special_file and not changed_while_backup:
# we must not memorize special files, because the contents of e.g. a
@ -1592,7 +1607,13 @@ class FilesystemObjectProcessors:
# changed while we backed it up.
cache.memorize_file(hashed_path, path_hash, st, [c.id for c in item.chunks])
self.stats.nfiles += 1
item.update(self.metadata_collector.stat_ext_attrs(st, path, fd=fd))
try:
item.update(self.metadata_collector.stat_ext_attrs(st, path, fd=fd))
except BackupOSError:
# reading extended attributes (bsdflags / xattrs / ACLs) failed after we already
# referenced the file's chunks - roll them back so a retry does not leak refcounts.
rollback_chunks()
raise
item.get_size(memorize=True)
return status

49
src/borg/testsuite/archiver.py
View file

@ -30,7 +30,7 @@ import pytest
import borg
import borg.helpers.errors
from .. import xattr, helpers, platform
from ..archive import Archive, ChunkBuffer, ChunksProcessor
from ..archive import Archive, ChunkBuffer, ChunksProcessor, MetadataCollector
from ..archiver import Archiver, parse_storage_quota, PURE_PYTHON_MSGPACK_WARNING
from ..cache import Cache, LocalCache
from ..chunker import has_seek_hole
@ -1325,6 +1325,45 @@ class ArchiverTestCase(ArchiverTestCaseBase):
for chunk_id, refs in item_chunk_refs.items():
assert refcounts[chunk_id] == refs
def test_create_ext_attrs_error_retry_rolls_back_chunks(self):
# a read error that happens AFTER chunking (here: while reading the extended attributes) must
# also roll back the chunk increfs, so a retry does not end up with bad (too high) refcounts.
# the chunker reads the file fine; the failure is injected into stat_ext_attrs instead.
from collections import Counter
chunk_size = 1000
# distinct content per block, so each block maps to its own (unique) chunk id:
self.create_regular_file('file', contents=b'A' * chunk_size + b'B' * chunk_size)
self.cmd('init', '--encryption=repokey', self.repository_location)
orig_stat_ext_attrs = MetadataCollector.stat_ext_attrs
failed = []
def failing_stat_ext_attrs(self, st, path, fd=None):
# fail exactly once, on the first regular file we read the extended attributes for:
if stat.S_ISREG(st.st_mode) and not failed:
failed.append(True)
raise borg.helpers.errors.BackupOSError(
'extended stat (xattrs)', OSError(errno.EIO, 'simulated I/O error', path))
return orig_stat_ext_attrs(self, st, path, fd=fd)
with patch.object(MetadataCollector, 'stat_ext_attrs', failing_stat_ext_attrs):
out = self.cmd('create', f'--chunker-params=fixed,{chunk_size}', '--list',
self.repository_location + '::test', 'input')
assert 'retry: 1 of ' in out
assert failed # the error was actually injected
assert 'E input/file' not in out # it was backed up ok on the retry
with Repository(self.repository_path, exclusive=True) as repository:
manifest, key = Manifest.load(repository, Manifest.NO_OPERATION_CHECK)
archive = Archive(repository, key, manifest, 'test')
item = [item for item in archive.iter_items() if item.path == 'input/file'][0]
item_chunk_refs = Counter(chunk.id for chunk in item.chunks)
with Cache(repository, key, manifest) as cache:
refcounts = {id: entry.refcount for id, entry in cache.chunks.iteritems()}
# a leaked chunk from the failed attempt would show up here as a too high refcount:
for chunk_id, refs in item_chunk_refs.items():
assert refcounts[chunk_id] == refs
def test_create_erroneous_file_with_part_files(self):
# if we have already written part files (checkpoints) for a file, a later read error must
# NOT trigger a retry: re-reading the file from the start would create duplicate / inconsistent
@ -4560,6 +4599,10 @@ class ArchiverTestCaseBinary(ArchiverTestCase):
def test_create_erroneous_file_with_part_files(self):
pass
@unittest.skip('patches objects')
def test_create_ext_attrs_error_retry_rolls_back_chunks(self):
pass
@unittest.skip('test_basic_functionality seems incompatible with fakeroot and/or the binary.')
def test_basic_functionality(self):
pass
@ -5024,6 +5067,10 @@ class RemoteArchiverTestCase(ArchiverTestCase):
def test_create_erroneous_file_read_retry_rolls_back_chunks(self):
pass
@unittest.skip('only works locally') # inspects cache chunk refcounts via a local Cache/Repository
def test_create_ext_attrs_error_retry_rolls_back_chunks(self):
pass
@unittest.skip('only works locally')
def test_config(self):
pass