diff --git a/core/git/git.go b/core/git/git.go index 043f8a37..610b2972 100644 --- a/core/git/git.go +++ b/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,12 +90,35 @@ 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. @@ -92,7 +126,7 @@ func (c *impl) Fetch(ctx context.Context, remote, ref string, options ...string) 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,7 +135,7 @@ 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. @@ -109,14 +143,16 @@ func (c *impl) Diff(ctx context.Context, baseRef, targetRef string, options ...s 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,7 +184,7 @@ 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. @@ -156,7 +192,7 @@ 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, diff --git a/core/git/git_test.go b/core/git/git_test.go index 5514593d..b1f87bff 100644 --- a/core/git/git_test.go +++ b/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} diff --git a/orchestrator/errors.go b/orchestrator/errors.go index 1a422946..df2eee23 100644 --- a/orchestrator/errors.go +++ b/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 +} diff --git a/orchestrator/errors_test.go b/orchestrator/errors_test.go index ea8881f5..29ef6539 100644 --- a/orchestrator/errors_test.go +++ b/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() diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index 26d744c7..0d454454 100644 --- a/orchestrator/native_orchestrator.go +++ b/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 {