Release inner cache read lock when outer lock fails

[v-fedorov-gh]

Why

LayeringWrapper.GetValue could leave a read lock held in the inner cache after the outer cache refused to grant a lock for the same key. When that happened the key stayed locked in the inner cache for the full 60s SentinelTTL, and the caller got back a combined sentinel that looked valid but that SetValue silently discarded, so the fetched value was never actually cached.

The bug

The guard meant to handle this was unreachable:

if lockOnMiss && sentinelInner == NoLockSentinel {

Earlier in the same function lockOnMiss is already forced to false whenever sentinelInner == NoLockSentinel, so the two halves of that condition can never both be true. The check was also looking at the wrong sentinel: what we care about is whether the outer cache granted a lock.

The fix

Check sentinelOuter instead, mirroring the correct logic that GetValues already uses:

if sentinelInner != NoLockSentinel && sentinelOuter == NoLockSentinel {

The extra sentinelInner != NoLockSentinel half is not in the GetValues version. It avoids a pointless Redis round trip when there is no inner sentinel to release, and is otherwise a no-op.

With this, combineSentinels("", "") returns NoLockSentinel, so callers correctly observe that they hold no lock rather than acting on a misleading one.

Test

Adds TestLayeringCache/TestReadLockReleasedWhenOuterLockFails, which pre-locks a key in the outer cache only, then does a layered GetValue(..., lockOnMiss=true). The inner cache grants a read lock, the outer cache refuses, and the test asserts both that the returned sentinel is NoLockSentinel and that the inner key was actually released.

Confirmed it fails on the unfixed code and passes after the change. The full infra/cache suite is green.

Notes for reviewers

These tests need a local Redis (inner on DB 0, outer on DB 1) via tools/start-redis.sh.

This fix came out of a broader concurrency audit of infra/cache. Three other findings are intentionally not addressed here and are worth separate issues:

• WriteSentinel (redis_provider.go) WATCHes only keys[0] while MSET writes all keys, and its MGET runs on c.redisClient rather than the tx, so it is not a consistent snapshot. This defeats the tombstone preservation logic.
• Flush uses a plain pipeline with no WATCH, accumulating Dels across the entire scan, so a tombstone written mid-scan is deleted even when flushTombstones=false (the mode the invalidation handler uses).
• RedisCacheCommunicationProvider.Shutdown sends on an unbuffered channel with a default case, so shutdown can silently no-op and leak the reader goroutine; the PubSub is never closed.