Clean up lease management (#17)

* Cleaned up lock tracking and destruction

* Update src/AVEVA/RocksDB/Plugin/Azure/BlobFilesystem.cpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Make sure to delete the lock file in UnlockFile

* Delete lockFile

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Author: njwright-aveva

include/AVEVA/RocksDB/Plugin/Azure/BlobFilesystem.hpp

@@ -19,8 +19,7 @@ namespace AVEVA::RocksDB::Plugin::Azure
     {
         std::unique_ptr<Impl::BlobFilesystemImpl> m_filesystem;
         std::shared_ptr<boost::log::sources::severity_logger_mt<boost::log::trivial::severity_level>> m_logger;
-        boost::intrusive::list<Azure::LockFile, boost::intrusive::constant_time_size<false>> m_lockFiles;
-
+        std::vector<Azure::LockFile*> m_lockFiles;
     public:
         BlobFilesystem(std::shared_ptr<rocksdb::FileSystem> rocksdbFs,
             std::unique_ptr<Impl::BlobFilesystemImpl> filesystem,

include/AVEVA/RocksDB/Plugin/Azure/Impl/BlobFilesystemImpl.hpp

@@ -47,7 +47,7 @@ namespace AVEVA::RocksDB::Plugin::Azure::Impl
         std::unordered_map<std::string, ServiceContainer, Core::StringHash, Core::StringEqual> m_clients;
         std::unordered_map<std::string, std::shared_ptr<Core::FileCache>, Core::StringHash, Core::StringEqual> m_fileCaches;
         std::mutex m_lockFilesMutex;
-        std::vector<std::shared_ptr<LockFileImpl>> m_locks;
+        boost::intrusive::list<LockFileImpl, boost::intrusive::constant_time_size<false>> m_locks;
         std::stop_source m_filesystemStopSource;
         std::jthread m_lockRenewalThread;
 
@@ -103,7 +103,7 @@ namespace AVEVA::RocksDB::Plugin::Azure::Impl
         [[nodiscard]] WriteableFileImpl ReuseWritableFile(const std::string& filePath);
         LoggerImpl CreateLogger(const std::string& filePath, int logLevel);
         std::shared_ptr<LockFileImpl> LockFile(const std::string& filePath);
-        bool UnlockFile(const LockFileImpl& lock);
+        void UnlockFile(LockFileImpl& lock);
         DirectoryImpl CreateDirectory(const std::string& directoryPath);
 
         [[nodiscard]] bool FileExists(const std::string& name);

include/AVEVA/RocksDB/Plugin/Azure/Impl/LockFileImpl.hpp

@@ -2,13 +2,14 @@
 // SPDX-FileCopyrightText: Copyright 2025 AVEVA
 
 #pragma once
+#include <boost/intrusive/list.hpp>
 #include <azure/storage/blobs/page_blob_client.hpp>
 #include <azure/storage/blobs/blob_lease_client.hpp>
 #include <memory>
 #include <chrono>
 namespace AVEVA::RocksDB::Plugin::Azure::Impl
 {
-    class LockFileImpl
+    class LockFileImpl : public boost::intrusive::list_base_hook<boost::intrusive::link_mode<boost::intrusive::auto_unlink>>
     {
         std::unique_ptr<::Azure::Storage::Blobs::PageBlobClient> m_file;
         std::unique_ptr<::Azure::Storage::Blobs::BlobLeaseClient> m_lease = nullptr;
@@ -23,5 +24,8 @@ namespace AVEVA::RocksDB::Plugin::Azure::Impl
 
         [[nodiscard]] std::chrono::seconds TimeSinceLastRenewal() const;
         [[nodiscard]] bool HasExceededLeaseLength() const;
+
+        void unlink();
+        bool is_linked();
     };
 }

include/AVEVA/RocksDB/Plugin/Azure/LockFile.hpp

@@ -4,13 +4,12 @@
 #pragma once
 #include "AVEVA/RocksDB/Plugin/Azure/Impl/LockFileImpl.hpp"
 
-#include <boost/intrusive/list.hpp>
 #include <rocksdb/db.h>
 
 #include <memory>
 namespace AVEVA::RocksDB::Plugin::Azure
 {
-    class LockFile : public rocksdb::FileLock, public boost::intrusive::list_base_hook<boost::intrusive::link_mode<boost::intrusive::auto_unlink>>
+    class LockFile : public rocksdb::FileLock
     {
         std::shared_ptr<Impl::LockFileImpl> m_lock;
     public:
@@ -19,7 +18,6 @@ namespace AVEVA::RocksDB::Plugin::Azure
         void Renew() const;
         void Unlock();
 
-        void unlink();
-        bool is_linked();
+        Impl::LockFileImpl& GetImpl() const;
     };
 }

src/AVEVA/RocksDB/Plugin/Azure/BlobFilesystem.cpp

@@ -23,10 +23,11 @@ namespace AVEVA::RocksDB::Plugin::Azure
 
     BlobFilesystem::~BlobFilesystem()
     {
-        m_lockFiles.clear_and_dispose([](Azure::LockFile* lock) static
+        for (auto lock : m_lockFiles)
         {
+            m_filesystem->UnlockFile(lock->GetImpl());
             delete lock;
-        });
+        }
     }
 
     const char* BlobFilesystem::Name() const
@@ -544,8 +545,8 @@ namespace AVEVA::RocksDB::Plugin::Azure
             *l = nullptr;
             auto lock = m_filesystem->LockFile(f);
             auto lockFileWrapper = new Plugin::Azure::LockFile(lock);
+            m_lockFiles.push_back(lockFileWrapper);
             *l = lockFileWrapper;
-            m_lockFiles.push_back(*lockFileWrapper);
             return rocksdb::IOStatus::OK();
         }
         catch (const ::Azure::Core::RequestFailedException& ex)
@@ -571,17 +572,16 @@ namespace AVEVA::RocksDB::Plugin::Azure
         try
         {
             auto lockFile = dynamic_cast<Plugin::Azure::LockFile*>(l);
-            if (lockFile != nullptr)
+            if (lockFile == nullptr)
             {
-                lockFile->Unlock();
-                delete lockFile;
-                return rocksdb::IOStatus::OK();
-            }
-            else
-            {
-                BOOST_LOG_SEV(*m_logger, error) << "Unable to case file lock to Azure::LockFile";
+                BOOST_LOG_SEV(*m_logger, error) << "Unable to cast file lock to Azure::LockFile";
                 return rocksdb::IOStatus::InvalidArgument();
             }
+
+            m_filesystem->UnlockFile(lockFile->GetImpl());
+            std::erase_if(m_lockFiles, [lockFile](const auto& l) { return l == lockFile; });
+            delete lockFile;
+            return rocksdb::IOStatus::OK();
         }
         catch (const ::Azure::Core::RequestFailedException& ex)
         {

src/AVEVA/RocksDB/Plugin/Azure/Impl/BlobFilesystemImpl.cpp

@@ -356,7 +356,8 @@ namespace AVEVA::RocksDB::Plugin::Azure::Impl
         auto lockFile = std::make_shared<LockFileImpl>(std::move(client), Configuration::LeaseLength);
         if (lockFile->Lock())
         {
-            m_locks.push_back(lockFile);
+            m_locks.push_back(*lockFile);
+            assert(lockFile->is_linked());
             return lockFile;
         }
         else
@@ -365,28 +366,13 @@ namespace AVEVA::RocksDB::Plugin::Azure::Impl
         }
     }
 
-    bool BlobFilesystemImpl::UnlockFile(const LockFileImpl& lock)
+    void BlobFilesystemImpl::UnlockFile(LockFileImpl& lock)
     {
         EnsureLiveness();
 
         std::scoped_lock _(m_lockFilesMutex);
-        const auto it = std::find_if(m_locks.cbegin(),
-            m_locks.cend(),
-            [&lock](const auto& f)
-            {
-                return f.get() == &lock;
-            });
-
-        if (it != m_locks.cend())
-        {
-            (*it)->Unlock();
-            m_locks.erase(it);
-            return true;
-        }
-        else
-        {
-            return false;
-        }
+        lock.Unlock();
+        lock.unlink();
     }
 
     DirectoryImpl BlobFilesystemImpl::CreateDirectory(const std::string& directoryPath)
@@ -775,11 +761,11 @@ namespace AVEVA::RocksDB::Plugin::Azure::Impl
 
                 {
                     std::scoped_lock lock(m_lockFilesMutex);
-                    std::vector<LockFileImpl*> needsRetry;
+                    std::vector<const LockFileImpl*> needsRetry;
                     needsRetry.reserve(m_locks.size());
                     for (const auto& l : m_locks)
                     {
-                        needsRetry.push_back(l.get());
+                        needsRetry.push_back(&l);
                     }
 
                     // Attempt to renew all locks with retries

src/AVEVA/RocksDB/Plugin/Azure/Impl/LockFileImpl.cpp

@@ -109,4 +109,14 @@ namespace AVEVA::RocksDB::Plugin::Azure::Impl
     {
         return TimeSinceLastRenewal() >= m_leaseLength;
     }
+
+    void LockFileImpl::unlink()
+    {
+        boost::intrusive::list_base_hook<boost::intrusive::link_mode<boost::intrusive::auto_unlink>>::unlink();
+    }
+
+    bool LockFileImpl::is_linked()
+    {
+        return boost::intrusive::list_base_hook<boost::intrusive::link_mode<boost::intrusive::auto_unlink>>::is_linked();
+    }
 }

src/AVEVA/RocksDB/Plugin/Azure/LockFile.cpp

@@ -24,13 +24,8 @@ namespace AVEVA::RocksDB::Plugin::Azure
         m_lock->Unlock();
     }
 
-    void LockFile::unlink()
+    Impl::LockFileImpl& LockFile::GetImpl() const
     {
-        boost::intrusive::list_base_hook<boost::intrusive::link_mode<boost::intrusive::auto_unlink>>::unlink();
-    }
-
-    bool LockFile::is_linked()
-    {
-        return boost::intrusive::list_base_hook<boost::intrusive::link_mode<boost::intrusive::auto_unlink>>::is_linked();
+        return *m_lock;
     }
 }

tests/AVEVA/RocksDB/Plugin/Azure/Impl/BlobFilesystemIntegrationTests.cpp

@@ -638,10 +638,9 @@ TEST_F(BlobFilesystemIntegrationTests, UnlockFile_ReleasesLock)
     EXPECT_EQ(1, m_filesystem->GetLeaseClientCount());
 
     // Act
-    bool unlocked = m_filesystem->UnlockFile(*lock);
+    m_filesystem->UnlockFile(*lock);
 
     // Assert
-    EXPECT_TRUE(unlocked);
     EXPECT_EQ(0, m_filesystem->GetLeaseClientCount());
 
     // Cleanup
@@ -661,10 +660,7 @@ TEST_F(BlobFilesystemIntegrationTests, UnlockFile_InvalidLock_ReturnsFalse)
     m_filesystem->UnlockFile(*lock1);
 
     // Act - try to unlock lock1 again
-    bool unlocked = m_filesystem->UnlockFile(*lock1);
-
-    // Assert
-    EXPECT_FALSE(unlocked);
+    EXPECT_THROW(m_filesystem->UnlockFile(*lock1), std::runtime_error);
 
     // Cleanup
     m_filesystem->UnlockFile(*lock2);