diff options
| author | Stefan Boberg <[email protected]> | 2026-02-24 13:23:52 +0100 |
|---|---|---|
| committer | Stefan Boberg <[email protected]> | 2026-02-24 13:27:53 +0100 |
| commit | 3c89c486338890ce39ddebe5be4722a09e85701a (patch) | |
| tree | 1382d8e81683072f7cb3a7505e6af3bda7cd0312 | |
| parent | implement yaml generation (#774) (diff) | |
| download | zen-3c89c486338890ce39ddebe5be4722a09e85701a.tar.xz zen-3c89c486338890ce39ddebe5be4722a09e85701a.zip | |
Fix correctness and concurrency bugs found during code review
zenstore fixes:
- cas.cpp: GetFileCasResults Results param passed by value instead of reference (large chunk results were silently lost)
- structuredcachestore.cpp: MissCount unconditionally incremented (counted hits as misses)
- cacherpc.cpp: Wrong boolean in Incomplete response array (all entries marked incomplete)
- cachedisklayer.cpp: sizeof(sizeof(...)) in two validation checks computed sizeof(size_t) instead of struct size
- buildstore.cpp: Wrong hash tracked in GC key list (BlobHash pushed twice instead of MetadataHash)
- buildstore.cpp: Removed duplicate m_LastAccessTimeUpdateCount increment in PutBlob
zenserver fixes:
- httpbuildstore.cpp: Reversed subtraction in HTTP range calculation (unsigned underflow)
- hubservice.cpp: Deadlock in Provision() calling Wake() while holding m_Lock (extracted WakeLocked helper)
- zipfs.cpp: Data race in GetFile() lazy initialization (added RwLock with shared/exclusive paths)
Co-Authored-By: Claude Opus 4.6 <[email protected]>
| -rw-r--r-- | src/zenserver/frontend/zipfs.cpp | 20 | ||||
| -rw-r--r-- | src/zenserver/frontend/zipfs.h | 2 | ||||
| -rw-r--r-- | src/zenserver/hub/hubservice.cpp | 12 | ||||
| -rw-r--r-- | src/zenserver/storage/buildstore/httpbuildstore.cpp | 2 | ||||
| -rw-r--r-- | src/zenstore/buildstore/buildstore.cpp | 3 | ||||
| -rw-r--r-- | src/zenstore/cache/cachedisklayer.cpp | 4 | ||||
| -rw-r--r-- | src/zenstore/cache/cacherpc.cpp | 2 | ||||
| -rw-r--r-- | src/zenstore/cache/structuredcachestore.cpp | 5 | ||||
| -rw-r--r-- | src/zenstore/cas.cpp | 12 |
9 files changed, 42 insertions, 20 deletions
diff --git a/src/zenserver/frontend/zipfs.cpp b/src/zenserver/frontend/zipfs.cpp index f9c2bc8ff..42df0520f 100644 --- a/src/zenserver/frontend/zipfs.cpp +++ b/src/zenserver/frontend/zipfs.cpp @@ -149,13 +149,25 @@ ZipFs::ZipFs(IoBuffer&& Buffer) IoBuffer ZipFs::GetFile(const std::string_view& FileName) const { - FileMap::iterator Iter = m_Files.find(FileName); - if (Iter == m_Files.end()) { - return {}; + RwLock::SharedLockScope _(m_FilesLock); + + FileMap::const_iterator Iter = m_Files.find(FileName); + if (Iter == m_Files.end()) + { + return {}; + } + + const FileItem& Item = Iter->second; + if (Item.GetSize() > 0) + { + return IoBuffer(IoBuffer::Wrap, Item.GetData(), Item.GetSize()); + } } - FileItem& Item = Iter->second; + RwLock::ExclusiveLockScope _(m_FilesLock); + + FileItem& Item = m_Files.find(FileName)->second; if (Item.GetSize() > 0) { return IoBuffer(IoBuffer::Wrap, Item.GetData(), Item.GetSize()); diff --git a/src/zenserver/frontend/zipfs.h b/src/zenserver/frontend/zipfs.h index 1fa7da451..19f96567c 100644 --- a/src/zenserver/frontend/zipfs.h +++ b/src/zenserver/frontend/zipfs.h @@ -3,6 +3,7 @@ #pragma once #include <zencore/iobuffer.h> +#include <zencore/thread.h> #include <unordered_map> @@ -20,6 +21,7 @@ public: private: using FileItem = MemoryView; using FileMap = std::unordered_map<std::string_view, FileItem>; + mutable RwLock m_FilesLock; FileMap mutable m_Files; IoBuffer m_Buffer; }; diff --git a/src/zenserver/hub/hubservice.cpp b/src/zenserver/hub/hubservice.cpp index 4d9da3a57..a00446a75 100644 --- a/src/zenserver/hub/hubservice.cpp +++ b/src/zenserver/hub/hubservice.cpp @@ -151,6 +151,7 @@ struct StorageServerInstance inline uint16_t GetBasePort() const { return m_ServerInstance.GetBasePort(); } private: + void WakeLocked(); RwLock m_Lock; std::string m_ModuleId; std::atomic<bool> m_IsProvisioned{false}; @@ -211,7 +212,7 @@ StorageServerInstance::Provision() if (m_IsHibernated) { - Wake(); + WakeLocked(); } else { @@ -294,9 +295,14 @@ StorageServerInstance::Hibernate() void StorageServerInstance::Wake() { - // Start server in-place using existing data - RwLock::ExclusiveLockScope _(m_Lock); + WakeLocked(); +} + +void +StorageServerInstance::WakeLocked() +{ + // Start server in-place using existing data if (!m_IsHibernated) { diff --git a/src/zenserver/storage/buildstore/httpbuildstore.cpp b/src/zenserver/storage/buildstore/httpbuildstore.cpp index f5ba30616..bf7afcc02 100644 --- a/src/zenserver/storage/buildstore/httpbuildstore.cpp +++ b/src/zenserver/storage/buildstore/httpbuildstore.cpp @@ -185,7 +185,7 @@ HttpBuildStoreService::GetBlobRequest(HttpRouterRequest& Req) { const HttpRange& Range = Ranges.front(); const uint64_t BlobSize = Blob.GetSize(); - const uint64_t MaxBlobSize = Range.Start < BlobSize ? Range.Start - BlobSize : 0; + const uint64_t MaxBlobSize = Range.Start < BlobSize ? BlobSize - Range.Start : 0; const uint64_t RangeSize = Min(Range.End - Range.Start + 1, MaxBlobSize); if (Range.Start + RangeSize > BlobSize) { diff --git a/src/zenstore/buildstore/buildstore.cpp b/src/zenstore/buildstore/buildstore.cpp index 04a0781d3..aa37e75fe 100644 --- a/src/zenstore/buildstore/buildstore.cpp +++ b/src/zenstore/buildstore/buildstore.cpp @@ -266,13 +266,12 @@ BuildStore::PutBlob(const IoHash& BlobHash, const IoBuffer& Payload) m_BlobLookup.insert({BlobHash, NewBlobIndex}); } - m_LastAccessTimeUpdateCount++; if (m_TrackedBlobKeys) { m_TrackedBlobKeys->push_back(BlobHash); if (MetadataHash != IoHash::Zero) { - m_TrackedBlobKeys->push_back(BlobHash); + m_TrackedBlobKeys->push_back(MetadataHash); } } } diff --git a/src/zenstore/cache/cachedisklayer.cpp b/src/zenstore/cache/cachedisklayer.cpp index ead7e4f3a..b73b3e6fb 100644 --- a/src/zenstore/cache/cachedisklayer.cpp +++ b/src/zenstore/cache/cachedisklayer.cpp @@ -626,7 +626,7 @@ BucketManifestSerializer::ReadSidecarFile(RwLock::ExclusiveLockScope& B return false; } - const uint64_t ExpectedEntryCount = (FileSize - sizeof(sizeof(BucketMetaHeader))) / sizeof(ManifestData); + const uint64_t ExpectedEntryCount = (FileSize - sizeof(BucketMetaHeader)) / sizeof(ManifestData); if (Header.EntryCount > ExpectedEntryCount) { ZEN_WARN( @@ -1057,7 +1057,7 @@ ZenCacheDiskLayer::CacheBucket::ReadIndexFile(RwLock::ExclusiveLockScope&, const return 0; } - const uint64_t ExpectedEntryCount = (FileSize - sizeof(sizeof(cache::impl::CacheBucketIndexHeader))) / sizeof(DiskIndexEntry); + const uint64_t ExpectedEntryCount = (FileSize - sizeof(cache::impl::CacheBucketIndexHeader)) / sizeof(DiskIndexEntry); if (Header.EntryCount > ExpectedEntryCount) { return 0; diff --git a/src/zenstore/cache/cacherpc.cpp b/src/zenstore/cache/cacherpc.cpp index 94abcf547..e1fd0a3e6 100644 --- a/src/zenstore/cache/cacherpc.cpp +++ b/src/zenstore/cache/cacherpc.cpp @@ -966,7 +966,7 @@ CacheRpcHandler::HandleRpcGetCacheRecords(const CacheRequestContext& Context, Cb } else { - ResponseObject.AddBool(true); + ResponseObject.AddBool(false); } } ResponseObject.EndArray(); diff --git a/src/zenstore/cache/structuredcachestore.cpp b/src/zenstore/cache/structuredcachestore.cpp index 52b494e45..4e8475293 100644 --- a/src/zenstore/cache/structuredcachestore.cpp +++ b/src/zenstore/cache/structuredcachestore.cpp @@ -608,7 +608,10 @@ ZenCacheStore::GetBatch::Commit() m_CacheStore.m_HitCount++; OpScope.SetBytes(Result.Value.GetSize()); } - m_CacheStore.m_MissCount++; + else + { + m_CacheStore.m_MissCount++; + } } } } diff --git a/src/zenstore/cas.cpp b/src/zenstore/cas.cpp index ed017988f..7402d92d3 100644 --- a/src/zenstore/cas.cpp +++ b/src/zenstore/cas.cpp @@ -300,12 +300,12 @@ GetCompactCasResults(CasContainerStrategy& Strategy, }; static void -GetFileCasResults(FileCasStrategy& Strategy, - CasStore::InsertMode Mode, - std::span<IoBuffer> Data, - std::span<IoHash> ChunkHashes, - std::span<size_t> Indexes, - std::vector<CasStore::InsertResult> Results) +GetFileCasResults(FileCasStrategy& Strategy, + CasStore::InsertMode Mode, + std::span<IoBuffer> Data, + std::span<IoHash> ChunkHashes, + std::span<size_t> Indexes, + std::vector<CasStore::InsertResult>& Results) { for (size_t Index : Indexes) { |