From 7020d2b5691ae054ac83406b4551e5b480fba543 Mon Sep 17 00:00:00 2001 From: lizrabuya <115472349+lizrabuya@users.noreply.github.com> Date: Mon, 11 May 2026 16:53:49 +1000 Subject: [PATCH 1/9] feat: enable git lfs checkout --- clicommand/bootstrap.go | 7 ++ internal/job/checkout.go | 24 ++++++ internal/job/checkout_test.go | 155 ++++++++++++++++++++++++++++++++++ internal/job/config.go | 3 + internal/job/executor.go | 4 + internal/job/git.go | 16 ++++ 6 files changed, 209 insertions(+) diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index cc6e759a5c..598ef39038 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -60,6 +60,7 @@ type BootstrapConfig struct { PullRequest string `cli:"pullrequest"` PullRequestUsingMergeRefspec bool `cli:"pull-request-using-merge-refspec"` GitSubmodules bool `cli:"git-submodules"` + GitLFSEnabled bool `cli:"git-lfs-enabled"` SSHKeyscan bool `cli:"ssh-keyscan"` AgentName string `cli:"agent" validate:"required"` Queue string `cli:"queue"` @@ -301,6 +302,11 @@ var BootstrapCommand = cli.Command{ Usage: "Enable git submodules (default: true)", EnvVar: "BUILDKITE_GIT_SUBMODULES", }, + cli.BoolFlag{ + Name: "git-lfs-enabled", + Usage: "Enable Git LFS object download during checkout (default: false)", + EnvVar: "BUILDKITE_GIT_LFS_ENABLED", + }, cli.BoolTFlag{ Name: "pty", Usage: "Run jobs within a pseudo terminal (default: true)", @@ -450,6 +456,7 @@ var BootstrapCommand = cli.Command{ GitCloneFlags: cfg.GitCloneFlags, GitCloneMirrorFlags: cfg.GitCloneMirrorFlags, GitFetchFlags: cfg.GitFetchFlags, + GitLFSEnabled: cfg.GitLFSEnabled, GitMirrorsLockTimeout: cfg.GitMirrorsLockTimeout, GitMirrorsPath: cfg.GitMirrorsPath, GitMirrorCheckoutMode: cfg.GitMirrorCheckoutMode, diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 6d8c80157a..77f162df36 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "os/exec" "path/filepath" "runtime" "slices" @@ -939,6 +940,13 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) { } } + // Fail fast before any git work + if e.GitLFSEnabled { + if _, err := exec.LookPath("git-lfs"); err != nil { + return fmt.Errorf("BUILDKITE_GIT_LFS_ENABLED=true but git-lfs binary is not found on PATH: %w", err) + } + } + // Git clean prior to checkout, we do this even if submodules have been // disabled to ensure previous submodules are cleaned up if hasGitSubmodules(e.shell) { @@ -951,6 +959,14 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) { return fmt.Errorf("cleaning git repository: %w", err) } + // Install filter before git fetch operations + if e.GitLFSEnabled { + e.shell.Commentf("Installing Git LFS filter") + if err := e.shell.Command("git", "lfs", "install", "--local").Run(ctx); err != nil { + return fmt.Errorf("installing git lfs filter: %w", err) + } + } + if err := e.fetchSource(ctx); err != nil { return err } @@ -1056,6 +1072,14 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) { } } + if e.GitLFSEnabled { + // gitLFSFetchCheckout returns distinct "git lfs fetch: ..." or + // "git lfs checkout: ..." errors so the failing step is clear from logs. + if err := gitLFSFetchCheckout(ctx, e.shell); err != nil { + return err + } + } + // Git clean after checkout. We need to do this because submodules could have // changed in between the last checkout and this one. A double clean is the only // good solution to this problem that we've found diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index a23167d737..168bea2adc 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -3,6 +3,9 @@ package job import ( "context" "os" + "os/exec" + "path/filepath" + "strings" "testing" "time" @@ -276,3 +279,155 @@ func TestDefaultCheckoutPhase_DelayedRefCreation(t *testing.T) { t.Fatalf("tt.executor.defaultCheckoutPhase(ctx) error = %v, want nil", err) } } + +func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { + // Not parallel: subtests manipulate PATH via t.Setenv, which modifies + // process-global state. + ctx := context.Background() + + t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") + t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") + t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") + t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") + + // Note: "LFS enabled" test bypasses GIT_LFS_SKIP_SMUDGE=1 + // setUp sets GIT_LFS_SKIP_SMUDGE=1 to prevent implicit LFS downloads + // during git checkout. These tests call defaultCheckoutPhase directly so they + // don't exercise that env var, but the test repo has no LFS-tracked files so + // smudge filters are never triggered. + tests := []struct { + name string + lfsEnabled bool + // setupPath runs before the shell and executor are created. Use it to + // manipulate PATH for binary-presence tests. + setupPath func(t *testing.T) + wantErr string + }{ + { + name: "LFS disabled", + lfsEnabled: false, + }, + { + name: "LFS enabled binary present", + lfsEnabled: true, + setupPath: func(t *testing.T) { + if _, err := exec.LookPath("git-lfs"); err != nil { + t.Skip("git-lfs not installed") + } + }, + }, + { + name: "LFS enabled binary missing", + lfsEnabled: true, + setupPath: func(t *testing.T) { + gitBin, err := exec.LookPath("git") + if err != nil { + t.Fatalf("exec.LookPath(\"git\") error = %v", err) + } + binDir := t.TempDir() + if err := os.Symlink(gitBin, filepath.Join(binDir, "git")); err != nil { + t.Fatalf("os.Symlink() error = %v", err) + } + t.Setenv("PATH", binDir) + }, + wantErr: "git-lfs binary is not found on PATH", + }, + { + name: "LFS enabled git lfs command fails", + lfsEnabled: true, + setupPath: func(t *testing.T) { + gitBin, err := exec.LookPath("git") + if err != nil { + t.Fatalf("exec.LookPath(\"git\") error = %v", err) + } + binDir := t.TempDir() + if err := os.Symlink(gitBin, filepath.Join(binDir, "git")); err != nil { + t.Fatalf("os.Symlink() error = %v", err) + } + fakeLFS := filepath.Join(binDir, "git-lfs") + if err := os.WriteFile(fakeLFS, []byte("#!/bin/sh\nexit 1\n"), 0o755); err != nil { + t.Fatalf("os.WriteFile() error = %v", err) + } + t.Setenv("PATH", binDir) + }, + wantErr: "installing git lfs filter", + }, + { + name: "LFS enabled git lfs fetch fails", + lfsEnabled: true, + setupPath: func(t *testing.T) { + gitBin, err := exec.LookPath("git") + if err != nil { + t.Fatalf("exec.LookPath(\"git\") error = %v", err) + } + binDir := t.TempDir() + if err := os.Symlink(gitBin, filepath.Join(binDir, "git")); err != nil { + t.Fatalf("os.Symlink() error = %v", err) + } + // install exits 0; every other subcommand (fetch, checkout) exits 1. + fakeLFS := filepath.Join(binDir, "git-lfs") + script := "#!/bin/sh\ncase \"$1\" in\n install) exit 0 ;;\n *) exit 1 ;;\nesac\n" + if err := os.WriteFile(fakeLFS, []byte(script), 0o755); err != nil { + t.Fatalf("os.WriteFile() error = %v", err) + } + t.Setenv("PATH", binDir) + }, + wantErr: "git lfs fetch", + }, + } + + s := githttptest.NewServer() + t.Cleanup(s.Close) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.setupPath != nil { + tt.setupPath(t) + } + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + + projectName := "lfs-" + strings.ReplaceAll(strings.ToLower(tt.name), " ", "-") + if err := s.CreateRepository(projectName); err != nil { + t.Fatalf("s.CreateRepository(%q) error = %v", projectName, err) + } + out, err := s.InitRepository(projectName) + if err != nil { + t.Fatalf("s.InitRepository(%q) error = %v, output: %s", projectName, err, out) + } + + checkoutDir := t.TempDir() + sh.Env.Set("BUILDKITE_BUILD_CHECKOUT_PATH", checkoutDir) + + executor := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + Commit: "HEAD", + Branch: "main", + GitCleanFlags: "-f -d -x", + BuildPath: t.TempDir(), + Repository: s.RepoURL(projectName), + GitLFSEnabled: tt.lfsEnabled, + }, + } + + err = executor.defaultCheckoutPhase(ctx) + if tt.wantErr == "" { + if err != nil { + t.Errorf("defaultCheckoutPhase() error = %v, want nil", err) + } + return + } + if err == nil { + t.Errorf("defaultCheckoutPhase() error = nil, want error containing %q", tt.wantErr) + return + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("defaultCheckoutPhase() error = %q, want it to contain %q", err.Error(), tt.wantErr) + } + }) + } +} diff --git a/internal/job/config.go b/internal/job/config.go index 5301609407..205c41b19d 100644 --- a/internal/job/config.go +++ b/internal/job/config.go @@ -53,6 +53,9 @@ type ExecutorConfig struct { // Should git submodules be checked out GitSubmodules bool `env:"BUILDKITE_GIT_SUBMODULES"` + // Whether to enable Git LFS operations during checkout + GitLFSEnabled bool `env:"BUILDKITE_GIT_LFS_ENABLED"` + // If the commit was part of a pull request, this will container the PR number PullRequest string diff --git a/internal/job/executor.go b/internal/job/executor.go index d1152203aa..9176d8af1b 100644 --- a/internal/job/executor.go +++ b/internal/job/executor.go @@ -916,6 +916,10 @@ func (e *Executor) setUp(ctx context.Context) (retErr error) { // Disable any interactive Git/SSH prompting e.shell.Env.Set("GIT_TERMINAL_PROMPT", "0") + // Force-set GIT_LFS_SKIP_SMUDGE=1 before any git operations so LFS + // objects are not downloaded automatically during checkout. + e.shell.Env.Set("GIT_LFS_SKIP_SMUDGE", "1") + // Fetch and set secrets before environment hook execution if e.Secrets != "" { if err := e.fetchAndSetSecrets(ctx); err != nil { diff --git a/internal/job/git.go b/internal/job/git.go index aef370940a..32edf5fc34 100644 --- a/internal/job/git.go +++ b/internal/job/git.go @@ -143,6 +143,22 @@ func gitRepack(ctx context.Context, sh *shell.Shell, args ...string) error { return nil } +// gitLFSFetchCheckout fetches LFS objects for the current HEAD then materialises +// them. Fetch and checkout failures are wrapped with distinct messages so that a +// caller can tell which step failed from the error string alone. +func gitLFSFetchCheckout(ctx context.Context, sh *shell.Shell) error { + fetchArgs := []string{"lfs", "fetch"} + + if err := sh.Command("git", fetchArgs...).Run(ctx); err != nil { + return fmt.Errorf("git lfs fetch: %w", err) + } + + if err := sh.Command("git", "lfs", "checkout").Run(ctx); err != nil { + return fmt.Errorf("git lfs checkout: %w", err) + } + return nil +} + type gitFetchArgs struct { Shell *shell.Shell // The shell to run the command in GitFlags string // Global git flags to pass to the command From c13ebbd3dc20f9497329058f21cb6e55b23803b9 Mon Sep 17 00:00:00 2001 From: lizrabuya <115472349+lizrabuya@users.noreply.github.com> Date: Wed, 13 May 2026 14:33:37 +1000 Subject: [PATCH 2/9] Fix breaking tests when ran on Windows due to restricted PATH --- internal/job/checkout.go | 12 ++-- internal/job/checkout_test.go | 125 +++++++++++++++++++--------------- internal/job/git.go | 25 +++---- 3 files changed, 88 insertions(+), 74 deletions(-) diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 77f162df36..2404ad46a7 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -940,7 +940,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) { } } - // Fail fast before any git work + // Fail fast before any git work if git-lfs is required but missing. if e.GitLFSEnabled { if _, err := exec.LookPath("git-lfs"); err != nil { return fmt.Errorf("BUILDKITE_GIT_LFS_ENABLED=true but git-lfs binary is not found on PATH: %w", err) @@ -959,12 +959,18 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) { return fmt.Errorf("cleaning git repository: %w", err) } - // Install filter before git fetch operations + // Install LFS filter before fetch so the filter is registered before any + // network operation, following the conventional git-lfs setup order. if e.GitLFSEnabled { e.shell.Commentf("Installing Git LFS filter") if err := e.shell.Command("git", "lfs", "install", "--local").Run(ctx); err != nil { return fmt.Errorf("installing git lfs filter: %w", err) } + // Force-set GIT_LFS_SKIP_SMUDGE=1 so checkout writes pointer files to + // disk rather than downloading objects inline. Intentionally not + // restored — git lfs checkout materialises files from cache without + // triggering the smudge filter. + e.shell.Env.Set("GIT_LFS_SKIP_SMUDGE", "1") } if err := e.fetchSource(ctx); err != nil { @@ -1073,8 +1079,6 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) { } if e.GitLFSEnabled { - // gitLFSFetchCheckout returns distinct "git lfs fetch: ..." or - // "git lfs checkout: ..." errors so the failing step is clear from logs. if err := gitLFSFetchCheckout(ctx, e.shell); err != nil { return err } diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index 168bea2adc..547c034644 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -2,9 +2,11 @@ package job import ( "context" + "fmt" "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" "time" @@ -290,18 +292,54 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") - // Note: "LFS enabled" test bypasses GIT_LFS_SKIP_SMUDGE=1 - // setUp sets GIT_LFS_SKIP_SMUDGE=1 to prevent implicit LFS downloads - // during git checkout. These tests call defaultCheckoutPhase directly so they - // don't exercise that env var, but the test repo has no LFS-tracked files so - // smudge filters are never triggered. + // gitOnlyBinDir returns a temp dir containing git (via a symlink on Unix or + // a .bat wrapper on Windows) but no git-lfs, so exec.LookPath("git-lfs") + // will fail while git commands still work. + gitOnlyBinDir := func(t *testing.T) string { + t.Helper() + gitBin, err := exec.LookPath("git") + if err != nil { + t.Fatalf("exec.LookPath(\"git\") error = %v", err) + } + binDir := t.TempDir() + if runtime.GOOS == "windows" { + // Use a .bat wrapper to avoid copying the multi-MB binary and to + // sidestep the symlink-privilege requirement on Windows. + wrapper := fmt.Sprintf("@echo off\r\n\"%s\" %%*\r\n", gitBin) + if err := os.WriteFile(filepath.Join(binDir, "git.bat"), []byte(wrapper), 0o755); err != nil { + t.Fatalf("os.WriteFile() error = %v", err) + } + } else { + if err := os.Symlink(gitBin, filepath.Join(binDir, "git")); err != nil { + t.Fatalf("os.Symlink() error = %v", err) + } + } + return binDir + } + + // fakeLFSBinDir returns a temp dir that has git (via gitOnlyBinDir) plus a + // fake git-lfs whose behaviour is defined by the provided scripts. + // unixScript is a #!/bin/sh script; winBatch is a .bat file body. + fakeLFSBinDir := func(t *testing.T, unixScript, winBatch string) string { + t.Helper() + binDir := gitOnlyBinDir(t) + if runtime.GOOS == "windows" { + if err := os.WriteFile(filepath.Join(binDir, "git-lfs.bat"), []byte(winBatch), 0o755); err != nil { + t.Fatalf("os.WriteFile() error = %v", err) + } + } else { + if err := os.WriteFile(filepath.Join(binDir, "git-lfs"), []byte(unixScript), 0o755); err != nil { + t.Fatalf("os.WriteFile() error = %v", err) + } + } + return binDir + } + tests := []struct { name string lfsEnabled bool - // setupPath runs before the shell and executor are created. Use it to - // manipulate PATH for binary-presence tests. - setupPath func(t *testing.T) - wantErr string + setupPath func(t *testing.T) + wantErr string }{ { name: "LFS disabled", @@ -320,15 +358,7 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { name: "LFS enabled binary missing", lfsEnabled: true, setupPath: func(t *testing.T) { - gitBin, err := exec.LookPath("git") - if err != nil { - t.Fatalf("exec.LookPath(\"git\") error = %v", err) - } - binDir := t.TempDir() - if err := os.Symlink(gitBin, filepath.Join(binDir, "git")); err != nil { - t.Fatalf("os.Symlink() error = %v", err) - } - t.Setenv("PATH", binDir) + t.Setenv("PATH", gitOnlyBinDir(t)) }, wantErr: "git-lfs binary is not found on PATH", }, @@ -336,19 +366,10 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { name: "LFS enabled git lfs command fails", lfsEnabled: true, setupPath: func(t *testing.T) { - gitBin, err := exec.LookPath("git") - if err != nil { - t.Fatalf("exec.LookPath(\"git\") error = %v", err) - } - binDir := t.TempDir() - if err := os.Symlink(gitBin, filepath.Join(binDir, "git")); err != nil { - t.Fatalf("os.Symlink() error = %v", err) - } - fakeLFS := filepath.Join(binDir, "git-lfs") - if err := os.WriteFile(fakeLFS, []byte("#!/bin/sh\nexit 1\n"), 0o755); err != nil { - t.Fatalf("os.WriteFile() error = %v", err) - } - t.Setenv("PATH", binDir) + t.Setenv("PATH", fakeLFSBinDir(t, + "#!/bin/sh\nexit 1\n", + "@echo off\r\nexit /b 1\r\n", + )) }, wantErr: "installing git lfs filter", }, @@ -356,21 +377,10 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { name: "LFS enabled git lfs fetch fails", lfsEnabled: true, setupPath: func(t *testing.T) { - gitBin, err := exec.LookPath("git") - if err != nil { - t.Fatalf("exec.LookPath(\"git\") error = %v", err) - } - binDir := t.TempDir() - if err := os.Symlink(gitBin, filepath.Join(binDir, "git")); err != nil { - t.Fatalf("os.Symlink() error = %v", err) - } - // install exits 0; every other subcommand (fetch, checkout) exits 1. - fakeLFS := filepath.Join(binDir, "git-lfs") - script := "#!/bin/sh\ncase \"$1\" in\n install) exit 0 ;;\n *) exit 1 ;;\nesac\n" - if err := os.WriteFile(fakeLFS, []byte(script), 0o755); err != nil { - t.Fatalf("os.WriteFile() error = %v", err) - } - t.Setenv("PATH", binDir) + t.Setenv("PATH", fakeLFSBinDir(t, + "#!/bin/sh\ncase \"$1\" in\n install) exit 0 ;;\n *) exit 1 ;;\nesac\n", + "@echo off\r\nif \"%1\"==\"install\" exit /b 0\r\nexit /b 1\r\n", + )) }, wantErr: "git lfs fetch", }, @@ -379,8 +389,20 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { s := githttptest.NewServer() t.Cleanup(s.Close) - for _, tt := range tests { + for i, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Set up the remote repository BEFORE restricting PATH so that + // githttptest's git operations use the real git binary. + projectName := fmt.Sprintf("lfs-test-%d", i) + if err := s.CreateRepository(projectName); err != nil { + t.Fatalf("s.CreateRepository(%q) error = %v", projectName, err) + } + out, err := s.InitRepository(projectName) + if err != nil { + t.Fatalf("s.InitRepository(%q) error = %v, output: %s", projectName, err, out) + } + + // Restrict PATH after the repo is initialised. if tt.setupPath != nil { tt.setupPath(t) } @@ -390,15 +412,6 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { t.Fatalf("shell.New() error = %v", err) } - projectName := "lfs-" + strings.ReplaceAll(strings.ToLower(tt.name), " ", "-") - if err := s.CreateRepository(projectName); err != nil { - t.Fatalf("s.CreateRepository(%q) error = %v", projectName, err) - } - out, err := s.InitRepository(projectName) - if err != nil { - t.Fatalf("s.InitRepository(%q) error = %v, output: %s", projectName, err, out) - } - checkoutDir := t.TempDir() sh.Env.Set("BUILDKITE_BUILD_CHECKOUT_PATH", checkoutDir) diff --git a/internal/job/git.go b/internal/job/git.go index 32edf5fc34..297678c6af 100644 --- a/internal/job/git.go +++ b/internal/job/git.go @@ -133,32 +133,29 @@ func gitCleanSubmodules(ctx context.Context, sh *shell.Shell, gitCleanFlags stri return nil } -func gitRepack(ctx context.Context, sh *shell.Shell, args ...string) error { - commandArgs := []string{"repack"} - commandArgs = append(commandArgs, args...) - - if err := sh.Command("git", commandArgs...).Run(ctx); err != nil { - return &gitError{error: err, Type: gitErrorRepack} - } - return nil -} - // gitLFSFetchCheckout fetches LFS objects for the current HEAD then materialises // them. Fetch and checkout failures are wrapped with distinct messages so that a // caller can tell which step failed from the error string alone. func gitLFSFetchCheckout(ctx context.Context, sh *shell.Shell) error { - fetchArgs := []string{"lfs", "fetch"} - - if err := sh.Command("git", fetchArgs...).Run(ctx); err != nil { + if err := sh.Command("git", "lfs", "fetch").Run(ctx); err != nil { return fmt.Errorf("git lfs fetch: %w", err) } - if err := sh.Command("git", "lfs", "checkout").Run(ctx); err != nil { return fmt.Errorf("git lfs checkout: %w", err) } return nil } +func gitRepack(ctx context.Context, sh *shell.Shell, args ...string) error { + commandArgs := []string{"repack"} + commandArgs = append(commandArgs, args...) + + if err := sh.Command("git", commandArgs...).Run(ctx); err != nil { + return &gitError{error: err, Type: gitErrorRepack} + } + return nil +} + type gitFetchArgs struct { Shell *shell.Shell // The shell to run the command in GitFlags string // Global git flags to pass to the command From b609aad53079211f00a9cbed206e772e662306f8 Mon Sep 17 00:00:00 2001 From: lizrabuya <115472349+lizrabuya@users.noreply.github.com> Date: Fri, 22 May 2026 13:22:56 +1000 Subject: [PATCH 3/9] Temporarily disable tests that verify git-lfs commands for windows runner --- internal/job/checkout_test.go | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index 547c034644..405090bf58 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -366,6 +366,13 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { name: "LFS enabled git lfs command fails", lfsEnabled: true, setupPath: func(t *testing.T) { + // Git for Windows ships its own git-lfs.exe inside + // GIT_EXEC_PATH, which git resolves before falling back to + // PATH. We can't fool git's subcommand lookup with a PATH + // override the way we can fool Go's exec.LookPath. + if runtime.GOOS == "windows" { + t.Skip("git for Windows uses bundled git-lfs.exe regardless of PATH") + } t.Setenv("PATH", fakeLFSBinDir(t, "#!/bin/sh\nexit 1\n", "@echo off\r\nexit /b 1\r\n", @@ -377,6 +384,9 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { name: "LFS enabled git lfs fetch fails", lfsEnabled: true, setupPath: func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("git for Windows uses bundled git-lfs.exe regardless of PATH") + } t.Setenv("PATH", fakeLFSBinDir(t, "#!/bin/sh\ncase \"$1\" in\n install) exit 0 ;;\n *) exit 1 ;;\nesac\n", "@echo off\r\nif \"%1\"==\"install\" exit /b 0\r\nexit /b 1\r\n", @@ -389,11 +399,11 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { s := githttptest.NewServer() t.Cleanup(s.Close) - for i, tt := range tests { + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Set up the remote repository BEFORE restricting PATH so that // githttptest's git operations use the real git binary. - projectName := fmt.Sprintf("lfs-test-%d", i) + projectName := "test-" + strings.ReplaceAll(strings.ToLower(tt.name), " ", "-") if err := s.CreateRepository(projectName); err != nil { t.Fatalf("s.CreateRepository(%q) error = %v", projectName, err) } @@ -412,7 +422,24 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { t.Fatalf("shell.New() error = %v", err) } - checkoutDir := t.TempDir() + // Use os.MkdirTemp + best-effort cleanup rather than t.TempDir(): + // on Windows, git's child processes (credential helpers, git-lfs + // filter-process) can hold file handles open past their parent's + // exit, and t.TempDir()'s strict cleanup fails the test. + checkoutDir, err := os.MkdirTemp("", "checkout-path-") + if err != nil { + t.Fatalf("os.MkdirTemp() error = %v", err) + } + t.Cleanup(func() { + os.RemoveAll(checkoutDir) //nolint:errcheck // Best-effort cleanup. + }) + buildDir, err := os.MkdirTemp("", "build-path-") + if err != nil { + t.Fatalf("os.MkdirTemp() error = %v", err) + } + t.Cleanup(func() { + os.RemoveAll(buildDir) //nolint:errcheck // Best-effort cleanup. + }) sh.Env.Set("BUILDKITE_BUILD_CHECKOUT_PATH", checkoutDir) executor := &Executor{ @@ -421,7 +448,7 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { Commit: "HEAD", Branch: "main", GitCleanFlags: "-f -d -x", - BuildPath: t.TempDir(), + BuildPath: buildDir, Repository: s.RepoURL(projectName), GitLFSEnabled: tt.lfsEnabled, }, From 5a4e6d662e16ace4aa844a3926c31507c820a2d6 Mon Sep 17 00:00:00 2001 From: lizrabuya <115472349+lizrabuya@users.noreply.github.com> Date: Fri, 22 May 2026 13:39:19 +1000 Subject: [PATCH 4/9] Modify checkout dir cleanup to be more robust when tests are ran on windows --- internal/job/checkout_test.go | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index 405090bf58..890eaddf46 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -366,13 +366,6 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { name: "LFS enabled git lfs command fails", lfsEnabled: true, setupPath: func(t *testing.T) { - // Git for Windows ships its own git-lfs.exe inside - // GIT_EXEC_PATH, which git resolves before falling back to - // PATH. We can't fool git's subcommand lookup with a PATH - // override the way we can fool Go's exec.LookPath. - if runtime.GOOS == "windows" { - t.Skip("git for Windows uses bundled git-lfs.exe regardless of PATH") - } t.Setenv("PATH", fakeLFSBinDir(t, "#!/bin/sh\nexit 1\n", "@echo off\r\nexit /b 1\r\n", @@ -384,9 +377,6 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { name: "LFS enabled git lfs fetch fails", lfsEnabled: true, setupPath: func(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("git for Windows uses bundled git-lfs.exe regardless of PATH") - } t.Setenv("PATH", fakeLFSBinDir(t, "#!/bin/sh\ncase \"$1\" in\n install) exit 0 ;;\n *) exit 1 ;;\nesac\n", "@echo off\r\nif \"%1\"==\"install\" exit /b 0\r\nexit /b 1\r\n", @@ -422,16 +412,19 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { t.Fatalf("shell.New() error = %v", err) } - // Use os.MkdirTemp + best-effort cleanup rather than t.TempDir(): - // on Windows, git's child processes (credential helpers, git-lfs - // filter-process) can hold file handles open past their parent's - // exit, and t.TempDir()'s strict cleanup fails the test. - checkoutDir, err := os.MkdirTemp("", "checkout-path-") - if err != nil { - t.Fatalf("os.MkdirTemp() error = %v", err) - } + checkoutDir := t.TempDir() t.Cleanup(func() { - os.RemoveAll(checkoutDir) //nolint:errcheck // Best-effort cleanup. + if runtime.GOOS != "windows" { + return // t.TempDir handles it + } + // Best-effort: try git lfs uninstall to release filter handles, + // then retry RemoveAll a few times. + for i := 0; i < 10; i++ { + if err := os.RemoveAll(checkoutDir); err == nil { + return + } + time.Sleep(100 * time.Millisecond) + } }) buildDir, err := os.MkdirTemp("", "build-path-") if err != nil { From 6e494546b3dd6dff9577f947e1ff901c99e7978d Mon Sep 17 00:00:00 2001 From: lizrabuya <115472349+lizrabuya@users.noreply.github.com> Date: Fri, 22 May 2026 13:55:59 +1000 Subject: [PATCH 5/9] Need to skip test for git-lfs command_fails & fetch_fails because they are not runnable on windows --- internal/job/checkout_test.go | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index 890eaddf46..0d38655118 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -366,6 +366,13 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { name: "LFS enabled git lfs command fails", lfsEnabled: true, setupPath: func(t *testing.T) { + // Git for Windows ships its own git-lfs.exe inside + // GIT_EXEC_PATH, which git resolves before falling back to + // PATH. We can't fool git's subcommand lookup with a PATH + // override the way we can fool Go's exec.LookPath. + if runtime.GOOS == "windows" { + t.Skip("Not runnable on Windows: git for Windows uses bundled git-lfs.exe regardless of PATH") + } t.Setenv("PATH", fakeLFSBinDir(t, "#!/bin/sh\nexit 1\n", "@echo off\r\nexit /b 1\r\n", @@ -377,6 +384,9 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { name: "LFS enabled git lfs fetch fails", lfsEnabled: true, setupPath: func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Not runnable on Windows: git for Windows uses bundled git-lfs.exe regardless of PATH") + } t.Setenv("PATH", fakeLFSBinDir(t, "#!/bin/sh\ncase \"$1\" in\n install) exit 0 ;;\n *) exit 1 ;;\nesac\n", "@echo off\r\nif \"%1\"==\"install\" exit /b 0\r\nexit /b 1\r\n", @@ -412,19 +422,16 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { t.Fatalf("shell.New() error = %v", err) } - checkoutDir := t.TempDir() + // Use os.MkdirTemp + best-effort cleanup rather than t.TempDir(): + // on Windows, git's child processes (credential helpers, git-lfs + // filter-process) can hold file handles open past their parent's + // exit, and t.TempDir()'s strict cleanup fails the test. + checkoutDir, err := os.MkdirTemp("", "checkout-path-") + if err != nil { + t.Fatalf("os.MkdirTemp() error = %v", err) + } t.Cleanup(func() { - if runtime.GOOS != "windows" { - return // t.TempDir handles it - } - // Best-effort: try git lfs uninstall to release filter handles, - // then retry RemoveAll a few times. - for i := 0; i < 10; i++ { - if err := os.RemoveAll(checkoutDir); err == nil { - return - } - time.Sleep(100 * time.Millisecond) - } + os.RemoveAll(checkoutDir) //nolint:errcheck // Best-effort cleanup. }) buildDir, err := os.MkdirTemp("", "build-path-") if err != nil { From 90cf458936485a1bae427dbe0f32a0adf3cae68d Mon Sep 17 00:00:00 2001 From: lizrabuya <115472349+lizrabuya@users.noreply.github.com> Date: Fri, 22 May 2026 14:33:53 +1000 Subject: [PATCH 6/9] Add retrier to git lfs fetch+checkout --- internal/job/checkout.go | 5 ++++- internal/job/git.go | 47 ++++++++++++++++++++++++++++++++++------ internal/job/git_test.go | 28 ++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 8 deletions(-) diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 2404ad46a7..5acb871bde 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -1079,7 +1079,10 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) { } if e.GitLFSEnabled { - if err := gitLFSFetchCheckout(ctx, e.shell); err != nil { + if err := gitLFSFetchCheckout(ctx, gitLFSFetchCheckoutArgs{ + Shell: e.shell, + Retry: true, + }); err != nil { return err } } diff --git a/internal/job/git.go b/internal/job/git.go index 297678c6af..eebc47a41b 100644 --- a/internal/job/git.go +++ b/internal/job/git.go @@ -133,17 +133,50 @@ func gitCleanSubmodules(ctx context.Context, sh *shell.Shell, gitCleanFlags stri return nil } +type gitLFSFetchCheckoutArgs struct { + Shell *shell.Shell + Retry bool // Whether to retry the fetch+checkout on failure +} + // gitLFSFetchCheckout fetches LFS objects for the current HEAD then materialises // them. Fetch and checkout failures are wrapped with distinct messages so that a // caller can tell which step failed from the error string alone. -func gitLFSFetchCheckout(ctx context.Context, sh *shell.Shell) error { - if err := sh.Command("git", "lfs", "fetch").Run(ctx); err != nil { - return fmt.Errorf("git lfs fetch: %w", err) - } - if err := sh.Command("git", "lfs", "checkout").Run(ctx); err != nil { - return fmt.Errorf("git lfs checkout: %w", err) +// +// When args.Retry is true, the fetch+checkout pair is retried with exponential +// backoff to ride out transient network errors talking to the LFS server. +// Unlike gitFetch, we don't smelt for specific error strings: git-lfs uses +// different exit codes and error vocabulary than git itself, so we retry +// indiscriminately on any failure and rely on the retry budget to bound the +// damage from a genuinely permanent error. +func gitLFSFetchCheckout(ctx context.Context, args gitLFSFetchCheckoutArgs) error { + retrier := roko.NewRetrier( + roko.WithStrategy(roko.Constant(0)), + roko.WithMaxAttempts(1), + ) + + if args.Retry { + retrier = roko.NewRetrier( + roko.WithStrategy(roko.ExponentialSubsecond(1*time.Second)), + roko.WithMaxAttempts(10), // 10 attempts will take ~2 minutes 17s + roko.WithJitter(), + ) } - return nil + + return retrier.DoWithContext(ctx, func(retrier *roko.Retrier) error { + if err := args.Shell.Command("git", "lfs", "fetch").Run(ctx); err != nil { + if args.Retry { + args.Shell.Commentf("%s", retrier) + } + return fmt.Errorf("git lfs fetch: %w", err) + } + if err := args.Shell.Command("git", "lfs", "checkout").Run(ctx); err != nil { + if args.Retry { + args.Shell.Commentf("%s", retrier) + } + return fmt.Errorf("git lfs checkout: %w", err) + } + return nil + }) } func gitRepack(ctx context.Context, sh *shell.Shell, args ...string) error { diff --git a/internal/job/git_test.go b/internal/job/git_test.go index 2cf6b62df8..077b06ed8d 100644 --- a/internal/job/git_test.go +++ b/internal/job/git_test.go @@ -311,3 +311,31 @@ func TestGitFetch(t *testing.T) { t.Errorf("executed commands diff (-got +want):\n%s", diff) } } + +func TestGitLFSFetchCheckout(t *testing.T) { + t.Parallel() + ctx := context.Background() + + var gotLog [][]string + sh := shell.NewTestShell(t, shell.WithDryRun(true), shell.WithCommandLog(&gotLog)) + + absoluteGit, err := sh.AbsolutePath("git") + if err != nil { + t.Fatalf("sh.AbsolutePath(git) = %v", err) + } + + if err := gitLFSFetchCheckout(ctx, gitLFSFetchCheckoutArgs{ + Shell: sh, + Retry: true, + }); err != nil { + t.Fatalf("gitLFSFetchCheckout(ctx, ...) = %v", err) + } + + wantLog := [][]string{ + {absoluteGit, "lfs", "fetch"}, + {absoluteGit, "lfs", "checkout"}, + } + if diff := cmp.Diff(gotLog, wantLog); diff != "" { + t.Errorf("executed commands diff (-got +want):\n%s", diff) + } +} From 9a649dd60fd4d51cf9ee6e5051e350a4ad8da376 Mon Sep 17 00:00:00 2001 From: lizrabuya <115472349+lizrabuya@users.noreply.github.com> Date: Fri, 5 Jun 2026 12:39:51 +1000 Subject: [PATCH 7/9] Improve the retry mechanism for git LFS checkout --- internal/job/git.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/internal/job/git.go b/internal/job/git.go index eebc47a41b..c68f64c291 100644 --- a/internal/job/git.go +++ b/internal/job/git.go @@ -33,6 +33,9 @@ const ( gitErrorClean gitErrorCleanSubmodules gitErrorRepack + // LFS fetch or checkout failure; distinct from gitErrorFetch because the + // gitFetch retry-clean/bad-object recovery paths don't apply to LFS. + gitErrorLFS ) const ( @@ -148,6 +151,11 @@ type gitLFSFetchCheckoutArgs struct { // different exit codes and error vocabulary than git itself, so we retry // indiscriminately on any failure and rely on the retry budget to bound the // damage from a genuinely permanent error. +// +// On exhaustion, the error is wrapped as a *gitError with WasRetried=true so +// that the outer checkout retrier in defaultCheckoutPhase's caller does not +// loop on top of this one — without that signal, a permanent LFS failure +// could be attempted 6 × 5 = 30 times instead of 5. func gitLFSFetchCheckout(ctx context.Context, args gitLFSFetchCheckoutArgs) error { retrier := roko.NewRetrier( roko.WithStrategy(roko.Constant(0)), @@ -157,12 +165,12 @@ func gitLFSFetchCheckout(ctx context.Context, args gitLFSFetchCheckoutArgs) erro if args.Retry { retrier = roko.NewRetrier( roko.WithStrategy(roko.ExponentialSubsecond(1*time.Second)), - roko.WithMaxAttempts(10), // 10 attempts will take ~2 minutes 17s + roko.WithMaxAttempts(5), // 5 attempts will take ~16s roko.WithJitter(), ) } - return retrier.DoWithContext(ctx, func(retrier *roko.Retrier) error { + err := retrier.DoWithContext(ctx, func(retrier *roko.Retrier) error { if err := args.Shell.Command("git", "lfs", "fetch").Run(ctx); err != nil { if args.Retry { args.Shell.Commentf("%s", retrier) @@ -177,6 +185,11 @@ func gitLFSFetchCheckout(ctx context.Context, args gitLFSFetchCheckoutArgs) erro } return nil }) + + if err != nil && args.Retry { + return &gitError{error: err, Type: gitErrorLFS, WasRetried: true} + } + return err } func gitRepack(ctx context.Context, sh *shell.Shell, args ...string) error { From caa99c845ddbb59f0049531cd786a5c4ef37f0aa Mon Sep 17 00:00:00 2001 From: lizrabuya <115472349+lizrabuya@users.noreply.github.com> Date: Mon, 15 Jun 2026 15:38:21 +1000 Subject: [PATCH 8/9] Update based on PR comments --- internal/job/checkout.go | 20 +++++++----------- internal/job/checkout_test.go | 40 ++++++++++++++++++++++++++++------- 2 files changed, 40 insertions(+), 20 deletions(-) diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 5acb871bde..954cb019eb 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -263,6 +263,14 @@ func (e *Executor) checkout(ctx context.Context) error { break } + // Fail fast before any git work if git-lfs is required but missing. + // This operation only handles default checkout behavior, so it's possible for a custom checkout hook to require git-lfs but not have this check. That's a bit unfortunate, but we can add it to custom hooks later if needed. + if e.GitLFSEnabled { + if _, err := exec.LookPath("git-lfs"); err != nil { + return fmt.Errorf("BUILDKITE_GIT_LFS_ENABLED=true but git-lfs binary is not found on PATH: %w", err) + } + } + maxAttempts := e.CheckoutAttempts if maxAttempts <= 0 { maxAttempts = 6 @@ -940,13 +948,6 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) { } } - // Fail fast before any git work if git-lfs is required but missing. - if e.GitLFSEnabled { - if _, err := exec.LookPath("git-lfs"); err != nil { - return fmt.Errorf("BUILDKITE_GIT_LFS_ENABLED=true but git-lfs binary is not found on PATH: %w", err) - } - } - // Git clean prior to checkout, we do this even if submodules have been // disabled to ensure previous submodules are cleaned up if hasGitSubmodules(e.shell) { @@ -966,11 +967,6 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) { if err := e.shell.Command("git", "lfs", "install", "--local").Run(ctx); err != nil { return fmt.Errorf("installing git lfs filter: %w", err) } - // Force-set GIT_LFS_SKIP_SMUDGE=1 so checkout writes pointer files to - // disk rather than downloading objects inline. Intentionally not - // restored — git lfs checkout materialises files from cache without - // triggering the smudge filter. - e.shell.Env.Set("GIT_LFS_SKIP_SMUDGE", "1") } if err := e.fetchSource(ctx); err != nil { diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index 0d38655118..d6d3ed11c6 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -282,6 +282,38 @@ func TestDefaultCheckoutPhase_DelayedRefCreation(t *testing.T) { } } +func TestGitLFSBinaryMissing(t *testing.T) { + // Not parallel: the test manipulates PATH via t.Setenv, which modifies + // process-global state. + + ctx := context.Background() + + // Restrict PATH so exec.LookPath("git-lfs") fails. An empty temp dir + // suffices — the fail-fast check returns before any git call. + t.Setenv("PATH", t.TempDir()) + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v, want nil", err) + } + + executor := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + Repository: "https://github.com/buildkite/agent.git", + GitLFSEnabled: true, + }, + } + + err = executor.checkout(ctx) + if err == nil { + t.Fatalf("executor.checkout(ctx) error = nil, want error containing %q", "git-lfs binary is not found on PATH") + } + if !strings.Contains(err.Error(), "git-lfs binary is not found on PATH") { + t.Errorf("executor.checkout(ctx) error = %q, want it to contain %q", err.Error(), "git-lfs binary is not found on PATH") + } +} + func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { // Not parallel: subtests manipulate PATH via t.Setenv, which modifies // process-global state. @@ -354,14 +386,6 @@ func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { } }, }, - { - name: "LFS enabled binary missing", - lfsEnabled: true, - setupPath: func(t *testing.T) { - t.Setenv("PATH", gitOnlyBinDir(t)) - }, - wantErr: "git-lfs binary is not found on PATH", - }, { name: "LFS enabled git lfs command fails", lfsEnabled: true, From 9df1a63ea704a5c94dbd631ec14251d7b913e25b Mon Sep 17 00:00:00 2001 From: lizrabuya <115472349+lizrabuya@users.noreply.github.com> Date: Tue, 16 Jun 2026 09:38:10 +1000 Subject: [PATCH 9/9] Fix lint issues --- internal/job/checkout_test.go | 5 ++--- internal/job/git_test.go | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index 19e42e7664..dd4c85d2a8 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -1,7 +1,6 @@ package job import ( - "context" "fmt" "os" "os/exec" @@ -451,7 +450,7 @@ func TestGitLFSBinaryMissing(t *testing.T) { // Not parallel: the test manipulates PATH via t.Setenv, which modifies // process-global state. - ctx := context.Background() + ctx := t.Context() // Restrict PATH so exec.LookPath("git-lfs") fails. An empty temp dir // suffices — the fail-fast check returns before any git call. @@ -482,7 +481,7 @@ func TestGitLFSBinaryMissing(t *testing.T) { func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { // Not parallel: subtests manipulate PATH via t.Setenv, which modifies // process-global state. - ctx := context.Background() + ctx := t.Context() t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") diff --git a/internal/job/git_test.go b/internal/job/git_test.go index 44455155f4..c12c8d4a8c 100644 --- a/internal/job/git_test.go +++ b/internal/job/git_test.go @@ -313,7 +313,7 @@ func TestGitFetch(t *testing.T) { func TestGitLFSFetchCheckout(t *testing.T) { t.Parallel() - ctx := context.Background() + ctx := t.Context() var gotLog [][]string sh := shell.NewTestShell(t, shell.WithDryRun(true), shell.WithCommandLog(&gotLog))