ci(#6129): add 429 quota retry to GCP IAM provisioning - #6130
ci(#6129): add 429 quota retry to GCP IAM provisioning#6130fullsend-ai-coder[bot] wants to merge 2 commits into
Conversation
GCP IAM operations (WIF pool/provider create, update, enable; service account create) now retry on HTTP 429 (Too Many Requests) with exponential backoff and jitter, using the existing iamRetryDelay function. Previously these operations failed immediately on quota exhaustion, causing cascading failures when a flaky E2E test triggered rapid merge queue retries that overwhelmed GCP IAM API quotas. The retry applies to: CreateServiceAccount, CreateWIFPool, CreateWIFProvider, UpdateWIFProvider, and enableWIFProvider. Up to 5 attempts with 500ms-10s exponential backoff (same parameters as the existing IAM conflict retry). Context cancellation is respected during backoff. Note: golangci-lint could not run (not installed in sandbox). pre-commit could not run (sandbox blocks git fetch). Closes #6129
|
🤖 Finished Review · ✅ Success · Started 1:35 AM UTC · Completed 1:52 AM UTC Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsHigh
Low
Labels: PR modifies GCP IAM provisioning code under internal/dispatch/gcf/ Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
component/dispatch
|
/fs-fix address feedback, amend the commit to use the correct prefix and increase the coverage, is too low. |
|
🤖 Finished Fix · ✅ Success · Started 7:39 AM UTC · Completed 7:48 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.55 |
- Change PR title prefix from ci to fix (production code, not CI) - Fix resource leak: replace defer resp.Body.Close() with explicit close on each return path in CreateServiceAccount for consistency with other retry-wrapped functions - Integrate retry doc comments as continuous prose matching existing SetProjectIAMBinding style - Align post-loop fallback error messages with in-loop messages - Add exhaustion and context-cancellation subtests for CreateServiceAccount, CreateWIFPool, CreateWIFProvider, and UpdateWIFProvider to increase patch coverage Addresses review feedback on #6130
🔧 Fix agent — iteration 1 (human-triggered)Addressed 5 of 7 review findings with code changes. Changed PR title prefix from ci to fix (commit prefix updated, PR title update blocked by sandbox policy). Fixed resource leak consistency in CreateServiceAccount, aligned doc comment style and error messages across all retry-wrapped functions. Added 8 new test subtests to increase patch coverage. Disagreed with 2 findings: naming-convention (reviewer said no change needed) and error-message-consistency (different failure modes justify distinct wording, but aligned internal consistency). Fixed (5):
Disagreed (2):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 7:50 AM UTC · Completed 8:12 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.21 |
|
Risk Assessment: moderate (2/5) DetailsModerate risk driven primarily by high git-history churn: gcp.go is a hotspot with 8 commits in 30 days, 6 distinct authors, and 14 fix/revert commits in 90 days, indicating an actively-evolving and regression-prone area. However, the PR itself is well-scoped (focused 429 retry with backoff), has strong test coverage (50% test file ratio, +357 test lines), touches no protected paths or security-sensitive code, and is a partial fix matching the linked issue scope. |
| if resp.StatusCode == http.StatusTooManyRequests { | ||
| resp.Body.Close() | ||
| if attempt == iamQuotaRetries-1 { | ||
| return fmt.Errorf("creating service account: quota exhausted after %d attempts", iamQuotaRetries) |
There was a problem hiding this comment.
[low] error-message-consistency
The existing doWIFRequestWithRetry on main uses 'rate limited (HTTP 429) after %d attempts'. The new inline retry loops use 'quota exhausted after %d attempts'. Two different phrasings for the same HTTP 429 condition. (Dependent on merge conflict resolution in [api-contract] finding.)
| @@ -189,7 +189,8 @@ func NewLiveGCFClient(quotaProject string) *LiveGCFClient { | |||
| return &LiveGCFClient{Client: c} | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[low] documentation-style
Retry behavior is documented by appending a sentence to the first doc comment line rather than as continuous prose in a separate paragraph. Existing retry functions (SetProjectIAMBinding, setSecretIAMBindingWithMode) use multi-line continuous prose doc blocks.
| if attempt == iamQuotaRetries-1 { | ||
| return fmt.Errorf("creating service account: quota exhausted after %d attempts", iamQuotaRetries) | ||
| } | ||
| log.Printf("create service account: 429 quota exhausted, retrying (attempt %d/%d)", attempt+1, iamQuotaRetries) |
There was a problem hiding this comment.
[low] logging-convention
The log message format varies across functions: 'create service account', 'create WIF pool', 'create WIF provider', 'update WIF provider', but 'WIF provider enable' breaks the verb-noun pattern used by the others.
1 participant
Summary
CreateServiceAccount,CreateWIFPool,CreateWIFProvider,UpdateWIFProvider, andenableWIFProvideriamRetryDelayfunction (500ms base, doubling to 10s cap, with 50-100% jitter) for up to 5 attemptsMotivation
PR #6078 was ejected from the merge queue 14 times. After 10+ rapid retries, GCP IAM API quotas were exhausted, producing
unexpected status 429 enabling WIF provider: Resource has been exhausted. The provisioning code had no retry logic for 429 responses — it failed immediately, turning a flaky test into an 11-hour outage.Testing
internal/dispatch/gcf/continue to pass with-raceCloses #6129
Post-script verification
agent/6129-gcp-iam-429-retry)420cb6e06e022bea88b7f02b29f0692aec6038e7..HEAD)