memCache.set can spin forever while holding the write lock, blocking all plugin declaration reads #783

Summary

memCache.set in pkg/utils/cache/helper/combined.go can enter an infinite loop while holding the cache write lock. When it does, every code path that reads a plugin declaration blocks permanently, one CPU core is pinned at 100%, and goroutines accumulate without bound until the process is restarted. No error is logged and the process stays "healthy" from the outside.

The file is unchanged since 0e9c3cfb (#757), so this applies to current main.

Affected code

 94  for c.itemSize >= maxMemCacheSize {        // maxMemCacheSize = 1024
 95      var leastKey string
 96      var leastCount int64 = -1
 97      var oldestAccess = time.Now()
 98
 99      for k, v := range c.items {
...
107      }
108
109      if leastKey != "" {
110          c.itemSize--
111          delete(c.items, leastKey)
112      }
113  }
...
116  c.items[key] = &memCacheItem{...}
121  c.itemSize++

Root cause

itemSize is maintained by hand alongside c.items, and line 121 increments it unconditionally — including when set overwrites a key that already exists, where the map length does not change.

That happens routinely under concurrency. CombinedGetPluginDeclaration does a get, and on a miss fetches and calls set. When several goroutines miss on the same key at the same time, each of them calls set: the first inserts, the rest overwrite. Each overwrite adds 1 to itemSize without adding a map entry.

TTL expiry amplifies this into a thundering herd. If N goroutines hit the same expired key concurrently, exactly one wins the double-check in get and decrements (itemSize--, delete); the other N−1 find the key already gone and decrement nothing. All N then call set. Net effect: itemSize gains N−1 while the map gains one entry.

So itemSize − len(c.items) only ever grows, and it grows faster the more concurrent the workload.

Terminal condition

Let D = itemSize − len(c.items). Once itemSize >= maxMemCacheSize, the eviction loop deletes one entry per pass until the map is empty, leaving itemSize == D.

If D >= maxMemCacheSize, the loop cannot exit: the map is empty, so the inner range body never runs, leastKey stays "", nothing is deleted, and itemSize is never decremented. The loop spins forever holding c.Lock().

Because oldestAccess = time.Now() is on line 97, the spin also calls time.Now() millions of times per second.

Observed impact

One goroutine stuck at combined.go:99, unchanged across repeated samples minutes apart:

helper.(*memCache).set                 combined.go:99   [runnable]
helper.CombinedGetPluginDeclaration    combined.go:190
service.InstallMultiplePluginsToTenant
server.(*App).pluginManagementGroup.InstallPluginFromIdentifiers

A 20s CPU profile, with the process otherwise idle:

Duration: 20s, Total samples = 20000ms (100%)
  16330ms 81.65%  time.runtimeNow
   1020ms  5.10%  helper.(*memCache).set     (97.05% cum)
   1460ms  7.30%  runtime.mapIterStart

Meanwhile ~1900 goroutines were blocked on sync.RWMutex.RLock, the oldest for over 48 hours, still growing. Every reader of the declaration cache is affected:

Callers see requests that are accepted and then never answered — the daemon's own access-log middleware never records them, because the gin logger only writes after the handler returns. To an HTTP client this surfaces as Client.Timeout exceeded while awaiting headers, which reads like a network fault and sends you looking in the wrong place.

Restarting clears it, but the counter starts drifting again immediately.

Suggested fix

1. Drop itemSize and use len(c.items). The map already knows its size; the parallel counter is the entire bug. This also fixes the get path, which decrements on a delete that may already have happened.
2. If the counter is kept, make line 121 conditional on the key not already existing, and add if leastKey == "" { break } as a guard so no future counter drift can turn into an infinite loop.
3. Separately, the eviction loop is an O(n) scan per evicted item, so evicting k items is O(n·k) under an exclusive lock. A heap or an LRU list would avoid that, and would keep a full sweep from stalling all readers even when the loop does terminate.