fix: allow lazy load evaluations when $inited key is not set#508
fix: allow lazy load evaluations when $inited key is not set#508kinyoklion wants to merge 4 commits into
Conversation
In lazy load / daemon mode, the SDK's Initialized() check was blocking all flag evaluations when the $inited key was not found in the persistent store. This is problematic because in daemon mode, an external process (like Relay Proxy) populates the store, and the $inited key may not always be present. The fix changes LazyLoad::Initialized() to always return true, allowing evaluations to proceed using available data. When the underlying source reports not initialized ($inited key not found), a warning is logged to alert operators that a Relay Proxy or other SDK should set this key. This aligns with the Go SDK behavior where daemon mode (ExternalUpdatesOnly) always considers the data source initialized. Updated unit tests to reflect the new behavior and added tests verifying the warning is logged appropriately. Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Reworked approach based on review feedback: Initialized() should return false when $inited is not set (consistent with other SDK implementations), and the evaluation path should handle this case by warning and proceeding rather than blocking. Changes: - Added CanEvaluateWhenNotInitialized() virtual method to IDataSystem interface (defaults to false) - LazyLoad overrides to return true (can serve on demand) - PreEvaluationChecks warns and proceeds when data system can evaluate while not initialized, instead of returning CLIENT_NOT_READY - AllFlagsState similarly warns and proceeds instead of returning empty - Reverted LazyLoad::Initialized() to original behavior (truthfully reports whether $inited key exists) - Added unit test for CanEvaluateWhenNotInitialized() This matches the pattern used in the Erlang SDK where the evaluation path distinguishes between 'not initialized' (blocks) and 'store initialized' (warns but proceeds). Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
|
Devin, review the human checklist for items which you can verify. |
|
I've reviewed the human checklist items. Here's what I can verify: 1. Warning log volume
|
- LazyLoad::Initialize() always sets kValid immediately (resolves StartAsync future) - LazyLoad::Initialized() always returns true (data system is always initialized) - Warning logged in Initialize() when store's $inited key is missing - Removed CanEvaluateWhenNotInitialized() from IDataSystem interface - Reverted PreEvaluationChecks/AllFlagsState to original behavior - Removed unused RefreshInitState(), initialized_ member, kInitialized key - Updated tests to verify new behavior Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
Summary
In lazy load (daemon) mode the SDK gated all evaluations on the persistent store's
$initedkey, so a store populated by the Relay Proxy or another SDK that hadn't set$initedcaused every evaluation to returnCLIENT_NOT_READY. This makesLazyLoadalways report initialized and evaluate on demand.Initialized()now always returnstrue— the lazy load system can fetch data on demand from the store, so it is always ready to evaluate.Initialize()setskValidimmediately, and logs a one-time warning if the store's$initedkey is absent (evaluations still proceed).RefreshInitState(),initialized_member, andKeys::kInitialized.Initialized(), immediatekValidstatus, and the warning-on-missing-$initedbehavior.No changes to
client_impl.cpp: sinceInitialized()returnstrue,PreEvaluationChecksandAllFlagsStateproceed with their existing logic.Design note
LazyLoadis the persistent-store (daemon) data system and never runs alongside a streaming/polling data source — streaming uses the in-memory Background Sync / FDv2 systems. There is therefore no configuration where$initedneeds to gate evaluation, so returningtrueunconditionally is safe. This matches the server-side contract tests (the "no data source" persistence tests populate the store'sfeatureswithout$initedand still expect real evaluations) and the Go/Java/.NET daemon-mode pattern, where aNullDataSourcereports initialized immediately.Behavioral change
When the store is completely unreachable, evaluations now proceed and return the default with
FLAG_NOT_FOUNDrather thanCLIENT_NOT_READY, consistent with treating the system as always initialized.Testing
Not built locally (CMake toolchain unavailable); relying on CI for the first compile. Follow-up: run the daemon-mode persistent-store contract tests against Redis end-to-end after this and #502 land (this PR fixes the initialization gating; #502 has the TTL field-name fix).
Link to Devin session | Requested by: @kinyoklion
Note
Medium Risk
Changes core readiness gating for daemon/persistent-store evaluations; unreachable stores now surface flag-not-found defaults instead of CLIENT_NOT_READY, which is intentional but affects operational failure modes.
Overview
Lazy load (daemon) mode no longer blocks evaluations on the persistent store’s
$initedkey. Previously missing$initedcould leave the client not ready and returnCLIENT_NOT_READY; evaluations can proceed using on-demand reads from the store.LazyLoad::Initialized()always returnstrue, andInitialize()always moves the data source tokValidafter a one-time check of the reader. If the store lacks$inited, the SDK logs a warning but still treats the system as ready—aligned with Go/Java/.NET daemon behavior.The TTL-based init refresh path is removed (
RefreshInitState,initialized_, and theinitializedexpiration key), and tests now assert always-true initialization, immediatekValid, and warning/no-warning logging.Reviewed by Cursor Bugbot for commit 5da2bd3. Bugbot is set up for automated code reviews on this repo. Configure here.