Skip to content

fix: better error logging in gitter #5892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Ly-Joey merged 3 commits into from
Aug 27, 2026
+64 −76
Merged

fix: better error logging in gitter #5892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
916c43f
move error logging into the singleflight Do
Ly-Joey Aug 21, 2026
d11de19
Put http ststus code mapping into helper
Ly-Joey Aug 24, 2026
7682792
424 -> 502 when it's upstream error
Ly-Joey Aug 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions go/cmd/gitter/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package main

import (
"net/http"
"regexp"
"strconv"
"strings"
Expand Down Expand Up @@ -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):

@cuixq cuixq Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about using 502 Bad Gateway (http.StatusBadGateway) instead of 424 Failed Dependency here?

  • 424 is a 4xx code (client error), which indicates that the caller made an invalid request or provided invalid state, but isRemoteHostError seems focus more on the upstream server errors, where the caller's request was valid.
  • gitter acts as an intermediary/proxy to upstream remote git hosts, 502 Bad Gateway feels like the more suitable status code for upstream server failures.

@Ly-Joey Ly-Joey Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Updated :D

return http.StatusBadGateway // 502
default:
return http.StatusInternalServerError // 500
}
}
16 changes: 12 additions & 4 deletions go/cmd/gitter/git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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)
Expand Down
103 changes: 31 additions & 72 deletions go/cmd/gitter/gitter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -471,11 +465,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)

Expand Down Expand Up @@ -523,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
Expand Down Expand Up @@ -594,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
Expand Down Expand Up @@ -722,32 +708,24 @@ 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
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
}

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)

Expand All @@ -758,26 +736,19 @@ 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)
})
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
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))
}
if isNotFoundError(errLsRemote) {
invalidRepoCache.SetWithTTL(repoURL, http.StatusNotFound, 1, invalidRepoTTL)
statusCode = http.StatusNotFound
http.Error(w, "Repository not found", statusCode)

return
return tags, err
})
if errLsRemote != nil {
statusCode = errorToHTTPStatusCode(errLsRemote)
if statusCode == http.StatusForbidden || statusCode == http.StatusNotFound {
invalidRepoCache.SetWithTTL(repoURL, statusCode, 1, invalidRepoTTL)
}
logger.ErrorContext(ctx, "Error running git ls-remote", slog.Any("error", errLsRemote))
statusCode = http.StatusInternalServerError
http.Error(w, "Error listing remote tags", statusCode)
http.Error(w, fmt.Sprintf("Error listing remote tags: %v", errLsRemote), statusCode)

return
}
Expand Down Expand Up @@ -835,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
Expand Down Expand Up @@ -921,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
Expand Down
Loading