Fix artifacts v4 backend upload problems (#36805)

modules/storage/local.go

@@ -5,6 +5,7 @@ package storage
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"net/url"
@@ -27,25 +28,32 @@ type LocalStorage struct {
 
 // NewLocalStorage returns a local files
 func NewLocalStorage(ctx context.Context, config *setting.Storage) (ObjectStorage, error) {
+	// prepare storage root path
 	if !filepath.IsAbs(config.Path) {
-		return nil, fmt.Errorf("LocalStorageConfig.Path should have been prepared by setting/storage.go and should be an absolute path, but not: %q", config.Path)
-	}
-	log.Info("Creating new Local Storage at %s", config.Path)
-	if err := os.MkdirAll(config.Path, os.ModePerm); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("LocalStorage config.Path should have been prepared by setting/storage.go and should be an absolute path, but not: %q", config.Path)
 	}
+	storageRoot := util.FilePathJoinAbs(config.Path)
 
-	if config.TemporaryPath == "" {
-		config.TemporaryPath = filepath.Join(config.Path, "tmp")
+	// prepare storage temporary path
+	storageTmp := config.TemporaryPath
+	if storageTmp == "" {
+		storageTmp = filepath.Join(storageRoot, "tmp")
+	}
+	if !filepath.IsAbs(storageTmp) {
+		return nil, fmt.Errorf("LocalStorage config.TemporaryPath should be an absolute path, but not: %q", config.TemporaryPath)
 	}
-	if !filepath.IsAbs(config.TemporaryPath) {
-		return nil, fmt.Errorf("LocalStorageConfig.TemporaryPath should be an absolute path, but not: %q", config.TemporaryPath)
+	storageTmp = util.FilePathJoinAbs(storageTmp)
+
+	// create the storage root if not exist
+	log.Info("Creating new Local Storage at %s", storageRoot)
+	if err := os.MkdirAll(storageRoot, os.ModePerm); err != nil {
+		return nil, err
 	}
 
 	return &LocalStorage{
 		ctx:    ctx,
-		dir:    config.Path,
-		tmpdir: config.TemporaryPath,
+		dir:    storageRoot,
+		tmpdir: storageTmp,
 	}, nil
 }
 
@@ -108,44 +116,60 @@ func (l *LocalStorage) Stat(path string) (os.FileInfo, error) {
 	return os.Stat(l.buildLocalPath(path))
 }
 
-// Delete delete a file
+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 {
+			// since the target file has been deleted, parent dir error is not related to the file deletion itself.
+			break
+		}
+	}
+}
+
+// Delete deletes the file in storage and removes the empty parent directories (if possible)
 func (l *LocalStorage) Delete(path string) error {
-	return util.Remove(l.buildLocalPath(path))
+	localFullPath := l.buildLocalPath(path)
+	err := util.Remove(localFullPath)
+	l.deleteEmptyParentDirs(localFullPath)
+	return err
 }
 
 // URL gets the redirect URL to a file
 func (l *LocalStorage) URL(path, name, _ string, reqParams url.Values) (*url.URL, error) {
 	return nil, ErrURLNotSupported
 }
 
+func (l *LocalStorage) normalizeWalkError(err error) error {
+	if errors.Is(err, os.ErrNotExist) {
+		// ignore it because the file may be deleted during the walk, and we don't care about it
+		return nil
+	}
+	return err
+}
+
 // IterateObjects iterates across the objects in the local storage
 func (l *LocalStorage) IterateObjects(dirName string, fn func(path string, obj Object) error) error {
 	dir := l.buildLocalPath(dirName)
-	return filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error {
-		if err != nil {
+	return filepath.WalkDir(dir, func(path string, d os.DirEntry, errWalk error) error {
+		if err := l.ctx.Err(); err != nil {
 			return err
 		}
-		select {
-		case <-l.ctx.Done():
-			return l.ctx.Err()
-		default:
+		if errWalk != nil {
+			return l.normalizeWalkError(errWalk)
 		}
-		if path == l.dir {
-			return nil
-		}
-		if d.IsDir() {
+		if path == l.dir || d.IsDir() {
 			return nil
 		}
+
 		relPath, err := filepath.Rel(l.dir, path)
 		if err != nil {
-			return err
+			return l.normalizeWalkError(err)
 		}
 		obj, err := os.Open(path)
 		if err != nil {
-			return err
+			return l.normalizeWalkError(err)
 		}
 		defer obj.Close()
-		return fn(relPath, obj)
+		return fn(filepath.ToSlash(relPath), obj)
 	})
 }
 

modules/storage/local_test.go

@@ -4,11 +4,14 @@
 package storage
 
 import (
+	"os"
+	"strings"
 	"testing"
 
 	"code.gitea.io/gitea/modules/setting"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestBuildLocalPath(t *testing.T) {
@@ -53,6 +56,49 @@ func TestBuildLocalPath(t *testing.T) {
 	}
 }
 
+func TestLocalStorageDelete(t *testing.T) {
+	rootDir := t.TempDir()
+	st, err := NewLocalStorage(t.Context(), &setting.Storage{Path: rootDir})
+	require.NoError(t, err)
+
+	assertExists := func(t *testing.T, path string, exists bool) {
+		_, err = os.Stat(rootDir + "/" + path)
+		if exists {
+			require.NoError(t, err)
+		} else {
+			require.ErrorIs(t, err, os.ErrNotExist)
+		}
+	}
+
+	_, err = st.Save("dir/sub1/1-a.txt", strings.NewReader(""), -1)
+	require.NoError(t, err)
+	_, err = st.Save("dir/sub1/1-b.txt", strings.NewReader(""), -1)
+	require.NoError(t, err)
+	_, err = st.Save("dir/sub2/2-a.txt", strings.NewReader(""), -1)
+	require.NoError(t, err)
+
+	assertExists(t, "dir/sub1/1-a.txt", true)
+	assertExists(t, "dir/sub1/1-b.txt", true)
+	assertExists(t, "dir/sub2/2-a.txt", true)
+
+	require.NoError(t, st.Delete("dir/sub1/1-a.txt"))
+	assertExists(t, "dir/sub1", true)
+	assertExists(t, "dir/sub1/1-a.txt", false)
+	assertExists(t, "dir/sub1/1-b.txt", true)
+	assertExists(t, "dir/sub2/2-a.txt", true)
+
+	require.NoError(t, st.Delete("dir/sub1/1-b.txt"))
+	assertExists(t, ".", true)
+	assertExists(t, "dir/sub1", false)
+	assertExists(t, "dir/sub1/1-a.txt", false)
+	assertExists(t, "dir/sub1/1-b.txt", false)
+	assertExists(t, "dir/sub2/2-a.txt", true)
+
+	require.NoError(t, st.Delete("dir/sub2/2-a.txt"))
+	assertExists(t, ".", true)
+	assertExists(t, "dir", false)
+}
+
 func TestLocalStorageIterator(t *testing.T) {
 	testStorageIterator(t, setting.LocalStorageType, &setting.Storage{Path: t.TempDir()})
 }

modules/storage/storage.go

@@ -68,7 +68,12 @@ type ObjectStorage interface {
 	Stat(path string) (os.FileInfo, error)
 	Delete(path string) error
 	URL(path, name, method string, reqParams url.Values) (*url.URL, error)
-	IterateObjects(path string, iterator func(path string, obj Object) error) error
+
+	// IterateObjects calls the iterator function for each object in the storage with the given path as prefix
+	// The "fullPath" argument in callback is the full path in this storage.
+	// * IterateObjects("", ...): iterate all objects in this storage
+	// * IterateObjects("sub-path", ...): iterate all objects with "sub-path" as prefix in this storage, the "fullPath" will be like "sub-path/xxx"
+	IterateObjects(basePath string, iterator func(fullPath string, obj Object) error) error
 }
 
 // Copy copies a file from source ObjectStorage to dest ObjectStorage

modules/util/path.go

@@ -75,19 +75,21 @@ const filepathSeparator = string(os.PathSeparator)
 //	{`/foo`, ``, `bar`} => `/foo/bar`
 //	{`/foo`, `..`, `bar`} => `/foo/bar`
 func FilePathJoinAbs(base string, sub ...string) string {
-	elems := make([]string, 1, len(sub)+1)
-
 	// 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() {
-		elems[0] = filepath.Clean(base)
-	} else {
-		elems[0] = filepath.Clean(strings.ReplaceAll(base, "\\", filepathSeparator))
+	if !isOSWindows() {
+		base = strings.ReplaceAll(base, "\\", filepathSeparator)
+	}
+	if !filepath.IsAbs(base) {
+		// This shouldn't happen. If it is really necessary to handle relative paths, use filepath.Abs() to get absolute paths first
+		panic(fmt.Sprintf("FilePathJoinAbs: %q (for path %v) is not absolute, do not guess a relative path based on current working directory", base, sub))
 	}
-	if !filepath.IsAbs(elems[0]) {
-		// This shouldn't happen. If there is really necessary to pass in relative path, return the full path with filepath.Abs() instead
-		panic(fmt.Sprintf("FilePathJoinAbs: %q (for path %v) is not absolute, do not guess a relative path based on current working directory", elems[0], elems))
+	if len(sub) == 0 {
+		return filepath.Clean(base)
 	}
+
+	elems := make([]string, 1, len(sub)+1)
+	elems[0] = base
 	for _, s := range sub {
 		if s == "" {
 			continue
@@ -98,7 +100,7 @@ func FilePathJoinAbs(base string, sub ...string) string {
 			elems = append(elems, filepath.Clean(filepathSeparator+strings.ReplaceAll(s, "\\", filepathSeparator)))
 		}
 	}
-	// the elems[0] must be an absolute path, just join them together
+	// the elems[0] must be an absolute path, just join them together, and Join will also do Clean
 	return filepath.Join(elems...)
 }
 

routers/api/actions/artifacts.go

@@ -241,7 +241,7 @@ func (ar artifactRoutes) uploadArtifact(ctx *ArtifactContext) {
 	}
 
 	// get upload file size
-	fileRealTotalSize, contentLength := getUploadFileSize(ctx)
+	fileRealTotalSize := getUploadFileSize(ctx)
 
 	// get artifact retention days
 	expiredDays := setting.Actions.ArtifactRetentionDays
@@ -265,17 +265,17 @@ func (ar artifactRoutes) uploadArtifact(ctx *ArtifactContext) {
 		return
 	}
 
-	// save chunk to storage, if success, return chunk stotal size
+	// save chunk to storage, if success, return chunks total size
 	// if artifact is not gzip when uploading, chunksTotalSize ==  fileRealTotalSize
 	// if artifact is gzip when uploading, chunksTotalSize <  fileRealTotalSize
-	chunksTotalSize, err := saveUploadChunk(ar.fs, ctx, artifact, contentLength, runID)
+	chunksTotalSize, err := saveUploadChunkV3GetTotalSize(ar.fs, ctx, artifact, runID)
 	if err != nil {
 		log.Error("Error save upload chunk: %v", err)
 		ctx.HTTPError(http.StatusInternalServerError, "Error save upload chunk")
 		return
 	}
 
-	// update artifact size if zero or not match, over write artifact size
+	// update artifact size if zero or not match, overwrite artifact size
 	if artifact.FileSize == 0 ||
 		artifact.FileCompressedSize == 0 ||
 		artifact.FileSize != fileRealTotalSize ||