From 51508b4b7edb8d1e81fb39cf5f953bce8a482f82 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 12 Oct 2025 23:34:14 -0700 Subject: [PATCH 1/9] Remove unnecessary AddTestPullRequestTask invoking and adjust adding push comments before checking --- services/issue/comments.go | 7 +++++++ services/pull/merge.go | 12 +----------- services/pull/pull.go | 7 +++++-- services/pull/update.go | 13 +------------ services/repository/push.go | 3 ++- tests/integration/pull_comment_test.go | 3 ++- 6 files changed, 18 insertions(+), 27 deletions(-) diff --git a/services/issue/comments.go b/services/issue/comments.go index 9442701029b57..0e341a0dfed85 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -164,6 +164,13 @@ func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) (err e c.IsForcePush = data.IsForcePush + if err := c.LoadIssue(ctx); err != nil { + return err + } + if err := c.Issue.LoadRepo(ctx); err != nil { + return err + } + if c.IsForcePush { if len(data.CommitIDs) != 2 { return nil diff --git a/services/pull/merge.go b/services/pull/merge.go index f1ad8fa17d9cc..9c9b69bc2a9fd 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -247,18 +247,8 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U return fmt.Errorf("lock.Lock: %w", err) } defer releaser() - defer func() { - go AddTestPullRequestTask(TestPullRequestOptions{ - RepoID: pr.BaseRepo.ID, - Doer: doer, - Branch: pr.BaseBranch, - IsSync: false, - IsForcePush: false, - OldCommitID: "", - NewCommitID: "", - }) - }() + // since test pull request task will be called after push, so we don't need to check again here _, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase) releaser() if err != nil { diff --git a/services/pull/pull.go b/services/pull/pull.go index 619347055b7d3..fa25e30bc51e5 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -376,7 +376,6 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) { // There is no sensible way to shut this down ":-(" // If you don't let it run all the way then you will lose data - // TODO: graceful: AddTestPullRequestTask needs to become a queue! repo, err := repo_model.GetRepositoryByID(ctx, opts.RepoID) if err != nil { @@ -402,11 +401,15 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { continue } - StartPullRequestCheckImmediately(ctx, pr) + // create push comment first then check pull request status so the test + // will get a stable result comment, err := CreatePushPullComment(ctx, opts.Doer, pr, opts.OldCommitID, opts.NewCommitID, opts.IsForcePush) if err == nil && comment != nil { notify_service.PullRequestPushCommits(ctx, opts.Doer, pr, comment) } + // FIXME: since AddTestPullRequestTask was called in a goroutine or a queue worker, is it still necessary to + // call another queue to handle the PR check? Maybe we can use checkPullRequestMergeable directly here? + StartPullRequestCheckImmediately(ctx, pr) } if opts.IsSync { diff --git a/services/pull/update.go b/services/pull/update.go index cce39374516b6..6f6e690a08017 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -62,18 +62,7 @@ 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() { - go AddTestPullRequestTask(TestPullRequestOptions{ - RepoID: pr.BaseRepo.ID, - Doer: doer, - Branch: pr.BaseBranch, - IsSync: false, - IsForcePush: false, - OldCommitID: "", - NewCommitID: "", - }) - }() - + // we don't need to run pull request check here because it will be triggered by push if rebase { return updateHeadByRebaseOnToBase(ctx, pr, doer) } diff --git a/services/repository/push.go b/services/repository/push.go index 7c68a7f176308..5ec67395b3a9d 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -310,7 +310,8 @@ func pushUpdateBranch(_ context.Context, repo *repo_model.Repository, pusher *us } // only update branch can trigger pull request task because the pull request hasn't been created yet when creating a branch - go pull_service.AddTestPullRequestTask(pull_service.TestPullRequestOptions{ + // since pushUpdateBrach is called in a queue worker, we don't need to call it in a go routine again + pull_service.AddTestPullRequestTask(pull_service.TestPullRequestOptions{ RepoID: repo.ID, Doer: pusher, Branch: branch, diff --git a/tests/integration/pull_comment_test.go b/tests/integration/pull_comment_test.go index abab65247ba74..d84f7824808a5 100644 --- a/tests/integration/pull_comment_test.go +++ b/tests/integration/pull_comment_test.go @@ -62,7 +62,8 @@ func testPullCommentRebase(t *testing.T, u *url.URL, session *TestSession) { doGitPushTestRepositoryFail(dstPath, "base-repo", "local-branch/rebase:test-branch/rebase")(t) doGitPushTestRepository(dstPath, "--force", "base-repo", "local-branch/rebase:test-branch/rebase")(t) - // reload the pr + // FIXME: Both the status of the pull request before pushing and the waiting are mergeable. So that + // if it's fast enough here, the pull request checking might be not process yet. prIssue := testWaitForPullRequestStatus(t, &issues_model.Issue{Title: testPRTitle}, issues_model.PullRequestStatusMergeable) comments, err := issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{ IssueID: prIssue.ID, From fc52fa21c8c32ba842f8486d52fc5f656cb8f1de Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 13 Oct 2025 10:19:39 -0700 Subject: [PATCH 2/9] Remove unnecessary pr lock --- services/pull/check.go | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/services/pull/check.go b/services/pull/check.go index 5b28ec9658d58..fcb3ac6226725 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -108,18 +108,9 @@ func StartPullRequestCheckOnView(ctx context.Context, pr *issues_model.PullReque // So duplicate "start" requests will be ignored if there is already a task in the queue or one is running. // Ideally in the future we should decouple the "unique queue" feature from the "start" request. if pr.Status == issues_model.PullRequestStatusChecking { - if setting.IsInTesting { - // In testing mode, there might be an "immediate" queue, which is not a real queue, everything is executed in the same goroutine - // So we can't use the global lock here, otherwise it will cause a deadlock. - AddPullRequestToCheckQueue(pr.ID) - } else { - // When a PR check starts, the task is popped from the queue and the task handler acquires the global lock - // So we need to acquire the global lock here to prevent from duplicate tasks - _, _ = globallock.TryLockAndDo(ctx, getPullWorkingLockKey(pr.ID), func(ctx context.Context) error { - AddPullRequestToCheckQueue(pr.ID) // the queue is a unique queue and won't add the same task again - return nil - }) - } + // We can't use the global lock here, otherwise it will cause a deadlock. A global lock per pull request has been + // implemented in the function checkPullRequestMergeable, so it is safe to ignore duplicate "start" requests here. + AddPullRequestToCheckQueue(pr.ID) } } From 65c14b41aaa164c772024f18d4f4f9651e1d86bf Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 13 Oct 2025 11:43:02 -0700 Subject: [PATCH 3/9] Fix test --- services/pull/check.go | 15 ++++++++++++--- services/repository/push.go | 12 ++++++++++-- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/services/pull/check.go b/services/pull/check.go index fcb3ac6226725..5b28ec9658d58 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -108,9 +108,18 @@ func StartPullRequestCheckOnView(ctx context.Context, pr *issues_model.PullReque // So duplicate "start" requests will be ignored if there is already a task in the queue or one is running. // Ideally in the future we should decouple the "unique queue" feature from the "start" request. if pr.Status == issues_model.PullRequestStatusChecking { - // We can't use the global lock here, otherwise it will cause a deadlock. A global lock per pull request has been - // implemented in the function checkPullRequestMergeable, so it is safe to ignore duplicate "start" requests here. - AddPullRequestToCheckQueue(pr.ID) + if setting.IsInTesting { + // In testing mode, there might be an "immediate" queue, which is not a real queue, everything is executed in the same goroutine + // So we can't use the global lock here, otherwise it will cause a deadlock. + AddPullRequestToCheckQueue(pr.ID) + } else { + // When a PR check starts, the task is popped from the queue and the task handler acquires the global lock + // So we need to acquire the global lock here to prevent from duplicate tasks + _, _ = globallock.TryLockAndDo(ctx, getPullWorkingLockKey(pr.ID), func(ctx context.Context) error { + AddPullRequestToCheckQueue(pr.ID) // the queue is a unique queue and won't add the same task again + return nil + }) + } } } diff --git a/services/repository/push.go b/services/repository/push.go index 5ec67395b3a9d..e0ec2bfd15f76 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -311,7 +311,7 @@ func pushUpdateBranch(_ context.Context, repo *repo_model.Repository, pusher *us // only update branch can trigger pull request task because the pull request hasn't been created yet when creating a branch // since pushUpdateBrach is called in a queue worker, we don't need to call it in a go routine again - pull_service.AddTestPullRequestTask(pull_service.TestPullRequestOptions{ + testPullRequestOpts := pull_service.TestPullRequestOptions{ RepoID: repo.ID, Doer: pusher, Branch: branch, @@ -319,7 +319,15 @@ func pushUpdateBranch(_ context.Context, repo *repo_model.Repository, pusher *us IsForcePush: isForcePush, OldCommitID: opts.OldCommitID, NewCommitID: opts.NewCommitID, - }) + } + if setting.IsInTesting { + // testing mode use immediate queue which runs in the same goroutine which will + // cause a deadlock because checkPullRequestMergeable also needs to acquire the global lock + go pull_service.AddTestPullRequestTask(testPullRequestOpts) + } else { + // real queue will run in a different goroutine so it is safe to call here + pull_service.AddTestPullRequestTask(testPullRequestOpts) + } if isForcePush { log.Trace("Push %s is a force push", opts.NewCommitID) From 504741c5af3ba75657d7e2b698dccf0e676ab32d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 13 Oct 2025 12:20:45 -0700 Subject: [PATCH 4/9] Add AddTestPullRequestTask back with comments --- services/pull/merge.go | 14 +++++++++++++- services/pull/update.go | 15 ++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/services/pull/merge.go b/services/pull/merge.go index 9c9b69bc2a9fd..e9571d19fe0dd 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -247,8 +247,20 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U return fmt.Errorf("lock.Lock: %w", err) } defer releaser() + defer func() { + // FIXME: This is a duplicated call to AddTestPullRequestTask in case the merge or push successfully but + // the post-receive hook failed. This will ensure the test task is added. + go AddTestPullRequestTask(TestPullRequestOptions{ + RepoID: pr.BaseRepo.ID, + Doer: doer, + Branch: pr.BaseBranch, + IsSync: false, + IsForcePush: false, + OldCommitID: "", + NewCommitID: "", + }) + }() - // since test pull request task will be called after push, so we don't need to check again here _, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase) releaser() if err != nil { diff --git a/services/pull/update.go b/services/pull/update.go index 6f6e690a08017..bebbfd9c17462 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -62,7 +62,20 @@ 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) } - // we don't need to run pull request check here because it will be triggered by push + defer func() { + // FIXME: This is a duplicated call to AddTestPullRequestTask in case the git push successfully but + // the post-receive hook failed. This will ensure the test task is added. + go AddTestPullRequestTask(TestPullRequestOptions{ + RepoID: pr.BaseRepo.ID, + Doer: doer, + Branch: pr.BaseBranch, + IsSync: false, + IsForcePush: false, + OldCommitID: "", + NewCommitID: "", + }) + }() + if rebase { return updateHeadByRebaseOnToBase(ctx, pr, doer) } From 035dd1e961b75a32798c0b971b2310ce8d052649 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Oct 2025 11:21:39 +0800 Subject: [PATCH 5/9] fix --- services/pull/pull.go | 8 ++++---- services/repository/push.go | 10 +--------- tests/integration/pull_comment_test.go | 2 -- 3 files changed, 5 insertions(+), 15 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index fa25e30bc51e5..2cc429c7ac59f 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -401,14 +401,14 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { continue } - // create push comment first then check pull request status so the test - // will get a stable result + // create push comment before check pull request status, + // then when the status is mergeable, the comment is already in database, to make testing easy and stable comment, err := CreatePushPullComment(ctx, opts.Doer, pr, opts.OldCommitID, opts.NewCommitID, opts.IsForcePush) if err == nil && comment != nil { notify_service.PullRequestPushCommits(ctx, opts.Doer, pr, comment) } - // FIXME: since AddTestPullRequestTask was called in a goroutine or a queue worker, is it still necessary to - // call another queue to handle the PR check? Maybe we can use checkPullRequestMergeable directly here? + // The caller can be in a goroutine or a "push queue", "conflict check" can be time-consuming, + // and the concurrency should be limited, so the conflict check will be done in another queue StartPullRequestCheckImmediately(ctx, pr) } diff --git a/services/repository/push.go b/services/repository/push.go index e0ec2bfd15f76..576743a02c54d 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -310,7 +310,6 @@ func pushUpdateBranch(_ context.Context, repo *repo_model.Repository, pusher *us } // only update branch can trigger pull request task because the pull request hasn't been created yet when creating a branch - // since pushUpdateBrach is called in a queue worker, we don't need to call it in a go routine again testPullRequestOpts := pull_service.TestPullRequestOptions{ RepoID: repo.ID, Doer: pusher, @@ -320,14 +319,7 @@ func pushUpdateBranch(_ context.Context, repo *repo_model.Repository, pusher *us OldCommitID: opts.OldCommitID, NewCommitID: opts.NewCommitID, } - if setting.IsInTesting { - // testing mode use immediate queue which runs in the same goroutine which will - // cause a deadlock because checkPullRequestMergeable also needs to acquire the global lock - go pull_service.AddTestPullRequestTask(testPullRequestOpts) - } else { - // real queue will run in a different goroutine so it is safe to call here - pull_service.AddTestPullRequestTask(testPullRequestOpts) - } + go pull_service.AddTestPullRequestTask(testPullRequestOpts) if isForcePush { log.Trace("Push %s is a force push", opts.NewCommitID) diff --git a/tests/integration/pull_comment_test.go b/tests/integration/pull_comment_test.go index d84f7824808a5..ed6027b3600d5 100644 --- a/tests/integration/pull_comment_test.go +++ b/tests/integration/pull_comment_test.go @@ -62,8 +62,6 @@ func testPullCommentRebase(t *testing.T, u *url.URL, session *TestSession) { doGitPushTestRepositoryFail(dstPath, "base-repo", "local-branch/rebase:test-branch/rebase")(t) doGitPushTestRepository(dstPath, "--force", "base-repo", "local-branch/rebase:test-branch/rebase")(t) - // FIXME: Both the status of the pull request before pushing and the waiting are mergeable. So that - // if it's fast enough here, the pull request checking might be not process yet. prIssue := testWaitForPullRequestStatus(t, &issues_model.Issue{Title: testPRTitle}, issues_model.PullRequestStatusMergeable) comments, err := issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{ IssueID: prIssue.ID, From 50c5cd181467b76683c7e57ad812afbecf94f178 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Oct 2025 11:30:34 +0800 Subject: [PATCH 6/9] fix --- services/issue/comments.go | 34 +++++++++++++------------- services/pull/pull.go | 3 --- services/repository/push.go | 5 ++-- tests/integration/pull_comment_test.go | 1 + 4 files changed, 20 insertions(+), 23 deletions(-) diff --git a/services/issue/comments.go b/services/issue/comments.go index 0e341a0dfed85..871f2a8712026 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -15,6 +15,7 @@ import ( user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/json" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/timeutil" git_service "code.gitea.io/gitea/services/git" notify_service "code.gitea.io/gitea/services/notify" @@ -151,33 +152,31 @@ func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_m } // LoadCommentPushCommits Load push commits -func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) (err error) { +func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) error { if c.Content == "" || c.Commits != nil || c.Type != issues_model.CommentTypePullRequestPush { return nil } var data issues_model.PushActionContent - err = json.Unmarshal([]byte(c.Content), &data) - if err != nil { - return err + if err := json.Unmarshal([]byte(c.Content), &data); err != nil { + log.Debug("Unmarshal: %v", err) // no need to show 500 error to end user when the JSON is broken + return nil } c.IsForcePush = data.IsForcePush - - if err := c.LoadIssue(ctx); err != nil { - return err - } - if err := c.Issue.LoadRepo(ctx); err != nil { - return err - } - if c.IsForcePush { if len(data.CommitIDs) != 2 { return nil } - c.OldCommit = data.CommitIDs[0] - c.NewCommit = data.CommitIDs[1] + c.OldCommit, c.NewCommit = data.CommitIDs[0], data.CommitIDs[1] } else { + if err := c.LoadIssue(ctx); err != nil { + return err + } + if err := c.Issue.LoadRepo(ctx); err != nil { + return err + } + gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, c.Issue.Repo) if err != nil { return err @@ -186,10 +185,11 @@ func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) (err e c.Commits, err = git_service.ConvertFromGitCommit(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs), c.Issue.Repo) if err != nil { - return err + log.Debug("GetCommitsFromIDs: %v", err) // no need to show 500 error to end user when the commit does not exist + } else { + c.CommitsNum = int64(len(c.Commits)) } - c.CommitsNum = int64(len(c.Commits)) } - return err + return nil } diff --git a/services/pull/pull.go b/services/pull/pull.go index 2cc429c7ac59f..2b0bcaebe8550 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -374,9 +374,6 @@ type TestPullRequestOptions struct { func AddTestPullRequestTask(opts TestPullRequestOptions) { log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", opts.RepoID, opts.Branch) graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) { - // There is no sensible way to shut this down ":-(" - // If you don't let it run all the way then you will lose data - repo, err := repo_model.GetRepositoryByID(ctx, opts.RepoID) if err != nil { log.Error("GetRepositoryByID: %v", err) diff --git a/services/repository/push.go b/services/repository/push.go index 576743a02c54d..7c68a7f176308 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -310,7 +310,7 @@ func pushUpdateBranch(_ context.Context, repo *repo_model.Repository, pusher *us } // only update branch can trigger pull request task because the pull request hasn't been created yet when creating a branch - testPullRequestOpts := pull_service.TestPullRequestOptions{ + go pull_service.AddTestPullRequestTask(pull_service.TestPullRequestOptions{ RepoID: repo.ID, Doer: pusher, Branch: branch, @@ -318,8 +318,7 @@ func pushUpdateBranch(_ context.Context, repo *repo_model.Repository, pusher *us IsForcePush: isForcePush, OldCommitID: opts.OldCommitID, NewCommitID: opts.NewCommitID, - } - go pull_service.AddTestPullRequestTask(testPullRequestOpts) + }) if isForcePush { log.Trace("Push %s is a force push", opts.NewCommitID) diff --git a/tests/integration/pull_comment_test.go b/tests/integration/pull_comment_test.go index ed6027b3600d5..abab65247ba74 100644 --- a/tests/integration/pull_comment_test.go +++ b/tests/integration/pull_comment_test.go @@ -62,6 +62,7 @@ func testPullCommentRebase(t *testing.T, u *url.URL, session *TestSession) { doGitPushTestRepositoryFail(dstPath, "base-repo", "local-branch/rebase:test-branch/rebase")(t) doGitPushTestRepository(dstPath, "--force", "base-repo", "local-branch/rebase:test-branch/rebase")(t) + // reload the pr prIssue := testWaitForPullRequestStatus(t, &issues_model.Issue{Title: testPRTitle}, issues_model.PullRequestStatusMergeable) comments, err := issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{ IssueID: prIssue.ID, From f06c1a31a135845c0d5e4f62d03112a940706f7b Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Oct 2025 11:35:26 +0800 Subject: [PATCH 7/9] fix --- services/issue/comments.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/issue/comments.go b/services/issue/comments.go index 871f2a8712026..3ce2e2a5e13db 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -164,6 +164,7 @@ func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) error } c.IsForcePush = data.IsForcePush + if c.IsForcePush { if len(data.CommitIDs) != 2 { return nil @@ -185,7 +186,7 @@ func LoadCommentPushCommits(ctx context.Context, c *issues_model.Comment) error c.Commits, err = git_service.ConvertFromGitCommit(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs), c.Issue.Repo) if err != nil { - log.Debug("GetCommitsFromIDs: %v", err) // no need to show 500 error to end user when the commit does not exist + log.Debug("ConvertFromGitCommit: %v", err) // no need to show 500 error to end user when the commit does not exist } else { c.CommitsNum = int64(len(c.Commits)) } From 565c13a4401dabafff5f0dc0ef8bb4ff57e0dd4e Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Oct 2025 11:41:42 +0800 Subject: [PATCH 8/9] fix comment --- services/pull/pull.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/pull/pull.go b/services/pull/pull.go index 2b0bcaebe8550..a519a1032f1ca 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -374,6 +374,8 @@ type TestPullRequestOptions struct { func AddTestPullRequestTask(opts TestPullRequestOptions) { log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", opts.RepoID, opts.Branch) graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) { + // this function does a lot of operations to various models, if the process gets killed in the middle, + // there is no way to recover at the moment. The best workaround is to let end user push again. repo, err := repo_model.GetRepositoryByID(ctx, opts.RepoID) if err != nil { log.Error("GetRepositoryByID: %v", err) From a903fdd6406234b430383d65cde4f2156d5bbf1c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Oct 2025 12:05:46 +0800 Subject: [PATCH 9/9] fix comment --- services/pull/merge.go | 7 +++++-- services/pull/update.go | 5 +++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/services/pull/merge.go b/services/pull/merge.go index e9571d19fe0dd..1e8e9d444b593 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -248,8 +248,11 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U } defer releaser() defer func() { - // FIXME: This is a duplicated call to AddTestPullRequestTask in case the merge or push successfully but - // the post-receive hook failed. This will ensure the test task is added. + // 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, diff --git a/services/pull/update.go b/services/pull/update.go index bebbfd9c17462..436e3b52a657b 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -63,8 +63,9 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. } defer func() { - // FIXME: This is a duplicated call to AddTestPullRequestTask in case the git push successfully but - // the post-receive hook failed. This will ensure the test task is added. + // 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,