Merge branch '281-dont-replace-good-lookups-with-errored' into 'master'

Do not replace entries in cache when lookup has errored

See merge request gitlab-org/gitlab-pages!435

4 files changed, 242 insertions, 42 deletions

diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go
index 77291033..71b8e745 100644
--- a/internal/source/gitlab/cache/cache.go
+++ b/internal/source/gitlab/cache/cache.go
@@ -80,14 +80,14 @@ func (c *Cache) Resolve(ctx context.Context, domain string) *api.Lookup {
 	}
 
 	if entry.NeedsRefresh() {
-		entry.Refresh(c.client, c.store)
+		entry.Refresh(c.store)
 
 		metrics.DomainsSourceCacheHit.Inc()
 		return entry.Lookup()
 	}
 
 	metrics.DomainsSourceCacheMiss.Inc()
-	return entry.Retrieve(ctx, c.client)
+	return entry.Retrieve(ctx)
 }
 
 // Status calls the client Status to check connectivity with the API
diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go
index c960be8a..0b980774 100644
--- a/internal/source/gitlab/cache/entry.go
+++ b/internal/source/gitlab/cache/entry.go
@@ -3,9 +3,11 @@ package cache
 import (
 	"context"
 	"errors"
+	"os"
 	"sync"
 	"time"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 )
 
@@ -13,27 +15,30 @@ import (
 // holds a pointer to *api.Lookup when the domain lookup has been retrieved
 // successfully
 type Entry struct {
-	domain         string
-	created        time.Time
-	retrieve       *sync.Once
-	refresh        *sync.Once
-	mux            *sync.RWMutex
-	retrieved      chan struct{}
-	response       *api.Lookup
-	refreshTimeout time.Duration
-	retriever      *Retriever
+	domain                     string
+	created                    time.Time
+	refreshedOriginalTimestamp time.Time
+	retrieve                   *sync.Once
+	refresh                    *sync.Once
+	mux                        *sync.RWMutex
+	retrieved                  chan struct{}
+	response                   *api.Lookup
+	refreshTimeout             time.Duration
+	expirationTimeout          time.Duration
+	retriever                  *Retriever
 }
 
-func newCacheEntry(domain string, refreshTimeout time.Duration, retriever *Retriever) *Entry {
+func newCacheEntry(domain string, refreshTimeout, entryExpirationTimeout time.Duration, retriever *Retriever) *Entry {
 	return &Entry{
-		domain:         domain,
-		created:        time.Now(),
-		retrieve:       &sync.Once{},
-		refresh:        &sync.Once{},
-		mux:            &sync.RWMutex{},
-		retrieved:      make(chan struct{}),
-		refreshTimeout: refreshTimeout,
-		retriever:      retriever,
+		domain:            domain,
+		created:           time.Now(),
+		retrieve:          &sync.Once{},
+		refresh:           &sync.Once{},
+		mux:               &sync.RWMutex{},
+		retrieved:         make(chan struct{}),
+		refreshTimeout:    refreshTimeout,
+		expirationTimeout: entryExpirationTimeout,
+		retriever:         retriever,
 	}
 }
 
@@ -43,7 +48,7 @@ func (e *Entry) IsUpToDate() bool {
 	e.mux.RLock()
 	defer e.mux.RUnlock()
 
-	return e.isResolved() && !e.isExpired()
+	return e.isResolved() && !e.isOutdated()
 }
 
 // NeedsRefresh return true if the entry has been resolved correctly but it has
@@ -52,7 +57,7 @@ func (e *Entry) NeedsRefresh() bool {
 	e.mux.RLock()
 	defer e.mux.RUnlock()
 
-	return e.isResolved() && e.isExpired()
+	return e.isResolved() && e.isOutdated()
 }
 
 // Lookup returns a retriever Lookup response.
@@ -64,7 +69,7 @@ func (e *Entry) Lookup() *api.Lookup {
 }
 
 // Retrieve perform a blocking retrieval of the cache entry response.
-func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lookup) {
+func (e *Entry) Retrieve(ctx context.Context) (lookup *api.Lookup) {
 	// We run the code within an additional func() to run both `e.setResponse`
 	// and `e.retrieve.Retrieve` asynchronously.
 	e.retrieve.Do(func() { go func() { e.setResponse(e.retriever.Retrieve(e.domain)) }() })
@@ -79,17 +84,30 @@ func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lo
 	return lookup
 }
 
-// Refresh will update the entry in the store only when it gets resolved.
-func (e *Entry) Refresh(client api.Client, store Store) {
+// Refresh will update the entry in the store only when it gets resolved successfully.
+// If an existing successful entry exists, it will only be replaced if the new resolved
+// entry is successful too.
+// Errored refreshed Entry responses will not replace the previously successful entry.response
+// for a maximum time of e.expirationTimeout.
+func (e *Entry) Refresh(store Store) {
 	e.refresh.Do(func() {
-		go func() {
-			entry := newCacheEntry(e.domain, e.refreshTimeout, e.retriever)
+		go e.refreshFunc(store)
+	})
+}
 
-			entry.Retrieve(context.Background(), client)
+func (e *Entry) refreshFunc(store Store) {
+	entry := newCacheEntry(e.domain, e.refreshTimeout, e.expirationTimeout, e.retriever)
 
-			store.ReplaceOrCreate(e.domain, entry)
-		}()
-	})
+	entry.Retrieve(context.Background())
+
+	// do not replace existing Entry `e.response` when `entry.response` has an error
+	// and `e` has not expired. See https://gitlab.com/gitlab-org/gitlab-pages/-/issues/281.
+	if !e.isExpired() && entry.hasTemporaryError() {
+		entry.response = e.response
+		entry.refreshedOriginalTimestamp = e.created
+	}
+
+	store.ReplaceOrCreate(e.domain, entry)
 }
 
 func (e *Entry) setResponse(lookup api.Lookup) {
@@ -100,10 +118,39 @@ func (e *Entry) setResponse(lookup api.Lookup) {
 	close(e.retrieved)
 }
 
-func (e *Entry) isExpired() bool {
+func (e *Entry) isOutdated() bool {
+	if !e.refreshedOriginalTimestamp.IsZero() {
+		return time.Since(e.refreshedOriginalTimestamp) > e.refreshTimeout
+	}
+
 	return time.Since(e.created) > e.refreshTimeout
 }
 
 func (e *Entry) isResolved() bool {
 	return e.response != nil
 }
+
+func (e *Entry) isExpired() bool {
+	if !e.refreshedOriginalTimestamp.IsZero() {
+		return time.Since(e.refreshedOriginalTimestamp) > e.expirationTimeout
+	}
+
+	return time.Since(e.created) > e.expirationTimeout
+}
+
+func (e *Entry) domainExists() bool {
+	return !errors.Is(e.response.Error, domain.ErrDomainDoesNotExist)
+}
+
+// hasTemporaryError checks currently refreshed entry for errors after resolving the lookup again
+// and is different to domain.ErrDomainDoesNotExist (this is an edge case to prevent serving
+// a page right after being deleted).
+func (e *Entry) hasTemporaryError() bool {
+	if os.Getenv("FF_DISABLE_REFRESH_TEMPORARY_ERROR") == "true" {
+		return false
+	}
+
+	return e.response != nil &&
+		e.response.Error != nil &&
+		e.domainExists()
+}
diff --git a/internal/source/gitlab/cache/entry_test.go b/internal/source/gitlab/cache/entry_test.go
index 36aee522..0ccbeed2 100644
--- a/internal/source/gitlab/cache/entry_test.go
+++ b/internal/source/gitlab/cache/entry_test.go
@@ -1,11 +1,17 @@
 package cache
 
 import (
+	"context"
+	"errors"
+	"net/http"
+	"os"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/require"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 )
 
@@ -49,7 +55,7 @@ func TestIsUpToDateAndNeedsRefresh(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			entry := newCacheEntry("my.gitlab.com", testCacheConfig.EntryRefreshTimeout, nil)
+			entry := newCacheEntry("my.gitlab.com", testCacheConfig.EntryRefreshTimeout, testCacheConfig.CacheExpiry, nil)
 			if tt.resolved {
 				entry.response = &api.Lookup{}
 			}
@@ -62,3 +68,148 @@ func TestIsUpToDateAndNeedsRefresh(t *testing.T) {
 		})
 	}
 }
+
+func TestEntryRefresh(t *testing.T) {
+	client := &lookupMock{
+		successCount: 1,
+		responses: map[string]api.Lookup{
+			"test.gitlab.io": api.Lookup{
+				Name: "test.gitlab.io",
+				Domain: &api.VirtualDomain{
+					LookupPaths: nil,
+				},
+			},
+			"error.gitlab.io": api.Lookup{
+				Name:  "error.gitlab.io",
+				Error: errors.New("something went wrong"),
+			},
+		},
+	}
+	cc := &config.Cache{
+		CacheExpiry:          100 * time.Millisecond,
+		EntryRefreshTimeout:  time.Millisecond,
+		RetrievalTimeout:     50 * time.Millisecond,
+		MaxRetrievalInterval: time.Millisecond,
+		MaxRetrievalRetries:  1,
+	}
+
+	store := newMemStore(client, cc)
+
+	t.Run("entry is the same after refreshed lookup has error", func(t *testing.T) {
+		entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever)
+		originalEntryCreated := entry.created
+
+		ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout)
+		defer cancel()
+
+		lookup := entry.Retrieve(ctx)
+		require.NoError(t, lookup.Error)
+
+		require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh")
+
+		entry.refreshFunc(store)
+
+		require.True(t, client.failed, "refresh should have failed")
+
+		storedEntry := loadEntry(t, "test.gitlab.io", store)
+
+		require.NoError(t, storedEntry.Lookup().Error, "resolving failed but lookup should still be valid")
+		require.Equal(t, storedEntry.refreshedOriginalTimestamp.UnixNano(), originalEntryCreated.UnixNano(),
+			"refreshed entry timestamp should be the same as the original entry created timestamp")
+		require.Equal(t, storedEntry.Lookup(), entry.Lookup(), "lookup should be the same")
+	})
+
+	t.Run("entry is the different when FF_DISABLE_REFRESH_TEMPORARY_ERROR is set to true", func(t *testing.T) {
+		client.failed = false
+		err := os.Setenv("FF_DISABLE_REFRESH_TEMPORARY_ERROR", "true")
+
+		entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever)
+
+		ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout)
+		defer cancel()
+
+		lookup := entry.Retrieve(ctx)
+		require.NoError(t, lookup.Error)
+
+		require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh")
+
+		require.NoError(t, err)
+
+		entry.refreshFunc(store)
+
+		require.True(t, client.failed, "refresh should have failed")
+
+		storedEntry := loadEntry(t, "test.gitlab.io", store)
+
+		require.Error(t, storedEntry.Lookup().Error, "resolving failed")
+		require.True(t, storedEntry.refreshedOriginalTimestamp.IsZero())
+		require.NotEqual(t, storedEntry.Lookup(), entry.Lookup(), "lookup should be different")
+	})
+
+	t.Run("entry is different after it expired and calling refresh on it", func(t *testing.T) {
+		client.failed = false
+
+		entry := newCacheEntry("error.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever)
+
+		ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout)
+		defer cancel()
+
+		lookup := entry.Retrieve(ctx)
+		require.Error(t, lookup.Error)
+		require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh")
+
+		// wait for entry to expire
+		time.Sleep(cc.CacheExpiry)
+		// refreshing the entry after it has expired should create a completely new one
+		entry.refreshFunc(store)
+
+		require.True(t, client.failed, "refresh should have failed")
+
+		storedEntry := loadEntry(t, "error.gitlab.io", store)
+		require.NotEqual(t, storedEntry, entry, "stored entry should be different")
+		require.Greater(t, storedEntry.created.UnixNano(), entry.created.UnixNano(), "")
+	})
+}
+
+func loadEntry(t *testing.T, domain string, store Store) *Entry {
+	t.Helper()
+
+	i, exists := store.(*memstore).store.Get(domain)
+	require.True(t, exists)
+
+	return i.(*Entry)
+}
+
+type lookupMock struct {
+	currentCount int
+	successCount int
+	failed       bool
+	responses    map[string]api.Lookup
+}
+
+func (lm *lookupMock) GetLookup(ctx context.Context, domainName string) api.Lookup {
+	lookup := api.Lookup{
+		Name: domainName,
+	}
+
+	lookup, ok := lm.responses[domainName]
+	if !ok {
+		lookup.Error = domain.ErrDomainDoesNotExist
+		return lookup
+	}
+
+	// return error after lm.successCount
+	lm.currentCount++
+	if lm.currentCount > lm.successCount {
+		lm.currentCount = 0
+		lm.failed = true
+
+		lookup.Error = http.ErrServerClosed
+	}
+
+	return lookup
+}
+
+func (lm *lookupMock) Status() error {
+	return nil
+}
diff --git a/internal/source/gitlab/cache/memstore.go b/internal/source/gitlab/cache/memstore.go
index 0bf9bb79..8ba069e4 100644
--- a/internal/source/gitlab/cache/memstore.go
+++ b/internal/source/gitlab/cache/memstore.go
@@ -11,20 +11,22 @@ import (
 )
 
 type memstore struct {
-	store               *cache.Cache
-	mux                 *sync.RWMutex
-	retriever           *Retriever
-	entryRefreshTimeout time.Duration
+	store                  *cache.Cache
+	mux                    *sync.RWMutex
+	retriever              *Retriever
+	entryRefreshTimeout    time.Duration
+	entryExpirationTimeout time.Duration
 }
 
 func newMemStore(client api.Client, cc *config.Cache) Store {
 	retriever := NewRetriever(client, cc.RetrievalTimeout, cc.MaxRetrievalInterval, cc.MaxRetrievalRetries)
 
 	return &memstore{
-		store:               cache.New(cc.CacheExpiry, cc.CacheCleanupInterval),
-		mux:                 &sync.RWMutex{},
-		retriever:           retriever,
-		entryRefreshTimeout: cc.EntryRefreshTimeout,
+		store:                  cache.New(cc.CacheExpiry, cc.CacheCleanupInterval),
+		mux:                    &sync.RWMutex{},
+		retriever:              retriever,
+		entryRefreshTimeout:    cc.EntryRefreshTimeout,
+		entryExpirationTimeout: cc.CacheExpiry,
 	}
 }
 
@@ -46,7 +48,7 @@ func (m *memstore) LoadOrCreate(domain string) *Entry {
 		return entry.(*Entry)
 	}
 
-	newEntry := newCacheEntry(domain, m.entryRefreshTimeout, m.retriever)
+	newEntry := newCacheEntry(domain, m.entryRefreshTimeout, m.entryExpirationTimeout, m.retriever)
 	m.store.SetDefault(domain, newEntry)
 
 	return newEntry