Re-attempt to delete temporary upload if the file is locked by another process (#12447)

Replace all calls to os.Remove/os.RemoveAll by retrying util.Remove/util.RemoveAll and remove circular dependencies from util.

Fix #12339

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: silverwind <me@silverwind.io>

12 files changed, 49 insertions, 46 deletions

diff --git a/models/admin.go b/models/admin.go
index 01f0ba3486..ebd01419a8 100644
--- a/models/admin.go
+++ b/models/admin.go
@@ -6,10 +6,10 @@ package models
 
 import (
 	"fmt"
-	"os"
 
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/unknwon/com"
 )
@@ -66,7 +66,7 @@ func RemoveAllWithNotice(title, path string) {
 }
 
 func removeAllWithNotice(e Engine, title, path string) {
-	if err := os.RemoveAll(path); err != nil {
+	if err := util.RemoveAll(path); err != nil {
 		desc := fmt.Sprintf("%s [%s]: %v", title, path, err)
 		log.Warn(title+" [%s]: %v", path, err)
 		if err = createNotice(e, NoticeRepository, desc); err != nil {
diff --git a/models/attachment.go b/models/attachment.go
index 6c3996ff01..6215de95b5 100644
--- a/models/attachment.go
+++ b/models/attachment.go
@@ -13,6 +13,7 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 
 	gouuid "github.com/google/uuid"
 	"xorm.io/xorm"
@@ -237,7 +238,7 @@ func DeleteAttachments(attachments []*Attachment, remove bool) (int, error) {
 
 	if remove {
 		for i, a := range attachments {
-			if err := os.Remove(a.LocalPath()); err != nil {
+			if err := util.Remove(a.LocalPath()); err != nil {
 				return i, err
 			}
 		}
diff --git a/models/helper_directory.go b/models/helper_directory.go
index 8119ec91a3..aed2dbcf9b 100644
--- a/models/helper_directory.go
+++ b/models/helper_directory.go
@@ -13,6 +13,7 @@ import (
 
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 )
 
 // LocalCopyPath returns the local repository temporary copy path.
@@ -41,7 +42,7 @@ func CreateTemporaryPath(prefix string) (string, error) {
 // RemoveTemporaryPath removes the temporary path
 func RemoveTemporaryPath(basePath string) error {
 	if _, err := os.Stat(basePath); !os.IsNotExist(err) {
-		return os.RemoveAll(basePath)
+		return util.RemoveAll(basePath)
 	}
 	return nil
 }
diff --git a/models/migrations/v112.go b/models/migrations/v112.go
index 76d9933689..7e80037700 100644
--- a/models/migrations/v112.go
+++ b/models/migrations/v112.go
@@ -6,10 +6,10 @@ package migrations
 
 import (
 	"fmt"
-	"os"
 	"path"
 
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"xorm.io/builder"
 	"xorm.io/xorm"
@@ -31,7 +31,7 @@ func removeAttachmentMissedRepo(x *xorm.Engine) error {
 
 		for i := 0; i < len(attachments); i++ {
 			uuid := attachments[i].UUID
-			if err = os.RemoveAll(path.Join(setting.AttachmentPath, uuid[0:1], uuid[1:2], uuid)); err != nil {
+			if err = util.RemoveAll(path.Join(setting.AttachmentPath, uuid[0:1], uuid[1:2], uuid)); err != nil {
 				fmt.Printf("Error: %v", err)
 			}
 		}
diff --git a/models/migrations/v115.go b/models/migrations/v115.go
index 9b5c70a7ac..fe3b086119 100644
--- a/models/migrations/v115.go
+++ b/models/migrations/v115.go
@@ -15,6 +15,7 @@ import (
 
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"xorm.io/xorm"
 )
@@ -110,8 +111,8 @@ func renameExistingUserAvatarName(x *xorm.Engine) error {
 	log.Info("Deleting %d old avatars ...", deleteCount)
 	i := 0
 	for file := range deleteList {
-		if err := os.Remove(file); err != nil {
-			log.Warn("os.Remove: %v", err)
+		if err := util.Remove(file); err != nil {
+			log.Warn("util.Remove: %v", err)
 		}
 		i++
 		select {
diff --git a/models/migrations/v96.go b/models/migrations/v96.go
index 9840248f61..5decb832cd 100644
--- a/models/migrations/v96.go
+++ b/models/migrations/v96.go
@@ -5,10 +5,10 @@
 package migrations
 
 import (
-	"os"
 	"path"
 
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"xorm.io/xorm"
 )
@@ -58,7 +58,7 @@ func deleteOrphanedAttachments(x *xorm.Engine) error {
 		}
 
 		for _, attachment := range attachements {
-			if err := os.RemoveAll(AttachmentLocalPath(attachment.UUID)); err != nil {
+			if err := util.RemoveAll(AttachmentLocalPath(attachment.UUID)); err != nil {
 				return err
 			}
 		}
diff --git a/models/org.go b/models/org.go
index 9fa213e17e..0915e7fd56 100644
--- a/models/org.go
+++ b/models/org.go
@@ -7,12 +7,12 @@ package models
 
 import (
 	"fmt"
-	"os"
 	"strings"
 
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/unknwon/com"
 	"xorm.io/builder"
@@ -305,14 +305,14 @@ func deleteOrg(e *xorm.Session, u *User) error {
 	//	so just keep error logs of those operations.
 	path := UserPath(u.Name)
 
-	if err := os.RemoveAll(path); err != nil {
+	if err := util.RemoveAll(path); err != nil {
 		return fmt.Errorf("Failed to RemoveAll %s: %v", path, err)
 	}
 
 	if len(u.Avatar) > 0 {
 		avatarPath := u.CustomAvatarPath()
 		if com.IsExist(avatarPath) {
-			if err := os.Remove(avatarPath); err != nil {
+			if err := util.Remove(avatarPath); err != nil {
 				return fmt.Errorf("Failed to remove %s: %v", avatarPath, err)
 			}
 		}
diff --git a/models/repo.go b/models/repo.go
index b2b6e1a26f..9f7ce8af1e 100644
--- a/models/repo.go
+++ b/models/repo.go
@@ -1440,7 +1440,7 @@ func updateRepository(e Engine, repo *Repository, visibilityChanged bool) (err e
 		// Create/Remove git-daemon-export-ok for git-daemon...
 		daemonExportFile := path.Join(repo.RepoPath(), `git-daemon-export-ok`)
 		if repo.IsPrivate && com.IsExist(daemonExportFile) {
-			if err = os.Remove(daemonExportFile); err != nil {
+			if err = util.Remove(daemonExportFile); err != nil {
 				log.Error("Failed to remove %s: %v", daemonExportFile, err)
 			}
 		} else if !repo.IsPrivate && !com.IsExist(daemonExportFile) {
@@ -1708,7 +1708,7 @@ func DeleteRepository(doer *User, uid, repoID int64) error {
 	if len(repo.Avatar) > 0 {
 		avatarPath := repo.CustomAvatarPath()
 		if com.IsExist(avatarPath) {
-			if err := os.Remove(avatarPath); err != nil {
+			if err := util.Remove(avatarPath); err != nil {
 				return fmt.Errorf("Failed to remove %s: %v", avatarPath, err)
 			}
 		}
@@ -1852,7 +1852,7 @@ func DeleteRepositoryArchives(ctx context.Context) error {
 					return ErrCancelledf("before deleting repository archives for %s", repo.FullName())
 				default:
 				}
-				return os.RemoveAll(filepath.Join(repo.RepoPath(), "archives"))
+				return util.RemoveAll(filepath.Join(repo.RepoPath(), "archives"))
 			})
 }
 
@@ -1911,7 +1911,7 @@ func deleteOldRepositoryArchives(ctx context.Context, olderThan time.Duration, i
 				}
 				toDelete := filepath.Join(path, info.Name())
 				// This is a best-effort purge, so we do not check error codes to confirm removal.
-				if err = os.Remove(toDelete); err != nil {
+				if err = util.Remove(toDelete); err != nil {
 					log.Trace("Unable to delete %s, but proceeding: %v", toDelete, err)
 				}
 			}
@@ -2280,7 +2280,7 @@ func (repo *Repository) UploadAvatar(data []byte) error {
 	}
 
 	if len(oldAvatarPath) > 0 && oldAvatarPath != repo.CustomAvatarPath() {
-		if err := os.Remove(oldAvatarPath); err != nil {
+		if err := util.Remove(oldAvatarPath); err != nil {
 			return fmt.Errorf("UploadAvatar: Failed to remove old repo avatar %s: %v", oldAvatarPath, err)
 		}
 	}
@@ -2311,7 +2311,7 @@ func (repo *Repository) DeleteAvatar() error {
 	}
 
 	if _, err := os.Stat(avatarPath); err == nil {
-		if err := os.Remove(avatarPath); err != nil {
+		if err := util.Remove(avatarPath); err != nil {
 			return fmt.Errorf("DeleteAvatar: Failed to remove %s: %v", avatarPath, err)
 		}
 	} else {
diff --git a/models/ssh_key.go b/models/ssh_key.go
index 64b859fa02..753ad57934 100644
--- a/models/ssh_key.go
+++ b/models/ssh_key.go
@@ -28,6 +28,7 @@ import (
 	"code.gitea.io/gitea/modules/process"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/unknwon/com"
 	"golang.org/x/crypto/ssh"
@@ -227,7 +228,11 @@ func SSHKeyGenParsePublicKey(key string) (string, int, error) {
 	if err != nil {
 		return "", 0, fmt.Errorf("writeTmpKeyFile: %v", err)
 	}
-	defer os.Remove(tmpName)
+	defer func() {
+		if err := util.Remove(tmpName); err != nil {
+			log.Warn("Unable to remove temporary key file: %s: Error: %v", tmpName, err)
+		}
+	}()
 
 	stdout, stderr, err := process.GetManager().Exec("SSHKeyGenParsePublicKey", setting.SSH.KeygenPath, "-lf", tmpName)
 	if err != nil {
@@ -423,7 +428,11 @@ func calcFingerprintSSHKeygen(publicKeyContent string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	defer os.Remove(tmpPath)
+	defer func() {
+		if err := util.Remove(tmpPath); err != nil {
+			log.Warn("Unable to remove temporary key file: %s: Error: %v", tmpPath, err)
+		}
+	}()
 	stdout, stderr, err := process.GetManager().Exec("AddPublicKey", "ssh-keygen", "-lf", tmpPath)
 	if err != nil {
 		if strings.Contains(stderr, "is not a public key file") {
@@ -692,7 +701,9 @@ func rewriteAllPublicKeys(e Engine) error {
 	}
 	defer func() {
 		t.Close()
-		os.Remove(tmpPath)
+		if err := util.Remove(tmpPath); err != nil {
+			log.Warn("Unable to remove temporary authorized keys file: %s: Error: %v", tmpPath, err)
+		}
 	}()
 
 	if setting.SSH.AuthorizedKeysBackup && com.IsExist(fPath) {
diff --git a/models/unit_tests.go b/models/unit_tests.go
index 9266b3a658..dca363ffac 100644
--- a/models/unit_tests.go
+++ b/models/unit_tests.go
@@ -12,10 +12,10 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
-	"time"
 
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/unknwon/com"
@@ -67,19 +67,19 @@ func MainTest(m *testing.M, pathToGiteaRoot string) {
 		fatalTestError("url.Parse: %v\n", err)
 	}
 
-	if err = removeAllWithRetry(setting.RepoRootPath); err != nil {
-		fatalTestError("os.RemoveAll: %v\n", err)
+	if err = util.RemoveAll(setting.RepoRootPath); err != nil {
+		fatalTestError("util.RemoveAll: %v\n", err)
 	}
 	if err = com.CopyDir(filepath.Join(pathToGiteaRoot, "integrations", "gitea-repositories-meta"), setting.RepoRootPath); err != nil {
 		fatalTestError("com.CopyDir: %v\n", err)
 	}
 
 	exitStatus := m.Run()
-	if err = removeAllWithRetry(setting.RepoRootPath); err != nil {
-		fatalTestError("os.RemoveAll: %v\n", err)
+	if err = util.RemoveAll(setting.RepoRootPath); err != nil {
+		fatalTestError("util.RemoveAll: %v\n", err)
 	}
-	if err = removeAllWithRetry(setting.AppDataPath); err != nil {
-		fatalTestError("os.RemoveAll: %v\n", err)
+	if err = util.RemoveAll(setting.AppDataPath); err != nil {
+		fatalTestError("util.RemoveAll: %v\n", err)
 	}
 	os.Exit(exitStatus)
 }
@@ -103,18 +103,6 @@ func CreateTestEngine(fixturesDir string) error {
 	return InitFixtures(fixturesDir)
 }
 
-func removeAllWithRetry(dir string) error {
-	var err error
-	for i := 0; i < 20; i++ {
-		err = os.RemoveAll(dir)
-		if err == nil {
-			break
-		}
-		time.Sleep(100 * time.Millisecond)
-	}
-	return err
-}
-
 // PrepareTestDatabase load test fixtures into test database
 func PrepareTestDatabase() error {
 	return LoadFixtures()
@@ -124,7 +112,7 @@ func PrepareTestDatabase() error {
 // by tests that use the above MainTest(..) function.
 func PrepareTestEnv(t testing.TB) {
 	assert.NoError(t, PrepareTestDatabase())
-	assert.NoError(t, removeAllWithRetry(setting.RepoRootPath))
+	assert.NoError(t, util.RemoveAll(setting.RepoRootPath))
 	metaPath := filepath.Join(giteaRoot, "integrations", "gitea-repositories-meta")
 	assert.NoError(t, com.CopyDir(metaPath, setting.RepoRootPath))
 	base.SetupGiteaRoot() // Makes sure GITEA_ROOT is set
diff --git a/models/upload.go b/models/upload.go
index 21c73a43f7..3ad1129c25 100644
--- a/models/upload.go
+++ b/models/upload.go
@@ -13,6 +13,7 @@ import (
 	"path"
 
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	gouuid "github.com/google/uuid"
 	"github.com/unknwon/com"
@@ -129,7 +130,7 @@ func DeleteUploads(uploads ...*Upload) (err error) {
 			continue
 		}
 
-		if err := os.Remove(localPath); err != nil {
+		if err := util.Remove(localPath); err != nil {
 			return fmt.Errorf("remove upload: %v", err)
 		}
 	}
diff --git a/models/user.go b/models/user.go
index 608bf219cf..6bd9d18b07 100644
--- a/models/user.go
+++ b/models/user.go
@@ -582,7 +582,7 @@ func (u *User) UploadAvatar(data []byte) error {
 func (u *User) DeleteAvatar() error {
 	log.Trace("DeleteAvatar[%d]: %s", u.ID, u.CustomAvatarPath())
 	if len(u.Avatar) > 0 {
-		if err := os.Remove(u.CustomAvatarPath()); err != nil {
+		if err := util.Remove(u.CustomAvatarPath()); err != nil {
 			return fmt.Errorf("Failed to remove %s: %v", u.CustomAvatarPath(), err)
 		}
 	}
@@ -1282,14 +1282,14 @@ func deleteUser(e *xorm.Session, u *User) error {
 	//	so just keep error logs of those operations.
 	path := UserPath(u.Name)
 
-	if err := os.RemoveAll(path); err != nil {
+	if err := util.RemoveAll(path); err != nil {
 		return fmt.Errorf("Failed to RemoveAll %s: %v", path, err)
 	}
 
 	if len(u.Avatar) > 0 {
 		avatarPath := u.CustomAvatarPath()
 		if com.IsExist(avatarPath) {
-			if err := os.Remove(avatarPath); err != nil {
+			if err := util.Remove(avatarPath); err != nil {
 				return fmt.Errorf("Failed to remove %s: %v", avatarPath, err)
 			}
 		}