mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
feat(coderd/externalauth/gitprovider): use conditional requests for GitHub JSON reads (#27628)
Fixes #27627. ## What The chat diff-status gitsync worker polls open pull requests on a fixed 10s interval and re-downloads the full JSON body every tick, even when nothing changed, because the GitHub client never sends `If-None-Match` / ETag. This adds a small, concurrency-safe, bounded in-memory ETag+body cache (`coderd/externalauth/gitprovider/conditional.go`) and wires it into `githubProvider.decodeJSON`. When an ETag is cached for a request, we send `If-None-Match`; on `304 Not Modified` we decode the cached body; on `200` we cache `{etag, body}` when an ETag is present and the body is under a size cap. ## Why `304` responses do not count against GitHub's primary rate limit, but full `200`s do. Today every unchanged poll burns quota that the same token also needs for interactive Git and API operations, so busy instances can hit rate-limit errors and stalls elsewhere. Unchanged PRs now revalidate for free with no behavior change; only genuine changes transfer a body. ## Details - Cache key = request URL + a hash of the token, so one token's response is never served under another; raw tokens are not retained. - Bounded by entry count (LRU eviction, default 2048) and per-body size (1 MiB) to cap memory. - Scope limited to the JSON reads through `decodeJSON`; the raw-diff path (`fetchDiff`, up to `MaxDiffSize`) is intentionally left out to avoid caching large bodies. ## Tests `TestConditionalRequestReuse` in `github_test.go` covers: - `NotModifiedReusesCachedBody` — a warm poll sends `If-None-Match` with the prior ETag and reuses the cached body on `304`, yielding the same result with exactly two upstream requests. - `DifferentTokenDoesNotShareCache` — a different token never sends another token's cached ETag. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79
This commit is contained in:
@@ -973,3 +973,92 @@ func TestEscapePathPreserveSlashes(t *testing.T) {
|
||||
got := gp.BuildBranchURL("owner", "repo", "feat/my thing")
|
||||
assert.Equal(t, "https://github.com/owner/repo/tree/feat/my%20thing", got)
|
||||
}
|
||||
|
||||
func TestConditionalRequestReuse(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
t.Run("NotModifiedReusesCachedBody", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const etag = `"abc123etag"`
|
||||
var srvURL string
|
||||
var requests int
|
||||
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
requests++
|
||||
// After the first response, the provider must revalidate
|
||||
// with the ETag we handed out.
|
||||
if inm := r.Header.Get("If-None-Match"); inm != "" {
|
||||
assert.Equal(t, etag, inm)
|
||||
w.Header().Set("ETag", etag)
|
||||
w.WriteHeader(http.StatusNotModified)
|
||||
return
|
||||
}
|
||||
htmlURL := fmt.Sprintf("https://%s/owner/repo/pull/42",
|
||||
strings.TrimPrefix(strings.TrimPrefix(srvURL, "http://"), "https://"))
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.Header().Set("ETag", etag)
|
||||
_, _ = w.Write([]byte(fmt.Sprintf(`[{"html_url":%q,"number":42}]`, htmlURL)))
|
||||
}))
|
||||
defer srv.Close()
|
||||
srvURL = srv.URL
|
||||
|
||||
gp, err := gitprovider.New("github", srv.URL+"/api/v3", srv.Client())
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, gp)
|
||||
|
||||
branch := gitprovider.BranchRef{Owner: "owner", Repo: "repo", Branch: "feat"}
|
||||
|
||||
// Cold fetch: 200 + full body, populates the cache.
|
||||
first, err := gp.ResolveBranchPullRequest(context.Background(), "test-token", branch)
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, first)
|
||||
assert.Equal(t, 42, first.Number)
|
||||
|
||||
// Warm fetch: server returns 304, provider reuses the cached
|
||||
// body and yields the same result.
|
||||
second, err := gp.ResolveBranchPullRequest(context.Background(), "test-token", branch)
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, second)
|
||||
assert.Equal(t, 42, second.Number)
|
||||
assert.Equal(t, first.Owner, second.Owner)
|
||||
assert.Equal(t, first.Repo, second.Repo)
|
||||
|
||||
assert.Equal(t, 2, requests, "expected exactly two upstream requests")
|
||||
})
|
||||
|
||||
t.Run("DifferentTokenDoesNotShareCache", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
var srvURL string
|
||||
var conditionalRequests int
|
||||
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.Header.Get("If-None-Match") != "" {
|
||||
conditionalRequests++
|
||||
}
|
||||
htmlURL := fmt.Sprintf("https://%s/owner/repo/pull/7",
|
||||
strings.TrimPrefix(strings.TrimPrefix(srvURL, "http://"), "https://"))
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.Header().Set("ETag", `"tok-etag"`)
|
||||
_, _ = w.Write([]byte(fmt.Sprintf(`[{"html_url":%q,"number":7}]`, htmlURL)))
|
||||
}))
|
||||
defer srv.Close()
|
||||
srvURL = srv.URL
|
||||
|
||||
gp, err := gitprovider.New("github", srv.URL+"/api/v3", srv.Client())
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, gp)
|
||||
|
||||
branch := gitprovider.BranchRef{Owner: "owner", Repo: "repo", Branch: "feat"}
|
||||
|
||||
_, err = gp.ResolveBranchPullRequest(context.Background(), "token-a", branch)
|
||||
require.NoError(t, err)
|
||||
// A different token must not reuse token-a's cached ETag.
|
||||
_, err = gp.ResolveBranchPullRequest(context.Background(), "token-b", branch)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.Equal(t, 0, conditionalRequests,
|
||||
"a different token must not send If-None-Match from another token's cache")
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user