From 7dbcce07630094778cf18712c556e2a86c2dea24 Mon Sep 17 00:00:00 2001 From: Benjamin Pannell Date: Sat, 11 Jul 2026 22:42:50 +0100 Subject: [PATCH 1/2] feat: retry failed targets and classify transient failures as user errors Add a 'retries' policy property (default 1) so an individual target that fails is retried before the error is reported, smoothing over transient network and remote-side failures. Retries apply per-target and compose with the existing git auto-recovery behaviour. Reclassify transient, user-facing failures from System to User errors so they are no longer reported as bugs via telemetry: - reqwest timeouts and request/connection failures (e.g. connection reset) - git clone/prepare failures (network and pack errors) --- docs/.vuepress/config.ts | 6 +- docs/advanced/retries.md | 44 ++++++++ src/engines/git.rs | 4 +- src/errors.rs | 76 +++++++++++-- src/pairing.rs | 229 ++++++++++++++++++++++++++++++++++++++- src/policy.rs | 87 +++++++++++++++ 6 files changed, 434 insertions(+), 12 deletions(-) create mode 100644 docs/advanced/retries.md diff --git a/docs/.vuepress/config.ts b/docs/.vuepress/config.ts index 5c3f25f..0b16259 100644 --- a/docs/.vuepress/config.ts +++ b/docs/.vuepress/config.ts @@ -56,7 +56,8 @@ export default defineUserConfig({ '/advanced/filters.md', '/advanced/query-params.md', '/advanced/refspecs.md', - '/advanced/recovery.md' + '/advanced/recovery.md', + '/advanced/retries.md' ] }, { @@ -99,7 +100,8 @@ export default defineUserConfig({ '/advanced/filters.md', '/advanced/query-params.md', '/advanced/refspecs.md', - '/advanced/recovery.md' + '/advanced/recovery.md', + '/advanced/retries.md' ] } ], diff --git a/docs/advanced/retries.md b/docs/advanced/retries.md new file mode 100644 index 0000000..4ab2d79 --- /dev/null +++ b/docs/advanced/retries.md @@ -0,0 +1,44 @@ +# Retries +When backing up large numbers of repositories, releases, and gists, it is normal to +occasionally hit a transient failure: a network connection is dropped or reset, a remote +server is momentarily overloaded, or a request times out. These failures usually clear up +on their own, so `github-backup` will automatically retry an individual target which fails +before reporting the error. + +How many times a failed target is retried is controlled by the `retries` property on your +backup policy. By default each target is retried **once** (for a total of two attempts) +before the failure is reported. + +```yaml{7-8} title="config.yaml" +schedule: "0 * * * *" + +backups: + - kind: github/repo + from: "repos/my-org/repo" + to: /backups/work + properties: + retries: "3" +``` + +| Value | Behaviour | +|-------------|-----------------------------------------------------------------------------------------------| +| `0` | Disable retries; the first failure of a target is reported immediately. | +| `1` | Retry a failed target once before reporting the error. This is the **default**. | +| `n` | Retry a failed target up to `n` times (for a total of `n + 1` attempts) before giving up. | + +## How Retries Work +Retries apply to each individual target independently. When a source entity (such as a +repository) is mirrored to several targets, each target is retried on its own, so a +transient failure writing to one destination will not affect the others. + +Retries wrap the entire backup of a target, which means they compose with the git +[automatic recovery](./recovery.md) behaviour: each attempt performs its own recovery +steps, and only once every attempt has been exhausted is the failure reported. + +::: tip +Retries smooth over transient problems, but they will not paper over a persistent +misconfiguration such as an invalid access token or an unreachable remote — those failures +will simply be retried the configured number of times and then reported as usual. If you +are seeing repeated failures, check the reported error rather than increasing the retry +count. +::: diff --git a/src/engines/git.rs b/src/engines/git.rs index cac6353..c2416e3 100644 --- a/src/engines/git.rs +++ b/src/engines/git.rs @@ -84,7 +84,7 @@ impl GitEngine { repo.clone_url, target.display() ); - let mut fetch = gix::prepare_clone(repo.clone_url.as_str(), target).wrap_system_err( + let mut fetch = gix::prepare_clone(repo.clone_url.as_str(), target).wrap_user_err( format!("Failed to clone the repository {}.", &repo.clone_url), &["Please make sure that the target directory is writable and that the repository is accessible."], )?; @@ -101,7 +101,7 @@ impl GitEngine { } trace!("Running clone in bare mode (not checking out files)"); - let (repository, _outcome) = fetch.fetch_only(Discard, cancel).wrap_system_err( + let (repository, _outcome) = fetch.fetch_only(Discard, cancel).wrap_user_err( format!("Unable to clone remote repository '{}'", repo.clone_url), &["Make sure that your internet connectivity is working correctly, and that your local git configuration is able to clone this repo."], )?; diff --git a/src/errors.rs b/src/errors.rs index 1b24165..6c488ec 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -14,12 +14,12 @@ impl HumanizableError for reqwest::Error { "Make sure that your internet connection is working correctly and the service is not blocked by your firewall.", ], ) - } else if self.is_decode() { - human_errors::wrap_system( + } else if self.is_timeout() { + human_errors::wrap_user( self, - "We could not decode the response from the remote server.", + "We timed out making a web request.", &[ - "This is likely due to a problem with the remote server, please try again later and report the problem to us on GitHub if the issue persists.", + "This is usually caused by a slow or unreliable network connection, or a remote server which is temporarily overloaded. Please try again later.", ], ) } else if self.is_redirect() { @@ -30,12 +30,20 @@ impl HumanizableError for reqwest::Error { "This is likely due to a problem with the remote server, please try again later and report the problem to us on GitHub if the issue persists.", ], ) - } else if self.is_timeout() { + } else if self.is_request() { + human_errors::wrap_user( + self, + "We could not complete a web request to the remote server.", + &[ + "This is usually caused by a transient network problem, such as a dropped or reset connection. Please make sure your internet connection is working correctly and try again later.", + ], + ) + } else if self.is_decode() { human_errors::wrap_system( self, - "We timed out making a web request.", + "We could not decode the response from the remote server.", &[ - "This is likely due to a problem with the remote server or your internet connection, please try again later and report the problem to us on GitHub if the issue persists.", + "This is likely due to a problem with the remote server, please try again later and report the problem to us on GitHub if the issue persists.", ], ) } else { @@ -153,3 +161,57 @@ impl std::fmt::Display for ResponseError { } impl std::error::Error for ResponseError {} + +#[cfg(test)] +mod tests { + use super::HumanizableError; + + #[tokio::test] + async fn connect_failure_is_user_error() { + // Port 1 on localhost is (essentially) never listening, so the + // connection attempt fails before a request can even be sent. This + // stands in for the transient connectivity failures we now classify as + // user errors so that they are retried rather than reported as bugs. + let err = reqwest::Client::new() + .get("http://127.0.0.1:1/") + .send() + .await + .expect_err("the connection to a closed port should fail"); + + let human = err.to_human_error(); + assert!( + human.is(human_errors::Kind::User), + "connectivity failures should be reported as user errors, got: {human}" + ); + } + + #[tokio::test] + async fn timeout_is_user_error() { + let server = wiremock::MockServer::start().await; + wiremock::Mock::given(wiremock::matchers::method("GET")) + .respond_with( + wiremock::ResponseTemplate::new(200).set_delay(std::time::Duration::from_secs(30)), + ) + .mount(&server) + .await; + + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_millis(50)) + .build() + .expect("a reqwest client"); + + let err = client + .get(server.uri()) + .send() + .await + .expect_err("the request should time out"); + + assert!(err.is_timeout(), "expected a timeout error, got: {err}"); + + let human = err.to_human_error(); + assert!( + human.is(human_errors::Kind::User), + "timeouts should be reported as user errors, got: {human}" + ); + } +} diff --git a/src/pairing.rs b/src/pairing.rs index 144ef44..06bc07a 100644 --- a/src/pairing.rs +++ b/src/pairing.rs @@ -92,6 +92,14 @@ impl< } } + let retries = match policy.retries() { + Ok(retries) => retries, + Err(e) => { + yield Err(e); + return; + } + }; + let mut join_set: JoinSet> = JoinSet::new(); for await entity in self.source.load(policy, cancel).trace(tracing::info_span!("backup.source.load")) { @@ -146,7 +154,30 @@ impl< let entity = entity.clone(); join_set.spawn(async move { debug!("Starting backup of {entity}"); - target.backup(&entity, &to, cancel).await.map(|state| (entity, state)) + + // A single target is backed up, up to `retries + 1` times, + // so that transient failures (dropped connections, momentarily + // unavailable remotes, and similar) don't cause an otherwise + // healthy backup to be reported as a failure. + let mut attempt = 0usize; + loop { + attempt += 1; + match target.backup(&entity, &to, cancel).await { + Ok(state) => break Ok((entity, state)), + Err(e) => { + if attempt > retries + || cancel.load(std::sync::atomic::Ordering::Relaxed) + { + break Err(e); + } + + warn!( + "Backup of {entity} failed on attempt {attempt} of {}, retrying: {e}", + retries + 1 + ); + } + } + } }.instrument(span)); } } @@ -272,6 +303,69 @@ mod tests { } } + /// A source which yields exactly one repository, used to exercise the + /// per-target retry behaviour with deterministic attempt counts. + struct SingleRepoSource; + + impl BackupSource for SingleRepoSource { + fn kind(&self) -> &str { + "mock" + } + + fn validate(&self, _policy: &BackupPolicy) -> Result<(), crate::Error> { + Ok(()) + } + + fn load<'a>( + &'a self, + _policy: &'a BackupPolicy, + _cancel: &'a AtomicBool, + ) -> impl Stream> + 'a { + async_stream::stream! { + yield Ok(GitRepo::new( + "octocat/Hello-World", + "https://example.com/repo.git", + None, + )); + } + } + } + + /// An engine which fails its first `failures_remaining` attempts before + /// succeeding, recording how many times it was invoked so that the retry + /// behaviour can be asserted. + #[derive(Clone)] + struct FlakyEngine { + failures_remaining: std::sync::Arc, + attempts: std::sync::Arc, + } + + #[async_trait::async_trait] + impl BackupEngine for FlakyEngine { + async fn backup( + &self, + entity: &GitRepo, + _target: &crate::BackupTarget, + _cancel: &AtomicBool, + ) -> Result { + use std::sync::atomic::Ordering; + + self.attempts.fetch_add(1, Ordering::SeqCst); + + // Consume one of the budgeted failures, if any remain. + let consumed = self.failures_remaining.fetch_update( + Ordering::SeqCst, + Ordering::SeqCst, + |remaining| remaining.checked_sub(1), + ); + + match consumed { + Ok(_) => Err(human_errors::user("simulated transient failure", &[])), + Err(_) => Ok(BackupState::New(Some(entity.name.clone()))), + } + } + } + #[derive(Clone)] struct MockEngine; @@ -356,4 +450,137 @@ mod tests { MatchType::GreaterOrEqual => assert!(count >= matches), } } + + async fn collect_backup_results( + source: SingleRepoSource, + engine: FlakyEngine, + retries: &str, + ) -> Vec> { + let policy: BackupPolicy = serde_yaml::from_str(&format!( + r#" + kind: mock + from: mock + to: /tmp + properties: + retries: "{retries}" + "# + )) + .unwrap(); + + let pairing = Pairing::new(source, engine); + let stream = pairing.run_all_backups(&policy, &CANCEL); + tokio::pin!(stream); + + let mut results = Vec::new(); + while let Some(result) = stream.next().await { + results.push(result); + } + results + } + + #[tokio::test] + async fn retries_recover_transient_failures() { + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; + + let attempts = Arc::new(AtomicUsize::new(0)); + let engine = FlakyEngine { + // Fail once, then succeed on the retry. + failures_remaining: Arc::new(AtomicUsize::new(1)), + attempts: attempts.clone(), + }; + + let results = collect_backup_results(SingleRepoSource, engine, "1").await; + + assert_eq!(results.len(), 1); + assert!( + results[0].is_ok(), + "the backup should succeed once the transient failure clears" + ); + assert_eq!( + attempts.load(Ordering::SeqCst), + 2, + "the target should have been attempted twice (one failure + one retry)" + ); + } + + #[tokio::test] + async fn retries_give_up_after_exhausting_the_budget() { + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; + + let attempts = Arc::new(AtomicUsize::new(0)); + let engine = FlakyEngine { + // Fail more times than the retry budget allows. + failures_remaining: Arc::new(AtomicUsize::new(5)), + attempts: attempts.clone(), + }; + + let results = collect_backup_results(SingleRepoSource, engine, "1").await; + + assert_eq!(results.len(), 1); + assert!( + results[0].is_err(), + "the backup should fail once the retry budget is exhausted" + ); + assert_eq!( + attempts.load(Ordering::SeqCst), + 2, + "the target should have been attempted exactly retries + 1 times" + ); + } + + #[tokio::test] + async fn retries_can_be_disabled() { + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; + + let attempts = Arc::new(AtomicUsize::new(0)); + let engine = FlakyEngine { + failures_remaining: Arc::new(AtomicUsize::new(1)), + attempts: attempts.clone(), + }; + + let results = collect_backup_results(SingleRepoSource, engine, "0").await; + + assert_eq!(results.len(), 1); + assert!( + results[0].is_err(), + "the first failure should be reported immediately when retries are disabled" + ); + assert_eq!( + attempts.load(Ordering::SeqCst), + 1, + "the target should only have been attempted once when retries are disabled" + ); + } + + #[tokio::test] + async fn invalid_retries_property_is_reported() { + let policy: BackupPolicy = serde_yaml::from_str( + r#" + kind: mock + from: mock + to: /tmp + properties: + retries: "not-a-number" + "#, + ) + .unwrap(); + + let pairing = Pairing::new(MockRepoSource, MockEngine); + let stream = pairing.run_all_backups(&policy, &CANCEL); + tokio::pin!(stream); + + let mut results = Vec::new(); + while let Some(result) = stream.next().await { + results.push(result); + } + + assert_eq!(results.len(), 1); + assert!( + results[0].is_err(), + "an invalid 'retries' property should be reported as an error" + ); + } } diff --git a/src/policy.rs b/src/policy.rs index b747ef4..2c72acc 100644 --- a/src/policy.rs +++ b/src/policy.rs @@ -6,6 +6,13 @@ use crate::Filter; use crate::entities::Credentials; use crate::target::BackupTargets; +/// The number of times an individual target backup is retried if it fails +/// before the failure is reported, unless overridden by the `retries` policy +/// property. Backing up a failed target one more time before giving up smooths +/// over the transient network and remote-side failures which are common when +/// mirroring large numbers of repositories. +pub const DEFAULT_RETRIES: usize = 1; + #[derive(Deserialize, Default)] pub struct BackupPolicy { pub kind: String, @@ -20,6 +27,28 @@ pub struct BackupPolicy { pub properties: HashMap, } +impl BackupPolicy { + /// The number of times the backup of an individual target should be retried + /// if it fails before the error is reported to the user. + /// + /// Configured through the optional `retries` policy property and defaulting + /// to [`DEFAULT_RETRIES`]. A value of `0` disables retries entirely, causing + /// the first failure to be reported immediately. + pub fn retries(&self) -> Result { + match self.properties.get("retries") { + Some(value) => value.trim().parse().map_err(|_| { + human_errors::user( + format!("The 'retries' property value '{value}' is not a valid number."), + &[ + "Set the 'retries' property to a non-negative whole number, such as '1', or remove it to use the default.", + ], + ) + }), + None => Ok(DEFAULT_RETRIES), + } + } +} + impl Display for BackupPolicy { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!(f, "{}/{}", self.kind, self.from) @@ -73,4 +102,62 @@ mod tests { assert_eq!(format!("{}", policy), "backup/source"); assert_eq!(format!("{:?}", policy), "backup/source"); } + + #[test] + fn test_retries_default() { + let policy = BackupPolicy::default(); + assert_eq!( + policy + .retries() + .expect("the default retry count to be valid"), + DEFAULT_RETRIES + ); + } + + #[test] + fn test_retries_configured() { + let policy: BackupPolicy = serde_yaml::from_str( + r#" + kind: backup + from: source + properties: + retries: "3" + "#, + ) + .unwrap(); + + assert_eq!(policy.retries().expect("a valid retry count"), 3); + } + + #[test] + fn test_retries_zero_disables_retries() { + let policy: BackupPolicy = serde_yaml::from_str( + r#" + kind: backup + from: source + properties: + retries: "0" + "#, + ) + .unwrap(); + + assert_eq!(policy.retries().expect("a valid retry count"), 0); + } + + #[test] + fn test_retries_invalid() { + let policy: BackupPolicy = serde_yaml::from_str( + r#" + kind: backup + from: source + properties: + retries: "not-a-number" + "#, + ) + .unwrap(); + + policy + .retries() + .expect_err("an invalid retry count to be rejected"); + } } From 7667d162223703517d0973f9e22335677a6a6676 Mon Sep 17 00:00:00 2001 From: Benjamin Pannell Date: Sat, 11 Jul 2026 22:53:59 +0100 Subject: [PATCH 2/2] style: remove redundant references in format! arguments Clippy's useless_borrows_in_formatting lint (denied in CI via -D warnings on the newer stable toolchain) flags redundant & borrows in format! arguments. Drop them so 'cargo clippy --all-targets --all-features -- -D warnings' passes. --- src/config.rs | 2 +- src/engines/git.rs | 26 +++++++++++++------------- src/helpers/github/client.rs | 2 +- src/sources/github_releases.rs | 6 +++--- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/config.rs b/src/config.rs index 8798d32..ff8d511 100644 --- a/src/config.rs +++ b/src/config.rs @@ -21,7 +21,7 @@ impl TryFrom<&Args> for Config { fn try_from(value: &Args) -> Result { let content = std::fs::read_to_string(&value.config).wrap_user_err( - format!("Failed to read the config file {}.", &value.config), + format!("Failed to read the config file {}.", value.config), &["Make sure that the configuration file exists and can be ready by the process."], )?; let config: Config = serde_yaml::from_str(&content).wrap_user_err( diff --git a/src/engines/git.rs b/src/engines/git.rs index c2416e3..1634878 100644 --- a/src/engines/git.rs +++ b/src/engines/git.rs @@ -85,7 +85,7 @@ impl GitEngine { target.display() ); let mut fetch = gix::prepare_clone(repo.clone_url.as_str(), target).wrap_user_err( - format!("Failed to clone the repository {}.", &repo.clone_url), + format!("Failed to clone the repository {}.", repo.clone_url), &["Please make sure that the target directory is writable and that the repository is accessible."], )?; @@ -120,7 +120,7 @@ impl GitEngine { })?; let head_id = repository.head_id().wrap_user_err( - format!("The repository '{}' did not have a valid HEAD, which may indicate that there is something wrong with the source repository.", &repo.clone_url), + format!("The repository '{}' did not have a valid HEAD, which may indicate that there is something wrong with the source repository.", repo.clone_url), &["Make sure that the remote repository is valid."], )?; @@ -139,8 +139,8 @@ impl GitEngine { let repository = gix::open(target).wrap_user_err( format!( "Failed to open the repository '{}' at '{}'", - &repo.clone_url, - &target.display() + repo.clone_url, + target.display() ), &["Make sure that the target directory is a valid git repository."], )?; @@ -154,8 +154,8 @@ impl GitEngine { let repository = gix::open(target).wrap_user_err( format!( "Failed to open the repository '{}' at '{}'", - &repo.clone_url, - &target.display() + repo.clone_url, + target.display() ), &["Make sure that the target directory is a valid git repository."], )?; @@ -172,7 +172,7 @@ impl GitEngine { format!( "Failed to find the remote '{}' in the repository '{}'", repo.clone_url, - &target.display() + target.display() ), &["Make sure that the repository is correctly configured and that the remote exists."], )? @@ -186,8 +186,8 @@ impl GitEngine { .wrap_user_err( format!( "Failed to configure the remote '{}' in the repository '{}' to fetch all branches.", - &repo.clone_url, - &target.display() + repo.clone_url, + target.display() ), &["Make sure that the repository is correctly configured and that the remote exists."], )?; @@ -198,7 +198,7 @@ impl GitEngine { .wrap_user_err( format!( "Unable to establish connection to remote git repository '{}'", - &repo.clone_url + repo.clone_url ), &["Make sure that the repository is available and correctly configured."], )?; @@ -214,7 +214,7 @@ impl GitEngine { .wrap_user_err( format!( "Unable to prepare fetch from remote git repository '{}'", - &repo.clone_url + repo.clone_url ), &["Make sure that the repository is available and correctly configured."], )? @@ -223,13 +223,13 @@ impl GitEngine { .wrap_user_err( format!( "Unable to fetch from remote git repository '{}'", - &repo.clone_url + repo.clone_url ), &["Make sure that the repository is available and correctly configured."], )?; let head_id = repository.head_id().wrap_user_err( - format!("The repository '{}' did not have a valid HEAD, which may indicate that there is something wrong with the source repository.", &repo.clone_url), + format!("The repository '{}' did not have a valid HEAD, which may indicate that there is something wrong with the source repository.", repo.clone_url), &["Make sure that the remote repository is valid."], )?; diff --git a/src/helpers/github/client.rs b/src/helpers/github/client.rs index 32bbe65..62f62ae 100644 --- a/src/helpers/github/client.rs +++ b/src/helpers/github/client.rs @@ -83,7 +83,7 @@ impl GitHubClient { Err(err) => { Err(human_errors::wrap_system( err, - format!("Unable to parse GitHub response into the expected structure when requesting '{}'.", &url), + format!("Unable to parse GitHub response into the expected structure when requesting '{}'.", url), &["Please report this issue to us on GitHub."], ))?; } diff --git a/src/sources/github_releases.rs b/src/sources/github_releases.rs index a17e12f..daf1423 100644 --- a/src/sources/github_releases.rs +++ b/src/sources/github_releases.rs @@ -48,7 +48,7 @@ impl GitHubReleasesSource { }; let mut entity = Release::new( - format!("{}/{}", &repo.full_name, &release.tag_name), + format!("{}/{}", repo.full_name, release.tag_name), repo.full_name.as_str(), release.tag_name.as_str(), ) @@ -60,7 +60,7 @@ impl GitHubReleasesSource { if let Some(tarball_url) = &release.tarball_url { entity = entity.with_asset( - HttpFile::new(format!("{}/{}/source.tar.gz", &repo.full_name, &release.tag_name), tarball_url) + HttpFile::new(format!("{}/{}/source.tar.gz", repo.full_name, release.tag_name), tarball_url) .with_metadata_source(repo) .with_metadata_source(&release) .with_metadata("asset.source-code", true) @@ -86,7 +86,7 @@ impl GitHubReleasesSource { let asset_url = format!("{}/releases/assets/{}", repo.url, asset.id); entity = entity.with_asset( - HttpFile::new(format!("{}/{}/{}", &repo.full_name, &release.tag_name, &asset.name), asset_url) + HttpFile::new(format!("{}/{}/{}", repo.full_name, release.tag_name, asset.name), asset_url) .with_content_type(Some("application/octet-stream".to_string())) .with_credentials(match &policy.credentials { Credentials::Token(token) => Credentials::UsernamePassword {