Fix (P)TTL issue with non-existing keys. Improve logging for binary values. Fix RENAME bugs

src/facade/facade.cc

@@ -2,10 +2,10 @@
 // See LICENSE for licensing terms.
 //
 
+#include <absl/strings/escaping.h>
 #include <absl/strings/str_cat.h>
 
 #include "base/logging.h"
-
 #include "facade/conn_context.h"
 #include "facade/dragonfly_connection.h"
 #include "facade/error.h"
@@ -112,16 +112,17 @@ RedisReplyBuilder* ConnectionContext::operator->() {
 
 }  // namespace facade
 
-
 namespace std {
 
+using facade::ArgS;
+
 ostream& operator<<(ostream& os, facade::CmdArgList ras) {
   os << "[";
   if (!ras.empty()) {
     for (size_t i = 0; i < ras.size() - 1; ++i) {
-      os << facade::ArgS(ras, i) << ",";
+      os << absl::CHexEscape(ArgS(ras, i)) << ",";
     }
-    os << facade::ArgS(ras, ras.size() - 1);
+    os << absl::CHexEscape(ArgS(ras, ras.size() - 1));
   }
   os << "]";
 

src/server/generic_family.cc

@@ -321,14 +321,18 @@ void GenericFamily::TtlGeneric(CmdArgList args, ConnectionContext* cntx, TimeUni
   if (result) {
     long ttl = (unit == TimeUnit::SEC) ? (result.value() + 500) / 1000 : result.value();
     (*cntx)->SendLong(ttl);
-  } else {
-    switch (result.status()) {
-      case OpStatus::KEY_NOTFOUND:
-        (*cntx)->SendLong(-1);
-        break;
-      default:
-        (*cntx)->SendLong(-2);
-    }
+    return;
+  }
+
+  switch (result.status()) {
+    case OpStatus::KEY_NOTFOUND:
+      (*cntx)->SendLong(-2);
+      break;
+    default:
+      LOG_IF(ERROR, result.status() != OpStatus::SKIPPED)
+          << "Unexpected status " << result.status();
+      (*cntx)->SendLong(-1);
+      break;
   }
 }
 
@@ -528,7 +532,7 @@ OpResult<uint32_t> GenericFamily::OpExists(const OpArgs& op_args, ArgSlice keys)
 OpResult<void> GenericFamily::OpRen(const OpArgs& op_args, string_view from, string_view to,
                                     bool skip_exists) {
   auto& db_slice = op_args.shard->db_slice();
-  auto [from_it, expire_it] = db_slice.FindExt(op_args.db_ind, from);
+  auto [from_it, from_expire] = db_slice.FindExt(op_args.db_ind, from);
   if (!IsValid(from_it))
     return OpStatus::KEY_NOTFOUND;
 
@@ -538,25 +542,32 @@ OpResult<void> GenericFamily::OpRen(const OpArgs& op_args, string_view from, str
       return OpStatus::KEY_EXISTS;
   }
 
-  uint64_t exp_ts = IsValid(expire_it) ? expire_it->second.duration() : 0;
-  if (IsValid(to_it)) {
-    to_it->second = std::move(from_it->second);
-    from_it->second.SetExpire(IsValid(expire_it));
+  uint64_t exp_ts =
+      IsValid(from_expire) ? db_slice.expire_base() + from_expire->second.duration() : 0;
 
-    if (IsValid(to_expire)) {
-      to_it->second.SetExpire(true);
-      to_expire->second.Set(exp_ts);
-    } else {
-      to_it->second.SetExpire(false);
-      db_slice.Expire(op_args.db_ind, to_it, exp_ts + db_slice.expire_base());
-    }
+  // we keep the value we want to move.
+  PrimeValue from_obj = std::move(from_it->second);
+
+  // Restore the expire flag on 'from'.
+  from_it->second.SetExpire(IsValid(from_expire));
+
+  if (IsValid(to_it)) {
+    to_it->second = std::move(from_obj);
+    to_it->second.SetExpire(IsValid(to_expire));  // keep the expire flag on 'to'.
+
+    // It is guaranteed that Expire() call does not erase the element because then
+    // from_it would be invalid. Therefore, it Expire does not invalidate any iterators and
+    // we can delete via from_it.
+    db_slice.Expire(op_args.db_ind, to_it, exp_ts);
+    CHECK(db_slice.Del(op_args.db_ind, from_it));
   } else {
-    db_slice.AddNew(op_args.db_ind, to, std::move(from_it->second),
-                    exp_ts + db_slice.expire_base());
-    // Need search again since the container might invalidate the iterators.
-    from_it = db_slice.FindExt(op_args.db_ind, from).first;
+    // Here we first delete from_it because AddNew below could invalidate from_it.
+    // On the other hand, AddNew does not rely on the iterators - this is why we keep
+    // the value in `from_obj`.
+    CHECK(db_slice.Del(op_args.db_ind, from_it));
+    db_slice.AddNew(op_args.db_ind, to, std::move(from_obj), exp_ts);
   }
-  CHECK(db_slice.Del(op_args.db_ind, from_it));
+
   return OpStatus::OK;
 }
 

src/server/generic_family_test.cc

@@ -1,4 +1,4 @@
-// Copyright 2021, Roman Gershman.  All rights reserved.
+// Copyright 2022, Roman Gershman.  All rights reserved.
 // See LICENSE for licensing terms.
 //
 
@@ -79,6 +79,14 @@ TEST_F(GenericFamilyTest, Del) {
   del_fb.join();
 }
 
+TEST_F(GenericFamilyTest, TTL) {
+  EXPECT_EQ(-2, CheckedInt({"ttl", "foo"}));
+  EXPECT_EQ(-2, CheckedInt({"pttl", "foo"}));
+  Run({"set", "foo", "bar"});
+  EXPECT_EQ(-1, CheckedInt({"ttl", "foo"}));
+  EXPECT_EQ(-1, CheckedInt({"pttl", "foo"}));
+}
+
 TEST_F(GenericFamilyTest, Exists) {
   Run({"mset", "x", "0", "y", "1"});
   auto resp = Run({"exists", "x", "y", "x"});
@@ -128,4 +136,14 @@ TEST_F(GenericFamilyTest, Rename) {
 
 }
 
+TEST_F(GenericFamilyTest, RenameBinary) {
+  const char kKey1[] = "\x01\x02\x03\x04";
+  const char kKey2[] = "\x05\x06\x07\x08";
+
+  Run({"set", kKey1, "bar"});
+  Run({"rename", kKey1, kKey2});
+  EXPECT_THAT(Run({"get", kKey1}), ElementsAre(ArgType(RespExpr::NIL)));
+  EXPECT_THAT(Run({"get", kKey2}), RespEq("bar"));
+}
+
 }  // namespace dfly

src/server/main_service.cc

@@ -393,7 +393,7 @@ void Service::DispatchCommand(CmdArgList args, facade::ConnectionContext* cntx)
   string_view cmd_name{cid->name()};
 
   if (cntx->req_auth && !cntx->authenticated) {
-    if (cmd_name != "AUTH") {
+    if (cmd_name != "AUTH" && cmd_name != "QUIT") {
       return (*cntx)->SendError("-NOAUTH Authentication required.");
     }
   }