vMCP: Terminate() retains backend connections in the node-local cache until eviction #6762

Bug description

Manager.Terminate() deletes a vMCP session from the data storage but leaves the corresponding entry in the node-local ValidatingCache. That entry holds the session's live backend connections and its copies of the aggregated capability set, so terminated sessions can continue holding memory and backend connections until another eviction trigger occurs.

The Terminate doc comment states the intended reclamation path, and the sentence right after it explains why that path is unreliable:

https://github.com/stacklok/toolhive/blob/5692011cf/pkg/vmcp/server/sessionmanager/session_manager.go#L502-L507

• MultiSession (Phase 2): the storage key is deleted. The node-local cache
  self-heals on the next Get: checkSession detects ErrSessionNotFound,
  evicts the entry, and onEvict closes backend connections. After deletion
  Validate() reports the absent key as (isTerminated=true, nil), so the next
  request — on this or any other replica — is rejected with a definitive 404
  and the client re-initializes instead of retrying the dead session.

Reclamation is deferred to "the next Get" for that session ID. After termination, well-behaved clients generally stop using that session ID and re-initialize with a new one. Consequently, the cache entry may never be accessed again, leaving its backend connections retained until another eviction trigger occurs. checkSession only runs on the cache-hit path, so without a subsequent Get, it never returns ErrExpired and onEvict is never called.

ValidatingCache also offers no way to fix this at the call site: its public API is Get / Set / Len / RemoveMatching. There is no Remove(key), and the only non-test caller of RemoveMatching is EvictStaleSessions, which fires solely when a backend disappears from the registry.

There is no background sweep either, so sessions whose clients disconnect without sending DELETE are affected the same way: the storage record expires on its TTL, but nothing ever touches the cache entry.

That leaves capacity-triggered LRU eviction as the only reclamation path that does not require another access to the terminated session or a backend registry change. The other two are real but conditional: EvictStaleSessions fires only when a backend leaves the registry, and an incidental Get for a terminated ID — a client retrying before it learns the session is gone — would evict it via checkSession. Neither is something a terminated session can rely on, and capacity eviction itself only makes progress while new sessions keep arriving: if traffic stops below the capacity threshold, nothing reclaims the entries at all.

The retained session-cache footprint is bounded by CacheCapacity × per-session footprint, independently of the number of live sessions. CacheCapacity is not configurable through the deployed vMCP configuration: FactoryConfig.CacheCapacity has no non-test caller, so it falls back to defaultCacheCapacity = 1000, and it is not exposed through the VirtualMCPServer CRD, the vmcp config file, or a CLI flag.

Steps to reproduce

1. Deploy a VirtualMCPServer aggregating several backends (ours: 5 backends, 59 tools).
2. Drive normal client traffic that opens sessions and terminates them properly with DELETE /mcp.
3. Watch container memory alongside the session counters in the logs.

A deterministic unit-level repro: create a session via Manager.CreateSession, call Manager.Terminate(sessionID), then observe that the session cache's Len() is unchanged and the session's Close() was never called.

Expected behavior

After a client terminates a session, its backend connections are closed and its memory is released. Retained session-cache entries should track the number of live sessions, rather than cumulative session creations.

Actual behavior

Retained memory tracks the cumulative number of sessions ever created, independently of how many are live. From a production pod (limit 512Mi, 5 backends, 59 aggregated tools):

live stays flat at ~20 across the window while memory keeps climbing with the cumulative count — 90 properly terminated sessions were never reclaimed. The pod is OOMKilled (exit 137) every 8–17 hours depending on load, and the session cache: session evicted from node-local cache debug line never appears, which is consistent with the cache never shrinking below its high-water mark.

A caveat on these numbers: they are container RSS, which also covers Go runtime heap management, GC timing and everything else in the process, so they bound the retention loosely rather than measuring it. An idle pod of the same build sits at 27Mi, which puts the per-session residue somewhere around 2.3Mi, but that should be read as an order of magnitude, not a figure to extrapolate from. I don't have a heap profile to tighten it — vMCP does not register net/http/pprof anywhere, so there is no endpoint to scrape on a stock build.

The code path above establishes the retention mechanism; the observed RSS trend is consistent with it, but does not independently establish causality. A unit-level assertion (terminate N sessions, observe the cache still holds all N and Close() was never called) demonstrates the retention deterministically without depending on RSS at all. I'm happy to add that as part of a fix.

Environment (if relevant)

• OS/version: linux/amd64 (Kubernetes, thv-operator + VirtualMCPServer)
• ToolHive version: v0.51.4 — also present on main @ 5692011cf

Additional context

What each retained cache entry holds (pkg/vmcp/session/default_session.go):

type defaultMultiSession struct {
    transportsession.Session

    connections     map[string]backend.Session  // one open connection per backend
    routingTable    *vmcp.RoutingTable
    tools           []vmcp.Tool                 // advertised tools
    allTools        []vmcp.Tool                 // all resolved tools
    resources       []vmcp.Resource
    prompts         []vmcp.Prompt
    backendSessions map[string]string

    queue AdmissionQueue
}

Configuring spec.sessionStorage with Redis does not help: that swaps Manager.storage, which only ever holds map[string]string metadata. The retained object is Manager.sessions, described in its own field comment as "a node-local cache of live MultiSession objects, separate from storage because MultiSession contains un-serialisable runtime state (HTTP connections, routing tables)" — it stays in-process regardless of the storage backend.

Suggested fix

Two small changes, which I'm happy to open a PR for:

1. Add Remove(key K) bool to ValidatingCache, mirroring RemoveMatching's existing lock discipline (removal under mu, onEvict drained off the lock via drainEvictions). The underlying lru.Cache.Remove is already used internally.
2. Call sm.sessions.Remove(sessionID) after storage.Delete in Terminate's Phase 2 branch. Deleting the storage record first allows the existing loadSession SET XX guard to reject restores that have not yet completed their metadata update. A narrower cache-insertion race remains, as described below. onEvict already closes the backend connections, so no new teardown logic is needed. Other replicas continue to self-heal through checkSession on subsequent cache access.

Scope, and a residual race this does not close

The change above targets the deterministic case — explicitly terminated sessions whose cache entries are already present — and I want to be precise that it does not make termination race-free.

loadSession already guards the main race. It finishes with storage.Update (SET XX) and, on (false, nil), closes the restored session and returns ErrSessionNotFound:

// We use Update (SET XX) rather than Upsert so we never resurrect a key
// that was concurrently deleted (Terminate / TTL expiry).

So a restore whose metadata update runs after Terminate's storage.Delete cannot successfully complete that update and reach the cache.

A narrow window remains between that Update returning true and Get's miss path inserting the value:

v, loadErr := c.load(ctx, key)                            // Update returned true
// <- a Terminate landing entirely here deletes storage, finds the cache
//    empty, and the in-flight Get then inserts a retained entry
if alreadySet, _ := c.lruCache.ContainsOrAdd(key, v); alreadySet {

There is no I/O in that gap, so it is very narrow, and it is pre-existing — this patch neither widens it nor depends on it being closed; the entry it can produce is exactly the kind this issue describes. Closing it would require additional synchronization or validation around cache insertion; I'd rather defer to your preference on the approach than settle on one here.

One note either way: the cache's existing singleflight group is not a candidate on its own, since Do coalesces callers and would hand a Remove the in-flight Get's result without ever running its own function. Happy to take this on as a follow-up, or in the same PR if you'd rather see both together.

Finally, sessions whose clients disconnect without sending DELETE still need a periodic sweep — RemoveMatching is already the right primitive — but that adds a storage read per cached session, so it seems better as its own change too.