fix: safer re-merging with updated upstream

server/events/project_command_runner.go

@@ -528,7 +528,7 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*model
 	}
 	defer unlockFn()
 
-	p.WorkingDir.SetSafeToReClone()
+	p.WorkingDir.SetCheckForUpstreamChanges()
 	// Clone is idempotent so okay to run even if the repo was already cloned.
 	repoDir, hasDiverged, cloneErr := p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, ctx.Workspace)
 	if cloneErr != nil {

server/events/vcs/testdata/fixtures.go

@@ -22,7 +22,7 @@ import (
 	"testing"
 
 	"github.com/golang-jwt/jwt/v5"
-	"github.com/google/go-github/v52/github"
+	"github.com/google/go-github/v53/github"
 	"github.com/mcdafydd/go-azuredevops/azuredevops"
 )
 

server/events/working_dir.go

@@ -52,7 +52,7 @@ type WorkingDir interface {
 	// Set a flag in the workingdir so Clone() can know that it is safe to re-clone the workingdir if
 	// the upstream branch has been modified. This is only safe after grabbing the project lock
 	// and before running any plans
-	SetSafeToReClone()
+	SetCheckForUpstreamChanges()
 }
 
 // FileWorkspace implements WorkingDir with the file system.
@@ -79,8 +79,8 @@ type FileWorkspace struct {
 	GithubAppEnabled bool
 	// use the global setting without overriding
 	GpgNoSigningEnabled bool
-	// flag indicating if a re-clone will be safe (project lock held, about to run plan)
-	SafeToReClone bool
+	// flag indicating if we have to merge with potential new changes upstream (directly after grabbing project lock)
+	CheckForUpstreamChanges bool
 }
 
 // Clone git clones headRepo, checks out the branch and then returns the absolute
@@ -95,8 +95,7 @@ func (w *FileWorkspace) Clone(
 	p models.PullRequest,
 	workspace string) (string, bool, error) {
 	cloneDir := w.cloneDir(p.BaseRepo, p, workspace)
-	hasDiverged := false
-	defer func() { w.SafeToReClone = false }()
+	defer func() { w.CheckForUpstreamChanges = false }()
 
 	// If the directory already exists, check if it's at the right commit.
 	// If so, then we do nothing.
@@ -124,9 +123,9 @@ func (w *FileWorkspace) Clone(
 		// We're prefix matching here because BitBucket doesn't give us the full
 		// commit, only a 12 character prefix.
 		if strings.HasPrefix(currCommit, p.HeadCommit) {
-			if w.SafeToReClone && w.CheckoutMerge && w.recheckDiverged(log, p, headRepo, cloneDir) {
-				log.Info("base branch has been updated, using merge strategy and will clone again")
-				hasDiverged = true
+			if w.CheckForUpstreamChanges && w.CheckoutMerge && w.recheckDiverged(log, p, headRepo, cloneDir) {
+				log.Info("base branch has been updated, using merge strategy and will merge again")
+				return cloneDir, true, w.mergeAgain(log, cloneDir, headRepo, p)
 			} else {
 				log.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit)
 				return cloneDir, false, nil
@@ -138,7 +137,7 @@ func (w *FileWorkspace) Clone(
 	}
 
 	// Otherwise we clone the repo.
-	return cloneDir, hasDiverged, w.forceClone(log, cloneDir, headRepo, p)
+	return cloneDir, false, w.forceClone(log, cloneDir, headRepo, p)
 }
 
 // recheckDiverged returns true if the branch we're merging into has diverged
@@ -242,26 +241,7 @@ func (w *FileWorkspace) forceClone(log logging.SimpleLogging,
 		baseCloneURL = w.TestingOverrideBaseCloneURL
 	}
 
-	runGit := func(args ...string) error {
-		cmd := exec.Command("git", args...) // nolint: gosec
-		cmd.Dir = cloneDir
-		// The git merge command requires these env vars are set.
-		cmd.Env = append(os.Environ(), []string{
-			"[email protected]",
-			"GIT_AUTHOR_NAME=atlantis",
-			"GIT_COMMITTER_NAME=atlantis",
-		}...)
-
-		cmdStr := w.sanitizeGitCredentials(strings.Join(cmd.Args, " "), p.BaseRepo, headRepo)
-		output, err := cmd.CombinedOutput()
-		sanitizedOutput := w.sanitizeGitCredentials(string(output), p.BaseRepo, headRepo)
-		if err != nil {
-			sanitizedErrMsg := w.sanitizeGitCredentials(err.Error(), p.BaseRepo, headRepo)
-			return fmt.Errorf("running %s: %s: %s", cmdStr, sanitizedOutput, sanitizedErrMsg)
-		}
-		log.Debug("ran: %s. Output: %s", cmdStr, strings.TrimSuffix(sanitizedOutput, "\n"))
-		return nil
-	}
+	runGit := w.makeGitRunner(log, cloneDir, headRepo, p)
 
 	// if branch strategy, use depth=1
 	if !w.CheckoutMerge {
@@ -284,7 +264,66 @@ func (w *FileWorkspace) forceClone(log logging.SimpleLogging,
 	if err := runGit("remote", "add", "head", headCloneURL); err != nil {
 		return err
 	}
+	if w.GpgNoSigningEnabled {
+		if err := runGit("config", "--local", "commit.gpgsign", "false"); err != nil {
+			return err
+		}
+	}
+
+	return w.mergeToBaseBranch(p, runGit)
+}
+
+// There is a new upstream update that we need, and we want to update to it
+// without deleting any existing plans
+func (w *FileWorkspace) mergeAgain(log logging.SimpleLogging,
+	cloneDir string,
+	headRepo models.Repo,
+	p models.PullRequest) error {
 
+	value, _ := cloneLocks.LoadOrStore(cloneDir, new(sync.Mutex))
+	mutex := value.(*sync.Mutex)
+
+	defer mutex.Unlock()
+	if locked := mutex.TryLock(); !locked {
+		mutex.Lock()
+		return nil
+	}
+
+	runGit := w.makeGitRunner(log, cloneDir, headRepo, p)
+
+	// Reset branch as if it was cloned again
+	if err := runGit("reset", "--hard", fmt.Sprintf("refs/remotes/head/%s", p.BaseBranch)); err != nil {
+		return err
+	}
+
+	return w.mergeToBaseBranch(p, runGit)
+}
+
+func (w *FileWorkspace) makeGitRunner(log logging.SimpleLogging, cloneDir string, headRepo models.Repo, p models.PullRequest) func(args ...string) error {
+	return func(args ...string) error {
+		cmd := exec.Command("git", args...) // nolint: gosec
+		cmd.Dir = cloneDir
+		// The git merge command requires these env vars are set.
+		cmd.Env = append(os.Environ(), []string{
+			"[email protected]",
+			"GIT_AUTHOR_NAME=atlantis",
+			"GIT_COMMITTER_NAME=atlantis",
+		}...)
+
+		cmdStr := w.sanitizeGitCredentials(strings.Join(cmd.Args, " "), p.BaseRepo, headRepo)
+		output, err := cmd.CombinedOutput()
+		sanitizedOutput := w.sanitizeGitCredentials(string(output), p.BaseRepo, headRepo)
+		if err != nil {
+			sanitizedErrMsg := w.sanitizeGitCredentials(err.Error(), p.BaseRepo, headRepo)
+			return fmt.Errorf("running %s: %s: %s", cmdStr, sanitizedOutput, sanitizedErrMsg)
+		}
+		log.Debug("ran: %s. Output: %s", cmdStr, strings.TrimSuffix(sanitizedOutput, "\n"))
+		return nil
+	}
+}
+
+// Merge the PR into the base branch.
+func (w *FileWorkspace) mergeToBaseBranch(p models.PullRequest, runGit func(args ...string) error) error {
 	fetchRef := fmt.Sprintf("+refs/heads/%s:", p.HeadBranch)
 	fetchRemote := "head"
 	if w.GithubAppEnabled {
@@ -303,11 +342,6 @@ func (w *FileWorkspace) forceClone(log logging.SimpleLogging,
 		}
 	}
 
-	if w.GpgNoSigningEnabled {
-		if err := runGit("config", "--local", "commit.gpgsign", "false"); err != nil {
-			return err
-		}
-	}
 	if err := runGit("merge-base", p.BaseBranch, "FETCH_HEAD"); err != nil {
 		// git merge-base returning error means that we did not receive enough commits in shallow clone.
 		// Fall back to retrieving full repo history.
@@ -369,7 +403,7 @@ func (w *FileWorkspace) sanitizeGitCredentials(s string, base models.Repo, head
 	return strings.Replace(baseReplaced, head.CloneURL, head.SanitizedCloneURL, -1)
 }
 
-// Set the flag that indicates it is safe to re-clone if necessary
-func (w *FileWorkspace) SetSafeToReClone() {
-	w.SafeToReClone = true
+// Set the flag that indicates we need to check for upstream changes (if using merge checkout strategy)
+func (w *FileWorkspace) SetCheckForUpstreamChanges() {
+	w.CheckForUpstreamChanges = true
 }

server/events/working_dir_test.go

@@ -480,7 +480,7 @@ func TestClone_MasterHasDiverged(t *testing.T) {
 	Assert(t, hasDiverged == false, "Clone with CheckoutMerge=false should not merge")
 
 	wd.CheckoutMerge = true
-	wd.SetSafeToReClone()
+	wd.SetCheckForUpstreamChanges()
 	// Run the clone twice with the merge strategy, the first run should
 	// return true for hasDiverged, subsequent runs should
 	// return false since the first call is supposed to merge.
@@ -492,7 +492,7 @@ func TestClone_MasterHasDiverged(t *testing.T) {
 	Ok(t, err)
 	Assert(t, hasDiverged == true, "First clone with CheckoutMerge=true with diverged base should have merged")
 
-	wd.SetSafeToReClone()
+	wd.SetCheckForUpstreamChanges()
 	_, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{
 		BaseRepo:   models.Repo{CloneURL: repoDir},
 		HeadBranch: "second-pr",