From b5474148ef7638a5324480deb5f785ac2058962a Mon Sep 17 00:00:00 2001 From: Bee Klimt Date: Wed, 2 Sep 2026 03:25:50 +0000 Subject: [PATCH 1/4] feat: Initialize client FDv2 from the local cache and track freshness --- libs/client-sdk/src/CMakeLists.txt | 2 + .../data_sources/data_source_update_sink.hpp | 3 +- .../data_sources/fdv2/cache_initializer.cpp | 61 ++++++++ .../data_sources/fdv2/cache_initializer.hpp | 80 ++++++++++ .../src/flag_manager/context_index.cpp | 10 ++ .../src/flag_manager/context_index.hpp | 13 ++ .../src/flag_manager/flag_manager.cpp | 4 + .../src/flag_manager/flag_manager.hpp | 13 ++ .../src/flag_manager/flag_persistence.cpp | 69 ++++++-- .../src/flag_manager/flag_persistence.hpp | 48 +++++- .../tests/fdv2_cache_initializer_test.cpp | 147 ++++++++++++++++++ .../tests/flag_persistence_test.cpp | 129 ++++++++++++++- 12 files changed, 555 insertions(+), 24 deletions(-) create mode 100644 libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp create mode 100644 libs/client-sdk/src/data_sources/fdv2/cache_initializer.hpp create mode 100644 libs/client-sdk/tests/fdv2_cache_initializer_test.cpp diff --git a/libs/client-sdk/src/CMakeLists.txt b/libs/client-sdk/src/CMakeLists.txt index 1ff2452b4..625775ca5 100644 --- a/libs/client-sdk/src/CMakeLists.txt +++ b/libs/client-sdk/src/CMakeLists.txt @@ -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 @@ -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 diff --git a/libs/client-sdk/src/data_sources/data_source_update_sink.hpp b/libs/client-sdk/src/data_sources/data_source_update_sink.hpp index 124e7bbc3..73cbdb236 100644 --- a/libs/client-sdk/src/data_sources/data_source_update_sink.hpp +++ b/libs/client-sdk/src/data_sources/data_source_update_sink.hpp @@ -50,7 +50,8 @@ 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. Cached data is not written back to the cache, and does not + * count as the service confirming the cache is current. */ virtual void Apply(Context const& context, FlagChangeSet change_set, diff --git a/libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp b/libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp new file mode 100644 index 000000000..59fd87900 --- /dev/null +++ b/libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp @@ -0,0 +1,61 @@ +#include "cache_initializer.hpp" + +#include + +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) + : cache_(cache), context_(std::move(context)), logger_(logger) {} + +async::Future 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 and lets initialization + // continue, which is what a "none" intent means. + 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) + : cache_(cache), context_(std::move(context)), logger_(logger) {} + +std::unique_ptr FDv2CacheInitializerFactory::Build() { + return std::make_unique(cache_, context_, logger_); +} + +} // namespace launchdarkly::client_side::data_sources diff --git a/libs/client-sdk/src/data_sources/fdv2/cache_initializer.hpp b/libs/client-sdk/src/data_sources/fdv2/cache_initializer.hpp new file mode 100644 index 000000000..4adf976d7 --- /dev/null +++ b/libs/client-sdk/src/data_sources/fdv2/cache_initializer.hpp @@ -0,0 +1,80 @@ +#pragma once + +#include "ifdv2_initializer.hpp" +#include "ifdv2_initializer_factory.hpp" + +#include "../../flag_manager/flag_persistence.hpp" + +#include +#include +#include + +#include + +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. + * + * The cache is read on the calling thread. The read is fast enough that + * dispatching it to the executor would cost more than it saves. + * + * Cached data never carries a selector. A selector names a state the service + * can compute changes against, and the SDK does not verify that persisted + * data is intact. Asking for a delta against data that may not be what the + * service thinks it is would corrupt the store silently. Initialization + * therefore continues past the cache to a network source, which supplies both + * data and a selector. + * + * Run() reads the cache on the calling thread and returns a future that is + * already resolved. Close() may be called from any thread and has nothing to + * cancel. + */ +class FDv2CacheInitializer final : public IFDv2Initializer { + public: + /** + * @param cache The 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 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. The cache pointer and + * context it hands to each initializer are fixed at construction. + */ +class FDv2CacheInitializerFactory final : public IFDv2InitializerFactory { + public: + FDv2CacheInitializerFactory(flag_manager::FlagPersistence* cache, + Context context, + Logger const& logger); + + std::unique_ptr 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 diff --git a/libs/client-sdk/src/flag_manager/context_index.cpp b/libs/client-sdk/src/flag_manager/context_index.cpp index a8e0fddb9..6d4996323 100644 --- a/libs/client-sdk/src/flag_manager/context_index.cpp +++ b/libs/client-sdk/src/flag_manager/context_index.cpp @@ -23,6 +23,16 @@ void ContextIndex::Notice( } } +std::optional> +ContextIndex::TimestampFor(std::string const& id) const { + for (auto const& entry : index_) { + if (entry.id == id) { + return entry.timestamp; + } + } + return std::nullopt; +} + std::vector ContextIndex::Prune(std::size_t maxContexts) { if (index_.size() <= maxContexts) { return {}; diff --git a/libs/client-sdk/src/flag_manager/context_index.hpp b/libs/client-sdk/src/flag_manager/context_index.hpp index bf96bc235..946c1d3f3 100644 --- a/libs/client-sdk/src/flag_manager/context_index.hpp +++ b/libs/client-sdk/src/flag_manager/context_index.hpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -21,6 +22,10 @@ namespace launchdarkly::client_side::flag_manager { * 2. timestamp when it was last accessed, to support an LRU * eviction pattern. */ +/** + * Not thread-safe. Instances are short-lived values read out of persistence, + * modified, and written back by a caller holding its own lock. + */ class ContextIndex { public: /** @@ -52,6 +57,14 @@ class ContextIndex { [[nodiscard]] Index const& Entries() const; + /** + * The timestamp recorded for the given id, or nullopt if the id is not in + * the index. + */ + [[nodiscard]] std::optional< + std::chrono::time_point> + TimestampFor(std::string const& id) const; + /** * Prune the index returning a list of the removed context keys * diff --git a/libs/client-sdk/src/flag_manager/flag_manager.cpp b/libs/client-sdk/src/flag_manager/flag_manager.cpp index 3ac3227bc..bdeba00b8 100644 --- a/libs/client-sdk/src/flag_manager/flag_manager.cpp +++ b/libs/client-sdk/src/flag_manager/flag_manager.cpp @@ -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); } diff --git a/libs/client-sdk/src/flag_manager/flag_manager.hpp b/libs/client-sdk/src/flag_manager/flag_manager.hpp index 1c692ce7b..854264d95 100644 --- a/libs/client-sdk/src/flag_manager/flag_manager.hpp +++ b/libs/client-sdk/src/flag_manager/flag_manager.hpp @@ -8,6 +8,13 @@ namespace launchdarkly::client_side::flag_manager { +/** + * Owns the flag store and the update pipeline that feeds it. + * + * Thread-safe to the extent its parts are: the store, the updater, and the + * persistence layer each carry their own lock. The accessors hand out + * references to those parts and take no lock of their own. + */ class FlagManager { public: FlagManager(std::string const& sdk_key, @@ -18,6 +25,12 @@ class FlagManager { IFlagNotifier& Notifier(); FlagStore const& Store() const; + /** + * The local cache the SDK persists flag data to, and which the FDv2 cache + * initializer reads from. + */ + FlagPersistence& Cache(); + void LoadCache(Context const& context); private: diff --git a/libs/client-sdk/src/flag_manager/flag_persistence.cpp b/libs/client-sdk/src/flag_manager/flag_persistence.cpp index 57ca7a102..14bd714f3 100644 --- a/libs/client-sdk/src/flag_manager/flag_persistence.cpp +++ b/libs/client-sdk/src/flag_manager/flag_persistence.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -59,17 +60,22 @@ 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> +FlagPersistence::ReadCached(Context const& context) { if (!persistence_ || !context.Valid()) { - return; + return std::nullopt; } std::lock_guard lock(persistence_mutex_); @@ -77,7 +83,7 @@ void FlagPersistence::LoadCached(Context const& context) { environment_namespace_, PersistenceEncodeKey(context.CanonicalKey())); if (!data) { - return; + return std::nullopt; } boost::system::error_code error_code; @@ -86,7 +92,7 @@ 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>(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{}); + return res.value().value_or( + std::unordered_map{}); +} + +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> +FlagPersistence::FreshnessFor(Context const& context) { + if (!persistence_ || !context.Valid()) { + return std::nullopt; + } + + std::lock_guard lock(persistence_mutex_); + return ReadIndexAt(freshness_key_).TimestampFor(FreshnessId(context)); } void FlagPersistence::StoreCache(std::string const& context_id) { @@ -112,7 +151,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) { @@ -127,11 +166,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; diff --git a/libs/client-sdk/src/flag_manager/flag_persistence.hpp b/libs/client-sdk/src/flag_manager/flag_persistence.hpp index b74dfb6d0..1324cdbf6 100644 --- a/libs/client-sdk/src/flag_manager/flag_persistence.hpp +++ b/libs/client-sdk/src/flag_manager/flag_persistence.hpp @@ -3,6 +3,8 @@ #include #include #include +#include +#include #include "../data_sources/data_source_update_sink.hpp" #include "context_index.hpp" @@ -16,6 +18,15 @@ namespace launchdarkly::client_side::flag_manager { std::string PersistenceEncodeKey(std::string const& input); +/** + * Mirrors data source updates into the persistent store on their way to the + * next sink, and reads them back when a context is loaded. + * + * Thread-safe. The methods that touch the store take persistence_mutex_, + * which makes each stored index read-modify-write atomic. It does not span + * the call to the downstream sink, so an update reaches the store and the + * cache at slightly different times. + */ class FlagPersistence : public IDataSourceUpdateSink { public: using TimeStampsource = @@ -45,23 +56,54 @@ class FlagPersistence : public IDataSourceUpdateSink { void LoadCached(Context const& context); + /** + * The flag data stored for the given context, or nullopt when nothing is + * stored for it and when persistence is disabled. An empty map means an + * environment with no flags was stored, which is distinct from nothing + * being stored at all. + */ + [[nodiscard]] std::optional> + ReadCached(Context const& context); + + /** + * When the service last confirmed the flag data for this context was + * current, or nullopt if it never has. + * + * Keyed by the context's whole set of attributes rather than its key, + * because changing an attribute can change how flags evaluate, and the + * answer for the old attributes says nothing about the new ones. + */ + [[nodiscard]] std::optional< + std::chrono::time_point> + FreshnessFor(Context const& context); + private: inline static std::string global_namespace_ = "LaunchDarkly"; inline static std::string index_key_ = "ContextIndex"; + inline static std::string freshness_key_ = "ContextFreshness"; Logger& logger_; std::size_t max_cached_contexts_; IDataSourceUpdateSink& sink_; - std::shared_ptr persistence_; - mutable std::recursive_mutex persistence_mutex_; FlagStore& flag_store_; + // Serializes the read-modify-write of the stored index and flag data, so + // that two contexts being cached at once cannot lose an index entry. + mutable std::recursive_mutex persistence_mutex_; + std::shared_ptr persistence_; + std::string environment_namespace_; TimeStampsource time_stamper_; - ContextIndex GetIndex(); void StoreCache(std::string const& context_id); + + // Records that the service confirmed this context's data is current, as + // of now. + void RecordFreshness(Context const& context); + + // Must be called with persistence_mutex_ held. + ContextIndex ReadIndexAt(std::string const& key); }; } // namespace launchdarkly::client_side::flag_manager diff --git a/libs/client-sdk/tests/fdv2_cache_initializer_test.cpp b/libs/client-sdk/tests/fdv2_cache_initializer_test.cpp new file mode 100644 index 000000000..15baa00e9 --- /dev/null +++ b/libs/client-sdk/tests/fdv2_cache_initializer_test.cpp @@ -0,0 +1,147 @@ +#include + +#include +#include + +#include +#include + +#include +#include +#include +#include + +using launchdarkly::ContextBuilder; +using launchdarkly::Value; +using launchdarkly::client_side::flag_manager::FlagManager; +using launchdarkly::client_side::flag_manager::PersistenceEncodeKey; +using launchdarkly::data_model::ChangeSetType; + +using namespace launchdarkly::client_side::data_sources; + +namespace { + +class TestPersistence : public IPersistence { + public: + using StoreType = + std::map>>; + + explicit TestPersistence(StoreType store) : store_(std::move(store)) {} + + void Set(std::string storageNamespace, + std::string key, + std::string data) noexcept override { + store_[storageNamespace][key] = data; + } + + void Remove(std::string storageNamespace, + std::string key) noexcept override { + store_[storageNamespace].erase(key); + } + + std::optional Read(std::string storageNamespace, + std::string key) noexcept override { + return store_[storageNamespace][key]; + } + + StoreType store_; +}; + +// The environment namespace and context id the client derives for SDK key +// "the-key" and context user:user-key. +char const* const kEnvironment = + "LaunchDarkly_rUTcjlHPv6Vegd27YmtGYkEGkEUGaEbn5M0JYTFQUpA="; +char const* const kContextId = "CEXjZY7cHJG_ydFy7q4-YEFwVrG3_pkJwA4FAjrbfx0="; + +} // namespace + +TEST(FDv2CacheInitializerTest, CacheHitProducesAFullDataSet) { + auto context = ContextBuilder().Kind("user", "user-key").Build(); + auto logger = launchdarkly::logging::NullLogger(); + auto persistence = + std::make_shared(TestPersistence::StoreType{ + {kEnvironment, + {{kContextId, R"({"flagA":{"version":1,"value":"test"}})"}}}}); + FlagManager flag_manager("the-key", logger, 5, persistence); + + FDv2CacheInitializer initializer(&flag_manager.Cache(), context, logger); + auto future = initializer.Run(); + ASSERT_TRUE(future.IsFinished()); + auto result = future.GetResult(); + + auto* change_set = std::get_if(&result->value); + ASSERT_NE(nullptr, change_set); + EXPECT_EQ(ChangeSetType::kFull, change_set->change_set.type); + ASSERT_EQ(1u, change_set->change_set.data.size()); + EXPECT_EQ("flagA", change_set->change_set.data[0].key); + EXPECT_EQ(Value("test"), + change_set->change_set.data[0].item.item->Detail().Value()); +} + +// Asking the service for a delta against unverified cached data could corrupt +// the store silently, so the cache never supplies a basis. +TEST(FDv2CacheInitializerTest, CachedDataCarriesNoSelector) { + auto context = ContextBuilder().Kind("user", "user-key").Build(); + auto logger = launchdarkly::logging::NullLogger(); + auto persistence = + std::make_shared(TestPersistence::StoreType{ + {kEnvironment, + {{kContextId, R"({"flagA":{"version":1,"value":"test"}})"}}}}); + FlagManager flag_manager("the-key", logger, 5, persistence); + + FDv2CacheInitializer initializer(&flag_manager.Cache(), context, logger); + auto result = initializer.Run().GetResult(); + + auto* change_set = std::get_if(&result->value); + ASSERT_NE(nullptr, change_set); + EXPECT_FALSE(change_set->change_set.selector.value.has_value()); +} + +// A miss leaves the data set unchanged and lets the chain proceed, rather than +// reporting an error. +TEST(FDv2CacheInitializerTest, CacheMissProducesANoneIntent) { + auto context = ContextBuilder().Kind("user", "unknown").Build(); + auto logger = launchdarkly::logging::NullLogger(); + auto persistence = + std::make_shared(TestPersistence::StoreType()); + FlagManager flag_manager("the-key", logger, 5, persistence); + + FDv2CacheInitializer initializer(&flag_manager.Cache(), context, logger); + auto future = initializer.Run(); + ASSERT_TRUE(future.IsFinished()); + auto result = future.GetResult(); + + auto* change_set = std::get_if(&result->value); + ASSERT_NE(nullptr, change_set); + EXPECT_EQ(ChangeSetType::kNone, change_set->change_set.type); + EXPECT_TRUE(change_set->change_set.data.empty()); +} + +TEST(FDv2CacheInitializerTest, NoPersistenceConfiguredProducesANoneIntent) { + auto context = ContextBuilder().Kind("user", "user-key").Build(); + auto logger = launchdarkly::logging::NullLogger(); + FlagManager flag_manager("the-key", logger, 5, nullptr); + + FDv2CacheInitializer initializer(&flag_manager.Cache(), context, logger); + auto future = initializer.Run(); + ASSERT_TRUE(future.IsFinished()); + auto result = future.GetResult(); + + auto* change_set = std::get_if(&result->value); + ASSERT_NE(nullptr, change_set); + EXPECT_EQ(ChangeSetType::kNone, change_set->change_set.type); +} + +// The orchestrator needs to tell cache initializers apart from network ones, +// so that a miss with nothing else configured still starts the SDK. +TEST(FDv2CacheInitializerTest, FactoryIdentifiesItselfAsReadingTheCache) { + auto context = ContextBuilder().Kind("user", "user-key").Build(); + auto logger = launchdarkly::logging::NullLogger(); + FlagManager flag_manager("the-key", logger, 5, nullptr); + + FDv2CacheInitializerFactory factory(&flag_manager.Cache(), context, logger); + + EXPECT_TRUE(factory.IsFromCache()); + EXPECT_NE(nullptr, factory.Build()); +} diff --git a/libs/client-sdk/tests/flag_persistence_test.cpp b/libs/client-sdk/tests/flag_persistence_test.cpp index 25cf29bd2..79d1284b7 100644 --- a/libs/client-sdk/tests/flag_persistence_test.cpp +++ b/libs/client-sdk/tests/flag_persistence_test.cpp @@ -211,7 +211,7 @@ TEST(FlagPersistenceTests, StoresCacheOnApply) { ["CEXjZY7cHJG_ydFy7q4-YEFwVrG3_pkJwA4FAjrbfx0="]); } -TEST(FlagPersistenceTests, ApplyOfNoneChangeSetDoesNotWriteTheCache) { +TEST(FlagPersistenceTests, ApplyOfNoneChangeSetDoesNotWriteTheFlagData) { auto context = ContextBuilder().Kind("user", "user-key").Build(); auto store = FlagStore(); auto updater = FlagUpdater(store); @@ -227,11 +227,130 @@ TEST(FlagPersistenceTests, ApplyOfNoneChangeSetDoesNotWriteTheCache) { FlagChangeSet{ChangeSetType::kNone, {}, Selector{}}, /* from_cache= */ false); - // Nothing is written to the cache. - EXPECT_TRUE(persistence->store_.empty()); + // The context's flag data is not written. + auto& space = persistence->store_.begin()->second; + EXPECT_EQ(0, space.count(PersistenceEncodeKey("user:user-key"))); +} + +TEST(FlagPersistenceTests, RecordsFreshnessOnAPayload) { + auto context = ContextBuilder().Kind("user", "user-key").Build(); + auto store = FlagStore(); + auto updater = FlagUpdater(store); + auto persistence = + std::make_shared(TestPersistence::StoreType()); + auto logger = launchdarkly::logging::NullLogger(); + + FlagPersistence flag_persistence( + "the-key", updater, store, persistence, logger, 5, []() { + return std::chrono::system_clock::time_point{ + std::chrono::milliseconds{500}}; + }); + + EXPECT_FALSE(flag_persistence.FreshnessFor(context).has_value()); + + flag_persistence.Apply( + context, + FlagChangeSet{ + ChangeSetType::kFull, + {FlagChange{"flagA", + ItemDescriptor{EvaluationResult{ + 1, std::nullopt, false, false, std::nullopt, + EvaluationDetailInternal{ + Value("test"), std::nullopt, std::nullopt}}}}}, + Selector{}}, + /* from_cache= */ false); + + EXPECT_EQ( + std::chrono::system_clock::time_point{std::chrono::milliseconds{500}}, + flag_persistence.FreshnessFor(context)); +} + +// A "none" intent is the service confirming the SDK's data is current, which +// is exactly as good as receiving it again. +TEST(FlagPersistenceTests, RecordsFreshnessOnANoneChangeSet) { + auto context = ContextBuilder().Kind("user", "user-key").Build(); + auto store = FlagStore(); + auto updater = FlagUpdater(store); + auto persistence = + std::make_shared(TestPersistence::StoreType()); + auto logger = launchdarkly::logging::NullLogger(); + + FlagPersistence flag_persistence( + "the-key", updater, store, persistence, logger, 5, []() { + return std::chrono::system_clock::time_point{ + std::chrono::milliseconds{700}}; + }); + + flag_persistence.Apply(context, + FlagChangeSet{ChangeSetType::kNone, {}, Selector{}}, + /* from_cache= */ false); + + EXPECT_EQ( + std::chrono::system_clock::time_point{std::chrono::milliseconds{700}}, + flag_persistence.FreshnessFor(context)); +} + +// The freshness record is keyed by the whole context, because changing an +// attribute can change how flags evaluate. +TEST(FlagPersistenceTests, FreshnessIsPerContextAttributeSet) { + auto store = FlagStore(); + auto updater = FlagUpdater(store); + auto persistence = + std::make_shared(TestPersistence::StoreType()); + auto logger = launchdarkly::logging::NullLogger(); + + FlagPersistence flag_persistence( + "the-key", updater, store, persistence, logger, 5, []() { + return std::chrono::system_clock::time_point{ + std::chrono::milliseconds{500}}; + }); + + auto plain = ContextBuilder().Kind("user", "user-key").Build(); + auto with_attribute = + ContextBuilder().Kind("user", "user-key").Set("country", "US").Build(); + + flag_persistence.Apply(plain, + FlagChangeSet{ChangeSetType::kNone, {}, Selector{}}, + /* from_cache= */ false); + + EXPECT_TRUE(flag_persistence.FreshnessFor(plain).has_value()); + EXPECT_FALSE(flag_persistence.FreshnessFor(with_attribute).has_value()); +} + +// A stored context that has aged out of the cache should not keep a freshness +// record alive either. +TEST(FlagPersistenceTests, PrunesFreshnessBeyondMaxContexts) { + auto store = FlagStore(); + auto updater = FlagUpdater(store); + auto persistence = + std::make_shared(TestPersistence::StoreType()); + auto logger = launchdarkly::logging::NullLogger(); + + uint64_t now = 0; + FlagPersistence flag_persistence( + "the-key", updater, store, persistence, logger, 2, [&now]() { + return std::chrono::system_clock::time_point{ + std::chrono::milliseconds{now}}; + }); + + auto first = ContextBuilder().Kind("user", "first").Build(); + for (auto const& key : {"first", "second", "third"}) { + flag_persistence.Apply( + ContextBuilder().Kind("user", key).Build(), + FlagChangeSet{ChangeSetType::kNone, {}, Selector{}}, + /* from_cache= */ false); + now++; + } + + EXPECT_FALSE(flag_persistence.FreshnessFor(first).has_value()); + EXPECT_TRUE( + flag_persistence + .FreshnessFor(ContextBuilder().Kind("user", "third").Build()) + .has_value()); } -// Writing data straight back to the cache it was read from would be a no-op. +// Data read out of the cache was never confirmed current by the service, so +// writing it back or counting it as fresh would be misleading. TEST(FlagPersistenceTests, ApplyFromCacheDoesNotWriteTheCache) { auto context = ContextBuilder().Kind("user", "user-key").Build(); auto store = FlagStore(); @@ -258,6 +377,8 @@ TEST(FlagPersistenceTests, ApplyFromCacheDoesNotWriteTheCache) { // Nothing is written back. EXPECT_TRUE(persistence->store_.empty()); + // Nor was it confirmed current by the service, so it is not freshness. + EXPECT_FALSE(flag_persistence.FreshnessFor(context).has_value()); // The data is still applied to the store, so evaluation can use it. ASSERT_TRUE(store.Get("flagA")); } From 191860ab6b5eb82b2d2ec6cc0e159df36f082f69 Mon Sep 17 00:00:00 2001 From: Bee Klimt Date: Mon, 14 Sep 2026 15:08:02 -0700 Subject: [PATCH 2/4] refactor: Rename the FDv2 freshness accessors to ReadFreshness and GetTimestamp --- .../src/flag_manager/context_index.cpp | 2 +- .../src/flag_manager/context_index.hpp | 2 +- .../src/flag_manager/flag_persistence.cpp | 4 ++-- .../src/flag_manager/flag_persistence.hpp | 2 +- libs/client-sdk/tests/flag_persistence_test.cpp | 16 ++++++++-------- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/libs/client-sdk/src/flag_manager/context_index.cpp b/libs/client-sdk/src/flag_manager/context_index.cpp index 6d4996323..387d30034 100644 --- a/libs/client-sdk/src/flag_manager/context_index.cpp +++ b/libs/client-sdk/src/flag_manager/context_index.cpp @@ -24,7 +24,7 @@ void ContextIndex::Notice( } std::optional> -ContextIndex::TimestampFor(std::string const& id) const { +ContextIndex::GetTimestamp(std::string const& id) const { for (auto const& entry : index_) { if (entry.id == id) { return entry.timestamp; diff --git a/libs/client-sdk/src/flag_manager/context_index.hpp b/libs/client-sdk/src/flag_manager/context_index.hpp index 946c1d3f3..f418fb5b5 100644 --- a/libs/client-sdk/src/flag_manager/context_index.hpp +++ b/libs/client-sdk/src/flag_manager/context_index.hpp @@ -63,7 +63,7 @@ class ContextIndex { */ [[nodiscard]] std::optional< std::chrono::time_point> - TimestampFor(std::string const& id) const; + GetTimestamp(std::string const& id) const; /** * Prune the index returning a list of the removed context keys diff --git a/libs/client-sdk/src/flag_manager/flag_persistence.cpp b/libs/client-sdk/src/flag_manager/flag_persistence.cpp index 14bd714f3..620c4059c 100644 --- a/libs/client-sdk/src/flag_manager/flag_persistence.cpp +++ b/libs/client-sdk/src/flag_manager/flag_persistence.cpp @@ -136,13 +136,13 @@ void FlagPersistence::RecordFreshness(Context const& context) { } std::optional> -FlagPersistence::FreshnessFor(Context const& context) { +FlagPersistence::ReadFreshness(Context const& context) { if (!persistence_ || !context.Valid()) { return std::nullopt; } std::lock_guard lock(persistence_mutex_); - return ReadIndexAt(freshness_key_).TimestampFor(FreshnessId(context)); + return ReadIndexAt(freshness_key_).GetTimestamp(FreshnessId(context)); } void FlagPersistence::StoreCache(std::string const& context_id) { diff --git a/libs/client-sdk/src/flag_manager/flag_persistence.hpp b/libs/client-sdk/src/flag_manager/flag_persistence.hpp index 1324cdbf6..9672a3b3a 100644 --- a/libs/client-sdk/src/flag_manager/flag_persistence.hpp +++ b/libs/client-sdk/src/flag_manager/flag_persistence.hpp @@ -75,7 +75,7 @@ class FlagPersistence : public IDataSourceUpdateSink { */ [[nodiscard]] std::optional< std::chrono::time_point> - FreshnessFor(Context const& context); + ReadFreshness(Context const& context); private: inline static std::string global_namespace_ = "LaunchDarkly"; diff --git a/libs/client-sdk/tests/flag_persistence_test.cpp b/libs/client-sdk/tests/flag_persistence_test.cpp index 79d1284b7..0139dedf0 100644 --- a/libs/client-sdk/tests/flag_persistence_test.cpp +++ b/libs/client-sdk/tests/flag_persistence_test.cpp @@ -246,7 +246,7 @@ TEST(FlagPersistenceTests, RecordsFreshnessOnAPayload) { std::chrono::milliseconds{500}}; }); - EXPECT_FALSE(flag_persistence.FreshnessFor(context).has_value()); + EXPECT_FALSE(flag_persistence.ReadFreshness(context).has_value()); flag_persistence.Apply( context, @@ -262,7 +262,7 @@ TEST(FlagPersistenceTests, RecordsFreshnessOnAPayload) { EXPECT_EQ( std::chrono::system_clock::time_point{std::chrono::milliseconds{500}}, - flag_persistence.FreshnessFor(context)); + flag_persistence.ReadFreshness(context)); } // A "none" intent is the service confirming the SDK's data is current, which @@ -287,7 +287,7 @@ TEST(FlagPersistenceTests, RecordsFreshnessOnANoneChangeSet) { EXPECT_EQ( std::chrono::system_clock::time_point{std::chrono::milliseconds{700}}, - flag_persistence.FreshnessFor(context)); + flag_persistence.ReadFreshness(context)); } // The freshness record is keyed by the whole context, because changing an @@ -313,8 +313,8 @@ TEST(FlagPersistenceTests, FreshnessIsPerContextAttributeSet) { FlagChangeSet{ChangeSetType::kNone, {}, Selector{}}, /* from_cache= */ false); - EXPECT_TRUE(flag_persistence.FreshnessFor(plain).has_value()); - EXPECT_FALSE(flag_persistence.FreshnessFor(with_attribute).has_value()); + EXPECT_TRUE(flag_persistence.ReadFreshness(plain).has_value()); + EXPECT_FALSE(flag_persistence.ReadFreshness(with_attribute).has_value()); } // A stored context that has aged out of the cache should not keep a freshness @@ -342,10 +342,10 @@ TEST(FlagPersistenceTests, PrunesFreshnessBeyondMaxContexts) { now++; } - EXPECT_FALSE(flag_persistence.FreshnessFor(first).has_value()); + EXPECT_FALSE(flag_persistence.ReadFreshness(first).has_value()); EXPECT_TRUE( flag_persistence - .FreshnessFor(ContextBuilder().Kind("user", "third").Build()) + .ReadFreshness(ContextBuilder().Kind("user", "third").Build()) .has_value()); } @@ -378,7 +378,7 @@ TEST(FlagPersistenceTests, ApplyFromCacheDoesNotWriteTheCache) { // Nothing is written back. EXPECT_TRUE(persistence->store_.empty()); // Nor was it confirmed current by the service, so it is not freshness. - EXPECT_FALSE(flag_persistence.FreshnessFor(context).has_value()); + EXPECT_FALSE(flag_persistence.ReadFreshness(context).has_value()); // The data is still applied to the store, so evaluation can use it. ASSERT_TRUE(store.Get("flagA")); } From 95183e7fad793836414fd8209aaeb61d34110e0f Mon Sep 17 00:00:00 2001 From: Bee Klimt Date: Thu, 24 Sep 2026 18:45:42 -0700 Subject: [PATCH 3/4] docs: Tighten comments in the FDv2 cache initializer and its tests --- .../data_sources/data_source_update_sink.hpp | 3 +-- .../data_sources/fdv2/cache_initializer.cpp | 3 +-- .../data_sources/fdv2/cache_initializer.hpp | 20 ++------------ .../src/flag_manager/context_index.hpp | 11 +++----- .../src/flag_manager/flag_manager.hpp | 9 ++----- .../src/flag_manager/flag_persistence.hpp | 26 ++++++++----------- .../tests/fdv2_cache_initializer_test.cpp | 21 +++------------ .../tests/flag_persistence_test.cpp | 3 ++- 8 files changed, 26 insertions(+), 70 deletions(-) diff --git a/libs/client-sdk/src/data_sources/data_source_update_sink.hpp b/libs/client-sdk/src/data_sources/data_source_update_sink.hpp index 73cbdb236..e6b9ec6fc 100644 --- a/libs/client-sdk/src/data_sources/data_source_update_sink.hpp +++ b/libs/client-sdk/src/data_sources/data_source_update_sink.hpp @@ -50,8 +50,7 @@ class IDataSourceUpdateSink { * instead. * * @param from_cache Whether the changeset was loaded from the local - * cache. Cached data is not written back to the cache, and does not - * count as the service confirming the cache is current. + * cache. Such data is not written back, nor treated as current. */ virtual void Apply(Context const& context, FlagChangeSet change_set, diff --git a/libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp b/libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp index 59fd87900..3adea1c03 100644 --- a/libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp +++ b/libs/client-sdk/src/data_sources/fdv2/cache_initializer.cpp @@ -16,8 +16,7 @@ async::Future FDv2CacheInitializer::Run() { if (!data) { LD_LOG(logger_, LogLevel::kDebug) << kIdentity << ": no cached data for this context"; - // A miss leaves the data set unchanged and lets initialization - // continue, which is what a "none" intent means. + // A miss leaves the data set unchanged so initialization can continue. return async::MakeFuture(FDv2SourceResult{FDv2SourceResult::ChangeSet{ FlagChangeSet{data_model::ChangeSetType::kNone, {}, diff --git a/libs/client-sdk/src/data_sources/fdv2/cache_initializer.hpp b/libs/client-sdk/src/data_sources/fdv2/cache_initializer.hpp index 4adf976d7..a86fd4ea5 100644 --- a/libs/client-sdk/src/data_sources/fdv2/cache_initializer.hpp +++ b/libs/client-sdk/src/data_sources/fdv2/cache_initializer.hpp @@ -16,26 +16,11 @@ 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. - * - * The cache is read on the calling thread. The read is fast enough that - * dispatching it to the executor would cost more than it saves. - * - * Cached data never carries a selector. A selector names a state the service - * can compute changes against, and the SDK does not verify that persisted - * data is intact. Asking for a delta against data that may not be what the - * service thinks it is would corrupt the store silently. Initialization - * therefore continues past the cache to a network source, which supplies both - * data and a selector. - * - * Run() reads the cache on the calling thread and returns a future that is - * already resolved. Close() may be called from any thread and has nothing to - * cancel. */ class FDv2CacheInitializer final : public IFDv2Initializer { public: /** - * @param cache The local cache to read. Non-owning. Must outlive this - * object. + * @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. */ @@ -58,8 +43,7 @@ class FDv2CacheInitializer final : public IFDv2Initializer { /** * Builds fresh FDv2CacheInitializer instances on demand. * - * Thread-safe: Build() may be called from any thread. The cache pointer and - * context it hands to each initializer are fixed at construction. + * Thread-safe: Build() may be called from any thread. */ class FDv2CacheInitializerFactory final : public IFDv2InitializerFactory { public: diff --git a/libs/client-sdk/src/flag_manager/context_index.hpp b/libs/client-sdk/src/flag_manager/context_index.hpp index f418fb5b5..5d1aef1ad 100644 --- a/libs/client-sdk/src/flag_manager/context_index.hpp +++ b/libs/client-sdk/src/flag_manager/context_index.hpp @@ -21,10 +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. Instances are short-lived values read out of persistence, - * modified, and written back by a caller holding its own lock. + * + * Not thread-safe. */ class ContextIndex { public: @@ -57,10 +55,7 @@ class ContextIndex { [[nodiscard]] Index const& Entries() const; - /** - * The timestamp recorded for the given id, or nullopt if the id is not in - * the index. - */ + /** Returns the timestamp recorded for the id, or nullopt if absent. */ [[nodiscard]] std::optional< std::chrono::time_point> GetTimestamp(std::string const& id) const; diff --git a/libs/client-sdk/src/flag_manager/flag_manager.hpp b/libs/client-sdk/src/flag_manager/flag_manager.hpp index 854264d95..ad09bb55e 100644 --- a/libs/client-sdk/src/flag_manager/flag_manager.hpp +++ b/libs/client-sdk/src/flag_manager/flag_manager.hpp @@ -11,9 +11,7 @@ namespace launchdarkly::client_side::flag_manager { /** * Owns the flag store and the update pipeline that feeds it. * - * Thread-safe to the extent its parts are: the store, the updater, and the - * persistence layer each carry their own lock. The accessors hand out - * references to those parts and take no lock of their own. + * Thread-safe: each part it exposes is itself thread-safe. */ class FlagManager { public: @@ -25,10 +23,7 @@ class FlagManager { IFlagNotifier& Notifier(); FlagStore const& Store() const; - /** - * The local cache the SDK persists flag data to, and which the FDv2 cache - * initializer reads from. - */ + /** Returns the local cache the SDK persists flag data to. */ FlagPersistence& Cache(); void LoadCache(Context const& context); diff --git a/libs/client-sdk/src/flag_manager/flag_persistence.hpp b/libs/client-sdk/src/flag_manager/flag_persistence.hpp index 9672a3b3a..da60099c0 100644 --- a/libs/client-sdk/src/flag_manager/flag_persistence.hpp +++ b/libs/client-sdk/src/flag_manager/flag_persistence.hpp @@ -22,10 +22,7 @@ std::string PersistenceEncodeKey(std::string const& input); * Mirrors data source updates into the persistent store on their way to the * next sink, and reads them back when a context is loaded. * - * Thread-safe. The methods that touch the store take persistence_mutex_, - * which makes each stored index read-modify-write atomic. It does not span - * the call to the downstream sink, so an update reaches the store and the - * cache at slightly different times. + * Thread-safe: concurrent reads and updates are serialized. */ class FlagPersistence : public IDataSourceUpdateSink { public: @@ -57,21 +54,20 @@ class FlagPersistence : public IDataSourceUpdateSink { void LoadCached(Context const& context); /** - * The flag data stored for the given context, or nullopt when nothing is - * stored for it and when persistence is disabled. An empty map means an - * environment with no flags was stored, which is distinct from nothing + * Returns the flag data stored for the given context, or nullopt when + * nothing is stored for it or persistence is disabled. An empty map means + * an environment with no flags was stored, which is distinct from nothing * being stored at all. */ [[nodiscard]] std::optional> ReadCached(Context const& context); /** - * When the service last confirmed the flag data for this context was - * current, or nullopt if it never has. + * Returns the time the service last confirmed the flag data for this + * context was current, or nullopt if it never has. * - * Keyed by the context's whole set of attributes rather than its key, - * because changing an attribute can change how flags evaluate, and the - * answer for the old attributes says nothing about the new ones. + * Keyed by the context's whole set of attributes, not just its key, + * because changing an attribute can change how flags evaluate. */ [[nodiscard]] std::optional< std::chrono::time_point> @@ -98,11 +94,11 @@ class FlagPersistence : public IDataSourceUpdateSink { void StoreCache(std::string const& context_id); - // Records that the service confirmed this context's data is current, as - // of now. + // Records that the service just confirmed this context's data is current. void RecordFreshness(Context const& context); - // Must be called with persistence_mutex_ held. + // Reads the stored index at the given key, or an empty index if none is + // stored. Must be called with persistence_mutex_ held. ContextIndex ReadIndexAt(std::string const& key); }; diff --git a/libs/client-sdk/tests/fdv2_cache_initializer_test.cpp b/libs/client-sdk/tests/fdv2_cache_initializer_test.cpp index 15baa00e9..837bd14e4 100644 --- a/libs/client-sdk/tests/fdv2_cache_initializer_test.cpp +++ b/libs/client-sdk/tests/fdv2_cache_initializer_test.cpp @@ -70,6 +70,7 @@ TEST(FDv2CacheInitializerTest, CacheHitProducesAFullDataSet) { ASSERT_TRUE(future.IsFinished()); auto result = future.GetResult(); + // The result is a full change set carrying the cached flag. auto* change_set = std::get_if(&result->value); ASSERT_NE(nullptr, change_set); EXPECT_EQ(ChangeSetType::kFull, change_set->change_set.type); @@ -77,24 +78,9 @@ TEST(FDv2CacheInitializerTest, CacheHitProducesAFullDataSet) { EXPECT_EQ("flagA", change_set->change_set.data[0].key); EXPECT_EQ(Value("test"), change_set->change_set.data[0].item.item->Detail().Value()); -} -// Asking the service for a delta against unverified cached data could corrupt -// the store silently, so the cache never supplies a basis. -TEST(FDv2CacheInitializerTest, CachedDataCarriesNoSelector) { - auto context = ContextBuilder().Kind("user", "user-key").Build(); - auto logger = launchdarkly::logging::NullLogger(); - auto persistence = - std::make_shared(TestPersistence::StoreType{ - {kEnvironment, - {{kContextId, R"({"flagA":{"version":1,"value":"test"}})"}}}}); - FlagManager flag_manager("the-key", logger, 5, persistence); - - FDv2CacheInitializer initializer(&flag_manager.Cache(), context, logger); - auto result = initializer.Run().GetResult(); - - auto* change_set = std::get_if(&result->value); - ASSERT_NE(nullptr, change_set); + // A delta against unverified cached data could silently corrupt the store, + // so the cache supplies no selector. EXPECT_FALSE(change_set->change_set.selector.value.has_value()); } @@ -128,6 +114,7 @@ TEST(FDv2CacheInitializerTest, NoPersistenceConfiguredProducesANoneIntent) { ASSERT_TRUE(future.IsFinished()); auto result = future.GetResult(); + // Not configuring persistence acts like a cache miss, not an error. auto* change_set = std::get_if(&result->value); ASSERT_NE(nullptr, change_set); EXPECT_EQ(ChangeSetType::kNone, change_set->change_set.type); diff --git a/libs/client-sdk/tests/flag_persistence_test.cpp b/libs/client-sdk/tests/flag_persistence_test.cpp index 0139dedf0..7ba501342 100644 --- a/libs/client-sdk/tests/flag_persistence_test.cpp +++ b/libs/client-sdk/tests/flag_persistence_test.cpp @@ -260,6 +260,7 @@ TEST(FlagPersistenceTests, RecordsFreshnessOnAPayload) { Selector{}}, /* from_cache= */ false); + // The apply records the clock's time as freshness. EXPECT_EQ( std::chrono::system_clock::time_point{std::chrono::milliseconds{500}}, flag_persistence.ReadFreshness(context)); @@ -377,7 +378,7 @@ TEST(FlagPersistenceTests, ApplyFromCacheDoesNotWriteTheCache) { // Nothing is written back. EXPECT_TRUE(persistence->store_.empty()); - // Nor was it confirmed current by the service, so it is not freshness. + // Nor did the service confirm it current, so no freshness is recorded. EXPECT_FALSE(flag_persistence.ReadFreshness(context).has_value()); // The data is still applied to the store, so evaluation can use it. ASSERT_TRUE(store.Get("flagA")); From 60b04ea21d5834082cdb61bcd8f9ed180353168a Mon Sep 17 00:00:00 2001 From: Bee Klimt Date: Thu, 24 Sep 2026 23:46:24 -0700 Subject: [PATCH 4/4] fix: Report FDv2 freshness only while the context's flags are cached --- .../src/flag_manager/flag_persistence.cpp | 4 + .../src/flag_manager/flag_persistence.hpp | 3 +- .../tests/flag_persistence_test.cpp | 98 +++++++++++++++++-- 3 files changed, 98 insertions(+), 7 deletions(-) diff --git a/libs/client-sdk/src/flag_manager/flag_persistence.cpp b/libs/client-sdk/src/flag_manager/flag_persistence.cpp index 620c4059c..6ffa27c87 100644 --- a/libs/client-sdk/src/flag_manager/flag_persistence.cpp +++ b/libs/client-sdk/src/flag_manager/flag_persistence.cpp @@ -142,6 +142,10 @@ FlagPersistence::ReadFreshness(Context const& context) { } 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)); } diff --git a/libs/client-sdk/src/flag_manager/flag_persistence.hpp b/libs/client-sdk/src/flag_manager/flag_persistence.hpp index da60099c0..f9f2ff07b 100644 --- a/libs/client-sdk/src/flag_manager/flag_persistence.hpp +++ b/libs/client-sdk/src/flag_manager/flag_persistence.hpp @@ -64,7 +64,8 @@ class FlagPersistence : public IDataSourceUpdateSink { /** * Returns the time the service last confirmed the flag data for this - * context was current, or nullopt if it never has. + * context was current. Returns nullopt if the service never confirmed it, + * or if no flag data is cached for the context now. * * Keyed by the context's whole set of attributes, not just its key, * because changing an attribute can change how flags evaluate. diff --git a/libs/client-sdk/tests/flag_persistence_test.cpp b/libs/client-sdk/tests/flag_persistence_test.cpp index 7ba501342..316a5a92c 100644 --- a/libs/client-sdk/tests/flag_persistence_test.cpp +++ b/libs/client-sdk/tests/flag_persistence_test.cpp @@ -276,12 +276,26 @@ TEST(FlagPersistenceTests, RecordsFreshnessOnANoneChangeSet) { std::make_shared(TestPersistence::StoreType()); auto logger = launchdarkly::logging::NullLogger(); + uint64_t now = 500; FlagPersistence flag_persistence( - "the-key", updater, store, persistence, logger, 5, []() { + "the-key", updater, store, persistence, logger, 5, [&now]() { return std::chrono::system_clock::time_point{ - std::chrono::milliseconds{700}}; + std::chrono::milliseconds{now}}; }); + auto item = ItemDescriptor{EvaluationResult{ + 1, std::nullopt, false, false, std::nullopt, + EvaluationDetailInternal{Value("test"), std::nullopt, std::nullopt}}}; + + // A payload caches the data and its freshness. + flag_persistence.Apply( + context, + FlagChangeSet{ + ChangeSetType::kFull, {FlagChange{"flagA", item}}, Selector{}}, + /* from_cache= */ false); + + // A later "none" advances the freshness to when it arrived. + now = 700; flag_persistence.Apply(context, FlagChangeSet{ChangeSetType::kNone, {}, Selector{}}, /* from_cache= */ false); @@ -306,13 +320,19 @@ TEST(FlagPersistenceTests, FreshnessIsPerContextAttributeSet) { std::chrono::milliseconds{500}}; }); + auto item = ItemDescriptor{EvaluationResult{ + 1, std::nullopt, false, false, std::nullopt, + EvaluationDetailInternal{Value("test"), std::nullopt, std::nullopt}}}; + auto plain = ContextBuilder().Kind("user", "user-key").Build(); auto with_attribute = ContextBuilder().Kind("user", "user-key").Set("country", "US").Build(); - flag_persistence.Apply(plain, - FlagChangeSet{ChangeSetType::kNone, {}, Selector{}}, - /* from_cache= */ false); + flag_persistence.Apply( + plain, + FlagChangeSet{ + ChangeSetType::kFull, {FlagChange{"flagA", item}}, Selector{}}, + /* from_cache= */ false); EXPECT_TRUE(flag_persistence.ReadFreshness(plain).has_value()); EXPECT_FALSE(flag_persistence.ReadFreshness(with_attribute).has_value()); @@ -334,11 +354,16 @@ TEST(FlagPersistenceTests, PrunesFreshnessBeyondMaxContexts) { std::chrono::milliseconds{now}}; }); + auto item = ItemDescriptor{EvaluationResult{ + 1, std::nullopt, false, false, std::nullopt, + EvaluationDetailInternal{Value("test"), std::nullopt, std::nullopt}}}; + auto first = ContextBuilder().Kind("user", "first").Build(); for (auto const& key : {"first", "second", "third"}) { flag_persistence.Apply( ContextBuilder().Kind("user", key).Build(), - FlagChangeSet{ChangeSetType::kNone, {}, Selector{}}, + FlagChangeSet{ + ChangeSetType::kFull, {FlagChange{"flagA", item}}, Selector{}}, /* from_cache= */ false); now++; } @@ -350,6 +375,67 @@ TEST(FlagPersistenceTests, PrunesFreshnessBeyondMaxContexts) { .has_value()); } +// A freshness timestamp must not outlive the flag data it describes. The +// freshness and flag-data indexes prune independently, and a "none" intent +// advances only the freshness entry, so a context's flag data can age out while +// its freshness lingers. Reporting that timestamp would make a poll wait on +// data that is no longer cached. +TEST(FlagPersistenceTests, DoesNotReportFreshnessAfterFlagDataEvicted) { + auto store = FlagStore(); + auto updater = FlagUpdater(store); + auto persistence = + std::make_shared(TestPersistence::StoreType()); + auto logger = launchdarkly::logging::NullLogger(); + + uint64_t now = 0; + // Room for only two contexts, so caching a third is what orphans an entry. + FlagPersistence flag_persistence( + "the-key", updater, store, persistence, logger, 2, [&now]() { + return std::chrono::system_clock::time_point{ + std::chrono::milliseconds{now}}; + }); + + auto item = ItemDescriptor{EvaluationResult{ + 1, std::nullopt, false, false, std::nullopt, + EvaluationDetailInternal{Value("test"), std::nullopt, std::nullopt}}}; + + auto a = ContextBuilder().Kind("user", "a").Build(); + auto b = ContextBuilder().Kind("user", "b").Build(); + auto c = ContextBuilder().Kind("user", "c").Build(); + + flag_persistence.Apply( + a, + FlagChangeSet{ + ChangeSetType::kFull, {FlagChange{"flagA", item}}, Selector{}}, + /* from_cache= */ false); + now = 1; + flag_persistence.Apply( + b, + FlagChangeSet{ + ChangeSetType::kFull, {FlagChange{"flagB", item}}, Selector{}}, + /* from_cache= */ false); + // A "none" refreshes only a's freshness entry, reordering the freshness + // index against the flag-data index. + now = 2; + flag_persistence.Apply(a, + FlagChangeSet{ChangeSetType::kNone, {}, Selector{}}, + /* from_cache= */ false); + // Caching a third context evicts the oldest of each index: a's flag data, + // but b's freshness. a is left with a freshness entry and no flag data. + now = 3; + flag_persistence.Apply( + c, + FlagChangeSet{ + ChangeSetType::kFull, {FlagChange{"flagC", item}}, Selector{}}, + /* from_cache= */ false); + + // a's flag data was evicted, so its freshness must not be reported. + EXPECT_FALSE(flag_persistence.ReadCached(a).has_value()); + EXPECT_FALSE(flag_persistence.ReadFreshness(a).has_value()); + // b still has flag data behind it. + EXPECT_TRUE(flag_persistence.ReadCached(b).has_value()); +} + // Data read out of the cache was never confirmed current by the service, so // writing it back or counting it as fresh would be misleading. TEST(FlagPersistenceTests, ApplyFromCacheDoesNotWriteTheCache) {