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

@@ -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
+}

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()

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

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)

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()

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()

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

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