Enable FILE_SHARE_DELETE on standalone files in disk buckets

This allows us to delete the file even if it is open for read.
We do a delete, the rename since we are not allowed to do a rename-overwrite, only delete.
As we have the shard lock for the file we want to replace we can safely do a delete+rename.
In the rare case that we fail to rename the file into place the old data is lost.
As this is a *cache* and it should be very rare this is OK.

3 files changed, 123 insertions, 107 deletions

diff --git a/zencore/include/zencore/iobuffer.h b/zencore/include/zencore/iobuffer.h
index 5d9daa1c7..bf658922d 100644
--- a/zencore/include/zencore/iobuffer.h
+++ b/zencore/include/zencore/iobuffer.h
@@ -403,6 +403,7 @@ class IoBufferBuilder
 {
 public:
 	ZENCORE_API static IoBuffer MakeFromFile(const std::filesystem::path& FileName, uint64_t Offset = 0, uint64_t Size = ~0ull);
+	ZENCORE_API static IoBuffer MakeFromFileWithSharedDelete(const std::filesystem::path& FileName);
 	ZENCORE_API static IoBuffer MakeFromTemporaryFile(const std::filesystem::path& FileName);
 	ZENCORE_API static IoBuffer MakeFromFileHandle(void* FileHandle, uint64_t Offset = 0, uint64_t Size = ~0ull);
 	ZENCORE_API static IoBuffer ReadFromFileMaybe(IoBuffer& InBuffer);
diff --git a/zencore/iobuffer.cpp b/zencore/iobuffer.cpp
index c4b7f7bdf..61db25d0f 100644
--- a/zencore/iobuffer.cpp
+++ b/zencore/iobuffer.cpp
@@ -470,6 +470,51 @@ IoBufferBuilder::MakeFromFileHandle(void* FileHandle, uint64_t Offset, uint64_t
 }
 
 IoBuffer
+IoBufferBuilder::MakeFromFileWithSharedDelete(const std::filesystem::path& FileName)
+{
+	uint64_t Size;
+
+#if ZEN_PLATFORM_WINDOWS
+	CAtlFile DataFile;
+
+	HRESULT hRes = DataFile.Create(FileName.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_DELETE, OPEN_EXISTING);
+
+	if (FAILED(hRes))
+	{
+		return {};
+	}
+
+	DataFile.GetSize((ULONGLONG&)Size);
+#else
+	int Fd = open(FileName.c_str(), O_RDONLY | O_CLOEXEC);
+	if (Fd < 0)
+	{
+		return {};
+	}
+
+	static_assert(sizeof(decltype(stat::st_size)) == sizeof(uint64_t), "fstat() doesn't support large files");
+	struct stat Stat;
+	fstat(Fd, &Stat);
+	Size   = Stat.st_size;
+#endif	// ZEN_PLATFORM_WINDOWS
+
+	if (Size)
+	{
+#if ZEN_PLATFORM_WINDOWS
+		void* Fd = DataFile.Detach();
+#endif
+		return IoBuffer(IoBuffer::File, (void*)uintptr_t(Fd), 0, Size);
+	}
+
+#if !ZEN_PLATFORM_WINDOWS
+	close(Fd);
+#endif
+
+	// For an empty file, we may as well just return an empty memory IoBuffer
+	return IoBuffer(IoBuffer::Wrap, "", 0);
+}
+
+IoBuffer
 IoBufferBuilder::MakeFromFile(const std::filesystem::path& FileName, uint64_t Offset, uint64_t Size)
 {
 	uint64_t FileSize;
diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp
index e4dbf390a..d1b242c5c 100644
--- a/zenserver/cache/structuredcachestore.cpp
+++ b/zenserver/cache/structuredcachestore.cpp
@@ -1206,7 +1206,7 @@ ZenCacheDiskLayer::CacheBucket::GetStandaloneCacheValue(const DiskLocation& Loc,
 
 	RwLock::SharedLockScope ValueLock(LockForHash(HashKey));
 
-	if (IoBuffer Data = IoBufferBuilder::MakeFromFile(DataFilePath.ToPath()))
+	if (IoBuffer Data = IoBufferBuilder::MakeFromFileWithSharedDelete(DataFilePath.ToPath()))
 	{
 		OutValue.Value = Data;
 		OutValue.Value.SetContentType(Loc.GetContentType());
@@ -1831,110 +1831,79 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c
 	BuildPath(DataFilePath, HashKey);
 	std::filesystem::path FsPath{DataFilePath.ToPath()};
 
-	// We retry to move the file since it can be held open for read.
-	// This happens if the server processes a Get request for the file or
-	// if we are busy sending the file upstream
-	int RetryCount = 4;
-	do
-	{
-		Ec.clear();
-		{
-			RwLock::ExclusiveLockScope ValueLock(LockForHash(HashKey));
-
-			DataFile.MoveTemporaryIntoPlace(FsPath, Ec);
-
-			// Once we have called MoveTemporaryIntoPlace automatic clean up the temp file
-			// will be disabled as the file handle has already been closed
-			CleanUpTempFile = Ec ? true : false;
+	RwLock::ExclusiveLockScope ValueLock(LockForHash(HashKey));
 
-			if (Ec)
-			{
-				std::error_code ExistingEc;
-				uint64_t		OldFileSize = std::filesystem::file_size(FsPath, ExistingEc);
-				if (!ExistingEc && (OldFileSize == NewFileSize))
-				{
-					ZEN_INFO(
-						"Failed to move temporary file '{}' to '{}' for '{}'. Target file has same size, assuming concurrent write of same "
-						"value, "
-						"move "
-						"failed with reason '{}'",
-						DataFile.GetPath(),
-						FsPath.string(),
-						m_BucketDir,
-						Ec.message());
-					return;
-				}
-			}
-		}
+	std::filesystem::remove(FsPath, Ec);
+	if (Ec && Ec.value() != ENOENT)
+	{
+		throw std::system_error(Ec, fmt::format("Failed to replace file '{}' for put in '{}'", DataFilePath.ToUtf8(), m_BucketDir));
+	}
 
-		if (!Ec)
+	DataFile.MoveTemporaryIntoPlace(FsPath, Ec);
+	if (Ec)
+	{
+		std::filesystem::path ParentPath = FsPath.parent_path();
+		if (std::filesystem::is_directory(ParentPath))
 		{
-			uint8_t EntryFlags = DiskLocation::kStandaloneFile;
-
-			if (Value.Value.GetContentType() == ZenContentType::kCbObject)
-			{
-				EntryFlags |= DiskLocation::kStructured;
-			}
-			else if (Value.Value.GetContentType() == ZenContentType::kCompressedBinary)
-			{
-				EntryFlags |= DiskLocation::kCompressed;
-			}
-
-			DiskLocation Loc(NewFileSize, EntryFlags);
-			IndexEntry	 Entry = IndexEntry(Loc, GcClock::TickCount());
-
-			uint64_t				   OldFileSize = 0;
-			RwLock::ExclusiveLockScope _(m_IndexLock);
-			if (auto It = m_Index.find(HashKey); It == m_Index.end())
-			{
-				// Previously unknown object
-				m_Index.insert({HashKey, Entry});
-			}
-			else
-			{
-				// TODO: should check if write is idempotent and bail out if it is?
-				OldFileSize = It.value().Location.Size();
-				It.value()	= Entry;
-			}
-
-			m_SlogFile.Append({.Key = HashKey, .Location = Loc});
-			if (OldFileSize <= NewFileSize)
-			{
-				m_TotalSize.fetch_add(NewFileSize - OldFileSize, std::memory_order::relaxed);
-			}
-			else
-			{
-				m_TotalSize.fetch_sub(OldFileSize - NewFileSize, std::memory_order::relaxed);
-			}
-			return;
+			throw std::system_error(Ec, fmt::format("Failed to finalize file '{}' for put in '{}'", DataFilePath.ToUtf8(), m_BucketDir));
 		}
-
-		std::filesystem::path ParentPath = FsPath.parent_path();
-		if (!std::filesystem::is_directory(ParentPath))
+		Ec.clear();
+		std::filesystem::create_directories(ParentPath, Ec);
+		if (Ec)
 		{
-			Ec.clear();
-			std::filesystem::create_directories(ParentPath, Ec);
-			if (!Ec)
-			{
-				// Retry without sleep
-				continue;
-			}
 			throw std::system_error(
 				Ec,
 				fmt::format("Failed to create parent directory '{}' for file '{}' for put in '{}'", ParentPath, FsPath, m_BucketDir));
 		}
 
-		ZEN_INFO("Failed renaming temporary file '{}' to '{}' for put in '{}', pausing and retrying, reason '{}'",
-				 DataFile.GetPath().string(),
-				 FsPath.string(),
-				 m_BucketDir,
-				 Ec.message());
+		DataFile.MoveTemporaryIntoPlace(FsPath, Ec);
+		if (Ec)
+		{
+			throw std::system_error(Ec, fmt::format("Failed to finalize file '{}' for put in '{}'", DataFilePath.ToUtf8(), m_BucketDir));
+		}
+	}
+
+	// Once we have called MoveTemporaryIntoPlace automatic clean up the temp file
+	// will be disabled as the file handle has already been closed
+	CleanUpTempFile = false;
 
-		// Semi arbitrary back-off
-		zen::Sleep(200 * (5 - RetryCount));	 // Sleep at most for a total of 3 seconds
-	} while (RetryCount-- > 0);
+	uint8_t EntryFlags = DiskLocation::kStandaloneFile;
 
-	throw std::system_error(Ec, fmt::format("Failed to finalize file '{}' for put in '{}'", DataFilePath.ToUtf8(), m_BucketDir));
+	if (Value.Value.GetContentType() == ZenContentType::kCbObject)
+	{
+		EntryFlags |= DiskLocation::kStructured;
+	}
+	else if (Value.Value.GetContentType() == ZenContentType::kCompressedBinary)
+	{
+		EntryFlags |= DiskLocation::kCompressed;
+	}
+
+	DiskLocation Loc(NewFileSize, EntryFlags);
+	IndexEntry	 Entry = IndexEntry(Loc, GcClock::TickCount());
+
+	uint64_t				   OldFileSize = 0;
+	RwLock::ExclusiveLockScope _(m_IndexLock);
+	if (auto It = m_Index.find(HashKey); It == m_Index.end())
+	{
+		// Previously unknown object
+		m_Index.insert({HashKey, Entry});
+	}
+	else
+	{
+		// TODO: should check if write is idempotent and bail out if it is?
+		OldFileSize = It.value().Location.Size();
+		It.value()	= Entry;
+	}
+
+	m_SlogFile.Append({.Key = HashKey, .Location = Loc});
+	if (OldFileSize <= NewFileSize)
+	{
+		m_TotalSize.fetch_add(NewFileSize - OldFileSize, std::memory_order::relaxed);
+	}
+	else
+	{
+		m_TotalSize.fetch_sub(OldFileSize - NewFileSize, std::memory_order::relaxed);
+	}
 }
 
 void
@@ -3384,7 +3353,7 @@ TEST_CASE("z$.blocked.disklayer.put")
 
 	const auto CreateCacheValue = [](size_t Size) -> CbObject {
 		std::vector<uint8_t> Buf;
-		Buf.resize(Size);
+		Buf.resize(Size, Size & 0xff);
 
 		CbObjectWriter Writer;
 		Writer.AddBinary("Binary"sv, Buf.data(), Buf.size());
@@ -3406,25 +3375,26 @@ TEST_CASE("z$.blocked.disklayer.put")
 	ZenCacheValue BufferGet;
 	CHECK(Zcs.Get("test_bucket", HashKey, BufferGet));
 
-	// Overwriting with a value of same size should go fine
-	Zcs.Put("test_bucket", HashKey, {.Value = Buffer});
-
 	CbObject CacheValue2 = CreateCacheValue(64 * 1024 + 64 + 1);
 	IoBuffer Buffer2	 = CacheValue2.GetBuffer().AsIoBuffer();
 	Buffer2.SetContentType(ZenContentType::kCbObject);
-#	if ZEN_PLATFORM_WINDOWS
-	// On Windows platform, overwriting with different size while we have
-	// it open for read should throw exception if file is held open
-	CHECK_THROWS(Zcs.Put("test_bucket", HashKey, {.Value = Buffer2}));
-#	else
-	// Other platforms should handle overwrite just fine
+
+	// We should be able to overwrite even if the file is open for read
 	Zcs.Put("test_bucket", HashKey, {.Value = Buffer2});
-#	endif
 
-	BufferGet = ZenCacheValue{};
+	MemoryView OldView = BufferGet.Value.GetView();
 
-	// Read access has been removed, we should now be able to overwrite it
-	Zcs.Put("test_bucket", HashKey, {.Value = Buffer2});
+	ZenCacheValue BufferGet2;
+	CHECK(Zcs.Get("test_bucket", HashKey, BufferGet2));
+	MemoryView NewView = BufferGet2.Value.GetView();
+
+	// Make sure file openend for read before we wrote it still have old data
+	CHECK(OldView.GetSize() == Buffer.GetSize());
+	CHECK(memcmp(OldView.GetData(), Buffer.GetData(), OldView.GetSize()) == 0);
+
+	// Make sure we get the new data when reading after we write new data
+	CHECK(NewView.GetSize() == Buffer2.GetSize());
+	CHECK(memcmp(NewView.GetData(), Buffer2.GetData(), NewView.GetSize()) == 0);
 }
 
 #endif