fix: update batch handling to ensure each operation has its own unique idempotency-token - #2905
Merged
BenWhitehead merged 1 commit intoJan 30, 2025
Merged
Conversation
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
…e idempotency-token
tritone
approved these changes
Jan 30, 2025
tritone
left a comment
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Overall LGTM, a few minor questions.
Sorry, something went wrong.
tritone
approved these changes
Jan 30, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
1 more reviewer2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
If batch is used to update an object, and that subrequest has an x-goog-idempotency-token set on it, that token can be used to short circuit execution of the request if that token has already been processed (essentially interpreting the request as a retry).
The bug
Previously, we didn't set idempotency token for batch operations, but the IdempotencyIdInterceptor would still attach one if it was visible in the thread local.
The fix
Update our handling, so that we alway set a new unique token for each batch operation making it immune to any possibly still visible thread local value.
HTTP Request logs of what this will now look like.
2025-01-30 14:20:38,489 CONFIG com.google.api.client.http.HttpTransport - -------------- REQUEST -------------- POST https://storage.googleapis.com//batch/storage/v1 Accept-Encoding: gzip User-Agent: Google-HTTP-Java-Client/1.45.3 (gzip) Content-Type: multipart/mixed; boundary=__END_OF_PART__6fd28e8d-0df8-4fdb-9384-7a6f3aa0a6bd__ Content-Length: 3725 2025-01-30 14:20:38,494 CONFIG com.google.api.client.http.HttpTransport - --__END_OF_PART__6fd28e8d-0df8-4fdb-9384-7a6f3aa0a6bd__ Content-Length: 1668 Content-Type: application/http content-id: 1 content-transfer-encoding: binary POST https://storage.googleapis.com/storage/v1/b/java-storage-grpc-70b3f6c6-d3a8-4366-b478-a20e5ca332bf/o/com.google.cloud.storage.it.ITBatchTest.batchSuccessiveUpdatesWork-0001?projection=full HTTP/1.1 x-http-method-override: PATCH x-goog-gcs-idempotency-token: 73169f09-8372-4333-8168-8ecb17b51cf7 {"bucket":"java-storage-grpc-70b3f6c6-d3a8-4366-b478-a20e5ca332bf","customTime":"2025-01-30T19:20:38.485Z","name":"com.google.cloud.storage.it.ITBatchTest.batchSuccessiveUpdatesWork-0001"} --__END_OF_PART__6fd28e8d-0df8-4fdb-9384-7a6f3aa0a6bd__ Content-Length: 1668 Content-Type: application/http content-id: 2 content-transfer-encoding: binary POST https://storage.googleapis.com/storage/v1/b/java-storage-grpc-70b3f6c6-d3a8-4366-b478-a20e5ca332bf/o/com.google.cloud.storage.it.ITBatchTest.batchSuccessiveUpdatesWork-0002?projection=full HTTP/1.1 x-http-method-override: PATCH x-goog-gcs-idempotency-token: 761fe5ca-38e1-4538-af20-f831a68d6c91 {"bucket":"java-storage-grpc-70b3f6c6-d3a8-4366-b478-a20e5ca332bf","customTime":"2025-01-30T19:20:38.485Z","name":"com.google.cloud.storage.it.ITBatchTest.batchSuccessiveUpdatesWork-0002"} --__END_OF_PART__6fd28e8d-0df8-4fdb-9384-7a6f3aa0a6bd__--In this particular case, the leak of the thread local value occurred during a json resumable upload. The json resumable upload has been updated to properly cleanup its token after execution.
Fixes #2902 by ensuring every operation has a unique id. In 2902 if an token had leaked it would be applied to the first batch and processed as expected, but when reaching the second batch the token would still be the same, and the request was interpreted as already happening and the response from the first batch was returned.