Close two gaps in the git cache

docs/configuration.md

@@ -899,8 +899,14 @@ commits, the request waits for their fetch, so a clone right after a push gets
 them; if the fetch fails or stalls, the cached copy is served. A
 `stale_max_age` of `0` never serves a copy the upstream has not just
 confirmed. A repository the upstream reports missing (404), refuses (401, 403)
-or redirects is not served from the cache. Redirects are never followed, so
-point the upstream at a repository's current location.
+or redirects is not served from the cache, also to requests that skip the ref
+listing and go straight to `git-upload-pack`, until a later check succeeds.
+Redirects are never followed, so point the upstream at a repository's current
+location. A `git-upload-pack` request does not check the upstream itself
+unless the upstream refused the repository at the last check or the copy was
+last checked more than a minute past `stale_max_age` ago, so the requests of a
+clone that follow its ref listing check nothing again; otherwise the
+repository is checked first, as for a ref listing.
 
 The proxy runs `git` 2.31 or newer, which must be installed (the Docker image includes it).
 The `git` processes never reach the network themselves: their fetches go

internal/gitcache/budget.go

@@ -2,6 +2,7 @@ package gitcache
 
 import (
 	"context"
+	"errors"
 	"sync/atomic"
 	"time"
 )
@@ -29,8 +30,15 @@ type reservation struct {
 	// cut, guarded by c.mu, marks a fetch stopped because the cache could
 	// not make room for it.
 	cut bool
+	// ended, guarded by c.mu, marks a reservation given back. A read
+	// relayed after that, by a git helper the fetch left behind, reserves
+	// nothing.
+	ended bool
 }
 
+// errReservationEnded is returned by cover once the fetch has ended.
+var errReservationEnded = errors.New("fetch has ended")
+
 func (c *Cache) newReservation() *reservation {
 	return &reservation{c: c}
 }
@@ -48,6 +56,10 @@ func (r *reservation) cover(ctx context.Context, relayed int64) error {
 	var timeout <-chan time.Time
 	for {
 		c.mu.Lock()
+		if r.ended {
+			c.mu.Unlock()
+			return errReservationEnded
+		}
 		if r.cut {
 			c.mu.Unlock()
 			return ErrCacheFull
@@ -113,6 +125,7 @@ func (r *reservation) release() {
 
 func (r *reservation) releaseLocked() {
 	c := r.c
+	r.ended = true
 	n := r.reserved.Swap(0)
 	if r.cut {
 		r.cut = false

internal/gitcache/budget_test.go

@@ -1,10 +1,15 @@
 package gitcache
 
 import (
+	"bytes"
 	"context"
 	"crypto/rand"
 	"errors"
+	"io"
 	"net/http"
+	"net/http/httptest"
+	"net/http/httputil"
+	"net/url"
 	"os"
 	"path/filepath"
 	"sync"
@@ -216,3 +221,91 @@ func TestConcurrentClonesStayWithinMaxSize(t *testing.T) {
 		t.Errorf("left %d entries in the temporary directory", len(entries))
 	}
 }
+
+// TestCoverAfterReleaseReservesNothing has a relayed read arrive after the
+// fetch gave back its reservation, as one from an orphaned git helper would.
+func TestCoverAfterReleaseReservesNothing(t *testing.T) {
+	tc := newTestCache(t, "", func(o *Options) { o.MaxSize = 100 })
+	res := tc.newReservation()
+	if err := res.cover(t.Context(), 10); err != nil {
+		t.Fatal(err)
+	}
+	res.release()
+	ctx, cancel := context.WithCancel(t.Context())
+	cancel()
+	_ = res.cover(ctx, 20)
+	if reserved, _ := tc.budget(); reserved != 0 {
+		t.Errorf("reserved %d after release", reserved)
+	}
+}
+
+// TestCancelledFetchStopsRelaying cancels a fetch while the upstream is
+// midway through sending the pack: the upstream connection is closed, and
+// nothing stays reserved when the rest arrives.
+func TestCancelledFetchStopsRelaying(t *testing.T) {
+	upstream := newFakeUpstream(t)
+	upstream.newRepo("repo")
+	upstreamURL, err := url.Parse(upstream.server.URL)
+	if err != nil {
+		t.Fatal(err)
+	}
+	arrived := make(chan struct{})
+	release := make(chan struct{})
+	result := make(chan string, 1)
+	forward := httputil.NewSingleHostReverseProxy(upstreamURL)
+	front := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		body, _ := io.ReadAll(r.Body)
+		r.Body = io.NopCloser(bytes.NewReader(body))
+		if r.Method != http.MethodPost || !bytes.Contains(body, []byte("command=fetch")) {
+			forward.ServeHTTP(w, r)
+			return
+		}
+		rec := httptest.NewRecorder()
+		forward.ServeHTTP(rec, r.Clone(context.Background()))
+		pack := rec.Body.Bytes()
+		w.Header().Set("Content-Type", rec.Header().Get("Content-Type"))
+		_, _ = w.Write(pack[:len(pack)/2])
+		w.(http.Flusher).Flush()
+		close(arrived)
+		select {
+		case <-r.Context().Done():
+			result <- "closed"
+		case <-release:
+			_, _ = w.Write(pack[len(pack)/2:])
+			w.(http.Flusher).Flush()
+			result <- "sent"
+		}
+	}))
+	defer front.Close()
+	defer close(release)
+	tc := newTestCache(t, front.URL, func(o *Options) { o.MaxSize = 1 << 30 })
+
+	dir := t.TempDir()
+	if _, err := tc.run(t.Context(), dir, "init", "--quiet", "--bare"); err != nil {
+		t.Fatal(err)
+	}
+	ctx, cancel := context.WithCancel(t.Context())
+	res := tc.newReservation()
+	done := make(chan error, 1)
+	go func() { done <- tc.fetch(ctx, dir, front.URL+"/repo", 0, res, nil, branchesAndTags...) }()
+	<-arrived
+	waitFor(t, func() bool { reserved, _ := tc.budget(); return reserved > 0 })
+	cancel()
+	if err := <-done; err == nil {
+		t.Error("cancelled fetch succeeded")
+	}
+	res.release()
+
+	select {
+	case <-result:
+	case <-time.After(10 * time.Second):
+		t.Error("upstream connection still open after the fetch was cancelled")
+		release <- struct{}{}
+		<-result
+	}
+	// Give a relay still running the time to read what the upstream sent.
+	time.Sleep(200 * time.Millisecond)
+	if reserved, _ := tc.budget(); reserved != 0 {
+		t.Errorf("reserved %d after the fetch ended", reserved)
+	}
+}

internal/gitcache/cache.go

@@ -121,14 +121,17 @@ type Cache struct {
 	failedWants map[string]time.Time
 }
 
-// repo is a cached repository, or one being cloned. users, refreshing and
-// the map entry itself are guarded by Cache.mu.
+// repo is a cached repository, or one being cloned. users, refreshing,
+// refused and the map entry itself are guarded by Cache.mu.
 type repo struct {
 	key string
 	dir string
 
 	users      int
 	refreshing *refreshCall
+	// refused is the error of the last refresh when the upstream reported
+	// the repository missing or refused it, until a ref check succeeds.
+	refused error
 	// writes serializes git fetches into the repository.
 	writes chan struct{}
 

internal/gitcache/cache_test.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"maps"
+	"math"
 	"net/http"
 	"net/http/httptest"
 	"net/http/httputil"
@@ -786,3 +787,200 @@ func TestCloseTwice(t *testing.T) {
 		t.Errorf("closing twice: %v", err)
 	}
 }
+
+// lsRefs sends a protocol v2 ls-refs request for "repo" straight to
+// git-upload-pack, as a client may without asking for info/refs first, and
+// returns the status.
+func (tc *testCache) lsRefs(t *testing.T) int {
+	t.Helper()
+	body := pktLine("command=ls-refs\n") + pktDelim + pktLine("peel\n") + pktFlush
+	req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, tc.url("repo")+"/git-upload-pack", strings.NewReader(body))
+	if err != nil {
+		t.Fatal(err)
+	}
+	req.Header.Set("Content-Type", requestType)
+	req.Header.Set("Git-Protocol", "version=2")
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_ = resp.Body.Close()
+	return resp.StatusCode
+}
+
+// TestUploadPackChecksCopyPastStaleMaxAge sends upload-pack requests without
+// info/refs: a copy not checked within stale_max_age is checked first.
+func TestUploadPackChecksCopyPastStaleMaxAge(t *testing.T) {
+	upstream := newFakeUpstream(t)
+	upstream.newRepo("repo")
+	tc := newTestCache(t, upstream.server.URL, nil)
+	tc.clone(t, "repo")
+	if status := tc.lsRefs(t); status != http.StatusOK {
+		t.Fatalf("ls-refs on a fresh copy: status %d", status)
+	}
+
+	waitIdle(t, tc.Cache)
+	rp := tc.acquire("up/repo.git")
+	rp.lastChecked.Store(time.Now().Add(-2 * time.Hour).UnixNano())
+	tc.release(rp)
+	upstream.status.Store(http.StatusNotFound)
+	if status := tc.lsRefs(t); status == http.StatusOK {
+		t.Errorf("served a copy older than stale_max_age to ls-refs")
+	}
+}
+
+// TestUploadPackRefusedAfterUpstreamRefusal has the upstream refuse a
+// refresh: upload-pack requests are refused too until a check succeeds.
+func TestUploadPackRefusedAfterUpstreamRefusal(t *testing.T) {
+	upstream := newFakeUpstream(t)
+	upstream.newRepo("repo")
+	tc := newTestCache(t, upstream.server.URL, nil)
+	tc.clone(t, "repo")
+
+	upstream.status.Store(http.StatusForbidden)
+	if out, err := tryGit(t, "", "ls-remote", tc.url("repo")); err == nil {
+		t.Fatalf("served the cached copy after 403:\n%s", out)
+	}
+	if status := tc.lsRefs(t); status == http.StatusOK {
+		t.Errorf("ls-refs served after the upstream refused the repository")
+	}
+
+	upstream.status.Store(0)
+	tc.clone(t, "repo")
+	if status := tc.lsRefs(t); status != http.StatusOK {
+		t.Errorf("ls-refs after a successful check: status %d", status)
+	}
+}
+
+// TestCloneChecksUpstreamOnce clones with a stale_max_age of 0: the
+// upload-pack requests after info/refs do not check the upstream again.
+func TestCloneChecksUpstreamOnce(t *testing.T) {
+	upstream := newFakeUpstream(t)
+	upstream.newRepo("repo")
+	tc := newTestCache(t, upstream.server.URL, func(o *Options) { o.StaleMaxAge = 0 })
+	tc.clone(t, "repo")
+	waitIdle(t, tc.Cache)
+
+	checks := func() int32 { return upstream.requests.Load() - upstream.gitRequests.Load() }
+	before := checks()
+	resp, err := http.Get(tc.url("repo") + "/info/refs?service=git-upload-pack")
+	if err != nil {
+		t.Fatal(err)
+	}
+	_ = resp.Body.Close()
+	perCheck := checks() - before
+
+	before = checks()
+	tc.clone(t, "repo")
+	if got := checks() - before; got != perCheck {
+		t.Errorf("clone made %d upstream check requests, want %d", got, perCheck)
+	}
+}
+
+// TestInfoRefsRefusedWhileRefusalStands checks that once the upstream
+// refused a repository, a later check that runs past RefreshWait, which
+// would otherwise serve the cached copy, doesn't advertise its refs.
+func TestInfoRefsRefusedWhileRefusalStands(t *testing.T) {
+	upstream := newFakeUpstream(t)
+	upstream.newRepo("repo")
+	tc := newTestCache(t, upstream.server.URL, func(o *Options) { o.RefreshWait = 100 * time.Millisecond })
+	tc.clone(t, "repo")
+
+	upstream.status.Store(http.StatusForbidden)
+	if out, err := tryGit(t, "", "ls-remote", tc.url("repo")); err == nil {
+		t.Fatalf("served the cached copy after 403:\n%s", out)
+	}
+	upstream.status.Store(0)
+	release := upstream.holdRequests()
+	defer release()
+	// Protocol v0 advertises the refs in the info/refs response itself.
+	if out, err := tryGit(t, "", "-c", "protocol.version=0", "ls-remote", tc.url("repo")); err == nil {
+		t.Errorf("advertised refs while the refusal stands:\n%s", out)
+	}
+}
+
+// refuse has the upstream refuse "repo" with a 403 and checks that the
+// cached copy is no longer served.
+func (tc *testCache) refuse(t *testing.T, upstream *fakeUpstream) {
+	t.Helper()
+	upstream.status.Store(http.StatusForbidden)
+	if out, err := tryGit(t, "", "ls-remote", tc.url("repo")); err == nil {
+		t.Fatalf("served the cached copy after 403:\n%s", out)
+	}
+	upstream.status.Store(0)
+}
+
+// TestUploadPackClearsRefusal has a client that only sends upload-pack
+// requests: once the upstream allows the repository again, it is served.
+func TestUploadPackClearsRefusal(t *testing.T) {
+	upstream := newFakeUpstream(t)
+	upstream.newRepo("repo")
+	tc := newTestCache(t, upstream.server.URL, nil)
+	tc.clone(t, "repo")
+	tc.refuse(t, upstream)
+
+	if status := tc.lsRefs(t); status != http.StatusOK {
+		t.Errorf("ls-refs after the upstream allowed the repository again: status %d", status)
+	}
+}
+
+// TestRefCheckClearsRefusal has the upstream list the refs again after a
+// refusal but fail the fetch that follows: the check alone clears the
+// refusal, and the cached copy is served.
+func TestRefCheckClearsRefusal(t *testing.T) {
+	upstream := newFakeUpstream(t)
+	work := upstream.newRepo("repo")
+	tc := newTestCache(t, upstream.server.URL, nil)
+	tc.clone(t, "repo")
+	tc.refuse(t, upstream)
+
+	upstream.commit(work, "second")
+	runGit(t, work, "push", "--quiet", "origin", "main")
+	upstream.fetchStatus.Store(http.StatusInternalServerError)
+	if out, err := tryGit(t, "", "ls-remote", tc.url("repo")); err != nil {
+		t.Errorf("cached copy refused after the upstream listed its refs: %v\n%s", err, out)
+	}
+	if status := tc.lsRefs(t); status != http.StatusOK {
+		t.Errorf("ls-refs after the upstream listed its refs: status %d", status)
+	}
+}
+
+// TestHugeStaleMaxAgeUploadPackChecksNothing sets stale_max_age near the
+// largest duration: upload-pack requests on a fresh copy do not check the
+// upstream.
+func TestHugeStaleMaxAgeUploadPackChecksNothing(t *testing.T) {
+	upstream := newFakeUpstream(t)
+	upstream.newRepo("repo")
+	tc := newTestCache(t, upstream.server.URL, func(o *Options) { o.StaleMaxAge = math.MaxInt64 - time.Second })
+	tc.clone(t, "repo")
+	waitIdle(t, tc.Cache)
+
+	before := upstream.requests.Load()
+	if status := tc.lsRefs(t); status != http.StatusOK {
+		t.Fatalf("ls-refs: status %d", status)
+	}
+	waitIdle(t, tc.Cache)
+	if got := upstream.requests.Load() - before; got != 0 {
+		t.Errorf("ls-refs made %d upstream requests, want 0", got)
+	}
+}
+
+// TestFetchRedirectRefusesRepository has the upstream list the refs but
+// redirect the fetch that follows: the repository is refused, as when the
+// ref listing is redirected.
+func TestFetchRedirectRefusesRepository(t *testing.T) {
+	upstream := newFakeUpstream(t)
+	work := upstream.newRepo("repo")
+	tc := newTestCache(t, upstream.server.URL, nil)
+	tc.clone(t, "repo")
+
+	upstream.commit(work, "second")
+	runGit(t, work, "push", "--quiet", "origin", "main")
+	upstream.fetchStatus.Store(http.StatusFound)
+	if out, err := tryGit(t, "", "ls-remote", tc.url("repo")); err == nil {
+		t.Errorf("served the cached copy after the fetch was redirected:\n%s", out)
+	}
+	if status := tc.lsRefs(t); status == http.StatusOK {
+		t.Errorf("ls-refs served after the fetch was redirected")
+	}
+}