project/oplog delete improvements (#105)

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

4 files changed, 112 insertions, 53 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9283f2a6f..e54c6fd9c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,12 +6,14 @@
 - Bugfix: Absolute paths and paths going outside the root path for workspace shares are now blocked
 - Bugfix: Fix ASSERT that would trigger in GC under certain conditions if source block was empty
 - Bugfix: Skip and report invalid configurations for workspaces instead of crashing
+- Bugfix: Report back error to http caller if removal of oplog fails
 - Improvement: `zen workspace-share create` now resolves relative root paths to absolute paths
 - Improvement: Add better output/logging when failing to initialize shared mutex
 - Improvement: Validate data when gathering attachments in GCv2
 - Improvement: Add file and size of file when reading a iobuffer into memory via ReadFromFileMaybe
 - Improvement: Add hardening to gracefully handle malformed oplogs in project store
 - Improvement: Catch exceptions in threaded work to avoid uncaught exception errors
+- Improvement: Make oplog/project removal more robust
 
 ## 5.5.3
 - Feature: New 'workspaces' service which allows a user to share a local folder via zenserver. A workspace can have mulitple workspace shares and they provie an HTTP API that is compatible with the project oplog HTTP API. Workspaces and shares are preserved between runs. Workspaces feature is disabled by default - enable with `--workspaces-enabled` option when launching zenserver.
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;