From e4ea8681754fca16906d0b3dc5fcc82bb27fc45c Mon Sep 17 00:00:00 2001
From: Roman Gershman <roman@dragonflydb.io>
Date: Thu, 24 Aug 2023 17:02:02 +0300
Subject: [PATCH] fix: Fix inconsistency when deleting dash entries (#1734)

Partially addresses #1730.

Beforehand we removed entries from mcflag only inside DbSlice::Del function
but we delete entries in other cases too (like expiration) and in this case we left
dangling entries in mcflag table. The fix adds the deletion of mcflag entries into PerformDeletion function.

It by itself does not explain the crash but
dangling entries cause UB (because the keys are references to non-existent PrimeKeys)
so maybe this somehow causes inconsistency where a key is not present in mcflags.

I am not sure I fixed the original issue but I surely fixed a corruption bug.
In addition, I removed the redundant check fail and added error logs to follow up on this later.

Signed-off-by: Roman Gershman <roman@dragonflydb.io>
---
 src/server/db_slice.cc      | 29 +++++++++++++++++++++++------
 src/server/string_family.cc |  5 ++++-
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/server/db_slice.cc b/src/server/db_slice.cc
index fdfdd47ea..c3135af7d 100644
--- a/src/server/db_slice.cc
+++ b/src/server/db_slice.cc
@@ -38,6 +38,13 @@ void PerformDeletion(PrimeIterator del_it, ExpireIterator exp_it, EngineShard* s
     table->expire.Erase(exp_it);
   }
 
+  if (del_it->second.HasFlag()) {
+    if (table->mcflag.Erase(del_it->first) == 0) {
+      LOG(ERROR) << "Internal error, inconsistent state, mcflag should be present but not found "
+                 << del_it->first.ToString();
+    }
+  }
+
   DbTableStats& stats = table->stats;
   const PrimeValue& pv = del_it->second;
   if (pv.IsExternal()) {
@@ -442,6 +449,7 @@ tuple<PrimeIterator, ExpireIterator, bool> DbSlice::AddOrFind2(const Context& cn
       SlotId sid = ClusterConfig::KeySlot(key);
       db.slots_stats[sid].key_count += 1;
     }
+
     return make_tuple(it, ExpireIterator{}, true);
   }
 
@@ -464,7 +472,11 @@ tuple<PrimeIterator, ExpireIterator, bool> DbSlice::AddOrFind2(const Context& cn
       db.expire.Erase(expire_it);
 
       if (existing->second.HasFlag()) {
-        db.mcflag.Erase(existing->first);
+        if (db.mcflag.Erase(existing->first) == 0) {
+          LOG(ERROR)
+              << "Internal error, inconsistent state, mcflag should be present but not found "
+              << existing->first.ToString();
+        }
       }
 
       // Keep the entry but reset the object.
@@ -493,9 +505,6 @@ bool DbSlice::Del(DbIndex db_ind, PrimeIterator it) {
   }
 
   auto& db = db_arr_[db_ind];
-  if (it->second.HasFlag()) {
-    CHECK_EQ(1u, db->mcflag.Erase(it->first));
-  }
 
   auto obj_type = it->second.ObjType();
   if (doc_del_cb_ && (obj_type == OBJ_JSON || obj_type == OBJ_HASH)) {
@@ -638,7 +647,10 @@ bool DbSlice::UpdateExpire(DbIndex db_ind, PrimeIterator it, uint64_t at) {
 void DbSlice::SetMCFlag(DbIndex db_ind, PrimeKey key, uint32_t flag) {
   auto& db = *db_arr_[db_ind];
   if (flag == 0) {
-    db.mcflag.Erase(key);
+    if (db.mcflag.Erase(key) == 0) {
+      LOG(ERROR) << "Internal error, inconsistent state, mcflag should be present but not found "
+                 << key.ToString();
+    }
   } else {
     auto [it, inserted] = db.mcflag.Insert(std::move(key), flag);
     if (!inserted)
@@ -649,7 +661,12 @@ void DbSlice::SetMCFlag(DbIndex db_ind, PrimeKey key, uint32_t flag) {
 uint32_t DbSlice::GetMCFlag(DbIndex db_ind, const PrimeKey& key) const {
   auto& db = *db_arr_[db_ind];
   auto it = db.mcflag.Find(key);
-  return it.is_done() ? 0 : it->second;
+  if (it.is_done()) {
+    LOG(ERROR) << "Internal error, inconsistent state, mcflag should be present but not found "
+               << key.ToString();
+    return 0;
+  }
+  return it->second;
 }
 
 PrimeIterator DbSlice::AddNew(const Context& cntx, string_view key, PrimeValue obj,
diff --git a/src/server/string_family.cc b/src/server/string_family.cc
index 344cd9ba3..58f68dfed 100644
--- a/src/server/string_family.cc
+++ b/src/server/string_family.cc
@@ -1340,7 +1340,10 @@ auto StringFamily::OpMGet(bool fetch_mcflag, bool fetch_mcver, const Transaction
 
     dest.value = GetString(shard, it->second);
     if (fetch_mcflag) {
-      dest.mc_flag = db_slice.GetMCFlag(t->GetDbIndex(), it->first);
+      if (it->second.HasFlag()) {
+        dest.mc_flag = db_slice.GetMCFlag(t->GetDbIndex(), it->first);
+      }
+
       if (fetch_mcver) {
         dest.mc_ver = it.GetVersion();
       }