Skip to content

refactor: handle errors instead of panicking with unwrap in non-test code #619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
notheotherben merged 2 commits into from
Jul 8, 2026
Merged

refactor: handle errors instead of panicking with unwrap in non-test code #619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d28418d
refactor: handle errors instead of panicking with unwrap in non-test …
notheotherben Jul 7, 2026
f3a497b
style: Cleanup clippy warnings
notheotherben Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,18 @@ impl HumanizableError for reqwest::header::InvalidHeaderValue {
}
}

impl HumanizableError for tokio::task::JoinError {
fn to_human_error(self) -> human_errors::Error {
human_errors::wrap_system(
self,
"A backup task terminated unexpectedly before it could complete.",
&[
"This is likely a bug in github-backup, please report it to us on GitHub along with the error details above.",
],
)
}
}

#[derive(Debug)]
pub struct ResponseError {
pub status_code: StatusCode,
Expand Down
6 changes: 3 additions & 3 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ async fn run(args: Args, session: &Session) -> Result<(), Error> {
.as_ref()
.and_then(|s| s.find_next_occurrence(&chrono::Utc::now(), false).ok());

let handler = LoggingPairingHandler::new(&session);
let handler = LoggingPairingHandler::new(session);

pinger.on_start().await;

Expand Down Expand Up @@ -178,7 +178,7 @@ impl<E: BackupEntity> PairingHandler<E> for LoggingPairingHandler<'_> {

if error.is(human_errors::Kind::System) {
let info = tracing_batteries::ErrorInfo::new(&error)
.with_metadata("policy.kind", format!("{}", policy.kind));
.with_metadata("policy.kind", policy.kind.to_string());

self.session.record_custom_error(info);
} else {
Expand All @@ -197,7 +197,7 @@ impl<E: BackupEntity> PairingHandler<E> for LoggingPairingHandler<'_> {
self.session.record_event(
"policy::run",
[
("policy.kind", format!("{}", policy.kind)),
("policy.kind", policy.kind.to_string()),
("stats.new", summary.new.to_string()),
("stats.unchanged", summary.unchanged.to_string()),
("stats.updated", summary.updated.to_string()),
Expand Down
12 changes: 10 additions & 2 deletions src/pairing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use tracing_batteries::prelude::*;
use crate::{
BackupEntity, BackupPolicy, BackupSource,
engines::{BackupEngine, BackupState},
errors::HumanizableError as _,
};

pub struct Pairing<E: BackupEntity, S: BackupSource<E>, T: BackupEngine<E>> {
Expand Down Expand Up @@ -132,7 +133,11 @@ impl<
for to in policy.to.iter() {
while join_set.len() >= self.concurrency_limit {
debug!("Reached concurrency limit of {}, waiting for a task to complete", self.concurrency_limit);
yield join_set.join_next().await.unwrap().unwrap();
match join_set.join_next().await {
Some(Ok(result)) => yield result,
Some(Err(e)) => yield Err(e.to_human_error()),
None => break,
}
}

let span = tracing_batteries::prelude::info_span!(parent: &span, "backup.step", item=%entity, target=%to);
Expand All @@ -147,7 +152,10 @@ impl<
}

while let Some(fut) = join_set.join_next().await {
yield fut.unwrap();
match fut {
Ok(result) => yield result,
Err(e) => yield Err(e.to_human_error()),
}
}
}
}
Expand Down
55 changes: 27 additions & 28 deletions src/sources/github_gist.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,35 +38,34 @@ impl BackupSource<GitRepo> for GitHubGistSource {
policy: &'a BackupPolicy,
cancel: &'a AtomicBool,
) -> impl Stream<Item = Result<GitRepo, human_errors::Error>> + 'a {
let target: GitHubRepoSourceKind = policy.from.as_str().parse().unwrap();

let url = format!(
"{}/{}?{}",
policy
.properties
.get("api_url")
.unwrap_or(&"https://api.github.com".to_string())
.trim_end_matches('/'),
target.api_endpoint(GitHubArtifactKind::Gist),
policy.properties.get("query").unwrap_or(&"".to_string())
)
.trim_end_matches('?')
.to_string();

tracing_batteries::prelude::debug!("Calling {} to fetch gists", &url);

let refspecs = policy
.properties
.get("refspecs")
.map(|r| r.split(',').map(|r| r.to_string()).collect::<Vec<String>>());

let recovery_mode: RecoveryMode = policy
.properties
.get("recovery")
.map(|mode| mode.parse().unwrap())
.unwrap_or_default();

async_stream::try_stream! {
let target: GitHubRepoSourceKind = policy.from.as_str().parse()?;

let url = format!(
"{}/{}?{}",
policy
.properties
.get("api_url")
.unwrap_or(&"https://api.github.com".to_string())
.trim_end_matches('/'),
target.api_endpoint(GitHubArtifactKind::Gist),
policy.properties.get("query").unwrap_or(&"".to_string())
)
.trim_end_matches('?')
.to_string();

tracing_batteries::prelude::debug!("Calling {} to fetch gists", &url);

let refspecs = policy
.properties
.get("refspecs")
.map(|r| r.split(',').map(|r| r.to_string()).collect::<Vec<String>>());

let recovery_mode: RecoveryMode = match policy.properties.get("recovery") {
Some(mode) => mode.parse()?,
None => RecoveryMode::default(),
};

if matches!(target, GitHubRepoSourceKind::Gist(_)) {
let gist: GitHubGist = self.client.get(&url, &policy.credentials, cancel).await?;
yield GitRepo::new(
Expand Down
60 changes: 34 additions & 26 deletions src/sources/github_releases.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,13 @@ impl GitHubReleasesSource {
let releases_url = format!("{}/releases", repo.url);

for await release in self.client.get_paginated(&releases_url, &policy.credentials, cancel) {
if let Err(e) = release {
yield Err(e);
continue;
}

let release: GitHubRelease = release.unwrap();
let release: GitHubRelease = match release {
Ok(release) => release,
Err(e) => {
yield Err(e);
continue;
}
};

let mut entity = Release::new(
format!("{}/{}", &repo.full_name, &release.tag_name),
Expand Down Expand Up @@ -170,21 +171,27 @@ impl BackupSource<Release> for GitHubReleasesSource {
policy: &'a BackupPolicy,
cancel: &'a AtomicBool,
) -> impl Stream<Item = Result<Release, human_errors::Error>> + 'a {
let target: GitHubRepoSourceKind = policy.from.as_str().parse().unwrap();
let url = format!(
"{}/{}?{}",
policy
.properties
.get("api_url")
.unwrap_or(&"https://api.github.com".to_string())
.trim_end_matches('/'),
target.api_endpoint(GitHubArtifactKind::Release),
policy.properties.get("query").unwrap_or(&"".to_string())
)
.trim_end_matches('?')
.to_string();

async_stream::stream! {
let target: GitHubRepoSourceKind = match policy.from.as_str().parse() {
Ok(target) => target,
Err(e) => {
yield Err(e);
return;
}
};
let url = format!(
"{}/{}?{}",
policy
.properties
.get("api_url")
.unwrap_or(&"https://api.github.com".to_string())
.trim_end_matches('/'),
target.api_endpoint(GitHubArtifactKind::Release),
policy.properties.get("query").unwrap_or(&"".to_string())
)
.trim_end_matches('?')
.to_string();

if matches!(target, GitHubRepoSourceKind::Repo(_)) {
let repo: GitHubRepo = self.client.get(&url, &policy.credentials, cancel).await?;

Expand All @@ -193,12 +200,13 @@ impl BackupSource<Release> for GitHubReleasesSource {
}
} else {
for await repo in self.client.get_paginated(&url, &policy.credentials, cancel) {
if let Err(e) = repo {
yield Err(e);
continue;
}

let repo: GitHubRepo = repo.unwrap();
let repo: GitHubRepo = match repo {
Ok(repo) => repo,
Err(e) => {
yield Err(e);
continue;
}
};

for await file in self.load_releases(policy, &repo, cancel) {
yield file;
Expand Down
54 changes: 27 additions & 27 deletions src/sources/github_repo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,34 +38,34 @@ impl BackupSource<GitRepo> for GitHubRepoSource {
policy: &'a BackupPolicy,
cancel: &'a AtomicBool,
) -> impl Stream<Item = Result<GitRepo, human_errors::Error>> + 'a {
let target: GitHubRepoSourceKind = policy.from.as_str().parse().unwrap();
let url = format!(
"{}/{}?{}",
policy
.properties
.get("api_url")
.unwrap_or(&"https://api.github.com".to_string())
.trim_end_matches('/'),
target.api_endpoint(GitHubArtifactKind::Repo),
policy.properties.get("query").unwrap_or(&"".to_string())
)
.trim_end_matches('?')
.to_string();

tracing_batteries::prelude::debug!("Calling {} to fetch repos", &url);

let refspecs = policy
.properties
.get("refspecs")
.map(|r| r.split(',').map(|r| r.to_string()).collect::<Vec<String>>());

let recovery_mode: RecoveryMode = policy
.properties
.get("recovery")
.map(|mode| mode.parse().unwrap())
.unwrap_or_default();

async_stream::try_stream! {
let target: GitHubRepoSourceKind = policy.from.as_str().parse()?;

let url = format!(
"{}/{}?{}",
policy
.properties
.get("api_url")
.unwrap_or(&"https://api.github.com".to_string())
.trim_end_matches('/'),
target.api_endpoint(GitHubArtifactKind::Repo),
policy.properties.get("query").unwrap_or(&"".to_string())
)
.trim_end_matches('?')
.to_string();

tracing_batteries::prelude::debug!("Calling {} to fetch repos", &url);

let refspecs = policy
.properties
.get("refspecs")
.map(|r| r.split(',').map(|r| r.to_string()).collect::<Vec<String>>());

let recovery_mode: RecoveryMode = match policy.properties.get("recovery") {
Some(mode) => mode.parse()?,
None => RecoveryMode::default(),
};

if matches!(target, GitHubRepoSourceKind::Repo(_)) {
let repo: GitHubRepo = self.client.get(&url, &policy.credentials, cancel).await?;
yield GitRepo::new(
Expand Down
Loading