From 317db147017cf45f3e938dd3cb5908a303b1c252 Mon Sep 17 00:00:00 2001 From: silverwind Date: Sat, 7 Nov 2026 02:59:15 +0000 Subject: [PATCH] fix: don't roll back branches on out-of-order post-receive hooks (#40110) (#40255) Backport https://github.com/go-gitea/gitea/pull/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 | 43 ++++++++++++++++++++++ 3 files changed, 80 insertions(+), 39 deletions(-) create mode 100644 services/repository/branch_test.go 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 73988a093ca..f261af77628 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" @@ -286,7 +287,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. @@ -298,27 +299,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) } @@ -326,26 +331,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 new file mode 100644 index 00000000000..10bcd45879e --- /dev/null +++ b/services/repository/branch_test.go @@ -0,0 +1,43 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package repository + +import ( + "testing" + + "gitea.dev/models/db" + git_model "gitea.dev/models/git" + repo_model "gitea.dev/models/repo" + "gitea.dev/models/unittest" + "gitea.dev/modules/git" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +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) +}