feat(Storage): implement GCS idempotency tokens for all API operations#10
feat(Storage): implement GCS idempotency tokens for all API operations#10salilg-eng wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces automatic idempotency token generation and injection for Google Cloud Storage requests by implementing a new StorageRequestWrapper and updating the Rest connection class. It also updates retry behaviors and adds corresponding unit and system tests. The review feedback highlights several improvement opportunities, including performing case-insensitive checks when detecting or replacing user-supplied idempotency headers to prevent duplicates, and ensuring that tokens extracted from invocation headers are validated before use.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
dc0fde8 to
098feee
Compare
098feee to
f0d8907
Compare
|
|
||
| // Second update uses the exact same UUID, simulating a network retry. | ||
| // Even though the metageneration has changed, the backend recognizes | ||
| // the idempotency token and returns 200 OK instead of 412 Precondition Failed. |
There was a problem hiding this comment.
ensure the idempotency token is resused in retry, don't change anyother header, add the scenario in unit tests
This PR adds support for GCS Idempotency Tokens to the Storage client (Fixes #280811217).
The goal here is to send a unique
x-goog-gcs-idempotency-tokenheader (UUID) on all JSON API requests. This allows the backend to safely retry operations without the risk of duplicating them.To get this working correctly across the board—especially for resumable upload chunks which bypass Rest.php—I subclassed
RequestWrapperinto a newStorageRequestWrapper. This lets us hook into the very bottom of the request flow just for Storage, without polluting the sharedgoogle/cloud-corepackage.A few technical notes on how it behaves:
Standard Retries: Because we inject the token right before the
ExponentialBackoffloop runs, normal retries (like 5xx or 429) will correctly reuse the exact same token.Mid-stream Downloads: If a download drops mid-stream and we have to fire off a new byte-range request to resume, the retry listener in
Rest.phpexplicitly generates a brand new token for it, as required by the spec.Header Alignment: To keep things clean, it extracts and reuses the UUID from the
gccl-invocation-idmetric header when available, rather than generating a second redundant UUID.Idempotency Config: I moved
objects.delete,insert,patch, andupdatefrom being conditionally idempotent to fully idempotent inRetryTrait. Since we're sending tokens now, the backend safely handles idempotency for these operations natively.I've also updated the unit tests in
RestTest.phpto validate the new wrapper and ensure the token is being preserved or regenerated in the right scenarios. All tests are passing locally!