[enhance] buffer storage

[iaojnh]

resolve #64

Greptile Summary

This PR replaces the BufferManager-based storage backend with a new purpose-built VecBufferPool — a file-backed, LRU-evicting, ref-counted buffer pool using a lock-free concurrent queue (moodycamel::ConcurrentQueue). It also wires in proper context->reset() calls after Add() and Search() operations, fixes result-clearing in FlatStreamerContext and HnswContext, and adapts Neighbors to use copy semantics for the new MemoryBlock type.

Key changes and findings:

• open() fails for empty indices: When the index file has no segments, max_segment_size_ remains 0, causing buffer_pool_->init(buffer_size_, 0, 0) to return -1 on the block_size == 0 guard, breaking open() entirely for newly-created indices.
• Silent error return in fetch() / read(const void**): Both methods return 0 when the buffer pool fails to supply a buffer, making it impossible for callers to distinguish an I/O error from a legitimate zero-byte read.
• MemoryBlock holds a dangling raw pointer: data.reset(owner_->buffer_pool_handle_.get(), ...) stores a raw pointer from a shared_ptr. If close_index() resets buffer_pool_handle_ while a MemoryBlock is still alive, the stored pointer becomes dangling and subsequent release_one() calls in the MemoryBlock destructor are undefined behaviour.
• Several previously flagged issues (destructor buffer leak, fd leak on fstat failure, hardcoded pool size, wrong param key, release() vs reset(), null-pointer arithmetic) have been correctly addressed in this revision.

Confidence Score: 2/5

• Not safe to merge — open() can fail unconditionally for empty indices, and the MemoryBlock dangling pointer is a latent use-after-free.
• Three new logic bugs remain unresolved: a hard failure for empty/new indices, ambiguous error returns in two segment-read methods, and an ownership gap between shared_ptr<VecBufferPoolHandle> and the raw pointer stored in MemoryBlock. The last one is a use-after-free waiting to happen in any shutdown-under-load scenario. The PR has addressed many prior review findings, but these new issues need to be fixed before merging.
• src/core/utility/buffer_storage.cc requires the most attention (all three new issues originate here); src/include/zvec/core/framework/index_storage.h should be updated to store a shared_ptr rather than a raw pointer in MemoryBlock.

Important Files Changed

Filename	Overview
src/ailego/buffer/buffer_pool.cc	New file implementing LRUCache, LPMap, VecBufferPool, and VecBufferPoolHandle — core lock-free buffer pool logic. Addressed previous review issues (destructor cleanup, fstat fd leak, pread error return), but recycle() is still called without holding mutex_ in acquire_buffer().
src/include/zvec/ailego/buffer/buffer_pool.h	New header defining the buffer pool class hierarchy. Most previously flagged issues are resolved (destructor frees all buffers, block_size_ is size_t with default initializer). VecBufferPool::lp_map_ is public, exposing internal LPMap state directly.
src/core/utility/buffer_storage.cc	Rewrites storage backend from BufferManager to VecBufferPool. Three new issues: open() fails for empty indices when max_segment_size_==0; fetch() and read() silently return 0 on buffer pool failure; MemoryBlock stores a raw pointer from shared_ptr::get() that can become dangling after close_index().
src/include/zvec/core/framework/index_storage.h	Adds MemoryBlock struct with MBT_MMAP and MBT_BUFFERPOOL variants, correct copy/move semantics and RAII ref-count management. The raw pointer stored from buffer_pool_handle_.get() can dangle if the storage is closed while a MemoryBlock is live (see buffer_storage.cc comment).
src/core/algorithm/hnsw/hnsw_entity.h	Changes Neighbors constructor from move-semantic MemoryBlock&& to const-reference const MemoryBlock&, necessary for the new ref-counted MemoryBlock model.
src/core/interface/index.cc	Adds context->reset() calls on all exit paths of Add() and Search() to properly clear per-context state and release any held MemoryBlocks.

Sequence Diagram

sequenceDiagram
    participant C as Caller
    participant BS as BufferStorage
    participant WS as WrappedSegment
    participant BPH as VecBufferPoolHandle
    participant BP as VecBufferPool
    participant LPM as LPMap

    C->>BS: open(path)
    BS->>BP: new VecBufferPool(path)
    BS->>BPH: get_handle()
    BS->>BS: ParseToMapping() → fills segments_, max_segment_size_
    BS->>BP: init(buffer_size_, max_segment_size_, segments_.size())
    BP->>LPM: init(segment_count+10)

    C->>BS: get(segment_id)
    BS-->>C: WrappedSegment

    C->>WS: read(offset, MemoryBlock, len)
    WS->>BPH: get_block(buffer_offset, capacity_, segment_id_)
    BPH->>BP: acquire_buffer(block_id, offset, size, 5)
    BP->>LPM: acquire_block(block_id)
    alt block cached
        LPM-->>BP: char* (ref_count++)
    else block not cached
        BP->>BP: free_buffers_.try_dequeue()
        BP->>BP: pread(fd_, buffer, size, offset)
        BP->>LPM: set_block_acquired(block_id, buffer)
        LPM-->>BP: placed_buffer
    end
    BP-->>BPH: char*
    BPH-->>WS: char*
    WS->>WS: data.reset(handle.get(), segment_id_, raw+offset)
    WS-->>C: MemoryBlock (holds ref)

    C->>C: use MemoryBlock
    C->>C: ~MemoryBlock()
    note over C: destructor calls release_one()
    C->>BPH: release_one(block_id)
    BPH->>LPM: release_block(block_id)
    note over LPM: ref_count-- → if 0, enqueue to LRU cache

Loading

Comments Outside Diff (11)

1. src/core/utility/buffer_storage.cc, line 75-93 (link)

   Reference count leaked on every fetch() call

   get_buffer() calls acquire_buffer() → acquire_block(), which unconditionally performs a ref_count.fetch_add(1). The fetch() function copies the data and returns, but never calls release_block() to decrement the reference. Every call to fetch() permanently increments the block's reference count by one.

   Consequences:

   1. The block is never eligible for eviction (release_block adds it to the LRU only when ref_count drops to 0).
   2. On a hot block, ref_count will grow unboundedly until signed-integer overflow, which is undefined behaviour in C++.
   3. No buffers are returned to free_buffers_, so the pool gradually starves.

   The fix is to call release_block after copying the data. The simplest approach is to mirror the RAII pattern already used in read(MemoryBlock &):

   size_t fetch(size_t offset, void *buf, size_t len) const override {
     // ... bounds check ...
     size_t buffer_offset = ...;
     MemoryBlock block;
     size_t result = const_cast<WrappedSegment*>(this)->read(0, block, capacity_);
     if (!result) return 0;
     auto *data = static_cast<const char*>(block.data()) + offset;
     memmove(buf, data, len);
     return len;
   }

   Or use an explicit scope guard that calls release_block. The same problem affects read(size_t, const void**, size_t) (line 96–113): it also acquires a reference via get_buffer() and returns a raw pointer without ever releasing the reference.

2. src/core/utility/buffer_storage.cc, line 96-113 (link)

   Reference count leaked in read(void**) path

   Same issue as fetch(): get_buffer() increments the block's reference count through acquire_block(), but this read overload returns a raw *data pointer and never calls release_block().

   The pointer returned to the caller remains valid indefinitely (the block cannot be evicted because ref_count ≥ 1 forever), which accidentally provides safety for the raw pointer access. However:

   1. Every call permanently consumes one "slot" in the block's ref_count.
   2. Frequent reads of the same block will cause ref_count to grow without bound, eventually causing signed integer overflow (UB).
   3. Buffer pool resources are never reclaimed.

   // After this line, raw's block has ref_count incremented:
   auto *raw = owner_->get_buffer(buffer_offset, capacity_, segment_id_);
   if (!raw) {
     return 0;
   }
   *data = raw + offset;
   return len;  // ref_count is leaked — release_block is never called

   Consider using the MemoryBlock-based overload internally, or add an explicit release_block call after using the pointer.

3. src/include/zvec/core/framework/index_storage.h, line 54-57 (link)

   acquire_one ignores its return value — silent use-after-free risk in copy assignment

   VecBufferPoolHandle::acquire_one() forwards to LPMap::acquire_block(), which returns nullptr if the block has been concurrently evicted (i.e. its ref_count has been set to INT_MIN). acquire_one discards this return value entirely:

   void VecBufferPoolHandle::acquire_one(block_id_t block_id) {
     pool_.lp_map_.acquire_block(block_id);  // return value silently dropped
   }

   This is exercised in both the copy constructor (line 56) and copy-assignment operator (line 88) of MemoryBlock. In the copy-assignment path, reset() releases the old reference first (decrementing ref_count), potentially allowing another thread to evict the block between the release_one and acquire_one calls. If the eviction succeeds, acquire_block returns nullptr — but acquire_one ignores this. The MemoryBlock now stores a pointer to a buffer that may have been recycled and overwritten, causing a silent use-after-free when data() is dereferenced.

   At minimum, acquire_one should check the return value and propagate failure to the caller.

4. src/core/utility/buffer_storage.cc, line 199-202 (link)

   max_segment_size_ of zero causes buffer_pool_->init() to allocate only 10 buffers

   max_segment_size_ is computed inside ParseSegment. If the index file contains no segments (e.g. a freshly-created index), max_segment_size_ remains 0 and is passed as block_size to buffer_pool_->init():

   ret = buffer_pool_->init(buffer_size_, max_segment_size_, segments_.size());

   Inside init():

   if (block_size == 0) {
     LOG_ERROR("block_size must not be 0");
     return -1;
   }

   This causes open() to return a failure for any empty index, making it impossible to open a freshly-initialised index via BufferStorage. This was not a problem with the previous MMapFile storage path because it did not require a pre-computed block size at open time.

5. src/core/utility/buffer_storage.cc, line 75-82 (link)

   fetch() is a non-functional stub — silently discards all data

   fetch() has been replaced with return 0, meaning it never copies anything into buf. This is a critical regression: memory_read_storage.cc:78 calls block_->fetch(...) expecting data to be written into buf, and will now silently receive zero bytes. Any code path that reads segment data via fetch() will operate on uninitialised memory.

   The full implementation that bounds-checks offset + len and reads from the buffer pool (analogous to the read(MemoryBlock&...) overload below) must be restored.

6. src/core/utility/buffer_storage.cc, line 80-83 (link)

   read(size_t, const void**, size_t) is a non-functional stub — callers receive stale/null pointers

   This overload now unconditionally returns 0 without writing to *data. Every call site that uses the pointer-return variant is now broken:

   • src/core/algorithm/flat/flat_searcher.cc:52,142 — mapping / keys_ pointers left uninitialised
   • src/core/algorithm/hnsw/hnsw_searcher_entity.cc:247,259 — data left uninitialised, leading to UB on dereference
   • src/core/algorithm/hnsw_sparse/hnsw_sparse_searcher_entity.cc:258,270 — same issue
   • src/core/framework/index_helper.cc:83 — version data will be garbage

   All of these callers check the return value (!= len) and proceed to dereference *data when they believe the read succeeded. Since this stub always returns 0 (≠ len), the callers will at minimum log an error and bail — but any caller that does not strictly check might proceed with a stale or null pointer, causing undefined behaviour.

   A concrete implementation that reads from the buffer pool — mirroring the read(MemoryBlock&...) overload — must be provided, or callers must be migrated to the MemoryBlock variant.

7. src/include/zvec/core/framework/index_storage.h, line 154-155 (link)

   Dangling pointer when close_index() races with live MemoryBlock destructors

   buffer_pool_handle_ is stored as a raw non-owning pointer. It points to the VecBufferPoolHandle object whose lifetime is controlled by std::shared_ptr<VecBufferPoolHandle> buffer_pool_handle_ in BufferStorage.

   When close_index() is called (e.g., from close() or the destructor), it calls buffer_pool_handle_.reset(), which destroys the VecBufferPoolHandle. If any MemoryBlock instance (e.g., inside a Neighbors struct still held by a caller) outlives that reset, its stored buffer_pool_handle_ raw pointer is now dangling. The MemoryBlock destructor then calls buffer_pool_handle_->release_one(buffer_block_id_) on that dangling pointer — undefined behaviour that may crash or silently corrupt memory.

   Consider having MemoryBlock hold a std::shared_ptr<VecBufferPoolHandle> instead of a raw pointer to prevent the handle from being destroyed while any block is still pinned.

8. src/core/utility/buffer_storage.cc, line 164-174 (link)

   buffer_pool_->init() called with max_segment_size_ which can be 0

   max_segment_size_ is only updated inside the ParseSegment loop, so it remains 0 if the index file contains no data-carrying segments (e.g., a freshly created index with only the version segment, or an empty index). Passing 0 as block_size to VecBufferPool::init() hits the guard if (block_size == 0) { return -1; }, causing open() to return a hard error and making the index unopenable.

   Additionally, even for valid indices, the max_segment_size_ is only the size of the largest segment. Segments smaller than the max will waste an entire max_segment_size_-sized buffer slot per read. Consider validating max_segment_size_ > 0 before calling init() and returning a clear error if the file has no usable segments.

9. src/core/utility/buffer_storage.cc, line 199-201 (link)

   open() fails for empty or newly-created indices

   max_segment_size_ is initialized to 0 and is only updated inside the for loop in ParseSegment(). If the index file contains no segments (e.g., a freshly written index before any data is added), max_segment_size_ remains 0 after ParseToMapping() returns. This causes buffer_pool_->init() to be called with block_size == 0, which hits the explicit guard at buffer_pool.cc:187 and returns -1, making open() fail entirely.

   A guard (or a fallback minimum block size) is needed before calling init():

   if (max_segment_size_ == 0) {
     // No segments yet; pool will be re-initialized on first open with data.
     return 0;
   }
   ret = buffer_pool_->init(buffer_size_, max_segment_size_, segments_.size());

   Alternatively, init() could fall back to a sensible minimum block size instead of hard-failing when block_size == 0.

10. src/core/utility/buffer_storage.cc, line 87-89 (link)

    fetch() and read(const void**) silently return 0 on buffer pool failure

    When get_buffer() returns nullptr (pool exhausted or I/O error), both fetch() (line 88) and read(const void**, size_t) (line 109) return 0. Since these methods have a size_t return type, callers that use the pattern if (returned_len != requested_len) to detect errors will see 0 != len and may interpret this as a partial/truncated read rather than a hard I/O error — potentially producing silent data corruption downstream.

    The same return 0 pattern also appears at line 109 in read(size_t, const void**, size_t).

    These methods should propagate a clear failure signal. For fetch(), returning 0 can be misread as "zero bytes available". Consider returning a sentinel like (size_t)-1 on error, consistent with POSIX read() semantics, or changing the return type to int/ssize_t for error-aware callers. At minimum, add a LOG_ERROR at these early-exit paths so the failure is visible.

11. src/core/utility/buffer_storage.cc, line 131 (link)

    MemoryBlock stores a dangling raw pointer when storage is closed

    owner_->buffer_pool_handle_.get() returns a raw VecBufferPoolHandle* from the shared_ptr. The MemoryBlock stores this raw pointer and calls buffer_pool_handle_->release_one(buffer_block_id_) in its destructor.

    In close_index() (line 468), buffer_pool_handle_.reset() destroys the VecBufferPoolHandle object. If any MemoryBlock returned by read() is still alive at that point (e.g., held by a caller), the raw pointer stored in the MemoryBlock becomes dangling. When the MemoryBlock destructor subsequently calls release_one(), this is undefined behaviour — a use-after-free.

    To fix this, MemoryBlock should share ownership of the handle's lifetime. For example, store a shared_ptr<VecBufferPoolHandle> instead of a raw pointer, and adjust the MemoryBlock constructors and reset() overloads accordingly. This guarantees the handle stays alive at least as long as any MemoryBlock referencing it.

_{Last reviewed commit: 4231a53}

[greptile-apps]

Greptile Summary

This PR replaces the old BufferManager/BufferHandle-based buffer storage with a new custom VecBufferPool implementation backed by an LRU-eviction map (LPMap) and a lock-free concurrent queue. It adds context->reset() calls throughout Index::Add() and Index::Search() to promptly release buffer-pool references.

The implementation resolves several prior issues:

• Parameter handling: Correctly reads BUFFER_STORAGE_MEMORY_SIZE from params (default 200 MiB)
• Resource cleanup: Destructor properly frees all buffers and file descriptor, even on init failure
• Memory safety: Proper error checking in ParseHeader(), ParseFooter(), ParseSegment()

Critical remaining issues:

• LPMap entry-count calculation (buffer_pool.cc:159) can be smaller than actual segment count, causing out-of-bounds access and crashes for files with many small segments
• fetch() / read(MemoryBlock) error handling (buffer_storage.cc:87-88, 127-128) returns 0 on buffer pool exhaustion, indistinguishable from an empty read—allowing silent data corruption
• Constructor type mismatch (index_storage.h:40) takes int block_id but field is size_t, silently truncating segment IDs on assignment
• Raw pointer lifetime (index_storage.h:157): buffer_pool_handle_ is a raw, non-owning pointer that can dangle if close_index() is called while MemoryBlocks are alive, causing use-after-free in destructors

Confidence Score: 2/5

• Not safe to merge — critical bugs can cause crashes (LPMap out-of-bounds) and silent data corruption (error handling returns 0)
• Four verified logic defects pose high risk: (1) LPMap entry-count calculation can be smaller than segment count, triggering out-of-bounds access and assert failure; (2) fetch()/read() return 0 on buffer acquisition failure, indistinguishable from successful empty reads, allowing callers to proceed with invalid data; (3) constructor type mismatch (int vs size_t) silently truncates segment IDs; (4) raw non-owning pointer to VecBufferPoolHandle can dangle if close_index() is called while MemoryBlocks are alive. These address fundamental safety contracts (bounds, error propagation, lifetime). While context-reset plumbing and parameter-reading fixes are correct, the core buffer pool implementation introduces new defects that outweigh improvements.
• src/ailego/buffer/buffer_pool.cc (entry-count sizing), src/core/utility/buffer_storage.cc (error handling), src/include/zvec/core/framework/index_storage.h (constructor type, pointer lifetime)

Important Files Changed

Filename	Overview
src/ailego/buffer/buffer_pool.cc	New file implementing LRUCache, LPMap, and VecBufferPool. Constructor properly closes fd on error (fixed). Destructor properly frees all buffers (fixed). Critical bug: entry-count calculation at line 159 can be smaller than the number of segments, causing out-of-bounds access and assert failures when segment IDs exceed entry_num.
src/include/zvec/ailego/buffer/buffer_pool.h	Buffer pool and LRU cache interface. VecBufferPoolHandle is now properly a value type with reference semantics (stored in VecBufferPool). Default initializers added. Low risk from this file itself; issues are in pool sizing and callers' type consistency.
src/core/utility/buffer_storage.cc	Major refactor replacing BufferManager with VecBufferPool. Parameter reading fixed (now uses BUFFER_STORAGE_MEMORY_SIZE). Critical bug: fetch() and read(MemoryBlock) return 0 on buffer pool exhaustion (lines 87-88, 127-128), indistinguishable from empty reads. This allows callers to silently continue with invalid/uninitialized data. ParseHeader/Footer/Segment error handling is correct (checks return values).
src/include/zvec/core/framework/index_storage.h	MemoryBlock redesigned to use raw ref-counting via VecBufferPoolHandle. Two critical bugs: (1) constructor takes int block_id but field is size_t, causing implicit narrowing conversion of segment IDs; (2) buffer_pool_handle_ is a raw, non-owning pointer that can dangle if close_index() is called before all MemoryBlocks are destroyed, leading to use-after-free in destructors.

Comments Outside Diff (4)

1. src/include/zvec/core/framework/index_storage.h, line 40 (link)

   The MemoryBlock constructor accepts int block_id (line 40) but assigns it to the field buffer_block_id_ declared as size_t (line 158), causing an implicit narrowing conversion. Meanwhile, the reset() overload correctly uses size_t block_id (line 135).

   Since callers pass size_t values (segment IDs are size_t), this inconsistency can silently truncate large valid segment IDs when passed through the constructor. The constructor should match the reset method's signature:

2. src/include/zvec/core/framework/index_storage.h, line 157 (link)

   buffer_pool_handle_ is a raw, non-owning pointer to the object managed by BufferStorage::buffer_pool_handle_ (a shared_ptr<VecBufferPoolHandle>). If close_index() resets the shared pointer while any MemoryBlock is still alive (e.g., held within a search context or result structure), the raw pointer becomes dangling. The destructor (line 121-122) will then call buffer_pool_handle_->release_one() on freed memory, causing undefined behaviour.

   The safest fix is to have MemoryBlock co-own the handle via a shared_ptr, ensuring the pool remains valid as long as any MemoryBlock references exist. Alternatively, add documentation and runtime assertions that all MemoryBlocks are destroyed before close_index() completes.

3. src/ailego/buffer/buffer_pool.cc, line 159-160 (link)

   The block_num calculation at line 159 can be significantly smaller than the actual number of segments, causing out-of-bounds access.

   Segment IDs are assigned sequentially as segments are parsed (0, 1, ..., N-1 where N = total segment count). These IDs are used directly as LPMap block indices, so the invariant segment_id < block_num must hold. However, block_num = file_size / block_size + 10 is based on the file-size ratio, not segment count.

   Example: 100 segments of 1 MB each plus one 50 MB segment → file_size ≈ 150 MB, max_segment_size = 50 MB, block_num = 3 + 10 = 13. But segment IDs reach 100, triggering an assert and crash in acquire_block()/release_block().

   The entry count should account for the actual segment count:

   // Calculate block_num based on both file size and segment count
   size_t block_num = std::max(segment_count, file_size_ / max_segment_size_) + 10;
   lp_map_.init(block_num);

   Pass segment_count from BufferStorage::open() after parsing metadata, ensuring the pool is initialized with a safe lower bound matching the segment count.

4. src/core/utility/buffer_storage.cc, line 87-88 (link)

   fetch() returns 0 when get_buffer() returns nullptr (line 88). Callers cannot distinguish this from a successful read of zero bytes, potentially allowing silent data corruption.

   The same issue occurs in read(MemoryBlock) (line 128): returning 0 on null buffer acquisition is indistinguishable from an empty segment. Additionally, if raw is null, the pointer arithmetic at line 131 (raw + offset) evaluates to a non-null address (implementation-defined), so the subsequent data.data() check passes as truthy, and the function returns len as if successful—handing the caller an invalid MemoryBlock.

   Both paths should return an error code to signal acquisition failure:

   The convention in this codebase is to return negative values for errors (e.g., IndexError_Runtime = -1), so these should return -1 instead of 0.

_{Last reviewed commit: 1e3712e}

[iaojnh]

@greptile

[iaojnh]

@greptile

[iaojnh]

@greptile

[richyreachy]

use pool_ as member variable

[iaojnh]

done

[richyreachy]

use std::move to avoid copy

[iaojnh]

pool_ is a reference member.

[richyreachy]

try unordered hashmap?

[iaojnh]

done

[richyreachy]

std::unique_ptr<char[]> buffer(new char[sizeof(footer_)]);

[iaojnh]

done

[richyreachy]

std::unique_ptr<char[]> buffer(new char[sizeof(footer_)]);

[iaojnh]

done

[richyreachy]

judge the return value of fetch_add?

[iaojnh]

done

[iaojnh]

@greptile

[iaojnh]

@greptile

[iaojnh]

@greptile

[iaojnh]

@greptile