fix: Retry plugin downloads that drop mid-body

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 {
@@ -340,48 +340,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.MaxDelay(MaxRetryWaitTime),
 	}
 	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, 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, 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 +394,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,109 @@
+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")
+)
+
+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, so the mid-body resets we get from the asset CDN 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",
+}
+
+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.EPIPE),
+		errors.Is(err, syscall.ETIMEDOUT):
+		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()
+}
+
+// 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)
+}