[llarco]

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.

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

• Delete now calls bkt.Object(name).Retryer(storage.WithPolicy(storage.RetryAlways)).Delete(ctx). Without RetryAlways, the SDK skips retries entirely for plain Delete because the default RetryIdempotent policy treats unconditional Delete as non-idempotent.
• newBucket now always sets WithMaxAttempts(MaxRetries) on the bucket handle. Previously this was only applied when the user set max_retries > 0. The ObjectHandle inherits the bucket's retry config, so the new Delete retry inherits the cap.
• DefaultConfig.MaxRetries = 3 sets the new default. newBucket falls back to this when gc.MaxRetries == 0, covering both the YAML config path (via DefaultConfig) and direct NewBucketWithConfig callers.
• Config.MaxRetries doc updated. MaxRetries: 1 to disable retries still works.

Verification

Added TestBucket_Delete_RetriesOnTransient5xx in providers/gcs/gcs_test.go. Stands up an httptest.NewServer that returns 503 on the first 2 DELETE requests and 204 on the third. Asserts bkt.Delete returns nil and the server saw exactly 3 attempts.

Context: this surfaced as recurring crashes in the Thanos compactor's BlocksCleaner.DeleteMarkedBlocks when GCS returned a transient 503 on a single delete during the cleanup pass. Discussion: thanos-io/thanos#8838 (now closed in favor of fixing the issue here in objstore).

[llarco]

Ran into this again in prod — a transient 503 on a meta.json delete took down my compactor (Thanos 0.37.1):

googleapi: Error 503: We encountered an internal error. Please try again., backendError
delete 01KTYYMM90S20K6MA099K1VRKE/meta.json
pkg/compact.(*BlocksCleaner).DeleteMarkedBlocks

The default RetryIdempotent policy skips Delete when there's no IfGenerationMatch, so it never retries.

Rebased on main. PTAL at your earliest convenience — happy to split the MaxRetries default into a separate PR if that's easier to review.

[GiedriusS]

What does this STORAGE_EMULATOR_HOST mean? Maybe there's a constant for it defined somewhere in some module that provides more context?

[llarco]

It's the GCS SDK's documented emulator override: https://cloud.google.com/storage/docs/emulator. It makes it so that the GCS client library points at a local endpoint instead of the real API. It's already used in other places in this file. There's no constant for it today, happy to add a constant but didn't want to touch more lines in this PR.

[GiedriusS]

If maxAttempts can only be 0 or above then maybe it could be a uint?

[llarco]

I think the idiomatic/standard convention in golang is to use int for this kind of variables while keeping uint and other variations of ints for specific operations (eg. bitwise operations, bitmasking, etc) and let the code logic enforce/validate the the value explicitly.

Also, golang does not have implicit type casting, so if for whatever reason we wanted to compare this value to another one line for i := 0; i < maxAttempts; i++, we would actually have to typecast it: for i := 0; i < int(maxAttempts); i++.

I found an interested thread about this topic: https://stackoverflow.com/questions/63105394/in-go-when-should-you-use-uint-vs-int

And also this quote "When you need an integer value you should use int unless you have a specific reason to use a sized or unsigned integer type" from https://go.dev/tour/basics/11.

I looked at the rest of the repo, and there are many cases where int is being used for non-negative numbers (eg. counters) rather than unit, so I figured I'd stick to majority of the code style across the repo.

Either way, if you want me to switch it to an uint, let me know and I'll make the change

[GiedriusS]

What's IfGenerationMatch?

[llarco]

IfGenerationMatch is GCS's way of handling race conditions for operations that would otherwise require atomicity (see https://docs.cloud.google.com/storage/docs/request-preconditions#precondition_criteria). Since we are not passing such configuration, the default policy is to not retry 5xx errors for delete operations.

I've updated the comment to explain this inline rather than just naming it.