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 <wxiaoguang@gmail.com>
Co-authored-by: Giteabot <teabot@gitea.io>

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

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

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