Skip to content

[Store] Skip BatchQuery when offloading_objects is empty (#2138)#2204

Open
io-wy wants to merge 3 commits into
kvcache-ai:mainfrom
io-wy:main
Open

[Store] Skip BatchQuery when offloading_objects is empty (#2138)#2204
io-wy wants to merge 3 commits into
kvcache-ai:mainfrom
io-wy:main

Conversation

@io-wy
Copy link
Copy Markdown

@io-wy io-wy commented May 24, 2026

Fixes #2138

When no objects need offloading (steady state without eviction), OffloadObjects() still built an empty bucket and called BatchQuerySegmentSlices with an empty keys vector. BatchQuery() returned an empty result, triggering an INVALID_REPLICA error log that confused operators.

Root cause

In the non-BucketStorageBackend path:

std::vector<std::string> keys;
keys.reserve(offloading_objects.size());  // 0 when empty
buckets_keys.emplace_back(std::move(keys)); // buckets_keys = { {} }

The empty bucket is then iterated, and BatchQuerySegmentSlices([]) returns INVALID_REPLICA because BatchQuery([]) yields an empty result vector.

Fix

Early-return from OffloadObjects when the input map is empty, avoiding the unnecessary query and log noise.

if (offloading_objects.empty()) return {};

io-wy added 3 commits May 24, 2026 15:15
The boundary check `qp_index > qp_list_.size()` misses the case
where `qp_index == size()`, allowing an out-of-bounds access to
`qp_list_[size()]`. This reads a wild pointer and passes it to
`ibv_modify_qp()`, leading to segfault or silent memory corruption.

Fix by changing `>` to `>=` so that all invalid indices are rejected.
…2138)

When no objects need offloading (steady state without eviction),
OffloadObjects() still built an empty bucket and called
BatchQuerySegmentSlices with an empty keys vector. BatchQuery()
returned an empty result, triggering an INVALID_REPLICA error log
that confused operators.

Fix by early-returning from OffloadObjects when the input map is
empty, avoiding the unnecessary query and log noise.
Copilot AI review requested due to automatic review settings May 24, 2026 07:27
@io-wy io-wy requested review from XucSh, YiXR, stmatengss and ykwd as code owners May 24, 2026 07:27
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a fast-path in FileStorage::OffloadObjects to skip bucketization/backend work when there’s nothing to offload.

Changes:

  • Return early from OffloadObjects when offloading_objects is empty.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an early return in the FileStorage::OffloadObjects function when the offloading_objects map is empty, preventing unnecessary processing. As there were no review comments provided for this change, I have no feedback to provide.

@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
mooncake-store/src/file_storage.cpp 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@ykwd ykwd requested a review from LujhCoconut May 25, 2026 03:24
@LujhCoconut
Copy link
Copy Markdown
Collaborator

Thanks for this fix. Your approach is correct. However, this issue has already been fixed in PR #2151.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

4 participants