From a944ec0d49db50126b3253ede071919e46b529b4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Jul 2026 14:27:48 +0000 Subject: [PATCH 1/2] feat: automatically recover repositories which fail to update When a backup run is interrupted part-way through a fetch (e.g. by a power failure or the process being killed), the local repository can be left in a state which blocks every subsequent update - most commonly because of stale git lock files such as packed-refs.lock - and until now the only way out was manual intervention. The git engine now attempts automatic recovery whenever an existing local repository fails to update, staged from least to most invasive and controlled by a new per-policy 'recovery' property: - none: never attempt recovery, report the error as before. - non-destructive (default): remove stale git lock files (older than 15 minutes, so locks held by live operations are never touched) and retry the fetch. - destructive (opt-in): additionally clone a fresh copy of the repository into a hidden staging directory alongside the backup and, only if the clone succeeds, swap it into place. A failed clone - unreachable remote, bad credentials, full disk - leaves the existing backup completely untouched, so transient remote failures can never destroy local data. Backups which complete through recovery are annotated as such in the backup summary. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017s9x49j7QCzeqBLWku995F --- docs/.vuepress/config.ts | 6 +- docs/advanced/recovery.md | 63 +++++ examples/config.yaml | 9 + src/engines/git.rs | 500 ++++++++++++++++++++++++++++++++++++- src/entities/mod.rs | 3 + src/entities/recovery.rs | 94 +++++++ src/sources/github_gist.rs | 14 +- src/sources/github_repo.rs | 42 +++- 8 files changed, 724 insertions(+), 7 deletions(-) create mode 100644 docs/advanced/recovery.md create mode 100644 src/entities/recovery.rs diff --git a/docs/.vuepress/config.ts b/docs/.vuepress/config.ts index f614b2f..5c3f25f 100644 --- a/docs/.vuepress/config.ts +++ b/docs/.vuepress/config.ts @@ -55,7 +55,8 @@ export default defineUserConfig({ children: [ '/advanced/filters.md', '/advanced/query-params.md', - '/advanced/refspecs.md' + '/advanced/refspecs.md', + '/advanced/recovery.md' ] }, { @@ -97,7 +98,8 @@ export default defineUserConfig({ children: [ '/advanced/filters.md', '/advanced/query-params.md', - '/advanced/refspecs.md' + '/advanced/refspecs.md', + '/advanced/recovery.md' ] } ], diff --git a/docs/advanced/recovery.md b/docs/advanced/recovery.md new file mode 100644 index 0000000..4ec01b4 --- /dev/null +++ b/docs/advanced/recovery.md @@ -0,0 +1,63 @@ +# Automatic Recovery +When repeatedly backing up repositories over long periods of time, the local copy of a +repository can occasionally end up in a broken state. The most common cause is a backup +run which is interrupted part-way through (e.g. by a power failure or the process being +killed), leaving behind git lock files which block every subsequent update with errors +like this one: + +``` +Unable to fetch from remote git repository 'https://github.com/example/repo.git' (User error) + +This was caused by: + - Failed to update references to their new position to match their remote locations + - The lock for the packed-ref file could not be obtained + - The lock for resource '/backups/repos/example/repo/.git/packed-refs' could not be obtained after 1.00s after 15 attempt(s). + The lockfile at '/backups/repos/example/repo/.git/packed-refs.lock' might need manual deletion. +``` + +To avoid the need for manual intervention in these situations, `github-backup` will +automatically attempt to recover repositories which fail to update. How far it is +willing to go is controlled by the `recovery` property on your backup policy. + +## Recovery Modes + +| Mode | Behaviour | +|-------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `none` | Never attempt automatic recovery, report the error and leave the local repository untouched. | +| `non-destructive` | Remove stale git lock files (older than 15 minutes) and retry the fetch. This is the **default**. | +| `destructive` | Everything `non-destructive` does and, if that fails, clone a fresh copy of the repository into a temporary directory and replace the local copy with it if successful. | + +```yaml{7-8} title="config.yaml" +schedule: "0 * * * *" + +backups: + - kind: github/repo + from: "repos/my-org/repo" + to: /backups/work + properties: + recovery: destructive +``` + +## How Destructive Recovery Works +When `recovery: destructive` is configured and a repository cannot be updated (even +after stale locks have been cleaned up), the engine will: + +1. Clone a fresh copy of the repository into a hidden staging directory alongside the + existing backup (e.g. `.repo.recovery`). +2. If (and only if) the clone succeeds, move the existing backup out of the way, move + the fresh clone into its place, and then remove the old copy. +3. If the clone fails, the existing backup is left completely untouched and the + original error is reported. + +Using a successful clone as the gate means that transient problems — an unreachable +remote, invalid credentials, or a full disk — will never cause your existing (possibly +still valuable) local data to be discarded. The local copy is only ever replaced by a +clone which is known to be complete and healthy. + +::: warning +Destructive recovery replaces the local copy of the repository with the remote's +current state. Any data which only existed in your local backup — for example refs +which were force-pushed over or deleted on the remote since the last successful +backup — will be lost. If retaining such history is important to you, keep the +default `non-destructive` mode and resolve persistent corruption manually. +::: diff --git a/examples/config.yaml b/examples/config.yaml index 39b362d..751ccd4 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -43,6 +43,15 @@ backups: from: repos/SierraSoftworks/github-backup to: /backup/github + # Automatically replace local repositories which fail to update due to local + # corruption with a fresh clone (the default "non-destructive" recovery mode + # only removes stale git lock files and retries) + - kind: github/repo + from: repos/SierraSoftworks/grey + to: /backup/github + properties: + recovery: destructive + - kind: github/release from: repos/SierraSoftworks/github-backup to: /backup/github-releases diff --git a/src/engines/git.rs b/src/engines/git.rs index 312c309..48f4546 100644 --- a/src/engines/git.rs +++ b/src/engines/git.rs @@ -1,4 +1,9 @@ -use std::{fmt::Display, path::Path, sync::atomic::AtomicBool}; +use std::{ + fmt::Display, + path::{Path, PathBuf}, + sync::atomic::AtomicBool, + time::Duration, +}; use gix::{ credentials::helper::Action, @@ -12,11 +17,18 @@ use tracing_batteries::prelude::*; use crate::{ BackupEntity, - entities::{Credentials, GitRepo}, + entities::{Credentials, GitRepo, RecoveryMode}, }; use super::BackupState; +/// How old a git lock file must be before it is considered stale and safe to +/// remove during automatic recovery. Healthy git operations only hold their +/// locks for a fraction of a second, so anything older than this was almost +/// certainly left behind by a process which was killed before it could clean +/// up after itself. +const STALE_LOCK_MAX_AGE: Duration = Duration::from_secs(15 * 60); + #[derive(Clone, Default)] pub struct GitEngine; @@ -37,7 +49,10 @@ impl GitEngine { "Git directory exists at {}/.git, using fetch mode.", target_path.display() ); - self.fetch(entity, &target_path, cancel) + match self.fetch(entity, &target_path, cancel) { + Ok(state) => Ok(state), + Err(error) => self.recover(entity, &target_path, cancel, error), + } } else { trace!( "No Git directory found at {}/.git, using clone mode.", @@ -230,6 +245,265 @@ impl GitEngine { Ok(BackupState::Updated(Some(format!("{}", head_id.to_hex())))) } + /// Attempts to automatically recover from a failed update of an existing + /// local repository, according to the entity's configured [`RecoveryMode`]. + /// + /// Recovery is staged from least to most invasive: first stale git lock + /// files (left behind by a previous run which was killed part-way through + /// a fetch) are removed and the fetch is retried; if that doesn't resolve + /// the problem and destructive recovery has been enabled, a fresh copy of + /// the repository is cloned into a temporary location and swapped into + /// place. The original error is reported if recovery isn't possible. + #[tracing::instrument(skip(self, repo, target, cancel, error), err, fields(mode = %repo.recovery_mode))] + fn recover( + &self, + repo: &GitRepo, + target: &Path, + cancel: &AtomicBool, + error: human_errors::Error, + ) -> Result { + if repo.recovery_mode == RecoveryMode::Disabled + || cancel.load(std::sync::atomic::Ordering::Relaxed) + { + return Err(error); + } + + warn!( + "Updating the local copy of '{}' failed, attempting automatic recovery: {}", + repo.name(), + error + ); + + let error = match self.remove_stale_locks(target, STALE_LOCK_MAX_AGE) { + Ok(removed) if !removed.is_empty() => { + warn!( + "Removed {} stale git lock file(s) from '{}', retrying the fetch.", + removed.len(), + target.display() + ); + + match self.fetch(repo, target, cancel) { + Ok(state) => { + return Ok(annotate_state( + state, + "recovered by removing stale lock files", + )); + } + Err(retry_error) => retry_error, + } + } + Ok(_) => error, + Err(cleanup_error) => { + warn!( + "Unable to clean up stale git lock files in '{}': {}", + target.display(), + cleanup_error + ); + error + } + }; + + if repo.recovery_mode != RecoveryMode::Destructive { + return Err(error); + } + + warn!( + "Attempting destructive recovery of '{}' by cloning a fresh copy of '{}'.", + target.display(), + repo.clone_url + ); + + match self.reclone_and_replace(repo, target, cancel) { + Ok(state) => Ok(annotate_state( + state, + "recovered by re-cloning the repository", + )), + Err(recovery_error) => Err(human_errors::wrap_user( + error, + format!( + "We also attempted to automatically recover the local repository at '{}' by re-cloning it, however this failed too: {}", + target.display(), + recovery_error + ), + &[ + "Make sure that the remote repository is accessible and that the backup target is writable.", + "If the problem persists, you may need to remove the local copy of the repository manually so that it can be re-cloned from scratch.", + ], + )), + } + } + + /// Removes any git lock files within the repository's `.git` directory + /// which are older than `max_age`, returning the paths of the files which + /// were removed. + /// + /// Git (and gitoxide) take out `*.lock` files next to the file they intend + /// to replace and rename them into place once the update is complete. When + /// a process is killed part-way through an update these lock files are left + /// behind and block every subsequent update. The age threshold ensures that + /// we only ever remove locks which cannot still be held by a live git + /// operation. + fn remove_stale_locks( + &self, + target: &Path, + max_age: Duration, + ) -> Result, human_errors::Error> { + let git_dir = target.join(".git"); + if !git_dir.is_dir() { + return Ok(Vec::new()); + } + + let now = std::time::SystemTime::now(); + let mut removed = Vec::new(); + let mut pending = vec![git_dir]; + + while let Some(dir) = pending.pop() { + let entries = std::fs::read_dir(&dir).wrap_system_err( + format!( + "Unable to enumerate the directory '{}' while looking for stale git lock files.", + dir.display() + ), + &["Make sure that the backup directory is readable by the backup process."], + )?; + + for entry in entries { + let entry = entry.wrap_system_err( + format!( + "Unable to enumerate the directory '{}' while looking for stale git lock files.", + dir.display() + ), + &["Make sure that the backup directory is readable by the backup process."], + )?; + + let path = entry.path(); + let Ok(file_type) = entry.file_type() else { + continue; + }; + + if file_type.is_dir() { + pending.push(path); + } else if file_type.is_file() && path.extension().is_some_and(|ext| ext == "lock") { + // Treat any lock whose age we cannot determine as fresh so + // that we never remove a lock which might still be held. + let stale = entry + .metadata() + .and_then(|metadata| metadata.modified()) + .ok() + .and_then(|modified| now.duration_since(modified).ok()) + .is_some_and(|age| age >= max_age); + + if stale { + match std::fs::remove_file(&path) { + Ok(()) => removed.push(path), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => { + return Err(human_errors::wrap_system( + err, + format!( + "Unable to remove the stale git lock file '{}'.", + path.display() + ), + &[ + "Make sure that the backup directory is writable by the backup process.", + ], + )); + } + } + } + } + } + } + + Ok(removed) + } + + /// Clones a fresh copy of the repository into a temporary directory + /// alongside the current one and, if the clone succeeds, replaces the + /// (presumed corrupt) local repository with the fresh clone. + /// + /// The clone acting as a canary is what makes this safe to attempt on any + /// failure: if the remote is unreachable, the credentials are wrong, or the + /// disk is full, the clone fails and the original repository is left + /// untouched. The local copy is only ever replaced by a clone which we know + /// to be complete and healthy. + fn reclone_and_replace( + &self, + repo: &GitRepo, + target: &Path, + cancel: &AtomicBool, + ) -> Result { + let (parent, dir_name) = match ( + target.parent(), + target.file_name().and_then(|n| n.to_str()), + ) { + (Some(parent), Some(dir_name)) => (parent, dir_name), + _ => { + return Err(human_errors::system( + format!( + "Unable to determine a temporary recovery location for the backup target '{}'.", + target.display() + ), + &["Please report this issue to us on GitHub."], + )); + } + }; + + // Both directories live alongside the target so that the renames below + // remain on the same filesystem (and therefore atomic). + let staging = parent.join(format!(".{dir_name}.recovery")); + let discard = parent.join(format!(".{dir_name}.discard")); + + for dir in [&staging, &discard] { + if dir.exists() { + std::fs::remove_dir_all(dir).wrap_system_err( + format!( + "Unable to remove the left-over recovery directory '{}'.", + dir.display() + ), + &["Make sure that the backup directory is writable by the backup process."], + )?; + } + } + + let state = self.clone(repo, &staging, cancel)?; + + // The fresh clone succeeded, so the failure was local to our copy of + // the repository and it is safe to replace it. + std::fs::rename(target, &discard).wrap_system_err( + format!( + "Unable to move the corrupt repository '{}' out of the way during recovery.", + target.display() + ), + &["Make sure that the backup directory is writable by the backup process."], + )?; + + if let Err(err) = std::fs::rename(&staging, target) { + // Try to put the original repository back so that we don't lose + // whatever data it still holds. + let _ = std::fs::rename(&discard, target); + let _ = std::fs::remove_dir_all(&staging); + + return Err(human_errors::wrap_system( + err, + format!( + "Unable to move the freshly cloned repository into place at '{}' during recovery.", + target.display() + ), + &["Make sure that the backup directory is writable by the backup process."], + )); + } + + if let Err(err) = std::fs::remove_dir_all(&discard) { + warn!( + "Unable to remove the old copy of the repository at '{}' after recovery, you may wish to remove it manually: {}", + discard.display(), + err + ); + } + + Ok(state) + } + fn authenticate_connection( connection: &mut Connection<'_, '_, '_, T>, creds: &Credentials, @@ -334,6 +608,25 @@ impl Display for GitEngine { } } +/// Appends a note to a [`BackupState`]'s description so that backups which +/// were only completed through automatic recovery are visible as such in the +/// backup summary. +fn annotate_state(state: BackupState, note: &str) -> BackupState { + let annotate = |detail: Option| { + Some(match detail { + Some(detail) => format!("{detail}; {note}"), + None => note.to_string(), + }) + }; + + match state { + BackupState::New(detail) => BackupState::New(annotate(detail)), + BackupState::Updated(detail) => BackupState::Updated(annotate(detail)), + BackupState::Unchanged(detail) => BackupState::Unchanged(annotate(detail)), + BackupState::Skipped => BackupState::Skipped, + } +} + #[cfg(test)] mod tests { use rstest::rstest; @@ -380,4 +673,205 @@ mod tests { "the repository should not have changed between backups" ); } + + #[test] + fn test_remove_stale_locks() { + let temp_dir = tempfile::tempdir().expect("a temporary directory"); + let git_dir = temp_dir.path().join(".git"); + std::fs::create_dir_all(git_dir.join("refs").join("heads")) + .expect("to be able to create the git directory structure"); + + let stale_locks = [ + git_dir.join("packed-refs.lock"), + git_dir.join("refs").join("heads").join("main.lock"), + ]; + let fresh_lock = git_dir.join("HEAD.lock"); + let config_file = git_dir.join("config"); + + for path in stale_locks.iter().chain([&fresh_lock, &config_file]) { + std::fs::write(path, "contents").expect("to be able to create the test file"); + } + + let stale_time = std::time::SystemTime::now() - Duration::from_secs(60 * 60); + for path in &stale_locks { + std::fs::File::options() + .write(true) + .open(path) + .expect("to be able to open the lock file") + .set_modified(stale_time) + .expect("to be able to age the lock file"); + } + + let removed = GitEngine + .remove_stale_locks(temp_dir.path(), STALE_LOCK_MAX_AGE) + .expect("lock cleanup to succeed"); + + assert_eq!( + removed.len(), + 2, + "only the stale lock files should have been removed" + ); + for path in &stale_locks { + assert!( + !path.exists(), + "the stale lock file '{}' should have been removed", + path.display() + ); + assert!(removed.contains(path)); + } + + assert!( + fresh_lock.exists(), + "recently created lock files should be left in place" + ); + assert!( + config_file.exists(), + "files which are not lock files should be left in place" + ); + } + + #[test] + fn test_remove_stale_locks_without_git_dir() { + let temp_dir = tempfile::tempdir().expect("a temporary directory"); + + let removed = GitEngine + .remove_stale_locks(temp_dir.path(), Duration::ZERO) + .expect("lock cleanup to succeed"); + + assert!( + removed.is_empty(), + "no locks should be removed when there is no .git directory" + ); + } + + #[rstest] + #[case(BackupState::New(Some("at abc123".into())), "new at abc123; recovered")] + #[case(BackupState::Updated(None), "updated recovered")] + #[case(BackupState::Unchanged(Some("at abc123".into())), "unchanged at abc123; recovered")] + #[case(BackupState::Skipped, "skipped")] + fn test_annotate_state(#[case] state: BackupState, #[case] expected: &str) { + assert_eq!(format!("{}", annotate_state(state, "recovered")), expected); + } + + #[cfg_attr(feature = "pure_tests", ignore)] + #[rstest] + #[case("SierraSoftworks/grey", "https://github.com/sierrasoftworks/grey.git")] + #[tokio::test] + async fn test_stale_lock_recovery(#[case] name: &str, #[case] url: &str) { + let temp_dir = tempfile::tempdir().expect("a temporary directory"); + + let agent = GitEngine; + let cancel = AtomicBool::new(false); + + let repo = GitRepo::new(name, url, None); + + agent + .backup(&repo, temp_dir.path(), &cancel) + .await + .expect("initial backup to succeed (clone)"); + + // Simulate a lock file which was left behind by a previous run that + // was killed part-way through a fetch operation. + let repo_path = temp_dir.path().join(repo.target_path()); + let lock_file = repo_path.join(".git").join("packed-refs.lock"); + std::fs::write(&lock_file, "").expect("to be able to create the lock file"); + std::fs::File::options() + .write(true) + .open(&lock_file) + .expect("to be able to open the lock file") + .set_modified(std::time::SystemTime::now() - Duration::from_secs(60 * 60)) + .expect("to be able to age the lock file"); + + let state = agent + .recover( + &repo, + &repo_path, + &cancel, + human_errors::user("simulated fetch failure", &[]), + ) + .expect("recovery to succeed after removing the stale lock file"); + + assert!( + !lock_file.exists(), + "the stale lock file should have been removed" + ); + assert!( + matches!(state, BackupState::Unchanged(..)), + "the retried fetch should report the repository as unchanged" + ); + } + + #[cfg_attr(feature = "pure_tests", ignore)] + #[rstest] + #[case("SierraSoftworks/grey", "https://github.com/sierrasoftworks/grey.git")] + #[tokio::test] + async fn test_destructive_recovery(#[case] name: &str, #[case] url: &str) { + let temp_dir = tempfile::tempdir().expect("a temporary directory"); + + let agent = GitEngine; + let cancel = AtomicBool::new(false); + + let repo = GitRepo::new(name, url, None); + + agent + .backup(&repo, temp_dir.path(), &cancel) + .await + .expect("initial backup to succeed (clone)"); + + // Corrupt the repository in a way which cannot be repaired + // non-destructively. + let repo_path = temp_dir.path().join(repo.target_path()); + let config_file = repo_path.join(".git").join("config"); + let corrupt_config = "this is not [a valid git config"; + std::fs::write(&config_file, corrupt_config).expect("to be able to corrupt the config"); + + agent + .backup(&repo, temp_dir.path(), &cancel) + .await + .expect_err("the backup should fail when the repository is corrupted"); + + let repo = repo.with_recovery_mode(RecoveryMode::Destructive); + let state = agent + .backup(&repo, temp_dir.path(), &cancel) + .await + .expect("destructive recovery to replace the corrupted repository"); + + assert!( + matches!(state, BackupState::New(..)), + "the repository should have been re-cloned" + ); + assert!( + repo_path.join(".git").exists(), + "the recovered repository should exist at the original location" + ); + assert_ne!( + std::fs::read_to_string(&config_file).expect("to be able to read the config"), + corrupt_config, + "the corrupted config should have been replaced" + ); + + let parent = repo_path.parent().expect("the repository to have a parent"); + let dir_name = repo_path + .file_name() + .and_then(|n| n.to_str()) + .expect("the repository to have a directory name"); + assert!( + !parent.join(format!(".{dir_name}.recovery")).exists(), + "the recovery staging directory should have been cleaned up" + ); + assert!( + !parent.join(format!(".{dir_name}.discard")).exists(), + "the discarded repository should have been cleaned up" + ); + + // A subsequent backup of the recovered repository should work normally. + let state = agent + .backup(&repo, temp_dir.path(), &cancel) + .await + .expect("a subsequent backup to succeed (fetch)"); + assert!( + matches!(state, BackupState::Unchanged(..)), + "the recovered repository should be fetchable" + ); + } } diff --git a/src/entities/mod.rs b/src/entities/mod.rs index 98f168e..5093f0c 100644 --- a/src/entities/mod.rs +++ b/src/entities/mod.rs @@ -1,11 +1,13 @@ mod credentials; #[macro_use] mod macros; +mod recovery; mod release; use crate::{FilterValue, Filterable}; pub use credentials::Credentials; +pub use recovery::RecoveryMode; pub use release::Release; use std::borrow::Cow; use std::collections::HashMap; @@ -61,4 +63,5 @@ entity!(HttpFile(url: U => String) { entity!(GitRepo(clone_url: U => String, refspecs: R => Option>) { with_credentials => credentials: Credentials, + with_recovery_mode => recovery_mode: RecoveryMode, }); diff --git a/src/entities/recovery.rs b/src/entities/recovery.rs new file mode 100644 index 0000000..00c200f --- /dev/null +++ b/src/entities/recovery.rs @@ -0,0 +1,94 @@ +use std::fmt::Display; +use std::str::FromStr; + +/// Controls how the git backup engine responds when an existing local copy of +/// a repository cannot be updated due to a local problem, such as stale lock +/// files left behind by an interrupted run, or a corrupted repository. +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +pub enum RecoveryMode { + /// Never attempt automatic recovery; report the error and leave the local + /// repository untouched. + Disabled, + + /// Attempt only non-destructive recovery steps (such as removing stale git + /// lock files) before retrying the backup. This is the default. + #[default] + NonDestructive, + + /// In addition to the non-destructive recovery steps, allow the engine to + /// clone a fresh copy of the repository into a temporary location and + /// replace the local copy with it if the clone succeeds. + Destructive, +} + +impl FromStr for RecoveryMode { + type Err = human_errors::Error; + + fn from_str(s: &str) -> Result { + match s.trim().to_lowercase().as_str() { + "none" | "disabled" | "off" => Ok(RecoveryMode::Disabled), + "non-destructive" | "nondestructive" => Ok(RecoveryMode::NonDestructive), + "destructive" => Ok(RecoveryMode::Destructive), + other => Err(human_errors::user( + format!("The recovery mode '{other}' is not recognized."), + &[ + "Use one of 'none', 'non-destructive', or 'destructive' as the 'recovery' property in your backup policy.", + ], + )), + } + } +} + +impl Display for RecoveryMode { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + RecoveryMode::Disabled => write!(f, "none"), + RecoveryMode::NonDestructive => write!(f, "non-destructive"), + RecoveryMode::Destructive => write!(f, "destructive"), + } + } +} + +#[cfg(test)] +mod tests { + use rstest::rstest; + + use super::*; + + #[rstest] + #[case("none", RecoveryMode::Disabled)] + #[case("disabled", RecoveryMode::Disabled)] + #[case("off", RecoveryMode::Disabled)] + #[case("non-destructive", RecoveryMode::NonDestructive)] + #[case("nondestructive", RecoveryMode::NonDestructive)] + #[case("destructive", RecoveryMode::Destructive)] + #[case("Destructive", RecoveryMode::Destructive)] + #[case(" destructive ", RecoveryMode::Destructive)] + fn parse(#[case] input: &str, #[case] expected: RecoveryMode) { + assert_eq!(input.parse::().unwrap(), expected); + } + + #[rstest] + #[case("")] + #[case("bogus")] + #[case("delete-everything")] + fn parse_invalid(#[case] input: &str) { + input + .parse::() + .expect_err("parsing should fail for unrecognized recovery modes"); + } + + #[test] + fn default_is_non_destructive() { + assert_eq!(RecoveryMode::default(), RecoveryMode::NonDestructive); + } + + #[rstest] + #[case(RecoveryMode::Disabled, "none")] + #[case(RecoveryMode::NonDestructive, "non-destructive")] + #[case(RecoveryMode::Destructive, "destructive")] + fn display_round_trips(#[case] mode: RecoveryMode, #[case] display: &str) { + assert_eq!(format!("{mode}"), display); + assert_eq!(display.parse::().unwrap(), mode); + } +} diff --git a/src/sources/github_gist.rs b/src/sources/github_gist.rs index 6239513..43e1f4b 100644 --- a/src/sources/github_gist.rs +++ b/src/sources/github_gist.rs @@ -5,7 +5,7 @@ use tokio_stream::Stream; use crate::helpers::github::GitHubGist; use crate::{ BackupSource, - entities::GitRepo, + entities::{GitRepo, RecoveryMode}, helpers::{ GitHubClient, github::{GitHubArtifactKind, GitHubRepoSourceKind}, @@ -26,6 +26,10 @@ impl BackupSource for GitHubGistSource { fn validate(&self, policy: &BackupPolicy) -> Result<(), human_errors::Error> { let _: GitHubRepoSourceKind = policy.from.as_str().parse()?; + if let Some(mode) = policy.properties.get("recovery") { + let _: RecoveryMode = mode.parse()?; + } + Ok(()) } @@ -56,6 +60,12 @@ impl BackupSource for GitHubGistSource { .get("refspecs") .map(|r| r.split(',').map(|r| r.to_string()).collect::>()); + let recovery_mode: RecoveryMode = policy + .properties + .get("recovery") + .map(|mode| mode.parse().unwrap()) + .unwrap_or_default(); + async_stream::try_stream! { if matches!(target, GitHubRepoSourceKind::Gist(_)) { let gist: GitHubGist = self.client.get(&url, &policy.credentials, cancel).await?; @@ -64,6 +74,7 @@ impl BackupSource for GitHubGistSource { gist.git_pull_url.as_str(), refspecs.clone()) .with_credentials(policy.credentials.clone()) + .with_recovery_mode(recovery_mode) .with_metadata_source(&gist); } else { for await gist in self.client.get_paginated(&url, &policy.credentials, cancel) { @@ -73,6 +84,7 @@ impl BackupSource for GitHubGistSource { gist.git_pull_url.as_str(), refspecs.clone()) .with_credentials(policy.credentials.clone()) + .with_recovery_mode(recovery_mode) .with_metadata_source(&gist); } } diff --git a/src/sources/github_repo.rs b/src/sources/github_repo.rs index 29d859e..10c978c 100644 --- a/src/sources/github_repo.rs +++ b/src/sources/github_repo.rs @@ -4,7 +4,7 @@ use tokio_stream::Stream; use crate::{ BackupSource, - entities::GitRepo, + entities::{GitRepo, RecoveryMode}, helpers::{ GitHubClient, github::GitHubRepo, @@ -25,6 +25,11 @@ impl BackupSource for GitHubRepoSource { fn validate(&self, policy: &BackupPolicy) -> Result<(), human_errors::Error> { let _: GitHubRepoSourceKind = policy.from.as_str().parse()?; + + if let Some(mode) = policy.properties.get("recovery") { + let _: RecoveryMode = mode.parse()?; + } + Ok(()) } @@ -54,6 +59,12 @@ impl BackupSource for GitHubRepoSource { .get("refspecs") .map(|r| r.split(',').map(|r| r.to_string()).collect::>()); + let recovery_mode: RecoveryMode = policy + .properties + .get("recovery") + .map(|mode| mode.parse().unwrap()) + .unwrap_or_default(); + async_stream::try_stream! { if matches!(target, GitHubRepoSourceKind::Repo(_)) { let repo: GitHubRepo = self.client.get(&url, &policy.credentials, cancel).await?; @@ -62,6 +73,7 @@ impl BackupSource for GitHubRepoSource { repo.clone_url.as_str(), refspecs.clone()) .with_credentials(policy.credentials.clone()) + .with_recovery_mode(recovery_mode) .with_metadata_source(&repo); } else { for await repo in self.client.get_paginated(&url, &policy.credentials, cancel) { @@ -71,6 +83,7 @@ impl BackupSource for GitHubRepoSource { repo.clone_url.as_str(), refspecs.clone()) .with_credentials(policy.credentials.clone()) + .with_recovery_mode(recovery_mode) .with_metadata_source(&repo); } } @@ -134,6 +147,33 @@ mod tests { } } + #[rstest] + #[case("none", true)] + #[case("non-destructive", true)] + #[case("destructive", true)] + #[case("bogus", false)] + fn validation_recovery_mode(#[case] mode: &str, #[case] success: bool) { + let source = GitHubRepoSource::default(); + + let policy = serde_yaml::from_str(&format!( + r#" + kind: github/repo + from: user + to: /tmp + properties: + recovery: {} + "#, + mode + )) + .expect("parse policy"); + + if success { + source.validate(&policy).expect("validation to succeed"); + } else { + source.validate(&policy).expect_err("validation to fail"); + } + } + #[rstest] #[case("users/notheotherben")] #[tokio::test] From 007095127a6141410f7b483d652656fb262606e4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Jul 2026 14:53:57 +0000 Subject: [PATCH 2/2] test: cover recovery error-handling branches with offline tests Addresses the codecov patch-coverage report on the recovery change by exercising the failure paths which the end-to-end tests don't reach: - recovery disabled or cancelled: the original error is reported and the repository is left untouched - the retried fetch after stale-lock removal fails: the retry error is reported - the destructive-recovery canary clone fails: the original repository is preserved, leftover staging directories are cleaned up, and the original error remains in the causal chain - a backup target without a parent directory is rejected - the 'recovery' policy property is parsed by the repo/gist sources' load paths and validated by the gist source Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017s9x49j7QCzeqBLWku995F --- src/engines/git.rs | 162 +++++++++++++++++++++++++++++++++++++ src/sources/github_gist.rs | 28 +++++++ src/sources/github_repo.rs | 2 + 3 files changed, 192 insertions(+) diff --git a/src/engines/git.rs b/src/engines/git.rs index 48f4546..cac6353 100644 --- a/src/engines/git.rs +++ b/src/engines/git.rs @@ -753,6 +753,168 @@ mod tests { assert_eq!(format!("{}", annotate_state(state, "recovered")), expected); } + /// Creates a directory containing a (not necessarily valid) `.git` + /// directory with a stale lock file inside it, simulating a repository + /// whose previous backup run was killed part-way through a fetch. + fn setup_repo_with_stale_lock(target: &Path) -> PathBuf { + std::fs::create_dir_all(target.join(".git")) + .expect("to be able to create the git directory"); + + let lock_file = target.join(".git").join("packed-refs.lock"); + std::fs::write(&lock_file, "").expect("to be able to create the lock file"); + std::fs::File::options() + .write(true) + .open(&lock_file) + .expect("to be able to open the lock file") + .set_modified(std::time::SystemTime::now() - Duration::from_secs(60 * 60)) + .expect("to be able to age the lock file"); + + lock_file + } + + #[test] + fn test_recover_disabled_returns_original_error() { + let temp_dir = tempfile::tempdir().expect("a temporary directory"); + let lock_file = setup_repo_with_stale_lock(temp_dir.path()); + + let repo = GitRepo::new("test/repo", "https://example.com/repo.git", None) + .with_recovery_mode(RecoveryMode::Disabled); + + let err = GitEngine + .recover( + &repo, + temp_dir.path(), + &AtomicBool::new(false), + human_errors::user("simulated fetch failure", &[]), + ) + .expect_err("recovery should not be attempted when it is disabled"); + + assert!( + format!("{err}").contains("simulated fetch failure"), + "the original error should be reported unchanged" + ); + assert!( + lock_file.exists(), + "the repository should have been left untouched" + ); + } + + #[test] + fn test_recover_skipped_when_cancelled() { + let temp_dir = tempfile::tempdir().expect("a temporary directory"); + let lock_file = setup_repo_with_stale_lock(temp_dir.path()); + + let repo = GitRepo::new("test/repo", "https://example.com/repo.git", None); + + let err = GitEngine + .recover( + &repo, + temp_dir.path(), + &AtomicBool::new(true), + human_errors::user("simulated fetch failure", &[]), + ) + .expect_err("recovery should not be attempted when the backup has been cancelled"); + + assert!(format!("{err}").contains("simulated fetch failure")); + assert!( + lock_file.exists(), + "the repository should have been left untouched" + ); + } + + #[test] + fn test_recover_reports_retry_failure() { + let temp_dir = tempfile::tempdir().expect("a temporary directory"); + let lock_file = setup_repo_with_stale_lock(temp_dir.path()); + + let repo = GitRepo::new("test/repo", "https://example.com/repo.git", None); + + // The stale lock is removed, but the retried fetch fails because the + // directory is not a valid git repository; that error is reported. + let err = GitEngine + .recover( + &repo, + temp_dir.path(), + &AtomicBool::new(false), + human_errors::user("simulated fetch failure", &[]), + ) + .expect_err("recovery should fail when the retried fetch fails"); + + assert!( + !lock_file.exists(), + "the stale lock file should have been removed" + ); + assert!( + format!("{err}").contains("Failed to open the repository"), + "the error from the retried fetch should be reported, got: {err}" + ); + } + + #[test] + fn test_destructive_recovery_leaves_original_when_clone_fails() { + let temp_dir = tempfile::tempdir().expect("a temporary directory"); + let target = temp_dir.path().join("owner").join("repo"); + std::fs::create_dir_all(target.join(".git")) + .expect("to be able to create the git directory"); + std::fs::write(target.join(".git").join("HEAD"), "ref: refs/heads/main") + .expect("to be able to create the HEAD file"); + + // Leftovers from a previously interrupted recovery attempt should be + // cleaned up before the fresh clone is attempted. + let staging = temp_dir.path().join("owner").join(".repo.recovery"); + let discard = temp_dir.path().join("owner").join(".repo.discard"); + for dir in [&staging, &discard] { + std::fs::create_dir_all(dir).expect("to be able to create the leftover directory"); + std::fs::write(dir.join("junk"), "junk").expect("to be able to create the junk file"); + } + + // The clone URL points at a location which doesn't exist, so the + // fresh clone (acting as the canary for destructive recovery) fails. + let missing_remote = temp_dir.path().join("missing-remote.git"); + let repo = GitRepo::new( + "owner/repo", + missing_remote.display().to_string(), + None::>, + ) + .with_recovery_mode(RecoveryMode::Destructive); + + let err = GitEngine + .recover( + &repo, + &target, + &AtomicBool::new(false), + human_errors::user("simulated fetch failure", &[]), + ) + .expect_err("recovery should fail when the fresh clone fails"); + + assert!( + format!("{err}").contains("simulated fetch failure"), + "the original error should be included in the causal chain, got: {err}" + ); + assert!( + target.join(".git").join("HEAD").exists(), + "the original repository should have been left untouched" + ); + assert!( + !discard.exists(), + "leftover recovery directories should have been cleaned up" + ); + } + + #[test] + fn test_reclone_rejects_target_without_parent() { + let repo = GitRepo::new("test/repo", "https://example.com/repo.git", None); + + let err = GitEngine + .reclone_and_replace(&repo, Path::new("/"), &AtomicBool::new(false)) + .expect_err("recovery should be rejected for a target without a parent directory"); + + assert!( + format!("{err}").contains("Unable to determine a temporary recovery location"), + "got: {err}" + ); + } + #[cfg_attr(feature = "pure_tests", ignore)] #[rstest] #[case("SierraSoftworks/grey", "https://github.com/sierrasoftworks/grey.git")] diff --git a/src/sources/github_gist.rs b/src/sources/github_gist.rs index 43e1f4b..b7a60d7 100644 --- a/src/sources/github_gist.rs +++ b/src/sources/github_gist.rs @@ -146,6 +146,32 @@ mod tests { } } + #[rstest] + #[case("non-destructive", true)] + #[case("destructive", true)] + #[case("bogus", false)] + fn validation_recovery_mode(#[case] mode: &str, #[case] success: bool) { + let source = GitHubGistSource::default(); + + let policy = serde_yaml::from_str(&format!( + r#" + kind: github/gist + from: user + to: /tmp + properties: + recovery: {} + "#, + mode + )) + .expect("parse policy"); + + if success { + source.validate(&policy).expect("validation to succeed"); + } else { + source.validate(&policy).expect_err("validation to fail"); + } + } + #[rstest] #[case("user", "/gists", "github.gists.0.json", 2)] #[case("users/octocat", "/users/octocat/gists", "github.gists.0.json", 2)] @@ -174,6 +200,8 @@ mod tests { kind: github/gist from: {} to: /tmp + properties: + recovery: non-destructive "#, target )) diff --git a/src/sources/github_repo.rs b/src/sources/github_repo.rs index 10c978c..e060b76 100644 --- a/src/sources/github_repo.rs +++ b/src/sources/github_repo.rs @@ -228,6 +228,8 @@ mod tests { kind: github/repo from: {} to: /tmp + properties: + recovery: non-destructive "#, target ))