From 916c43fa009f242fe66b8fa0656a70f9e2a08b17 Mon Sep 17 00:00:00 2001 From: Joey L Date: Fri, 21 Aug 2026 06:00:33 +0000 Subject: [PATCH 1/3] move error logging into the singleflight Do --- go/cmd/gitter/git.go | 16 ++++++++++++---- go/cmd/gitter/gitter.go | 25 ++++++++++++++++++------- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/go/cmd/gitter/git.go b/go/cmd/gitter/git.go index d81064ad1d2..60a8f352c5d 100644 --- a/go/cmd/gitter/git.go +++ b/go/cmd/gitter/git.go @@ -221,11 +221,15 @@ func refreshRepo(ctx context.Context, repoURL string, forceUpdate bool) error { func SyncRepoOnDisk(ctx context.Context, repoURL string, opts FetchOptions) (*Repository, error) { _, err, _ := gFetch.Do(repoURL, func() (any, error) { return runWithConcurrencyControl(ctx, opts.SkipReqConcurrencySemaphore, func() (any, error) { - return nil, refreshRepo(ctx, repoURL, opts.ForceUpdate) + err := refreshRepo(ctx, repoURL, opts.ForceUpdate) + if err != nil { + logger.ErrorContext(ctx, "Error syncing repository on disk", slog.Any("error", err)) + } + + return nil, err }) }) if err != nil { - logger.ErrorContext(ctx, "Error syncing repository on disk", slog.Any("error", err)) return nil, err } @@ -256,11 +260,15 @@ func LoadRepo(ctx context.Context, repoURL string, opts FetchOptions) (*Reposito repoLock.RLock() defer repoLock.RUnlock() - return LoadRepository(ctx, repoPath) + repo, err := LoadRepository(ctx, repoPath) + if err != nil { + logger.ErrorContext(ctx, "Failed to load repository", slog.Any("error", err)) + } + + return repo, err }) }) if err != nil { - logger.ErrorContext(ctx, "Failed to load repository", slog.Any("error", err)) return nil, err } repo := repoAny.(*Repository) diff --git a/go/cmd/gitter/gitter.go b/go/cmd/gitter/gitter.go index ea773c59c8b..48ac3e0f037 100644 --- a/go/cmd/gitter/gitter.go +++ b/go/cmd/gitter/gitter.go @@ -471,11 +471,15 @@ func gitHandler(w http.ResponseWriter, req *http.Request) { // Archive repo fileDataAny, err, _ := gArchive.Do(repoURL, func() (any, error) { return runWithConcurrencyControl(ctx, true, func() (any, error) { - return ArchiveRepo(ctx, repoURL) + data, err := ArchiveRepo(ctx, repoURL) + if err != nil { + logger.ErrorContext(ctx, "Error archiving blob", slog.Any("error", err)) + } + + return data, err }) }) if err != nil { - logger.ErrorContext(ctx, "Error archiving blob", slog.Any("error", err)) statusCode = http.StatusInternalServerError http.Error(w, fmt.Sprintf("Error archiving blob: %v", err), statusCode) @@ -722,7 +726,6 @@ func tagsHandler(w http.ResponseWriter, req *http.Request) { if repo.repoPath != "" { logger.DebugContext(ctx, "Local repo found, using show-ref") if _, errFetch := SyncRepoOnDisk(ctx, repoURL, FetchOptions{ForceUpdate: false, SkipReqConcurrencySemaphore: false}); errFetch != nil { - logger.ErrorContext(ctx, "Error fetching repo", slog.Any("error", errFetch)) if isAuthError(errFetch) || isForbiddenError(errFetch) { invalidRepoCache.SetWithTTL(repoURL, http.StatusForbidden, 1, invalidRepoTTL) statusCode = http.StatusForbidden @@ -744,10 +747,14 @@ func tagsHandler(w http.ResponseWriter, req *http.Request) { } tagsMapAny, errLocal, _ := gLocalTags.Do(repoURL, func() (any, error) { - return repo.GetLocalTags(ctx) + tags, err := repo.GetLocalTags(ctx) + if err != nil { + logger.ErrorContext(ctx, "Error parsing local tags", slog.Any("error", err)) + } + + return tags, err }) if errLocal != nil { - logger.ErrorContext(ctx, "Error parsing local tags", slog.Any("error", errLocal)) statusCode = http.StatusInternalServerError http.Error(w, "Error parsing local tags", statusCode) @@ -758,7 +765,12 @@ func tagsHandler(w http.ResponseWriter, req *http.Request) { // If repo is not on disk, we use ls-remote to get the tags instead logger.DebugContext(ctx, "Local repo not found, using ls-remote") tagsMapAny, errLsRemote, _ := gLsRemote.Do(repoURL, func() (any, error) { - return repo.GetRemoteTags(ctx) + tags, err := repo.GetRemoteTags(ctx) + if err != nil && !isAuthError(err) && !isForbiddenError(err) && !isNotFoundError(err) { + logger.ErrorContext(ctx, "Error running git ls-remote", slog.Any("error", err)) + } + + return tags, err }) if errLsRemote != nil { if isAuthError(errLsRemote) || isForbiddenError(errLsRemote) { @@ -775,7 +787,6 @@ func tagsHandler(w http.ResponseWriter, req *http.Request) { return } - logger.ErrorContext(ctx, "Error running git ls-remote", slog.Any("error", errLsRemote)) statusCode = http.StatusInternalServerError http.Error(w, "Error listing remote tags", statusCode) From d11de19a0187d3b4cae111054a4dd2937db810cf Mon Sep 17 00:00:00 2001 From: Joey L Date: Mon, 24 Aug 2026 03:44:47 +0000 Subject: [PATCH 2/3] Put http ststus code mapping into helper --- go/cmd/gitter/errors.go | 21 +++++++++++ go/cmd/gitter/gitter.go | 78 +++++++---------------------------------- 2 files changed, 34 insertions(+), 65 deletions(-) diff --git a/go/cmd/gitter/errors.go b/go/cmd/gitter/errors.go index 1db00e5a046..48d09f2d6d1 100644 --- a/go/cmd/gitter/errors.go +++ b/go/cmd/gitter/errors.go @@ -1,6 +1,7 @@ package main import ( + "net/http" "regexp" "strconv" "strings" @@ -168,3 +169,23 @@ func isFileNotFoundError(err error) bool { "does not exist in", ) } + +// errorToHTTPStatusCode maps an error from gitter operation into an appropriate HTTP response status code. +func errorToHTTPStatusCode(err error) int { + if err == nil { + return http.StatusOK + } + + switch { + case isNotFoundError(err): + return http.StatusNotFound // 404 + case isAuthError(err) || isForbiddenError(err): + return http.StatusForbidden // 403 + case isRateLimitError(err): + return http.StatusTooManyRequests // 429 + case isRemoteHostError(err): + return http.StatusFailedDependency // 424 + default: + return http.StatusInternalServerError // 500 + } +} diff --git a/go/cmd/gitter/gitter.go b/go/cmd/gitter/gitter.go index 48ac3e0f037..4bd452889aa 100644 --- a/go/cmd/gitter/gitter.go +++ b/go/cmd/gitter/gitter.go @@ -456,13 +456,7 @@ func gitHandler(w http.ResponseWriter, req *http.Request) { // Fetch repo first if _, err := SyncRepoOnDisk(ctx, repoURL, FetchOptions{ForceUpdate: forceUpdate, SkipReqConcurrencySemaphore: true}); err != nil { - if isAuthError(err) || isForbiddenError(err) { - statusCode = http.StatusForbidden - } else if isNotFoundError(err) { - statusCode = http.StatusNotFound - } else { - statusCode = http.StatusInternalServerError - } + statusCode = errorToHTTPStatusCode(err) http.Error(w, fmt.Sprintf("Error fetching blob: %v", err), statusCode) return @@ -527,13 +521,7 @@ func cacheHandler(w http.ResponseWriter, req *http.Request) { logger.DebugContext(ctx, "Received request: /cache") if _, err := LoadRepo(ctx, repoURL, FetchOptions{ForceUpdate: body.GetForceUpdate(), SkipReqConcurrencySemaphore: true}); err != nil { - if isAuthError(err) || isForbiddenError(err) { - statusCode = http.StatusForbidden - } else if isNotFoundError(err) { - statusCode = http.StatusNotFound - } else { - statusCode = http.StatusInternalServerError - } + statusCode = errorToHTTPStatusCode(err) http.Error(w, fmt.Sprintf("Error getting repo: %v", err), statusCode) return @@ -598,13 +586,7 @@ func affectedCommitsHandler(w http.ResponseWriter, req *http.Request) { repo, err := LoadRepo(ctx, repoURL, FetchOptions{ForceUpdate: body.GetForceUpdate(), SkipReqConcurrencySemaphore: false}) if err != nil { - if isAuthError(err) || isForbiddenError(err) { - statusCode = http.StatusForbidden - } else if isNotFoundError(err) { - statusCode = http.StatusNotFound - } else { - statusCode = http.StatusInternalServerError - } + statusCode = errorToHTTPStatusCode(err) http.Error(w, fmt.Sprintf("Error getting repo: %v", err), statusCode) return @@ -726,22 +708,11 @@ func tagsHandler(w http.ResponseWriter, req *http.Request) { if repo.repoPath != "" { logger.DebugContext(ctx, "Local repo found, using show-ref") if _, errFetch := SyncRepoOnDisk(ctx, repoURL, FetchOptions{ForceUpdate: false, SkipReqConcurrencySemaphore: false}); errFetch != nil { - if isAuthError(errFetch) || isForbiddenError(errFetch) { - invalidRepoCache.SetWithTTL(repoURL, http.StatusForbidden, 1, invalidRepoTTL) - statusCode = http.StatusForbidden - http.Error(w, fmt.Sprintf("Error fetching repository: %v", errFetch), statusCode) - - return + statusCode = errorToHTTPStatusCode(errFetch) + if statusCode == http.StatusForbidden || statusCode == http.StatusNotFound { + invalidRepoCache.SetWithTTL(repoURL, statusCode, 1, invalidRepoTTL) } - if isNotFoundError(errFetch) { - invalidRepoCache.SetWithTTL(repoURL, http.StatusNotFound, 1, invalidRepoTTL) - statusCode = http.StatusNotFound - http.Error(w, fmt.Sprintf("Error fetching repository: %v", errFetch), statusCode) - - return - } - statusCode = http.StatusInternalServerError - http.Error(w, "Error fetching repository", statusCode) + http.Error(w, fmt.Sprintf("Error fetching repository: %v", errFetch), statusCode) return } @@ -773,22 +744,11 @@ func tagsHandler(w http.ResponseWriter, req *http.Request) { return tags, err }) if errLsRemote != nil { - if isAuthError(errLsRemote) || isForbiddenError(errLsRemote) { - invalidRepoCache.SetWithTTL(repoURL, http.StatusForbidden, 1, invalidRepoTTL) - statusCode = http.StatusForbidden - http.Error(w, fmt.Sprintf("Repository authentication failed: %v", errLsRemote), statusCode) - - return - } - if isNotFoundError(errLsRemote) { - invalidRepoCache.SetWithTTL(repoURL, http.StatusNotFound, 1, invalidRepoTTL) - statusCode = http.StatusNotFound - http.Error(w, "Repository not found", statusCode) - - return + statusCode = errorToHTTPStatusCode(errLsRemote) + if statusCode == http.StatusForbidden || statusCode == http.StatusNotFound { + invalidRepoCache.SetWithTTL(repoURL, statusCode, 1, invalidRepoTTL) } - statusCode = http.StatusInternalServerError - http.Error(w, "Error listing remote tags", statusCode) + http.Error(w, fmt.Sprintf("Error listing remote tags: %v", errLsRemote), statusCode) return } @@ -846,13 +806,7 @@ func fileDiffsHandler(w http.ResponseWriter, req *http.Request) { repo, err := SyncRepoOnDisk(ctx, repoURL, FetchOptions{ForceUpdate: true, SkipReqConcurrencySemaphore: true}) if err != nil { - if isAuthError(err) || isForbiddenError(err) { - statusCode = http.StatusForbidden - } else if isNotFoundError(err) { - statusCode = http.StatusNotFound - } else { - statusCode = http.StatusInternalServerError - } + statusCode = errorToHTTPStatusCode(err) http.Error(w, fmt.Sprintf("Error getting repo: %v", err), statusCode) return @@ -932,13 +886,7 @@ func fileContentHandler(w http.ResponseWriter, req *http.Request) { repo, err := SyncRepoOnDisk(ctx, repoURL, FetchOptions{ForceUpdate: false, SkipReqConcurrencySemaphore: true}) if err != nil { - if isAuthError(err) || isForbiddenError(err) { - statusCode = http.StatusForbidden - } else if isNotFoundError(err) { - statusCode = http.StatusNotFound - } else { - statusCode = http.StatusInternalServerError - } + statusCode = errorToHTTPStatusCode(err) http.Error(w, fmt.Sprintf("Error getting repo: %v", err), statusCode) return From 7682792c9826d6d4cadf8591ae16eaf99d608512 Mon Sep 17 00:00:00 2001 From: Joey L Date: Mon, 24 Aug 2026 05:40:24 +0000 Subject: [PATCH 3/3] 424 -> 502 when it's upstream error --- go/cmd/gitter/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/cmd/gitter/errors.go b/go/cmd/gitter/errors.go index 48d09f2d6d1..92459ad1601 100644 --- a/go/cmd/gitter/errors.go +++ b/go/cmd/gitter/errors.go @@ -184,7 +184,7 @@ func errorToHTTPStatusCode(err error) int { case isRateLimitError(err): return http.StatusTooManyRequests // 429 case isRemoteHostError(err): - return http.StatusFailedDependency // 424 + return http.StatusBadGateway // 502 default: return http.StatusInternalServerError // 500 }