From e992b0a7cce09c2650898b294f206fc9e3097bc0 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 23 Jul 2026 22:31:29 +0800 Subject: [PATCH] refactor: retry file remove/rename when a file is busy and clean up os detection (#38588) Rename functions "util.Remove" (remove.go) to "util.RemoveWithRetry" (file_retry.go) and add comments to clarify their behaviors, also add tests. Refactor callers: when no concurrent access (cmd cli, migration, app init, test), use "os.Xxx" directly. More details are in `modules/util/file_retry.go` By the way, clean up OS (windows) detection, make FileURLToPath test always run --- cmd/dump.go | 2 +- cmd/embedded.go | 3 +- modelmigration/v1_10/v96.go | 4 +- modelmigration/v1_11/v112.go | 4 +- modelmigration/v1_11/v115.go | 5 +- models/repo/upload.go | 2 +- models/user/user.go | 1 + modules/git/attribute/main_test.go | 3 +- modules/git/catfile_batch_reader.go | 7 ++ modules/git/hook.go | 11 +- modules/git/hooks.go | 4 +- modules/git/localfs.go | 6 +- modules/graceful/net_unix.go | 10 +- modules/indexer/internal/bleve/util.go | 5 +- modules/setting/setting.go | 5 +- modules/storage/local.go | 8 +- modules/tempdir/tempdir.go | 4 +- modules/util/file_retry.go | 60 ++++++++++ modules/util/file_retry_test.go | 44 ++++++++ modules/util/path.go | 46 ++++---- modules/util/path_test.go | 42 +++---- modules/util/remove.go | 104 ------------------ modules/util/rotatingfilewriter/writer.go | 10 +- modules/util/runtime.go | 2 + services/asymkey/ssh_key_authorized_keys.go | 8 +- .../asymkey/ssh_key_authorized_principals.go | 8 +- services/org/org.go | 2 +- services/pull/patch.go | 6 +- services/repository/generate.go | 4 +- services/repository/repository.go | 20 ++-- services/user/user.go | 6 +- tests/integration/integration_test.go | 4 +- 32 files changed, 225 insertions(+), 225 deletions(-) create mode 100644 modules/util/file_retry.go create mode 100644 modules/util/file_retry_test.go delete mode 100644 modules/util/remove.go diff --git a/cmd/dump.go b/cmd/dump.go index 719e62d3af..037e788b28 100644 --- a/cmd/dump.go +++ b/cmd/dump.go @@ -198,7 +198,7 @@ func runDump(ctx context.Context, cmd *cli.Command) error { } defer func() { _ = dbDump.Close() - if err := util.Remove(dbDump.Name()); err != nil { + if err := os.Remove(dbDump.Name()); err != nil { log.Warn("Unable to remove temporary file: %s: Error: %v", dbDump.Name(), err) } }() diff --git a/cmd/embedded.go b/cmd/embedded.go index add8dda12b..2107071df6 100644 --- a/cmd/embedded.go +++ b/cmd/embedded.go @@ -18,7 +18,6 @@ import ( "gitea.dev/modules/public" "gitea.dev/modules/setting" "gitea.dev/modules/templates" - "gitea.dev/modules/util" "github.com/urfave/cli/v3" ) @@ -255,7 +254,7 @@ func extractAsset(d string, a assetFile, 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 := util.Rename(dest, dest+".bak"); err != nil { + if err := os.Rename(dest, dest+".bak"); err != nil { return fmt.Errorf("error creating backup for %s: %w", dest, err) } // Attempt to respect file permissions mask (even if user:group will be set anew) diff --git a/modelmigration/v1_10/v96.go b/modelmigration/v1_10/v96.go index 512c841f4b..0f443112b1 100644 --- a/modelmigration/v1_10/v96.go +++ b/modelmigration/v1_10/v96.go @@ -4,11 +4,11 @@ package v1_10 import ( + "os" "path/filepath" "gitea.dev/modelmigration/base" "gitea.dev/modules/setting" - "gitea.dev/modules/util" ) func DeleteOrphanedAttachments(x base.EngineMigration) error { @@ -52,7 +52,7 @@ func DeleteOrphanedAttachments(x base.EngineMigration) error { for _, attachment := range attachments { uuid := attachment.UUID - if err := util.RemoveAll(filepath.Join(setting.Attachment.Storage.Path, uuid[0:1], uuid[1:2], uuid)); err != nil { + if err := os.RemoveAll(filepath.Join(setting.Attachment.Storage.Path, uuid[0:1], uuid[1:2], uuid)); err != nil { return err } } diff --git a/modelmigration/v1_11/v112.go b/modelmigration/v1_11/v112.go index 14906d51cc..8250fc7f9f 100644 --- a/modelmigration/v1_11/v112.go +++ b/modelmigration/v1_11/v112.go @@ -4,12 +4,12 @@ package v1_11 import ( + "os" "path/filepath" "gitea.dev/modelmigration/base" "gitea.dev/modules/log" "gitea.dev/modules/setting" - "gitea.dev/modules/util" "xorm.io/builder" ) @@ -30,7 +30,7 @@ func RemoveAttachmentMissedRepo(x base.EngineMigration) error { for i := 0; i < len(attachments); i++ { uuid := attachments[i].UUID - if err = util.RemoveAll(filepath.Join(setting.Attachment.Storage.Path, uuid[0:1], uuid[1:2], uuid)); err != nil { + if err = os.RemoveAll(filepath.Join(setting.Attachment.Storage.Path, uuid[0:1], uuid[1:2], uuid)); err != nil { log.Warn("Unable to remove attachment file by UUID %s: %v", uuid, err) } } diff --git a/modelmigration/v1_11/v115.go b/modelmigration/v1_11/v115.go index ae777d2c37..1f086e79a7 100644 --- a/modelmigration/v1_11/v115.go +++ b/modelmigration/v1_11/v115.go @@ -16,7 +16,6 @@ import ( "gitea.dev/modules/container" "gitea.dev/modules/log" "gitea.dev/modules/setting" - "gitea.dev/modules/util" ) func RenameExistingUserAvatarName(x base.EngineMigration) error { @@ -110,8 +109,8 @@ func RenameExistingUserAvatarName(x base.EngineMigration) error { log.Info("Deleting %d old avatars ...", deleteCount) i := 0 for file := range deleteList { - if err := util.Remove(file); err != nil { - log.Warn("util.Remove: %v", err) + if err := os.Remove(file); err != nil { + log.Warn("Failed to remove avatar %s: %v", file, err) } i++ select { diff --git a/models/repo/upload.go b/models/repo/upload.go index d850d34bf1..6bfcd022ae 100644 --- a/models/repo/upload.go +++ b/models/repo/upload.go @@ -127,7 +127,7 @@ func DeleteUploads(ctx context.Context, uploads ...*Upload) (err error) { for _, upload := range uploads { localPath := upload.LocalPath() - if err := util.Remove(localPath); err != nil { + if err := util.RemoveWithRetry(localPath); err != nil { // just continue, don't fail the whole operation if a file is missing (removed by others) log.Error("unable to remove upload file %s: %v", localPath, err) } diff --git a/models/user/user.go b/models/user/user.go index 6d3d85c309..a34ca9ec1f 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -994,6 +994,7 @@ func GetInactiveUsers(ctx context.Context, olderThan time.Duration) ([]*User, er } // UserPath returns the path absolute path of user repositories. +// FIXME: it should be in "git/gitrepo" package func UserPath(userName string) string { //revive:disable-line:exported return filepath.Join(setting.RepoRootPath, filepath.Clean(strings.ToLower(userName))) } diff --git a/modules/git/attribute/main_test.go b/modules/git/attribute/main_test.go index 174c27c6f1..dfa39c237e 100644 --- a/modules/git/attribute/main_test.go +++ b/modules/git/attribute/main_test.go @@ -10,7 +10,6 @@ import ( "gitea.dev/modules/git" "gitea.dev/modules/setting" - "gitea.dev/modules/util" ) func testRun(m *testing.M) error { @@ -18,7 +17,7 @@ func testRun(m *testing.M) error { if err != nil { return fmt.Errorf("unable to create temp dir: %w", err) } - defer util.RemoveAll(gitHomePath) + defer os.RemoveAll(gitHomePath) setting.Git.HomePath = gitHomePath if err = git.InitFull(); err != nil { diff --git a/modules/git/catfile_batch_reader.go b/modules/git/catfile_batch_reader.go index fca670c1f2..3ba9511856 100644 --- a/modules/git/catfile_batch_reader.go +++ b/modules/git/catfile_batch_reader.go @@ -25,12 +25,16 @@ type catFileBatchCommunicator struct { reqWriter io.Writer respReader *bufio.Reader debugGitCmd *gitcmd.Command + closed chan struct{} } func (b *catFileBatchCommunicator) Close(err ...error) { if fn := b.closeFunc.Swap(nil); fn != nil { (*fn)(util.OptionalArg(err)) } + // make sure the git process has fully exited before we return from Close() + // otherwise, the opened files will block the directory renaming (rename a repo) on Windows + <-b.closed } // newCatFileBatch opens git cat-file --batch/--batch-check/--batch-command command and prepares the stdin/stdout pipes for communication. @@ -41,6 +45,7 @@ func newCatFileBatch(ctx context.Context, repo RepositoryFacade, cmdCatFile *git debugGitCmd: cmdCatFile, reqWriter: stdinWriter, respReader: bufio.NewReaderSize(stdoutReader, 32*1024), // use a buffered reader for rich operations + closed: make(chan struct{}), } ret.closeFunc.Store(new(func(err error) { ctxCancel(err) @@ -52,6 +57,7 @@ func newCatFileBatch(ctx context.Context, repo RepositoryFacade, cmdCatFile *git log.Error("Unable to start git command %v: %v", cmdCatFile.LogString(), err) // ideally here it should return the error, but it would require refactoring all callers // so just return a dummy communicator that does nothing, almost the same behavior as before, not bad + close(ret.closed) ret.Close(err) return ret } @@ -61,6 +67,7 @@ func newCatFileBatch(ctx context.Context, repo RepositoryFacade, cmdCatFile *git if err != nil && !errors.Is(err, context.Canceled) { log.Error("cat-file --batch command failed in repo %s, error: %v", repo.LogString(), err) } + close(ret.closed) ret.Close(err) }() diff --git a/modules/git/hook.go b/modules/git/hook.go index f94e6a5295..6682b9f9b3 100644 --- a/modules/git/hook.go +++ b/modules/git/hook.go @@ -72,16 +72,11 @@ func (h *Hook) Name() string { // Update updates hook settings. func (h *Hook) Update() error { if len(strings.TrimSpace(h.Content)) == 0 { - exist, err := util.IsExist(h.path) - if err != nil { + // empty content means to remove the file + err := util.RemoveWithRetry(h.path) + if err != nil && !os.IsNotExist(err) { return err } - if exist { - err := util.Remove(h.path) - if err != nil { - return err - } - } h.IsActive = false return nil } diff --git a/modules/git/hooks.go b/modules/git/hooks.go index d852087147..7fe1153777 100644 --- a/modules/git/hooks.go +++ b/modules/git/hooks.go @@ -124,7 +124,7 @@ func createDelegateHooks(hookDir string) (err error) { } // WARNING: This will override all old server-side hooks - if err = util.Remove(oldHookPath); err != nil && !os.IsNotExist(err) { + if err = util.RemoveWithRetry(oldHookPath); err != nil && !os.IsNotExist(err) { return fmt.Errorf("unable to pre-remove old hook file '%s' prior to rewriting: %w ", oldHookPath, err) } if err = os.WriteFile(oldHookPath, []byte(hookTpls[i]), 0o777); err != nil { @@ -135,7 +135,7 @@ func createDelegateHooks(hookDir string) (err error) { return fmt.Errorf("Unable to set %s executable. Error %w", oldHookPath, err) } - if err = util.Remove(newHookPath); err != nil && !os.IsNotExist(err) { + if err = util.RemoveWithRetry(newHookPath); err != nil && !os.IsNotExist(err) { return fmt.Errorf("unable to pre-remove new hook file '%s' prior to rewriting: %w", newHookPath, err) } if err = os.WriteFile(newHookPath, []byte(giteaHookTpls[i]), 0o777); err != nil { diff --git a/modules/git/localfs.go b/modules/git/localfs.go index 06a0f68a0f..8c8d8439b0 100644 --- a/modules/git/localfs.go +++ b/modules/git/localfs.go @@ -23,7 +23,7 @@ func IsRepositoryExist(ctx context.Context, repo RepositoryFacade) (bool, error) // DeleteRepository deletes the repository directory from the disk, it will return // nil if the repository does not exist. func DeleteRepository(ctx context.Context, repo RepositoryFacade) error { - return util.RemoveAll(gitrepo.RepoLocalPath(repo)) + return util.RemoveAllWithRetry(gitrepo.RepoLocalPath(repo)) } // RenameRepository renames a repository's name on disk @@ -33,7 +33,7 @@ func RenameRepository(ctx context.Context, repo, newRepo RepositoryFacade) error return fmt.Errorf("Failed to create dir %s: %w", filepath.Dir(dstDir), err) } - if err := util.Rename(gitrepo.RepoLocalPath(repo), dstDir); err != nil { + if err := util.RenameWithRetry(gitrepo.RepoLocalPath(repo), dstDir); err != nil { return fmt.Errorf("rename repository directory: %w", err) } return nil @@ -59,7 +59,7 @@ func IsRepoDirExist(ctx context.Context, repo RepositoryFacade, relativeDirPath func RemoveRepoFileOrDir(ctx context.Context, repo RepositoryFacade, relativeFileOrDirPath string) error { absoluteFilePath := filepath.Join(gitrepo.RepoLocalPath(repo), relativeFileOrDirPath) - return util.Remove(absoluteFilePath) + return util.RemoveWithRetry(absoluteFilePath) } func CreateRepoFile(ctx context.Context, repo RepositoryFacade, relativeFilePath string) (io.WriteCloser, error) { diff --git a/modules/graceful/net_unix.go b/modules/graceful/net_unix.go index 98227cb402..048cb99378 100644 --- a/modules/graceful/net_unix.go +++ b/modules/graceful/net_unix.go @@ -18,7 +18,6 @@ import ( "gitea.dev/modules/log" "gitea.dev/modules/setting" - "gitea.dev/modules/util" ) const ( @@ -121,8 +120,7 @@ func getProvidedFDs() (savedErr error) { continue } - // If needed we can handle packetconns here. - savedErr = fmt.Errorf("Error getting provided socket fd %d: %w", i, err) + savedErr = fmt.Errorf("error getting provided socket fd %d: %w", i, err) return } }) @@ -228,8 +226,8 @@ func GetListenerUnix(network string, address *net.UnixAddr) (*net.UnixListener, } // make a fresh listener - if err := util.Remove(address.Name); err != nil && !os.IsNotExist(err) { - return nil, fmt.Errorf("Failed to remove unix socket %s: %w", address.Name, err) + if err := os.Remove(address.Name); err != nil && !os.IsNotExist(err) { + return nil, fmt.Errorf("failed to remove unix socket %s: %w", address.Name, err) } l, err := net.ListenUnix(network, address) @@ -239,7 +237,7 @@ func GetListenerUnix(network string, address *net.UnixAddr) (*net.UnixListener, fileMode := os.FileMode(setting.UnixSocketPermission) if err = os.Chmod(address.Name, fileMode); err != nil { - return nil, fmt.Errorf("Failed to set permission of unix socket to %s: %w", fileMode.String(), err) + return nil, fmt.Errorf("failed to set permission of unix socket to %s: %w", fileMode.String(), err) } activeListeners = append(activeListeners, l) diff --git a/modules/indexer/internal/bleve/util.go b/modules/indexer/internal/bleve/util.go index 0914d4c269..61fd6b8336 100644 --- a/modules/indexer/internal/bleve/util.go +++ b/modules/indexer/internal/bleve/util.go @@ -10,7 +10,6 @@ import ( "gitea.dev/modules/log" "gitea.dev/modules/setting" - "gitea.dev/modules/util" "github.com/blevesearch/bleve/v2" unicode_tokenizer "github.com/blevesearch/bleve/v2/analysis/tokenizer/unicode" @@ -40,14 +39,14 @@ func openIndexer(path string, latestVersion int) (bleve.Index, int, error) { if metadata.Version < latestVersion { // the indexer is using a previous version, so we should delete it and // re-populate - return nil, metadata.Version, util.RemoveAll(path) + return nil, metadata.Version, os.RemoveAll(path) } index, err := bleve.Open(path) if err != nil { if errors.Is(err, upsidedown.IncompatibleVersion) { log.Warn("Indexer was built with a previous version of bleve, deleting and rebuilding") - return nil, 0, util.RemoveAll(path) + return nil, 0, os.RemoveAll(path) } return nil, 0, err } diff --git a/modules/setting/setting.go b/modules/setting/setting.go index 1b19923d9b..483b99622c 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -17,7 +17,8 @@ import ( "gitea.dev/modules/util" ) -// settings +const IsWindows = runtime.GOOS == "windows" + var ( // AppVer is the version of the current build of Gitea. It is set in main.go from main.Version. AppVer string @@ -27,7 +28,6 @@ var ( AppStartTime time.Time CfgProvider ConfigProvider - IsWindows bool // IsInTesting indicates whether the testing is running (unit test or integration test). It can be used for: // * Skip nonsense error logs during testing caused by unreliable code (TODO: this is only a temporary solution, we should make the test code more reliable) @@ -37,7 +37,6 @@ var ( ) func init() { - IsWindows = runtime.GOOS == "windows" if AppVer == "" { AppVer = "dev" } diff --git a/modules/storage/local.go b/modules/storage/local.go index 82c1173ca4..1e4637a646 100644 --- a/modules/storage/local.go +++ b/modules/storage/local.go @@ -84,7 +84,7 @@ func (l *LocalStorage) Save(path string, r io.Reader, size int64) (int64, error) tmpRemoved := false defer func() { if !tmpRemoved { - _ = util.Remove(tmp.Name()) + _ = util.RemoveWithRetry(tmp.Name()) } }() @@ -97,7 +97,7 @@ func (l *LocalStorage) Save(path string, r io.Reader, size int64) (int64, error) return 0, err } - if err := util.Rename(tmp.Name(), p); err != nil { + if err := util.RenameWithRetry(tmp.Name(), p); err != nil { return 0, err } // Golang's tmp file (os.CreateTemp) always have 0o600 mode, so we need to change the file to follow the umask (as what Create/MkDir does) @@ -118,7 +118,7 @@ func (l *LocalStorage) Stat(path string) (os.FileInfo, error) { func (l *LocalStorage) deleteEmptyParentDirs(localFullPath string) { for parent := filepath.Dir(localFullPath); len(parent) > len(l.dir); parent = filepath.Dir(parent) { - if err := os.Remove(parent); err != nil { + if err := util.RemoveWithRetry(parent); err != nil && !os.IsNotExist(err) { // since the target file has been deleted, parent dir error is not related to the file deletion itself. break } @@ -128,7 +128,7 @@ func (l *LocalStorage) deleteEmptyParentDirs(localFullPath string) { // Delete deletes the file in storage and removes the empty parent directories (if possible) func (l *LocalStorage) Delete(path string) error { localFullPath := l.buildLocalPath(path) - err := util.Remove(localFullPath) + err := util.RemoveWithRetry(localFullPath) l.deleteEmptyParentDirs(localFullPath) return err } diff --git a/modules/tempdir/tempdir.go b/modules/tempdir/tempdir.go index b33fb32be9..aaeae2f5b6 100644 --- a/modules/tempdir/tempdir.go +++ b/modules/tempdir/tempdir.go @@ -56,7 +56,7 @@ func (td *TempDir) MkdirTempRandom(elems ...string) (string, func(), error) { return "", nil, err } return dir, func() { - if err := util.RemoveAll(dir); err != nil { + if err := util.RemoveAllWithRetry(dir); err != nil { log.Error("Failed to remove temp directory %s: %v", dir, err) } }, nil @@ -75,7 +75,7 @@ func (td *TempDir) CreateTempFileRandom(elems ...string) (*os.File, func(), erro filename := f.Name() return f, func() { _ = f.Close() - if err := util.Remove(filename); err != nil { + if err := util.RemoveWithRetry(filename); err != nil { log.Error("Unable to remove temporary file: %s: Error: %v", filename, err) } }, err diff --git a/modules/util/file_retry.go b/modules/util/file_retry.go new file mode 100644 index 0000000000..6328896d5b --- /dev/null +++ b/modules/util/file_retry.go @@ -0,0 +1,60 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package util + +import ( + "errors" + "os" + "syscall" + "time" +) + +// On Windows, when a file or directory is in use (opened), the file or directory is not able to be removed or renamed. +// When renaming or removing a local git repository directory: +// * the "cat-batch" git process might be running in a goroutine +// * there can be a data-race between the "cat-batch" git process cancel+exit and the repo rename +// So we need to retry the rename/remove operation for a few times when the "cat-batch" git process is exiting. +// ref: https://github.com/go-gitea/gitea/issues/16427, https://github.com/go-gitea/gitea/issues/16475, https://github.com/go-gitea/gitea/pull/16479 +// Also some similar problems when removing a file, e.g.: https://github.com/go-gitea/gitea/issues/12339 +// +// Usually, if no concurrent access to a file, use "os.Xxx", otherwise, use "util.XxxWithRetry" + +func retryWhenFileBusyInternal(count int, delay time.Duration, f func() error) (err error) { + const errWindowsSharingViolationError = syscall.Errno(32) + for range count { + err = f() + if err == nil { + break + } + isErrBusy := errors.Is(err, syscall.EBUSY) || errors.Is(err, syscall.ENOTEMPTY) || errors.Is(err, syscall.EPERM) || errors.Is(err, syscall.EMFILE) || errors.Is(err, syscall.ENFILE) + isErrBusy = isErrBusy || (isOSWindows && errors.Is(err, errWindowsSharingViolationError)) + if !isErrBusy { + break + } + time.Sleep(delay) + } + return err +} + +func retryWhenFileBusy(f func() error) (err error) { + return retryWhenFileBusyInternal(5, 100*time.Millisecond, f) +} + +func RemoveWithRetry(path string) error { + return retryWhenFileBusy(func() error { + return os.Remove(path) + }) +} + +func RemoveAllWithRetry(path string) error { + return retryWhenFileBusy(func() error { + return os.RemoveAll(path) + }) +} + +func RenameWithRetry(oldpath, newpath string) error { + return retryWhenFileBusy(func() error { + return os.Rename(oldpath, newpath) + }) +} diff --git a/modules/util/file_retry_test.go b/modules/util/file_retry_test.go new file mode 100644 index 0000000000..f1b7ee4659 --- /dev/null +++ b/modules/util/file_retry_test.go @@ -0,0 +1,44 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package util + +import ( + "io" + "os" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestRetryWhenFileBusy(t *testing.T) { + var callCount int + testErrPath := &os.PathError{Op: "test", Path: "test", Err: syscall.EBUSY} + fn := func() error { + if callCount == 2 { + return nil + } + callCount++ + return testErrPath + } + + callCount = 0 + err := retryWhenFileBusyInternal(1, time.Millisecond, fn) + assert.Equal(t, testErrPath, err) + assert.Equal(t, 1, callCount) + + callCount = 0 + err = retryWhenFileBusyInternal(10, time.Millisecond, fn) + assert.NoError(t, err) + assert.Equal(t, 2, callCount) + + callCount = 0 + err = retryWhenFileBusyInternal(10, time.Millisecond, func() error { + callCount++ + return io.ErrUnexpectedEOF + }) + assert.Equal(t, io.ErrUnexpectedEOF, err) + assert.Equal(t, 1, callCount) +} diff --git a/modules/util/path.go b/modules/util/path.go index baed03b443..d251d5353e 100644 --- a/modules/util/path.go +++ b/modules/util/path.go @@ -10,8 +10,6 @@ import ( "os" "path" "path/filepath" - "regexp" - "runtime" "strings" ) @@ -77,7 +75,7 @@ const filepathSeparator = string(os.PathSeparator) func FilePathJoinAbs(base string, sub ...string) string { // POSIX filesystem can have `\` in file names. Windows: `\` and `/` are both used for path separators // to keep the behavior consistent, we do not allow `\` in file names, replace all `\` with `/` - if !isOSWindows() { + if !isOSWindows { base = strings.ReplaceAll(base, "\\", filepathSeparator) } if !filepath.IsAbs(base) { @@ -94,7 +92,7 @@ func FilePathJoinAbs(base string, sub ...string) string { if s == "" { continue } - if isOSWindows() { + if isOSWindows { elems = append(elems, filepath.Clean(filepathSeparator+s)) } else { elems = append(elems, filepath.Clean(filepathSeparator+strings.ReplaceAll(s, "\\", filepathSeparator))) @@ -245,30 +243,30 @@ func ListDirRecursively(rootDir string, opts *ListDirOptions) (res []string, err return res, nil } -func isOSWindows() bool { - return runtime.GOOS == "windows" -} +func fileURLToPathInternal(u *url.URL, isWindows bool) (string, error) { + if u.Scheme != "file" { + return "", errors.New("URL scheme is not 'file': " + u.String()) + } + if !isWindows { + return u.Path, nil + } -var driveLetterRegexp = regexp.MustCompile("/[A-Za-z]:/") + // If it is a Windows absolute path with drive letter "/C:/dir", strip off the leading slash. + if !strings.HasPrefix(u.Path, "/") || len(u.Path) < 3 { + return u.Path, nil + } + winPath := u.Path[1:] + first := winPath[0] + if ('a' <= first && first <= 'z' || 'A' <= first && first <= 'Z') && winPath[1] == ':' { + return winPath, nil + } + return u.Path, nil +} // FileURLToPath extracts the path information from a file://... url. // It returns an error only if the URL is not a file URL. func FileURLToPath(u *url.URL) (string, error) { - if u.Scheme != "file" { - return "", errors.New("URL scheme is not 'file': " + u.String()) - } - - path := u.Path - - if !isOSWindows() { - return path, nil - } - - // If it looks like there's a Windows drive letter at the beginning, strip off the leading slash. - if driveLetterRegexp.MatchString(path) { - return path[1:], nil - } - return path, nil + return fileURLToPathInternal(u, isOSWindows) } // HomeDir returns path of '~'(in Linux) on Windows, @@ -277,7 +275,7 @@ func HomeDir() (home string, err error) { // TODO: some users run Gitea with mismatched uid and "HOME=xxx" (they set HOME=xxx by environment manually) // TODO: when running gitea as a sub command inside git, the HOME directory is not the user's home directory // so at the moment we can not use `user.Current().HomeDir` - if isOSWindows() { + if isOSWindows { home = os.Getenv("USERPROFILE") if home == "" { home = os.Getenv("HOMEDRIVE") + os.Getenv("HOMEPATH") diff --git a/modules/util/path_test.go b/modules/util/path_test.go index 2469088b3a..1132d07cb7 100644 --- a/modules/util/path_test.go +++ b/modules/util/path_test.go @@ -7,7 +7,6 @@ import ( "net/url" "os" "path/filepath" - "runtime" "testing" "github.com/stretchr/testify/assert" @@ -18,43 +17,46 @@ func TestFileURLToPath(t *testing.T) { cases := []struct { url string expected string - haserror bool + invalid bool windows bool }{ - // case 0 { - url: "", - haserror: true, + url: "", + invalid: true, }, - // case 1 { - url: "http://test.io", - haserror: true, + url: "http://test.io", + invalid: true, }, - // case 2 { url: "file:///path", expected: "/path", }, - // case 3 { url: "file:///C:/path", expected: "C:/path", windows: true, }, + { + url: "file:///z:/path", + expected: "z:/path", + windows: true, + }, + { + url: "file:///path", + expected: "/path", + windows: true, + }, } - for n, c := range cases { - if c.windows && runtime.GOOS != "windows" { - continue - } + for _, c := range cases { u, _ := url.Parse(c.url) - p, err := FileURLToPath(u) - if c.haserror { - assert.Error(t, err, "case %d: should return error", n) + p, err := fileURLToPathInternal(u, c.windows) + if c.invalid { + assert.Error(t, err, "case %s: should return error", c.url) } else { - assert.NoError(t, err, "case %d: should not return error", n) - assert.Equal(t, c.expected, p, "case %d: should be equal", n) + assert.NoError(t, err, "case %s: should not return error", c.url) + assert.Equal(t, c.expected, p, "case %s: should be equal", c.url) } } } @@ -180,7 +182,7 @@ func TestCleanPath(t *testing.T) { } // for POSIX only, but the result is similar on Windows, because the first element must be an absolute path - if isOSWindows() { + if isOSWindows { cases = []struct { elems []string expected string diff --git a/modules/util/remove.go b/modules/util/remove.go deleted file mode 100644 index 3db0b5a796..0000000000 --- a/modules/util/remove.go +++ /dev/null @@ -1,104 +0,0 @@ -// Copyright 2020 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package util - -import ( - "os" - "runtime" - "syscall" - "time" -) - -const windowsSharingViolationError syscall.Errno = 32 - -// Remove removes the named file or (empty) directory with at most 5 attempts. -func Remove(name string) error { - var err error - for range 5 { - err = os.Remove(name) - 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 unwrapped == windowsSharingViolationError && runtime.GOOS == "windows" { - // try again - <-time.After(100 * time.Millisecond) - continue - } - - if unwrapped == syscall.ENOENT { - // it's already gone - return nil - } - } - return err -} - -// RemoveAll removes the named file or (empty) directory with at most 5 attempts. -func RemoveAll(name string) error { - var err error - for range 5 { - err = os.RemoveAll(name) - 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 unwrapped == windowsSharingViolationError && runtime.GOOS == "windows" { - // try again - <-time.After(100 * time.Millisecond) - continue - } - - if unwrapped == syscall.ENOENT { - // it's already gone - return nil - } - } - return err -} - -// Rename renames (moves) oldpath to newpath with at most 5 attempts. -func Rename(oldpath, newpath string) error { - var err error - for i := range 5 { - err = os.Rename(oldpath, newpath) - if err == nil { - break - } - unwrapped := err.(*os.LinkError).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 unwrapped == windowsSharingViolationError && runtime.GOOS == "windows" { - // 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 -} diff --git a/modules/util/rotatingfilewriter/writer.go b/modules/util/rotatingfilewriter/writer.go index 28ba5bd70d..fc5c10881b 100644 --- a/modules/util/rotatingfilewriter/writer.go +++ b/modules/util/rotatingfilewriter/writer.go @@ -152,7 +152,7 @@ func (rfw *RotatingFileWriter) DoRotate() error { return err } - if err := util.Rename(fd.Name(), fname); err != nil { + if err := util.RenameWithRetry(fd.Name(), fname); err != nil { return err } @@ -203,12 +203,12 @@ func compressOldFile(fname string, compressionLevel int) error { if err != nil { _ = zw.Close() _ = fw.Close() - _ = util.Remove(fname + ".gz") + _ = util.RemoveWithRetry(fname + ".gz") return fmt.Errorf("compressOldFile: failed to write to gz file: %w", err) } _ = reader.Close() - err = util.Remove(fname) + err = util.RemoveWithRetry(fname) if err != nil { return fmt.Errorf("compressOldFile: failed to delete old file: %w", err) } @@ -235,7 +235,9 @@ func deleteOldFiles(dir, prefix string, removeBefore time.Time) { } if info.ModTime().Before(removeBefore) { if strings.HasPrefix(filepath.Base(path), prefix) { - return util.Remove(path) + if err = util.RemoveWithRetry(path); err != nil && !os.IsNotExist(err) { + errorf("deleteOldFiles: failed to delete old file %s: %v", path, err) + } } } return nil diff --git a/modules/util/runtime.go b/modules/util/runtime.go index 2acdbeeb63..30759f614f 100644 --- a/modules/util/runtime.go +++ b/modules/util/runtime.go @@ -5,6 +5,8 @@ package util import "runtime" +const isOSWindows = runtime.GOOS == "windows" + func CallerFuncName(optSkipParent ...int) string { pc := make([]uintptr, 1) skipParent := 0 diff --git a/services/asymkey/ssh_key_authorized_keys.go b/services/asymkey/ssh_key_authorized_keys.go index e0d72cc61d..79b21cbaf8 100644 --- a/services/asymkey/ssh_key_authorized_keys.go +++ b/services/asymkey/ssh_key_authorized_keys.go @@ -50,8 +50,8 @@ func rewriteAllPublicKeys(ctx context.Context) error { return err } defer func() { - t.Close() - if err := util.Remove(tmpPath); err != nil { + _ = t.Close() + if err := util.RemoveWithRetry(tmpPath); err != nil { log.Warn("Unable to remove temporary authorized keys file: %s: Error: %v", tmpPath, err) } }() @@ -74,6 +74,6 @@ func rewriteAllPublicKeys(ctx context.Context) error { return err } - t.Close() - return util.Rename(tmpPath, fPath) + _ = t.Close() + return util.RenameWithRetry(tmpPath, fPath) } diff --git a/services/asymkey/ssh_key_authorized_principals.go b/services/asymkey/ssh_key_authorized_principals.go index 6ecb693bda..9a5163b3be 100644 --- a/services/asymkey/ssh_key_authorized_principals.go +++ b/services/asymkey/ssh_key_authorized_principals.go @@ -61,8 +61,8 @@ func rewriteAllPrincipalKeys(ctx context.Context) error { return err } defer func() { - t.Close() - os.Remove(tmpPath) + _ = t.Close() + _ = util.RemoveWithRetry(tmpPath) }() if setting.SSH.AuthorizedPrincipalsBackup { @@ -83,8 +83,8 @@ func rewriteAllPrincipalKeys(ctx context.Context) error { return err } - t.Close() - return util.Rename(tmpPath, fPath) + _ = t.Close() + return util.RenameWithRetry(tmpPath, fPath) } func regeneratePrincipalKeys(ctx context.Context, t io.Writer) error { diff --git a/services/org/org.go b/services/org/org.go index 7ab8493aa4..e51331cbe7 100644 --- a/services/org/org.go +++ b/services/org/org.go @@ -89,7 +89,7 @@ func DeleteOrganization(ctx context.Context, org *org_model.Organization, purge // so just keep error logs of those operations. path := user_model.UserPath(org.Name) - if err := util.RemoveAll(path); err != nil { + if err := util.RemoveAllWithRetry(path); err != nil { return fmt.Errorf("failed to RemoveAll %s: %w", path, err) } diff --git a/services/pull/patch.go b/services/pull/patch.go index 2a099cf0c5..747aba56af 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -188,7 +188,7 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, t } root = strings.TrimSpace(root) defer func() { - _ = util.Remove(filepath.Join(tmpBasePath, root)) + _ = util.RemoveWithRetry(filepath.Join(tmpBasePath, root)) }() base, _, err := gitcmd.NewCommand("unpack-file").AddDynamicArguments(file.stage2.sha).WithRepo(tmpGitRepo).RunStdString(ctx) @@ -197,7 +197,7 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, t } base = strings.TrimSpace(filepath.Join(tmpBasePath, base)) defer func() { - _ = util.Remove(base) + _ = util.RemoveWithRetry(base) }() head, _, err := gitcmd.NewCommand("unpack-file").AddDynamicArguments(file.stage3.sha).WithRepo(tmpGitRepo).RunStdString(ctx) if err != nil { @@ -205,7 +205,7 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, t } head = strings.TrimSpace(head) defer func() { - _ = util.Remove(filepath.Join(tmpBasePath, head)) + _ = util.RemoveWithRetry(filepath.Join(tmpBasePath, head)) }() // now git merge-file annoyingly takes a different order to the merge-tree ... diff --git a/services/repository/generate.go b/services/repository/generate.go index eedcffe5bd..050afea999 100644 --- a/services/repository/generate.go +++ b/services/repository/generate.go @@ -232,7 +232,7 @@ func processGiteaTemplateFile(ctx context.Context, tmpDir string, templateRepo, if err != nil { return nil, err } - if err = util.RemoveAll(util.FilePathJoinAbs(tmpDir, ".git")); err != nil { + if err = util.RemoveAllWithRetry(util.FilePathJoinAbs(tmpDir, ".git")); err != nil { return nil, err } return skippedFiles, nil @@ -256,7 +256,7 @@ func generateRepoCommit(ctx context.Context, repo, templateRepo, generateRepo *r return fmt.Errorf("GetTemplateSubmoduleCommits: %w", err) } - if err = util.RemoveAll(filepath.Join(tmpDir, ".git")); err != nil { + if err = util.RemoveAllWithRetry(filepath.Join(tmpDir, ".git")); err != nil { return fmt.Errorf("remove git dir: %w", err) } diff --git a/services/repository/repository.go b/services/repository/repository.go index c7686021d0..7a0513c957 100644 --- a/services/repository/repository.go +++ b/services/repository/repository.go @@ -11,14 +11,14 @@ import ( activities_model "gitea.dev/models/activities" "gitea.dev/models/db" - "gitea.dev/models/git" + git_model "gitea.dev/models/git" issues_model "gitea.dev/models/issues" "gitea.dev/models/organization" access_model "gitea.dev/models/perm/access" repo_model "gitea.dev/models/repo" "gitea.dev/models/unit" user_model "gitea.dev/models/user" - git2 "gitea.dev/modules/git" + "gitea.dev/modules/git" "gitea.dev/modules/git/gitrepo" "gitea.dev/modules/graceful" issue_indexer "gitea.dev/modules/indexer/issues" @@ -33,9 +33,9 @@ import ( // WebSearchRepository represents a repository returned by web search type WebSearchRepository struct { - Repository *structs.Repository `json:"repository"` - LatestCommitStatus *git.CommitStatus `json:"latest_commit_status"` - LocaleLatestCommitStatus string `json:"locale_latest_commit_status"` + Repository *structs.Repository `json:"repository"` + LatestCommitStatus *git_model.CommitStatus `json:"latest_commit_status"` + LocaleLatestCommitStatus string `json:"locale_latest_commit_status"` } // WebSearchResults results of a successful web search @@ -218,7 +218,7 @@ func CheckDaemonExportOK(ctx context.Context, repo *repo_model.Repository) error // Create/Remove git-daemon-export-ok for git-daemon... daemonExportFile := `git-daemon-export-ok` - isExist, err := git2.IsRepoFileExist(ctx, repo, daemonExportFile) + isExist, err := git.IsRepoFileExist(ctx, repo, daemonExportFile) if err != nil { log.Error("Unable to check if %s exists. Error: %v", daemonExportFile, err) return err @@ -226,11 +226,11 @@ func CheckDaemonExportOK(ctx context.Context, repo *repo_model.Repository) error isPublic := !repo.IsPrivate && repo.Owner.Visibility == structs.VisibleTypePublic if !isPublic && isExist { - if err = git2.RemoveRepoFileOrDir(ctx, repo, daemonExportFile); err != nil { + if err = git.RemoveRepoFileOrDir(ctx, repo, daemonExportFile); err != nil { log.Error("Failed to remove %s: %v", daemonExportFile, err) } } else if isPublic && !isExist { - if f, err := git2.CreateRepoFile(ctx, repo, daemonExportFile); err != nil { + if f, err := git.CreateRepoFile(ctx, repo, daemonExportFile); err != nil { log.Error("Failed to create %s: %v", daemonExportFile, err) } else { f.Close() @@ -309,7 +309,7 @@ func updateRepository(ctx context.Context, repo *repo_model.Repository, visibili } func HasWiki(ctx context.Context, repo *repo_model.Repository) bool { - hasWiki, err := git2.IsRepositoryExist(ctx, repo.WikiStorageRepo()) + hasWiki, err := git.IsRepositoryExist(ctx, repo.WikiStorageRepo()) if err != nil { log.Error("gitrepo.IsRepositoryExist: %v", err) } @@ -333,7 +333,7 @@ func CheckCreateRepository(ctx context.Context, doer, owner *user_model.User, na return repo_model.ErrRepoAlreadyExist{Uname: owner.Name, Name: name} } repo := gitrepo.CodeRepoByName(owner.Name, name) - isExist, err := git2.IsRepositoryExist(ctx, repo) + isExist, err := git.IsRepositoryExist(ctx, repo) if err != nil { log.Error("Unable to check if repo %s/%s exists, error: %v", owner.Name, name, err) return err diff --git a/services/user/user.go b/services/user/user.go index 91551fe2be..43052819d6 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -98,7 +98,7 @@ func RenameUser(ctx context.Context, u *user_model.User, newUserName string, doe } // Do not fail if directory does not exist - if err = util.Rename(user_model.UserPath(oldUserName), user_model.UserPath(newUserName)); err != nil && !os.IsNotExist(err) { + if err = util.RenameWithRetry(user_model.UserPath(oldUserName), user_model.UserPath(newUserName)); err != nil && !os.IsNotExist(err) { u.Name = oldUserName u.LowerName = strings.ToLower(oldUserName) return fmt.Errorf("rename user directory: %w", err) @@ -107,7 +107,7 @@ func RenameUser(ctx context.Context, u *user_model.User, newUserName string, doe if err = committer.Commit(); err != nil { u.Name = oldUserName u.LowerName = strings.ToLower(oldUserName) - if err2 := util.Rename(user_model.UserPath(newUserName), user_model.UserPath(oldUserName)); err2 != nil && !os.IsNotExist(err2) { + if err2 := util.RenameWithRetry(user_model.UserPath(newUserName), user_model.UserPath(oldUserName)); err2 != nil && !os.IsNotExist(err2) { log.Error("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) } @@ -258,7 +258,7 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error { // Note: There are something just cannot be roll back, so just keep error logs of those operations. path := user_model.UserPath(u.Name) - if err := util.RemoveAll(path); err != nil { + if err := util.RemoveAllWithRetry(path); err != nil { err = fmt.Errorf("failed to RemoveAll %s: %w", path, err) _ = system_model.CreateNotice(ctx, system_model.NoticeTask, fmt.Sprintf("delete user '%s': %v", u.Name, err)) } diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index 6f0928a12e..1fcbdb8aad 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -66,10 +66,10 @@ func testMain(m *testing.M) int { // Instead, "No tests were found", last nonsense log is "According to the configuration, subsequent logs will not be printed to the console" exitCode := m.Run() - if err = util.RemoveAll(setting.Indexer.IssuePath); err != nil { + if err = os.RemoveAll(setting.Indexer.IssuePath); err != nil { log.Error("Failed to remove indexer path: %v", err) } - if err = util.RemoveAll(setting.Indexer.RepoPath); err != nil { + if err = os.RemoveAll(setting.Indexer.RepoPath); err != nil { log.Error("Failed to remove indexer path: %v", err) } return exitCode