Skip to content

fix: allow lazy load evaluations when $inited key is not set#508

Open
kinyoklion wants to merge 4 commits into
mainfrom
devin/1773359647-lazy-load-initialized-warning
Open

fix: allow lazy load evaluations when $inited key is not set#508
kinyoklion wants to merge 4 commits into
mainfrom
devin/1773359647-lazy-load-initialized-warning

Conversation

@kinyoklion

@kinyoklion kinyoklion commented Mar 12, 2026

Copy link
Copy Markdown
Member

Summary

In lazy load (daemon) mode the SDK gated all evaluations on the persistent store's $inited key, so a store populated by the Relay Proxy or another SDK that hadn't set $inited caused every evaluation to return CLIENT_NOT_READY. This makes LazyLoad always report initialized and evaluate on demand.

  • Initialized() now always returns true — the lazy load system can fetch data on demand from the store, so it is always ready to evaluate.
  • Initialize() sets kValid immediately, and logs a one-time warning if the store's $inited key is absent (evaluations still proceed).
  • Removed the now-unused RefreshInitState(), initialized_ member, and Keys::kInitialized.
  • Tests updated to cover always-true Initialized(), immediate kValid status, and the warning-on-missing-$inited behavior.

No changes to client_impl.cpp: since Initialized() returns true, PreEvaluationChecks and AllFlagsState proceed with their existing logic.

Design note

LazyLoad is 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 $inited needs to gate evaluation, so returning true unconditionally is safe. This matches the server-side contract tests (the "no data source" persistence tests populate the store's features without $inited and still expect real evaluations) and the Go/Java/.NET daemon-mode pattern, where a NullDataSource reports initialized immediately.

Behavioral change

When the store is completely unreachable, evaluations now proceed and return the default with FLAG_NOT_FOUND rather than CLIENT_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 $inited key. Previously missing $inited could leave the client not ready and return CLIENT_NOT_READY; evaluations can proceed using on-demand reads from the store.

LazyLoad::Initialized() always returns true, and Initialize() always moves the data source to kValid after 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 the initialized expiration key), and tests now assert always-true initialization, immediate kValid, 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.

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-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

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>
@kinyoklion

Copy link
Copy Markdown
Member Author

Devin, review the human checklist for items which you can verify.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

I've reviewed the human checklist items. Here's what I can verify:

1. Warning log volume ⚠️ Confirmed — no rate limiting

PreEvaluationChecks (line 430) and AllFlagsState (line 190) both log a warning on every call when !Initialized() and CanEvaluateWhenNotInitialized() is true. There is no "log once" or rate-limiting guard.

In a daemon-mode deployment where $inited is never set (which is the normal case when not using Relay Proxy), every single flag evaluation will produce a warning. This could be extremely noisy under load.

Recommendation: Add a std::once_flag or std::atomic<bool> member to ClientImpl to log the warning only once per client lifetime. This is consistent with the pattern used in other SDKs (Go's NullDataSource avoids the warning entirely; Erlang logs once on first access).

2. StartAsync() behavior ⚠️ Confirmed — future may never resolve

StartAsync() (line 162-178) registers a status change listener that calls pr->set_value(true) only when data_system_->Initialized() returns true. LazyLoad::Initialize() (line 80-85) only sets kValid if Initialized() returns true at startup. If $inited is never set in the store, the status stays at kInitializing and the future never resolves.

In entity_manager.cpp line 226, the contract test service calls init.wait_for(waitForClient) with a timeout (default 5s), so it won't hang — but the client will never report as "started." This is acceptable for daemon mode since evaluations proceed via the CanEvaluateWhenNotInitialized() path, but worth documenting.

3. AllFlagsState when store is empty/unreachable ✓ Verified — behavior is reasonable

When AllFlagsState proceeds (line 206), it calls data_system_->AllFlags() which goes through LazyLoad::AllFlags(). This triggers RefreshAllFlags() which fetches from the store. If the store is:

  • Empty (no flags, but reachable): Returns an empty map → AllFlagsState returns an empty-but-valid result (not an error). This is correct — there genuinely are no flags.
  • Unreachable: RefreshAll catches the error, logs it, sets status to kInterrupted, and the cache returns whatever was previously cached (or empty if first call). This degrades gracefully.

Both cases are reasonable. The previous behavior (returning {} immediately) was actually worse because it didn't even attempt to fetch data that might be available.

4. Contract test verification ✓ Verified via PR #502 CI

PR #502 (which includes this fix via merge) has all 45 CI checks passing, including the Redis persistent store contract tests:

  • 10 daemon-mode tests PASS (uses default prefix, uses custom prefix, ignores database initialization flag, can disable cache, caches flag for duration × 2, caches flag forever, plus prefix variants)
  • 12 read-write tests properly SKIPPED via suppressions file (not yet implemented)
  • No regressions in any other contract tests

Summary: Items 3 and 4 are verified and good. Items 1 and 2 are confirmed as noted in the checklist — item 1 (warning volume) is the most actionable concern. Want me to add a "log once" guard for the warning?

devin-ai-integration Bot and others added 2 commits March 19, 2026 16:16
- 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>
@kinyoklion kinyoklion marked this pull request as ready for review July 6, 2026 22:03
@kinyoklion kinyoklion requested a review from a team as a code owner July 6, 2026 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants