Skip to content

CBG-4847: legacy rev loading from bucket leave rev cache in inconsistent state#7744

Merged
gregns1 merged 3 commits into
mainfrom
CBG-4847
Sep 9, 2025
Merged

CBG-4847: legacy rev loading from bucket leave rev cache in inconsistent state#7744
gregns1 merged 3 commits into
mainfrom
CBG-4847

Conversation

@gregns1
Copy link
Copy Markdown
Contributor

@gregns1 gregns1 commented Sep 9, 2025

CBG-4847

  • Fix for edge case where loading some legacy backup revs from pre upgrade would result in the rev cache being in a weird state and returning the wrong data for fetches.
  • Makes use of the rev -> cv utility we now have to create a cv from revID to populate the rev cache hlv lookup map with

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

Integration Tests

Copilot AI review requested due to automatic review settings September 9, 2025 15:19
Copy link
Copy Markdown
Contributor

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 an edge case where loading legacy backup revisions from pre-upgrade would result in the revision cache being in an inconsistent state and returning incorrect data for fetches. The fix ensures that legacy revisions without Clock Vector (CV) values have proper CV entries generated from their revision IDs when loaded into the cache.

Key changes:

  • Enhanced revision cache loading to generate CV values for legacy revisions without existing CV data
  • Added proper error handling for HLV map population during cache loading
  • Improved cache consistency by ensuring both revision and CV lookup maps are properly populated

Reviewed Changes

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

File Description
db/revision_cache_lru.go Modified cache loading methods to generate CV from revID for legacy documents and added error handling for HLV map operations
db/revision_cache_test.go Added comprehensive test to verify legacy revision loading and cache consistency

Comment thread db/revision_cache_lru.go
Comment thread db/revision_cache_test.go Outdated
Comment thread db/revision_cache_test.go Outdated
Copy link
Copy Markdown
Collaborator

@torcolvin torcolvin 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 some nits comments on scoping err to avoid escaping if we refactor and unique names of error messages if we ever see this in warning logs

Comment thread db/revision_cache_lru.go
// check for memory based eviction
rc.revCacheMemoryBasedEviction(ctx)
rc.addToHLVMapPostLoad(docID, docRev.RevID, docRev.CV, collectionID)
err = rc.addToHLVMapPostLoad(docID, docRev.RevID, docRev.CV, collectionID)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
err = rc.addToHLVMapPostLoad(docID, docRev.RevID, docRev.CV, collectionID)
err := rc.addToHLVMapPostLoad(docID, docRev.RevID, docRev.CV, collectionID)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've intentionally not scoped the error here, essentially if we hit this code here the err is nil, and we if we error here and assign that error to err then below on line 224 we will enter the code block to remove the failed item form the cache altogether.

I will add comment to code explaining this so its clear.

Comment thread db/revision_cache_lru.go
Comment thread db/revision_cache_lru.go Outdated
Comment thread db/revision_cache_lru.go Outdated
@gregns1 gregns1 merged commit 1a7f0d8 into main Sep 9, 2025
58 of 59 checks passed
@gregns1 gregns1 deleted the CBG-4847 branch September 9, 2025 15:54
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.

3 participants