Retry git clone on secondary rate limit (bare 403/429) (#5260)

Clone-time 403s were classified as permanent permission errors and
failed the scan immediately, even though a bare 403 (no auth/remote
denial message) matches GitHub/GitLab secondary rate limiting, which
per their docs returns 403 or 429. Retry those like 429 with backoff.

pkg/sources/git/git.go

@@ -41,11 +41,18 @@ import (
 const (
 	SourceType = sourcespb.SourceType_SOURCE_TYPE_GIT
 	// maxCloneAttempts is the total number of times a clone is attempted when
-	// each failure is classified as a transient network error.
+	// each failure is classified as a transient network or rate-limit error.
 	maxCloneAttempts = 3
-	// cloneRetryBackoff is the base wait between clone attempts; it is
-	// multiplied by the number of failed attempts so far.
+	// cloneRetryBackoff is the base wait between clone attempts after a
+	// transient network error; it is multiplied by the number of failed
+	// attempts so far.
 	cloneRetryBackoff = 5 * time.Second
+	// cloneRateLimitBackoff is the base wait after a clone attempt fails
+	// with what looks like a GitHub/GitLab secondary rate limit (bare 403
+	// or 429). GitHub's guidance for secondary rate limits is to wait at
+	// least a minute before retrying, so this starts well above the
+	// network-error backoff and grows per attempt.
+	cloneRateLimitBackoff = 60 * time.Second
 )
 
 type Source struct {
@@ -484,8 +491,9 @@ type cloneParams struct {
 // outer function for centralized error handling and cleanup.
 //
 // Failures classified as transient network errors (e.g. a connection reset
-// mid-transfer) are retried up to maxCloneAttempts times, each attempt
-// starting from a fresh clone directory. All other failures, including clone
+// mid-transfer) or as a secondary rate limit (a bare 403 or 429 from the
+// remote) are retried up to maxCloneAttempts times, each attempt starting
+// from a fresh clone directory. All other failures, including clone
 // timeouts (see feature.GitCloneTimeoutDuration), are returned immediately.
 func CloneRepo(ctx context.Context, userInfo *url.Userinfo, gitURL string, clonePath string, authInUrl bool, args ...string) (string, *git.Repository, error) {
 	timeout := time.Duration(feature.GitCloneTimeoutDuration.Load())
@@ -520,15 +528,17 @@ func CloneRepo(ctx context.Context, userInfo *url.Userinfo, gitURL string, clone
 			return "", nil, fmt.Errorf("failed to clean clone path for retry: %w (original clone error: %w)", rmErr, err)
 		}
 
-		ctx.Logger().Info("git clone interrupted by network error; retrying",
+		delay := cloneRetryDelay(err, attempt)
+		ctx.Logger().Info("git clone interrupted; retrying",
 			"attempt", attempt,
 			"max_attempts", maxCloneAttempts,
+			"delay", delay.String(),
 			"error", err.Error(),
 		)
 		select {
 		case <-ctx.Done():
 			return "", nil, ctx.Err()
-		case <-time.After(cloneRetryBackoff * time.Duration(attempt)):
+		case <-time.After(delay):
 		}
 	}
 
@@ -536,10 +546,30 @@ func CloneRepo(ctx context.Context, userInfo *url.Userinfo, gitURL string, clone
 }
 
 // isRetryableCloneError reports whether a clone failure looks like a
-// transient network interruption (e.g. a connection reset mid-transfer)
-// rather than a permanent condition like an auth or permission error.
+// transient condition (a network interruption, e.g. a connection reset
+// mid-transfer, or a secondary rate limit) rather than a permanent
+// condition like an auth or permission error.
 func isRetryableCloneError(err error) bool {
-	return err != nil && ClassifyCloneError(err.Error()) == cloneFailureNetwork
+	if err == nil {
+		return false
+	}
+	switch ClassifyCloneError(err.Error()) {
+	case cloneFailureNetwork, cloneFailureRateLimit:
+		return true
+	default:
+		return false
+	}
+}
+
+// cloneRetryDelay returns how long to wait before the next clone attempt,
+// scaled by the failure class: rate-limit errors back off much more slowly
+// than transient network errors, since GitHub/GitLab secondary rate limits
+// take on the order of a minute or more to clear.
+func cloneRetryDelay(err error, attempt int) time.Duration {
+	if ClassifyCloneError(err.Error()) == cloneFailureRateLimit {
+		return cloneRateLimitBackoff * time.Duration(attempt)
+	}
+	return cloneRetryBackoff * time.Duration(attempt)
 }
 
 // createClonePath creates the directory a repository will be cloned into and

pkg/sources/git/git_test.go

@@ -76,14 +76,36 @@ error: 7457 bytes of body are still expected
 fetch-pack: unexpected disconnect while reading sideband packet
 fatal: early EOF
 fatal: fetch-pack: invalid index-pack output`,
+		// Bare 403/429 during clone matches GitHub/GitLab secondary rate
+		// limiting (no accompanying auth/permission message).
+		"could not clone repo: https://github.com/org/repo.git, error executing git clone: exit status 128, fatal: unable to access 'https://github.com/org/repo.git/': The requested URL returned error: 403",
+		"could not clone repo: https://github.com/org/repo.git, error executing git clone: exit status 128, The requested URL returned error: 429",
+		// Normal clone progress ("remote: Counting objects...") must not be
+		// mistaken for a denial explanation when a later 403 is just throttling.
+		`could not clone repo: https://github.com/org/repo.git, error executing git clone: exit status 128, remote: Enumerating objects: 100, done.
+remote: Counting objects: 100% (100/100), done.
+fatal: unable to access 'https://github.com/org/repo.git/': The requested URL returned error: 403`,
 	}
 	for _, msg := range retryable {
 		assert.True(t, isRetryableCloneError(errors.New(msg)), "expected retryable: %q", msg)
 	}
 
 	notRetryable := []string{
-		"could not clone repo: https://github.com/org/repo.git, error executing git clone: exit status 128, remote: The requested URL returned error: 403",
-		"could not clone repo: https://github.com/org/repo.git, error executing git clone: exit status 128, The requested URL returned error: 429",
+		// An explicit permission-denial message, as opposed to a bare 403.
+		"could not clone repo: https://github.com/org/repo.git, error executing git clone: exit status 128, remote: You are not allowed to download code from this project.",
+		// The explicit denial message must win even when the combined clone
+		// output also contains a literal "403" elsewhere.
+		"could not clone repo: https://gitlab.com/org/repo.git, error executing git clone: exit status 128, fatal: unable to access 'https://gitlab.com/org/repo.git/': The requested URL returned error: 403\nremote: You are not allowed to download code from this project.",
+		// GitHub's explicit permission-denial message, which also co-occurs
+		// with a literal "403" in the combined clone output.
+		"could not clone repo: https://github.com/org/repo.git, error executing git clone: exit status 128, remote: Permission to org/repo.git denied to user.\nfatal: unable to access 'https://github.com/org/repo.git/': The requested URL returned error: 403",
+		// GitHub Apps/fine-grained-token variant: "Write access ... not
+		// granted", also co-occurring with a literal 403.
+		"could not clone repo: https://github.com/org/repo.git, error executing git clone: exit status 128, remote: Write access to repository not granted.\nfatal: unable to access 'https://github.com/org/repo.git/': The requested URL returned error: 403",
+		// SAML SSO enforcement: an unrecognized "remote:" explanation
+		// co-occurring with a 403, which the classifier must treat as a
+		// permanent failure without needing this exact wording enumerated.
+		"could not clone repo: https://github.com/org/repo.git, error executing git clone: exit status 128, remote: The organization has enabled or enforced SAML SSO.\nfatal: unable to access 'https://github.com/org/repo.git/': The requested URL returned error: 403",
 		"git clone timed out (after 1h0m0s)",
 		"could not clone repo: https://github.com/org/repo.git, error executing git clone: exit status 128, fatal: repository 'https://github.com/org/repo.git/' not found",
 	}

pkg/sources/git/metrics.go

@@ -105,14 +105,27 @@ func ClassifyCloneError(errMsg string) string {
 		strings.Contains(errMsg, "redirect:") && strings.Contains(errMsg, "users/sign_in"):
 		return cloneFailureAuth
 
-	case strings.Contains(errMsg, "The requested URL returned error: 429") ||
-		strings.Contains(errMsg, "remote: Retry later"):
-		return cloneFailureRateLimit
-
-	case strings.Contains(errMsg, "The requested URL returned error: 403") ||
-		strings.Contains(errMsg, "remote: You are not allowed to download code from this project"):
+	// Checked before the generic 403/429 case below: when the git backend
+	// itself explains a 403/429 with a "remote:" line, that's a permanent
+	// failure (permission denial, SAML SSO enforcement, org policy, etc.),
+	// not throttling — the backend only sends an explanatory message once
+	// the request has been evaluated, whereas secondary-rate-limit responses
+	// are either a bare 403/429 with no remote explanation or one of the
+	// small, stable set of known throttling messages. Matching on "any
+	// unrecognized remote: explanation" avoids having to enumerate every
+	// provider's denial wording, which changes per provider and per feature
+	// (SSO, fine-grained tokens, GitHub Apps, ...).
+	case isRemotePermissionDenial(errMsg):
 		return cloneFailurePermission
 
+	case strings.Contains(errMsg, "The requested URL returned error: 429") ||
+		strings.Contains(errMsg, "The requested URL returned error: 403") ||
+		strings.Contains(errMsg, "remote: Retry later"):
+		// A bare "403" during clone (no accompanying auth/permission message)
+		// matches GitHub/GitLab secondary rate limiting and abuse-detection
+		// responses, which are documented to return either 403 or 429.
+		return cloneFailureRateLimit
+
 	case strings.Contains(errMsg, "RPC failed") ||
 		strings.Contains(errMsg, "unexpected disconnect") ||
 		strings.Contains(errMsg, "early EOF") ||
@@ -128,3 +141,63 @@ func ClassifyCloneError(errMsg string) string {
 		return cloneFailureOther
 	}
 }
+
+// knownRateLimitRemoteMessages are the (small, stable) set of ways GitHub and
+// GitLab phrase an actual throttling response in a "remote:" line. Any other
+// "remote:" explanation accompanying a 403/429 is treated as a permanent
+// failure rather than added here, since provider denial wording (permission,
+// SSO, org policy, ...) varies far more than throttling wording does.
+var knownRateLimitRemoteMessages = []string{
+	"retry later",
+	"secondary rate limit",
+	"rate limit exceeded",
+	"abuse detection",
+}
+
+// knownBenignRemoteProgressMessages are the small, standardized set of
+// progress lines git-upload-pack prints to "remote:" during a normal clone
+// (e.g. "remote: Counting objects: 100% (5/5), done."). These can appear
+// ahead of an unrelated 403/429 or network failure, so they must not be
+// mistaken for a denial explanation.
+var knownBenignRemoteProgressMessages = []string{
+	"enumerating objects",
+	"counting objects",
+	"compressing objects",
+	"writing objects",
+	"resolving deltas",
+	"total ",
+}
+
+// knownBenignRemoteMessages is the combined set of "remote:" line contents
+// that must NOT be treated as a permission denial: known throttling messages
+// and known clone-progress output.
+var knownBenignRemoteMessages = append(append([]string{}, knownRateLimitRemoteMessages...), knownBenignRemoteProgressMessages...)
+
+// isRemotePermissionDenial reports whether errMsg contains a server-side
+// "remote:" line explaining a 403/429 as something other than throttling or
+// normal clone progress output, as opposed to a bare curl-level 403/429 with
+// no explanation (or an explicit rate-limit message), which may just be
+// transient rate limiting.
+func isRemotePermissionDenial(errMsg string) bool {
+	if !strings.Contains(errMsg, "403") && !strings.Contains(errMsg, "429") {
+		return false
+	}
+	for _, line := range strings.Split(errMsg, "\n") {
+		idx := strings.Index(line, "remote:")
+		if idx == -1 {
+			continue
+		}
+		remoteMsg := strings.ToLower(line[idx:])
+		benign := false
+		for _, marker := range knownBenignRemoteMessages {
+			if strings.Contains(remoteMsg, marker) {
+				benign = true
+				break
+			}
+		}
+		if !benign {
+			return true
+		}
+	}
+	return false
+}