fix(gcsartifact): prevent artifact version race in concurrent Save#1172
Open
wolo-lab wants to merge 1 commit into
Open
fix(gcsartifact): prevent artifact version race in concurrent Save#1172wolo-lab wants to merge 1 commit into
wolo-lab wants to merge 1 commit into
Conversation
wolo-lab
force-pushed
the
wolo/gcs-artifact-version-race
branch
2 times, most recently
from
July 18, 2026 20:30
1081c43 to
e9db6f3
Compare
Save assigned versions non-atomically: it listed the existing versions, computed max+1, and wrote that blob. Two concurrent Save calls for the same artifact would read the same version set, pick the same next version, and the second write would silently overwrite the first — duplicate returned versions and a lost object. The code carried a TODO acknowledging this. Assign versions with optimistic concurrency instead: write the new blob with a "does not exist" generation precondition (x-goog-if-generation-match: 0). GCS has no transaction spanning the list and the write, so a colliding writer now fails the precondition (HTTP 412); Save re-reads the versions and retries with a fresh number, using a jittered backoff between attempts and bounded by maxSaveAttempts. It returns errVersionConflict if the budget is exhausted rather than overwriting an existing version. N concurrent saves therefore yield exactly versions 1..N with no lost writes. Testing Plan: - go build -mod=readonly ./... - go test -race -mod=readonly -count=1 -shuffle=on ./artifact/... - golangci-lint run ./artifact/... - go mod tidy -diff - TestGCSArtifactServiceConcurrentSave: concurrent savers assert distinct, contiguous versions and that nothing is overwritten (saver count kept within the retry budget). Verified it fails against the pre-fix code. - TestSaveReturnsWriteErrorWithoutRetry: a non-precondition write error is surfaced as-is and not retried. - TestSaveExhaustsRetriesOnPersistentConflict: persistent 412s give up with errVersionConflict after maxSaveAttempts. - TestBackoffDelayBounds: jittered backoff stays within bounds.
wolo-lab
force-pushed
the
wolo/gcs-artifact-version-race
branch
from
July 18, 2026 22:29
e9db6f3 to
99e5437
Compare
wolo-lab
marked this pull request as ready for review
July 19, 2026 14:14
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
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.
Problem
gcsService.Saveassigned version numbers non-atomically: it listed theexisting versions, computed
max+1, and wrote the blob at that version. Twoconcurrent
Savecalls for the same artifact read the same version set, pickthe same next version, and the second upload silently overwrites the first —
callers get duplicate version numbers and one object is lost. The code carried a
TODOacknowledging the race.Note: adk-python's
GcsArtifactServicehas the same unguardedmax(versions) + 1logic, so this is Go-specific hardening rather than a port.Summary
(
x-goog-if-generation-match: 0) viaObjectHandle.If(storage.Conditions{DoesNotExist: true}). GCS has notransaction spanning the list and the write, so a colliding writer now fails
the precondition with HTTP 412 instead of clobbering the existing object.
Savere-reads the versions and retries with a fresh number,bounded by
maxSaveAttempts(returnserrVersionConflictif exhausted). Nconcurrent saves therefore yield exactly versions
1..Nwith no lost writes.writeArtifact/isPreconditionFailedhelpers and added anifNotExist()method to the internalgcsObjectseam and its wrapper. Nopublic API change