From fbf6886734dcc1c9c377725c9d62676201535952 Mon Sep 17 00:00:00 2001 From: Thibaut Etienne Date: Fri, 3 Apr 2026 08:25:26 +0200 Subject: [PATCH 1/6] fix(CLI): cache ignored when using --parallel with phrase pull --- clients/cli/cmd/internal/pull.go | 91 +++++++++++++++++++++++++++----- 1 file changed, 78 insertions(+), 13 deletions(-) diff --git a/clients/cli/cmd/internal/pull.go b/clients/cli/cmd/internal/pull.go index 216cf5de..92a10f01 100644 --- a/clients/cli/cmd/internal/pull.go +++ b/clients/cli/cmd/internal/pull.go @@ -108,7 +108,7 @@ func (cmd *PullCommand) Run(config *phrase.Config) error { for _, target := range targets { var err error if cmd.Parallel && !cmd.Async { - err = target.PullParallel(client) + err = target.PullParallel(client, cache) } else { if cmd.Parallel && cmd.Async { print.Warn("--parallel is not supported with --async, ignoring parallel") @@ -181,9 +181,10 @@ func (target *Target) Pull(client *phrase.APIClient, async bool, cache *Download } type downloadResult struct { - message string - path string - errMsg string + message string + path string + errMsg string + notModified bool } func (target *Target) DownloadAndWriteToFile(client *phrase.APIClient, localeFile *LocaleFile, async bool, cache *DownloadCache) error { @@ -226,7 +227,7 @@ func (target *Target) DownloadAndWriteToFile(client *phrase.APIClient, localeFil return target.downloadSynchronously(client, localeFile, localVarOptionals, cache) } -func (target *Target) PullParallel(client *phrase.APIClient) error { +func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCache) error { if err := target.CheckPreconditions(); err != nil { return err } @@ -245,6 +246,7 @@ func (target *Target) PullParallel(client *phrase.APIClient) error { results := make([]downloadResult, len(localeFiles)) var rateMu sync.RWMutex + var cacheMu sync.Mutex ctx, cancel := context.WithTimeout(context.Background(), timeoutInMinutes) defer cancel() @@ -264,11 +266,57 @@ func (target *Target) PullParallel(client *phrase.APIClient) error { return err } - err = target.downloadWithRateGate(client, lf, opts, &rateMu) - if err != nil { - if openapiError, ok := err.(phrase.GenericOpenAPIError); ok { + var cacheKey string + if cache != nil { + cacheKey = CacheKey(target.ProjectID, lf.ID, opts) + cacheMu.Lock() + entry, ok := cache.Get(cacheKey) + cacheMu.Unlock() + if ok { + debugFprintln("Cache hit for", lf.ID, "- sending conditional request") + if entry.ETag != "" { + opts.IfNoneMatch = optional.NewString(entry.ETag) + } + if entry.LastModified != "" { + opts.IfModifiedSince = optional.NewString(entry.LastModified) + } + } else { + debugFprintln("Cache miss for", lf.ID) + } + } + + file, response, dlErr := target.downloadWithRateGateRaw(client, lf, opts, &rateMu) + + if response != nil && response.StatusCode == 304 { + debugFprintln("Not modified (304), skipping", lf.Path) + results[i] = downloadResult{ + message: lf.Message(), + path: lf.RelPath(), + notModified: true, + } + return nil + } + + if dlErr != nil { + if openapiError, ok := dlErr.(phrase.GenericOpenAPIError); ok { print.Warn("API response: %s", openapiError.Body()) } + dlErr = fmt.Errorf("%s for %s", dlErr, lf.Path) + results[i] = downloadResult{errMsg: dlErr.Error()} + return dlErr + } + + if cache != nil && response != nil { + etag := response.Header.Get("ETag") + lastMod := response.Header.Get("Last-Modified") + if etag != "" || lastMod != "" { + cacheMu.Lock() + cache.Set(cacheKey, CacheEntry{ETag: etag, LastModified: lastMod}) + cacheMu.Unlock() + } + } + + if err := copyToDestination(file, lf.Path); err != nil { err = fmt.Errorf("%s for %s", err, lf.Path) results[i] = downloadResult{errMsg: err.Error()} return err @@ -287,7 +335,9 @@ func (target *Target) PullParallel(client *phrase.APIClient) error { // Print results in original order: successes and failures var skipCount int for _, r := range results { - if r.path != "" { + if r.notModified { + print.Success("Not modified %s", r.path) + } else if r.path != "" { print.Success("Downloaded %s to %s", r.message, r.path) } else if r.errMsg != "" { print.Failure("Failed %s", r.errMsg) @@ -306,12 +356,24 @@ func (target *Target) PullParallel(client *phrase.APIClient) error { // Uses RWMutex as a broadcast gate: workers take a read lock (cheap, concurrent), // and a rate-limited worker takes the write lock to pause everyone until reset. func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile *LocaleFile, opts phrase.LocaleDownloadOpts, gate *sync.RWMutex) error { + file, _, err := target.downloadWithRateGateRaw(client, localeFile, opts, gate) + if err != nil { + return err + } + return copyToDestination(file, localeFile.Path) +} + +// downloadWithRateGateRaw is like downloadWithRateGate but returns the raw file and response. +func (target *Target) downloadWithRateGateRaw(client *phrase.APIClient, localeFile *LocaleFile, opts phrase.LocaleDownloadOpts, gate *sync.RWMutex) (*os.File, *phrase.APIResponse, error) { // Read-lock gate: blocks only when a writer (rate-limited worker) holds it gate.RLock() gate.RUnlock() file, response, err := client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &opts) if err != nil { + if response != nil && response.StatusCode == 304 { + return nil, response, nil + } if response != nil && response.Rate.Remaining == 0 { // TryLock ensures only one worker handles the rate limit pause. // Others will block on their next RLock until the pause is over. @@ -324,15 +386,18 @@ func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile gate.RUnlock() } - file, _, err = client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &opts) + // Strip conditional headers on retry to get a full response + opts.IfNoneMatch = optional.String{} + opts.IfModifiedSince = optional.String{} + file, response, err = client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &opts) if err != nil { - return err + return nil, response, err } } else { - return err + return nil, response, err } } - return copyToDestination(file, localeFile.Path) + return file, response, nil } // buildDownloadOpts prepares the LocaleDownloadOpts for a locale file download. From 77fff767cf6c2e6560b6df346728421e6fa98b31 Mon Sep 17 00:00:00 2001 From: Thibaut Etienne Date: Fri, 3 Apr 2026 08:33:34 +0200 Subject: [PATCH 2/6] refactor(CLI): simplify pull.go per code review --- clients/cli/cmd/internal/pull.go | 142 +++++++++++-------------------- 1 file changed, 49 insertions(+), 93 deletions(-) diff --git a/clients/cli/cmd/internal/pull.go b/clients/cli/cmd/internal/pull.go index 92a10f01..61f15add 100644 --- a/clients/cli/cmd/internal/pull.go +++ b/clients/cli/cmd/internal/pull.go @@ -105,14 +105,14 @@ func (cmd *PullCommand) Run(config *phrase.Config) error { } } + if cmd.Parallel && cmd.Async { + print.Warn("--parallel is not supported with --async, ignoring parallel") + } for _, target := range targets { var err error if cmd.Parallel && !cmd.Async { err = target.PullParallel(client, cache) } else { - if cmd.Parallel && cmd.Async { - print.Warn("--parallel is not supported with --async, ignoring parallel") - } err = target.Pull(client, cmd.Async, cache) } if err != nil { @@ -188,24 +188,9 @@ type downloadResult struct { } func (target *Target) DownloadAndWriteToFile(client *phrase.APIClient, localeFile *LocaleFile, async bool, cache *DownloadCache) error { - localVarOptionals := phrase.LocaleDownloadOpts{} - - if target.Params != nil { - localVarOptionals = target.Params.LocaleDownloadOpts - translationKeyPrefix, err := placeholders.ResolveTranslationKeyPrefix(target.Params.TranslationKeyPrefix, localeFile.Path) - if err != nil { - return err - } - localVarOptionals.TranslationKeyPrefix = translationKeyPrefix - } - - if localVarOptionals.FileFormat.Value() == "" { - localVarOptionals.FileFormat = optional.NewString(localeFile.FileFormat) - } - - if localeFile.Tag != "" { - localVarOptionals.Tags = optional.NewString(localeFile.Tag) - localVarOptionals.Tag = optional.EmptyString() + localVarOptionals, err := target.buildDownloadOpts(localeFile) + if err != nil { + return err } debugFprintln("Target file pattern:", target.File) @@ -270,30 +255,15 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach if cache != nil { cacheKey = CacheKey(target.ProjectID, lf.ID, opts) cacheMu.Lock() - entry, ok := cache.Get(cacheKey) + applyCacheHeaders(cache, cacheKey, &opts) cacheMu.Unlock() - if ok { - debugFprintln("Cache hit for", lf.ID, "- sending conditional request") - if entry.ETag != "" { - opts.IfNoneMatch = optional.NewString(entry.ETag) - } - if entry.LastModified != "" { - opts.IfModifiedSince = optional.NewString(entry.LastModified) - } - } else { - debugFprintln("Cache miss for", lf.ID) - } } - file, response, dlErr := target.downloadWithRateGateRaw(client, lf, opts, &rateMu) + file, response, dlErr := target.downloadWithRateGate(client, lf, opts, &rateMu) if response != nil && response.StatusCode == 304 { debugFprintln("Not modified (304), skipping", lf.Path) - results[i] = downloadResult{ - message: lf.Message(), - path: lf.RelPath(), - notModified: true, - } + results[i] = downloadResult{message: lf.Message(), path: lf.RelPath(), notModified: true} return nil } @@ -306,14 +276,10 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach return dlErr } - if cache != nil && response != nil { - etag := response.Header.Get("ETag") - lastMod := response.Header.Get("Last-Modified") - if etag != "" || lastMod != "" { - cacheMu.Lock() - cache.Set(cacheKey, CacheEntry{ETag: etag, LastModified: lastMod}) - cacheMu.Unlock() - } + if cache != nil { + cacheMu.Lock() + updateCache(cache, cacheKey, response) + cacheMu.Unlock() } if err := copyToDestination(file, lf.Path); err != nil { @@ -333,21 +299,16 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach waitErr := g.Wait() // Print results in original order: successes and failures - var skipCount int for _, r := range results { - if r.notModified { + switch { + case r.notModified: print.Success("Not modified %s", r.path) - } else if r.path != "" { + case r.path != "": print.Success("Downloaded %s to %s", r.message, r.path) - } else if r.errMsg != "" { + case r.errMsg != "": print.Failure("Failed %s", r.errMsg) - } else { - skipCount++ } } - if skipCount > 0 { - print.Warn("%d download(s) skipped due to earlier failure", skipCount) - } return waitErr } @@ -355,16 +316,7 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach // downloadWithRateGate downloads a locale file with rate-limit coordination. // Uses RWMutex as a broadcast gate: workers take a read lock (cheap, concurrent), // and a rate-limited worker takes the write lock to pause everyone until reset. -func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile *LocaleFile, opts phrase.LocaleDownloadOpts, gate *sync.RWMutex) error { - file, _, err := target.downloadWithRateGateRaw(client, localeFile, opts, gate) - if err != nil { - return err - } - return copyToDestination(file, localeFile.Path) -} - -// downloadWithRateGateRaw is like downloadWithRateGate but returns the raw file and response. -func (target *Target) downloadWithRateGateRaw(client *phrase.APIClient, localeFile *LocaleFile, opts phrase.LocaleDownloadOpts, gate *sync.RWMutex) (*os.File, *phrase.APIResponse, error) { +func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile *LocaleFile, opts phrase.LocaleDownloadOpts, gate *sync.RWMutex) (*os.File, *phrase.APIResponse, error) { // Read-lock gate: blocks only when a writer (rate-limited worker) holds it gate.RLock() gate.RUnlock() @@ -454,34 +406,43 @@ func (target *Target) downloadAsynchronously(client *phrase.APIClient, localeFil return fmt.Errorf("download failed: %s", asyncDownload.Error) } +func applyCacheHeaders(cache *DownloadCache, cacheKey string, opts *phrase.LocaleDownloadOpts) { + if entry, ok := cache.Get(cacheKey); ok { + debugFprintln("Cache hit for", cacheKey, "- sending conditional request") + if entry.ETag != "" { + opts.IfNoneMatch = optional.NewString(entry.ETag) + } + if entry.LastModified != "" { + opts.IfModifiedSince = optional.NewString(entry.LastModified) + } + } else { + debugFprintln("Cache miss for", cacheKey) + } +} + +func updateCache(cache *DownloadCache, cacheKey string, response *phrase.APIResponse) { + if response == nil { + return + } + etag, lastMod := response.Header.Get("ETag"), response.Header.Get("Last-Modified") + if etag != "" || lastMod != "" { + cache.Set(cacheKey, CacheEntry{ETag: etag, LastModified: lastMod}) + } +} + func (target *Target) downloadSynchronously(client *phrase.APIClient, localeFile *LocaleFile, downloadOpts phrase.LocaleDownloadOpts, cache *DownloadCache) error { - // Compute cache key before mutating opts var cacheKey string if cache != nil { cacheKey = CacheKey(target.ProjectID, localeFile.ID, downloadOpts) - if entry, ok := cache.Get(cacheKey); ok { - debugFprintln("Cache hit for", localeFile.ID, "- sending conditional request") - if entry.ETag != "" { - downloadOpts.IfNoneMatch = optional.NewString(entry.ETag) - } - if entry.LastModified != "" { - downloadOpts.IfModifiedSince = optional.NewString(entry.LastModified) - } - } else { - debugFprintln("Cache miss for", localeFile.ID) - } + applyCacheHeaders(cache, cacheKey, &downloadOpts) } file, response, err := client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &downloadOpts) - - // The SDK treats status >= 300 as an error, including 304 Not Modified. - // Check response.StatusCode before err to intercept the 304 case. - if response != nil && response.StatusCode == 304 { - debugFprintln("Not modified (304), skipping", localeFile.Path) - return errNotModified - } - if err != nil { + if response != nil && response.StatusCode == 304 { + debugFprintln("Not modified (304), skipping", localeFile.Path) + return errNotModified + } if response != nil && response.Rate.Remaining == 0 { waitForRateLimit(response.Rate) // Strip conditional headers on retry to get a full response @@ -496,13 +457,8 @@ func (target *Target) downloadSynchronously(client *phrase.APIClient, localeFile } } - // Update cache from response headers - if cache != nil && response != nil { - etag := response.Header.Get("ETag") - lastMod := response.Header.Get("Last-Modified") - if etag != "" || lastMod != "" { - cache.Set(cacheKey, CacheEntry{ETag: etag, LastModified: lastMod}) - } + if cache != nil { + updateCache(cache, cacheKey, response) } return copyToDestination(file, localeFile.Path) From 5af5678f89ca0dc9a9456776551c4e385f38efbb Mon Sep 17 00:00:00 2001 From: Thibaut Etienne Date: Fri, 3 Apr 2026 09:07:52 +0200 Subject: [PATCH 3/6] refactor(CLI): second-pass simplifications on pull.go --- clients/cli/cmd/internal/pull.go | 42 ++++++++++++-------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/clients/cli/cmd/internal/pull.go b/clients/cli/cmd/internal/pull.go index 61f15add..a7a3f367 100644 --- a/clients/cli/cmd/internal/pull.go +++ b/clients/cli/cmd/internal/pull.go @@ -108,14 +108,13 @@ func (cmd *PullCommand) Run(config *phrase.Config) error { if cmd.Parallel && cmd.Async { print.Warn("--parallel is not supported with --async, ignoring parallel") } + type pullFn func(*Target) error + pull := pullFn(func(t *Target) error { return t.Pull(client, cmd.Async, cache) }) + if cmd.Parallel && !cmd.Async { + pull = func(t *Target) error { return t.PullParallel(client, cache) } + } for _, target := range targets { - var err error - if cmd.Parallel && !cmd.Async { - err = target.PullParallel(client, cache) - } else { - err = target.Pull(client, cmd.Async, cache) - } - if err != nil { + if err := pull(target); err != nil { return err } } @@ -240,10 +239,6 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach for i, lf := range localeFiles { g.Go(func() error { - if ctx.Err() != nil { - return ctx.Err() - } - opts, err := target.buildDownloadOpts(lf) if err != nil { err = fmt.Errorf("%s for %s", err, lf.Path) @@ -317,9 +312,8 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach // Uses RWMutex as a broadcast gate: workers take a read lock (cheap, concurrent), // and a rate-limited worker takes the write lock to pause everyone until reset. func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile *LocaleFile, opts phrase.LocaleDownloadOpts, gate *sync.RWMutex) (*os.File, *phrase.APIResponse, error) { - // Read-lock gate: blocks only when a writer (rate-limited worker) holds it - gate.RLock() - gate.RUnlock() + waitForGate := func() { gate.RLock(); gate.RUnlock() } + waitForGate() file, response, err := client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &opts) if err != nil { @@ -327,18 +321,12 @@ func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile return nil, response, nil } if response != nil && response.Rate.Remaining == 0 { - // TryLock ensures only one worker handles the rate limit pause. - // Others will block on their next RLock until the pause is over. if gate.TryLock() { waitForRateLimit(response.Rate) gate.Unlock() } else { - // Another worker is already pausing; wait for it - gate.RLock() - gate.RUnlock() + waitForGate() } - - // Strip conditional headers on retry to get a full response opts.IfNoneMatch = optional.String{} opts.IfModifiedSince = optional.String{} file, response, err = client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &opts) @@ -387,7 +375,7 @@ func (target *Target) downloadAsynchronously(client *phrase.APIClient, localeFil return err } - for i := 0; asyncDownload.Status == "processing"; i++ { + for i := 0; asyncDownload.Status == "processing" && i <= asyncRetryCount; i++ { debugFprintln("Waiting for the files to be exported...") time.Sleep(asyncWaitTime) debugFprintln("Checking if the download is ready...") @@ -396,9 +384,9 @@ func (target *Target) downloadAsynchronously(client *phrase.APIClient, localeFil if err != nil { return err } - if i > asyncRetryCount { - return fmt.Errorf("download is taking too long") - } + } + if asyncDownload.Status == "processing" { + return fmt.Errorf("download is taking too long") } if asyncDownload.Status == "completed" { return downloadExportedLocale(asyncDownload.Result.Url, localeFile.Path) @@ -495,8 +483,8 @@ func downloadExportedLocale(url string, localName string) error { return err } defer response.Body.Close() - io.Copy(file, response.Body) - return nil + _, err = io.Copy(file, response.Body) + return err } // asyncDownloadParams converts the optional parameters from the Pull command into a LocaleDownloadCreateParameters struct. From 0a1e94cf80ee34c821aa01626e7af7aa21e1d589 Mon Sep 17 00:00:00 2001 From: Thibaut Etienne Date: Fri, 3 Apr 2026 09:13:15 +0200 Subject: [PATCH 4/6] fix(CLI): address expert review findings --- clients/cli/cmd/internal/download_cache.go | 9 +++++++++ clients/cli/cmd/internal/pull.go | 10 ++++------ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/clients/cli/cmd/internal/download_cache.go b/clients/cli/cmd/internal/download_cache.go index 4efc76e3..10096a06 100644 --- a/clients/cli/cmd/internal/download_cache.go +++ b/clients/cli/cmd/internal/download_cache.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "reflect" + "sync" "github.com/antihax/optional" "github.com/phrase/phrase-go/v4" @@ -28,6 +29,7 @@ type DownloadCache struct { Entries map[string]CacheEntry `json:"entries"` path string dirty bool + mu sync.Mutex } func LoadDownloadCache() *DownloadCache { @@ -65,11 +67,15 @@ func loadFromPath(path string) *DownloadCache { } func (dc *DownloadCache) Get(key string) (CacheEntry, bool) { + dc.mu.Lock() + defer dc.mu.Unlock() e, ok := dc.Entries[key] return e, ok } func (dc *DownloadCache) Set(key string, entry CacheEntry) { + dc.mu.Lock() + defer dc.mu.Unlock() dc.Entries[key] = entry dc.dirty = true } @@ -133,6 +139,9 @@ func serializeOpts(opts phrase.LocaleDownloadOpts) string { if len(results) > 0 && results[0].Bool() { m[name] = valueMethod.Call(nil)[0].Interface() } + } else if !field.IsZero() { + // plain field (string, bool, int, etc.) — include if non-zero + m[name] = field.Interface() } } diff --git a/clients/cli/cmd/internal/pull.go b/clients/cli/cmd/internal/pull.go index a7a3f367..05cdd881 100644 --- a/clients/cli/cmd/internal/pull.go +++ b/clients/cli/cmd/internal/pull.go @@ -230,7 +230,6 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach results := make([]downloadResult, len(localeFiles)) var rateMu sync.RWMutex - var cacheMu sync.Mutex ctx, cancel := context.WithTimeout(context.Background(), timeoutInMinutes) defer cancel() @@ -249,9 +248,7 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach var cacheKey string if cache != nil { cacheKey = CacheKey(target.ProjectID, lf.ID, opts) - cacheMu.Lock() applyCacheHeaders(cache, cacheKey, &opts) - cacheMu.Unlock() } file, response, dlErr := target.downloadWithRateGate(client, lf, opts, &rateMu) @@ -272,9 +269,7 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach } if cache != nil { - cacheMu.Lock() updateCache(cache, cacheKey, response) - cacheMu.Unlock() } if err := copyToDestination(file, lf.Path); err != nil { @@ -296,7 +291,7 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach // Print results in original order: successes and failures for _, r := range results { switch { - case r.notModified: + case r.notModified && r.path != "": print.Success("Not modified %s", r.path) case r.path != "": print.Success("Downloaded %s to %s", r.message, r.path) @@ -331,6 +326,9 @@ func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile opts.IfModifiedSince = optional.String{} file, response, err = client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &opts) if err != nil { + if response != nil && response.StatusCode == 304 { + return nil, response, nil + } return nil, response, err } } else { From 8bfcc5c21f82d027dfa7120fea3ff5bf382982e4 Mon Sep 17 00:00:00 2001 From: Thibaut Etienne Date: Fri, 3 Apr 2026 09:34:28 +0200 Subject: [PATCH 5/6] chore(CLI): update go.sum --- clients/cli/go.sum | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/clients/cli/go.sum b/clients/cli/go.sum index 76a512db..6960c6ae 100644 --- a/clients/cli/go.sum +++ b/clients/cli/go.sum @@ -215,12 +215,8 @@ github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc= github.com/pelletier/go-toml v1.2.0 h1:T5zMGML61Wp+FlcbWjRDT7yAxhJNAiPPLOFECq181zc= github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic= -github.com/phrase/phrase-go/v4 v4.18.1 h1:y1sv4z8ufEQB+kJA8ymSiH8nRAvH8gGoVSB5/7jvYEQ= -github.com/phrase/phrase-go/v4 v4.18.1/go.mod h1:4XplKvrbHS2LDaXfFp9xrVDtO5xk2WHFm0htutwwd8c= -github.com/phrase/phrase-go/v4 v4.19.0 h1:tNliCxO/0SMu2viLE9idzADBUoY9C6CqrDmp3ntgpQI= -github.com/phrase/phrase-go/v4 v4.19.0/go.mod h1:4XplKvrbHS2LDaXfFp9xrVDtO5xk2WHFm0htutwwd8c= -github.com/phrase/phrase-go/v4 v4.20.0 h1:UTgos4elHzv83XbC6hK2ulDVEvvi1215r8NzE8jC3bc= -github.com/phrase/phrase-go/v4 v4.20.0/go.mod h1:4XplKvrbHS2LDaXfFp9xrVDtO5xk2WHFm0htutwwd8c= +github.com/phrase/phrase-go/v4 v4.21.0 h1:rYTuU6126RHpu0NPI6Owe9pJOpJmYXEY2bbFlHtZ/+8= +github.com/phrase/phrase-go/v4 v4.21.0/go.mod h1:4XplKvrbHS2LDaXfFp9xrVDtO5xk2WHFm0htutwwd8c= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= From 4b8b5f8afe9fd8b3b80c1764bc6b1f37091136a7 Mon Sep 17 00:00:00 2001 From: Thibaut Etienne Date: Fri, 3 Apr 2026 10:13:05 +0200 Subject: [PATCH 6/6] fix(CLI): address expert review findings Restore ctx.Err() check in PullParallel goroutines to respect context cancellation after first failure. Make copyToDestination fail explicitly on nil file instead of silently creating empty files. Remove dead 304 check after rate-limit retry. Simplify pull dispatch by replacing pullFn type with if/else. Add mutex to Save() and json:"-" to mu field for defensive thread safety. --- clients/cli/cmd/internal/download_cache.go | 4 ++- clients/cli/cmd/internal/pull.go | 32 ++++++++++++---------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/clients/cli/cmd/internal/download_cache.go b/clients/cli/cmd/internal/download_cache.go index 10096a06..c3b7adc8 100644 --- a/clients/cli/cmd/internal/download_cache.go +++ b/clients/cli/cmd/internal/download_cache.go @@ -29,7 +29,7 @@ type DownloadCache struct { Entries map[string]CacheEntry `json:"entries"` path string dirty bool - mu sync.Mutex + mu sync.Mutex `json:"-"` } func LoadDownloadCache() *DownloadCache { @@ -81,6 +81,8 @@ func (dc *DownloadCache) Set(key string, entry CacheEntry) { } func (dc *DownloadCache) Save() error { + dc.mu.Lock() + defer dc.mu.Unlock() if !dc.dirty { return nil } diff --git a/clients/cli/cmd/internal/pull.go b/clients/cli/cmd/internal/pull.go index 05cdd881..b609eeea 100644 --- a/clients/cli/cmd/internal/pull.go +++ b/clients/cli/cmd/internal/pull.go @@ -108,13 +108,14 @@ func (cmd *PullCommand) Run(config *phrase.Config) error { if cmd.Parallel && cmd.Async { print.Warn("--parallel is not supported with --async, ignoring parallel") } - type pullFn func(*Target) error - pull := pullFn(func(t *Target) error { return t.Pull(client, cmd.Async, cache) }) - if cmd.Parallel && !cmd.Async { - pull = func(t *Target) error { return t.PullParallel(client, cache) } - } for _, target := range targets { - if err := pull(target); err != nil { + var err error + if cmd.Parallel && !cmd.Async { + err = target.PullParallel(client, cache) + } else { + err = target.Pull(client, cmd.Async, cache) + } + if err != nil { return err } } @@ -238,6 +239,10 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach for i, lf := range localeFiles { g.Go(func() error { + if ctx.Err() != nil { + return ctx.Err() + } + opts, err := target.buildDownloadOpts(lf) if err != nil { err = fmt.Errorf("%s for %s", err, lf.Path) @@ -326,9 +331,6 @@ func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile opts.IfModifiedSince = optional.String{} file, response, err = client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &opts) if err != nil { - if response != nil && response.StatusCode == 304 { - return nil, response, nil - } return nil, response, err } } else { @@ -451,17 +453,17 @@ func (target *Target) downloadSynchronously(client *phrase.APIClient, localeFile } func copyToDestination(file *os.File, path string) error { + if file == nil { + return fmt.Errorf("no content to write to %s", path) + } + defer file.Close() destFile, err := os.Create(path) if err != nil { return err } defer destFile.Close() - if file != nil { - defer file.Close() - _, err = io.Copy(destFile, file) - return err - } - return nil + _, err = io.Copy(destFile, file) + return err } func downloadExportedLocale(url string, localName string) error {