Speed up mirror checkouts: 100ms lock polling and snapshot-based commit verification

internal/job/checkout.go

@@ -445,7 +445,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context, previousAttempts in
 	}
 
 	if err := e.traceOp(ctx, "git.verify_commit", func(ctx context.Context) error {
-		return e.verifyCommit(ctx)
+		return e.verifyCommit(ctx, mirror)
 	}); err != nil {
 		return err
 	}

internal/job/checkout_mirror.go

@@ -43,6 +43,17 @@ func remoteMirrorStagingDir(mirrorDir string) string {
 type mirrorReference struct {
 	dir        string
 	isSnapshot bool
+
+	// branchTipFresh records that refs/heads/<branch> in the snapshot was
+	// synced from the canonical repository during this job: either the mirror
+	// was freshly cloned from canonical, or this job's mirror update fetched
+	// the build branch from canonical. Commit verification may then check
+	// branch ancestry against the snapshot instead of fetching the branch tip
+	// from canonical again. It is never set when the branch fetch was skipped
+	// (commit already present in the mirror), when refs came from a remote
+	// mirror, or with --git-mirrors-skip-update — in those cases the tip may
+	// be stale and verification must ask the canonical repository.
+	branchTipFresh bool
 }
 
 func (e *Executor) getOrUpdateMirror(ctx context.Context, repository string, attempt *remoteMirrorAttempt) (mirrorReference, error) {
@@ -225,7 +236,9 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem
 					// --reference clone transfers only the missing delta.
 					attempt.outcome = remoteMirrorOutcomeMiss
 				}
-				return e.snapshotMirror(ctx, repository, mirrorDir)
+				// Refs came from the remote mirror, not canonical, so the
+				// branch tip is not known to be current.
+				return e.snapshotMirror(ctx, repository, mirrorDir, false)
 			}
 
 			if stagingCleanupErr == nil && tempMirrorDir != "" {
@@ -257,7 +270,11 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem
 		if err := e.disableMirrorAutoMaintenance(ctx, mirrorDir); err != nil {
 			return mirrorReference{}, err
 		}
-		return e.snapshotMirror(ctx, repository, mirrorDir)
+		// A fresh clone from canonical: every ref, including the build
+		// branch's tip, is current as of this job. Freshness only means
+		// anything for the main repository's branch, so don't claim it for
+		// submodule mirrors.
+		return e.snapshotMirror(ctx, repository, mirrorDir, isMainRepository)
 	}
 
 	// If it exists, immediately release the clone lock.
@@ -347,8 +364,12 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem
 		}
 	}
 
+	// Set when this job fetches refs/heads/<branch> from canonical below, so
+	// the snapshot's branch tip is current and commit verification can use it.
+	branchTipFresh := false
+
 	if isMainRepository && !commitAlreadyPresent && !remoteMirrorHit {
-		var refspecs []string
+		var refspecs, rawRefspecs []string
 		var retry bool
 
 		switch {
@@ -368,17 +389,38 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem
 			retry = true
 		default:
 			// Fetch the build branch from the upstream repository into the mirror.
-			refspecs = []string{e.Branch}
+			//
+			// For a branch build, fetch by explicit forced refspec rather than
+			// the bare name: a bare name resolves refs/tags/ before refs/heads/,
+			// so a tag sharing the branch's name would be fetched instead,
+			// leaving refs/heads/<branch> stale — while branchTipFresh claims
+			// otherwise. The explicit form updates exactly the ref that
+			// snapshot-based commit verification later reads, and it is passed
+			// raw (unsplit) because quotes are legal in ref names. This mirrors
+			// checkCommitOnBranch's own fetch construction.
+			//
+			// Tag builds may set BUILDKITE_BRANCH to the tag's name, and some
+			// setups supply an already-qualified ref, where refs/heads/<branch>
+			// may not exist. Those keep the historical bare-name fetch and
+			// never claim freshness (verification skips tag builds anyway).
+			branchRef := "refs/heads/" + e.Branch
+			if e.Tag == "" && e.Branch != "" && !strings.HasPrefix(e.Branch, "refs/") && gitCheckRefFormat(branchRef) {
+				rawRefspecs = []string{"+" + branchRef + ":" + branchRef}
+				branchTipFresh = true
+			} else {
+				refspecs = []string{e.Branch}
+			}
 		}
 
 		// Fetch the refspecs from the upstream repository into the mirror.
 		if err := e.traceOp(ctx, "git.mirror.fetch", func(ctx context.Context) error {
 			return gitFetch(ctx, gitFetchArgs{
-				Shell:      e.shell,
-				GitFlags:   []string{"--git-dir", mirrorDir},
-				Repository: "origin",
-				RefSpecs:   refspecs,
-				Retry:      retry,
+				Shell:       e.shell,
+				GitFlags:    []string{"--git-dir", mirrorDir},
+				Repository:  "origin",
+				RefSpecs:    refspecs,
+				RawRefSpecs: rawRefspecs,
+				Retry:       retry,
 			})
 		}); err != nil {
 			return mirrorReference{}, err
@@ -411,7 +453,7 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem
 		e.shell.Warningf("Couldn't run mirror maintenance: %v", err)
 	}
 
-	return e.snapshotMirror(ctx, repository, mirrorDir)
+	return e.snapshotMirror(ctx, repository, mirrorDir, branchTipFresh)
 }
 
 // snapshotMirror creates a snapshot of the mirror. It returns the reference
@@ -459,7 +501,12 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem
 // checkout phase, which could break many git operations in the command phase.
 // Presently we have no way to pass cleanup instructions between containers,
 // which would enable this case.
-func (e *Executor) snapshotMirror(ctx context.Context, repository, mirrorDir string) (mirrorReference, error) {
+// branchTipFresh reports whether refs/heads/<branch> in the mirror (and so in
+// the snapshot) was synced from the canonical repository during this job; see
+// mirrorReference.branchTipFresh. It only travels with a snapshot: the durable
+// mirror is mutable and unlocked once this returns, so nothing may be assumed
+// about its refs later.
+func (e *Executor) snapshotMirror(ctx context.Context, repository, mirrorDir string, branchTipFresh bool) (mirrorReference, error) {
 	if !e.CleanCheckout || !e.includePhase("command") {
 		return mirrorReference{dir: mirrorDir}, nil
 	}
@@ -496,7 +543,7 @@ func (e *Executor) snapshotMirror(ctx context.Context, repository, mirrorDir str
 		return mirrorReference{}, err
 	}
 
-	return mirrorReference{dir: snapshotDir, isSnapshot: true}, nil
+	return mirrorReference{dir: snapshotDir, isSnapshot: true, branchTipFresh: branchTipFresh}, nil
 }
 
 // disableMirrorAutoMaintenance persistently prevents git from starting

internal/job/checkout_mirror_freshness_test.go

@@ -0,0 +1,182 @@
+package job
+
+import (
+	"os"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/buildkite/agent/v4/internal/shell"
+)
+
+// mirrorsTempDir makes a short temp dir for GitMirrorsPath with best-effort
+// cleanup. Not t.TempDir(): its path embeds this file's long test names, and
+// the deepest snapshot paths (mirrors path + "snapshots" +
+// dirForRepository(repo URL) + a pack file name) then exceed Windows'
+// 260-character MAX_PATH, failing git with a bare exit status 128. Removal is
+// best-effort because on Windows git child processes can hold handles past
+// exit, which t.TempDir()'s strict cleanup would fail on.
+func mirrorsTempDir(t *testing.T) string {
+	t.Helper()
+	dir, err := os.MkdirTemp("", "mirrors-")
+	if err != nil {
+		t.Fatalf("os.MkdirTemp error = %v", err)
+	}
+	t.Cleanup(func() { os.RemoveAll(dir) }) //nolint:errcheck // Best-effort cleanup.
+	return dir
+}
+
+// TestUpdateGitMirrorBranchTipFreshDespiteSameNamedTag exercises the
+// branchTipFresh invariant end to end through getOrUpdateMirror: when the
+// warm-path mirror update claims freshness, refs/heads/<branch> in the
+// snapshot must be the canonical branch tip — even when a tag shares the
+// branch's name. A bare-name fetch would resolve the tag (refs/tags/ wins),
+// leave the branch ref stale, and let snapshot verification vouch for a
+// commit against an outdated tip.
+func TestUpdateGitMirrorBranchTipFreshDespiteSameNamedTag(t *testing.T) {
+	ctx := t.Context()
+	sh, repoURL, commit := newFileBackedRepo(t, ctx, "mirror-fresh")
+
+	// Branch "release": a <- b, pushed to canonical.
+	base := commit("a.txt")
+	firstTip := commit("b.txt")
+	if err := sh.Command("git", "branch", "-m", "release").Run(ctx); err != nil {
+		t.Fatalf("git branch -m release error = %v", err)
+	}
+	if err := sh.Command("git", "push", "origin", "release").Run(ctx); err != nil {
+		t.Fatalf("git push release error = %v", err)
+	}
+
+	// Tag "release" on a divergent commit (a <- t), pushed to canonical.
+	if err := sh.Command("git", "checkout", "-b", "tagline", base).Run(ctx); err != nil {
+		t.Fatalf("git checkout -b tagline error = %v", err)
+	}
+	commit("t.txt")
+	if err := sh.Command("git", "tag", "release").Run(ctx); err != nil {
+		t.Fatalf("git tag release error = %v", err)
+	}
+	if err := sh.Command("git", "push", "origin", "refs/tags/release").Run(ctx); err != nil {
+		t.Fatalf("git push tag release error = %v", err)
+	}
+
+	e := New(ExecutorConfig{
+		Repository:            repoURL,
+		Commit:                firstTip,
+		Branch:                "release",
+		PullRequest:           "false",
+		GitMirrorsPath:        mirrorsTempDir(t),
+		GitMirrorsLockTimeout: 30,
+		CleanCheckout:         true,
+		Phases:                []string{"checkout", "command"},
+	})
+	e.shell = shell.NewTestShell(t, shell.WithSignalGracePeriod(10*time.Millisecond))
+
+	snapshotBranchTip := func(mirror mirrorReference) string {
+		t.Helper()
+		out, err := e.shell.Command(
+			"git", "--git-dir", mirror.dir, "rev-parse", "refs/heads/release",
+		).RunAndCaptureStdout(ctx)
+		if err != nil {
+			t.Fatalf("rev-parse refs/heads/release in snapshot error = %v", err)
+		}
+		return strings.TrimSpace(out)
+	}
+
+	// First job: fresh clone from canonical.
+	mirror, err := e.getOrUpdateMirror(ctx, e.Repository, nil)
+	if err != nil {
+		t.Fatalf("getOrUpdateMirror() error = %v", err)
+	}
+	if !mirror.isSnapshot || !mirror.branchTipFresh {
+		t.Fatalf("getOrUpdateMirror() = %+v, want a fresh snapshot from a canonical clone", mirror)
+	}
+	if got := snapshotBranchTip(mirror); got != firstTip {
+		t.Errorf("snapshot refs/heads/release = %s, want %s", got, firstTip)
+	}
+
+	// The branch advances on canonical: a <- b <- c.
+	if err := sh.Command("git", "checkout", "release").Run(ctx); err != nil {
+		t.Fatalf("git checkout release error = %v", err)
+	}
+	newTip := commit("c.txt")
+	// The push must name the ref fully: with the tag present, a bare
+	// "release" is ambiguous on the pushing side too.
+	if err := sh.Command("git", "push", "origin", "refs/heads/release:refs/heads/release").Run(ctx); err != nil {
+		t.Fatalf("git push release error = %v", err)
+	}
+
+	// Second job for the new commit: the warm-path update must fetch
+	// refs/heads/release itself, not the same-named tag.
+	e.Commit = newTip
+	mirror, err = e.getOrUpdateMirror(ctx, e.Repository, nil)
+	if err != nil {
+		t.Fatalf("getOrUpdateMirror() error = %v", err)
+	}
+	if !mirror.isSnapshot || !mirror.branchTipFresh {
+		t.Fatalf("getOrUpdateMirror() = %+v, want a fresh snapshot from a warm branch fetch", mirror)
+	}
+	if got := snapshotBranchTip(mirror); got != newTip {
+		t.Errorf("snapshot refs/heads/release = %s, want %s (bare-name fetch resolves the tag and leaves the branch stale)", got, newTip)
+	}
+}
+
+// TestUpdateGitMirrorNoFreshnessForTagBuilds pins the conservative cases: tag
+// builds keep the historical bare-name fetch and must not claim a fresh
+// branch tip, and a mirror update that skips the fetch (commit already
+// present) must not either.
+func TestUpdateGitMirrorNoFreshnessForTagBuilds(t *testing.T) {
+	ctx := t.Context()
+	sh, repoURL, commit := newFileBackedRepo(t, ctx, "mirror-tag")
+
+	commit("a.txt")
+	tip := commit("b.txt")
+	if err := sh.Command("git", "branch", "-m", "v1").Run(ctx); err != nil {
+		t.Fatalf("git branch -m v1 error = %v", err)
+	}
+	if err := sh.Command("git", "push", "origin", "v1").Run(ctx); err != nil {
+		t.Fatalf("git push v1 error = %v", err)
+	}
+
+	e := New(ExecutorConfig{
+		Repository:            repoURL,
+		Commit:                tip,
+		Branch:                "v1",
+		Tag:                   "v1",
+		PullRequest:           "false",
+		GitMirrorsPath:        mirrorsTempDir(t),
+		GitMirrorsLockTimeout: 30,
+		CleanCheckout:         true,
+		Phases:                []string{"checkout", "command"},
+	})
+	e.shell = shell.NewTestShell(t, shell.WithSignalGracePeriod(10*time.Millisecond))
+
+	// First call clones the mirror; run again with a new commit so the second
+	// call takes the warm fetch path with Tag set.
+	if _, err := e.getOrUpdateMirror(ctx, e.Repository, nil); err != nil {
+		t.Fatalf("getOrUpdateMirror() error = %v", err)
+	}
+	newTip := commit("c.txt")
+	if err := sh.Command("git", "push", "origin", "v1").Run(ctx); err != nil {
+		t.Fatalf("git push v1 error = %v", err)
+	}
+	e.Commit = newTip
+	mirror, err := e.getOrUpdateMirror(ctx, e.Repository, nil)
+	if err != nil {
+		t.Fatalf("getOrUpdateMirror() error = %v", err)
+	}
+	if !mirror.isSnapshot {
+		t.Fatalf("getOrUpdateMirror() = %+v, want a snapshot", mirror)
+	}
+	if mirror.branchTipFresh {
+		t.Errorf("branchTipFresh = true for a tag build's warm fetch, want false")
+	}
+
+	// Same commit again: the fetch is skipped, so no freshness either.
+	mirror, err = e.getOrUpdateMirror(ctx, e.Repository, nil)
+	if err != nil {
+		t.Fatalf("getOrUpdateMirror() error = %v", err)
+	}
+	if mirror.branchTipFresh {
+		t.Errorf("branchTipFresh = true when the mirror fetch was skipped, want false")
+	}
+}