Skip to content

Adjust http client to support rate limits#775

Merged
mcncl merged 3 commits intomainfrom
TE-5541/api_rate_limits
Apr 10, 2026
Merged

Adjust http client to support rate limits#775
mcncl merged 3 commits intomainfrom
TE-5541/api_rate_limits

Conversation

@mcncl
Copy link
Copy Markdown
Contributor

@mcncl mcncl commented Apr 10, 2026

Description

Changed the way that the CLI implements rate limits and the back off. We've removed the dependency on roko, which shouldn't be required with the availability of the RateLimit- headers that we can get from client calls.

Moves the logic in to a reusable HTTP transport so commands/clients can share it.

Changes

  • Added RateLimitTransport in internal/http/ratelimit.go:
    • Retries on HTTP 429 up to MaxRetries
    • Uses RateLimit-Reset for backoff, with MaxRetryDelay cap
  • Removed retry/backoff code from internal/http/client.go:
    • Deleted WithMaxRetries, WithMaxRetryDelay, WithOnRetry
    • Removed RetryAfter/retry loop from Client.Do
  • Updated call sites to use transport-based retry:
  • Extended pkg/cmd/factory/factory.go with WithTransport(http.RoundTripper) and composed it with debug transport.

Caveats

  • Removed github.com/buildkite/roko from go.mod/go.sum.

Testing

  • Tests have run locally (with go test ./...)
  • Code is formatted (with go fmt ./...)

Disclosures / Credits

I used AI assistance (OpenCode / GPT-5.3 Codex) to review the diff, it highlighted a regression in some logic I'd written where if RateLimit-Reset was equal to 0 on check it would wait 10s.

@mcncl mcncl requested review from a team as code owners April 10, 2026 00:47
@mcncl mcncl force-pushed the TE-5541/api_rate_limits branch 2 times, most recently from 4c07823 to 8467387 Compare April 10, 2026 00:55
Comment thread cmd/preflight/preflight.go Outdated
mcncl added 2 commits April 10, 2026 14:40
Changed the way that the CLI implements rate limits and the back off. We've removed the dependency on `roko`, which shouldn't be required with the availability of the `RateLimit-` headers that we can get from client calls.
@mcncl mcncl force-pushed the TE-5541/api_rate_limits branch from a1b7ebe to 901b59c Compare April 10, 2026 04:40
@mcncl mcncl requested a review from matthewborden April 10, 2026 04:41
Comment on lines +61 to +65
func WithTransport(t http.RoundTripper) FactoryOpt {
return func(c *factoryConfig) {
c.transport = t
}
}
Copy link
Copy Markdown
Contributor

@matthewborden matthewborden Apr 10, 2026

Choose a reason for hiding this comment

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

Does this overwrite the retry behaviour for 429s in https://github.com/buildkite/go-buildkite/blob/main/buildkite.go#L383-L403? Could there be two backoff loops on top of each other for 429s if it doesn't?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@matthewborden it does; go-buildkite uses a generic exponential backoff which will act as a fallback if our checks fail for some reason (missing headers or something)

We're using the RateLimit-Reset, which will mean we have an exact time for retrying, rather than the exponential value that go-buildkite might use

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.

Let's address this in a followup PR, I'd prefer to have one backoff loop managing rate limit retries.

@mcncl mcncl enabled auto-merge (squash) April 10, 2026 06:26
@mcncl mcncl merged commit 896f866 into main Apr 10, 2026
3 checks passed
@mcncl mcncl deleted the TE-5541/api_rate_limits branch April 10, 2026 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants