Skip to content

Retry GCS rate-limit errors when deleting and copying snapshots - #2143

Merged
Max Smythe (maxsmythe) merged 1 commit into
agent-substrate:mainfrom
sairajp-rewind:fix/gcs-write-retries
Oct 21, 2026
Merged

Max Smythe (maxsmythe) merged 1 commit into
agent-substrate:mainfrom
sairajp-rewind:fix/gcs-write-retries

Conversation

@sairajp-rewind

@sairajp-rewind Sairaj Pokale (sairajp-rewind) commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

What this PR does and why we need it ?

GCS answers 429 while it scales a cold bucket up to a burst of suspends. ateapi
used the default retry policy, which never retries a delete or copy without a
precondition, so a single 429 failed a snapshot release or a tag copy. List did
retry, but with no cap, until the caller's context ended.

This change:

  • ateapi: every GCS call (list, delete, copy) retries transient errors with
    RetryAlways, backing off from 250ms up to 5s, at most 7 attempts per request
    (at most 12.75s of sleep). The settings sit on the bucket handle, so objects
    inherit them. That includes the copy's destination, which is where the SDK
    reads them.
  • atelet: drops WithMaxAttempts(5), which never capped uploads, and backs
    off from 250ms up to 5s. On transient errors, compose, delete, copy and reads
    now retry until their context ends instead of stopping after 5 attempts.
  • atelet: scratch cleanup (removeScratch, for upload parts and compose
    intermediates) ran on context.WithoutCancel(ctx) with no deadline, so
    without the cap it could retry forever. Each cleanup now deletes its objects 8
    at a time within a 30s timeout. It is best-effort: objects it can't delete are
    logged and left next to the object, instead of failing an upload that already
    succeeded.

RetryAlways is safe here. A retried delete that already landed gets a 404,
which Delete already treats as success. A retried copy writes the same
unchanging source to the same destination.

Behavior change

  • ateapi: the cap covers every caller, not just suspend: actor delete, revert,
    and tag create and delete. Delete and copy used to fail on the first 429, so
    for them this is only more retrying. List used to retry until the caller's
    context ended. In a long GCS outage, these calls now fail after at most 12.75s
    of sleep:
    • Delete, revert and tag return the error, and their caller must retry.
    • Suspend logs the error and leaves the replaced snapshot in storage until the
      actor is deleted.
  • atelet: in a long GCS outage, calls retry until their context ends. For work
    ateapi starts over gRPC, that is its deadline of at most 10 minutes. The
    golden-actor template reconciler is the exception: it calls SuspendActor,
    ResumeActor and DeleteActor in-process with no deadline, so the atelet
    calls it causes have none. Uploads already ignored the cap there. A follow-up
    can bound those calls.
  • atelet: as before, an upload the SDK sends as resumable (at least its
    Writer's ChunkSize) also gives up once a chunk has been retrying for 32s,
    the Writer's default ChunkRetryDeadline.
  • atelet: a scratch cleanup failure no longer fails the upload.

Testing

  • internal/objectstore/gcs_retry_test.go: an httptest fake GCS answers a
    scripted number of errors per call. The tests check attempt counts, not
    timing (they back off 1ms), and run in make test without an emulator.
  • pkg/objectstorage/gcs_retry_test.go, against the same kind of fake:
    • An upload and a compose each get through six 429s.
    • An upload that only ever gets 429s keeps retrying until its context ends.
    • Scratch deletes run in parallel, within the limit.
    • Deletes that keep failing stop at the cleanup timeout, even when the
      upload's context is already canceled.
    • An upload whose part deletes all fail still succeeds.
  • Each check fails if its change is reverted: 5 attempts in ateapi,
    WithMaxAttempts(5) in atelet, no cleanup timeout, cleanup on the upload's
    context, deleting one object at a time, or returning the cleanup error.
  • The new and changed tests pass 20 runs in a row with -race.
  • Tests pass
  • Appropriate changes to documentation are included in the PR

Comment thread internal/objectstore/gcs.go Resolved
Comment thread pkg/objectstorage/gcs.go Resolved
@chw120

Copy link
Copy Markdown
Collaborator

LGTM, thanks Sairaj for the updates and the extra test cases!

@chw120

Copy link
Copy Markdown
Collaborator

Julian Gutierrez Oschmann (@juli4n) could you help approve this PR as well? Thanks.

Comment thread pkg/objectstorage/gcscompose.go Resolved
Comment thread pkg/objectstorage/gcscompose.go Resolved
Comment thread pkg/objectstorage/gcs.go Resolved
Comment thread pkg/objectstorage/gcs_retry_test.go Resolved
@chw120

Copy link
Copy Markdown
Collaborator

LGTM. Let's see if Max Smythe (@maxsmythe) has any other suggestions. Thanks.

@maxsmythe Max Smythe (maxsmythe) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

auto-merge was automatically disabled October 19, 2026 17:00

Head branch was pushed to by a user without write access

@sairajp-rewind

Copy link
Copy Markdown
Collaborator Author

Rebased onto main. The e2e CI wouldn't trigger and the module rename needed a one-line import fix in the new test (99616c8); nothing else changed. Max Smythe (@maxsmythe) could you re-approve?

GCS answers 429 while it scales a cold bucket up to a burst of
suspends. ateapi's GCS store used the default retry policy, which never
retries a delete or copy without a precondition, so a single 429 failed
a snapshot release, an actor delete, a revert or a tag copy. Give every
ateapi GCS call RetryAlways with a backoff from 250ms up to 5s and at
most 7 attempts.

atelet: drop the 5-attempt cap, which never applied to uploads, and
back off from 250ms up to 5s, so calls retry until their context ends.
Scratch cleanup ran on context.WithoutCancel with no deadline, deleted
one object at a time, and failed an upload that had already succeeded
when a delete failed. It now deletes 8 objects at a time within a 30s
timeout and only logs what it could not delete.

Note in setRetry that an upload the SDK sends as resumable also stops
once a chunk has been retrying for ChunkRetryDeadline.

@maxsmythe Max Smythe (maxsmythe) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@maxsmythe
Max Smythe (maxsmythe) added this pull request to the merge queue Oct 21, 2026
Merged via the queue into agent-substrate:main with commit 87baab0 Oct 21, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants