create: implement retries for individual fs files (backport of #7351)#9812
Open
ThomasWaldmann wants to merge 7 commits into
Open
create: implement retries for individual fs files (backport of #7351)#9812ThomasWaldmann wants to merge 7 commits into
ThomasWaldmann wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 1.4-maint #9812 +/- ##
=============================================
+ Coverage 82.03% 82.33% +0.29%
=============================================
Files 38 38
Lines 11396 11442 +46
Branches 1797 1804 +7
=============================================
+ Hits 9349 9421 +72
+ Misses 1462 1434 -28
- Partials 585 587 +2 ☔ View full report in Codecov by Harness. |
f5aba18 to
7133066
Compare
afe0eb5 to
e2c4398
Compare
…ckup#7351) Backport of borgbackup#7351 to 1.4-maint. Retry reading a backup source file when it raises a BackupError / BackupOSError (e.g. transient I/O error, or the file changed while we read it): - retry the same file after some sleep time (1ms, growing exponentially up to 10s, 10 tries) - BackupOSError: if retries do not help, skip the file, log it with "E" status - BackupError (changed while reading): last try backs it up, logged with "C" status Works for borg create's builtin fs recursion, --paths-from-command and --paths-from-stdin. 1.4-specific: do NOT retry a file once checkpoint part files (.borg_part_N) have been written for it. Re-reading it from the start would create duplicate / inconsistent part files (concatenating all part files would then no longer yield the complete file). ChunksProcessor.last_part_number tracks this; such a file is kept as-is ("C") or skipped ("E") instead of being retried. Permission errors (EPERM/EACCES) are not retried (they won't get better), so we don't waste ~15s of backoff on every unreadable file. Adds a ChunkerFailing test chunker (chunker-params=fail,block_size,map) to drive the error handling, and tests: - test_create_erroneous_file: file is read on a later retry - test_create_no_permission_file: permission error is not retried, file skipped - test_create_erroneous_file_with_part_files: file with part files is not retried Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Junie <junie@jetbrains.com>
Co-authored-by: Junie <junie@jetbrains.com>
The changed-file retry path already rolls back the chunks it added before re-reading. Do the same when a read error (BackupOSError) interrupts reading a file: the chunks added since the last checkpoint (item.chunks[from_chunk:]) are not referenced by any committed part item, so without rolling them back they leak as an inflated chunk refcount (or, if content also shifted, an orphan chunk). The rollback lives in process_file_chunks, where from_chunk is known: chunks before from_chunk are referenced by part files we already wrote and must be kept; only the uncommitted tail is decref'd. Adds test_create_erroneous_file_read_retry_rolls_back_chunks: the re-read chunks dedup to the same ids, so the leak shows up as a too-high refcount (not an orphan id) - the test checks refcounts directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
test_create_changed_file_retry_rolls_back_chunks and test_create_erroneous_file_read_retry_rolls_back_chunks open a local Cache/Repository to inspect chunk refcounts. In RemoteArchiverTestCase the repo was created via a remote (__testsuite__:) location, so opening it locally triggers the "repository was previously located at ssh://..." relocation prompt, which reads stdin and fails under pytest. These checks are inherently local (they poke at cache internals), so skip them in the remote variant, like test_debug_put_get_delete_obj already does. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
unittest.mock.patch does not reach the borg.exe subprocess, so these two tests would fail under ArchiverTestCaseBinary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
e2c4398 to
396b8cd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #7351 to
1.4-maint.Retry reading a backup source file when it raises a
BackupError/BackupOSError:BackupOSError: if retries don't help, skip the file, log it withEstatusBackupError(changed while reading): last try backs it up, logged withCstatusWorks for
borg create's builtin fs recursion,--paths-from-commandand--paths-from-stdin.1.4-specific care: checkpoint part files
1.4 has checkpointing that may write
.borg_part_Nfiles. A naive retry would re-read the file from the start and write a second set of part files, so concatenating all part files would no longer reproduce the complete file. Therefore a file is not retried once part files have been written for it (ChunksProcessor.last_part_numbertracks this) — it is kept as-is (C) or skipped (E) instead.Permission errors
EPERM/EACCESare not retried (they won't get better) — avoids ~15s of pointless backoff on every unreadable file. This is the post-#7351 follow-up behavior; included here to avoid a regression vs. current 1.4.Tests
Adds a
ChunkerFailingtest chunker (--chunker-params=fail,block_size,map) to drive the error handling, and:test_create_erroneous_file(from create: implement retries for individual fs files #7351) — file is read on a later retrytest_create_no_permission_file— permission error is not retried, file skippedtest_create_erroneous_file_with_part_files— new, 1.4-specific — a file with part files is not retried (no master equivalent, since master 2.0 has no part files)🤖 Generated with Claude Code