fix(handlers): stale repository cache after rename (25c2a595) · Commits · GitLab.org / container-registry

registry/datastore/mocks/repository.go

+12 −0

Original line number	Diff line number	Diff line
		@@ -99,6 +99,18 @@ func (mr *MockRepositoryCacheMockRecorder) HasSizeWithDescendantsTimedOut(ctx, r
		return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasSizeWithDescendantsTimedOut", reflect.TypeOf((*MockRepositoryCache)(nil).HasSizeWithDescendantsTimedOut), ctx, r)
		}

		// Invalidate mocks base method.
		func (m *MockRepositoryCache) Invalidate(ctx context.Context, path string) {
		m.ctrl.T.Helper()
		m.ctrl.Call(m, "Invalidate", ctx, path)
		}

		// Invalidate indicates an expected call of Invalidate.
		func (mr MockRepositoryCacheMockRecorder) Invalidate(ctx, path any) gomock.Call {
		mr.mock.ctrl.T.Helper()
		return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Invalidate", reflect.TypeOf((*MockRepositoryCache)(nil).Invalidate), ctx, path)
		}

		// InvalidateSize mocks base method.
		func (m MockRepositoryCache) InvalidateSize(ctx context.Context, repo models.Repository) {
		m.ctrl.T.Helper()

registry/datastore/repository.go

+41 −0

Original line number	Diff line number	Diff line
		@@ -245,6 +245,9 @@ type RepositoryCache interface {
		Get(ctx context.Context, path string) *models.Repository
		Set(ctx context.Context, repo *models.Repository)
		InvalidateSize(ctx context.Context, repo *models.Repository)
		// Invalidate removes repository-scoped cached entries for the given repository path, including derived keys.
		// This is intended to prevent stale path->repository mappings when repository paths are reused.
		Invalidate(ctx context.Context, path string)

		SizeWithDescendantsTimedOut(ctx context.Context, r *models.Repository)
		HasSizeWithDescendantsTimedOut(ctx context.Context, r *models.Repository) bool
		@@ -278,6 +281,7 @@ func NewNoOpRepositoryCache() RepositoryCache {
		func (noOpRepositoryCache) Get(context.Context, string) models.Repository { return nil }
		func (noOpRepositoryCache) Set(context.Context, models.Repository) {}
		func (noOpRepositoryCache) InvalidateSize(context.Context, models.Repository) {}
		func (*noOpRepositoryCache) Invalidate(context.Context, string) {}
		func (noOpRepositoryCache) SizeWithDescendantsTimedOut(context.Context, models.Repository) {}
		func (noOpRepositoryCache) HasSizeWithDescendantsTimedOut(context.Context, models.Repository) bool {
		return false
		@@ -327,6 +331,15 @@ func (c singleRepositoryCache) InvalidateSize(_ context.Context, r models.Repo
		}
		}

		func (c *singleRepositoryCache) Invalidate(_ context.Context, path string) {
		if c.r == nil {
		return
		}
		if c.r.Path == path {
		c.r = nil
		}
		}

		// SizeWithDescendantsTimedOut is a noop. We're phasing out the singleRepositoryCache cache implementation in favor of
		// the centralRepositoryCache one, and the only place where we'll be making use of the related functionality (estimated
		// size), the GitLab V1 API repositories handler, is explicitly making use of the latter.
		@@ -475,6 +488,34 @@ func (c centralRepositoryCache) InvalidateSize(ctx context.Context, r models.R
		}
		}

		// Invalidate implements RepositoryCache.
		func (c *centralRepositoryCache) Invalidate(ctx context.Context, path string) {
		if path == "" {
		err := errors.New("can not invalidate an empty path")
		log.GetLogger(log.WithContext(ctx)).WithError(err).Warn("failed to delete repository cache keys")
		errortracking.Capture(err, errortracking.WithContext(ctx), errortracking.WithStackTrace())
		return
		}
		delCtx, cancel := context.WithTimeout(ctx, cacheOpTimeout)
		defer cancel()

		keys := []string{
		c.key(path),
		c.sizeWithDescendantsTimedOutKey(path),
		c.sizeWithDescendantsKey(path),
		c.lsnKey(path),
		}

		// Best-effort: avoid failing the request due to cache cleanup issues.
		if err := c.cache.DeleteMany(delCtx, keys...); err != nil {
		log.GetLogger(log.WithContext(ctx)).WithError(err).WithFields(log.Fields{
		"path": path,
		"keys": keys,
		}).Warn("failed to delete repository cache keys")
		errortracking.Capture(err, errortracking.WithContext(ctx), errortracking.WithStackTrace())
		}
		}

		// sizeWithDescendantsKey generates a valid Redis key string for the cached result of the last "size with descendants"
		// query for a given repository.
		// This flag is stored as a separate key instead of being embedded in the repository struct because we need it to

registry/datastore/repository_test.go

+33 −0

Original line number	Diff line number	Diff line
		@@ -62,6 +62,39 @@ func TestCentralRepositoryCache(t *testing.T) {
		require.NoError(t, redisMock.ExpectationsWereMet())
		}

		func TestCentralRepositoryCache_Invalidate(t *testing.T) {
		ttl := 30 * time.Minute
		redisCache, redisMock := testutil.RedisCacheMock(t, ttl)
		cache := datastore.NewCentralRepositoryCache(redisCache)
		ctx := context.Background()

		repo := &models.Repository{Path: "gitlab-org/gitlab"}
		hex := digest.FromString(repo.Path).Hex()
		base := fmt.Sprintf("registry:db:{repository:%s:%s}", repo.TopLevelPathSegment(), hex)

		keys := []string{
		base,
		base + ":swd-timeout",
		base + ":swd",
		base + ":lsn",
		}

		redisMock.ExpectDel(keys...).SetVal(int64(len(keys)))
		cache.Invalidate(ctx, repo.Path)

		require.NoError(t, redisMock.ExpectationsWereMet())
		}

		func TestCentralRepositoryCache_Invalidate_EmptyPath_NoOp(t *testing.T) {
		ttl := 30 * time.Minute
		redisCache, redisMock := testutil.RedisCacheMock(t, ttl)
		cache := datastore.NewCentralRepositoryCache(redisCache)

		// No expectations: calling Invalidate with an empty path should not touch Redis.
		cache.Invalidate(context.Background(), "")
		require.NoError(t, redisMock.ExpectationsWereMet())
		}

		// Why the SHA1 and not the actual lsnUpdateScript script source: Redis can cache the source of scripts so that clients
		// don't have to re-send the script source with every invocation. Upon a first script EVAL, a script is hashed and then
		// clients can use that SHA1 to invoke the same command with EVALSHA without transmitting its source. The

registry/handlers/repositories.go

+5 −0

Original line number	Diff line number	Diff line
		@@ -1234,6 +1234,11 @@ func handleRenameStoreOperation(ctx context.Context, w http.ResponseWriter, repo
		w.WriteHeader(http.StatusNoContent)
		isRenamed = true

		// Clean up repository cache entries so that old/new paths cannot serve stale data if paths are reused.
		repoCache := datastore.NewCentralRepositoryCache(cache)
		repoCache.Invalidate(ctx, repo.source.Path)
		repoCache.Invalidate(ctx, repo.newPath)

		// When a lease fails to be destroyed after it is no longer needed it should not impact the response to the caller.
		// The lease will eventually expire regardless, but we still need to record these failed cases.
		if err := rlstore.Destroy(ctx, lease); err != nil {

registry/internal/redis/redis.go

+9 −0

Original line number	Diff line number	Diff line
		@@ -104,6 +104,15 @@ func (c *Cache) Delete(ctx context.Context, key string) error {
		return c.cache.Delete(ctx, key)
		}

		// DeleteMany removes multiple cached items by their keys using a single Redis DEL command.
		// This is useful to clean up small, known sets of related keys while avoiding per-key round trips.
		func (c *Cache) DeleteMany(ctx context.Context, keys ...string) error {
		if len(keys) == 0 {
		return nil
		}
		return c.client.Del(ctx, keys...).Err()
		}

		// UnmarshalGet retrieves and unmarshal a cached object into the provided object argument.
		func (c *Cache) UnmarshalGet(ctx context.Context, key string, object any) error {
		_, err := c.marshaler.Get(ctx, key, object)