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>

8 files changed, 32 insertions, 27 deletions

diff --git a/integrations/api_repo_test.go b/integrations/api_repo_test.go
index f0f43ac32c..0d0d4a117b 100644
--- a/integrations/api_repo_test.go
+++ b/integrations/api_repo_test.go
@@ -9,12 +9,12 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/url"
-	"os"
 	"testing"
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -341,7 +341,7 @@ func testAPIRepoMigrateConflict(t *testing.T, u *url.URL) {
 		httpContext.Reponame = "repo-tmp-17"
 		dstPath, err := ioutil.TempDir("", httpContext.Reponame)
 		assert.NoError(t, err)
-		defer os.RemoveAll(dstPath)
+		defer util.RemoveAll(dstPath)
 		t.Run("CreateRepo", doAPICreateRepository(httpContext, false))
 
 		user, err := models.GetUserByName(httpContext.Username)
@@ -404,7 +404,7 @@ func testAPIRepoCreateConflict(t *testing.T, u *url.URL) {
 		httpContext.Reponame = "repo-tmp-17"
 		dstPath, err := ioutil.TempDir("", httpContext.Reponame)
 		assert.NoError(t, err)
-		defer os.RemoveAll(dstPath)
+		defer util.RemoveAll(dstPath)
 		t.Run("CreateRepo", doAPICreateRepository(httpContext, false))
 
 		req := NewRequestWithJSON(t, "POST", "/api/v1/user/repos?token="+httpContext.Token,
diff --git a/integrations/create_no_session_test.go b/integrations/create_no_session_test.go
index 8d5b6f2f7c..5380d17509 100644
--- a/integrations/create_no_session_test.go
+++ b/integrations/create_no_session_test.go
@@ -14,6 +14,7 @@ import (
 	"testing"
 
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/routers/routes"
 
 	"gitea.com/macaron/session"
@@ -72,7 +73,7 @@ func TestSessionFileCreation(t *testing.T) {
 	assert.NoError(t, err)
 	defer func() {
 		if _, err := os.Stat(tmpDir); !os.IsNotExist(err) {
-			_ = os.RemoveAll(tmpDir)
+			_ = util.RemoveAll(tmpDir)
 		}
 	}()
 	config.ProviderConfig = tmpDir
diff --git a/integrations/git_helper_for_declarative_test.go b/integrations/git_helper_for_declarative_test.go
index 13b3e92c14..5823ce38dc 100644
--- a/integrations/git_helper_for_declarative_test.go
+++ b/integrations/git_helper_for_declarative_test.go
@@ -21,6 +21,7 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/ssh"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/unknwon/com"
@@ -30,7 +31,7 @@ func withKeyFile(t *testing.T, keyname string, callback func(string)) {
 
 	tmpDir, err := ioutil.TempDir("", "key-file")
 	assert.NoError(t, err)
-	defer os.RemoveAll(tmpDir)
+	defer util.RemoveAll(tmpDir)
 
 	err = os.Chmod(tmpDir, 0700)
 	assert.NoError(t, err)
@@ -119,7 +120,7 @@ func doGitCloneFail(u *url.URL) func(*testing.T) {
 	return func(t *testing.T) {
 		tmpDir, err := ioutil.TempDir("", "doGitCloneFail")
 		assert.NoError(t, err)
-		defer os.RemoveAll(tmpDir)
+		defer util.RemoveAll(tmpDir)
 		assert.Error(t, git.Clone(u.String(), tmpDir, git.CloneRepoOptions{}))
 		assert.False(t, com.IsExist(filepath.Join(tmpDir, "README.md")))
 	}
diff --git a/integrations/git_test.go b/integrations/git_test.go
index 7dd0bc056f..c3c1126829 100644
--- a/integrations/git_test.go
+++ b/integrations/git_test.go
@@ -10,7 +10,6 @@ import (
 	"math/rand"
 	"net/http"
 	"net/url"
-	"os"
 	"path"
 	"path/filepath"
 	"strconv"
@@ -21,6 +20,7 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -51,7 +51,7 @@ func testGit(t *testing.T, u *url.URL) {
 
 		dstPath, err := ioutil.TempDir("", httpContext.Reponame)
 		assert.NoError(t, err)
-		defer os.RemoveAll(dstPath)
+		defer util.RemoveAll(dstPath)
 
 		t.Run("CreateRepoInDifferentUser", doAPICreateRepository(forkedUserCtx, false))
 		t.Run("AddUserAsCollaborator", doAPIAddCollaborator(forkedUserCtx, httpContext.Username, models.AccessModeRead))
@@ -99,7 +99,7 @@ func testGit(t *testing.T, u *url.URL) {
 			//Setup clone folder
 			dstPath, err := ioutil.TempDir("", sshContext.Reponame)
 			assert.NoError(t, err)
-			defer os.RemoveAll(dstPath)
+			defer util.RemoveAll(dstPath)
 
 			t.Run("Clone", doGitClone(dstPath, sshURL))
 
@@ -124,7 +124,7 @@ func testGit(t *testing.T, u *url.URL) {
 func ensureAnonymousClone(t *testing.T, u *url.URL) {
 	dstLocalPath, err := ioutil.TempDir("", "repo1")
 	assert.NoError(t, err)
-	defer os.RemoveAll(dstLocalPath)
+	defer util.RemoveAll(dstLocalPath)
 	t.Run("CloneAnonymous", doGitClone(dstLocalPath, u))
 
 }
@@ -498,7 +498,7 @@ func doPushCreate(ctx APITestContext, u *url.URL) func(t *testing.T) {
 		// Create a temporary directory
 		tmpDir, err := ioutil.TempDir("", ctx.Reponame)
 		assert.NoError(t, err)
-		defer os.RemoveAll(tmpDir)
+		defer util.RemoveAll(tmpDir)
 
 		// Now create local repository to push as our test and set its origin
 		t.Run("InitTestRepository", doGitInitTestRepository(tmpDir))
diff --git a/integrations/gpg_git_test.go b/integrations/gpg_git_test.go
index e01f0f31c1..8c78faf7d2 100644
--- a/integrations/gpg_git_test.go
+++ b/integrations/gpg_git_test.go
@@ -16,6 +16,7 @@ import (
 	"code.gitea.io/gitea/modules/process"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 	"github.com/stretchr/testify/assert"
 	"golang.org/x/crypto/openpgp"
 	"golang.org/x/crypto/openpgp/armor"
@@ -28,7 +29,7 @@ func TestGPGGit(t *testing.T) {
 	// OK Set a new GPG home
 	tmpDir, err := ioutil.TempDir("", "temp-gpg")
 	assert.NoError(t, err)
-	defer os.RemoveAll(tmpDir)
+	defer util.RemoveAll(tmpDir)
 
 	err = os.Chmod(tmpDir, 0700)
 	assert.NoError(t, err)
diff --git a/integrations/integration_test.go b/integrations/integration_test.go
index e99264beb8..36e0682099 100644
--- a/integrations/integration_test.go
+++ b/integrations/integration_test.go
@@ -29,6 +29,7 @@ import (
 	"code.gitea.io/gitea/modules/graceful"
 	"code.gitea.io/gitea/modules/queue"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/routers"
 	"code.gitea.io/gitea/routers/routes"
 
@@ -98,11 +99,11 @@ func TestMain(m *testing.M) {
 
 	writerCloser.t = nil
 
-	if err = os.RemoveAll(setting.Indexer.IssuePath); err != nil {
-		fmt.Printf("os.RemoveAll: %v\n", err)
+	if err = util.RemoveAll(setting.Indexer.IssuePath); err != nil {
+		fmt.Printf("util.RemoveAll: %v\n", err)
 		os.Exit(1)
 	}
-	if err = os.RemoveAll(setting.Indexer.RepoPath); err != nil {
+	if err = util.RemoveAll(setting.Indexer.RepoPath); err != nil {
 		fmt.Printf("Unable to remove repo indexer: %v\n", err)
 		os.Exit(1)
 	}
@@ -138,7 +139,7 @@ func initIntegrationTest() {
 
 	setting.SetCustomPathAndConf("", "", "")
 	setting.NewContext()
-	os.RemoveAll(models.LocalCopyPath())
+	util.RemoveAll(models.LocalCopyPath())
 	setting.CheckLFSVersion()
 	setting.InitDBConfig()
 
@@ -230,7 +231,7 @@ func prepareTestEnv(t testing.TB, skip ...int) func() {
 	}
 	deferFn := PrintCurrentTest(t, ourSkip)
 	assert.NoError(t, models.LoadFixtures())
-	assert.NoError(t, os.RemoveAll(setting.RepoRootPath))
+	assert.NoError(t, util.RemoveAll(setting.RepoRootPath))
 
 	assert.NoError(t, com.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"),
 		setting.RepoRootPath))
@@ -473,7 +474,7 @@ func GetCSRF(t testing.TB, session *TestSession, urlStr string) string {
 func resetFixtures(t *testing.T) {
 	assert.NoError(t, queue.GetManager().FlushAll(context.Background(), -1))
 	assert.NoError(t, models.LoadFixtures())
-	assert.NoError(t, os.RemoveAll(setting.RepoRootPath))
+	assert.NoError(t, util.RemoveAll(setting.RepoRootPath))
 	assert.NoError(t, com.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"),
 		setting.RepoRootPath))
 }
diff --git a/integrations/migration-test/migration_test.go b/integrations/migration-test/migration_test.go
index 4ee045db38..976a59a579 100644
--- a/integrations/migration-test/migration_test.go
+++ b/integrations/migration-test/migration_test.go
@@ -24,6 +24,7 @@ import (
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/charset"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/unknwon/com"
@@ -58,7 +59,7 @@ func initMigrationTest(t *testing.T) func() {
 	setting.NewContext()
 
 	assert.True(t, len(setting.RepoRootPath) != 0)
-	assert.NoError(t, os.RemoveAll(setting.RepoRootPath))
+	assert.NoError(t, util.RemoveAll(setting.RepoRootPath))
 	assert.NoError(t, com.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath))
 
 	setting.CheckLFSVersion()
@@ -129,7 +130,7 @@ func restoreOldDB(t *testing.T, version string) bool {
 
 	switch {
 	case setting.Database.UseSQLite3:
-		os.Remove(setting.Database.Path)
+		util.Remove(setting.Database.Path)
 		err := os.MkdirAll(path.Dir(setting.Database.Path), os.ModePerm)
 		assert.NoError(t, err)
 
diff --git a/integrations/ssh_key_test.go b/integrations/ssh_key_test.go
index d445c7f9e5..91774f6335 100644
--- a/integrations/ssh_key_test.go
+++ b/integrations/ssh_key_test.go
@@ -9,13 +9,13 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/url"
-	"os"
 	"path/filepath"
 	"testing"
 	"time"
 
 	"code.gitea.io/gitea/modules/git"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -63,7 +63,7 @@ func testPushDeployKeyOnEmptyRepo(t *testing.T, u *url.URL) {
 		// Setup the testing repository
 		dstPath, err := ioutil.TempDir("", "repo-tmp-deploy-key-empty-repo-1")
 		assert.NoError(t, err)
-		defer os.RemoveAll(dstPath)
+		defer util.RemoveAll(dstPath)
 
 		t.Run("InitTestRepository", doGitInitTestRepository(dstPath))
 
@@ -109,7 +109,7 @@ func testKeyOnlyOneType(t *testing.T, u *url.URL) {
 		t.Run("KeyCanOnlyBeUser", func(t *testing.T) {
 			dstPath, err := ioutil.TempDir("", ctx.Reponame)
 			assert.NoError(t, err)
-			defer os.RemoveAll(dstPath)
+			defer util.RemoveAll(dstPath)
 
 			sshURL := createSSHUrl(ctx.GitPath(), u)
 
@@ -135,7 +135,7 @@ func testKeyOnlyOneType(t *testing.T, u *url.URL) {
 		t.Run("KeyCanBeAnyDeployButNotUserAswell", func(t *testing.T) {
 			dstPath, err := ioutil.TempDir("", ctx.Reponame)
 			assert.NoError(t, err)
-			defer os.RemoveAll(dstPath)
+			defer util.RemoveAll(dstPath)
 
 			sshURL := createSSHUrl(ctx.GitPath(), u)
 
@@ -153,7 +153,7 @@ func testKeyOnlyOneType(t *testing.T, u *url.URL) {
 			otherSSHURL := createSSHUrl(otherCtx.GitPath(), u)
 			dstOtherPath, err := ioutil.TempDir("", otherCtx.Reponame)
 			assert.NoError(t, err)
-			defer os.RemoveAll(dstOtherPath)
+			defer util.RemoveAll(dstOtherPath)
 
 			t.Run("AddWriterDeployKeyToOther", doAPICreateDeployKey(otherCtx, keyname, keyFile, false))
 
@@ -170,7 +170,7 @@ func testKeyOnlyOneType(t *testing.T, u *url.URL) {
 			otherSSHURL := createSSHUrl(otherCtx.GitPath(), u)
 			dstOtherPath, err := ioutil.TempDir("", otherCtx.Reponame)
 			assert.NoError(t, err)
-			defer os.RemoveAll(dstOtherPath)
+			defer util.RemoveAll(dstOtherPath)
 
 			t.Run("DeleteRepository", doAPIDeleteRepository(ctx))
 
@@ -192,7 +192,7 @@ func testKeyOnlyOneType(t *testing.T, u *url.URL) {
 
 			dstPath, err := ioutil.TempDir("", ctx.Reponame)
 			assert.NoError(t, err)
-			defer os.RemoveAll(dstPath)
+			defer util.RemoveAll(dstPath)
 
 			sshURL := createSSHUrl(ctx.GitPath(), u)