From fb067e111558779829507778b91e04f17a41050f Mon Sep 17 00:00:00 2001 From: silverwind Date: Sat, 3 Oct 2026 03:58:36 +0200 Subject: [PATCH] fix(git): tolerate concurrent repacks in go-git storage (#39536) Since `transfer.fsckObjects` makes fetches keep a pack, git 2.54+ background maintenance repacks a mirror right after its sync fetch, and go-git then misses objects mid-repack. Fixes these flakes: - https://github.com/go-gitea/gitea/actions/runs/36875305717/job/110419770196 - https://github.com/go-gitea/gitea/actions/runs/36900439473/job/110498845882 Changes: - Keep reindexing while the pack set changes instead of retrying once - List packs only once their `.idx` exists and don't fail the listing on files removed mid-repack - Look up large objects again when their file is gone before reading --- modules/git/repo_base_gogit.go | 99 +++++++++++++++++++++++++++++---- modules/git/repo_branch_test.go | 23 +++++++- 2 files changed, 109 insertions(+), 13 deletions(-) diff --git a/modules/git/repo_base_gogit.go b/modules/git/repo_base_gogit.go index ef98ac3223b..83415005f1f 100644 --- a/modules/git/repo_base_gogit.go +++ b/modules/git/repo_base_gogit.go @@ -8,9 +8,13 @@ package git import ( "errors" + "io" + "os" "path/filepath" "slices" + "strings" + "gitea.dev/modules/container" "gitea.dev/modules/git/gitrepo" "gitea.dev/modules/setting" @@ -34,23 +38,96 @@ type Repository struct { // reindexingStorage reloads the pack index when git added or removed packs after go-git loaded it // https://github.com/go-git/go-git/issues/2439 https://github.com/go-git/go-git/issues/1623 +// FIXME: gogit workaround, remove with the gogit build type reindexingStorage struct { *filesystem.Storage packs []plumbing.Hash } -func (s *reindexingStorage) EncodedObject(t plumbing.ObjectType, h plumbing.Hash) (plumbing.EncodedObject, error) { - obj, err := s.Storage.EncodedObject(t, h) - if !errors.Is(err, plumbing.ErrObjectNotFound) && !errors.Is(err, dotgit.ErrPackfileNotFound) { - return obj, err +func isRepackError(err error) bool { + return errors.Is(err, plumbing.ErrObjectNotFound) || errors.Is(err, dotgit.ErrPackfileNotFound) || errors.Is(err, os.ErrNotExist) +} + +// retry reruns fn while a concurrent repack keeps changing the packs +func (s *reindexingStorage) retry(fn func() error) error { + for { + err := fn() + if !isRepackError(err) { + return err + } + packs, _ := s.ObjectPacks() + if slices.Equal(packs, s.packs) { + return err + } + s.packs = packs + s.Reindex() } - packs, _ := s.ObjectPacks() - if slices.Equal(packs, s.packs) { - return obj, err +} + +func (s *reindexingStorage) EncodedObject(t plumbing.ObjectType, h plumbing.Hash) (obj plumbing.EncodedObject, err error) { + err = s.retry(func() (err error) { + obj, err = s.Storage.EncodedObject(t, h) + return err + }) + if err != nil { + return nil, err } - s.packs = packs - s.Reindex() - return s.Storage.EncodedObject(t, h) + if _, ok := obj.(*plumbing.MemoryObject); ok { + return obj, nil + } + return &lazyObject{EncodedObject: obj, storage: s}, nil +} + +// lazyObject looks up a large object again when its file got removed before Reader reopened it +// FIXME: gogit workaround, remove with the gogit build +type lazyObject struct { + plumbing.EncodedObject + storage *reindexingStorage +} + +func (o *lazyObject) Reader() (rc io.ReadCloser, err error) { + rc, err = o.EncodedObject.Reader() + if !isRepackError(err) { + return rc, err + } + err = o.storage.retry(func() error { + obj, err := o.storage.Storage.EncodedObject(o.Type(), o.Hash()) + if err == nil { + o.EncodedObject = obj + rc, err = obj.Reader() + } + return err + }) + return rc, err +} + +// packIdxFS lists packs like git, only while their .idx exists +// FIXME: gogit workaround, remove with the gogit build +type packIdxFS struct { + billy.Filesystem +} + +func (f packIdxFS) ReadDir(dir string) ([]os.FileInfo, error) { + if dir != f.Join("objects", "pack") { + return f.Filesystem.ReadDir(dir) + } + dirFile, err := os.Open(filepath.Join(f.Root(), dir)) + if err != nil { + return nil, err + } + defer dirFile.Close() + infos, err := dirFile.Readdir(-1) // skips files removed before their lstat, unlike billy's ReadDir + if err != nil { + return nil, err + } + names := make(container.Set[string], len(infos)) + for _, info := range infos { + names.Add(info.Name()) + } + return slices.DeleteFunc(infos, func(info os.FileInfo) bool { + base, isPack := strings.CutSuffix(info.Name(), ".pack") + return isPack && !names.Contains(base+".idx") + }), nil } func openRepositoryInternal(gitRepo *Repository) error { @@ -72,7 +149,7 @@ func openRepositoryInternal(gitRepo *Repository) error { altFs = osfs.New("/") } gitRepo.objectFormatCache = ParseGogitHash(plumbing.ZeroHash).Type() - storage := filesystem.NewStorageWithOptions(fs, cache.NewObjectLRUDefault(), filesystem.Options{KeepDescriptors: true, LargeObjectThreshold: setting.Git.LargeObjectThreshold, AlternatesFS: altFs}) + storage := filesystem.NewStorageWithOptions(packIdxFS{fs}, cache.NewObjectLRUDefault(), filesystem.Options{KeepDescriptors: true, LargeObjectThreshold: setting.Git.LargeObjectThreshold, AlternatesFS: altFs}) packs, _ := storage.ObjectPacks() gitRepo.gogitStorage = &reindexingStorage{Storage: storage, packs: packs} gitRepo.gogitRepo, err = gogit.Open(gitRepo.gogitStorage, fs) diff --git a/modules/git/repo_branch_test.go b/modules/git/repo_branch_test.go index 8b7613a39f5..83ad43c2af3 100644 --- a/modules/git/repo_branch_test.go +++ b/modules/git/repo_branch_test.go @@ -4,10 +4,14 @@ package git import ( + "fmt" + "os" "path/filepath" + "strings" "testing" "gitea.dev/modules/git/gitcmd" + "gitea.dev/modules/setting" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -41,11 +45,13 @@ func TestRepository_GetBranches(t *testing.T) { assert.ElementsMatch(t, []string{}, branches) } -func TestGetBranchNamesAfterRepack(t *testing.T) { +// FIXME: covers the gogit workarounds in repo_base_gogit.go, remove with the gogit build +func TestReadsAfterConcurrentRepack(t *testing.T) { repoDir := t.TempDir() require.NoError(t, gitcmd.NewCommand("init", "--bare").AddDynamicArguments(repoDir).Run(t.Context())) + content := strings.Repeat("a", int(setting.Git.LargeObjectThreshold)+1) for _, from := range []string{"", "from refs/heads/main^0\n"} { - stdin := "commit refs/heads/main\ncommitter a 0 +0000\ndata 0\n" + from + stdin := fmt.Sprintf("commit refs/heads/main\ncommitter a 0 +0000\ndata 0\n%sM 100644 inline f\ndata %d\n%s\n", from, len(content), content) require.NoError(t, gitcmd.NewCommand("fast-import").WithDir(repoDir).WithStdinBytes([]byte(stdin)).Run(t.Context())) require.NoError(t, gitcmd.NewCommand("repack", "-d").WithDir(repoDir).Run(t.Context())) } @@ -54,11 +60,24 @@ func TestGetBranchNamesAfterRepack(t *testing.T) { require.NoError(t, err) defer repo.Close() require.False(t, repo.IsObjectExist(t.Context(), "0000000000000000000000000000000000000001")) + blobRepo, err := OpenRepositoryLocal(t.Context(), repoDir) + require.NoError(t, err) + defer blobRepo.Close() + commit, err := blobRepo.GetBranchCommit(t.Context(), "main") + require.NoError(t, err) + readBlob := func() string { + data, err := commit.GetFileContent(t.Context(), blobRepo, "f", len(content)) + require.NoError(t, err) + return data + } + require.Equal(t, content, readBlob()) require.NoError(t, gitcmd.NewCommand("repack", "-a", "-d").WithDir(repoDir).Run(t.Context())) + require.NoError(t, os.WriteFile(filepath.Join(repoDir, "objects", "pack", "pack-"+strings.Repeat("1", 40)+".pack"), nil, 0o644)) branches, _, err := repo.GetBranchNames(t.Context(), 0, 0) require.NoError(t, err) assert.Equal(t, []string{"main"}, branches) + assert.Equal(t, content, readBlob()) } func BenchmarkRepository_GetBranches(b *testing.B) {