fix: avoid race condition across multiple running processes when deleting stale cache (#423)

Co-authored-by: Mark Lee <mark.lee@outreach.io>

Author: iamrz1

internal/modules/module.go

@@ -26,6 +26,7 @@ import (
 	"github.com/go-git/go-git/v5/plumbing"
 	githttp "github.com/go-git/go-git/v5/plumbing/transport/http"
 	"github.com/go-git/go-git/v5/storage/memory"
+	"github.com/gofrs/flock"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"gopkg.in/yaml.v3"
@@ -136,6 +137,16 @@ func (m *Module) RegisterExtensions(ctx context.Context, log logrus.FieldLogger,
 // Manifest downloads the module if not already downloaded and returns a parsed
 // configuration.TemplateRepositoryManifest of this module.
 func (m *Module) Manifest(ctx context.Context) (configuration.TemplateRepositoryManifest, error) {
+	lockDir := FSLockDir(m.PathSlug())
+	lock, err := exclusiveLockDirectory(lockDir)
+	if err != nil {
+		return configuration.TemplateRepositoryManifest{},
+			errors.Wrapf(err, "failed to lock module cache directory %q", lockDir)
+	}
+
+	//nolint:errcheck // Why: Unlock error can be safely ignored here
+	defer lock.Unlock()
+
 	fs, err := m.GetFS(ctx)
 	if err != nil {
 		return configuration.TemplateRepositoryManifest{}, errors.Wrap(err, "failed to download fs")
@@ -182,21 +193,20 @@ func (m *Module) GetFS(ctx context.Context) (billy.Filesystem, error) {
 		return m.fs, nil
 	}
 
-	cacheDir := filepath.Join(StencilCacheDir(), "module_fs", ModuleCacheDirectory(m.URI, m.Version))
-	logrus.Debug("cacheDir", cacheDir)
+	cacheDir := m.FSCacheDir()
 
-	if useModuleCache(cacheDir) {
+	if useCache(cacheDir) {
 		m.fs = osfs.New(cacheDir)
 		return m.fs, nil
 	}
 
 	err = os.RemoveAll(cacheDir)
 	if err != nil {
-		return nil, errors.Wrapf(err, "failed to remove stale cache %q", cacheDir)
+		return nil, errors.Wrapf(err, "failed to remove stale module cache directory %q", cacheDir)
 	}
 
 	if err = os.MkdirAll(cacheDir, 0o755); err != nil {
-		return nil, errors.Wrap(err, "failed to create cache directory")
+		return nil, errors.Wrapf(err, "failed to create module cache directory %q", cacheDir)
 	}
 
 	m.fs = osfs.New(cacheDir)
@@ -237,26 +247,88 @@ func (m *Module) GetFS(ctx context.Context) (billy.Filesystem, error) {
 	return m.fs, nil
 }
 
-// useModuleCache determines if the specified path should be used as a module cache.
-func useModuleCache(path string) bool {
+func (m *Module) PathSlug() string {
+	return PathSlug(m.URI, m.Version)
+}
+
+func (m *Module) FSCacheDir() string {
+	return FSCacheDir(m.PathSlug())
+}
+
+// exclusiveLockDirectory creates a new flock lock for the specified directory.
+func exclusiveLockDirectory(dir string) (*flock.Flock, error) {
+	lock := flock.New(filepath.Join(dir, "ex_dir.lock"))
+	for {
+		locked, err := lock.TryLock()
+		if err != nil {
+			if !errors.Is(err, os.ErrNotExist) {
+				return nil, err
+			}
+
+			if err = os.MkdirAll(dir, 0o755); err != nil {
+				return nil, errors.Wrapf(err, "failed to create directory %q", dir)
+			}
+			continue
+		}
+
+		if locked {
+			break
+		}
+	}
+
+	return lock, nil
+}
+
+// useCache determines if the specified path should be used as a module cache.
+func useCache(path string) bool {
 	info, err := os.Stat(path)
 	if err != nil || time.Since(info.ModTime()) > ModuleCacheTTL {
 		return false
 	}
 
+	if files, err := os.ReadDir(path); err != nil || len(files) == 0 {
+		return false
+	}
+
 	return true
 }
 
-// ModuleCacheDirectory generates a directory name for the module from the given URI and optional branch.
-func ModuleCacheDirectory(uri, branch string) string {
-	if branch == "" {
-		branch = "v0.0.0"
+// PathSlug returns a unique identifier for a module
+// using the given URI and versioning constraints.
+func PathSlug(uri, version string) string {
+	if version == "" {
+		version = "v0.0.0"
 	}
 
-	return regexp.MustCompile(`[^a-zA-Z0-9@]+`).ReplaceAllString(uri+"@"+branch, "_")
+	return regexp.MustCompile(`[^a-zA-Z0-9<>=.\[\]\-@]+`).ReplaceAllString(uri+"@"+version, "_")
 }
 
 // StencilCacheDir returns the directory where stencil caches its data.
 func StencilCacheDir() string {
 	return filepath.Join(os.TempDir(), "stencil_cache")
 }
+
+// CacheDir returns the cache directory for a module based on its type and ID.
+func CacheDir(cacheType, moduleID string) string {
+	return filepath.Join(StencilCacheDir(), cacheType, moduleID)
+}
+
+// FSCacheDir returns the cache directory for a module based on its ID.
+func FSCacheDir(moduleID string) string {
+	return CacheDir("module_fs", moduleID)
+}
+
+// VersionCacheDir returns the version cache directory for a module based on its ID.
+func VersionCacheDir(moduleID string) string {
+	return CacheDir("module_version", moduleID)
+}
+
+// VersionLockDir returns the lock directory for a module version based on its ID.
+func VersionLockDir(moduleID string) string {
+	return CacheDir("module_version_lock", moduleID)
+}
+
+// FSLockDir returns the lock directory for a module filesystem based on its ID.
+func FSLockDir(moduleID string) string {
+	return CacheDir("module_fs_lock", moduleID)
+}

internal/modules/module_test.go

@@ -7,7 +7,6 @@ package modules_test
 import (
 	"context"
 	"os"
-	"path/filepath"
 	"strings"
 	"testing"
 	"time"
@@ -388,7 +387,7 @@ func TestGetFS_CacheUsage(t *testing.T) {
 		Version: version,
 	}
 
-	cacheDir := filepath.Join(modules.StencilCacheDir(), "module_fs", modules.ModuleCacheDirectory(repoURL, version))
+	cacheDir := modules.FSCacheDir(modules.PathSlug(repoURL, version))
 	err := os.RemoveAll(cacheDir)
 	assert.NilError(t, err, "failed to remove cache directory")
 
@@ -412,6 +411,46 @@ func TestGetFS_CacheUsage(t *testing.T) {
 	)
 }
 
+func TestCanRecreateCacheAfterTimeout(t *testing.T) {
+	ctx := context.Background()
+	opts := &modules.ModuleResolveOptions{
+		ConcurrentResolvers: 5,
+		ServiceManifest: &configuration.ServiceManifest{
+			Name: "test-cache-timeout",
+			Modules: []*configuration.TemplateRepository{
+				{
+					Name:    "github.com/getoutreach/stencil-base",
+					Channel: "stable",
+				},
+				{
+					Name:    "github.com/getoutreach/stencil-base",
+					Version: ">=0.14.0",
+				},
+			},
+		},
+		Log: newLogger(),
+	}
+
+	mods, err := modules.GetModulesForService(ctx, opts)
+	assert.NilError(t, err, "failed to call GetModulesForService()")
+
+	cacheExpireDuration := modules.ModuleCacheTTL + time.Minute
+	for _, m := range mods {
+		err = os.Chtimes(m.FSCacheDir(), time.Now().Add(-cacheExpireDuration), time.Now().Add(-cacheExpireDuration))
+		assert.NilError(t, err, "failed to change cache directory times")
+	}
+
+	mods, err = modules.GetModulesForService(ctx, opts)
+	assert.NilError(t, err, "failed to call GetModulesForService()")
+
+	for _, m := range mods {
+		info, err := os.Stat(m.FSCacheDir())
+		assert.NilError(t, err, "failed to stat cache directory")
+		cachedTime := time.Since(info.ModTime())
+		assert.Assert(t, cachedTime < time.Minute, "expected new cache: cached %v ago", cachedTime)
+	}
+}
+
 func assertFSExists(t *testing.T, fs billy.Filesystem) {
 	t.Helper()
 

internal/modules/worklist.go

@@ -18,7 +18,6 @@ import (
 	"github.com/getoutreach/gobox/pkg/cfg"
 	"github.com/getoutreach/gobox/pkg/cli/updater/resolver"
 	"github.com/getoutreach/stencil/pkg/configuration"
-	"github.com/gofrs/flock"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
@@ -212,11 +211,21 @@ func (list *workList) getLatestModuleForConstraints(ctx context.Context, item *w
 		return module.version, nil
 	}
 
-	cacheFile := filepath.Join(StencilCacheDir(), "module_version",
-		ModuleCacheDirectory(item.uri, item.spec.conf.Channel), "version.json")
+	versionID := fmt.Sprintf("ch_%s_cons_%v", channel, constraints)
+	moduleID := PathSlug(item.uri, versionID)
+	lockDir := VersionLockDir(moduleID)
+	lock, err := exclusiveLockDirectory(lockDir)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to lock module version cache dir %q", lockDir)
+	}
 
-	if useModuleCache(cacheFile) {
-		return getCachedModuleVersion(cacheFile)
+	//nolint:errcheck // Why: Unlock error can be safely ignored here
+	defer lock.Unlock()
+
+	cacheDir := VersionCacheDir(moduleID)
+	cacheFile := filepath.Join(cacheDir, "version.json")
+	if useCache(cacheDir) {
+		return getCachedVersion(cacheFile)
 	}
 
 	v, err := resolver.Resolve(ctx, token, &resolver.Criteria{
@@ -244,16 +253,16 @@ func (list *workList) getLatestModuleForConstraints(ctx context.Context, item *w
 		return nil, errors.Wrapf(err, "failed to resolve module '%s' with constraints\n%s", m.conf.Name, errorString)
 	}
 
-	err = setModuleVersionCache(cacheFile, v)
+	err = setCachedVersion(cacheFile, v)
 	if err != nil {
 		return nil, err
 	}
 
 	return v, nil
 }
 
-// getCachedModuleVersion returns the module version from the cache file.
-func getCachedModuleVersion(cacheFile string) (*resolver.Version, error) {
+// getCachedVersion returns the module version from the cache file.
+func getCachedVersion(cacheFile string) (*resolver.Version, error) {
 	data, err := os.ReadFile(cacheFile)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to read cached version from file %s", cacheFile)
@@ -268,22 +277,23 @@ func getCachedModuleVersion(cacheFile string) (*resolver.Version, error) {
 	return &cached, nil
 }
 
-// setModuleVersionCache writes the version for a module to a local cache file.
-func setModuleVersionCache(cacheFile string, v *resolver.Version) error {
+// setCachedVersion writes the version for a module to a local cache file.
+func setCachedVersion(cacheFile string, v *resolver.Version) error {
+	cacheDir := filepath.Dir(cacheFile)
 	data, err := json.Marshal(v)
 	if err != nil {
 		return errors.Wrapf(err, "failed to serialize module version to cache file %s", cacheFile)
 	}
 
-	lockFile := cacheFile + ".lock"
-	fl := flock.New(lockFile)
-	locked, err := fl.TryLock()
-	if err != nil || !locked {
-		return nil
+	err = os.RemoveAll(cacheDir)
+	if err != nil {
+		return errors.Wrapf(err, "failed to remove module version cache %s", cacheDir)
 	}
 
-	//nolint:errcheck // Why: Unlock error can be safely ignored here
-	defer fl.Unlock()
+	err = os.MkdirAll(cacheDir, 0o755)
+	if err != nil {
+		return errors.Wrapf(err, "failed to create directory for module version cache %s", cacheDir)
+	}
 
 	err = os.WriteFile(cacheFile, data, 0o600)
 	if err != nil {