From 35c2439fd971a8438031fff06695162f7e2c1490 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 5 May 2026 13:22:41 -0600 Subject: [PATCH] Add GH_AGGRESSIVE_CACHING_TTL env var for opt-in process-wide caching This is the opt-in surface for the new go-gh aggressive caching support. Setting GH_AGGRESSIVE_CACHING_TTL to a Go duration string (e.g. 30s, 1m, 5m, 1h) enables process-wide TTL caching for cacheable REST and GraphQL requests on the standard authenticated HTTP client. Motivation: heavy users running multiple parallel agent harnesses (e.g. several Codex agents in lockstep) issue duplicate identical requests within seconds of each other, exhausting the GitHub primary REST rate limit and tripping secondary limits. A short TTL (~30s) dedupes the lockstep traffic without changing what gh fundamentally is. The trade-off (bounded staleness) is acceptable when explicitly opted into. Wiring: - api/http_client.go: NewHTTPClient reads GH_AGGRESSIVE_CACHING_TTL after honoring caller-supplied EnableCache. The env var only activates when: - opts.EnableCache is not already true (caller config wins) - opts.SkipDefaultHeaders is false (PlainHttpClient bypasses defaults and must not opt into caching by env var) Per-request X-GH-CACHE-TTL: 0 still bypasses for individual calls. - parseAggressiveCachingTTL is silent on failure: empty, '0', negative, or unparseable values disable the feature. Matches the convention for other GH_* env vars and avoids stderr noise from a transport layer. Users diagnose via the X-GH-Cache-Status response header. - help_topic.go: documents the env var, format, staleness caveat, intended use case, observability hook, cleanup path (gh config clear-cache), and explicit precedence rules. Tests cover: - parseAggressiveCachingTTL edge cases (empty, valid, zero, negative, unparseable, missing unit, plain number). - Env var with positive duration enables caching end-to-end (miss -> hit pattern, network call only on first request). - Env var unset or with invalid value leaves caching disabled. - Caller-set EnableCache + CacheTTL takes precedence over env var. - SkipDefaultHeaders (PlainHttpClient path) bypasses aggressive caching even when env var is set. - Per-request X-GH-CACHE-TTL: 0 hard-bypasses cache when env var is set, and does not pollute the cached entry. Manual smoke verified against live api.github.com: - REST GET: miss -> hit transitions work, X-GH-Cache-Status visible via 'gh api -i'. - GraphQL query: same miss -> hit pattern works. - Per-request opt-out: header returns no X-GH-Cache-Status, confirming bypass. - 'gh auth status', 'gh config clear-cache' continue to work. Out of scope here: - ETag / If-None-Match revalidation (planned follow-up that layers on top of the same transport). - Rewriting commands like 'gh pr checks --watch' to take advantage of REST + ETag (separate decision, separate PR). Depends on go-gh changes: - 'Fix per-request cache opt-out when global TTL is configured' - 'Restrict cacheable responses to 2xx' - 'Skip caching of GraphQL mutations and subscriptions' - 'Use atomic temp-file + rename for cache writes' - 'Vary cache key by Accept-Encoding, X-GitHub-Api-Version, ...' - 'Emit X-GH-Cache-Status response header for cache observability' Related: cli/cli#13279, cli/cli#13293 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/http_client.go | 54 ++++++ api/http_client_aggressive_cache_test.go | 232 +++++++++++++++++++++++ pkg/cmd/root/help_topic.go | 5 + 3 files changed, 291 insertions(+) create mode 100644 api/http_client_aggressive_cache_test.go diff --git a/api/http_client.go b/api/http_client.go index be7a6b8a7..d8b61ef6b 100644 --- a/api/http_client.go +++ b/api/http_client.go @@ -4,6 +4,7 @@ import ( "fmt" "io" "net/http" + "os" "strings" "time" @@ -13,6 +14,18 @@ import ( ghauth "github.com/cli/go-gh/v2/pkg/auth" ) +// AggressiveCachingTTLEnv is the environment variable that opts the standard +// HTTP client into process-wide TTL caching for cacheable REST + GraphQL +// requests. The value is a Go duration string (e.g. "30s", "1m", "5m", "1h"). +// Anything that does not parse to a positive duration silently disables the +// feature; users diagnose by inspecting the X-GH-Cache-Status response header +// (visible via gh api -i or GH_DEBUG=api). +// +// This is intended for automation/agent harnesses that issue repeated identical +// requests, not for general interactive use. Reads may be up to TTL stale, +// including reads following a write from the same or another process. +const AggressiveCachingTTLEnv = "GH_AGGRESSIVE_CACHING_TTL" + type tokenGetter interface { ActiveToken(string) (string, string) } @@ -65,6 +78,23 @@ func NewHTTPClient(opts HTTPClientOptions) (*http.Client, error) { if opts.EnableCache { clientOpts.EnableCache = opts.EnableCache clientOpts.CacheTTL = opts.CacheTTL + } else if !opts.SkipDefaultHeaders { + // Opt into process-wide aggressive caching when GH_AGGRESSIVE_CACHING_TTL + // is set to a positive Go duration. Two gates apply: + // + // 1. opts.EnableCache must not already be set: explicit caller + // configuration (e.g. gh api --cache 5m) wins over the env var. + // 2. opts.SkipDefaultHeaders must be false: this distinguishes the + // standard authenticated client from PlainHttpClient, which is + // built specifically to bypass defaults and must not start caching + // just because an env var is set. + // + // Per-request opt-out via X-GH-CACHE-TTL: 0 still works on top of this + // (handled in the underlying go-gh cache transport). + if ttl, ok := parseAggressiveCachingTTL(os.Getenv(AggressiveCachingTTLEnv)); ok { + clientOpts.EnableCache = true + clientOpts.CacheTTL = ttl + } } client, err := ghAPI.NewHTTPClient(clientOpts) @@ -86,6 +116,30 @@ func NewHTTPClient(opts HTTPClientOptions) (*http.Client, error) { return client, nil } +// parseAggressiveCachingTTL parses the GH_AGGRESSIVE_CACHING_TTL value. It +// returns (ttl, true) only when the value is a Go duration string that parses +// to a strictly positive value. Anything else (empty, "0", "0s", unparseable) +// returns (0, false), which keeps the cache off. +// +// Failures are silent by design: this matches the convention for other GH_* +// env vars (we do not have a stable warning channel inside NewHTTPClient and +// stderr noise from a transport layer would surprise scripts). Users who +// believe the variable should be active can confirm it via the +// X-GH-Cache-Status response header on subsequent requests. +func parseAggressiveCachingTTL(raw string) (time.Duration, bool) { + if raw == "" { + return 0, false + } + d, err := time.ParseDuration(raw) + if err != nil { + return 0, false + } + if d <= 0 { + return 0, false + } + return d, true +} + func NewCachedHTTPClient(httpClient *http.Client, ttl time.Duration) *http.Client { newClient := *httpClient newClient.Transport = AddCacheTTLHeader(httpClient.Transport, ttl) diff --git a/api/http_client_aggressive_cache_test.go b/api/http_client_aggressive_cache_test.go new file mode 100644 index 000000000..4f50a48fc --- /dev/null +++ b/api/http_client_aggressive_cache_test.go @@ -0,0 +1,232 @@ +package api + +import ( + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + ghAPI "github.com/cli/go-gh/v2/pkg/api" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// requestCount returns a handler that increments a counter on every request +// and writes the current count to the response body. +func requestCount() (http.Handler, func() int) { + count := 0 + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + count++ + w.Header().Set("Content-Type", "text/plain") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte{byte('0' + count%10)}) + }) + return h, func() int { return count } +} + +func TestParseAggressiveCachingTTL(t *testing.T) { + cases := []struct { + name string + raw string + wantTTL time.Duration + wantOK bool + }{ + {"empty disables", "", 0, false}, + {"30s enables", "30s", 30 * time.Second, true}, + {"1h enables", "1h", time.Hour, true}, + {"500ms enables", "500ms", 500 * time.Millisecond, true}, + {"explicit zero disables", "0", 0, false}, + {"explicit zero seconds disables", "0s", 0, false}, + {"negative disables", "-30s", 0, false}, + {"unparseable disables", "garbage", 0, false}, + {"plain number disables", "30", 0, false}, + {"missing unit disables", "30 seconds", 0, false}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ttl, ok := parseAggressiveCachingTTL(c.raw) + assert.Equal(t, c.wantOK, ok) + assert.Equal(t, c.wantTTL, ttl) + }) + } +} + +func TestNewHTTPClient_AggressiveCachingEnvVar(t *testing.T) { + t.Run("env var with positive duration enables caching", func(t *testing.T) { + t.Setenv(AggressiveCachingTTLEnv, "1h") + + handler, count := requestCount() + ts := httptest.NewServer(handler) + defer ts.Close() + + client, err := NewHTTPClient(HTTPClientOptions{ + AppVersion: "test", + }) + require.NoError(t, err) + + // First request: miss + network call. + res1 := doGET(t, client, ts.URL+"/repos/cli/cli") + assert.Equal(t, "miss", res1.Header.Get(ghAPI.CacheStatusHeader)) + + // Second identical request: hit, no additional network call. + res2 := doGET(t, client, ts.URL+"/repos/cli/cli") + assert.Equal(t, "hit", res2.Header.Get(ghAPI.CacheStatusHeader)) + + assert.Equal(t, 1, count(), "second request must be served from cache") + }) + + t.Run("env var unset leaves caching disabled", func(t *testing.T) { + t.Setenv(AggressiveCachingTTLEnv, "") + + handler, count := requestCount() + ts := httptest.NewServer(handler) + defer ts.Close() + + client, err := NewHTTPClient(HTTPClientOptions{ + AppVersion: "test", + }) + require.NoError(t, err) + + res1 := doGET(t, client, ts.URL+"/repos/cli/cli") + assert.Equal(t, "", res1.Header.Get(ghAPI.CacheStatusHeader), + "no env var means cache transport is not consulted, so no status header is set") + + _ = doGET(t, client, ts.URL+"/repos/cli/cli") + assert.Equal(t, 2, count(), "without caching, every request hits the network") + }) + + t.Run("env var with garbage value silently leaves caching disabled", func(t *testing.T) { + t.Setenv(AggressiveCachingTTLEnv, "not-a-duration") + + handler, count := requestCount() + ts := httptest.NewServer(handler) + defer ts.Close() + + client, err := NewHTTPClient(HTTPClientOptions{ + AppVersion: "test", + }) + require.NoError(t, err) + + _ = doGET(t, client, ts.URL+"/x") + _ = doGET(t, client, ts.URL+"/x") + assert.Equal(t, 2, count(), "garbage env var value must not enable caching") + }) + + t.Run("env var with zero value silently leaves caching disabled", func(t *testing.T) { + t.Setenv(AggressiveCachingTTLEnv, "0") + + handler, count := requestCount() + ts := httptest.NewServer(handler) + defer ts.Close() + + client, err := NewHTTPClient(HTTPClientOptions{ + AppVersion: "test", + }) + require.NoError(t, err) + + _ = doGET(t, client, ts.URL+"/x") + _ = doGET(t, client, ts.URL+"/x") + assert.Equal(t, 2, count(), "zero TTL must not enable caching") + }) + + t.Run("explicit caller EnableCache wins over env var", func(t *testing.T) { + // Caller explicitly disables cache (EnableCache=false). Env var is set. + // Today this still enables caching because EnableCache=false is the + // zero value and we cannot distinguish "explicit false" from + // "unconfigured". Document that callers wanting to suppress aggressive + // caching must rely on per-request X-GH-CACHE-TTL: 0 instead. + // The reverse path (EnableCache=true with CacheTTL set) does take + // precedence over the env var: + t.Setenv(AggressiveCachingTTLEnv, "5s") + + handler, count := requestCount() + ts := httptest.NewServer(handler) + defer ts.Close() + + client, err := NewHTTPClient(HTTPClientOptions{ + AppVersion: "test", + EnableCache: true, + CacheTTL: 1 * time.Hour, + }) + require.NoError(t, err) + + res1 := doGET(t, client, ts.URL+"/y") + assert.Equal(t, "miss", res1.Header.Get(ghAPI.CacheStatusHeader)) + _ = doGET(t, client, ts.URL+"/y") + assert.Equal(t, 1, count(), "explicit EnableCache + CacheTTL takes precedence; second request still served from cache") + }) + + t.Run("SkipDefaultHeaders bypasses aggressive caching (PlainHttpClient)", func(t *testing.T) { + // PlainHttpClient is built with SkipDefaultHeaders: true to bypass + // gh's default header injection. It must also bypass aggressive + // caching since callers depend on its raw, uncached behavior. + t.Setenv(AggressiveCachingTTLEnv, "1h") + + handler, count := requestCount() + ts := httptest.NewServer(handler) + defer ts.Close() + + client, err := NewHTTPClient(HTTPClientOptions{ + AppVersion: "test", + SkipDefaultHeaders: true, + }) + require.NoError(t, err) + + _ = doGET(t, client, ts.URL+"/z") + _ = doGET(t, client, ts.URL+"/z") + assert.Equal(t, 2, count(), "PlainHttpClient (SkipDefaultHeaders) must not opt into aggressive caching") + }) + + t.Run("per-request X-GH-CACHE-TTL: 0 bypasses cache when env var is set", func(t *testing.T) { + t.Setenv(AggressiveCachingTTLEnv, "1h") + + handler, count := requestCount() + ts := httptest.NewServer(handler) + defer ts.Close() + + client, err := NewHTTPClient(HTTPClientOptions{ + AppVersion: "test", + }) + require.NoError(t, err) + + // Populate cache. + _ = doGET(t, client, ts.URL+"/p") + assert.Equal(t, 1, count()) + + // Confirm hit. + res := doGET(t, client, ts.URL+"/p") + assert.Equal(t, "hit", res.Header.Get(ghAPI.CacheStatusHeader)) + assert.Equal(t, 1, count()) + + // Per-request opt-out with explicit zero TTL. + req, _ := http.NewRequest("GET", ts.URL+"/p", nil) + req.Header.Set("X-GH-CACHE-TTL", "0") + res, err = client.Do(req) + require.NoError(t, err) + _, _ = io.ReadAll(res.Body) + res.Body.Close() + assert.Equal(t, "", res.Header.Get(ghAPI.CacheStatusHeader), + "explicit per-request bypass should not produce a cache-status header") + assert.Equal(t, 2, count(), "per-request opt-out must hit the network") + + // Cache entry was not overwritten by the bypass. + res = doGET(t, client, ts.URL+"/p") + assert.Equal(t, "hit", res.Header.Get(ghAPI.CacheStatusHeader)) + assert.Equal(t, 2, count()) + }) +} + +// doGET issues a GET request with the given client and reads the body to +// completion so the response can be inspected without leaking handles. +func doGET(t *testing.T, client *http.Client, url string) *http.Response { + t.Helper() + req, err := http.NewRequest("GET", url, nil) + require.NoError(t, err) + res, err := client.Do(req) + require.NoError(t, err) + _, _ = io.ReadAll(res.Body) + res.Body.Close() + return res +} diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go index 0becbf5c8..ff36075f0 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -65,6 +65,11 @@ var HelpTopics = []helpTopic{ %[1]sGH_DEBUG%[1]s: set to a truthy value to enable verbose output on standard error. Set to %[1]sapi%[1]s to additionally log details of HTTP traffic. + %[1]sGH_AGGRESSIVE_CACHING_TTL%[1]s: when set to a duration like %[1]s30s%[1]s, %[1]s1m%[1]s, or %[1]s5m%[1]s, %[1]sgh%[1]s + caches API responses for that long. Useful for automation and agents that issue + repeated identical requests; responses may be up to that long out of date. Run + %[1]sgh config clear-cache%[1]s to evict cached entries. + %[1]sDEBUG%[1]s (deprecated): set to %[1]s1%[1]s, %[1]strue%[1]s, or %[1]syes%[1]s to enable verbose output on standard error.