fix unlink handling

CHANGELOG.md

@@ -28,6 +28,7 @@
 * Added check version support to remote mounts
 * Fixed handling of `FALLOC_FL_KEEP_SIZE` on Linux
 * Fixed intermittent client hang on remote mount server disconnect
+* Fixed `unlink()` return `EBADF` if open file handles exist when removed
 
 ## v2.0.7-release
 

repertory/librepertory/include/file_manager/file_manager.hpp

@@ -61,6 +61,8 @@ private:
   i_provider &provider_;
 
 private:
+  std::unordered_map<std::string, std::shared_ptr<i_closeable_open_file>>
+      closed_file_lookup_;
   std::unique_ptr<i_file_mgr_db> mgr_db_;
   std::atomic<std::uint64_t> next_handle_{0U};
   mutable std::recursive_mutex open_file_mtx_;
@@ -73,8 +75,6 @@ private:
   std::unique_ptr<std::thread> upload_thread_;
 
 private:
-  [[nodiscard]] auto close_all(const std::string &api_path) -> bool;
-
   void close_timed_out_files();
 
   [[nodiscard]] auto get_open_file_by_handle(std::uint64_t handle) const

repertory/librepertory/include/file_manager/i_open_file.hpp

@@ -110,9 +110,13 @@ public:
 
   [[nodiscard]] virtual auto is_modified() const -> bool = 0;
 
+  [[nodiscard]] virtual auto is_unlinked() const -> bool = 0;
+
   virtual void remove(std::uint64_t handle) = 0;
 
   virtual void remove_all() = 0;
+
+  virtual void set_unlinked(bool value) = 0;
 };
 } // namespace repertory
 

repertory/librepertory/include/file_manager/open_file.hpp

@@ -111,6 +111,8 @@ public:
 
   [[nodiscard]] auto is_complete() const -> bool override;
 
+  [[nodiscard]] auto is_removed() const -> bool override;
+
   [[nodiscard]] auto is_write_supported() const -> bool override {
     return true;
   }
@@ -131,6 +133,8 @@ public:
 
   [[nodiscard]] auto resize(std::uint64_t new_file_size) -> api_error override;
 
+  void set_removed(bool value) override;
+
   [[nodiscard]] auto write(std::uint64_t write_offset, const data_buffer &data,
                            std::size_t &bytes_written) -> api_error override;
 };

repertory/librepertory/include/file_manager/open_file_base.hpp

@@ -119,6 +119,9 @@ private:
   };
   bool modified_{false};
   bool removed_{false};
+#if !defined(_WIN32)
+  bool unlinked_{false};
+#endif // !defined(_WIN32)
 
 private:
   void file_io_thread();
@@ -208,6 +211,8 @@ public:
     return fsi_.directory;
   }
 
+  [[nodiscard]] auto is_unlinked() const -> bool override;
+
   [[nodiscard]] auto is_modified() const -> bool override;
 
   void remove(std::uint64_t handle) override;
@@ -215,6 +220,8 @@ public:
   void remove_all() override;
 
   void set_api_path(const std::string &api_path) override;
+
+  void set_unlinked(bool value) override;
 };
 } // namespace repertory
 

repertory/librepertory/src/drives/remote/remote_open_file_table.cpp

@@ -267,6 +267,7 @@ void remote_open_file_table::remove_and_close_all(const native_handle &handle) {
 #else  // !defined(_WIN32)
     close(open_handle);
 #endif // defined(_WIN32)
+
     remove_open_info(open_handle);
   }
 }

repertory/librepertory/src/file_manager/file_manager.cpp

@@ -78,32 +78,34 @@ file_manager::~file_manager() {
 
 void file_manager::close(std::uint64_t handle) {
   unique_recur_mutex_lock file_lock(open_file_mtx_);
-  auto closeable_file = get_open_file_by_handle(handle);
+  bool is_closed{false};
+  auto closeable_file = get_open_file_by_handle(handle, is_closed);
   if (not closeable_file) {
     return;
   }
   file_lock.unlock();
 
   closeable_file->remove(handle);
-}
 
-auto file_manager::close_all(const std::string &api_path) -> bool {
-  REPERTORY_USES_FUNCTION_NAME();
-
-  unique_recur_mutex_lock file_lock(open_file_mtx_);
-  auto file_iter = open_file_lookup_.find(api_path);
-  if (file_iter == open_file_lookup_.end()) {
-    return false;
+  file_lock.lock();
+  if (not(is_closed && closeable_file->get_open_file_count() == 0U)) {
+    return;
   }
 
-  auto closeable_file = file_iter->second;
-  open_file_lookup_.erase(api_path);
+  closed_file_lookup_.erase(closeable_file->get_api_path());
   file_lock.unlock();
 
-  closeable_file->remove_all();
   closeable_file->close();
 
-  return closeable_file->get_allocated();
+  auto file = utils::file::file{closeable_file->get_source_path()};
+  if (file.remove()) {
+    return;
+  }
+
+  utils::error::raise_api_path_error(
+      function_name, closeable_file->get_api_path(),
+      closeable_file->get_source_path(), utils::get_last_error_code(),
+      "failed to delete source");
 }
 
 void file_manager::close_timed_out_files() {
@@ -307,6 +309,15 @@ auto file_manager::get_next_handle() -> std::uint64_t {
 
 auto file_manager::get_open_file_by_handle(std::uint64_t handle) const
     -> std::shared_ptr<i_closeable_open_file> {
+  {
+    auto file_iter = std::ranges::find_if(
+        closed_file_lookup_, [&handle](auto &&item) -> bool {
+          return item.second->has_handle(handle);
+        });
+    return (file_iter == closed_file_lookup_.end()) ? nullptr
+                                                    : file_iter->second;
+  }
+
   auto file_iter =
       std::ranges::find_if(open_file_lookup_, [&handle](auto &&item) -> bool {
         return item.second->has_handle(handle);
@@ -622,7 +633,7 @@ void file_manager::queue_upload(const std::string &api_path,
                                 const std::string &source_path, bool no_lock) {
   REPERTORY_USES_FUNCTION_NAME();
 
-  if (provider_.is_read_only()) {
+  if (provider_.is_read_only() || file.is_removed()) {
     return;
   }
 
@@ -659,7 +670,7 @@ auto file_manager::remove_file(const std::string &api_path) -> api_error {
     return res;
   }
 
-  auto allocated = close_all(api_path);
+  unique_recur_mutex_lock open_lock(open_file_mtx_);
 
   unique_mutex_lock upload_lock(upload_mtx_);
   remove_upload(api_path, true);
@@ -667,15 +678,39 @@ auto file_manager::remove_file(const std::string &api_path) -> api_error {
   upload_notify_.notify_all();
   upload_lock.unlock();
 
-  recur_mutex_lock open_lock(open_file_mtx_);
-
   res = provider_.remove_file(api_path);
   if (res != api_error::success) {
     return res;
   }
 
-  remove_source_and_shrink_cache(api_path, fsi.source_path, fsi.size,
-                                 allocated);
+  auto file_iter = open_file_lookup_.find(api_path);
+  if (file_iter == open_file_lookup_.end()) {
+    return api_error::success;
+  }
+
+  if (closed_file_lookup_.contains(api_path)) {
+    auto closed_file = closed_file_lookup_.at(api_path).second;
+    closed_file_lookup_[api_path] = file_iter.second;
+
+    for (auto &[handle, ofd] : closed_file->get_open_data()) {
+      closed_file_lookup_.at(api_path).add(handle, ofd);
+    }
+  } else {
+    closed_file_lookup_[api_path] = file_iter.second;
+  }
+
+  open_file_lookup_.erase(api_path);
+
+  auto closed_file = closed_file_lookup_.at(api_path).second;
+  closed_file->set_unlinked(true);
+  open_lock.unlock();
+
+  res = cache_size_mgr::instance().shrink(closed_file->get_file_size());
+  if (res != api_error::success) {
+    utils::error::raise_api_path_error(function_name, api_path, source_path,
+                                       res, "failed to shrink cache");
+  }
+
   return api_error::success;
 }
 
@@ -1081,7 +1116,7 @@ void file_manager::stop() {
 void file_manager::store_resume(const i_open_file &file) {
   REPERTORY_USES_FUNCTION_NAME();
 
-  if (provider_.is_read_only()) {
+  if (provider_.is_read_only() || file.is_removed()) {
     return;
   }
 

repertory/librepertory/src/file_manager/open_file.cpp

@@ -236,6 +236,10 @@ auto open_file::close() -> bool {
 
   nf_->close();
 
+  if (is_unlinked()) {
+    return true;
+  }
+
   if (is_modified()) {
     if (err == api_error::success) {
       mgr_.queue_upload(*this);
@@ -534,6 +538,10 @@ auto open_file::native_operation(
 
   set_file_size(new_file_size);
   auto now = std::to_string(utils::time::get_time_now());
+  if (is_unlinked()) {
+    return api_error::success:
+  }
+
   res = get_provider().set_item_meta(
       get_api_path(), {
                           {META_CHANGED, now},
@@ -653,6 +661,13 @@ auto open_file::resize(std::uint64_t new_file_size) -> api_error {
 }
 
 void open_file::set_modified() {
+  if (is_unlinked()) {
+    if (not is_modified()) {
+      open_file_base::set_modified(true);
+    }
+    return;
+  }
+
   if (not is_modified()) {
     open_file_base::set_modified(true);
     mgr_.store_resume(*this);
@@ -765,6 +780,10 @@ auto open_file::write(std::uint64_t write_offset, const data_buffer &data,
     return set_api_error(res);
   }
 
+  if (is_unlinked()) {
+    return api_error::success:
+  }
+
   auto now = std::to_string(utils::time::get_time_now());
   res = get_provider().set_item_meta(get_api_path(), {
                                                          {META_CHANGED, now},

repertory/librepertory/src/file_manager/open_file_base.cpp

@@ -244,6 +244,11 @@ void open_file_base::set_source_path(std::string source_path) {
   fsi_.source_path = std::move(source_path);
 }
 
+void open_file_base::set_unlinked(bool value) {
+  recur_mutex_lock file_lock(file_mtx_);
+  unlinked_ = value;
+}
+
 auto open_file_base::get_filesystem_item() const -> filesystem_item {
   recur_mutex_lock file_lock(file_mtx_);
   return fsi_;
@@ -308,6 +313,11 @@ auto open_file_base::is_removed() const -> bool {
   return removed_;
 }
 
+auto open_file_base::is_unlinked() const -> bool {
+  recur_mutex_lock file_lock(file_mtx_);
+  return unlinked_;
+}
+
 void open_file_base::notify_io() {
   mutex_lock io_lock(io_thread_mtx_);
   io_thread_notify_.notify_all();