Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions libs/client-sdk/src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ target_sources(${LIBNAME} PRIVATE
data_sources/fdv2/polling_synchronizer.cpp
data_sources/fdv2/streaming_synchronizer.cpp
data_sources/fdv2/fdv2_data_source.cpp
data_sources/fdv2/cache_initializer.cpp
data_sources/data_source_event_handler.cpp
data_sources/polling_data_source.cpp
flag_manager/flag_store.cpp
Expand Down Expand Up @@ -49,6 +50,7 @@ target_sources(${LIBNAME} PRIVATE
data_sources/fdv2/polling_synchronizer.hpp
data_sources/fdv2/streaming_synchronizer.hpp
data_sources/fdv2/fdv2_data_source.hpp
data_sources/fdv2/cache_initializer.hpp
flag_manager/flag_store.hpp
flag_manager/flag_updater.hpp
bindings/c/sdk.cpp
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class IDataSourceUpdateSink {
* instead.
*
* @param from_cache Whether the changeset was loaded from the local
* cache, in which case it is not written back to it.
* cache. Such data is not written back, nor treated as current.
*/
virtual void Apply(Context const& context,
FlagChangeSet change_set,
Expand Down
60 changes: 60 additions & 0 deletions libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#include "cache_initializer.hpp"

#include <utility>

namespace launchdarkly::client_side::data_sources {

static char const* const kIdentity = "FDv2 cache initializer";

FDv2CacheInitializer::FDv2CacheInitializer(flag_manager::FlagPersistence* cache,
Context context,
Logger const& logger)

Check warning on line 11 in libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp:11:44 [modernize-pass-by-value]

pass by value and use std::move
: cache_(cache), context_(std::move(context)), logger_(logger) {}

async::Future<FDv2SourceResult> FDv2CacheInitializer::Run() {
auto data = cache_->ReadCached(context_);
if (!data) {
LD_LOG(logger_, LogLevel::kDebug)
<< kIdentity << ": no cached data for this context";
// A miss leaves the data set unchanged so initialization can continue.
return async::MakeFuture(FDv2SourceResult{FDv2SourceResult::ChangeSet{
FlagChangeSet{data_model::ChangeSetType::kNone,
{},
data_model::Selector{}}}});
}

LD_LOG(logger_, LogLevel::kDebug)
<< kIdentity << ": loaded " << data->size()
<< " flags for this context";

FlagChangeSetData changes;
changes.reserve(data->size());
for (auto& [key, item] : *data) {
changes.push_back(FlagChange{key, std::move(item)});
}

return async::MakeFuture(FDv2SourceResult{FDv2SourceResult::ChangeSet{
FlagChangeSet{data_model::ChangeSetType::kFull, std::move(changes),
data_model::Selector{}}}});
}

void FDv2CacheInitializer::Close() {
// Run() completes on the calling thread, so there is nothing to cancel.
}

std::string const& FDv2CacheInitializer::Identity() const {
static std::string const identity = kIdentity;
return identity;
}

FDv2CacheInitializerFactory::FDv2CacheInitializerFactory(
flag_manager::FlagPersistence* cache,
Context context,
Logger const& logger)

Check warning on line 53 in libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp:53:5 [modernize-pass-by-value]

pass by value and use std::move
: cache_(cache), context_(std::move(context)), logger_(logger) {}

std::unique_ptr<IFDv2Initializer> FDv2CacheInitializerFactory::Build() {
return std::make_unique<FDv2CacheInitializer>(cache_, context_, logger_);
}

} // namespace launchdarkly::client_side::data_sources
64 changes: 64 additions & 0 deletions libs/client-sdk/src/data_sources/fdv2/cache_initializer.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
#pragma once

#include "ifdv2_initializer.hpp"
#include "ifdv2_initializer_factory.hpp"

#include "../../flag_manager/flag_persistence.hpp"

#include <launchdarkly/async/promise.hpp>
#include <launchdarkly/context.hpp>
#include <launchdarkly/logging/logger.hpp>

#include <string>

namespace launchdarkly::client_side::data_sources {

/**
* Loads flag data the SDK persisted for this context on a previous run, so
* that evaluation can begin before the network answers.
*/
class FDv2CacheInitializer final : public IFDv2Initializer {
public:
/**
* @param cache Local cache to read. Non-owning. Must outlive this object.
* @param context The evaluation context to load data for.
* @param logger Receives diagnostic logging.
*/
FDv2CacheInitializer(flag_manager::FlagPersistence* cache,
Context context,
Logger const& logger);

async::Future<FDv2SourceResult> Run() override;

void Close() override;

[[nodiscard]] std::string const& Identity() const override;

private:
flag_manager::FlagPersistence* const cache_;
Context const context_;
Logger logger_;
};

/**
* Builds fresh FDv2CacheInitializer instances on demand.
*
* Thread-safe: Build() may be called from any thread.
*/
class FDv2CacheInitializerFactory final : public IFDv2InitializerFactory {
public:
FDv2CacheInitializerFactory(flag_manager::FlagPersistence* cache,
Context context,
Logger const& logger);

std::unique_ptr<IFDv2Initializer> Build() override;

[[nodiscard]] bool IsFromCache() const override { return true; }

private:
flag_manager::FlagPersistence* const cache_;
Context const context_;
Logger logger_;
};

} // namespace launchdarkly::client_side::data_sources
10 changes: 10 additions & 0 deletions libs/client-sdk/src/flag_manager/context_index.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,23 @@
}
}

std::optional<std::chrono::time_point<std::chrono::system_clock>>
ContextIndex::GetTimestamp(std::string const& id) const {
for (auto const& entry : index_) {
if (entry.id == id) {
return entry.timestamp;
}
}
return std::nullopt;
}

std::vector<std::string> ContextIndex::Prune(std::size_t maxContexts) {
if (index_.size() <= maxContexts) {
return {};
}

std::sort(index_.begin(), index_.end(),
[](IndexEntry const& a, IndexEntry const& b) {

Check warning on line 42 in libs/client-sdk/src/flag_manager/context_index.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

libs/client-sdk/src/flag_manager/context_index.cpp:42:57 [readability-identifier-length]

parameter name 'b' is too short, expected at least 3 characters

Check warning on line 42 in libs/client-sdk/src/flag_manager/context_index.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

libs/client-sdk/src/flag_manager/context_index.cpp:42:36 [readability-identifier-length]

parameter name 'a' is too short, expected at least 3 characters
return a.timestamp > b.timestamp;
});

Expand Down Expand Up @@ -60,7 +70,7 @@
auto& top = json_value.emplace_object();
auto arr = boost::json::array();

for (auto& entry : index.Entries()) {

Check warning on line 73 in libs/client-sdk/src/flag_manager/context_index.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

libs/client-sdk/src/flag_manager/context_index.cpp:73:10 [readability-qualified-auto]

'auto &entry' can be declared as 'const auto &entry'
auto obj = boost::json::object();
obj.emplace("id", entry.id);
obj.emplace("timestamp",
Expand All @@ -78,15 +88,15 @@

auto index = ContextIndex::Index();
if (json_value.is_object()) {
auto arr = json_value.as_object().find("index");

Check warning on line 91 in libs/client-sdk/src/flag_manager/context_index.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

libs/client-sdk/src/flag_manager/context_index.cpp:91:9 [readability-qualified-auto]

'auto arr' can be declared as 'const auto *arr'
if (arr != json_value.as_object().end() && arr->value().is_array()) {
for (auto& item : arr->value().as_array()) {

Check warning on line 93 in libs/client-sdk/src/flag_manager/context_index.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

libs/client-sdk/src/flag_manager/context_index.cpp:93:18 [readability-qualified-auto]

'auto &item' can be declared as 'const auto &item'
if (item.is_object()) {
auto& obj = item.as_object();

Check warning on line 95 in libs/client-sdk/src/flag_manager/context_index.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

libs/client-sdk/src/flag_manager/context_index.cpp:95:21 [readability-qualified-auto]

'auto &obj' can be declared as 'const auto &obj'
auto* id_iter = obj.find("id");

Check warning on line 96 in libs/client-sdk/src/flag_manager/context_index.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

libs/client-sdk/src/flag_manager/context_index.cpp:96:21 [readability-qualified-auto]

'auto *id_iter' can be declared as 'const auto *id_iter'
auto id = ValueAsOpt<std::string>(id_iter, obj.end());

auto* timestamp_iter = obj.find("timestamp");

Check warning on line 99 in libs/client-sdk/src/flag_manager/context_index.cpp

View workflow job for this annotation

GitHub Actions / cpp-linter

libs/client-sdk/src/flag_manager/context_index.cpp:99:21 [readability-qualified-auto]

'auto *timestamp_iter' can be declared as 'const auto *timestamp_iter'
auto timestamp =
ValueAsOpt<uint64_t>(timestamp_iter, obj.end());

Expand Down
8 changes: 8 additions & 0 deletions libs/client-sdk/src/flag_manager/context_index.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <chrono>
#include <mutex>
#include <optional>
#include <string>
#include <vector>

Expand All @@ -20,6 +21,8 @@ namespace launchdarkly::client_side::flag_manager {
* 1. a context identifier (hashed fully-qualified key) and
* 2. timestamp when it was last accessed, to support an LRU
* eviction pattern.
*
* Not thread-safe.
*/
class ContextIndex {
public:
Expand Down Expand Up @@ -52,6 +55,11 @@ class ContextIndex {

[[nodiscard]] Index const& Entries() const;

/** Returns the timestamp recorded for the id, or nullopt if absent. */
[[nodiscard]] std::optional<
std::chrono::time_point<std::chrono::system_clock>>
GetTimestamp(std::string const& id) const;

/**
* Prune the index returning a list of the removed context keys
*
Expand Down
4 changes: 4 additions & 0 deletions libs/client-sdk/src/flag_manager/flag_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ FlagStore const& FlagManager::Store() const {
return flag_store_;
}

FlagPersistence& FlagManager::Cache() {
return persistence_updater_;
}

void FlagManager::LoadCache(Context const& context) {
persistence_updater_.LoadCached(context);
}
Expand Down
8 changes: 8 additions & 0 deletions libs/client-sdk/src/flag_manager/flag_manager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@

namespace launchdarkly::client_side::flag_manager {

/**
* Owns the flag store and the update pipeline that feeds it.
*
* Thread-safe: each part it exposes is itself thread-safe.
*/
class FlagManager {
public:
FlagManager(std::string const& sdk_key,
Expand All @@ -18,6 +23,9 @@ class FlagManager {
IFlagNotifier& Notifier();
FlagStore const& Store() const;

/** Returns the local cache the SDK persists flag data to. */
FlagPersistence& Cache();

void LoadCache(Context const& context);

private:
Expand Down
73 changes: 57 additions & 16 deletions libs/client-sdk/src/flag_manager/flag_persistence.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <launchdarkly/encoding/sha_256.hpp>

#include <launchdarkly/detail/serialization/json_primitives.hpp>
#include <launchdarkly/serialization/json_context.hpp>
#include <launchdarkly/serialization/json_evaluation_result.hpp>
#include <launchdarkly/serialization/json_item_descriptor.hpp>

Expand Down Expand Up @@ -59,25 +60,30 @@ void FlagPersistence::Apply(Context const& context,
change_set.type != data_model::ChangeSetType::kNone;
sink_.Apply(context, std::move(change_set), from_cache);
if (from_cache) {
// Writing cached data back to the cache it came from would be a no-op.
// Writing cached data back to the cache would be a no-op, and it was
// never confirmed current by the service.
return;
}
// Both a payload and a confirmation that nothing changed mean the cache
// is up to date as of now.
RecordFreshness(context);
if (changed_data) {
StoreCache(PersistenceEncodeKey(context.CanonicalKey()));
}
}

void FlagPersistence::LoadCached(Context const& context) {
std::optional<std::unordered_map<std::string, ItemDescriptor>>
FlagPersistence::ReadCached(Context const& context) {
if (!persistence_ || !context.Valid()) {
return;
return std::nullopt;
}

std::lock_guard lock(persistence_mutex_);
auto data = persistence_->Read(
environment_namespace_, PersistenceEncodeKey(context.CanonicalKey()));

if (!data) {
return;
return std::nullopt;
}

boost::system::error_code error_code;
Expand All @@ -86,24 +92,61 @@ void FlagPersistence::LoadCached(Context const& context) {
LD_LOG(logger_, LogLevel::kError)
<< "Failed to parse flag data from persistence: "
<< error_code.message();
return;
return std::nullopt;
}

auto res = boost::json::value_to<tl::expected<
std::optional<std::unordered_map<std::string, ItemDescriptor>>,
JsonError>>(parsed);
if (!res) {
LD_LOG(logger_, LogLevel::kError)
<< "Failed to parse flag data from persistence: "
<< error_code.message();
return;
<< "Failed to parse flag data from persistence";
return std::nullopt;
}

// If the map was null or omitted, treat it like an empty data set.
auto map =
res.value().value_or(std::unordered_map<std::string, ItemDescriptor>{});
return res.value().value_or(
std::unordered_map<std::string, ItemDescriptor>{});
}

void FlagPersistence::LoadCached(Context const& context) {
if (auto data = ReadCached(context)) {
sink_.Init(context, std::move(*data));
}
}

// Identifies a context by everything it carries, not just its key. Changing
// an attribute can change how flags evaluate.
static std::string FreshnessId(Context const& context) {
return PersistenceEncodeKey(
boost::json::serialize(boost::json::value_from(context)));
}

sink_.Init(context, std::move(map));
void FlagPersistence::RecordFreshness(Context const& context) {
if (!persistence_ || !context.Valid()) {
return;
}

std::lock_guard lock(persistence_mutex_);
auto index = ReadIndexAt(freshness_key_);
index.Notice(FreshnessId(context), time_stamper_());
index.Prune(max_cached_contexts_);
persistence_->Set(environment_namespace_, freshness_key_,
boost::json::serialize(boost::json::value_from(index)));
}

std::optional<std::chrono::time_point<std::chrono::system_clock>>
FlagPersistence::ReadFreshness(Context const& context) {
if (!persistence_ || !context.Valid()) {
return std::nullopt;
}

std::lock_guard lock(persistence_mutex_);
// Freshness must not outlive the flag data it describes.
if (!ReadCached(context)) {
return std::nullopt;
}
return ReadIndexAt(freshness_key_).GetTimestamp(FreshnessId(context));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Freshness outlives overwritten flag data

Medium Severity

ReadFreshness treats any cached flags for the context's CanonicalKey as proof the stored timestamp still describes that data. Flag data is keyed only by that key, while freshness is keyed by a hash of the full context, so a later payload for the same key with different attributes overwrites the flags and leaves the earlier timestamp in place. ReadFreshness then reports the old time for the new data, which can make a poll wait on evaluations that were never confirmed for this attribute set.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit fc7d735. Configure here.

}

void FlagPersistence::StoreCache(std::string const& context_id) {
Expand All @@ -112,7 +155,7 @@ void FlagPersistence::StoreCache(std::string const& context_id) {
}

std::lock_guard lock(persistence_mutex_);
auto index = GetIndex();
auto index = ReadIndexAt(index_key_);
index.Notice(context_id, time_stamper_());
auto pruned = index.Prune(max_cached_contexts_);
for (auto& id : pruned) {
Expand All @@ -127,11 +170,9 @@ void FlagPersistence::StoreCache(std::string const& context_id) {
boost::json::serialize(v));
}

ContextIndex FlagPersistence::GetIndex() {
ContextIndex FlagPersistence::ReadIndexAt(std::string const& key) {
if (persistence_) {
std::lock_guard lock(persistence_mutex_);
auto index_data =
persistence_->Read(environment_namespace_, index_key_);
auto index_data = persistence_->Read(environment_namespace_, key);

if (index_data) {
boost::system::error_code error_code;
Expand Down
Loading
Loading