feat(git): classify git command timeouts as infra errors#235
Merged
Conversation
Summary: Intent: - Git commands (checkout, fetch, clone, diff, apply patch, commit, submodule update) already run under a fixed timeout, but a timeout failure was indistinguishable from any other git error, so it always surfaced as an unclassified error at the top-level orchestrator. Changes: - Added a sentinel `git.ErrTimeout` and a `wrapError` helper in `core/git` that wraps a failing git operation's error with its name, and additionally wraps ErrTimeout when the failure was caused by the command's own timeout (as opposed to a parent context cancellation). - Added `orchestrator.classifyGitError`, used at the checkout, apply-requests, and treehash-compute call sites in `native_orchestrator.go`, which classifies the error as `tangoerrors.NewInfra` when it wraps `git.ErrTimeout`, per the classification conventions in `docs/errors/errors.md`. --- <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>
blockingRunner never actually needed to block — the derived timeout context is already Done before the runner is invoked in these tests, since the parent's deadline/cancellation already elapsed. Reuse the existing mockRunner with a returnCtxErr flag instead of a second runner type.
core/git already wraps failures with the operation name, so classifyGitError only needs to classify, not wrap again.
yushan8
marked this pull request as ready for review
July 23, 2026 23:44
xytan0056
reviewed
Jul 24, 2026
xytan0056
reviewed
Jul 24, 2026
| // classifyGitError classifies err as an infra error when it was caused by a | ||
| // git command timing out. | ||
| func classifyGitError(err error) error { | ||
| if errors.Is(err, git.ErrTimeout) { |
Contributor
There was a problem hiding this comment.
if the error is "exit status 128", then it wouldn't be classified as NewInfra. It should be infra? The unwrapped error if not timeout, would be treated as infra by tangoerrors.GetErrorCode, becuase it just happens to be unclassfied.
Contributor
Author
There was a problem hiding this comment.
Yea it should be NewInfra. I can classify them in this PR.
…outs Unconditionally wrap git-command failures with NewInfra instead of only timeouts. context.Canceled is still correctly reported as ErrorCancelled downstream since GetErrorCode checks for it ahead of the TangoError code, and context.DeadlineExceeded correctly falls through to ErrorInfra.
wrapError now includes the failing command's actual arguments (e.g. the ref/remote being acted on) instead of just the operation name, and wraps ErrFatal when a command exits with a fatal (128) or usage (129) exit code, as opposed to a non-fatal, conditional exit code such as 1. classifyGitError explicitly classifies git.ErrTimeout and git.ErrFatal as infra; other git failures are returned unclassified, falling through to core/errors' default infra classification.
core/git only knows the ref it was asked to check out, not which BuildDescription triggered it. Wrap the error with the remote and base SHA here so failures are traceable back to the specific build being processed, addressing PR review feedback that this context was lost.
xytan0056
approved these changes
Jul 24, 2026
… context Same rationale as the checkout call site: core/git only knows the ref or patch it was given, not which BuildDescription triggered it. Wrap remaining classifyGitError call sites with remote and base SHA so failures are traceable back to the build being processed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Intent:
submodule update) already run under a fixed timeout, but a timeout
failure was indistinguishable from any other git error or context.Cancelled error.
Changes:
git.ErrTimeoutand awrapErrorhelper incore/gitthat wraps a failing git operation's error with its name,and additionally wraps ErrTimeout when the failure was caused by the
command's own timeout (as opposed to a parent context cancellation).
orchestrator.classifyGitError, used at the checkout,apply-requests, and treehash-compute call sites in
native_orchestrator.go, which classifies the error astangoerrors.NewInfrawhen it wrapsgit.ErrTimeout, per theclassification conventions in
docs/errors/errors.md.Test Plan
unit test
Issues