Prevent lazy config lookups in tempdir cleaners

12 files changed, 61 insertions, 82 deletions

diff --git a/internal/cache/keyer.go b/internal/cache/keyer.go
index b8d4609d0..ab60e7f63 100644
--- a/internal/cache/keyer.go
+++ b/internal/cache/keyer.go
@@ -13,6 +13,7 @@ import (
 
 	"github.com/golang/protobuf/proto"
 	"github.com/google/uuid"
+	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/safe"
 	"gitlab.com/gitlab-org/gitaly/internal/tempdir"
@@ -213,7 +214,12 @@ func newPendingLease(repo *gitalypb.Repository) (string, error) {
 
 // cacheDir is $STORAGE/+gitaly/cache
 func cacheDir(repo *gitalypb.Repository) (string, error) {
-	return tempdir.CacheDir(repo.GetStorageName())
+	storage, ok := config.Config.Storage(repo.StorageName)
+	if !ok {
+		return "", fmt.Errorf("storage not found for %v", repo)
+	}
+
+	return tempdir.CacheDir(storage), nil
 }
 
 func currentLeases(repo *gitalypb.Repository) ([]os.FileInfo, error) {
diff --git a/internal/cache/walk_test.go b/internal/cache/walk_test.go
index bfe037ff4..e7260214b 100644
--- a/internal/cache/walk_test.go
+++ b/internal/cache/walk_test.go
@@ -4,9 +4,10 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"gitlab.com/gitlab-org/gitaly/internal/config"
 )
 
 func TestcleanWalkDirNotExists(t *testing.T) {
-	err := cleanWalk("/path/that/does/not/exist")
+	err := cleanWalk(config.Storage{Name: "foo", Path: "/path/that/does/not/exist"})
 	assert.NoError(t, err, "cleanWalk returned an error for a non existing directory")
 }
diff --git a/internal/cache/walker.go b/internal/cache/walker.go
index 6e0ba685a..aacd3c58a 100644
--- a/internal/cache/walker.go
+++ b/internal/cache/walker.go
@@ -16,13 +16,8 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/tempdir"
 )
 
-func cleanWalk(storageName string) error {
-	cachePath, err := tempdir.CacheDir(storageName)
-	if err != nil {
-		return err
-	}
-
-	walkErr := filepath.Walk(cachePath, func(path string, info os.FileInfo, err error) error {
+func cleanWalk(storage config.Storage) error {
+	walkErr := filepath.Walk(tempdir.CacheDir(storage), func(path string, info os.FileInfo, err error) error {
 		if err != nil {
 			return err
 		}
@@ -71,7 +66,7 @@ func startCleanWalker(storage config.Storage) {
 	walkTick := time.NewTicker(cleanWalkFrequency)
 	go func() {
 		for {
-			if err := cleanWalk(storage.Name); err != nil {
+			if err := cleanWalk(storage); err != nil {
 				logrus.WithField("storage", storage.Name).Error(err)
 			}
 
@@ -95,16 +90,7 @@ func moveAndClear(storage config.Storage) error {
 	logger := logrus.WithField("storage", storage.Name)
 	logger.Info("clearing disk cache object folder")
 
-	cachePath, err := tempdir.CacheDir(storage.Name)
-	if err != nil {
-		return err
-	}
-
-	tempPath, err := tempdir.TempDir(storage.Name)
-	if err != nil {
-		return err
-	}
-
+	tempPath := tempdir.TempDir(storage)
 	if err := os.MkdirAll(tempPath, 0755); err != nil {
 		return err
 	}
@@ -115,6 +101,7 @@ func moveAndClear(storage config.Storage) error {
 	}
 
 	logger.Infof("moving disk cache object folder to %s", tmpDir)
+	cachePath := tempdir.CacheDir(storage)
 	if err := os.Rename(cachePath, filepath.Join(tmpDir, "moved")); err != nil {
 		if os.IsNotExist(err) {
 			logger.Info("disk cache object folder doesn't exist, no need to remove")
@@ -137,8 +124,8 @@ func moveAndClear(storage config.Storage) error {
 }
 
 func init() {
-	config.RegisterHook(func() error {
-		for _, storage := range config.Config.Storages {
+	config.RegisterHook(func(cfg config.Cfg) error {
+		for _, storage := range cfg.Storages {
 			if err := moveAndClear(storage); err != nil {
 				return err
 			}
diff --git a/internal/cache/walker_test.go b/internal/cache/walker_test.go
index b3d3044d6..efef681b0 100644
--- a/internal/cache/walker_test.go
+++ b/internal/cache/walker_test.go
@@ -31,8 +31,7 @@ func TestDiskCacheObjectWalker(t *testing.T) {
 		{"2b/ancient", 24 * time.Hour, true},
 		{"cd/baby", time.Second, false},
 	} {
-		cacheDir, err := tempdir.CacheDir(t.Name()) // test name is storage name
-		require.NoError(t, err)
+		cacheDir := tempdir.CacheDir(config.Config.Storages[0])
 
 		path := filepath.Join(cacheDir, tt.name)
 		require.NoError(t, os.MkdirAll(filepath.Dir(path), 0755))
@@ -76,8 +75,7 @@ func TestDiskCacheInitialClear(t *testing.T) {
 	cleanup := setupDiskCacheWalker(t)
 	defer cleanup()
 
-	cacheDir, err := tempdir.CacheDir(t.Name()) // test name is storage name
-	require.NoError(t, err)
+	cacheDir := tempdir.CacheDir(config.Config.Storages[0])
 
 	canary := filepath.Join(cacheDir, "canary.txt")
 	require.NoError(t, os.MkdirAll(filepath.Dir(canary), 0755))
diff --git a/internal/config/config.go b/internal/config/config.go
index fb711adda..372ab182f 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -25,7 +25,7 @@ var (
 	// Config stores the global configuration
 	Config Cfg
 
-	hooks []func() error
+	hooks []func(Cfg) error
 )
 
 // Cfg is a container for all config derived from config.toml.
@@ -122,12 +122,16 @@ func Load(file io.Reader) error {
 	return nil
 }
 
-// RegisterHook adds a post-validation callback.
-func RegisterHook(f func() error) {
+// RegisterHook adds a post-validation callback. Your hook should only
+// access config via the Cfg instance it gets passed. This avoids race
+// conditions during testing, when the global config.Config instance gets
+// updated after these hooks have run.
+func RegisterHook(f func(c Cfg) error) {
 	hooks = append(hooks, f)
 }
 
-// Validate checks the current Config for sanity.
+// Validate checks the current Config for sanity. It also runs all hooks
+// registered with RegisterHook.
 func Validate() error {
 	for _, err := range []error{
 		validateListeners(),
@@ -144,7 +148,7 @@ func Validate() error {
 	}
 
 	for _, f := range hooks {
-		if err := f(); err != nil {
+		if err := f(Config); err != nil {
 			return err
 		}
 	}
@@ -256,13 +260,19 @@ func SetGitPath() error {
 
 // StoragePath looks up the base path for storageName. The second boolean
 // return value indicates if anything was found.
-func StoragePath(storageName string) (string, bool) {
-	for _, storage := range Config.Storages {
+func (c Cfg) StoragePath(storageName string) (string, bool) {
+	storage, ok := c.Storage(storageName)
+	return storage.Path, ok
+}
+
+// Storage looks up storageName.
+func (c Cfg) Storage(storageName string) (Storage, bool) {
+	for _, storage := range c.Storages {
 		if storage.Name == storageName {
-			return storage.Path, true
+			return storage, true
 		}
 	}
-	return "", false
+	return Storage{}, false
 }
 
 func validateBinDir() error {
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 36cc77276..338b27ef2 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -276,15 +276,11 @@ func TestValidateStorages(t *testing.T) {
 }
 
 func TestStoragePath(t *testing.T) {
-	defer func(oldStorages []Storage) {
-		Config.Storages = oldStorages
-	}(Config.Storages)
-
-	Config.Storages = []Storage{
+	cfg := Cfg{Storages: []Storage{
 		{Name: "default", Path: "/home/git/repositories1"},
 		{Name: "other", Path: "/home/git/repositories2"},
 		{Name: "third", Path: "/home/git/repositories3"},
-	}
+	}}
 
 	testCases := []struct {
 		in, out string
@@ -297,7 +293,7 @@ func TestStoragePath(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		out, ok := StoragePath(tc.in)
+		out, ok := cfg.StoragePath(tc.in)
 		if !assert.Equal(t, tc.ok, ok, "%+v", tc) {
 			continue
 		}
diff --git a/internal/git/catfile/batch_cache.go b/internal/git/catfile/batch_cache.go
index b78fdbfbb..7d08cc598 100644
--- a/internal/git/catfile/batch_cache.go
+++ b/internal/git/catfile/batch_cache.go
@@ -32,8 +32,8 @@ var cache *batchCache
 func init() {
 	prometheus.MustRegister(catfileCacheMembers)
 
-	config.RegisterHook(func() error {
-		cache = newCache(DefaultBatchfileTTL, config.Config.Git.CatfileCacheSize)
+	config.RegisterHook(func(cfg config.Cfg) error {
+		cache = newCache(DefaultBatchfileTTL, cfg.Git.CatfileCacheSize)
 		return nil
 	})
 }
diff --git a/internal/helper/repo.go b/internal/helper/repo.go
index c35725b25..9274949a4 100644
--- a/internal/helper/repo.go
+++ b/internal/helper/repo.go
@@ -60,7 +60,7 @@ func GetPath(repo repository.GitRepo) (string, error) {
 // GetStorageByName will return the path for the storage, which is fetched by
 // its key. An error is return if it cannot be found.
 func GetStorageByName(storageName string) (string, error) {
-	storagePath, ok := config.StoragePath(storageName)
+	storagePath, ok := config.Config.StoragePath(storageName)
 	if !ok {
 		return "", status.Errorf(codes.InvalidArgument, "Storage can not be found by name '%s'", storageName)
 	}
diff --git a/internal/linguist/linguist.go b/internal/linguist/linguist.go
index 8ce217e4c..3b215fd9a 100644
--- a/internal/linguist/linguist.go
+++ b/internal/linguist/linguist.go
@@ -57,8 +57,8 @@ func Color(language string) string {
 }
 
 // LoadColors loads the name->color map from the Linguist gem.
-func LoadColors() error {
-	jsonReader, err := openLanguagesJSON()
+func LoadColors(cfg config.Cfg) error {
+	jsonReader, err := openLanguagesJSON(cfg)
 	if err != nil {
 		return err
 	}
@@ -67,8 +67,8 @@ func LoadColors() error {
 	return json.NewDecoder(jsonReader).Decode(&colorMap)
 }
 
-func openLanguagesJSON() (io.ReadCloser, error) {
-	if jsonPath := config.Config.Ruby.LinguistLanguagesPath; jsonPath != "" {
+func openLanguagesJSON(cfg config.Cfg) (io.ReadCloser, error) {
+	if jsonPath := cfg.Ruby.LinguistLanguagesPath; jsonPath != "" {
 		// This is a fallback for environments where dynamic discovery of the
 		// linguist path via Bundler is not working for some reason, for example
 		// https://gitlab.com/gitlab-org/gitaly/issues/1119.
diff --git a/internal/linguist/linguist_test.go b/internal/linguist/linguist_test.go
index 8522c6f45..93c3e8b2b 100644
--- a/internal/linguist/linguist_test.go
+++ b/internal/linguist/linguist_test.go
@@ -11,7 +11,7 @@ import (
 
 func TestLoadLanguages(t *testing.T) {
 	colorMap = make(map[string]Language)
-	require.NoError(t, LoadColors(), "load colors")
+	require.NoError(t, LoadColors(config.Config), "load colors")
 
 	require.Equal(t, "#701516", Color("Ruby"), "color value for 'Ruby'")
 }
@@ -23,7 +23,7 @@ func TestLoadLanguagesCustomPath(t *testing.T) {
 	config.Config.Ruby.LinguistLanguagesPath = jsonPath
 
 	colorMap = make(map[string]Language)
-	require.NoError(t, LoadColors(), "load colors")
+	require.NoError(t, LoadColors(config.Config), "load colors")
 
 	require.Equal(t, "foo color", Color("FooBar"))
 }
diff --git a/internal/tempdir/tempdir.go b/internal/tempdir/tempdir.go
index 49ce183f1..b24117726 100644
--- a/internal/tempdir/tempdir.go
+++ b/internal/tempdir/tempdir.go
@@ -11,7 +11,6 @@ import (
 
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
-	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/housekeeping"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
@@ -38,24 +37,10 @@ const (
 )
 
 // CacheDir returns the path to the cache dir for a storage location
-func CacheDir(storageName string) (string, error) {
-	storagePath, err := helper.GetStorageByName(storageName)
-	if err != nil {
-		return "", err
-	}
-
-	return filepath.Join(storagePath, cachePrefix), nil
-}
+func CacheDir(storage config.Storage) string { return filepath.Join(storage.Path, cachePrefix) }
 
 // TempDir returns the path to the temp dir for a storage location
-func TempDir(storageName string) (string, error) {
-	storagePath, err := helper.GetStorageByName(storageName)
-	if err != nil {
-		return "", err
-	}
-
-	return filepath.Join(storagePath, tmpRootPrefix), nil
-}
+func TempDir(storage config.Storage) string { return filepath.Join(storage.Path, tmpRootPrefix) }
 
 // ForDeleteAllRepositories returns a temporary directory for the given storage. It is not context-scoped but it will get removed eventuall (after MaxAge).
 func ForDeleteAllRepositories(storageName string) (string, error) {
@@ -84,12 +69,12 @@ func NewAsRepository(ctx context.Context, repo *gitalypb.Repository) (*gitalypb.
 }
 
 func newAsRepository(ctx context.Context, storageName string, prefix string) (*gitalypb.Repository, string, error) {
-	storageDir, err := helper.GetStorageByName(storageName)
-	if err != nil {
-		return nil, "", err
+	storage, ok := config.Config.Storage(storageName)
+	if !ok {
+		return nil, "", fmt.Errorf("storage not found: %v", storageName)
 	}
 
-	root := tmpRoot(storageDir)
+	root := TempDir(storage)
 	if err := os.MkdirAll(root, 0700); err != nil {
 		return nil, "", err
 	}
@@ -105,24 +90,20 @@ func newAsRepository(ctx context.Context, storageName string, prefix string) (*g
 	}()
 
 	newAsRepo := &gitalypb.Repository{StorageName: storageName}
-	newAsRepo.RelativePath, err = filepath.Rel(storageDir, tempDir)
+	newAsRepo.RelativePath, err = filepath.Rel(storage.Path, tempDir)
 	return newAsRepo, tempDir, err
 }
 
-func tmpRoot(storageRoot string) string {
-	return filepath.Join(storageRoot, tmpRootPrefix)
-}
-
 // StartCleaning starts tempdir cleanup goroutines.
 func StartCleaning() {
 	for _, st := range config.Config.Storages {
-		go func(name string, dir string) {
+		go func(storage config.Storage) {
 			start := time.Now()
-			err := clean(tmpRoot(dir))
+			err := clean(TempDir(storage))
 
 			entry := log.WithFields(log.Fields{
 				"time_ms": int(1000 * time.Since(start).Seconds()),
-				"storage": name,
+				"storage": storage.Name,
 			})
 			if err != nil {
 				entry = entry.WithError(err)
@@ -130,7 +111,7 @@ func StartCleaning() {
 			entry.Info("finished tempdir cleaner walk")
 
 			time.Sleep(1 * time.Hour)
-		}(st.Name, st.Path)
+		}(st)
 	}
 }
 
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index adee54c2d..fd55df70b 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -121,7 +121,7 @@ func GitalyServersMetadata(t *testing.T, serverSocketPath string) metadata.MD {
 }
 
 func testRepoValid(repo *gitalypb.Repository) bool {
-	storagePath, _ := config.StoragePath(repo.GetStorageName())
+	storagePath, _ := config.Config.StoragePath(repo.GetStorageName())
 	if _, err := os.Stat(path.Join(storagePath, repo.RelativePath, "objects")); err != nil {
 		return false
 	}