feat(Storage): implement GCS idempotency tokens for all API operations- #10 - #9490
feat(Storage): implement GCS idempotency tokens for all API operations- #10#9490salilg-eng wants to merge 4 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
66b4ed7 to
02d4e0f
Compare
|
please resolve the failing cla check |
cy-yun
left a comment
There was a problem hiding this comment.
Overall, great approach using StorageRequestWrapper to seamlessly add the token at the bottom of the request flow! The token generation and retry header fallback look very solid.
However, it looks like you forgot to commit the changes to RetryTrait.php that you mentioned in the PR description ("I moved objects.delete, insert, patch, and update from being conditionally idempotent to fully idempotent...").
You can use the suggestions below to apply the missing changes:
File: Storage/src/Connection/RetryTrait.php
Line: ~71-73 (End of $idempotentOps array)
'serviceaccount.get',
'signBlob.execute',
'objects.delete',
'objects.insert',
'objects.patch',
'objects.update'
];
File: Storage/src/Connection/RetryTrait.php
Line: ~80-93 ($condIdempotentOps array)
private static $condIdempotentOps = [
'buckets.patch' => ['ifMetagenerationMatch', 'etag'],
// Currently etag is not supported, so this preCondition never available
'buckets.setIamPolicy' => ['etag'],
'buckets.update' => ['ifMetagenerationMatch', 'etag'],
'hmacKey.update' => ['etag'],
'objects.compose' => ['ifGenerationMatch'],
'objects.copy' => ['ifGenerationMatch'],
'objects.rewrite' => ['ifGenerationMatch']
];
|
I moved objects.delete, insert, patch, and update from being conditionally idempotent to fully idempotent in RetryTrait. Since we're sending tokens now, the backend safely handles idempotency for these operations natively. |
019088e to
89047db
Compare
22e86e7 to
8e6664d
Compare
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-token header (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 RequestWrapper into a new StorageRequestWrapper. This lets us hook into the very bottom of the request flow just for Storage, without polluting the shared google/cloud-core package.
A few technical notes on how it behaves:
Standard Retries: Because we inject the token right before the ExponentialBackoff loop 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.php explicitly 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-id metric header when available, rather than generating a second redundant UUID.
Idempotency Config: I moved objects.delete, insert, patch, and update from being conditionally idempotent to fully idempotent in RetryTrait. Since we're sending tokens now, the backend safely handles idempotency for these operations natively.
I've also updated the unit tests in RestTest.php to validate the new wrapper and ensure the token is being preserved or regenerated in the right scenarios. All tests are passing locally!