perf(client): implement Phase 76 Client Latency Optimization#9
perf(client): implement Phase 76 Client Latency Optimization#9rthellend wants to merge 20 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Phase 76 of the DistFS plan, focusing on client-side latency optimizations. Key changes include the introduction of a concurrent write pipeline for chunk uploads, configurable hedged read delays to reduce tail latency, throttled cache pruning in the native storage engine, and an auto-tuning dynamic prefetch window for sequential reads. Feedback was provided regarding critical error handling in the asynchronous write path to prevent potential data corruption, ensuring that configuration values (like max prefetch) can be explicitly disabled, and improving the accuracy of the cache size estimate during file removal failures.
…etch, and size estimation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements Phase 76 of the DistFS plan, introducing several client-side latency optimizations. Key improvements include pipelined background write uploads using a semaphore-controlled goroutine pool, configurable hedged read delays to reduce tail latency, throttled cache pruning in the NativeStore based on an atomic byte estimate, and an auto-tuning dynamic prefetch window for sequential reads. These features are exposed through new client builder methods and CLI configuration flags. Feedback was provided regarding a potential performance regression in the distfs CLI due to the full directory walk performed during NativeStore initialization, suggesting that the byte estimate should be persisted in the metadata database to avoid expensive I/O on every command execution.
…ctory walks on startup
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements Phase 76 of the DistFS plan, introducing several client-side latency optimizations including pipelined background writes, configurable hedged read delays, a dynamic auto-tuning prefetch window, and throttled cache pruning. Feedback highlights a performance concern regarding frequent BoltDB fsyncs during byte tracking, a potential race condition when updating the cache size estimate in NativeStore, and a bug where the prefetch window reset logic fails to respect a zeroed maximum prefetch setting.
…d updates in Prune, and fix seek prefetch window reset
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements Phase 76 of the DistFS plan, focusing on client-side latency optimizations. Key enhancements include pipelined background writes using a semaphore-controlled goroutine pool, configurable hedged read delays, and an auto-tuning dynamic prefetch window for sequential reads. Additionally, NativeStore now utilizes atomic byte estimation and rate-limited pruning to reduce filesystem overhead. The review feedback identifies critical issues to address, including potential race conditions in the asynchronous write pipeline and cache size estimation logic, as well as an idiomatic improvement for handling atomic pointers in background error reporting.
|
/gemini summary |
|
Warning Gemini encountered an error creating the summary. You can try again by commenting |
|
/gemini summary |
|
Warning Gemini encountered an error creating the summary. You can try again by commenting |
|
/gemini summary |
Summary of ChangesThis pull request implements Phase 76 of the client latency optimization project, focusing on reducing bottlenecks in data transfer and metadata management. By introducing asynchronous pipelining for writes, hedged reads for replicas, dynamic prefetching for reads, and efficient cache tracking, the changes deliver significant throughput and latency improvements. Additionally, a new signature verification cache reduces CPU overhead for repeated cryptographic operations, further enhancing overall client performance. Highlights
Activity
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements Phase 76 of the Client Latency Optimization plan, introducing several performance enhancements: an LRU cache for post-quantum signature verifications, a configurable hedge delay for replica reads, an auto-tuning dynamic prefetch window for sequential reads, and a pipelined background upload mechanism for file writes. It also adds a short-lived in-memory metadata TTL cache and improves local storage pruning efficiency. Feedback highlights a high-severity issue where the memory cache returns mutable pointers to internal Inode objects, potentially leading to cache corruption. Additionally, reviewers noted concerns regarding serving incomplete inodes from the cache if transient fields are not populated, potential manifest corruption due to orphaned placeholders on synchronous write failures, and an opportunity to improve the robustness of the storage byte estimate during pruning.
There was a problem hiding this comment.
Code Review
This pull request implements Phase 76 of the DistFS plan, introducing several client-side latency optimizations: pipelined background write uploads, configurable hedged chunk reads, throttled cache pruning in NativeStore, an auto-tuning dynamic prefetch window, and a metadata TTL cache. It also adds a thread-safe LRU cache for PQC signature verifications to reduce CPU overhead. Critical feedback was provided regarding thread-safety and cache corruption risks when returning direct pointers from the metadata cache, as well as inconsistent error handling in the asynchronous write pipeline that could lead to corrupted manifests. An improvement was also suggested to prevent drift in the cache size estimate by resetting the counter to the actual size during pruning.
…its, placeholder cleanup on sync failure, and NativeStore prune drift correction
rthellend
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements Phase 76 (Client Latency Optimization) for DistFS, introducing pipelined background write uploads, configurable hedged chunk reads, throttled and estimated cache pruning, an auto-tuning dynamic prefetch window, and an in-memory LRU cache for post-quantum signature verifications. Additionally, deep copying of Inode objects is added to prevent cache pollution. The review feedback highlights two issues in the native storage implementation: a potential edge case where the cache remains over-limit indefinitely due to rate-limiting during write bursts, and a persistence bug where corrected size estimates are not saved to BoltDB when no bytes are removed during pruning.
…e-limit deferred timer and persistence
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements Phase 76 (Client Latency Optimization) to resolve several performance bottlenecks in the client-side read, write, caching, and path-resolution pipelines. Key changes include pipelined background write uploads, configurable hedge delays, throttled and estimated cache pruning in NativeStore, an auto-tuning dynamic prefetch window in FileReader, and an in-memory metadata TTL cache for decrypted Inodes supported by deep-copy cloning. Additionally, a thread-safe post-quantum signature verification LRU cache has been introduced to accelerate ML-DSA-65 verifications. The review feedback highlights a critical cryptographic signature bypass in the metadata cache when verify is true, a potential goroutine leak in FileWriter.Close() upon Finish() failure, a race condition in NativeStore.Close() where background pruning can access a closed database, and silent error ignoring during MetadataTTL parsing in distfs-bench.
…re cache hits, writer leaks, native store close synchronization, and bench logs
…plete verification. Fixed getInodeInternal where nested fetches were incorrectly cached as verified: true before top-level signature verification queue was processed. Added regression test TestClient_CachePoisoning_IncompleteVerification.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements Phase 76 (Client Latency Optimization) for DistFS, introducing pipelined background write uploads, configurable hedged chunk reads, throttled cache pruning, an auto-tuning dynamic prefetch window, and an in-memory metadata TTL cache. It also adds a post-quantum signature verification LRU cache to speed up ML-DSA-65 verifications. The code review identified critical security and reliability issues that must be addressed: a cryptographic signature bypass when serving inodes from the metadata TTL cache, a data leak vulnerability where the memory cache is shared across cloned client identities, lease leaks in both OpenBlobWriteWithLease and FileWriter.Close() upon failures, and a potential panic in NativeStore.Close() if background pruning is active during database shutdown.
This commit ensures that all read and write operations involving the UserCache and GroupCache (verified and unverified) return or store deep-copied clones of the User and Group structs. This prevents callers from intentionally or accidentally mutating cached identities, fixing a critical vector where an attacker could corrupt cryptographic keys or permissions for subsequent lookups. It also fixes a bug where root owner identity pinning was sharing a byte slice with server-provided identity data, opening a cache mutation vector. Includes a comprehensive TestCacheImmutability suite.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements Phase 76 client latency optimizations, introducing pipelined background write uploads, configurable hedged chunk reads, throttled cache pruning, an auto-tuning dynamic prefetch window, and an LRU cache for PQC signature verifications. It also adds deep-copy cloning to Inode, User, and Group models to ensure cache immutability. The review feedback highlights a critical race condition in NativeStore where a deferred pruning timer could fire after Close() is called, potentially causing a panic or writing to a closed database. To resolve this, the reviewer suggests tracking the store's lifecycle with a closed boolean flag, setting it in Close(), and checking it before scheduling pruning runs.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements Phase 76 client latency optimizations, introducing pipelined background write uploads, configurable hedged chunk read delays, throttled and estimated cache pruning, and an auto-tuning dynamic prefetch window. It also adds deep cloning for cache isolation of inodes, users, and groups, and integrates an LRU cache for post-quantum signature verifications. The review feedback highlights critical race conditions and potential panics in NativeStore during shutdown, recommending the addition of a closed flag protected by a mutex to safely coordinate background pruning goroutines and timers with the Close() operation.
| func (s *NativeStore) Close() error { | ||
| s.mu.Lock() | ||
| if s.pruneTimer != nil { | ||
| s.pruneTimer.Stop() | ||
| s.pruneTimer = nil | ||
| } | ||
| s.mu.Unlock() | ||
|
|
||
| s.pruneWg.Wait() // Wait for active background pruning to complete | ||
|
|
||
| s.saveEstimatedBytes() | ||
| return s.db.Close() | ||
| } |
There was a problem hiding this comment.
There is a critical race condition where a deferred pruning timer can fire after or during Close(), leading to a panic (sync: WaitGroup misuse: Add called concurrently with Wait) or attempting to write to a closed BoltDB database. We must set the closed flag under the lock to prevent any new background pruning goroutines from being scheduled or calling pruneWg.Add(1) concurrently with pruneWg.Wait().
func (s *NativeStore) Close() error {
s.mu.Lock()
if s.closed {
s.mu.Unlock()
return nil
}
s.closed = true
if s.pruneTimer != nil {
s.pruneTimer.Stop()
s.pruneTimer = nil
}
s.mu.Unlock()
s.pruneWg.Wait() // Wait for active background pruning to complete
s.saveEstimatedBytes()
return s.db.Close()
}| // maybeSchedulePrune schedules a single background Prune run if: | ||
| // - No prune is already running (single-flight via atomic CAS), and | ||
| // - At least pruneInterval has elapsed since the last prune run. | ||
| func (s *NativeStore) maybeSchedulePrune() { | ||
| // Single-flight: only one prune goroutine at a time. | ||
| if !s.pruning.CompareAndSwap(0, 1) { | ||
| return | ||
| } | ||
| s.mu.Lock() | ||
| since := time.Since(s.lastPrune) | ||
| interval := s.pruneInterval | ||
| if interval > 0 && since < interval { | ||
| // Rate-limited. Cancel any existing timer and schedule a new one | ||
| // for the remaining duration of the rate-limit window. | ||
| if s.pruneTimer != nil { | ||
| s.pruneTimer.Stop() | ||
| } | ||
| delay := interval - since | ||
| s.pruneTimer = time.AfterFunc(delay, func() { | ||
| s.maybeSchedulePrune() | ||
| }) | ||
| s.mu.Unlock() | ||
| s.pruning.Store(0) | ||
| return | ||
| } | ||
| if s.pruneTimer != nil { | ||
| s.pruneTimer.Stop() | ||
| s.pruneTimer = nil | ||
| } | ||
| s.lastPrune = time.Now() | ||
| s.mu.Unlock() | ||
|
|
||
| s.pruneWg.Add(1) | ||
| go func() { | ||
| defer s.pruning.Store(0) | ||
| defer s.pruneWg.Done() | ||
| _ = s.Prune() | ||
| }() | ||
| } |
There was a problem hiding this comment.
To prevent scheduling background pruning after the store is closed, we must check the closed flag under the lock. Additionally, s.pruneWg.Add(1) must be called while holding the lock to avoid a race condition with s.pruneWg.Wait() in Close().
func (s *NativeStore) maybeSchedulePrune() {
// Single-flight: only one prune goroutine at a time.
if !s.pruning.CompareAndSwap(0, 1) {
return
}
s.mu.Lock()
if s.closed {
s.mu.Unlock()
s.pruning.Store(0)
return
}
since := time.Since(s.lastPrune)
interval := s.pruneInterval
if interval > 0 && since < interval {
// Rate-limited. Cancel any existing timer and schedule a new one
// for the remaining duration of the rate-limit window.
if s.pruneTimer != nil {
s.pruneTimer.Stop()
}
delay := interval - since
s.pruneTimer = time.AfterFunc(delay, func() {
s.maybeSchedulePrune()
})
s.mu.Unlock()
s.pruning.Store(0)
return
}
if s.pruneTimer != nil {
s.pruneTimer.Stop()
s.pruneTimer = nil
}
s.lastPrune = time.Now()
s.pruneWg.Add(1) // Must be called under lock to avoid race with Wait() in Close()
s.mu.Unlock()
go func() {
defer s.pruning.Store(0)
defer s.pruneWg.Done()
_ = s.Prune()
}()
}| maxBytes int64 | ||
| db *bbolt.DB | ||
| mu sync.RWMutex | ||
|
|
||
| // Phase 76.3: Throttled & estimated cache pruning. | ||
| // estimatedBytes tracks the approximate on-disk chunk usage atomically, | ||
| // updated on every put/delete so we can skip Prune when under the limit. | ||
| estimatedBytes atomic.Int64 | ||
| // pruning is a single-flight flag: 0=idle, 1=running. | ||
| pruning atomic.Int32 | ||
| // lastPrune records when the last prune completed (guarded by mu). | ||
| lastPrune time.Time | ||
| // pruneInterval is the rate-limit interval for auto-pruning. | ||
| pruneInterval time.Duration | ||
| // pruneTimer is a deferred timer to run pruning when skipped due to rate limit. | ||
| pruneTimer *time.Timer | ||
| // pruneWg tracks active background pruning goroutines. | ||
| pruneWg sync.WaitGroup | ||
| } | ||
|
|
||
| func NewNativeStore(baseDir string, maxBytes int64) (*NativeStore, error) { |
There was a problem hiding this comment.
To prevent race conditions and panics during Close(), we need to track whether the store has been closed. Please add a closed boolean field to the NativeStore struct, which will be protected by mu.
type NativeStore struct {
baseDir string
maxBytes int64
db *bbolt.DB
mu sync.RWMutex
closed bool
// Phase 76.3: Throttled & estimated cache pruning.
// estimatedBytes tracks the approximate on-disk chunk usage atomically,
// updated on every put/delete so we can skip Prune when under the limit.
estimatedBytes atomic.Int64
// pruning is a single-flight flag: 0=idle, 1=running.
pruning atomic.Int32
// lastPrune records when the last prune completed (guarded by mu).
lastPrune time.Time
// pruneInterval is the rate-limit interval for auto-pruning.
pruneInterval time.Duration
// pruneTimer is a deferred timer to run pruning when skipped due to rate limit.
pruneTimer *time.Timer
// pruneWg tracks active background pruning goroutines.
pruneWg sync.WaitGroup
}1 participant
Summary of Changes
This pull request implements Phase 76 of the client latency optimization project, focusing on reducing bottlenecks in data transfer and metadata management. By introducing asynchronous pipelining for writes, hedged reads for replicas, dynamic prefetching for reads, and efficient cache tracking, the changes deliver significant throughput and latency improvements. Additionally, a new signature verification cache reduces CPU overhead for repeated cryptographic operations, further enhancing overall client performance.
Highlights