From 680f2a259897f4c413e14753f46d277a19ab5334 Mon Sep 17 00:00:00 2001 From: silverwind Date: Sat, 31 Oct 2026 17:31:47 +0100 Subject: [PATCH] fix: don't roll back branches on out-of-order post-receive hooks (#40110) The post-receive hook wrote each push's new commit into the branch table. Each push runs its own hook after the ref is updated, so with pushes in close succession, like CI pushing back or a quick force-push, a slower first hook can finish last. The table then keeps the older commit, or marks a re-created branch as deleted. - The sync reads each pushed branch's state from git under a repo-scoped lock - Deletions go through the same sync, so a re-created branch stays active - A branch that fails to sync is logged and skipped instead of aborting the batch --------- Co-authored-by: wxiaoguang Co-authored-by: Giteabot --- routers/private/hook_post_receive.go | 23 +++--------- services/repository/branch.go | 53 +++++++++++++++++----------- services/repository/branch_test.go | 27 ++++++++++++++ 3 files changed, 64 insertions(+), 39 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 98928c5adcf..d6dbce009cf 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -9,7 +9,6 @@ import ( "net/http" audit_model "gitea.dev/models/audit" - git_model "gitea.dev/models/git" issues_model "gitea.dev/models/issues" repo_model "gitea.dev/models/repo" "gitea.dev/modules/git" @@ -51,24 +50,18 @@ func hookPostReceiveCollectPushUpdates(opts *private.HookOptions, repo *repo_mod } func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts *private.HookOptions, repo *repo_model.Repository, updates []*repo_module.PushUpdateOptions) { - branchesToSync := make([]*repo_module.PushUpdateOptions, 0, len(updates)) + branchNames := make([]string, 0, len(updates)) for _, update := range updates { if !update.RefFullName.IsBranch() { continue } - if update.IsDelRef() { - if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, update.RefFullName.BranchName(), update.PusherID); err != nil { - log.Error("failed to mark branch %s as deleted: %v", update.RefFullName, err) - continue - } - } else { - branchesToSync = append(branchesToSync, update) - // TODO: should we return the error and return the error when pushing? Currently it will log the error and not prevent the pushing + branchNames = append(branchNames, update.RefFullName.BranchName()) + if !update.IsDelRef() { pull_service.UpdatePullsRefs(ctx, repo, update) } } - if len(branchesToSync) == 0 { + if len(branchNames) == 0 { return } @@ -78,13 +71,7 @@ func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts return } - branchNames := make([]string, 0, len(branchesToSync)) - commitIDs := make([]string, 0, len(branchesToSync)) - for _, update := range branchesToSync { - branchNames = append(branchNames, update.RefFullName.BranchName()) - commitIDs = append(commitIDs, update.NewCommitID) - } - if err = repo_service.SyncBranchesToDB(ctx, repo.ID, opts.UserID, gitRepo, branchNames, commitIDs); err != nil { + if err = repo_service.SyncBranchesToDB(ctx, repo.ID, opts.UserID, gitRepo, branchNames); err != nil { log.Error("failed to sync branch to DB: %v", err) } } diff --git a/services/repository/branch.go b/services/repository/branch.go index 71a6f2c2eae..320acdf8f12 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -18,6 +18,7 @@ import ( user_model "gitea.dev/models/user" "gitea.dev/modules/cache" "gitea.dev/modules/git" + "gitea.dev/modules/globallock" "gitea.dev/modules/graceful" "gitea.dev/modules/json" "gitea.dev/modules/log" @@ -304,7 +305,7 @@ func checkBranchName(ctx context.Context, gitRepo *git.Repository, name string) // It will check whether the branches of the repository have never been synced before. // If so, it will sync all branches of the repository. // Otherwise, it will sync the branches that need to be updated. -func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, gitRepo *git.Repository, branchNames, commitIDs []string) error { +func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, gitRepo *git.Repository, branchNames []string) error { // Some designs that make the code look strange but are made for performance optimization purposes: // 1. Sync branches in a batch to reduce the number of DB queries. // 2. Lazy load commit information since it may be not necessary. @@ -316,27 +317,31 @@ func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, gitRepo *git. // For the first batch, it will hit optimization 3. // For other batches, it will hit optimization 4. - if len(branchNames) != len(commitIDs) { - return errors.New("branchNames and commitIDs length not match") + // In case there are concurrent pushes, we need to lock the sync operation for the same repository + // and always use the latest branch information in the database. + releaser, err := globallock.Lock(ctx, fmt.Sprintf("repo_branch_sync_%d", repoID)) + if err != nil { + return err } + defer releaser() return db.WithTx(ctx, func(ctx context.Context) error { - branches, err := git_model.GetBranches(ctx, repoID, branchNames, true) + dbBranches, err := git_model.GetBranches(ctx, repoID, branchNames, true) if err != nil { return fmt.Errorf("git_model.GetBranches: %v", err) } - if len(branches) == 0 { - // if user haven't visit UI but directly push to a branch after upgrading from 1.20 -> 1.21, + if len(dbBranches) == 0 { + // if user haven't visited UI but directly push to a branch after upgrading from 1.20 -> 1.21, // we cannot simply insert the branch but need to check we have branches or not - hasBranch, err := db.Exist[git_model.Branch](ctx, git_model.FindBranchOptions{ + hasAnyActiveBranch, err := db.Exist[git_model.Branch](ctx, git_model.FindBranchOptions{ RepoID: repoID, IsDeletedBranch: optional.Some(false), }.ToConds()) if err != nil { return err } - if !hasBranch { + if !hasAnyActiveBranch { if _, err = repo_module.SyncRepoBranches(ctx, repoID, pusherID); err != nil { return fmt.Errorf("repo_module.SyncRepoBranches %d failed: %v", repoID, err) } @@ -344,26 +349,32 @@ func SyncBranchesToDB(ctx context.Context, repoID, pusherID int64, gitRepo *git. } } - branchMap := make(map[string]*git_model.Branch, len(branches)) - for _, branch := range branches { - branchMap[branch.Name] = branch + dbBranchMap := make(map[string]*git_model.Branch, len(dbBranches)) + for _, branch := range dbBranches { + dbBranchMap[branch.Name] = branch } newBranches := make([]*git_model.Branch, 0, len(branchNames)) - for i, branchName := range branchNames { - commitID := commitIDs[i] - branch, exist := branchMap[branchName] - if exist && branch.CommitID == commitID && !branch.IsDeleted { + for _, branchName := range branchNames { + branch, existInDb := dbBranchMap[branchName] + commit, err := gitRepo.GetCommit(ctx, git.RefNameFromBranch(branchName).String()) + if git.IsErrNotExist(err) { + if existInDb && !branch.IsDeleted { + if err := git_model.MarkBranchAsDeleted(ctx, repoID, branchName, pusherID); err != nil { + log.Error("SyncBranchesToDB: mark branch %d:%s deleted: %v", repoID, branchName, err) + } + } + continue + } else if err != nil { + log.Error("SyncBranchesToDB: get commit id of %s: %v", branchName, err) + continue + } + if existInDb && branch.CommitID == commit.ID.String() && !branch.IsDeleted { continue } - commit, err := gitRepo.GetCommit(ctx, commitID) - if err != nil { - return fmt.Errorf("get commit of %s failed: %v", branchName, err) - } - - if exist { + if existInDb { if _, err := git_model.UpdateBranch(ctx, repoID, pusherID, branchName, commit); err != nil { return fmt.Errorf("git_model.UpdateBranch %d:%s failed: %v", repoID, branchName, err) } diff --git a/services/repository/branch_test.go b/services/repository/branch_test.go index 95314521465..d695553efef 100644 --- a/services/repository/branch_test.go +++ b/services/repository/branch_test.go @@ -12,6 +12,7 @@ import ( "gitea.dev/models/unit" "gitea.dev/models/unittest" user_model "gitea.dev/models/user" + "gitea.dev/modules/git" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -42,3 +43,29 @@ func TestCanDeleteBranchDefaultTargets(t *testing.T) { assert.ErrorIs(t, CanDeleteBranch(ctx, repo, "branch2", owner), ErrBranchIsDefault) } + +func TestSyncBranchesToDBUsesGitState(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + ctx := t.Context() + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + gitRepo, err := git.OpenRepository(ctx, repo) + require.NoError(t, err) + defer gitRepo.Close() + + _, err = db.GetEngine(ctx).Where("repo_id=? AND name=?", repo.ID, "master").Update(&git_model.Branch{CommitID: "985f0301dba5e7b34be866819cd15ad3d8f508ee"}) + require.NoError(t, err) + _, err = db.GetEngine(ctx).Where("repo_id=? AND name=?", repo.ID, "foo").Cols("is_deleted").Update(&git_model.Branch{IsDeleted: false}) + require.NoError(t, err) + require.NoError(t, git_model.MarkBranchAsDeleted(ctx, repo.ID, "develop", 1)) + + require.NoError(t, SyncBranchesToDB(ctx, repo.ID, 2, gitRepo, []string{"master", "foo", "develop", "pr-to-update"})) + + loadBranch := func(name string) *git_model.Branch { + return unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: name}) + } + assert.Equal(t, "65f1bf27bc3bf70f64657658635e66094edbcb4d", loadBranch("master").CommitID) + assert.True(t, loadBranch("foo").IsDeleted) + assert.False(t, loadBranch("develop").IsDeleted) + assert.Equal(t, "62fb502a7172d4453f0322a2cc85bddffa57f07a", loadBranch("pr-to-update").CommitID) +}