Fixed bug in z$ service returning partial cache records and enable small object GC by default.

9 files changed, 46 insertions, 25 deletions

diff --git a/zenserver/admin/admin.cpp b/zenserver/admin/admin.cpp
index 8a3ab460c..15314b12a 100644
--- a/zenserver/admin/admin.cpp
+++ b/zenserver/admin/admin.cpp
@@ -43,9 +43,9 @@ HttpAdminService::HttpAdminService(GcScheduler& Scheduler) : m_GcScheduler(Sched
 			const HttpServerRequest::QueryParams Params	 = HttpReq.GetQueryParams();
 			GcScheduler::TriggerParams			 GcParams;
 
-			if (auto Param = Params.GetValue("smallobjects"); Param == "true"sv)
+			if (auto Param = Params.GetValue("smallobjects"); Param.empty() == false)
 			{
-				GcParams.CollectSmallObjects = true;
+				GcParams.CollectSmallObjects = Param == "true"sv;
 			}
 
 			if (auto Param = Params.GetValue("maxcacheduration"); Param.empty() == false)
diff --git a/zenserver/cache/structuredcache.cpp b/zenserver/cache/structuredcache.cpp
index e74030e07..22c8c4afb 100644
--- a/zenserver/cache/structuredcache.cpp
+++ b/zenserver/cache/structuredcache.cpp
@@ -847,29 +847,37 @@ HttpStructuredCacheService::HandleRpcGetCacheRecords(zen::HttpServerRequest& Req
 		if (m_CacheStore.Get(Key.Bucket, Key.Hash, CacheValue))
 		{
 			CbObjectView CacheRecord(CacheValue.Value.Data());
-
-			if (!SkipAttachments)
-			{
-				CacheRecord.IterateAttachments([this, &MissingCount, &RpcResponse](CbFieldView AttachmentHash) {
+			CacheRecord.IterateAttachments([this, SkipAttachments, &MissingCount, &RpcResponse](CbFieldView AttachmentHash) {
+				if (SkipAttachments && MissingCount == 0)
+				{
+					if (!m_CidStore.ContainsChunk(AttachmentHash.AsHash()))
+					{
+						MissingCount++;
+					}
+				}
+				else
+				{
 					if (IoBuffer Chunk = m_CidStore.FindChunkByCid(AttachmentHash.AsHash()))
 					{
+						ZEN_ASSERT(Chunk.GetSize() > 0);
 						RpcResponse.AddAttachment(CbAttachment(CompressedBuffer::FromCompressed(SharedBuffer(Chunk))));
 					}
 					else
 					{
 						MissingCount++;
 					}
-				});
-			}
+				}
+			});
 		}
 
 		if (CacheValue.Value && (MissingCount == 0 || PartialOnError))
 		{
-			ZEN_DEBUG("HIT - '{}/{}' {} '{}' (LOCAL)",
+			ZEN_DEBUG("HIT - '{}/{}' {} '{}' (LOCAL) {}",
 					  Key.Bucket,
 					  Key.Hash,
 					  NiceBytes(CacheValue.Value.Size()),
-					  ToString(CacheValue.Value.GetContentType()));
+					  ToString(CacheValue.Value.GetContentType()),
+					  MissingCount ? "(PARTIAl)" : ""sv);
 
 			CacheValues[KeyIndex] = std::move(CacheValue.Value);
 			m_CacheStats.HitCount++;
@@ -880,7 +888,7 @@ HttpStructuredCacheService::HandleRpcGetCacheRecords(zen::HttpServerRequest& Req
 		}
 		else
 		{
-			ZEN_DEBUG("MISS - '{}/{}' {}", Key.Bucket, Key.Hash, MissingCount ? "(partial)"sv : ""sv);
+			ZEN_DEBUG("MISS - '{}/{}' {}", Key.Bucket, Key.Hash, MissingCount ? "(PARTIAl)"sv : ""sv);
 			m_CacheStats.MissCount++;
 		}
 
@@ -1091,6 +1099,8 @@ HttpStructuredCacheService::HandleRpcGetCachePayloads(zen::HttpServerRequest& Re
 		{
 			if (IoBuffer Chunk = m_CidStore.FindChunkByCid(ChunkRequest.ChunkId))
 			{
+				ZEN_ASSERT(Chunk.GetSize() > 0);
+
 				ZEN_DEBUG("HIT - '{}/{}/{}' {} '{}' ({})",
 						  ChunkRequest.Key.Bucket,
 						  ChunkRequest.Key.Hash,
diff --git a/zenserver/config.cpp b/zenserver/config.cpp
index e57a97c8b..5e5676494 100644
--- a/zenserver/config.cpp
+++ b/zenserver/config.cpp
@@ -314,7 +314,7 @@ ParseCliOptions(int argc, char* argv[], ZenServerOptions& ServerOptions)
 					   "",
 					   "gc-small-objects",
 					   "Whether garbage collection is enabled or not.",
-					   cxxopts::value<bool>(ServerOptions.GcConfig.CollectSmallObjects)->default_value("false"),
+					   cxxopts::value<bool>(ServerOptions.GcConfig.CollectSmallObjects)->default_value("true"),
 					   "");
 
 	options.add_option("gc",
diff --git a/zenserver/config.h b/zenserver/config.h
index bc8768961..8a507df39 100644
--- a/zenserver/config.h
+++ b/zenserver/config.h
@@ -76,7 +76,7 @@ struct ZenGcConfig
 	ZenCasEvictionPolicy   Cas;
 	ZenCacheEvictionPolicy Cache;
 	int32_t				   IntervalSeconds	   = 0;
-	bool				   CollectSmallObjects = false;
+	bool				   CollectSmallObjects = true;
 	bool				   Enabled			   = true;
 };
 
diff --git a/zenstore/CAS.cpp b/zenstore/CAS.cpp
index 18abd2cf5..d4023bdc4 100644
--- a/zenstore/CAS.cpp
+++ b/zenstore/CAS.cpp
@@ -101,6 +101,7 @@ public:
 	virtual void				   Initialize(const CasStoreConfiguration& InConfig) override;
 	virtual CasStore::InsertResult InsertChunk(IoBuffer Chunk, const IoHash& ChunkHash) override;
 	virtual IoBuffer			   FindChunk(const IoHash& ChunkHash) override;
+	virtual bool				   ContainsChunk(const IoHash& ChunkHash) override;
 	virtual void				   FilterChunks(CasChunkSet& InOutChunks) override;
 	virtual void				   Flush() override;
 	virtual void				   Scrub(ScrubContext& Ctx) override;
@@ -282,6 +283,12 @@ CasImpl::FindChunk(const IoHash& ChunkHash)
 	return IoBuffer{};
 }
 
+bool
+CasImpl::ContainsChunk(const IoHash& ChunkHash)
+{
+	return m_SmallStrategy.HaveChunk(ChunkHash) || m_TinyStrategy.HaveChunk(ChunkHash) || m_LargeStrategy.HaveChunk(ChunkHash);
+}
+
 void
 CasImpl::FilterChunks(CasChunkSet& InOutChunks)
 {
diff --git a/zenstore/cidstore.cpp b/zenstore/cidstore.cpp
index 33dc216b5..c5396cff3 100644
--- a/zenstore/cidstore.cpp
+++ b/zenstore/cidstore.cpp
@@ -107,20 +107,17 @@ struct CidStore::Impl
 
 	bool ContainsChunk(const IoHash& DecompressedId)
 	{
-		RwLock::SharedLockScope _(m_Lock);
-		// Note that we do not check CAS here. This is optimistic but usually
-		// what we want.
-		auto It = m_CidMap.find(DecompressedId);
+		IoHash CasHash = IoHash::Zero;
 
-		if (It == m_CidMap.end())
 		{
-			// Not in map
-			return false;
+			RwLock::SharedLockScope _(m_Lock);
+			if (const auto It = m_CidMap.find(DecompressedId); It != m_CidMap.end())
+			{
+				CasHash = It->second;
+			}
 		}
 
-		ZEN_ASSERT(It->second != IoHash::Zero);
-
-		return true;
+		return CasHash != IoHash::Zero ? m_CasStore.ContainsChunk(CasHash) : false;
 	}
 
 	void InitializeIndex(const std::filesystem::path& RootDir)
diff --git a/zenstore/gc.cpp b/zenstore/gc.cpp
index d5cb4901b..307adc04e 100644
--- a/zenstore/gc.cpp
+++ b/zenstore/gc.cpp
@@ -268,6 +268,10 @@ CasGc::CollectGarbage(GcContext& GcCtx)
 		Contributor->GatherReferences(GcCtx);
 	}
 
+	// Cache records reference CAS chunks with the uncompressed
+	// raw hash (Cid). Map the content ID to CAS hash to enable
+	// the CAS storage backends to filter valid chunks.
+
 	if (CidStore* CidStore = m_CidStore)
 	{
 		std::vector<IoHash> CasHashes;
@@ -301,6 +305,8 @@ CasGc::CollectGarbage(GcContext& GcCtx)
 		Storage->CollectGarbage(GcCtx);
 	}
 
+	// Remove Cid to CAS hash mappings. Scrub?
+	
 	if (CidStore* CidStore = m_CidStore)
 	{
 		CidStore->RemoveCids(GcCtx.DeletedCas());
diff --git a/zenstore/include/zenstore/CAS.h b/zenstore/include/zenstore/CAS.h
index 5f1565f81..2e530c418 100644
--- a/zenstore/include/zenstore/CAS.h
+++ b/zenstore/include/zenstore/CAS.h
@@ -126,6 +126,7 @@ public:
 	virtual void		 Initialize(const CasStoreConfiguration& Config)	 = 0;
 	virtual InsertResult InsertChunk(IoBuffer Data, const IoHash& ChunkHash) = 0;
 	virtual IoBuffer	 FindChunk(const IoHash& ChunkHash)					 = 0;
+	virtual bool		 ContainsChunk(const IoHash& ChunkHash)				 = 0;
 	virtual void		 FilterChunks(CasChunkSet& InOutChunks)				 = 0;
 	virtual void		 Flush()											 = 0;
 	virtual void		 Scrub(ScrubContext& Ctx)							 = 0;
diff --git a/zenstore/include/zenstore/gc.h b/zenstore/include/zenstore/gc.h
index 9b0025403..b752d159a 100644
--- a/zenstore/include/zenstore/gc.h
+++ b/zenstore/include/zenstore/gc.h
@@ -167,8 +167,8 @@ struct GcSchedulerConfig
 	std::chrono::seconds  MonitorInterval{30};
 	std::chrono::seconds  Interval{};
 	std::chrono::seconds  MaxCacheDuration{86400};
-	bool				  CollectSmallObjects = false;
-	bool				  Enabled			  = false;
+	bool				  CollectSmallObjects = true;
+	bool				  Enabled			  = true;
 };
 
 /**