Bug Fix Windows: Remove lmdb files when delete_library is called

cpp/arcticdb/storage/library.hpp

@@ -122,6 +122,10 @@ class Library {
         return storages_->fast_delete();
     }
 
+    void cleanup() {
+        storages_->cleanup();
+    }
+
     bool key_exists(const VariantKey& key) {
         return storages_->key_exists(key);
     }

cpp/arcticdb/storage/library_manager.cpp

@@ -156,9 +156,12 @@ std::shared_ptr<Library> LibraryManager::get_library(const LibraryPath& path, co
     return lib;
 }
 
-void LibraryManager::close_library_if_open(const LibraryPath &path) {
+void LibraryManager::cleanup_library_if_open(const LibraryPath &path) {
     std::lock_guard<std::mutex> lock{open_libraries_mutex_};
-    open_libraries_.erase(path);
+    if (auto it = open_libraries_.find(path); it != open_libraries_.end()) {
+        it->second->cleanup();
+        open_libraries_.erase(it);
+    }
 }
 
 std::vector<LibraryPath> LibraryManager::get_library_paths() const {

cpp/arcticdb/storage/library_manager.hpp

@@ -32,7 +32,7 @@ namespace arcticdb::storage {
 
         [[nodiscard]] std::shared_ptr<Library> get_library(const LibraryPath& path, const StorageOverride& storage_override = StorageOverride{});
 
-        void close_library_if_open(const LibraryPath& path);
+        void cleanup_library_if_open(const LibraryPath& path);
 
         [[nodiscard]] std::vector<LibraryPath> get_library_paths() const;
 

cpp/arcticdb/storage/lmdb/lmdb_storage.cpp

@@ -259,6 +259,43 @@ void LmdbStorage::do_iterate_type(KeyType key_type, const IterateTypeVisitor& vi
     }
 }
 
+void remove_db_files(const fs::path& lib_path) {
+    std::vector<std::string> files = {"lock.mdb", "data.mdb"};
+
+    for (const auto& file : files) {
+        fs::path file_path = lib_path / file;
+        try {
+            if (fs::exists(file_path)) {
+                fs::remove(file_path);
+            }
+        } catch (const std::filesystem::filesystem_error& e) {
+            raise<ErrorCode::E_UNEXPECTED_LMDB_ERROR>(
+                    fmt::format("Unexpected LMDB Error: Failed to remove LMDB file at path: {} error: {}",
+                                file_path.string(), e.what()));
+        }
+    }
+
+    if (fs::exists(lib_path)) {
+        if (!fs::is_empty(lib_path)) {
+            log::storage().warn(fmt::format("Skipping deletion of directory holding LMDB library during "
+                                            "library deletion as it contains files unrelated to LMDB"));
+        } else {
+            try {
+                fs::remove_all(lib_path);
+            } catch (const fs::filesystem_error& e) {
+                raise<ErrorCode::E_UNEXPECTED_LMDB_ERROR>(
+                        fmt::format("Unexpected LMDB Error: Failed to remove directory: {} error: {}",
+                                    lib_path.string(), e.what()));
+            }
+        }
+    }
+}
+
+void LmdbStorage::cleanup() {
+    env_.reset();
+    remove_db_files(lib_dir_);
+}
+
 
 namespace {
 template<class T>

cpp/arcticdb/storage/lmdb/lmdb_storage.hpp

@@ -46,11 +46,19 @@ class LmdbStorage final : public Storage {
 
     inline bool do_fast_delete() final;
 
+    void cleanup() override;
+
     void do_iterate_type(KeyType key_type, const IterateTypeVisitor& visitor, const std::string &prefix) final;
 
     bool do_key_exists(const VariantKey & key) final;
 
-    ::lmdb::env& env() { return *env_;  }
+    ::lmdb::env& env() {
+        if (!env_) {
+            raise<ErrorCode::E_UNEXPECTED_LMDB_ERROR>("Unexpected LMDB Error: Invalid operation: LMDB environment has been removed. "
+                                                      "Possibly because the library has been deleted");
+        }
+        return *env_;
+    }
 
     std::string do_key_path(const VariantKey&) const final { return {}; };
 

cpp/arcticdb/storage/python_bindings.cpp

@@ -156,8 +156,8 @@ void register_bindings(py::module& storage) {
         .def("get_library", [](LibraryManager& library_manager, std::string_view library_path, const StorageOverride& storage_override){
             return library_manager.get_library(LibraryPath{library_path, '.'}, storage_override);
         }, py::arg("library_path"), py::arg("storage_override") = StorageOverride{})
-        .def("close_library_if_open", [](LibraryManager& library_manager, std::string_view library_path) {
-            return library_manager.close_library_if_open(LibraryPath{library_path, '.'});
+        .def("cleanup_library_if_open", [](LibraryManager& library_manager, std::string_view library_path) {
+            return library_manager.cleanup_library_if_open(LibraryPath{library_path, '.'});
         })
         .def("has_library", [](const LibraryManager& library_manager, std::string_view library_path){
             return library_manager.has_library(LibraryPath{library_path, '.'});

cpp/arcticdb/storage/storage.hpp

@@ -152,6 +152,8 @@ class Storage {
         return do_fast_delete();
     }
 
+    virtual void cleanup() { }
+
     inline bool key_exists(const VariantKey &key) {
         return do_key_exists(key);
     }

cpp/arcticdb/storage/storages.hpp

@@ -59,6 +59,10 @@ class Storages {
         return primary().fast_delete();
     }
 
+    void cleanup() {
+        primary().cleanup();
+    }
+
     bool key_exists(const VariantKey& key) {
         return primary().key_exists(key);
     }

cpp/arcticdb/storage/test/test_storage_exceptions.cpp

@@ -45,25 +45,30 @@ class LMDBStorageFactory : public StorageFactory {
 private:
     uint64_t map_size;
     bool use_mock;
+    fs::path db_path;
+    std::string lib_name;
 public:
-    explicit LMDBStorageFactory(bool use_mock = false) : map_size(128ULL * (1ULL << 20) /* 128MB */), use_mock(use_mock) { }
+    explicit LMDBStorageFactory(uint64_t map_size, bool use_mock = false) : map_size(map_size), use_mock(use_mock), db_path(TEST_DATABASES_PATH / "test_lmdb"), lib_name("test_lib") { }
 
-    explicit LMDBStorageFactory(uint64_t map_size, bool use_mock = false) : map_size(map_size), use_mock(use_mock) { }
+    explicit LMDBStorageFactory(bool use_mock = false) : LMDBStorageFactory(128ULL * (1ULL << 20)  /* 128MB */, use_mock) { }
 
     std::unique_ptr<arcticdb::storage::Storage> create() override {
         arcticdb::proto::lmdb_storage::Config cfg;
 
-        fs::path db_name = "test_lmdb";
-        cfg.set_path((TEST_DATABASES_PATH / db_name).generic_string());
+        cfg.set_path((db_path).generic_string());
         cfg.set_map_size(map_size);
         cfg.set_recreate_if_exists(true);
         cfg.set_use_mock_storage_for_testing(use_mock);
 
-        arcticdb::storage::LibraryPath library_path{"a", "b"};
+        arcticdb::storage::LibraryPath library_path(lib_name, '/');
 
         return std::make_unique<arcticdb::storage::lmdb::LmdbStorage>(library_path, arcticdb::storage::OpenMode::DELETE, cfg);
     }
 
+    fs::path get_lib_path() const {
+        return db_path / lib_name;
+    }
+
     void setup() override {
         if (!fs::exists(TEST_DATABASES_PATH)) {
             fs::create_directories(TEST_DATABASES_PATH);
@@ -311,6 +316,40 @@ TEST_F(LMDBStorageTestBase, MockUnexpectedLMDBErrorException) {
     ASSERT_EQ(list_in_store(*storage), symbols);
 }
 
+TEST_F(LMDBStorageTestBase, RemoveLibPath) {
+    LMDBStorageFactory factory;
+    auto storage = factory.create();
+    auto path = factory.get_lib_path();
+
+    storage->cleanup();
+    ASSERT_FALSE(fs::exists(path));
+    // Once we call close, any other operations should throw UnexpectedLMDBErrorException as lmdb env is closed
+    ASSERT_THROW({
+                     write_in_store(*storage, "sym1");
+                 },  UnexpectedLMDBErrorException);
+
+    ASSERT_THROW({
+                     update_in_store(*storage, "sym1");
+                 },  UnexpectedLMDBErrorException);
+
+    ASSERT_THROW({
+                     remove_in_store(*storage, {"sym1"});
+                 },  UnexpectedLMDBErrorException);
+
+    ASSERT_THROW({
+                     read_in_store(*storage, "sym1");
+                 },  UnexpectedLMDBErrorException);
+
+    ASSERT_THROW({
+                     exists_in_store(*storage, "sym1");
+                 },  UnexpectedLMDBErrorException);
+
+    ASSERT_THROW({
+                     list_in_store(*storage);
+                 },  UnexpectedLMDBErrorException);
+
+}
+
 // S3 error handling with mock client
 // Note: Exception handling is different for S3 as compared to other storages.
 // S3 does not return an error if you rewrite an existing key. It overwrites it.

python/arcticdb/adapters/arctic_library_adapter.py

@@ -62,9 +62,6 @@ def config_library(self) -> Library:
     def get_library_config(self, name: str, library_options: LibraryOptions):
         raise NotImplementedError
 
-    def cleanup_library(self, library_name: str):
-        pass
-
     def get_storage_override(self) -> StorageOverride:
         return StorageOverride()
 

python/arcticdb/adapters/lmdb_library_adapter.py

@@ -167,26 +167,6 @@ def get_library_config(self, name, library_options: LibraryOptions):
             env_cfg, _DEFAULT_ENV, name, encoding_version=library_options.encoding_version
         )
 
-    def cleanup_library(self, library_name: str):
-        lmdb_files_removed = True
-        for file in ("lock.mdb", "data.mdb"):
-            path = os.path.join(self._path, library_name, file)
-            try:
-                os.remove(path)
-            except Exception as e:
-                lmdb_files_removed = False
-                _rm_errorhandler(None, path, e)
-        dir_path = os.path.join(self._path, library_name)
-        if os.path.exists(dir_path):
-            if os.listdir(dir_path):
-                log.warn(
-                    "Skipping deletion of directory holding LMDB library during library deletion as it contains "
-                    f"files unrelated to LMDB. LMDB files {'have' if lmdb_files_removed else 'have not'} been "
-                    f"removed. directory=[{dir_path}]"
-                )
-            else:
-                shutil.rmtree(os.path.join(self._path, library_name), onerror=_rm_errorhandler)
-
     def get_storage_override(self) -> StorageOverride:
         lmdb_override = LmdbOverride()
         lmdb_override.path = self._path

python/arcticdb/adapters/prefixing_library_adapter_decorator.py

@@ -59,10 +59,6 @@ def get_library_config(self, name: str, library_options: LibraryOptions):
         lib_mgr_name = self.get_name_for_library_manager(name)
         return self._inner.get_library_config(lib_mgr_name, library_options)
 
-    def cleanup_library(self, library_name: str):
-        lib_mgr_name = self.get_name_for_library_manager(library_name)
-        return self._inner.cleanup_library(lib_mgr_name)
-
     def __repr__(self):
         return repr(self._inner)
 

python/arcticdb/arctic.py

@@ -218,16 +218,12 @@ def delete_library(self, name: str) -> None:
         except LibraryNotFound:
             return
         lib._nvs.version_store.clear()
-        del lib
         lib_mgr_name = self._library_adapter.get_name_for_library_manager(name)
-        self._library_manager.close_library_if_open(lib_mgr_name)
-        try:
-            self._library_adapter.cleanup_library(name)
-        finally:
-            self._library_manager.remove_library_config(lib_mgr_name)
+        self._library_manager.cleanup_library_if_open(lib_mgr_name)
+        self._library_manager.remove_library_config(lib_mgr_name)
 
-            if self._created_lib_names and name in self._created_lib_names:
-                self._created_lib_names.remove(name)
+        if self._created_lib_names and name in self._created_lib_names:
+            self._created_lib_names.remove(name)
 
     def has_library(self, name: str) -> bool:
         """

python/tests/integration/arcticdb/test_lmdb.py

@@ -164,6 +164,12 @@ def test_map_size_bad_input(options):
 
     assert "Incorrect format for map_size" in str(e.value)
 
+def test_delete_library(lmdb_storage):
+    ac = lmdb_storage.create_arctic()
+    lib = ac.create_library("library")
+    ac.delete_library("library")
+    with pytest.raises(StorageException) as e:
+        lib.write("sym1", pd.DataFrame())
 
 @pytest.mark.parametrize("options", ["MAP_SIZE=1GB", "atlas_shape=1GB"])
 def test_lmdb_options_unknown_option(options):