fix(internal/fetch): preserve last error in download retry loop (#3188)

The downloadTarball function was using := instead of = when capturing
the error from downloadAttempt. This created a new variable scoped to
the if statement, shadowing the function-level err variable.

When all retry attempts failed, the error message showed `last
error=%!w(<nil>)` because the outer err was never assigned.

A test is added to verify the error message includes the actual failure.
TestDownloadTarballRetry is also refactor to remove the unnecessary
t.Run.

Author: julieqiu

internal/fetch/fetch.go

@@ -38,6 +38,8 @@ var (
 	defaultBackoff      = 10 * time.Second
 )
 
+const maxDownloadRetries = 3
+
 // Endpoints defines the endpoints used to access GitHub.
 type Endpoints struct {
 	// API defines the endpoint used to make API calls.
@@ -142,7 +144,7 @@ func TarballLink(githubDownload string, repo *Repo, sha string) string {
 
 // DownloadTarball downloads a tarball from the given url to the target
 // path, verifying its SHA256 checksum matches expectedSha256. It retries up to
-// 3 times with exponential backoff on failure.
+// maxDownloadRetries times with exponential backoff on failure.
 func DownloadTarball(ctx context.Context, target, url, expectedSha256 string) error {
 	if fileExists(target) {
 		return nil
@@ -169,7 +171,6 @@ func DownloadTarball(ctx context.Context, target, url, expectedSha256 string) er
 	if err := downloadTarball(ctx, tempPath, url); err != nil {
 		return err
 	}
-
 	sha, err := computeSHA256(tempPath)
 	if err != nil {
 		return err
@@ -184,10 +185,10 @@ func DownloadTarball(ctx context.Context, target, url, expectedSha256 string) er
 }
 
 // downloadTarball downloads a tarball from the given source URL to the target
-// path. It retries up to 3 times with exponential backoff on failure.
+// path. It retries up to maxDownloadRetries times with exponential backoff on failure.
 func downloadTarball(ctx context.Context, target, source string) error {
 	var err error
-	for i := range 3 {
+	for i := range maxDownloadRetries {
 		if i > 0 {
 			select {
 			case <-time.After(defaultBackoff):
@@ -197,16 +198,15 @@ func downloadTarball(ctx context.Context, target, source string) error {
 			}
 		}
 
-		if err := downloadAttempt(ctx, target, source); err != nil {
+		if err = downloadAttempt(ctx, target, source); err != nil {
 			if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
 				return err
 			}
 			continue
 		}
 		return nil
-
 	}
-	return fmt.Errorf("download failed after 3 attempts, last error=%w", err)
+	return fmt.Errorf("download failed after %d attempts, last error=%w", maxDownloadRetries, err)
 }
 
 func downloadAttempt(ctx context.Context, target, source string) (err error) {
@@ -237,11 +237,9 @@ func downloadAttempt(ctx context.Context, target, source string) (err error) {
 	if response.StatusCode >= 300 {
 		return fmt.Errorf("http error in download %s", response.Status)
 	}
-
 	if _, err := io.Copy(file, response.Body); err != nil {
 		return err
 	}
-
 	return nil
 }
 

internal/fetch/fetch_test.go

@@ -590,43 +590,62 @@ func TestLatestCommitAndChecksum(t *testing.T) {
 	}
 }
 
-func TestDownloadTarballRetry(t *testing.T) {
-	t.Run("succeeds after a few retries", func(t *testing.T) {
-		// Set a short backoff for this test to speed up retries.
-		defaultBackoff = time.Millisecond
-		t.Cleanup(func() {
-			defaultBackoff = 10 * time.Second
-		})
-		testDir := t.TempDir()
-		tarball := makeTestContents(t)
-		var requestCount int
-		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			requestCount++
-			if requestCount < 3 {
-				w.WriteHeader(http.StatusInternalServerError)
-				return
-			}
-			w.WriteHeader(http.StatusOK)
-			w.Write(tarball.Contents)
-		}))
-		defer server.Close()
+func TestDownloadTarballRetryErrorIncludesLastFailure(t *testing.T) {
+	defaultBackoff = time.Millisecond
+	t.Cleanup(func() {
+		defaultBackoff = 10 * time.Second
+	})
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusInternalServerError)
+	}))
+	defer server.Close()
 
-		target := path.Join(testDir, "target-file")
-		if err := DownloadTarball(t.Context(), target, server.URL+"/test.tar.gz", tarball.Sha256); err != nil {
-			t.Fatal(err)
-		}
+	target := path.Join(t.TempDir(), "target-file")
+	err := DownloadTarball(t.Context(), target, server.URL+"/test.tar.gz", "any-sha")
+	if err == nil {
+		t.Fatal("expected an error")
+	}
+	if strings.Contains(err.Error(), "<nil>") {
+		t.Errorf("error should contain the last failure, not <nil>: %v", err)
+	}
+	if !strings.Contains(err.Error(), "500") {
+		t.Errorf("error should mention the HTTP status code: %v", err)
+	}
+}
 
-		if requestCount != 3 {
-			t.Errorf("expected 3 requests, got %d", requestCount)
-		}
-		got, err := os.ReadFile(target)
-		if err != nil {
-			t.Fatal(err)
-		}
-		if diff := cmp.Diff(tarball.Contents, got); diff != "" {
-			t.Errorf("mismatch (-want +got):\n%s", diff)
-		}
+func TestDownloadTarballRetrySucceeds(t *testing.T) {
+	defaultBackoff = time.Millisecond
+	t.Cleanup(func() {
+		defaultBackoff = 10 * time.Second
 	})
+	tarball := makeTestContents(t)
+	var requestCount int
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		requestCount++
+		if requestCount < 3 {
+			w.WriteHeader(http.StatusInternalServerError)
+			return
+		}
+		w.WriteHeader(http.StatusOK)
+		w.Write(tarball.Contents)
+	}))
+	defer server.Close()
+
+	target := path.Join(t.TempDir(), "target-file")
+	if err := DownloadTarball(t.Context(), target, server.URL+"/test.tar.gz", tarball.Sha256); err != nil {
+		t.Fatal(err)
+	}
+
+	if requestCount != 3 {
+		t.Errorf("expected 3 requests, got %d", requestCount)
+	}
+	got, err := os.ReadFile(target)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if diff := cmp.Diff(tarball.Contents, got); diff != "" {
+		t.Errorf("mismatch (-want +got):\n%s", diff)
+	}
 }
 
 func TestLatestCommitAndChecksumFailure(t *testing.T) {