Skip to content

Commit a72710f

Browse files
committed
fix(runway): ISS-004 retry transient Git failures
Summary: Intent: - Prevent temporary Git remote and checkout failures from being dead-lettered on their first delivery. - Keep every other Git failure fast-failing, so a deterministic error is not replayed through the retry budget. Changes: - Add structured Git command errors and a Git classifier that opts a failure into retryability only on a known diagnostic/operation pair. - Surface a cancelled context at the Git execution boundary, so cancellation reaches the generic classifier instead of dying as an opaque "signal: killed". - Derive the Git subcommand through one guarded helper and wire the classifier into the Runway primary consumer. Reproduction: - A merge delivery runs `git fetch origin` or `git push origin ...` while the remote temporarily resets the connection, producing a wrapped `*exec.ExitError`. - Previously Runway registered only generic and MySQL classifiers, so the error stayed non-retryable and the consumer rejected it to the DLQ after one attempt. - With this change the structured Git error is classified as a retryable dependency failure, so the consumer nacks it for redelivery. Retryability is an allowlist. Git has no typed status to read, so the classifier 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, so a deleted target branch, an empty squash commit or a rejected push still dead-letters on the first delivery rather than re-running the fetch, reset and cherry-picks behind it on every attempt. `os/exec` reports a context-killed child as a bare `*exec.ExitError` reading "signal: killed", with neither `context.Canceled` nor `context.DeadlineExceeded` anywhere in the chain. `gitexec.CommandFailure` reads `ctx.Err()` and surfaces it, which is what lets the generic classifier recognise a cancelled merge rather than seeing an unexplained Git failure. --- <sub>Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace</sub>
1 parent c9ee7a8 commit a72710f

14 files changed

Lines changed: 820 additions & 34 deletions

File tree

‎platform/errs/README.md‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ One operational consequence worth knowing before relying on any of this: **retry
8989

9090
## Adding a Backend-Specific Classifier
9191

92-
Backend classifiers live alongside the extension they classify, under `platform/errs/<backend>/`. The canonical examples are `platform/errs/mysql` (MySQL driver errors), `platform/errs/http` (rejected status codes and transport failures from clients built on `platform/http`), `platform/errs/yarpc` (YARPC status codes), and `platform/errs/generic` (transport-agnostic concerns such as `context.Canceled`).
92+
Backend classifiers live alongside the extension they classify, under `platform/errs/<backend>/`. The canonical examples are `platform/errs/mysql` (MySQL driver errors), `platform/errs/http` (rejected status codes and transport failures from clients built on `platform/http`), `platform/errs/git` (structured Git process failures), `platform/errs/yarpc` (YARPC status codes), and `platform/errs/generic` (transport-agnostic concerns such as `context.Canceled`).
9393

9494
A classifier:
9595

@@ -122,6 +122,7 @@ Servers wire each classifier into the consumer's `ErrorProcessor`. Order matters
122122
import (
123123
"github.com/uber/submitqueue/platform/errs"
124124
genericerrs "github.com/uber/submitqueue/platform/errs/generic"
125+
giterrs "github.com/uber/submitqueue/platform/errs/git"
125126
httperrs "github.com/uber/submitqueue/platform/errs/http"
126127
mysqlerrs "github.com/uber/submitqueue/platform/errs/mysql"
127128
yarpcerrs "github.com/uber/submitqueue/platform/errs/yarpc"
@@ -130,6 +131,7 @@ import (
130131
c := consumer.New(logger, scope, registry,
131132
errs.NewClassifierProcessor(
132133
genericerrs.Classifier,
134+
giterrs.Classifier,
133135
httperrs.Classifier,
134136
yarpcerrs.Classifier,
135137
mysqlerrs.Classifier,
@@ -143,7 +145,9 @@ Classifiers are not installed globally. A host that wants YARPC statuses classif
143145

144146
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.
145147

146-
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/yarpc/yarpc_test.go`, and `platform/errs/generic/generic_test.go`.
148+
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.
149+
150+
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`.
147151

148152
## Overriding Classification from a Controller
149153

‎platform/errs/git/BUILD.bazel‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
load("@rules_go//go:def.bzl", "go_library", "go_test")
2+
3+
go_library(
4+
name = "go_default_library",
5+
srcs = ["git.go"],
6+
importpath = "github.com/uber/submitqueue/platform/errs/git",
7+
visibility = ["//visibility:public"],
8+
deps = [
9+
"//platform/errs:go_default_library",
10+
"//platform/git/exec:go_default_library",
11+
],
12+
)
13+
14+
go_test(
15+
name = "go_default_test",
16+
srcs = ["git_test.go"],
17+
embed = [":go_default_library"],
18+
deps = [
19+
"//platform/errs:go_default_library",
20+
"//platform/errs/generic:go_default_library",
21+
"//platform/git/exec:go_default_library",
22+
"@com_github_stretchr_testify//assert:go_default_library",
23+
],
24+
)

‎platform/errs/git/git.go‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
// Copyright (c) 2026 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
// Package git provides an errs.Classifier for failures from Git processes.
16+
//
17+
// Git has no typed status to read: it reports almost everything as a non-zero
18+
// exit and a line of prose, so a connection reset and a deleted branch both
19+
// leave `git fetch` looking identical to a caller that only checks the code.
20+
// The subcommand that was run and the diagnostic git printed are therefore the
21+
// only signals available, and the classification pairs them: a fragment is
22+
// evidence of a transient failure only for the operations it can actually
23+
// arise from, so a transport fault counts against a command that talks to the
24+
// remote and lock contention counts against any command that writes to the
25+
// checkout.
26+
//
27+
// Only a recognised pair is retryable. Everything else is a permanent
28+
// infrastructure failure, including a diagnostic this package has never seen.
29+
// The direction is deliberate: an unlisted transient failure costs one lost
30+
// retry, while a permanent failure that defaulted to retryable would replay a
31+
// deterministic error — a deleted target branch, an empty squash commit, a
32+
// rejected push — through the whole retry budget, re-running the fetch, reset
33+
// and cherry-picks behind it each time, before dead-lettering anyway.
34+
//
35+
// Git's wording drifts between versions, so the fragment lists are expected to
36+
// grow. Adding one is cheap and safe; the cost of a missing fragment is bounded
37+
// at a single lost retry, which is what makes the allowlist maintainable.
38+
//
39+
// Cancellation is deliberately absent. A git process killed because its
40+
// context ended dies with "signal: killed" and no trace of the cancellation,
41+
// so it is gitexec.CommandFailure — not this classifier — that puts
42+
// context.Canceled back in the chain, leaving the generic classifier to
43+
// recognise it as it does for every other cancelled operation.
44+
package git
45+
46+
import (
47+
"strings"
48+
49+
"github.com/uber/submitqueue/platform/errs"
50+
gitexec "github.com/uber/submitqueue/platform/git/exec"
51+
)
52+
53+
// Classifier recognises Git process failures, reporting a known transient
54+
// diagnostic on an operation it can arise from as retryable and every other
55+
// Git failure as permanent. See the package doc for why the default runs that
56+
// way.
57+
//
58+
// The classifier is stateless; this package-level singleton is the canonical
59+
// handle. Pass it as one of the variadic classifiers to
60+
// errs.NewClassifierProcessor; the resulting processor is what gets handed to
61+
// consumer.New.
62+
var Classifier errs.Classifier = classifier{}
63+
64+
type classifier struct{}
65+
66+
// remoteOperations are the Git subcommands that exchange data with the
67+
// configured remote. They attribute their failures to that remote, and they
68+
// are the only operations a transport fragment can legitimately describe.
69+
var remoteOperations = map[string]bool{
70+
"clone": true,
71+
"fetch": true,
72+
"ls-remote": true,
73+
"pull": true,
74+
"push": true,
75+
}
76+
77+
// transientTransportFragments are diagnostics that mean the exchange with the
78+
// remote did not complete, weighed only for a remoteOperations subcommand.
79+
// A rejected push or a failed authentication is the remote answering, not
80+
// failing to answer, and stays permanent.
81+
var transientTransportFragments = []string{
82+
"502 bad gateway",
83+
"503 service unavailable",
84+
"504 gateway timeout",
85+
"broken pipe",
86+
"connection refused",
87+
"connection reset by peer",
88+
"connection timed out",
89+
"could not resolve host",
90+
"early eof",
91+
"network is unreachable",
92+
"no route to host",
93+
"operation timed out",
94+
"ssh_exchange_identification",
95+
"temporary failure in name resolution",
96+
"transfer closed with outstanding read data remaining",
97+
}
98+
99+
// transientCheckoutFragments are diagnostics that mean another process held
100+
// the checkout, weighed for every subcommand: a remote operation writes refs
101+
// and the index too, so it can lose the same race a local one can.
102+
var transientCheckoutFragments = []string{
103+
".lock': file exists",
104+
"cannot lock ref",
105+
"index.lock",
106+
"resource temporarily unavailable",
107+
}
108+
109+
// Classify inspects a single node. Per the errs.Classifier contract, this must
110+
// not call errors.Is / errors.As — the classifier-processor owns the chain
111+
// walk.
112+
func (classifier) Classify(err error) errs.Verdict {
113+
commandErr, ok := err.(*gitexec.CommandError)
114+
if !ok {
115+
// The only Unknown this classifier returns, and it means "not my
116+
// node" rather than "no opinion on this failure". Returning a verdict
117+
// here would claim every error the walk passes — a MySQL driver error
118+
// among them — before its own classifier were asked.
119+
return errs.Unknown
120+
}
121+
122+
diagnostic := strings.ToLower(commandErr.Diagnostic())
123+
remote := remoteOperations[commandErr.Operation()]
124+
125+
transient := containsAny(diagnostic, transientCheckoutFragments) ||
126+
(remote && containsAny(diagnostic, transientTransportFragments))
127+
128+
switch {
129+
case transient && remote:
130+
return errs.InfraDependencyRetryable
131+
case transient:
132+
return errs.InfraRetryable
133+
case remote:
134+
return errs.InfraDependency
135+
default:
136+
return errs.Infra
137+
}
138+
}
139+
140+
func containsAny(diagnostic string, fragments []string) bool {
141+
for _, fragment := range fragments {
142+
if strings.Contains(diagnostic, fragment) {
143+
return true
144+
}
145+
}
146+
return false
147+
}

0 commit comments

Comments
 (0)