Skip to content

Bug fix S3fifo race condition #22156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

cpegeric
Copy link
Contributor

@cpegeric cpegeric commented Jul 11, 2025

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #21838

What this PR does / why we need it:

fix race condition


PR Type

Bug fix


Description

  • Fix S3-FIFO cache race conditions with mutex protection

  • Replace atomic operations with mutex-protected reference counting

  • Add double-free protection and panic checks for deallocated buffers

  • Implement proper thread-safe item deletion and eviction


Changes diagram

flowchart LR
  A["Atomic Operations"] --> B["Mutex Protection"]
  C["Race Conditions"] --> D["Thread-Safe Access"]
  E["Double Free Issues"] --> F["Deletion Tracking"]
  G["Cache Item Management"] --> H["Reference Counting"]
Loading

Changes walkthrough 📝

Relevant files
Bug fix
7 files
bytes.go
Replace atomic with mutex for thread safety                           
+45/-12 
data_cache.go
Update cache operations and path deletion                               
+14/-21 
fifo.go
Rewrite S3-FIFO with mutex-based synchronization                 
+238/-198
queue.go
Add mutex protection to queue operations                                 
+23/-4   
data.go
Change Release method to return boolean                                   
+1/-1     
io_vector.go
Handle Release return value properly                                         
+7/-2     
mem_cache.go
Remove redundant retain/release calls in callbacks             
+1/-9     
Tests
4 files
bytes_test.go
Add panic tests for deallocated buffers                                   
+19/-0   
data_cache_test.go
Update test interface and allocation runs                               
+3/-2     
fifo_test.go
Update tests for new cache structure                                         
+9/-9     
shardmap_test.go
Add tests for sharded map functionality                                   
+45/-0   
Enhancement
1 files
shardmap.go
New thread-safe sharded map implementation                             
+128/-0 

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    kind/bug Something isn't working Review effort 4/5 size/L Denotes a PR that changes [500,999] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants