fix: follow-up fixes for conditional VCS requests (#27711)

Follow-ups from #27627 

- Memoizes `Config.Git()` with a mutex so the provider's ETag response
cache survives across calls. Only successful construction is cached;
errors are retried.
- Moves the HTTP client onto `Config.HTTPClient`, wired through
`ConvertConfig`, so `Git()` no longer takes a per-call argument that
would be silently ignored after memoization.
- `newGitHub` and `newGitLab` now return `(Provider, error)`,
eliminating the typed-nil-interface class in `gitprovider.New` rather
than the single instance.
- Gates the 304 branch on a `haveCached` flag instead of a nil body
check.
- Only caches bodies that decode successfully, preventing poisoned
entries.
- Keys the response cache on the full token digest rather than a
truncated prefix.
- Tests added: `TestConfigGitMemoizesProvider`,
`TestConfigGitRetriesOnConstructorError`,
`TestGitLabConstructorErrorReturnsNilInterface`,
`TestResponseCacheStore`,
`TestConditionalRequestReuse/MalformedResponseNotCached`;
`TestConvertYAML/CustomScopesAndEndpoint` now asserts
`Config.HTTPClient` wiring.

Follow-ups tracked in #28139, #28140, #28141, #28142.

> 🤖 Generated by Coder Agents on behalf of @johnstcn.

coderd/externalauth/gitprovider/conditional.go

@@ -1,6 +1,7 @@
 package gitprovider
 
 import (
+	"bytes"
 	"container/list"
 	"crypto/sha256"
 	"encoding/hex"
@@ -13,7 +14,10 @@ import (
 const (
 	// defaultResponseCacheEntries is the maximum number of cached
 	// responses retained. Once exceeded, the least-recently-used
-	// entry is evicted.
+	// entry is evicted. Sized above the gitsync worker's steady-state
+	// working set: defaultBatchSize (50) rows re-acquired every
+	// DiffStatusTTL (120s) over defaultInterval (10s) ticks, at 2-3
+	// cache keys per row. See coderd/x/gitsync.
 	defaultResponseCacheEntries = 2048
 
 	// maxCachedBodyBytes is the largest response body that will be
@@ -60,7 +64,8 @@ func newResponseCache(maxSize int) *responseCache {
 }
 
 // load returns the cached ETag and body for key, if present, and
-// marks the entry as most-recently-used.
+// marks the entry as most-recently-used. The returned body aliases
+// the cache's copy and must not be mutated.
 func (c *responseCache) load(key string) (etag string, body []byte, ok bool) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
@@ -82,6 +87,7 @@ func (c *responseCache) store(key, etag string, body []byte) {
 		return
 	}
 
+	stored := bytes.Clone(body)
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
@@ -89,15 +95,14 @@ func (c *responseCache) store(key, etag string, body []byte) {
 		c.ll.MoveToFront(elem)
 		cr := elem.Value.(*cachedResponse)
 		cr.etag = etag
-		cr.body = body
+		// Replace the body slice entirely; never write into the
+		// existing slice in place. A concurrent reader may hold a
+		// reference to the old slice while json.Unmarshal is reading
+		// it.
+		cr.body = stored
 		return
 	}
 
-	// Copy the body so we never retain a slice that the caller may
-	// later reuse or mutate.
-	stored := make([]byte, len(body))
-	copy(stored, body)
-
 	elem := c.ll.PushFront(&cachedResponse{key: key, etag: etag, body: stored})
 	c.entries[key] = elem
 
@@ -123,5 +128,5 @@ func (c *responseCache) evictOldest() {
 // another, without keeping raw credentials in memory.
 func responseCacheKey(requestURL, token string) string {
 	sum := sha256.Sum256([]byte(token))
-	return requestURL + "\x00" + hex.EncodeToString(sum[:8])
+	return requestURL + "\x00" + hex.EncodeToString(sum[:])
 }

coderd/externalauth/gitprovider/github.go

@@ -37,7 +37,7 @@ type githubProvider struct {
 	repositorySSHPathPattern *regexp.Regexp
 }
 
-func newGitHub(apiBaseURL string, httpClient *http.Client, clock quartz.Clock) *githubProvider {
+func newGitHub(apiBaseURL string, httpClient *http.Client, clock quartz.Clock) (Provider, error) {
 	if apiBaseURL == "" {
 		apiBaseURL = defaultGitHubAPIBaseURL
 	}
@@ -72,7 +72,7 @@ func newGitHub(apiBaseURL string, httpClient *http.Client, clock quartz.Clock) *
 		repositorySSHPathPattern: regexp.MustCompile(
 			`^(?:ssh://)?git@` + escapedHost + `[:/]([A-Za-z0-9_.-]+)/([A-Za-z0-9_.-]+?)(?:\.git)?/?$`,
 		),
-	}
+	}, nil
 }
 
 // deriveWebBaseURL converts a GitHub API base URL to the
@@ -412,12 +412,13 @@ func (g *githubProvider) decodeJSON(
 	// changed, which is cheaper than a full body and does not count
 	// against the primary REST rate limit.
 	cacheKey := responseCacheKey(requestURL, token)
-	var cachedBody []byte
-	if g.cache != nil {
-		if etag, body, ok := g.cache.load(cacheKey); ok {
-			req.Header.Set("If-None-Match", etag)
-			cachedBody = body
-		}
+	var (
+		cachedBody []byte
+		haveCached bool
+	)
+	if etag, body, ok := g.cache.load(cacheKey); ok {
+		req.Header.Set("If-None-Match", etag)
+		cachedBody, haveCached = body, true
 	}
 
 	resp, err := g.httpClient.Do(req)
@@ -427,7 +428,7 @@ func (g *githubProvider) decodeJSON(
 	defer resp.Body.Close()
 
 	// Nothing changed since the cached response: reuse the stored body.
-	if resp.StatusCode == http.StatusNotModified && cachedBody != nil {
+	if resp.StatusCode == http.StatusNotModified && haveCached {
 		if err := json.Unmarshal(cachedBody, dest); err != nil {
 			return xerrors.Errorf("decode cached github response: %w", err)
 		}
@@ -457,14 +458,13 @@ func (g *githubProvider) decodeJSON(
 		return xerrors.Errorf("read github response: %w", err)
 	}
 
-	// Cache the validator so the next poll can be made conditional.
-	if g.cache != nil {
-		g.cache.store(cacheKey, resp.Header.Get("ETag"), body)
-	}
-
 	if err := json.Unmarshal(body, dest); err != nil {
 		return xerrors.Errorf("decode github response: %w", err)
 	}
+
+	// Only cache bodies we could successfully decode, so a malformed
+	// response does not poison the cache.
+	g.cache.store(cacheKey, resp.Header.Get("ETag"), body)
 	return nil
 }
 

coderd/externalauth/gitprovider/github_test.go

@@ -1061,4 +1061,40 @@ func TestConditionalRequestReuse(t *testing.T) {
 		assert.Equal(t, 0, conditionalRequests,
 			"a different token must not send If-None-Match from another token's cache")
 	})
+
+	t.Run("MalformedResponseNotCached", func(t *testing.T) {
+		t.Parallel()
+
+		const etag = `"poison-etag"`
+		var conditionalRequests int
+
+		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if r.Header.Get("If-None-Match") != "" {
+				conditionalRequests++
+				w.Header().Set("ETag", etag)
+				w.WriteHeader(http.StatusNotModified)
+				return
+			}
+			w.Header().Set("Content-Type", "application/json")
+			w.Header().Set("ETag", etag)
+			// A body that cannot be decoded as a pull request list.
+			_, _ = w.Write([]byte(`<html>proxy error</html>`))
+		}))
+		defer srv.Close()
+
+		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(), "test-token", branch)
+		require.Error(t, err)
+
+		_, err = gp.ResolveBranchPullRequest(context.Background(), "test-token", branch)
+		require.Error(t, err)
+
+		assert.Equal(t, 0, conditionalRequests,
+			"a body that failed to decode must not be cached with its ETag")
+	})
 }

coderd/externalauth/gitprovider/gitlab.go

@@ -25,7 +25,7 @@ type gitlabProvider struct {
 	clock      quartz.Clock
 }
 
-func newGitLab(baseURL string, httpClient *http.Client, clock quartz.Clock) (*gitlabProvider, error) {
+func newGitLab(baseURL string, httpClient *http.Client, clock quartz.Clock) (Provider, error) {
 	if baseURL == "" {
 		baseURL = "https://gitlab.com"
 	}

coderd/externalauth/gitprovider/gitlab_test.go

@@ -262,6 +262,18 @@ func TestGitLabResolveBranchPullRequest(t *testing.T) {
 	})
 }
 
+func TestGitLabConstructorErrorReturnsNilInterface(t *testing.T) {
+	t.Parallel()
+
+	// A construction error must return a true nil Provider, not a
+	// non-nil interface boxing a nil *gitlabProvider. The gp == nil
+	// check is what callers write, and require.Nil alone would pass
+	// for a non-nil interface holding a nil pointer.
+	gp, err := gitprovider.New("gitlab", "://invalid/", nil)
+	require.Error(t, err)
+	require.True(t, gp == nil, "provider must be nil when construction fails, got %T", gp)
+}
+
 func TestGitLabRateLimit(t *testing.T) {
 	t.Parallel()
 

coderd/externalauth/gitprovider/gitprovider.go

@@ -192,7 +192,7 @@ func New(providerType string, apiBaseURL string, httpClient *http.Client, opts .
 
 	switch providerType {
 	case "github":
-		return newGitHub(apiBaseURL, httpClient, o.clock), nil
+		return newGitHub(apiBaseURL, httpClient, o.clock)
 	case "gitlab":
 		return newGitLab(apiBaseURL, httpClient, o.clock)
 	default:
@@ -207,9 +207,6 @@ func New(providerType string, apiBaseURL string, httpClient *http.Client, opts .
 // resetHeader (unix timestamp). Returns zero if no recognizable header
 // is present.
 func parseRetryAfter(h http.Header, resetHeader string, clk quartz.Clock) time.Duration {
-	if clk == nil {
-		clk = quartz.NewReal()
-	}
 	// Retry-After header: seconds until retry.
 	if ra := h.Get("Retry-After"); ra != "" {
 		if secs, err := strconv.Atoi(ra); err == nil {
@@ -236,9 +233,6 @@ func checkRateLimitError(resp *http.Response, clk quartz.Clock, resetHeader stri
 	if resp.StatusCode != http.StatusForbidden && resp.StatusCode != http.StatusTooManyRequests {
 		return nil
 	}
-	if clk == nil {
-		clk = quartz.NewReal()
-	}
 	retryAfter := parseRetryAfter(resp.Header, resetHeader, clk)
 	if retryAfter <= 0 {
 		return nil

coderd/externalauth/gitprovider/gitprovider_internal_test.go

@@ -7,6 +7,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/coder/quartz"
 )
@@ -112,13 +113,85 @@ func TestParseRetryAfter(t *testing.T) {
 		d := parseRetryAfter(h, "X-Ratelimit-Reset", clk)
 		assert.Equal(t, 60*time.Second, d)
 	})
+}
 
-	t.Run("NilClock", func(t *testing.T) {
+func TestResponseCacheStore(t *testing.T) {
+	t.Parallel()
+
+	// Stores the same key twice using a buffer the caller mutates
+	// between stores, and verifies load returns the bodies passed at
+	// store time rather than the mutated buffer.
+	t.Run("UpdateReplacesBody", func(t *testing.T) {
 		t.Parallel()
-		h := http.Header{}
-		h.Set("Retry-After", "1")
-		d := parseRetryAfter(h, "X-Ratelimit-Reset", nil)
-		assert.Equal(t, time.Second, d)
+
+		cache := newResponseCache(4)
+		const key = "k"
+
+		buf := []byte(`{"v":1}`)
+		cache.store(key, `"etag-1"`, buf)
+		// Mutate the caller's buffer: the cache must hold its own copy.
+		for i := range buf {
+			buf[i] = 'X'
+		}
+
+		etag, body, ok := cache.load(key)
+		require.True(t, ok)
+		assert.Equal(t, `"etag-1"`, etag)
+		assert.Equal(t, `{"v":1}`, string(body))
+
+		// Reuse the same buffer for a second store of the same key.
+		buf = append(buf[:0], `{"v":2}`...)
+		cache.store(key, `"etag-2"`, buf)
+		for i := range buf {
+			buf[i] = 'Y'
+		}
+
+		etag, body, ok = cache.load(key)
+		require.True(t, ok)
+		assert.Equal(t, `"etag-2"`, etag)
+		assert.Equal(t, `{"v":2}`, string(body))
+	})
+
+	// Fills the cache past maxSize and verifies the
+	// least-recently-used entry is evicted, not merely the
+	// oldest-inserted one.
+	t.Run("EvictsLeastRecentlyUsed", func(t *testing.T) {
+		t.Parallel()
+
+		cache := newResponseCache(2)
+		cache.store("a", `"etag-a"`, []byte(`{"k":"a"}`))
+		cache.store("b", `"etag-b"`, []byte(`{"k":"b"}`))
+		// Access "a" so "b" becomes the least-recently-used entry.
+		_, _, ok := cache.load("a")
+		require.True(t, ok)
+		// This third store exceeds maxSize and must evict "b",
+		// not the older "a".
+		cache.store("c", `"etag-c"`, []byte(`{"k":"c"}`))
+
+		_, _, ok = cache.load("b")
+		assert.False(t, ok, "least-recently-used entry must be evicted")
+
+		etag, body, ok := cache.load("a")
+		require.True(t, ok, "recently-accessed entry must survive eviction")
+		assert.Equal(t, `"etag-a"`, etag)
+		assert.Equal(t, `{"k":"a"}`, string(body))
+
+		etag, body, ok = cache.load("c")
+		require.True(t, ok)
+		assert.Equal(t, `"etag-c"`, etag)
+		assert.Equal(t, `{"k":"c"}`, string(body))
+	})
+
+	// Bodies larger than maxCachedBodyBytes are not stored, so a
+	// single oversized response cannot unbound the cache.
+	t.Run("OversizedBodyNotStored", func(t *testing.T) {
+		t.Parallel()
+
+		cache := newResponseCache(4)
+		cache.store("big", `"etag-big"`, make([]byte, maxCachedBodyBytes+1))
+
+		_, _, ok := cache.load("big")
+		assert.False(t, ok, "bodies exceeding maxCachedBodyBytes must not be cached")
 	})
 }