Skip to content

Commit b7886b7

Browse files
JAORMXMecatl
andauthored
Clarify session eviction cleanup guarantees (#6833)
Distinguish eager cache removal from callback completion under shared eviction draining. Correct lock ownership guarantees and stale lazy-eviction test comments after #6807 without changing behavior. Co-authored-by: Mecatl <noreply@mecatl.dev>
1 parent 607cbe8 commit b7886b7

3 files changed

Lines changed: 71 additions & 66 deletions

File tree

‎pkg/cache/validating_cache.go‎

Lines changed: 48 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -38,15 +38,18 @@ type evictedEntry[K comparable, V any] struct {
3838
// Concurrent callers for the same key share the result, coalescing both
3939
// liveness checks and storage round-trips into a single operation per key.
4040
//
41-
// # onEvict runs off the cache lock
41+
// # onEvict runs without cache mutexes held by its invoking goroutine
4242
//
4343
// The user onEvict may perform slow teardown (e.g. closing network
44-
// connections). To keep that work off the shared cache lock, the LRU's own
45-
// eviction callback only *buffers* each evicted entry (bufferEvicted); every
46-
// mutating operation then drains the buffer and invokes the user onEvict via
47-
// drainEvictions after releasing mu. onEvict therefore never runs while mu is
48-
// held, so a slow onEvict never blocks Get/Set/Remove/RemoveMatching on other
49-
// keys.
44+
// connections). The LRU's own eviction callback only *buffers* each evicted
45+
// entry (bufferEvicted). drainEvictions snapshots the buffer under evictMu,
46+
// releases it, then invokes onEvict; direct callbacks on discarded miss-load
47+
// values likewise run without either cache mutex held by their invoking
48+
// goroutine. A drain may claim entries buffered by another operation, so a
49+
// slow callback can delay that drainer's return, including one for a different
50+
// key. Conversely, an operation can return before its own eviction callbacks
51+
// finish if another drainer claimed them. The invoking goroutine holds neither
52+
// cache mutex during callbacks, though other goroutines may hold them.
5053
type ValidatingCache[K comparable, V any] struct {
5154
lruCache *lru.Cache[K, V]
5255
flight singleflight.Group
@@ -65,9 +68,8 @@ type ValidatingCache[K comparable, V any] struct {
6568
// acquired, so it introduces no lock-ordering cycle with mu even though
6669
// bufferEvicted may run while mu is held.
6770
evictMu sync.Mutex
68-
// pending holds entries evicted by the LRU but not yet handed to the user
69-
// onEvict. Drained (and the user onEvict invoked) by drainEvictions after
70-
// mu is released. Always nil when onEvict is nil.
71+
// pending holds entries evicted by the LRU but not yet claimed by
72+
// drainEvictions. Always nil when onEvict is nil.
7173
pending []evictedEntry[K, V]
7274
}
7375

@@ -82,9 +84,9 @@ type ValidatingCache[K comparable, V any] struct {
8284
// key and the cached value so callers can inspect the value without a separate
8385
// read. Returning ErrExpired evicts the entry; any other error is transient
8486
// (cached value returned unchanged). It must not be nil.
85-
// onEvict is called after any eviction (LRU or expiry); it may be nil. It is
86-
// always invoked outside the cache lock (see the type doc), so it is safe for
87-
// onEvict to perform slow teardown without blocking other cache operations.
87+
// onEvict is called after any eviction (LRU or expiry); it may be nil. Its
88+
// invoking goroutine holds neither cache mutex while it runs; see the type doc
89+
// for drain and return-time semantics.
8890
func New[K comparable, V any](
8991
capacity int,
9092
load func(context.Context, K) (V, error),
@@ -109,7 +111,8 @@ func New[K comparable, V any](
109111

110112
// When onEvict is nil there is nothing to defer, so the LRU gets no
111113
// callback and evicted values are simply dropped. Otherwise the LRU
112-
// callback only buffers; drainEvictions runs the real onEvict off the lock.
114+
// callback only buffers; drainEvictions invokes the real callback without a
115+
// cache mutex held by its invoking goroutine.
113116
var lruEvict func(K, V)
114117
if onEvict != nil {
115118
lruEvict = c.bufferEvicted
@@ -124,19 +127,22 @@ func New[K comparable, V any](
124127
}
125128

126129
// bufferEvicted is the LRU's eviction callback. The LRU fires it from Add/Remove
127-
// while the caller may still hold mu (getHit, Set, RemoveMatching), so it must
128-
// not run the user onEvict inline. It records the entry for drainEvictions to
129-
// hand to onEvict after mu is released. Only installed when onEvict is non-nil.
130+
// while the caller may still hold mu (getHit, Set, Remove, RemoveMatching), so it
131+
// must not run the user onEvict inline. It records the entry for a later
132+
// drainEvictions call. Only installed when onEvict is non-nil.
130133
func (c *ValidatingCache[K, V]) bufferEvicted(key K, val V) {
131134
c.evictMu.Lock()
132135
c.pending = append(c.pending, evictedEntry[K, V]{key: key, val: val})
133136
c.evictMu.Unlock()
134137
}
135138

136-
// drainEvictions invokes the user onEvict for every entry buffered since the
137-
// last drain. Callers MUST NOT hold mu, so onEvict runs off the cache lock. It
138-
// is safe to call when onEvict is nil (nothing is ever buffered) and when there
139-
// is nothing pending.
139+
// drainEvictions invokes the user onEvict for every entry it claims from the
140+
// buffer. It snapshots and clears pending while holding evictMu, then releases
141+
// it before invoking callbacks; callers MUST NOT hold mu. Another drain may
142+
// claim entries buffered by this operation, so this call may process entries
143+
// buffered by another operation. See the type doc for the cache-lock invariant.
144+
// It is safe to call when onEvict is nil (nothing is ever buffered) and when
145+
// there is nothing pending.
140146
func (c *ValidatingCache[K, V]) drainEvictions() {
141147
if c.onEvict == nil {
142148
return
@@ -153,8 +159,8 @@ func (c *ValidatingCache[K, V]) drainEvictions() {
153159

154160
// getHit validates a known-present cache entry and returns its value.
155161
// If the entry has definitively expired it is removed (its eviction buffered
156-
// for the caller to drain) and (zero, false) is returned. Transient check
157-
// errors leave the entry in place and return the cached value.
162+
// for a later drain) and (zero, false) is returned. Transient check errors
163+
// leave the entry in place and return the cached value.
158164
func (c *ValidatingCache[K, V]) getHit(ctx context.Context, key K, val V) (V, bool) {
159165
if err := c.check(ctx, key, val); err != nil {
160166
if errors.Is(err, ErrExpired) {
@@ -164,7 +170,7 @@ func (c *ValidatingCache[K, V]) getHit(ctx context.Context, key K, val V) (V, bo
164170
// freshly-written value that the caller intended to keep.
165171
c.mu.Lock()
166172
if current, ok := c.lruCache.Peek(key); ok && sameEntry(current, val) {
167-
// Remove buffers the eviction; Get drains it after mu is released.
173+
// Remove buffers the eviction; Get drains after getHit releases mu.
168174
c.lruCache.Remove(key)
169175
}
170176
c.mu.Unlock()
@@ -221,7 +227,7 @@ func (c *ValidatingCache[K, V]) Get(ctx context.Context, key K) (V, bool) {
221227
if winner, ok := c.lruCache.Get(key); ok {
222228
// Winner confirmed: v is definitively discarded — release its
223229
// resources. v was never stored, so it is not buffered; invoke
224-
// onEvict directly (this runs outside mu, so it is off the lock).
230+
// onEvict directly; the invoking goroutine holds no cache mutex.
225231
if c.onEvict != nil {
226232
c.onEvict(key, v)
227233
}
@@ -235,9 +241,10 @@ func (c *ValidatingCache[K, V]) Get(ctx context.Context, key K) (V, bool) {
235241
return result{v: v}, nil
236242
})
237243
// Drain any evictions buffered by getHit's Remove or the miss path's
238-
// ContainsOrAdd/Add (LRU capacity eviction). mu is not held here, so onEvict
239-
// runs off the lock. Coalesced singleflight waiters also drain; a drain that
240-
// finds nothing pending is a cheap no-op.
244+
// ContainsOrAdd/Add (LRU capacity eviction). The invoking goroutine holds no
245+
// cache mutex while drainEvictions runs; see the type doc. Coalesced
246+
// singleflight waiters also drain; a drain that finds nothing pending is a
247+
// cheap no-op.
241248
c.drainEvictions()
242249
if err != nil {
243250
var zero V
@@ -248,9 +255,9 @@ func (c *ValidatingCache[K, V]) Get(ctx context.Context, key K) (V, bool) {
248255
}
249256

250257
// Set stores value under key, moving the entry to the MRU position. If the
251-
// cache is at capacity, the least-recently-used entry is evicted first and
252-
// onEvict is called for it — after mu is released, so a slow onEvict does not
253-
// block concurrent cache operations.
258+
// cache is at capacity, the least-recently-used entry is evicted first and its
259+
// callback is drained without a cache mutex held by this goroutine; see the
260+
// type doc for return-time semantics.
254261
func (c *ValidatingCache[K, V]) Set(key K, value V) {
255262
c.mu.Lock()
256263
c.lruCache.Add(key, value)
@@ -265,10 +272,9 @@ func (c *ValidatingCache[K, V]) Len() int {
265272

266273
// Remove deletes the entry for key and reports whether one was present.
267274
//
268-
// It mirrors RemoveMatching's lock discipline: the removal runs under mu (which
269-
// buffers the eviction via bufferEvicted) and the user onEvict runs in
270-
// drainEvictions after mu is released, so a slow teardown never blocks
271-
// Get/Set/Remove/RemoveMatching on other keys.
275+
// It mirrors RemoveMatching's lock discipline: removal runs under mu and
276+
// buffers the eviction; drainEvictions runs after this goroutine releases mu.
277+
// The callback and return-time semantics are described in the type doc.
272278
//
273279
// Removing an absent key is a no-op returning false, so callers on a
274280
// termination path may invoke it unconditionally without first probing the
@@ -278,7 +284,7 @@ func (c *ValidatingCache[K, V]) Remove(key K) bool {
278284
removed := c.lruCache.Remove(key) // buffers the eviction; drained below
279285
c.mu.Unlock()
280286

281-
// Run onEvict off the lock, matching RemoveMatching.
287+
// Invoke callbacks after this goroutine releases mu; see the type doc.
282288
c.drainEvictions()
283289
return removed
284290
}
@@ -287,12 +293,11 @@ func (c *ValidatingCache[K, V]) Remove(key K) bool {
287293
// user onEvict for each removed entry, and returns the number removed.
288294
//
289295
// pred runs under the cache lock (the same lock Set contends for) and must not
290-
// call back into the cache. onEvict, however, runs via drainEvictions after all
291-
// removals complete and the lock is released, so a slow onEvict (e.g. closing a
292-
// hung backend connection) delays only the drain, never the shared cache lock:
293-
// a concurrent Set/Get on any key proceeds while a slow teardown is in flight.
294-
// This is intended for infrequent bulk eviction (e.g. reconciling sessions
295-
// after a backend is removed from the registry), not a hot path.
296+
// call back into the cache. This operation calls drainEvictions after removals
297+
// complete and this goroutine releases mu. Another drainer may invoke callbacks
298+
// earlier. See the type doc for callback and return-time semantics. This is
299+
// intended for infrequent bulk eviction (e.g. reconciling sessions after a
300+
// backend is removed from the registry), not a hot path.
296301
func (c *ValidatingCache[K, V]) RemoveMatching(pred func(K, V) bool) int {
297302
// Snapshot the candidate keys under a short lock. Keys() returns a copy and
298303
// Peek does not update recency, so evaluating pred here leaves the LRU order
@@ -318,8 +323,7 @@ func (c *ValidatingCache[K, V]) RemoveMatching(pred func(K, V) bool) int {
318323
}
319324
c.mu.Unlock()
320325
}
321-
// Run onEvict for the removed entries off the lock, so a slow teardown of one
322-
// session never blocks Get/Set for the rest of the node.
326+
// Invoke callbacks after this goroutine releases mu; see the type doc.
323327
c.drainEvictions()
324328
return removed
325329
}

‎pkg/vmcp/server/sessionmanager/session_manager.go‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -500,16 +500,17 @@ func (sm *Manager) Validate(sessionID string) (isTerminated bool, err error) {
500500
// sessions with open backend connections:
501501
//
502502
// - MultiSession (Phase 2): the storage key is deleted and the node-local
503-
// cache entry is removed, which runs onEvict and closes the session's
504-
// backend connections immediately. Storage is deleted first so that a
505-
// loadSession whose metadata Update runs after the delete is rejected by
506-
// the pre-existing SET XX guard; the residual race between that Update
507-
// succeeding and the cache insertion is out of scope here. After deletion
508-
// Validate() reports the absent key as (isTerminated=true, nil), so the
509-
// next request — on this or any other replica — is rejected with a
510-
// definitive 404 and the client re-initializes instead of retrying the dead
511-
// session. Other replicas still self-heal through checkSession on their
512-
// next cache access.
503+
// cache entry is removed, eagerly scheduling onEvict to close the session's
504+
// backend connections. Storage is deleted first so that a loadSession whose
505+
// metadata Update runs after the delete is rejected by the pre-existing SET
506+
// XX guard; the residual race between that Update succeeding and the cache
507+
// insertion is out of scope here. Removal drains evictions, but a concurrent
508+
// operation may claim the callback, so Terminate can return before Close
509+
// finishes. After deletion Validate() reports the absent key as
510+
// (isTerminated=true, nil), so the next request — on this or any other
511+
// replica — is rejected with a definitive 404 and the client re-initializes
512+
// instead of retrying the dead session. Other replicas still self-heal
513+
// through checkSession on their next cache access.
513514
//
514515
// - Placeholder (Phase 1): the session is marked terminated=true and left
515516
// for TTL cleanup. This prevents CreateSession() from opening backend
@@ -539,8 +540,10 @@ func (sm *Manager) Terminate(sessionID string) (isNotAllowed bool, err error) {
539540
}
540541

541542
if _, isFullSession := metadata[sessiontypes.MetadataKeyIdentityBinding]; isFullSession {
542-
// Phase 2 (full MultiSession): delete from storage, then drop the
543-
// node-local cache entry so its backend connections are closed now.
543+
// Phase 2 (full MultiSession): delete from storage, then remove the
544+
// node-local cache entry to eagerly schedule backend cleanup. A
545+
// concurrent eviction drain can claim onEvict, so Terminate need not
546+
// wait for Close to finish.
544547
//
545548
// Order matters. Deleting the storage record first means a loadSession
546549
// whose metadata Update runs after this delete is rejected by the

‎pkg/vmcp/server/sessionmanager/session_manager_test.go‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -604,9 +604,9 @@ func TestSessionManager_Terminate(t *testing.T) {
604604
t.Run("closes MultiSession backend connections", func(t *testing.T) {
605605
t.Parallel()
606606

607-
// After Terminate deletes the session from storage, the next GetMultiSession
608-
// call triggers checkSession → ErrExpired → onEvict → Close(). This verifies
609-
// that backend connections are eventually closed via lazy eviction.
607+
// Terminate eagerly removes the cache entry and, in this serial test,
608+
// onEvict closes its backend connections before Terminate returns. The
609+
// subsequent GetMultiSession verifies the terminated session is unretrievable.
610610
tools := []vmcp.Tool{{Name: "t1", Description: "tool 1"}}
611611
ctrl := gomock.NewController(t)
612612

@@ -632,8 +632,7 @@ func TestSessionManager_Terminate(t *testing.T) {
632632
createdSess.EXPECT().BackendSessions().Return(nil).AnyTimes()
633633
createdSess.EXPECT().GetRoutingTable().Return(nil).AnyTimes()
634634
createdSess.EXPECT().Prompts().Return(nil).AnyTimes()
635-
// Close() is called by onEvict when checkSession detects the session
636-
// is gone from storage on the next GetMultiSession call.
635+
// Close() is called by onEvict when Terminate removes the cache entry.
637636
createdSess.EXPECT().Close().Return(nil).Times(1)
638637
return createdSess, nil
639638
}).Times(1)
@@ -648,18 +647,17 @@ func TestSessionManager_Terminate(t *testing.T) {
648647
require.NoError(t, err)
649648
require.NotNil(t, createdSess)
650649

651-
// CreateSession already persists tokenHashMeta via sess.GetMetadata(),
650+
// CreateSession already persists bindingMeta via sess.GetMetadata(),
652651
// so Terminate will take the Phase 2 path (storage.Delete) without
653652
// any additional seeding.
654653

655-
// Terminate deletes from storage; the cache entry is evicted lazily on
656-
// the next GetMultiSession call when checkSession detects ErrSessionNotFound.
654+
// Terminate deletes from storage and eagerly removes the cache entry,
655+
// invoking onEvict and Close() in this serial test.
657656
isNotAllowed, err := sm.Terminate(sessionID)
658657
require.NoError(t, err)
659658
assert.False(t, isNotAllowed)
660659

661-
// The next GetMultiSession triggers checkSession: storage returns
662-
// ErrSessionNotFound → ErrExpired → onEvict → Close().
660+
// The subsequent GetMultiSession verifies the deleted session is not returned.
663661
_, ok := sm.GetMultiSession(t.Context(), sessionID)
664662
assert.False(t, ok, "terminated session must not be returned")
665663
// gomock verifies Close() was called exactly once via Times(1)

0 commit comments

Comments
 (0)