Skip to content

Conversation

@gregns1
Copy link
Contributor

@gregns1 gregns1 commented Nov 24, 2025

CBG-5020

  • Defensive checks when evicting based off of memory
  • Moves some code around to have defer use for unlocking

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 AI review requested due to automatic review settings November 24, 2025 17:21
Copy link
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 addresses a panic in the revision cache eviction logic when memory statistics become out of sync with the actual cache state. The fix adds defensive checks when performing type assertions and introduces safeguards to detect and correct inconsistent memory statistics.

Key changes:

  • Added defensive type assertion checks in cache removal operations
  • Implemented detection and correction of inconsistent memory statistics during eviction
  • Refactored locking to use defer statements for safer unlock behavior

Reviewed changes

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

File Description
db/revision_cache_lru.go Added type assertion checks, moved to defer-based locking, and added logic to detect/correct inconsistent memory stats during eviction
db/revision_cache_test.go Added test case to reproduce and verify the fix for the panic scenario with out-of-sync stats

@gregns1 gregns1 requested a review from bbrks November 24, 2025 17:49
Comment on lines 942 to 944
correctionVal := rc.currMemoryUsage.Value()
rc.currMemoryUsage.Add(-correctionVal)
rc.cacheMemoryBytesStat.Add(-correctionVal)
Copy link
Member

Choose a reason for hiding this comment

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

It would be much clearer and potentially safer to use Set(0) for this than doing two separate Value and Add IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this, I have added some comments on this area, we can;t quite set cacheMemoryBytesStat to 0 given this is the memory usage across all shards.

@bbrks bbrks assigned gregns1 and unassigned bbrks Nov 26, 2025
@gregns1 gregns1 assigned bbrks and unassigned gregns1 Nov 26, 2025
@bbrks bbrks merged commit 12666a4 into main Nov 26, 2025
42 checks passed
@bbrks bbrks deleted the CBG-5020 branch November 26, 2025 17:52
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