Fix: erroneous creation of properly closed mark

include/metall/detail/properly_closed_mark.hpp

@@ -22,6 +22,8 @@ namespace metall::mtlldetail {
  */
 struct properly_closed_mark {
 private:
+  // if mark_path is empty, this mark is in an invalid state
+  // and should not create the mark on close
   std::filesystem::path m_mark_path{};
   int m_mark_fd = -1;
   bool m_read_only = false;
@@ -64,29 +66,45 @@ struct properly_closed_mark {
     return ret == 0;
   }
 
+  bool open_impl() {
+    if (m_read_only) {
+      return lock_shared();
+    } else {
+      if (!lock_exclusive()) {
+        return false;
+      }
+
+      return remove_file(m_mark_path);
+    }
+  }
+
 public:
   properly_closed_mark() noexcept = default;
 
   bool create(const std::filesystem::path &path) {
     m_mark_path = path;
     m_read_only = false;
 
-    return remove_file(m_mark_path);
+    if (!remove_file(m_mark_path)) {
+      // could not create, prevent mark creation on close
+      m_mark_path.clear();
+      return false;
+    }
+
+    return true;
   }
 
   bool open(const std::filesystem::path &path, const bool read_only) {
     m_mark_path = path;
     m_read_only = read_only;
 
-    if (read_only) {
-      return lock_shared();
-    } else {
-      if (!lock_exclusive()) {
-        return false;
-      }
-
-      return remove_file(m_mark_path);
+    if (!open_impl()) {
+      // could not open, prevent mark creation on close
+      m_mark_path.clear();
+      return false;
     }
+
+    return true;
   }
 
   properly_closed_mark(const properly_closed_mark &other) = delete;
@@ -111,7 +129,7 @@ struct properly_closed_mark {
   }
 
   void close() {
-    if (!m_read_only) {
+    if (!m_read_only && !m_mark_path.empty()) {
       create_file(m_mark_path);
     }
     unlock();

test/kernel/manager_test.cpp

@@ -150,6 +150,49 @@ TEST(ManagerTest, CreateAndOpenModes) {
   }
 }
 
+TEST(ManagerTest, ProperlyClosedMarkBug) {
+  manager_type::remove(dir_path());
+
+  {
+    manager_type mg1{metall::create_only, dir_path()};
+    ASSERT_FALSE(manager_type::consistent(dir_path())); // properly closed mark should not exist
+
+    {
+      manager_type mg2{metall::open_only, dir_path()};
+    }
+
+    ASSERT_FALSE(manager_type::consistent(dir_path())); // properly closed mark should still not exist (mg2 should not create it)
+  }
+
+  ASSERT_TRUE(manager_type::consistent(dir_path())); // now mg1 should have created it
+}
+
+TEST(ManagerTest, MoveManager) {
+  auto const p1 = dir_path() / "1";
+  auto const p2 = dir_path() / "2";
+
+  manager_type::remove(p1);
+  manager_type::remove(p2);
+
+  {
+    manager_type mg1{metall::create_only, p1};
+    ASSERT_FALSE(manager_type::consistent(p1)); // properly closed mark should not exist
+
+    {
+      manager_type mg2{metall::create_only, p2};
+      auto tmp = std::move(mg2);
+      mg2 = std::move(mg1);
+      mg1 = std::move(tmp);
+    }
+
+    ASSERT_TRUE(manager_type::consistent(p1));
+    ASSERT_FALSE(manager_type::consistent(p2));
+  }
+
+  ASSERT_TRUE(manager_type::consistent(p1));
+  ASSERT_TRUE(manager_type::consistent(p2));
+}
+
 TEST(ManagerTest, ConstructArray) {
   {
     {