Skip to content

fix: codis slot migrate #3233

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

Open
chejinge wants to merge 13 commits into
base: unstable
Choose a base branch
from
+22 −4
Open

fix: codis slot migrate #3233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
655745e
fix:remove dependence in CI
chejinge Aug 8, 2024
81f5c98
Revert "fix: change storage ttl time from seconds to milliseconds (#2…
cheniujh Aug 8, 2024
e041fb1
fix CI
brother-jin Aug 9, 2024
65715a0
Merge branch 'OpenAtomFoundation:unstable' into unstable
chejinge Aug 19, 2024
b3d763e
Merge branch 'OpenAtomFoundation:unstable' into unstable
chejinge Nov 4, 2024
29bfafa
Merge branch 'OpenAtomFoundation:unstable' into unstable
chejinge Nov 20, 2024
2b63123
Merge branch 'OpenAtomFoundation:unstable' into unstable
chejinge Dec 4, 2024
6d092c7
Merge branch 'OpenAtomFoundation:unstable' into unstable
chejinge Sep 2, 2025
9526008
Merge branch 'OpenAtomFoundation:unstable' into unstable
chejinge Sep 8, 2025
71d23c3
Merge branch 'OpenAtomFoundation:unstable' into unstable
chejinge Oct 11, 2025
fb9a156
Merge branch 'OpenAtomFoundation:unstable' into unstable
chejinge Oct 15, 2025
5771e2f
Merge branch 'OpenAtomFoundation:unstable' into unstable
chejinge Feb 3, 2026
1292ab8
fix_slot_migrate
brother-jin Apr 9, 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
2 changes: 1 addition & 1 deletion src/pika_migrate_thread.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,7 @@ int PikaMigrateThread::ReqMigrateOne(const std::string &key, const std::shared_p
NotifyRequestMigrate();
}

return 1;
return 0;
}

void PikaMigrateThread::GetMigrateStatus(std::string *ip, int64_t* port, int64_t *slot, bool *migrating, int64_t *moved,
Expand Down
24 changes: 21 additions & 3 deletions src/pika_slot_command.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1439,10 +1439,28 @@ void SlotsMgrtExecWrapperCmd::Do() {
res_.AppendArrayLen(2);
int ret = g_pika_server->SlotsMigrateOne(key_, db_);
switch (ret) {
case 0:
res_.AppendInteger(0);
res_.AppendInteger(0);
case 0: {
res_.AppendInteger(2);
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 9, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Document the new 2 result code.

The contract immediately above Do() still says this wrapper only returns -1/0/1, but Line 1443 now emits 2. Please update the command contract/tests with the new discriminator so callers do not keep treating this path as an unknown response.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/pika_slot_command.cc` at line 1443, The command wrapper now returns a new
discriminator value 2 (res_.AppendInteger(2)) but the Do() contract and related
tests still document/expect only -1/0/1; update the contract comment above Do()
to list and describe the new result code 2 and modify any unit/integration tests
that assert allowed responses to include 2 so callers handle it explicitly
(search for Do(), res_.AppendInteger(2), and tests referencing -1/0/1 to locate
all occurrences).

PikaCmdArgsType args;
for (size_t i = 2; i < argv_.size(); ++i) {
args.push_back(argv_[i]);
}
std::string opt = args[0];
pstd::StringToLower(opt);
std::shared_ptr<Cmd> c_ptr = g_pika_cmd_table_manager->GetCmd(opt);
if (!c_ptr) {
res_.SetRes(CmdRes::kErrOther, "unknown command");
return;
}
c_ptr->Initial(args, db_->GetDBName());
if (!c_ptr->res().ok()) {
res_.SetRes(CmdRes::kErrOther, c_ptr->res().message());
return;
}
c_ptr->Execute();
Comment on lines +1444 to +1460
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 9, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Reject wrapped commands that do not resolve back to key_.

This branch executes client-supplied args verbatim, but never checks that the parsed command actually targets the wrapper key. A request like slotsmgrtexecwrapper foo set bar 1 will route on foo and mutate bar, and multi-key/admin commands can also slip through this path. During slot migration that breaks the single-slot guarantee and can send writes to the wrong shard. Validate args before indexing args[0], then reject anything whose current_key() is empty, multi-key, or different from key_ before Execute().

🔒 Suggested guardrails 
       PikaCmdArgsType args;
       for (size_t i = 2; i < argv_.size(); ++i) {
         args.push_back(argv_[i]);
       }
+      if (args.empty()) {
+        res_.SetRes(CmdRes::kWrongNum, kCmdNameSlotsMgrtExecWrapper);
+        return;
+      }
       std::string opt = args[0];
       pstd::StringToLower(opt);
       std::shared_ptr<Cmd> c_ptr = g_pika_cmd_table_manager->GetCmd(opt);
       if (!c_ptr) {
         res_.SetRes(CmdRes::kErrOther, "unknown command");
         return;
       }
       c_ptr->Initial(args, db_->GetDBName());
       if (!c_ptr->res().ok()) {
         res_.SetRes(CmdRes::kErrOther, c_ptr->res().message());
         return;
       }
+      const auto keys = c_ptr->current_key();
+      if (keys.size() != 1 || keys[0] != key_) {
+        res_.SetRes(CmdRes::kErrOther, "wrapped command must target the wrapper key");
+        return;
+      }
       c_ptr->Execute();
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/pika_slot_command.cc` around lines 1444 - 1460, The handler currently
builds args and then directly indexes args[0], gets a Cmd via
g_pika_cmd_table_manager->GetCmd(opt) and runs Initial()/Execute() without
verifying the resolved target key; change the flow to first ensure args is
non-empty, then lookup and call c_ptr->Initial(args, db_->GetDBName()), check
c_ptr->res().ok(), then inspect c_ptr->current_key(): if current_key() is empty,
indicates multi-key (or multi-key flag if available), or current_key() != key_,
reject by setting res_.SetRes(CmdRes::kErrOther, "wrapped command targets wrong
key") (or similar), and only call c_ptr->Execute() when current_key() is single
and equals key_; use the symbols args, opt, c_ptr, Initial, res().ok(),
current_key(), key_, and Execute to locate and implement this guard.

res_.AppendStringRaw(c_ptr->res().message());
return;
}
case 1:
res_.AppendInteger(1);
res_.AppendInteger(1);
Expand Down
Loading