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>
This commit is contained in:
Kynan Ware
parent
7c439196c1
commit
35c2439fd9
3 changed files with 291 additions and 0 deletions
|
|
@ -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)
|
||||
|
|
|
|||
232
api/http_client_aggressive_cache_test.go
Normal file
232
api/http_client_aggressive_cache_test.go
Normal file
|
|
@ -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
|
||||
}
|
||||
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue