CBG-4879: fix for data race when invalidating cache for local wins

db/change_cache.go

@@ -512,7 +512,7 @@ func (c *changeCache) DocChanged(event sgbucket.FeedEvent, docType DocumentType)
 			SourceID: syncData.RevAndVersion.CurrentSource,
 			Value:    base.HexCasToUint64(syncData.RevAndVersion.CurrentVersion),
 		}
-		collection.revisionCache.RemoveWithCV(ctx, docID, &vrs)
+		collection.revisionCache.RemoveCVOnly(ctx, docID, &vrs)
 	}
 
 	change := &LogEntry{

db/revision_cache_bypass.go

@@ -132,6 +132,10 @@ func (rc *BypassRevisionCache) RemoveRevOnly(ctx context.Context, docID, revID s
 	// no-op
 }
 
+func (rc *BypassRevisionCache) RemoveCVOnly(ctx context.Context, docID string, cv *Version, collectionID uint32) {
+	// no-op
+}
+
 func (rc *BypassRevisionCache) RemoveWithCV(ctx context.Context, docID string, cv *Version, collectionID uint32) {
 	// no-op
 }

db/revision_cache_interface.go

@@ -55,8 +55,12 @@ type RevisionCache interface {
 	// RemoveWithCV evicts a revision from the cache using its current version.
 	RemoveWithCV(ctx context.Context, docID string, cv *Version, collectionID uint32)
 
+	// RemoveRevOnly removes the specified key from the revID lookup map in the cache
 	RemoveRevOnly(ctx context.Context, docID, revID string, collectionID uint32)
 
+	// RemoveCVOnly removes the specified key from the HLV lookup map in the cache
+	RemoveCVOnly(ctx context.Context, docID string, cv *Version, collectionID uint32)
+
 	// UpdateDelta stores the given toDelta value in the given rev if cached
 	UpdateDelta(ctx context.Context, docID, revID string, collectionID uint32, toDelta RevisionDelta)
 
@@ -178,6 +182,10 @@ func (c *collectionRevisionCache) RemoveRevOnly(ctx context.Context, docID, revI
 	(*c.revCache).RemoveRevOnly(ctx, docID, revID, c.collectionID)
 }
 
+func (c *collectionRevisionCache) RemoveCVOnly(ctx context.Context, docID string, cv *Version) {
+	(*c.revCache).RemoveCVOnly(ctx, docID, cv, c.collectionID)
+}
+
 // RemoveWithCV is for per collection access to Remove method
 func (c *collectionRevisionCache) RemoveWithCV(ctx context.Context, docID string, cv *Version) {
 	(*c.revCache).RemoveWithCV(ctx, docID, cv, c.collectionID)

db/revision_cache_lru.go

@@ -97,6 +97,10 @@ func (sc *ShardedLRURevisionCache) RemoveRevOnly(ctx context.Context, docID, rev
 	sc.getShard(docID).RemoveRevOnly(ctx, docID, revID, collectionID)
 }
 
+func (sc *ShardedLRURevisionCache) RemoveCVOnly(ctx context.Context, docID string, cv *Version, collectionID uint32) {
+	sc.getShard(docID).RemoveCVOnly(ctx, docID, cv, collectionID)
+}
+
 // An LRU cache of document revision bodies, together with their channel access.
 type LRURevisionCache struct {
 	backingStores        map[uint32]RevisionCacheBackingStore
@@ -533,6 +537,10 @@ func (rc *LRURevisionCache) RemoveWithCV(ctx context.Context, docID string, cv *
 	rc.removeFromCacheByCV(ctx, docID, cv, collectionID)
 }
 
+func (rc *LRURevisionCache) RemoveCVOnly(ctx context.Context, docID string, cv *Version, collectionID uint32) {
+	rc.removeFromCVLookup(ctx, docID, cv, collectionID)
+}
+
 // RemoveRevOnly removes a rev from revision cache lookup map, if present.
 func (rc *LRURevisionCache) RemoveRevOnly(ctx context.Context, docID, revID string, collectionID uint32) {
 	// This will only remove the entry from the rev lookup map, not the lru list
@@ -587,11 +595,21 @@ func (rc *LRURevisionCache) removeFromRevLookup(ctx context.Context, docID, revI
 	key := IDAndRev{DocID: docID, RevID: revID, CollectionID: collectionID}
 	rc.lock.Lock()
 	defer rc.lock.Unlock()
-	// only delete from rev lookup map, if we delete underlying element in list, the now elem in the HLV lookup map
+	// only delete from rev lookup map, if we delete underlying element in list, the elem in the HLV lookup map
 	// will never be evicted leading to potential unbounded growth of HLV lookup map
 	delete(rc.cache, key)
 }
 
+// removeFromCVLookup will only remove the entry from the CV lookup map, if present. Underlying element must stay in list for eviction to work.
+func (rc *LRURevisionCache) removeFromCVLookup(ctx context.Context, docID string, cv *Version, collectionID uint32) {
+	key := IDandCV{DocID: docID, Source: cv.SourceID, Version: cv.Value, CollectionID: collectionID}
+	rc.lock.Lock()
+	defer rc.lock.Unlock()
+	// only delete from cv lookup map, if we delete underlying element in list, the item left in the revID lookup map for
+	// this item will never be evicted leading to potential unbounded growth of revID lookup map
+	delete(rc.hlvCache, key)
+}
+
 // removeValue removes a value from the revision cache, if present and the value matches the the value. If there's an item in the revision cache with a matching docID and revID but the document is different, this item will not be removed from the rev cache.
 func (rc *LRURevisionCache) removeValue(value *revCacheValue) {
 	rc.lock.Lock()
@@ -627,7 +645,16 @@ func (rc *LRURevisionCache) _numberCapacityEviction() (numItemsEvicted int64, nu
 		}
 		hlvKey := IDandCV{DocID: value.id, Source: value.cv.SourceID, Version: value.cv.Value, CollectionID: value.collectionID}
 		revKey := IDAndRev{DocID: value.id, RevID: value.revID, CollectionID: value.collectionID}
-		delete(rc.hlvCache, hlvKey)
+		if elem := rc.hlvCache[hlvKey]; elem != nil {
+			revValue := elem.Value.(*revCacheValue)
+			// we need to check if the value pointed to by the cv lookup map is the same value we're evicting, this is
+			// because we can currently have two items with the same docID and CV, but different revIDs due to
+			// local wins conflict resolution not generating a new CV but generating a new revID.
+			if revValue.revID == value.revID {
+				// this cv lookup item matches the value we're evicting, so remove it
+				delete(rc.hlvCache, hlvKey)
+			}
+		}
 		if elem := rc.cache[revKey]; elem != nil {
 			revValue := elem.Value.(*revCacheValue)
 			// we need to check if the value pointed to by the rev lookup map is the same value we're evicting, this is
@@ -888,7 +915,17 @@ func (rc *LRURevisionCache) performEviction(ctx context.Context) {
 			}
 			revKey := IDAndRev{DocID: value.id, RevID: value.revID, CollectionID: value.collectionID}
 			hlvKey := IDandCV{DocID: value.id, Source: value.cv.SourceID, Version: value.cv.Value, CollectionID: value.collectionID}
-			delete(rc.hlvCache, hlvKey)
+			// same below but for hlv lookup map
+			if elem := rc.hlvCache[hlvKey]; elem != nil {
+				revValue := elem.Value.(*revCacheValue)
+				// we need to check if the value pointed to by the cv lookup map is the same value we're evicting, this is
+				// because we can currently have two items with the same docID and CV, but different revIDs due to
+				// local wins conflict resolution not generating a new CV but generating a new revID.
+				if revValue.revID == value.revID {
+					// this cv lookup item matches the value we're evicting, so remove it
+					delete(rc.hlvCache, hlvKey)
+				}
+			}
 			if elem := rc.cache[revKey]; elem != nil {
 				revValue := elem.Value.(*revCacheValue)
 				// we need to check if the value pointed to by the rev lookup map is the same value we're evicting, this is

db/revision_cache_test.go

@@ -2347,3 +2347,72 @@ func TestLoadFromBucketLegacyRevsThatAreBackedUpPreUpgrade(t *testing.T) {
 	// 3 misses are from the initial load from bucket after rev cache flush above
 	assert.Equal(t, int64(3), db.DbStats.Cache().RevisionCacheMisses.Value())
 }
+
+func TestRaceRemovingStaleCVValue(t *testing.T) {
+	cacheHitCounter, cacheMissCounter, cacheNumItems, memoryBytesCounted := base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}
+	backingStoreMap := CreateTestSingleBackingStoreMap(&noopBackingStore{}, testCollectionID)
+	cacheOptions := &RevisionCacheOptions{
+		MaxItemCount: 10,
+		MaxBytes:     0,
+	}
+	cache := NewLRURevisionCache(cacheOptions, backingStoreMap, &cacheHitCounter, &cacheMissCounter, &cacheNumItems, &memoryBytesCounted)
+
+	ctx := base.TestCtx(t)
+
+	var wg sync.WaitGroup
+	wg.Add(2)
+
+	go func() {
+		cache.Put(ctx, DocumentRevision{BodyBytes: []byte(`{"some":"data"}`), DocID: "doc1", RevID: "1-abc", CV: &Version{Value: 123, SourceID: "test"}, History: Revisions{"start": 1}}, testCollectionID)
+		wg.Done()
+	}()
+
+	go func() {
+		cache.RemoveCVOnly(ctx, "doc1", &Version{Value: 123, SourceID: "test"}, testCollectionID)
+		wg.Done()
+	}()
+
+	wg.Wait()
+
+}
+
+func TestEvictionWhenStaleCVRemoved(t *testing.T) {
+	cacheHitCounter, cacheMissCounter, cacheNumItems, memoryBytesCounted := base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}, base.SgwIntStat{}
+	backingStoreMap := CreateTestSingleBackingStoreMap(&noopBackingStore{}, testCollectionID)
+	cacheOptions := &RevisionCacheOptions{
+		MaxItemCount: 2,
+		MaxBytes:     0,
+	}
+	cache := NewLRURevisionCache(cacheOptions, backingStoreMap, &cacheHitCounter, &cacheMissCounter, &cacheNumItems, &memoryBytesCounted)
+
+	ctx := base.TestCtx(t)
+
+	cache.Put(ctx, DocumentRevision{BodyBytes: []byte(`{"some":"data"}`), DocID: "doc1", RevID: "1-abc", CV: &Version{Value: 123, SourceID: "test"}, History: Revisions{"start": 1}}, testCollectionID)
+	cache.Put(ctx, DocumentRevision{BodyBytes: []byte(`{"some":"data"}`), DocID: "doc2", RevID: "1-abc", CV: &Version{Value: 1245, SourceID: "test"}, History: Revisions{"start": 1}}, testCollectionID)
+
+	// remove cv from lookup map for doc1
+	cache.RemoveCVOnly(ctx, "doc1", &Version{Value: 123, SourceID: "test"}, testCollectionID)
+
+	assert.Equal(t, 2, cache.lruList.Len())
+	assert.Equal(t, 2, len(cache.cache))
+	assert.Equal(t, 1, len(cache.hlvCache))
+	// assert that doc2 is only item in hlv lookup
+	_, ok := cache.hlvCache[IDandCV{DocID: "doc2", Source: "test", Version: 1245, CollectionID: testCollectionID}]
+	assert.True(t, ok)
+
+	// add new doc revision for doc1 (same cv different revID simulating local wins scenario) to trigger eviction on doc1 first revision
+	cache.Put(ctx, DocumentRevision{BodyBytes: []byte(`{"some":"data"}`), DocID: "doc1", RevID: "2-abc", CV: &Version{Value: 123, SourceID: "test"}, History: Revisions{"start": 1}}, testCollectionID)
+
+	assert.Equal(t, 2, cache.lruList.Len())
+	assert.Equal(t, 2, len(cache.cache))
+	assert.Equal(t, 2, len(cache.hlvCache))
+
+	// assert doc1 entry in hlv map has revID 2-abc
+	val := cache.hlvCache[IDandCV{DocID: "doc1", Source: "test", Version: 123, CollectionID: testCollectionID}]
+	revValue := val.Value.(*revCacheValue)
+	assert.Equal(t, "2-abc", revValue.revID)
+	// assert doc1 entry in revID map has revID 2-abc
+	valRevEntry := cache.cache[IDAndRev{DocID: "doc1", RevID: "2-abc", CollectionID: testCollectionID}]
+	revValueRevEntry := valRevEntry.Value.(*revCacheValue)
+	assert.Equal(t, "2-abc", revValueRevEntry.revID)
+}