Change batch put responses for client reporting

Conflicts are now treated as successes, and we optionally return a Details array instead of an ErrorMessages array.  Details are returned  for all requests in a batch, or no requests in a batch depending on whether there are any details to be shared about any of the put requests.  The details for a conflict include the raw hash and raw size of the item.  If the item is a record, we also include the record as an object.

9 files changed, 119 insertions, 41 deletions

diff --git a/src/zenserver-test/zenserver-test.cpp b/src/zenserver-test/zenserver-test.cpp
index 79e5db554..1cbd10194 100644
--- a/src/zenserver-test/zenserver-test.cpp
+++ b/src/zenserver-test/zenserver-test.cpp
@@ -1556,6 +1556,7 @@ TEST_CASE("zcache.rpc")
 			{
 				CHECK(ResponseSuccess);
 			}
+			CHECK(ParsedResult.Details.empty());
 		}
 
 		auto CheckRecordCorrectness = [&](const ZenConfig& Cfg) {
@@ -1619,9 +1620,18 @@ TEST_CASE("zcache.rpc")
 			CbPackage Response = ParsePackageMessage(zen::IoBuffer(zen::IoBuffer::Wrap, Result.text.data(), Result.text.size()));
 			CHECK(!Response.IsNull());
 			CHECK(ParsedResult.Parse(Response));
+			CHECK(Request.Requests.size() == ParsedResult.Success.size());
 			for (bool ResponseSuccess : ParsedResult.Success)
 			{
-				CHECK(!ResponseSuccess);
+				CHECK(ResponseSuccess);
+			}
+			CHECK(Request.Requests.size() == ParsedResult.Details.size());
+			for (const CbObjectView& Details : ParsedResult.Details)
+			{
+				CHECK(Details);
+				CHECK(Details["RawHash"sv].IsHash());
+				CHECK(Details["RawSize"sv].IsInteger());
+				CHECK(Details["Object"sv].IsObject());
 			}
 		}
 
@@ -1690,6 +1700,7 @@ TEST_CASE("zcache.rpc")
 			{
 				CHECK(ResponseSuccess);
 			}
+			CHECK(ParsedResult.Details.empty());
 		}
 
 		auto CheckRecordCorrectness = [&](const ZenConfig& Cfg) {
@@ -1753,6 +1764,7 @@ TEST_CASE("zcache.rpc")
 			{
 				CHECK(ResponseSuccess);
 			}
+			CHECK(ParsedResult.Details.empty());
 		}
 
 		auto CheckRecordCorrectness = [&](const ZenConfig& Cfg) {
diff --git a/src/zenserver/cache/httpstructuredcache.cpp b/src/zenserver/cache/httpstructuredcache.cpp
index c83065506..dc14465de 100644
--- a/src/zenserver/cache/httpstructuredcache.cpp
+++ b/src/zenserver/cache/httpstructuredcache.cpp
@@ -1221,8 +1221,11 @@ HttpStructuredCacheService::HandlePutCacheRecord(HttpServerRequest& Request, con
 				break;
 		}
 
-		return PutResult.Message.empty() ? Request.WriteResponse(ResponseCode)
-										 : Request.WriteResponse(ResponseCode, zen::HttpContentType::kText, PutResult.Message);
+		if (PutResult.Details)
+		{
+			Request.WriteResponse(ResponseCode, PutResult.Details);
+		}
+		return Request.WriteResponse(ResponseCode);
 	};
 
 	const HttpContentType ContentType = Request.RequestContentType();
diff --git a/src/zenstore/cache/cachedisklayer.cpp b/src/zenstore/cache/cachedisklayer.cpp
index fd52cdab5..9a56844fe 100644
--- a/src/zenstore/cache/cachedisklayer.cpp
+++ b/src/zenstore/cache/cachedisklayer.cpp
@@ -1968,17 +1968,19 @@ ZenCacheDiskLayer::CacheBucket::ShouldRejectPut(const IoHash&				  HashKey,
 					IndexLock.ReleaseNow();
 					if (!cache::impl::UpdateValueWithRawSizeAndHash(InOutValue))
 					{
-						OutPutResult = PutResult{zen::PutStatus::Fail, "Value provided is of bad format"};
+						CbObjectWriter DetailWriter;
+						DetailWriter.AddString("Value provided is of bad format");
+						OutPutResult = PutResult{zen::PutStatus::Fail, DetailWriter.Save()};
 						return true;
 					}
 					else if (MetaData.RawSize != InOutValue.RawSize || MetaData.RawHash != InOutValue.RawHash)
 					{
-						OutPutResult = PutResult{
-							zen::PutStatus::Conflict,
-							fmt::format("Value exists with different size '{}' or hash '{}'", MetaData.RawSize, MetaData.RawHash)};
-						return true;
+						// Deliberate fall through without return so that we load the value and include it in the result
+					}
+					else
+					{
+						return false;
 					}
-					return false;
 				}
 			}
 
@@ -2008,16 +2010,27 @@ ZenCacheDiskLayer::CacheBucket::ShouldRejectPut(const IoHash&				  HashKey,
 				{
 					if (!cache::impl::UpdateValueWithRawSizeAndHash(InOutValue))
 					{
-						OutPutResult = PutResult{zen::PutStatus::Fail, "Value provided is of bad format"};
+						CbObjectWriter DetailWriter;
+						DetailWriter.AddString("Value provided is of bad format");
+						OutPutResult = PutResult{zen::PutStatus::Fail, DetailWriter.Save()};
 						return true;
 					}
 
 					if (ExistingValue.RawSize != InOutValue.RawSize || ExistingValue.RawHash != InOutValue.RawHash)
 					{
-						OutPutResult = PutResult{zen::PutStatus::Conflict,
-												 fmt::format("Value exists with different size '{}' or hash '{}'",
-															 ExistingValue.RawSize,
-															 ExistingValue.RawHash)};
+						CbObjectWriter DetailWriter;
+						if (Location.IsFlagSet(DiskLocation::kStructured))
+						{
+							DetailWriter.AddInteger("RawSize", ExistingValue.RawSize);
+							DetailWriter.AddHash("RawHash", ExistingValue.RawHash);
+							DetailWriter.AddObject("Object", CbObjectView(ExistingValue.Value.GetData()));
+						}
+						else
+						{
+							DetailWriter.AddInteger("RawSize", ExistingValue.RawSize);
+							DetailWriter.AddHash("RawHash", ExistingValue.RawHash);
+						}
+						OutPutResult = PutResult{zen::PutStatus::Conflict, DetailWriter.Save()};
 						return true;
 					}
 				}
diff --git a/src/zenstore/cache/cacherpc.cpp b/src/zenstore/cache/cacherpc.cpp
index 83301f863..5cecc8b6b 100644
--- a/src/zenstore/cache/cacherpc.cpp
+++ b/src/zenstore/cache/cacherpc.cpp
@@ -310,7 +310,7 @@ CacheRpcHandler::HandleRpcPutCacheRecords(const CacheRequestContext& Context, co
 	}
 	DefaultPolicy = !PolicyText.empty() ? ParseCachePolicy(PolicyText) : CachePolicy::Default;
 
-	eastl::fixed_vector<bool, 32> Results;
+	eastl::fixed_vector<ZenCacheStore::PutResult, 32> Results;
 
 	CbArrayView RequestsArray = Params["Requests"sv].AsArrayView();
 	for (CbFieldView RequestField : RequestsArray)
@@ -332,33 +332,50 @@ CacheRpcHandler::HandleRpcPutCacheRecords(const CacheRequestContext& Context, co
 									 .Policy	   = std::move(Policy),
 									 .Context	   = Context};
 
-		PutStatus Result = PutCacheRecord(PutRequest, &BatchRequest);
+		ZenCacheStore::PutResult Result = PutCacheRecord(PutRequest, &BatchRequest);
 
-		if (Result == PutStatus::Invalid)
+		if (Result.Status == PutStatus::Invalid)
 		{
 			return CbPackage{};
 		}
-		Results.push_back(Result == PutStatus::Success);
+		Results.push_back(Result);
 	}
 	if (Results.empty())
 	{
 		return CbPackage{};
 	}
 
+	bool		   bWriteAllDetails = false;
 	CbObjectWriter ResponseObject{256};
 	ResponseObject.BeginArray("Result"sv);
-	for (bool Value : Results)
+	for (const ZenCacheStore::PutResult& Result : Results)
 	{
-		ResponseObject.AddBool(Value);
+		// Conflicts will be treated for response purposes
+		// as successful puts because the key is present in the cache.
+		ResponseObject.AddBool((Result.Status == PutStatus::Success) || (Result.Status == PutStatus::Conflict));
+		if (Result.Details)
+		{
+			bWriteAllDetails = true;
+		}
 	}
 	ResponseObject.EndArray();
 
+	if (bWriteAllDetails)
+	{
+		ResponseObject.BeginArray("Details"sv);
+		for (const ZenCacheStore::PutResult& Result : Results)
+		{
+			ResponseObject.AddObject(Result.Details);
+		}
+		ResponseObject.EndArray();
+	}
+
 	CbPackage RpcResponse;
 	RpcResponse.SetObject(ResponseObject.Save());
 	return RpcResponse;
 }
 
-PutStatus
+ZenCacheStore::PutResult
 CacheRpcHandler::PutCacheRecord(PutRequestData& Request, const CbPackage* Package)
 {
 	CbObjectView Record			  = Request.RecordObject;
@@ -420,7 +437,9 @@ CacheRpcHandler::PutCacheRecord(PutRequestData& Request, const CbPackage* Packag
 
 	if (Count.Invalid > 0)
 	{
-		return PutStatus::Invalid;
+		CbObjectWriter DetailWriter;
+		DetailWriter.AddString(fmt::format("Found {}/{} invalid attachments", Count.Invalid, Count.Total));
+		return ZenCacheStore::PutResult{PutStatus::Invalid, DetailWriter.Save()};
 	}
 
 	ZenCacheValue CacheValue;
@@ -440,7 +459,7 @@ CacheRpcHandler::PutCacheRecord(PutRequestData& Request, const CbPackage* Packag
 														  nullptr);
 	if (PutResult.Status != zen::PutStatus::Success)
 	{
-		return PutResult.Status;
+		return PutResult;
 	}
 	m_CacheStats.WriteCount++;
 
@@ -478,7 +497,7 @@ CacheRpcHandler::PutCacheRecord(PutRequestData& Request, const CbPackage* Packag
 										 .Key			  = Request.Key,
 										 .ValueContentIds = std::move(ValidAttachments)});
 	}
-	return PutStatus::Success;
+	return PutResult;
 }
 
 CbPackage
@@ -1048,7 +1067,9 @@ CacheRpcHandler::HandleRpcPutCacheValues(const CacheRequestContext& Context, con
 				}
 				else
 				{
-					Results.push_back({zen::PutStatus::Fail, fmt::format("Missing attachment with raw hash {}", RawHash)});
+					CbObjectWriter DetailWriter;
+					DetailWriter.AddString(fmt::format("Missing attachment with raw hash {}", RawHash));
+					Results.push_back({zen::PutStatus::Fail, DetailWriter.Save()});
 				}
 			}
 			// We do not search the Upstream. No data in a put means the caller is probing for whether they need to do a heavy put.
@@ -1099,29 +1120,32 @@ CacheRpcHandler::HandleRpcPutCacheValues(const CacheRequestContext& Context, con
 		ZEN_TRACE_CPU("Z$::RpcPutCacheValues::Response");
 		CbObjectWriter ResponseObject{1024};
 		ResponseObject.BeginArray("Result"sv);
-		bool bAnyErrors = false;
+		bool bWriteAllDetails = false;
 		for (const ZenCacheStore::PutResult& Value : Results)
 		{
-			if (Value.Status == zen::PutStatus::Success)
+			// Conflicts will be treated for response purposes
+			// as successful puts because the key is present in the cache.
+			if ((Value.Status == zen::PutStatus::Success) || (Value.Status == zen::PutStatus::Conflict))
 			{
 				ResponseObject.AddBool(true);
 			}
 			else
 			{
-				bAnyErrors = true;
 				ResponseObject.AddBool(false);
 			}
+			// If one result has details, we must write all details
+			if (Value.Details)
+			{
+				bWriteAllDetails = true;
+			}
 		}
 		ResponseObject.EndArray();
-		if (bAnyErrors)
+		if (bWriteAllDetails)
 		{
-			ResponseObject.BeginArray("ErrorMessages"sv);
+			ResponseObject.BeginArray("Details"sv);
 			for (const ZenCacheStore::PutResult& Value : Results)
 			{
-				if (Value.Status != zen::PutStatus::Success)
-				{
-					ResponseObject.AddString(Value.Message);
-				}
+				ResponseObject.AddObject(Value.Details);
 			}
 			ResponseObject.EndArray();
 		}
diff --git a/src/zenstore/cache/structuredcachestore.cpp b/src/zenstore/cache/structuredcachestore.cpp
index 3f27e6d21..51c80dbc7 100644
--- a/src/zenstore/cache/structuredcachestore.cpp
+++ b/src/zenstore/cache/structuredcachestore.cpp
@@ -730,7 +730,9 @@ ZenCacheStore::Put(const CacheRequestContext& Context,
 	if (IsKnownBadBucketName(Bucket))
 	{
 		m_RejectedWriteCount++;
-		return PutResult{zen::PutStatus::Invalid, "Bad bucket name"};
+		CbObjectWriter DetailWriter;
+		DetailWriter.AddString("Bad bucket name");
+		return PutResult{zen::PutStatus::Invalid, DetailWriter.Save()};
 	}
 
 	ZEN_MEMSCOPE(GetCacheStoreTag());
@@ -777,7 +779,10 @@ ZenCacheStore::Put(const CacheRequestContext& Context,
 			 Namespace,
 			 Bucket,
 			 HashKey.ToHexString());
-	return PutResult{zen::PutStatus::Fail, fmt::format("Unknown namespace '{}'", Namespace)};
+
+	CbObjectWriter DetailWriter;
+	DetailWriter.AddString(fmt::format("Unknown namespace '{}'", Namespace));
+	return PutResult{zen::PutStatus::Fail, DetailWriter.Save()};
 }
 
 bool
diff --git a/src/zenstore/include/zenstore/cache/cachedisklayer.h b/src/zenstore/include/zenstore/cache/cachedisklayer.h
index 49c52f847..10c61681b 100644
--- a/src/zenstore/include/zenstore/cache/cachedisklayer.h
+++ b/src/zenstore/include/zenstore/cache/cachedisklayer.h
@@ -4,6 +4,7 @@
 
 #include "cacheshared.h"
 
+#include <zencore/compactbinary.h>
 #include <zencore/stats.h>
 #include <zenstore/accesstime.h>
 #include <zenstore/blockstore.h>
@@ -180,7 +181,7 @@ public:
 	struct PutResult
 	{
 		zen::PutStatus Status;
-		std::string	   Message;
+		CbObject	   Details;
 	};
 
 	explicit ZenCacheDiskLayer(GcManager& Gc, JobQueue& JobQueue, const std::filesystem::path& RootDir, const Configuration& Config);
diff --git a/src/zenstore/include/zenstore/cache/cacherpc.h b/src/zenstore/include/zenstore/cache/cacherpc.h
index 104746aba..e0c8738ca 100644
--- a/src/zenstore/include/zenstore/cache/cacherpc.h
+++ b/src/zenstore/include/zenstore/cache/cacherpc.h
@@ -5,6 +5,7 @@
 #include <zencore/iobuffer.h>
 #include <zencore/logging.h>
 #include <zenstore/cache/cacheshared.h>
+#include <zenstore/cache/structuredcachestore.h>
 #include <zenutil/cache/cache.h>
 
 #include <atomic>
@@ -28,7 +29,6 @@ class CidStore;
 class DiskWriteBlocker;
 class HttpStructuredCacheService;
 class UpstreamCacheClient;
-class ZenCacheStore;
 
 enum class CachePolicy : uint32_t;
 enum class RpcAcceptOptions : uint16_t;
@@ -101,7 +101,7 @@ private:
 	CbPackage HandleRpcGetCacheValues(const CacheRequestContext& Context, CbObjectView BatchRequest);
 	CbPackage HandleRpcGetCacheChunks(const CacheRequestContext& Context, RpcAcceptOptions AcceptOptions, CbObjectView BatchRequest);
 
-	PutStatus PutCacheRecord(PutRequestData& Request, const CbPackage* Package);
+	ZenCacheStore::PutResult PutCacheRecord(PutRequestData& Request, const CbPackage* Package);
 
 	/** HandleRpcGetCacheChunks Helper: Parse the Body object into RecordValue Requests and Value Requests. */
 	bool ParseGetCacheChunksRequest(std::string&							   Namespace,
diff --git a/src/zenutil/cache/cacherequests.cpp b/src/zenutil/cache/cacherequests.cpp
index 7c6f493f2..b8169182d 100644
--- a/src/zenutil/cache/cacherequests.cpp
+++ b/src/zenutil/cache/cacherequests.cpp
@@ -313,6 +313,17 @@ namespace cacherequests {
 			Success.push_back(It.AsBool());
 			It++;
 		}
+
+		CbArrayView DetailsArray = Package.GetObject()["Details"].AsArrayView();
+		if (DetailsArray)
+		{
+			It = DetailsArray.CreateViewIterator();
+			while (It.HasValue())
+			{
+				Details.push_back(It.AsObjectView());
+				It++;
+			}
+		}
 		return true;
 	}
 
@@ -325,7 +336,15 @@ namespace cacherequests {
 			ResponseObject.AddBool(Value);
 		}
 		ResponseObject.EndArray();
-
+		if (!Details.empty())
+		{
+			ResponseObject.BeginArray("Details");
+			for (CbObjectView Value : Details)
+			{
+				ResponseObject.AddObject(Value);
+			}
+			ResponseObject.EndArray();
+		}
 		OutPackage.SetObject(ResponseObject.Save());
 		return true;
 	}
diff --git a/src/zenutil/include/zenutil/cache/cacherequests.h b/src/zenutil/include/zenutil/cache/cacherequests.h
index fbf3e08cc..2842e29f0 100644
--- a/src/zenutil/include/zenutil/cache/cacherequests.h
+++ b/src/zenutil/include/zenutil/cache/cacherequests.h
@@ -85,7 +85,8 @@ namespace cacherequests {
 
 	struct PutCacheRecordsResult
 	{
-		std::vector<bool> Success;
+		std::vector<bool>		  Success;
+		std::vector<CbObjectView> Details;
 
 		bool Parse(const CbPackage& Package);
 		bool Format(CbPackage& OutPackage) const;