Skip to content

Commit 04f5add

Browse files
authored
fix: Prevent panic on non-retryable download errors (#619)
<!-- CURSOR_AGENT_PR_BODY_BEGIN --> #### Summary Fixes a panic in `downloadFile` caused by an unsafe type assertion (`err.(retry.Error)`) when the retry library returns a plain error. Changes included: - Replace string-based retry classification with sentinel errors + `errors.Is` - Remove the unsafe type assertion path and return clean errors for both retry and non-retry failures - Add regression tests that verify `downloadFile` does not panic on: - invalid/non-retryable request errors - 404 responses In `github.com/avast/retry-go/v5`, it can return a non-retry.Error in these cases: 1. `LastErrorOnly(true)` is set. It returns the last underlying error directly. 2. Context is already done before retries start. Do() returns `context.Cause(ctx)` directly (often `context.Canceled` / `context.DeadlineExceeded`, which can be `*errors.errorString`). 3. Attempts(0) / UntilSucceeded() mode. If the function returns an unrecoverable error (`retry.Unrecoverable(...)`) or `RetryIf` says “don’t retry”, it returns that raw error directly. If context is canceled in this mode (and `WrapContextErrorWithLastError(false)`), it also returns raw context error. ---
1 parent 28543e4 commit 04f5add

2 files changed

Lines changed: 56 additions & 7 deletions

File tree

managedplugin/download.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -340,10 +340,13 @@ func downloadFile(ctx context.Context, localPath string, downloadURL string, dop
340340
}
341341
defer out.Close()
342342

343+
errStatusCodeNotOK := errors.New("statusCode != 200")
344+
errNotFound := errors.New("not found")
345+
343346
checksum := ""
344347
options := []retry.Option{
345348
retry.RetryIf(func(err error) bool {
346-
return err.Error() == "statusCode != 200"
349+
return errors.Is(err, errStatusCodeNotOK)
347350
}),
348351
retry.Context(ctx),
349352
retry.Attempts(RetryAttempts),
@@ -366,10 +369,10 @@ func downloadFile(ctx context.Context, localPath string, downloadURL string, dop
366369
defer resp.Body.Close()
367370
// Check server response
368371
if resp.StatusCode == http.StatusNotFound {
369-
return errors.New("not found")
372+
return errNotFound
370373
} else if resp.StatusCode != http.StatusOK {
371374
fmt.Printf("Failed downloading %s with status code %d. Retrying\n", downloadURL, resp.StatusCode)
372-
return errors.New("statusCode != 200")
375+
return errStatusCodeNotOK
373376
}
374377

375378
urlForLog := downloadURL
@@ -398,10 +401,8 @@ func downloadFile(ctx context.Context, localPath string, downloadURL string, dop
398401
return nil
399402
})
400403
if err != nil {
401-
for _, e := range err.(retry.Error) {
402-
if e.Error() == "not found" {
403-
return "", e
404-
}
404+
if errors.Is(err, errNotFound) {
405+
return "", errNotFound
405406
}
406407
return "", fmt.Errorf("failed downloading URL %q. Error %w", downloadURL, err)
407408
}

managedplugin/download_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,62 @@ package managedplugin
22

33
import (
44
"context"
5+
"net/http"
6+
"net/http/httptest"
57
"path"
8+
"path/filepath"
69
"strings"
710
"testing"
811

912
cloudquery_api "github.com/cloudquery/cloudquery-api-go"
1013
"github.com/rs/zerolog"
1114
)
1215

16+
func TestDownloadFileNonRetryErrorDoesNotPanic(t *testing.T) {
17+
t.Helper()
18+
19+
localPath := filepath.Join(t.TempDir(), "plugin.zip")
20+
21+
defer func() {
22+
if r := recover(); r != nil {
23+
t.Fatalf("downloadFile panicked: %v", r)
24+
}
25+
}()
26+
27+
_, err := downloadFile(context.Background(), localPath, "://invalid-url", DownloaderOptions{NoProgress: true})
28+
if err == nil {
29+
t.Fatal("expected error, got nil")
30+
}
31+
if !strings.Contains(err.Error(), "failed downloading URL") {
32+
t.Fatalf("expected wrapped download error, got: %v", err)
33+
}
34+
}
35+
36+
func TestDownloadFileNotFoundDoesNotPanic(t *testing.T) {
37+
t.Helper()
38+
39+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
40+
w.WriteHeader(http.StatusNotFound)
41+
}))
42+
t.Cleanup(server.Close)
43+
44+
localPath := filepath.Join(t.TempDir(), "plugin.zip")
45+
46+
defer func() {
47+
if r := recover(); r != nil {
48+
t.Fatalf("downloadFile panicked: %v", r)
49+
}
50+
}()
51+
52+
_, err := downloadFile(context.Background(), localPath, server.URL, DownloaderOptions{NoProgress: true})
53+
if err == nil {
54+
t.Fatal("expected error, got nil")
55+
}
56+
if err.Error() != "not found" {
57+
t.Fatalf("expected not found error, got: %v", err)
58+
}
59+
}
60+
1361
func TestDownloadPluginFromGithubIntegration(t *testing.T) {
1462
tmp := t.TempDir()
1563
cases := []struct {

0 commit comments

Comments
 (0)