[CLI] Retry Download #183
[CLI] Retry Download #183
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements retry logic for downloading definitions from GitHub releases, addressing issue #159. The changes improve reliability by attempting downloads multiple times before failing.
Key changes:
- Added retry mechanism with up to 3 attempts for both release metadata and asset downloads
- Changed error formatting from
{}to{:?}for better error debugging throughout - Removed unused
sizefield from theAssetstruct
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut retries = 0; | ||
| let mut result = None; | ||
| let mut succeeded = false; | ||
|
|
||
| while !succeeded { | ||
| 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_retries { | ||
| panic!("Reached max retries while downloading release.") | ||
| } | ||
|
|
||
| retries += 1; | ||
| error!( | ||
| "Retrying ({}/{}) download. Failed with status code: {:?}", | ||
| retries, | ||
| max_retries, | ||
| release_request.status() | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| let release_request = match result { | ||
| Some(r) => r, | ||
| None => panic!("Failed to download release"), | ||
| }; |
There was a problem hiding this comment.
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.
| let mut asset_retries = 0; | ||
| let mut asset_result = None; | ||
| let mut asset_success = false; | ||
|
|
||
| while !asset_success { | ||
| 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_retries >= max_retries { | ||
| panic!("Reached max retries while downloading asset!"); | ||
| } | ||
|
|
||
| asset_retries += 1; | ||
| error!( | ||
| "Retrying ({}/{}) asset download. Failed with status code: {:?}", | ||
| asset_retries, | ||
| max_retries, | ||
| response.status() | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| let response = match asset_result { | ||
| Some(r) => r, | ||
| None => panic!("Failed to download asset!"), | ||
| }; |
There was a problem hiding this comment.
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.
| while !succeeded { | ||
| 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_retries { | ||
| panic!("Reached max retries while downloading release.") | ||
| } | ||
|
|
||
| retries += 1; | ||
| error!( | ||
| "Retrying ({}/{}) download. Failed with status code: {:?}", | ||
| retries, | ||
| max_retries, | ||
| release_request.status() | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| result = Some(release_request); | ||
| } else { | ||
| if retries >= max_retries { | ||
| panic!("Reached max retries while downloading release.") |
There was a problem hiding this comment.
The panic messages in this file are inconsistent regarding trailing punctuation. Some have periods (line 89), some have exclamation marks (lines 152, 167), and some have neither (line 104). Consider standardizing the format for consistency.
| panic!("Reached max retries while downloading release.") | |
| panic!("Reached max retries while downloading release") |
|
|
||
| // 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; |
There was a problem hiding this comment.
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.
| let directory = match entry { | ||
| Ok(directory) => directory, | ||
| Err(e) => { | ||
| panic!("Warning: Failed to read directory entry {:?}", e); |
There was a problem hiding this comment.
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.
| panic!("Warning: Failed to read directory entry {:?}", e); | |
| error_without_trace(format!("Warning: Failed to read directory entry {:?}", e)); | |
| return; |
| panic!( | ||
| "Warning: Failed to create parent directory {}: {}", | ||
| "Warning: Failed to create parent directory {}: {:?}", | ||
| p.display(), | ||
| e | ||
| ); |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
Resolves: #159