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.