Skip to content

[Store] Fix with_hard_pin failure in python API #1873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stmatengss merged 4 commits into from
Apr 12, 2026
Merged

[Store] Fix with_hard_pin failure in python API #1873

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dbd3417
fix python interface
0oshowero0 Apr 12, 2026
9c319ac
update python docs and update rust/go
0oshowero0 Apr 12, 2026
420077a
fix
0oshowero0 Apr 12, 2026
27840cc
fix for rust integration
0oshowero0 Apr 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mooncake-integration/store/store_py.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,6 +1397,7 @@ PYBIND11_MODULE(store, m) {
.def(py::init<>())
.def_readwrite("replica_num", &ReplicateConfig::replica_num)
.def_readwrite("with_soft_pin", &ReplicateConfig::with_soft_pin)
.def_readwrite("with_hard_pin", &ReplicateConfig::with_hard_pin)
.def_readwrite("preferred_segments",
Comment on lines 1398 to 1401
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

The new Python-exposed with_hard_pin flag should be covered by the existing Python wheel tests that validate ReplicateConfig property access (e.g., default value and assignment), otherwise this regression can reappear unnoticed.

Copilot uses AI. Check for mistakes.
&ReplicateConfig::preferred_segments)
.def_readwrite("preferred_segment", &ReplicateConfig::preferred_segment)
Expand Down
1 change: 1 addition & 0 deletions mooncake-store/include/store_c.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ typedef void *mooncake_store_t;
struct mooncake_replicate_config {
size_t replica_num;
int with_soft_pin;
int with_hard_pin;
const char **preferred_segments;
Comment on lines 27 to 31
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

Adding with_hard_pin into mooncake_replicate_config changes the struct layout/size for the public C API. Existing callers compiled against the old header will pass a smaller/differently-laid-out struct, and the library will misinterpret preferred_segments as with_hard_pin (and shift the remaining fields), which can cause UB or unintended hard-pinning. Consider introducing a versioned config struct (e.g., mooncake_replicate_config_v2 + new entrypoints) or adding an explicit struct_size/version field so the callee can detect whether with_hard_pin is present; also update downstream language bindings that construct this struct by field name (e.g., the Rust wrapper currently uses a struct literal).

Copilot uses AI. Check for mistakes.
size_t preferred_segments_count;
};
Expand Down
1 change: 1 addition & 0 deletions mooncake-store/src/store_c.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ mooncake::ReplicateConfig to_replicate_config(

config.replica_num = c_config->replica_num;
config.with_soft_pin = c_config->with_soft_pin != 0;
config.with_hard_pin = c_config->with_hard_pin != 0;
if (c_config->preferred_segments &&
Comment on lines 46 to 49
Copy link

Copilot AI Apr 12, 2026

Choose a reason for hiding this comment

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

to_replicate_config() now unconditionally reads c_config->with_hard_pin. With the current C API change, any caller compiled against an older store_c.h (without the new field / different layout) can trigger incorrect reads (e.g., interpreting preferred_segments as the flag) and potentially crash or silently enable hard pinning. If backward compatibility is required, the C API needs a way to communicate the config struct version/size before this field is read.

Copilot uses AI. Check for mistakes.
c_config->preferred_segments_count > 0) {
for (size_t i = 0; i < c_config->preferred_segments_count; ++i) {
Expand Down
Loading