fix(install): retry download body stream, verification, and extraction

crates/vite_install/src/request.rs

@@ -3,7 +3,7 @@ use std::{path::Path, time::Duration};
 use backon::{ExponentialBuilder, Retryable};
 use flate2::read::GzDecoder;
 use futures_util::stream::StreamExt;
-use reqwest::Response;
+use reqwest::{Response, StatusCode};
 use serde::de::DeserializeOwned;
 use sha1::Sha1;
 use sha2::{Digest, Sha224, Sha256, Sha512};
@@ -110,31 +110,65 @@ impl HttpClient {
         let target_path = target_path.as_ref();
         tracing::debug!("Downloading {} to {:?}", url, target_path);
 
-        let response = self.get(url).await?;
+        let client = vite_shared::shared_http_client();
 
-        self.write_response_to_file(response, target_path).await?;
+        // Make the request *and* the body stream a single retried unit. Doing
+        // the request inline (instead of calling `self.get`) avoids a double
+        // retry layer. A truncated download (bytes written != advertised
+        // Content-Length) returns an error so the retry re-downloads.
+        (|| async {
+            let response = client.get(url).send().await?.error_for_status()?;
+            Self::write_response_to_file(response, target_path).await
+        })
+        .retry(
+            ExponentialBuilder::default()
+                .with_jitter()
+                .with_min_delay(Duration::from_millis(self.min_delay))
+                .with_max_times(self.max_times),
+        )
+        .await?;
 
         tracing::debug!("Download completed: {:?}", target_path);
         Ok(())
     }
 
-    /// Internal helper to write response body to file
-    async fn write_response_to_file(
-        &self,
-        response: Response,
-        target_path: &Path,
-    ) -> Result<(), Error> {
+    /// Internal helper to write response body to file.
+    ///
+    /// Captures the advertised `Content-Length` before streaming and verifies
+    /// the number of bytes written matches it, so a truncated/short read
+    /// surfaces as an error that the caller's retry can re-download.
+    async fn write_response_to_file(response: Response, target_path: &Path) -> Result<(), Error> {
+        let content_length = response.content_length();
+
         // Create the target file
         let mut file = fs::File::create(target_path).await?;
 
         // Stream the response body to the file
+        let mut bytes_written: u64 = 0;
         let mut stream = response.bytes_stream();
         while let Some(chunk_result) = stream.next().await {
             let chunk = chunk_result?;
+            bytes_written += chunk.len() as u64;
             file.write_all(&chunk).await?;
         }
 
         file.flush().await?;
+
+        // Detect truncation: if a Content-Length was advertised and the bytes
+        // written don't match, the download is incomplete — error out so the
+        // retry re-downloads from scratch.
+        if let Some(expected_len) = content_length
+            && bytes_written != expected_len
+        {
+            return Err(Error::Io(std::io::Error::new(
+                std::io::ErrorKind::UnexpectedEof,
+                vite_str::format!(
+                    "incomplete download: expected {expected_len} bytes, got {bytes_written}"
+                )
+                .to_string(),
+            )));
+        }
+
         Ok(())
     }
 }
@@ -177,12 +211,45 @@ pub async fn download_and_extract_tgz_with_hash(
         expected_hash
     );
 
-    // Create target directory
-    fs::create_dir_all(&target_dir).await?;
+    // This is the single retry layer for the whole download → verify → extract
+    // pipeline: each attempt does one download (no nested retry — see
+    // `_once`), so a transient network error, a truncated download, a corrupt
+    // archive, or a hash mismatch all re-download from scratch exactly once per
+    // attempt. A 404 (version not found) and permanent config errors fail fast
+    // and propagate unchanged so the caller in `package_manager.rs` can map a
+    // 404 to `PackageManagerVersionNotFound`.
+    (|| async { download_and_extract_tgz_with_hash_once(url, &target_dir, expected_hash).await })
+        .retry(
+            ExponentialBuilder::default()
+                .with_jitter()
+                .with_min_delay(Duration::from_millis(500))
+                .with_max_times(3),
+        )
+        .when(is_retryable_download_error)
+        .await
+}
 
-    // Download the tgz file with retry logic using HttpClient
+/// A single download → verify → extract attempt.
+///
+/// Starts from clean state by removing and recreating `target_dir`, so a
+/// partially-extracted or corrupt prior attempt cannot interfere with a retry.
+async fn download_and_extract_tgz_with_hash_once(
+    url: &str,
+    target_dir: &Path,
+    expected_hash: Option<&str>,
+) -> Result<(), Error> {
+    // Reset target directory so a partial prior attempt can't interfere.
+    if fs::try_exists(target_dir).await.unwrap_or(false) {
+        fs::remove_dir_all(target_dir).await?;
+    }
+    fs::create_dir_all(target_dir).await?;
+
+    // Download the tgz file with a single attempt (no internal retry). The
+    // pipeline retry in `download_and_extract_tgz_with_hash` owns all retries;
+    // letting `download_file` retry here too would nest two retry layers and
+    // multiply attempts (up to N×M downloads) for a persistent failure.
     let tgz_file = target_dir.join("package.tgz");
-    let client = HttpClient::new();
+    let client = HttpClient::with_config(0, 0);
     client.download_file(url, &tgz_file).await?;
 
     // Verify hash if provided
@@ -192,7 +259,7 @@ pub async fn download_and_extract_tgz_with_hash(
 
     // Extract the tgz file to the target directory
     let tgz_file_for_extract = tgz_file.clone();
-    let target_dir_for_extract = target_dir.clone();
+    let target_dir_for_extract = target_dir.to_path_buf();
     tokio::task::spawn_blocking(move || {
         extract_tgz(&tgz_file_for_extract, &target_dir_for_extract)
     })
@@ -204,6 +271,27 @@ pub async fn download_and_extract_tgz_with_hash(
     Ok(())
 }
 
+/// Predicate for the single download → verify → extract retry in
+/// [`download_and_extract_tgz_with_hash`].
+///
+/// Retries transient failures that a fresh re-download can fix; everything else
+/// fails fast:
+/// - `Reqwest`: retry transient network/HTTP errors, but NOT a 404 — that means
+///   the version doesn't exist, so it must propagate unchanged for the caller's
+///   `PackageManagerVersionNotFound` mapping (and there's no point retrying it).
+/// - `Io` / `IoWithPath`: truncated download or corrupt-archive extraction
+///   (e.g. tar `UnexpectedEof`).
+/// - `HashMismatch`: integrity failure, most likely a corrupt/truncated download.
+/// - Everything else (bad hash format, unsupported algorithm, a `JoinError`
+///   from a `spawn_blocking` panic, …) is permanent and is not retried.
+fn is_retryable_download_error(err: &Error) -> bool {
+    match err {
+        Error::Reqwest(e) => e.status() != Some(StatusCode::NOT_FOUND),
+        Error::Io(_) | Error::IoWithPath { .. } | Error::HashMismatch { .. } => true,
+        _ => false,
+    }
+}
+
 /// Computes the hash of the given content using the specified digest algorithm.
 ///
 /// # Type Parameters
@@ -540,6 +628,65 @@ mod tests {
         // TempDir automatically cleans up when it goes out of scope
     }
 
+    /// Regression test for flaky package-manager / node downloads.
+    ///
+    /// The retry logic used to wrap only the HTTP request setup
+    /// (`send().await?.error_for_status()`), which returns as soon as the
+    /// response *headers* arrive. The body stream and tar extraction happened
+    /// *after* the retry closure returned, so a corrupt or truncated download
+    /// surfaced immediately as an `UnexpectedEof` extraction error without ever
+    /// being retried. A corrupt archive served with a 200 status must instead
+    /// be retried by the full download+extract pipeline.
+    #[tokio::test]
+    async fn test_download_and_extract_retries_on_corrupt_archive() {
+        let server = MockServer::start();
+        let mock = server.mock(|when, then| {
+            when.method(GET).path("/corrupt.tgz");
+            then.status(200)
+                .header("content-type", "application/octet-stream")
+                .body("this is not a valid gzip archive");
+        });
+
+        let temp_dir = TempDir::new().unwrap();
+        let target_dir = temp_dir.path().join("extracted");
+        let url = format!("{}/corrupt.tgz", server.base_url());
+
+        let result = download_and_extract_tgz_with_hash(&url, &target_dir, None).await;
+        assert!(result.is_err(), "corrupt archive should fail to extract: {result:?}");
+        assert!(
+            mock.hits() > 1,
+            "a corrupt download must be retried by the full pipeline, but it was only attempted {} time(s)",
+            mock.hits()
+        );
+    }
+
+    /// A complete-but-wrong download (hash mismatch) must also be retried,
+    /// since a transient corrupt/truncated download is the most likely cause of
+    /// an integrity mismatch for an immutable npm tarball.
+    #[tokio::test]
+    async fn test_download_and_extract_retries_on_hash_mismatch() {
+        let server = MockServer::start();
+        let mock_tgz = create_mock_package_tgz();
+        let mock = server.mock(|when, then| {
+            when.method(GET).path("/mismatch.tgz");
+            then.status(200).header("content-type", "application/octet-stream").body(mock_tgz);
+        });
+
+        let temp_dir = TempDir::new().unwrap();
+        let target_dir = temp_dir.path().join("extracted");
+        let url = format!("{}/mismatch.tgz", server.base_url());
+        let wrong_hash = "sha512.0000000000000000000000000000000000000000000000000000000000000000\
+             0000000000000000000000000000000000000000000000000000000000000000";
+
+        let result = download_and_extract_tgz_with_hash(&url, &target_dir, Some(wrong_hash)).await;
+        assert!(result.is_err(), "hash mismatch should fail: {result:?}");
+        assert!(
+            mock.hits() > 1,
+            "a hash mismatch must be retried, but it was only attempted {} time(s)",
+            mock.hits()
+        );
+    }
+
     #[tokio::test]
     #[ignore] // Flaky on musl/Alpine — temp file race condition
     async fn test_verify_file_hash_sha1() {

crates/vite_js_runtime/src/download.rs

@@ -41,73 +41,101 @@ pub async fn download_file(
 
     tracing::debug!("Downloading {url} to {target_path:?}");
 
-    let response = (|| async { client.get(url).send().await?.error_for_status() })
-        .retry(
-            ExponentialBuilder::default()
-                .with_jitter()
-                .with_min_delay(Duration::from_millis(500))
-                .with_max_times(3),
-        )
-        .await
-        .map_err(|e| Error::DownloadFailed {
-            url: url.into(),
-            reason: vite_shared::format_error_chain(&e).into(),
-        })?;
-
-    // Get Content-Length for progress bar
-    let total_size = response.content_length();
-
-    // Create progress bar (only in TTY and not in CI)
+    // Create progress bar (only in TTY and not in CI). Built once and reused
+    // across retry attempts; its position is reset at the start of every
+    // attempt so a retried download doesn't double-count bytes.
     let is_ci = vite_shared::EnvConfig::get().is_ci;
     let progress = if std::io::stderr().is_terminal() && !is_ci {
-        let pb = if let Some(size) = total_size {
-            let pb = ProgressBar::new(size);
-            pb.set_style(
-                ProgressStyle::default_bar()
-                    .template(
-                        "{msg}\n{spinner:.green} [{elapsed_precise}] [{bar:40.blue/white}] \
-                         {bytes}/{total_bytes} ({bytes_per_sec}, {eta})",
-                    )
-                    .expect("valid progress bar template")
-                    .progress_chars("#>-"),
-            );
-            pb
-        } else {
-            let pb = ProgressBar::new_spinner();
-            pb.set_style(
-                ProgressStyle::default_spinner()
-                    .template(
-                        "{msg}\n{spinner:.green} [{elapsed_precise}] {bytes} ({bytes_per_sec})",
-                    )
-                    .expect("valid spinner template"),
-            );
-            pb.enable_steady_tick(Duration::from_millis(100));
-            pb
-        };
+        let pb = ProgressBar::new_spinner();
+        pb.set_style(
+            ProgressStyle::default_spinner()
+                .template("{msg}\n{spinner:.green} [{elapsed_precise}] {bytes} ({bytes_per_sec})")
+                .expect("valid spinner template"),
+        );
+        pb.enable_steady_tick(Duration::from_millis(100));
         pb.set_message(message.to_string());
         Some(pb)
     } else {
         None
     };
 
-    // Stream to file with progress updates
-    let mut file = fs::File::create(target_path).await?;
-    let mut stream = response.bytes_stream();
+    // Make the request *and* the body stream a single retried unit, so a
+    // truncated download (bytes written != advertised Content-Length) triggers
+    // a re-download instead of surfacing as a corrupt archive later.
+    let result = (|| async {
+        let response = client.get(url).send().await?.error_for_status()?;
+
+        // Advertised length, used both for the progress bar and the
+        // truncation check below.
+        let total_size = response.content_length();
 
-    while let Some(chunk_result) = stream.next().await {
-        let chunk = chunk_result?;
         if let Some(ref pb) = progress {
-            pb.inc(chunk.len() as u64);
+            // Reset for this attempt.
+            pb.set_position(0);
+            if let Some(size) = total_size {
+                pb.set_length(size);
+                pb.set_style(
+                    ProgressStyle::default_bar()
+                        .template(
+                            "{msg}\n{spinner:.green} [{elapsed_precise}] [{bar:40.blue/white}] \
+                             {bytes}/{total_bytes} ({bytes_per_sec}, {eta})",
+                        )
+                        .expect("valid progress bar template")
+                        .progress_chars("#>-"),
+                );
+            }
         }
-        file.write_all(&chunk).await?;
-    }
 
-    file.flush().await?;
+        // Stream to file with progress updates.
+        let mut file = fs::File::create(target_path).await?;
+        let mut stream = response.bytes_stream();
+        let mut bytes_written: u64 = 0;
+
+        while let Some(chunk_result) = stream.next().await {
+            let chunk = chunk_result?;
+            bytes_written += chunk.len() as u64;
+            if let Some(ref pb) = progress {
+                pb.inc(chunk.len() as u64);
+            }
+            file.write_all(&chunk).await?;
+        }
+
+        file.flush().await?;
+
+        // Detect truncation: a short read against an advertised Content-Length
+        // is an incomplete download — error out so the retry re-downloads.
+        if let Some(expected_len) = total_size
+            && bytes_written != expected_len
+        {
+            return Err(Error::Io(std::io::Error::new(
+                std::io::ErrorKind::UnexpectedEof,
+                vite_str::format!(
+                    "incomplete download: expected {expected_len} bytes, got {bytes_written}"
+                )
+                .to_string(),
+            )));
+        }
+
+        Ok(())
+    })
+    .retry(
+        ExponentialBuilder::default()
+            .with_jitter()
+            .with_min_delay(Duration::from_millis(500))
+            .with_max_times(3),
+    )
+    .await
+    .map_err(|e| Error::DownloadFailed {
+        url: url.into(),
+        reason: vite_shared::format_error_chain(&e).into(),
+    });
 
     if let Some(pb) = progress {
         pb.finish_and_clear();
     }
 
+    result?;
+
     tracing::debug!("Download completed: {target_path:?}");
 
     Ok(())