Add retries to GitHub client for GetModifiedFiles

This is a follow on to resolve similar issues to runatlantis#1019.

In runatlantis#1131 retries were added to GetPullRequest. And in runatlantis#1810 a backoff
was included.

However, those only resolve one potential request at the very
beginning of a PR creation. The other request that happens early on
during auto-plan is one to ListFiles to detect the modified files. This
too can sometimes result in a 404 due to async updates on the GitHub
side.

server/events/vcs/github_client.go

@@ -60,19 +60,19 @@ type GithubAppTemporarySecrets struct {
 
 // NewGithubClient returns a valid GitHub client.
 func NewGithubClient(hostname string, credentials GithubCredentials, logger logging.SimpleLogging) (*GithubClient, error) {
-	transport, err := credentials.Client()
+	credentialedClient, err := credentials.Client()
 	if err != nil {
-		return nil, errors.Wrap(err, "error initializing github authentication transport")
+		return nil, errors.Wrap(err, "error initializing github authentication client")
 	}
 
 	var graphqlURL string
 	var client *github.Client
 	if hostname == "github.com" {
-		client = github.NewClient(transport)
+		client = github.NewClient(credentialedClient)
 		graphqlURL = "https://api.github.com/graphql"
 	} else {
 		apiURL := resolveGithubAPIURL(hostname)
-		client, err = github.NewEnterpriseClient(apiURL.String(), apiURL.String(), transport)
+		client, err = github.NewEnterpriseClient(apiURL.String(), apiURL.String(), credentialedClient)
 		if err != nil {
 			return nil, err
 		}
@@ -88,7 +88,7 @@ func NewGithubClient(hostname string, credentials GithubCredentials, logger logg
 	// shurcooL's libraries completely.
 	v4MutateClient := graphql.NewClient(
 		graphqlURL,
-		transport,
+		credentialedClient,
 		graphql.WithHeader("Accept", "application/vnd.github.queen-beryl-preview+json"),
 	)
 
@@ -111,19 +111,40 @@ func NewGithubClient(hostname string, credentials GithubCredentials, logger logg
 // relative to the repo root, e.g. parent/child/file.txt.
 func (g *GithubClient) GetModifiedFiles(repo models.Repo, pull models.PullRequest) ([]string, error) {
 	var files []string
-	nextPage := 0
+	nextPage := 1
 	for {
 		opts := github.ListOptions{
+			Page:    nextPage,
 			PerPage: 300,
 		}
-		if nextPage != 0 {
-			opts.Page = nextPage
-		}
-		g.logger.Debug("GET /repos/%v/%v/pulls/%d/files", repo.Owner, repo.Name, pull.Num)
-		pageFiles, resp, err := g.client.PullRequests.ListFiles(g.ctx, repo.Owner, repo.Name, pull.Num, &opts)
-		if err != nil {
-			return files, err
+
+		g.logger.Debug("GET /repos/%v/%v/pulls/%d/files&page=%d", repo.Owner, repo.Name, pull.Num, opts.Page)
+
+		var err error
+		var resp *github.Response
+		var pageFiles []*github.CommitFile
+
+		// Similar to GetPullRequest:
+		// GitHub has started to return 404's here (#1019) even after they send the webhook.
+		// They've got some eventual consistency issues going on so we're just going
+		// to attempt up to 5 times with exponential backoff.
+		maxAttempts := 5
+		attemptDelay := 0 * time.Second
+		for i := 0; i < maxAttempts; i++ {
+			// First don't sleep, then sleep 1, 3, 7, etc.
+			time.Sleep(attemptDelay)
+			attemptDelay = 2*attemptDelay + 1*time.Second
+
+			pageFiles, resp, err = g.client.PullRequests.ListFiles(g.ctx, repo.Owner, repo.Name, pull.Num, &opts)
+			if err == nil {
+				break
+			}
+			ghErr, ok := err.(*github.ErrorResponse)
+			if !ok || ghErr.Response.StatusCode != 404 {
+				return files, err
+			}
 		}
+
 		for _, f := range pageFiles {
 			files = append(files, f.GetFilename())
 

server/events/vcs/github_client_test.go

@@ -40,19 +40,37 @@ func TestGithubClient_GetModifiedFiles(t *testing.T) {
 ]`
 	firstResp := fmt.Sprintf(respTemplate, "file1.txt")
 	secondResp := fmt.Sprintf(respTemplate, "file2.txt")
+	maxCalls := 2
+	numCalls := 0
 	testServer := httptest.NewTLSServer(
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			switch r.RequestURI {
 			// The first request should hit this URL.
-			case "/api/v3/repos/owner/repo/pulls/1/files?per_page=300":
-				// We write a header that means there's an additional page.
-				w.Header().Add("Link", `<https://api.github.com/resource?page=2>; rel="next",
+			case "/api/v3/repos/owner/repo/pulls/1/files?page=1&per_page=300":
+				if numCalls < maxCalls {
+					numCalls++
+					t.Logf("forcing retry %q", r.RequestURI)
+					http.Error(w, "not found", http.StatusNotFound)
+					return
+				} else {
+					// decrement numCalls to force one retry on page 2 as well
+					numCalls--
+					// We write a header that means there's an additional page.
+					w.Header().Add("Link", `<https://api.github.com/resource?page=2>; rel="next",
       <https://api.github.com/resource?page=2>; rel="last"`)
-				w.Write([]byte(firstResp)) // nolint: errcheck
-				return
+					w.Write([]byte(firstResp)) // nolint: errcheck
+					return
+				}
 				// The second should hit this URL.
 			case "/api/v3/repos/owner/repo/pulls/1/files?page=2&per_page=300":
-				w.Write([]byte(secondResp)) // nolint: errcheck
+				if numCalls < maxCalls {
+					numCalls++
+					t.Logf("forcing retry %q", r.RequestURI)
+					http.Error(w, "not found", http.StatusNotFound)
+					return
+				} else {
+					w.Write([]byte(secondResp)) // nolint: errcheck
+				}
 			default:
 				t.Errorf("got unexpected request at %q", r.RequestURI)
 				http.Error(w, "not found", http.StatusNotFound)
@@ -105,7 +123,7 @@ func TestGithubClient_GetModifiedFilesMovedFile(t *testing.T) {
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			switch r.RequestURI {
 			// The first request should hit this URL.
-			case "/api/v3/repos/owner/repo/pulls/1/files?per_page=300":
+			case "/api/v3/repos/owner/repo/pulls/1/files?page=1&per_page=300":
 				w.Write([]byte(resp)) // nolint: errcheck
 				return
 			default:
@@ -864,7 +882,7 @@ func TestGithubClient_SplitComments(t *testing.T) {
 }
 
 // Test that we retry the get pull request call if it 404s.
-func TestGithubClient_Retry404(t *testing.T) {
+func TestGithubClient_GetPullRequestRetry404(t *testing.T) {
 	var numCalls = 0
 
 	testServer := httptest.NewTLSServer(