From 7c7317adc945238e396c15bfe55c7c5a69b63135 Mon Sep 17 00:00:00 2001 From: NimBold Date: Wed, 17 Jun 2026 09:27:17 +0330 Subject: [PATCH] fix(retry): honor attempt caps per download URL --- src-tauri/src/download.rs | 62 ++++++++++++++++++++++++--------------- src-tauri/src/retry.rs | 26 +++++++++++----- 2 files changed, 57 insertions(+), 31 deletions(-) diff --git a/src-tauri/src/download.rs b/src-tauri/src/download.rs index c1245b9..491a16f 100644 --- a/src-tauri/src/download.rs +++ b/src-tauri/src/download.rs @@ -429,9 +429,12 @@ async fn download_file( // The legacy `max_tries` payload field is still honored as a cap on // attempts, but transient backoff is additionally bounded by // `retry::MAX_RETRIES` so a single URL cannot spin forever. - let mut strike = 0_usize; + let max_attempts = payload.max_tries.max(1) as usize; 'url: for url in &payload.urls { + let mut strike = 0_usize; + let mut attempts = 0_usize; loop { + attempts += 1; match download_attempt(&events, &client, &payload, url, &mut control_rx).await { Ok(()) => return DownloadOutcome::Completed, Err(AttemptError::Controlled(DownloadControl::Pause)) => { @@ -446,34 +449,45 @@ async fn download_file( } Err(AttemptError::Failed(error)) => { last_error = error.clone(); - let transient = crate::retry::is_transient_network_error(&error); - let strikes_left = strike < crate::retry::MAX_RETRIES; - if !(transient && strikes_left) { - // Permanent error (e.g. HTTP 404 / disk full) or the - // 3-strike budget is exhausted — advance to the next URL. - strike = 0; + + if attempts >= max_attempts { continue 'url; } - // Transient: announce `Retrying`, back off, then retry. - // The backoff sleep is itself cancelable so a user - // pause/cancel during the wait is honored immediately. - events.emit_retrying(payload.id, strike, error); - let delay = crate::retry::backoff_for(strike); - tokio::select! { - _ = tokio::time::sleep(delay) => {} - control = control_rx.recv() => { - return match control.unwrap_or(DownloadControl::Cancel) { - DownloadControl::Pause => DownloadOutcome::Paused, - DownloadControl::Cancel => { - let _ = fs::remove_file(&payload.output_path).await; - DownloadOutcome::Cancelled - } - DownloadControl::Replace => DownloadOutcome::Cancelled, - }; + let transient = crate::retry::is_transient_network_error(&error); + let strikes_left = strike < crate::retry::MAX_RETRIES; + + if transient && strikes_left { + // Transient: announce `Retrying`, back off, then retry. + // The backoff sleep is itself cancelable so a user + // pause/cancel during the wait is honored immediately. + events.emit_retrying(payload.id, strike, error); + let delay = crate::retry::backoff_for(strike); + tokio::select! { + _ = tokio::time::sleep(delay) => {} + control = control_rx.recv() => { + return match control.unwrap_or(DownloadControl::Cancel) { + DownloadControl::Pause => DownloadOutcome::Paused, + DownloadControl::Cancel => { + let _ = fs::remove_file(&payload.output_path).await; + DownloadOutcome::Cancelled + } + DownloadControl::Replace => DownloadOutcome::Cancelled, + }; + } } + strike += 1; + continue; } - strike += 1; + + if !transient && !crate::retry::is_permanent_network_error(&error) { + // Legacy `max_tries` cap for ambiguous HTTP statuses (e.g. + // 500) that are neither clearly transient nor permanent. + continue; + } + + // Permanent error or transient strike budget exhausted. + continue 'url; } } } diff --git a/src-tauri/src/retry.rs b/src-tauri/src/retry.rs index 955438c..9f9de72 100644 --- a/src-tauri/src/retry.rs +++ b/src-tauri/src/retry.rs @@ -72,12 +72,8 @@ pub fn backoff_for(strike: usize) -> Duration { /// 401/403/404/410/451, "not found", permission denied, out-of-disk. The /// permanent list is checked first so a composite message (e.g. an HTTP 404 /// that also mentions "timeout" in a URL) still fails fast. -pub fn is_transient_network_error(message: &str) -> bool { +pub fn is_permanent_network_error(message: &str) -> bool { let m = message.to_ascii_lowercase(); - - // Permanent conditions win — never retry a 404/403/410/401/451, a missing - // file, a permission error, or a full disk, even if the message also - // contains a transient keyword (e.g. "timeout" inside a URL path). const PERMANENT: [&str; 9] = [ "http 401", "http 403", @@ -89,11 +85,17 @@ pub fn is_transient_network_error(message: &str) -> bool { "permission denied", "no space left on device", ]; - if PERMANENT.iter().any(|p| m.contains(p)) { + PERMANENT.iter().any(|p| m.contains(p)) +} + +pub fn is_transient_network_error(message: &str) -> bool { + if is_permanent_network_error(message) { return false; } - const TRANSIENT: [&str; 18] = [ + let m = message.to_ascii_lowercase(); + + const TRANSIENT: [&str; 20] = [ // reqwest / hyper / OS socket-layer "timed out", "timeout", @@ -112,6 +114,8 @@ pub fn is_transient_network_error(message: &str) -> bool { // HTTP-level transient "http 408", "request timeout", + "http 503", + "503 service unavailable", // aria2c log phrasing "connection was closed", "timeout.", @@ -204,6 +208,14 @@ mod tests { assert!(is_transient_network_error("request timeout")); } + #[test] + fn classifies_http_503_as_transient() { + assert!(is_transient_network_error("HTTP 503 Service Unavailable")); + assert!(is_transient_network_error( + "http://127.0.0.1/file returned HTTP 503 Service Unavailable" + )); + } + #[test] fn classifies_ytdlp_and_aria2_phrasing_as_transient() { assert!(is_transient_network_error(