From 38553bca7bf171942c165a7bbc92c0a81183379e Mon Sep 17 00:00:00 2001
From: Johannes Batzill <johannes.batzill@harness.io>
Date: Mon, 20 May 2024 18:45:09 +0000
Subject: [PATCH] Lock pr number for dryrun instead of across whole repo
 (#2038)

---
 app/api/controller/pullreq/merge.go        | 12 +++++++++++-
 app/services/pullreq/handlers_branch.go    |  4 ++--
 app/services/pullreq/handlers_mergeable.go |  5 +++++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/app/api/controller/pullreq/merge.go b/app/api/controller/pullreq/merge.go
index 356931b804..5e7d736732 100644
--- a/app/api/controller/pullreq/merge.go
+++ b/app/api/controller/pullreq/merge.go
@@ -113,6 +113,11 @@ func (c *Controller) Merge(
 	// the max time we give a merge to succeed
 	const timeout = 3 * time.Minute
 
+	var lockID int64 // 0 means locking repo level for prs (for actual merging)
+	if in.DryRun {
+		lockID = pullreqNum // dryrun doesn't need repo level lock
+	}
+
 	// if two requests for merging comes at the same time then unlock will lock
 	// first one and second one will wait, when first one is done then second one
 	// continue with latest data from db with state merged and return error that
@@ -120,7 +125,7 @@ func (c *Controller) Merge(
 	unlock, err := c.locker.LockPR(
 		ctx,
 		targetRepo.ID,
-		0,                      // 0 means locks all PRs for this repo
+		lockID,
 		timeout+30*time.Second, // add 30s to the lock to give enough time for pre + post merge
 	)
 	if err != nil {
@@ -247,6 +252,11 @@ func (c *Controller) Merge(
 				if pr.SourceSHA != mergeOutput.HeadSHA.String() {
 					return errors.New("source SHA has changed")
 				}
+				// actual merge is using a different lock - ensure we don't overwrite any merge results.
+				if pr.State != enum.PullReqStateOpen {
+					return usererror.BadRequest("Pull request must be open")
+				}
+
 				if len(mergeOutput.ConflictFiles) > 0 {
 					pr.MergeCheckStatus = enum.MergeCheckStatusConflict
 					pr.MergeBaseSHA = mergeOutput.MergeBaseSHA.String()
diff --git a/app/services/pullreq/handlers_branch.go b/app/services/pullreq/handlers_branch.go
index 3bb84ddc6e..6d6f5b44ea 100644
--- a/app/services/pullreq/handlers_branch.go
+++ b/app/services/pullreq/handlers_branch.go
@@ -83,7 +83,7 @@ func (s *Service) triggerPREventOnBranchUpdate(ctx context.Context,
 
 		// Update the database with the latest source commit SHA and the merge base SHA.
 		pr, err = s.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error {
-			// to avoid racing conditions
+			// to avoid racing conditions with merge
 			if pr.State != enum.PullReqStateOpen {
 				return errPRNotOpen
 			}
@@ -163,7 +163,7 @@ func (s *Service) closePullReqOnBranchDelete(ctx context.Context,
 
 		var activitySeqBranchDeleted, activitySeqPRClosed int64
 		pr, err = s.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error {
-			// to avoid racing conditions
+			// to avoid racing conditions with merge
 			if pr.State != enum.PullReqStateOpen {
 				return errPRNotOpen
 			}
diff --git a/app/services/pullreq/handlers_mergeable.go b/app/services/pullreq/handlers_mergeable.go
index 1d477834d8..d3ce4dbbc1 100644
--- a/app/services/pullreq/handlers_mergeable.go
+++ b/app/services/pullreq/handlers_mergeable.go
@@ -212,6 +212,11 @@ func (s *Service) updateMergeData(
 
 	// Update DB in both cases (failure or success)
 	_, err = s.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error {
+		// to avoid racing conditions with merge
+		if pr.State != enum.PullReqStateOpen {
+			return errPRNotOpen
+		}
+
 		if pr.SourceSHA != newSHA {
 			return events.NewDiscardEventErrorf("PR SHA %s is newer than %s", pr.SourceSHA, newSHA)
 		}