Unified Storage Performance Optimizations

[akshayutture-augment]

Test 10

---

Replicated from ai-code-review-evaluation/grafana-greptile#10

[greptile-apps]

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

[akshayutture-augment]

@greptileai

[greptile-apps]

Greptile Overview

Confidence Score: 2/5

• This PR has a critical race condition that will cause runtime panics during initialization
• The optimization to reduce lock contention in bleve.go introduced a race condition where TotalDocs() reads the cache without synchronization while concurrent goroutines write to it. This will cause "concurrent map read/write" panics when the search index initializes.
• Pay close attention to pkg/storage/unified/search/bleve.go - the TotalDocs() method must acquire a read lock before iterating the cache

Important Files Changed

File Analysis

Filename	Score	Overview
pkg/storage/unified/search/bleve.go	2/5	Moved mutex acquisition after index building to reduce lock contention, but introduced critical race condition in TotalDocs() method
pkg/storage/unified/resource/search.go	4/5	Fixed context propagation in tracing spans and improved logging, replaced incorrect logger import with proper instance logger
pkg/storage/unified/resource/server.go	5/5	Moved initialization to constructor and removed redundant Init() checks from each operation, improving startup reliability and reducing overhead

Sequence Diagram

sequenceDiagram
    participant Constructor as NewResourceServer
    participant Server as server.Init()
    participant Search as searchSupport.init()
    participant Bleve as bleveBackend
    participant Storage as SQL Backend
    participant Watcher as Event Watcher

    Note over Constructor: Initialization moved to constructor
    Constructor->>Server: Call Init(ctx)
    Server->>Search: search.init(ctx)
    
    Note over Search: Parallel index building
    Search->>Storage: GetResourceStats(ctx)
    Storage-->>Search: Return stats
    
    par For each resource type
        Search->>Search: build(ctx, nsr, size, rv)
        Search->>Bleve: BuildIndex(ctx, key, ...)
        Note over Bleve: Lock moved after build
        Bleve->>Bleve: Create index & build documents
        Bleve->>Bleve: Lock cache & add index
        Bleve-->>Search: Return index
    end
    
    Note over Search: RACE: TotalDocs called here
    Search->>Bleve: TotalDocs()
    Note over Bleve: Iterates cache WITHOUT lock
    Bleve-->>Search: Return count
    
    Search->>Storage: WatchWriteEvents(ctx)
    Storage-->>Search: Event channel
    Search->>Watcher: Start goroutine
    
    Server->>Server: initWatcher()
    Server-->>Constructor: Return initialized server
    
    Note over Server: Operations no longer call Init()
    Server->>Server: Create/Update/Delete/Read/List/Watch
    Note over Server: Removed redundant Init() checks

Loading

[greptile-apps]

Additional Comments (1)

1. pkg/storage/unified/search/bleve.go, line 144-154 (link)

   logic: Race condition: TotalDocs() iterates over b.cache without holding a lock, while BuildIndex modifies the cache on line 137-139. This can cause concurrent map read/write panics when called during initialization.

_{5 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}