diff --git a/platform/errs/README.md b/platform/errs/README.md index 141a3b4d3..5431c1c31 100644 --- a/platform/errs/README.md +++ b/platform/errs/README.md @@ -145,7 +145,7 @@ Classifiers are not installed globally. A host that wants YARPC statuses classif The YARPC classifier reads the typed status code rather than matching its rendered message. Cancellation is retryable caller-side infrastructure; transient or ambiguous server codes (`Unknown`, `DeadlineExceeded`, `ResourceExhausted`, `Aborted`, `Internal`, and `Unavailable`) are retryable dependency failures; request verdicts and permanent server failures are non-retryable dependency failures. A deadline may expire after a mutating RPC succeeded, so this classification relies on the repository-wide requirement that queue-driven operations are idempotent. -The Git classifier reads `gitexec.CommandError`, which preserves the Git subcommand and the underlying `os/exec` error through contextual wrapping. Git has no typed status to read — a connection reset and a deleted branch both leave `fetch` at a non-zero exit — so the classifier pairs the subcommand with the diagnostic git printed: a transport fragment counts only against a command that talks to the remote, and a lock fragment counts against any command that writes to the checkout. Only a recognised pair is retryable; every other Git failure, including a diagnostic the package has never seen, is a permanent infrastructure failure attributed to the remote or to this service. The direction is deliberate — an unlisted transient failure costs one lost retry, while a permanent failure defaulting to retryable would replay a deterministic error through the whole retry budget before dead-lettering anyway — and it is what makes the fragment lists safe to extend as Git's wording drifts between versions. Cancellation is not the Git classifier's to report: `os/exec` kills a context-cancelled child and reports only `signal: killed`, so `gitexec.CommandFailure` puts `context.Canceled` back in the chain and the generic classifier recognises it there. +The Git classifier reads the `giterrs.CommandFailure` contract: the Git subcommand and the diagnostic Git printed. The local execution package's `gitexec.CommandError` satisfies that contract, and remote execution backends can satisfy it with their own native error type instead of reconstructing a local process error. Backends that need the policy without the classifier adapter can call `giterrs.ClassifyCommand` directly. Git has no typed status to read — a connection reset and a deleted branch both leave `fetch` at a non-zero exit — so the policy pairs the subcommand with the diagnostic: a transport fragment counts only against a command that talks to the remote, and a lock fragment counts against any command that writes to the checkout. Only a recognised pair is retryable; every other Git failure, including a diagnostic the package has never seen, is a permanent infrastructure failure attributed to the remote or to this service. The direction is deliberate — an unlisted transient failure costs one lost retry, while a permanent failure defaulting to retryable would replay a deterministic error through the whole retry budget before dead-lettering anyway — and it is what makes the fragment lists safe to extend as Git's wording drifts between versions. Cancellation is not the Git classifier's to report: `os/exec` kills a context-cancelled child and reports only `signal: killed`, so `gitexec.CommandFailure` puts `context.Canceled` back in the chain and the generic classifier recognises it there. Tests follow the same shape: assert per-node behaviour against `Classifier.Classify(node)` directly, and assert end-to-end behaviour by running `errs.NewClassifierProcessor(Classifier).Process(err)` and checking the helpers (`IsRetryable`, `IsUserError`, …) on the result. See `platform/errs/mysql/mysql_test.go`, `platform/errs/git/git_test.go`, `platform/errs/yarpc/yarpc_test.go`, and `platform/errs/generic/generic_test.go`. diff --git a/platform/errs/git/BUILD.bazel b/platform/errs/git/BUILD.bazel index c8c9421d3..a1f87a762 100644 --- a/platform/errs/git/BUILD.bazel +++ b/platform/errs/git/BUILD.bazel @@ -5,10 +5,7 @@ go_library( srcs = ["git.go"], importpath = "github.com/uber/submitqueue/platform/errs/git", visibility = ["//visibility:public"], - deps = [ - "//platform/errs:go_default_library", - "//platform/git/exec:go_default_library", - ], + deps = ["//platform/errs:go_default_library"], ) go_test( diff --git a/platform/errs/git/git.go b/platform/errs/git/git.go index 1bb8c1d0b..061db07e1 100644 --- a/platform/errs/git/git.go +++ b/platform/errs/git/git.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package git provides an errs.Classifier for failures from Git processes. +// Package git classifies failures reported by Git commands. // // Git has no typed status to read: it reports almost everything as a non-zero // exit and a line of prose, so a connection reset and a deleted branch both @@ -47,7 +47,6 @@ import ( "strings" "github.com/uber/submitqueue/platform/errs" - gitexec "github.com/uber/submitqueue/platform/git/exec" ) // Classifier recognises Git process failures, reporting a known transient @@ -63,6 +62,25 @@ var Classifier errs.Classifier = classifier{} type classifier struct{} +// CommandFailure is the Git failure information needed by Classifier. +// +// Git does not expose a structured failure protocol: unrelated failures +// collapse to generic non-zero exit statuses, leaving the command and its text +// output as the only useful classification signals. +// +// Execution backends may satisfy this contract with their native error type; +// they do not need to reconstruct an error from another Git implementation. +type CommandFailure interface { + error + // Operation returns the Git subcommand that failed, such as fetch, commit, + // or push. It excludes the Git executable and later arguments. + Operation() string + // Diagnostic returns the complete failure output reported by Git. It must + // retain all stderr or stdout lines because a classifying fragment may + // appear after introductory advice or hints. + Diagnostic() string +} + // remoteOperations are the Git subcommands that exchange data with the // configured remote. They attribute their failures to that remote, and they // are the only operations a transport fragment can legitimately describe. @@ -106,21 +124,13 @@ var transientCheckoutFragments = []string{ "resource temporarily unavailable", } -// Classify inspects a single node. Per the errs.Classifier contract, this must -// not call errors.Is / errors.As — the classifier-processor owns the chain -// walk. -func (classifier) Classify(err error) errs.Verdict { - commandErr, ok := err.(*gitexec.CommandError) - if !ok { - // The only Unknown this classifier returns, and it means "not my - // node" rather than "no opinion on this failure". Returning a verdict - // here would claim every error the walk passes — a MySQL driver error - // among them — before its own classifier were asked. - return errs.Unknown - } - - diagnostic := strings.ToLower(commandErr.Diagnostic()) - remote := remoteOperations[commandErr.Operation()] +// ClassifyCommand classifies a Git subcommand and its rendered diagnostic. +// +// It is the shared policy for local processes, remote Git execution services, +// and any other backend that can report those two values. +func ClassifyCommand(operation, diagnostic string) errs.Verdict { + diagnostic = strings.ToLower(diagnostic) + remote := remoteOperations[operation] transient := containsAny(diagnostic, transientCheckoutFragments) || (remote && containsAny(diagnostic, transientTransportFragments)) @@ -137,6 +147,17 @@ func (classifier) Classify(err error) errs.Verdict { } } +// Classify inspects a single node. Per the errs.Classifier contract, this must +// not call errors.Is / errors.As — the classifier-processor owns the chain +// walk. +func (classifier) Classify(err error) errs.Verdict { + commandFailure, ok := err.(CommandFailure) + if !ok { + return errs.Unknown + } + return ClassifyCommand(commandFailure.Operation(), commandFailure.Diagnostic()) +} + func containsAny(diagnostic string, fragments []string) bool { for _, fragment := range fragments { if strings.Contains(diagnostic, fragment) { diff --git a/platform/errs/git/git_test.go b/platform/errs/git/git_test.go index 5e778f79f..b9a46be1a 100644 --- a/platform/errs/git/git_test.go +++ b/platform/errs/git/git_test.go @@ -32,79 +32,137 @@ func gitError(operation, diagnostic string) error { return gitexec.NewCommandError(operation, diagnostic, errors.New("exit status 128")) } -func TestClassifier(t *testing.T) { +type remoteCommandFailure struct { + operation string + diagnostic string +} + +func (e remoteCommandFailure) Error() string { + return e.diagnostic +} + +func (e remoteCommandFailure) Operation() string { + return e.operation +} + +func (e remoteCommandFailure) Diagnostic() string { + return e.diagnostic +} + +func TestClassifyCommand(t *testing.T) { tests := []struct { - name string - err error - want errs.Verdict + name string + operation string + diagnostic string + want errs.Verdict }{ { - name: "transport fault on fetch is a retryable dependency failure", - err: gitError("fetch", "fatal: unable to access 'https://host/r.git/': Connection reset by peer"), - want: errs.InfraDependencyRetryable, + name: "transport fault on fetch is a retryable dependency failure", + operation: "fetch", + diagnostic: "fatal: unable to access 'https://host/r.git/': Connection reset by peer", + want: errs.InfraDependencyRetryable, }, { - name: "unresolvable host on ls-remote is a retryable dependency failure", - err: gitError("ls-remote", "fatal: Could not resolve host: github.example.com"), - want: errs.InfraDependencyRetryable, + name: "unresolvable host on ls-remote is a retryable dependency failure", + operation: "ls-remote", + diagnostic: "fatal: Could not resolve host: github.example.com", + want: errs.InfraDependencyRetryable, }, { - name: "checkout contention on a local commit is a retryable local failure", - err: gitError("commit", "fatal: Unable to create '/checkout/.git/index.lock': File exists."), - want: errs.InfraRetryable, + name: "checkout contention on a local commit is a retryable local failure", + operation: "commit", + diagnostic: "fatal: Unable to create '/checkout/.git/index.lock': File exists.", + want: errs.InfraRetryable, }, { - name: "checkout contention during fetch is attributed to the remote it ran against", - err: gitError("fetch", "error: cannot lock ref 'refs/remotes/origin/main'"), - want: errs.InfraDependencyRetryable, + name: "checkout contention during fetch is attributed to the remote it ran against", + operation: "fetch", + diagnostic: "error: cannot lock ref 'refs/remotes/origin/main'", + want: errs.InfraDependencyRetryable, }, { - name: "transport fragment on a local operation is not evidence of a transient failure", - err: gitError("merge", "error: could not resolve host mentioned in a commit message"), - want: errs.Infra, + name: "transport fragment on a local operation is not evidence of a transient failure", + operation: "merge", + diagnostic: "error: could not resolve host mentioned in a commit message", + want: errs.Infra, }, { - name: "unknown revision is a permanent local failure", - err: gitError("rev-parse", "fatal: ambiguous argument 'origin/main': unknown revision or path not in the working tree."), - want: errs.Infra, + name: "unknown revision is a permanent local failure", + operation: "rev-parse", + diagnostic: "fatal: ambiguous argument 'origin/main': unknown revision or path not in the working tree.", + want: errs.Infra, }, { - name: "empty squash commit is a permanent local failure", - err: gitError("commit", "exit status 1"), - want: errs.Infra, + name: "empty squash commit is a permanent local failure", + operation: "commit", + diagnostic: "exit status 1", + want: errs.Infra, }, { - name: "failure with no diagnostic at all is a permanent local failure", - err: gitError("cat-file", ""), - want: errs.Infra, + name: "failure with no diagnostic at all is a permanent local failure", + operation: "cat-file", + want: errs.Infra, }, { - name: "path outside the repository is a permanent local failure", - err: gitError("clean", "fatal: '/etc': '/etc' is outside repository at '/checkout'"), - want: errs.Infra, + name: "path outside the repository is a permanent local failure", + operation: "clean", + diagnostic: "fatal: '/etc': '/etc' is outside repository at '/checkout'", + want: errs.Infra, }, { - name: "non-fast-forward push is a permanent dependency failure", - err: gitError("push", "! [rejected] main -> main (fetch first)"), - want: errs.InfraDependency, + name: "non-fast-forward push is a permanent dependency failure", + operation: "push", + diagnostic: "! [rejected] main -> main (fetch first)", + want: errs.InfraDependency, }, { - name: "authentication failure is a permanent dependency failure", - err: gitError("fetch", "fatal: Authentication failed for 'https://host/r.git/'"), - want: errs.InfraDependency, + name: "authentication failure is a permanent dependency failure", + operation: "fetch", + diagnostic: "fatal: Authentication failed for 'https://host/r.git/'", + want: errs.InfraDependency, }, { // Git prints this trailer under permanent failures too — a missing // remote, absent access rights — so it is not evidence of a // transient one. - name: "generic remote trailer is a permanent dependency failure", - err: gitError("fetch", "fatal: 'origin' does not appear to be a git repository\nfatal: Could not read from remote repository."), - want: errs.InfraDependency, + name: "generic remote trailer is a permanent dependency failure", + operation: "fetch", + diagnostic: "fatal: 'origin' does not appear to be a git repository\nfatal: Could not read from remote repository.", + want: errs.InfraDependency, + }, + { + name: "unknown subcommand is a permanent local failure", + operation: "bisect", + diagnostic: "fatal: something went wrong", + want: errs.Infra, }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, ClassifyCommand(tt.operation, tt.diagnostic)) + }) + } +} + +func TestClassifier(t *testing.T) { + tests := []struct { + name string + err error + want errs.Verdict + }{ { - name: "unknown subcommand is a permanent local failure", - err: gitError("bisect", "fatal: something went wrong"), - want: errs.Infra, + name: "local process failure", + err: gitError("fetch", "fatal: Connection reset by peer"), + want: errs.InfraDependencyRetryable, + }, + { + name: "remote execution failure", + err: remoteCommandFailure{ + operation: "commit", + diagnostic: "fatal: Unable to create '/checkout/.git/index.lock': File exists.", + }, + want: errs.InfraRetryable, }, { name: "non-Git error is not this classifier's node", @@ -113,7 +171,6 @@ func TestClassifier(t *testing.T) { }, { name: "nil is not this classifier's node", - err: nil, want: errs.Unknown, }, } @@ -129,13 +186,13 @@ func TestClassifier_FragmentsMatchRegardlessOfCase(t *testing.T) { for _, fragment := range transientTransportFragments { t.Run(fragment, func(t *testing.T) { shouted := "fatal: " + strings.ToUpper(fragment) - assert.Equal(t, errs.InfraDependencyRetryable, Classifier.Classify(gitError("fetch", shouted))) + assert.Equal(t, errs.InfraDependencyRetryable, ClassifyCommand("fetch", shouted)) }) } for _, fragment := range transientCheckoutFragments { t.Run(fragment, func(t *testing.T) { shouted := "fatal: " + strings.ToUpper(fragment) - assert.Equal(t, errs.InfraRetryable, Classifier.Classify(gitError("commit", shouted))) + assert.Equal(t, errs.InfraRetryable, ClassifyCommand("commit", shouted)) }) } }