From f3c1162001fd2562e9f75024d9e33120d753d709 Mon Sep 17 00:00:00 2001 From: ian <141902143+yumosx@users.noreply.github.com> Date: Thu, 26 Feb 2026 17:35:26 +0800 Subject: [PATCH] feat(skills): add retry for HTTP requests in skill installer (#261) * feat(skills): add retry mechanism for HTTP requests Implement a retry mechanism with exponential backoff for HTTP requests in the skill installer. This improves reliability when fetching skills from GitHub by automatically retrying failed requests up to 3 times. Add comprehensive tests to verify retry behavior under different scenarios including success on different attempts and proper delay between retries. * fix: improve http request retry logic with status code checks Add shouldRetry helper function to determine retryable status codes. Close response body between retry attempts and break early for non-retryable status codes. * refactor: remove unused BuiltinSkill struct The struct was not being used anywhere in the codebase, so it's safe to remove it to reduce clutter and improve maintainability. * refactor(http): move retry logic to utils package Extract HTTP retry functionality from skills package to utils for better reusability Add context-aware sleep function and comprehensive tests * refactor(http): extract retry delay unit to variable Extract hardcoded retry delay unit to a variable for better testability and flexibility. Update tests to use milliseconds for faster execution while maintaining the same behavior. * test(http_retry): remove t.Parallel from test cases * test(http_retry): remove redundant test cases for retry success The removed test cases for success on second and third attempts were redundant since the retry logic is already covered by other tests. This simplifies the test suite while maintaining coverage. --- pkg/skills/installer.go | 6 +- pkg/utils/http_retry.go | 57 +++++++++++++++++ pkg/utils/http_retry_test.go | 118 +++++++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 2 deletions(-) create mode 100644 pkg/utils/http_retry.go create mode 100644 pkg/utils/http_retry_test.go diff --git a/pkg/skills/installer.go b/pkg/skills/installer.go index 3210509df..f9b5705f1 100644 --- a/pkg/skills/installer.go +++ b/pkg/skills/installer.go @@ -9,6 +9,8 @@ import ( "os" "path/filepath" "time" + + "github.com/sipeed/picoclaw/pkg/utils" ) type SkillInstaller struct { @@ -44,7 +46,7 @@ func (si *SkillInstaller) InstallFromGitHub(ctx context.Context, repo string) er return fmt.Errorf("failed to create request: %w", err) } - resp, err := client.Do(req) + resp, err := utils.DoRequestWithRetry(client, req) if err != nil { return fmt.Errorf("failed to fetch skill: %w", err) } @@ -94,7 +96,7 @@ func (si *SkillInstaller) ListAvailableSkills(ctx context.Context) ([]AvailableS return nil, fmt.Errorf("failed to create request: %w", err) } - resp, err := client.Do(req) + resp, err := utils.DoRequestWithRetry(client, req) if err != nil { return nil, fmt.Errorf("failed to fetch skills list: %w", err) } diff --git a/pkg/utils/http_retry.go b/pkg/utils/http_retry.go new file mode 100644 index 000000000..e90fa2129 --- /dev/null +++ b/pkg/utils/http_retry.go @@ -0,0 +1,57 @@ +package utils + +import ( + "context" + "fmt" + "net/http" + "time" +) + +const maxRetries = 3 + +var retryDelayUnit = time.Second + +func shouldRetry(statusCode int) bool { + return statusCode == http.StatusTooManyRequests || + statusCode >= 500 +} + +func DoRequestWithRetry(client *http.Client, req *http.Request) (*http.Response, error) { + var resp *http.Response + var err error + + for i := range maxRetries { + if i > 0 && resp != nil { + resp.Body.Close() + } + + resp, err = client.Do(req) + if err == nil { + if resp.StatusCode == http.StatusOK { + break + } + if !shouldRetry(resp.StatusCode) { + break + } + } + + if i < maxRetries-1 { + if err = sleepWithCtx(req.Context(), retryDelayUnit*time.Duration(i+1)); err != nil { + return nil, fmt.Errorf("failed to sleep: %w", err) + } + } + } + return resp, err +} + +func sleepWithCtx(ctx context.Context, d time.Duration) error { + timer := time.NewTimer(d) + defer timer.Stop() + + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } +} diff --git a/pkg/utils/http_retry_test.go b/pkg/utils/http_retry_test.go new file mode 100644 index 000000000..1c2dbe115 --- /dev/null +++ b/pkg/utils/http_retry_test.go @@ -0,0 +1,118 @@ +package utils + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDoRequestWithRetry(t *testing.T) { + retryDelayUnit = time.Millisecond + t.Cleanup(func() { retryDelayUnit = time.Second }) + + testcases := []struct { + name string + serverBehavior func(*httptest.Server) int + wantSuccess bool + wantAttempts int + }{ + { + name: "success-on-first-attempt", + serverBehavior: func(server *httptest.Server) int { + return 0 + }, + wantSuccess: true, + wantAttempts: 1, + }, + { + name: "fail-all-attempts", + serverBehavior: func(server *httptest.Server) int { + return 4 + }, + wantSuccess: false, + wantAttempts: 3, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + attempts := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts++ + if attempts <= tc.serverBehavior(nil) { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + w.Write([]byte("success")) + })) + + t.Cleanup(func() { + server.Close() + }) + + client := &http.Client{Timeout: 5 * time.Second} + req, err := http.NewRequest(http.MethodGet, server.URL, nil) + require.NoError(t, err) + + resp, err := DoRequestWithRetry(client, req) + + if tc.wantSuccess { + require.NoError(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusOK, resp.StatusCode) + resp.Body.Close() + } else { + require.NotNil(t, resp) + assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) + resp.Body.Close() + } + + assert.Equal(t, tc.wantAttempts, attempts) + }) + } +} + +func TestDoRequestWithRetry_Delay(t *testing.T) { + retryDelayUnit = time.Millisecond + t.Cleanup(func() { retryDelayUnit = time.Second }) + + var start time.Time + delays := []time.Duration{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if len(delays) == 0 { + delays = append(delays, 0) + w.WriteHeader(http.StatusInternalServerError) + return + } + if len(delays) == 1 { + start = time.Now() + delays = append(delays, 0) + w.WriteHeader(http.StatusInternalServerError) + return + } + if len(delays) == 2 { + elapsed := time.Since(start) + delays = append(delays, elapsed) + w.WriteHeader(http.StatusOK) + w.Write([]byte("success")) + } + })) + defer server.Close() + + client := &http.Client{Timeout: 10 * time.Second} + req, err := http.NewRequest(http.MethodGet, server.URL, nil) + require.NoError(t, err) + + resp, err := DoRequestWithRetry(client, req) + require.NoError(t, err) + require.NotNil(t, resp) + assert.Equal(t, http.StatusOK, resp.StatusCode) + resp.Body.Close() + + assert.GreaterOrEqual(t, delays[2], time.Millisecond) +}