From 0ab3d569b4944d2b4603bb0228d6cfa4ae6ea15e Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 28 Jul 2026 19:12:53 +0800 Subject: [PATCH] fix: repo home page 500 due to the timeout of "get last commit info" (#38678) Regression of the ctx removal from "git.Repository" struct (the old code was already wrong and can still cause 500, the "ctx removal" just makes the problem easier to reproduce). Merge duplicate code. Reviewd by codex: no actionable findings. --- modules/git/commit_info.go | 98 ++++++++++++++++++++++++- modules/git/commit_info_gogit.go | 86 ---------------------- modules/git/commit_info_nogogit.go | 93 ----------------------- modules/git/commit_info_nogogit_test.go | 5 +- modules/git/commit_info_test.go | 3 +- modules/git/repo.go | 17 +++-- routers/web/repo/view.go | 9 +-- routers/web/repo/wiki.go | 2 +- 8 files changed, 113 insertions(+), 200 deletions(-) diff --git a/modules/git/commit_info.go b/modules/git/commit_info.go index 87a6ddd8d15..19ed12fb9d8 100644 --- a/modules/git/commit_info.go +++ b/modules/git/commit_info.go @@ -3,7 +3,15 @@ package git -import "context" +import ( + "context" + "maps" + "path" + "time" + + "gitea.dev/modules/log" + "gitea.dev/modules/util" +) // CommitInfo describes the first commit with the provided entry type CommitInfo struct { @@ -23,3 +31,91 @@ func GetCommitInfoSubmoduleFile(ctx context.Context, repoLink, fullPath string, } return NewCommitSubmoduleFile(repoLink, fullPath, submodule.URL, refCommitID.String()), nil } + +func getLastCommitForPathsWithTimeout(ctx context.Context, timeout time.Duration, gitRepo *Repository, commit *Commit, treePath string, entryNames []string) (map[string]*Commit, error) { + // Actually using "timeout ctx" here is not ideal: it creates a new git cat-file batch process + // (the other one uses the request's ctx and is cached by the gitRepo). + // Ideally, the underlying functions only need to return in predictable time, + // so it should use the shared (cached) git cat-file batcher from the request context and just use "time.After" for deadline. + // However, it's difficult to make it right at the moment. + ctxInner := ctx + if timeout > 0 { + var cancel context.CancelFunc + ctxInner, cancel = context.WithTimeout(ctx, timeout) + defer cancel() + } + revs, err := GetLastCommitForPaths(ctxInner, gitRepo, commit, treePath, entryNames) + if err != nil && ctxInner.Err() == nil { + // only return the err if context is not canceled + return nil, err + } + return revs, nil +} + +// GetCommitsInfo gets information of all commits that are corresponding to these entries +func (tes Entries) GetCommitsInfo(ctx context.Context, timeout time.Duration, repoLink string, gitRepo *Repository, commit *Commit, treePath string) (commitsInfo []CommitInfo, treeLastCommit *Commit, err error) { + entryNames := make([]string, 0, len(tes)+1) + entryNames = append(entryNames, "") // include the commit for the treePath itself + for _, entry := range tes { + entryNames = append(entryNames, entry.Name()) + } + + var revs map[string]*Commit + var remainingEntryNames []string + if gitRepo.LastCommitCache != nil { + revs, remainingEntryNames, err = getLastCommitForPathsByCache(ctx, commit.ID.String(), treePath, entryNames, gitRepo.LastCommitCache) + if err != nil { + return nil, nil, err + } + } else { + revs, remainingEntryNames = map[string]*Commit{}, entryNames + } + + if len(remainingEntryNames) > 0 { + remainingRevs, err := getLastCommitForPathsWithTimeout(ctx, timeout, gitRepo, commit, treePath, remainingEntryNames) + if err != nil { + return nil, nil, err + } + maps.Copy(revs, remainingRevs) + } + + for _, entry := range tes { + ci := CommitInfo{Entry: entry, Commit: revs[entry.Name()]} + if ci.Commit == nil { + log.Debug("missing commit for %s", entry.Name()) + } + // If the entry is a submodule, add a submodule file for this + if entry.IsSubModule() { + ci.SubmoduleFile, err = GetCommitInfoSubmoduleFile(ctx, repoLink, path.Join(treePath, entry.Name()), gitRepo, commit, entry.ID) + if err != nil { + return nil, nil, err + } + } + commitsInfo = append(commitsInfo, ci) + } + + // Retrieve the commit for the treePath itself (see above). We basically + // get it for free during the tree traversal, and it's used for listing + // pages to display information about the newest commit for a given path. + treeLastCommit = util.Iif(treePath == "", commit, revs[""]) + return commitsInfo, treeLastCommit, nil +} + +func getLastCommitForPathsByCache(ctx context.Context, commitID, treePath string, paths []string, cache *LastCommitCache) (map[string]*Commit, []string, error) { + var unHitEntryPaths []string + results := make(map[string]*Commit) + for _, p := range paths { + lastCommit, err := cache.Get(ctx, commitID, path.Join(treePath, p)) + if err != nil { + return nil, nil, err + } + if lastCommit != nil { + results[p] = lastCommit + continue + } + + unHitEntryPaths = append(unHitEntryPaths, p) + } + + return results, unHitEntryPaths, nil +} diff --git a/modules/git/commit_info_gogit.go b/modules/git/commit_info_gogit.go index c0233b7f824..7c578580c82 100644 --- a/modules/git/commit_info_gogit.go +++ b/modules/git/commit_info_gogit.go @@ -7,7 +7,6 @@ package git import ( "context" - "maps" "path" "github.com/emirpasic/gods/trees/binaryheap" @@ -16,72 +15,6 @@ import ( cgobject "github.com/go-git/go-git/v5/plumbing/object/commitgraph" ) -// GetCommitsInfo gets information of all commits that are corresponding to these entries -func (tes Entries) GetCommitsInfo(ctx context.Context, repoLink string, gitRepo *Repository, commit *Commit, treePath string) ([]CommitInfo, *Commit, error) { - entryPaths := make([]string, len(tes)+1) - // Get the commit for the treePath itself - entryPaths[0] = "" - for i, entry := range tes { - entryPaths[i+1] = entry.Name() - } - - var revs map[string]*Commit - var err error - if gitRepo.LastCommitCache != nil { - var unHitPaths []string - revs, unHitPaths, err = getLastCommitForPathsByCache(ctx, commit.ID.String(), treePath, entryPaths, gitRepo.LastCommitCache) - if err != nil { - return nil, nil, err - } - if len(unHitPaths) > 0 { - revs2, err := GetLastCommitForPaths(ctx, gitRepo, commit, treePath, unHitPaths) - if err != nil { - return nil, nil, err - } - - maps.Copy(revs, revs2) - } - } else { - revs, err = GetLastCommitForPaths(ctx, gitRepo, commit, treePath, entryPaths) - } - if err != nil { - return nil, nil, err - } - - gitRepo.gogitStorage.Close() - - commitsInfo := make([]CommitInfo, len(tes)) - for i, entry := range tes { - commitsInfo[i] = CommitInfo{ - Entry: entry, - } - - // Check if we have found a commit for this entry in time - if entryCommit, ok := revs[entry.Name()]; ok { - commitsInfo[i].Commit = entryCommit - } - - // If the entry is a submodule, add a submodule file for this - if entry.IsSubModule() { - commitsInfo[i].SubmoduleFile, err = GetCommitInfoSubmoduleFile(ctx, repoLink, path.Join(treePath, entry.Name()), gitRepo, commit, entry.ID) - if err != nil { - return nil, nil, err - } - } - } - - // Retrieve the commit for the treePath itself (see above). We basically - // get it for free during the tree traversal and it's used for listing - // pages to display information about newest commit for a given path. - var treeCommit *Commit - if treePath == "" { - treeCommit = commit - } else { - treeCommit = revs[""] - } - return commitsInfo, treeCommit, nil -} - type commitAndPaths struct { commit cgobject.CommitNode // Paths that are still on the branch represented by commit @@ -132,25 +65,6 @@ func getFileHashes(c cgobject.CommitNode, treePath string, paths []string) (map[ return hashes, nil } -func getLastCommitForPathsByCache(ctx context.Context, commitID, treePath string, paths []string, cache *LastCommitCache) (map[string]*Commit, []string, error) { - var unHitEntryPaths []string - results := make(map[string]*Commit) - for _, p := range paths { - lastCommit, err := cache.Get(ctx, commitID, path.Join(treePath, p)) - if err != nil { - return nil, nil, err - } - if lastCommit != nil { - results[p] = lastCommit - continue - } - - unHitEntryPaths = append(unHitEntryPaths, p) - } - - return results, unHitEntryPaths, nil -} - // GetLastCommitForPaths returns last commit information func GetLastCommitForPaths(ctx context.Context, gitRepo *Repository, commit *Commit, treePath string, paths []string) (map[string]*Commit, error) { commitNodeIndex, closer := gitRepo.CommitNodeIndex() diff --git a/modules/git/commit_info_nogogit.go b/modules/git/commit_info_nogogit.go index 24753882733..c6ceb78d916 100644 --- a/modules/git/commit_info_nogogit.go +++ b/modules/git/commit_info_nogogit.go @@ -7,101 +7,8 @@ package git import ( "context" - "maps" - "path" - "sort" - - "gitea.dev/modules/log" ) -// GetCommitsInfo gets information of all commits that are corresponding to these entries -func (tes Entries) GetCommitsInfo(ctx context.Context, repoLink string, gitRepo *Repository, commit *Commit, treePath string) ([]CommitInfo, *Commit, error) { - entryPaths := make([]string, len(tes)+1) - // Get the commit for the treePath itself - entryPaths[0] = "" - for i, entry := range tes { - entryPaths[i+1] = entry.Name() - } - - var err error - - var revs map[string]*Commit - if gitRepo.LastCommitCache != nil { - var unHitPaths []string - revs, unHitPaths, err = getLastCommitForPathsByCache(ctx, commit.ID.String(), treePath, entryPaths, gitRepo.LastCommitCache) - if err != nil { - return nil, nil, err - } - if len(unHitPaths) > 0 { - sort.Strings(unHitPaths) - commits, err := GetLastCommitForPaths(ctx, gitRepo, commit, treePath, unHitPaths) - if err != nil { - return nil, nil, err - } - - maps.Copy(revs, commits) - } - } else { - sort.Strings(entryPaths) - revs, err = GetLastCommitForPaths(ctx, gitRepo, commit, treePath, entryPaths) - } - if err != nil { - return nil, nil, err - } - - commitsInfo := make([]CommitInfo, len(tes)) - for i, entry := range tes { - commitsInfo[i] = CommitInfo{ - Entry: entry, - } - - // Check if we have found a commit for this entry in time - if entryCommit, ok := revs[entry.Name()]; ok { - commitsInfo[i].Commit = entryCommit - } else { - log.Debug("missing commit for %s", entry.Name()) - } - - // If the entry is a submodule, add a submodule file for this - if entry.IsSubModule() { - commitsInfo[i].SubmoduleFile, err = GetCommitInfoSubmoduleFile(ctx, repoLink, path.Join(treePath, entry.Name()), gitRepo, commit, entry.ID) - if err != nil { - return nil, nil, err - } - } - } - - // Retrieve the commit for the treePath itself (see above). We basically - // get it for free during the tree traversal, and it's used for listing - // pages to display information about the newest commit for a given path. - var treeCommit *Commit - if treePath == "" { - treeCommit = commit - } else { - treeCommit = revs[""] - } - return commitsInfo, treeCommit, nil -} - -func getLastCommitForPathsByCache(ctx context.Context, commitID, treePath string, paths []string, cache *LastCommitCache) (map[string]*Commit, []string, error) { - var unHitEntryPaths []string - results := make(map[string]*Commit) - for _, p := range paths { - lastCommit, err := cache.Get(ctx, commitID, path.Join(treePath, p)) - if err != nil { - return nil, nil, err - } - if lastCommit != nil { - results[p] = lastCommit - continue - } - - unHitEntryPaths = append(unHitEntryPaths, p) - } - - return results, unHitEntryPaths, nil -} - // GetLastCommitForPaths returns last commit information func GetLastCommitForPaths(ctx context.Context, gitRepo *Repository, commit *Commit, treePath string, paths []string) (map[string]*Commit, error) { // We read backwards from the commit to obtain all of the commits diff --git a/modules/git/commit_info_nogogit_test.go b/modules/git/commit_info_nogogit_test.go index f1f9bfcb1ef..5ec14e87526 100644 --- a/modules/git/commit_info_nogogit_test.go +++ b/modules/git/commit_info_nogogit_test.go @@ -9,6 +9,7 @@ import ( "context" "path/filepath" "testing" + "time" "gitea.dev/modules/test" "gitea.dev/modules/util" @@ -39,14 +40,14 @@ func TestEntries_GetCommitsInfo_ContextErr(t *testing.T) { defer test.MockVariableValue(&walkGitLogDebugBeforeNext)() walkGitLogDebugBeforeNext = cancel - commitInfos, _, err := entries.GetCommitsInfo(ctx, "/any/repo-link", repo, commit, "") + commitInfos, _, err := entries.GetCommitsInfo(ctx, time.Second, "/any/repo-link", repo, commit, "") assert.NoError(t, err) nilCommits, nonNilCommits := countCommitInfosCommit(commitInfos) assert.Equal(t, 0, nonNilCommits) // no commit info due to canceled (or deadline-exceeded) context assert.Equal(t, 3, nilCommits) walkGitLogDebugBeforeNext = nil - commitInfos, _, err = entries.GetCommitsInfo(t.Context(), "/any/repo-link", repo, commit, "") + commitInfos, _, err = entries.GetCommitsInfo(t.Context(), time.Second, "/any/repo-link", repo, commit, "") assert.NoError(t, err) nilCommits, nonNilCommits = countCommitInfosCommit(commitInfos) assert.Equal(t, 3, nonNilCommits) diff --git a/modules/git/commit_info_test.go b/modules/git/commit_info_test.go index ceed96867f4..80d4a639839 100644 --- a/modules/git/commit_info_test.go +++ b/modules/git/commit_info_test.go @@ -105,8 +105,7 @@ func testGetCommitsInfo(t *testing.T, repo1 *Repository) { continue } - // FIXME: Context.TODO() - if graceful has started we should use its Shutdown context otherwise use install signals in TestMain. - commitsInfo, treeCommit, err := entries.GetCommitsInfo(t.Context(), "/any/repo-link", repo1, commit, testCase.Path) + commitsInfo, treeCommit, err := entries.GetCommitsInfo(t.Context(), time.Second, "/any/repo-link", repo1, commit, testCase.Path) assert.NoError(t, err, "Unable to get commit information for entries of subtree: %s in commit: %s from testcase due to error: %v", testCase.Path, testCase.CommitID, err) if err != nil { t.FailNow() diff --git a/modules/git/repo.go b/modules/git/repo.go index e9cf9743817..23a5272dabe 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -280,14 +280,14 @@ func (repo *Repository) CatFileBatch(ctx context.Context) (_ CatFileBatch, close repo.mu.Lock() defer repo.mu.Unlock() - if repo.catFileBatchCloser != nil && !repo.catFileBatchInUse { - if ctx != repo.catFileBatchCloser.Context() { - repo.catFileBatchCloser.Close() - repo.catFileBatchCloser = nil - repo.catFileBatchInUse = false - } + // clean up the cached but canceled batcher + if repo.catFileBatchCloser != nil && repo.catFileBatchCloser.Context().Err() != nil && !repo.catFileBatchInUse { + repo.catFileBatchCloser.Close() + repo.catFileBatchCloser = nil + repo.catFileBatchInUse = false } + // if no cached batcher, make a new managed one, and cache it if repo.catFileBatchCloser == nil { repo.catFileBatchCloser, err = NewBatch(ctx, repo) if err != nil { @@ -296,7 +296,10 @@ func (repo *Repository) CatFileBatch(ctx context.Context) (_ CatFileBatch, close } } - if !repo.catFileBatchInUse { + // if the cached batcher is in use, or the cached ctx is not the current ctx, then we need a new temp one + // for example: the cached one is from request context, the current ctx is a cancelable ctx with timeout + needTemp := repo.catFileBatchInUse || repo.catFileBatchCloser.Context() != ctx + if !needTemp { repo.catFileBatchInUse = true return CatFileBatch(repo.catFileBatchCloser), func() { repo.mu.Lock() diff --git a/routers/web/repo/view.go b/routers/web/repo/view.go index e3583fd332d..cc10322101f 100644 --- a/routers/web/repo/view.go +++ b/routers/web/repo/view.go @@ -298,14 +298,7 @@ func renderDirectoryFiles(ctx *context.Context, timeout time.Duration) (*git.Tre } subEntries.CustomSort(base.NaturalSortCompare) - commitInfoCtx := gocontext.Context(ctx) - if timeout > 0 { - var cancel gocontext.CancelFunc - commitInfoCtx, cancel = gocontext.WithTimeout(ctx, timeout) - defer cancel() - } - - files, latestCommit, err := subEntries.GetCommitsInfo(commitInfoCtx, ctx.Repo.RepoLink, ctx.Repo.GitRepo, ctx.Repo.Commit, ctx.Repo.TreePath) + files, latestCommit, err := subEntries.GetCommitsInfo(ctx, timeout, ctx.Repo.RepoLink, ctx.Repo.GitRepo, ctx.Repo.Commit, ctx.Repo.TreePath) if err != nil { ctx.ServerError("GetCommitsInfo", err) return nil, nil diff --git a/routers/web/repo/wiki.go b/routers/web/repo/wiki.go index 13e4be88c28..4d82c67fd1a 100644 --- a/routers/web/repo/wiki.go +++ b/routers/web/repo/wiki.go @@ -563,7 +563,7 @@ func WikiPages(ctx *context.Context) { } allEntries.CustomSort(base.NaturalSortCompare) - entries, _, err := allEntries.GetCommitsInfo(ctx, ctx.Repo.RepoLink, wikiGitRepo, commit, treePath) + entries, _, err := allEntries.GetCommitsInfo(ctx, 0, ctx.Repo.RepoLink, wikiGitRepo, commit, treePath) if err != nil { ctx.ServerError("GetCommitsInfo", err) return