Skip to content

perf(#6059): make LiveGCFClient poll delay injectable for tests - #6062

Merged
ralphbean merged 1 commit into
mainfrom
agent/6059-gcf-injectable-poll-delay
Aug 13, 2026
Merged

perf(#6059): make LiveGCFClient poll delay injectable for tests#6062
ralphbean merged 1 commit into
mainfrom
agent/6059-gcf-injectable-poll-delay

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

  • Add an injectable pollDelay field to LiveGCFClient (defaults to time.After) so tests can bypass real sleeps in operation-polling loops
  • Update WaitForOperation, waitForIAMOperation, and waitForCloudRunOperation to use the injectable delay
  • Inject an immediate-return delay in 4 tests that exercise polling paths (TestLiveGCFClient_WaitForOperation/completes_on_second_poll and 3 UpdateServiceEnvVars polling sub-tests)

Testing

  • go test ./internal/dispatch/gcf/... -v — all tests pass (0.164s total, down from ~8s+ with real sleeps)
  • go vet ./internal/dispatch/gcf/... — clean
  • gofmt — clean
  • scan-secrets — clean

Notes

  • Follows the same injectable-function pattern already used by iamRetryDelay in this package
  • golangci-lint was not available in the sandbox; pre-commit failed due to sandbox network policy blocking git fetch

Closes #6059

Post-script verification

  • Branch is not main/master (agent/6059-gcf-injectable-poll-delay)
  • Secret scan passed (gitleaks — 483ad9bd31407310940d21003a6cfe1b6b52b5c8..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

Add a pollDelay field to LiveGCFClient that defaults to time.After.
WaitForOperation, waitForIAMOperation, and waitForCloudRunOperation
now use this injectable function instead of calling time.After
directly. Tests inject an immediate-return function to avoid real
5-second (or 2-3 second) sleeps during polling loops.

The "completes on second poll" sub-test of
TestLiveGCFClient_WaitForOperation previously took ~5s and now
completes in <10ms. Three UpdateServiceEnvVars polling tests that
exercised waitForCloudRunOperation are also updated.

Note: golangci-lint was not available in the sandbox. pre-commit
failed due to sandbox network policy (git fetch blocked). go vet
and gofmt passed.

Closes #6059
@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner August 10, 2026 19:50
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Aug 10, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:51 PM UTC · Completed 8:05 PM UTC

Commit: bc07fa8 · View workflow run →

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/dispatch/gcf/gcp.go 70.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Low

  • [comment-style-consistency] internal/dispatch/gcf/gcp.go:182 — The pollDelay field uses a multi-line doc comment, while the adjacent skipUploadURLCheck test-only field uses an inline comment (// testing only: ...). For consistency within the struct, consider matching the inline comment style.

  • [pattern-inconsistency] internal/dispatch/gcf/gcp.go — This PR introduces a struct-field injection pattern (pollDelay) for test time control, while the same file uses a package-level var pattern (iamRetryDelay) for a related concern. The patterns have different signatures and semantics (exponential backoff with attempt count vs. simple time.After replacement), so the divergence is partially inherent. Consider filing a follow-up to harmonize toward the struct-field pattern.


Labels: PR modifies GCF dispatch polling infrastructure and is primarily a test performance improvement

Comment thread internal/dispatch/gcf/gcp.go
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge component/dispatch Workflow dispatch and triggers testing labels Aug 10, 2026

@ralphbean ralphbean left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@ralphbean
ralphbean added this pull request to the merge queue Aug 11, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 11, 2026
@ralphbean
ralphbean added this pull request to the merge queue Aug 11, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 11, 2026
@ralphbean
ralphbean added this pull request to the merge queue Aug 13, 2026
Merged via the queue into main with commit ed0deb8 Aug 13, 2026
35 of 37 checks passed
@ralphbean
ralphbean deleted the agent/6059-gcf-injectable-poll-delay branch August 13, 2026 17:56
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 13, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 5:58 PM UTC · Completed 6:13 PM UTC

Commit: bc07fa8 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #6062 — make LiveGCFClient poll delay injectable for tests

Timeline

Time (UTC) Event
Aug 10, 19:36 Issue #6059 filed
Aug 10, 19:40 Triage agent completed (~4 min)
Aug 10, 19:50 Code agent completed (~10 min), PR created
Aug 10, 20:05 Review agent approved with 2 low-severity findings
Aug 11, 14:36 Human reviewer (@ralphbean) approved — "LGTM"
Aug 13, 17:56 PR merged

What went well

  • Single-pass implementation. The code agent produced a clean, correct implementation on the first try — no fix iterations required. The change follows an established injectable-function pattern (iamRetryDelay) already in the codebase.
  • Fast pipeline. Issue-to-PR took ~14 minutes of active agent time (triage + code). Total triage-to-merge was ~3 days, dominated by human review wait time.
  • Appropriate review findings. The review agent identified two reasonable low-severity issues: comment style inconsistency between test-only fields, and dual injection patterns (struct field vs package variable) that could be harmonized.
  • Review agent matched human. The human reviewer approved without additional findings, confirming the review agent's analysis was adequate for this class of change (small, focused Go performance improvement).

Coverage gap — main finding

The code agent introduced a getPollDelay() helper with a nil-guard default branch (if c.pollDelay != nil { return c.pollDelay }; return time.After). Tests only exercise the injected (non-nil) path, leaving the default path uncovered. Result: 70% patch coverage, below the repo's 80% Codecov threshold. The PR was merged with the codecov/patch check failing.

This is already addressed. #2810 ("Code agent should validate patch coverage meets repo threshold before opening PR") was implemented via PR #6138 and closed today (Aug 13). That PR added coverage verification guidance to AGENTS.md, docs/contributing/go-code.md, and a new check-patch-coverage skill. PR #6062's code agent run (Aug 10) predates the fix, so this is an expected gap.

Evidence for existing open issues

  • #5148: The review agent's correctness sub-agent found the untested waitForIAMOperation polling loop but classified it as info severity and filtered it below the low threshold. This PR is a concrete case where info-level severity is insufficient for untested conditional branches.
  • #4057: The code agent's tests exercise the injected path of getPollDelay() but not the default (nil) path — a concrete example of tests not exercising the specific code path being changed.
  • fullsend-ai/agents#397: The review agent could have estimated aggregate patch coverage and flagged the below-threshold result, but this capability is not yet implemented.

No new proposals

All identified improvement opportunities are tracked by existing open issues. The main finding (missing coverage verification) was addressed by #2810 just today. This retro provides supporting evidence for the existing issues listed above rather than novel findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/dispatch Workflow dispatch and triggers ready-for-merge All reviewers approved — ready to merge ready-for-review Triggers review agent dispatch testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gcf: LiveGCFClient.WaitForOperation polls sleep for real in unit tests

1 participant