fix(CLI): cache ignored when using --parallel with phrase pull

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 `json:"-"`
 }
 
 func LoadDownloadCache() *DownloadCache {
@@ -65,16 +67,22 @@ 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
 }
 
 func (dc *DownloadCache) Save() error {
+	dc.mu.Lock()
+	defer dc.mu.Unlock()
 	if !dc.dirty {
 		return nil
 	}
@@ -133,6 +141,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()
 		}
 	}
 

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)
+			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 {
@@ -181,30 +181,16 @@ 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 {
-	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)
@@ -226,7 +212,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
 	}
@@ -264,11 +250,34 @@ 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)
+				applyCacheHeaders(cache, cacheKey, &opts)
+			}
+
+			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}
+				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 {
+				updateCache(cache, cacheKey, response)
+			}
+
+			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
@@ -285,54 +294,50 @@ func (target *Target) PullParallel(client *phrase.APIClient) error {
 	waitErr := g.Wait()
 
 	// Print results in original order: successes and failures
-	var skipCount int
 	for _, r := range results {
-		if r.path != "" {
+		switch {
+		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)
-		} 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
 }
 
 // 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 {
-	// Read-lock gate: blocks only when a writer (rate-limited worker) holds it
-	gate.RLock()
-	gate.RUnlock()
+func (target *Target) downloadWithRateGate(client *phrase.APIClient, localeFile *LocaleFile, opts phrase.LocaleDownloadOpts, gate *sync.RWMutex) (*os.File, *phrase.APIResponse, error) {
+	waitForGate := func() { gate.RLock(); gate.RUnlock() }
+	waitForGate()
 
 	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.
 			if gate.TryLock() {
 				waitForRateLimit(response.Rate)
 				gate.Unlock()
 			} else {
-				// Another worker is already pausing; wait for it
-				gate.RLock()
-				gate.RUnlock()
+				waitForGate()
 			}
-
-			file, _, err = client.LocalesApi.LocaleDownload(Auth, target.ProjectID, localeFile.ID, &opts)
+			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.
@@ -370,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...")
@@ -379,44 +384,53 @@ 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)
 	}
 	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
@@ -431,30 +445,25 @@ 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)
 }
 
 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 {
@@ -474,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.

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=