fix(#6546): add retry-with-backoff for transient errors in GCP client

[fullsend-ai-coder]

Summary

Add retry-with-exponential-backoff to the GCP HTTP client (internal/gcp/client.go) for transient transport errors and server error status codes. This fixes WIF pool provisioning failures caused by TCP connection resets during fullsend inference provision, and makes all GCP API callers (Secret Manager, Cloud Run, IAM bindings) resilient to transient network issues.

Related Issue

Closes #6546

Changes

• Add retry loop to DoRequest with 3 retries, exponential backoff (1s base, 10s cap), and 50-100% jitter
• Add isRetryableTransportError for connection resets (ECONNRESET), connection refused (ECONNREFUSED), unexpected EOF, and network timeouts — following the pattern from internal/fetch/fetch.go
• Add isRetryableStatusCode for HTTP 500/502/503/504 (429 excluded to avoid double-retry with doWIFRequestWithRetry)
• Add injectable retryDelayFn field to Client for test determinism (zero-delay in NewClientWithHTTP)
• Update GCF test get_after_template_update_failure to account for DoRequest's new retry behavior on 500 responses

Testing

• make lint passes (stage changes first, then run)
• Tests added/updated for new or modified logic
• Unit tests cover: retry on connection reset, retry on 500/502/503/504, no retry on 400/401/403/404/429, max retries exhausted, request body preserved across retries, context cancellation, backoff delay bounds
• Patch coverage ≥80% (93.4%)

---

Closes #6546

Post-script verification

• Branch is not main/master (agent/6546-retry-gcp-iam-api)
• Secret scan passed (gitleaks — 74aebe0ffe9e1a6bb6e1e8a6c7a4b97917457be7..HEAD)
• PR body secret scan passed (gitleaks — no-git)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:10 PM UTC · Completed 4:31 PM UTC

Commit: cd0e891 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.89

[codecov]

Codecov Report

❌ Patch coverage is 89.39394% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/gcp/client.go	89.39%	7 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Risk Assessment: moderate (2/5)

Details

Score unchanged from prior assessment at moderate (2). The BLAST_RADIUS=large flag in Tier 1 is offset by all other metadata signals scoring 1 (no protected paths, no security-sensitive files, strong 67% test ratio, bot author). Tier 2 shows moderate churn on gcp_test.go and an active fix-commit history in this GCP area. Tier 3 confirms the PR scope is proportional to the linked bug and rollback is safe.

Previous run

Risk Assessment: moderate (2/5)

Details

Low-risk bug fix adding retry-with-backoff to GCP client with strong test coverage (67% test files), but moderate git history churn on the test file and behavioral modification to existing client code nudge the score to moderate.

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Low-risk bug fix adding retry-with-backoff to GCP client with strong test coverage (67% test files), but moderate git history churn on the test file and behavioral modification to existing client code nudge the score to moderate.

[fullsend-ai-review]

Review

Findings

Low

• [edge-case] internal/gcp/client.go:127 — The access token is fetched once before the retry loop and reused across up to 4 attempts (~7s with backoff). Well within GCP token lifetimes (1 hour), but a near-expiry token would not benefit from retries.

• [naming-convention] internal/gcp/client.go:27 — maxRetries = 3 means "3 retries + 1 initial = 4 total attempts" but forge clients use maxRetries to mean total loop iterations. The doc comment clarifies this, but the semantic inconsistency across packages could cause confusion.

Previous run

Review

Findings

Medium

• [edge-case] internal/gcp/client.go:145 — Status-code retries (500, 502, 503, 504) are applied to all HTTP methods, including non-idempotent POST and PATCH. The transport-error path correctly guards on isIdempotentMethod, but the status-code path does not — this diverges from the convention in internal/forge/gitlab and internal/forge/jira, which both gate 5xx status-code retries on method idempotency. If the server processes a non-idempotent request (e.g., AddSecretVersion POST to :addVersion) but returns 500 due to a transient backend failure, DoRequest will retry and could create a duplicate secret version.
  Remediation: Apply the same isIdempotentMethod(method) guard to the status-code retry path, or document that 5xx retries on non-idempotent methods are an accepted risk with an explanation of why it is safe for all current callers.

Low

• [edge-case] internal/gcp/client.go:115 — The access token is fetched once before the retry loop and reused across up to 4 attempts (~17s with backoff). Well within GCP token lifetimes (1 hour), but a near-expiry token would not benefit from retries.

• [scope-divergence] internal/gcp/client.go — Issue Add retry-with-backoff to GCP IAM API calls in WIF pool provisioning #6546 specifies retrying HTTP 429, but the PR intentionally excludes it to avoid double-retry with doWIFRequestWithRetry. Sound architectural decision; non-WIF callers won't get 429 retry.

• [naming-convention] internal/gcp/client.go — isRetryableTransportError / isRetryableStatusCode / defaultRetryDelay use different naming than analogous functions in fetch.go (isTransientRequestError / isTransientStatusCode / retryBackoff). The codebase has 3+ different conventions across packages, so the gcp package's internal consistency is reasonable.

• [api-shape-pattern] internal/gcp/client.go:42 — The retryDelayFn struct field for test-time delay injection differs from the afterFunc pattern in forge clients and the package-level var pattern in dispatch/gcf. The simpler duration-returning function is adequate for this use case.

• [naming-convention] internal/gcp/client.go:27 — maxRetries = 3 means "3 retries + 1 initial = 4 total attempts" but forge clients use maxRetries to mean total loop iterations. The doc comment clarifies this, but the semantic inconsistency across packages could cause confusion.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (2)

Review

Findings

Medium

• [edge-case] internal/gcp/client.go:111 — DoRequest retries transport errors (ECONNRESET, timeout, EOF) for all HTTP methods, including non-idempotent ones (POST, PATCH, DELETE). If a transport error occurs after the server processes the request but before the response arrives, the retry re-sends the request. Most GCP create endpoints handle this safely via 409 Conflict, but AddSecretVersion (POST to :addVersion) would silently create a duplicate secret version. The codebase already guards against this — gitlab.go and jira.go both check isIdempotent(method) before retrying transport errors.
  Remediation: Restrict automatic transport-error retries to idempotent methods (GET, PUT, DELETE), or add an opt-in flag for non-idempotent requests known-safe to retry. Alternatively, document this behavior and accept the trade-off.

Low

• [edge-case] internal/gcp/client.go:105 — The access token is fetched once before the retry loop and reused across up to 4 attempts (~17s with backoff). Well within GCP token lifetimes (1 hour), but a near-expiry token would not benefit from retries.

• [scope-divergence] internal/gcp/client.go — Issue Add retry-with-backoff to GCP IAM API calls in WIF pool provisioning #6546 specifies retrying HTTP 429, but the PR intentionally excludes it to avoid double-retry with doWIFRequestWithRetry. Sound architectural decision; non-WIF callers won't get 429 retry.

• [naming-convention] internal/gcp/client.go — isRetryableTransportError / isRetryableStatusCode / defaultRetryDelay use different naming than analogous functions in fetch.go (isTransientRequestError / isTransientStatusCode / retryBackoff). The codebase has 3+ different conventions across packages, so the gcp package's internal consistency is reasonable.

• [api-shape-pattern] internal/gcp/client.go:42 — The retryDelayFn struct field for test-time delay injection differs from the afterFunc pattern in forge clients and the package-level var pattern in dispatch/gcf. The simpler duration-returning function is adequate for this use case.

• [naming-convention] internal/gcp/client.go:27 — maxRetries = 3 means "3 retries + 1 initial = 4 total attempts" but forge clients use maxRetries to mean total loop iterations. The doc comment clarifies this, but the semantic inconsistency across packages could cause confusion.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 4:33 PM UTC · Completed 4:42 PM UTC

Commit: cd0e891 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.89

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (bot-triggered)

Restricted transport-error retries in DoRequest to idempotent HTTP methods (GET, HEAD, PUT, DELETE) by adding isIdempotentMethod guard, matching the established convention in internal/forge/gitlab and internal/forge/jira. Added tests covering POST and PATCH not being retried on transport errors. The five low-severity findings were all acknowledged as acceptable by the reviewer and require no changes.

Fixed (1):

1. DoRequest retries transport errors for all HTTP methods including non-idempotent ones (POST, PATCH, DELETE), risking duplicate side effects like AddSecretVersion creating an extra secret version (internal/gcp/client.go): Added isIdempotentMethod guard to transport-error retry path so only GET/HEAD/PUT/DELETE are retried on transient transport errors, matching the convention in internal/forge/gitlab and internal/forge/jira. Added tests for non-idempotent methods (POST, PATCH) verifying they are not retried on transport errors, and a unit test for isIdempotentMethod.

Disagreed (5):

1. Access token fetched once before retry loop; near-expiry token would not benefit from retries: GCP tokens have a 1-hour lifetime and the retry loop spans ~17s maximum with backoff. The probability of a token expiring mid-retry is negligible, and re-fetching the token on each attempt would add unnecessary complexity and latency. The reviewer acknowledged this as a low-severity edge case.
2. Issue Add retry-with-backoff to GCP IAM API calls in WIF pool provisioning #6546 specifies retrying HTTP 429, but the PR intentionally excludes it to avoid double-retry with doWIFRequestWithRetry: The reviewer explicitly notes this is a 'Sound architectural decision.' No change needed.
3. isRetryableTransportError / isRetryableStatusCode / defaultRetryDelay use different naming than analogous functions in fetch.go: The reviewer notes the codebase already has 3+ different conventions across packages, so the gcp package's internal consistency is reasonable. No change needed.
4. retryDelayFn struct field for test-time delay injection differs from the afterFunc pattern in forge clients: The reviewer states 'The simpler duration-returning function is adequate for this use case.' No change needed.
5. maxRetries = 3 semantic inconsistency with forge clients where maxRetries means total loop iterations: The reviewer notes 'The doc comment clarifies this.' The constant has a clear doc comment explaining total attempts = maxRetries + 1 = 4. No change needed.

Tests: passed

Decision points

• Whether to also restrict status-code retries (500/502/503/504) to idempotent methods (alternatives: Restrict both transport-error and status-code retries to idempotent methods (matching gitlab/jira pattern), Only restrict transport-error retries (reviewer's specific request); rationale: The reviewer specifically flagged transport-error retries for non-idempotent methods. Status-code retries are a different concern — the server explicitly returned a response, providing more information about whether the request was processed. Restricting only transport-error retries is the smallest correct fix that addresses the review finding.)

---

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:43 PM UTC · Completed 5:04 PM UTC

Commit: 9147466 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.96

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 5:06 PM UTC · Completed 5:13 PM UTC

Commit: 9147466 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.45

[fullsend-ai-coder]

🔧 Fix agent — iteration 2 (bot-triggered)

Fixed the medium-severity finding: added isIdempotentMethod guard to the 5xx status-code retry path so non-idempotent methods (POST, PATCH) are no longer retried on server errors. This prevents potential duplicate side effects (e.g. AddSecretVersion). Updated doc comment, added 8 new test cases, and fixed existing body-preservation test. The 5 low-severity findings were all informational observations that the reviewer explicitly acknowledged as acceptable.

Fixed (1):

1. Status-code retries (500, 502, 503, 504) applied to all HTTP methods including non-idempotent POST and PATCH (internal/gcp/client.go): Added isIdempotentMethod(method) guard to the status-code retry path, matching the transport-error path and the convention in internal/forge/gitlab and internal/forge/jira. Updated DoRequest doc comment to clarify unified idempotency policy. Added 8 test cases (POST/PATCH × 500/502/503/504) and fixed TestDoRequest_RetryPreservesRequestBody to use PUT instead of POST.

Disagreed (5):

1. Access token fetched once before retry loop and reused across up to 4 attempts (~17s with backoff): Informational observation. The reviewer acknowledges this is well within GCP token lifetimes (1 hour). No code change needed.
2. Issue Add retry-with-backoff to GCP IAM API calls in WIF pool provisioning #6546 specifies retrying HTTP 429, but the PR intentionally excludes it: The reviewer calls this a 'sound architectural decision'. No code change needed — excluding 429 avoids double-retry with doWIFRequestWithRetry.
3. isRetryableTransportError / isRetryableStatusCode / defaultRetryDelay use different naming than fetch.go: Informational observation. The reviewer acknowledges the gcp package's internal consistency is 'reasonable'. No code change needed.
4. retryDelayFn struct field differs from the afterFunc pattern in forge clients: Informational observation. The reviewer states the simpler duration-returning function is 'adequate for this use case'. No code change needed.
5. maxRetries = 3 means '3 retries + 1 initial = 4 total attempts' but forge clients use maxRetries differently: Informational observation. The reviewer notes 'The doc comment clarifies this'. No code change needed — the constant has a clear doc comment explaining the semantics.

Tests: passed

Decision points

• Whether to gate status-code retries on idempotency or document the risk as accepted (alternatives: Apply isIdempotentMethod guard (matching forge convention), Document as accepted risk; rationale: Applied the guard to match the existing convention in internal/forge/gitlab and internal/forge/jira, and to be consistent with the transport-error path in the same function.)

---

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:16 PM UTC · Completed 5:35 PM UTC

Commit: 68a7787 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.11

Superseded by updated review

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:52 PM UTC · Completed 7:07 PM UTC

Commit: 68a7787 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.03

[fullsend-ai-retro]

Retro: PR #6861 — retry-with-backoff for GCP client

This fully agent-driven PR (code → 3 reviews → 2 fixes → human approval → merge) added retry-with-exponential-backoff to internal/gcp/client.go to fix WIF pool provisioning failures (#6546). Total agent cost was $21.30 across ~2.75 hours wall time.

Timeline

1. Code agent (run 33529822076, $5.93 est.) produced a working implementation that followed the retry pattern from internal/fetch/fetch.go. However, fetch.go only makes GET requests and never needed idempotency guards — the code agent didn't recognize that DoRequest handles POST/PATCH/DELETE too.
2. Review docs: Add agent-compatible code problem document #1 (33529883399, $6.89) correctly caught the missing idempotency guard on the transport-error retry path (medium) plus 5 low findings.
3. Fix docs: Add agent-compatible code problem document #1 (33532323715, $1.89) added isIdempotentMethod() to the transport-error path but missed applying the same guard to the status-code retry path in the same function.
4. Review Add problem areas: Tekton pipeline review, migration path, multi-tenancy #2 (33533363767, $5.96) caught the status-code path gap (new medium) and repeated the same 5 low findings.
5. Fix Add problem areas: Tekton pipeline review, migration path, multi-tenancy #2 (33535641068, $1.45) applied the guard to the status-code path and added 8 new test cases.
6. Review docs: Add codebase context problem document and trim CLAUDE.md #3 (33536537496, $5.11) approved with only 2 residual low findings. Human reviewer (ralphbean) approved without comments. Merged at 18:50 UTC.

What worked well

• Review agent caught a real correctness bug — retrying non-idempotent HTTP methods (POST/PATCH) on transport errors and 5xx responses could cause duplicate side effects (e.g., creating duplicate secret versions). This is the kind of subtle safety issue that matters.
• Final code quality is high — comprehensive test coverage (93.4% patch), proper idempotency guards, injectable delay for test determinism, and clear documentation in the code.
• The review → fix → re-review loop converged in 2 iterations to a correct result.

Evidence for existing issues

• Fix agent should generalize validation fixes to cover adjacent edge cases of the same pattern #5256 / Code agent: apply validation fixes across all code paths in the same function #1737 (fix agent generalization): Fix iteration 1 guarded the transport-error path but missed the status-code path in the same function — the exact pattern described in these issues. This gap cost an extra review ($5.96) + fix ($1.45) cycle and ~45 minutes of wall time.
• Review agent should deduplicate findings across re-review iterations on the same PR #2959 / Review agent: persistent finding cache for cross-iteration deduplication and dismissal enforcement #2816 / Review agent should suppress findings already dismissed by the PR author #4682 (review agent repeating dismissed findings): All 3 reviews repeated the same 5 low-severity findings, even after the fix agent explicitly disagreed with them in iteration 1. The fix agent's disagreement reasons (e.g., 429 exclusion was intentional architecture, naming conventions are subjective) were sound. Repeating them inflated review costs by an estimated $3–5 across the later reviews.
• Add retry with backoff for openshell binary download in sandbox setup #6828 (OpenShell download retry): Behaviour test PR_label_dispatches_PR-only_harness failed on the first push due to curl: (35) Recv failure: Connection reset by peer downloading OpenShell checksums — a transient network failure unrelated to the PR's code changes.
• Mint WASM isolate poisons permanently after GitHub API timeout, causing cascading 503s #6821 (mint isolate poisoning): Behaviour test Fork_PR_label_dispatches_harness failed due to mint service HTTP 503 (mint instance poisoned after timeout — awaiting isolate recycle) — a Cloudflare Worker isolate lifecycle issue, also unrelated to PR changes.

Assessment

The workflow operated correctly — the review agent caught genuine bugs, the fix agent resolved them, and the final code is solid. The main inefficiency was the fix agent not generalizing its idempotency fix across both retry paths in the same function, costing an extra iteration. All improvement opportunities are tracked by existing open issues. No new proposals needed.