fix(runway): ISS-004 retry transient Git failures

[sbalabanov]

Summary

Intent:

• Prevent temporary Git process and remote failures from being dead-lettered on their first delivery.
• Preserve terminal merge-result behavior and fail fast for recognizable permanent Git configuration failures.

Changes:

• Add structured Git command errors and a centralized Git classifier for retryability and dependency attribution.
• Preserve Git process causes through Runway merger wrapping and wire the classifier into the primary consumer.
• Cover classifier decisions and verify Runway nacks transient Git failures while rejecting unknown failures.

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; authentication, invalid repository/configuration, process-start, and unknown command failures remain non-retryable.

---

_{Generated by the 🪄 pr-create skill in devexp-agent-marketplace}

Test Plan

AI Verification

Validated at 623ae95 on Sep 4 22:20 UTC · 14 files analyzed · 2s

Validator	Status	Issues
fix-disclosure	not_applicable	0
arc-lint	not_applicable	0
visual-web	not_applicable	0
go-thrift-lint	not_applicable	0
android-coverage	not_applicable	0
ios-lint	not_applicable	0
web-lint	not_applicable	0
web-repocheck	not_applicable	0
diff-template	not_applicable	0
go-coverage	not_applicable	0
visual-ios	not_applicable	0
go-proto-lint	not_applicable	0
android-lint	not_applicable	0
java-lint	not_applicable	0
java-coverage	not_applicable	0
uber-one	not_applicable	0
web-typecheck	not_applicable	0
go-gazelle	not_applicable	0
web-coverage	not_applicable	0
web-unit	not_applicable	0
merge-conflict	not_applicable	0
go-lint	not_applicable	0
arc-unit	not_applicable	0
visual-android	not_applicable	0
ios-test	not_applicable	0
ureview	completed	0
custom	not_applicable	0

0 issues detected

_{Skipped validators: claude · EngWiki}

Prior runs

Run at c9ee7a8 on Sep 4 22:19 UTC · 14 files · 2s · 0 issues detected

Issues

T3-ISS-004

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[sbalabanov]

Reviewed for classifier idiomaticity against platform/errs/README.md.

What's right

• CommandError is a clean carrier: it records provenance without assigning retry policy, which is exactly what "Extensions return plain errors" asks for.
• Classify type-asserts a single node and never calls errors.Is/errors.As — the contract the README is emphatic about.
• Terminal merge outcomes still short-circuit in runway/controller/merge (merger.IsTerminal → FAILED result + ack), so conflicts and invalid requests never reach the processor. That's the load-bearing bit and it survives the change.
• Dependency attribution (InfraDependency* for the remote subcommands) is worth having on its own — errs.Attribution feeds the failure record regardless of retryability.
• Extracting newPrimaryErrorProcessor so the wiring is testable is a nice touch.

The classification policy is inverted

The README states the contract as: "Non-retryable by default … Retryability must be explicitly opted into. This prevents accidental infinite retry loops from unclassified errors."

This classifier does the reverse. Any of 14 allowlisted subcommands that exits non-zero is retryable unless its stderr happens to contain one of 9 English substrings. So the axis is the subcommand — but a subcommand carries no information about whether its failure is transient. git fetch fails transiently on a connection reset and permanently on a deleted branch; both land in the same bucket.

I ran the classifier against real git failures, constructed through the same runAs shape the merger uses (git 2.x, throwaway repo):

command	git's stderr	verdict
rev-parse origin/main (branch absent)	fatal: ambiguous argument 'origin/main': unknown revision or path not in the working tree.	InfraRetryable
cat-file -e <missing sha>	(empty)	InfraRetryable
commit -m … (nothing to commit)	(empty)	InfraRetryable
merge-base --is-ancestor HEAD HEAD (unborn HEAD)	fatal: Not a valid object name HEAD	InfraRetryable
clean -fdx -- /etc	fatal: '/etc' is outside repository at …	InfraRetryable
push origin main (non-fast-forward)	! [rejected] main -> main (fetch first)	InfraDependencyRetryable

All six are deterministic and fail identically on every redelivery. Row 1 is not hypothetical: both resetToRemote and refetchTipSHA run rev-parse <remote>/<target>, so a misconfigured or deleted target branch lands there.

Blast radius, stated honestly: Runway's primary subscriptions use DefaultSubscriptionConfig (Retry.MaxAttempts = 3), so this is 3 attempts instead of 1 before the DLQ — bounded, not an infinite loop. But each redelivery re-runs the whole merge (fetch, reset --hard, clean -fdx, the cherry-picks, and in promote up to MaxPushAttempts pushes against the remote), so it is 3× the git and remote work for something that can never succeed, and it delays the FAILED signal the client is waiting on by the backoff. The reason I'd still call it blocking is the direction of the default rather than today's cost: a classifier is a platform component, and this one makes unrecognised git failures retryable, which is the specific thing the README's default exists to prevent.

Suggested direction

Key on the failure, not the subcommand — an allowlist of known-transient signals, everything else Unknown:

1. Killed by a signal (ExitError.ProcessState, Signaled() / ExitCode() == -1) — OOM-kill, SIGTERM on drain, context-cancel kill. This is the one genuinely transient case, and it's currently caught only as a side effect of the blanket default.
2. Known-transient remote diagnostics on fetch/push/ls-remote: connection reset by peer, could not resolve host, the remote end hung up unexpectedly, early eof, rpc failed, operation timed out, connection refused, 502/503, ssh_exchange_identification, remote end hung up.
3. Local contention: index.lock / unable to create ... File exists, which resolves on retry.

Everything else returns Unknown and dead-letters on attempt 1 as it does today. That inverts the failure mode: a transient case you forgot to list costs one lost retry, instead of a permanent case you forgot to list costing three full merge runs. It also shrinks the list you have to maintain — the transient set is short and stable, the permanent set is unbounded.

Details inline.

---

_{🤖 [posted by agent] — automated review by Claude Code, requested by @sbalabanov.}

[sbalabanov]

Blocking — this is the inverted default.

The switch makes retryability a property of the subcommand. It isn't. Every entry on the second case has a permanent failure mode that is at least as common as a transient one:

• rev-parse — unknown revision (exit 128). Reached from headSHA, refetchTipSHA, commitTreeSHA.
• cat-file — missing object (exit 1, no stderr at all).
• commit — nothing to commit (exit 1). Reachable from the squash path at git_merger.go:651.
• clean — path outside repository, or an undeletable path.
• merge-base — not a valid object name.
• reset — unknown revision, e.g. reset --hard <remote>/<target> when the target branch was deleted.

and on the first case, push covers non-fast-forward, branch protection, and pre-receive hook declines — all of which exit 1 with a ! [rejected] line that matches no fragment.

promote is the worst compounding: it already loops MaxPushAttempts times internally and then returns exceeded %d promote attempts due to remote contention: %w wrapping the push CommandError. A permanently rejected push now costs MaxPushAttempts × 3 pushes against the remote before the DLQ.

Suggest replacing both case arms with a positive test for known-transient signals (see the review body). If you want to keep the operation axis for attribution — fetch/push/ls-remote → dependency, rest → local — that part is sound and worth keeping; it's only the retryable/non-retryable decision that needs to move off it.

---

_{🤖 [posted by agent] — automated review by Claude Code, requested by @sbalabanov.}

[sbalabanov]

yes. Only known errors should become retryable. Make a change.

[sbalabanov]

Done — retryability is now an allowlist keyed on the pair, in e70bb3ae.

Classify recognises a transient failure only when a known fragment appears on an operation it can arise from: transientTransportFragments are weighed only for clone/fetch/ls-remote/pull/push, and transientCheckoutFragments (lock contention) for any subcommand, since a remote operation writes refs and the index too and can lose the same race. Everything else is permanent. The operation allowlist that used to drive retryability is gone; remoteOperations now decides attribution only.

On errs.Infra as the default — agreed and changed. Every *gitexec.CommandError now leaves with a verdict: InfraDependencyRetryable, InfraRetryable, InfraDependency, or Infra. Nothing about a Git failure falls through to Unknown any more.

What errs.Unknown is for — it is the "not my node" signal, not "no opinion on this failure", and the classifier returns it in exactly one place now: when the node is not a *gitexec.CommandError at all. It has to. NewClassifierProcessor walks the chain node by node and asks every registered classifier about each one, first non-Unknown wins. Runway registers generic, git, mysql. If the Git classifier returned Infra for a node it does not recognise, the first node of every error in the service would come back Infra — a *gomysql.MySQLError for a deadlock would be classified as a permanent Git failure and mysqlerrs would never be asked, so lock-wait timeouts and deadlocks would stop retrying. Unknown is what keeps a classifier scoped to its own errors; Infra is the default within that scope, which is what the code now does.

Verified against real git (throwaway repo, same wrapping the merger uses). All previously-retryable permanent failures now dead-letter on attempt 1, attribution intact, and the ISS-004 case still nacks:

git rev-parse origin/main                retryable=false dependency=false
git cat-file -e <missing sha>            retryable=false dependency=false
git commit -m … (nothing to commit)      retryable=false dependency=false
git merge-base --is-ancestor HEAD HEAD   retryable=false dependency=false
git clean -fdx -- /etc                   retryable=false dependency=false
git fetch origin   (no such remote)      retryable=false dependency=true
git push origin main (rejected)          retryable=false dependency=true
git config --bogus                       retryable=false dependency=false
ISS-004 connection reset on fetch        retryable=true  dependency=true

---

_{🤖 [addressed by agent] — Claude Code, at @sbalabanov's request.}

[sbalabanov]

A deny-list of rendered diagnostics can't carry this decision, for two reasons.

1. There is often no text to match. git cat-file -e <missing-sha> and git commit with nothing staged both exit non-zero with an entirely empty stderr, so Diagnostic() is just "exit status 1". No fragment list can ever reach those.

2. The messages that do exist don't match these fragments. Real stderr for permanent failures I reproduced:

fatal: ambiguous argument 'origin/main': unknown revision or path not in the working tree.
fatal: Not a valid object name HEAD
fatal: '/etc' is outside repository at '/tmp/...'
! [rejected]        main -> main (fetch first)

not a git repository is in the list; git actually says outside repository here. The list is chasing a surface that git is free to re-word between versions — and the README holds up the neighbouring classifier for doing the opposite: "The YARPC classifier reads the typed status code rather than matching its rendered message."

(Locale is not the risk it would normally be, since gitexec.Env builds the environment from scratch and doesn't inherit LANG/LC_ALL — worth knowing, but it only removes one of several failure modes. A deployment that puts a locale var in PassthroughEnv would reintroduce it.)

Minor inconsistency while this block exists: with a permanent diagnostic, an unrecognised operation returns errs.Infra (line 56, terminating the walk), while the same unrecognised operation without one returns errs.Unknown (line 68, continuing it). Same transport outcome, different attribution and different effect on classifiers registered after this one — worth picking one.

---

_{🤖 [posted by agent] — automated review by Claude Code, requested by @sbalabanov.}

[sbalabanov]

git does not have binary protocol (always returns code 1) so we have to rely on our knowledge of operation performed (fetch, checkout etc) and stderr. It can change between versions, so we have to keep fragments relevant to all versions and update as needed.
Add more fragments as you see fit. Make sure only known fragment/operation combination become retryable, and everything else will be non-retryable by default.
errs.Infra should always be the default. What errs.Unknown is even used for?

[sbalabanov]

Done. permanentDiagnosticFragments is gone; transientTransportFragments and transientCheckoutFragments replace it, and matching one is now the only way a Git failure becomes retryable.

The two cases that motivated the comment behave correctly now: git cat-file -e <missing sha> and an empty git commit produce no stderr at all, so there is nothing to match and both are permanent by default rather than by omission.

Writing the allowlist turned up a live trap worth recording. I had included could not read from remote repository as a transport signature; the probe caught it immediately, because Git prints that line as a trailer under permanent failures too:

fatal: 'origin' does not appear to be a git repository
fatal: Could not read from remote repository.

It is removed, and TestClassifier now pins that exact output as InfraDependency. I also dropped a bare unable to create in favour of .lock': file exists for the same reason — too broad to be evidence of anything. The fragment lists are documented as expected to grow, with the rule that an entry must name a failure a later attempt can actually resolve.

---

_{🤖 [addressed by agent] — Claude Code, at @sbalabanov's request.}

[sbalabanov]

The gate throws away the one signal that reliably means "transient".

*exec.ExitError carries ProcessState, so ExitCode() and Signaled()/Signal() are both available here — and a git process killed by a signal (OOM-killer, SIGTERM during drain, the kill exec.CommandContext issues on context cancellation) is the clearest transient case there is. Right now that case is only classified correctly by accident, because the blanket default happens to cover it.

Note that context.Canceled never reaches genericerrs.Classifier on this path either: CommandContext kills the child and returns *exec.ExitError ("signal: killed"), so the chain is fmt.Errorf → *CommandError → *exec.ExitError with no context.Canceled node anywhere. Graceful-shutdown nacking works today only via the blanket default. If the default is inverted per the other comment, signal-detection has to be added explicitly or shutdown behaviour regresses — worth a test either way.

Concretely: have CommandError capture exitCode int and signaled bool at construction, and branch on those rather than on the subcommand.

---

_{🤖 [posted by agent] — automated review by Claude Code, requested by @sbalabanov.}

[sbalabanov]

Signal termination should be handled by generic classifier, not git-specific classifier. Context cancellation should be properly surfaced up. Investigate if it is not the case and post here.

[sbalabanov]

Done — signal handling is out of the Git classifier, and the investigation you asked for is below.

Context cancellation is not surfaced today. os/exec swallows it. Probe against Go 1.26:

cancelled  err="signal: killed"  errors.Is(err, context.Canceled)=false  errors.As(err, &exec.ExitError)=true  ctx.Err()=context canceled
deadline   err="signal: killed"  errors.Is(err, context.DeadlineExceeded)=false  errors.As(err, &exec.ExitError)=true  ctx.Err()=context deadline exceeded

exec.CommandContext kills the child and Wait reports only the death. Neither context sentinel appears anywhere in the chain — ctx.Err() is the only place the distinction still exists. So genericerrs.Classifier, which tests err == context.Canceled, could never fire on a cancelled merge no matter where it sits in the list. That was true before this PR too; nothing was routing it.

Fix: gitexec.CommandFailure(ctx, args, message, cause) is now the single construction point for a failed invocation, used by Output, runAs, runCombinedAs and isAncestor. When ctx.Err() != nil it returns that error wrapped with the operation and diagnostic, so the chain carries context.Canceled and the generic classifier recognises it; otherwise it returns a *CommandError as before. The Git classifier no longer looks at signals at all, and TestClassifier_LeavesCancellationToTheGenericClassifier pins that it returns Unknown for a cancelled command so the generic one gets the node.

Two things worth knowing, neither fixed here:

1. genericerrs.Classifier handles context.Canceled but not context.DeadlineExceeded. A merge that blows its deadline now surfaces DeadlineExceeded into the chain, matches nothing, and stays non-retryable. Cheap to add — one case in platform/errs/generic — but that classifier is shared across every domain, so I left it alone rather than changing cross-domain retry behaviour inside a Runway fix. Happy to do it here or in a follow-up, your call.
2. A git process killed by something other than a context — OOM killer, an operator SIGKILL — has no context error to read and no transient fragment in its output, so it is now permanent where the previous revision retried it. That follows directly from "only known combinations become retryable", so I have left it that way; if you want it retryable the honest place is a signal check in the generic classifier, since it is not Git-specific.

---

_{🤖 [addressed by agent] — Claude Code, at @sbalabanov's request.}

[sbalabanov]

This is the gate for the entire classifier, and it type-asserts the immediate cause only. Any caller that adds context before constructing — NewCommandError(op, msg, fmt.Errorf("...: %w", err)) — silently turns classification off for that call site, with no test able to notice.

It fails safe (non-retryable), so it isn't a bug today. But given it's load-bearing, I'd rather it not depend on caller discipline: resolve the process outcome once at construction and store it as data.

type CommandError struct {
    operation string
    message   string
    exitCode  int  // -1 when the process never exited normally
    signaled  bool
    cause     error
}

That also makes the classifier a pure function of recorded data, drops the os/exec dependency from Classify, and removes the need for tests to spawn a real process to get a real *exec.ExitError (see the fixture comments).

---

_{🤖 [posted by agent] — automated review by Claude Code, requested by @sbalabanov.}

[sbalabanov]

explain with the example (sequence) of events.

[sbalabanov]

Explained below — and the gate is gone, so the hazard no longer exists.

The sequence I had in mind, on the previous revision:

1. runAs runs git fetch; the remote resets the connection; cmd.Run() returns *exec.ExitError.
2. A boundary adds context before reporting — NewCommandError("fetch", msg, fmt.Errorf("fetch %s: %w", remote, err)). Nothing forbade this; isAncestor already passed a hand-built cause, and the three construction sites each assembled their own.
3. ProcessExited() did e.cause.(*exec.ExitError) — a type assertion on the immediate cause. The cause is now the fmt.Errorf wrapper, so the assertion fails and it returns false.
4. Classify gated on !commandErr.ProcessExited() and returned Unknown.
5. The transient fetch dead-letters on attempt 1 — the exact bug this PR exists to fix, silently reintroduced by a wrapping change two layers away, with every classifier test still green because they all constructed the cause directly.

Step 3 is the whole failure: errors.As walks a chain, a type assertion looks at one node, and the gate used the latter on a value assembled by callers.

Resolution: the allowlist made the gate unnecessary. A failure is retryable only on a known diagnostic/operation pair, so ProcessExited has no decision left to guard and is deleted along with the os/exec dependency in that file. CommandError is back to operation + message + cause, as in your original. Nothing now reads the cause's concrete type to decide retryability, so no wrapping change upstream can flip a classification.

---

_{🤖 [addressed by agent] — Claude Code, at @sbalabanov's request.}

[sbalabanov]

commandOperation is unexported, so it's used in exactly one place (Output) while the operation is derived two other ways elsewhere:

• git_merger.go:1050 and :1074 index args[0] directly (which also panics on empty args — the guard here exists for precisely that).
• git_merger.go:1008 hardcodes the literal "merge-base".

Three derivations of the same value, and the classifier's correctness depends on all three agreeing. Either export this helper, or — cleaner — have NewCommandError take args []string and derive the operation itself, so the boundary can't disagree with itself.

There's also a correctness edge in the current derivation: Output passes the raw args, so a leading global flag becomes the "operation". TestOutput_PreservesCommandFailure asserts wantOperation: "-test.run=[", which locks that in as expected behaviour. Today's callers all pass the subcommand first, so nothing is broken — but a future gitexec.Output(ctx, git, dir, "-c", "foo=bar", "fetch", ...) would silently classify as Unknown. Skipping leading --prefixed tokens (and their values for -c) would make the derivation match what the field claims to hold.

---

_{🤖 [posted by agent] — automated review by Claude Code, requested by @sbalabanov.}

[sbalabanov]

ignore

[sbalabanov]

Acknowledged — not pursued as its own change. No leading-flag skipping, and TestOutput_PreservesCommandFailure keeps asserting -test.run=[ as the operation.

One piece did have to move to fix the panic on the thread below: commandOperation is exported as gitexec.Operation so the merger can share the empty-args guard instead of indexing args[0]. Behaviour is unchanged — args[0] when present, "" when empty. TestCommandOperation is renamed TestOperation with the same two cases and no new ones.

---

_{🤖 [addressed by agent] — Claude Code, at @sbalabanov's request.}

[sbalabanov]

args[0] panics if args is empty. gitexec.commandOperation already guards this but isn't exported, so the guard doesn't reach here (same at line 1074).

No current caller passes zero args, so this is latent rather than live — but it's the kind of thing that gets introduced by a refactor and shows up as a panic in a queue consumer. See the note on command_error.go for the version of this that fixes both sites at once.

---

_{🤖 [posted by agent] — automated review by Claude Code, requested by @sbalabanov.}

[sbalabanov]

address - make backwards compatible behavior

[sbalabanov]

Addressed, behaviour-preserving.

All three sites now go through gitexec.CommandFailure(ctx, args, message, err), which derives the subcommand via the exported gitexec.Operation — the same guarded helper Output already used. runAs and runCombinedAs no longer index args[0], and isAncestor no longer hardcodes "merge-base"; it builds its args slice once and passes it, so the command that runs and the operation that gets reported cannot drift apart.

Backwards compatible: Operation(args) returns args[0] for every non-empty argument list, which is every current caller. The only changed case is the one that used to panic — empty args now yield "" and classify as a permanent local failure. TestCommandFailure/empty_arguments_do_not_panic covers it.

---

_{🤖 [addressed by agent] — Claude Code, at @sbalabanov's request.}

[sbalabanov]

The git classifier is added to the examples list and the wiring snippet, but not to the semantics section further down — where httperrs/mysqlerrs ordering and the YARPC status-code mapping each get a paragraph explaining why they classify the way they do.

That gap matters more than usual here, because two paragraphs below this line the README says:

The YARPC classifier reads the typed status code rather than matching its rendered message.

and the git classifier does the opposite. If the message-matching approach survives review, it needs a paragraph saying why git is the exception, what happens when the fragment list goes stale, and which direction the default errs in. If it's inverted per the main comment, it needs a paragraph saying the retryable set is a closed allowlist of transient signals and that unlisted failures dead-letter on the first attempt.

Also worth stating explicitly: this classifier is registered before mysqlerrs, and unlike the httperrs/mysqlerrs pair there's no net.Error ambiguity between them (*exec.ExitError doesn't satisfy it), so the ordering is free. A sentence saying so saves the next person the analysis.

---

_{🤖 [posted by agent] — automated review by Claude Code, requested by @sbalabanov.}

[sbalabanov]

ignore

[sbalabanov]

Acknowledged, and my comment was wrong on its premise: the PR did add a semantics paragraph. I skipped that hunk when reading the diff and claimed it was missing. Apologies.

I have not acted on what the comment actually asked for — no note about the YARPC contrast, none about classifier ordering. The paragraph is edited only because the behaviour it documents changed: it described retryable-unless-permanent, which is no longer what the code does. It now states the pair rule, that only a recognised pair is retryable, and where cancellation is handled. Same position, comparable length.

---

_{🤖 [addressed by agent] — Claude Code, at @sbalabanov's request.}

[sbalabanov]

The exec.Command(os.Args[0], "-test.run=[") trick — re-running the test binary with a deliberately invalid regex to manufacture an *exec.ExitError — appears three times across the PR (here, gitexec_test.go:145, main_test.go:64). It's a real subprocess spawn per test, and in main_test.go it re-executes the Runway server's test binary, which is a heavier thing to fork than it looks.

If CommandError records the exit outcome as data (see command_error.go), all three disappear: the fixture becomes NewCommandError("fetch", "connection reset", exitCode: 128). If the *exec.ExitError shape has to stay, the stdlib helper-process pattern (-test.run=TestHelperProcess + GO_WANT_HELPER_PROCESS=1) is the idiomatic spelling and reads as intentional rather than as an accident being exploited.

Small nit further down: wantSame: false at line 144 is the zero value and the field is only read when true — it reads as if it asserts something, but it's inert. Drop it.

Coverage gaps worth closing whichever way the classifier lands: a signal-killed process, a push rejected non-fast-forward, and a CommandError whose Diagnostic() is empty (git cat-file -e on a missing object produces exactly that, and it's the case the fragment list can't see).

---

_{🤖 [posted by agent] — automated review by Claude Code, requested by @sbalabanov.}

[sbalabanov]

ignore

[sbalabanov]

Acknowledged — not pursued. No helper-process refactor; exec.Command(os.Args[0], "-test.run=[") stays exactly as you wrote it in gitexec_test.go, which is now the only place it appears.

The other two copies disappeared as a side effect rather than as a cleanup: platform/errs/git and service/runway/server no longer need a real *exec.ExitError, because the classifier reads the operation and diagnostic instead of the process state. Their fixtures are plain NewCommandError values.

git_test.go in this package is rewritten against the new policy — the old cases asserted verdicts that no longer exist. wantSame: false went with it.

---

_{🤖 [addressed by agent] — Claude Code, at @sbalabanov's request.}