Handle existing old-style cache entry directories (#5098)

Fixes: #5097

Also moves defer statements after error checks when getting a cacheEntry
to avoid panics on tmpCleanup with a nil cacheEntry.

e2e/cache/cache.go

@@ -10,12 +10,11 @@ import (
 	"path/filepath"
 	"testing"
 
-	"github.com/sylabs/singularity/internal/pkg/util/fs"
-
 	"github.com/sylabs/scs-library-client/client"
 	"github.com/sylabs/singularity/e2e/internal/e2e"
 	"github.com/sylabs/singularity/e2e/internal/testhelper"
 	"github.com/sylabs/singularity/internal/pkg/cache"
+	"github.com/sylabs/singularity/internal/pkg/util/fs"
 )
 
 type cacheTests struct {
@@ -282,5 +281,6 @@ func E2ETests(env e2e.TestEnv) testhelper.Tests {
 	return testhelper.Tests{
 		"interactive commands":     np(c.testInteractiveCacheCmds),
 		"non-interactive commands": np(c.testNoninteractiveCacheCmds),
+		"issue5097":                np(c.issue5097),
 	}
 }

e2e/cache/regressions.go

@@ -0,0 +1,73 @@
+// Copyright (c) 2020, Sylabs Inc. All rights reserved.
+// This software is licensed under a 3-clause BSD license. Please consult the
+// LICENSE.md file distributed with the sources of this project regarding your
+// rights to use or distribute this software.
+
+package cache
+
+import (
+	"os"
+	"path"
+	"path/filepath"
+	"testing"
+
+	"github.com/sylabs/scs-library-client/client"
+	"github.com/sylabs/singularity/e2e/internal/e2e"
+	"github.com/sylabs/singularity/internal/pkg/util/fs"
+)
+
+// issue5097 - need to handle an existing directory entry present in the cache
+// from older singularity versions.
+func (c cacheTests) issue5097(t *testing.T) {
+	imgCacheDir, cleanCache := e2e.MakeCacheDir(t, c.env.TestDir)
+	defer cleanCache(t)
+	c.env.ImgCacheDir = imgCacheDir
+
+	tempDir, imgStoreCleanup := e2e.MakeTempDir(t, "", "", "image store")
+	defer imgStoreCleanup(t)
+	imagePath := filepath.Join(tempDir, imgName)
+
+	// Pull through the cache - will give us a new style file in the cache
+	c.env.RunSingularity(
+		t,
+		e2e.WithProfile(e2e.UserProfile),
+		e2e.WithCommand("pull"),
+		e2e.WithArgs([]string{"--force", imagePath, imgURL}...),
+		e2e.ExpectExit(0),
+	)
+
+	// Replace the cache entry with a directory, containing the image,
+	// like in older versions of singularity
+	hash, err := client.ImageHash(imagePath)
+	if err != nil {
+		t.Fatalf("Could not calculate hash of test image: %v", err)
+	}
+	cachePath := path.Join(imgCacheDir, "cache", "library", hash)
+	err = os.Remove(cachePath)
+	if err != nil {
+		t.Fatalf("Could not remove cached image '%s': %v", cachePath, err)
+	}
+	err = os.Mkdir(cachePath, 0700)
+	if err != nil {
+		t.Fatalf("Could not create directory '%s': %v", cachePath, err)
+	}
+	err = fs.CopyFile(imagePath, path.Join(cachePath, hash), 0700)
+	if err != nil {
+		t.Fatalf("Could not copy file to directory '%s': %v", cachePath, err)
+	}
+
+	// Pull through the cache - it should work as we now remove the directory and
+	// re-pull a file into the cache
+	c.env.RunSingularity(
+		t,
+		e2e.WithProfile(e2e.UserProfile),
+		e2e.WithCommand("pull"),
+		e2e.WithArgs([]string{"--force", imagePath, imgURL}...),
+		e2e.ExpectExit(0),
+	)
+
+	if !fs.IsFile(cachePath) {
+		t.Fatalf("Cache entry '%s' is not a file", cachePath)
+	}
+
+}

internal/pkg/cache/cache.go

@@ -111,15 +111,30 @@ func (h *Handle) GetEntry(cacheType string, hash string) (e *Entry, err error) {
 
 	cacheDir, err := h.GetFileCacheDir(cacheType)
 	if err != nil {
-		return nil, fmt.Errorf("cannot get cache entry: %v", err)
+		return nil, fmt.Errorf("cannot get '%s' cache directory: %v", cacheType, err)
 	}
 
 	e.Path = filepath.Join(cacheDir, hash)
 
-	_, err = os.Stat(e.Path)
-	// If there is no existing dir return an entry with a TmpPath for the caller
+	// If there is a directory it's from an older version of Singularity
+	// We need to remove it as we work with single files per hash only now
+	if fs.IsDir(e.Path) {
+		sylog.Debugf("Removing old cache directory: %s", e.Path)
+		err := os.RemoveAll(e.Path)
+		// Allow IsNotExist in case a concurrent process already removed it
+		if err != nil && !os.IsNotExist(err) {
+			return nil, fmt.Errorf("could not remove old cache directory '%s': %v", e.Path, err)
+		}
+	}
+
+	// If there is no existing file return an entry with a TmpPath for the caller
 	// to use and then Finalize
-	if err != nil && os.IsNotExist(err) {
+	pathExists, err := fs.PathExists(e.Path)
+	if err != nil {
+		return nil, fmt.Errorf("could not check for cache entry '%s': %v", e.Path, err)
+	}
+
+	if !pathExists {
 		e.Exists = false
 		f, err := fs.MakeTmpFile(cacheDir, "tmp_", 0700)
 		if err != nil {
@@ -132,12 +147,13 @@ func (h *Handle) GetEntry(cacheType string, hash string) (e *Entry, err error) {
 		e.TmpPath = f.Name()
 		return e, nil
 	}
-	// Other error in Stat
-	if err != nil {
-		return nil, err
+
+	// Double check that there isn't something else weird there
+	if !fs.IsFile(e.Path) {
+		return nil, fmt.Errorf("path '%s' exists but is not a file", e.Path)
 	}
 
-	// It exists. Caller can use the Path directly
+	// It exists in the cache and it's a file. Caller can use the Path directly
 	e.Exists = true
 	return e, nil
 }

internal/pkg/cache/entry.go

@@ -10,9 +10,8 @@ import (
 	"fmt"
 	"os"
 
-	"github.com/sylabs/singularity/internal/pkg/util/fs"
-
 	"github.com/sylabs/singularity/internal/pkg/sylog"
+	"github.com/sylabs/singularity/internal/pkg/util/fs"
 )
 
 // Entry is a structure representing an entry in the cache. An entry is a file under the

internal/pkg/client/library/pull.go

@@ -53,10 +53,10 @@ func pull(ctx context.Context, imgCache *cache.Handle, directTo, pullFrom string
 
 	} else {
 		cacheEntry, err := imgCache.GetEntry(cache.LibraryCacheType, libraryImage.Hash)
-		defer cacheEntry.CleanTmp()
 		if err != nil {
 			return "", fmt.Errorf("unable to check if %v exists in cache: %v", libraryImage.Hash, err)
 		}
+		defer cacheEntry.CleanTmp()
 		if !cacheEntry.Exists {
 			sylog.Infof("Downloading library image")
 

internal/pkg/client/net/pull.go

@@ -139,10 +139,10 @@ func pull(ctx context.Context, imgCache *cache.Handle, directTo, pullFrom string
 
 	} else {
 		cacheEntry, err := imgCache.GetEntry(cache.NetCacheType, hash)
-		defer cacheEntry.CleanTmp()
 		if err != nil {
 			return "", fmt.Errorf("unable to check if %v exists in cache: %v", hash, err)
 		}
+		defer cacheEntry.CleanTmp()
 
 		if !cacheEntry.Exists {
 			sylog.Infof("Downloading network image")

internal/pkg/client/oci/pull.go

@@ -40,10 +40,10 @@ func pull(ctx context.Context, imgCache *cache.Handle, directTo, pullFrom, tmpDi
 	} else {
 
 		cacheEntry, err := imgCache.GetEntry(cache.OciTempCacheType, hash)
-		defer cacheEntry.CleanTmp()
 		if err != nil {
 			return "", fmt.Errorf("unable to check if %v exists in cache: %v", hash, err)
 		}
+		defer cacheEntry.CleanTmp()
 		if !cacheEntry.Exists {
 			sylog.Infof("Converting OCI blobs to SIF format")
 

internal/pkg/client/oras/pull.go

@@ -32,10 +32,10 @@ func pull(ctx context.Context, imgCache *cache.Handle, directTo, pullFrom string
 
 	} else {
 		cacheEntry, err := imgCache.GetEntry(cache.OrasCacheType, hash)
-		defer cacheEntry.CleanTmp()
 		if err != nil {
 			return "", fmt.Errorf("unable to check if %v exists in cache: %v", hash, err)
 		}
+		defer cacheEntry.CleanTmp()
 		if !cacheEntry.Exists {
 			sylog.Infof("Downloading oras image")
 

internal/pkg/client/shub/pull.go

@@ -141,10 +141,10 @@ func pull(ctx context.Context, imgCache *cache.Handle, directTo, pullFrom string
 		imagePath = directTo
 	} else {
 		cacheEntry, err := imgCache.GetEntry(cache.ShubCacheType, manifest.Commit)
-		defer cacheEntry.CleanTmp()
 		if err != nil {
 			return "", fmt.Errorf("unable to check if %v exists in cache: %v", manifest.Commit, err)
 		}
+		defer cacheEntry.CleanTmp()
 		if !cacheEntry.Exists {
 			sylog.Infof("Downloading shub image")