Add ETag conditional requests to the REST transport - #3026
Open
joshfree wants to merge 3 commits into
Open
Conversation
Every REST request was issued unconditionally: the ETag returned by the GitHub API was never stored or replayed, so repeated tool calls that read the same resource (for example pull request reads, file and commit listings, and reviews) re-downloaded the full response each time. This adds an ETagTransport round tripper that caches the ETag and body of cacheable GET responses and sends If-None-Match on the next identical request. When the API answers 304 Not Modified, the cached body is served instead of re-downloading it. The transport is inserted below the user-agent and auth layers in createGitHubClients(), so cached entries are scoped by the request's Authorization header and never shared across tokens. The cache is bounded (LRU) and safe for concurrent use. Every request is still sent to the server, so responses are always revalidated and never served stale. Per the GitHub REST API docs, a 304 Not Modified response does not count against the token's primary rate limit, so repeated reads conserve rate-limit budget and bandwidth while returning identical data. Rate-limit headers are surfaced from the live 304 response so downstream rate-limit accounting stays correct. Closes github#3025 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79
Contributor
There was a problem hiding this comment.
Pull request overview
Adds ETag-based conditional revalidation to GitHub REST requests to reduce bandwidth and rate-limit usage.
Changes:
- Adds a concurrency-safe LRU ETag transport.
- Integrates it into REST client construction.
- Adds header constants and transport tests.
Show a summary per file
| File | Description |
|---|---|
pkg/http/transport/etag.go |
Implements ETag caching and revalidation. |
pkg/http/transport/etag_test.go |
Tests conditional-request behavior. |
pkg/http/headers/headers.go |
Adds ETag header constants. |
internal/ghmcp/server.go |
Enables the transport for REST clients. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
Comment on lines
+131
to
+132
| if resp.StatusCode == http.StatusOK { | ||
| if etag := resp.Header.Get(headers.ETagHeader); etag != "" { |
Comment on lines
+131
to
+133
| if resp.StatusCode == http.StatusOK { | ||
| if etag := resp.Header.Get(headers.ETagHeader); etag != "" { | ||
| body, readErr := io.ReadAll(resp.Body) |
The do/helper closures returned *http.Response, which the bodyclose linter flags at each call site even though the body is closed inside the closure. Return only the asserted values (status code, body, and headers) so no response escapes the helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79
Collaborator
|
Thanks for this, the aims make sense, however most of our usage is through the hosted remote server, which is horizontally scaled and so would need to think about this because it's pretty expensive to add a distributed store there, and additional Redis usage, extra network request per request that would normally be false adding latency, in-memory store would likely be much less useful and we also handle hundreds of RPS so really would need to think about this in that context. |
revive's redefines-builtin-id flags the local variable named max in the LRU eviction loop, which shadows the Go 1.21 builtin. Rename it to limit; no behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79
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.
Summary
Adds HTTP conditional-request (ETag /
If-None-Match) support to the REST transport so unchanged resources are revalidated with a304 Not Modifiedinstead of being re-downloaded in full.Why
Every REST request is currently issued unconditionally: the
ETagreturned by the GitHub API is never stored or replayed, so repeated tool calls that read the same resource (for example pull request reads, file and commit listings, and reviews) download the full response each time.Fixes #3025
What changed
ETagTransport, a bounded (LRU), concurrency-safehttp.RoundTripperthat caches theETagand body of cacheableGETresponses and sendsIf-None-Matchon the next identical request; a304 Not Modifiedis served from the cached body.ETagTransportintocreateGitHubClients()below the user-agent and auth layers, so cached entries are scoped by the request'sAuthorizationheader and never shared across tokens.304response so downstream rate-limit accounting stays correct.ETag/If-None-Matchheader constants.Every request is still sent to the server, so responses are always revalidated and never served stale. Per the GitHub REST API docs, a
304 Not Modifieddoes not count against the token's primary rate limit, so repeated reads conserve rate-limit budget and bandwidth while returning identical data.MCP impact
Security / limits
Authorizationheader, so cached bodies are never served across different tokens.Tool renaming
Lint & tests
go test ./pkg/http/transport/...(newetag_test.gocovers the 304-serves-cached-body flow, per-token scoping, rate-limit header pass-through, GET-only caching, and caller-supplied conditional headers).go build ./...andgo vet ./...pass.Docs