fix: removal of Expiry bit when overriding external strings

The bug: during the override of the existing external string, we called
`TieredStorage::Delete` to delete the external reference. This function
called CompactObj::Reset that cleared all the attributes on the value, including
expiry.

The fix: preserve the mask but clear the external state from the object.
Fixes #4672

Signed-off-by: Roman Gershman <[email protected]>

Author: romange

src/core/compact_object.cc

@@ -875,6 +875,8 @@ void CompactObj::InitRobj(CompactObjType type, unsigned encoding, void* obj) {
 }
 
 void CompactObj::SetInt(int64_t val) {
+  DCHECK(!IsExternal());
+
   if (INT_TAG != taglen_) {
     SetMeta(INT_TAG, mask_ & ~kEncMask);
   }

src/core/compact_object.h

@@ -140,7 +140,7 @@ class CompactObj {
     // IO_PENDING is set when the tiered storage has issued an i/o request to save the value. It is
     // cleared when the io request finishes or is cancelled.
     IO_PENDING = 0x20,
-    
+
     // Applied only on keys that should be deleted asynchronously.
     // (it can be the same value as IO_PENDING) that is applied only on values.
     KEY_ASYNC_DELETE = 0x20,
@@ -364,6 +364,12 @@ class CompactObj {
 
   void SetExternal(size_t offset, uint32_t sz);
 
+  // Switches to empty, non-external string.
+  // Preserves all the attributes.
+  void RemoveExternal() {
+    SetMeta(0, mask_);
+  }
+
   // Assigns a cooling record to the object together with its external slice.
   void SetCool(size_t offset, uint32_t serialized_size, detail::TieredColdRecord* record);
 

src/server/string_family.cc

@@ -869,19 +869,6 @@ OpStatus SetCmd::SetExisting(const SetParams& params, DbSlice::Iterator it,
         e_it->second = db_slice.FromAbsoluteTime(at_ms);
       } else {
         // Add new expiry information.
-
-        // Note: some consistency checks, following #4672. Once it's resolved we can remove them.
-        // -------------------------------------------------------------------------------------
-        ExpireTable* etable = db_slice.GetTables(op_args_.db_cntx.db_index).second;
-        ExpireIterator check_it = etable->Find(it->first.AsRef());
-        if (IsValid(check_it)) {
-          LOG(ERROR) << "Inconsistent state in SetCmd::SetExisting "
-                     << " key: " << key << ", "
-                     << "it.key:" << it.key() << ", "
-                     << "it->first:" << it->first.ToString()
-                     << " params.prev_val: " << params.prev_val << " " << params.flags;
-        }
-        // ------------------------------------------------
         db_slice.AddExpire(op_args_.db_cntx.db_index, it, at_ms);
       }
     } else {
@@ -893,6 +880,8 @@ OpStatus SetCmd::SetExisting(const SetParams& params, DbSlice::Iterator it,
     it->first.SetSticky(true);
   }
 
+  bool has_expire = prime_value.HasExpire();
+
   // Update flags
   prime_value.SetFlag(params.memcache_flags != 0);
   db_slice.SetMCFlag(op_args_.db_cntx.db_index, it->first.AsRef(), params.memcache_flags);
@@ -902,9 +891,13 @@ OpStatus SetCmd::SetExisting(const SetParams& params, DbSlice::Iterator it,
     shard->tiered_storage()->Delete(op_args_.db_cntx.db_index, &prime_value);
   }
 
+  DCHECK_EQ(has_expire, prime_value.HasExpire());
+
   // overwrite existing entry.
   prime_value.SetString(value);
 
+  DCHECK_EQ(has_expire, prime_value.HasExpire());
+
   PostEdit(params, key, value, &prime_value);
   return OpStatus::OK;
 }

src/server/tiered_storage.cc

@@ -424,6 +424,8 @@ bool TieredStorage::TryStash(DbIndex dbid, string_view key, PrimeValue* value) {
 
 void TieredStorage::Delete(DbIndex dbid, PrimeValue* value) {
   DCHECK(value->IsExternal());
+  DCHECK(!value->HasStashPending());
+
   ++stats_.total_deletes;
 
   tiering::DiskSegment segment = value->GetExternalSlice();
@@ -433,7 +435,7 @@ void TieredStorage::Delete(DbIndex dbid, PrimeValue* value) {
   }
 
   // In any case we delete the offloaded segment and reset the value.
-  value->Reset();
+  value->RemoveExternal();
   op_manager_->DeleteOffloaded(dbid, segment);
 }
 

src/server/tiered_storage_test.cc

@@ -334,4 +334,25 @@ TEST_F(TieredStorageTest, Expiry) {
   EXPECT_EQ(resp, val);
 }
 
+TEST_F(TieredStorageTest, SetExistingExpire) {
+  absl::FlagSaver saver;
+  SetFlag(&FLAGS_tiered_offload_threshold, 0.0f);  // offload all values
+  SetFlag(&FLAGS_tiered_experimental_cooling, false);
+
+  const int kNum = 20;
+  for (size_t i = 0; i < kNum; i++) {
+    Run({"SETEX", absl::StrCat("k", i), "100", BuildString(256)});
+  }
+  ExpectConditionWithinTimeout([&] { return GetMetrics().tiered_stats.total_stashes > 1; });
+
+  for (size_t i = 0; i < kNum; i++) {
+    Run({"SETEX", absl::StrCat("k", i), "100", BuildString(256)});
+  }
+
+  for (size_t i = 0; i < kNum; i++) {
+    auto resp = Run({"TTL", absl::StrCat("k", i)});
+    EXPECT_THAT(resp, IntArg(100));
+  }
+}
+
 }  // namespace dfly