From 34077e9d9ef59206287a84a02f9b09fcec600052 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Behrendt?= Date: Thu, 8 Oct 2026 15:37:14 +0200 Subject: [PATCH] fix(cli): retry locale downloads on transport errors --- clients/cli/cmd/internal/pull.go | 70 +++++++++++---- clients/cli/cmd/internal/pull_retry_test.go | 94 +++++++++++++++++++++ 2 files changed, 148 insertions(+), 16 deletions(-) create mode 100644 clients/cli/cmd/internal/pull_retry_test.go diff --git a/clients/cli/cmd/internal/pull.go b/clients/cli/cmd/internal/pull.go index f8810157f..72e514a0f 100644 --- a/clients/cli/cmd/internal/pull.go +++ b/clients/cli/cmd/internal/pull.go @@ -31,9 +31,13 @@ const ( const maxParallelDownloads = 4 // Phrase API allows max 4 concurrent requests +const maxDownloadAttempts = 3 + var ( Config *phrase.Config errNotModified = errors.New("not modified") + + downloadRetrySleep = time.Sleep ) type PullCommand struct { @@ -315,7 +319,7 @@ func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile waitForGate := func() { gate.RLock(); gate.RUnlock() } waitForGate() - file, response, err := client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &opts) + file, response, err := downloadLocale(client, target.ProjectID, localeFile.ID, &opts) if err != nil { if response != nil && response.StatusCode == 304 { return nil, response, nil @@ -329,7 +333,7 @@ func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile } opts.IfNoneMatch = optional.String{} opts.IfModifiedSince = optional.String{} - file, response, err = client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &opts) + file, response, err = downloadLocale(client, target.ProjectID, localeFile.ID, &opts) if err != nil { return nil, response, err } @@ -340,6 +344,36 @@ func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile return file, response, nil } +// downloadLocale calls the locale download endpoint and retries transport +// errors, e.g. an HTTP/2 stream reset by an intermediary while reading the +// body. API errors, including 304 and rate limiting, are returned to the +// caller unchanged. +func downloadLocale(client *phrase.APIClient, projectID string, localeID string, opts *phrase.LocaleDownloadOpts) (*os.File, *phrase.APIResponse, error) { + var file *os.File + var response *phrase.APIResponse + err := retryTransient(func() (bool, error) { + var err error + file, response, err = client.LocalesApi.LocaleDownload(Auth, projectID, localeID, opts) + var apiErr phrase.GenericOpenAPIError + return !errors.As(err, &apiErr), err + }) + return file, response, err +} + +// retryTransient calls fn up to maxDownloadAttempts times for as long as it +// fails with an error it reports as transient. +func retryTransient(fn func() (transient bool, err error)) error { + for attempt := 1; ; attempt++ { + transient, err := fn() + if err == nil || !transient || attempt >= maxDownloadAttempts { + return err + } + delay := time.Second << (attempt - 1) // 1s, 2s + debugFprintln(fmt.Sprintf("Download attempt %d/%d failed: %s, retrying in %s", attempt, maxDownloadAttempts, err, delay)) + downloadRetrySleep(delay) + } +} + // buildDownloadOpts prepares the LocaleDownloadOpts for a locale file download. func (target *Target) buildDownloadOpts(localeFile *LocaleFile) (phrase.LocaleDownloadOpts, error) { opts := phrase.LocaleDownloadOpts{} @@ -425,7 +459,7 @@ func (target *Target) downloadSynchronously(client *phrase.APIClient, localeFile applyCacheHeaders(cache, cacheKey, &downloadOpts) } - file, response, err := client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &downloadOpts) + file, response, err := downloadLocale(client, target.ProjectID, localeFile.ID, &downloadOpts) if err != nil { if response != nil && response.StatusCode == 304 { debugFprintln("Not modified (304), skipping", localeFile.Path) @@ -436,7 +470,7 @@ func (target *Target) downloadSynchronously(client *phrase.APIClient, localeFile // Strip conditional headers on retry to get a full response downloadOpts.IfNoneMatch = optional.String{} downloadOpts.IfModifiedSince = optional.String{} - file, response, err = client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &downloadOpts) + file, response, err = downloadLocale(client, target.ProjectID, localeFile.ID, &downloadOpts) if err != nil { return err } @@ -477,23 +511,27 @@ func copyToDestination(file *os.File, path string) error { func downloadExportedLocale(url string, localName string) error { debugFprintln("Downloading file from ", url) - file, err := os.Create(localName) - if err != nil { - return err - } - defer file.Close() request, err := http.NewRequest("GET", url, nil) if err != nil { return err } request.Header.Set("Authorization", "Bearer "+Config.Credentials.Token) - response, err := http.DefaultClient.Do(request) - if err != nil { - return err - } - defer response.Body.Close() - _, err = io.Copy(file, response.Body) - return err + return retryTransient(func() (bool, error) { + response, err := http.DefaultClient.Do(request) + if err != nil { + return true, err + } + defer response.Body.Close() + if response.StatusCode >= 300 { + return false, fmt.Errorf("downloading exported file failed: %s", response.Status) + } + // Read the whole body first so a failed attempt never leaves a partial file. + data, err := io.ReadAll(response.Body) + if err != nil { + return true, err + } + return false, os.WriteFile(localName, data, 0o666) + }) } // asyncDownloadParams converts the optional parameters from the Pull command into a LocaleDownloadCreateParameters struct. diff --git a/clients/cli/cmd/internal/pull_retry_test.go b/clients/cli/cmd/internal/pull_retry_test.go new file mode 100644 index 000000000..3e74b56d6 --- /dev/null +++ b/clients/cli/cmd/internal/pull_retry_test.go @@ -0,0 +1,94 @@ +package internal + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "sync/atomic" + "testing" + "time" + + "github.com/phrase/phrase-go/v4" +) + +func TestDownloadRetries(t *testing.T) { + downloads := map[string]func(url, path string) error{ + "locale download": func(url, path string) error { + cfg := phrase.NewConfiguration() + cfg.BasePath = url + file, _, err := downloadLocale(phrase.NewAPIClient(cfg), "proj1", "en", &phrase.LocaleDownloadOpts{}) + if err != nil { + return err + } + return copyToDestination(file, path) + }, + "async export download": downloadExportedLocale, + } + cases := []struct { + name string + aborts int32 // requests dropped mid-body before responding with status + status int + wantErr bool + wantRequests int32 + }{ + {"retries interrupted body", 2, http.StatusOK, false, 3}, + {"gives up after max attempts", 3, http.StatusOK, true, 3}, + {"does not retry API errors", 0, http.StatusServiceUnavailable, true, 1}, + } + + prevAuth, prevConfig, prevSleep := Auth, Config, downloadRetrySleep + Auth, Config, downloadRetrySleep = context.Background(), &phrase.Config{}, func(time.Duration) {} + t.Cleanup(func() { Auth, Config, downloadRetrySleep = prevAuth, prevConfig, prevSleep }) + + for name, download := range downloads { + for _, c := range cases { + t.Run(name+"/"+c.name, func(t *testing.T) { + var requests int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if atomic.AddInt32(&requests, 1) <= c.aborts { + abortBody(t, w) + return + } + w.WriteHeader(c.status) + io.WriteString(w, "new") + })) + defer srv.Close() + path := filepath.Join(t.TempDir(), "en.json") + os.WriteFile(path, []byte("old"), 0o644) + + err := download(srv.URL, path) + + if (err != nil) != c.wantErr { + t.Errorf("expected error: %v, got %v", c.wantErr, err) + } + if requests != c.wantRequests { + t.Errorf("expected %d requests, got %d", c.wantRequests, requests) + } + want := "new" + if c.wantErr { + want = "old" + } + if data, _ := os.ReadFile(path); string(data) != want { + t.Errorf("expected file content %q, got %q", want, data) + } + }) + } + } +} + +// abortBody announces a body but drops the connection before sending it all, +// so the client fails while reading the response body. +func abortBody(t *testing.T, w http.ResponseWriter) { + w.Header().Set("Content-Length", "100") + io.WriteString(w, "partial") + w.(http.Flusher).Flush() + conn, _, err := w.(http.Hijacker).Hijack() + if err != nil { + t.Errorf("hijack failed: %v", err) + return + } + conn.Close() +}