Repository navigation
Space requests after the rate limiter has been idle, and update the modules - #76
Merged
Merged
Conversation
…idle A request's slot was one interval after the last reservation, which, after the client had been idle, was in the past, so a burst of concurrent requests all took slots which had passed, and were sent at once, exceeding the rate, such as AcoustID's three each second. A slot is now the next one, or now, when that's passed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix preserves synchronized reservations and adds regression coverage for the reported failure, with no unresolved issues identified.
No open issues 🎉
What changed in this PR
This PR fixes idle-period bursts in RateLimitTransport by preventing requests from reserving slots in the past.
Changes:
- Reserve the later of the current time or the next scheduled slot, preserving immediate dispatch after idle while spacing subsequent requests.
- Add regression coverage for concurrent requests arriving after an idle period.
| File | Description |
|---|---|
pkg/transport/ratelimit.go |
Clamp reservations to the current time to prevent idle-burst dispatch. |
pkg/transport/ratelimit_test.go |
Verify request spacing for a concurrent burst after idle. |
Review effort: Balanced
…v1.7 The modules are updated, including go-server v1.6.24 to v1.7.16, OpenTelemetry v1.43.0 to v1.47.0, and golang.org/x, gRPC and kong. The tests use go-server's types.ContentTypeAny, ContentTypeJSON and ContentTypeJSONStream, which replaced the client's own, and the streaming example's handler is a JSONStreamHandlerFunc, which receives and sends with a JSONStream. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
djthorpe
changed the title
…tions go-server v1.7.16 needs Go 1.27, so the tests run, and the image is built, with Go 1.27, rather than 1.25 and 1.26, and the workflows use checkout v5 and setup-go v6. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the rate limiter letting a burst of requests through at once after the client has been idle, and updates the modules.
The bug
RateLimitTransportgave each request the slot one interval after the last reservation. After the client had been idle, that slot was in the past, so concurrent requests each reserved a slot one interval after the last, all of which had passed, and all were sent at once. omodie's indexing hit this with AcoustID, which allows three requests a second: several lookups arriving together after a quiet spell were rejected with429 rate limit (4.000000 requests per second) exceeded.The fix
A request's slot is the next one after the last reservation, or now, when that's passed. The first request, and one after an idle spell, are still sent straight away, and the rest of a burst is spaced by the interval.
Tests
TestRateLimit_ConcurrentRequestsAfterIdleAreThrottledsends a request, waits longer than a burst would take, and then sends four concurrent requests, which must be about an interval apart. Without the fix, they were sent within 50µs of each other.pkg/transportpasses with-race.Module updates
client.ContentTypeAny,ContentTypeJsonandContentTypeJsonStream, which no longer exist, so didn't build: they use go-server'stypes.ContentTypeAny,ContentTypeJSONandContentTypeJSONStream.example/streamingdidn't build against go-server v1.7: its handler is now aJSONStreamHandlerFunc, which receives withstream.Recv()and sends withstream.Send().go vet ./...is clean, and everything passes exceptpkg/homeassistant, whose tests need a Home Assistant server on the network.