From 20ea3ec2ef5e55e8ea4bd17f6652cc8e7aac077d Mon Sep 17 00:00:00 2001 From: Tmwakalasya Date: Tue, 6 Oct 2026 00:21:21 -0400 Subject: [PATCH] fix(git): preserve repository errors for orchestrator retries --- platform/git/repo/repo.go | 4 +- platform/git/repo/repo_test.go | 68 +++++++++++++++++++ .../orchestrator/server/BUILD.bazel | 2 + .../submitqueue/orchestrator/server/main.go | 2 + .../orchestrator/server/main_test.go | 62 +++++++++++++++++ 5 files changed, 136 insertions(+), 2 deletions(-) diff --git a/platform/git/repo/repo.go b/platform/git/repo/repo.go index f0aa88f07..bbed86167 100644 --- a/platform/git/repo/repo.go +++ b/platform/git/repo/repo.go @@ -247,7 +247,7 @@ func (r *Repo) outputOf(ctx context.Context, args ...string) (string, error) { if message == "" { message = err.Error() } - return "", fmt.Errorf("git %s: %s", strings.Join(args, " "), message) + return "", fmt.Errorf("git %s: %w", strings.Join(args, " "), gitexec.CommandFailure(ctx, args, message, err)) } return string(out), nil } @@ -269,7 +269,7 @@ func SetConfig(ctx context.Context, path, key, value string) error { if message == "" { message = err.Error() } - return fmt.Errorf("git config %s: %s", key, message) + return fmt.Errorf("git config %s: %w", key, gitexec.CommandFailure(ctx, []string{"config"}, message, err)) } return nil } diff --git a/platform/git/repo/repo_test.go b/platform/git/repo/repo_test.go index abc5287c9..07b51c674 100644 --- a/platform/git/repo/repo_test.go +++ b/platform/git/repo/repo_test.go @@ -17,6 +17,7 @@ package gitrepo import ( "context" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -114,3 +115,70 @@ func TestNewRepo_RejectsAnIncompleteConfiguration(t *testing.T) { }) } } + +func TestRunRaw_PreservesCommandFailure(t *testing.T) { + repo, err := NewRepo(RepoConfig{ + Git: gitexectest.Git(t), + Path: t.TempDir(), + RemoteURL: "unused", + Target: "main", + }) + require.NoError(t, err) + + _, err = repo.RunRaw(context.Background(), "not-a-git-command") + + var commandErr *gitexec.CommandError + require.ErrorAs(t, err, &commandErr) + assert.Equal(t, "not-a-git-command", commandErr.Operation()) + var exitErr *exec.ExitError + assert.ErrorAs(t, err, &exitErr) +} + +func TestSetConfig_PreservesCommandFailure(t *testing.T) { + t.Setenv("GIT_EXECUTABLE", gitexectest.Git(t)) + err := SetConfig(context.Background(), t.TempDir(), "user.name", "Test") + + var commandErr *gitexec.CommandError + require.ErrorAs(t, err, &commandErr) + assert.Equal(t, "config", commandErr.Operation()) + var exitErr *exec.ExitError + assert.ErrorAs(t, err, &exitErr) +} + +func TestCommands_PreserveCancellation(t *testing.T) { + git := gitexectest.Git(t) + t.Setenv("GIT_EXECUTABLE", git) + repo, err := NewRepo(RepoConfig{ + Git: git, + Path: t.TempDir(), + RemoteURL: "unused", + Target: "main", + }) + require.NoError(t, err) + + for _, tt := range []struct { + name string + run func(context.Context) error + }{ + { + name: "raw command", + run: func(ctx context.Context) error { + _, err := repo.RunRaw(ctx, "status") + return err + }, + }, + { + name: "set config", + run: func(ctx context.Context) error { + return SetConfig(ctx, repo.cfg.Path, "user.name", "Test") + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + assert.ErrorIs(t, tt.run(ctx), context.Canceled) + }) + } +} diff --git a/service/submitqueue/orchestrator/server/BUILD.bazel b/service/submitqueue/orchestrator/server/BUILD.bazel index a1ec263bc..5bd25680f 100644 --- a/service/submitqueue/orchestrator/server/BUILD.bazel +++ b/service/submitqueue/orchestrator/server/BUILD.bazel @@ -21,6 +21,7 @@ go_library( "//platform/buildkite:go_default_library", "//platform/errs:go_default_library", "//platform/errs/generic:go_default_library", + "//platform/errs/git:go_default_library", "//platform/errs/http:go_default_library", "//platform/errs/mysql:go_default_library", "//platform/extension/consumergate:go_default_library", @@ -123,6 +124,7 @@ go_test( "//platform/base/change:go_default_library", "//platform/errs:go_default_library", "//platform/git/exectest:go_default_library", + "//platform/git/repo:go_default_library", "//platform/githubactions:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/buildrunner:go_default_library", diff --git a/service/submitqueue/orchestrator/server/main.go b/service/submitqueue/orchestrator/server/main.go index 40f6fdbfb..2f1038c8e 100644 --- a/service/submitqueue/orchestrator/server/main.go +++ b/service/submitqueue/orchestrator/server/main.go @@ -35,6 +35,7 @@ import ( pb "github.com/uber/submitqueue/api/submitqueue/orchestrator/protopb" "github.com/uber/submitqueue/platform/errs" genericerrs "github.com/uber/submitqueue/platform/errs/generic" + giterrs "github.com/uber/submitqueue/platform/errs/git" httperrs "github.com/uber/submitqueue/platform/errs/http" mysqlerrs "github.com/uber/submitqueue/platform/errs/mysql" "github.com/uber/submitqueue/platform/extension/consumergate" @@ -430,6 +431,7 @@ func defaultProfilesConfig() profilesConfig { func primaryErrorClassifiers() []errs.Classifier { return []errs.Classifier{ genericerrs.Classifier, + giterrs.Classifier, // HTTP must precede MySQL's broad net.Error match to retain dependency attribution. httperrs.Classifier, mysqlerrs.Classifier, diff --git a/service/submitqueue/orchestrator/server/main_test.go b/service/submitqueue/orchestrator/server/main_test.go index 3ac20b2b0..1778b9910 100644 --- a/service/submitqueue/orchestrator/server/main_test.go +++ b/service/submitqueue/orchestrator/server/main_test.go @@ -20,12 +20,15 @@ import ( "fmt" "io" "net/http" + "os" + "path/filepath" "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber/submitqueue/platform/errs" + gitrepo "github.com/uber/submitqueue/platform/git/repo" "github.com/uber/submitqueue/platform/githubactions" ) @@ -76,6 +79,65 @@ func TestPrimaryErrorClassifiers_Storage(t *testing.T) { assert.False(t, errs.IsDependencyError(err)) } +func TestPrimaryErrorClassifiers_GitFetch(t *testing.T) { + processor := errs.NewClassifierProcessor(primaryErrorClassifiers()...) + for _, tt := range []struct { + name string + diagnostic string + cancelled bool + retryable bool + dependency bool + }{ + { + name: "temporary DNS failure", + diagnostic: "fatal: Could not resolve host: git.example.invalid", + retryable: true, + dependency: true, + }, + { + name: "authentication failure", + diagnostic: "fatal: Authentication failed for 'https://git.example.invalid/repo'", + dependency: true, + }, + { + name: "unknown failure", + diagnostic: "fatal: unexpected failure", + dependency: true, + }, + { + name: "shutdown", + cancelled: true, + retryable: true, + }, + } { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + git := filepath.Join(dir, "git") + diagnostic := "'" + strings.ReplaceAll(tt.diagnostic, "'", "'\\''") + "'" + script := "#!/bin/sh\nprintf '%s\\n' " + diagnostic + " >&2\nexit 128\n" + require.NoError(t, os.WriteFile(git, []byte(script), 0o755)) + repo, err := gitrepo.NewRepo(gitrepo.RepoConfig{ + Git: git, + Path: dir, + RemoteURL: "https://git.example.invalid/repo", + Target: "main", + }) + require.NoError(t, err) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if tt.cancelled { + cancel() + } + err = repo.FetchTarget(ctx) + require.Error(t, err) + out := processor.Process(fmt.Errorf("fetch target: %w", err)) + assert.Equal(t, tt.retryable, errs.IsRetryable(out)) + assert.Equal(t, tt.dependency, errs.IsDependencyError(out)) + }) + } +} + type roundTripFunc func(*http.Request) (*http.Response, error) func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) {