From 38747d48fe06ae751fa1b501c3a5496b71b70286 Mon Sep 17 00:00:00 2001 From: bircni Date: Tue, 25 Aug 2026 19:04:49 +0200 Subject: [PATCH] fix(pull): keep the merged state in sync with git (#39062) Signed-off-by: wxiaoguang Co-authored-by: wxiaoguang Co-authored-by: silverwind --- routers/api/v1/repo/pull.go | 5 +-- routers/web/repo/pull.go | 12 +++-- services/automerge/automerge.go | 2 +- services/pull/merge.go | 64 ++++++++++++++++----------- services/pull/update.go | 29 ++++++------ services/repository/merge_upstream.go | 2 +- tests/integration/pull_merge_test.go | 12 ++--- 7 files changed, 66 insertions(+), 60 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index cdc0ec00c89..d5c2476a324 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -22,7 +22,6 @@ import ( "gitea.dev/modules/base" "gitea.dev/modules/git" "gitea.dev/modules/git/gitcmd" - "gitea.dev/modules/graceful" "gitea.dev/modules/log" "gitea.dev/modules/optional" "gitea.dev/modules/setting" @@ -1041,7 +1040,7 @@ func MergePullRequest(ctx *context.APIContext) { } } - if err := pull_service.Merge(ctx, pr, ctx.Doer, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil { + if err := pull_service.Merge(pr, ctx.Doer, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil { if pull_service.IsErrInvalidMergeStyle(err) { ctx.APIError(http.StatusMethodNotAllowed, fmt.Sprintf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do))) } else if conflictError, ok := err.(pull_service.ErrMergeConflicts); ok { @@ -1275,7 +1274,7 @@ func UpdatePullRequest(ctx *context.APIContext) { // default merge commit message message := fmt.Sprintf("Merge branch '%s' into %s", pr.BaseBranch, pr.HeadBranch) - if err = pull_service.Update(graceful.GetManager().ShutdownContext(), pr, ctx.Doer, message, rebase); err != nil { + if err = pull_service.Update(pr, ctx.Doer, message, rebase); err != nil { if pull_service.IsErrMergeConflicts(err) { ctx.APIError(http.StatusConflict, "merge failed because of conflict") return diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 23ad59dae84..82f171176c1 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -29,7 +29,6 @@ import ( "gitea.dev/modules/git" "gitea.dev/modules/git/gitcmd" "gitea.dev/modules/glob" - "gitea.dev/modules/graceful" issue_template "gitea.dev/modules/issue/template" "gitea.dev/modules/log" "gitea.dev/modules/optional" @@ -1002,9 +1001,7 @@ func UpdatePullRequest(ctx *context.Context) { // default merge commit message message := fmt.Sprintf("Merge branch '%s' into %s", issue.PullRequest.BaseBranch, issue.PullRequest.HeadBranch) - // The update process should not be canceled by the user - // so we set the context to be a background context - if err = pull_service.Update(graceful.GetManager().ShutdownContext(), issue.PullRequest, ctx.Doer, message, rebase); err != nil { + if err = pull_service.Update(issue.PullRequest, ctx.Doer, message, rebase); err != nil { if conflictError, ok := err.(pull_service.ErrMergeConflicts); ok { flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{ "Message": ctx.Tr("repo.pulls.merge_conflict"), @@ -1149,7 +1146,7 @@ func MergePullRequest(ctx *context.Context) { } } - if err := pull_service.Merge(ctx, pr, ctx.Doer, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil { + if err := pull_service.Merge(pr, ctx.Doer, repo_model.MergeStyle(form.Do), form.HeadCommitID, message, false); err != nil { if pull_service.IsErrInvalidMergeStyle(err) { ctx.JSONError(ctx.Tr("repo.pulls.invalid_merge_option")) } else if conflictError, ok := err.(pull_service.ErrMergeConflicts); ok { @@ -1213,13 +1210,14 @@ func MergePullRequest(ctx *context.Context) { } log.Trace("Pull request merged: %d", pr.ID) + // FIXME: calling it here is wrong. + // 1. the ctx might have been canceled ("Merge" might take a very long time and the user closes their browser) + // 2. it is inconsistent with API/AutoMerge which all miss the call if err := stopTimerIfAvailable(ctx, ctx.Doer, issue); err != nil { ctx.ServerError("stopTimerIfAvailable", err) return } - log.Trace("Pull request merged: %d", pr.ID) - if deleteBranchAfterMerge { deleteBranchAfterMergeAndFlashMessage(ctx, pr.ID) if ctx.Written() { diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index d96f6b8d2aa..f9ede5795c4 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -259,7 +259,7 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { return } - if err := pull_service.Merge(ctx, pr, doer, scheduledPRM.MergeStyle, "", scheduledPRM.Message, true); err != nil { + if err := pull_service.Merge(pr, doer, scheduledPRM.MergeStyle, "", scheduledPRM.Message, true); err != nil { log.Error("pull_service.Merge: %v", err) // FIXME: if merge failed, we should display some error message to the pull request page. // The resolution is add a new column on automerge table named `error_message` to store the error message and displayed diff --git a/services/pull/merge.go b/services/pull/merge.go index 737d000b019..d48fa95da35 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -27,6 +27,7 @@ import ( "gitea.dev/modules/git" "gitea.dev/modules/git/gitcmd" "gitea.dev/modules/globallock" + "gitea.dev/modules/graceful" "gitea.dev/modules/httplib" "gitea.dev/modules/log" "gitea.dev/modules/references" @@ -234,9 +235,28 @@ func (err ErrInvalidMergeStyle) Unwrap() error { return util.ErrInvalidArgument } +func addTestPullRequestTaskAfterWebOperation(pr *issues_model.PullRequest, doer *user_model.User) { + // This is a duplicated call to AddTestPullRequestTask (it will also be called by the post-receive hook, via a push queue). + // This call will do some operations (push to base repo, sync commit divergence, add PR conflict check queue task, etc) + // immediately instead of waiting for the "push queue"'s task. The code is from https://github.com/go-gitea/gitea/pull/7082. + // But it's really questionable whether it's worth to do it ahead without waiting for the "push queue" task to run. + // TODO: DUPLICATE-PR-TASK: maybe can try to remove this in 1.26 to see if there is any issue. + go AddTestPullRequestTask(TestPullRequestOptions{ + RepoID: pr.BaseRepo.ID, + Doer: doer, + Branch: pr.BaseBranch, + IsSync: false, + IsForcePush: false, + OldCommitID: "", + NewCommitID: "", + }) +} + // Merge merges pull request to base repository. // Caller should check PR is ready to be merged (review and status checks) -func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, wasAutoMerged bool) error { +func Merge(pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, wasAutoMerged bool) error { + ctx := graceful.GetManager().HammerContext() // don't abort the git operation even if the user's request is canceled + if err := pr.LoadBaseRepo(ctx); err != nil { log.Error("Unable to load base repo: %v", err) return fmt.Errorf("unable to load base repo: %w", err) @@ -257,37 +277,27 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U return ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: mergeStyle} } - releaser, err := globallock.Lock(ctx, getPullWorkingLockKey(pr.ID)) - if err != nil { - log.Error("lock.Lock(): %v", err) - return fmt.Errorf("lock.Lock: %w", err) - } - defer releaser() - defer func() { - // This is a duplicated call to AddTestPullRequestTask (it will also be called by the post-receive hook, via a push queue). - // This call will do some operations (push to base repo, sync commit divergence, add PR conflict check queue task, etc) - // immediately instead of waiting for the "push queue"'s task. The code is from https://github.com/go-gitea/gitea/pull/7082. - // But it's really questionable whether it's worth to do it ahead without waiting for the "push queue" task to run. - // TODO: DUPLICATE-PR-TASK: maybe can try to remove this in 1.26 to see if there is any issue. - go AddTestPullRequestTask(TestPullRequestOptions{ - RepoID: pr.BaseRepo.ID, - Doer: doer, - Branch: pr.BaseBranch, - IsSync: false, - IsForcePush: false, - OldCommitID: "", - NewCommitID: "", - }) - }() - - _, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase) - releaser() + err = globallock.LockAndDo(ctx, getPullWorkingLockKey(pr.ID), func(ctx context.Context) error { + _, err := doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase) + return err + }) + defer addTestPullRequestTaskAfterWebOperation(pr, doer) // keep the same behavior as old code: always call AddTestPullRequestTask + // TODO: the "merge" operation has finished, there could still be some edge cases: + // * if the post-process hook isn't executed correctly: + // * the commit has been merged into target branch + // * the PR's status is still "open (unmerged)" + // * something wrong happens (e.g.: out of sync?) + // * maybe this is the reason that why the duplicate AddTestPullRequestTask is called in defer func above if err != nil { return err } + // TODO: it is questionable whether it should return error here, the "merge" operation has succeeded + return handleMergePostProcess(ctx, pr.ID, doer, wasAutoMerged) +} +func handleMergePostProcess(ctx context.Context, prID int64, doer *user_model.User, wasAutoMerged bool) error { // reload pull request because it has been updated by post receive hook - pr, err = issues_model.GetPullRequestByID(ctx, pr.ID) + pr, err := issues_model.GetPullRequestByID(ctx, prID) if err != nil { return err } diff --git a/services/pull/update.go b/services/pull/update.go index d4840b5fb12..e4bc5327ad7 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -16,12 +16,14 @@ import ( user_model "gitea.dev/models/user" "gitea.dev/modules/git" "gitea.dev/modules/globallock" + "gitea.dev/modules/graceful" "gitea.dev/modules/log" "gitea.dev/modules/repository" ) // Update updates pull request with base branch. -func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, message string, rebase bool) error { +func Update(pr *issues_model.PullRequest, doer *user_model.User, message string, rebase bool) error { + ctx := graceful.GetManager().HammerContext() // don't abort the git operation even if the user's request is canceled if pr.Flow == issues_model.PullRequestFlowAGit { // TODO: update of agit flow pull request's head branch is unsupported return errors.New("update of agit flow pull request's head branch is unsupported") @@ -62,20 +64,10 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. return fmt.Errorf("unable to load HeadRepo for PR[%d] during update-by-merge: %w", pr.ID, err) } - defer func() { - // The code is from https://github.com/go-gitea/gitea/pull/9784, - // it seems a simple copy-paste from https://github.com/go-gitea/gitea/pull/7082 without a real reason. - // TODO: DUPLICATE-PR-TASK: search and see another TODO comment for more details - go AddTestPullRequestTask(TestPullRequestOptions{ - RepoID: pr.BaseRepo.ID, - Doer: doer, - Branch: pr.BaseBranch, - IsSync: false, - IsForcePush: false, - OldCommitID: "", - NewCommitID: "", - }) - }() + // TODO: The code is from https://github.com/go-gitea/gitea/pull/9784, + // it seems a simple copy-paste from https://github.com/go-gitea/gitea/pull/7082 without a real reason. + // TODO: DUPLICATE-PR-TASK: search and see another TODO comment for more details + defer addTestPullRequestTaskAfterWebOperation(pr, doer) if rebase { return updateHeadByRebaseOnToBase(ctx, pr, doer) @@ -97,6 +89,13 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. } _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, repository.PushTriggerPRUpdateWithBase) + // TODO: the "update" (merge target branch to PR head branch) operation has finished, there could still be some edge cases: + // * the database was already out of sync: the target branch was already in head branch: + // * so no post-receive hook is really executed, no PR status update + // * then the PR status is stuck in "behind the target branch" (a new push can be used as a workaround) + // * "merge" operation does finish, but the post-receive hook isn't correctly executed due to other reasons: + // * although the target branch is merged into head branch by this "update" (head branch receives new commits) + // * but database isn't updated, so the PR status is still "behind the target branch" return err } diff --git a/services/repository/merge_upstream.go b/services/repository/merge_upstream.go index cceb842995e..6820fca3372 100644 --- a/services/repository/merge_upstream.go +++ b/services/repository/merge_upstream.go @@ -87,7 +87,7 @@ func MergeUpstream(ctx reqctx.RequestContext, doer *user_model.User, repo *repo_ BaseBranch: divergingInfo.BaseBranchName, } fakeIssue.PullRequest = fakePR - err = pull.Update(ctx, fakePR, doer, "merge upstream", false) + err = pull.Update(fakePR, doer, "merge upstream", false) if err != nil { return "", err } diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 1d1f5ed2e95..eecff58eaad 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -360,11 +360,11 @@ func TestCantMergeConflict(t *testing.T) { BaseBranch: "base", }) - err := pull_service.Merge(t.Context(), pr, user1, repo_model.MergeStyleMerge, "", "CONFLICT", false) + err := pull_service.Merge(pr, user1, repo_model.MergeStyleMerge, "", "CONFLICT", false) assert.Error(t, err, "Merge should return an error due to conflict") assert.True(t, pull_service.IsErrMergeConflicts(err), "Merge error is not a conflict error") - err = pull_service.Merge(t.Context(), pr, user1, repo_model.MergeStyleRebase, "", "CONFLICT", false) + err = pull_service.Merge(pr, user1, repo_model.MergeStyleRebase, "", "CONFLICT", false) assert.Error(t, err, "Merge should return an error due to conflict") assert.True(t, pull_service.IsErrRebaseConflicts(err), "Merge error is not a conflict error") }) @@ -455,7 +455,7 @@ func TestCantMergeUnrelated(t *testing.T) { BaseBranch: "base", }) - err = pull_service.Merge(t.Context(), pr, user1, repo_model.MergeStyleMerge, "", "UNRELATED", false) + err = pull_service.Merge(pr, user1, repo_model.MergeStyleMerge, "", "UNRELATED", false) assert.Error(t, err, "Merge should return an error due to unrelated") assert.True(t, pull_service.IsErrMergeUnrelatedHistories(err), "Merge error is not a unrelated histories error") }) @@ -491,7 +491,7 @@ func TestFastForwardOnlyMerge(t *testing.T) { BaseBranch: "master", }) - err := pull_service.Merge(t.Context(), pr, user1, repo_model.MergeStyleFastForwardOnly, "", "FAST-FORWARD-ONLY", false) + err := pull_service.Merge(pr, user1, repo_model.MergeStyleFastForwardOnly, "", "FAST-FORWARD-ONLY", false) assert.NoError(t, err) }) } @@ -578,7 +578,7 @@ func TestFastForwardOnlyMergeWithRequiredSignedCommits(t *testing.T) { pb.RequireSignedCommits = false require.NoError(t, git_model.UpdateProtectBranch(t.Context(), repo1, pb, git_model.WhitelistOptions{})) - require.NoError(t, pull_service.Merge(t.Context(), pr, user1, repo_model.MergeStyleFastForwardOnly, "", "FAST-FORWARD-ONLY", false)) + require.NoError(t, pull_service.Merge(pr, user1, repo_model.MergeStyleFastForwardOnly, "", "FAST-FORWARD-ONLY", false)) }) } @@ -613,7 +613,7 @@ func TestCantFastForwardOnlyMergeDiverging(t *testing.T) { BaseBranch: "master", }) - err := pull_service.Merge(t.Context(), pr, user1, repo_model.MergeStyleFastForwardOnly, "", "DIVERGING", false) + err := pull_service.Merge(pr, user1, repo_model.MergeStyleFastForwardOnly, "", "DIVERGING", false) assert.Error(t, err, "Merge should return an error due to being for a diverging branch") assert.True(t, pull_service.IsErrMergeDivergingFastForwardOnly(err), "Merge error is not a diverging fast-forward-only error") })