Retry rename on lock induced failures (#16435)

* Retry rename on lock induced failures

Due to external locking on Windows it is possible for an
os.Rename to fail if the files or directories are being
used elsewhere.

This PR simply suggests retrying the rename again similar
to how we handle the os.Remove problems.

Fix #16427

Signed-off-by: Andrew Thornton <art27@cantab.net>
* resolve CI fail

Co-authored-by: techknowlogick <techknowlogick@gitea.io>

diff --git a/cmd/embedded.go b/cmd/embedded.go
index 363b85c066eaa4a93dd02af60db258d224a7bcdc..528f32402e1a2d55d642251b2ada8c8fa5650dc4 100644 (file)
--- a/cmd/embedded.go
+++ b/cmd/embedded.go
@@ -19,6 +19,7 @@ import (
        "code.gitea.io/gitea/modules/public"
        "code.gitea.io/gitea/modules/setting"
        "code.gitea.io/gitea/modules/templates"
+       "code.gitea.io/gitea/modules/util"
 
        "github.com/gobwas/glob"
        "github.com/urfave/cli"
@@ -271,7 +272,7 @@ func extractAsset(d string, a asset, overwrite, rename bool) error {
        } else if !fi.Mode().IsRegular() {
                return fmt.Errorf("%s already exists, but it's not a regular file", dest)
        } else if rename {
-               if err := os.Rename(dest, dest+".bak"); err != nil {
+               if err := util.Rename(dest, dest+".bak"); err != nil {
                        return fmt.Errorf("Error creating backup for %s: %v", dest, err)
                }
                // Attempt to respect file permissions mask (even if user:group will be set anew)

diff --git a/models/repo.go b/models/repo.go
index 143dff9ac3490c5134889faa7cc30cd60a7d9205..d6abc1b5e3883c0c004a14238c3a57dea5c58367 100644 (file)
--- a/models/repo.go
+++ b/models/repo.go
@@ -1227,7 +1227,7 @@ func ChangeRepositoryName(doer *User, repo *Repository, newRepoName string) (err
        }
 
        newRepoPath := RepoPath(repo.Owner.Name, newRepoName)
-       if err = os.Rename(repo.RepoPath(), newRepoPath); err != nil {
+       if err = util.Rename(repo.RepoPath(), newRepoPath); err != nil {
                return fmt.Errorf("rename repository directory: %v", err)
        }
 
@@ -1238,7 +1238,7 @@ func ChangeRepositoryName(doer *User, repo *Repository, newRepoName string) (err
                return err
        }
        if isExist {
-               if err = os.Rename(wikiPath, WikiPath(repo.Owner.Name, newRepoName)); err != nil {
+               if err = util.Rename(wikiPath, WikiPath(repo.Owner.Name, newRepoName)); err != nil {
                        return fmt.Errorf("rename repository wiki: %v", err)
                }
        }

diff --git a/models/repo_transfer.go b/models/repo_transfer.go
index c5d1a3a3c2172849d3db80bc882f3ac256751b6f..d7ef0a8ca6b3be29c160f1ed06bfb8cccfa86ba8 100644 (file)
--- a/models/repo_transfer.go
+++ b/models/repo_transfer.go
@@ -210,13 +210,13 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e
                }
 
                if repoRenamed {
-                       if err := os.Rename(RepoPath(newOwnerName, repo.Name), RepoPath(oldOwnerName, repo.Name)); err != nil {
+                       if err := util.Rename(RepoPath(newOwnerName, repo.Name), RepoPath(oldOwnerName, repo.Name)); err != nil {
                                log.Critical("Unable to move repository %s/%s directory from %s back to correct place %s: %v", oldOwnerName, repo.Name, RepoPath(newOwnerName, repo.Name), RepoPath(oldOwnerName, repo.Name), err)
                        }
                }
 
                if wikiRenamed {
-                       if err := os.Rename(WikiPath(newOwnerName, repo.Name), WikiPath(oldOwnerName, repo.Name)); err != nil {
+                       if err := util.Rename(WikiPath(newOwnerName, repo.Name), WikiPath(oldOwnerName, repo.Name)); err != nil {
                                log.Critical("Unable to move wiki for repository %s/%s directory from %s back to correct place %s: %v", oldOwnerName, repo.Name, WikiPath(newOwnerName, repo.Name), WikiPath(oldOwnerName, repo.Name), err)
                        }
                }
@@ -358,7 +358,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e
                return fmt.Errorf("Failed to create dir %s: %v", dir, err)
        }
 
-       if err := os.Rename(RepoPath(oldOwner.Name, repo.Name), RepoPath(newOwner.Name, repo.Name)); err != nil {
+       if err := util.Rename(RepoPath(oldOwner.Name, repo.Name), RepoPath(newOwner.Name, repo.Name)); err != nil {
                return fmt.Errorf("rename repository directory: %v", err)
        }
        repoRenamed = true
@@ -370,7 +370,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) (err e
                log.Error("Unable to check if %s exists. Error: %v", wikiPath, err)
                return err
        } else if isExist {
-               if err := os.Rename(wikiPath, WikiPath(newOwner.Name, repo.Name)); err != nil {
+               if err := util.Rename(wikiPath, WikiPath(newOwner.Name, repo.Name)); err != nil {
                        return fmt.Errorf("rename repository wiki: %v", err)
                }
                wikiRenamed = true

diff --git a/models/ssh_key.go b/models/ssh_key.go
index e35fc12e080a28549cb650245dc476e397bdb0b9..12c7bc91162ae4812b870283196f7bd21c12694a 100644 (file)
--- a/models/ssh_key.go
+++ b/models/ssh_key.go
@@ -842,7 +842,7 @@ func rewriteAllPublicKeys(e Engine) error {
        }
 
        t.Close()
-       return os.Rename(tmpPath, fPath)
+       return util.Rename(tmpPath, fPath)
 }
 
 // RegeneratePublicKeys regenerates the authorized_keys file
@@ -1324,7 +1324,7 @@ func rewriteAllPrincipalKeys(e Engine) error {
        }
 
        t.Close()
-       return os.Rename(tmpPath, fPath)
+       return util.Rename(tmpPath, fPath)
 }
 
 // ListPrincipalKeys returns a list of principals belongs to given user.

diff --git a/models/user.go b/models/user.go
index 3b8ce79251f0c2dbb8df68b3045227643b5be111..f606da53d65f08f6c17a2b7afbde23bc9d181b68 100644 (file)
--- a/models/user.go
+++ b/models/user.go
@@ -1027,7 +1027,7 @@ func ChangeUserName(u *User, newUserName string) (err error) {
        }
 
        // Do not fail if directory does not exist
-       if err = os.Rename(UserPath(oldUserName), UserPath(newUserName)); err != nil && !os.IsNotExist(err) {
+       if err = util.Rename(UserPath(oldUserName), UserPath(newUserName)); err != nil && !os.IsNotExist(err) {
                return fmt.Errorf("Rename user directory: %v", err)
        }
 
@@ -1036,7 +1036,7 @@ func ChangeUserName(u *User, newUserName string) (err error) {
        }
 
        if err = sess.Commit(); err != nil {
-               if err2 := os.Rename(UserPath(newUserName), UserPath(oldUserName)); err2 != nil && !os.IsNotExist(err2) {
+               if err2 := util.Rename(UserPath(newUserName), UserPath(oldUserName)); err2 != nil && !os.IsNotExist(err2) {
                        log.Critical("Unable to rollback directory change during failed username change from: %s to: %s. DB Error: %v. Filesystem Error: %v", oldUserName, newUserName, err, err2)
                        return fmt.Errorf("failed to rollback directory change during failed username change from: %s to: %s. DB Error: %w. Filesystem Error: %v", oldUserName, newUserName, err, err2)
                }

diff --git a/modules/log/file.go b/modules/log/file.go
index d5b38d4e0172d52e081bffe244327b464dc4df9a..79cbe740fdb8fee6d6cc8ff32fe206659c65e424 100644 (file)
--- a/modules/log/file.go
+++ b/modules/log/file.go
@@ -177,7 +177,7 @@ func (log *FileLogger) DoRotate() error {
 
                // close fd before rename
                // Rename the file to its newfound home
-               if err = os.Rename(log.Filename, fname); err != nil {
+               if err = util.Rename(log.Filename, fname); err != nil {
                        return fmt.Errorf("Rotate: %v", err)
                }
 

diff --git a/modules/storage/local.go b/modules/storage/local.go
index 46e5d60e6b5d31d1d85b9797d1194e403545ace1..1329f722c285e9f0c213c34d74d95aa2f1027ac6 100644 (file)
--- a/modules/storage/local.go
+++ b/modules/storage/local.go
@@ -96,7 +96,7 @@ func (l *LocalStorage) Save(path string, r io.Reader, size int64) (int64, error)
                return 0, err
        }
 
-       if err := os.Rename(tmp.Name(), p); err != nil {
+       if err := util.Rename(tmp.Name(), p); err != nil {
                return 0, err
        }
 

diff --git a/modules/util/remove.go b/modules/util/remove.go
index f2bbbc30b928cdd45fc1976e5e4da51afe8f9513..23104365256fc6f45e9fc98ed42e775c560ad671 100644 (file)
--- a/modules/util/remove.go
+++ b/modules/util/remove.go
@@ -33,7 +33,7 @@ func Remove(name string) error {
        return err
 }
 
-// RemoveAll removes the named file or (empty) directory with at most 5 attempts.Remove
+// RemoveAll removes the named file or (empty) directory with at most 5 attempts.
 func RemoveAll(name string) error {
        var err error
        for i := 0; i < 5; i++ {
@@ -55,3 +55,30 @@ func RemoveAll(name string) error {
        }
        return err
 }
+
+// Rename renames (moves) oldpath to newpath with at most 5 attempts.
+func Rename(oldpath, newpath string) error {
+       var err error
+       for i := 0; i < 5; i++ {
+               err = os.Rename(oldpath, newpath)
+               if err == nil {
+                       break
+               }
+               unwrapped := err.(*os.PathError).Err
+               if unwrapped == syscall.EBUSY || unwrapped == syscall.ENOTEMPTY || unwrapped == syscall.EPERM || unwrapped == syscall.EMFILE || unwrapped == syscall.ENFILE {
+                       // try again
+                       <-time.After(100 * time.Millisecond)
+                       continue
+               }
+
+               if i == 0 && os.IsNotExist(err) {
+                       return err
+               }
+
+               if unwrapped == syscall.ENOENT {
+                       // it's already gone
+                       return nil
+               }
+       }
+       return err
+}