Merge pull request #99 from pior/memleak

Fix single-item cache bug

Author: karlseguin

cache.go

@@ -331,7 +331,7 @@ doAllDeletes:
 }
 
 func (c *Cache[T]) doDelete(item *Item[T]) {
-	if item.next == nil && item.prev == nil {
+	if !item.inList {
 		item.promotions = -2
 	} else {
 		c.size -= item.size
@@ -344,12 +344,12 @@ func (c *Cache[T]) doDelete(item *Item[T]) {
 }
 
 func (c *Cache[T]) doPromote(item *Item[T]) bool {
-	//already deleted
+	// already deleted
 	if item.promotions == -2 {
 		return false
 	}
 
-	if item.next != nil || item.prev != nil { // not a new item
+	if item.inList {
 		if item.shouldPromote(c.getsPerPromote) {
 			c.list.MoveToFront(item)
 			item.promotions = 0

cache_test.go

@@ -150,6 +150,49 @@ func Test_CacheOnDeleteCallbackCalled(t *testing.T) {
 	assert.Equal(t, atomic.LoadInt32(&onDeleteFnCalled), 1)
 }
 
+func Test_CacheSingleItemSizeAccounting(t *testing.T) {
+	cache := New(Configure[*SizedItem]().GetsPerPromote(1))
+	defer cache.Stop()
+
+	// Add a single item to the cache
+	cache.Set("only", &SizedItem{1, 10}, time.Minute)
+	cache.SyncUpdates()
+	assert.Equal(t, cache.GetSize(), 10)
+
+	// Get the item to trigger promotion
+	// With a single item, next==nil && prev==nil, so it may be
+	// incorrectly treated as a "new" item and size double-counted
+	cache.Get("only")
+	cache.Get("only")
+	cache.SyncUpdates()
+	assert.Equal(t, cache.GetSize(), 10) // Should still be 10, not 20 or 30
+}
+
+func Test_CacheSingleItemDelete(t *testing.T) {
+	onDeleteCalled := int32(0)
+	onDeleteFn := func(item *Item[string]) {
+		atomic.AddInt32(&onDeleteCalled, 1)
+	}
+
+	cache := New(Configure[string]().OnDelete(onDeleteFn))
+	defer cache.Stop()
+
+	// Add a single item
+	cache.Set("only", "value", time.Minute)
+	cache.SyncUpdates()
+	assert.Equal(t, cache.GetSize(), 1)
+
+	// Delete the only item in the cache
+	// With a single item, next==nil && prev==nil, so the delete
+	// path may incorrectly skip size decrement and onDelete callback
+	cache.Delete("only")
+	cache.SyncUpdates()
+
+	assert.Equal(t, cache.Get("only"), nil)
+	assert.Equal(t, atomic.LoadInt32(&onDeleteCalled), 1) // onDelete should be called
+	assert.Equal(t, cache.GetSize(), 0)                   // Size should be 0
+}
+
 func Test_CacheFetchesExpiredItems(t *testing.T) {
 	cache := New(Configure[string]())
 	defer cache.Stop()

item.go

@@ -29,6 +29,7 @@ type Item[T any] struct {
 	value      T
 	next       *Item[T]
 	prev       *Item[T]
+	inList     bool
 }
 
 func newItem[T any](key string, value T, expires int64, track bool) *Item[T] {

layeredcache.go

@@ -293,7 +293,7 @@ drain:
 }
 
 func (c *LayeredCache[T]) doDelete(item *Item[T]) {
-	if item.prev == nil && item.next == nil {
+	if !item.inList {
 		item.promotions = -2
 	} else {
 		c.size -= item.size
@@ -311,7 +311,7 @@ func (c *LayeredCache[T]) doPromote(item *Item[T]) bool {
 		return false
 	}
 
-	if item.next != nil || item.prev != nil { // not a new item
+	if item.inList {
 		if item.shouldPromote(c.getsPerPromote) {
 			c.list.MoveToFront(item)
 			item.promotions = 0

layeredcache_test.go

@@ -163,6 +163,41 @@ func Test_LayedCache_OnDeleteCallbackCalled(t *testing.T) {
 	assert.Equal(t, atomic.LoadInt32(&onDeleteFnCalled), 1)
 }
 
+func Test_LayeredCache_SingleItemSizeAccounting(t *testing.T) {
+	cache := Layered(Configure[*SizedItem]().GetsPerPromote(1))
+	defer cache.Stop()
+
+	cache.Set("group", "only", &SizedItem{1, 10}, time.Minute)
+	cache.SyncUpdates()
+	assert.Equal(t, cache.GetSize(), 10)
+
+	cache.Get("group", "only")
+	cache.Get("group", "only")
+	cache.SyncUpdates()
+	assert.Equal(t, cache.GetSize(), 10)
+}
+
+func Test_LayeredCache_SingleItemDelete(t *testing.T) {
+	onDeleteCalled := int32(0)
+	onDeleteFn := func(item *Item[string]) {
+		atomic.AddInt32(&onDeleteCalled, 1)
+	}
+
+	cache := Layered(Configure[string]().OnDelete(onDeleteFn))
+	defer cache.Stop()
+
+	cache.Set("group", "only", "value", time.Minute)
+	cache.SyncUpdates()
+	assert.Equal(t, cache.GetSize(), 1)
+
+	cache.Delete("group", "only")
+	cache.SyncUpdates()
+
+	assert.Equal(t, cache.Get("group", "only"), nil)
+	assert.Equal(t, atomic.LoadInt32(&onDeleteCalled), 1)
+	assert.Equal(t, cache.GetSize(), 0)
+}
+
 func Test_LayedCache_DeletesALayer(t *testing.T) {
 	cache := newLayered[string]()
 	defer cache.Stop()

list.go

@@ -26,6 +26,7 @@ func (l *List[T]) Remove(item *Item[T]) {
 	}
 	item.next = nil
 	item.prev = nil
+	item.inList = false
 }
 
 func (l *List[T]) MoveToFront(item *Item[T]) {
@@ -36,6 +37,7 @@ func (l *List[T]) MoveToFront(item *Item[T]) {
 func (l *List[T]) Insert(item *Item[T]) {
 	head := l.Head
 	l.Head = item
+	item.inList = true
 	if head == nil {
 		l.Tail = item
 		return