Repository navigation
feat: Obey Octopus Server rate limits - #493
Conversation
YuKitsune
changed the title
YuKitsune
left a comment
YuKitsune
left a comment
There was a problem hiding this comment.
The design is good and follows normal Go practice: a RoundTripper wrapper, a client helper that does not change base, retries only when GetBody is set, and waits that stop when the context is cancelled. There are no blocking issues. See the inline comments for two fixes and some smaller items.
Also: http.Client.Timeout includes the retry and throttle waits. If Timeout < MaxWait, a retry can fail with a timeout. A note in the README or the NewClient doc would help.
Reviewed by Claude Code.
YuKitsune
changed the title
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ient Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Default MaxWait when it is 0, pass CloseIdleConnections to the base transport, and remove upper-bound timing checks from the tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
db6bf13 to
40b04b4
Compare
Port the GCRA pacer and the retry delay from the C# and TypeScript clients, with their test cases. Rename the options to match, and default to 5 retries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sathvikkumar-octo
left a comment
sathvikkumar-octo
left a comment
There was a problem hiding this comment.
Approved it, some nits that I'll let you consider
| const ( | ||
| // PolicyHeader is the advisory header that tells the burst limit and the requests per minute. | ||
| // For example: "l=200;rpm=600". | ||
| PolicyHeader = "Octopus-RateLimit-Policy" |
There was a problem hiding this comment.
Do we need to export PolicyHeader or RateLimitHeader from this package?
| require.Equal(t, 3, server.requestCount()) | ||
| } | ||
|
|
||
| func TestRoundTripWaitsForRetryAfter(t *testing.T) { |
There was a problem hiding this comment.
Nit, non-blocking: A few tests overlap and could go to keep the suite lean:
TestRoundTripWaitsForRetryAftersleeps a real 1s.retry_testcovers theRetry-Afterdelay, and…WaitsTheDefaultDelay…already showsRoundTripwaits.TestRetryDelayRetriesImmediatelyAtEachAttemptWhenRetryAfterIsZerois the same as theZerocase.TestNewHttpClientKeepsTheSettingsOfBaseonly tests Go's struct copy.TestNewHttpClientRetriesTooManyRequestsand…CloseIdleConnectionsReachesTheBaseTransportare covered by the transport tests.TestNewClientWithoutRateLimitedHttpClientReturnsTooManyRequeststests the client without rate limiting, which this PR doesn't change.TestDefaultOptionsrestates the defaults.
TestNewTransportPacesOnlyWhenUseRateLimitHeadersIsTrue checks the private pacer field. A behaviour test would cover the same thing without depending on internals: with UseRateLimitHeaders: false, an r=0 response shouldn't cause a wait.
Depends on OctopusDeploy/go-octopusdeploy#493 Upgrades `go-octopusdeploy` to v2.122.0, and starts using the new rate-limiting HTTP client with the default settings. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Adds the option to respect the rate limiting headers returned by Octopus Server.
The GCRA algorithm is copied from the TypeScript client used in the frontend. The defaults are also copied from the TS client.
GCRA: https://brandur.org/rate-limiting