fix: Retry plugin downloads that drop mid-body

go.mod

@@ -20,7 +20,7 @@ require (
 	github.com/thoas/go-funk v0.9.3
 	golang.org/x/exp v0.0.0-20260611194520-c48552f49976
 	google.golang.org/grpc v1.83.0
-	google.golang.org/protobuf v1.36.11
+	google.golang.org/protobuf v1.36.12
 	gopkg.in/yaml.v3 v3.0.1
 )
 

go.sum

@@ -173,8 +173,8 @@ google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa h1:
 google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8=
 google.golang.org/grpc v1.83.0 h1:JeNZEKJFbQxArAMl+hiytHauacDNqJUllNfmIMmpqnQ=
 google.golang.org/grpc v1.83.0/go.mod h1:kDyl6SKsiHKt0uylY5gtn5cEjkrIOhQOGDgIc4JGwzQ=
-google.golang.org/protobuf v1.36.11 h1:fV6ZwhNocDyBLK0dj+fg8ektcVegBBuEolpbTQyBNVE=
-google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco=
+google.golang.org/protobuf v1.36.12 h1:pJOKDDOyeXErUroCihFAd5LQuwXBSpVnKGrj5o/fwxc=
+google.golang.org/protobuf v1.36.12/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=

managedplugin/download.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"net/url"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -25,6 +24,7 @@ const (
 	DefaultDownloadDir = ".cq"
 	RetryAttempts      = 5
 	RetryWaitTime      = 1 * time.Second
+	MaxRetryWaitTime   = 8 * time.Second
 )
 
 func APIBaseURL() string {
@@ -71,28 +71,31 @@ func getURLLocation(ctx context.Context, org string, name string, version string
 	var (
 		err404 = errors.New("404")
 		err401 = errors.New("401")
-		err429 = errors.New("429")
 	)
 
 	options := []retry.Option{
 		retry.RetryIf(func(err error) bool {
-			return err == err401 || err == err429
+			// The classifier treats 401 as permanent; this probe has always
+			// retried it because the GitHub asset host returns it spuriously.
+			return errors.Is(err, err401) || isRetryableDownloadError(err)
 		}),
 		retry.Context(ctx),
-		retry.Attempts(RetryAttempts),
-		retry.Delay(RetryWaitTime),
+		retry.Attempts(downloadRetryAttempts),
+		retry.Delay(downloadRetryDelay),
+		retry.MaxDelay(downloadRetryMaxDelay),
 		retry.LastErrorOnly(true),
 	}
 	retrier := retry.New(options...)
 	for _, downloadURL := range urls {
+		urlForLog := redactURLQuery(downloadURL)
 		err := retrier.Do(func() error {
 			req, err := http.NewRequestWithContext(ctx, http.MethodGet, downloadURL, nil)
 			if err != nil {
-				return fmt.Errorf("failed create request %s: %w", downloadURL, err)
+				return fmt.Errorf("failed create request %s: %w", urlForLog, redactURLError(err))
 			}
 			resp, err := http.DefaultClient.Do(req)
 			if err != nil {
-				return fmt.Errorf("failed to get url %s: %w", downloadURL, err)
+				return fmt.Errorf("failed to get url %s: %w", urlForLog, redactURLError(err))
 			}
 			resp.Body.Close()
 			// Check server response
@@ -102,17 +105,18 @@ func getURLLocation(ctx context.Context, org string, name string, version string
 			case http.StatusNotFound:
 				return err404
 			case http.StatusUnauthorized:
-				fmt.Printf("Failed downloading %s with status code %d. Retrying\n", downloadURL, resp.StatusCode)
+				fmt.Printf("Failed downloading %s with status code %d. Retrying\n", urlForLog, resp.StatusCode)
 				return err401
-			case http.StatusTooManyRequests:
-				fmt.Printf("Failed downloading %s with status code %d. Retrying\n", downloadURL, resp.StatusCode)
-				return err429
 			default:
-				fmt.Printf("Failed downloading %s with status code %d\n", downloadURL, resp.StatusCode)
-				return fmt.Errorf("statusCode %d", resp.StatusCode)
+				if isRetryableStatusCode(resp.StatusCode) {
+					fmt.Printf("Failed downloading %s with status code %d. Retrying\n", urlForLog, resp.StatusCode)
+				} else {
+					fmt.Printf("Failed downloading %s with status code %d\n", urlForLog, resp.StatusCode)
+				}
+				return &httpStatusError{statusCode: resp.StatusCode}
 			}
 		})
-		if err == err404 {
+		if errors.Is(err, err404) {
 			continue
 		}
 		return downloadURL, err
@@ -340,48 +344,49 @@ func downloadFile(ctx context.Context, localPath string, downloadURL string, dop
 	}
 	defer out.Close()
 
-	errStatusCodeNotOK := errors.New("statusCode != 200")
-	errNotFound := errors.New("not found")
+	urlForLog := redactURLQuery(downloadURL)
 
 	checksum := ""
 	options := []retry.Option{
-		retry.RetryIf(func(err error) bool {
-			return errors.Is(err, errStatusCodeNotOK)
-		}),
+		retry.RetryIf(isRetryableDownloadError),
 		retry.Context(ctx),
-		retry.Attempts(RetryAttempts),
-		retry.Delay(RetryWaitTime),
+		retry.Attempts(downloadRetryAttempts),
+		retry.Delay(downloadRetryDelay),
+		retry.MaxDelay(downloadRetryMaxDelay),
 	}
 	retrier := retry.New(options...)
 	err = retrier.Do(func() error {
 		checksum = ""
+		// Each attempt rewrites the file from the start, so a body that was cut off
+		// mid-copy cannot leave its bytes in front of the next attempt's download.
+		if err := truncateFile(out); err != nil {
+			return err
+		}
+
 		// Get the data
 		req, err := http.NewRequestWithContext(ctx, http.MethodGet, downloadURL, nil)
 		if err != nil {
-			return fmt.Errorf("failed create request %s: %w", downloadURL, err)
+			return fmt.Errorf("failed create request %s: %w", urlForLog, redactURLError(err))
 		}
 
 		// Do http request
 		resp, err := http.DefaultClient.Do(req)
 		if err != nil {
-			return fmt.Errorf("failed to get url %s: %w", downloadURL, err)
+			return fmt.Errorf("failed to get url %s: %w", urlForLog, redactURLError(err))
 		}
 		defer resp.Body.Close()
 		// Check server response
 		if resp.StatusCode == http.StatusNotFound {
 			return errNotFound
 		} else if resp.StatusCode != http.StatusOK {
-			fmt.Printf("Failed downloading %s with status code %d. Retrying\n", downloadURL, resp.StatusCode)
-			return errStatusCodeNotOK
+			if isRetryableStatusCode(resp.StatusCode) {
+				fmt.Printf("Failed downloading %s with status code %d. Retrying\n", urlForLog, resp.StatusCode)
+			} else {
+				fmt.Printf("Failed downloading %s with status code %d\n", urlForLog, resp.StatusCode)
+			}
+			return &httpStatusError{statusCode: resp.StatusCode}
 		}
 
-		urlForLog := downloadURL
-		parsedURL, err := url.Parse(downloadURL)
-		if err == nil {
-			parsedURL.RawQuery = ""
-			parsedURL.Fragment = ""
-			urlForLog = parsedURL.String()
-		}
 		fmt.Printf("Downloading %s\n", urlForLog)
 
 		s := sha256.New()
@@ -393,22 +398,35 @@ func downloadFile(ctx context.Context, localPath string, downloadURL string, dop
 		}
 
 		// Write the body to file
-		_, err = io.Copy(io.MultiWriter(writers...), resp.Body)
+		written, err := io.Copy(io.MultiWriter(writers...), resp.Body)
 		if err != nil {
 			return fmt.Errorf("failed to copy body to file %s: %w", out.Name(), err)
 		}
+		if resp.ContentLength >= 0 && written != resp.ContentLength {
+			return fmt.Errorf("%w: %s got %d bytes, want %d", errShortRead, out.Name(), written, resp.ContentLength)
+		}
 		checksum = fmt.Sprintf("%x", s.Sum(nil))
 		return nil
 	})
 	if err != nil {
 		if errors.Is(err, errNotFound) {
 			return "", errNotFound
 		}
-		return "", fmt.Errorf("failed downloading URL %q. Error %w", downloadURL, err)
+		return "", fmt.Errorf("failed downloading URL %q. Error %w", urlForLog, err)
 	}
 	return checksum, nil
 }
 
+func truncateFile(f *os.File) error {
+	if err := f.Truncate(0); err != nil {
+		return fmt.Errorf("failed to truncate file %s: %w", f.Name(), err)
+	}
+	if _, err := f.Seek(0, io.SeekStart); err != nil {
+		return fmt.Errorf("failed to rewind file %s: %w", f.Name(), err)
+	}
+	return nil
+}
+
 func downloadProgressBar(maxBytes int64, description ...string) *progressbar.ProgressBar {
 	desc := ""
 	if len(description) > 0 {

managedplugin/download_retry.go

@@ -0,0 +1,146 @@
+package managedplugin
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"io"
+	"net"
+	"net/http"
+	"net/url"
+	"strings"
+	"syscall"
+)
+
+var (
+	errNotFound  = errors.New("not found")
+	errShortRead = errors.New("truncated response body")
+)
+
+// Overridable so tests do not pay the real backoff.
+var (
+	downloadRetryAttempts = uint(RetryAttempts)
+	downloadRetryDelay    = RetryWaitTime
+	downloadRetryMaxDelay = MaxRetryWaitTime
+)
+
+type httpStatusError struct {
+	statusCode int
+}
+
+func (e *httpStatusError) Error() string {
+	return fmt.Sprintf("statusCode %d", e.statusCode)
+}
+
+func isRetryableStatusCode(statusCode int) bool {
+	switch statusCode {
+	case http.StatusRequestTimeout, http.StatusTooManyRequests:
+		return true
+	}
+	return statusCode >= http.StatusInternalServerError
+}
+
+// Go does not export the HTTP/2 stream and connection error types used by the
+// net/http transport, and on Windows the syscall.E* constants are synthetic
+// values that never match a real WSA socket error, so these failures can only be
+// matched on their message.
+var transientTransportMessages = []string{
+	"stream error",
+	"server sent goaway",
+	"connection reset by peer",
+	"broken pipe",
+	"unexpected eof",
+	"use of closed network connection",
+	"server closed idle connection",
+	"transport connection broken",
+	"i/o timeout",
+	"connection refused",
+	"no such host",
+	"network is unreachable",
+	"no route to host",
+	// Windows WSAECONNRESET, WSAECONNREFUSED and WSAETIMEDOUT respectively.
+	"forcibly closed by the remote host",
+	"actively refused it",
+	"did not properly respond after a period of time",
+}
+
+func isRetryableDownloadError(err error) bool {
+	if err == nil {
+		return false
+	}
+	if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
+		return false
+	}
+	if errors.Is(err, errNotFound) {
+		return false
+	}
+
+	var statusErr *httpStatusError
+	if errors.As(err, &statusErr) {
+		return isRetryableStatusCode(statusErr.statusCode)
+	}
+
+	switch {
+	case errors.Is(err, errShortRead),
+		errors.Is(err, io.ErrUnexpectedEOF),
+		errors.Is(err, io.EOF),
+		errors.Is(err, syscall.ECONNRESET),
+		errors.Is(err, syscall.ECONNREFUSED),
+		errors.Is(err, syscall.EPIPE),
+		errors.Is(err, syscall.ETIMEDOUT),
+		errors.Is(err, syscall.EHOSTUNREACH),
+		errors.Is(err, syscall.ENETUNREACH):
+		return true
+	}
+
+	// The asset host is fixed, so a resolution failure against it is a resolver
+	// problem rather than a bad name.
+	var dnsErr *net.DNSError
+	if errors.As(err, &dnsErr) {
+		return true
+	}
+
+	var netErr net.Error
+	if errors.As(err, &netErr) && netErr.Timeout() {
+		return true
+	}
+
+	msg := strings.ToLower(err.Error())
+	for _, transient := range transientTransportMessages {
+		if strings.Contains(msg, transient) {
+			return true
+		}
+	}
+	return false
+}
+
+// redactURLQuery strips the query string so that the signed download token never
+// reaches stdout or a log aggregator.
+func redactURLQuery(rawURL string) string {
+	parsed, err := url.Parse(rawURL)
+	if err != nil {
+		return rawURL
+	}
+	parsed.RawQuery = ""
+	parsed.Fragment = ""
+	return parsed.String()
+}
+
+// redactURLError rewrites the URL that *url.Error prints verbatim. Wrapping such
+// an error re-exposes the signed token that redactURLQuery removed from the
+// surrounding message.
+func redactURLError(err error) error {
+	var urlErr *url.Error
+	if errors.As(err, &urlErr) {
+		urlErr.URL = redactURLQuery(urlErr.URL)
+	}
+	return err
+}
+
+// IsTransientDownloadError reports whether err is a transient plugin download
+// failure - a network or server-side problem rather than a bad plugin reference.
+// Callers use it to keep advice about plugin resolution off errors that have
+// nothing to do with it.
+func IsTransientDownloadError(err error) bool {
+	return isRetryableDownloadError(err)
+}