fix: crash for lazy expired sets (#6979)

Author: BorysTheDev

src/server/set_family.cc

@@ -54,6 +54,24 @@ bool IsDenseEncoding(const CompactObj& co) {
   return co.Encoding() == kEncodingStrMap2;
 }
 
+// After iterating a StringSet with set_time(), lazy member expiry may have emptied it.
+// Per Redis semantics empty collections must not exist as keys, so delete the stale key.
+// Returns true if the key was deleted.
+bool DeleteSetIfEmpty(DbSlice& db_slice, const DbContext& db_cntx, string_view key,
+                      const PrimeValue& pv) {
+  if (!IsDenseEncoding(pv))
+    return false;
+
+  if (StringSet* ss = (StringSet*)pv.RObjPtr(); !ss->Empty())
+    return false;
+
+  if (auto res = db_slice.FindMutable(db_cntx, key, OBJ_SET); res) {
+    db_slice.DelMutable(db_cntx, std::move(*res));
+    return true;
+  }
+  return false;
+}
+
 intset* IntsetAddSafe(string_view val, intset* is, bool* success, bool* added) {
   long long llval;
   *added = false;
@@ -725,8 +743,9 @@ OpResult<StringVec> OpUnion(const OpArgs& op_args, ShardArgs::Iterator start,
   DCHECK(start != end);
   absl::flat_hash_set<string> uniques;
 
+  auto& db_slice = op_args.GetDbSlice();
   for (; start != end; ++start) {
-    auto find_res = op_args.GetDbSlice().FindReadOnly(op_args.db_cntx, *start, OBJ_SET);
+    auto find_res = db_slice.FindReadOnly(op_args.db_cntx, *start, OBJ_SET);
     if (find_res) {
       const PrimeValue& pv = find_res.value()->second;
       if (IsDenseEncoding(pv)) {
@@ -737,6 +756,7 @@ OpResult<StringVec> OpUnion(const OpArgs& op_args, ShardArgs::Iterator start,
         uniques.emplace(ce.ToString());
         return true;
       });
+      DeleteSetIfEmpty(db_slice, op_args.db_cntx, *start, pv);
       continue;
     }
 
@@ -772,7 +792,11 @@ OpResult<StringVec> OpDiff(const OpArgs& op_args, ShardArgs::Iterator start,
     return true;
   });
 
-  DCHECK(!uniques.empty());  // otherwise the key would not exist.
+  // Lazy per-member TTL expiry during iteration may have emptied the set.
+  // Delete the stale key and return KEY_NOTFOUND per Redis empty-key semantics.
+  if (DeleteSetIfEmpty(db_slice, op_args.db_cntx, *start, pv)) {
+    return OpStatus::KEY_NOTFOUND;
+  }
 
   for (++start; start != end; ++start) {
     auto diff_res = db_slice.FindReadOnly(op_args.db_cntx, *start, OBJ_SET);
@@ -783,7 +807,8 @@ OpResult<StringVec> OpDiff(const OpArgs& op_args, ShardArgs::Iterator start,
       continue;  // KEY_NOTFOUND
     }
 
-    SetType st2{diff_res.value()->second.RObjPtr(), diff_res.value()->second.Encoding()};
+    const PrimeValue& diff_pv = diff_res.value()->second;
+    SetType st2{diff_pv.RObjPtr(), diff_pv.Encoding()};
     if (st2.second == kEncodingIntSet) {
       int ii = 0;
       intset* is = (intset*)st2.first;
@@ -796,6 +821,7 @@ OpResult<StringVec> OpDiff(const OpArgs& op_args, ShardArgs::Iterator start,
       }
     } else {
       DiffStrSet(op_args.db_cntx, st2, &uniques);
+      DeleteSetIfEmpty(db_slice, op_args.db_cntx, *start, diff_pv);
     }
   }
 
@@ -830,6 +856,7 @@ OpResult<StringVec> OpInter(const Transaction* t, EngineShard* es, bool remove_f
                                   result.push_back(ce.ToString());
                                   return true;
                                 });
+    DeleteSetIfEmpty(db_slice, t->GetDbContext(), *it, pv);
     return result;
   }
 

src/server/set_family_test.cc

@@ -538,4 +538,52 @@ TEST_F(SetFamilyTest, StoreOverwritesNonSetKeyAccounting) {
   EXPECT_EQ(after3.db_stats[0].memory_usage_by_type[OBJ_LIST], 0u);
 }
 
+// Regression test for #6973: SDIFF/SDIFFSTORE crash when all set members
+// have expired via per-member TTL, leaving the key present but the set empty.
+TEST_F(SetFamilyTest, SDiffAllMembersExpired) {
+  TEST_current_time_ms = kMemberExpiryBase * 1000;
+
+  // Add members with a short TTL.
+  Run({"saddex", "src", "1", "a", "b", "c"});
+  Run({"sadd", "other", "x"});
+
+  // Advance time so all members in "src" expire.
+  AdvanceTime(2000);
+
+  // SDIFF should return empty (like KEY_NOTFOUND), not crash.
+  auto resp = Run({"sdiff", "src", "other"});
+  EXPECT_THAT(resp, ArrLen(0));
+
+  // The key must be deleted after lazy expiry emptied the set.
+  EXPECT_THAT(Run({"exists", "src"}), IntArg(0));
+
+  // SDIFFSTORE should store nothing and return 0.
+  Run({"saddex", "src", "1", "a", "b", "c"});
+  AdvanceTime(2000);
+  resp = Run({"sdiffstore", "dest", "src", "other"});
+  EXPECT_THAT(resp, IntArg(0));
+  EXPECT_THAT(Run({"exists", "src"}), IntArg(0));
+}
+
+// Verify key deletion after lazy member expiry for SUNION and SINTER.
+TEST_F(SetFamilyTest, SetOpsDeleteEmptyAfterExpiry) {
+  TEST_current_time_ms = kMemberExpiryBase * 1000;
+
+  Run({"saddex", "s1", "1", "a", "b"});
+  AdvanceTime(2000);
+
+  // SUNION triggers iteration which expires all members — key should be deleted.
+  auto resp = Run({"sunion", "s1"});
+  EXPECT_THAT(resp, ArrLen(0));
+  EXPECT_THAT(Run({"exists", "s1"}), IntArg(0));
+
+  Run({"saddex", "s2", "1", "a", "b"});
+  AdvanceTime(2000);
+
+  // SINTER single-key path — same behavior.
+  resp = Run({"sinter", "s2"});
+  EXPECT_THAT(resp, ArrLen(0));
+  EXPECT_THAT(Run({"exists", "s2"}), IntArg(0));
+}
+
 }  // namespace dfly