aboutsummaryrefslogtreecommitdiff
path: root/src/zenstore/cache
diff options
context:
space:
mode:
authorStefan Boberg <[email protected]>2026-03-06 12:39:06 +0100
committerGitHub Enterprise <[email protected]>2026-03-06 12:39:06 +0100
commit5115b419cefd41e8d5cc465c8c7ae5140cde71d4 (patch)
treec97f8d658fa0ec24664264b97327120f2c30bd7f /src/zenstore/cache
parentClaude config, some bug fixes (#813) (diff)
downloadzen-5115b419cefd41e8d5cc465c8c7ae5140cde71d4.tar.xz
zen-5115b419cefd41e8d5cc465c8c7ae5140cde71d4.zip
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`
Diffstat (limited to 'src/zenstore/cache')
-rw-r--r--src/zenstore/cache/cachedisklayer.cpp32
-rw-r--r--src/zenstore/cache/cachepolicy.cpp14
-rw-r--r--src/zenstore/cache/cacherpc.cpp4
-rw-r--r--src/zenstore/cache/structuredcachestore.cpp8
4 files changed, 30 insertions, 28 deletions
diff --git a/src/zenstore/cache/cachedisklayer.cpp b/src/zenstore/cache/cachedisklayer.cpp
index b73b3e6fb..d53f9f369 100644
--- a/src/zenstore/cache/cachedisklayer.cpp
+++ b/src/zenstore/cache/cachedisklayer.cpp
@@ -602,7 +602,7 @@ BucketManifestSerializer::ReadSidecarFile(RwLock::ExclusiveLockScope& B
if (FileSize < sizeof(BucketMetaHeader))
{
- ZEN_WARN("Failed to read sidecar file '{}'. Minimum size {} expected, actual size: ",
+ ZEN_WARN("Failed to read sidecar file '{}'. Minimum size {} expected, actual size: {}",
SidecarPath,
sizeof(BucketMetaHeader),
FileSize);
@@ -654,6 +654,7 @@ BucketManifestSerializer::ReadSidecarFile(RwLock::ExclusiveLockScope& B
SidecarPath,
sizeof(ManifestData),
CurrentReadOffset);
+ break;
}
CurrentReadOffset += sizeof(ManifestData);
@@ -1011,7 +1012,7 @@ ZenCacheDiskLayer::CacheBucket::WriteIndexSnapshotLocked(uint64_t LogPosi
{
// This is non-critical, it only means that we will replay the events of the log over the snapshot - inefficent but in
// the end it will be the same result
- ZEN_WARN("snapshot failed to clean log file '{}', reason: '{}'", LogPath, IndexPath, Ec.message());
+ ZEN_WARN("snapshot failed to clean log file '{}', reason: '{}'", LogPath, Ec.message());
}
m_SlogFile.Open(LogPath, CasLogFile::Mode::kWrite);
}
@@ -1267,10 +1268,10 @@ ZenCacheDiskLayer::CacheBucket::InitializeIndexFromDisk(RwLock::ExclusiveLockSco
{
RemoveMemCachedData(IndexLock, Payload);
RemoveMetaData(IndexLock, Payload);
+ Location.Flags |= DiskLocation::kTombStone;
+ MissingEntries.push_back(DiskIndexEntry{.Key = It.first, .Location = Location});
}
}
- Location.Flags |= DiskLocation::kTombStone;
- MissingEntries.push_back(DiskIndexEntry{.Key = It.first, .Location = Location});
}
ZEN_ASSERT(!MissingEntries.empty());
@@ -2812,7 +2813,7 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c
m_BucketDir,
Ec.message(),
RetriesLeft);
- Sleep(100 - (3 - RetriesLeft) * 100); // Total 600 ms
+ Sleep((3 - RetriesLeft) * 100); // Total 600 ms
Ec.clear();
DataFile.MoveTemporaryIntoPlace(FsPath, Ec);
RetriesLeft--;
@@ -2866,11 +2867,12 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c
{
EntryIndex = It.value();
ZEN_ASSERT_SLOW(EntryIndex < PayloadIndex(m_AccessTimes.size()));
- BucketPayload& Payload = m_Payloads[EntryIndex];
- uint64_t OldSize = Payload.Location.Size();
+ BucketPayload& Payload = m_Payloads[EntryIndex];
+ uint64_t OldSize = Payload.Location.Size();
+ RemoveMemCachedData(IndexLock, Payload);
+ RemoveMetaData(IndexLock, Payload);
Payload = BucketPayload{.Location = Loc};
m_AccessTimes[EntryIndex] = GcClock::TickCount();
- RemoveMemCachedData(IndexLock, Payload);
m_StandaloneSize.fetch_sub(OldSize, std::memory_order::relaxed);
}
if ((Value.RawSize != 0 || Value.RawHash != IoHash::Zero) && Value.RawSize <= std::numeric_limits<std::uint32_t>::max())
@@ -3521,7 +3523,7 @@ ZenCacheDiskLayer::CacheBucket::GetReferences(const LoggerRef& Logger,
}
else
{
- ZEN_WARN("Cache record {} payload is malformed. Reason: ", RawHash, ToString(Error));
+ ZEN_WARN("Cache record {} payload is malformed. Reason: {}", RawHash, ToString(Error));
}
return false;
};
@@ -4282,8 +4284,8 @@ ZenCacheDiskLayer::DiscoverBuckets()
RwLock SyncLock;
WorkerThreadPool& Pool = GetLargeWorkerPool(EWorkloadType::Burst);
- std::atomic<bool> AbortFlag;
- std::atomic<bool> PauseFlag;
+ std::atomic<bool> AbortFlag{false};
+ std::atomic<bool> PauseFlag{false};
ParallelWork Work(AbortFlag, PauseFlag, WorkerThreadPool::EMode::DisableBacklog);
try
{
@@ -4454,8 +4456,8 @@ ZenCacheDiskLayer::Flush()
}
{
WorkerThreadPool& Pool = GetMediumWorkerPool(EWorkloadType::Burst);
- std::atomic<bool> AbortFlag;
- std::atomic<bool> PauseFlag;
+ std::atomic<bool> AbortFlag{false};
+ std::atomic<bool> PauseFlag{false};
ParallelWork Work(AbortFlag, PauseFlag, WorkerThreadPool::EMode::DisableBacklog);
try
{
@@ -4496,8 +4498,8 @@ ZenCacheDiskLayer::Scrub(ScrubContext& Ctx)
RwLock::SharedLockScope _(m_Lock);
- std::atomic<bool> Abort;
- std::atomic<bool> Pause;
+ std::atomic<bool> Abort{false};
+ std::atomic<bool> Pause{false};
ParallelWork Work(Abort, Pause, WorkerThreadPool::EMode::DisableBacklog);
try
diff --git a/src/zenstore/cache/cachepolicy.cpp b/src/zenstore/cache/cachepolicy.cpp
index ce6a14bd9..c1e7dc5b3 100644
--- a/src/zenstore/cache/cachepolicy.cpp
+++ b/src/zenstore/cache/cachepolicy.cpp
@@ -403,13 +403,13 @@ TEST_CASE("cacherecordpolicy")
RecordPolicy.Save(Writer);
CbObject Saved = Writer.Save()->AsObject();
CacheRecordPolicy Loaded = CacheRecordPolicy::Load(Saved).Get();
- CHECK(!RecordPolicy.IsUniform());
- CHECK(RecordPolicy.GetRecordPolicy() == UnionPolicy);
- CHECK(RecordPolicy.GetBasePolicy() == DefaultPolicy);
- CHECK(RecordPolicy.GetValuePolicy(PartialOid) == PartialOverlap);
- CHECK(RecordPolicy.GetValuePolicy(NoOverlapOid) == NoOverlap);
- CHECK(RecordPolicy.GetValuePolicy(OtherOid) == DefaultValuePolicy);
- CHECK(RecordPolicy.GetValuePolicies().size() == 2);
+ CHECK(!Loaded.IsUniform());
+ CHECK(Loaded.GetRecordPolicy() == UnionPolicy);
+ CHECK(Loaded.GetBasePolicy() == DefaultPolicy);
+ CHECK(Loaded.GetValuePolicy(PartialOid) == PartialOverlap);
+ CHECK(Loaded.GetValuePolicy(NoOverlapOid) == NoOverlap);
+ CHECK(Loaded.GetValuePolicy(OtherOid) == DefaultValuePolicy);
+ CHECK(Loaded.GetValuePolicies().size() == 2);
}
}
diff --git a/src/zenstore/cache/cacherpc.cpp b/src/zenstore/cache/cacherpc.cpp
index e1fd0a3e6..90c5a5e60 100644
--- a/src/zenstore/cache/cacherpc.cpp
+++ b/src/zenstore/cache/cacherpc.cpp
@@ -866,8 +866,8 @@ CacheRpcHandler::HandleRpcGetCacheRecords(const CacheRequestContext& Context, Cb
Request.Complete = false;
}
}
- Request.ElapsedTimeUs += Timer.GetElapsedTimeUs();
}
+ Request.ElapsedTimeUs += Timer.GetElapsedTimeUs();
};
m_UpstreamCache.GetCacheRecords(*Namespace, UpstreamRequests, std::move(OnCacheRecordGetComplete));
@@ -934,7 +934,7 @@ CacheRpcHandler::HandleRpcGetCacheRecords(const CacheRequestContext& Context, Cb
*Namespace,
Key.Bucket,
Key.Hash,
- Request.RecordObject ? ""sv : " (PARTIAL)"sv,
+ Request.RecordObject ? " (PARTIAL)"sv : ""sv,
Request.Source ? Request.Source->Url : "LOCAL"sv,
NiceLatencyNs(Request.ElapsedTimeUs * 1000));
m_CacheStats.MissCount++;
diff --git a/src/zenstore/cache/structuredcachestore.cpp b/src/zenstore/cache/structuredcachestore.cpp
index 18023e2d6..cff0e9a35 100644
--- a/src/zenstore/cache/structuredcachestore.cpp
+++ b/src/zenstore/cache/structuredcachestore.cpp
@@ -686,8 +686,8 @@ ZenCacheStore::Get(const CacheRequestContext& Context,
return false;
}
ZEN_WARN("request for unknown namespace '{}' in ZenCacheStore::Get [{}], bucket '{}', key '{}'",
- Context,
Namespace,
+ Context,
Bucket,
HashKey.ToHexString());
@@ -722,8 +722,8 @@ ZenCacheStore::Get(const CacheRequestContext& Context,
}
ZEN_WARN("request for unknown namespace '{}' in ZenCacheStore::Get [{}], bucket '{}', key '{}'",
- Context,
Namespace,
+ Context,
Bucket,
HashKey.ToHexString());
@@ -790,8 +790,8 @@ ZenCacheStore::Put(const CacheRequestContext& Context,
}
ZEN_WARN("request for unknown namespace '{}' in ZenCacheStore::Put [{}] bucket '{}', key '{}'",
- Context,
Namespace,
+ Context,
Bucket,
HashKey.ToHexString());
@@ -816,7 +816,7 @@ ZenCacheStore::DropNamespace(std::string_view InNamespace)
{
std::function<void()> PostDropOp;
{
- RwLock::SharedLockScope _(m_NamespacesLock);
+ RwLock::ExclusiveLockScope _(m_NamespacesLock);
if (auto It = m_Namespaces.find(std::string(InNamespace)); It != m_Namespaces.end())
{
ZenCacheNamespace& Namespace = *It->second;