Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update batch handling to ensure each operation has its own unique idempotency-token #2905

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

BenWhitehead
Copy link
Collaborator

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.

@BenWhitehead BenWhitehead requested a review from a team as a code owner January 30, 2025 19:37
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: storage Issues related to the googleapis/java-storage API. labels Jan 30, 2025
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Overall LGTM, a few minor questions.

@BenWhitehead BenWhitehead merged commit 8d79b8d into main Jan 30, 2025
21 checks passed
@BenWhitehead BenWhitehead deleted the fix/batch-idempotency-tokens branch January 30, 2025 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/java-storage API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage batch setEventBasedHold=false does not work
2 participants