Do not refresh errored archives

Remove to-do and update test

3 files changed, 56 insertions, 9 deletions

diff --git a/internal/vfs/zip/archive.go b/internal/vfs/zip/archive.go
index 47295dc0..df175764 100644
--- a/internal/vfs/zip/archive.go
+++ b/internal/vfs/zip/archive.go
@@ -68,11 +68,8 @@ func newArchive(fs *zipVFS, path string, openTimeout time.Duration) *zipArchive
 
 func (a *zipArchive) openArchive(parentCtx context.Context) (err error) {
 	// return early if openArchive was done already in a concurrent request
-	select {
-	case <-a.done:
-		return a.err
-
-	default:
+	if ok, err := a.openStatus(); ok {
+		return err
 	}
 
 	ctx, cancel := context.WithTimeout(parentCtx, a.openTimeout)
@@ -283,3 +280,13 @@ func (a *zipArchive) Readlink(ctx context.Context, name string) (string, error)
 func (a *zipArchive) onEvicted() {
 	metrics.ZipArchiveEntriesCached.Sub(float64(len(a.files)))
 }
+
+func (a *zipArchive) openStatus() (bool, error) {
+	select {
+	case <-a.done:
+		return true, a.err
+
+	default:
+		return false, nil
+	}
+}
diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go
index f176a1d6..3b69d1e9 100644
--- a/internal/vfs/zip/vfs.go
+++ b/internal/vfs/zip/vfs.go
@@ -158,10 +158,11 @@ func (fs *zipVFS) findOrCreateArchive(ctx context.Context, path string) (*zipArc
 	if found {
 		metrics.ZipCacheRequests.WithLabelValues("archive", "hit").Inc()
 
-		// TODO: do not refreshed errored archives https://gitlab.com/gitlab-org/gitlab-pages/-/merge_requests/351
-		if time.Until(expiry) < fs.cacheRefreshInterval {
-			// refresh item
-			fs.cache.SetDefault(path, archive)
+		if opened, err := archive.(*zipArchive).openStatus(); opened && err == nil {
+			if time.Until(expiry) < fs.cacheRefreshInterval {
+				// refresh item that has been opened successfully
+				fs.cache.SetDefault(path, archive)
+			}
 		}
 	} else {
 		archive = newArchive(fs, path, fs.openTimeout)
diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go
index 878e015a..2c3e8949 100644
--- a/internal/vfs/zip/vfs_test.go
+++ b/internal/vfs/zip/vfs_test.go
@@ -131,3 +131,42 @@ func TestVFSArchiveCacheEvict(t *testing.T) {
 	archivesCountEnd := testutil.ToFloat64(archivesMetric)
 	require.Equal(t, float64(1), archivesCountEnd-archivesCount, "all expired archives are evicted")
 }
+
+func TestVFSFindOrCreateArchiveErrored(t *testing.T) {
+	testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil)
+	defer cleanup()
+
+	path := testServerURL + "/unknown.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.Error(t, err)
+	require.Nil(t, archive)
+
+	item, exp1, found := vfs.cache.GetWithExpiration(path)
+	require.True(t, found)
+
+	// item has been opened and has an error
+	opened, err1 := item.(*zipArchive).openStatus()
+	require.True(t, opened)
+	require.Error(t, err1)
+	require.Equal(t, err, err1, "same error as findOrOpenArchive")
+
+	// should return errored archive
+	archive2, err2 := vfs.findOrOpenArchive(context.Background(), path)
+	require.Error(t, err2)
+	require.Nil(t, archive2)
+	require.Equal(t, err1, err2, "same error for the same archive")
+
+	_, exp2, found := vfs.cache.GetWithExpiration(path)
+	require.True(t, found)
+	require.Equal(t, exp1, exp2, "archive has not been refreshed")
+
+	archivesCountEnd := testutil.ToFloat64(archivesMetric)
+	require.Equal(t, float64(1), archivesCountEnd-archivesCount, "only one archive cached")
+}