diff --git a/go.mod b/go.mod index c7c7d32e..7f76a372 100644 --- a/go.mod +++ b/go.mod @@ -5,13 +5,18 @@ go 1.24.0 toolchain go1.25.10 require ( + github.com/cenkalti/backoff/v7 v7.0.0 github.com/jessevdk/go-flags v1.6.1 github.com/shurcooL/githubv4 v0.0.0-20240727222349-48295856cce7 + github.com/stretchr/testify v1.11.1 golang.org/x/oauth2 v0.33.0 ) require ( + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/shurcooL/graphql v0.0.0-20220606043923-3cf50f8a0a29 // indirect golang.org/x/net v0.38.0 // indirect golang.org/x/sys v0.31.0 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index 2045d0e4..a9626332 100644 --- a/go.sum +++ b/go.sum @@ -1,12 +1,24 @@ +github.com/cenkalti/backoff/v7 v7.0.0 h1:ZP+QAaaOnVUHo+ufFpZ835hbT3x2fy+h2lecVEosZ6A= +github.com/cenkalti/backoff/v7 v7.0.0/go.mod h1:qcKBGwsu4hpxHtQ8tWYsQ+ifzx2+sS+Xx/3jfe30lI8= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/jessevdk/go-flags v1.6.1 h1:Cvu5U8UGrLay1rZfv/zP7iLpSHGUZ/Ou68T0iX1bBK4= github.com/jessevdk/go-flags v1.6.1/go.mod h1:Mk8T1hIAWpOiJiHa9rJASDK2UGWji0EuPGBnNLMooyc= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/shurcooL/githubv4 v0.0.0-20240727222349-48295856cce7 h1:cYCy18SHPKRkvclm+pWm1Lk4YrREb4IOIb/YdFO0p2M= github.com/shurcooL/githubv4 v0.0.0-20240727222349-48295856cce7/go.mod h1:zqMwyHmnN/eDOZOdiTohqIUKUrTFX62PNlu7IJdu0q8= github.com/shurcooL/graphql v0.0.0-20220606043923-3cf50f8a0a29 h1:B1PEwpArrNp4dkQrfxh/abbBAOZBVp0ds+fBEOUOqOc= github.com/shurcooL/graphql v0.0.0-20220606043923-3cf50f8a0a29/go.mod h1:AuYgA5Kyo4c7HfUmvRGs/6rGlMMV/6B1bVnB9JxJEEg= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= golang.org/x/net v0.38.0 h1:vRMAPTMaeGqVhG5QyLJHqNDwecKTomGeqbnfZyKlBI8= golang.org/x/net v0.38.0/go.mod h1:ivrbrMbzFq5J41QOQh0siUuly180yBYtLp+CKbEaFx8= golang.org/x/oauth2 v0.33.0 h1:4Q+qn+E5z8gPRJfmRy7C2gGG3T4jIprK6aSYgTXGRpo= golang.org/x/oauth2 v0.33.0/go.mod h1:lzm5WQJQwKZ3nwavOZ3IS5Aulzxi68dUSgRHujetwEA= golang.org/x/sys v0.31.0 h1:ioabZlmFYtWhL+TRYpcnNlLwhyxaM9kWTDEmfnprqik= golang.org/x/sys v0.31.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/main.go b/main.go index 6b3e25d1..de26a534 100644 --- a/main.go +++ b/main.go @@ -10,7 +10,9 @@ import ( "os" "os/exec" "strings" + "time" + "github.com/cenkalti/backoff/v7" "github.com/jessevdk/go-flags" "github.com/shurcooL/githubv4" "golang.org/x/oauth2" @@ -21,14 +23,16 @@ const githubGraphQLURL = "https://api.github.com/graphql" var version = "development" var opts struct { - Adds []string `short:"a" long:"add" description:"Added or modified file to commit. Use multiple times for multiple files." env:"GHCOMMIT_ADD"` - Deletes []string `short:"d" long:"delete" description:"Deleted file to commit. Use multiple times for multiple files." env:"GHCOMMIT_DELETE"` - Empty bool `short:"e" long:"empty" description:"Allow empty commit." env:"GHCOMMIT_EMPTY"` - Message string `short:"m" long:"message" description:"Commit message" env:"GHCOMMIT_MESSAGE" required:"true"` - Repository string `short:"r" long:"repository" description:"Owner/Repository to commit to." env:"GHCOMMIT_REPOSITORY" required:"true"` - Branch string `short:"b" long:"branch" description:"Branch to commit to." env:"GHCOMMIT_BRANCH" required:"true"` - HeadSHA string `short:"s" long:"sha" description:"Commit SHA of the HEAD branch to apply to. Acts as a safety check to ensure the right branch is modified. The output of 'git rev-parse HEAD' is used if not set" env:"GHCOMMIT_SHA"` - Version bool `short:"v" long:"version" description:"Print version and exit"` + Adds []string `short:"a" long:"add" description:"Added or modified file to commit. Use multiple times for multiple files." env:"GHCOMMIT_ADD"` + Deletes []string `short:"d" long:"delete" description:"Deleted file to commit. Use multiple times for multiple files." env:"GHCOMMIT_DELETE"` + Empty bool `short:"e" long:"empty" description:"Allow empty commit." env:"GHCOMMIT_EMPTY"` + Message string `short:"m" long:"message" description:"Commit message" env:"GHCOMMIT_MESSAGE" required:"true"` + Repository string `short:"r" long:"repository" description:"Owner/Repository to commit to." env:"GHCOMMIT_REPOSITORY" required:"true"` + Branch string `short:"b" long:"branch" description:"Branch to commit to." env:"GHCOMMIT_BRANCH" required:"true"` + HeadSHA string `short:"s" long:"sha" description:"Commit SHA of the HEAD branch to apply to. Acts as a safety check to ensure the right branch is modified. The output of 'git rev-parse HEAD' is used if not set" env:"GHCOMMIT_SHA"` + Retries int `long:"retries" description:"Number of times to retry the commit if the branch head moved during the run (a concurrent push causes a STALE_DATA conflict). On conflict the current branch head is re-read and the same file changes are re-applied on top of it." env:"GHCOMMIT_RETRIES" default:"0"` + RetryWait time.Duration `long:"retry-wait" description:"Base backoff between retries; grows exponentially with jitter. Only used when --retries > 0." env:"GHCOMMIT_RETRY_WAIT" default:"2s"` + Version bool `short:"v" long:"version" description:"Print version and exit"` } func main() { @@ -100,15 +104,6 @@ func main() { }) } - // the actual mutation request - var m struct { - CreateCommitOnBranch struct { - Commit struct { - URL string - } - } `graphql:"createCommitOnBranch(input:$input)"` - } - // create the $input struct for the graphQL createCommitOnBranch mutation request: input := githubv4.CreateCommitOnBranchInput{ Branch: githubv4.CommittableBranch{ @@ -126,10 +121,103 @@ func main() { ExpectedHeadOid: githubv4.GitObjectID(expectedHeadOid), } - if err := client.Mutate(ctx, &m, input, nil); err != nil { + url, err := commitWithRetry(ctx, client, opts.Repository, opts.Branch, input, opts.Retries, opts.RetryWait) + if err != nil { log.Fatal(err) } - log.Printf("Success. New commit: %s", m.CreateCommitOnBranch.Commit.URL) + log.Printf("Success. New commit: %s", url) +} + +// commitWithRetry runs the createCommitOnBranch mutation, retrying on stale-head +// conflicts up to retries times. On conflict it re-reads the branch head and +// updates input.ExpectedHeadOid so the retry re-applies the same file changes on +// top of whatever concurrently moved the branch. Non-conflict errors are +// permanent. retries == 0 makes exactly one attempt. +func commitWithRetry(ctx context.Context, client *githubv4.Client, repository, branch string, input githubv4.CreateCommitOnBranchInput, retries int, wait time.Duration) (string, error) { + var m struct { + CreateCommitOnBranch struct { + Commit struct { + URL string + } + } `graphql:"createCommitOnBranch(input:$input)"` + } + + commit := func() (string, error) { + err := client.Mutate(ctx, &m, input, nil) + if err == nil { + return m.CreateCommitOnBranch.Commit.URL, nil + } + if !isStaleHeadConflict(err) { + return "", backoff.Permanent(err) + } + newHead, refreshErr := currentHeadOid(ctx, client, repository, branch) + if refreshErr != nil { + return "", backoff.Permanent(fmt.Errorf("commit conflicted with a concurrent update, but refreshing the branch head failed: %v (original error: %w)", refreshErr, err)) + } + log.Printf("commit conflicted with a concurrent update; refreshed head to %s, retrying", newHead) + input.ExpectedHeadOid = newHead + return "", err + } + + b := backoff.NewExponentialBackOff() + if wait > 0 { + b.InitialInterval = wait + } + url, err := backoff.Retry(ctx, commit, + backoff.WithBackOff(b), + backoff.WithMaxTries(uint(retries)+1), // tries = 1 + retries + backoff.WithMaxElapsedTime(0), // bound by try count only, not wall-clock + ) + if err != nil { + // Unwrap so callers see the underlying GitHub error, not the RetryError. + if re := backoff.AsRetryError(err); re != nil && re.LastErr != nil { + return "", re.LastErr + } + return "", err + } + return url, nil +} + +// isStaleHeadConflict matches on the message text because githubv4 surfaces the +// GraphQL STALE_DATA error as a plain error with no typed code. +func isStaleHeadConflict(err error) bool { + if err == nil { + return false + } + msg := strings.ToLower(err.Error()) + return strings.Contains(msg, "expected branch to point to") || + strings.Contains(msg, "pull and try again") +} + +// currentHeadOid returns the commit oid at the tip of branch. It reuses the +// caller's client so the GITHUB_GRAPHQL_URL (GHES) endpoint is honored. +func currentHeadOid(ctx context.Context, client *githubv4.Client, repository, branch string) (githubv4.GitObjectID, error) { + owner, name, ok := strings.Cut(repository, "/") + if !ok || owner == "" || name == "" { + return "", fmt.Errorf("invalid repository %q, expected \"owner/name\"", repository) + } + + var q struct { + Repository struct { + Ref *struct { + Target *struct { + Oid githubv4.GitObjectID + } + } `graphql:"ref(qualifiedName: $qualifiedName)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + vars := map[string]interface{}{ + "owner": githubv4.String(owner), + "name": githubv4.String(name), + "qualifiedName": githubv4.String("refs/heads/" + branch), + } + if err := client.Query(ctx, &q, vars); err != nil { + return "", err + } + if q.Repository.Ref == nil || q.Repository.Ref.Target == nil { + return "", fmt.Errorf("branch %q not found in %s", branch, repository) + } + return q.Repository.Ref.Target.Oid, nil } func base64EncodeFile(path string) (string, error) { diff --git a/main_test.go b/main_test.go new file mode 100644 index 00000000..c18144ff --- /dev/null +++ b/main_test.go @@ -0,0 +1,167 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestIsStaleHeadConflict(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {"nil error", nil, false}, + {"real stale-head message", errors.New(`Expected branch to point to "abc123" but it did not. Pull and try again.`), true}, + {"message fragment, different casing", errors.New("EXPECTED BRANCH TO POINT TO something"), true}, + {"pull and try again fragment only", errors.New("some wrapper: pull and try again"), true}, + {"unrelated auth error", errors.New("401 Unauthorized: bad credentials"), false}, + {"unrelated not found error", errors.New("Could not resolve to a Repository with the name 'owner/repo'."), false}, + {"unrelated network error", errors.New("dial tcp: connection refused"), false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isStaleHeadConflict(tt.err)) + }) + } +} + +const staleErrorBody = `{"errors":[{"message":"Expected branch to point to \"deadbeef\" but it did not. Pull and try again.","type":"STALE_DATA"}]}` + +// mockGitHub is an httptest handler standing in for the GitHub GraphQL API. The +// createCommitOnBranch mutation returns a stale-head conflict for its first +// conflictsBeforeSuccess calls (or mutateErrBody forever, if set), then succeeds; +// the head-refresh query returns a fresh oid each time. +type mockGitHub struct { + conflictsBeforeSuccess int + mutateErrBody string + + mu sync.Mutex + mutateCalls int + refreshCalls int + mutateOids []string // expectedHeadOid sent on each mutation, in order +} + +func (g *mockGitHub) ServeHTTP(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + var env struct { + Query string `json:"query"` + Variables struct { + Input struct { + ExpectedHeadOid string `json:"expectedHeadOid"` + } `json:"input"` + } `json:"variables"` + } + if err := json.Unmarshal(body, &env); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + g.mu.Lock() + defer g.mu.Unlock() + + var resp string + if strings.Contains(env.Query, "createCommitOnBranch") { + g.mutateCalls++ + g.mutateOids = append(g.mutateOids, env.Variables.Input.ExpectedHeadOid) + switch { + case g.mutateErrBody != "": + resp = g.mutateErrBody + case g.mutateCalls <= g.conflictsBeforeSuccess: + resp = staleErrorBody + default: + resp = `{"data":{"createCommitOnBranch":{"commit":{"url":"https://example.test/commit/final"}}}}` + } + } else { + g.refreshCalls++ + resp = fmt.Sprintf(`{"data":{"repository":{"ref":{"target":{"oid":"refreshed-%d"}}}}}`, g.refreshCalls) + } + _, _ = io.WriteString(w, resp) +} + +// newTestClient starts an httptest server backed by gh and returns a real +// githubv4 client pointed at it, so the tests exercise the actual request +// encoding and GraphQL error decoding rather than a hand-faked client. +func newTestClient(t *testing.T, gh *mockGitHub) *githubv4.Client { + t.Helper() + srv := httptest.NewServer(gh) + t.Cleanup(srv.Close) + return githubv4.NewEnterpriseClient(srv.URL, srv.Client()) +} + +func newTestInput() githubv4.CreateCommitOnBranchInput { + return githubv4.CreateCommitOnBranchInput{ + Branch: githubv4.CommittableBranch{ + RepositoryNameWithOwner: githubv4.NewString("dash0hq/ghcommit"), + BranchName: githubv4.NewString("main"), + }, + Message: githubv4.CommitMessage{Headline: "test"}, + ExpectedHeadOid: githubv4.GitObjectID("original-oid"), + } +} + +func TestCommitWithRetry(t *testing.T) { + t.Run("succeeds on first attempt without refreshing", func(t *testing.T) { + gh := &mockGitHub{conflictsBeforeSuccess: 0} + client := newTestClient(t, gh) + + url, err := commitWithRetry(context.Background(), client, "dash0hq/ghcommit", "main", newTestInput(), 0, time.Millisecond) + + require.NoError(t, err) + assert.Equal(t, "https://example.test/commit/final", url) + assert.Equal(t, 1, gh.mutateCalls) + assert.Equal(t, 0, gh.refreshCalls) + }) + + t.Run("recovers within retries, refreshing the head each conflict", func(t *testing.T) { + gh := &mockGitHub{conflictsBeforeSuccess: 2} + client := newTestClient(t, gh) + + url, err := commitWithRetry(context.Background(), client, "dash0hq/ghcommit", "main", newTestInput(), 3, time.Millisecond) + + require.NoError(t, err) + assert.Equal(t, "https://example.test/commit/final", url) + assert.Equal(t, 3, gh.mutateCalls) + assert.Equal(t, 2, gh.refreshCalls) + // Each retry must carry the freshly-read head oid, not the stale one. + assert.Equal(t, []string{"original-oid", "refreshed-1", "refreshed-2"}, gh.mutateOids) + }) + + t.Run("returns the conflict error when retries are exhausted", func(t *testing.T) { + gh := &mockGitHub{conflictsBeforeSuccess: 100} + client := newTestClient(t, gh) + + url, err := commitWithRetry(context.Background(), client, "dash0hq/ghcommit", "main", newTestInput(), 2, time.Millisecond) + + require.Error(t, err) + assert.True(t, isStaleHeadConflict(err), "error should be the stale-head conflict, got: %v", err) + assert.Empty(t, url) + assert.Equal(t, 3, gh.mutateCalls) // initial attempt + 2 retries + }) + + t.Run("does not retry a non-conflict error", func(t *testing.T) { + gh := &mockGitHub{mutateErrBody: `{"errors":[{"message":"Could not resolve to a Repository with the name 'x'."}]}`} + client := newTestClient(t, gh) + + _, err := commitWithRetry(context.Background(), client, "dash0hq/ghcommit", "main", newTestInput(), 3, time.Millisecond) + + require.Error(t, err) + assert.False(t, isStaleHeadConflict(err), "non-conflict error should not be treated as a stale-head conflict") + assert.Equal(t, 1, gh.mutateCalls) + assert.Equal(t, 0, gh.refreshCalls) + }) +}