fix(git): fail closed on ref discovery checks

internal/gitclone/manager.go

@@ -705,8 +705,7 @@ func (r *Repository) EnsureRefsUpToDate(ctx context.Context) (needsFetch bool, e
 		r.mu.Unlock()
 		return false, nil
 	}
-	r.lastRefCheck = time.Now()
-	r.refCheckValid = true
+	r.refCheckValid = false
 	r.mu.Unlock()
 
 	localRefs, err := r.GetLocalRefs(ctx)
@@ -719,9 +718,6 @@ func (r *Repository) EnsureRefsUpToDate(ctx context.Context) (needsFetch bool, e
 
 	upstreamRefs, err := r.GetUpstreamRefs(lsCtx)
 	if err != nil {
-		r.mu.Lock()
-		r.refCheckValid = false
-		r.mu.Unlock()
 		return false, errors.Wrap(err, "get upstream refs")
 	}
 
@@ -741,6 +737,10 @@ func (r *Repository) EnsureRefsUpToDate(ctx context.Context) (needsFetch bool, e
 		}
 	}
 
+	r.mu.Lock()
+	r.lastRefCheck = time.Now()
+	r.refCheckValid = true
+	r.mu.Unlock()
 	return false, nil
 }
 

internal/gitclone/manager_test.go

@@ -725,6 +725,172 @@ func TestRepository_EnsureRefs(t *testing.T) {
 	assert.Equal(t, newSHA, resolved[head])
 }
 
+func TestRepositoryEnsureRefsUpToDateDoesNotCacheLocalError(t *testing.T) {
+	config := testRepoConfig()
+	config.RefCheckInterval = time.Hour
+	repo := &Repository{
+		config:      config,
+		path:        filepath.Join(t.TempDir(), "missing"),
+		upstreamURL: filepath.Join(t.TempDir(), "upstream.git"),
+	}
+
+	_, err := repo.EnsureRefsUpToDate(t.Context())
+
+	assert.Error(t, err)
+	repo.mu.Lock()
+	defer repo.mu.Unlock()
+	assert.False(t, repo.refCheckValid)
+}
+
+func TestRepositoryEnsureRefsUpToDateDoesNotCacheInFlightCheck(t *testing.T) {
+	tmpDir := t.TempDir()
+	upstreamPath := createBareRepo(t, tmpDir)
+	config := testRepoConfig()
+	config.RefCheckInterval = time.Hour
+	repo := &Repository{
+		state:       StateEmpty,
+		config:      config,
+		path:        filepath.Join(tmpDir, "clone"),
+		upstreamURL: upstreamPath,
+		fetchSem:    make(chan struct{}, 1),
+	}
+	repo.fetchSem <- struct{}{}
+	assert.NoError(t, repo.Clone(t.Context()))
+
+	realGit, err := exec.LookPath("git")
+	assert.NoError(t, err)
+	binDir := t.TempDir()
+	started := filepath.Join(binDir, "started")
+	release := filepath.Join(binDir, "release")
+	script := fmt.Sprintf(`#!/bin/sh
+if [ "$1" = "ls-remote" ]; then
+  touch %q
+  while [ ! -e %q ]; do sleep 0.01; done
+fi
+exec %q "$@"
+`, started, release, realGit)
+	assert.NoError(t, os.WriteFile(filepath.Join(binDir, "git"), []byte(script), 0o755))
+	t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	type result struct {
+		stale bool
+		err   error
+	}
+	resultCh := make(chan result, 1)
+	go func() {
+		stale, err := repo.EnsureRefsUpToDate(t.Context())
+		resultCh <- result{stale: stale, err: err}
+	}()
+
+	deadline := time.Now().Add(5 * time.Second)
+	for {
+		if _, err := os.Stat(started); err == nil {
+			break
+		}
+		if time.Now().After(deadline) {
+			t.Fatal("ls-remote did not start")
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+
+	repo.mu.Lock()
+	valid := repo.refCheckValid
+	repo.mu.Unlock()
+	assert.False(t, valid)
+	assert.NoError(t, os.WriteFile(release, nil, 0o644))
+
+	got := <-resultCh
+	assert.NoError(t, got.err)
+	assert.False(t, got.stale)
+	repo.mu.Lock()
+	defer repo.mu.Unlock()
+	assert.True(t, repo.refCheckValid)
+}
+
+func TestRepositoryEnsureRefsUpToDateConcurrentStaleResultInvalidatesFreshResult(t *testing.T) {
+	tmpDir := t.TempDir()
+	upstreamPath := createBareRepo(t, tmpDir)
+	config := testRepoConfig()
+	config.RefCheckInterval = time.Hour
+	repo := &Repository{
+		state:       StateEmpty,
+		config:      config,
+		path:        filepath.Join(tmpDir, "clone"),
+		upstreamURL: upstreamPath,
+		fetchSem:    make(chan struct{}, 1),
+	}
+	repo.fetchSem <- struct{}{}
+	assert.NoError(t, repo.Clone(t.Context()))
+	localRefs, err := repo.GetLocalRefs(t.Context())
+	assert.NoError(t, err)
+	var headRef, localSHA string
+	for ref, sha := range localRefs {
+		if strings.HasPrefix(ref, "refs/heads/") {
+			headRef, localSHA = ref, sha
+			break
+		}
+	}
+	assert.NotEqual(t, "", headRef)
+
+	realGit, err := exec.LookPath("git")
+	assert.NoError(t, err)
+	binDir := t.TempDir()
+	claim := filepath.Join(binDir, "claim")
+	staleStarted := filepath.Join(binDir, "stale-started")
+	releaseStale := filepath.Join(binDir, "release-stale")
+	script := fmt.Sprintf(`#!/bin/sh
+if [ "$1" = "ls-remote" ]; then
+  if mkdir %q 2>/dev/null; then
+    touch %q
+    while [ ! -e %q ]; do sleep 0.01; done
+    printf 'ffffffffffffffffffffffffffffffffffffffff\t%s\n'
+  else
+    printf '%s\t%s\n'
+  fi
+  exit 0
+fi
+exec %q "$@"
+`, claim, staleStarted, releaseStale, headRef, localSHA, headRef, realGit)
+	assert.NoError(t, os.WriteFile(filepath.Join(binDir, "git"), []byte(script), 0o755))
+	t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	type result struct {
+		stale bool
+		err   error
+	}
+	staleResult := make(chan result, 1)
+	go func() {
+		stale, err := repo.EnsureRefsUpToDate(t.Context())
+		staleResult <- result{stale: stale, err: err}
+	}()
+
+	deadline := time.Now().Add(5 * time.Second)
+	for {
+		if _, err := os.Stat(staleStarted); err == nil {
+			break
+		}
+		if time.Now().After(deadline) {
+			t.Fatal("stale ref check did not start")
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+
+	fresh, err := repo.EnsureRefsUpToDate(t.Context())
+	assert.NoError(t, err)
+	assert.False(t, fresh)
+	repo.mu.Lock()
+	assert.True(t, repo.refCheckValid)
+	repo.mu.Unlock()
+
+	assert.NoError(t, os.WriteFile(releaseStale, nil, 0o644))
+	got := <-staleResult
+	assert.NoError(t, got.err)
+	assert.True(t, got.stale)
+	repo.mu.Lock()
+	defer repo.mu.Unlock()
+	assert.False(t, repo.refCheckValid)
+}
+
 // TestMirrorConfigAllowsUnreachableSHA verifies that the mirror config lets
 // git upload-pack serve objects that are present in the ODB but unreachable
 // from any ref (e.g. after a force-push orphans a commit).

internal/strategy/git/git.go

@@ -415,13 +415,29 @@ func (s *Strategy) handleGitRequest(w http.ResponseWriter, r *http.Request, host
 
 func (s *Strategy) serveReadyRepo(w http.ResponseWriter, r *http.Request, repo *gitclone.Repository, host, pathValue string, isInfoRefs bool) error {
 	ctx := r.Context()
+	logger := logging.FromContext(ctx)
+
+	isDiscovery := isInfoRefs
+	if !isDiscovery {
+		var err error
+		isDiscovery, err = requestIsLsRefs(pathValue, r)
+		if err != nil {
+			return errors.Wrap(err, "inspect ls-refs request")
+		}
+	}
 
-	stale, _ := s.checkRefsStale(ctx, repo) //nolint:errcheck // best-effort; treat as non-stale on failure
-	if isInfoRefs && stale {
+	stale, err := s.checkRefsStale(ctx, repo)
+	if isDiscovery && err != nil {
+		logger.ErrorContext(ctx, "Failed to check refs freshness, forwarding to upstream", "upstream", repo.UpstreamURL(),
+			"error", err)
+		s.forwardToUpstream(w, r, host, pathValue)
+		return nil
+	}
+	if isDiscovery && stale {
 		// Mirror is behind upstream. Forward to upstream so the client gets
 		// fresh refs immediately, and kick off a background fetch so the
 		// mirror catches up for subsequent requests.
-		logging.FromContext(ctx).InfoContext(ctx, "Refs stale, forwarding to upstream and fetching in background", "upstream", repo.UpstreamURL())
+		logger.InfoContext(ctx, "Refs stale, forwarding to upstream and fetching in background", "upstream", repo.UpstreamURL())
 		s.submitFetch(repo)
 		s.forwardToUpstream(w, r, host, pathValue)
 		return nil
@@ -445,7 +461,7 @@ func (s *Strategy) serveReadyRepo(w http.ResponseWriter, r *http.Request, repo *
 		// The mirror is missing the requested object — most likely a commit
 		// that was advertised before a concurrent force-push fetch orphaned
 		// it. Fall back to upstream so the client is not left with an error.
-		logging.FromContext(ctx).InfoContext(ctx, "Falling back to upstream due to 'not our ref'", "path", pathValue)
+		logger.InfoContext(ctx, "Falling back to upstream due to 'not our ref'", "path", pathValue)
 		if bodyBytes != nil {
 			r.Body = io.NopCloser(bytes.NewReader(bodyBytes))
 			r.ContentLength = int64(len(bodyBytes))