Skip to content

Commit

Permalink
Extend clone lock so it covers outside the "are we current" test
Browse files Browse the repository at this point in the history
There is a race condition here where we test if we are current, and
only then if we are not current we grab the lock. In the meantime,
that information could be stale.

Extend the lock to cover all operations, and unconditionally wait for
the lock. We can't assume anything can be skipped if we have to wait for
the lock.
  • Loading branch information
finnag committed Apr 25, 2024
1 parent fa0bc83 commit e46932a
Showing 1 changed file with 7 additions and 18 deletions.
25 changes: 7 additions & 18 deletions server/events/working_dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,13 @@ type FileWorkspace struct {
// multiple dirs of the same repo without deleting existing plans.
func (w *FileWorkspace) Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) {
cloneDir := w.cloneDir(p.BaseRepo, p, workspace)

// Unconditionally wait for the clone lock here, if anyone else is doing any clone
// operation in this directory, we wait for it to finish before we check anything.
value, _ := cloneLocks.LoadOrStore(cloneDir, new(sync.Mutex))
mutex := value.(*sync.Mutex)
mutex.Lock()
defer mutex.Unlock()
defer func() { w.CheckForUpstreamChanges = false }()

c := wrappedGitContext{cloneDir, headRepo, p}
Expand Down Expand Up @@ -217,15 +224,6 @@ func (w *FileWorkspace) HasDiverged(logger logging.SimpleLogging, cloneDir strin
}

func (w *FileWorkspace) forceClone(logger logging.SimpleLogging, c wrappedGitContext) error {
value, _ := cloneLocks.LoadOrStore(c.dir, new(sync.Mutex))
mutex := value.(*sync.Mutex)

defer mutex.Unlock()
if locked := mutex.TryLock(); !locked {
mutex.Lock()
return nil
}

err := os.RemoveAll(c.dir)
if err != nil {
return errors.Wrapf(err, "deleting dir '%s' before cloning", c.dir)
Expand Down Expand Up @@ -280,15 +278,6 @@ func (w *FileWorkspace) forceClone(logger logging.SimpleLogging, c wrappedGitCon
// 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(logger logging.SimpleLogging, c wrappedGitContext) error {
value, _ := cloneLocks.LoadOrStore(c.dir, new(sync.Mutex))
mutex := value.(*sync.Mutex)

defer mutex.Unlock()
if locked := mutex.TryLock(); !locked {
mutex.Lock()
return nil
}

// Reset branch as if it was cloned again
if err := w.wrappedGit(logger, c, "reset", "--hard", fmt.Sprintf("refs/remotes/origin/%s", c.pr.BaseBranch)); err != nil {
return err
Expand Down

0 comments on commit e46932a

Please sign in to comment.