Simplify git mirrors: dissociate by default, per-submodule references, less opportunistic maintenance

clicommand/global.go

@@ -296,8 +296,8 @@ var (
 
 	GitMirrorCheckoutModeFlag = &cli.StringFlag{
 		Name:    "git-mirror-checkout-mode",
-		Value:   "reference",
-		Usage:   fmt.Sprintf("Changes how clones of a mirror are made; available modes are %v. In ′dissociate′ mode, clones from a mirror uses the git clone ′--dissociate′ flag, which copies underlying objects from the mirror, making the clone robust to changes in the mirror such as garbage collection, at the expense of additional disk usage and setup time. ′reference′ mode does not pass ′--dissociate′, which causes the clone to directly use objects from the mirror, which is more fragile and can cause the clone to break under entirely normal operation of the mirror, but is slightly faster to clone and uses less disk space.", mirrorCheckoutModes),
+		Value:   "dissociate",
+		Usage:   fmt.Sprintf("Changes how clones of a mirror are made; available modes are %v. In ′dissociate′ mode (the default), clones from a mirror use the git clone ′--dissociate′ flag, which copies underlying objects from the mirror, making the clone robust to changes in the mirror such as garbage collection, at the expense of additional disk usage and setup time. ′reference′ mode does not pass ′--dissociate′, which causes the clone to depend on the mirror's object store for its whole lifetime; it is faster to clone and uses less disk space, but is only supported when you guarantee that the mirror's object store is immutable (never garbage collected or repacked) and outlives every checkout that references it. Under normal mirror operation, ′reference′ clones can be corrupted. Clean checkouts derive from an immutable per-job mirror snapshot instead, which is unaffected by this setting.", mirrorCheckoutModes),
 		Sources: cli.EnvVars("BUILDKITE_GIT_MIRROR_CHECKOUT_MODE"),
 	}
 

internal/job/checkout.go

@@ -608,61 +608,72 @@ func (e *Executor) updateGitSubmodules(ctx context.Context) (retErr error) {
 	}
 
 	// Checking for submodule repositories
-	submoduleRepos, err := gitEnumerateSubmoduleURLs(ctx, e.shell)
+	submodules, err := gitEnumerateSubmodules(ctx, e.shell)
 	if err != nil {
 		e.shell.Warningf("Failed to enumerate git submodules: %v", err)
 		return nil
 	}
 
-	submodulesSpan.SetAttributes(attribute.Int("git.count", len(submoduleRepos)))
+	submodulesSpan.SetAttributes(attribute.Int("git.count", len(submodules)))
 
-	mirrorSubmodules := e.GitMirrorsPath != ""
-	if mirrorSubmodules {
-		for _, repository := range submoduleRepos {
+	if e.GitMirrorsPath != "" {
+		for _, submodule := range submodules {
 			// getOrUpdateMirror is shared with the main repo's mirror update, so
 			// this produces the same sub-tree of spans; git.repo distinguishes
 			// submodules since the span names repeat.
 			subMirrorSpan, subMirrorCtx := e.traceOpSpan(ctx, "git.mirror.update")
-			subMirrorSpan.SetAttributes(attribute.String("git.repo", redact.URLCredentials(repository)))
+			subMirrorSpan.SetAttributes(attribute.String("git.repo", redact.URLCredentials(submodule.url)))
 
-			subMirror, err := e.getOrUpdateMirror(subMirrorCtx, repository, nil)
+			subMirror, err := e.getOrUpdateMirror(subMirrorCtx, submodule.url, nil)
 
 			tracetools.FinishWithError(subMirrorSpan, err)
 
 			if err != nil {
 				return fmt.Errorf("getting/updating mirror dir for submodules: %w", err)
 			}
-			mirrorDir := subMirror.dir
 
 			// Switch back to the checkout dir, doing other operations from GitMirrorsPath will fail.
 			if err := e.createCheckoutDir(); err != nil {
 				return fmt.Errorf("creating checkout dir: %w", err)
 			}
 
-			submoduleArgs := slices.Clone(args)
-			if mirrorDir != "" {
-				submoduleArgs = append(submoduleArgs, "submodule", "update", "--init", "--recursive", "--force", "--reference", mirrorDir)
-				if e.GitMirrorCheckoutMode == "dissociate" {
-					submoduleArgs = append(submoduleArgs, "--dissociate")
-				}
-			} else {
-				// Fall back to a clean update, rather than failing the checkout and therefore the build
-				submoduleArgs = append(submoduleArgs, "submodule", "update", "--init", "--recursive", "--force")
+			if subMirror.dir == "" {
+				// The mirror was bypassed (e.g. skip-update with no existing
+				// mirror). The recursive pass below initializes this submodule
+				// from its canonical URL rather than failing the build.
+				continue
 			}
 
+			// Initialize only this submodule path, without --recursive: git
+			// applies --reference to every submodule one invocation
+			// initializes, and this mirror only matches this path. Nested
+			// submodules are initialized by the recursive pass below.
+			submoduleArgs := append(slices.Clone(args), "submodule", "update", "--init", "--force", "--reference", subMirror.dir)
+			// A durable mirror garbage-collects objects during normal
+			// operation, so in dissociate mode the submodule must own its
+			// objects. A per-job snapshot is immutable for the life of the
+			// job's checkout, so referencing it is safe (see
+			// prepareCheckoutWorkdir).
+			if !subMirror.isSnapshot && e.GitMirrorCheckoutMode == "dissociate" {
+				submoduleArgs = append(submoduleArgs, "--dissociate")
+			}
+			submoduleArgs = append(submoduleArgs, "--", submodule.path)
+
 			if err := e.traceOp(ctx, "git.submodule.update", func(ctx context.Context) error {
 				return e.shell.Command("git", submoduleArgs...).Run(ctx)
 			}); err != nil {
-				return fmt.Errorf("updating submodules: %w", err)
+				return fmt.Errorf("updating submodule %q: %w", submodule.path, err)
 			}
 		}
-	} else { // no submodule mirrors
-		args = append(args, "submodule", "update", "--init", "--recursive", "--force")
-		if err := e.traceOp(ctx, "git.submodule.update", func(ctx context.Context) error {
-			return e.shell.Command("git", args...).Run(ctx)
-		}); err != nil {
-			return fmt.Errorf("updating submodules: %w", err)
-		}
+	}
+
+	// One recursive pass initializes everything not covered above: nested
+	// submodules, and all submodules when mirrors are disabled or bypassed.
+	args = append(args, "submodule", "update", "--init", "--recursive", "--force")
+	if err := e.traceOp(ctx, "git.submodule.update", func(ctx context.Context) error {
+		return e.shell.Command("git", args...).Run(ctx)
+	}); err != nil {
+		return fmt.Errorf("updating submodules: %w", err)
 	}
 
 	cmd := e.shell.Command("git", "submodule", "foreach", "--recursive", "git reset --hard")

internal/job/checkout_mirror.go

@@ -58,9 +58,11 @@ func (e *Executor) getOrUpdateMirror(ctx context.Context, repository string, att
 			return mirrorReference{}, nil
 		}
 
-		// If git mirror updates are skipped, we assume there's no change
-		// to the mirror objects, so no need for snapshotting. (Snapshotting
-		// would also be unsafe: skipping the update means no lock is taken.)
+		// When mirror updates are skipped, no lock is taken, so snapshotting
+		// the mirror here would be unsafe: an external process may be
+		// updating it concurrently. Return the mirror as a plain clone
+		// reference; the checkout-mode setting (dissociate by default)
+		// decides whether the checkout keeps depending on it.
 		return mirrorReference{dir: mirrorDir}, nil
 	}
 
@@ -319,8 +321,7 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem
 	// repository renames. This must happen before a remote-mirror hit can skip
 	// the canonical fetch: dirForRepository is lossy, so distinct canonical
 	// URLs can share one durable mirror directory.
-	urlChanged, err := e.updateRemoteURL(ctx, mirrorDir, repository)
-	if err != nil {
+	if err := e.updateRemoteURL(ctx, mirrorDir, repository); err != nil {
 		return mirrorReference{}, fmt.Errorf("setting remote URL: %w", err)
 	}
 
@@ -399,18 +400,6 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string, attem
 		}
 	}
 
-	if urlChanged {
-		// Let's opportunistically fsck and gc.
-		// 1. In case of remote URL confusion (bug introduced in #1959), and
-		// 2. There's possibly some object churn when remotes are renamed.
-		if err := e.shell.Command("git", "--git-dir", mirrorDir, "fsck").Run(ctx); err != nil {
-			e.shell.Warningf("Couldn't run git fsck: %v", err)
-		}
-		if err := e.shell.Command("git", "--git-dir", mirrorDir, "gc").Run(ctx); err != nil {
-			e.shell.Warningf("Couldn't run git gc: %v", err)
-		}
-	}
-
 	// With implicit maintenance disabled, this synchronous run (still under
 	// the update lock, and before the snapshot) is what keeps the mirror's
 	// object store consolidated over time. Failing maintenance is not worth
@@ -554,11 +543,11 @@ func (e *Executor) runMirrorAutoMaintenance(ctx context.Context, mirrorDir strin
 	})
 }
 
-// updateRemoteURL updates the URL for 'origin' and reports whether the
-// URL changed from something else. If gitDir == "", it assumes the
-// local repo is in the current directory, otherwise it includes --git-dir.
+// updateRemoteURL updates the URL for 'origin' if it differs from repository.
+// If gitDir == "", it assumes the local repo is in the current directory,
+// otherwise it includes --git-dir.
 // If the remote has changed, it logs some extra information.
-func (e *Executor) updateRemoteURL(ctx context.Context, gitDir, repository string) (bool, error) {
+func (e *Executor) updateRemoteURL(ctx context.Context, gitDir, repository string) error {
 	// Update the origin of the repository so we can gracefully handle
 	// repository renames.
 
@@ -572,7 +561,7 @@ func (e *Executor) updateRemoteURL(ctx context.Context, gitDir, repository strin
 	}
 	allURLs, err := e.shell.Command("git", args...).RunAndCaptureStdout(ctx)
 	if err != nil {
-		return false, err
+		return err
 	}
 
 	var gotURL string
@@ -586,7 +575,7 @@ func (e *Executor) updateRemoteURL(ctx context.Context, gitDir, repository strin
 		}
 		gotURL, err = e.shell.Command("git", args...).RunAndCaptureStdout(ctx)
 		if err != nil {
-			return false, err
+			return err
 		}
 	} else {
 		// Single URL - use config output directly to avoid insteadOf transformation.
@@ -595,7 +584,7 @@ func (e *Executor) updateRemoteURL(ctx context.Context, gitDir, repository strin
 
 	if gotURL == repository {
 		// No need to update anything
-		return false, nil
+		return nil
 	}
 
 	gd := gitDir
@@ -611,7 +600,7 @@ func (e *Executor) updateRemoteURL(ctx context.Context, gitDir, repository strin
 	if gitDir != "" {
 		args = append([]string{"--git-dir", gitDir}, args...)
 	}
-	return true, e.shell.Command("git", args...).Run(ctx)
+	return e.shell.Command("git", args...).Run(ctx)
 }
 
 // This is the same thing that git does at the end of clone when it is

internal/job/checkout_snapshot_derive_test.go

@@ -244,6 +244,11 @@ func TestPrepareCheckoutWorkdirIncompatibleFlagsUseSnapshotAsReferenceOnly(t *te
 	if got, want := gitOutputForMirrorTest(t, filepath.Join(e.shell.Getwd(), ".git"), "config", "--get", "remote.origin.url"), e.Repository; got != want {
 		t.Errorf("remote.origin.url = %q, want canonical %q", got, want)
 	}
+	// The snapshot is deleted at the end of the job, so a reference to it must
+	// always be dissociated, regardless of the checkout mode.
+	if osutil.FileExists(filepath.Join(e.shell.Getwd(), ".git", "objects", "info", "alternates")) {
+		t.Error("fallback clone retains an alternate into the per-job snapshot")
+	}
 }
 
 func TestCloneFlagsAllowSnapshotDerive(t *testing.T) {

internal/job/checkout_workdir.go

@@ -49,29 +49,36 @@ func (e *Executor) prepareCheckoutWorkdir(
 
 	existingGitDir := filepath.Join(e.shell.Getwd(), ".git")
 	if osutil.FileExists(existingGitDir) {
-		if _, err := e.updateRemoteURL(ctx, "", e.Repository); err != nil {
+		if err := e.updateRemoteURL(ctx, "", e.Repository); err != nil {
 			return false, fmt.Errorf("setting origin: %w", err)
 		}
 
-		if mirror.dir == "" && e.GitMirrorsPath != "" {
+		switch {
+		case mirror.dir == "" && e.GitMirrorsPath != "":
 			// A bypassed mirror must not remain reachable through a stale alternate.
 			if err := e.traceOp(ctx, "git.dissociate", func(ctx context.Context) error {
 				return e.dissociateIfNeeded(ctx, existingGitDir)
 			}); err != nil {
 				return false, fmt.Errorf("dissociating bypassed mirror: %w", err)
 			}
-		} else if mirror.dir != "" {
-			switch e.GitMirrorCheckoutMode {
-			case "dissociate":
-				if err := e.traceOp(ctx, "git.dissociate", func(ctx context.Context) error {
-					return e.dissociateIfNeeded(ctx, existingGitDir)
-				}); err != nil {
-					return false, fmt.Errorf("dissociating existing reference clone: %w", err)
-				}
-			case "reference":
-				if err := e.reassociateIfNeeded(ctx, existingGitDir, mirror.dir); err != nil {
-					return false, fmt.Errorf("reassociating existing clone: %w", err)
-				}
+
+		case mirror.dir != "" && e.GitMirrorCheckoutMode == "dissociate":
+			// Convert a legacy reference clone into a self-owning checkout, so
+			// it no longer breaks when the mirror garbage-collects objects it
+			// depends on (#2208).
+			if err := e.traceOp(ctx, "git.dissociate", func(ctx context.Context) error {
+				return e.dissociateIfNeeded(ctx, existingGitDir)
+			}); err != nil {
+				return false, fmt.Errorf("dissociating existing reference clone: %w", err)
+			}
+
+		case mirror.dir != "" && e.GitMirrorCheckoutMode == "reference" && !mirror.isSnapshot:
+			// Explicit reference mode: repair a missing alternates file so the
+			// checkout keeps borrowing objects from the durable mirror. Never
+			// point an existing checkout at a snapshot: snapshots are deleted
+			// at the end of the job, but this checkout outlives it.
+			if err := e.reassociateIfNeeded(ctx, existingGitDir, mirror.dir); err != nil {
+				return false, fmt.Errorf("reassociating existing clone: %w", err)
 			}
 		}
 		return false, nil
@@ -81,7 +88,7 @@ func (e *Executor) prepareCheckoutWorkdir(
 
 	if mirror.dir != "" && !deriveFromSnapshot {
 		gitCloneFlags = append(gitCloneFlags, "--reference", mirror.dir)
-		if e.GitMirrorCheckoutMode == "dissociate" {
+		if e.mirrorReferenceNeedsDissociate(mirror) {
 			gitCloneFlags = append(gitCloneFlags, "--dissociate")
 		}
 	}
@@ -260,9 +267,6 @@ func (e *Executor) deriveCheckoutFromSnapshot(ctx context.Context, snapshotDir s
 	// git checkout of the build's target commit, instead of first
 	// materializing the snapshot's HEAD.
 	flags = append(flags, "--no-checkout", "--no-local", "--reference", snapshotDir)
-	if e.GitMirrorCheckoutMode == "dissociate" {
-		flags = append(flags, "--dissociate")
-	}
 
 	err := gitClone(ctx, e.shell, nil, flags, snapshotDir, ".")
 	if err == nil {
@@ -334,6 +338,20 @@ var snapshotDeriveIncompatibleCloneFlags = []string{
 	"--bu", // --bundle-uri: bootstraps objects from elsewhere
 }
 
+// mirrorReferenceNeedsDissociate reports whether a fresh clone that passes
+// --reference mirror.dir must also pass --dissociate. Dissociating copies the
+// borrowed objects into the checkout, making it self-owning.
+//
+// In "dissociate" mode that is simply the configured behavior. Independent of
+// the mode, a reference to a per-job snapshot must always be dissociated:
+// the snapshot is deleted at the end of the job, and this path is only
+// reached when the clone flags were incompatible with deriving from the
+// snapshot — including flags like --separate-git-dir, which can make the git
+// dir outlive the checkout directory's clean-checkout removal.
+func (e *Executor) mirrorReferenceNeedsDissociate(mirror mirrorReference) bool {
+	return mirror.isSnapshot || e.GitMirrorCheckoutMode == "dissociate"
+}
+
 // cloneFlagsAllowSnapshotDerive reports whether the user-supplied clone flags
 // are compatible with deriving the fresh checkout from the mirror snapshot.
 func cloneFlagsAllowSnapshotDerive(flags []string) bool {