fix: don't panic when closing registry querier

cmd/opm/serve/serve.go

@@ -103,10 +103,10 @@ func (s *serve) run(ctx context.Context) error {
 	s.logger = s.logger.WithFields(logrus.Fields{"configs": s.configDir, "port": s.port})
 
 	store, err := registry.NewQuerierFromFS(os.DirFS(s.configDir), s.cacheDir)
-	defer store.Close()
 	if err != nil {
 		return err
 	}
+	defer store.Close()
 	if s.cacheOnly {
 		return nil
 	}

pkg/registry/query.go

@@ -27,7 +27,12 @@ type Querier struct {
 }
 
 func (q Querier) Close() error {
-	return q.cache.close()
+	if q.cache != nil {
+		if err := q.cache.close(); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 type apiBundleKey struct {
@@ -47,26 +52,22 @@ func (s *SliceBundleSender) Send(b *api.Bundle) error {
 var _ GRPCQuery = &Querier{}
 
 func NewQuerierFromFS(fbcFS fs.FS, cacheDir string) (*Querier, error) {
-	q := &Querier{}
-	var err error
-	q.cache, err = newCache(cacheDir, &fbcCacheModel{
+	cache, err := newCache(cacheDir, &fbcCacheModel{
 		FBC:   fbcFS,
 		Cache: os.DirFS(cacheDir),
 	})
 	if err != nil {
-		return q, err
+		return nil, err
 	}
-	return q, nil
+	return &Querier{cache: cache}, nil
 }
 
 func NewQuerier(m model.Model) (*Querier, error) {
-	q := &Querier{}
-	var err error
-	q.cache, err = newCache("", &nonDigestableModel{Model: m})
+	cache, err := newCache("", &nonDigestableModel{Model: m})
 	if err != nil {
-		return q, err
+		return nil, err
 	}
-	return q, nil
+	return &Querier{cache: cache}, nil
 }
 
 func (q Querier) loadAPIBundle(k apiBundleKey) (*api.Bundle, error) {
@@ -414,7 +415,13 @@ func newCache(baseDir string, model digestableModel) (*cache, error) {
 	if err != nil {
 		return nil, err
 	}
-	return qc, qc.load(model)
+	if err := qc.load(model); err != nil {
+		// try to clean up after ourselves by closing the cache (by cleaning
+		// up any temporary files/directories) before returning the error.
+		_ = qc.close()
+		return nil, err
+	}
+	return qc, nil
 }
 
 func (qc cache) close() error {
@@ -430,6 +437,9 @@ func newEphemeralCache() (*cache, error) {
 		return nil, err
 	}
 	if err := os.MkdirAll(filepath.Join(baseDir, "cache"), cachePermissionDir); err != nil {
+		// try to clean up after ourselves, so we don't leave a directory around
+		// when returning an error.
+		_ = os.RemoveAll(baseDir)
 		return nil, err
 	}
 	return &cache{

pkg/registry/query_test.go

@@ -3,6 +3,7 @@ package registry
 import (
 	"context"
 	"io/fs"
+	"os"
 	"testing"
 	"testing/fstest"
 
@@ -248,6 +249,45 @@ func TestQuerier_BadBundleRaisesError(t *testing.T) {
 	})
 }
 
+func TestQuerier_NewQuerierFromFS_FailMkdir(t *testing.T) {
+	origTmpDir, tmpDirSet := os.LookupEnv("TMPDIR")
+	tmpDir := t.TempDir()
+	os.Chmod(tmpDir, 0444)
+	require.NoError(t, os.Setenv("TMPDIR", tmpDir))
+	defer func() {
+		if tmpDirSet {
+			require.NoError(t, os.Setenv("TMPDIR", origTmpDir))
+		} else {
+			os.Unsetenv("TMPDIR")
+		}
+	}()
+
+	q, err := NewQuerierFromFS(validFS, "")
+	require.Nil(t, q)
+	require.Error(t, err)
+}
+
+func TestQuerier_NewQuerierFromFS_FailCleanup(t *testing.T) {
+	origTmpDir, tmpDirSet := os.LookupEnv("TMPDIR")
+	tmpDir := t.TempDir()
+	require.NoError(t, os.Setenv("TMPDIR", tmpDir))
+	defer func() {
+		if tmpDirSet {
+			require.NoError(t, os.Setenv("TMPDIR", origTmpDir))
+		} else {
+			os.Unsetenv("TMPDIR")
+		}
+	}()
+
+	q, err := NewQuerierFromFS(badBundleFS, "")
+	require.Nil(t, q)
+	require.Error(t, err)
+
+	tmpDirEntries, err := os.ReadDir(tmpDir)
+	require.Len(t, tmpDirEntries, 0)
+	require.NoError(t, err)
+}
+
 func genTestQueriers(t *testing.T, fbcFS fs.FS) []*Querier {
 	t.Helper()