Creating push comments before invoke pull request checking

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

services/pull/merge.go

@@ -248,6 +248,8 @@ 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.
 		go AddTestPullRequestTask(TestPullRequestOptions{
 			RepoID:      pr.BaseRepo.ID,
 			Doer:        doer,

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 {

services/pull/update.go

@@ -63,6 +63,8 @@ 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.
 		go AddTestPullRequestTask(TestPullRequestOptions{
 			RepoID:      pr.BaseRepo.ID,
 			Doer:        doer,

services/repository/push.go

@@ -310,15 +310,24 @@ 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
+	testPullRequestOpts := pull_service.TestPullRequestOptions{
 		RepoID:      repo.ID,
 		Doer:        pusher,
 		Branch:      branch,
 		IsSync:      true,
 		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)

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,