Skip to content

Conversation

gregns1
Copy link
Contributor

@gregns1 gregns1 commented Sep 25, 2025

CBG-4879

  • Remove just from cv map to invalidate the cv entry for documents that are a result of local wins
  • The race is from reading revID from rev cache value for removal from the revID lookup in the caching goroutine whilst the write goroutine is populating the rev cache value on Put at the cache
  • This means we have to do some extra checks at eviction time like we needed to do in CBG-4734: fix for race when removing a value that is being loaded into rev cache #7640

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

  • Link upstream PRs
  • Update Go module dependencies when merged

Integration Tests

@Copilot Copilot AI review requested due to automatic review settings September 25, 2025 15:47
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a data race in the revision cache when invalidating cache entries for documents that result from local wins in conflict resolution. Instead of removing the entire cache entry, it now only removes the CV (current version) map entry to prevent race conditions between read and write operations on the cache.

  • Adds a new RemoveCVOnly method to selectively remove CV lookup entries without affecting revID lookups
  • Enhances eviction logic to handle scenarios where multiple cache entries share the same CV but have different revision IDs
  • Updates the change cache to use the new selective removal method for local wins

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
db/revision_cache_interface.go Adds the RemoveCVOnly method to the RevisionCache interface and collection wrapper
db/revision_cache_lru.go Implements RemoveCVOnly method and updates eviction logic to handle CV/revID mismatches
db/revision_cache_bypass.go Adds no-op implementation of RemoveCVOnly for bypass cache
db/change_cache.go Updates DocChanged to use RemoveCVOnly instead of RemoveWithCV for local wins
db/revision_cache_test.go Adds tests to verify race condition fix and proper eviction behavior

@gregns1 gregns1 requested a review from bbrks September 25, 2025 17:12
bbrks
bbrks previously approved these changes Sep 26, 2025
Copy link
Member

@bbrks bbrks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just +1'd copilots suggestion

@bbrks bbrks assigned gregns1 and unassigned bbrks Sep 26, 2025
@bbrks bbrks merged commit f43c0ff into main Sep 26, 2025
42 checks passed
@bbrks bbrks deleted the CBG-4879 branch September 26, 2025 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants