project/oplog delete improvements (#105)

* make oplog/project folder removeal more robust
* report back error to http caller if removal fails

3 files changed, 110 insertions, 53 deletions

diff --git a/src/zenserver/projectstore/httpprojectstore.cpp b/src/zenserver/projectstore/httpprojectstore.cpp
index 83038372e..585cee26f 100644
--- a/src/zenserver/projectstore/httpprojectstore.cpp
+++ b/src/zenserver/projectstore/httpprojectstore.cpp
@@ -1381,10 +1381,17 @@ HttpProjectService::HandleOpLogRequest(HttpRouterRequest& Req)
 			{
 				ZEN_INFO("deleting oplog '{}/{}'", ProjectId, OplogId);
 
-				Project->DeleteOplog(OplogId);
-
-				m_ProjectStats.OpLogDeleteCount++;
-				return HttpReq.WriteResponse(HttpResponseCode::OK);
+				if (Project->DeleteOplog(OplogId))
+				{
+					m_ProjectStats.OpLogDeleteCount++;
+					return HttpReq.WriteResponse(HttpResponseCode::OK);
+				}
+				else
+				{
+					return HttpReq.WriteResponse(HttpResponseCode::Locked,
+												 HttpContentType::kText,
+												 fmt::format("oplog {}/{} is in use", ProjectId, OplogId));
+				}
 			}
 			break;
 
diff --git a/src/zenserver/projectstore/projectstore.cpp b/src/zenserver/projectstore/projectstore.cpp
index 0f9481210..a0dbed741 100644
--- a/src/zenserver/projectstore/projectstore.cpp
+++ b/src/zenserver/projectstore/projectstore.cpp
@@ -43,7 +43,7 @@ namespace zen {
 namespace {
 	bool PrepareDirectoryDelete(const std::filesystem::path& Dir, std::filesystem::path& OutDeleteDir)
 	{
-		int DropIndex = 0;
+		std::filesystem::path DroppedBucketPath;
 		do
 		{
 			if (!std::filesystem::exists(Dir))
@@ -51,28 +51,55 @@ namespace {
 				return true;
 			}
 
-			std::string			  DroppedName		= fmt::format("[dropped]{}({})", Dir.filename().string(), DropIndex);
-			std::filesystem::path DroppedBucketPath = Dir.parent_path() / DroppedName;
+			StringBuilder<64> MovedId;
+			Oid::NewOid().ToString(MovedId);
+
+			std::string DroppedName = fmt::format("[dropped]{}({})", Dir.filename().string(), MovedId);
+			DroppedBucketPath		= Dir.parent_path() / DroppedName;
 			if (std::filesystem::exists(DroppedBucketPath))
 			{
-				DropIndex++;
-				continue;
+				if (!DeleteDirectories(DroppedBucketPath))
+				{
+					ZEN_INFO("Drop directory {} already exists and but and not be removed, attempting different name.", DroppedBucketPath);
+					continue;
+				}
+				if (std::filesystem::exists(DroppedBucketPath))
+				{
+					ZEN_INFO("Drop directory {} still exists after remove, attempting different name.", DroppedBucketPath);
+					continue;
+				}
 			}
 
-			std::error_code Ec;
-			std::filesystem::rename(Dir, DroppedBucketPath, Ec);
-			if (!Ec)
-			{
-				OutDeleteDir = DroppedBucketPath;
-				return true;
-			}
-			if (Ec && !std::filesystem::exists(DroppedBucketPath))
+			int RenameAttempt = 0;
+			do
 			{
-				// We can't move our folder, probably because it is busy, bail..
-				return false;
-			}
-			Sleep(100);
+				std::error_code Ec;
+				std::filesystem::rename(Dir, DroppedBucketPath, Ec);
+				if (!Ec)
+				{
+					OutDeleteDir = DroppedBucketPath;
+					return true;
+				}
+				if (std::filesystem::exists(DroppedBucketPath))
+				{
+					ZEN_INFO("Drop directory {} exists and can not be overwritten, attempting different name.", DroppedBucketPath);
+					break;
+				}
+				if (++RenameAttempt == 10)
+				{
+					ZEN_INFO("Can't rename '{}' to drop directory '{}'. Reason: {}.", Dir, DroppedBucketPath, Ec.message());
+					return false;
+				}
+				ZEN_INFO("Can't rename '{}' to drop directory '{}', pausing and retrying. Reason: {}.",
+						 Dir,
+						 DroppedBucketPath,
+						 Ec.message());
+				Sleep(100);
+			} while (true);
+
 		} while (true);
+
+		return false;
 	}
 
 	struct CreateRemoteStoreResult
@@ -245,7 +272,14 @@ struct ProjectStore::OplogStorage : public RefCounted
 	~OplogStorage()
 	{
 		ZEN_INFO("closing oplog storage at {}", m_OplogStoragePath);
-		Flush();
+		try
+		{
+			Flush();
+		}
+		catch (const std::exception& Ex)
+		{
+			ZEN_WARN("Flushing oplog at '{}' failed. Reason: '{}'", m_OplogStoragePath, Ex.what());
+		}
 	}
 
 	[[nodiscard]] bool		  Exists() const { return Exists(m_OplogStoragePath); }
@@ -760,8 +794,8 @@ ProjectStore::Oplog::TotalSize() const
 	return TotalSize(m_BasePath);
 }
 
-std::filesystem::path
-ProjectStore::Oplog::PrepareForDelete(bool MoveFolder)
+void
+ProjectStore::Oplog::ResetState()
 {
 	RwLock::ExclusiveLockScope _(m_OplogLock);
 	m_ChunkMap.clear();
@@ -770,16 +804,23 @@ ProjectStore::Oplog::PrepareForDelete(bool MoveFolder)
 	m_OpAddressMap.clear();
 	m_LatestOpMap.clear();
 	m_Storage = {};
-	if (!MoveFolder)
-	{
-		return {};
-	}
-	std::filesystem::path MovedDir;
-	if (PrepareDirectoryDelete(m_BasePath, MovedDir))
+}
+
+bool
+ProjectStore::Oplog::PrepareForDelete(std::filesystem::path& OutRemoveDirectory)
+{
+	RwLock::ExclusiveLockScope _(m_OplogLock);
+	m_ChunkMap.clear();
+	m_MetaMap.clear();
+	m_FileMap.clear();
+	m_OpAddressMap.clear();
+	m_LatestOpMap.clear();
+	m_Storage = {};
+	if (PrepareDirectoryDelete(m_BasePath, OutRemoveDirectory))
 	{
-		return MovedDir;
+		return true;
 	}
-	return {};
+	return false;
 }
 
 bool
@@ -1974,46 +2015,52 @@ ProjectStore::Project::OpenOplog(std::string_view OplogId)
 	return nullptr;
 }
 
-std::filesystem::path
-ProjectStore::Project::RemoveOplog(std::string_view OplogId)
+bool
+ProjectStore::Project::RemoveOplog(std::string_view OplogId, std::filesystem::path& OutDeletePath)
 {
 	RwLock::ExclusiveLockScope _(m_ProjectLock);
 
-	std::filesystem::path DeletePath;
 	if (auto OplogIt = m_Oplogs.find(std::string(OplogId)); OplogIt == m_Oplogs.end())
 	{
 		std::filesystem::path OplogBasePath = BasePathForOplog(OplogId);
 
 		if (Oplog::ExistsAt(OplogBasePath))
 		{
-			std::filesystem::path MovedDir;
-			if (PrepareDirectoryDelete(DeletePath, MovedDir))
+			if (!PrepareDirectoryDelete(OplogBasePath, OutDeletePath))
 			{
-				DeletePath = MovedDir;
+				return false;
 			}
 		}
 	}
 	else
 	{
 		std::unique_ptr<Oplog>& Oplog = OplogIt->second;
-		DeletePath					  = Oplog->PrepareForDelete(true);
+		if (!Oplog->PrepareForDelete(OutDeletePath))
+		{
+			return false;
+		}
 		m_DeletedOplogs.emplace_back(std::move(Oplog));
 		m_Oplogs.erase(OplogIt);
 	}
 	m_LastAccessTimes.erase(std::string(OplogId));
-	return DeletePath;
+	return true;
 }
 
-void
+bool
 ProjectStore::Project::DeleteOplog(std::string_view OplogId)
 {
-	std::filesystem::path DeletePath = RemoveOplog(OplogId);
+	std::filesystem::path DeletePath;
+	if (!RemoveOplog(OplogId, DeletePath))
+	{
+		return false;
+	}
 
 	// Erase content on disk
 	if (!DeletePath.empty())
 	{
-		OplogStorage::Delete(DeletePath);
+		return OplogStorage::Delete(DeletePath);
 	}
+	return true;
 }
 
 std::vector<std::string>
@@ -2156,8 +2203,7 @@ ProjectStore::Project::PrepareForDelete(std::filesystem::path& OutDeletePath)
 
 	for (auto& It : m_Oplogs)
 	{
-		// We don't care about the moved folder
-		It.second->PrepareForDelete(false);
+		It.second->ResetState();
 		m_DeletedOplogs.emplace_back(std::move(It.second));
 	}
 
@@ -2518,7 +2564,7 @@ ProjectStore::CollectGarbage(GcContext& GcCtx)
 			ZEN_DEBUG("ProjectStore::CollectGarbage garbage collected oplog '{}' in project '{}'. Removing storage on disk",
 					  OplogId,
 					  Project->Identifier);
-			Project->DeleteOplog(OplogId);
+			(void)Project->DeleteOplog(OplogId);
 		}
 		Project->Flush();
 	}
@@ -4089,13 +4135,16 @@ ProjectStore::RemoveExpiredData(GcCtx& Ctx, GcStats& Stats)
 		{
 			for (const std::string& OplogId : ExpiredOplogs)
 			{
-				std::filesystem::path RemovePath = Project->RemoveOplog(OplogId);
-				if (!RemovePath.empty())
+				std::filesystem::path RemovePath;
+				if (Project->RemoveOplog(OplogId, RemovePath))
 				{
-					OplogPathsToRemove.push_back(RemovePath);
+					if (!RemovePath.empty())
+					{
+						OplogPathsToRemove.push_back(RemovePath);
+					}
+					Stats.DeletedCount++;
 				}
 			}
-			Stats.DeletedCount += ExpiredOplogs.size();
 			Project->Flush();
 		}
 	}
diff --git a/src/zenserver/projectstore/projectstore.h b/src/zenserver/projectstore/projectstore.h
index 1b72a2688..98cc29c2a 100644
--- a/src/zenserver/projectstore/projectstore.h
+++ b/src/zenserver/projectstore/projectstore.h
@@ -146,7 +146,8 @@ public:
 			return m_LatestOpMap.size();
 		}
 
-		std::filesystem::path PrepareForDelete(bool MoveFolder);
+		void ResetState();
+		bool PrepareForDelete(std::filesystem::path& OutRemoveDirectory);
 
 		void AddChunkMappings(const std::unordered_map<Oid, IoHash, Oid::Hasher>& ChunkMappings);
 
@@ -250,8 +251,8 @@ public:
 
 		Oplog*					 NewOplog(std::string_view OplogId, const std::filesystem::path& MarkerPath);
 		Oplog*					 OpenOplog(std::string_view OplogId);
-		void					 DeleteOplog(std::string_view OplogId);
-		std::filesystem::path	 RemoveOplog(std::string_view OplogId);
+		bool					 DeleteOplog(std::string_view OplogId);
+		bool					 RemoveOplog(std::string_view OplogId, std::filesystem::path& OutDeletePath);
 		void					 IterateOplogs(std::function<void(const RwLock::SharedLockScope&, const Oplog&)>&& Fn) const;
 		void					 IterateOplogs(std::function<void(const RwLock::SharedLockScope&, Oplog&)>&& Fn);
 		std::vector<std::string> ScanForOplogs() const;