lib/segment/src/index/field_index/full_text_index/inverted_index/mod.rs (1)

144-146: De-duplicate Document heap estimators to avoid drift.

Line [144] currently repeats the same computation as Line [121]. Consider delegating one to the other so future changes stay in sync.

♻️ Minimal refactor

 pub fn heap_size(&self) -> usize {
-    self.0.capacity() * std::mem::size_of::<TokenId>()
+    self.heap_bytes()
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/segment/src/index/field_index/full_text_index/inverted_index/mod.rs`
around lines 144 - 146, The heap_size calculation in pub fn heap_size(&self) ->
usize duplicates the same capacity * size_of::<TokenId>() logic found in the
Document heap estimator; refactor by delegating one estimator to the other to
avoid drift—replace the duplicated arithmetic with a call to the existing
Document heap estimator (e.g., call the other struct/impl's heap_size method) so
both functions share a single implementation and future changes stay in sync;
keep references to TokenId and the original heap_size method names when locating
the code to update.

lib/segment/src/vector_storage/volatile_chunked_vectors.rs (1)

54-65: Include outer chunks Vec buffer in heap_size_bytes() for a more complete estimate.

Current estimate counts inner chunk payload buffers but not the heap backing array for self.chunks.

♻️ Suggested refinement

 pub fn heap_size_bytes(&self) -> usize {
@@
-    chunks
+    let outer_vec_bytes = chunks.capacity() * mem::size_of::<Vec<T>>();
+    outer_vec_bytes
+        + chunks
         .iter()
         .map(|chunk| chunk.capacity() * mem::size_of::<T>())
         .sum()
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/segment/src/vector_storage/volatile_chunked_vectors.rs` around lines 54 -
65, heap_size_bytes currently sums only inner chunk payloads; include the heap
backing buffer for the outer Vec by adding chunks.capacity() *
mem::size_of::<Vec<T>>() to the total. In the heap_size_bytes method (function
name heap_size_bytes) reference the struct field chunks and compute the outer
buffer size via chunks.capacity() * mem::size_of::<Vec<T>>() and add it to the
existing sum of inner chunk.capacity() * mem::size_of::<T>() so the returned
value accounts for both the outer Vec's heap array and each inner chunk's
payload.

lib/quantization/src/encoded_vectors_pq.rs (1)

643-653: Consider including metadata container allocations in this estimate.

Right now this counts inner centroid buffers but not the outer Vec<Vec<f32>> allocation (and vector_division allocation), which systematically underestimates heap usage.

♻️ Suggested refinement

 fn heap_size_bytes(&self) -> usize {
     let storage_heap = self.encoded_vectors.heap_size_bytes();
-    // PQ centroids: Vec<Vec<f32>> — each inner Vec holds dim floats
-    let centroids_heap: usize = self
+    let centroids_inner_heap: usize = self
         .metadata
         .centroids
         .iter()
         .map(|c| c.capacity() * std::mem::size_of::<f32>())
         .sum();
-    storage_heap + centroids_heap
+    let centroids_outer_heap =
+        self.metadata.centroids.capacity() * std::mem::size_of::<Vec<f32>>();
+    let vector_division_heap =
+        self.metadata.vector_division.capacity() * std::mem::size_of::<std::ops::Range<usize>>();
+
+    storage_heap + centroids_inner_heap + centroids_outer_heap + vector_division_heap
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/quantization/src/encoded_vectors_pq.rs` around lines 643 - 653, The
heap_size_bytes implementation only sums inner centroid buffers and storage but
misses heap allocations for the outer Vec<Vec<f32>> and any vector_division
container in metadata; update heap_size_bytes to also add
metadata.centroids.capacity() * std::mem::size_of::<Vec<f32>>() to account for
the outer Vec's heap, and add the heap cost of metadata.vector_division (or the
actual field name) as metadata.vector_division.capacity() *
std::mem::size_of::<T>() using the correct element type (e.g., usize or u32), so
the function (in encoded_vectors_pq::heap_size_bytes) returns storage_heap +
centroids_heap + outer_centroids_heap + vector_division_heap.

lib/collection/src/shards/local_shard/mod.rs (1)

1096-1105: Drop the segment-holder lock before running the probes.

This keeps segments.read() held across every per-segment memory_report() call, and those probes can be slow. Please clone the LockedSegments first, release the holder lock, then walk the cloned list so this endpoint does not block shard mutations for its full runtime.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/collection/src/shards/local_shard/mod.rs` around lines 1096 - 1105, The
current code holds the segments read lock across every per-segment probe
(segments.read() then iterating and calling
collect_memory_reports/locked_segment.memory_report()), causing long blocking of
shard mutations; fix by cloning the LockedSegment references (e.g., collect
locked_segment.clone() or clone Arc/handles from the iterator into a
Vec<LockedSegment>) immediately after calling segments.read(), drop the read
guard, and then iterate the cloned Vec to call collect_memory_reports /
memory_report, so the holder lock is released before running the slow probes.

tests/manual/test_memory_reporting.py (1)

148-151: Remove unnecessary f-prefix from string without placeholders.

✏️ Proposed fix

-    print(f"\nCollection has:")
+    print("\nCollection has:")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/manual/test_memory_reporting.py` around lines 148 - 151, The three
print statements in tests/manual/test_memory_reporting.py use f-strings without
placeholders (print(f"\nCollection has:"), print(f"  Indexed payload fields:
{indexed_fields}"), print(f"  Dense vector fields: {dense_vector_names}"),
print(f"  Sparse vector fields: {sparse_vector_names}")). Remove the unnecessary
f-prefix from the literal-only strings (e.g., change print(f"\nCollection has:")
to print("\nCollection has:")); keep the f-strings for lines that actually
interpolate variables (or convert all three to non-f-strings and use
concatenation/formatting consistently if desired). Ensure the changes target the
print calls referencing indexed_fields, dense_vector_names, and
sparse_vector_names.

lib/collection/src/common/memory_reporter.rs (3)

102-102: Optional: Consider using HashMap for O(1) lookups in merge.

The current implementation uses linear search (find) for each vector name during merge. With many shards or named vectors, this is O(n²). A HashMap<String, usize> mapping names to indices would provide O(1) lookups.

Given that named vectors per collection are typically few and this is a reporting endpoint (not a hot path), this is low priority.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/collection/src/common/memory_reporter.rs` at line 102, The merge loop
uses result.vectors.iter_mut().find(|v| v.name == *name) causing O(n²) behavior;
change Merge logic in memory_reporter (the function where result.vectors is
built) to first build a HashMap<String, usize> mapping vector name to its index,
then use that map for O(1) lookups and update entries by index in result.vectors
instead of calling find each time; ensure the map is kept in sync when inserting
new named vectors into result.vectors.

---

221-229: Consider using a match for FileStorageIntent and logging metadata failures.

Two observations:

1. The if equality check won't surface new FileStorageIntent variants at compile time—a match would.
2. Silently ignoring files that fail metadata() differs from the Unix version which propagates errors. Consider logging a warning for debuggability.

Suggested change using match

     for entry in files {
         let ComponentFileEntry { path, intent } = &entry;
-        if let Ok(meta) = std::fs::metadata(path) {
-            disk_bytes += meta.len();
-            if *intent == FileStorageIntent::Cached {
-                expected_cache_bytes += meta.len();
+        match std::fs::metadata(path) {
+            Ok(meta) => {
+                disk_bytes += meta.len();
+                match intent {
+                    FileStorageIntent::Cached => {
+                        expected_cache_bytes += meta.len();
+                    }
+                    FileStorageIntent::Pinned | FileStorageIntent::OnDisk => {}
+                }
             }
+            Err(err) => {
+                log::debug!("Failed to get metadata for {}: {err}", path.display());
+            }
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/collection/src/common/memory_reporter.rs` around lines 221 - 229, Replace
the `if *intent == FileStorageIntent::Cached` check with a `match` on `intent`
so new `FileStorageIntent` variants are handled at compile time (locate the loop
iterating `files` and pattern `let ComponentFileEntry { path, intent }`). Also
change the `if let Ok(meta) = std::fs::metadata(path)` to explicitly handle the
`Err` case by logging a warning (include the `path` and error) instead of
silently ignoring failures; continue to update `disk_bytes` and
`expected_cache_bytes` based on `meta.len()` inside the successful branch.

---

105-109: Prefer explicit match arms over catch-all _.

Per coding guidelines, explicit exhaustive matches are preferred to ensure future changes don't silently fall through.

Suggested change

                     match (&mut existing.quantized, quantized) {
                         (Some(a), Some(b)) => a.merge(b),
                         (None, Some(b)) => existing.quantized = Some(b.clone()),
-                        _ => {}
+                        (Some(_), None) | (None, None) => {}
                     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/collection/src/common/memory_reporter.rs` around lines 105 - 109, The
match on (&mut existing.quantized, quantized) uses a catch-all `_ => {}`;
replace it with explicit arms for the remaining cases to satisfy exhaustive
matching: add (Some(_), None) => { /* no-op or comment */ } and (None, None) =>
{ /* no-op or comment */ } (or return/unreachable if appropriate) so the match
covers (Some(a), Some(b)), (None, Some(b)), (Some(_), None) and (None, None) for
existing.quantized and quantized in memory_reporter.rs.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/manual/test_memory_reporting.py (1)

121-121: ⚠️ Potential issue | 🟡 Minor

Remove unnecessary f-prefix.

This f-string has no placeholders. Use a regular string instead.

-    print(f"\nUploading snapshot to Qdrant...")
+    print("\nUploading snapshot to Qdrant...")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/manual/test_memory_reporting.py` at line 121, The print call uses an
unnecessary f-string with no placeholders (print(f"\nUploading snapshot to
Qdrant...")), so replace it with a regular string literal by removing the
leading 'f' (i.e., change the print call in
tests/manual/test_memory_reporting.py that prints "\nUploading snapshot to
Qdrant..." to use print("\nUploading snapshot to Qdrant...")).

lib/segment/src/index/field_index/map_index/immutable_map_index.rs (1)

233-260: ⚠️ Potential issue | 🟠 Major

Refresh cached RAM after remove_point mutations.

cached_ram_usage_bytes is initialized once (Line 136) but not updated after Line 233–260 mutates in-memory containers, so reported RAM can become stale.

💡 Suggested fix

     pub fn remove_point(&mut self, idx: PointOffsetType) -> OperationResult<()> {
@@
         }
         self.point_to_values.remove_point(idx);
+        self.cached_ram_usage_bytes = self.compute_ram_usage_bytes();
         Ok(())
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/segment/src/index/field_index/map_index/immutable_map_index.rs` around
lines 233 - 260, The remove_point method mutates in-memory containers
(value_to_points, value_to_points_container, deleted_value_to_points_container,
point_to_values, values_count, indexed_points) but never updates
cached_ram_usage_bytes, causing stale RAM reports; after performing the removals
and before returning from remove_point, recompute and assign the fresh RAM usage
to self.cached_ram_usage_bytes (by invoking the existing RAM-calculation helper
if one exists, or by adding a short inline recalculation) so
cached_ram_usage_bytes reflects the updated memory state.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/manual/test_memory_reporting.py (3)

45-63: ⚠️ Potential issue | 🟡 Minor

Add timeouts to prevent indefinite hangs.

The helper functions get_allocated_bytes(), get_memory_report(), and get_collection_info() make HTTP requests without timeouts. If the server becomes unresponsive, the test will hang indefinitely.

Proposed fix

 def get_allocated_bytes() -> int:
     """Get jemalloc allocated_bytes from telemetry."""
-    resp = requests.get(f"{BASE_URL}/telemetry", params={"details_level": 2})
+    resp = requests.get(f"{BASE_URL}/telemetry", params={"details_level": 2}, timeout=30)
     resp.raise_for_status()
     return resp.json()["result"]["memory"]["allocated_bytes"]


 def get_memory_report(collection_name: str) -> dict:
     """Get the collection memory report."""
-    resp = requests.get(f"{BASE_URL}/collections/{collection_name}/memory")
+    resp = requests.get(f"{BASE_URL}/collections/{collection_name}/memory", timeout=30)
     resp.raise_for_status()
     return resp.json()["result"]


 def get_collection_info(collection_name: str) -> dict:
     """Get collection info."""
-    resp = requests.get(f"{BASE_URL}/collections/{collection_name}")
+    resp = requests.get(f"{BASE_URL}/collections/{collection_name}", timeout=30)
     resp.raise_for_status()
     return resp.json()["result"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/manual/test_memory_reporting.py` around lines 45 - 63, The three
helpers get_allocated_bytes(), get_memory_report(), and get_collection_info()
call requests.get without a timeout which can cause tests to hang; update each
requests.get call to pass a sensible timeout (e.g., timeout=10 or a shared
constant like REQUEST_TIMEOUT_SECONDS) so the test fails fast on unresponsive
servers, preserving resp.raise_for_status() and the existing JSON parsing logic
in each function.

---

109-121: ⚠️ Potential issue | 🟡 Minor

Add timeouts and remove unnecessary f-prefix.

Lines 110 and 115 need timeouts, and line 121 has an f-string without placeholders.

Proposed fix

     # Verify server is up
-    resp = requests.get(f"{BASE_URL}/collections")
+    resp = requests.get(f"{BASE_URL}/collections", timeout=10)
     resp.raise_for_status()
     print("Qdrant is running.")

     # Clean up any leftover collection from a previous run
-    requests.delete(f"{BASE_URL}/collections/{COLLECTION_NAME}", params={"wait": "true"})
+    requests.delete(f"{BASE_URL}/collections/{COLLECTION_NAME}", params={"wait": "true"}, timeout=60)

     # Get snapshot from local cache (download once if needed)
     snapshot_path = get_cached_snapshot()

     # Upload snapshot to Qdrant
-    print(f"\nUploading snapshot to Qdrant...")
+    print("\nUploading snapshot to Qdrant...")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/manual/test_memory_reporting.py` around lines 109 - 121, The requests
calls lack timeouts and the print uses an unnecessary f-string; update the
requests.get and requests.delete calls to include a timeout parameter (e.g.,
timeout=10) to avoid hanging (modify the call sites in this snippet where
requests.get(...) and requests.delete(..., params=...) are used), and change
print(f"\nUploading snapshot to Qdrant...") to print("\nUploading snapshot to
Qdrant...") to remove the unused f-prefix; keep the existing params and behavior
of get_cached_snapshot() and resp.raise_for_status() unchanged.

---

77-93: ⚠️ Potential issue | 🟡 Minor

Add timeouts to delete operations.

The delete helper functions should also have timeouts to prevent hanging.

Proposed fix

 def delete_payload_index(collection_name: str, field_name: str):
     """Delete a payload index."""
     resp = requests.delete(
         f"{BASE_URL}/collections/{collection_name}/index/{field_name}",
         params={"wait": "true"},
+        timeout=120,
     )
     resp.raise_for_status()


 def delete_vector_field(collection_name: str, vector_name: str):
     """Delete a named vector field entirely."""
     resp = requests.delete(
         f"{BASE_URL}/collections/{collection_name}/vectors/{vector_name}",
         params={"wait": "true"},
+        timeout=120,
     )
     resp.raise_for_status()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/manual/test_memory_reporting.py` around lines 77 - 93, The two helper
functions delete_payload_index and delete_vector_field lack request timeouts and
can hang; update both to pass a timeout (e.g., timeout=10) to requests.delete
(same place where params is passed) so the HTTP call times out predictably, then
keep resp.raise_for_status() unchanged to surface errors.

lib/segment/src/index/field_index/full_text_index/inverted_index/mod.rs (1)

144-146: Avoid duplicating heap-size logic in Document.

heap_size() currently re-implements the exact same formula as heap_bytes(). This can drift later; make one delegate to the other.

♻️ Proposed refactor

     pub fn heap_size(&self) -> usize {
-        self.0.capacity() * std::mem::size_of::<TokenId>()
+        self.heap_bytes()
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/segment/src/index/field_index/full_text_index/inverted_index/mod.rs`
around lines 144 - 146, The heap_size() implementation duplicates the same
formula as heap_bytes(), so change heap_size() to delegate to the existing
heap_bytes() calculation instead of re-implementing it: replace the body of
heap_size() to simply return the result of heap_bytes() (ensuring types match,
i.e., usize) so only heap_bytes() contains the actual logic; update references
to TokenId only in heap_bytes() if needed and keep function names heap_size and
heap_bytes as the single source of truth.

lib/segment/src/payload_storage/memory_reporter.rs (1)

9-12: Avoid silent 0 fallback on RAM estimate failure.

At Line 11, swallowing errors with unwrap_or(0) can hide estimation failures and produce misleading reports. Consider logging the failure before fallback.

💡 Suggested adjustment

 impl MemoryReporter for PayloadStorageEnum {
     fn memory_usage(&self) -> ComponentMemoryUsage {
         match self {
             #[cfg(feature = "testing")]
             PayloadStorageEnum::InMemoryPayloadStorage(s) => {
-                // Purely in-memory, no files. Approximate RAM from storage size.
-                ComponentMemoryUsage::ram_only(s.get_storage_size_bytes().unwrap_or(0) as u64)
+                // Purely in-memory, no files. Approximate RAM from storage size.
+                let bytes = match s.get_storage_size_bytes() {
+                    Ok(v) => v as u64,
+                    Err(err) => {
+                        log::warn!("Failed to estimate in-memory payload storage size: {err}");
+                        0
+                    }
+                };
+                ComponentMemoryUsage::ram_only(bytes)
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/segment/src/payload_storage/memory_reporter.rs` around lines 9 - 12,
Replace the silent unwrap_or(0) in the
PayloadStorageEnum::InMemoryPayloadStorage branch: call
s.get_storage_size_bytes(), match its Result, and on Err log the error with
details (using the crate's logger/tracing) before falling back to 0; then pass
the numeric size into ComponentMemoryUsage::ram_only so failures are recorded
instead of silently producing 0.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes