From 3c89c486338890ce39ddebe5be4722a09e85701a Mon Sep 17 00:00:00 2001 From: Stefan Boberg Date: Tue, 24 Feb 2026 13:23:52 +0100 Subject: 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 --- src/zenstore/cas.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'src/zenstore/cas.cpp') 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 Data, - std::span ChunkHashes, - std::span Indexes, - std::vector Results) +GetFileCasResults(FileCasStrategy& Strategy, + CasStore::InsertMode Mode, + std::span Data, + std::span ChunkHashes, + std::span Indexes, + std::vector& Results) { for (size_t Index : Indexes) { -- cgit v1.2.3 From 075bac3ca870a1297e9f62230d56e63aec13a77d Mon Sep 17 00:00:00 2001 From: Stefan Boberg Date: Tue, 24 Feb 2026 13:36:44 +0100 Subject: Revert "Fix correctness and concurrency bugs found during code review" This reverts commit 3c89c486338890ce39ddebe5be4722a09e85701a. --- src/zenstore/cas.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'src/zenstore/cas.cpp') diff --git a/src/zenstore/cas.cpp b/src/zenstore/cas.cpp index 7402d92d3..ed017988f 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 Data, - std::span ChunkHashes, - std::span Indexes, - std::vector& Results) +GetFileCasResults(FileCasStrategy& Strategy, + CasStore::InsertMode Mode, + std::span Data, + std::span ChunkHashes, + std::span Indexes, + std::vector Results) { for (size_t Index : Indexes) { -- cgit v1.2.3 From 5c5e12d1f02bb7cc1f42750e47a2735dc933c194 Mon Sep 17 00:00:00 2001 From: Stefan Boberg Date: Tue, 24 Feb 2026 14:56:57 +0100 Subject: Various bug fixes (#778) zencore fixes: - filesystem.cpp: ReadFile error reporting logic - compactbinaryvalue.h: CbValue::As*String error reporting logic zenhttp fixes: - httpasio BindAcceptor would `return 0;` in a function returning `std::string` (UB) - httpsys async workpool initialization race 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) --- src/zenstore/cas.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'src/zenstore/cas.cpp') 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 Data, - std::span ChunkHashes, - std::span Indexes, - std::vector Results) +GetFileCasResults(FileCasStrategy& Strategy, + CasStore::InsertMode Mode, + std::span Data, + std::span ChunkHashes, + std::span Indexes, + std::vector& Results) { for (size_t Index : Indexes) { -- cgit v1.2.3 From d604351cb5dc3032a7cb8c84d6ad5f1480325e5c Mon Sep 17 00:00:00 2001 From: Stefan Boberg Date: Mon, 2 Mar 2026 09:37:14 +0100 Subject: Add test suites (#799) Makes all test cases part of a test suite. Test suites are named after the module and the name of the file containing the implementation of the test. * This allows for better and more predictable filtering of which test cases to run which should also be able to reduce the time CI spends in tests since it can filter on the tests for that particular module. Also improves `xmake test` behaviour: * instead of an explicit list of projects just enumerate the test projects which are available based on build system state * also introduces logic to avoid running `xmake config` unnecessarily which would invalidate the existing build and do lots of unnecessary work since dependencies were invalidated by the updated config * also invokes build only for the chosen test targets As a bonus, also adds `xmake sln --open` which allows opening IDE after generation of solution/xmake project is done. --- src/zenstore/cas.cpp | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'src/zenstore/cas.cpp') diff --git a/src/zenstore/cas.cpp b/src/zenstore/cas.cpp index 7402d92d3..f80952322 100644 --- a/src/zenstore/cas.cpp +++ b/src/zenstore/cas.cpp @@ -512,6 +512,8 @@ CreateCasStore(GcManager& Gc) #if ZEN_WITH_TESTS +TEST_SUITE_BEGIN("store.cas"); + TEST_CASE("CasStore") { ScopedTemporaryDirectory TempDir; @@ -553,6 +555,8 @@ TEST_CASE("CasStore") CHECK(Lookup2); } +TEST_SUITE_END(); + void CAS_forcelink() { -- cgit v1.2.3 From 5115b419cefd41e8d5cc465c8c7ae5140cde71d4 Mon Sep 17 00:00:00 2001 From: Stefan Boberg Date: Fri, 6 Mar 2026 12:39:06 +0100 Subject: zenstore bug-fixes from static analysis pass (#815) **Bug fixes across zenstore, zenremotestore, and related subsystems, primarily surfaced by static analysis.** ## Cache subsystem (cachedisklayer.cpp) - Fixed tombstone scoping bug: tombstone flag and missing entry were recorded outside the block where data was removed, causing non-missing entries to be incorrectly tombstoned - Fixed use-after-overwrite: `RemoveMemCachedData`/`RemoveMetaData` were called after `Payload` was overwritten on cache put, leaking stale data - Fixed incorrect retry sleep formula (`100 - (3 - RetriesLeft) * 100` always produced the same or negative value; corrected to `(3 - RetriesLeft) * 100`) - Fixed broken `break` missing from sidecar file read loop, causing reads past valid data - Fixed missing format argument in three `ZEN_WARN`/`ZEN_ERROR` log calls (format string had `{}` placeholders with no corresponding argument, or vice versa) - Fixed elapsed timer being accumulated inside the wrong scope in `HandleRpcGetCacheRecords` - Fixed test asserting against unserialized `RecordPolicy` instead of the deserialized `Loaded` copy - Initialized `AbortFlag`/`PauseFlag` atomics at declaration (UB if read before first write) ## Build store (buildstore.cpp / buildstore.h) - Fixed wrong variable used in warning log: used loop index `ResultIndex` instead of `Index`/`MetaLocationResultIndexes[Index]`, logging wrong hash values - Fixed `sizeof(AccessTimesHeader)` used instead of `sizeof(AccessTimeRecord)` when advancing write offset, corrupting the access times file if the sizes differ - Initialized `m_LastAccessTimeUpdateCount` atomic member (was uninitialized) - Changed map iteration loops to use `const auto&` to avoid unnecessary copies ## Project store (projectstore.cpp / projectstore.h) - Fixed wrong iterator dereferenced in `IterateChunks`: used `ChunkIt->second` (from a different map lookup) instead of `MetaIt->second` - Fixed wrong assert variable: `Sizes[Index]` should be `RawSizes[Index]` - Fixed `MakeTombstone`/`IsTombstone` inconsistency: `MakeTombstone` was zeroing `OpLsn` but `IsTombstone` checks `OpLsn.Number != 0`; tombstone creation now preserves `OpLsn` - Fixed uninitialized `InvalidEntries` counter - Fixed format string mismatch in warning log - Initialized `AbortFlag`/`PauseFlag` atomics; changed map iteration to `const auto&` ## Workspaces (workspaces.cpp) - Fixed missing alias registration when a workspace share is updated: alias was deleted but never re-inserted - Fixed integer overflow in range clamping: `(RequestedOffset + RequestedSize) > Size` could wrap; corrected to `RequestedSize > Size - RequestedOffset` - Changed map iteration loops to `const auto&` ## CAS subsystem (cas.cpp, caslog.cpp, compactcas.cpp, filecas.cpp) - Fixed `IterateChunks` passing original `Payload` buffer instead of the modified `Chunk` buffer (content type was set on the copy but the original was sent to the callback) - Fixed invalid `std::future::get()` call on default-constructed futures - Fixed sign-comparison in `CasLogFile::Replay` loop (`int i` vs `size_t`) - Changed `CasLogFile::IsValid` and `Open` to take `const std::filesystem::path&` instead of by value - Fixed format string in `~CasContainerStrategy` error log ## Remote store (zenremotestore) - Fixed `FolderContent::operator==` always returning true: loop variable `PathCount` was initialized to 0 instead of `Paths.size()` - Fixed `GetChunkIndexForRawHash` looking up from wrong map (`RawHashToSequenceIndex` instead of `ChunkHashToChunkIndex`) - Fixed double-counted `UniqueSequencesFound` stat (incremented in both branches of an if/else) - Fixed `RawSize` sentinel value truncation: `(uint32_t)-1` assigned to a `uint64_t` field; corrected to `(uint64_t)-1` - Initialized uninitialized atomic and struct members across `buildstorageoperations.h`, `chunkblock.h`, and `remoteprojectstore.h` --- src/zenstore/cas.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'src/zenstore/cas.cpp') diff --git a/src/zenstore/cas.cpp b/src/zenstore/cas.cpp index f80952322..8855c87d8 100644 --- a/src/zenstore/cas.cpp +++ b/src/zenstore/cas.cpp @@ -153,7 +153,10 @@ CasImpl::Initialize(const CidStoreConfiguration& InConfig) } for (std::future& Result : Work) { - Result.get(); + if (Result.valid()) + { + Result.get(); + } } } } @@ -426,7 +429,7 @@ CasImpl::IterateChunks(std::span DecompressedIds, [&](size_t Index, const IoBuffer& Payload) { IoBuffer Chunk(Payload); Chunk.SetContentType(ZenContentType::kCompressedBinary); - return AsyncCallback(Index, Payload); + return AsyncCallback(Index, Chunk); }, OptionalWorkerPool, LargeSizeLimit == 0 ? m_Config.HugeValueThreshold : Min(LargeSizeLimit, m_Config.HugeValueThreshold))) @@ -439,7 +442,7 @@ CasImpl::IterateChunks(std::span DecompressedIds, [&](size_t Index, const IoBuffer& Payload) { IoBuffer Chunk(Payload); Chunk.SetContentType(ZenContentType::kCompressedBinary); - return AsyncCallback(Index, Payload); + return AsyncCallback(Index, Chunk); }, OptionalWorkerPool, LargeSizeLimit == 0 ? m_Config.TinyValueThreshold : Min(LargeSizeLimit, m_Config.TinyValueThreshold))) @@ -452,7 +455,7 @@ CasImpl::IterateChunks(std::span DecompressedIds, [&](size_t Index, const IoBuffer& Payload) { IoBuffer Chunk(Payload); Chunk.SetContentType(ZenContentType::kCompressedBinary); - return AsyncCallback(Index, Payload); + return AsyncCallback(Index, Chunk); }, OptionalWorkerPool)) { -- cgit v1.2.3