CBG-5020 Avoid double-counting memory correction (#7900)

Author: adamcfraser

db/revision_cache_lru.go

@@ -940,13 +940,10 @@ func (rc *LRURevisionCache) evictBasedOffMemoryUsage(ctx context.Context) int64
 					// list is empty, nothing more to evict but stats are wrong so zero stats and return
 					base.DebugfCtx(ctx, base.KeyCache, "Revision cache memory stats inconsistent for this shard, resetting to zero")
 					correctionVal := rc.currMemoryUsage.Value()
-					// Correct overall memory stat across shards to remove this shards memory usage. We cannot set this
-					// to 0 as other shards usage is included in this
+					// Correct overall memory stat across shards to remove this shards memory usage, and set this shard to zero
 					rc.cacheMemoryBytesStat.Add(-correctionVal)
-					// Set this shards memory usage to zero. We can set this to 0 given this count is for
-					// this particular shard.
 					rc.currMemoryUsage.Set(0)
-					break
+					return numItemsRemoved
 				}
 			}
 			revKey := IDAndRev{DocID: value.id, RevID: value.revID, CollectionID: value.collectionID}

db/revision_cache_test.go

@@ -2447,31 +2447,77 @@ func TestEvictionWhenStaleCVRemoved(t *testing.T) {
 // TestUpdateDeltaRevCacheMemoryStatPanic:
 //   - Test will interleave underlying rev cache operations for UpdateDelta process and Remove item process to reproduce a race between
 //     threads updating a delta (and that thread updating the cache memory stats) and the underlying value being removed/evicted from the cache
-func TestUpdateDeltaRevCacheMemoryStatPanic(t *testing.T) {
+func TestUpdateDeltaRevCacheMemoryStatPanicSingleEntry(t *testing.T) {
 	cacheHitCounter, cacheMissCounter, getDocumentCounter, getRevisionCounter, cacheNumItems, memoryBytesCounted := base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}
 	backingStoreMap := CreateTestSingleBackingStoreMap(&testBackingStore{nil, &getDocumentCounter, &getRevisionCounter}, testCollectionID)
 	cacheOptions := &RevisionCacheOptions{
 		MaxItemCount: 5000,
-		MaxBytes:     125,
+		MaxBytes:     500,
 	}
 	cache := NewLRURevisionCache(cacheOptions, backingStoreMap, &cacheHitCounter, &cacheMissCounter, &cacheNumItems, &memoryBytesCounted)
 
 	firstDelta := bytes.Repeat([]byte("a"), 1000)
 	ctx := base.TestCtx(t)
 
-	// Trigger load into cache
+	// Trigger load into cache.
 	docRev, err := cache.GetWithRev(ctx, "doc1", "1-abc", testCollectionID, RevCacheIncludeDelta)
 	require.NoError(t, err, "Error adding to cache")
 
-	revCacheDelta := newRevCacheDelta(firstDelta, "1-abc", docRev, false, nil)
+	revCacheDelta2 := newRevCacheDelta(firstDelta, "1-abc", docRev, false, nil)
 
 	// Thread 1: UpdateDelta - start
 	value := cache.getValue(ctx, "doc1", "1-abc", testCollectionID, false)
 	if value != nil {
 		// Thread 2: Remove - start - drop value underneath UpdateDelta thread
 		cache.RemoveWithRev(ctx, "doc1", "1-abc", testCollectionID)
 		// Thread 2: Remove - end
-		outGoingBytes := value.updateDelta(revCacheDelta)
+		outGoingBytes := value.updateDelta(revCacheDelta2)
+		if outGoingBytes != 0 {
+			cache.currMemoryUsage.Add(outGoingBytes)
+			cache.cacheMemoryBytesStat.Add(outGoingBytes)
+		}
+		// check for memory based eviction
+		cache.revCacheMemoryBasedEviction(ctx)
+	}
+	// Thread 1: UpdateDelta - end
+
+	assert.Equal(t, 0, cache.lruList.Len())
+	assert.Equal(t, int64(0), memoryBytesCounted.Value())
+}
+
+// TestUpdateDeltaRevCacheMemoryStatPanic:
+//   - Test will interleave underlying rev cache operations for UpdateDelta process and Remove item process to reproduce a race between
+//     threads updating a delta (and that thread updating the cache memory stats) and the underlying value being removed/evicted from the cache
+func TestUpdateDeltaRevCacheMemoryStatPanicMultipleEntries(t *testing.T) {
+	cacheHitCounter, cacheMissCounter, getDocumentCounter, getRevisionCounter, cacheNumItems, memoryBytesCounted := base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}
+	backingStoreMap := CreateTestSingleBackingStoreMap(&testBackingStore{nil, &getDocumentCounter, &getRevisionCounter}, testCollectionID)
+	cacheOptions := &RevisionCacheOptions{
+		MaxItemCount: 5000,
+		MaxBytes:     500,
+	}
+	cache := NewLRURevisionCache(cacheOptions, backingStoreMap, &cacheHitCounter, &cacheMissCounter, &cacheNumItems, &memoryBytesCounted)
+
+	firstDelta := bytes.Repeat([]byte("a"), 1000)
+	ctx := base.TestCtx(t)
+
+	// Trigger load into cache.  Add one revision (doc1) without a delta that's less than 500 bytes, then a
+	// revision (doc2) with a delta update that will exceed memory limit
+	// when added.  Both should be removed and cache memory stats should be zero.
+	_, err := cache.GetWithRev(ctx, "doc1", "1-abc", testCollectionID, RevCacheIncludeDelta)
+	require.NoError(t, err, "Error adding to cache")
+
+	docRev2, err := cache.GetWithRev(ctx, "doc2", "1-abc", testCollectionID, RevCacheIncludeDelta)
+	require.NoError(t, err, "Error adding to cache")
+
+	revCacheDelta2 := newRevCacheDelta(firstDelta, "1-abc", docRev2, false, nil)
+
+	// Thread 1: UpdateDelta - start
+	value := cache.getValue(ctx, "doc2", "1-abc", testCollectionID, false)
+	if value != nil {
+		// Thread 2: Remove - start - drop value underneath UpdateDelta thread
+		cache.RemoveWithRev(ctx, "doc2", "1-abc", testCollectionID)
+		// Thread 2: Remove - end
+		outGoingBytes := value.updateDelta(revCacheDelta2)
 		if outGoingBytes != 0 {
 			cache.currMemoryUsage.Add(outGoingBytes)
 			cache.cacheMemoryBytesStat.Add(outGoingBytes)