From 8a5a5199b2b2943506891227987eb757a9c2f675 Mon Sep 17 00:00:00 2001 From: Luis Larco Date: Sun, 31 May 2026 16:07:30 -0700 Subject: [PATCH 1/2] gcs: retry Delete on transient 5xx The default RetryIdempotent policy doesn't cover Delete without an IfGenerationMatch precondition, so a single 503 from GCS bubbles up to the caller. Switch Delete to RetryAlways. Also apply MaxRetries (default 3 when unset) to all operations so the cap is no longer opt-in. See https://docs.cloud.google.com/storage/docs/retry-strategy. Signed-off-by: Luis Larco --- CHANGELOG.md | 1 + providers/gcs/gcs.go | 21 ++++++++++++++------- providers/gcs/gcs_test.go | 29 +++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 120720817f..878d29a99a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re - [#157](https://github.com/thanos-io/objstore/pull/157) Azure: Add `az_tenant_id`, `client_id` and `client_secret` configs. ### Fixed +- [#261](https://github.com/thanos-io/objstore/pull/261) GCS: retry Delete on transient 5xx, and cap all operations at MaxRetries (default 3 when unset). - [#196](https://github.com/thanos-io/objstore/pull/196) GCS: fix error check in Exists method when object does not exist. - [#153](https://github.com/thanos-io/objstore/pull/153) Metrics: Fix `objstore_bucket_operation_duration_seconds_*` for `get` and `get_range` operations. - [#141](https://github.com/thanos-io/objstore/pull/142) S3: Fix missing encryption configuration for `Bucket.Exists()` and `Bucket.Attributes()` calls. diff --git a/providers/gcs/gcs.go b/providers/gcs/gcs.go index b8723b8ca0..fd48b567c2 100644 --- a/providers/gcs/gcs.go +++ b/providers/gcs/gcs.go @@ -35,6 +35,7 @@ const DirDelim = "/" var DefaultConfig = Config{ HTTPConfig: exthttp.DefaultHTTPConfig, + MaxRetries: 3, } // Config stores the configuration for gcs bucket. @@ -55,9 +56,8 @@ type Config struct { ChunkSizeBytes int `yaml:"chunk_size_bytes"` noAuth bool `yaml:"no_auth"` - // MaxRetries controls the number of retries for idempotent operations. - // Overrides the default gcs storage client behavior if this value is greater than 0. - // Set this to 1 to disable retries. + // MaxRetries controls the number of attempts for retryable operations. + // Defaults to 3 when unset (see DefaultConfig). Set this to 1 to disable retries. MaxRetries int `yaml:"max_retries"` } @@ -179,9 +179,14 @@ func newBucket(ctx context.Context, logger log.Logger, gc Config, opts []option. chunkSize: gc.ChunkSizeBytes, } - if gc.MaxRetries > 0 { - bkt.bkt = bkt.bkt.Retryer(storage.WithMaxAttempts(gc.MaxRetries)) + // Cap retries on transient errors. See + // https://docs.cloud.google.com/storage/docs/retry-strategy for what + // counts as transient and how the SDK handles backoff. + maxAttempts := gc.MaxRetries + if maxAttempts == 0 { + maxAttempts = DefaultConfig.MaxRetries } + bkt.bkt = bkt.bkt.Retryer(storage.WithMaxAttempts(maxAttempts)) return bkt, nil } @@ -349,9 +354,11 @@ func (b *Bucket) Upload(ctx context.Context, name string, r io.Reader, opts ...o return w.Close() } -// Delete removes the object with the given name. +// Delete removes the object with the given name. RetryAlways overrides the +// default RetryIdempotent policy, which would otherwise exclude Delete because +// we don't pass IfGenerationMatch. func (b *Bucket) Delete(ctx context.Context, name string) error { - return b.bkt.Object(name).Delete(ctx) + return b.bkt.Object(name).Retryer(storage.WithPolicy(storage.RetryAlways)).Delete(ctx) } // IsObjNotFoundErr returns true if error means that object is not found. Relevant to Get operations. diff --git a/providers/gcs/gcs_test.go b/providers/gcs/gcs_test.go index 80951d7aef..a91db702f2 100644 --- a/providers/gcs/gcs_test.go +++ b/providers/gcs/gcs_test.go @@ -9,6 +9,8 @@ import ( "net/http" "net/http/httptest" "os" + "strings" + "sync/atomic" "testing" "time" @@ -178,3 +180,30 @@ func TestNewBucketWithErrorRoundTripper(t *testing.T) { testutil.NotOk(t, err) testutil.Assert(t, errutil.IsMockedError(err), "Expected RoundTripper error, got: %v", err) } + +func TestBucket_Delete_RetriesOnTransient5xx(t *testing.T) { + var deleteAttempts atomic.Int32 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "/o/") { + n := deleteAttempts.Add(1) + if n < 3 { + w.WriteHeader(http.StatusServiceUnavailable) + _, _ = w.Write([]byte(`{"error":{"code":503,"message":"backendError"}}`)) + return + } + w.WriteHeader(http.StatusNoContent) + return + } + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + t.Setenv("STORAGE_EMULATOR_HOST", srv.Listener.Addr().String()) + + bkt, err := newBucket(context.Background(), log.NewNopLogger(), Config{Bucket: "test-bucket"}, []option.ClientOption{}) + testutil.Ok(t, err) + + testutil.Ok(t, bkt.Delete(context.Background(), "test-object")) + testutil.Equals(t, int32(3), deleteAttempts.Load()) +} From 83eb1cfc48e938f5ffab676f1fab21a95aa745e7 Mon Sep 17 00:00:00 2001 From: Luis Larco Date: Sun, 31 May 2026 16:25:16 -0700 Subject: [PATCH 2/2] Replace sync/atomic with go.uber.org/atomic to fix lint error Signed-off-by: Luis Larco --- providers/gcs/gcs_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/providers/gcs/gcs_test.go b/providers/gcs/gcs_test.go index a91db702f2..6852f77f22 100644 --- a/providers/gcs/gcs_test.go +++ b/providers/gcs/gcs_test.go @@ -10,7 +10,6 @@ import ( "net/http/httptest" "os" "strings" - "sync/atomic" "testing" "time" @@ -19,6 +18,7 @@ import ( "github.com/go-kit/log" "github.com/prometheus/common/model" "github.com/thanos-io/objstore/errutil" + "go.uber.org/atomic" "google.golang.org/api/option" )