Re-attempt to delete temporary upload if the file is locked by anothe…

…r 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>

build/generate-gitignores.go

@@ -15,6 +15,8 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+
+	"code.gitea.io/gitea/modules/util"
 )
 
 func main() {
@@ -33,7 +35,7 @@ func main() {
 		log.Fatalf("Failed to create temp file. %s", err)
 	}
 
-	defer os.Remove(file.Name())
+	defer util.Remove(file.Name())
 
 	resp, err := http.Get(url)
 

build/generate-licenses.go

@@ -15,6 +15,8 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+
+	"code.gitea.io/gitea/modules/util"
 )
 
 func main() {
@@ -33,7 +35,7 @@ func main() {
 		log.Fatalf("Failed to create temp file. %s", err)
 	}
 
-	defer os.Remove(file.Name())
+	defer util.Remove(file.Name())
 
 	resp, err := http.Get(url)
 

cmd/doctor.go

@@ -24,6 +24,7 @@ import (
 	"code.gitea.io/gitea/modules/options"
 	"code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 	"xorm.io/builder"
 
 	"github.com/urfave/cli"
@@ -312,7 +313,7 @@ func runDoctorWritableDir(path string) error {
 	if err != nil {
 		return err
 	}
-	if err := os.Remove(tmpFile.Name()); err != nil {
+	if err := util.Remove(tmpFile.Name()); err != nil {
 		fmt.Printf("Warning: can't remove temporary file: '%s'\n", tmpFile.Name())
 	}
 	tmpFile.Close()

cmd/dump.go

@@ -18,6 +18,7 @@ import (
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"gitea.com/macaron/session"
 	archiver "github.com/mholt/archiver/v3"
@@ -243,7 +244,11 @@ func runDump(ctx *cli.Context) error {
 	if err != nil {
 		fatal("Failed to create tmp file: %v", err)
 	}
-	defer os.Remove(dbDump.Name())
+	defer func() {
+		if err := util.Remove(dbDump.Name()); err != nil {
+			log.Warn("Unable to remove temporary file: %s: Error: %v", dbDump.Name(), err)
+		}
+	}()
 
 	targetDBType := ctx.String("database")
 	if len(targetDBType) > 0 && targetDBType != setting.Database.Type {
@@ -313,7 +318,7 @@ func runDump(ctx *cli.Context) error {
 
 	if fileName != "-" {
 		if err = w.Close(); err != nil {
-			_ = os.Remove(fileName)
+			_ = util.Remove(fileName)
 			fatal("Failed to save %s: %v", fileName, err)
 		}
 

contrib/pr/checkout.go

@@ -29,6 +29,7 @@ import (
 	"code.gitea.io/gitea/modules/markup"
 	"code.gitea.io/gitea/modules/markup/external"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/routers"
 	"code.gitea.io/gitea/routers/routes"
 
@@ -108,8 +109,8 @@ func runPR() {
 		os.Exit(1)
 	}
 	models.LoadFixtures()
-	os.RemoveAll(setting.RepoRootPath)
-	os.RemoveAll(models.LocalCopyPath())
+	util.RemoveAll(setting.RepoRootPath)
+	util.RemoveAll(models.LocalCopyPath())
 	com.CopyDir(path.Join(curDir, "integrations/gitea-repositories-meta"), setting.RepoRootPath)
 
 	log.Printf("[PR] Setting up router\n")
@@ -141,20 +142,20 @@ func runPR() {
 
 	log.Printf("[PR] Cleaning up ...\n")
 	/*
-		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)
 		}
 	*/
-	if err = os.RemoveAll(setting.RepoRootPath); err != nil {
-		log.Fatalf("os.RemoveAll: %v\n", err)
+	if err = util.RemoveAll(setting.RepoRootPath); err != nil {
+		log.Fatalf("util.RemoveAll: %v\n", err)
 	}
-	if err = os.RemoveAll(setting.AppDataPath); err != nil {
-		log.Fatalf("os.RemoveAll: %v\n", err)
+	if err = util.RemoveAll(setting.AppDataPath); err != nil {
+		log.Fatalf("util.RemoveAll: %v\n", err)
 	}
 }
 

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,

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

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")))
 	}

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))

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)

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))
 }

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)