fix bad access to unlocked state (#527)

* don't touch non-locked data when creating manifest
* safety assert for test dir

3 files changed, 27 insertions, 17 deletions

diff --git a/src/zenserver/cache/cachedisklayer.cpp b/src/zenserver/cache/cachedisklayer.cpp
index 2d28c4875..4289c96f9 100644
--- a/src/zenserver/cache/cachedisklayer.cpp
+++ b/src/zenserver/cache/cachedisklayer.cpp
@@ -972,9 +972,10 @@ ZenCacheDiskLayer::CacheBucket::Flush()
 		m_BlockStore.Flush(/*ForceNewBlock*/ false);
 		m_SlogFile.Flush();
 
-		std::vector<AccessTime>	   AccessTimes;
-		std::vector<BucketPayload> Payloads;
-		IndexMap				   Index;
+		std::vector<AccessTime>		AccessTimes;
+		std::vector<BucketPayload>	Payloads;
+		std::vector<BucketMetaData> MetaDatas;
+		IndexMap					Index;
 
 		{
 			RwLock::SharedLockScope IndexLock(m_IndexLock);
@@ -982,8 +983,9 @@ ZenCacheDiskLayer::CacheBucket::Flush()
 			Index		= m_Index;
 			Payloads	= m_Payloads;
 			AccessTimes = m_AccessTimes;
+			MetaDatas	= m_MetaDatas;
 		}
-		SaveManifest(MakeManifest(std::move(Index), std::move(AccessTimes), Payloads));
+		SaveManifest(MakeManifest(std::move(Index), std::move(AccessTimes), Payloads, MetaDatas));
 	}
 	catch (std::exception& Ex)
 	{
@@ -1026,15 +1028,16 @@ ZenCacheDiskLayer::CacheBucket::SaveManifest(CbObject&& Manifest, const std::fun
 }
 
 CbObject
-ZenCacheDiskLayer::CacheBucket::MakeManifest(IndexMap&&						   Index,
-											 std::vector<AccessTime>&&		   AccessTimes,
-											 const std::vector<BucketPayload>& Payloads)
+ZenCacheDiskLayer::CacheBucket::MakeManifest(IndexMap&&							Index,
+											 std::vector<AccessTime>&&			AccessTimes,
+											 const std::vector<BucketPayload>&	Payloads,
+											 const std::vector<BucketMetaData>& MetaDatas)
 {
 	using namespace std::literals;
 
 	ZEN_TRACE_CPU("Z$::Disk::Bucket::MakeManifest");
 
-	size_t ItemCount = m_Index.size();
+	size_t ItemCount = Index.size();
 
 	// This tends to overestimate a little bit but it is still way more accurate than what we get with exponential growth
 	// And we don't need to reallocate theunderying buffer in almost every case
@@ -1045,7 +1048,7 @@ ZenCacheDiskLayer::CacheBucket::MakeManifest(IndexMap&&						   Index,
 	Writer << "BucketId"sv << m_BucketId;
 	Writer << "Version"sv << CurrentDiskBucketVersion;
 
-	if (!m_Index.empty())
+	if (!Index.empty())
 	{
 		Writer.AddInteger("Count"sv, gsl::narrow<std::uint64_t>(Index.size()));
 		Writer.BeginArray("Keys"sv);
@@ -1064,7 +1067,7 @@ ZenCacheDiskLayer::CacheBucket::MakeManifest(IndexMap&&						   Index,
 		}
 		Writer.EndArray();
 
-		if (!m_MetaDatas.empty())
+		if (!MetaDatas.empty())
 		{
 			Writer.BeginArray("RawHash"sv);
 			for (auto& Kv : Index)
@@ -1072,7 +1075,7 @@ ZenCacheDiskLayer::CacheBucket::MakeManifest(IndexMap&&						   Index,
 				const BucketPayload& Payload = Payloads[Kv.second];
 				if (Payload.MetaData)
 				{
-					Writer.AddHash(m_MetaDatas[Payload.MetaData].RawHash);
+					Writer.AddHash(MetaDatas[Payload.MetaData].RawHash);
 				}
 				else
 				{
@@ -1087,7 +1090,7 @@ ZenCacheDiskLayer::CacheBucket::MakeManifest(IndexMap&&						   Index,
 				const BucketPayload& Payload = Payloads[Kv.second];
 				if (Payload.MetaData)
 				{
-					Writer.AddInteger(m_MetaDatas[Payload.MetaData].RawSize);
+					Writer.AddInteger(MetaDatas[Payload.MetaData].RawSize);
 				}
 				else
 				{
@@ -1671,17 +1674,20 @@ ZenCacheDiskLayer::CacheBucket::CollectGarbage(GcContext& GcCtx)
 
 		try
 		{
-			std::vector<AccessTime>	   AccessTimes;
-			std::vector<BucketPayload> Payloads;
-			IndexMap				   Index;
+			std::vector<AccessTime>		AccessTimes;
+			std::vector<BucketPayload>	Payloads;
+			std::vector<BucketMetaData> MetaDatas;
+			IndexMap					Index;
 			{
 				RwLock::SharedLockScope IndexLock(m_IndexLock);
 				MakeIndexSnapshot([&]() { return GcCtx.ClaimGCReserve(); });
 				Index		= m_Index;
 				Payloads	= m_Payloads;
 				AccessTimes = m_AccessTimes;
+				MetaDatas	= m_MetaDatas;
 			}
-			SaveManifest(MakeManifest(std::move(Index), std::move(AccessTimes), Payloads), [&]() { return GcCtx.ClaimGCReserve(); });
+			SaveManifest(MakeManifest(std::move(Index), std::move(AccessTimes), Payloads, MetaDatas),
+						 [&]() { return GcCtx.ClaimGCReserve(); });
 		}
 		catch (std::exception& Ex)
 		{
diff --git a/src/zenserver/cache/cachedisklayer.h b/src/zenserver/cache/cachedisklayer.h
index 4efdeebd7..d46d629e4 100644
--- a/src/zenserver/cache/cachedisklayer.h
+++ b/src/zenserver/cache/cachedisklayer.h
@@ -335,7 +335,10 @@ private:
 		uint64_t ReadIndexFile(const std::filesystem::path& IndexPath, uint32_t& OutVersion);
 		uint64_t ReadLog(const std::filesystem::path& LogPath, uint64_t LogPosition);
 		void	 OpenLog(const bool IsNew);
-		CbObject MakeManifest(IndexMap&& Index, std::vector<AccessTime>&& AccessTimes, const std::vector<BucketPayload>& Payloads);
+		CbObject MakeManifest(IndexMap&&						 Index,
+							  std::vector<AccessTime>&&			 AccessTimes,
+							  const std::vector<BucketPayload>&	 Payloads,
+							  const std::vector<BucketMetaData>& MetaDatas);
 		void	 SaveManifest(
 				CbObject&&						 Manifest,
 				const std::function<uint64_t()>& ClaimDiskReserveFunc = []() { return 0; });
diff --git a/src/zenutil/zenserverprocess.cpp b/src/zenutil/zenserverprocess.cpp
index b1666ad0a..83c6668ba 100644
--- a/src/zenutil/zenserverprocess.cpp
+++ b/src/zenutil/zenserverprocess.cpp
@@ -438,6 +438,7 @@ ZenServerEnvironment::CreateNewTestDir()
 	TestDir << "test"sv << int64_t(++ZenServerTestCounter);
 
 	std::filesystem::path TestPath = m_TestBaseDir / TestDir.c_str();
+	ZEN_ASSERT(!std::filesystem::exists(TestPath));
 
 	ZEN_INFO("Creating new test dir @ '{}'", TestPath);