Skip to content

Commit aaabf46

Browse files
authored
fix(gitter): fix fallback when rate limited (#5375)
#5299 adds reclone fallback at the first sign of trouble. If gitter hits a rate limit from GitHub (or other hosts), the fallback logic will make it attempt to reclone (which will still fail with 403). This leaves us with no repo, which is worse than an outdated local repo. This PR adds logic to skip the fallback attempt if the error is 403 and log warning accordingly.
1 parent a01950f commit aaabf46

2 files changed

Lines changed: 38 additions & 8 deletions

File tree

go/cmd/gitter/gitter.go

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -346,8 +346,16 @@ func isAuthError(err error) bool {
346346
errString := err.Error()
347347

348348
return strings.Contains(errString, "could not read Username") ||
349-
strings.Contains(errString, "Authentication failed") ||
350-
strings.Contains(errString, "The requested URL returned error: 403")
349+
strings.Contains(errString, "Authentication failed")
350+
}
351+
352+
func isForbiddenError(err error) bool {
353+
if err == nil {
354+
return false
355+
}
356+
errString := err.Error()
357+
358+
return strings.Contains(errString, "The requested URL returned error: 403")
351359
}
352360

353361
func isNotFoundError(err error) bool {
@@ -540,6 +548,11 @@ func FetchRepo(ctx context.Context, repoURL string, forceUpdate bool) error {
540548

541549
// If still failing or recovery wasn't attempted, reclone the repo as final fallback
542550
if err != nil {
551+
if isForbiddenError(err) {
552+
logger.WarnContext(ctx, "Fetch failed with 403 Forbidden. Using local repo.", slog.Duration("sinceLastFetch", time.Since(accessTime)), slog.Any("err", err))
553+
return nil
554+
}
555+
543556
logger.WarnContext(ctx, "Fetch and reset failed after recovery attempt, deleting repo and recloning", slog.Any("err", err))
544557
if err := os.RemoveAll(repoPath); err != nil {
545558
return fmt.Errorf("failed to remove repo directory for reclone: %w", err)
@@ -732,7 +745,7 @@ func gitHandler(w http.ResponseWriter, r *http.Request) {
732745

733746
// Fetch repo first
734747
if err := doFetch(ctx, repoURL, forceUpdate); err != nil {
735-
if isAuthError(err) {
748+
if isAuthError(err) || isForbiddenError(err) {
736749
statusCode = http.StatusForbidden
737750
} else if isNotFoundError(err) {
738751
statusCode = http.StatusNotFound
@@ -799,7 +812,7 @@ func cacheHandler(w http.ResponseWriter, r *http.Request) {
799812
logger.DebugContext(ctx, "Received request: /cache")
800813

801814
if _, err := getFreshRepo(ctx, repoURL, body.GetForceUpdate()); err != nil {
802-
if isAuthError(err) {
815+
if isAuthError(err) || isForbiddenError(err) {
803816
statusCode = http.StatusForbidden
804817
} else if isNotFoundError(err) {
805818
statusCode = http.StatusNotFound
@@ -870,7 +883,7 @@ func affectedCommitsHandler(w http.ResponseWriter, r *http.Request) {
870883

871884
repo, err := getFreshRepo(ctx, repoURL, body.GetForceUpdate())
872885
if err != nil {
873-
if isAuthError(err) {
886+
if isAuthError(err) || isForbiddenError(err) {
874887
statusCode = http.StatusForbidden
875888
} else if isNotFoundError(err) {
876889
statusCode = http.StatusNotFound
@@ -1010,7 +1023,7 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) {
10101023
return nil, FetchRepo(ctx, repoURL, false)
10111024
}); errFetch != nil {
10121025
logger.ErrorContext(ctx, "Error fetching repo", slog.Any("error", errFetch))
1013-
if isAuthError(errFetch) {
1026+
if isAuthError(errFetch) || isForbiddenError(errFetch) {
10141027
invalidRepoCache.SetWithTTL(repoURL, http.StatusForbidden, 1, invalidRepoTTL)
10151028
statusCode = http.StatusForbidden
10161029
http.Error(w, fmt.Sprintf("Error fetching repository: %v", errFetch), statusCode)
@@ -1048,7 +1061,7 @@ func tagsHandler(w http.ResponseWriter, r *http.Request) {
10481061
return repo.GetRemoteTags(ctx)
10491062
})
10501063
if errLsRemote != nil {
1051-
if isAuthError(errLsRemote) {
1064+
if isAuthError(errLsRemote) || isForbiddenError(errLsRemote) {
10521065
invalidRepoCache.SetWithTTL(repoURL, http.StatusForbidden, 1, invalidRepoTTL)
10531066
statusCode = http.StatusForbidden
10541067
http.Error(w, fmt.Sprintf("Repository authentication failed: %v", errLsRemote), statusCode)

go/cmd/gitter/gitter_test.go

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,9 +146,9 @@ func TestIsAuthError(t *testing.T) {
146146
{errors.New("fatal: Authentication failed for 'https://github.com/google/this-repo-does-not-exist-12345.git/'"), true},
147147
{errors.New("remote: Repository not found"), false},
148148
{errors.New("fatal: could not read Username for 'https://github.com': terminal prompts disabled"), true},
149-
{errors.New("The requested URL returned error: 403"), true},
150149
{errors.New("some other error"), false},
151150
{errors.New("git clone failed: exit status 128"), false},
151+
{nil, false},
152152
}
153153

154154
for _, tt := range tests {
@@ -158,6 +158,23 @@ func TestIsAuthError(t *testing.T) {
158158
}
159159
}
160160

161+
func TestIsForbiddenError(t *testing.T) {
162+
tests := []struct {
163+
err error
164+
expected bool
165+
}{
166+
{errors.New("fatal: unable to access 'https://github.com/composer/composer/': The requested URL returned error: 403"), true},
167+
{errors.New("some other error"), false},
168+
{nil, false},
169+
}
170+
171+
for _, tt := range tests {
172+
if result := isForbiddenError(tt.err); result != tt.expected {
173+
t.Errorf("isForbiddenError(%v) = %v, expected %v", tt.err, result, tt.expected)
174+
}
175+
}
176+
}
177+
161178
func TestIsNotFoundError(t *testing.T) {
162179
tests := []struct {
163180
err error

0 commit comments

Comments
 (0)