(kvrocks) branch unstable updated: fix(set): propagate storage errors in Set instead of treating as NotFound (#3428)

[jihuayu]

This is an automated email from the ASF dual-hosted git repository.

jihuayu pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git


The following commit(s) were added to refs/heads/unstable by this push:
     new b272b5c1b fix(set): propagate storage errors in Set instead of 
treating as NotFound (#3428)
b272b5c1b is described below

commit b272b5c1b673385ca6e58cf5c595fce252782e22
Author: Songqing Zhang <zhangsongqing...@163.com>
AuthorDate: Thu Apr 9 09:01:42 2026 +0800

    fix(set): propagate storage errors in Set instead of treating as NotFound 
(#3428)
    
    Previously, when `storage_->Get()` returned a non-NotFound error (e.g.,
    I/O error or corruption) during SADD, the code fell through to the
    `batch->Put` path, incorrectly treating the error as "member not found"
    and adding the member. This could silently inflate `metadata.size` and
    insert duplicate members on storage failures.
    
    Add an explicit `!s.IsNotFound()` check to propagate real errors, which
    is consistent with how `Set::Remove` already handles the same pattern.
    
    ---------
    
    Co-authored-by: 纪华裕 <jihuayu...@gmail.com>
---
 src/types/redis_set.cc          |  4 +++-
 tests/cppunit/types/set_test.cc | 51 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/types/redis_set.cc b/src/types/redis_set.cc
index 07d5c7829..a2f4c4ad8 100644
--- a/src/types/redis_set.cc
+++ b/src/types/redis_set.cc
@@ -78,6 +78,7 @@ rocksdb::Status Set::Add(engine::Context &ctx, const Slice 
&user_key, const std:
     std::string sub_key = InternalKey(ns_key, member, metadata.version, 
storage_->IsSlotIdEncoded()).Encode();
     s = storage_->Get(ctx, ctx.GetReadOptions(), sub_key, &value);
     if (s.ok()) continue;
+    if (!s.IsNotFound()) return s;
     s = batch->Put(sub_key, Slice());
     if (!s.ok()) return s;
     *added_cnt += 1;
@@ -114,7 +115,8 @@ rocksdb::Status Set::Remove(engine::Context &ctx, const 
Slice &user_key, const s
     }
     std::string sub_key = InternalKey(ns_key, member, metadata.version, 
storage_->IsSlotIdEncoded()).Encode();
     s = storage_->Get(ctx, ctx.GetReadOptions(), sub_key, &value);
-    if (!s.ok()) continue;
+    if (s.IsNotFound()) continue;
+    if (!s.ok()) return s;
     s = batch->Delete(sub_key);
     if (!s.ok()) return s;
     *removed_cnt += 1;
diff --git a/tests/cppunit/types/set_test.cc b/tests/cppunit/types/set_test.cc
index 44f230d23..41746f4fd 100644
--- a/tests/cppunit/types/set_test.cc
+++ b/tests/cppunit/types/set_test.cc
@@ -248,6 +248,57 @@ TEST_F(RedisSetTest, InterCard) {
   s = set_->Del(*ctx_, k4);
 }
 
+TEST_F(RedisSetTest, AddExistingMembers) {
+  uint64_t ret = 0;
+  rocksdb::Status s = set_->Add(*ctx_, key_, fields_, &ret);
+  EXPECT_TRUE(s.ok());
+  EXPECT_EQ(fields_.size(), ret);
+
+  s = set_->Add(*ctx_, key_, fields_, &ret);
+  EXPECT_TRUE(s.ok());
+  EXPECT_EQ(0, ret);
+
+  uint64_t card = 0;
+  s = set_->Card(*ctx_, key_, &card);
+  EXPECT_TRUE(s.ok());
+  EXPECT_EQ(fields_.size(), card);
+
+  std::vector<Slice> mixed_members = {"set-key-1", "set-key-new-1", 
"set-key-new-2"};
+  s = set_->Add(*ctx_, key_, mixed_members, &ret);
+  EXPECT_TRUE(s.ok());
+  EXPECT_EQ(2, ret);
+
+  s = set_->Card(*ctx_, key_, &card);
+  EXPECT_TRUE(s.ok());
+  EXPECT_EQ(fields_.size() + 2, card);
+}
+
+TEST_F(RedisSetTest, RemoveNonExistingMembers) {
+  uint64_t ret = 0;
+  rocksdb::Status s = set_->Add(*ctx_, key_, fields_, &ret);
+  EXPECT_TRUE(s.ok());
+  EXPECT_EQ(fields_.size(), ret);
+
+  std::vector<Slice> non_existing = {"non-exist-1", "non-exist-2"};
+  s = set_->Remove(*ctx_, key_, non_existing, &ret);
+  EXPECT_TRUE(s.ok());
+  EXPECT_EQ(0, ret);
+
+  uint64_t card = 0;
+  s = set_->Card(*ctx_, key_, &card);
+  EXPECT_TRUE(s.ok());
+  EXPECT_EQ(fields_.size(), card);
+
+  std::vector<Slice> mixed = {"set-key-1", "non-exist-1"};
+  s = set_->Remove(*ctx_, key_, mixed, &ret);
+  EXPECT_TRUE(s.ok());
+  EXPECT_EQ(1, ret);
+
+  s = set_->Card(*ctx_, key_, &card);
+  EXPECT_TRUE(s.ok());
+  EXPECT_EQ(fields_.size() - 1, card);
+}
+
 TEST_F(RedisSetTest, Overwrite) {
   uint64_t ret = 0;
   rocksdb::Status s = set_->Add(*ctx_, key_, fields_, &ret);