From ade6ae40d4847c3a9c9ab4088c0be50b8aea321f Mon Sep 17 00:00:00 2001 From: maui Date: Fri, 12 Jun 2026 09:11:37 +0200 Subject: [PATCH 1/2] fix(update): verify download completeness and retry truncated downloads The installer streamed the release archive and broke its read loop on the first EOF without ever checking received == Content-Length. A TLS-inspecting proxy / antivirus middlebox cutting a large HTTPS download short ends the body with a clean EOF, so a truncated zip was silently accepted and handed to extraction, which failed with the baffling 'ZipError: invalid Zip archive: Could not find EOCD'. - verify_complete(received, total): pure, unit-tested truncation gate - download_once: one attempt that truncates the file, streams, and verifies the full Content-Length arrived - download_archive: up to DOWNLOAD_ATTEMPTS (3) attempts with linear backoff, since middlebox truncation is typically transient A short download now fails as an honest 'Download incomplete: received N of M bytes' (with a hint to check VPN/proxy/AV) and self-heals on retry, instead of surfacing later as an unexplained extract error. Co-Authored-By: Claude Fable 5 --- src/update.rs | 200 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 140 insertions(+), 60 deletions(-) diff --git a/src/update.rs b/src/update.rs index 2ca528e..4c0281f 100644 --- a/src/update.rs +++ b/src/update.rs @@ -189,6 +189,103 @@ fn dev_update_url_override() -> Option { None } +/// How many times to (re)attempt the archive download before giving +/// up. Truncation by a middlebox is usually transient, so a couple of +/// fresh attempts clear it; 3 keeps the worst-case wait bounded. +#[cfg(not(target_os = "macos"))] +const DOWNLOAD_ATTEMPTS: u32 = 3; + +/// Verdict on whether a finished download received the whole body. +/// Pure so the truncation rule is unit-testable without a socket. A +/// `None` total means the server sent no `Content-Length` and we +/// can't verify — we accept it rather than reject a legitimate +/// chunked response. +#[cfg(not(target_os = "macos"))] +fn verify_complete(received: u64, total: Option) -> Result<(), String> { + match total { + Some(total) if received != total => Err(format!( + "Download incomplete: received {received} of {total} bytes \ + (connection truncated — check a VPN/proxy or antivirus that \ + inspects HTTPS, then retry)" + )), + _ => Ok(()), + } +} + +/// Download `url` into `archive_path`, streaming byte-progress into +/// `state`, with completeness verification and a small retry budget. +/// Returns the user-facing failure message on the final failure. +#[cfg(not(target_os = "macos"))] +fn download_archive( + client: &reqwest::blocking::Client, + url: &str, + archive_path: &std::path::Path, + state: &UpdateState, +) -> Result<(), String> { + let mut last_err = String::from("Download failed"); + for attempt in 1..=DOWNLOAD_ATTEMPTS { + match download_once(client, url, archive_path, state) { + Ok(()) => return Ok(()), + Err(err) => { + last_err = err; + if attempt < DOWNLOAD_ATTEMPTS { + // Linear backoff; the file is recreated (truncated) + // on the next attempt by `download_once`. + std::thread::sleep(std::time::Duration::from_millis( + 750 * attempt as u64, + )); + } + } + } + } + Err(last_err) +} + +/// One download attempt: (re)create the file, stream the body, verify +/// the full length arrived. Each call truncates `archive_path` so a +/// retry never appends onto a partial file. +#[cfg(not(target_os = "macos"))] +fn download_once( + client: &reqwest::blocking::Client, + url: &str, + archive_path: &std::path::Path, + state: &UpdateState, +) -> Result<(), String> { + let mut archive_file = std::fs::File::create(archive_path).map_err(|err| { + format!("Could not create archive file {}: {err:#}", archive_path.display()) + })?; + + let mut response = client + .get(url) + .send() + .map_err(|err| format!("Download request failed: {err:#}"))?; + response + .error_for_status_ref() + .map_err(|err| format!("Download failed: {err:#}"))?; + + let total = response.content_length(); + let mut received: u64 = 0; + *state.write() = UpdateStatus::Downloading { received, total }; + + let mut buf = [0u8; 64 * 1024]; + loop { + let n = match std::io::Read::read(&mut response, &mut buf) { + Ok(0) => break, + Ok(n) => n, + Err(err) => return Err(format!("Download read error: {err:#}")), + }; + std::io::Write::write_all(&mut archive_file, &buf[..n]) + .map_err(|err| format!("Write error: {err:#}"))?; + received += n as u64; + *state.write() = UpdateStatus::Downloading { received, total }; + } + std::io::Write::flush(&mut archive_file) + .map_err(|err| format!("Flush error: {err:#}"))?; + drop(archive_file); // close before Extract reads it + + verify_complete(received, total) +} + #[cfg(not(target_os = "macos"))] fn run_install(state: &UpdateState, info: ReleaseInfo) { // The first Downloading{received, total} state is written once the @@ -209,19 +306,6 @@ fn run_install(state: &UpdateState, info: ReleaseInfo) { let archive_path = temp_dir.path().join(&info.archive_name); - let archive_file = match std::fs::File::create(&archive_path) { - Ok(f) => f, - Err(err) => { - *state.write() = UpdateStatus::Failed { - message: format!( - "Could not create archive file {}: {err:#}", - archive_path.display() - ), - }; - return; - } - }; - // Stream the download ourselves so we can drive byte-progress into // the state slot. self_update::Download has no programmatic progress // hook in 0.41. reqwest::blocking is safe here — run_install is on a @@ -239,55 +323,18 @@ fn run_install(state: &UpdateState, info: ReleaseInfo) { } }; - let mut response = match client.get(&download_url).send() { - Ok(r) => r, - Err(err) => { - *state.write() = UpdateStatus::Failed { - message: format!("Download request failed: {err:#}"), - }; - return; - } - }; - if let Err(err) = response.error_for_status_ref() { - *state.write() = UpdateStatus::Failed { - message: format!("Download failed: {err:#}"), - }; - return; - } - - let total = response.content_length(); - let mut archive_file = archive_file; // rebind mut for write_all - let mut received: u64 = 0; - *state.write() = UpdateStatus::Downloading { received, total }; - - let mut buf = [0u8; 64 * 1024]; - loop { - let n = match std::io::Read::read(&mut response, &mut buf) { - Ok(0) => break, - Ok(n) => n, - Err(err) => { - *state.write() = UpdateStatus::Failed { - message: format!("Download read error: {err:#}"), - }; - return; - } - }; - if let Err(err) = std::io::Write::write_all(&mut archive_file, &buf[..n]) { - *state.write() = UpdateStatus::Failed { - message: format!("Write error: {err:#}"), - }; - return; - } - received += n as u64; - *state.write() = UpdateStatus::Downloading { received, total }; - } - if let Err(err) = std::io::Write::flush(&mut archive_file) { - *state.write() = UpdateStatus::Failed { - message: format!("Flush error: {err:#}"), - }; + // Download with completeness verification + a few retries. A + // TLS-inspecting proxy / middlebox cutting a large download short + // ends the body with a clean EOF, so a naive read loop accepts a + // truncated zip and the failure only surfaces later as a baffling + // "Could not find EOCD" extract error (#270-follow-up). Verifying + // received == Content-Length here turns that into an honest + // "download incomplete", and retrying self-heals transient + // truncation. + if let Err(message) = download_archive(&client, &download_url, &archive_path, state) { + *state.write() = UpdateStatus::Failed { message }; return; } - drop(archive_file); // close before Extract reads it *state.write() = UpdateStatus::Installing; @@ -388,3 +435,36 @@ pub fn open_releases_page() -> anyhow::Result<()> { ); open::that(&url).map_err(Into::into) } + +#[cfg(all(test, not(target_os = "macos")))] +mod tests { + use super::verify_complete; + + #[test] + fn full_download_is_complete() { + assert!(verify_complete(5_717_213, Some(5_717_213)).is_ok()); + } + + #[test] + fn truncated_download_is_rejected() { + // A middlebox cutting the stream short: fewer bytes than the + // advertised Content-Length must fail here, before extract. + let err = verify_complete(1_000, Some(5_717_213)).unwrap_err(); + assert!(err.contains("received 1000 of 5717213"), "{err}"); + assert!(err.to_lowercase().contains("incomplete"), "{err}"); + } + + #[test] + fn unknown_total_cannot_be_verified_so_accepts() { + // No Content-Length header → we can't prove truncation, so we + // don't reject a legitimate chunked response. + assert!(verify_complete(1_000, None).is_ok()); + } + + #[test] + fn over_read_is_not_silently_accepted_as_equal() { + // Defensive: more bytes than advertised is also "not equal" and + // must not pass the equality gate. + assert!(verify_complete(6_000_000, Some(5_717_213)).is_err()); + } +} From 0a5d4b093be97f36fcbc3ad3af133c56089a7666 Mon Sep 17 00:00:00 2001 From: maui Date: Fri, 12 Jun 2026 09:15:15 +0200 Subject: [PATCH 2/2] fix(update): distinct message for over-read vs truncated download MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review: received > total is not a truncation, so it no longer borrows the 'incomplete/connection truncated' wording — it reports an honest 'Download size mismatch' instead. Co-Authored-By: Claude Fable 5 --- src/update.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/update.rs b/src/update.rs index 4c0281f..29fc4f0 100644 --- a/src/update.rs +++ b/src/update.rs @@ -203,11 +203,20 @@ const DOWNLOAD_ATTEMPTS: u32 = 3; #[cfg(not(target_os = "macos"))] fn verify_complete(received: u64, total: Option) -> Result<(), String> { match total { - Some(total) if received != total => Err(format!( + // Short read — the common, retryable case: a middlebox cutting + // a large HTTPS download ends the body early with a clean EOF. + Some(total) if received < total => Err(format!( "Download incomplete: received {received} of {total} bytes \ (connection truncated — check a VPN/proxy or antivirus that \ inspects HTTPS, then retry)" )), + // Over-read — the server sent more than it advertised. Not a + // truncation; the archive is suspect either way, so reject with + // an honest, distinct message rather than the "incomplete" one. + Some(total) if received > total => Err(format!( + "Download size mismatch: received {received} bytes but the \ + server advertised {total} (unexpected — retry)" + )), _ => Ok(()), } } @@ -462,9 +471,11 @@ mod tests { } #[test] - fn over_read_is_not_silently_accepted_as_equal() { - // Defensive: more bytes than advertised is also "not equal" and - // must not pass the equality gate. - assert!(verify_complete(6_000_000, Some(5_717_213)).is_err()); + fn over_read_is_rejected_with_a_distinct_message() { + // More bytes than advertised must fail, but not as a + // "truncated/incomplete" download — that wording would be wrong. + let err = verify_complete(6_000_000, Some(5_717_213)).unwrap_err(); + assert!(err.contains("size mismatch"), "{err}"); + assert!(!err.to_lowercase().contains("incomplete"), "{err}"); } }