From d97936d1ed0198fa4dbe67d7ea7b22743701e30c Mon Sep 17 00:00:00 2001 From: Vladimir Fedorov Date: Fri, 4 Sep 2026 18:07:19 -0700 Subject: [PATCH] Release inner cache read lock when outer lock fails LayeringWrapper.GetValue never released the read lock it took in the inner cache when the outer cache refused to grant one for the same key. The guard was unreachable: `lockOnMiss` is already forced to false earlier in the function whenever `sentinelInner == NoLockSentinel`, so `lockOnMiss && sentinelInner == NoLockSentinel` could never be true. Check `sentinelOuter` instead, mirroring GetValues, and skip the release round trip when there is no inner sentinel to release. Without this, the inner key stayed locked for the full 60s SentinelTTL and the caller got back a combined sentinel that looked valid but that SetValue silently discarded, so the value was never cached. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: efdc36a8-3708-4c0a-977c-70e78d179640 --- infra/cache/cache_layering_wrapper.go | 2 +- infra/cache/cache_layering_wrapper_test.go | 29 ++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/infra/cache/cache_layering_wrapper.go b/infra/cache/cache_layering_wrapper.go index ca94bdc..90eb1dd 100644 --- a/infra/cache/cache_layering_wrapper.go +++ b/infra/cache/cache_layering_wrapper.go @@ -84,7 +84,7 @@ func (l *LayeringWrapper) GetValue(ctx context.Context, key Key, lockOnMiss bool } // If we didn't get a lock in the cache further away, release the lock we got in the cache closer to the data consumer - if lockOnMiss && sentinelInner == NoLockSentinel { + if sentinelInner != NoLockSentinel && sentinelOuter == NoLockSentinel { l.cacheInner.ReleaseSentinel(ctx, []Key{key}, sentinelInner) sentinelInner = NoLockSentinel } diff --git a/infra/cache/cache_layering_wrapper_test.go b/infra/cache/cache_layering_wrapper_test.go index ce68dc1..69e3617 100644 --- a/infra/cache/cache_layering_wrapper_test.go +++ b/infra/cache/cache_layering_wrapper_test.go @@ -4,6 +4,8 @@ import ( "context" "testing" + "github.com/gofrs/uuid" + "userclouds.com/infra/assert" ) @@ -127,4 +129,31 @@ func TestLayeringCache(t *testing.T) { lcp := getLayeringCacheProvider(t, "") testSupportedRateLimitsSingleThreaded(ctx, t, lcp) }) + + t.Run("TestReadLockReleasedWhenOuterLockFails", func(t *testing.T) { + t.Parallel() + lcp := getLayeringCacheProvider(t, "") + testReadLockReleasedWhenOuterLockFails(ctx, t, lcp) + }) +} + +// testReadLockReleasedWhenOuterLockFails validates that a read lock taken in the inner cache is released when +// the outer cache refuses to grant a lock for the same key. Otherwise the inner cache key stays locked until the +// sentinel expires, blocking reads of that key, and the caller gets back a sentinel that it can't use to set a value. +func testReadLockReleasedWhenOuterLockFails(ctx context.Context, t *testing.T, l *LayeringWrapper) { + key := Key(uuid.Must(uuid.NewV4()).String()) + + // Take a read lock on the key in the outer cache only, so the outer cache refuses to grant a lock for it below + _, _, outerSentinel, _, err := l.cacheOuter.GetValue(ctx, key, true) + assert.NoErr(t, err) + assert.NotEqual(t, outerSentinel, NoLockSentinel, assert.Must(), assert.Errorf("Expected to lock key %v in the outer cache", key)) + + // The key is missing from the inner cache, so this read takes a lock there, but it can't take one in the outer cache + val, _, sentinel, _, err := l.GetValue(ctx, key, true) + assert.NoErr(t, err) + assert.IsNil(t, val, assert.Errorf("Expected a miss on key %v in both caches", key)) + + // Since we couldn't lock the key in both caches, we don't hold a usable lock and the inner cache lock must be released + assert.Equal(t, sentinel, NoLockSentinel, assert.Errorf("Expected no lock on key %v after failing to lock the outer cache", key)) + validateKeyContents(t, l.cacheInner, string(key), "", false, false) }