diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 1faa279ac9..70ad8ab579 100644 --- a/internal/job/checkout.go +++ b/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 } diff --git a/internal/job/checkout_mirror.go b/internal/job/checkout_mirror.go index d72c9050fd..117ed505ed 100644 --- a/internal/job/checkout_mirror.go +++ b/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/ 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/ 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/ 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/ + // 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/ 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 diff --git a/internal/job/checkout_mirror_freshness_test.go b/internal/job/checkout_mirror_freshness_test.go new file mode 100644 index 0000000000..8c6d774574 --- /dev/null +++ b/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/ 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") + } +} diff --git a/internal/job/commit_verification.go b/internal/job/commit_verification.go index 7b7eda5778..284842723f 100644 --- a/internal/job/commit_verification.go +++ b/internal/job/commit_verification.go @@ -244,9 +244,72 @@ func stripRefSuppressingFetchFlags(flags []string) []string { return out } +// verifyCommitAgainstSnapshot attempts the branch-ancestry check against the +// per-job mirror snapshot instead of fetching the branch tip from the +// canonical repository again. It returns true only on a definitive positive: +// the snapshot has refs/heads/ and the build commit is reachable from +// it. Every other outcome (missing ref, missing objects, command failure, +// negative ancestry) returns false so the caller falls back to canonical +// verification. A local negative is never treated as a verification failure, +// because it is only trustworthy when the check is set up perfectly; the +// canonical check remains the authority for failing a job. +// +// Callers must only use this when the snapshot's refs/heads/ was +// synced from the canonical repository during this job (mirrorReference. +// branchTipFresh). A positive is then a canonical observation made earlier in +// this same checkout — before mirror maintenance, snapshotting, and the source +// fetch — rather than at the verification probe itself. A force-push landing +// inside that window passes here where a fresh canonical fetch would fail; +// that is the same class of check-then-use window the fetch-based check +// already has between its fetch and the job actually using the commit, just +// slightly wider. Without that freshness guarantee (mirror +// update skipped because the commit was already present, refs populated from +// a remote mirror, --git-mirrors-skip-update) a stale tip could vouch for a +// commit that has since been force-pushed off the branch, so those paths fall +// back to the canonical check. +// +// Both commands run against the snapshot itself (--git-dir) rather than the +// checkout: the snapshot is immutable for the life of the job and holds both +// the branch tip and the build commit, and the checkout's +// refs/remotes/origin/ may have been pinned to the build commit +// itself (syncOriginBranchRef), which would make an ancestry check against it +// vacuously true. +func (e *Executor) verifyCommitAgainstSnapshot(ctx context.Context, snapshotDir string) bool { + ref := "refs/heads/" + e.Branch + if !gitCheckRefFormat(ref) { + return false + } + // ^{commit} both dereferences and type-checks: rev-parse fails unless the + // ref resolves to a commit. + tip, err := e.shell.Command( + "git", "--git-dir", snapshotDir, + "rev-parse", "--verify", "--quiet", ref+"^{commit}", + ).RunAndCaptureStdout(ctx, shell.ShowStderr(false)) + if err != nil { + return false + } + tip = strings.TrimSpace(tip) + + // merge-base --is-ancestor exit 0 is definitive even on unusual + // topologies; anything else falls back to canonical verification. The -- + // keeps an externally controlled e.Commit from being parsed as an option. + if err := e.shell.Command( + "git", "--git-dir", snapshotDir, + "merge-base", "--is-ancestor", "--", e.Commit, tip, + ).Run(ctx, shell.ShowStderr(false)); err != nil { + return false + } + e.shell.Commentf("Verified commit %q is on branch %q against the mirror snapshot", e.Commit, e.Branch) + return true +} + // verifyCommit ensures that the commit we are asked to build exists and is -// reachable on the branch we are given. -func (e *Executor) verifyCommit(ctx context.Context) error { +// reachable on the branch we are given. When this job's mirror update synced +// the branch tip from the canonical repository into a per-job snapshot, the +// ancestry check runs against the snapshot first (no network); any +// inconclusive or negative local result falls back to the canonical +// fetch-based check. +func (e *Executor) verifyCommit(ctx context.Context, mirror mirrorReference) error { switch e.GitCommitVerification { case GitCommitVerificationOff: e.shell.Commentf("Skipping commit verification: mode is off") @@ -296,6 +359,13 @@ func (e *Executor) verifyCommit(ctx context.Context) error { return nil } + // A snapshot whose branch tip was synced from canonical during this job + // can answer the same question locally. Only a definitive positive counts; + // anything else falls through to the canonical fetch-based check. + if mirror.isSnapshot && mirror.branchTipFresh && e.verifyCommitAgainstSnapshot(ctx, mirror.dir) { + return nil + } + // Perform the verification err := e.checkCommitOnBranch(ctx) diff --git a/internal/job/commit_verification_test.go b/internal/job/commit_verification_test.go index 0bd09bcfce..c2ea196f70 100644 --- a/internal/job/commit_verification_test.go +++ b/internal/job/commit_verification_test.go @@ -216,7 +216,7 @@ func TestVerifyCommit(t *testing.T) { shell: sh, ExecutorConfig: tt.config, } - err = e.verifyCommit(t.Context()) + err = e.verifyCommit(t.Context(), mirrorReference{}) if err != nil { t.Errorf("verifyCommit() error = %v, want nil", err) } @@ -395,7 +395,7 @@ func TestVerifyCommit(t *testing.T) { }, } - if err := e.verifyCommit(ctx); !errors.Is(err, ErrCommitVerificationFailed) { + if err := e.verifyCommit(ctx, mirrorReference{}); !errors.Is(err, ErrCommitVerificationFailed) { t.Errorf("verifyCommit() with mode %q error = %v, want ErrCommitVerificationFailed", mode.value, err) } }) @@ -889,7 +889,7 @@ func TestVerifyCommit(t *testing.T) { } // A skip would return nil; verifyCommit must surface the failure instead. - if err := e.verifyCommit(ctx); !errors.Is(err, ErrCommitVerificationFailed) { + if err := e.verifyCommit(ctx, mirrorReference{}); !errors.Is(err, ErrCommitVerificationFailed) { t.Errorf("verifyCommit() error = %v, want ErrCommitVerificationFailed (a non-PR build must not be skipped)", err) } }) @@ -948,12 +948,148 @@ func TestVerifyCommit(t *testing.T) { } // Even in strict mode, an unavailable check must not fail the build. - if err := e.verifyCommit(ctx); err != nil { + if err := e.verifyCommit(ctx, mirrorReference{}); err != nil { t.Errorf("verifyCommit() in strict mode with unavailable check error = %v, want nil", err) } }) } +// snapshotOfRepo creates a bare --mirror clone of repoURL, standing in for a +// per-job mirror snapshot. +func snapshotOfRepo(t *testing.T, ctx context.Context, sh *shell.Shell, repoURL string) string { + t.Helper() + snapshotDir, err := os.MkdirTemp("", "verify-snapshot-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(snapshotDir) }) //nolint:errcheck // Best-effort cleanup. + if err := sh.Command("git", "clone", "--mirror", repoURL, snapshotDir).Run(ctx); err != nil { + t.Fatalf("git clone --mirror error = %v", err) + } + return snapshotDir +} + +func TestVerifyCommitAgainstSnapshot(t *testing.T) { + ctx := t.Context() + repoURL, deepAncestor, offBranchCommit := setupFileBackedRepo(t, ctx, "feature") + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + snapshotDir := snapshotOfRepo(t, ctx, sh, repoURL) + + tests := []struct { + name string + commit string + branch string + want bool + }{ + {"commit on branch is a definitive positive", deepAncestor, "feature", true}, + {"commit not on branch is not trusted locally", offBranchCommit, "feature", false}, + {"missing branch ref falls back", deepAncestor, "no-such-branch", false}, + {"invalid branch name falls back", deepAncestor, "bad..name", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: GitCommitVerificationStrict, + Commit: tt.commit, + Branch: tt.branch, + }, + } + if got := e.verifyCommitAgainstSnapshot(ctx, snapshotDir); got != tt.want { + t.Errorf("verifyCommitAgainstSnapshot() = %t, want %t", got, tt.want) + } + }) + } +} + +func TestVerifyCommitSnapshotShortCircuitAndFallback(t *testing.T) { + ctx := t.Context() + repoURL, deepAncestor, offBranchCommit := setupFileBackedRepo(t, ctx, "feature") + + // The checkout: a clone of canonical with both branches' objects present. + cloneDir, err := os.MkdirTemp("", "verify-commit-test-") + if err != nil { + t.Fatalf("MkdirTemp error = %v", err) + } + t.Cleanup(func() { os.RemoveAll(cloneDir) }) //nolint:errcheck // Best-effort cleanup. + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + if err := sh.Command("git", "clone", repoURL, cloneDir).Run(ctx); err != nil { + t.Fatalf("git clone error = %v", err) + } + if err := sh.Chdir(cloneDir); err != nil { + t.Fatalf("Chdir error = %v", err) + } + if err := sh.Command("git", "fetch", "origin", "other").Run(ctx); err != nil { + t.Fatalf("git fetch error = %v", err) + } + + // A snapshot whose refs/heads/feature includes the off-branch commit, + // disagreeing with canonical (where that commit is only on "other"). This + // simulates the fetch-then-force-push race a fresh snapshot may observe. + snapshotDir := snapshotOfRepo(t, ctx, sh, repoURL) + if err := sh.Command("git", "--git-dir", snapshotDir, "update-ref", "refs/heads/feature", offBranchCommit).Run(ctx); err != nil { + t.Fatalf("git update-ref error = %v", err) + } + + exec := func(commit string, mirror mirrorReference) error { + e := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + GitCommitVerification: GitCommitVerificationStrict, + Commit: commit, + Branch: "feature", + }, + } + return e.verifyCommit(ctx, mirror) + } + + freshSnapshot := mirrorReference{dir: snapshotDir, isSnapshot: true, branchTipFresh: true} + + t.Run("fresh snapshot positive short-circuits the canonical check", func(t *testing.T) { + // Canonical would definitively fail this commit, so a nil error proves + // the snapshot answered. + if err := exec(offBranchCommit, freshSnapshot); err != nil { + t.Errorf("verifyCommit() error = %v, want nil (snapshot short-circuit)", err) + } + }) + + t.Run("stale snapshot is ignored and canonical fails the commit", func(t *testing.T) { + stale := freshSnapshot + stale.branchTipFresh = false + if err := exec(offBranchCommit, stale); !errors.Is(err, ErrCommitVerificationFailed) { + t.Errorf("verifyCommit() error = %v, want ErrCommitVerificationFailed (canonical fallback)", err) + } + }) + + t.Run("non-snapshot mirror is ignored and canonical fails the commit", func(t *testing.T) { + durable := mirrorReference{dir: snapshotDir, branchTipFresh: true} + if err := exec(offBranchCommit, durable); !errors.Is(err, ErrCommitVerificationFailed) { + t.Errorf("verifyCommit() error = %v, want ErrCommitVerificationFailed (canonical fallback)", err) + } + }) + + t.Run("inconclusive snapshot falls back to a canonical positive", func(t *testing.T) { + // A snapshot without the branch ref cannot answer, so the canonical + // check must run and verify the commit. + bare := snapshotOfRepo(t, ctx, sh, repoURL) + if err := sh.Command("git", "--git-dir", bare, "update-ref", "-d", "refs/heads/feature").Run(ctx); err != nil { + t.Fatalf("git update-ref -d error = %v", err) + } + mirror := mirrorReference{dir: bare, isSnapshot: true, branchTipFresh: true} + if err := exec(deepAncestor, mirror); err != nil { + t.Errorf("verifyCommit() error = %v, want nil (canonical verifies)", err) + } + }) +} + func TestStripShallowFetchFlags(t *testing.T) { tests := []struct { name string diff --git a/internal/job/git.go b/internal/job/git.go index d49c4dbcf5..01121de449 100644 --- a/internal/job/git.go +++ b/internal/job/git.go @@ -322,7 +322,8 @@ type gitFetchArgs struct { GitFetchFlags string // Flags to pass to the fetch command Repository string // The remote to fetch from Retry bool // Whether to retry the fetch on certain errors - RefSpecs []string // Refspecs to fetch + RefSpecs []string // Refspecs to fetch; each is shellword-split + RawRefSpecs []string // Refspecs passed verbatim as single arguments HidePrompt bool // Never log argv, including in shell debug mode } @@ -353,6 +354,11 @@ func gitFetch(ctx context.Context, args gitFetchArgs) error { commandArgs = append(commandArgs, individualRefSpecs...) } + // Raw refspecs skip shellword splitting: quotes and backslashes are legal + // in git ref names, so a split would corrupt a refspec built from an + // externally supplied branch. Callers pass exactly one refspec per element. + commandArgs = append(commandArgs, args.RawRefSpecs...) + smelt := map[string]bool{ gitErrStrBadObject: false, gitErrStrBadReference: false, diff --git a/internal/shell/shell.go b/internal/shell/shell.go index 39cc5fed49..8f2f9c80fd 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -31,7 +31,19 @@ import ( "go.opentelemetry.io/otel/trace" ) -const lockRetryDuration = time.Second +// lockRetryDuration is how often a blocked LockFile call re-attempts the +// lock. LockFile's callers are the git mirror clone/update locks, which are +// typically held for well under a second (a no-op fetch check, +// auto-maintenance thresholds, a hardlink snapshot), so a long interval +// quantizes every waiting job to whole multiples of it: with N same-host jobs +// racing for the same mirror, the last one waits ~N×interval regardless of +// how briefly the lock is actually held. +const lockRetryDuration = 100 * time.Millisecond + +// lockWaitLogInterval is how often a blocked LockFile call logs that it is +// still waiting, so a long wait stays visible in the job log without a line +// per retry. +const lockWaitLogInterval = 30 * time.Second // ErrShellNotStarted is returned when the shell has not started a process. var ErrShellNotStarted = errors.New("shell not started") @@ -233,6 +245,10 @@ func (s *Shell) LockFile(ctx context.Context, path string) (Unlocker, error) { lock := flock.New(absolutePathToLock) + retryTimer := time.NewTimer(lockRetryDuration) + defer retryTimer.Stop() + + var waitingSince, lastWaitLog time.Time retryLoop: for { // Keep trying the lock until we get it @@ -243,19 +259,29 @@ retryLoop: return nil, err case !gotLock: - s.Commentf("Could not acquire lock on %q (locked by another process)", absolutePathToLock) + // Log once when we start waiting and periodically while blocked, + // not on every retry. + switch now := time.Now(); { + case waitingSince.IsZero(): + waitingSince = now + lastWaitLog = now + s.Commentf("Could not acquire lock on %q (locked by another process); retrying every %v...", absolutePathToLock, lockRetryDuration) + case now.Sub(lastWaitLog) >= lockWaitLogInterval: + lastWaitLog = now + s.Commentf("Still waiting to acquire lock on %q (%v elapsed)", absolutePathToLock, now.Sub(waitingSince).Round(time.Second)) + } default: + if !waitingSince.IsZero() { + s.Commentf("Acquired lock on %q after %v", absolutePathToLock, time.Since(waitingSince).Round(time.Millisecond)) + } break retryLoop } - s.Commentf("Trying again in %v...", lockRetryDuration) - timer := time.NewTimer(lockRetryDuration) - defer timer.Stop() - select { - case <-timer.C: + case <-retryTimer.C: // Ready to retry! + retryTimer.Reset(lockRetryDuration) case <-ctx.Done(): return nil, ctx.Err()