CBG-4734: fix for race when removing a value that is being loaded into rev cache (#7640)

Author: gregns1

db/change_cache.go

@@ -503,7 +503,7 @@ func (c *changeCache) DocChanged(event sgbucket.FeedEvent, docType DocumentType)
 
 	// Now add the entry for the new doc revision:
 	if len(rawUserXattr) > 0 {
-		collection.revisionCache.RemoveWithRev(ctx, docID, syncData.CurrentRev)
+		collection.revisionCache.RemoveRevOnly(ctx, docID, syncData.CurrentRev)
 	}
 
 	change := &LogEntry{

db/crud.go

@@ -2496,7 +2496,7 @@ func (db *DatabaseCollectionWithUser) updateAndReturnDoc(ctx context.Context, do
 
 			// Prior to saving doc, remove the revision in cache
 			if createNewRevIDSkipped {
-				db.revisionCache.RemoveWithRev(ctx, doc.ID, doc.CurrentRev)
+				db.revisionCache.RemoveRevOnly(ctx, doc.ID, doc.CurrentRev)
 			}
 
 			base.DebugfCtx(ctx, base.KeyCRUD, "Saving doc (seq: #%d, id: %v rev: %v)", doc.Sequence, base.UD(doc.ID), doc.CurrentRev)

db/revision_cache_bypass.go

@@ -128,6 +128,10 @@ func (rc *BypassRevisionCache) RemoveWithRev(ctx context.Context, docID, revID s
 	// no-op
 }
 
+func (rc *BypassRevisionCache) RemoveRevOnly(ctx context.Context, docID, revID string, 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,6 +55,8 @@ 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(ctx context.Context, docID, revID string, 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)
 
@@ -172,6 +174,10 @@ func (c *collectionRevisionCache) RemoveWithRev(ctx context.Context, docID, revI
 	(*c.revCache).RemoveWithRev(ctx, docID, revID, c.collectionID)
 }
 
+func (c *collectionRevisionCache) RemoveRevOnly(ctx context.Context, docID, revID string) {
+	(*c.revCache).RemoveRevOnly(ctx, docID, revID, 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

@@ -93,6 +93,10 @@ func (sc *ShardedLRURevisionCache) RemoveWithCV(ctx context.Context, docID strin
 	sc.getShard(docID).RemoveWithCV(ctx, docID, cv, collectionID)
 }
 
+func (sc *ShardedLRURevisionCache) RemoveRevOnly(ctx context.Context, docID, revID string, collectionID uint32) {
+	sc.getShard(docID).RemoveRevOnly(ctx, docID, revID, collectionID)
+}
+
 // An LRU cache of document revision bodies, together with their channel access.
 type LRURevisionCache struct {
 	backingStores        map[uint32]RevisionCacheBackingStore
@@ -507,6 +511,12 @@ func (rc *LRURevisionCache) RemoveWithCV(ctx context.Context, docID string, cv *
 	rc.removeFromCacheByCV(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
+	rc.removeFromRevLookup(ctx, docID, revID, collectionID)
+}
+
 // removeFromCacheByCV removes an entry from rev cache by CV
 func (rc *LRURevisionCache) removeFromCacheByCV(ctx context.Context, docID string, cv *Version, collectionID uint32) {
 	key := IDandCV{DocID: docID, Source: cv.SourceID, Version: cv.Value, CollectionID: collectionID}
@@ -518,6 +528,7 @@ func (rc *LRURevisionCache) removeFromCacheByCV(ctx context.Context, docID strin
 	}
 	// grab the revid key from the value to enable us to remove the reference from the rev lookup map too
 	elem := element.Value.(*revCacheValue)
+
 	legacyKey := IDAndRev{DocID: docID, RevID: elem.revID, CollectionID: collectionID}
 	rc.lruList.Remove(element)
 	delete(rc.hlvCache, key)
@@ -538,6 +549,7 @@ func (rc *LRURevisionCache) removeFromCacheByRev(ctx context.Context, docID, rev
 	}
 	// grab the cv key from the value to enable us to remove the reference from the rev lookup map too
 	elem := element.Value.(*revCacheValue)
+
 	hlvKey := IDandCV{DocID: docID, Source: elem.cv.SourceID, Version: elem.cv.Value, CollectionID: collectionID}
 	rc.lruList.Remove(element)
 	// decrement the overall memory bytes count
@@ -548,6 +560,16 @@ func (rc *LRURevisionCache) removeFromCacheByRev(ctx context.Context, docID, rev
 	rc.cacheNumItems.Add(-1)
 }
 
+// removeFromRevLookup will only remove the entry from the rev lookup map, if present. Underlying element must stay in list for eviction to work.
+func (rc *LRURevisionCache) removeFromRevLookup(ctx context.Context, docID, revID string, collectionID uint32) {
+	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
+	// will never be evicted leading to potential unbounded growth of HLV lookup map
+	delete(rc.cache, 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()
@@ -583,8 +605,17 @@ 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.cache, revKey)
 		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
+			// because we can have can currently have two items with the same docID and revID, but different CVs due to
+			// a new HLV being generated for user xattr updates where we don't generate a new revID.
+			if revValue.cv.String() == value.cv.String() {
+				// this rev lookup item matches the value we're evicting, so remove it
+				delete(rc.cache, revKey)
+			}
+		}
 		numItemsEvicted++
 		numBytesEvicted += value.getItemBytes()
 	}
@@ -832,8 +863,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.cache, revKey)
 			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
+				// because we can have can currently have two items with the same docID and revID, but different CVs due to
+				// a new HLV being generated for user xattr updates where we don't generate a new revID.
+				if revValue.cv.String() == value.cv.String() {
+					// this rev lookup item matches the value we're evicting, so remove it
+					delete(rc.cache, revKey)
+				}
+			}
 			numItemsRemoved++
 			valueBytes := value.getItemBytes()
 			numBytesRemoved += valueBytes

db/revision_cache_test.go

@@ -2239,3 +2239,55 @@ func TestRevCacheOnDemandImportNoCache(t *testing.T) {
 	_, exists = collection.revisionCache.Peek(ctx, docID, doc.CurrentRev)
 	require.False(t, exists)
 }
+
+func TestRemoveFromRevLookup(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)
+
+	// Fill up the rev cache with the first 10 docs
+	for docID := 0; docID < 10; docID++ {
+		id := strconv.Itoa(docID)
+		vrs := uint64(docID)
+		cache.Put(ctx, DocumentRevision{BodyBytes: []byte(`{}`), DocID: id, RevID: "1-abc", CV: &Version{Value: vrs, SourceID: "test"}, History: Revisions{"start": 1}}, testCollectionID)
+	}
+	assert.Equal(t, int64(10), cacheNumItems.Value())
+	assert.Equal(t, int64(20), memoryBytesCounted.Value())
+	assert.Equal(t, 10, len(cache.cache))
+	assert.Equal(t, 10, len(cache.hlvCache))
+
+	// simulate a user xattr update:
+	// 1. Removes form rev lookup cache
+	// 2. Enter new entry with same revID and docID but diff CV
+	// 3. Assert that eviction eventually aligns the cache items in each lookup map
+	cache.RemoveRevOnly(ctx, "1", "1-abc", testCollectionID)
+	assert.Equal(t, int64(10), cacheNumItems.Value())
+	assert.Equal(t, int64(20), memoryBytesCounted.Value())
+	assert.Equal(t, 10, cache.lruList.Len())
+	assert.Equal(t, 9, len(cache.cache))
+	assert.Equal(t, 10, len(cache.hlvCache))
+
+	// add new entry to cache for docID 1 with a different CV but same revID
+	cache.Put(ctx, DocumentRevision{BodyBytes: []byte(`{}`), DocID: "1", RevID: "1-abc", CV: &Version{Value: 1234, SourceID: "test"}, History: Revisions{"start": 1}}, testCollectionID)
+	assert.Equal(t, int64(10), cacheNumItems.Value())
+	assert.Equal(t, int64(20), memoryBytesCounted.Value())
+	assert.Equal(t, 10, cache.lruList.Len())
+	assert.Equal(t, 9, len(cache.cache)) // we should have 9 items in the rev lookup as we removed one item above
+	assert.Equal(t, 10, len(cache.hlvCache))
+
+	// add new doc to trigger eviction of old doc "1" value in the cache that has entry in hlv cache to an item but the revID lookup
+	// for this item will be to a different value (given the simulation of user xattr update above). This means that this
+	// will trigger an eviction from CV lookup but no revID lookup thus aligning the lookup maps once again.
+	cache.Put(ctx, DocumentRevision{BodyBytes: []byte(`{}`), DocID: "someNewDoc", RevID: "1-abc", CV: &Version{Value: 123456, SourceID: "test"}, History: Revisions{"start": 1}}, testCollectionID)
+	assert.Equal(t, int64(10), cacheNumItems.Value())
+	assert.Equal(t, int64(20), memoryBytesCounted.Value())
+	assert.Equal(t, 10, cache.lruList.Len())
+	assert.Equal(t, 10, len(cache.cache)) // we should now have 10 items in rev lookup given the item above aligned the lookups after eviction
+	assert.Equal(t, 10, len(cache.hlvCache))
+}