feat(git): classify git command timeouts as infra errors

core/git/git.go

@@ -36,6 +36,17 @@ const (
 	_gitTimeout = 10 * time.Minute
 )
 
+// ErrFatal is returned (wrapped) when a git command exits with a fatal (128)
+// or usage (129) exit code -- e.g. an unreachable remote, failed
+// authentication, an invalid repository, or a malformed invocation -- as
+// opposed to a non-fatal, conditional exit code such as 1. Callers can check
+// for it with errors.Is.
+var ErrFatal = errors.New("git command failed fatally")
+
+// ErrTimeout is returned (wrapped) when a git command does not complete
+// within the configured timeout. Callers can check for it with errors.Is.
+var ErrTimeout = errors.New("git command timed out")
+
 // DiffEntry represents a single file change from git diff --name-status.
 type DiffEntry struct {
 	// Status is the single-character status code: "A", "D", "M", "R", etc.
@@ -79,20 +90,43 @@ func New(directory string, logger *zap.SugaredLogger) Interface {
 	}
 }
 
+// wrapError wraps a non-nil err with the failing git command's arguments,
+// additionally wrapping ErrFatal if the command exited with a fatal (128) or
+// usage (129) exit code, as opposed to a non-fatal, conditional exit code
+// such as 1, and ErrTimeout if ctx's own timeout (rather than a parent
+// cancellation) has elapsed.
+func wrapError(ctx context.Context, args []string, err error) error {
+	if err == nil {
+		return nil
+	}
+	var exitErr *exec.ExitError
+	if errors.As(err, &exitErr) && isFatalExitCode(exitErr.ExitCode()) {
+		err = fmt.Errorf("%w: %w", ErrFatal, err)
+	}
+	if ctx.Err() == context.DeadlineExceeded {
+		err = fmt.Errorf("%w: %w", ErrTimeout, err)
+	}
+	return fmt.Errorf("git %s: %w", strings.Join(args, " "), err)
+}
+
+func isFatalExitCode(code int) bool {
+	return code == 128 || code == 129
+}
+
 // Checkout checks out a specific reference in the repository.
 func (c *impl) Checkout(ctx context.Context, ref string, options ...string) error {
 	ctx, cancel := context.WithTimeout(ctx, _gitTimeout)
 	defer cancel()
 	args := append([]string{"checkout", ref}, options...)
-	return c.runner.run(ctx, c.directory, "git", args...)
+	return wrapError(ctx, args, c.runner.run(ctx, c.directory, "git", args...))
 }
 
 // Fetch runs git fetch for a remote ref.
 func (c *impl) Fetch(ctx context.Context, remote, ref string, options ...string) error {
 	ctx, cancel := context.WithTimeout(ctx, _gitTimeout)
 	defer cancel()
 	args := append([]string{"fetch", remote, ref}, options...)
-	return c.runner.run(ctx, c.directory, "git", args...)
+	return wrapError(ctx, args, c.runner.run(ctx, c.directory, "git", args...))
 }
 
 // Clone clones the target repository to the destination.
@@ -101,22 +135,24 @@ func (c *impl) Clone(ctx context.Context, target, destination string, options ..
 	ctx, cancel := context.WithTimeout(ctx, _gitTimeout)
 	defer cancel()
 	args := append(append([]string{"clone"}, options...), target, destination)
-	return c.runner.run(ctx, c.directory, "git", args...)
+	return wrapError(ctx, args, c.runner.run(ctx, c.directory, "git", args...))
 }
 
 // Diff returns the diff between two references.
 func (c *impl) Diff(ctx context.Context, baseRef, targetRef string, options ...string) ([]byte, error) {
 	ctx, cancel := context.WithTimeout(ctx, _gitTimeout)
 	defer cancel()
 	args := append([]string{"diff", baseRef, targetRef}, options...)
-	return c.runner.output(ctx, c.directory, "git", args...)
+	out, err := c.runner.output(ctx, c.directory, "git", args...)
+	return out, wrapError(ctx, args, err)
 }
 
 // ApplyPatch applies a patch to the repository.
 func (c *impl) ApplyPatch(ctx context.Context, patch []byte) error {
 	ctx, cancel := context.WithTimeout(ctx, _gitTimeout)
 	defer cancel()
-	return c.runner.runWithStdin(ctx, c.directory, "git", patch, "apply", "--3way", "--whitespace", "nowarn", "--index", "-")
+	args := []string{"apply", "--3way", "--whitespace", "nowarn", "--index", "-"}
+	return wrapError(ctx, args, c.runner.runWithStdin(ctx, c.directory, "git", patch, args...))
 }
 
 // RevParse returns the revision hash of a reference.
@@ -148,15 +184,15 @@ func (c *impl) Commit(ctx context.Context, message string, options ...string) er
 	ctx, cancel := context.WithTimeout(ctx, _gitTimeout)
 	defer cancel()
 	args := append([]string{"commit", "-am", message}, options...)
-	return c.runner.run(ctx, c.directory, "git", args...)
+	return wrapError(ctx, args, c.runner.run(ctx, c.directory, "git", args...))
 }
 
 // SubmoduleUpdate updates the submodules in the repository.
 func (c *impl) SubmoduleUpdate(ctx context.Context) error {
 	ctx, cancel := context.WithTimeout(ctx, _gitTimeout)
 	defer cancel()
 	args := []string{"submodule", "update", "--init", "--recursive"}
-	return c.runner.run(ctx, c.directory, "git", args...)
+	return wrapError(ctx, args, c.runner.run(ctx, c.directory, "git", args...))
 }
 
 // DiffWithStatus returns the list of changed files with their status between two refs,

core/git/git_test.go

@@ -17,8 +17,11 @@ package git
 import (
 	"context"
 	"errors"
+	"fmt"
+	"os/exec"
 	"reflect"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -37,21 +40,86 @@ type mockRunner struct {
 	calls []runnerCall
 	out   []byte
 	err   error
+	// returnCtxErr, if true, makes run/output/runWithStdin return ctx.Err()
+	// instead of err — simulating what an exec-based runner returns when
+	// its command is killed by a canceled/expired context.
+	returnCtxErr bool
 }
 
-func (m *mockRunner) run(_ context.Context, dir string, name string, args ...string) error {
+func (m *mockRunner) run(ctx context.Context, dir string, name string, args ...string) error {
 	m.calls = append(m.calls, runnerCall{kind: "run", dir: dir, name: name, args: append([]string(nil), args...)})
+	if m.returnCtxErr {
+		return ctx.Err()
+	}
 	return m.err
 }
-func (m *mockRunner) output(_ context.Context, dir string, name string, args ...string) ([]byte, error) {
+func (m *mockRunner) output(ctx context.Context, dir string, name string, args ...string) ([]byte, error) {
 	m.calls = append(m.calls, runnerCall{kind: "output", dir: dir, name: name, args: append([]string(nil), args...)})
+	if m.returnCtxErr {
+		return nil, ctx.Err()
+	}
 	return append([]byte(nil), m.out...), m.err
 }
-func (m *mockRunner) runWithStdin(_ context.Context, dir string, name string, stdin []byte, args ...string) error {
+func (m *mockRunner) runWithStdin(ctx context.Context, dir string, name string, stdin []byte, args ...string) error {
 	m.calls = append(m.calls, runnerCall{kind: "runWithStdin", dir: dir, name: name, args: append([]string(nil), args...), stdin: append([]byte(nil), stdin...)})
+	if m.returnCtxErr {
+		return ctx.Err()
+	}
 	return m.err
 }
 
+func TestCheckout_wrapsErrTimeoutWhenTimeoutElapses(t *testing.T) {
+	// A parent deadline already in the past causes the derived timeout
+	// context to be immediately Done with context.DeadlineExceeded, before
+	// the runner is even invoked.
+	ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Hour))
+	defer cancel()
+	m := &mockRunner{returnCtxErr: true}
+	g := &impl{directory: "/repo", runner: m}
+	err := g.Checkout(ctx, "feature")
+	require.Error(t, err)
+	assert.ErrorIs(t, err, ErrTimeout)
+	assert.ErrorIs(t, err, context.DeadlineExceeded)
+}
+
+func TestCheckout_doesNotWrapErrTimeoutOnParentCancellation(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+	m := &mockRunner{returnCtxErr: true}
+	g := &impl{directory: "/repo", runner: m}
+	err := g.Checkout(ctx, "feature")
+	require.Error(t, err)
+	assert.NotErrorIs(t, err, ErrTimeout)
+	assert.ErrorIs(t, err, context.Canceled)
+}
+
+func TestWrapError_marksFatalExitCodesAsErrFatal(t *testing.T) {
+	tests := []struct {
+		name      string
+		exitCode  int
+		wantFatal bool
+	}{
+		{name: "fatal error exit code", exitCode: 128, wantFatal: true},
+		{name: "usage error exit code", exitCode: 129, wantFatal: true},
+		{name: "generic/conditional exit code", exitCode: 1, wantFatal: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			runErr := exec.Command("sh", "-c", fmt.Sprintf("exit %d", tt.exitCode)).Run()
+			require.Error(t, runErr)
+
+			err := wrapError(context.Background(), []string{"checkout", "feature"}, runErr)
+			require.Error(t, err)
+			if tt.wantFatal {
+				assert.ErrorIs(t, err, ErrFatal)
+			} else {
+				assert.NotErrorIs(t, err, ErrFatal)
+			}
+		})
+	}
+}
+
 func TestClone_usesRunnerWithDirAndArgs(t *testing.T) {
 	m := &mockRunner{}
 	g := &impl{directory: "/repo", runner: m}

orchestrator/errors.go

@@ -19,6 +19,7 @@ import (
 	"fmt"
 
 	tangoerrors "github.com/uber/tango/core/errors"
+	"github.com/uber/tango/core/git"
 	"github.com/uber/tango/core/repomanager"
 )
 
@@ -29,3 +30,14 @@ func classifyLeaseError(err error) error {
 	}
 	return tangoerrors.NewInfra(wrappedErr)
 }
+
+// classifyGitError classifies a git-command failure as an infra error when
+// it was caused by a git command timing out (git.ErrTimeout) or exiting
+// with a fatal or usage error (git.ErrFatal). Other git failures are
+// returned unclassified.
+func classifyGitError(err error) error {
+	if errors.Is(err, git.ErrTimeout) || errors.Is(err, git.ErrFatal) {
+		return tangoerrors.NewInfra(err)
+	}
+	return err
+}

orchestrator/errors_test.go

@@ -22,9 +22,57 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	tangoerrors "github.com/uber/tango/core/errors"
+	"github.com/uber/tango/core/git"
 	"github.com/uber/tango/core/repomanager"
 )
 
+func TestClassifyGitError(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		err      error
+		wantCode tangoerrors.ErrorCode
+	}{
+		{
+			name:     "generic error is unclassified but defaults to infra",
+			err:      fmt.Errorf("exit status 1"),
+			wantCode: tangoerrors.ErrorInfra,
+		},
+		{
+			name:     "context cancellation is reported as cancelled despite unclassified wrapping",
+			err:      fmt.Errorf("checkout: %w", context.Canceled),
+			wantCode: tangoerrors.ErrorCancelled,
+		},
+		{
+			name:     "bare context deadline exceeded is unclassified but defaults to infra",
+			err:      fmt.Errorf("checkout: %w", context.DeadlineExceeded),
+			wantCode: tangoerrors.ErrorInfra,
+		},
+		{
+			name:     "git timeout is infra",
+			err:      fmt.Errorf("checkout: %w", git.ErrTimeout),
+			wantCode: tangoerrors.ErrorInfra,
+		},
+		{
+			name:     "fatal git exit code is infra",
+			err:      fmt.Errorf("checkout: %w", git.ErrFatal),
+			wantCode: tangoerrors.ErrorInfra,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			got := classifyGitError(tt.err)
+			require.Error(t, got)
+			assert.Equal(t, tt.wantCode, tangoerrors.GetErrorCode(got))
+			assert.ErrorIs(t, got, tt.err)
+		})
+	}
+}
+
 func TestClassifyLeaseError(t *testing.T) {
 	t.Parallel()
 

orchestrator/native_orchestrator.go

@@ -138,7 +138,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetT
 	err = ws.Checkout(ctx, build.Remote, build.BaseSha)
 	recordStep(e, "checkout_duration", checkoutStart)
 	if err != nil {
-		return nil, fmt.Errorf("checkout %s@%s: %w", build.Remote, build.BaseSha, err)
+		return nil, classifyGitError(err)
 	}
 	logger.Infow("GetTargetGraph: Checked out base revision")
 
@@ -160,14 +160,14 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, req entity.GetT
 	err = ws.ApplyRequests(ctx, requests)
 	recordStep(e, "apply_requests_duration", applyStart)
 	if err != nil {
-		return nil, fmt.Errorf("apply requests: %w", err)
+		return nil, classifyGitError(err)
 	}
 	logger.Infow("GetTargetGraph: Applied requests", zap.Int("request_count", len(requests)))
 
 	// Compute the treehash and download the target graph from storage if exists.
 	treehash, err := gitModule.RevParse(ctx, "HEAD^{tree}")
 	if err != nil {
-		return nil, fmt.Errorf("compute treehash: %w", err)
+		return nil, classifyGitError(err)
 	}
 	treehashPath := cachekey.GetGraphByTreeHash(build.Remote, treehash, build.Strategy, req.ExcludeFilesRegex)
 	if !req.BypassCache {