Skip to content

feat: add gitlab/github provider API retries - #2854

Open
chmouel wants to merge 1 commit into
tektoncd:mainfrom
chmouel:SRVKP-12884-api-retry
Open

feat: add gitlab/github provider API retries#2854
chmouel wants to merge 1 commit into
tektoncd:mainfrom
chmouel:SRVKP-12884-api-retry

Conversation

@chmouel

@chmouel chmouel commented Jul 15, 2026

Copy link
Copy Markdown
Member

📝 Description of the Change

Allow administrators to opt in to retrying temporary GitHub and GitLab API failures. This prevents short rate-limit windows and provider outages from immediately dropping webhook work that could succeed a few moments later.

Key aspects:

  • The feature is off by default. With it off, GitHub performs no retries and GitLab keeps the retry behaviour of the upstream GitLab client, so no existing installation changes behaviour.
  • Administrators control the attempt count and the maximum wait, and both providers read those settings the same way.
  • Retries are spread over time to avoid request floods. Provider rate-limit headers are only honoured on an actual rate-limit response; other temporary failures get a short bounded delay.
  • PAC stops retrying when the provider asks it to wait longer than the configured limit, and never repeats a request that could duplicate a change on the provider.
  • Consistent behaviour across GitHub and GitLab operations, including GitHub App setup and temporary provider clients.

🔗 Linked GitHub Issue

SRVKP-12884

🧪 Testing Strategy

  • Unit tests
  • Integration tests
  • End-to-end tests
  • Manual testing
  • Not Applicable

Testing coverage: Unit tests document enabled, disabled, exhausted, and successful retry scenarios, the attempt-limit and wait-cap behaviour, the idempotency rules, and the transient-failure backoff bounds. A full end-to-end test is not included because safely forcing rate limits on shared live provider accounts is disruptive and unreliable.

🤖 AI Assistance

  • I have not used any AI assistance for this PR.
  • I have used AI assistance for this PR.

✅ Submitter Checklist

  • 📝 My commit messages are clear, informative, and follow the project's How to write a git commit message guide. The Gitlint linter ensures in CI it's properly validated
  • ✨ I have ensured my commit message prefix (e.g., fix:, feat:) matches the "Type of Change" I selected above.
  • ♽ I have run make test and make lint locally to check for and fix any issues. For an efficient workflow, I have considered installing pre-commit and running pre-commit install to automate these checks.
  • 📖 I have added or updated documentation for any user-facing changes.
  • 🧪 I have added sufficient unit tests for my code changes.
  • 🎁 I have added end-to-end tests where feasible. See README for more details.
  • 🔎 I have addressed any CI test flakiness or provided a clear reason to bypass it.
  • If adding a provider feature, I have filled in the following and updated the provider documentation:
    • GitHub App
    • GitHub Webhook
    • Gitea/Forgejo
    • GitLab
    • Bitbucket Cloud
    • Bitbucket Data Center

@chmouel chmouel added feature New feature or request documentation Improvements or additions to documentation labels Jul 15, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a rate-limit-aware API retry mechanism for GitHub and GitLab providers, configurable via settings and configmaps, using a new retryhttp package that wraps http.RoundTripper with exponential backoff and jitter. Feedback on the changes highlights three key issues: first, the GitLab retry policy incorrectly disables retries for network-level errors (when resp is nil) even on idempotent requests; second, a potential integer overflow can occur in retryhttp backoff calculations if the attempt count is extremely high; and third, returning a previously closed response when req.GetBody() fails during a retry can lead to unexpected behavior.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread pkg/provider/gitlab/gitlab.go Outdated
Comment thread pkg/provider/retryhttp/retryhttp.go Outdated
Comment thread pkg/provider/retryhttp/retryhttp.go
@chmouel
chmouel force-pushed the SRVKP-12884-api-retry branch from b6b281d to e0ed9ca Compare July 15, 2026 12:26
@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.45652% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.82%. Comparing base (f9e1a7a) to head (b52e222).

Files with missing lines Patch % Lines
pkg/provider/retryhttp/retryhttp.go 70.78% 17 Missing and 9 partials ⚠️
pkg/provider/gitlab/gitlab.go 81.69% 8 Missing and 5 partials ⚠️
pkg/adapter/incoming.go 0.00% 2 Missing and 1 partial ⚠️
pkg/provider/github/github.go 85.71% 1 Missing and 1 partial ⚠️
pkg/provider/github/parse_payload.go 60.00% 1 Missing and 1 partial ⚠️
pkg/provider/gitlab/task.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2854      +/-   ##
==========================================
+ Coverage   68.77%   68.82%   +0.05%     
==========================================
  Files         197      198       +1     
  Lines       16866    17041     +175     
==========================================
+ Hits        11599    11728     +129     
- Misses       4405     4434      +29     
- Partials      862      879      +17     
Flag Coverage Δ
unit-tests 68.82% <74.45%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chmouel chmouel removed the documentation Improvements or additions to documentation label Jul 15, 2026
@chmouel
chmouel force-pushed the SRVKP-12884-api-retry branch from e0ed9ca to 8145908 Compare July 15, 2026 13:01
@chmouel
chmouel force-pushed the SRVKP-12884-api-retry branch from 8145908 to 5a1975f Compare July 15, 2026 13:09
@chmouel
chmouel marked this pull request as ready for review July 15, 2026 18:08
@chmouel

chmouel commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

hard to test to be honest, the only way to do this is to have a fake GitHub/Gitlab API server which is a bit crazy overblown

@chmouel

chmouel commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces Git provider API retry capabilities for GitHub and GitLab in Pipelines-as-Code. It adds configuration settings (enable-api-retry, api-retry-max-attempts, and api-retry-max-wait-seconds) and implements a rate-limit-aware retrying HTTP transport (retryhttp) using exponential backoff with jitter. Feedback on the changes highlights several potential nil pointer dereference vulnerabilities when handling HTTP responses and headers (specifically in retryhttp.go and gitlab.go), a potential timer leak when using time.After in a select block, and unhandled errors when initializing the GitHub enterprise client.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread pkg/provider/retryhttp/retryhttp.go Outdated
Comment thread pkg/provider/retryhttp/retryhttp.go
Comment thread pkg/provider/retryhttp/retryhttp.go
Comment thread pkg/provider/retryhttp/retryhttp.go
Comment thread pkg/provider/gitlab/gitlab.go
Comment thread pkg/provider/github/parse_payload.go
@chmouel
chmouel force-pushed the SRVKP-12884-api-retry branch from 5a1975f to eea202d Compare July 16, 2026 11:14
Comment thread pkg/provider/gitlab/gitlab.go Outdated
Comment thread pkg/provider/gitlab/gitlab.go
@zakisk

zakisk commented Jul 24, 2026

Copy link
Copy Markdown
Member

Paco Review ⚠️

This PR adds optional, provider-side HTTP retry support for GitHub and GitLab API clients, controlled by three new settings (enable-api-retry, api-retry-max-attempts, api-retry-max-wait-seconds). It introduces a new retryhttp package wrapping GitHub's oauth2/app transports with a rate-limit-aware backoff transport, and wires GitLab's client-go options to use retryablehttp with a custom retry policy/backoff that honors Retry-After/RateLimit-Reset headers and avoids retrying non-idempotent methods after ambiguous failures. It also refactors GitHub client construction to route through provider-level MakeClient so retry settings can be injected, and adds substantial unit test coverage for both providers' retry behavior.

Reviewed commit: 5621eac

@zakisk zakisk 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.

Paco inline comments -- see the Paco Review summary comment for the overview.

Comment thread pkg/provider/gitlab/gitlab.go
Copilot AI review requested due to automatic review settings July 27, 2026 07:39
@zakisk
zakisk force-pushed the SRVKP-12884-api-retry branch from 5621eac to f7c11a9 Compare July 27, 2026 07:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (2)

pkg/provider/gitlab/gitlab.go:113

  • When retries are disabled, clientOptions appends gitlab.WithoutRetries(), which disables the GitLab client-go library’s built-in retries (the vendored client defaults to RetryMax: 5). Previously, the provider created the client with only WithBaseURL, so adding WithoutRetries() changes behavior even though the feature is meant to be disabled by default.
	opts := []gitlab.ClientOptionFunc{gitlab.WithBaseURL(apiURL)}
	if v.pacInfo == nil || !v.pacInfo.EnableAPIRetry {
		return append(opts, gitlab.WithoutRetries())
	}

pkg/provider/retryhttp/retryhttp.go:140

  • Retry-After can be either a delay in seconds or an HTTP-date. This transport only parses the seconds form, so an HTTP-date value would be ignored and the code would fall back to exponential backoff (potentially retrying earlier than the server asked).
		if s := resp.Header.Get("Retry-After"); s != "" {
			if secs, err := strconv.Atoi(s); err == nil {
				wait := time.Duration(secs) * time.Second
				if wait > t.opts.MaxWait {
					return 0, false

@chmouel

chmouel commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

Pushed an update that reworks this after the review feedback. Summary of how it behaves now.

Default behaviour (enable-api-retry: "false")

This was the main thing wrong before. The previous revision passed gitlab.WithoutRetries() when the setting was off, which removed retries that the GitLab client has always done, so "disabled by default preserves existing behaviour" was not true for GitLab (thanks @theakshaypant).

Now, with the setting off:

  • GitHub does no retries, same as before this PR.
  • GitLab keeps the upstream GitLab client behaviour untouched.

Turning the setting on is what switches both providers over to the rate-limit aware policy added here.

Settings

Setting Default Meaning
enable-api-retry false Opt in to the PAC retry policy
api-retry-max-attempts 4 Total attempts, initial request included
api-retry-max-wait-seconds 120 Cap on the wait between attempts

Both providers now read these through the same settings.DefaultAPIRetryMaxAttempts / DefaultAPIRetryMaxWaitSeconds constants, so an unset or invalid api-retry-max-attempts falls back to 4 on GitHub and GitLab. Previously GitLab clipped it to 1, which silently disabled retries (thanks @zakisk).

What gets retried

  • Rate limits — 429, and on GitHub 403 with rate-limit headers.
  • Transient failures — 5xx and network errors, but only for GET, HEAD and OPTIONS. Mutations are never repeated after an uncertain failure, since the server may already have applied them and we would end up with duplicate comments or statuses.

Backoff

  • On an actual rate-limit response, Retry-After / X-RateLimit-Reset / RateLimit-Reset are honoured, with jitter, capped at api-retry-max-wait-seconds. If the reset is further away than the cap, PAC gives up rather than holding the event.
  • On any other retryable failure, a short bounded delay is used.

That second point is a fix from this round: the GitLab backoff used to call LinearJitterBackoff(minWait, maxWait, ...), which draws a uniform random value in [1s, 120s] rather than backing off exponentially. A single transient 500 on a GET could therefore stall event processing for minutes. The rate-limit headers were also being consulted on non-429 responses, and GitLab sets RateLimit-Reset on ordinary responses, so a plain 500 could be delayed until the rate-limit window reset.

Unrelated regressions fixed

While validating this I found the branch had accidentally reverted several nil-safety fixes from e309d1a67 ("fix: detect and prevent nil pointer crashes"), most likely a bad rebase. Four existing tests were failing on the previous tip, two with nil-pointer panics. Restored:

  • MakeClient returning an error instead of a nil client on an invalid enterprise URL.
  • expandGlobAndAddRepoIDs reporting an invalid glob pattern.
  • The resp == nil guards in three paginated GitHub loops.

make lint and go test ./pkg/... are green.

@chmouel
chmouel force-pushed the SRVKP-12884-api-retry branch from a500a4f to 5a0859c Compare July 31, 2026 10:31
@chmouel

chmouel commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

I have enabled this on our dogfood infra so we can dogfood it a bit when it merges
image

Allow administrators to opt in to retrying temporary GitHub and GitLab
API failures. This prevents short rate-limit windows and provider
outages from immediately dropping webhook work that could succeed a
few moments later.

Keep the feature disabled by default so existing installations retain
their current behavior. With the setting off, GitHub performs no retries
and GitLab keeps the retry behavior of the upstream GitLab client. When
enabled, administrators can control how many times an operation is
attempted and how long PAC may wait for the provider to recover, and
both providers read those settings the same way.

Spread retries over time rather than sending every request again at
once. Use the provider rate-limit headers only on an actual rate-limit
response and give other temporary failures a short bounded delay. Stop
retrying when the provider asks PAC to wait longer than the configured
limit, avoiding long-running webhook work and reducing the risk of
releasing a large backlog in one wave.

Apply the same behavior consistently to GitHub and GitLab operations,
including GitHub App setup and temporary provider clients. Avoid
repeating requests when doing so could accidentally create duplicate
changes on the provider.

Document the new configuration and cover enabled, disabled, exhausted,
and successful retry scenarios with automated tests. A full end-to-end
test is not included because safely forcing rate limits on shared live
provider accounts is disruptive and unreliable.

Co-Authored-By: Claude <noreply@anthropic.com>
Jira: https://issues.redhat.com/browse/SRVKP-12884
Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request github gitlab

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants