This workarounds a bug in `patrickmn/go-cache`

If an expired item is set, it will not be evicted.
This workarounds a known bug: https://github.com/patrickmn/go-cache/issues/48

2 files changed, 63 insertions, 7 deletions

diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go
index d6436010..78a77e1c 100644
--- a/internal/vfs/zip/vfs.go
+++ b/internal/vfs/zip/vfs.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"net/url"
+	"sync"
 	"time"
 
 	"github.com/patrickmn/go-cache"
@@ -35,7 +36,8 @@ var (
 
 // zipVFS is a simple cached implementation of the vfs.VFS interface
 type zipVFS struct {
-	cache *cache.Cache
+	cache     *cache.Cache
+	cacheLock sync.Mutex
 
 	dataOffsetCache *lruCache
 	readlinkCache   *lruCache
@@ -88,10 +90,17 @@ func (fs *zipVFS) Name() string {
 	return "zip"
 }
 
-// findOrOpenArchive if found in fs.cache refresh if needed and return it.
-// otherwise open the archive and try to save it, if saving fails it's because
-// the archive has already been cached (e.g. by another concurrent request)
-func (fs *zipVFS) findOrOpenArchive(ctx context.Context, path string) (*zipArchive, error) {
+// findOrCreateArchive if found in fs.cache refresh if needed and return it.
+// otherwise creates the archive entry in a cache and try to save it,
+// if saving fails it's because the archive has already been cached
+// (e.g. by another concurrent request)
+func (fs *zipVFS) findOrCreateArchive(ctx context.Context, path string) (*zipArchive, error) {
+	// This needs to happen in lock to ensure that
+	// concurrent access will not remove it
+	// it is needed due to the bug https://github.com/patrickmn/go-cache/issues/48
+	fs.cacheLock.Lock()
+	defer fs.cacheLock.Unlock()
+
 	archive, expiry, found := fs.cache.GetWithExpiration(path)
 	if found {
 		metrics.ZipCacheRequests.WithLabelValues("archive", "hit").Inc()
@@ -104,18 +113,32 @@ func (fs *zipVFS) findOrOpenArchive(ctx context.Context, path string) (*zipArchi
 	} else {
 		archive = newArchive(fs, path, DefaultOpenTimeout)
 
+		// We call delete to ensure that expired item
+		// is properly evicted as there's a bug in a cache library:
+		// https://github.com/patrickmn/go-cache/issues/48
+		fs.cache.Delete(path)
+
 		// if adding the archive to the cache fails it means it's already been added before
 		// this is done to find concurrent additions.
 		if fs.cache.Add(path, archive, cache.DefaultExpiration) != nil {
 			return nil, errAlreadyCached
 		}
+
 		metrics.ZipCacheRequests.WithLabelValues("archive", "miss").Inc()
 		metrics.ZipCachedEntries.WithLabelValues("archive").Inc()
 	}
 
-	zipArchive := archive.(*zipArchive)
+	return archive.(*zipArchive), nil
+}
+
+// findOrOpenArchive gets archive from cache and tries to open it
+func (fs *zipVFS) findOrOpenArchive(ctx context.Context, path string) (*zipArchive, error) {
+	zipArchive, err := fs.findOrCreateArchive(ctx, path)
+	if err != nil {
+		return nil, err
+	}
 
-	err := zipArchive.openArchive(ctx)
+	err = zipArchive.openArchive(ctx)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go
index 434fc84c..a795b214 100644
--- a/internal/vfs/zip/vfs_test.go
+++ b/internal/vfs/zip/vfs_test.go
@@ -6,7 +6,10 @@ import (
 	"testing"
 	"time"
 
+	"github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
 func TestVFSRoot(t *testing.T) {
@@ -95,3 +98,33 @@ func TestVFSFindOrOpenArchiveConcurrentAccess(t *testing.T) {
 		return err == errAlreadyCached
 	}, time.Second, time.Nanosecond)
 }
+
+func TestVFSFindOrCreateArchiveCacheEvict(t *testing.T) {
+	testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil)
+	defer cleanup()
+
+	path := testServerURL + "/public.zip"
+
+	vfs := New().(*zipVFS)
+
+	archivesMetric := metrics.ZipCachedEntries.WithLabelValues("archive")
+	archivesCount := testutil.ToFloat64(archivesMetric)
+
+	// create a new archive and increase counters
+	archive, err := vfs.findOrOpenArchive(context.Background(), path)
+	require.NoError(t, err)
+	require.NotNil(t, archive)
+
+	// inject into cache to be "expired"
+	// (we could as well wait `defaultCacheExpirationInterval`)
+	vfs.cache.Set(path, archive, time.Nanosecond)
+
+	// a new object is created
+	archive2, err := vfs.findOrOpenArchive(context.Background(), path)
+	require.NoError(t, err)
+	require.NotNil(t, archive2)
+	require.NotEqual(t, archive, archive2, "a different archive is returned")
+
+	archivesCountEnd := testutil.ToFloat64(archivesMetric)
+	require.Equal(t, float64(1), archivesCountEnd-archivesCount, "all expired archives are evicted")
+}