diff --git a/internal/vfs/zip/archive.go b/internal/vfs/zip/archive.go index 47295dc0d714cadea6f562c06c66cb67dfa916ad..df175764412efd61140102c672f79ae4e39dcacf 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 f176a1d66fdfbddbdb9e61c96d475c3cb6338669..3b69d1e9e8c0f93d0494d2ec83c6ed3de2cd6f2c 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 878e015aa3f56b667cc29d3143deb8a41c160c5f..8a5e77a8d67df5df429087238ff1682655687529 100644 --- a/internal/vfs/zip/vfs_test.go +++ b/internal/vfs/zip/vfs_test.go @@ -101,33 +101,125 @@ func TestVFSFindOrOpenArchiveConcurrentAccess(t *testing.T) { }, time.Second, time.Nanosecond) } -func TestVFSArchiveCacheEvict(t *testing.T) { +func TestVFSFindOrOpenArchiveRefresh(t *testing.T) { testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil) defer cleanup() - path := testServerURL + "/public.zip" + // It should be large enough to not have flaky executions + const expiryInterval = 10 * time.Millisecond - vfs := New( - WithCacheExpirationInterval(time.Nanosecond), - ).(*zipVFS) + tests := map[string]struct { + path string + expirationInterval time.Duration + refreshInterval time.Duration - archivesMetric := metrics.ZipCachedEntries.WithLabelValues("archive") - archivesCount := testutil.ToFloat64(archivesMetric) + expectNewArchive bool + expectOpenError bool + expectArchiveRefreshed bool + }{ + "after cache expiry of successful open a new archive is returned": { + path: "/public.zip", + expirationInterval: expiryInterval, + expectNewArchive: true, + expectOpenError: false, + }, + "after cache expiry of errored open a new archive is returned": { + path: "/unknown.zip", + expirationInterval: expiryInterval, + expectNewArchive: true, + expectOpenError: true, + }, + "subsequent open during refresh interval does refresh archive": { + path: "/public.zip", + expirationInterval: time.Second, + refreshInterval: time.Second, // refresh always + expectNewArchive: false, + expectOpenError: false, + expectArchiveRefreshed: true, + }, + "subsequent open before refresh interval does not refresh archive": { + path: "/public.zip", + expirationInterval: time.Second, + refreshInterval: time.Millisecond, // very short interval should not refresh + expectNewArchive: false, + expectOpenError: false, + expectArchiveRefreshed: false, + }, + "subsequent open of errored archive during refresh interval does not refresh": { + path: "/unknown.zip", + expirationInterval: time.Second, + refreshInterval: time.Second, // refresh always (if not error) + expectNewArchive: false, + expectOpenError: true, + expectArchiveRefreshed: false, + }, + } - // create a new archive and increase counters - archive, err := vfs.Root(context.Background(), path) - require.NoError(t, err) - require.NotNil(t, archive) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + withExpectedArchiveCount(t, 1, func(t *testing.T) { + vfs := New( + WithCacheExpirationInterval(test.expirationInterval), + WithCacheRefreshInterval(test.refreshInterval), + ).(*zipVFS) + + path := testServerURL + test.path + + // create a new archive and increase counters + archive1, err1 := vfs.findOrOpenArchive(context.Background(), path) + if test.expectOpenError { + require.Error(t, err1) + require.Nil(t, archive1) + } else { + require.NoError(t, err1) + } + + item1, exp1, found := vfs.cache.GetWithExpiration(path) + require.True(t, found) + + // give some time to for timeouts to fire + time.Sleep(expiryInterval) + + if test.expectNewArchive { + // should return a new archive + archive2, err2 := vfs.findOrOpenArchive(context.Background(), path) + if test.expectOpenError { + require.Error(t, err2) + require.Nil(t, archive2) + } else { + require.NoError(t, err2) + require.NotEqual(t, archive1, archive2, "a new archive should be returned") + } + return + } + + // should return exactly the same archive + archive2, err2 := vfs.findOrOpenArchive(context.Background(), path) + require.Equal(t, archive1, archive2, "same archive is returned") + require.Equal(t, err1, err2, "same error for the same archive") + + item2, exp2, found := vfs.cache.GetWithExpiration(path) + require.True(t, found) + require.Equal(t, item1, item2, "same item is returned") + + if test.expectArchiveRefreshed { + require.Greater(t, exp2.UnixNano(), exp1.UnixNano(), "archive should be refreshed") + } else { + require.Equal(t, exp1.UnixNano(), exp2.UnixNano(), "archive has not been refreshed") + } + }) + }) + } +} - // wait for archive to expire - time.Sleep(time.Nanosecond) +func withExpectedArchiveCount(t *testing.T, archiveCount int, fn func(t *testing.T)) { + t.Helper() - // a new object is created - archive2, err := vfs.Root(context.Background(), path) - require.NoError(t, err) - require.NotNil(t, archive2) - require.NotEqual(t, archive, archive2, "a different archive is returned") + archivesMetric := metrics.ZipCachedEntries.WithLabelValues("archive") + archivesCount := testutil.ToFloat64(archivesMetric) + + fn(t) archivesCountEnd := testutil.ToFloat64(archivesMetric) - require.Equal(t, float64(1), archivesCountEnd-archivesCount, "all expired archives are evicted") + require.Equal(t, float64(archiveCount), archivesCountEnd-archivesCount, "exact number of archives is cached") }