1 files changed, 48 insertions, 15 deletions

diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go
index 3b69d1e9..692a4a69 100644
--- a/internal/vfs/zip/vfs.go
+++ b/internal/vfs/zip/vfs.go
@@ -110,6 +110,20 @@ func New(options ...Option) vfs.VFS {
 	return zipVFS
 }
 
+func (fs *zipVFS) keyFromPath(path string) (string, error) {
+	// We assume that our URL is https://.../artifacts.zip?content-sign=aaa
+	// our caching key is `https://.../artifacts.zip`
+	// TODO: replace caching key with file_sha256
+	// https://gitlab.com/gitlab-org/gitlab-pages/-/issues/489
+	key, err := url.Parse(path)
+	if err != nil {
+		return "", err
+	}
+	key.RawQuery = ""
+	key.Fragment = ""
+	return key.String(), nil
+}
+
 // Root opens an archive given a URL path and returns an instance of zipArchive
 // that implements the vfs.VFS interface.
 // To avoid using locks, the findOrOpenArchive function runs inside of a for
@@ -118,14 +132,14 @@ func New(options ...Option) vfs.VFS {
 // to try and find the cached archive or return if there's an error, for example
 // if the context is canceled.
 func (fs *zipVFS) Root(ctx context.Context, path string) (vfs.Root, error) {
-	urlPath, err := url.Parse(path)
+	key, err := fs.keyFromPath(path)
 	if err != nil {
 		return nil, err
 	}
 
 	// we do it in loop to not use any additional locks
 	for {
-		root, err := fs.findOrOpenArchive(ctx, urlPath.String())
+		root, err := fs.findOrOpenArchive(ctx, key, path)
 		if err == errAlreadyCached {
 			continue
 		}
@@ -147,34 +161,53 @@ func (fs *zipVFS) Name() string {
 // 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) {
+func (fs *zipVFS) findOrCreateArchive(ctx context.Context, key 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)
+	archive, expiry, found := fs.cache.GetWithExpiration(key)
 	if found {
-		metrics.ZipCacheRequests.WithLabelValues("archive", "hit").Inc()
+		status, _ := archive.(*zipArchive).openStatus()
+		switch status {
+		case archiveOpening:
+			metrics.ZipCacheRequests.WithLabelValues("archive", "hit-opening").Inc()
 
-		if opened, err := archive.(*zipArchive).openStatus(); opened && err == nil {
+		case archiveOpenError:
+			// this means that archive is likely corrupted
+			// we keep it for duration of cache entry expiry (negative cache)
+			metrics.ZipCacheRequests.WithLabelValues("archive", "hit-open-error").Inc()
+
+		case archiveOpened:
 			if time.Until(expiry) < fs.cacheRefreshInterval {
-				// refresh item that has been opened successfully
-				fs.cache.SetDefault(path, archive)
+				fs.cache.SetDefault(key, archive)
+				metrics.ZipCacheRequests.WithLabelValues("archive", "hit-refresh").Inc()
+			} else {
+				metrics.ZipCacheRequests.WithLabelValues("archive", "hit").Inc()
 			}
+
+		case archiveCorrupted:
+			// this means that archive is likely changed
+			// we should invalidate it immediately
+			metrics.ZipCacheRequests.WithLabelValues("archive", "corrupted").Inc()
+			archive = nil
 		}
-	} else {
-		archive = newArchive(fs, path, fs.openTimeout)
+	}
+
+	if archive == nil {
+		archive = newArchive(fs, fs.openTimeout)
 
 		// 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)
+		fs.cache.Delete(key)
 
 		// 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, fs.cacheExpirationInterval) != nil {
+		if fs.cache.Add(key, archive, fs.cacheExpirationInterval) != nil {
+			metrics.ZipCacheRequests.WithLabelValues("archive", "already-cached").Inc()
 			return nil, errAlreadyCached
 		}
 
@@ -186,13 +219,13 @@ func (fs *zipVFS) findOrCreateArchive(ctx context.Context, path string) (*zipArc
 }
 
 // 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)
+func (fs *zipVFS) findOrOpenArchive(ctx context.Context, key, path string) (*zipArchive, error) {
+	zipArchive, err := fs.findOrCreateArchive(ctx, key)
 	if err != nil {
 		return nil, err
 	}
 
-	err = zipArchive.openArchive(ctx)
+	err = zipArchive.openArchive(ctx, path)
 	if err != nil {
 		return nil, err
 	}