fix(runway): ISS-004 retry transient Git failures - #678
Conversation
|
|
sbalabanov
left a comment
There was a problem hiding this comment.
Reviewed for classifier idiomaticity against platform/errs/README.md.
What's right
CommandErroris a clean carrier: it records provenance without assigning retry policy, which is exactly what "Extensions return plain errors" asks for.Classifytype-asserts a single node and never callserrors.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.Attributionfeeds the failure record regardless of retryability. - Extracting
newPrimaryErrorProcessorso 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:
- 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. - 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. - 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.
623ae95 to
f3a437d
Compare
f3a437d to
e70bb3a
Compare
behinddwalls
left a comment
There was a problem hiding this comment.
Inline comments on the remaining high-signal issues after the allowlist revision:
rpc failed/remote end hung up unexpectedly/unexpected disconnect while reading sideband packetover-match permanent git-http 4xx.classifyMergeFailure's doc comment still says those non-conflict failures should be retried.
Architecture otherwise looks right (CommandError as carrier, fail-closed pair rule, cancellation left to generic).
Generated by Cursor. Posted on behalf of @preetam_UBER.
e70bb3a to
a72710f
Compare
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>
a72710f to
250eabc
Compare
Summary
Intent:
Changes:
Reproduction:
git fetch originorgit push origin ...while the remote temporarily resets the connection, producing a wrapped*exec.ExitError.Generated by the 🪄 pr-create skill in devexp-agent-marketplace
Test Plan
Issues
T3-ISS-004