Lock pr number for dryrun instead of across whole repo (#2038)

app/api/controller/pullreq/merge.go

@@ -113,14 +113,19 @@ 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
 	// pr is already merged.
 	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()

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
 			}

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)
 		}