From 3d7e83504661e892f4828bd8dee6853a2b6cf9e8 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Fri, 7 Aug 2026 15:14:10 +0300 Subject: [PATCH] address review feedback on cache and session store --- management/server/auth/session.go | 2 +- management/server/auth/session_test.go | 10 +++++----- management/server/cache/memory.go | 6 ++++++ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/management/server/auth/session.go b/management/server/auth/session.go index b4c3b612e..778146589 100644 --- a/management/server/auth/session.go +++ b/management/server/auth/session.go @@ -42,7 +42,7 @@ func (s *SessionStore) RegisterToken(ctx context.Context, token string, expiresA key := usedTokenKeyPrefix + hashToken(token) created, err := s.cache.SetNX(ctx, key, usedTokenMarker, ttl) if err != nil { - return fmt.Errorf("failed to store used token entry: %w", err) + return fmt.Errorf("store used token entry: %w", err) } if !created { return ErrTokenAlreadyUsed diff --git a/management/server/auth/session_test.go b/management/server/auth/session_test.go index cdc825e06..7c82dfc43 100644 --- a/management/server/auth/session_test.go +++ b/management/server/auth/session_test.go @@ -64,12 +64,12 @@ func TestSessionStore_ConcurrentRegistrationAllowsOneCaller(t *testing.T) { case errors.Is(err, ErrTokenAlreadyUsed): alreadyUsed++ default: - require.NoError(t, err) + require.NoError(t, err, "concurrent registration returned an unexpected error") } } - assert.Equal(t, 1, succeeded) - assert.Equal(t, attempts-1, alreadyUsed) + assert.Equal(t, 1, succeeded, "exactly one concurrent caller should register the token") + assert.Equal(t, attempts-1, alreadyUsed, "every other caller should be rejected as already used") } func TestSessionStore_RegisterDifferentTokensAreIndependent(t *testing.T) { @@ -119,8 +119,8 @@ func TestSessionStore_CacheErrorIsReturned(t *testing.T) { s := NewSessionStore(failingTokenCache{err: cacheErr}) err := s.RegisterToken(context.Background(), "token", time.Now().Add(time.Hour)) - require.Error(t, err) - assert.ErrorIs(t, err, cacheErr) + require.Error(t, err, "cache failure should be surfaced to the caller") + assert.ErrorIs(t, err, cacheErr, "cache error should be wrapped, not replaced") } func TestHashToken_StableAndDoesNotLeak(t *testing.T) { diff --git a/management/server/cache/memory.go b/management/server/cache/memory.go index 62ab62c74..f140f3ec8 100644 --- a/management/server/cache/memory.go +++ b/management/server/cache/memory.go @@ -33,6 +33,12 @@ func (s *goCacheStore) SetNX(_ context.Context, key, value string, ttl time.Dura return true, nil } +// GetDel reads the value under key and removes it. go-cache has no native read-and-delete +// and releases its own lock between the two calls, so mu holds the pair together and no +// value is consumed twice. +// +// Writes do not take mu: a Set landing mid-pair is lost, since GetDel returns the prior +// value and deletes the new one. Callers must write a consumed key only once. func (s *goCacheStore) GetDel(_ context.Context, key string) (string, bool, error) { s.mu.Lock() defer s.mu.Unlock()