Skip to content

[CLI] Retry Download #183

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
raphael-goetz merged 3 commits into from
Dec 19, 2025
Merged

[CLI] Retry Download #183

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7b4af1
feat: added network retry and better error messages
raphael-goetz Dec 19, 2025
177bf43
ref: fixed spelling
raphael-goetz Dec 19, 2025
1f5ad59
fix: correct while conditions
raphael-goetz Dec 19, 2025
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
201 changes: 120 additions & 81 deletions crates/cli/src/command/download.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::formatter::{error_without_trace, info, success};
use bytes::Bytes;
use log::error;
use reqwest::header::{ACCEPT, USER_AGENT};
use reqwest::{Client, Response};
use serde::Deserialize;
use std::fs;
use std::fs::File;
Expand All @@ -17,7 +19,6 @@ struct Release {
struct Asset {
name: String,
browser_download_url: String,
size: u64,
}

pub async fn handle_download(tag: Option<String>, features: Option<Vec<String>>) {
Expand All @@ -26,13 +27,7 @@ pub async fn handle_download(tag: Option<String>, features: Option<Vec<String>>)

// Download the definitions
info("Starting download process...".to_string());
let bytes = match download_definitions_as_bytes(tag).await {
Some(bytes) => bytes,
None => {
error_without_trace(String::from("Download failed."));
return;
}
};
let bytes = download_definitions_as_bytes(tag).await;

// Extract the zip file
convert_bytes_to_folder(bytes, zip_path).await;
Comment on lines 27 to 33

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from returning Option and gracefully handling failures to panicking on errors represents a significant regression in error handling. The previous code allowed users to receive a friendly error message and continue, while the new code will cause the entire application to crash with a panic. This is particularly problematic for a CLI tool where users expect graceful error messages. Consider restoring the Option return type or using Result to allow proper error propagation and user-friendly error messages.

Copilot uses AI. Check for mistakes.
Expand All @@ -52,7 +47,7 @@ pub async fn handle_download(tag: Option<String>, features: Option<Vec<String>>)
));
}

async fn download_definitions_as_bytes(tag: Option<String>) -> Option<bytes::Bytes> {
async fn download_definitions_as_bytes(tag: Option<String>) -> Bytes {
let client = reqwest::Client::new();

let url = match tag {
Expand All @@ -66,23 +61,47 @@ async fn download_definitions_as_bytes(tag: Option<String>) -> Option<bytes::Byt
}
};

let release_request = match client
.get(url)
.header(USER_AGENT, "code0-definition-cli")
.header(ACCEPT, "application/vnd.github+json")
.send()
.await
{
Ok(response) => {
if response.status().is_success() {
response
} else {
return None;
}
async fn download_release(client: &Client, url: String) -> Response {
match client
.get(url)
.header(USER_AGENT, "code0-definition-cli")
.header(ACCEPT, "application/vnd.github+json")
.send()
.await
{
Ok(r) => r,
Err(e) => panic!("Request failed: {:?}", e),
}
Err(e) => {
panic!("Request failed: {}", e);
}

let max_retires = 3;
Comment thread
raphael-goetz marked this conversation as resolved.
Outdated
let mut retries = 0;
let mut result = None;
let mut succeeded = false;

while succeeded {
Comment thread
raphael-goetz marked this conversation as resolved.
Outdated
let release_request = download_release(&client, url.clone()).await;
if release_request.status().is_success() {
succeeded = true;
result = Some(release_request);
} else {
if retries >= max_retires {
panic!("Reached max retires while downloading release.")
Comment thread
raphael-goetz marked this conversation as resolved.
Outdated
}

retries += 1;
Comment thread
raphael-goetz marked this conversation as resolved.
error!(
"Retrying ({}/{}) download. Failed with status code: {:?}",
retries,
max_retires,
release_request.status()
);
}
}
Comment on lines +82 to +100

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry loop performs immediate retries without any delay or backoff between attempts. This can overwhelm the server with rapid successive requests and may violate rate limits. Consider adding exponential backoff or at least a small delay between retry attempts to be more respectful of the GitHub API and increase the likelihood of success.

Copilot uses AI. Check for mistakes.

let release_request = match result {
Some(r) => r,
None => panic!("Failed to download release"),
};
Comment on lines +78 to 105

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable 'result' is initialized as None and only set to Some when succeeded is true, but the loop exits when succeeded is true. This means the match on line 102-105 will never encounter a None case - it's defensive code that can never execute. Consider removing the Option wrapper and the redundant match, or restructure the loop to make the logic clearer.

Copilot uses AI. Check for mistakes.

let release: Release = match release_request.json::<Release>().await {
Expand All @@ -91,7 +110,7 @@ async fn download_definitions_as_bytes(tag: Option<String>) -> Option<bytes::Byt
release
}
Err(e) => {
panic!("Request failed: {}", e);
panic!("Request failed: {:?}", e);
}
};

Expand All @@ -108,34 +127,53 @@ async fn download_definitions_as_bytes(tag: Option<String>) -> Option<bytes::Byt
}
};

match client
.get(&asset.browser_download_url)
.header(USER_AGENT, "code0-definition-cli")
.send()
.await
{
Ok(response) => {
if response.status().is_success() {
match response.bytes().await {
Ok(bytes) => {
info("Download completed successfully".to_string());
Some(bytes)
}
Err(e) => {
error_without_trace(format!("Failed to read download data: {e}"));
None
}
}
} else {
error_without_trace(format!(
"Download failed with status: {}",
response.status()
));
None
let mut asset_retires = 0;
Comment thread
raphael-goetz marked this conversation as resolved.
Outdated
let mut asset_result = None;
let mut asset_success = false;

while asset_success {
Comment thread
raphael-goetz marked this conversation as resolved.
Outdated
let response = match client
.get(&asset.browser_download_url)
.header(USER_AGENT, "code0-definition-cli")
.send()
.await
{
Ok(response) => response,
Err(e) => {
panic!("Download request failed: {:?}", e);
}
};

if response.status().is_success() {
asset_success = true;
asset_result = Some(response);
} else {
if asset_retires >= max_retires {
panic!("Reached max retires while downloading asset!");
Comment thread
raphael-goetz marked this conversation as resolved.
Outdated
Comment thread
raphael-goetz marked this conversation as resolved.
Outdated
}

asset_retires += 1;
error!(
"Retrying ({}/{}) asset download. Failed with status code: {:?}",
asset_retires,
Comment thread
raphael-goetz marked this conversation as resolved.
Outdated
max_retires,
response.status()
);
}
}
Comment on lines +134 to +163

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the release download, the asset download retry loop has no delay between attempts. This can result in rapid-fire requests that may trigger rate limiting or be rejected by the server. Adding backoff between retries would improve reliability.

Copilot uses AI. Check for mistakes.

let response = match asset_result {
Some(r) => r,
None => panic!("Failed to download asset!"),
};
Comment on lines +130 to +168

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the release download retry logic, 'asset_result' is initialized as None and only set to Some when asset_success is true, making the None case in the match at lines 165-168 unreachable. This defensive code adds unnecessary complexity.

Copilot uses AI. Check for mistakes.

match response.bytes().await {
Ok(bytes) => {
info("Download completed successfully".to_string());
bytes
}
Err(e) => {
panic!("Download request failed: {e}");
panic!("Failed to read downloaded data: {:?}", e);
}
}
}
Expand All @@ -148,14 +186,14 @@ async fn convert_bytes_to_folder(bytes: Bytes, zip_path: &str) {
let zip_file = match File::open(zip_path) {
Ok(file) => file,
Err(e) => {
panic!("Failed to open zip file: {e}");
panic!("Failed to open zip file: {:?}", e);
}
};

let mut archive = match ZipArchive::new(zip_file) {
Ok(archive) => archive,
Err(e) => {
panic!("Failed to read zip archive: {e}");
panic!("Failed to read zip archive: {:?}", e);
}
};

Expand All @@ -166,7 +204,7 @@ async fn convert_bytes_to_folder(bytes: Bytes, zip_path: &str) {
let mut file = match archive.by_index(i) {
Ok(file) => file,
Err(e) => {
panic!("Failed to read file at index {i}: {e}");
panic!("Failed to read file at index {i}: {:?}", e);
}
};

Expand All @@ -177,15 +215,15 @@ async fn convert_bytes_to_folder(bytes: Bytes, zip_path: &str) {

if file.name().ends_with('/') {
if let Err(e) = fs::create_dir_all(&out_path) {
panic!("Failed to create directory {}: {}", out_path.display(), e);
panic!("Failed to create directory {}: {:?}", out_path.display(), e);
}
} else {
if let Some(p) = out_path.parent()
&& !p.exists()
&& let Err(e) = fs::create_dir_all(p)
{
panic!(
"Warning: Failed to create parent directory {}: {}",
"Warning: Failed to create parent directory {}: {:?}",
p.display(),
e
);
Comment on lines 225 to 229

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The panic message starts with "Warning:" which is contradictory - panics are fatal errors, not warnings. If creating the parent directory is truly optional or can be skipped, use error_without_trace or just log the error instead of panicking. If it's a fatal error, remove the "Warning:" prefix.

Copilot uses AI. Check for mistakes.
Expand All @@ -194,11 +232,11 @@ async fn convert_bytes_to_folder(bytes: Bytes, zip_path: &str) {
match File::create(&out_path) {
Ok(mut outfile) => {
if let Err(e) = std::io::copy(&mut file, &mut outfile) {
panic!("Warning: Failed to extract {}: {}", out_path.display(), e);
panic!("Warning: Failed to extract {}: {:?}", out_path.display(), e);

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The panic message starts with "Warning:" which is contradictory - panics are fatal errors, not warnings. If extraction failure for individual files is non-fatal, use error_without_trace and continue processing other files. If it's a fatal error that should stop the program, remove the "Warning:" prefix.

Copilot uses AI. Check for mistakes.
}
}
Err(e) => {
panic!("Failed to create file {}: {}", out_path.display(), e);
panic!("Failed to create file {}: {:?}", out_path.display(), e);
}
}
}
Expand All @@ -209,40 +247,41 @@ async fn convert_bytes_to_folder(bytes: Bytes, zip_path: &str) {

match fs::remove_file(zip_path) {
Ok(_) => info("Temporary zip file removed".to_string()),
Err(e) => error_without_trace(format!("Warning: Failed to remove temporary zip file: {e}")),
Err(e) => error_without_trace(format!(
"Warning: Failed to remove temporary zip file: {:?}",
e
)),
}
}

async fn filter_features(selected_features: Vec<String>) {
let definitions_path = "./definitions";

match fs::read_dir(definitions_path) {
Ok(entries) => {
for entry in entries {
let directory = match entry {
Ok(directory) => directory,
Err(e) => {
panic!(
"{}",
format!("Warning: Failed to read directory entry: {e}")
);
}
};
let entries = match fs::read_dir(definitions_path) {
Ok(entries) => entries,
Err(e) => {
error_without_trace(format!("Failed to read definitions directory: {:?}", e));
return;
}
};

let name = directory.file_name().to_str().unwrap_or("").to_string();
for entry in entries {
let directory = match entry {
Ok(directory) => directory,
Err(e) => {
panic!("Warning: Failed to read directory entry {:?}", e);

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The panic message starts with "Warning:" which is contradictory - panics are fatal errors, not warnings. If this is truly a warning that should be logged but not cause the application to crash, use error_without_trace instead of panic (as done on line 282). If it's a fatal error that should panic, remove the "Warning:" prefix.

Suggested change
panic!("Warning: Failed to read directory entry {:?}", e);
error_without_trace(format!("Warning: Failed to read directory entry {:?}", e));
return;

Copilot uses AI. Check for mistakes.
}
};

if !selected_features.contains(&name) {
match fs::remove_dir_all(directory.path()) {
Ok(_) => {}
Err(e) => {
error_without_trace(format!("Warning: Failed to remove directory: {e}"))
}
}
let name = directory.file_name().to_str().unwrap_or("").to_string();

if !selected_features.contains(&name) {
match fs::remove_dir_all(directory.path()) {
Ok(_) => {}
Err(e) => {
error_without_trace(format!("Warning: Failed to remove directory: {:?}", e))
}
}
}
Err(e) => {
error_without_trace(format!("Failed to read definitions directory: {e}"));
}
}
}