Option to delay conflict checking of old pull requests until page view (#27779)

`[repository.pull-request] DELAY_CHECK_FOR_INACTIVE_DAYS` is a new
setting to delay the mergeable check for pull requests that have been
inactive for the specified number of days.

This avoids potentially long delays for big repositories with many pull
requests. and reduces system load overall when there are many
repositories or pull requests.

When viewing the PR, checking will start immediately and the PR merge
box will automatically reload when complete. Accessing the PR through
the API will also start checking immediately.

The default value of `7` provides a balance between system load, and
keeping behavior similar to what it was before both for users and API
access. With `0` all conflict checking will be delayed, while `-1`
always checks immediately to restore the previous behavior.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

services/pull/check.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"strconv"
 	"strings"
+	"time"
 
 	"code.gitea.io/gitea/models/db"
 	git_model "code.gitea.io/gitea/models/git"
@@ -25,6 +26,7 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/process"
 	"code.gitea.io/gitea/modules/queue"
+	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/timeutil"
 	asymkey_service "code.gitea.io/gitea/services/asymkey"
 	notify_service "code.gitea.io/gitea/services/notify"
@@ -34,27 +36,88 @@ import (
 var prPatchCheckerQueue *queue.WorkerPoolQueue[string]
 
 var (
-	ErrIsClosed              = errors.New("pull is closed")
-	ErrUserNotAllowedToMerge = ErrDisallowedToMerge{}
-	ErrHasMerged             = errors.New("has already been merged")
-	ErrIsWorkInProgress      = errors.New("work in progress PRs cannot be merged")
-	ErrIsChecking            = errors.New("cannot merge while conflict checking is in progress")
-	ErrNotMergeableState     = errors.New("not in mergeable state")
-	ErrDependenciesLeft      = errors.New("is blocked by an open dependency")
+	ErrIsClosed            = errors.New("pull is closed")
+	ErrNoPermissionToMerge = errors.New("no permission to merge")
+	ErrNotReadyToMerge     = errors.New("not ready to merge")
+	ErrHasMerged           = errors.New("has already been merged")
+	ErrIsWorkInProgress    = errors.New("work in progress PRs cannot be merged")
+	ErrIsChecking          = errors.New("cannot merge while conflict checking is in progress")
+	ErrNotMergeableState   = errors.New("not in mergeable state")
+	ErrDependenciesLeft    = errors.New("is blocked by an open dependency")
 )
 
-// AddToTaskQueue adds itself to pull request test task queue.
-func AddToTaskQueue(ctx context.Context, pr *issues_model.PullRequest) {
+func markPullRequestStatusAsChecking(ctx context.Context, pr *issues_model.PullRequest) bool {
 	pr.Status = issues_model.PullRequestStatusChecking
 	err := pr.UpdateColsIfNotMerged(ctx, "status")
 	if err != nil {
-		log.Error("AddToTaskQueue(%-v).UpdateCols.(add to queue): %v", pr, err)
+		log.Error("UpdateColsIfNotMerged failed, pr: %-v, err: %v", pr, err)
+		return false
+	}
+	pr, err = issues_model.GetPullRequestByID(ctx, pr.ID)
+	if err != nil {
+		log.Error("GetPullRequestByID failed, pr: %-v, err: %v", pr, err)
+		return false
+	}
+	return pr.Status == issues_model.PullRequestStatusChecking
+}
+
+var AddPullRequestToCheckQueue = realAddPullRequestToCheckQueue
+
+func realAddPullRequestToCheckQueue(prID int64) {
+	err := prPatchCheckerQueue.Push(strconv.FormatInt(prID, 10))
+	if err != nil && !errors.Is(err, queue.ErrAlreadyInQueue) {
+		log.Error("Error adding %v to the pull requests check queue: %v", prID, err)
+	}
+}
+
+func StartPullRequestCheckImmediately(ctx context.Context, pr *issues_model.PullRequest) {
+	if !markPullRequestStatusAsChecking(ctx, pr) {
 		return
 	}
-	log.Trace("Adding %-v to the test pull requests queue", pr)
-	err = prPatchCheckerQueue.Push(strconv.FormatInt(pr.ID, 10))
-	if err != nil && err != queue.ErrAlreadyInQueue {
-		log.Error("Error adding %-v to the test pull requests queue: %v", pr, err)
+	AddPullRequestToCheckQueue(pr.ID)
+}
+
+// StartPullRequestCheckDelayable will delay the check if the pull request was not updated recently.
+// When the "base" branch gets updated, all PRs targeting that "base" branch need to re-check whether
+// they are mergeable.
+// When there are too many stale PRs, each "base" branch update will consume a lot of system resources.
+// So we can delay the checks for PRs that were not updated recently, only mark their status as
+// "checking", and then next time when these PRs are updated or viewed, the real checks will run.
+func StartPullRequestCheckDelayable(ctx context.Context, pr *issues_model.PullRequest) {
+	if !markPullRequestStatusAsChecking(ctx, pr) {
+		return
+	}
+
+	if setting.Repository.PullRequest.DelayCheckForInactiveDays >= 0 {
+		if err := pr.LoadIssue(ctx); err != nil {
+			return
+		}
+		duration := 24 * time.Hour * time.Duration(setting.Repository.PullRequest.DelayCheckForInactiveDays)
+		if pr.Issue.UpdatedUnix.AddDuration(duration) <= timeutil.TimeStampNow() {
+			return
+		}
+	}
+
+	AddPullRequestToCheckQueue(pr.ID)
+}
+
+func StartPullRequestCheckOnView(ctx context.Context, pr *issues_model.PullRequest) {
+	// TODO: its correctness totally depends on the "unique queue" feature and the global lock.
+	// 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
+			})
+		}
 	}
 }
 
@@ -84,7 +147,7 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc
 			log.Error("Error whilst checking if %-v is allowed to merge %-v: %v", doer, pr, err)
 			return err
 		} else if !allowedMerge {
-			return ErrUserNotAllowedToMerge
+			return ErrNoPermissionToMerge
 		}
 
 		if mergeCheckType == MergeCheckTypeManually {
@@ -105,7 +168,7 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc
 		}
 
 		if err := CheckPullBranchProtections(ctx, pr, false); err != nil {
-			if !IsErrDisallowedToMerge(err) {
+			if !errors.Is(err, ErrNotReadyToMerge) {
 				log.Error("Error whilst checking pull branch protection for %-v: %v", pr, err)
 				return err
 			}
@@ -172,10 +235,10 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer
 	return sign, err
 }
 
-// checkAndUpdateStatus checks if pull request is possible to leaving checking status,
+// markPullRequestAsMergeable checks if pull request is possible to leaving checking status,
 // and set to be either conflict or mergeable.
-func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) {
-	// If status has not been changed to conflict by testPatch then we are mergeable
+func markPullRequestAsMergeable(ctx context.Context, pr *issues_model.PullRequest) {
+	// If status has not been changed to conflict by testPullRequestTmpRepoBranchMergeable then we are mergeable
 	if pr.Status == issues_model.PullRequestStatusChecking {
 		pr.Status = issues_model.PullRequestStatusMergeable
 	}
@@ -310,6 +373,10 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
 
 // InitializePullRequests checks and tests untested patches of pull requests.
 func InitializePullRequests(ctx context.Context) {
+	// If we prefer to delay the checks, then no need to do any check during startup, there should be not much difference
+	if setting.Repository.PullRequest.DelayCheckForInactiveDays >= 0 {
+		return
+	}
 	prs, err := issues_model.GetPullRequestIDsByCheckStatus(ctx, issues_model.PullRequestStatusChecking)
 	if err != nil {
 		log.Error("Find Checking PRs: %v", err)
@@ -320,24 +387,12 @@ func InitializePullRequests(ctx context.Context) {
 		case <-ctx.Done():
 			return
 		default:
-			log.Trace("Adding PR[%d] to the pull requests patch checking queue", prID)
-			if err := prPatchCheckerQueue.Push(strconv.FormatInt(prID, 10)); err != nil {
-				log.Error("Error adding PR[%d] to the pull requests patch checking queue %v", prID, err)
-			}
+			AddPullRequestToCheckQueue(prID)
 		}
 	}
 }
 
-// handle passed PR IDs and test the PRs
-func handler(items ...string) []string {
-	for _, s := range items {
-		id, _ := strconv.ParseInt(s, 10, 64)
-		testPR(id)
-	}
-	return nil
-}
-
-func testPR(id int64) {
+func checkPullRequestMergeable(id int64) {
 	ctx := graceful.GetManager().HammerContext()
 	releaser, err := globallock.Lock(ctx, getPullWorkingLockKey(id))
 	if err != nil {
@@ -351,7 +406,7 @@ func testPR(id int64) {
 
 	pr, err := issues_model.GetPullRequestByID(ctx, id)
 	if err != nil {
-		log.Error("Unable to GetPullRequestByID[%d] for testPR: %v", id, err)
+		log.Error("Unable to GetPullRequestByID[%d] for checkPullRequestMergeable: %v", id, err)
 		return
 	}
 
@@ -370,15 +425,15 @@ func testPR(id int64) {
 		return
 	}
 
-	if err := TestPatch(pr); err != nil {
-		log.Error("testPatch[%-v]: %v", pr, err)
+	if err := testPullRequestBranchMergeable(pr); err != nil {
+		log.Error("testPullRequestTmpRepoBranchMergeable[%-v]: %v", pr, err)
 		pr.Status = issues_model.PullRequestStatusError
 		if err := pr.UpdateCols(ctx, "status"); err != nil {
 			log.Error("update pr [%-v] status to PullRequestStatusError failed: %v", pr, err)
 		}
 		return
 	}
-	checkAndUpdateStatus(ctx, pr)
+	markPullRequestAsMergeable(ctx, pr)
 }
 
 // CheckPRsForBaseBranch check all pulls with baseBrannch
@@ -387,17 +442,21 @@ func CheckPRsForBaseBranch(ctx context.Context, baseRepo *repo_model.Repository,
 	if err != nil {
 		return err
 	}
-
 	for _, pr := range prs {
-		AddToTaskQueue(ctx, pr)
+		StartPullRequestCheckImmediately(ctx, pr)
 	}
-
 	return nil
 }
 
 // Init runs the task queue to test all the checking status pull requests
 func Init() error {
-	prPatchCheckerQueue = queue.CreateUniqueQueue(graceful.GetManager().ShutdownContext(), "pr_patch_checker", handler)
+	prPatchCheckerQueue = queue.CreateUniqueQueue(graceful.GetManager().ShutdownContext(), "pr_patch_checker", func(items ...string) []string {
+		for _, s := range items {
+			id, _ := strconv.ParseInt(s, 10, 64)
+			checkPullRequestMergeable(id)
+		}
+		return nil
+	})
 
 	if prPatchCheckerQueue == nil {
 		return errors.New("unable to create pr_patch_checker queue")